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.

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

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

Merged
merged 6 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions add-on/src/lib/ipfs-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,11 @@ export function sameGateway (url, gwUrl) {
url.hostname = '127.0.0.1'
}

// Additional check to avoid false-positives when user has some unrelated HTTP server running on localhost:8080
// It is not "sameGateway" if "localhost" URL does not look like Gateway or RPC URL.
// This removes surface for bugs like https://github.com/ipfs/ipfs-companion/issues/1162
if (!(isIPFS.url(url.toString()) || isIPFS.subdomain(url.toString()) || url.pathname.startsWith('/api/v0/') || url.pathname.startsWith('/webui'))) return false

const gws = [gwUrl.host]

// localhost gateway has more than one hostname
Expand Down
16 changes: 14 additions & 2 deletions test/functional/lib/ipfs-path.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,23 @@ describe('ipfs-path.js', function () {
const gw = 'http://localhost:8080'
expect(sameGateway(url, gw)).to.equal(true)
})
it('should return true on 127.0.0.1/0.0.0.0 host match', function () {
it('should return true on localhost subdomain host match', function () {
const url = 'http://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.localhost:8080/foo/bar'
Comment on lines +187 to +188
Copy link
Member

Choose a reason for hiding this comment

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

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

const gw = 'http://localhost:8080'
expect(sameGateway(url, gw)).to.equal(true)
})
it('should return true on RPC webui 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/webui'
const api = 'http://127.0.0.1:5001'
expect(sameGateway(url, api)).to.equal(true)
})
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'
Comment on lines +198 to +201
Copy link
Member

Choose a reason for hiding this comment

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

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

expect(sameGateway(url, api)).to.equal(true)
})
it('should return false on hostname match but different port', function () {
const url = 'https://localhost:8081/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR/foo/bar'
const gw = 'http://localhost:8080'
Expand Down Expand Up @@ -289,7 +301,7 @@ describe('ipfs-path.js', function () {
expect(ipfsPathValidator.isRedirectPageActionsContext(url)).to.equal(true)
})
it('should return false for /ipfs/ URL at Local Gateway', function () {
const url = `${state.gwURL}/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest`
const url = `${state.gwURL}ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR?argTest#hashTest`
expect(ipfsPathValidator.isRedirectPageActionsContext(url)).to.equal(false)
})
it('should return false for IPFS content loaded from IPFS API port', function () {
Expand Down
31 changes: 26 additions & 5 deletions test/functional/lib/ipfs-request-gateway-redirect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ describe('modifyRequest.onBeforeRequest:', function () {
describe('request for IPFS path at the localhost', function () {
// we do not touch local requests, as it may interfere with other nodes running at the same machine
// or could produce false-positives such as redirection from localhost:5001/ipfs/path to localhost:8080/ipfs/path
it('should fix localhost API hostname to IP', function () {
it('should fix localhost Kubo RPC hostname to IP', function () {
const request = url2request('http://localhost:5001/ipfs/QmPhnvn747LqwPYMJmQVorMaGbMSgA7mRRoyyZYz3DoZRQ/')
// expectNoRedirect(modifyRequest, request)
expect(modifyRequest.onBeforeRequest(request).redirectUrl)
Expand All @@ -375,14 +375,14 @@ describe('modifyRequest.onBeforeRequest:', function () {
expect(modifyRequest.onBeforeRequest(request).redirectUrl)
.to.equal('http://127.0.0.1:5001/ipfs/QmPhnvn747LqwPYMJmQVorMaGbMSgA7mRRoyyZYz3DoZRQ/')
})
it('should fix localhost API to IP', function () {
it('should be left untouched if /webui on localhost Kubo RPC port', function () {
// https://github.com/ipfs/ipfs-companion/issues/291
const request = url2request('http://localhost:5001/webui')
// expectNoRedirect(modifyRequest, request)
expect(modifyRequest.onBeforeRequest(request).redirectUrl)
.to.equal('http://127.0.0.1:5001/webui')
})
it('should be left untouched if localhost API IP is used, even when x-ipfs-path is present', function () {
it('should be left untouched if localhost Kubo RPC IP is used, even when x-ipfs-path is present', function () {
// https://github.com/ipfs-shipyard/ipfs-companion/issues/604
const request = url2request('http://127.0.0.1:5001/ipfs/QmPhnvn747LqwPYMJmQVorMaGbMSgA7mRRoyyZYz3DoZRQ/')
request.responseHeaders = [{ name: 'X-Ipfs-Path', value: '/ipfs/QmPhnvn747LqwPYMJmQVorMaGbMSgA7mRRoyyZYz3DDIFF' }]
Expand Down Expand Up @@ -431,17 +431,38 @@ describe('modifyRequest.onBeforeRequest:', function () {
global.browser = browser
state.ipfsNodeType = 'external'
state.redirect = true
state.peerCount = -1
state.peerCount = -1 // this simulates Kubo RPC being offline
state.gwURLString = 'http://localhost:8080'
state.gwURL = new URL('http://localhost:8080')
state.pubGwURLString = 'https://ipfs.io'
state.pubGwURL = new URL('https://ipfs.io')
})
it('should present recovery page if node is offline', function () {
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)
})
Comment on lines +440 to +465
Copy link
Member

@lidel lidel Feb 22, 2023

Choose a reason for hiding this comment

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

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

})

after(function () {
Expand Down