fix: webRequest should be able to modify CORS headers (7-1-x) (#21123)

* test: move webRequest spec to main runner (#19992)

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

docs/api/web-request.md

@@ -99,7 +99,7 @@ Some examples of valid `urls`:
     * `timestamp` Double
     * `requestHeaders` Record<string, string>
   * `callback` Function
-    * `response` Object
+    * `beforeSendResponse` Object
       * `cancel` Boolean (optional)
       * `requestHeaders` Record<string, string | string[]> (optional) - When provided, request will be made
   with these headers.
@@ -148,7 +148,7 @@ response are visible by the time this listener is fired.
     * `statusCode` Integer
     * `responseHeaders` Record<string, string> (optional)
   * `callback` Function
-    * `response` Object
+    * `headersReceivedResponse` Object
       * `cancel` Boolean (optional)
       * `responseHeaders` Record<string, string | string[]> (optional) - When provided, the server is assumed
         to have responded with these headers.

shell/browser/api/atom_api_web_request_ns.cc

@@ -332,6 +332,10 @@ void WebRequestNS::OnCompleted(extensions::WebRequestInfo* info,
   HandleSimpleEvent(kOnCompleted, info, request, net_error);
 }
 
+void WebRequestNS::OnRequestWillBeDestroyed(extensions::WebRequestInfo* info) {
+  callbacks_.erase(info->id);
+}
+
 template <WebRequestNS::SimpleEvent event>
 void WebRequestNS::SetSimpleListener(gin::Arguments* args) {
   SetListener<SimpleListener>(event, &simple_listeners_, args);

shell/browser/api/atom_api_web_request_ns.h

@@ -87,6 +87,7 @@ class WebRequestNS : public gin::Wrappable<WebRequestNS>, public WebRequestAPI {
   void OnCompleted(extensions::WebRequestInfo* info,
                    const network::ResourceRequest& request,
                    int net_error) override;
+  void OnRequestWillBeDestroyed(extensions::WebRequestInfo* info) override;
 
   enum SimpleEvent {
     kOnSendHeaders,

shell/browser/net/proxying_url_loader_factory.cc

@@ -47,9 +47,10 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
       proxied_loader_binding_(this, std::move(loader_request)),
       target_client_(std::move(client)),
       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,
@@ -58,7 +59,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, GURL());
+  }
 }
 
 void ProxyingURLLoaderFactory::InProgressRequest::Restart() {
@@ -82,8 +95,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() {
@@ -715,6 +727,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);
 

shell/browser/net/proxying_url_loader_factory.h

@@ -63,6 +63,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:

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

@@ -1,16 +1,11 @@
-const chai = require('chai')
-const dirtyChai = require('dirty-chai')
+import { expect } from 'chai'
+import * as http from 'http'
+import * as qs from 'querystring'
+import * as path from 'path'
+import { session, WebContents, webContents } from 'electron'
+import { AddressInfo } from 'net';
 
-const http = require('http')
-const qs = require('querystring')
-const remote = require('electron').remote
-const session = remote.session
-
-const { expect } = chai
-chai.use(dirtyChai)
-
-/* The whole webRequest API doesn't use standard callbacks */
-/* eslint-disable standard/no-callback-literal */
+const fixturesPath = path.resolve(__dirname, '..', 'spec', 'fixtures')
 
 describe('webRequest module', () => {
   const ses = session.defaultSession
@@ -25,15 +20,18 @@ 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)
     }
   })
-  let defaultURL = null
+  let defaultURL: string
 
   before((done) => {
     server.listen(0, '127.0.0.1', () => {
-      const port = server.address().port
-      defaultURL = 'http://127.0.0.1:' + port + '/'
+      const port = (server.address() as AddressInfo).port
+      defaultURL = `http://127.0.0.1:${port}/`
       done()
     })
   })
@@ -42,48 +40,43 @@ describe('webRequest module', () => {
     server.close()
   })
 
+  let contents: WebContents = null as unknown as WebContents
+  // NB. sandbox: true is used because it makes navigations much (~8x) faster.
+  before(async () => {
+    contents = (webContents as any).create({sandbox: true})
+    await contents.loadFile(path.join(fixturesPath, 'pages', 'jquery.html'))
+  })
+  after(() => (contents as any).destroy())
+
+  async function ajax (url: string, options = {}) {
+    return contents.executeJavaScript(`ajax("${url}", ${JSON.stringify(options)})`)
+  }
+
   describe('webRequest.onBeforeRequest', () => {
     afterEach(() => {
       ses.webRequest.onBeforeRequest(null)
     })
 
-    it('can cancel the request', (done) => {
+    it('can cancel the request', async () => {
       ses.webRequest.onBeforeRequest((details, callback) => {
         callback({
           cancel: true
         })
       })
-      $.ajax({
-        url: defaultURL,
-        success: () => {
-          done('unexpected success')
-        },
-        error: () => {
-          done()
-        }
-      })
+      await expect(ajax(defaultURL)).to.eventually.be.rejectedWith('404')
     })
 
-    it('can filter URLs', (done) => {
+    it('can filter URLs', async () => {
       const filter = { urls: [defaultURL + 'filter/*'] }
       ses.webRequest.onBeforeRequest(filter, (details, callback) => {
         callback({ cancel: true })
       })
-      $.ajax({
-        url: `${defaultURL}nofilter/test`,
-        success: (data) => {
-          expect(data).to.equal('/nofilter/test')
-          $.ajax({
-            url: `${defaultURL}filter/test`,
-            success: () => done('unexpected success'),
-            error: () => done()
-          })
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(`${defaultURL}nofilter/test`)
+      expect(data).to.equal('/nofilter/test')
+      await expect(ajax(`${defaultURL}filter/test`)).to.eventually.be.rejectedWith('404')
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onBeforeRequest((details, callback) => {
         expect(details.id).to.be.a('number')
         expect(details.timestamp).to.be.a('number')
@@ -91,20 +84,13 @@ describe('webRequest module', () => {
         expect(details.url).to.be.a('string').that.is.equal(defaultURL)
         expect(details.method).to.be.a('string').that.is.equal('GET')
         expect(details.resourceType).to.be.a('string').that.is.equal('xhr')
-        expect(details.uploadData).to.be.undefined()
         callback({})
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/')
     })
 
-    it('receives post data in details object', (done) => {
+    it('receives post data in details object', async () => {
       const postData = {
         name: 'post test',
         type: 'string'
@@ -117,16 +103,13 @@ describe('webRequest module', () => {
         expect(data).to.deep.equal(postData)
         callback({ cancel: true })
       })
-      $.ajax({
-        url: defaultURL,
+      await expect(ajax(defaultURL, {
         type: 'POST',
         data: postData,
-        success: () => {},
-        error: () => done()
-      })
+      })).to.eventually.be.rejectedWith('404')
     })
 
-    it('can redirect the request', (done) => {
+    it('can redirect the request', async () => {
       ses.webRequest.onBeforeRequest((details, callback) => {
         if (details.url === defaultURL) {
           callback({ redirectURL: `${defaultURL}redirect` })
@@ -134,14 +117,8 @@ describe('webRequest module', () => {
           callback({})
         }
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/redirect')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/redirect')
     })
   })
 
@@ -150,40 +127,37 @@ describe('webRequest module', () => {
       ses.webRequest.onBeforeSendHeaders(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onBeforeSendHeaders((details, callback) => {
         expect(details.requestHeaders).to.be.an('object')
         expect(details.requestHeaders['Foo.Bar']).to.equal('baz')
         callback({})
       })
-      $.ajax({
-        url: defaultURL,
-        headers: { 'Foo.Bar': 'baz' },
-        success: (data) => {
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL, { headers: { 'Foo.Bar': 'baz' } })
+      expect(data).to.equal('/')
     })
 
-    it('can change the request headers', (done) => {
+    it('can change the request headers', async () => {
       ses.webRequest.onBeforeSendHeaders((details, callback) => {
         const requestHeaders = details.requestHeaders
         requestHeaders.Accept = '*/*;test/header'
         callback({ requestHeaders: requestHeaders })
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/header/received')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
+      const { data } = await ajax(defaultURL)
+      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', (done) => {
+    it('resets the whole headers', async () => {
       const requestHeaders = {
         Test: 'header'
       }
@@ -192,12 +166,8 @@ describe('webRequest module', () => {
       })
       ses.webRequest.onSendHeaders((details) => {
         expect(details.requestHeaders).to.deep.equal(requestHeaders)
-        done()
-      })
-      $.ajax({
-        url: defaultURL,
-        error: (xhr, errorType) => done(errorType)
       })
+      await ajax(defaultURL)
     })
   })
 
@@ -206,18 +176,12 @@ describe('webRequest module', () => {
       ses.webRequest.onSendHeaders(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onSendHeaders((details) => {
         expect(details.requestHeaders).to.be.an('object')
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/')
     })
   })
 
@@ -226,71 +190,56 @@ describe('webRequest module', () => {
       ses.webRequest.onHeadersReceived(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onHeadersReceived((details, callback) => {
         expect(details.statusLine).to.equal('HTTP/1.1 200 OK')
         expect(details.statusCode).to.equal(200)
-        expect(details.responseHeaders['Custom']).to.deep.equal(['Header'])
+        expect(details.responseHeaders!['Custom']).to.deep.equal(['Header'])
         callback({})
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/')
     })
 
-    it('can change the response header', (done) => {
+    it('can change the response header', async () => {
       ses.webRequest.onHeadersReceived((details, callback) => {
-        const responseHeaders = details.responseHeaders
-        responseHeaders['Custom'] = ['Changed']
+        const responseHeaders = details.responseHeaders!
+        responseHeaders['Custom'] = ['Changed'] as any
         callback({ responseHeaders: responseHeaders })
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data, status, xhr) => {
-          expect(xhr.getResponseHeader('Custom')).to.equal('Changed')
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
+      const { headers } = await ajax(defaultURL)
+      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', (done) => {
+    it('does not change header by default', async () => {
       ses.webRequest.onHeadersReceived((details, callback) => {
         callback({})
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data, status, xhr) => {
-          expect(xhr.getResponseHeader('Custom')).to.equal('Header')
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data, headers } = await ajax(defaultURL)
+      expect(headers).to.match(/^custom: Header$/m)
+      expect(data).to.equal('/')
     })
 
-    it('follows server redirect', (done) => {
+    it('follows server redirect', async () => {
       ses.webRequest.onHeadersReceived((details, callback) => {
         const responseHeaders = details.responseHeaders
         callback({ responseHeaders: responseHeaders })
       })
-      $.ajax({
-        url: defaultURL + 'serverRedirect',
-        success: (data, status, xhr) => {
-          expect(xhr.getResponseHeader('Custom')).to.equal('Header')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { headers } = await ajax(defaultURL + 'serverRedirect')
+      expect(headers).to.match(/^custom: Header$/m)
     })
 
-    it('can change the header status', (done) => {
+    it('can change the header status', async () => {
       ses.webRequest.onHeadersReceived((details, callback) => {
         const responseHeaders = details.responseHeaders
         callback({
@@ -298,14 +247,19 @@ describe('webRequest module', () => {
           statusLine: 'HTTP/1.1 404 Not Found'
         })
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data, status, xhr) => {},
-        error: (xhr, errorType) => {
-          expect(xhr.getResponseHeader('Custom')).to.equal('Header')
-          done()
+      const { headers } = await contents.executeJavaScript(`new Promise((resolve, reject) => {
+        const options = {
+          ...${JSON.stringify({url: defaultURL})},
+          success: (data, status, request) => {
+            reject(new Error('expected failure'))
+          },
+          error: (xhr) => {
+            resolve({ headers: xhr.getAllResponseHeaders() })
+          }
         }
-      })
+        $.ajax(options)
+      })`)
+      expect(headers).to.match(/^custom: Header$/m)
     })
   })
 
@@ -314,22 +268,16 @@ describe('webRequest module', () => {
       ses.webRequest.onResponseStarted(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onResponseStarted((details) => {
         expect(details.fromCache).to.be.a('boolean')
         expect(details.statusLine).to.equal('HTTP/1.1 200 OK')
         expect(details.statusCode).to.equal(200)
-        expect(details.responseHeaders['Custom']).to.deep.equal(['Header'])
-      })
-      $.ajax({
-        url: defaultURL,
-        success: (data, status, xhr) => {
-          expect(xhr.getResponseHeader('Custom')).to.equal('Header')
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
+        expect(details.responseHeaders!['Custom']).to.deep.equal(['Header'])
       })
+      const { data, headers } = await ajax(defaultURL)
+      expect(headers).to.match(/^custom: Header$/m)
+      expect(data).to.equal('/')
     })
   })
 
@@ -339,7 +287,7 @@ describe('webRequest module', () => {
       ses.webRequest.onBeforeRequest(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       const redirectURL = defaultURL + 'redirect'
       ses.webRequest.onBeforeRequest((details, callback) => {
         if (details.url === defaultURL) {
@@ -354,14 +302,8 @@ describe('webRequest module', () => {
         expect(details.statusCode).to.equal(307)
         expect(details.redirectURL).to.equal(redirectURL)
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/redirect')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/redirect')
     })
   })
 
@@ -370,20 +312,14 @@ describe('webRequest module', () => {
       ses.webRequest.onCompleted(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onCompleted((details) => {
         expect(details.fromCache).to.be.a('boolean')
         expect(details.statusLine).to.equal('HTTP/1.1 200 OK')
         expect(details.statusCode).to.equal(200)
       })
-      $.ajax({
-        url: defaultURL,
-        success: (data) => {
-          expect(data).to.equal('/')
-          done()
-        },
-        error: (xhr, errorType) => done(errorType)
-      })
+      const { data } = await ajax(defaultURL)
+      expect(data).to.equal('/')
     })
   })
 
@@ -393,18 +329,14 @@ describe('webRequest module', () => {
       ses.webRequest.onBeforeRequest(null)
     })
 
-    it('receives details object', (done) => {
+    it('receives details object', async () => {
       ses.webRequest.onBeforeRequest((details, callback) => {
         callback({ cancel: true })
       })
       ses.webRequest.onErrorOccurred((details) => {
         expect(details.error).to.equal('net::ERR_BLOCKED_BY_CLIENT')
-        done()
-      })
-      $.ajax({
-        url: defaultURL,
-        success: () => done('unexpected success')
       })
+      await expect(ajax(defaultURL)).to.eventually.be.rejectedWith('404')
     })
   })
 })

spec/fixtures/pages/jquery.html

@@ -7,7 +7,6 @@
   window.ajax = (url, options) => {
     return new Promise((resolve, reject) => {
       options.url = url
-      options.cache = false
       options.success = (data, status, request) => {
         resolve({data, status: request.status, headers: request.getAllResponseHeaders()})
       }