diff --git a/shell/browser/net/proxying_url_loader_factory.cc b/shell/browser/net/proxying_url_loader_factory.cc index 706ea48143..c23fcb9453 100644 --- a/shell/browser/net/proxying_url_loader_factory.cc +++ b/shell/browser/net/proxying_url_loader_factory.cc @@ -20,6 +20,7 @@ #include "shell/common/options_switches.h" namespace electron { + ProxyingURLLoaderFactory::InProgressRequest::FollowRedirectParams:: FollowRedirectParams() = default; ProxyingURLLoaderFactory::InProgressRequest::FollowRedirectParams:: @@ -33,7 +34,7 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( uint32_t options, const network::ResourceRequest& request, const net::MutableNetworkTrafficAnnotationTag& traffic_annotation, - network::mojom::URLLoaderRequest loader_request, + mojo::PendingReceiver loader_receiver, mojo::PendingRemote client) : factory_(factory), request_(request), @@ -43,7 +44,7 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( network_service_request_id_(network_service_request_id), options_(options), traffic_annotation_(traffic_annotation), - proxied_loader_binding_(this, std::move(loader_request)), + proxied_loader_receiver_(this, std::move(loader_receiver)), target_client_(std::move(client)), current_response_(network::mojom::URLResponseHead::New()), // Always use "extraHeaders" mode to be compatible with old APIs, except @@ -55,8 +56,24 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( &ProxyingURLLoaderFactory::InProgressRequest::OnRequestError, weak_factory_.GetWeakPtr(), network::URLLoaderCompletionStatus(net::ERR_ABORTED))); + proxied_loader_receiver_.set_disconnect_handler(base::BindOnce( + &ProxyingURLLoaderFactory::InProgressRequest::OnRequestError, + weak_factory_.GetWeakPtr(), + network::URLLoaderCompletionStatus(net::ERR_ABORTED))); } +ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( + ProxyingURLLoaderFactory* factory, + uint64_t request_id, + const network::ResourceRequest& request) + : factory_(factory), + request_(request), + original_initiator_(request.request_initiator), + request_id_(request_id), + proxied_loader_receiver_(this), + for_cors_preflight_(true), + has_any_extra_headers_listeners_(true) {} + ProxyingURLLoaderFactory::InProgressRequest::~InProgressRequest() { // This is important to ensure that no outstanding blocking requests continue // to reference state owned by this object. @@ -96,13 +113,13 @@ void ProxyingURLLoaderFactory::InProgressRequest::UpdateRequestInfo() { current_request_uses_header_client_ = factory_->url_loader_header_client_receiver_.is_bound() && - network_service_request_id_ != 0 && has_any_extra_headers_listeners_; + (for_cors_preflight_ || network_service_request_id_ != 0) && + has_any_extra_headers_listeners_; } void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { DCHECK_EQ(info_->url, request_.url) << "UpdateRequestInfo must have been called first"; - request_completed_ = false; // If the header client will be used, we start the request immediately, and // OnBeforeSendHeaders and OnSendHeaders will be handled there. Otherwise, @@ -111,6 +128,9 @@ void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { if (current_request_uses_header_client_) { continuation = base::BindRepeating( &InProgressRequest::ContinueToStartRequest, weak_factory_.GetWeakPtr()); + } else if (for_cors_preflight_) { + // In this case we do nothing because extensions should see nothing. + return; } else { continuation = base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders, @@ -282,6 +302,15 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnComplete( 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 + // network service, so we observe mojo connection errors. + header_client_receiver_.set_disconnect_handler(base::BindOnce( + &ProxyingURLLoaderFactory::InProgressRequest::OnRequestError, + weak_factory_.GetWeakPtr(), + network::URLLoaderCompletionStatus(net::ERR_FAILED))); + } } void ProxyingURLLoaderFactory::InProgressRequest::OnBeforeSendHeaders( @@ -303,6 +332,12 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnHeadersReceived( OnHeadersReceivedCallback callback) { if (!current_request_uses_header_client_) { std::move(callback).Run(net::OK, base::nullopt, GURL()); + + if (for_cors_preflight_) { + // CORS preflight is supported only when "extraHeaders" is specified. + // Deletes |this|. + factory_->RemoveRequest(network_service_request_id_, request_id_); + } return; } @@ -316,6 +351,27 @@ 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_); + + auto result = requests_.insert(std::make_pair( + web_request_id, + std::make_unique(this, web_request_id, request))); + + result.first->second->OnLoaderCreated(std::move(receiver)); + result.first->second->Restart(); +} + void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders( int error_code) { if (error_code != net::OK) { @@ -324,6 +380,11 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders( } if (!current_request_uses_header_client_ && !redirect_url_.is_empty()) { + if (for_cors_preflight_) { + // CORS preflight doesn't support redirect. + OnRequestError(network::URLLoaderCompletionStatus(net::ERR_FAILED)); + return; + } HandleBeforeRequestRedirect(); return; } @@ -427,6 +488,13 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToStartRequest( 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. @@ -465,15 +533,34 @@ void ProxyingURLLoaderFactory::InProgressRequest:: current_response_->headers = override_headers_; } } + + if (for_cors_preflight_ && !redirect_url_.is_empty()) { + OnRequestError(network::URLLoaderCompletionStatus(net::ERR_FAILED)); + return; + } + std::move(on_headers_received_callback_).Run(net::OK, headers, redirect_url_); override_headers_ = nullptr; + if (for_cors_preflight_) { + // If this is for CORS preflight, there is no associated client. We notify + // the completion here, and deletes |this|. + info_->AddResponseInfoFromResourceResponse(*current_response_); + factory_->web_request_api()->OnResponseStarted(&info_.value(), request_); + factory_->web_request_api()->OnCompleted(&info_.value(), request_, net::OK); + + // Deletes |this|. + factory_->RemoveRequest(network_service_request_id_, request_id_); + return; + } + if (proxied_client_receiver_.is_bound()) proxied_client_receiver_.Resume(); } void ProxyingURLLoaderFactory::InProgressRequest::ContinueToResponseStarted( int error_code) { + DCHECK(!for_cors_preflight_); if (error_code != net::OK) { OnRequestError(network::URLLoaderCompletionStatus(error_code)); return; @@ -545,8 +632,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeRedirect( // reset the request body manually. if (request_.method == net::HttpRequestHeaders::kGetMethod) request_.request_body = nullptr; - - request_completed_ = true; } void ProxyingURLLoaderFactory::InProgressRequest:: @@ -655,11 +740,10 @@ void ProxyingURLLoaderFactory::InProgressRequest:: void ProxyingURLLoaderFactory::InProgressRequest::OnRequestError( const network::URLLoaderCompletionStatus& status) { - if (!request_completed_) { + if (target_client_) target_client_->OnComplete(status); - factory_->web_request_api()->OnErrorOccurred(&info_.value(), request_, - status.error_code); - } + factory_->web_request_api()->OnErrorOccurred(&info_.value(), request_, + status.error_code); // Deletes |this|. factory_->RemoveRequest(network_service_request_id_, request_id_); diff --git a/shell/browser/net/proxying_url_loader_factory.h b/shell/browser/net/proxying_url_loader_factory.h index 58a6de1a6d..2a5f8f611f 100644 --- a/shell/browser/net/proxying_url_loader_factory.h +++ b/shell/browser/net/proxying_url_loader_factory.h @@ -43,6 +43,7 @@ class ProxyingURLLoaderFactory public network::mojom::URLLoaderClient, public network::mojom::TrustedHeaderClient { public: + // For usual requests. InProgressRequest( ProxyingURLLoaderFactory* factory, int64_t web_request_id, @@ -51,8 +52,12 @@ class ProxyingURLLoaderFactory uint32_t options, const network::ResourceRequest& request, const net::MutableNetworkTrafficAnnotationTag& traffic_annotation, - network::mojom::URLLoaderRequest loader_request, + mojo::PendingReceiver loader_receiver, mojo::PendingRemote client); + // For CORS preflights. + InProgressRequest(ProxyingURLLoaderFactory* factory, + uint64_t request_id, + const network::ResourceRequest& request); ~InProgressRequest() override; void Restart(); @@ -111,12 +116,12 @@ class ProxyingURLLoaderFactory ProxyingURLLoaderFactory* factory_; network::ResourceRequest request_; const base::Optional original_initiator_; - const uint64_t request_id_; - const int32_t routing_id_; - const int32_t network_service_request_id_; - const uint32_t options_; + const uint64_t request_id_ = 0; + const int32_t routing_id_ = 0; + const int32_t network_service_request_id_ = 0; + const uint32_t options_ = 0; const net::MutableNetworkTrafficAnnotationTag traffic_annotation_; - mojo::Binding proxied_loader_binding_; + mojo::Receiver proxied_loader_receiver_; mojo::Remote target_client_; base::Optional info_; @@ -129,7 +134,7 @@ class ProxyingURLLoaderFactory this}; network::mojom::URLLoaderPtr target_loader_; - bool request_completed_ = false; + const bool for_cors_preflight_ = false; // If |has_any_extra_headers_listeners_| is set to true, the request will be // sent with the network::mojom::kURLLoadOptionUseHeaderClient option, and @@ -201,7 +206,7 @@ class ProxyingURLLoaderFactory void OnLoaderForCorsPreflightCreated( const network::ResourceRequest& request, mojo::PendingReceiver receiver) - override {} + override; WebRequestAPI* web_request_api() { return web_request_api_; } diff --git a/spec-main/api-web-request-spec.ts b/spec-main/api-web-request-spec.ts index 63a1ee1840..7b5a97f66f 100644 --- a/spec-main/api-web-request-spec.ts +++ b/spec-main/api-web-request-spec.ts @@ -3,7 +3,7 @@ import * as http from 'http' import * as qs from 'querystring' import * as path from 'path' import * as WebSocket from 'ws' -import { ipcMain, session, WebContents, webContents } from 'electron' +import { ipcMain, protocol, session, WebContents, webContents } from 'electron' import { AddressInfo } from 'net' import { emittedOnce } from './events-helpers' @@ -31,6 +31,7 @@ describe('webRequest module', () => { let defaultURL: string before((done) => { + protocol.registerStringProtocol('neworigin', (req, cb) => cb('')) server.listen(0, '127.0.0.1', () => { const port = (server.address() as AddressInfo).port defaultURL = `http://127.0.0.1:${port}/` @@ -40,6 +41,7 @@ describe('webRequest module', () => { after(() => { server.close() + protocol.unregisterProtocol('neworigin') }) let contents: WebContents = null as unknown as WebContents @@ -158,7 +160,7 @@ describe('webRequest module', () => { expect(data).to.equal('/header/received') }) - it('can change CORS headers', async () => { + it('can change request origin', async () => { ses.webRequest.onBeforeSendHeaders((details, callback) => { const requestHeaders = details.requestHeaders requestHeaders.Origin = 'http://new-origin' @@ -168,6 +170,16 @@ describe('webRequest module', () => { expect(data).to.equal('/new/origin') }) + it('can capture CORS requests', async () => { + let called = false + ses.webRequest.onBeforeSendHeaders((details, callback) => { + called = true + callback({ requestHeaders: details.requestHeaders }) + }) + await ajax('neworigin://host') + expect(called).to.be.true() + }) + it('resets the whole headers', async () => { const requestHeaders = { Test: 'header' @@ -222,7 +234,7 @@ describe('webRequest module', () => { expect(headers).to.match(/^custom: Changed$/m) }) - it('can change CORS headers', async () => { + it('can change response origin', async () => { ses.webRequest.onHeadersReceived((details, callback) => { const responseHeaders = details.responseHeaders! responseHeaders['access-control-allow-origin'] = ['http://new-origin'] as any @@ -232,6 +244,16 @@ describe('webRequest module', () => { expect(headers).to.match(/^access-control-allow-origin: http:\/\/new-origin$/m) }) + it('can change headers of CORS responses', async () => { + ses.webRequest.onHeadersReceived((details, callback) => { + const responseHeaders = details.responseHeaders! + responseHeaders['Custom'] = ['Changed'] as any + callback({ responseHeaders: responseHeaders }) + }) + const { headers } = await ajax('neworigin://host') + expect(headers).to.match(/^custom: Changed$/m) + }) + it('does not change header by default', async () => { ses.webRequest.onHeadersReceived((details, callback) => { callback({})