diff --git a/docs/08-relay-setup.md b/docs/08-relay-setup.md index 26e8b78..db341c5 100644 --- a/docs/08-relay-setup.md +++ b/docs/08-relay-setup.md @@ -114,7 +114,12 @@ Two quirks that cost time: ## Operations - Put the relay behind TLS (`wss://`), because an HTTPS page may not open - `ws://` (except for `localhost`). + `ws://` (except for `localhost`). The app derives the scheme from the host in + the space address and counts exactly `localhost`, `127.0.0.1` and `[::1]` as + local, each with an optional port — so a dev relay bound to IPv6 is reached + as `[::1]:8080`. Every other host gets `wss://`, including one that merely + begins with `localhost` (`isLocalRelayHost` in `src/nostr/group-address.ts`; + CON-45). - The web app is a static bundle on any host. - Backup = event export as JSONL. Because everything is signed, an export is verifiably restorable on another relay. That is also the migration strategy: diff --git a/src/nostr/group-address.test.ts b/src/nostr/group-address.test.ts new file mode 100644 index 0000000..d8299ca --- /dev/null +++ b/src/nostr/group-address.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, it } from 'vitest' +import { formatGroupAddress, isLocalRelayHost, parseGroupAddress } from './group-address' + +describe('parseGroupAddress', () => { + // CON-45. The old test was `host.startsWith('localhost')`, so every one of + // these — all registrable by anyone — got an unencrypted ws:// connection, + // over which the client then signs a NIP-42 AUTH event with the user's + // pubkey. The address comes out of a route, so a link is enough to pick it. + it.each([ + 'localhost.evil.example', + '127.0.0.1.evil.example', + 'localhostile.example', + '127.0.0.1.attacker.tld', + // with a port, because a fix that stripped the port before matching would + // pass every row above and still downgrade this one + 'localhost.evil.example:8080', + ])('connects to %s over wss, not ws', (host) => { + const group = parseGroupAddress(`${host}'engineering`) + expect(group).toEqual({ host, id: 'engineering', relayUrl: `wss://${host}` }) + }) + + it.each([ + ['localhost', 'ws://localhost'], + ['localhost:8080', 'ws://localhost:8080'], + ['127.0.0.1:8080', 'ws://127.0.0.1:8080'], + // a dev relay bound to IPv6 is as local as one bound to IPv4 + ['[::1]:8080', 'ws://[::1]:8080'], + ['[::1]', 'ws://[::1]'], + // `URL` drops a written-out `:443`, being https' default port. It has to + // survive anyway: under ws:// it is not the default, so dropping it would + // silently move the connection to port 80. + ['localhost:443', 'ws://localhost:443'], + ])('still reaches the dev relay on %s over ws', (host, relayUrl) => { + expect(parseGroupAddress(`${host}'engineering`)).toEqual({ + host, + id: 'engineering', + relayUrl, + }) + }) + + it.each([ + // reads as the real relay, resolves to evil.example + ["evil.example/#@real'engineering", 'a host that only reads as another one'], + ["evil.example/path'engineering", 'a path'], + ["a b'engineering", 'whitespace'], + ["user@evil.example'engineering", 'userinfo'], + ["evil.example?x'engineering", 'a query'], + ["'engineering", 'an empty host'], + ["localhost'", 'a missing group id'], + // React Router hands the param over still encoded when its own decode + // fails, so this is what actually arrives here. It used to throw URIError + // out of render — a white screen. + ["%E0%A4%A'group", 'a malformed percent escape'], + ["localhost:99999'engineering", 'a port above 65535'], + ["localhost:65536'engineering", 'a port one past the maximum'], + // the `:443` case above accepts the port as written, not any spelling of + // it: input and parse still have to agree exactly + ["localhost:0443'engineering", 'a zero-padded port'], + ])('rejects %j — %s', (raw) => { + expect(parseGroupAddress(raw)).toBeNull() + }) + + it('keeps the port of a host that is not local', () => { + expect(parseGroupAddress("evil.example:8080'engineering")).toEqual({ + host: 'evil.example:8080', + id: 'engineering', + relayUrl: 'wss://evil.example:8080', + }) + expect(parseGroupAddress("relay.example:443'engineering")?.relayUrl).toBe( + 'wss://relay.example:443', + ) + }) + + it('normalises the case of the host, as a connection would', () => { + expect(parseGroupAddress("LocalHost:8080'engineering")?.host).toBe('localhost:8080') + expect(parseGroupAddress("EVIL.example'engineering")?.relayUrl).toBe('wss://evil.example') + }) + + it('splits on the first apostrophe, so the id may contain one', () => { + expect(parseGroupAddress("localhost:8080'it's-fine")?.id).toBe("it's-fine") + }) + + // The decision on the double decode (CON-45): React Router already decoded + // this param, so parseGroupAddress does not decode again. Both of these + // pin that — the first would throw URIError in a second decodeURIComponent, + // the second would decode into host `evil.example` and id `x`. + it('takes its input as already decoded and never decodes a second time', () => { + expect(parseGroupAddress("localhost:8080'100%")).toEqual({ + host: 'localhost:8080', + id: '100%', + relayUrl: 'ws://localhost:8080', + }) + expect(parseGroupAddress("evil.example%27x'engineering")).toBeNull() + }) + + it('round-trips through formatGroupAddress', () => { + const raw = "relay.example'engineering" + const group = parseGroupAddress(raw) + expect(group).not.toBeNull() + expect(formatGroupAddress(group!)).toBe(raw) + expect(parseGroupAddress(formatGroupAddress(group!))).toEqual(group) + }) +}) + +describe('isLocalRelayHost', () => { + it.each([ + 'localhost', + 'localhost:8080', + '127.0.0.1', + '127.0.0.1:8080', + '[::1]', + '[::1]:8080', + 'localhost:443', + 'LocalHost:8080', + ])('holds for %s', (host) => { + expect(isLocalRelayHost(host)).toBe(true) + }) + + it.each([ + 'localhost.evil.example', + 'localhostile.example', + '127.0.0.1.attacker.tld', + 'evil.example', + 'notlocalhost', + '127.0.0.10', + ])('does not hold for %s', (host) => { + expect(isLocalRelayHost(host)).toBe(false) + }) + + // The predicate is exported for callers that hold a host from anywhere, not + // only for `parseGroupAddress`, which has already validated one. So it has to + // answer for itself rather than assume: on the bare regex every row here is + // `true`, and a caller would pick ws:// for a host no connection can be made + // to — or, on the last two, for a string that is not a host at all. + it.each([ + 'localhost:99999', + 'localhost:65536', + 'localhost:00000', + 'localhost:0443', + 'localhost/evil.example', + '', + ])('does not hold for %j, which is not a reachable host', (host) => { + expect(isLocalRelayHost(host)).toBe(false) + }) +}) diff --git a/src/nostr/group-address.ts b/src/nostr/group-address.ts index f070b78..14d6377 100644 --- a/src/nostr/group-address.ts +++ b/src/nostr/group-address.ts @@ -12,14 +12,120 @@ export type GroupAddress = { relayUrl: string } +/** + * The hosts a plaintext `ws://` connection is acceptable for: the developer's + * own machine. Anchored at both ends, because a prefix test is what let + * `localhost.evil.example` — a name anyone can register — resolve to `ws://` + * (CON-45). The host reaches us from the route `/s/'`, so it is + * attacker-choosable through a link, and `NostrClient` signs a NIP-42 AUTH + * event on connect: an unencrypted connection puts the signed-in user's + * pubkey on the wire in the clear. + * + * `[::1]` is in the set on purpose: a dev relay bound to IPv6 is reached as + * `[::1]:8080` and is no less local than `127.0.0.1`. + * + * This only names the shapes that count as local; it never sees an + * unvalidated port, because `isLocalRelayHost` normalises first. + */ +const LOCAL_HOST = /^(?:localhost|127\.0\.0\.1|\[::1\])(?::\d{1,5})?$/ + +/** + * Validates the shape of the host part and returns it canonicalised, or null. + * + * Round-tripping through `URL` rather than matching a regex: the parser is + * the same one the platform uses, so whatever it reads as the host is what a + * connection would actually go to. Anything that makes those two disagree is + * rejected — `evil.example/#@real-relay.example` parses to host + * `evil.example` and only *reads* as the real relay — and paths, queries, + * userinfo, whitespace and out-of-range ports fail the same way. + * + * Only ASCII hostnames are accepted: `URL` punycodes a Unicode name, so + * `exämple.example` no longer equals its input and is rejected. IDN is out of + * scope here rather than silently half-supported. + * + * Idempotent: feeding a host this returned back in yields the same host, so + * callers may normalise defensively without having to know whether someone + * already did. + */ +function normalizeHost(host: string): string | null { + if (host.length === 0) return null + const lower = host.toLowerCase() + try { + const url = new URL(`https://${lower}`) + // equal means the host carries no path, query or userinfo: whatever a + // connection would read as the host is exactly what was written + if (url.host === lower) return url.host + // `URL` drops a port that is the default for the scheme it was handed, so + // `https://:443` parses back without the `:443`. That is the one + // disagreement between input and parse that is not a confusion — the port + // was written out, it is in range, and it names the same endpoint — and + // dropping it would be wrong rather than merely strict, because a local + // host keeps its port into a `ws://` URL, where `:443` is not the default + // and `ws://localhost:443` is a different connection than `ws://localhost`. + // Reached in practice: `my-spaces.ts` and `CreateSpaceForm.tsx` derive this + // host by stripping the scheme off `DEFAULT_RELAY_URL`, so a deployment + // whose `VITE_RELAY_URL` spells the port out produces exactly this shape. + // The comparison stays exact, so a padded `:0443` is still rejected. + if (url.port === '' && `${url.host}:443` === lower) return lower + return null + } catch { + return null + } +} + +/** + * Whether this host may be reached over plaintext `ws://`. Exported so every + * place that needs the distinction asks the same question — a second, inline + * copy of the condition is exactly how CON-45 started. + * + * Normalises before matching rather than assuming a validated host, so the + * answer is correct for whatever a caller holds. Without that the regex alone + * answers `true` for `localhost:99999`, `localhost:65536` and `localhost:00000` + * — ports no connection can be made to — and a caller that had not already run + * the host through `normalizeHost` would pick `ws://` for them. It also keeps + * the port's range check in one place: `URL` decides it, here and in + * `parseGroupAddress` alike. + */ +export function isLocalRelayHost(host: string): boolean { + const normalized = normalizeHost(host) + return normalized !== null && LOCAL_HOST.test(normalized) +} + +/** + * Reads `'` into its parts, or returns null. + * + * `raw` is expected already decoded. React Router decodes path params before + * handing them to `useParams`, and both callers pass one straight through, so + * decoding again here would be a second decoder with no matching encoder — + * dropped deliberately (CON-45), for two reasons: + * + * - it broke legitimate addresses: a group id containing a percent sign + * (`localhost:8080'100%`) is fine after the router's decode but throws + * `URIError` in a second `decodeURIComponent`. + * - it reintroduced the confusion the host check above exists to stop: a + * link reading `evil.example%27x` decodes twice into host `evil.example` + * plus id `x`, so what the address bar shows and what gets connected to + * part ways again. + * + * With the decode gone nothing in here throws — `new URL` is caught — so a + * malformed route such as `/s/%E0%A4%A'group`, which React Router hands over + * still encoded because its own decode failed, returns null instead of + * escaping out of render as a white screen. + * + * Order matters: normalise the host first, then ask whether it is local, then + * build the URL — so the local test and the connection only ever see a host + * that has already passed validation. + * + * The group id itself is not validated; that is out of scope here. + */ export function parseGroupAddress(raw: string): GroupAddress | null { - const decoded = decodeURIComponent(raw) - const at = decoded.indexOf("'") - if (at <= 0 || at === decoded.length - 1) return null - const host = decoded.slice(0, at) - const id = decoded.slice(at + 1) - const local = host.startsWith('localhost') || host.startsWith('127.0.0.1') - return { host, id, relayUrl: `${local ? 'ws' : 'wss'}://${host}` } + const at = raw.indexOf("'") + if (at <= 0 || at === raw.length - 1) return null + const host = normalizeHost(raw.slice(0, at)) + if (host === null) return null + const id = raw.slice(at + 1) + const scheme = isLocalRelayHost(host) ? 'ws' : 'wss' + return { host, id, relayUrl: `${scheme}://${host}` } } export function formatGroupAddress(address: GroupAddress): string {