fix: handle redirects within registered protocols

shell/browser/net/electron_url_loader_factory.cc

@@ -167,6 +167,83 @@ void OnWrite(std::unique_ptr<WriteData> write_data, MojoResult result) {
 
 }  // namespace
 
+ElectronURLLoaderFactory::RedirectedRequest::RedirectedRequest(
+    const net::RedirectInfo& redirect_info,
+    mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver,
+    int32_t request_id,
+    uint32_t options,
+    const network::ResourceRequest& request,
+    mojo::PendingRemote<network::mojom::URLLoaderClient> client,
+    const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
+    mojo::PendingRemote<network::mojom::URLLoaderFactory> target_factory_remote)
+    : redirect_info_(redirect_info),
+      request_id_(request_id),
+      options_(options),
+      request_(request),
+      client_(std::move(client)),
+      traffic_annotation_(traffic_annotation) {
+  loader_receiver_.Bind(std::move(loader_receiver));
+  loader_receiver_.set_disconnect_handler(
+      base::BindOnce(&ElectronURLLoaderFactory::RedirectedRequest::DeleteThis,
+                     base::Unretained(this)));
+  target_factory_remote_.Bind(std::move(target_factory_remote));
+  target_factory_remote_.set_disconnect_handler(base::BindOnce(
+      &ElectronURLLoaderFactory::RedirectedRequest::OnTargetFactoryError,
+      base::Unretained(this)));
+}
+
+ElectronURLLoaderFactory::RedirectedRequest::~RedirectedRequest() = default;
+
+void ElectronURLLoaderFactory::RedirectedRequest::FollowRedirect(
+    const std::vector<std::string>& removed_headers,
+    const net::HttpRequestHeaders& modified_headers,
+    const net::HttpRequestHeaders& modified_cors_exempt_headers,
+    const absl::optional<GURL>& new_url) {
+  // Update |request_| with info from the redirect, so that it's accurate
+  // The following references code in WorkerScriptLoader::FollowRedirect
+  bool should_clear_upload = false;
+  net::RedirectUtil::UpdateHttpRequest(
+      request_.url, request_.method, redirect_info_, removed_headers,
+      modified_headers, &request_.headers, &should_clear_upload);
+  request_.cors_exempt_headers.MergeFrom(modified_cors_exempt_headers);
+  for (const std::string& name : removed_headers)
+    request_.cors_exempt_headers.RemoveHeader(name);
+
+  if (should_clear_upload)
+    request_.request_body = nullptr;
+
+  request_.url = redirect_info_.new_url;
+  request_.method = redirect_info_.new_method;
+  request_.site_for_cookies = redirect_info_.new_site_for_cookies;
+  request_.referrer = GURL(redirect_info_.new_referrer);
+  request_.referrer_policy = redirect_info_.new_referrer_policy;
+
+  // Create a new loader to process the redirect and destroy this one
+  target_factory_remote_->CreateLoaderAndStart(
+      loader_receiver_.Unbind(), request_id_, options_, request_,
+      std::move(client_), traffic_annotation_);
+
+  DeleteThis();
+}
+
+void ElectronURLLoaderFactory::RedirectedRequest::OnTargetFactoryError() {
+  // Can't create a new loader at this point, so the request can't continue
+  mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+      std::move(client_));
+  client_remote->OnComplete(
+      network::URLLoaderCompletionStatus(net::ERR_FAILED));
+  client_remote.reset();
+
+  DeleteThis();
+}
+
+void ElectronURLLoaderFactory::RedirectedRequest::DeleteThis() {
+  loader_receiver_.reset();
+  target_factory_remote_.reset();
+
+  delete this;
+}
+
 // static
 mojo::PendingRemote<network::mojom::URLLoaderFactory>
 ElectronURLLoaderFactory::Create(ProtocolType type,
@@ -199,12 +276,18 @@ void ElectronURLLoaderFactory::CreateLoaderAndStart(
     mojo::PendingRemote<network::mojom::URLLoaderClient> client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  mojo::PendingRemote<network::mojom::URLLoaderFactory> proxy_factory;
+
+  // |StartLoading| is used for both intercepted and registered protocols,
+  // and on redirects it needs a factory to use to create a loader for the
+  // new request. So in this case, this factory is the target factory.
+  mojo::PendingRemote<network::mojom::URLLoaderFactory> target_factory;
+  this->Clone(target_factory.InitWithNewPipeAndPassReceiver());
+
   handler_.Run(
       request,
       base::BindOnce(&ElectronURLLoaderFactory::StartLoading, std::move(loader),
                      request_id, options, request, std::move(client),
-                     traffic_annotation, std::move(proxy_factory), type_));
+                     traffic_annotation, std::move(target_factory), type_));
 }
 
 // static
@@ -227,7 +310,7 @@ void ElectronURLLoaderFactory::StartLoading(
     const network::ResourceRequest& request,
     mojo::PendingRemote<network::mojom::URLLoaderClient> client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-    mojo::PendingRemote<network::mojom::URLLoaderFactory> proxy_factory,
+    mojo::PendingRemote<network::mojom::URLLoaderFactory> target_factory,
     ProtocolType type,
     gin::Arguments* args) {
   // Send network error when there is no argument passed.
@@ -277,47 +360,22 @@ void ElectronURLLoaderFactory::StartLoading(
         request.url.Resolve(location),
         net::RedirectUtil::GetReferrerPolicyHeader(head->headers.get()), false);
 
-    network::ResourceRequest new_request = request;
-    new_request.method = redirect_info.new_method;
-    new_request.url = redirect_info.new_url;
-    new_request.site_for_cookies = redirect_info.new_site_for_cookies;
-    new_request.referrer = GURL(redirect_info.new_referrer);
-    new_request.referrer_policy = redirect_info.new_referrer_policy;
-
     DCHECK(client.is_valid());
 
     mojo::Remote<network::mojom::URLLoaderClient> client_remote(
         std::move(client));
 
     client_remote->OnReceiveRedirect(redirect_info, std::move(head));
 
-    // Unbound client, so it an be passed to sub-methods
-    client = client_remote.Unbind();
-    // When the redirection comes from an intercepted scheme (which has
-    // |proxy_factory| passed), we ask the proxy factory to create a loader
-    // for new URL, otherwise we call |StartLoadingHttp|, which creates
-    // loader with default factory.
-    //
-    // Note that when handling requests for intercepted scheme, creating loader
-    // with default factory (i.e. calling StartLoadingHttp) would bypass the
-    // ProxyingURLLoaderFactory, we have to explicitly use the proxy factory to
-    // create loader so it is possible to have handlers of intercepted scheme
-    // getting called recursively, which is a behavior expected in protocol
-    // module.
-    //
-    // I'm not sure whether this is an intended behavior in Chromium.
-    if (proxy_factory.is_valid()) {
-      mojo::Remote<network::mojom::URLLoaderFactory> proxy_factory_remote(
-          std::move(proxy_factory));
-
-      proxy_factory_remote->CreateLoaderAndStart(
-          std::move(loader), request_id, options, new_request,
-          std::move(client), traffic_annotation);
-    } else {
-      StartLoadingHttp(std::move(loader), new_request, std::move(client),
-                       traffic_annotation,
-                       gin::Dictionary::CreateEmpty(args->isolate()));
-    }
+    // Bind the URLLoader receiver and wait for a FollowRedirect request, or for
+    // the remote to disconnect, which will happen if the request is aborted.
+    // That may happen when the redirect is to a different scheme, which will
+    // cause the URL loader to be destroyed and a new one created using the
+    // factory for that scheme.
+    new RedirectedRequest(redirect_info, std::move(loader), request_id, options,
+                          request, client_remote.Unbind(), traffic_annotation,
+                          std::move(target_factory));
+
     return;
   }
 
@@ -357,7 +415,7 @@ void ElectronURLLoaderFactory::StartLoading(
       }
       StartLoading(std::move(loader), request_id, options, request,
                    std::move(client), traffic_annotation,
-                   std::move(proxy_factory), type, args);
+                   std::move(target_factory), type, args);
       break;
   }
 }

shell/browser/net/electron_url_loader_factory.h

@@ -8,16 +8,19 @@
 #include <map>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "net/url_request/url_request_job_factory.h"
 #include "services/network/public/cpp/self_deleting_url_loader_factory.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/common/gin_helper/dictionary.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace electron {
 
@@ -43,6 +46,52 @@ using HandlersMap =
 // Implementation of URLLoaderFactory.
 class ElectronURLLoaderFactory : public network::SelfDeletingURLLoaderFactory {
  public:
+  // This class binds a URLLoader receiver in the case of a redirect, waiting
+  // for |FollowRedirect| to be called at which point the new request will be
+  // started, and the receiver will be unbound letting a new URLLoader bind it
+  class RedirectedRequest : public network::mojom::URLLoader {
+   public:
+    RedirectedRequest(
+        const net::RedirectInfo& redirect_info,
+        mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver,
+        int32_t request_id,
+        uint32_t options,
+        const network::ResourceRequest& request,
+        mojo::PendingRemote<network::mojom::URLLoaderClient> client,
+        const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
+        mojo::PendingRemote<network::mojom::URLLoaderFactory>
+            target_factory_remote);
+    ~RedirectedRequest() override;
+
+    // network::mojom::URLLoader:
+    void FollowRedirect(
+        const std::vector<std::string>& removed_headers,
+        const net::HttpRequestHeaders& modified_headers,
+        const net::HttpRequestHeaders& modified_cors_exempt_headers,
+        const absl::optional<GURL>& new_url) override;
+    void SetPriority(net::RequestPriority priority,
+                     int32_t intra_priority_value) override {}
+    void PauseReadingBodyFromNet() override {}
+    void ResumeReadingBodyFromNet() override {}
+
+    void OnTargetFactoryError();
+    void DeleteThis();
+
+   private:
+    net::RedirectInfo redirect_info_;
+
+    mojo::Receiver<network::mojom::URLLoader> loader_receiver_{this};
+    int32_t request_id_;
+    uint32_t options_;
+    network::ResourceRequest request_;
+    mojo::PendingRemote<network::mojom::URLLoaderClient> client_;
+    net::MutableNetworkTrafficAnnotationTag traffic_annotation_;
+
+    mojo::Remote<network::mojom::URLLoaderFactory> target_factory_remote_;
+
+    DISALLOW_COPY_AND_ASSIGN(RedirectedRequest);
+  };
+
   static mojo::PendingRemote<network::mojom::URLLoaderFactory> Create(
       ProtocolType type,
       const ProtocolHandler& handler);
@@ -64,7 +113,7 @@ class ElectronURLLoaderFactory : public network::SelfDeletingURLLoaderFactory {
       const network::ResourceRequest& request,
       mojo::PendingRemote<network::mojom::URLLoaderClient> client,
       const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-      mojo::PendingRemote<network::mojom::URLLoaderFactory> proxy_factory,
+      mojo::PendingRemote<network::mojom::URLLoaderFactory> target_factory,
       ProtocolType type,
       gin::Arguments* args);
 

spec-main/api-protocol-spec.ts

@@ -120,6 +120,24 @@ describe('protocol module', () => {
       const r = await ajax(protocolName + '://fake-host');
       expect(r.data).to.equal(text);
     });
+
+    it('can redirect to the same scheme', async () => {
+      registerStringProtocol(protocolName, (request, callback) => {
+        if (request.url === `${protocolName}://fake-host/redirect`) {
+          callback({
+            statusCode: 302,
+            headers: {
+              Location: `${protocolName}://fake-host`
+            }
+          });
+        } else {
+          expect(request.url).to.equal(`${protocolName}://fake-host`);
+          callback('redirected');
+        }
+      });
+      const r = await ajax(`${protocolName}://fake-host/redirect`);
+      expect(r.data).to.equal('redirected');
+    });
   });
 
   describe('protocol.unregisterProtocol', () => {

spec-main/api-web-request-spec.ts

@@ -181,6 +181,42 @@ describe('webRequest module', () => {
       expect(data).to.equal('/header/received');
     });
 
+    it('can change the request headers on a custom protocol redirect', async () => {
+      protocol.registerStringProtocol('custom-scheme', (req, callback) => {
+        if (req.url === 'custom-scheme://fake-host/redirect') {
+          callback({
+            statusCode: 302,
+            headers: {
+              Location: 'custom-scheme://fake-host'
+            }
+          });
+        } else {
+          let content = '';
+          if (req.headers.Accept === '*/*;test/header') {
+            content = 'header-received';
+          }
+          callback(content);
+        }
+      });
+
+      // Note that we need to do navigation every time after a protocol is
+      // registered or unregistered, otherwise the new protocol won't be
+      // recognized by current page when NetworkService is used.
+      await contents.loadFile(path.join(__dirname, 'fixtures', 'pages', 'jquery.html'));
+
+      try {
+        ses.webRequest.onBeforeSendHeaders((details, callback) => {
+          const requestHeaders = details.requestHeaders;
+          requestHeaders.Accept = '*/*;test/header';
+          callback({ requestHeaders: requestHeaders });
+        });
+        const { data } = await ajax('custom-scheme://fake-host/redirect');
+        expect(data).to.equal('header-received');
+      } finally {
+        protocol.unregisterProtocol('custom-scheme');
+      }
+    });
+
     it('can change request origin', async () => {
       ses.webRequest.onBeforeSendHeaders((details, callback) => {
         const requestHeaders = details.requestHeaders;