From 8e1176cbc03c0393b7e73269a2624d7763b9aa6f Mon Sep 17 00:00:00 2001 From: David Sanders Date: Thu, 3 Jun 2021 21:18:02 -0700 Subject: [PATCH] chore: pull ProxyingURLLoaderFactory closer to upstream class it mirrors (#29486) * chore: pull ProxyingURLLoaderFactory closer to upstream class it mirrors * chore: add another change which was accepted upstream --- .../net/proxying_url_loader_factory.cc | 260 +++++++++--------- .../browser/net/proxying_url_loader_factory.h | 30 +- 2 files changed, 152 insertions(+), 138 deletions(-) diff --git a/shell/browser/net/proxying_url_loader_factory.cc b/shell/browser/net/proxying_url_loader_factory.cc index 6455f1c05c65..45f3c9f7adf1 100644 --- a/shell/browser/net/proxying_url_loader_factory.cc +++ b/shell/browser/net/proxying_url_loader_factory.cc @@ -4,8 +4,11 @@ #include "shell/browser/net/proxying_url_loader_factory.h" +#include #include +#include "base/bind.h" +#include "base/callback_helpers.h" #include "base/command_line.h" #include "base/strings/string_split.h" #include "base/strings/string_util.h" @@ -14,12 +17,15 @@ #include "extensions/browser/extension_navigation_ui_data.h" #include "net/base/completion_repeating_callback.h" #include "net/base/load_flags.h" +#include "net/http/http_response_headers.h" #include "net/http/http_status_code.h" #include "net/http/http_util.h" +#include "net/url_request/redirect_info.h" #include "services/metrics/public/cpp/ukm_source_id.h" #include "services/network/public/cpp/features.h" #include "shell/browser/net/asar/asar_url_loader.h" #include "shell/common/options_switches.h" +#include "url/origin.h" namespace electron { @@ -43,9 +49,9 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( request_(request), original_initiator_(request.request_initiator), request_id_(web_request_id), + network_service_request_id_(network_service_request_id), view_routing_id_(view_routing_id), frame_routing_id_(frame_routing_id), - network_service_request_id_(network_service_request_id), options_(options), traffic_annotation_(traffic_annotation), proxied_loader_receiver_(this, std::move(loader_receiver)), @@ -159,7 +165,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { // they respond. |continuation| above will be invoked asynchronously to // continue or cancel the request. // - // We pause the binding here to prevent further client message processing. + // We pause the receiver here to prevent further client message processing. if (proxied_client_receiver_.is_bound()) proxied_client_receiver_.Pause(); @@ -229,6 +235,11 @@ void ProxyingURLLoaderFactory::InProgressRequest::ResumeReadingBodyFromNet() { target_loader_->ResumeReadingBodyFromNet(); } +void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveEarlyHints( + network::mojom::EarlyHintsPtr early_hints) { + target_client_->OnReceiveEarlyHints(std::move(early_hints)); +} + void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveResponse( network::mojom::URLResponseHeadPtr head) { if (current_request_uses_header_client_) { @@ -246,11 +257,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveResponse( } } -void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveEarlyHints( - network::mojom::EarlyHintsPtr early_hints) { - target_client_->OnReceiveEarlyHints(std::move(early_hints)); -} - void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveRedirect( const net::RedirectInfo& redirect_info, network::mojom::URLResponseHeadPtr head) { @@ -313,12 +319,17 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnComplete( factory_->RemoveRequest(network_service_request_id_, request_id_); } +bool ProxyingURLLoaderFactory::IsForServiceWorkerScript() const { + return loader_factory_type_ == content::ContentBrowserClient:: + URLLoaderFactoryType::kServiceWorkerScript; +} + void ProxyingURLLoaderFactory::InProgressRequest::OnLoaderCreated( mojo::PendingReceiver receiver) { header_client_receiver_.Bind(std::move(receiver)); if (for_cors_preflight_) { // In this case we don't have |target_loader_| and - // |proxied_client_binding_|, and |receiver| is the only connection to the + // |proxied_client_receiver_|, and |receiver| is the only connection to the // network service, so we observe mojo connection errors. header_client_receiver_.set_disconnect_handler(base::BindOnce( &ProxyingURLLoaderFactory::InProgressRequest::OnRequestError, @@ -365,25 +376,57 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnHeadersReceived( weak_factory_.GetWeakPtr())); } -void ProxyingURLLoaderFactory::OnLoaderForCorsPreflightCreated( - const network::ResourceRequest& request, - mojo::PendingReceiver receiver) { - // Please note that the URLLoader is now starting, without waiting for - // additional signals from here. The URLLoader will be blocked before - // sending HTTP request headers (TrustedHeaderClient.OnBeforeSendHeaders), - // but the connection set up will be done before that. This is acceptable from - // Web Request API because the extension has already allowed to set up - // a connection to the same URL (i.e., the actual request), and distinguishing - // two connections for the actual request and the preflight request before - // sending request headers is very difficult. - const uint64_t web_request_id = ++(*request_id_generator_); +void ProxyingURLLoaderFactory::InProgressRequest:: + HandleBeforeRequestRedirect() { + // The extension requested a redirect. Close the connection with the current + // URLLoader and inform the URLLoaderClient the WebRequest API generated a + // redirect. To load |redirect_url_|, a new URLLoader will be recreated + // after receiving FollowRedirect(). - auto result = requests_.insert(std::make_pair( - web_request_id, std::make_unique( - this, web_request_id, frame_routing_id_, request))); + // Forgetting to close the connection with the current URLLoader caused + // bugs. The latter doesn't know anything about the redirect. Continuing + // the load with it gives unexpected results. See + // https://crbug.com/882661#c72. + proxied_client_receiver_.reset(); + header_client_receiver_.reset(); + target_loader_.reset(); - result.first->second->OnLoaderCreated(std::move(receiver)); - result.first->second->Restart(); + constexpr int kInternalRedirectStatusCode = net::HTTP_TEMPORARY_REDIRECT; + + net::RedirectInfo redirect_info; + redirect_info.status_code = kInternalRedirectStatusCode; + redirect_info.new_method = request_.method; + redirect_info.new_url = redirect_url_; + redirect_info.new_site_for_cookies = + net::SiteForCookies::FromUrl(redirect_url_); + + auto head = network::mojom::URLResponseHead::New(); + std::string headers = base::StringPrintf( + "HTTP/1.1 %i Internal Redirect\n" + "Location: %s\n" + "Non-Authoritative-Reason: WebRequest API\n\n", + kInternalRedirectStatusCode, redirect_url_.spec().c_str()); + + // Cross-origin requests need to modify the Origin header to 'null'. Since + // CorsURLLoader sets |request_initiator| to the Origin request header in + // NetworkService, we need to modify |request_initiator| here to craft the + // Origin header indirectly. + // Following checks implement the step 10 of "4.4. HTTP-redirect fetch", + // https://fetch.spec.whatwg.org/#http-redirect-fetch + if (request_.request_initiator && + (!url::Origin::Create(redirect_url_) + .IsSameOriginWith(url::Origin::Create(request_.url)) && + !request_.request_initiator->IsSameOriginWith( + url::Origin::Create(request_.url)))) { + // Reset the initiator to pretend tainted origin flag of the spec is set. + request_.request_initiator = url::Origin(); + } + head->headers = base::MakeRefCounted( + net::HttpUtil::AssembleRawHeaders(headers)); + head->encoded_data_length = 0; + + current_response_ = std::move(head); + ContinueToBeforeRedirect(redirect_info, net::OK); } void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders( @@ -435,6 +478,50 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders( net::OK); } +void ProxyingURLLoaderFactory::InProgressRequest::ContinueToStartRequest( + int error_code) { + if (error_code != net::OK) { + OnRequestError(network::URLLoaderCompletionStatus(error_code)); + return; + } + + if (current_request_uses_header_client_ && !redirect_url_.is_empty()) { + HandleBeforeRequestRedirect(); + return; + } + + if (proxied_client_receiver_.is_bound()) + proxied_client_receiver_.Resume(); + + if (header_client_receiver_.is_bound()) + header_client_receiver_.Resume(); + + if (for_cors_preflight_) { + // For CORS preflight requests, we have already started the request in + // the network service. We did block the request by blocking + // |header_client_receiver_|, which we unblocked right above. + return; + } + + if (!target_loader_.is_bound() && factory_->target_factory_.is_bound()) { + // No extensions have cancelled us up to this point, so it's now OK to + // initiate the real network request. + uint32_t options = options_; + // Even if this request does not use the header client, future redirects + // might, so we need to set the option on the loader. + if (has_any_extra_headers_listeners_) + options |= network::mojom::kURLLoadOptionUseHeaderClient; + factory_->target_factory_->CreateLoaderAndStart( + mojo::MakeRequest(&target_loader_), network_service_request_id_, + options, request_, proxied_client_receiver_.BindNewPipeAndPassRemote(), + traffic_annotation_); + } + + // From here the lifecycle of this request is driven by subsequent events on + // either |proxied_loader_receiver_|, |proxied_client_receiver_|, or + // |header_client_receiver_|. +} + void ProxyingURLLoaderFactory::InProgressRequest::ContinueToSendHeaders( const std::set& removed_headers, const std::set& set_headers, @@ -485,50 +572,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToSendHeaders( ContinueToStartRequest(net::OK); } -void ProxyingURLLoaderFactory::InProgressRequest::ContinueToStartRequest( - int error_code) { - if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); - return; - } - - if (current_request_uses_header_client_ && !redirect_url_.is_empty()) { - HandleBeforeRequestRedirect(); - return; - } - - if (proxied_client_receiver_.is_bound()) - proxied_client_receiver_.Resume(); - - if (header_client_receiver_.is_bound()) - header_client_receiver_.Resume(); - - if (for_cors_preflight_) { - // For CORS preflight requests, we have already started the request in - // the network service. We did block the request by blocking - // |header_client_receiver_|, which we unblocked right above. - return; - } - - if (!target_loader_.is_bound() && factory_->target_factory_.is_bound()) { - // No extensions have cancelled us up to this point, so it's now OK to - // initiate the real network request. - uint32_t options = options_; - // Even if this request does not use the header client, future redirects - // might, so we need to set the option on the loader. - if (has_any_extra_headers_listeners_) - options |= network::mojom::kURLLoadOptionUseHeaderClient; - factory_->target_factory_->CreateLoaderAndStart( - mojo::MakeRequest(&target_loader_), network_service_request_id_, - options, request_, proxied_client_receiver_.BindNewPipeAndPassRemote(), - traffic_annotation_); - } - - // From here the lifecycle of this request is driven by subsequent events on - // either |proxy_loader_binding_|, |proxy_client_receiver_|, or - // |header_client_receiver_|. -} - void ProxyingURLLoaderFactory::InProgressRequest:: ContinueToHandleOverrideHeaders(int error_code) { if (error_code != net::OK) { @@ -648,59 +691,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeRedirect( request_.request_body = nullptr; } -void ProxyingURLLoaderFactory::InProgressRequest:: - HandleBeforeRequestRedirect() { - // The extension requested a redirect. Close the connection with the current - // URLLoader and inform the URLLoaderClient the WebRequest API generated a - // redirect. To load |redirect_url_|, a new URLLoader will be recreated - // after receiving FollowRedirect(). - - // Forgetting to close the connection with the current URLLoader caused - // bugs. The latter doesn't know anything about the redirect. Continuing - // the load with it gives unexpected results. See - // https://crbug.com/882661#c72. - proxied_client_receiver_.reset(); - header_client_receiver_.reset(); - target_loader_.reset(); - - constexpr int kInternalRedirectStatusCode = net::HTTP_TEMPORARY_REDIRECT; - - net::RedirectInfo redirect_info; - redirect_info.status_code = kInternalRedirectStatusCode; - redirect_info.new_method = request_.method; - redirect_info.new_url = redirect_url_; - redirect_info.new_site_for_cookies = - net::SiteForCookies::FromUrl(redirect_url_); - - auto head = network::mojom::URLResponseHead::New(); - std::string headers = base::StringPrintf( - "HTTP/1.1 %i Internal Redirect\n" - "Location: %s\n" - "Non-Authoritative-Reason: WebRequest API\n\n", - kInternalRedirectStatusCode, redirect_url_.spec().c_str()); - - // Cross-origin requests need to modify the Origin header to 'null'. Since - // CorsURLLoader sets |request_initiator| to the Origin request header in - // NetworkService, we need to modify |request_initiator| here to craft the - // Origin header indirectly. - // Following checks implement the step 10 of "4.4. HTTP-redirect fetch", - // https://fetch.spec.whatwg.org/#http-redirect-fetch - if (request_.request_initiator && - (!url::Origin::Create(redirect_url_) - .IsSameOriginWith(url::Origin::Create(request_.url)) && - !request_.request_initiator->IsSameOriginWith( - url::Origin::Create(request_.url)))) { - // Reset the initiator to pretend tainted origin flag of the spec is set. - request_.request_initiator = url::Origin(); - } - head->headers = base::MakeRefCounted( - net::HttpUtil::AssembleRawHeaders(headers)); - head->encoded_data_length = 0; - - current_response_ = std::move(head); - ContinueToBeforeRedirect(redirect_info, net::OK); -} - void ProxyingURLLoaderFactory::InProgressRequest:: HandleResponseOrRedirectHeaders(net::CompletionOnceCallback continuation) { override_headers_ = nullptr; @@ -785,8 +775,6 @@ ProxyingURLLoaderFactory::ProxyingURLLoaderFactory( ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); } -ProxyingURLLoaderFactory::~ProxyingURLLoaderFactory() = default; - bool ProxyingURLLoaderFactory::ShouldIgnoreConnectionsLimit( const network::ResourceRequest& request) { for (const auto& domain : ignore_connections_limit_domains_) { @@ -884,11 +872,29 @@ void ProxyingURLLoaderFactory::OnLoaderCreated( request_it->second->OnLoaderCreated(std::move(receiver)); } -bool ProxyingURLLoaderFactory::IsForServiceWorkerScript() const { - return loader_factory_type_ == content::ContentBrowserClient:: - URLLoaderFactoryType::kServiceWorkerScript; +void ProxyingURLLoaderFactory::OnLoaderForCorsPreflightCreated( + const network::ResourceRequest& request, + mojo::PendingReceiver receiver) { + // Please note that the URLLoader is now starting, without waiting for + // additional signals from here. The URLLoader will be blocked before + // sending HTTP request headers (TrustedHeaderClient.OnBeforeSendHeaders), + // but the connection set up will be done before that. This is acceptable from + // Web Request API because the extension has already allowed to set up + // a connection to the same URL (i.e., the actual request), and distinguishing + // two connections for the actual request and the preflight request before + // sending request headers is very difficult. + const uint64_t web_request_id = ++(*request_id_generator_); + + auto result = requests_.insert(std::make_pair( + web_request_id, std::make_unique( + this, web_request_id, frame_routing_id_, request))); + + result.first->second->OnLoaderCreated(std::move(receiver)); + result.first->second->Restart(); } +ProxyingURLLoaderFactory::~ProxyingURLLoaderFactory() = default; + void ProxyingURLLoaderFactory::OnTargetFactoryError() { target_factory_.reset(); proxy_receivers_.Clear(); diff --git a/shell/browser/net/proxying_url_loader_factory.h b/shell/browser/net/proxying_url_loader_factory.h index f2cf05a0f86f..e417ccac910a 100644 --- a/shell/browser/net/proxying_url_loader_factory.h +++ b/shell/browser/net/proxying_url_loader_factory.h @@ -5,27 +5,34 @@ #ifndef SHELL_BROWSER_NET_PROXYING_URL_LOADER_FACTORY_H_ #define SHELL_BROWSER_NET_PROXYING_URL_LOADER_FACTORY_H_ +#include #include #include #include #include #include +#include "base/macros.h" +#include "base/memory/weak_ptr.h" #include "content/public/browser/content_browser_client.h" #include "content/public/browser/render_frame_host.h" #include "extensions/browser/api/web_request/web_request_info.h" #include "mojo/public/cpp/bindings/pending_receiver.h" #include "mojo/public/cpp/bindings/pending_remote.h" +#include "mojo/public/cpp/bindings/receiver.h" #include "mojo/public/cpp/bindings/receiver_set.h" #include "mojo/public/cpp/bindings/remote.h" +#include "net/base/completion_once_callback.h" +#include "net/traffic_annotation/network_traffic_annotation.h" #include "services/network/public/cpp/resource_request.h" #include "services/network/public/mojom/network_context.mojom.h" #include "services/network/public/mojom/url_loader.mojom.h" +#include "services/network/public/mojom/url_loader_factory.mojom.h" #include "services/network/public/mojom/url_response_head.mojom.h" -#include "shell/browser/api/electron_api_web_request.h" #include "shell/browser/net/electron_url_loader_factory.h" #include "shell/browser/net/web_request_api_interface.h" #include "third_party/abseil-cpp/absl/types/optional.h" +#include "url/gurl.h" namespace electron { @@ -43,7 +50,7 @@ class ProxyingURLLoaderFactory public network::mojom::URLLoaderClient, public network::mojom::TrustedHeaderClient { public: - // For usual requests. + // For usual requests InProgressRequest( ProxyingURLLoaderFactory* factory, int64_t web_request_id, @@ -55,7 +62,7 @@ class ProxyingURLLoaderFactory const net::MutableNetworkTrafficAnnotationTag& traffic_annotation, mojo::PendingReceiver loader_receiver, mojo::PendingRemote client); - // For CORS preflights. + // For CORS preflights InProgressRequest(ProxyingURLLoaderFactory* factory, uint64_t request_id, int32_t frame_routing_id, @@ -76,9 +83,9 @@ class ProxyingURLLoaderFactory void ResumeReadingBodyFromNet() override; // network::mojom::URLLoaderClient: - void OnReceiveResponse(network::mojom::URLResponseHeadPtr head) override; void OnReceiveEarlyHints( network::mojom::EarlyHintsPtr early_hints) override; + void OnReceiveResponse(network::mojom::URLResponseHeadPtr head) override; void OnReceiveRedirect(const net::RedirectInfo& redirect_info, network::mojom::URLResponseHeadPtr head) override; void OnUploadProgress(int64_t current_position, @@ -114,18 +121,18 @@ class ProxyingURLLoaderFactory void ContinueToResponseStarted(int error_code); void ContinueToBeforeRedirect(const net::RedirectInfo& redirect_info, int error_code); - void HandleBeforeRequestRedirect(); void HandleResponseOrRedirectHeaders( net::CompletionOnceCallback continuation); void OnRequestError(const network::URLLoaderCompletionStatus& status); + void HandleBeforeRequestRedirect(); - ProxyingURLLoaderFactory* factory_; + ProxyingURLLoaderFactory* const factory_; network::ResourceRequest request_; const absl::optional original_initiator_; const uint64_t request_id_ = 0; + const int32_t network_service_request_id_ = 0; const int32_t view_routing_id_ = MSG_ROUTING_NONE; const int32_t frame_routing_id_ = MSG_ROUTING_NONE; - const int32_t network_service_request_id_ = 0; const uint32_t options_ = 0; const net::MutableNetworkTrafficAnnotationTag traffic_annotation_; mojo::Receiver proxied_loader_receiver_; @@ -133,14 +140,14 @@ class ProxyingURLLoaderFactory absl::optional info_; - network::mojom::URLResponseHeadPtr current_response_; - scoped_refptr override_headers_; - GURL redirect_url_; - mojo::Receiver proxied_client_receiver_{ this}; network::mojom::URLLoaderPtr target_loader_; + network::mojom::URLResponseHeadPtr current_response_; + scoped_refptr override_headers_; + GURL redirect_url_; + const bool for_cors_preflight_ = false; // If |has_any_extra_headers_listeners_| is set to true, the request will be @@ -192,6 +199,7 @@ class ProxyingURLLoaderFactory mojo::PendingReceiver header_client_receiver, content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type); + ~ProxyingURLLoaderFactory() override; // network::mojom::URLLoaderFactory: