fix: remove XHR CORS workaround for Firefox (#771)

This change removes workaround introduced in PR #494 and restores redirect in onBeforeRequest. The original bug was fixed in Firefox 69, that is why we also bump minimal version. More info at: #436 (comment)
ipfs · Oct 1, 2019 · 40b7ab7 · 40b7ab7
1 parent c4f1707
commit 40b7ab7
Show file tree

Hide file tree

Showing 6 changed files with 15 additions and 74 deletions.
diff --git a/.babelrc b/.babelrc
@@ -7,7 +7,7 @@
       "@babel/preset-env",
       {
         "targets": {
-          "firefox": 67,
+          "firefox": 69,
           "chrome": 70
         }
       }

diff --git a/README.md b/README.md
@@ -226,13 +226,6 @@ We are also available at the [#ipfs](https://webchat.freenode.net/?channels=ipfs
 
 Questions specific to this browser companion can be asked directly at [`#ipfs-in-web-browsers`](https://webchat.freenode.net/?channels=ipfs-in-web-browsers)
 
-#### Cross-Origin XHR are executed "twice" in Firefox
-
-Due to CORS bug XHRs in Firefox are handled via late redirects in `onHeadersReceived`.
-Original request is cancelled after response headers are received, so there is no overhead of reading response payload twice.
-
-For more details on this see [PR #511](https://github.com/ipfs-shipyard/ipfs-companion/pull/511).
-
 #### Upload via Right-Click Does Not Work in Firefox
 
 See [this workaround](https://github.com/ipfs/ipfs-companion/issues/227).

diff --git a/add-on/manifest.firefox.json b/add-on/manifest.firefox.json
@@ -8,7 +8,7 @@
   "applications": {
     "gecko": {
       "id": "ipfs-firefox-addon@lidel.org",
-      "strict_min_version": "67.0"
+      "strict_min_version": "69.0"
     }
   },
   "page_action": {

diff --git a/add-on/src/lib/ipfs-request.js b/add-on/src/lib/ipfs-request.js
@@ -20,9 +20,6 @@ const recoverableErrors = new Set([
   'net::ERR_INTERNET_DISCONNECTED' // no network
 ])
 
-// Tracking late redirects for edge cases such as https://github.com/ipfs-shipyard/ipfs-companion/issues/436
-const onHeadersReceivedRedirect = new Set()
-
 // Request modifier provides event listeners for the various stages of making an HTTP request
 // API Details: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest
 function createRequestModifier (getState, dnslinkResolver, ipfsPathValidator, runtime) {
@@ -316,19 +313,6 @@ function createRequestModifier (getState, dnslinkResolver, ipfsPathValidator, ru
       }
 
       if (state.redirect) {
-        // Late redirect as a workaround for edge cases such as:
-        // - CORS XHR in Firefox: https://github.com/ipfs-shipyard/ipfs-companion/issues/436
-        if (onHeadersReceivedRedirect.has(request.requestId)) {
-          onHeadersReceivedRedirect.delete(request.requestId)
-          if (state.dnslinkPolicy) {
-            const dnslinkRedirect = dnslinkResolver.dnslinkRedirect(request.url)
-            if (dnslinkRedirect) {
-              return dnslinkRedirect
-            }
-          }
-          return redirectToGateway(request.url, state, ipfsPathValidator)
-        }
-
         // Detect X-Ipfs-Path Header and upgrade transport to IPFS:
         // 1. Check if DNSLink exists and redirect to it.
         // 2. If there is no DNSLink, validate path from the header and redirect
@@ -417,19 +401,13 @@ function createRequestModifier (getState, dnslinkResolver, ipfsPathValidator, ru
           })
         }
       }
-
-      // Cleanup after https://github.com/ipfs-shipyard/ipfs-companion/issues/436
-      if (onHeadersReceivedRedirect.has(request.requestId)) {
-        onHeadersReceivedRedirect.delete(request.requestId)
-      }
     }
 
   }
 }
 
 exports.redirectOptOutHint = redirectOptOutHint
 exports.createRequestModifier = createRequestModifier
-exports.onHeadersReceivedRedirect = onHeadersReceivedRedirect
 
 function redirectToGateway (requestUrl, state, ipfsPathValidator) {
   // TODO: redirect to `ipfs://` if hasNativeProtocolHandler === true
@@ -450,21 +428,6 @@ function isSafeToRedirect (request, runtime) {
     return false
   }
 
-  // Ignore XHR requests for which redirect would fail due to CORS bug in Firefox
-  // See: https://github.com/ipfs-shipyard/ipfs-companion/issues/436
-  // TODO: revisit when upstream bug is addressed
-  if (runtime.isFirefox && request.type === 'xmlhttprequest' && !request.responseHeaders) {
-    // Sidenote on XHR Origin: Firefox 60 uses request.originUrl, Chrome 63 uses request.initiator
-    if (request.originUrl) {
-      const sourceOrigin = new URL(request.originUrl).origin
-      const targetOrigin = new URL(request.url).origin
-      if (sourceOrigin !== targetOrigin) {
-        log('Delaying redirect of CORS XHR until onHeadersReceived due to https://github.com/ipfs-shipyard/ipfs-companion/issues/436 :', request.url)
-        onHeadersReceivedRedirect.add(request.requestId)
-        return false
-      }
-    }
-  }
   return true
 }
 

diff --git a/test/functional/lib/ipfs-request-dnslink.test.js b/test/functional/lib/ipfs-request-dnslink.test.js
@@ -163,7 +163,7 @@ describe('modifyRequest processing', function () {
       const request = url2request('http://explore.ipld.io/index.html?argTest#hashTest')
       expect(modifyRequest.onBeforeRequest(request).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
     })
-    it('should redirect in onBeforeRequest if DNS TXT record exists, XHR is cross-origin and runtime is not Firefox', function () {
+    it('should redirect in onBeforeRequest if DNS TXT record exists, XHR is cross-origin and runtime is Chromium', function () {
       // stub existence of a valid DNS record
       const fqdn = 'explore.ipld.io'
       dnslinkResolver.readDnslinkFromTxtRecord = sinon.stub().withArgs(fqdn).returns('/ipfs/QmbfimSwTuCvGL8XBr3yk1iCjqgk2co2n21cWmcQohymDd')
@@ -173,19 +173,15 @@ describe('modifyRequest processing', function () {
       const xhrRequest = { url: 'http://explore.ipld.io/index.html?argTest#hashTest', type: 'xmlhttprequest', initiator: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
       expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
     })
-    it('should redirect later in onHeadersReceived if dnslink exists, XHR is cross-origin and runtime is Firefox', function () {
+    it('should redirect in onBeforeRequest if dnslink exists, XHR is cross-origin and runtime is Firefox', function () {
       // stub existence of a valid DNS record
       const fqdn = 'explore.ipld.io'
       dnslinkResolver.readDnslinkFromTxtRecord = sinon.stub().withArgs(fqdn).returns('/ipfs/QmbfimSwTuCvGL8XBr3yk1iCjqgk2co2n21cWmcQohymDd')
       //
-      // Context for CORS XHR problems in Firefox: https://github.com/ipfs-shipyard/ipfs-companion/issues/436
       runtime.isFirefox = true
       // Firefox uses 'originUrl' for origin
       const xhrRequest = { url: 'http://explore.ipld.io/index.html?argTest#hashTest', type: 'xmlhttprequest', originUrl: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
-      // onBeforeRequest should not change anything, as it will trigger false-positive CORS error
-      expect(modifyRequest.onBeforeRequest(xhrRequest)).to.equal(undefined)
-      // onHeadersReceived is after CORS validation happens, so its ok to cancel and redirect late
-      expect(modifyRequest.onHeadersReceived(xhrRequest).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
+      expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
     })
     it('should do nothing if dnslink does not exist and XHR is cross-origin in Firefox', function () {
       // stub no dnslink
@@ -325,18 +321,15 @@ describe('modifyRequest processing', function () {
           const xhrRequest = { url: 'http://explore.ipld.io/index.html?argTest#hashTest', type: 'xmlhttprequest', initiator: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
           expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
         })
-        it('should redirect later in onHeadersReceived if XHR is cross-origin and runtime is Firefox', function () {
+        it('should redirect in onBeforeRequest if XHR is cross-origin and runtime is Firefox', function () {
           // stub existence of a valid DNS record
           const fqdn = 'explore.ipld.io'
           dnslinkResolver.setDnslink(fqdn, '/ipfs/QmbfimSwTuCvGL8XBr3yk1iCjqgk2co2n21cWmcQohymDd')
           //
           // Context for CORS XHR problems in Firefox: https://github.com/ipfs-shipyard/ipfs-companion/issues/436
           runtime.isFirefox = true
           const xhrRequest = { url: 'http://explore.ipld.io/index.html?argTest#hashTest', type: 'xmlhttprequest', originUrl: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
-          // onBeforeRequest should not change anything, as it will trigger false-positive CORS error
-          expect(modifyRequest.onBeforeRequest(xhrRequest)).to.equal(undefined)
-          // onHeadersReceived is after CORS validation happens, so its ok to cancel and redirect late
-          expect(modifyRequest.onHeadersReceived(xhrRequest).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
+          expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal(activeGateway + '/ipns/explore.ipld.io/index.html?argTest#hashTest')
         })
         it('should do nothing if DNS TXT record is missing and XHR is cross-origin in Firefox', function () {
           // stub cached info about lack of dnslink

diff --git a/test/functional/lib/ipfs-request-gateway-redirect.test.js b/test/functional/lib/ipfs-request-gateway-redirect.test.js
@@ -119,24 +119,20 @@ describe('modifyRequest.onBeforeRequest:', function () {
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', originUrl: 'https://google.com/' }
         expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('http://127.0.0.1:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
-      it('should be served from custom gateway if fetched from the same origin and redirect is enabled in non-Firefox', function () {
+      it('should be served from custom gateway if fetched from the same origin and redirect is enabled in Chromium', function () {
         runtime.isFirefox = false
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', initiator: 'https://google.com/' }
         expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('http://127.0.0.1:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
-      it('should be served from custom gateway if XHR is cross-origin and redirect is enabled in non-Firefox', function () {
+      it('should be served from custom gateway if XHR is cross-origin and redirect is enabled in Chromium', function () {
         runtime.isFirefox = false
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', initiator: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
         expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('http://127.0.0.1:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
-      it('should be served from custom gateway via late redirect in onHeadersReceived if XHR is cross-origin and redirect is enabled in Firefox', function () {
-        // Context for CORS XHR problems in Firefox: https://github.com/ipfs-shipyard/ipfs-companion/issues/436
+      it('should be served from custom gateway if XHR is cross-origin and redirect is enabled in Firefox', function () {
         runtime.isFirefox = true
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', originUrl: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
-        // onBeforeRequest should not change anything, as it will trigger false-positive CORS error
-        expect(modifyRequest.onBeforeRequest(xhrRequest)).to.equal(undefined)
-        // onHeadersReceived is after CORS validation happens, so its ok to cancel and redirect late
-        expect(modifyRequest.onHeadersReceived(xhrRequest).redirectUrl).to.equal('http://127.0.0.1:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
+        expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('http://127.0.0.1:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
     })
     describe('with embedded node', function () {
@@ -148,24 +144,20 @@ describe('modifyRequest.onBeforeRequest:', function () {
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', originUrl: 'https://google.com/' }
         expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('https://ipfs.io/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
-      it('should be served from public gateway if fetched from the same origin and redirect is enabled in non-Firefox', function () {
+      it('should be served from public gateway if fetched from the same origin and redirect is enabled in Chromium', function () {
         runtime.isFirefox = false
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', initiator: 'https://google.com/' }
         expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('https://ipfs.io/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
-      it('should be served from public gateway if XHR is cross-origin and redirect is enabled in non-Firefox', function () {
+      it('should be served from public gateway if XHR is cross-origin and redirect is enabled in Chromium', function () {
         runtime.isFirefox = false
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', initiator: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
         expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('https://ipfs.io/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
-      it('should be served from public gateway via late redirect in onHeadersReceived if XHR is cross-origin and redirect is enabled in Firefox', function () {
-        // Context for CORS XHR problems in Firefox: https://github.com/ipfs-shipyard/ipfs-companion/issues/436
+      it('should be served from public gateway if XHR is cross-origin and redirect is enabled in Firefox', function () {
         runtime.isFirefox = true
         const xhrRequest = { url: 'https://google.com/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest', type: 'xmlhttprequest', originUrl: 'https://www.nasa.gov/foo.html', requestId: fakeRequestId() }
-        // onBeforeRequest should not change anything, as it will trigger false-positive CORS error
-        expect(modifyRequest.onBeforeRequest(xhrRequest)).to.equal(undefined)
-        // onHeadersReceived is after CORS validation happens, so its ok to cancel and redirect late
-        expect(modifyRequest.onHeadersReceived(xhrRequest).redirectUrl).to.equal('https://ipfs.io/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
+        expect(modifyRequest.onBeforeRequest(xhrRequest).redirectUrl).to.equal('https://ipfs.io/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest')
       })
     })
   })