From 507cbdc80a45580acfecf6c1c136471146910c83 Mon Sep 17 00:00:00 2001 From: marekharanczyk <48673767+marekharanczyk@users.noreply.github.com> Date: Mon, 21 Jun 2021 07:06:52 +0200 Subject: [PATCH] fix: do not cancel CORS preflight request on proxy auth. (#29266) * fix: do not cancel CORS preflight request on proxy auth. If connecting via proxy, preflight request can receive 407 header response from proxy. This does not mean request was finished even though it received headers (from proxy, not the destination server), so prevent "completing" and most importantly deleting it, which causes request to be canceled in network layer. Just continue to monitor it and await proper response from server. Also add circut breaker to cancel request if proxy auth failed 3 times (for example user keeps cancelling auth). This behavior happens only when app registered WebRequest api listeners. * Port chromium webrequest changes to electron code. Move relevant parts of chromium WebRequestProxyingURLLoaderFactory from https://chromium-review.googlesource.com/c/chromium/src/+/2011781 into electron ProxyingURLLoaderFactory. * Update code to upstreamed version and remove retyr count failsafe. Co-authored-by: Milan Burda --- .../browser/net/proxying_url_loader_factory.cc | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/shell/browser/net/proxying_url_loader_factory.cc b/shell/browser/net/proxying_url_loader_factory.cc index 85a886f4752e..8b87367fa364 100644 --- a/shell/browser/net/proxying_url_loader_factory.cc +++ b/shell/browser/net/proxying_url_loader_factory.cc @@ -326,6 +326,14 @@ bool ProxyingURLLoaderFactory::IsForServiceWorkerScript() const { void ProxyingURLLoaderFactory::InProgressRequest::OnLoaderCreated( mojo::PendingReceiver receiver) { + // When CORS is involved there may be multiple network::URLLoader associated + // with this InProgressRequest, because CorsURLLoader may create a new + // network::URLLoader for the same request id in redirect handling - see + // CorsURLLoader::FollowRedirect. In such a case the old network::URLLoader + // is going to be detached fairly soon, so we don't need to take care of it. + // We need this explicit reset to avoid a DCHECK failure in mojo::Receiver. + header_client_receiver_.reset(); + header_client_receiver_.Bind(std::move(receiver)); if (for_cors_preflight_) { // In this case we don't have |target_loader_| and @@ -600,13 +608,17 @@ void ProxyingURLLoaderFactory::InProgressRequest:: 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|. + // If this is for CORS preflight, there is no associated client. info_->AddResponseInfoFromResourceResponse(*current_response_); + // Do not finish proxied preflight requests that require proxy auth. + // The request is not finished yet, give control back to network service + // which will start authentication process. + if (info_->response_code == net::HTTP_PROXY_AUTHENTICATION_REQUIRED) + return; + // We notify the completion here, and delete |this|. 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; }