CON-45: no plaintext ws:// for a host that merely starts with localhost

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:

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)
+  })
+})

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/<host>'<group>`, 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://<host>: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 `<host>'<group-id>` 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 {