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: enforce origin isolation on subdomain gws #60

Merged
merged 10 commits into from
Feb 28, 2024

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 28, 2024

Description

Towards #30: this PR detects when a path gateway URL is loaded on host that supports subdomain gateway URLs, and redirects to unique origin.

To test:

  1. Open http://helia-sw-gateway.localhost:3000 to register SW
  2. Open http://helia-sw-gateway.localhost:3000/ipns/cid.ipfs.tech
  3. Page should redirect to http://cid-ipfs-tech.ipns.helia-sw-gateway.localhost:3000

Caveats:

  • in Brave this works only with helia-sw-gateway.localhost, they seem to block localhost, so use helia-sw-gateway.localhost for testing
  • check depends on CORS

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@lidel lidel force-pushed the origin-isolation-enforcement branch from 700ddbd to 1b09048 Compare February 28, 2024 03:05
Comment on lines +38 to +47
const isSubdomainIsolationSupported = async (location: Pick<Location, 'protocol' | 'host' | 'pathname'>): Promise<boolean> => {
// TODO: do this test once and expose it as cookie / config flag somehow
const testUrl = `${location.protocol}//bafkqaaa.ipfs.${location.host}`
try {
const response: Response = await fetch(testUrl)
return response.status === 200
} catch (_) {
return false
}
}
Copy link
Member Author

@lidel lidel Feb 28, 2024

Choose a reason for hiding this comment

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

💭 this is not a big deal because it only happens once when on /ipfs/cid paths, is not executed when on subdomain host, but perhaps we could leverage config passing setup based on iframe introduced in #24

Comment on lines +47 to 48
// TODO: why we need this origin here? where is targetOrigin used?
const targetOrigin = decodeURIComponent(window.location.search.split('origin=')[1])
Copy link
Member Author

Choose a reason for hiding this comment

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

@SgtPooki is this something we want to keep, or can it be removed?
iirc if we don't have query param, we would reuse cache, making subdomain page loads faster

Copy link
Member

@SgtPooki SgtPooki Feb 28, 2024

Choose a reason for hiding this comment

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

This is only for loading config in iframe. Iframe needs to know parent origin to postmessage.

Target origin (main window) is subdomain, iframe is root domain.

Copy link
Member

Choose a reason for hiding this comment

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

So if we can do that another way, we could remove this

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we use #hash instead? It does not get sent to server, but JS in inframe should see it.

@lidel
Copy link
Member Author

lidel commented Feb 28, 2024

Merging this PR will force redirect to subdomains when available and liberal CORS headers are present. This includes inbrowser.dev.

Given how sensitive the lack of isolation is for websites atm, I'm thinking about merging this as-is, to secure inbrowser.dev, and adding regression tests and optimizations in follow-up PRs. We should not block critical security fix on these.

Ok to proceed @SgtPooki @aschmahmann ?

@lidel lidel marked this pull request as ready for review February 28, 2024 15:11
return null
}

const isSubdomainIsolationSupported = async (location: Pick<Location, 'protocol' | 'host' | 'pathname'>): Promise<boolean> => {
Copy link
Member

Choose a reason for hiding this comment

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

its worth adding a comment here saying that for our SW usecase, we're only worried about checking for a 200 response and no TLS error.

// IPNS Names are represented as Base36 CIDv1 with libp2p-key codec
// https://specs.ipfs.tech/ipns/ipns-record/#ipns-name
// eslint-disable-next-line no-case-declarations
const ipnsName = CID.parse(id).toV1()
Copy link
Member

Choose a reason for hiding this comment

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

why are we CID.parse ing a dnslink/peerid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid CIDv1 in base32 will be also a valid DNS name.
So we parse string as CID first, and ONLY assume DNS if CID parsing failed.

If we did it the other way, ISP or DNS operator could return spoofed DNS name that is also a valid CID, and do MITM on users who only use cryptographic identifies.

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

a few comments / nits, but overall fine with getting this in

src/redirectPage.tsx Outdated Show resolved Hide resolved
src/redirectPage.tsx Outdated Show resolved Hide resolved
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
@SgtPooki
Copy link
Member

redirect to /wiki for http://helia-sw-gateway.localhost/ipns/en.wikipedia-on-ipfs.org doesn't work, but other than that, this seems to work well

@SgtPooki SgtPooki merged commit 3071332 into main Feb 28, 2024
14 checks passed
@SgtPooki SgtPooki deleted the origin-isolation-enforcement branch February 28, 2024 19:47
@lidel lidel mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants