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

feat: make routers optional and enable local insecure gateways #303

Closed
wants to merge 10 commits into from

Conversation

2color
Copy link
Member

@2color 2color commented Jun 19, 2024

Added by this PR:

  • Ability to disable delegated routing using a toggle. When disabled, the routers configuration will not be passed to Helia.
  • Probe for local gateway and add the local gateway to the configuration if available. If unavailable, remove the local gateway from the configuration.

Fixes #299 #288

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

@2color 2color changed the title Make routers optional and enable local insecure gateways feat: make routers optional and enable local insecure gateways Jun 19, 2024
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Did not run it locally yet, but dropping quick feedback / questions in the meantime.

src/lib/local-gateway.ts Outdated Show resolved Hide resolved
src/lib/config-db.ts Outdated Show resolved Hide resolved
src/pages/config.tsx Outdated Show resolved Hide resolved
@2color
Copy link
Member Author

2color commented Jun 20, 2024

I'm currently blocked by a bug I found, where verifiedFetch does not respect the allowInsecure and allowLocal config options passed to it.

I have a reproduction here: https://codepen.io/2color/pen/VwOXeMz. But I haven't been able to find out where the root of the problem is. Still digging into this.

@2color 2color added this to the IPFS Camp 2024 milestone Jun 24, 2024
@2color 2color requested a review from lidel June 26, 2024 13:27
@lidel lidel self-assigned this Jun 28, 2024
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

We don't have testing in place for this, so each flow needs to be inspected manually.
I did some testing in vanilla Google Chrome (empty profile each time) and both need a bit more polish.

  • Localhost detection works only when dancing a specific way:
    • 🟢 Works when visiting the main domain first (http://test1.localhost:3000/) and then manually open config detects and prepends my localhost:8080 gateway, and uses it just fine
    • 🚧 Does not work when visiting the main domain first with config URL opened (http://test2.localhost:3000/#/ipfs-sw-config)
    • 🚧 Does not work if I visit subdomain first (http://specs-ipfs-tech.ipns.test3.localhost:3000/ does not prepend localhost)
    • ⭕ The detection does dot work in Brave because they have very strick adblock (Brave Shields) rules:

      2024-06-28_19-58
      Due to this, a big chunk of audience won't be able to benefit from this anyway 😞

  • Disabling delegated routing works, but does more harm than good, making it not useful:
    • 🚧 Disabling delegated routing breaks DNSLink resolution (example), while it should only impact IPNS and not DNSLink

Between localhost detection and adjusting things in https://github.com/ipshipyard/waterworks-infra/issues/55 the latter improves things in all browsers, so we may want to park this and focus on that + the other quality-of-life improvements from the Camp milestone.

I feel we will get back to this once we simplify codebase after Camp. Right now, too much of a time sink.

@SgtPooki
Copy link
Member

@2color I created #403 to replace this one. Closing

@SgtPooki SgtPooki closed this Oct 24, 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.

ux: detect local trustless gw and default to it
3 participants