diff --git a/chrome/browser/extensions/api/web_request/web_request_apitest.cc b/chrome/browser/extensions/api/web_request/web_request_apitest.cc index e7d8ae83cbe76..ccaf5eb7072b8 100644 --- a/chrome/browser/extensions/api/web_request/web_request_apitest.cc +++ b/chrome/browser/extensions/api/web_request/web_request_apitest.cc @@ -2910,6 +2910,86 @@ IN_PROC_BROWSER_TEST_F( } #endif // BUILDFLAG(ENABLE_EXTENSIONS) +// Verifies that the proxying URLLoaderFactory forwards a distinct, locally +// assigned request id to the underlying factory for each request even when the +// caller reuses a single request id for concurrent requests. The forwarded id +// is what the network stack uses to route TrustedHeaderClient and auth events +// back to the proxy, so keeping it unique ensures those events cannot be +// delivered to the wrong in-progress request. +IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, + ProxyingFactoryAssignsUniqueRequestId) { + ASSERT_TRUE(StartEmbeddedTestServer()); + + // Make sure the WebRequest proxy is inserted into the URLLoaderFactory chain + // for subresource requests. + auto* web_request_api = + BrowserContextKeyedAPIFactory::Get(profile()); + ASSERT_TRUE(web_request_api); + web_request_api->ForceProxyForTesting(); + profile()->GetDefaultStoragePartition()->FlushNetworkInterfaceForTesting(); + + ASSERT_TRUE(NavigateToURL(GetActiveWebContents(), + embedded_test_server()->GetURL("/simple.html"))); + content::RenderFrameHost* frame = + GetActiveWebContents()->GetPrimaryMainFrame(); + + const GURL url_a = embedded_test_server()->GetURL("/echo?a"); + const GURL url_b = embedded_test_server()->GetURL("/echo?b"); + + // Observe the requests as they leave the WebRequest proxy on their way to the + // network service and record the request id each one carries. + std::map forwarded_request_ids; + content::URLLoaderInterceptor interceptor(base::BindLambdaForTesting( + [&](content::URLLoaderInterceptor::RequestParams* params) { + const GURL& url = params->url_request.url; + if (url == url_a || url == url_b) { + forwarded_request_ids[url] = params->request_id; + } + return false; + })); + + // Create a URLLoaderFactory equivalent to the one the renderer uses for + // subresource requests, proxied through the WebRequest layer, and drive it + // directly to simulate a caller that reuses the same request id. + mojo::Remote factory; + ASSERT_TRUE(frame->CreateNetworkServiceDefaultFactory( + factory.BindNewPipeAndPassReceiver())); + + constexpr int32_t kClientRequestId = 42; + + auto make_request = [&](const GURL& url) { + network::ResourceRequest request; + request.url = url; + request.request_initiator = frame->GetLastCommittedOrigin(); + return request; + }; + + network::TestURLLoaderClient client_a; + mojo::PendingRemote loader_a; + factory->CreateLoaderAndStart( + loader_a.InitWithNewPipeAndPassReceiver(), kClientRequestId, + network::mojom::kURLLoadOptionNone, make_request(url_a), + client_a.CreateRemote(), + net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS)); + + network::TestURLLoaderClient client_b; + mojo::PendingRemote loader_b; + factory->CreateLoaderAndStart( + loader_b.InitWithNewPipeAndPassReceiver(), kClientRequestId, + network::mojom::kURLLoadOptionNone, make_request(url_b), + client_b.CreateRemote(), + net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS)); + + client_a.RunUntilComplete(); + client_b.RunUntilComplete(); + + ASSERT_TRUE(forwarded_request_ids.contains(url_a)); + ASSERT_TRUE(forwarded_request_ids.contains(url_b)); + EXPECT_NE(forwarded_request_ids[url_a], 0); + EXPECT_NE(forwarded_request_ids[url_b], 0); + EXPECT_NE(forwarded_request_ids[url_a], forwarded_request_ids[url_b]); +} + // Tests that webRequest API can inspect window.open() requests initiated from // chrome-untrusted:// pages to Web origins, but not other WebUI origins. IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, diff --git a/extensions/browser/api/web_request/web_request_api.cc b/extensions/browser/api/web_request/web_request_api.cc index 1d116ef6b868c..51b2d46ed8696 100644 --- a/extensions/browser/api/web_request/web_request_api.cc +++ b/extensions/browser/api/web_request/web_request_api.cc @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -364,6 +365,14 @@ void WebRequestAPI::RequestIDGenerator::SaveID( } } +int32_t WebRequestAPI::RequestIDGenerator::GenerateNetworkRequestId() { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + if (network_request_id_ == std::numeric_limits::max()) { + network_request_id_ = 0; + } + return ++network_request_id_; +} + WebRequestAPI::WebRequestAPI(content::BrowserContext* context) : browser_context_(context), proxies_(std::make_unique()), diff --git a/extensions/browser/api/web_request/web_request_api.h b/extensions/browser/api/web_request/web_request_api.h index 8cc6966847086..ef4401b1631ea 100644 --- a/extensions/browser/api/web_request/web_request_api.h +++ b/extensions/browser/api/web_request/web_request_api.h @@ -178,8 +178,14 @@ class WebRequestAPI : public BrowserContextKeyedAPI, int32_t network_service_request_id, uint64_t request_id); + // Generates a non-zero id for a proxy to use as the request id it forwards + // to the network service. Returned values are unique for the lifetime of + // this generator (modulo int32_t wrap-around). + int32_t GenerateNetworkRequestId(); + private: int64_t id_ = 0; + int32_t network_request_id_ = 0; std::map, uint64_t> saved_id_map_; }; 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 b371efbb42090..654b581270ebe 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 @@ -143,6 +143,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( WebRequestProxyingURLLoaderFactory* factory, uint64_t request_id, int32_t network_service_request_id, + int32_t client_request_id, int32_t view_routing_id, int32_t frame_routing_id, uint32_t options, @@ -157,6 +158,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( original_initiator_(request.request_initiator), request_id_(request_id), network_service_request_id_(network_service_request_id), + client_request_id_(client_request_id), view_routing_id_(view_routing_id), frame_routing_id_(frame_routing_id), options_(options), @@ -1396,8 +1398,8 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: // URLLoader instead of continuing with FollowRedirect(). The saved ID will // be retrieved in the restarted request, which will call // RequestIDGenerator::Generate() with the same ID pair. - factory_->request_id_generator_->SaveID( - view_routing_id_, network_service_request_id_, request_id_); + factory_->request_id_generator_->SaveID(view_routing_id_, + client_request_id_, request_id_); state_ = State::kRedirectFollowedByAnotherInProgressRequest; // Deletes |this|. @@ -1560,7 +1562,17 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart( const uint64_t web_request_id = request_id_generator_->Generate(view_routing_id_, request_id); - if (request_id) { + // The caller-supplied `request_id` is not necessarily unique among the + // requests that reach this factory; in particular a `ThrottlingURLLoader` + // that restarts a request reuses the same value, and for renderer factories + // the value is chosen entirely by the renderer. To keep the maps below and in + // `proxies_` consistent (so that TrustedHeaderClient and auth events are + // always routed back to the request that produced them) allocate a fresh id + // to use when talking to `target_factory_`. + const int32_t network_service_request_id = + request_id ? request_id_generator_->GenerateNetworkRequestId() : 0; + + if (network_service_request_id) { // Only requests with a non-zero request ID can have their proxy // associated with said ID. This is necessary to support // correlation against any auth events received by the browser. @@ -1569,16 +1581,18 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart( 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); + network_service_request_id)); + network_request_id_to_web_request_id_.emplace(network_service_request_id, + web_request_id); } auto result = requests_.emplace( - web_request_id, std::make_unique( - this, web_request_id, request_id, view_routing_id_, - frame_routing_id_, options, ukm_source_id_, request, - traffic_annotation, std::move(loader_receiver), - std::move(client), navigation_response_task_runner_)); + web_request_id, + std::make_unique( + this, web_request_id, network_service_request_id, request_id, + view_routing_id_, frame_routing_id_, options, ukm_source_id_, request, + traffic_annotation, std::move(loader_receiver), std::move(client), + navigation_response_task_runner_)); result.first->second->Restart(); } diff --git a/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h b/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h index 13d3c330aba30..713f70e9d6908 100644 --- a/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h +++ b/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h @@ -81,6 +81,7 @@ class WebRequestProxyingURLLoaderFactory WebRequestProxyingURLLoaderFactory* factory, uint64_t request_id, int32_t network_service_request_id, + int32_t client_request_id, int32_t view_routing_id, int32_t frame_routing_id, uint32_t options, @@ -217,7 +218,14 @@ class WebRequestProxyingURLLoaderFactory network::ResourceRequest request_; const std::optional original_initiator_; const uint64_t request_id_ = 0; + // The request id assigned by this factory and forwarded to + // `target_factory_`; used to correlate TrustedHeaderClient and auth events + // that the network stack reports back. const int32_t network_service_request_id_ = 0; + // The request id supplied by the caller of `CreateLoaderAndStart()`; used + // only to preserve the extension-visible request id across a + // `ThrottlingURLLoader` restart. + const int32_t client_request_id_ = 0; const int32_t view_routing_id_ = IPC::mojom::kRoutingIdNone; const int32_t frame_routing_id_ = IPC::mojom::kRoutingIdNone; const uint32_t options_ = 0;