From 25c171f74295b341208db742970059133a5a6ed5 Mon Sep 17 00:00:00 2001 From: sami Date: Fri, 11 Sep 2026 12:25:27 +0400 Subject: [PATCH] ext: pairing-restart test + AMO permission justification tests/transport/storage.test.ts covers the storage half of "the pairing token survives a browser restart" (round-trip, unpair-clears, corrupted override falls back to auto). websocket.test.ts adds the transport half: a fresh WebSocketTransport instance over the same backing store reuses the persisted token with no re-pairing, plus pairWithCode/unpair coverage. "Wrong token rejected and rate-limited" was already covered (websocket.test.ts's NotPaired/RateLimited cases). docs/amo-permissions.md is the submission-ready permission justification for AMO's Notes to Reviewer field, covering every permission in manifest.json plus what was deliberately not requested and how cookie/ header data is handled. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Ed8KEmAW48v4YHdxLtqsMB --- extension/docs/amo-permissions.md | 49 ++++++++++++++++ extension/src/background/transport/types.ts | 5 ++ .../src/background/transport/websocket.ts | 27 +++++++++ extension/tests/transport/storage.test.ts | 56 +++++++++++++++++++ extension/tests/transport/websocket.test.ts | 53 ++++++++++++++++++ 5 files changed, 190 insertions(+) create mode 100644 extension/docs/amo-permissions.md create mode 100644 extension/tests/transport/storage.test.ts diff --git a/extension/docs/amo-permissions.md b/extension/docs/amo-permissions.md new file mode 100644 index 0000000..636d086 --- /dev/null +++ b/extension/docs/amo-permissions.md @@ -0,0 +1,49 @@ +# AMO permission justification + +Submitted with every version bump in the AMO "Notes to Reviewer" field. Kept here so the +justification is reviewed and versioned alongside the permission list itself +(`manifest.json`), instead of living only in a web form. docs/05 §7 is the design-time +version of this; this file is the submission-ready copy. + +## What Velox is + +A download manager. It intercepts a response Firefox is about to download, hands the +URL, request headers, and cookies to a companion native application (`veloxd`), and lets +that application fetch the file with resumable, multi-connection transfers. The +extension itself never stores or transfers file bytes — see `CLAUDE.md` §3 and the +`no-download-logic` ESLint gate (`eslint.config.mjs`) that fails CI if it ever does. + +## Permissions requested + +| Permission | Why | Narrowest alternative considered | +|---|---|---| +| `webRequest` + `webRequestBlocking` | The whole feature: inspect response headers on `onHeadersReceived` to decide whether to intercept a download, and `{cancel: true}` before Firefox starts its own download. This is the one thing MV3 Chrome removed and MV3 Firefox kept — it's why the extension can exist as designed (docs/05 §1). | None. Without blocking `webRequest` there is no way to stop Firefox's own download before it starts; polling `downloads.onCreated` alone (which we also use, see below) only catches what already started. | +| `downloads` | Belt-and-braces safety net (`capture/downloads-api.ts`): some downloads (form POSTs, service-worker blobs) never reach `onHeadersReceived` in a way we can act on and only surface via `downloads.onCreated`. Also used to `cancel`/`erase` a download we're taking over so Firefox doesn't keep two copies. | Drop the safety net and accept that those cases silently bypass Velox. Rejected — docs/05 §2 calls this out explicitly as a known gap the safety net exists to close. | +| `cookies` | A file behind a login (private CDN links, forum attachments) needs its session cookies handed to `veloxd`, or the daemon's fetch gets a 403 the browser's own request wouldn't have. Read via `cookies.getAll(url)` only for a URL we are about to offer to the daemon — never harvested in bulk or logged. | Skip cookies and only support anonymous URLs. Rejected — it's a top user-facing IDM-parity feature and the reason people leave Chrome download managers behind. | +| `contextMenus` | "Download with Velox" on a link/image/video, and "Download all links…" (docs/05 §3). Table-stakes UI for a download manager extension. | None smaller — there's no partial grant for context menus. | +| `storage` | `browser.storage.local` holds only extension-local state: the WebSocket pairing token, the manual transport override, the last-good WS port, and the user's default-category preference (`transport/storage.ts`, `options/prefs.ts`). No browsing data. | None — some persistence is required for pairing to survive a restart (M1 DoD), which is the point of the token existing at all. | +| `notifications` | Tells the user when the daemon can't be reached for a download that fell back to Firefox, and (native-messaging path) surfaces pairing prompts if the GUI isn't running. | Silent failure. Rejected — capture fails open by design (CLAUDE.md §4) and a silent fallback with no notification would look like a bug. | +| `nativeMessaging` | Opportunistic transport to `veloxd` over a Unix socket, for installs where it works (docs/adr/0003). Not the default path — WebSocket is — but shipped because it avoids the WebSocket port-scan on installs where the native host manifest is reachable. | Drop native messaging and use WebSocket exclusively. Considered and rejected in ADR 0003: keeping both means the extension keeps working across deb/snap/flatpak Firefox without per-flavour capture-logic forks. | +| `` (host permission) | Downloads happen from every site on the web; `webRequest`'s header inspection and `cookies.getAll` both need to run against whatever site the user is on. This is the item AMO reviewers push back on hardest for extensions of this shape. | A fixed list of "known download sites" — unworkable for a general-purpose download manager, and defeats the point of an IDM-style interceptor. **Mitigation, not a narrower permission:** the exclusion list in Options is front-and-center (`options.html` → "Capture policy") so a user can scope capture down to nothing on sites they don't want Velox touching, and the bypass modifier (default Alt) lets a single click skip capture without changing settings. | + +## What is explicitly *not* requested + +- No `` XHR/fetch use — `webRequest`/`cookies` read metadata about a request + Firefox is already making; the extension never issues its own network request for + file bytes (enforced by the ESLint gate above). +- No `identity`, `history`, `bookmarks`, `tabs` beyond what `contextMenus`/`commands` + already imply, `management`, or any permission unrelated to capturing and handing off + a download. +- No remote code: the manifest ships no CDN scripts and no `eval`; `web-ext lint` fails + the build otherwise (CI's `extension-lint` job). + +## Data handling + +- Cookies and headers are held in memory only long enough to answer one + `capture.offer` call to the local daemon (`capture/headers.ts`'s ring buffer, 5-minute + TTL) — never written to disk by the extension and never sent anywhere but + `127.0.0.1`. +- The daemon connection is local-only: `WebSocketTransport` connects to + `ws://127.0.0.1:`, never a remote host (docs/05 §4, conformance-tested). +- Nothing is sent to Anthropic, Mozilla, or any third party beyond the user's own local + `veloxd` process. diff --git a/extension/src/background/transport/types.ts b/extension/src/background/transport/types.ts index 562af61..20fa0bc 100644 --- a/extension/src/background/transport/types.ts +++ b/extension/src/background/transport/types.ts @@ -62,6 +62,11 @@ export interface VeloxTransport { /** Fires on every state change. Returns an unsubscribe. */ onStateChange(cb: (status: TransportStatus) => void): () => void; + + /** WebSocket transport only (pairing has no meaning over native messaging's uds + * socket, which has no token). Options renders these controls only when present. */ + pairWithCode?(code: string): Promise; + unpair?(): Promise; } // --- errors --------------------------------------------------------------------------- diff --git a/extension/src/background/transport/websocket.ts b/extension/src/background/transport/websocket.ts index 1676d3b..a46085f 100644 --- a/extension/src/background/transport/websocket.ts +++ b/extension/src/background/transport/websocket.ts @@ -97,6 +97,9 @@ export class WebSocketTransport implements VeloxTransport { private stopped = false; private connectPromise: Promise | null = null; private reconnectTimer: ReturnType | null = null; + /** Set only for the duration of pairWithCode(); consumed by pair(). docs/05 §4: "the + * user clicks Allow (or types the code in the extension options)." */ + private pendingPairCode: string | null = null; private readonly listeners = new Map>(); private readonly stateListeners = new Set<(status: TransportStatus) => void>(); @@ -139,6 +142,29 @@ export class WebSocketTransport implements VeloxTransport { return this.connectPromise; } + /** + * Options → "Pair" with a code typed from the daemon's dialog, for when the GUI isn't + * running to click Allow (docs/05 §4). Drops any stored token first so the handshake + * takes the pairing branch, then reconnects with the code attached. + */ + async pairWithCode(code: string): Promise { + this.disconnect(); + await this.deps.setToken(null); // drop any stale token so the handshake takes the pairing branch + this.pendingPairCode = code; + try { + await this.connect(); + } finally { + this.pendingPairCode = null; + } + } + + /** Options → "Unpair": revoke the local token. The daemon's own record of it is + * cleaned up on its side; this only ever forgets our copy. */ + async unpair(): Promise { + await this.deps.setToken(null); + this.disconnect(); + } + disconnect(): void { this.stopped = true; if (this.reconnectTimer) { @@ -322,6 +348,7 @@ export class WebSocketTransport implements VeloxTransport { const params: SessionPairParams = { clientName: this.clientName, extensionId: this.deps.extensionId, + code: this.pendingPairCode, }; try { const res = (await rpc.request( diff --git a/extension/tests/transport/storage.test.ts b/extension/tests/transport/storage.test.ts new file mode 100644 index 0000000..ae1ec06 --- /dev/null +++ b/extension/tests/transport/storage.test.ts @@ -0,0 +1,56 @@ +// browser.storage.local is backed by the real add-on profile on disk, so anything +// written through transport/storage.ts is what "survives a browser restart" means in +// practice — this covers the storage half of that DoD item; websocket.test.ts's +// "survives a browser restart" case covers the transport half (a fresh transport +// instance reusing a persisted token with no re-pairing). +import { beforeEach, describe, expect, it } from 'vitest'; + +import * as storage from '../../src/background/transport/storage.js'; + +describe('transport/storage', () => { + beforeEach(async () => { + await browser.storage.local.clear(); + }); + + it('round-trips the pairing token', async () => { + expect(await storage.getToken()).toBeNull(); + await storage.setToken('tok-abc'); + expect(await storage.getToken()).toBe('tok-abc'); + }); + + it('clears the token on unpair (null)', async () => { + await storage.setToken('tok-abc'); + await storage.setToken(null); + expect(await storage.getToken()).toBeNull(); + }); + + it('a fresh read after a simulated restart still sees the persisted token', async () => { + await storage.setToken('tok-survives'); + // Nothing here recreates browser.storage.local — that's the point: it is the one + // thing in the extension that outlives the background page's lifetime, restart + // included. A second, independent read call stands in for "the page reloaded". + expect(await storage.getToken()).toBe('tok-survives'); + expect(await storage.getToken()).toBe('tok-survives'); + }); + + it('round-trips the transport override, defaulting to auto', async () => { + expect(await storage.getOverride()).toBe('auto'); + await storage.setOverride('uds'); + expect(await storage.getOverride()).toBe('uds'); + await storage.setOverride('ws'); + expect(await storage.getOverride()).toBe('ws'); + }); + + it('ignores a corrupted override value and falls back to auto', async () => { + await browser.storage.local.set({ 'velox.transportOverride': 'not-a-transport' }); + expect(await storage.getOverride()).toBe('auto'); + }); + + it('round-trips the cached WebSocket port', async () => { + expect(await storage.getCachedWsPort()).toBeNull(); + await storage.setCachedWsPort(52003); + expect(await storage.getCachedWsPort()).toBe(52003); + await storage.setCachedWsPort(null); + expect(await storage.getCachedWsPort()).toBeNull(); + }); +}); diff --git a/extension/tests/transport/websocket.test.ts b/extension/tests/transport/websocket.test.ts index 1b94b5d..03e4954 100644 --- a/extension/tests/transport/websocket.test.ts +++ b/extension/tests/transport/websocket.test.ts @@ -93,6 +93,59 @@ describe('WebSocketTransport', () => { expect(lastHello?.token).toBe('tok-issued-1'); }); + it('the pairing token survives a browser restart: a fresh transport instance over the same storage reuses it, no re-pairing', async () => { + const port = nextPort(); + daemon = await FakeDaemon.start({ port, acceptToken: null }); + const h = memDeps(); // stands in for browser.storage.local, which outlives the page + transport = makeTransport(port, h); + + await transport.connect(); + expect(daemon.pairCount).toBe(1); + expect(h.store.token).toBe('tok-issued-1'); + transport.disconnect(); + + // Simulate "the browser restarted": a brand new transport instance, same backing + // store (in reality, the same on-disk profile), no in-memory state carried over. + const restarted = makeTransport(port, h); + await restarted.connect(); + try { + expect(restarted.state).toBe('connected'); + expect(daemon.pairCount).toBe(1); // still just the one pairing, ever + const lastHello = [...daemon.seen].reverse().find((s) => s.method === 'session.hello'); + expect(lastHello?.token).toBe('tok-issued-1'); + } finally { + restarted.disconnect(); + } + }); + + it('pairWithCode drops any stale token and pairs fresh with the typed code', async () => { + const port = nextPort(); + daemon = await FakeDaemon.start({ port, acceptToken: 'stale' }); + const h = memDeps({ token: 'stale' }); + transport = makeTransport(port, h); + + await transport.pairWithCode('4821'); + + expect(transport.state).toBe('connected'); + expect(daemon.pairCount).toBe(1); + const pairCall = daemon.seen.find((s) => s.method === 'session.pair'); + expect((pairCall?.params as { code?: string }).code).toBe('4821'); + expect(h.store.token).toBe('tok-issued-1'); + }); + + it('unpair clears the stored token and disconnects', async () => { + const port = nextPort(); + daemon = await FakeDaemon.start({ port, acceptToken: 'good-token' }); + const h = memDeps({ token: 'good-token' }); + transport = makeTransport(port, h); + await transport.connect(); + + await transport.unpair(); + + expect(h.store.token).toBeNull(); + expect(transport.state).toBe('disconnected'); + }); + it('with autoPair off, a wrong token surfaces needsPairing and does NOT retry', async () => { const port = nextPort(); daemon = await FakeDaemon.start({ port, acceptToken: 'the-real-one' });