fix(recovery): 🐛 false-positive for non-gateway URLs #1163

whizzzkid · 2023-02-22T09:04:46Z

Closes: #1162

This is a case I should've thought of when working on: #1125. The reason this happens:

When Automatic Mode is turned off, the redirect actually happens before the recovery mode begins.
Since the redirect happens to local node, the recovery mode gets triggered, asking to redirect to public gateway.
The public gateway redirects back to local and results in the endless loop.

Fix: ~~Add check for !state.redirect~~ remove false-positives from sameGateway check.

I would like to merge #1160 and #1161 before this and create a patch release.

add-on/src/lib/ipfs-request.js

lidel

The problem with Companion is that you need to think with portals ;-)

The !state.redirect hack was a band-aid, and was not fixing problem from https://twitter.com/unicomp21/status/1626244123102679041 at all.

We can improve sameGateway check and clearly detect when someone opens URL equal to the Gateway http://localhost:8080/ – I'll push fix shortly.

add-on/src/lib/ipfs-request.js

lidel · 2023-02-22T13:26:01Z

test/functional/lib/ipfs-request-gateway-redirect.test.js

@@ -436,12 +436,19 @@ describe('modifyRequest.onBeforeRequest:', function () {
      state.gwURL = new URL('http://localhost:8080')
      state.pubGwURLString = 'https://ipfs.io'
      state.pubGwURL = new URL('https://ipfs.io')
+      state.redirect = false


💭 It was defined twice in this beforeEach block. I've added more tests and set it explicitly in relevant it blocks.

lidel · 2023-02-22T13:30:52Z

test/functional/lib/ipfs-request-gateway-redirect.test.js

    })
    it('should present recovery page if node is offline', function () {
      expect(state.nodeActive).to.be.equal(false)
      const request = url2request('https://localhost:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR/foo/bar')
      expect(modifyRequest.onBeforeRequest(request).redirectUrl).to.equal('chrome-extension://testid/dist/recovery/recovery.html#https%3A%2F%2Fipfs.io%2Fipfs%2FQmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR%2Ffoo%2Fbar')
    })
+    it('should not block requests when the request is redirecting', function () {


💭 We need to be very clear what we are testing and why. 12 months from now, none of us will remember why we've added this – I've added below

+ it('should not show recovery page if node is offline, redirect is enabled, but non-gateway URL failed to load from the same port', function () { + // this covers https://github.com/ipfs/ipfs-companion/issues/1162 and https://twitter.com/unicomp21/status/1626244123102679041

lidel · 2023-02-22T13:58:42Z

add-on/src/lib/ipfs-request.js

+      if (!state.nodeActive && //                  node is not active
+        !state.redirect && //                      this is not a redirect request
+        request.type === 'main_frame' && //        this is a request for a root document
+        sameGateway(request.url, state.gwURL) //   this is a request to the local gateway


ℹ️ this was not enough to fix https://twitter.com/unicomp21/status/1626244123102679041 – this screen should never happen and user should not have to disable companion for this to go away.

sameGateway needed to be improved to not return false-positives for URLs that do not look like gateway or RPC requests.

Thanks for pointing this out, I was looking into the state and believed I saw redirect being set to true for all calls being redirected to local node, and redirect was set to false when the requests were not being redirected.

Is that wrong?

state is global, not per request. it informs redirect logic what are current settings and state of RPC.
(bit of a legacy thing, from the XUL time (before v2) when it was less expensive to cache options in one place, but it is still useful when writing tests, to simulate specific state of world).

you probably saw us setting it in tests per different requests to simultate specific flows, and assumed it is per request, but it is is not :)

lidel · 2023-02-22T14:36:29Z

add-on/src/lib/ipfs-request.js

@@ -144,7 +144,11 @@ export function createRequestModifier (getState, dnslinkResolver, ipfsPathValida

      // When local IPFS node is unreachable , show recovery page where user can redirect
      // to public gateway.
-      if (!state.nodeActive && request.type === 'main_frame' && sameGateway(request.url, state.gwURL)) {
+      if (!state.nodeActive && //                  node is not active
+        !state.redirect && //                      this is not a redirect request


💭 nit: this is a global flag that controls if companion is redirecting requests to the local gateway, or not.
ℹ️ This was not fixing the problem, and the comment was only confusing the reader – I've restored old version and applied proper fix in sameGateway.

#1163 (comment)

This ensures sameGateway returns true only if the tested URL is either a valid path, subdomain, or RPC URL related to local Gateway or Kubo instance. Everything else should be ignored, because people may run Kubo Gateway on localhost:8080, and then stop it, and then run some unrelated HTTP server on the same port. This is unfortunate papercut due to the 8080 being very very popular default port for all things HTTP. Luckily, Companion has enough information to make this right.

lidel

@whizzzkid pushed alternative fix for sameGateway check and some tests.
This approach should be more reliable.

lidel · 2023-02-22T15:02:43Z

test/functional/lib/ipfs-path.test.js

+    it('should return true on localhost subdomain host match', function () {
+      const url = 'http://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.localhost:8080/foo/bar'


ℹ️ we were missing subdomain test, so I've added it

lidel · 2023-02-22T15:03:29Z

test/functional/lib/ipfs-path.test.js

+    it('should return true on RPC /api/v0 path 127.0.0.1/0.0.0.0 host match', function () {
+      // this is misconfiguration, but handling it in sameGateway ensures users who do this dont waste our time debugging
+      const url = 'http://0.0.0.0:5001/api/v0/id'
+      const api = 'http://127.0.0.1:5001'


ℹ️ this saves us a day of debugging when user puts wrong port in wrong field in their config, and then tweets about it ;-)

lidel · 2023-02-22T15:07:12Z

test/functional/lib/ipfs-request-gateway-redirect.test.js

+    it('should present recovery page if node is offline and redirect is enabled', function () {
      expect(state.nodeActive).to.be.equal(false)
+      state.redirect = true
+      const request = url2request('https://localhost:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR/foo/bar')
+      expect(modifyRequest.onBeforeRequest(request).redirectUrl).to.equal('chrome-extension://testid/dist/recovery/recovery.html#https%3A%2F%2Fipfs.io%2Fipfs%2FQmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR%2Ffoo%2Fbar')
+    })
+    it('should present recovery page if node is offline and redirect is disabled', function () {
+      expect(state.nodeActive).to.be.equal(false)
+      state.redirect = false
      const request = url2request('https://localhost:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR/foo/bar')
      expect(modifyRequest.onBeforeRequest(request).redirectUrl).to.equal('chrome-extension://testid/dist/recovery/recovery.html#https%3A%2F%2Fipfs.io%2Fipfs%2FQmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR%2Ffoo%2Fbar')
    })
+    it('should not show recovery page if node is offline, redirect is enabled, but non-gateway URL failed to load from the same port', function () {
+      // this covers https://github.com/ipfs/ipfs-companion/issues/1162 and https://twitter.com/unicomp21/status/1626244123102679041
+      state.redirect = true
+      expect(state.nodeActive).to.be.equal(false)
+      const request = url2request('https://localhost:8080/')
+      expect(modifyRequest.onBeforeRequest(request)).to.equal(undefined)
+    })
+    it('should not show recovery page if extension is disabled', function () {
+      // allows user to quickly avoid anything similar to https://github.com/ipfs/ipfs-companion/issues/1162
+      state.active = false
+      expect(state.nodeActive).to.be.equal(false)
+      const request = url2request('https://localhost:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR/foo/bar')
+      expect(modifyRequest.onBeforeRequest(request)).to.equal(undefined)
+    })


ℹ️ I think this covers most prominent edge cases, and ensures recovery logic does not depend on redirect state (which was why it was initially difficult to reproduce the bug from #1162)

fix(recovery): 🐛 No recovery for URLs being redirected.

0d765a5

whizzzkid requested review from lidel and a team as code owners February 22, 2023 09:04

whizzzkid requested a review from SgtPooki February 22, 2023 09:08

autonome reviewed Feb 22, 2023

View reviewed changes

add-on/src/lib/ipfs-request.js Outdated Show resolved Hide resolved

whizzzkid added 3 commits February 22, 2023 03:27

fix(recovery): 🧪 test added.

6796c9d

fix(recovery): 📝 Adding comments

91a9567

fix(recovery): 🚨 linter warnings

74cb934

whizzzkid requested a review from autonome February 22, 2023 10:38

lidel requested changes Feb 22, 2023

View reviewed changes

lidel force-pushed the fix/recovery branch from 8670299 to 78ce18d Compare February 22, 2023 15:00

lidel changed the title ~~fix(recovery): 🐛 No recovery for URLs being redirected.~~ fix(recovery): 🐛 false-positive for non-gateway URLs Feb 22, 2023

test: ensure recovery does not depend on redirects

5fd1816

lidel approved these changes Feb 22, 2023

View reviewed changes

whizzzkid merged commit 0ee35d2 into main Feb 22, 2023

whizzzkid deleted the fix/recovery branch February 22, 2023 18:53

github-actions bot mentioned this pull request Feb 22, 2023

chore(main): release 2.22.1 #1166

Merged

github-actions bot mentioned this pull request Sep 21, 2023

chore(main): release 3.0.1 #1288

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(recovery): 🐛 false-positive for non-gateway URLs #1163

fix(recovery): 🐛 false-positive for non-gateway URLs #1163

whizzzkid commented Feb 22, 2023 •

edited by lidel

lidel left a comment •

edited

lidel Feb 22, 2023 •

edited

lidel Feb 22, 2023 •

edited

lidel Feb 22, 2023 •

edited

whizzzkid Feb 22, 2023

lidel Feb 22, 2023

lidel Feb 22, 2023 •

edited

whizzzkid Feb 22, 2023

lidel left a comment

lidel Feb 22, 2023

lidel Feb 22, 2023

lidel Feb 22, 2023 •

edited

		it('should return true on localhost subdomain host match', function () {
		const url = 'http://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.localhost:8080/foo/bar'

fix(recovery): 🐛 false-positive for non-gateway URLs #1163

fix(recovery): 🐛 false-positive for non-gateway URLs #1163

Conversation

whizzzkid commented Feb 22, 2023 • edited by lidel

lidel left a comment • edited

Choose a reason for hiding this comment

lidel Feb 22, 2023 • edited

Choose a reason for hiding this comment

lidel Feb 22, 2023 • edited

Choose a reason for hiding this comment

lidel Feb 22, 2023 • edited

Choose a reason for hiding this comment

whizzzkid Feb 22, 2023

Choose a reason for hiding this comment

lidel Feb 22, 2023

Choose a reason for hiding this comment

lidel Feb 22, 2023 • edited

Choose a reason for hiding this comment

whizzzkid Feb 22, 2023

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

lidel Feb 22, 2023

Choose a reason for hiding this comment

lidel Feb 22, 2023

Choose a reason for hiding this comment

lidel Feb 22, 2023 • edited

Choose a reason for hiding this comment

whizzzkid commented Feb 22, 2023 •

edited by lidel

lidel left a comment •

edited

lidel Feb 22, 2023 •

edited

lidel Feb 22, 2023 •

edited

lidel Feb 22, 2023 •

edited

lidel Feb 22, 2023 •

edited

lidel Feb 22, 2023 •

edited