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

allowInsecure: false should respect Secure Contexts #564

Closed
lidel opened this issue Jun 19, 2024 · 2 comments · Fixed by #579
Closed

allowInsecure: false should respect Secure Contexts #564

lidel opened this issue Jun 19, 2024 · 2 comments · Fixed by #579
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@lidel
Copy link
Member

lidel commented Jun 19, 2024

Problem

Setting allowInsecure: false should not be blindly blocking http://, it should not block requests to http://localhost and http://*.localhost because these are valid Secure Contexts.

This bug blocks users from using their own local gateway (ipfs desktop, kubo, rainbow).

Solution

Correctly recognize http://localhost[:port] and http://*.localhost[:port] as secure contexts.

Important

Only localhost label is marked as Secure Context, URLs with loopback 127.0.0.1 IPs are not.

Ref.

@lidel lidel added the kind/bug A bug in existing code (including security flaws) label Jun 19, 2024
@2color
Copy link
Member

2color commented Aug 1, 2024

According to https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy is seems that 127.0.0.1 is potentially trustworthy:

If origin’s host matches one of the CIDR notations 127.0.0.0/8 or ::1/128 [RFC4632], return "Potentially Trustworthy".

@SgtPooki
Copy link
Member

SgtPooki commented Aug 5, 2024

For anyone interested in taking this task on (e.g. Luca from 2024-08-01 Helia WG):

We will want to modify https://github.com/ipfs/helia/blob/74ccc92793a6d0bb4bee714d9fe4fa4183aa4ee8/packages/block-brokers/src/trustless-gateway/utils.ts#L10C17-L31 in the following ways:

  1. Modify the filter so that when allowInsecure=false, a multiaddr with "127.0.0.1" as the IP still returns true. add a test
    • we could allow the entire block of 127.0.0.0/8 but shouldn't need to
  2. Modify the filter so that when allowInsecure=false, a multiaddr with "localhost" as the domain still returns true. add a test
  3. Modify the filter so that when allowInsecure=false, a multiaddr with a parent domain of "localhost" as the domain still returns true (e.g. example.localhost, foobar.localhost, *.localhost). add a test

acul71 added a commit to acul71/helia-fork that referenced this issue Aug 8, 2024
@SgtPooki SgtPooki linked a pull request Aug 9, 2024 that will close this issue
3 tasks
SgtPooki added a commit that referenced this issue Aug 12, 2024
* Fix issue #564: Modify filtering logic and update related tests

* chore: fix linting issues

* Update packages/block-brokers/src/trustless-gateway/utils.ts

Co-authored-by: Russell Dempsey <[email protected]>

* refactor: simplify conditional logic in filterNonHTTPMultiaddrs

---------

Co-authored-by: Russell Dempsey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants