From 8ed3501627c555de7d095b5c919d9c249d7ab293 Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 15:54:43 +0200 Subject: [PATCH 1/2] CON-45: match the local relay host exactly instead of by prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `host.startsWith('localhost')` is true for `localhost.evil.example`, a name anyone can register. Such a host therefore got an unencrypted `ws://` connection — and because the host comes out of the route, whoever writes a link picks it. NostrClient signs a NIP-42 AUTH event on connect, so that put the signed-in user's pubkey on the wire in the clear. The rule is anchored now and lives in one exported predicate, so the trust gate in CON-46 asks the same question rather than keeping a second inline copy of the condition — which is how this bug started. `[::1]` joins the local set: a dev relay bound to IPv6 is no less local than 127.0.0.1. The host is also validated by round-tripping it through `URL`: whatever the platform's own parser reads as the host is what a connection would go to, so anything that makes input and parse disagree is rejected. That covers `evil.example/#@real-relay.example`, which only reads as the real relay, and paths, queries, userinfo, whitespace and out-of-range ports with it. The second `decodeURIComponent` is gone. React Router decodes path params before `useParams`, so it was a decoder with no matching encoder, and it did harm twice: a group id ending in a percent sign threw `URIError` out of render, and a double-encoded link decoded back into a host that the address bar never showed. --- src/nostr/group-address.test.ts | 99 +++++++++++++++++++++++++++++++++ src/nostr/group-address.ts | 99 ++++++++++++++++++++++++++++++--- 2 files changed, 191 insertions(+), 7 deletions(-) create mode 100644 src/nostr/group-address.test.ts diff --git a/src/nostr/group-address.test.ts b/src/nostr/group-address.test.ts new file mode 100644 index 0000000..2627dec --- /dev/null +++ b/src/nostr/group-address.test.ts @@ -0,0 +1,99 @@ +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', + ])('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'], + ])('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'], + ])('rejects %j — %s', (raw) => { + expect(parseGroupAddress(raw)).toBeNull() + }) + + 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'])( + '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) + }) +}) diff --git a/src/nostr/group-address.ts b/src/nostr/group-address.ts index f070b78..50ac1f2 100644 --- a/src/nostr/group-address.ts +++ b/src/nostr/group-address.ts @@ -12,14 +12,99 @@ 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`. + * + * The port is only shape-checked here (`\d{1,5}` still admits `:99999`); + * `normalizeHost` is what rejects an out-of-range port. + */ +const LOCAL_HOST = /^(localhost|127\.0\.0\.1|\[::1\])(:\d{1,5})?$/ + +/** + * 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. + * + * Lowercases first so the predicate is correct on its own; hosts coming out + * of `normalizeHost` are already lowercase. + */ +export function isLocalRelayHost(host: string): boolean { + return LOCAL_HOST.test(host.toLowerCase()) +} + +/** + * 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. + * + * A default port written out (`example.com:443`) is dropped by `URL` and so + * rejected too — a needless way to write the address, and not worth a special + * case inside a security check. + */ +function normalizeHost(host: string): string | null { + if (host.length === 0) return null + try { + const url = new URL(`https://${host}`) + // the host carries no path/query/userinfo; URL also normalises case + return url.host === host.toLowerCase() ? url.host : null + } catch { + return null + } +} + +/** + * 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 { From ed053cd2328a289ede83882cb409309791fe36fb Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 21:38:10 +0200 Subject: [PATCH 2/2] CON-45: `isLocalRelayHost` normalises before it decides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The predicate is exported for callers holding a host from anywhere, not only for one `parseGroupAddress` has already validated — so it has to answer for itself. On the bare regex `localhost:99999`, `localhost:65536` and `localhost:00000` are all local, and a caller would pick plaintext `ws://` for a host no connection can be made to. It runs the host through `normalizeHost` now, which also keeps the port's range check in one place. `normalizeHost` accepts a written-out `:443` again, as the single exception where input and parse may disagree: `URL` drops it as https' default port, but a local host carries its port into a `ws://` URL, where `:443` is *not* the default and `ws://localhost:443` is a different connection than `ws://localhost`. This is reached in practice — `my-spaces.ts` and `CreateSpaceForm.tsx` derive the host by stripping the scheme off `DEFAULT_RELAY_URL`, so a deployment whose `VITE_RELAY_URL` spells the port out produces exactly that shape. The comparison stays exact, so a padded `:0443` is still rejected. docs/08 records the rule with the relay operations, where somebody choosing a host will read it. --- docs/08-relay-setup.md | 7 +++- src/nostr/group-address.test.ts | 58 ++++++++++++++++++++++++++---- src/nostr/group-address.ts | 63 ++++++++++++++++++++++----------- 3 files changed, 100 insertions(+), 28 deletions(-) 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 index 2627dec..d8299ca 100644 --- a/src/nostr/group-address.test.ts +++ b/src/nostr/group-address.test.ts @@ -11,6 +11,9 @@ describe('parseGroupAddress', () => { '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}` }) @@ -22,6 +25,11 @@ describe('parseGroupAddress', () => { ['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, @@ -43,10 +51,26 @@ describe('parseGroupAddress', () => { // 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') @@ -79,12 +103,18 @@ describe('parseGroupAddress', () => { }) describe('isLocalRelayHost', () => { - it.each(['localhost', 'localhost:8080', '127.0.0.1', '127.0.0.1:8080', '[::1]', '[::1]:8080'])( - 'holds for %s', - (host) => { - expect(isLocalRelayHost(host)).toBe(true) - }, - ) + 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', @@ -96,4 +126,20 @@ describe('isLocalRelayHost', () => { ])('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 50ac1f2..14d6377 100644 --- a/src/nostr/group-address.ts +++ b/src/nostr/group-address.ts @@ -24,22 +24,10 @@ export type GroupAddress = { * `[::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`. * - * The port is only shape-checked here (`\d{1,5}` still admits `:99999`); - * `normalizeHost` is what rejects an out-of-range port. + * 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})?$/ - -/** - * 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. - * - * Lowercases first so the predicate is correct on its own; hosts coming out - * of `normalizeHost` are already lowercase. - */ -export function isLocalRelayHost(host: string): boolean { - return LOCAL_HOST.test(host.toLowerCase()) -} +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. @@ -55,21 +43,54 @@ export function isLocalRelayHost(host: string): boolean { * `exämple.example` no longer equals its input and is rejected. IDN is out of * scope here rather than silently half-supported. * - * A default port written out (`example.com:443`) is dropped by `URL` and so - * rejected too — a needless way to write the address, and not worth a special - * case inside a security check. + * 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://${host}`) - // the host carries no path/query/userinfo; URL also normalises case - return url.host === host.toLowerCase() ? url.host : null + 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. *