From a38adc5dd16d2152d0ebbc186875cbf1a3e1ca24 Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 21:38:10 +0200 Subject: [PATCH] CON-46: ask before connecting to a relay a link picked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A space carries its relay in its own address, 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, and — while signed in — 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. An unfamiliar host is asked about once, before anything is sent, and the answer is remembered per host in `localStorage`. Both the connection and the AUTH happen inside effects in `useRelay`, so "do not connect" cannot mean calling the hook and ignoring it — the hook must not be called. The shell is split for that: `Shell` decides, `SpaceShell` holds every relay-consuming hook including the ``, and either mounts or does not. Two reasons reach that one mechanism and stay apart as two screens, because a broken link and an undecided one are different things to be told. An address that does not parse no longer falls back to `DEFAULT_RELAY_URL`: until now it connected to the deployment's own relay purely so the view above could draw "Invalid group address." on top of it. Loopback and the host of `VITE_RELAY_URL` are trusted by construction — the first asks `isLocalRelayHost` rather than keeping a second copy of that rule (CON-45 is what one costs), the second is operator configuration, and prompting for a reader's own deployment would only teach them to click through. `/settings/spaces/'` stays outside the gate on purpose: that route calls no `useRelay`, so nothing holds the connection and `SpaceStore.start()` returns at its readiness check. `AppShell.trust.test.tsx` pins that, so wiring a relay through the settings route fails a test rather than quietly connecting. Revoking a remembered relay still means clearing the site's data; the "Trusted relays" list under /settings/profile is deliberately not in this ticket and is written down as residual risk in docs/09. --- docs/09-security-privacy.md | 138 +++++++++++++ src/nostr/known-relays.test.ts | 114 +++++++++++ src/nostr/known-relays.ts | 126 ++++++++++++ src/nostr/space-store.ts | 13 +- src/routes/space-route.ts | 11 +- src/ui/SpaceGate.tsx | 117 +++++++++++ src/ui/layout/AppShell.trust.test.tsx | 276 ++++++++++++++++++++++++++ src/ui/layout/AppShell.tsx | 167 ++++++++++++++-- src/ui/layout/Sidebar.tsx | 5 +- src/ui/layout/Topbar.tsx | 9 +- 10 files changed, 954 insertions(+), 22 deletions(-) create mode 100644 src/nostr/known-relays.test.ts create mode 100644 src/nostr/known-relays.ts create mode 100644 src/ui/SpaceGate.tsx create mode 100644 src/ui/layout/AppShell.trust.test.tsx diff --git a/docs/09-security-privacy.md b/docs/09-security-privacy.md index 8aeffc8..ceb6aff 100644 --- a/docs/09-security-privacy.md +++ b/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/'` — 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://` 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://` 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 `` — 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 `'` 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 diff --git a/src/nostr/known-relays.test.ts b/src/nostr/known-relays.test.ts new file mode 100644 index 0000000..7416ecc --- /dev/null +++ b/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() + }) +}) diff --git a/src/nostr/known-relays.ts b/src/nostr/known-relays.ts new file mode 100644 index 0000000..a185ea0 --- /dev/null +++ b/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/'` — 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)) +} diff --git a/src/nostr/space-store.ts b/src/nostr/space-store.ts index 8d132c2..c9fe5aa 100644 --- a/src/nostr/space-store.ts +++ b/src/nostr/space-store.ts @@ -28,7 +28,12 @@ export type SpaceSnapshot = { comments: Comment[] } -const EMPTY: SpaceSnapshot = { +/** + * A space that holds nothing and is done loading. Exported because the shell + * needs something to draw its chrome around while it is deliberately *not* + * talking to a relay — see the Gate comment in src/ui/layout/AppShell.tsx. + */ +export const EMPTY_SPACE: SpaceSnapshot = { loading: true, metadata: null, admins: [], @@ -45,7 +50,7 @@ const EMPTY: SpaceSnapshot = { * docs/02-data-model-events.md */ class SpaceStore { - private snapshot: SpaceSnapshot = EMPTY + private snapshot: SpaceSnapshot = EMPTY_SPACE private listeners = new Set<() => void>() private revisions = new Map() /** the winning placement per slug — src/domain/placement.ts */ @@ -123,8 +128,8 @@ class SpaceStore { this.comments.clear() this.metadataEvent = null this.groupEventAt.clear() - if (this.snapshot === EMPTY) return - this.snapshot = EMPTY + if (this.snapshot === EMPTY_SPACE) return + this.snapshot = EMPTY_SPACE for (const listener of this.listeners) listener() } diff --git a/src/routes/space-route.ts b/src/routes/space-route.ts index 5660d33..2a9bbd5 100644 --- a/src/routes/space-route.ts +++ b/src/routes/space-route.ts @@ -3,10 +3,10 @@ import { parseGroupAddress } from '../nostr/group-address' import type { GroupAddress } from '../nostr/group-address' import { useSpace } from '../nostr/space-store' import type { SpaceSnapshot } from '../nostr/space-store' -import { DEFAULT_RELAY_URL } from '../nostr/relay-status' export type SpaceRoute = { group: GroupAddress | null + /** empty string when the address does not parse — there is no relay then */ relayUrl: string space: SpaceSnapshot /** Base for links inside the space, e.g. /s/host'group */ @@ -22,7 +22,14 @@ export type SpaceRoute = { export function useSpaceRoute(): SpaceRoute { const params = useParams<{ group?: string; slug?: string }>() const group = params.group ? parseGroupAddress(params.group) : null - const relayUrl = group?.relayUrl ?? DEFAULT_RELAY_URL + // Deliberately no fall back to DEFAULT_RELAY_URL. An address that does not + // parse names no relay, and standing in the deployment's own relay for it + // was a connection nobody asked for — made, in the end, only so the view + // above could draw "Invalid group address." on top of it (CON-46). Every + // caller guards on `group` before it touches a relay, and `useSpace` with an + // empty group id subscribes to nothing, so the empty string never reaches a + // socket. docs/09-security-privacy.md + const relayUrl = group?.relayUrl ?? '' const space = useSpace(relayUrl, group?.id ?? '') return { group, diff --git a/src/ui/SpaceGate.tsx b/src/ui/SpaceGate.tsx new file mode 100644 index 0000000..322f40d --- /dev/null +++ b/src/ui/SpaceGate.tsx @@ -0,0 +1,117 @@ +import { useState } from 'react' +import { PageFrame, PageTitle } from './layout/PageFrame' +import { Button, ButtonLink, Callout } from './controls' + +/** + * What the shell shows *instead of* a space, for the two reasons it may refuse + * to open one. They are deliberately two screens rather than one with a + * condition in it: a link that does not parse is broken, while a link to an + * unfamiliar relay is fine and merely undecided, and telling a reader the + * wrong one of those is worse than telling them nothing. + * docs/09-security-privacy.md + */ + +/** + * The address in the URL is not `'`. There is no host in it to + * ask about and nothing to connect to, so this screen names the one thing that + * is actually known — the link is unreadable — and offers the way back. + * + * It replaces a silent fall back to the default relay: until CON-46 an + * unparsable address still opened a connection to `DEFAULT_RELAY_URL` behind + * an inline "Invalid group address." message, which connected to a relay the + * reader had not asked for in order to show them an error. + */ +export function InvalidSpaceLink({ address }: { address: string }) { + return ( + + + A space link reads host'space — for + example relay.example'engineering. This + one does not, so there is no relay to ask and no space to open. +

+ } + > + This link is not a space address + + +

+ What the address bar carried: {address} +

+ + Back to your spaces + + ) +} + +/** + * The address parses, and names a relay host this browser has no decision on + * record for. Asked once per host, before anything is sent. + * + * The point of the screen is that the reader can read the host before their + * browser talks to it, so the host is the largest thing on it and is spelled + * out in a monospace face — a name being read to decide whether it is the one + * you expected is a name that must not be re-rendered into something friendlier. + * + * "Remember this relay" is checked by default. Unchecked it still connects; + * the difference is only whether the shell asks again after a reload. Making + * the reader re-confirm their own team's relay on every visit would train them + * to click through it, which is the failure mode this whole screen exists to + * avoid. + */ +export function RelayTrustPrompt({ + host, + onConnect, +}: { + host: string + /** `remember`: also keep the decision across reloads. */ + onConnect: (remember: boolean) => void +}) { + const [remember, setRemember] = useState(true) + + return ( + + + This space lives on {host}, a relay this + browser has not talked to before. The link chose it — Akasha did not. +

+ } + > + Connect to {host}? + + + +
    +
  • Your IP address, and that you opened this link when you did.
    • +
  • + While you are signed in, an event signed with your key naming your npub — the relay + asks for it (NIP-42) and the app answers automatically once connected. +
    • +
+ + + +

+ Kept in this browser only. Clearing the site's data asks again. +

+ +
+ + Back to your spaces +
+ + ) +} diff --git a/src/ui/layout/AppShell.trust.test.tsx b/src/ui/layout/AppShell.trust.test.tsx new file mode 100644 index 0000000..360e576 --- /dev/null +++ b/src/ui/layout/AppShell.trust.test.tsx @@ -0,0 +1,276 @@ +// @vitest-environment jsdom +;(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { act } from 'react' +import { createRoot } from 'react-dom/client' +import type { Root } from 'react-dom/client' +import { MemoryRouter, Route, Routes } from 'react-router-dom' + +/** + * The trust gate, mounted for real (CON-46). + * + * The load-bearing assertion is the negative one: for a host nobody has + * approved, mounting the shell calls **neither** `fetch` **nor** + * `client.want`. Those two are the whole disclosure — `client.want` opens the + * WebSocket, on which `NostrClient.automaticallyAuth` answers the relay's + * NIP-42 challenge with an event signed by the reader's key, and `fetch` asks + * `https://` for the NIP-11 document. Both are fired from effects inside + * `useRelay`, which is why the gate has to keep the hook from being called at + * all rather than ignore what it returns; a test that only looked at the + * rendered markup would pass just as happily with the socket open behind it. + * + * Signed in on purpose: that is the case where following a link costs the + * reader a signed statement of who they are. + */ +vi.mock('../../session/session', () => ({ + SessionProvider: ({ children }: { children: unknown }) => children, + useSession: () => ({ + session: { + status: 'signed-in', + pubkey: 'a'.repeat(64), + npub: 'npub1example', + profile: null, + signer: {}, + }, + extension: 'available', + error: null, + login: vi.fn(), + logout: vi.fn(), + clearError: vi.fn(), + ensureSamePubkey: vi.fn(), + applyProfile: vi.fn(), + }), +})) + +import { AppShell } from './AppShell' +import { ThemeProvider } from '../../theme/theme' +import { client } from '../../nostr/client' +import { isTrustedRelay, trustRelay } from '../../nostr/known-relays' +import { useSpaceRoute } from '../../routes/space-route' + +const SPACE_MARKER = 'the space itself' + +/** + * Stands in for `SpaceSettingsRoute` at `/settings/spaces/:group`: that route + * reads the same `group` parameter through the same `useSpaceRoute` — and so + * the same `useSpace` — which is the only reason the settings exemption needs + * testing at all. The real screen is not mounted because its own contents are + * beside the point here; what is being measured is what the address alone can + * reach. + */ +function SettingsSpaceProbe() { + const { relayUrl } = useSpaceRoute() + return
{`settings route for [${relayUrl}]`}
+} + +let root: Root | null = null +let want: ReturnType +let fetchMock: ReturnType +let socket: ReturnType + +async function flush() { + for (let i = 0; i < 5; i++) await new Promise((resolve) => setTimeout(resolve, 0)) +} + +async function render(path: string) { + const host = document.createElement('div') + document.body.appendChild(host) + root = createRoot(host) + await act(async () => { + root!.render( + + + + }> + {SPACE_MARKER}} /> + } /> + } /> + not found} /> + + + + , + ) + }) + await act(async () => { + await flush() + }) +} + +function text(): string { + return document.body.textContent ?? '' +} + +function buttonSaying(label: string): HTMLButtonElement { + const found = [...document.querySelectorAll('button')].find( + (button) => button.textContent?.trim() === label, + ) + if (!found) throw new Error(`no button labelled "${label}" — page says: ${text()}`) + return found +} + +beforeEach(() => { + localStorage.clear() + // jsdom implements neither, and ThemeProvider/useRelay want both. + vi.stubGlobal('matchMedia', () => ({ + matches: false, + addEventListener: () => {}, + removeEventListener: () => {}, + })) + fetchMock = vi.fn().mockResolvedValue({ ok: false }) + vi.stubGlobal('fetch', fetchMock) + // Mocked rather than merely observed: a real want() would start connecting, + // and the point of the test is that nothing does. + want = vi.spyOn(client, 'want').mockReturnValue(() => {}) + // The socket itself, one level below `client.want`. `want` is the only path + // the shell is *supposed* to open a connection through, so watching it alone + // would miss a regression that reaches the pool some other way — a store + // subscribing without a hold, for instance. + socket = vi.fn() + vi.stubGlobal('WebSocket', socket) +}) + +/** Every relay url anything tried to reach, by either route. */ +function contacted(): string[] { + return [...want.mock.calls, ...socket.mock.calls, ...fetchMock.mock.calls].map((call) => + String(call[0]), + ) +} + +afterEach(async () => { + if (root) { + const current = root + await act(async () => current.unmount()) + root = null + } + document.body.innerHTML = '' + vi.unstubAllGlobals() + vi.restoreAllMocks() + localStorage.clear() +}) + +describe('AppShell: a space link naming an unfamiliar relay', () => { + it('asks first, and until it is answered contacts nothing at all', async () => { + await render(`/s/${encodeURIComponent("relay.example'team")}`) + + expect(text()).toContain('Connect to relay.example?') + // the space behind the link stays unmounted — it holds relay hooks of its own + expect(text()).not.toContain(SPACE_MARKER) + + expect(want, 'no WebSocket may be opened before the reader agrees').not.toHaveBeenCalled() + expect(fetchMock, 'no NIP-11 fetch may be made before the reader agrees').not.toHaveBeenCalled() + expect(socket, 'and none by any other route either').not.toHaveBeenCalled() + }) + + it('connects once the reader confirms, and remembers the host', async () => { + await render(`/s/${encodeURIComponent("relay.example'team")}`) + expect(isTrustedRelay('relay.example')).toBe(false) + + await act(async () => { + buttonSaying('Connect').click() + }) + await act(async () => { + await flush() + }) + + expect(text()).toContain(SPACE_MARKER) + expect(want).toHaveBeenCalledWith('wss://relay.example') + // "Remember this relay" is checked by default, so the next visit is silent + expect(isTrustedRelay('relay.example')).toBe(true) + }) + + it('does not remember the host when the box is unchecked', async () => { + await render(`/s/${encodeURIComponent("relay.example'team")}`) + + const box = document.querySelector('input[type="checkbox"]') as HTMLInputElement + expect(box.checked, 'remembering is the default').toBe(true) + await act(async () => { + box.click() + }) + await act(async () => { + buttonSaying('Connect').click() + }) + await act(async () => { + await flush() + }) + + expect(text()).toContain(SPACE_MARKER) + expect(want).toHaveBeenCalledWith('wss://relay.example') + expect(isTrustedRelay('relay.example')).toBe(false) + }) + + it('asks again for a host already remembered under a different port', async () => { + // Remembered first, or this asserts nothing about ports: the decision is + // keyed by the exact host, so approving relay.example says nothing about + // whatever answers on relay.example:8443. + trustRelay('relay.example') + + await render(`/s/${encodeURIComponent("relay.example:8443'team")}`) + + expect(text()).toContain('Connect to relay.example:8443?') + expect(text()).not.toContain(SPACE_MARKER) + expect(want, 'another port is another host').not.toHaveBeenCalled() + expect(fetchMock).not.toHaveBeenCalled() + }) +}) + +describe('AppShell: links that never reach the prompt', () => { + it('opens a remembered host straight away — asking once means once', async () => { + // The promise the whole feature makes: the answer survives the visit it + // was given in. Here it is a fresh mount reading the stored decision, not + // the in-memory `allowedOnce` set from a prompt earlier in the same run. + trustRelay('relay.example') + + await render(`/s/${encodeURIComponent("relay.example'team")}`) + + expect(text()).not.toContain('Connect to') + expect(text()).toContain(SPACE_MARKER) + expect(want).toHaveBeenCalledWith('wss://relay.example') + }) + + it('does not prompt for the local dev relay — that workflow stays frictionless', async () => { + await render(`/s/${encodeURIComponent("localhost:8080'team")}`) + + expect(text()).not.toContain('Connect to') + expect(text()).toContain(SPACE_MARKER) + expect(want).toHaveBeenCalledWith('ws://localhost:8080') + }) + + it('lets /settings/spaces/:group through ungated — and it still reaches nothing', async () => { + // That route reuses the `group` param, so a hand-written settings link can + // carry a host nobody approved. It is exempt from the gate on purpose: an + // interstitial in front of the settings navigation would buy no privacy, + // because nothing there discloses anything. + // + // Why it discloses nothing — the part that is an invariant rather than a + // fact about today's markup: the settings route holds no `useRelay`, so + // nobody calls `client.want` for that host, and `useSpace` on its own + // cannot reach a socket because `SpaceStore.start()` returns at + // `if (!connection.ready)` (src/nostr/space-store.ts) for a connection + // nothing ever asked for. A change that wires a relay through the settings + // route, or that lets a store subscribe without `want()`, breaks the + // exemption — and fails here. + await render(`/settings/spaces/${encodeURIComponent("relay.example'team")}`) + + expect(text()).not.toContain('Connect to relay.example?') + expect(text()).toContain('settings route for [wss://relay.example]') + + // Host-specific, because the shell does hold the deployment's own relay on + // every settings page — that one is operator configuration, not link input. + expect( + contacted().filter((url) => url.includes('relay.example')), + 'nothing may reach the host the link named', + ).toEqual([]) + }) + + it('refuses an unreadable address without touching a relay for it', async () => { + // No apostrophe, so parseGroupAddress returns null. This used to fall back + // to DEFAULT_RELAY_URL and connect there in order to draw an error. + await render('/s/not-an-address') + + expect(text()).toContain('This link is not a space address') + expect(text()).not.toContain(SPACE_MARKER) + expect(want, 'an unreadable address names no relay').not.toHaveBeenCalled() + expect(fetchMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/ui/layout/AppShell.tsx b/src/ui/layout/AppShell.tsx index dcc2e22..614dc32 100644 --- a/src/ui/layout/AppShell.tsx +++ b/src/ui/layout/AppShell.tsx @@ -1,13 +1,20 @@ import { useCallback, useEffect, useState } from 'react' import { Outlet, useLocation, useParams } from 'react-router-dom' +import type { ReactNode } from 'react' import { Topbar } from './Topbar' import { SessionNotice } from '../SessionNotice' import { Sidebar } from './Sidebar' import { TableOfContents } from './TableOfContents' import { TocProvider, useTocMarkdown } from './toc-context' +import { InvalidSpaceLink, RelayTrustPrompt } from '../SpaceGate' import { parseGroupAddress } from '../../nostr/group-address' +import type { GroupAddress } from '../../nostr/group-address' +import { isTrustedRelay, trustRelay } from '../../nostr/known-relays' import { DEFAULT_RELAY_URL, useRelay } from '../../nostr/relay-status' -import { useSpace } from '../../nostr/space-store' +import { EMPTY_SPACE, useSpace } from '../../nostr/space-store' +import type { SpaceSnapshot } from '../../nostr/space-store' +import type { RelayInfo } from '../../nostr/relay-status' +import type { RelaySnapshot } from '../../nostr/client' export function AppShell() { return ( @@ -17,15 +24,37 @@ export function AppShell() { ) } -const SIDEBAR_KEY = 'nc-sidebar-collapsed' - -function readCollapsed(): boolean { - try { - return localStorage.getItem(SIDEBAR_KEY) === '1' - } catch { - return false - } -} +/** + * Why the shell is split in two. + * + * The route parameter `/s/'` is chosen by whoever wrote the link, + * and everything downstream of it used to be automatic: `useRelay` opens a + * WebSocket to that host and fetches its NIP-11 document, and + * `NostrClient.automaticallyAuth` answers the relay's NIP-42 challenge with an + * event signed by the reader's key. Following a link was therefore enough to + * make a signed-in visitor's browser contact a stranger's host and hand it a + * signed statement of who they are. CON-46 puts one decision in front of that. + * + * Both of those happen in *effects* inside `useRelay`, so "do not connect" + * cannot mean "call the hook and ignore the answer" — it has to mean the hook + * is never called. A hook cannot be called conditionally, so the condition + * moves up one level: `Shell` decides, and `SpaceShell` — which holds every + * relay-consuming hook, `` included — either mounts or does not. + * + * Two independent reasons reach that one mechanism, and they stay apart in the + * code because they are different things to be told: + * + * - **unreadable** — `'` did not parse. There is no host here to + * ask about. Before CON-46 this connected to `DEFAULT_RELAY_URL` anyway and + * each view drew an inline "Invalid group address." over the top of it. + * - **untrusted** — it parsed, and names a host this browser has no decision + * on record for. `src/nostr/known-relays.ts` holds the answers. + * + * docs/09-security-privacy.md + */ +type Gate = + | { kind: 'unreadable'; address: string } + | { kind: 'untrusted'; group: GroupAddress } function Shell() { const params = useParams<{ group?: string }>() @@ -33,11 +62,125 @@ function Shell() { // /settings/spaces/:group reuses the `group` param name (so SpaceSettings // can share useSpaceRoute with the space's own routes) but is not "inside" // that space — the sidebar there is the settings nav, not the page tree. + // + // So a hand-written /settings/spaces/'group skips the gate. + // That is deliberate, and it is safe for a reason worth writing down, because + // it lives outside this file: the settings route calls no `useRelay`, so + // nothing calls `client.want` for that host, and `useSpace` alone never + // reaches a socket — `SpaceStore.start()` returns at `if (!connection.ready)` + // for a connection nobody asked for. Gating it would therefore buy no + // privacy and would only put an interstitial in front of the settings nav. + // `AppShell.trust.test.tsx` pins that, so a change which wires a relay + // through the settings route fails a test rather than quietly connecting. const inSettings = location.pathname.startsWith('/settings') - const group = !inSettings && params.group ? parseGroupAddress(params.group) : null + const address = !inSettings && params.group ? params.group : null + + // Hosts cleared for this visit but not written to storage — "Remember this + // relay" left unchecked. A set of hosts rather than one flag, so that + // following a link on to a *different* relay asks again instead of + // inheriting the answer given about the previous one. + const [allowedOnce, setAllowedOnce] = useState>(() => new Set()) + + const group = address !== null ? parseGroupAddress(address) : null + const gate: Gate | null = + address === null + ? null + : group === null + ? { kind: 'unreadable', address } + : isTrustedRelay(group.host) || allowedOnce.has(group.host) + ? null + : { kind: 'untrusted', group } + + if (gate !== null) { + return ( + + {gate.kind === 'unreadable' ? ( + + ) : ( + { + const host = gate.group.host + if (remember) trustRelay(host) + // Recorded here either way: `trustRelay` writes storage, but + // storage can be blocked, and re-reading it is not what makes + // this render show the space. + setAllowedOnce((current) => new Set(current).add(host)) + }} + /> + )} + + ) + } + + return +} + +/** + * Everything that talks to a relay. Mounted only once the address above has + * been read and cleared — see the `Gate` comment for why that has to be a + * mounting decision rather than a branch inside one component. + * + * `DEFAULT_RELAY_URL` is the fallback for the routes that are not inside a + * space at all (`/`, `/settings/*`). That is the deployment's own relay, + * configured at build time rather than named by a link, and it deliberately + * stays outside the gate. + */ +function SpaceShell({ group, inSettings }: { group: GroupAddress | null; inSettings: boolean }) { const relayUrl = group?.relayUrl ?? DEFAULT_RELAY_URL const { snapshot, info } = useRelay(relayUrl) const space = useSpace(relayUrl, group?.id ?? '') + + return ( + + + + ) +} + +const SIDEBAR_KEY = 'nc-sidebar-collapsed' + +function readCollapsed(): boolean { + try { + return localStorage.getItem(SIDEBAR_KEY) === '1' + } catch { + return false + } +} + +/** + * The chrome: top bar, left bar, canvas, table of contents. Purely + * presentational — it holds no relay state of its own, which is what lets the + * gate render the same frame around a prompt with `snapshot={null}` (no relay + * is being talked to, and a status badge claiming otherwise would be the one + * lie on that screen). + */ +function ShellFrame({ + group, + space, + snapshot, + info, + inSettings, + children, +}: { + group: GroupAddress | null + space: SpaceSnapshot + snapshot: RelaySnapshot | null + info: RelayInfo | null + inSettings: boolean + children: ReactNode +}) { + const location = useLocation() const tocMarkdown = useTocMarkdown() // Two separate states for one button. On a wide screen the bar is a column @@ -108,7 +251,7 @@ function Shell() { it by a hairline — the one detail that turns three panels into one surface. */}
- + {children}
diff --git a/src/ui/layout/Sidebar.tsx b/src/ui/layout/Sidebar.tsx index 88dae45..d0b44c2 100644 --- a/src/ui/layout/Sidebar.tsx +++ b/src/ui/layout/Sidebar.tsx @@ -28,7 +28,8 @@ import { type Props = { group: GroupAddress | null space: SpaceSnapshot - snapshot: RelaySnapshot + /** null while no relay is being talked to at all — see Topbar's own prop. */ + snapshot: RelaySnapshot | null info: RelayInfo | null /** * /settings/* — including /settings/spaces/:group, which reuses the @@ -369,7 +370,7 @@ export function Sidebar({ group, space, snapshot, info, inSettings }: Props) { - + {snapshot ? : null} ) } diff --git a/src/ui/layout/Topbar.tsx b/src/ui/layout/Topbar.tsx index 6b02b5a..ffe16e6 100644 --- a/src/ui/layout/Topbar.tsx +++ b/src/ui/layout/Topbar.tsx @@ -37,7 +37,12 @@ export function Topbar({ onToggleOverlay, }: { groupBase: string | null - snapshot: RelaySnapshot + /** + * null while the app is deliberately connected to no relay at all — the + * trust gate in AppShell. An indicator there could only report on a + * connection that does not exist, so the slot stays empty instead. + */ + snapshot: RelaySnapshot | null info: RelayInfo | null /** wide screens: the bar is a column that folds away */ columnHidden: boolean @@ -131,7 +136,7 @@ export function Topbar({
- + {snapshot ? : null}