diff --git a/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc b/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc index 9e52e0c7ec76a..20ea01bfaa411 100644 --- a/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc +++ b/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc @@ -38,6 +38,7 @@ #include "components/content_settings/core/browser/cookie_settings.h" #include "components/prefs/pref_member.h" #include "components/sync_preferences/testing_pref_service_syncable.h" +#include "content/public/browser/global_request_id.h" #include "content/public/common/url_constants.h" #include "content/public/test/browser_task_environment.h" #include "extensions/browser/api/declarative_net_request/test_utils.h" @@ -46,7 +47,9 @@ #include "extensions/browser/api/web_request/web_request_api_constants.h" #include "extensions/browser/api/web_request/web_request_api_helpers.h" #include "extensions/browser/api/web_request/web_request_info.h" +#include "extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h" #include "extensions/browser/extension_navigation_registry.h" +#include "extensions/browser/extension_navigation_ui_data.h" #include "extensions/buildflags/buildflags.h" #include "extensions/common/api/declarative_net_request.h" #include "extensions/common/api/web_request.h" @@ -54,8 +57,14 @@ #include "extensions/common/extension_builder.h" #include "extensions/common/features/feature.h" #include "google_apis/gaia/gaia_urls.h" +#include "ipc/constants.mojom-shared.h" +#include "mojo/public/cpp/bindings/remote.h" #include "net/http/http_util.h" +#include "net/traffic_annotation/network_traffic_annotation_test_helper.h" #include "services/network/public/cpp/features.h" +#include "services/network/public/cpp/url_loader_factory_builder.h" +#include "services/network/test/test_url_loader_client.h" +#include "services/network/test/test_url_loader_factory.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest-message.h" #include "testing/gtest/include/gtest/gtest.h" @@ -149,6 +158,17 @@ WebRequestEventRouter::RequestFilter MakeMainFrameFilter( filter.types.push_back(WebRequestResourceType::MAIN_FRAME); return filter; } + +class PlaceholderProxy : public WebRequestAPI::Proxy { + public: + void HandleAuthRequest( + const net::AuthChallengeInfo& auth_info, + scoped_refptr response_headers, + int32_t request_id, + WebRequestAPI::AuthRequestCallback callback) override {} + void OnDNRExtensionUnloaded(const Extension* extension) override {} +}; + } // namespace // Tests adding and removing listeners from the event router. @@ -532,6 +552,94 @@ TEST_F(ExtensionWebRequestTest, InitFromValue) { TestInitFromValue(&profile_, "blocking,asyncBlocking", false, 0); } +TEST_F(ExtensionWebRequestTest, AssociateProxyWithRequestIdCollision) { + WebRequestAPI::ProxySet proxy_set; + + auto proxy1 = std::make_unique(); + auto* proxy1_ptr = proxy1.get(); + proxy_set.AddProxy(std::move(proxy1)); + + content::GlobalRequestID id = + content::GlobalRequestID::MakeBrowserInitiated(); + EXPECT_TRUE(proxy_set.AssociateProxyWithRequestId(proxy1_ptr, id)); + + // Re-associating the same request ID should fail (collision). + EXPECT_FALSE(proxy_set.AssociateProxyWithRequestId(proxy1_ptr, id)); + // The original association should remain intact. + EXPECT_EQ(proxy_set.GetProxyFromRequestId(id), proxy1_ptr); + + auto proxy2 = std::make_unique(); + auto* proxy2_ptr = proxy2.get(); + proxy_set.AddProxy(std::move(proxy2)); + + // Associating the same request ID with a different proxy should also fail. + EXPECT_FALSE(proxy_set.AssociateProxyWithRequestId(proxy2_ptr, id)); + // The original association should still remain intact. + EXPECT_EQ(proxy_set.GetProxyFromRequestId(id), proxy1_ptr); + + // Destroy the colliding proxy and ensure the original mapping survives. + proxy_set.RemoveProxy(proxy2_ptr); + EXPECT_EQ(proxy_set.GetProxyFromRequestId(id), proxy1_ptr); +} + +// Verifies that when WebRequestProxyingURLLoaderFactory receives a request with +// a request ID that's already in use by another in-flight request, the new +// request is forwarded to the underlying factory with a request ID of zero so +// network-service callbacks for one request can't be routed to the other. +TEST_F(ExtensionWebRequestTest, ProxyingFactoryDuplicateRequestId) { + WebRequestAPI::ProxySet proxies; + WebRequestAPI::RequestIDGenerator request_id_generator; + + network::TestURLLoaderFactory target_factory; + mojo::Receiver target_receiver( + &target_factory); + + network::URLLoaderFactoryBuilder factory_builder; + WebRequestProxyingURLLoaderFactory::StartProxying( + &profile_, /*render_process_id=*/1, IPC::mojom::kRoutingIdNone, + IPC::mojom::kRoutingIdNone, &request_id_generator, + /*navigation_ui_data=*/nullptr, + /*navigation_id=*/std::nullopt, ukm::kInvalidSourceIdObj, factory_builder, + mojo::PendingReceiver(), + &proxies, + content::ContentBrowserClient::URLLoaderFactoryType::kDocumentSubResource, + /*navigation_response_task_runner=*/nullptr); + + mojo::Remote proxy_factory( + std::move(factory_builder) + .Finish>( + target_receiver.BindNewPipeAndPassRemote())); + + constexpr int32_t kRequestId = 42; + network::ResourceRequest request1; + request1.url = GURL("http://a.test/"); + network::ResourceRequest request2; + request2.url = GURL("http://b.test/"); + + mojo::PendingRemote loader1; + mojo::PendingRemote loader2; + network::TestURLLoaderClient client1; + network::TestURLLoaderClient client2; + + proxy_factory->CreateLoaderAndStart( + loader1.InitWithNewPipeAndPassReceiver(), kRequestId, /*options=*/0, + request1, client1.CreateRemote(), + net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS)); + proxy_factory->CreateLoaderAndStart( + loader2.InitWithNewPipeAndPassReceiver(), kRequestId, /*options=*/0, + request2, client2.CreateRemote(), + net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS)); + base::RunLoop().RunUntilIdle(); + + ASSERT_EQ(2u, target_factory.pending_requests()->size()); + EXPECT_EQ(kRequestId, target_factory.GetPendingRequest(0)->request_id); + EXPECT_EQ(GURL("http://a.test/"), + target_factory.GetPendingRequest(0)->request.url); + EXPECT_EQ(0, target_factory.GetPendingRequest(1)->request_id); + EXPECT_EQ(GURL("http://b.test/"), + target_factory.GetPendingRequest(1)->request.url); +} + TEST(ExtensionWebRequestHelpersTest, TestInDecreasingExtensionInstallationTimeOrder) { EventResponseDelta a("ext_1", base::Time::FromInternalValue(0)); diff --git a/extensions/browser/api/web_request/web_request_api.cc b/extensions/browser/api/web_request/web_request_api.cc index b059c83affa5b..5e483cb9ac46c 100644 --- a/extensions/browser/api/web_request/web_request_api.cc +++ b/extensions/browser/api/web_request/web_request_api.cc @@ -280,15 +280,18 @@ void WebRequestAPI::ProxySet::RemoveProxy(Proxy* proxy) { proxies_.erase(proxy_it); } -void WebRequestAPI::ProxySet::AssociateProxyWithRequestId( +bool WebRequestAPI::ProxySet::AssociateProxyWithRequestId( Proxy* proxy, const content::GlobalRequestID& id) { DCHECK(proxy); DCHECK(proxies_.count(proxy)); DCHECK(id.request_id); auto result = request_id_to_proxy_map_.emplace(id, proxy); - DCHECK(result.second) << "Unexpected request ID collision."; + if (!result.second) { + return false; + } proxy_to_request_id_map_[proxy].insert(id); + return true; } void WebRequestAPI::ProxySet::DisassociateProxyWithRequestId( diff --git a/extensions/browser/api/web_request/web_request_api.h b/extensions/browser/api/web_request/web_request_api.h index 132ab8065bf82..6cf1b499e24df 100644 --- a/extensions/browser/api/web_request/web_request_api.h +++ b/extensions/browser/api/web_request/web_request_api.h @@ -125,8 +125,12 @@ class WebRequestAPI : public BrowserContextKeyedAPI, // // Each Proxy may be responsible for multiple requests, but any given // request identified by `id` must be associated with only a single proxy. - void AssociateProxyWithRequestId(Proxy* proxy, - const content::GlobalRequestID& id); + // + // Returns true on success, or false if `id` is already associated with a + // proxy (the existing association is left unchanged). + [[nodiscard]] bool AssociateProxyWithRequestId( + Proxy* proxy, + const content::GlobalRequestID& id); // Disassociates `proxy` with `id`. `proxy` must already be registered // within this ProxySet. diff --git a/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc b/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc index dc8cf39d39448..d8d3838f9d108 100644 --- a/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc +++ b/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc @@ -1566,11 +1566,22 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart( // correlation against any auth events received by the browser. // Requests with a request ID of 0 therefore do not support // dispatching |WebRequest.onAuthRequired| events. - proxies_->AssociateProxyWithRequestId( - this, content::GlobalRequestID( - content::ToOriginatingProcessIdUnsafe(render_process_id_), - request_id)); - network_request_id_to_web_request_id_.emplace(request_id, web_request_id); + // + // The request ID is also used to route OnLoaderCreated and auth + // challenges from the network service back to the corresponding + // InProgressRequest. If it is already in use by another in-flight + // request, treat the new request as having a request ID of 0 so + // those events can't be misrouted between requests. + if (network_request_id_to_web_request_id_.contains(request_id) || + !proxies_->AssociateProxyWithRequestId( + this, + content::GlobalRequestID( + content::ToOriginatingProcessIdUnsafe(render_process_id_), + request_id))) { + request_id = 0; + } else { + network_request_id_to_web_request_id_.emplace(request_id, web_request_id); + } } auto result = requests_.emplace(