-
Notifications
You must be signed in to change notification settings - Fork 8
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: dynamic subdomain gateway detection #53
Conversation
Subdomain gateway won't have any path prefix, namespace and root identifier are in `Host` header. Closes #28
- removed hardcoded BASE_URL - improved detection based on matching from right to left to reduce false-positives related to dnslinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some highlights below.
export function isDnsLabel (label: string): boolean { | ||
return ['-', '.'].some((char) => label.includes(char)) | ||
export function isInlinedDnsLink (label: string): boolean { | ||
return isValidDnsLabel(label) && label.includes('-') && !label.includes('.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ to avoid false-positives added code to ensure an inlined DNSLink
- does not parse as a valid CID
- does not include
.
- includes at least one
-
import type { Helia } from '@helia/interface' | ||
|
||
export interface HeliaFetchOptions { | ||
path: string | ||
helia: Helia | ||
signal?: AbortSignal | ||
headers?: Headers | ||
origin?: string | null | ||
id?: string | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ had to rename, would be confusing for web devs ("origin" in web security context, including window.location
, means at minimum both protocol and the authority identifier).
src/lib/path-or-subdomain.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I did not clean this up to make the PR smaller and easier to review.
My plan is to move majority of subdomain handling to @helia/verified-fetch
in future PRs and then get back here and decide what minimal handling makes sense to remain in this project.
const urlString = request.url | ||
const url = new URL(urlString) | ||
const subdomain = url.hostname.replace(`.${BASE_URL}`, '') | ||
const subdomainRegex = /^(?<origin>[^/]+)\.(?<protocol>ip[fn]s)?$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ this produced invalid results for docs.ipfs.tech.ipns.example.com
, the new code walks labels from right to left, which is better order for avoiding false-positive matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we redirect from subdomains like "docs.ipfs.tech" to "docs-ipfs-tech" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I was not able to find where ?helia-sw
is handled – assumed it is not wired up yet, but I've updated _redirects
so it will work on web (subdomain or dnslink) gateway once we add it.
@SgtPooki what was the plan around ?helia-sw-subdomain
and ?helia-sw
? I want to add origin isolation enforcement in follow-up PR and most likely will have to touch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?helia-sw & ?helia-sw-subdomain are no longer required and should be able to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they were used to help with registering the service worker originally, but now, if the service worker is hit, we never render the jsx, and if the service worker isn't hit, we determine (in app.tsx) if we need to load the or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments and questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they were used to help with registering the service worker originally, but now, if the service worker is hit, we never render the jsx, and if the service worker isn't hit, we determine (in app.tsx) if we need to load the or not
* * /ipfs/CID (https://docs.ipfs.tech/concepts/content-addressing/) | ||
* * /ipns/DNSLink (https://dnslink.dev/) | ||
* * /ipns/IPNSName (https://specs.ipfs.tech/ipns/ipns-record/#ipns-name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
const urlString = request.url | ||
const url = new URL(urlString) | ||
const subdomain = url.hostname.replace(`.${BASE_URL}`, '') | ||
const subdomainRegex = /^(?<origin>[^/]+)\.(?<protocol>ip[fn]s)?$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we redirect from subdomains like "docs.ipfs.tech" to "docs-ipfs-tech" ?
// but the requested path is something it should, so show redirect and redirect to the same URL | ||
root.render( | ||
<RedirectPage /> | ||
) | ||
window.location.replace(window.location.href) | ||
} else { | ||
// TODO: add detection of DNSLink gateways (alowing use with Host: en.wikipedia-on-ipfs.org) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if en.wikipedia-on-ipfs.org.ipns.sw-host.tld
is requested, this will never be hit.
@@ -1,2 +1 @@ | |||
/ipns/* /?helia-sw=/ipns/:splat 302 | |||
/ipfs/* /?helia-sw=/ipfs/:splat 302 | |||
/* /?helia-sw=/:splat 302 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to remove this and just resolve #28
Description
This PR removes the concept of hardcoding
BASE_URL
at build time, which makes the same static payload work on any domain that follows subdomain convention, including inlined DNSlink names required for TLS wildcard setups:It allows us to have release artifacts which work everywhere.
Notes & open questions
Long term, we should make
@helia/verified-fetch
supporthttp(s)://
URLs for path, subdomain, and dnslink gateways, removing the need for creatingipfs://
andipns://
URLs.Change checklist