fix: webRequest should be able to modify CORS headers (electron#21099)

zcbenz · miniak · web-flow · commit b02a20e4dc25 · 2019-11-14T14:51:24.000+09:00
* fix: always use extraHeaders mode

* fix: clear pending callbacks

* fix: do not use "extraHeaders" for net module

* test: webRequest should be able to modify CROS headers

* chore: CROS =&gt; CORS

Co-Authored-By: Milan Burda &lt;milan.burda@gmail.com&gt;

* chore: CROS =&gt; CORS

Co-Authored-By: Milan Burda &lt;milan.burda@gmail.com&gt;
diff --git a/shell/browser/api/atom_api_web_request.cc b/shell/browser/api/atom_api_web_request.cc
@@ -332,6 +332,10 @@ void WebRequest::OnCompleted(extensions::WebRequestInfo* info,
   HandleSimpleEvent(kOnCompleted, info, request, net_error);
 }
 
+void WebRequest::OnRequestWillBeDestroyed(extensions::WebRequestInfo* info) {
+  callbacks_.erase(info->id);
+}
+
 template <WebRequest::SimpleEvent event>
 void WebRequest::SetSimpleListener(gin::Arguments* args) {
   SetListener<SimpleListener>(event, &simple_listeners_, args);
diff --git a/shell/browser/api/atom_api_web_request.h b/shell/browser/api/atom_api_web_request.h
@@ -84,6 +84,7 @@ class WebRequest : public gin::Wrappable<WebRequest>, public WebRequestAPI {
   void OnCompleted(extensions::WebRequestInfo* info,
                    const network::ResourceRequest& request,
                    int net_error) override;
+  void OnRequestWillBeDestroyed(extensions::WebRequestInfo* info) override;
 
   enum SimpleEvent {
     kOnSendHeaders,
diff --git a/shell/browser/net/proxying_url_loader_factory.cc b/shell/browser/net/proxying_url_loader_factory.cc
@@ -49,9 +49,10 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
       target_client_(std::move(client)),
       current_response_(network::mojom::URLResponseHead::New()),
       proxied_client_binding_(this),
-      // TODO(zcbenz): We should always use "extraHeaders" mode to be compatible
-      // with old APIs.
-      has_any_extra_headers_listeners_(false) {
+      // Always use "extraHeaders" mode to be compatible with old APIs, except
+      // when the |request_id_| is zero, which is not supported in Chromium and
+      // only happens in Electron when the request is started from net module.
+      has_any_extra_headers_listeners_(network_service_request_id != 0) {
   // If there is a client error, clean up the request.
   target_client_.set_connection_error_handler(base::BindOnce(
       &ProxyingURLLoaderFactory::InProgressRequest::OnRequestError,
@@ -60,7 +61,19 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
 }
 
 ProxyingURLLoaderFactory::InProgressRequest::~InProgressRequest() {
-  // TODO(zcbenz): Do cleanup here.
+  // This is important to ensure that no outstanding blocking requests continue
+  // to reference state owned by this object.
+  if (info_) {
+    factory_->web_request_api()->OnRequestWillBeDestroyed(&info_.value());
+  }
+  if (on_before_send_headers_callback_) {
+    std::move(on_before_send_headers_callback_)
+        .Run(net::ERR_ABORTED, base::nullopt);
+  }
+  if (on_headers_received_callback_) {
+    std::move(on_headers_received_callback_)
+        .Run(net::ERR_ABORTED, base::nullopt, base::nullopt);
+  }
 }
 
 void ProxyingURLLoaderFactory::InProgressRequest::Restart() {
@@ -84,8 +97,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::UpdateRequestInfo() {
 
   current_request_uses_header_client_ =
       factory_->url_loader_header_client_receiver_.is_bound() &&
-      network_service_request_id_ != 0 &&
-      false /* TODO(zcbenz): HasExtraHeadersListenerForRequest */;
+      network_service_request_id_ != 0 && has_any_extra_headers_listeners_;
 }
 
 void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() {
@@ -722,6 +734,10 @@ void ProxyingURLLoaderFactory::CreateLoaderAndStart(
   // don't use it for identity here.
   const uint64_t web_request_id = ++g_request_id;
 
+  // Notes: Chromium assumes that requests with zero-ID would never use the
+  // "extraHeaders" code path, however in Electron requests started from
+  // the net module would have zero-ID because they do not have renderer process
+  // associated.
   if (request_id)
     network_request_id_to_web_request_id_.emplace(request_id, web_request_id);
 
diff --git a/shell/browser/net/proxying_url_loader_factory.h b/shell/browser/net/proxying_url_loader_factory.h
@@ -67,6 +67,7 @@ class WebRequestAPI {
   virtual void OnCompleted(extensions::WebRequestInfo* info,
                            const network::ResourceRequest& request,
                            int net_error) = 0;
+  virtual void OnRequestWillBeDestroyed(extensions::WebRequestInfo* info) = 0;
 };
 
 // This class is responsible for following tasks when NetworkService is enabled:
diff --git a/spec-main/api-web-request-spec.ts b/spec-main/api-web-request-spec.ts
@@ -20,6 +20,9 @@ describe('webRequest module', () => {
       if (req.headers.accept === '*/*;test/header') {
         content += 'header/received'
       }
+      if (req.headers.origin === 'http://new-origin') {
+        content += 'new/origin'
+      }
       res.end(content)
     }
   })
@@ -145,6 +148,16 @@ describe('webRequest module', () => {
       expect(data).to.equal('/header/received')
     })
 
+    it('can change CORS headers', async () => {
+      ses.webRequest.onBeforeSendHeaders((details, callback) => {
+        const requestHeaders = details.requestHeaders
+        requestHeaders.Origin = 'http://new-origin'
+        callback({ requestHeaders: requestHeaders })
+      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/new/origin')
+    })
+
     it('resets the whole headers', async () => {
       const requestHeaders = {
         Test: 'header'
@@ -199,6 +212,16 @@ describe('webRequest module', () => {
       expect(headers).to.match(/^custom: Changed$/m)
     })
 
+    it('can change CORS headers', async () => {
+      ses.webRequest.onHeadersReceived((details, callback) => {
+        const responseHeaders = details.responseHeaders!
+        responseHeaders['access-control-allow-origin'] = ['http://new-origin'] as any
+        callback({ responseHeaders: responseHeaders })
+      })
+      const { headers } = await ajax(defaultURL)
+      expect(headers).to.match(/^access-control-allow-origin: http:\/\/new-origin$/m)
+    })
+
     it('does not change header by default', async () => {
       ses.webRequest.onHeadersReceived((details, callback) => {
         callback({})
