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

IPFS origins report wrong document.location.origin #13303

Open
da2x opened this issue Dec 27, 2020 · 15 comments
Open

IPFS origins report wrong document.location.origin #13303

da2x opened this issue Dec 27, 2020 · 15 comments
Labels
bug feature/web3/ipfs OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. security

Comments

@da2x
Copy link

da2x commented Dec 27, 2020

Description

Brave returns an unexpected document location.origin when browsing on pages through the ipfs: and ipns: protocol schemes. Other properties in the location object (e.g. .protocol) are effected too.

Steps to Reproduce

  1. Open any ipfs://hashhash or ipns://hashhash
  2. Open the developer tools console
  3. Execute document.location.origin

Expected result:

The function should return ipfs://hashhash.

Actual result:

It returns http://hashhash.ipfs.localhost:12345/

Reproduces how often:

Every time.

Brave version (brave://version info)

1.19.67

Version/Channel Information:

Beta only feature.

Miscellaneous Information:

This bug makes it more difficult to develop mixed HTTPS/IPFS websites as its more difficult to determine correctly what protocol scheme is being used. This might have unexpected security issues. The Chromium Extension API return the expected origin. However, some extensions get very confused when the tab.url and its document.location don’t match.

@diracdeltas
Copy link
Member

diracdeltas commented Dec 30, 2020

cc @yrliou @bbondy

i think it's fine to have the apparent origin be ipfs:// as long as domain isolation is preserved (#12367). ex: ipfs://foo and ipfs://bar should be treated as third parties according to the same rules as http://a.com and http://b.com.

re: what web features are accessible to an ipfs:// page, options are:

  1. treat it like remote HTTP, which means no access to web features which require a secure context (https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/features_restricted_to_secure_contexts)
  2. treat it like local HTTP, which counts as a secure context (https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy)
  3. treat it like HTTPS, which also counts as a secure context but may have some unintended side effects, such as HTTP mixed content being blocked.

i think 2 makes the most sense since IPFS documents are served locally.

@bbondy bbondy added the priority/P2 A bad problem. We might uplift this to the next planned release. label Jan 1, 2021
@bbondy bbondy added priority/P3 The next thing for us to work on. It'll ride the trains. and removed priority/P2 A bad problem. We might uplift this to the next planned release. labels Jan 13, 2021
@spylogsster spylogsster self-assigned this Feb 15, 2021
@spylogsster
Copy link

this is how it looks now:
image
@da2x @bbondy should we change host, hostname, href, protocol too?

@bbondy
Copy link
Member

bbondy commented Feb 18, 2021

I think we need protocol updated too.
@diracdeltas should hostname and host be just the CID? And href should be the ipfs path I believe

@diracdeltas
Copy link
Member

if the origin changes, href, protocol, host, and hostname (possibly other properties) should change accordingly. i think host/hostname would be the CID, href should be the full URL, and protocol should be IPFS if we decide to do this.

i'm somewhat concerned that this will be need to be a bigger change for consistency. for instance document.referrer and possibly other APIs that return URLs. this could cause some security issues if not done comprehensively, see https://bravesoftware.slack.com/archives/C0145NAJP51/p1612550288046100?thread_ts=1612304914.041400&cid=C0145NAJP51.

in https://docs.google.com/document/d/156bzOF8qkieyouqnhIQzwJsCxtsTNtyVcOiClSEgeU0/edit?disco=AAAAHwJ3OFc @lidel mentioned that this change might not have much practical value, in which case i think we should hold off on it unless it's causing major webcompat or developer issues.

@da2x
Copy link
Author

da2x commented Feb 19, 2021

You can see the expected result using the URL object: new URL('ipfs://ipfshash/some-dir/file.ext#hellohash'):

URL Object
hash: "hellohash"
host: "ipfshash"
hostname: "ipfshash"
href: "ipfs://ipfshash/some-dir/file.ext#hellohash"
origin: "ipfs://ipfshash"
password: ""
pathname: "/some-dir/file.ext"
port: ""
protocol: "ipfs:"
search: ""
searchParams: URLSearchParams {  }
username: ""

Chrome/Brave notably says the origin is null whereas Firefox correctly returns the expected ipfs://ipfshash origin.

@da2x
Copy link
Author

da2x commented Feb 19, 2021

this could cause some security issues if not done comprehensively [&] should hold off on it unless it's causing major webcompat or developer issues.

That’s the current state, though. There are all sorts of places where the double origin may cause issues and confusion for developers:

  • ipfs.localhost isn’t in the MPSL (nor is any of the built-in default IPFS–HTTP gateways!), so malicious things can more easily set cookies or access localStorage data from *.ipfs.localhost than from a random hash-based origins, which crucially, doesn’t share a second-level domain.
  • window.alert and other dialogs show that they’ve originated from a cryptic and unexpected origin the user has never seen.
  • navigate to ipns://example.eth, the document document.cookie = "test=test; domain=example.eth", but the cookie is either rejected or unavailable because it tries to set a cookie from a third-party origin (example-eth-ipns.localhost.etc).
  • A page delivered on both HTTPS and IPFS may change behavior after checking document.location.protocol. Maybe it turns off ads and says “now you’re thinking with portals!” or whatever when the protocol is ipfs:.
  • A webapp delivered over IPFS might need to do a CORS preflight with a regular HTTPS server. (For analytics, forms, or whatnot. Not everything will be P2P on day one.) The developer will assume that the origin to put in their Access-Control-Allow-Origin response header is ipns://their-cool-domain.eth and not the hidden internal one.

@bbondy
Copy link
Member

bbondy commented Feb 19, 2021

ipfs.localhost isn’t in the MPSL (nor is any of the built-in default IPFS–HTTP gateways!), so malicious things can more easily set cookies or access localStorage data from *.ipfs.localhost than from a random hash-based origins, which crucially, doesn’t share a second-level domain.

That's not true because of the work done here:
https://github.com/brave/brave-core/pull/7046/files

We're looking into a different way of handling this to help address other points though.

@spylogsster spylogsster removed their assignment Feb 19, 2021
@bridiver
Copy link
Contributor

bridiver commented Jun 29, 2021

I'm not sure this is an issue we want to fix. The problem here that we also ran into in another issue is that hostnames are not case sensitive, but CID is. Right now ipfs/ipns are effectively treated like file urls and that preserves the case. I was originally in favor of making a change like this for other reasons, but I'm no longer sure it's a good idea. There are too many different places where the case is handled for host/origin and I don't see a safe and reliable way to make sure it's consistent across the codebase.

@bridiver
Copy link
Contributor

bridiver commented Jun 29, 2021

I guess to be completely accurate DNS names are case insensitive, but in chromium that has translated to all hostnames being converted to lowercase and but there are multiple places this happens and not all of them provide access to the scheme to differentiate between ipfs and http/https

@bridiver
Copy link
Contributor

and really since ipfs is "InterPlanetary File System", maybe it makes more sense to treat these like file urls except maybe for dnslink. It would have been much simpler if dnslink used a different scheme...

@bridiver
Copy link
Contributor

bridiver commented Jun 29, 2021

having said all of that, http://hashhash.ipfs.localhost:12345/ is not what we want to return, but I think the expected result for document.location.origin should be an empty string or whatever it returns for a file url

@lidel
Copy link

lidel commented Jun 29, 2021

Some notes that may be useful when it is time to address this:

  • Every CID can be converted to CIDv0 in base-insensitive encoding, such as Base32
  • CIDv1 in Vase will become default at some point anyway
  • It may be easier to detect CIDv0 in Base58 and upgrade to CIDv1 before processing further, than make URL origin/authority parsing case-insensitive across the codebase

@bridiver
Copy link
Contributor

If every cid can be converted to case insensitive encoding then this would be more viable. @lidel I want to verify that you're saying CIDv1 is already case insensitive?

@MicahZoltu
Copy link

If every cid can be converted to case insensitive encoding then this would be more viable. @lidel I want to verify that you're saying CIDv1 is already case insensitive?

Yes. One of the primary purposes of CIDv1, and the reason there is an effort to deprecate CIDv0 in favor of CIDv1, is specifically that it is "all lowercase" so it can be used as a subdomain rather than being constrained to a path (which enables subdomain isolation between sites retrieved from a gateway).

@lidel
Copy link

lidel commented Mar 4, 2024

Yes, every CID can be turned into CIDv1 in base32 or base36.

Technically, one can encode CIDv1 using different multibase, such as case-sensitive base64:

$ ipfs cid format -b base64url bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi
uAXASIMPEcz7Ir_0Gz56f9Q_8a80uyFphcABLtwlmnDHelDka

But on the web, the default convention is to use base32 for /ipfs/ and base36 for /ipns/. This ensures CID is case-insensitive but also short enough to fit in a single DNS label (which has max length limit of 63), allowing use on subdomain gateways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/web3/ipfs OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants