CON-46: ask before connecting to a relay a link picked

docs/09-security-privacy.md

@@ -41,6 +41,7 @@
 | Impersonation via display names | The npub is the truth and stays one hover or one click away — the author tooltip, a revision's Details view, `/settings/profile`; the member badge only appears for entries in `39002` |
 | Spam in open spaces | Relay rate limits + moderated deletion (`9005`) + a "members only" UI filter |
 | Key theft through the app | No handling of nsec at all. NIP-07/NIP-46 only |
+| A link choosing which relay your browser talks to | An unfamiliar relay host is asked about once before anything is sent, and the answer is remembered per host (by default — the reader can answer for this visit only). See "Relay connections follow links" below |
 
 ## Images are loaded directly
 
@@ -64,6 +65,143 @@ server. Bringing the gate back for foreign origins is a small change: the two
 places that draw an image are `MarkdownImage` in `src/ui/Markdown.tsx` and
 `ImageWidget` in `src/ui/markdown-live.ts`.
 
+## Relay connections follow links
+
+A space carries its relay inside its own address — `/s/<host>'<group>` — which
+is what makes a link to a space self-contained. It also means the link's author
+picks the host, and until CON-46 the rest was automatic:
+
+1. `AppShell` and `useSpaceRoute` turn the route parameter into a `relayUrl`,
+   with no allowlist and no prompt.
+2. `useRelay(relayUrl)` does two things to that host from its effects:
+   `client.want()` opens a WebSocket to it, and `fetchRelayInfo` fetches
+   `https://<host>` for the NIP-11 document.
+3. `NostrClient` sets `pool.automaticallyAuth`, so a signed-in visitor answers
+   the relay's NIP-42 challenge with no interaction at all.
+
+Following a link was therefore enough to make a signed-in reader's browser
+contact a host a stranger chose and hand it a signed statement of who they are —
+npub, IP address, and the time they clicked. Nothing here was a bug in any one
+of those three steps; the problem is that they compose into a disclosure that
+the reader never agreed to.
+
+### What was considered
+
+1. **Leave it.** A relay in a link is how the addressing works, and a reader who
+   follows a link to a wiki has arguably asked to reach it. Rejected: the thing
+   disclosed is a signed identity, not a page view, and it is disclosed before
+   anything is on screen to judge.
+2. **Ask once per new host** — the host is named, the reader decides, the answer
+   is remembered. **Chosen.**
+3. **Connect, but withhold AUTH** until the reader has confirmed the host: the
+   socket and the NIP-11 fetch happen immediately, only the signed NIP-42 event
+   waits. Rejected, for three reasons that compound. It keeps back the npub but
+   not the IP, the timing, or *which space* is being opened — the NIP-11 fetch
+   to `https://<host>` has already said all of that before the first frame. It
+   does not save the prompt either: a `private` group serves an unauthenticated
+   reader zero events and no error at all (docs/04-permissions-nip29.md, the
+   "Stranger, no AUTH" rows), so the reader still has to be asked — in front of
+   a space that is empty for reasons nothing on screen can explain. And it
+   is the wider change of the two — `signAuth` is not only
+   `pool.automaticallyAuth` (`src/nostr/client.ts:172`), it is passed as
+   `onauth` to every read and publish and called by `refreshAuth`
+   (`client.ts:430, 486, 515, 556, 576, 597`), so a gate that touched only the
+   automatic hook would leave every read signing anyway. Option 2 withholds
+   everything this option withholds, plus the socket and the fetch, from one
+   decision in one place.
+4. **An allowlist fixed at build time.** Strongest, and it is what a
+   single-relay deployment should arguably do. Rejected as the general answer:
+   it makes every legitimate federated link a dead end, and the app's own
+   addressing scheme then only works for addresses the operator foresaw.
+
+### How it works
+
+`src/nostr/known-relays.ts` holds the decision, keyed by the bare host (the
+exact `GroupAddress.host`, e.g. `relay.example:8443`) in `localStorage` under
+`nc-trusted-relays`. The scheme is never part of the key — it is derived from
+the host — so one decision cannot acquire two spellings.
+
+`src/ui/layout/AppShell.tsx` is where it bites. The relay-consuming half of the
+shell — `useRelay`, `useSpace`, and `<Outlet>` — sits in a child component that
+either mounts or does not. That is structural rather than stylistic: both
+disclosures happen in *effects* inside `useRelay`, so withholding them means
+never calling the hook, and a hook cannot be called conditionally.
+
+Two independent reasons reach that one mechanism, kept apart in the code:
+
+- **unreadable** — the address is not `<host>'<group>` at all. It names no
+  relay, so nothing is contacted. This also replaces an older accident: an
+  unparsable address used to fall back to `DEFAULT_RELAY_URL` and connect
+  *there*, so that each view could draw "Invalid group address." over the top
+  of a connection nobody had asked for.
+- **untrusted** — it parses, and names a host with no decision on record.
+
+Two kinds of host are trusted by construction and never reach the prompt:
+loopback (asked through `isLocalRelayHost`, so the dev workflow is unaffected),
+and the host of `DEFAULT_RELAY_URL`.
+
+**Remembering is the default, not the only answer.** "Remember this relay" is
+checked when the prompt opens; unchecking it still connects, but the decision
+then lives only in the shell's own state (`allowedOnce` in `AppShell`) and is
+gone on reload. That set is keyed by host as well, so following a link on from
+an approved relay to a *second* unfamiliar one asks again rather than inheriting
+the first answer.
+
+**The gate stands in front of `/s/:group`, not `/settings/spaces/:group`.**
+Both routes read the same `group` parameter, so a hand-written settings link can
+still carry an unapproved host. It discloses nothing: that route calls no
+`useRelay`, so nobody ever calls `client.want` for that host, and `useSpace`
+alone cannot reach a socket — `SpaceStore.start()` returns at
+`if (!connection.ready)` on a connection nothing asked for. An interstitial
+there would buy no privacy and would sit in front of the settings navigation
+instead. `src/ui/layout/AppShell.trust.test.tsx` pins that, so the day something
+does wire a relay through the settings route, a test says so.
+
+### Not behind the prompt, on purpose
+
+`DEFAULT_RELAY_URL` (`VITE_RELAY_URL`) and the profile relays
+(`VITE_PROFILE_RELAYS`) are **operator configuration**, chosen by whoever built
+and deployed the app. They are not link input, and no visitor's link can change
+them. Putting them behind the same prompt would ask readers to approve their
+own deployment on every fresh browser, which teaches exactly the reflex —
+clicking "Connect" without reading the host — that the prompt exists to avoid.
+
+### Residual risk, stated plainly
+
+- **The prompt is a decision, and decisions get clicked through.** A reader in a
+  hurry approves an unfamiliar host in one click, and the disclosure follows
+  immediately. All this buys is that the host is on screen, in a monospace face,
+  before anything is sent.
+- **It is per browser, not per person.** `localStorage` is cleared by a private
+  window, a new device, or clearing site data — and lost trust means a second
+  prompt, which is the safe direction, but it does mean the prompt recurs.
+- **There is no in-app way to take an approval back.** `forgetRelay` exists in
+  `src/nostr/known-relays.ts` and nothing calls it: today, undoing a "Remember
+  this relay" clicked in haste means clearing the site's data, which drops every
+  other decision with it. A "Trusted relays" list under `/settings/profile` —
+  the screen that already owns per-browser state — is the obvious home for it
+  and is deliberately not in CON-46.
+- **Once approved, the host is approved for everything.** There is no
+  distinction between "let it see my IP" and "let it see my npub"; connecting is
+  one act, and NIP-42 follows from it automatically.
+- **A trusted relay is still just a relay.** It reads everything in the group in
+  plaintext (see "Confidentiality" above). The gate is about *which* relay gets
+  to, not about what it can then see.
+- **A group id can still be a lie.** The gate checks the host, not the space:
+  a link to a relay you have already approved can name any group on it.
+
+### It does not narrow the CSP
+
+CON-46's ticket text claimed a known-hosts list would let CON-44's
+`connect-src` be narrowed. That is wrong, and worth writing down so it is not
+tried. `connect-src` is baked into the document at build time; relay hosts are
+discovered at *runtime*, from links. Unless a deployment has exactly one relay
+host — in which case it should say so directly in its CSP and skip the general
+mechanism — the relay sources have to stay scheme-scoped (`https:` / `wss:`)
+however much the trust gate remembers. The two measures are independent: the
+CSP bounds what the page *can* reach, the gate bounds what it *does* reach
+without asking.
+
 ## Privacy note for users
 
 An npub is a permanent pseudonym: everything a person posts is linkable across

src/nostr/known-relays.test.ts

@@ -0,0 +1,114 @@
+// @vitest-environment jsdom
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { forgetRelay, isTrustedRelay, trustRelay } from './known-relays'
+
+/**
+ * The decider behind the trust gate (CON-46). Everything here is about the
+ * question "may the app open a socket to this host without asking", so the
+ * cases that matter are the ones where a wrong answer is a disclosure: a host
+ * nobody approved must not come back trusted, and a storage that is missing,
+ * blocked or full of nonsense must not be able to turn into a yes — or into a
+ * crash, which would take the gate down with it.
+ */
+const STORAGE_KEY = 'nc-trusted-relays'
+
+beforeEach(() => {
+  localStorage.clear()
+})
+
+afterEach(() => {
+  vi.restoreAllMocks()
+  localStorage.clear()
+})
+
+describe('isTrustedRelay / trustRelay / forgetRelay', () => {
+  it('remembers a host until it is forgotten again', () => {
+    expect(isTrustedRelay('relay.example')).toBe(false)
+
+    trustRelay('relay.example')
+    expect(isTrustedRelay('relay.example')).toBe(true)
+    // and only that host — trust is not a blanket yes
+    expect(isTrustedRelay('other.example')).toBe(false)
+    expect(isTrustedRelay('evil.relay.example')).toBe(false)
+
+    forgetRelay('relay.example')
+    expect(isTrustedRelay('relay.example')).toBe(false)
+  })
+
+  it('keys by host including the port, so a second port is a second decision', () => {
+    trustRelay('relay.example:8443')
+    expect(isTrustedRelay('relay.example:8443')).toBe(true)
+    expect(isTrustedRelay('relay.example')).toBe(false)
+  })
+
+  it('is idempotent — StrictMode runs effects twice', () => {
+    trustRelay('relay.example')
+    trustRelay('relay.example')
+    trustRelay('relay.example')
+
+    expect(JSON.parse(localStorage.getItem(STORAGE_KEY) ?? '[]')).toEqual(['relay.example'])
+    // and one forget is still enough to undo all three
+    forgetRelay('relay.example')
+    expect(isTrustedRelay('relay.example')).toBe(false)
+  })
+
+  it('treats corrupt storage as nothing remembered, not as a reason to throw', () => {
+    localStorage.setItem(STORAGE_KEY, '{not json at all')
+    expect(isTrustedRelay('relay.example')).toBe(false)
+
+    // the wrong shape counts the same way, and entries that are not strings
+    // are dropped rather than compared
+    localStorage.setItem(STORAGE_KEY, '{"relay.example":true}')
+    expect(isTrustedRelay('relay.example')).toBe(false)
+    localStorage.setItem(STORAGE_KEY, '[17,null,"relay.example"]')
+    expect(isTrustedRelay('relay.example')).toBe(true)
+
+    // and it stays writable afterwards
+    trustRelay('other.example')
+    expect(isTrustedRelay('other.example')).toBe(true)
+  })
+
+  it('stays usable when localStorage itself throws (a private window)', () => {
+    vi.spyOn(Storage.prototype, 'getItem').mockImplementation(() => {
+      throw new Error('access denied')
+    })
+    vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
+      throw new Error('access denied')
+    })
+
+    expect(() => trustRelay('relay.example')).not.toThrow()
+    // it cannot be remembered, so it is not trusted — failing closed is the
+    // only safe direction for this particular question
+    expect(isTrustedRelay('relay.example')).toBe(false)
+    // but the local relay still is, because that answer needs no storage
+    expect(isTrustedRelay('localhost:8080')).toBe(true)
+  })
+
+  it('trusts loopback without storage, so the dev workflow never meets the prompt', () => {
+    expect(localStorage.getItem(STORAGE_KEY)).toBeNull()
+    expect(isTrustedRelay('localhost:8080')).toBe(true)
+    expect(isTrustedRelay('127.0.0.1:8080')).toBe(true)
+    expect(isTrustedRelay('[::1]:8080')).toBe(true)
+
+    // trusting it writes nothing: isTrustedRelay already knows
+    trustRelay('localhost:8080')
+    expect(localStorage.getItem(STORAGE_KEY)).toBeNull()
+
+    // the CON-45 near-miss stays a near-miss
+    expect(isTrustedRelay('localhost.evil.example')).toBe(false)
+  })
+
+  it('trusts the deployment’s own relay host without storage', async () => {
+    vi.stubEnv('VITE_RELAY_URL', 'wss://relay.operator.example')
+    vi.resetModules()
+    const fresh = await import('./known-relays')
+
+    expect(fresh.isTrustedRelay('relay.operator.example')).toBe(true)
+    expect(localStorage.getItem(STORAGE_KEY)).toBeNull()
+    // a neighbour of it is not
+    expect(fresh.isTrustedRelay('relay.operator.example.evil.example')).toBe(false)
+
+    vi.unstubAllEnvs()
+    vi.resetModules()
+  })
+})

src/nostr/known-relays.ts

@@ -0,0 +1,126 @@
+import { isLocalRelayHost } from './group-address'
+import { DEFAULT_RELAY_URL } from './relay-status'
+
+/**
+ * Which relay hosts this browser has agreed to talk to.
+ *
+ * A space carries its relay in its own address — `/s/<host>'<group>` — so a
+ * link decides, on its author's behalf, which host the reader's browser opens
+ * a WebSocket to, fetches a NIP-11 document from over HTTPS, and answers a
+ * NIP-42 challenge from with an event signed by the reader's key. Following a
+ * link is not consent to any of that, so an unfamiliar host is asked about
+ * once and the answer is remembered here.
+ * docs/09-security-privacy.md
+ *
+ * Keyed by the bare host — the exact `GroupAddress.host`, e.g.
+ * `relay.example` or `relay.example:8443` — and never by a relay URL. The
+ * scheme is *derived* from the host (`isLocalRelayHost` decides `ws` vs
+ * `wss`), so carrying it in the key would only give one decision two
+ * spellings, and a `ws://` entry could then look like consent a `wss://`
+ * entry never gave.
+ */
+const STORAGE_KEY = 'nc-trusted-relays'
+
+/**
+ * Hosts reach this module from `GroupAddress.host`, which `normalizeHost`
+ * has already lowercased. Lowercasing again anyway keeps the predicate
+ * correct on its own, the way `isLocalRelayHost` does — a decider whose
+ * answer depends on who called it is the harder kind of bug to see.
+ */
+function key(host: string): string {
+  return host.toLowerCase()
+}
+
+function hostOf(relayUrl: string): string | null {
+  try {
+    return new URL(relayUrl).host.toLowerCase()
+  } catch {
+    return null
+  }
+}
+
+/**
+ * The relay the deployment itself is configured with. It is build-time
+ * operator configuration, not link input, so it is trusted without ever being
+ * stored — see `isTrustedRelay`.
+ */
+const DEFAULT_RELAY_HOST = hostOf(DEFAULT_RELAY_URL)
+
+/**
+ * One defensive read, like `readCollapsed()` in `AppShell` and the theme
+ * bootstrap: `localStorage` throws outright in some private-window
+ * configurations, and the value is editable by hand, so anything that is not
+ * an array of strings counts as "nothing remembered" rather than as a reason
+ * to take the whole app down with it.
+ */
+function readTrusted(): string[] {
+  try {
+    const raw: unknown = JSON.parse(localStorage.getItem(STORAGE_KEY) ?? '[]')
+    if (!Array.isArray(raw)) return []
+    return raw.filter((entry): entry is string => typeof entry === 'string')
+  } catch {
+    return []
+  }
+}
+
+function writeTrusted(hosts: string[]): void {
+  try {
+    localStorage.setItem(STORAGE_KEY, JSON.stringify(hosts))
+  } catch {
+    /* then the decision only holds for as long as this page is open */
+  }
+}
+
+/**
+ * Whether the app may open a connection to this host without asking.
+ *
+ * Two kinds of host are trusted by construction, with nothing in storage:
+ *
+ *  - **Loopback.** Asked through `isLocalRelayHost` rather than re-tested
+ *    here: CON-45 is what a second, inline copy of that rule costs. It keeps
+ *    the dev workflow free of the prompt — `scripts/dev-relay-up.sh` puts a
+ *    relay on `localhost:8080` and nobody should have to click through an
+ *    interstitial to reach it.
+ *  - **The host of `DEFAULT_RELAY_URL`.** That is `VITE_RELAY_URL`, chosen by
+ *    whoever built and deployed the app. Operator configuration is not link
+ *    input, and asking the reader to approve their own deployment's relay
+ *    would teach them to click "Connect" without reading it.
+ */
+export function isTrustedRelay(host: string): boolean {
+  const wanted = key(host)
+  if (isLocalRelayHost(wanted)) return true
+  if (DEFAULT_RELAY_HOST !== null && wanted === DEFAULT_RELAY_HOST) return true
+  return readTrusted().includes(wanted)
+}
+
+/**
+ * Remember the host across reloads.
+ *
+ * Trusting a host twice is trusting it once: React runs effects twice under
+ * StrictMode, and a confirm button can be clicked twice before the view
+ * changes. The early return also keeps the two implicitly-trusted kinds of
+ * host out of storage, so nothing accumulates an entry that says what
+ * `isTrustedRelay` already knows.
+ */
+export function trustRelay(host: string): void {
+  if (isTrustedRelay(host)) return
+  writeTrusted([...readTrusted(), key(host)])
+}
+
+/**
+ * Take a remembered decision back, so the host is asked about again.
+ *
+ * It cannot revoke the two implicit kinds above — those are not decisions
+ * stored here, and a `forgetRelay('localhost:8080')` that appeared to work
+ * would be the worse outcome.
+ *
+ * No screen calls this yet, so revoking an approval currently means clearing
+ * the site's data — which drops every other decision too. The half that is
+ * missing is a "Trusted relays" list under `/settings/profile`, and it is
+ * deliberately not in CON-46; the gap is written down under "Residual risk" in
+ * docs/09-security-privacy.md rather than left for a reader to discover.
+ */
+export function forgetRelay(host: string): void {
+  const wanted = key(host)
+  writeTrusted(readTrusted().filter((entry) => entry !== wanted))
+}