Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Feb 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0d765a5
fix(recovery): :bug: No recovery for URLs being redirected.
whizzzkid Feb 22, 2023
6796c9d
fix(recovery): :test_tube: test added.
whizzzkid Feb 22, 2023
91a9567
fix(recovery): :memo: Adding comments
whizzzkid Feb 22, 2023
74cb934
fix(recovery): :rotating_light: linter warnings
whizzzkid Feb 22, 2023
78ce18d
fix: false-positives in sameGateway
lidel Feb 22, 2023
5fd1816
test: ensure recovery does not depend on redirects
lidel Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion add-on/src/lib/ipfs-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@lidel lidel Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1163 (comment)

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
Copy link
Member

@lidel lidel Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marked this conversation as resolved.
Show resolved Hide resolved
const publicUri = ipfsPathValidator.resolveToPublicUrl(request.url, state.pubGwURLString)
return { redirectUrl: `${dropSlash(runtimeRoot)}${recoveryPagePath}#${encodeURIComponent(publicUri)}` }
}
Expand Down
7 changes: 7 additions & 0 deletions test/functional/lib/ipfs-request-gateway-redirect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@lidel lidel Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
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 () {
Copy link
Member

@lidel lidel Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 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

state.redirect = true
expect(state.nodeActive).to.be.equal(false)
const request = url2request('https://localhost:8080/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR/foo/bar')
expect(modifyRequest.onBeforeRequest(request)).to.equal(undefined)
})
})

after(function () {
Expand Down