relicario/docs/superpowers/specs/2026-05-04-security-polish-design.md

# Security & Docs Polish — Design

**Date:** 2026-05-04
**Status:** Proposed
**Source:** `docs/superpowers/reviews/2026-05-04-architecture-review.md` (P1.1, P1.7, P1.8)
**Effort estimate:** S (whole-PR; phase totals below)

## Summary

This is the one-day security and documentation PR that ships first in the v0.5.x architecture-followup train, independent of Plan B (CLI restructure) and Plan C (extension restructure). It closes the single defense-in-depth crypto gap the review flagged (`SessionHandle` has no `impl Drop`, so wasm-bindgen's `.free()` is a cleanup no-op while the master key sits in WASM linear memory), removes the JS-side error swallow that was masking that fact, brings the one undocumented security-critical core file (`recovery_qr.rs`) up to the documentation density of `crypto.rs` / `imgsecret.rs` / `backup.rs` / `tar_safe.rs`, and confirms the relay test/launcher polish items. Together these are mechanical, low-risk changes that fix the only finding in the review where "what the code looks like it does" diverges dangerously from "what it actually does" — which is exactly the wall a tinkerer learning Rust would hit hardest.

## Findings addressed

- **P1.1 — `SessionHandle` has no `impl Drop`; `.free()` is a cleanup no-op** — DEV-B (Rust headline) + DEV-C (JS partner finding). Files: `crates/relicario-wasm/src/lib.rs:15-23`, `crates/relicario-wasm/src/session.rs:1-58`, `extension/src/service-worker/session.ts:26`.
- **Partner JS swallow** — DEV-C P2 in service-worker. File: `extension/src/service-worker/session.ts:26` (the `try { current.free(); } catch { /* already freed */ }` block).
- **P1.7 — `recovery_qr.rs` is undocumented relative to the rest of core** — DEV-A. File: `crates/relicario-core/src/recovery_qr.rs:1-130`.
- **P1.8 — `tools/relay/queue.test.ts` assertion update** — DEV-C. File: `tools/relay/queue.test.ts:54`. Confirmed during planning: **already fixed in `061facd`** — line 54 now reads `assert.ok(isRole("dev-c"));` and the negative case `assert.ok(!isRole("dev-d"));` is on line 55. No code work remains; the plan tracks this as a verification step.
- **Launcher follow-up — `tools/relay/start.sh` still references three roles** — DEV-C P2. File: `tools/relay/start.sh:30-86`. Confirmed during planning: **not yet fixed.** Both `launch_tmux` and `launch_kitty` open only PM, Dev-A, Dev-B (no Dev-C); the manual-mode banner still says "Open 3 new terminals"; the post-launch summary lines (`:61, :81`) print "3 windows (PM, Dev-A, Dev-B)"; no `*-dev-c-prompt.md` is discovered or used. This phase is real.

## Approach

The PR is mechanical and uniform across four phases:

- **Adds** one `impl Drop for SessionHandle` block in `crates/relicario-wasm/src/lib.rs`, one Rust test that exercises construct → drop → registry-empty, one module-level `//!` doc-block in `recovery_qr.rs`, one ASCII layout diagram next to the magic constants there, four per-function doc-comments on the public API there, and one `DEV_C_PROMPT` discovery line + a fourth window in each launcher mode of `start.sh`.
- **Removes** the `try { current.free(); } catch { /* already freed */ }` swallow at `extension/src/service-worker/session.ts:26`.
- **Verifies** every `.free()` callsite under `extension/src/` (the audit deliverable for P1.1's JS side; today the audit itself is small — only one match — but the deliverable is the recorded grep + a check that every code path that holds a `SessionHandle` calls `wasm.lock(handle)` before `free()`).
- **Confirms in passing** that `tools/relay/queue.test.ts:54` already matches the new role union (it does — committed in `061facd`).

No public API changes on the Rust side beyond the `Drop` impl (which is observably equivalent to "calling `lock(handle)` was previously the only way to clean up"). No JS public API changes. No file moves. No new dependencies (`wasm-bindgen-test` is already in `[dev-dependencies]` for the wasm crate).

## Implementation phases

### Phase 1 — Rust-side `impl Drop for SessionHandle` + test

- **Goal:** make wasm-bindgen's auto-generated `.free()` actually clear the master key from WASM linear memory, regardless of whether JS also called `lock(handle)`.
- **Changes:**
  - `crates/relicario-wasm/src/lib.rs` — add `impl Drop for SessionHandle { fn drop(&mut self) { let _ = session::remove(self.0); } }` immediately after the existing `#[wasm_bindgen] impl SessionHandle { ... value() ... }` block (around line 23). Update the doc-comment on `SessionHandle` (line 15) to document the contract: "Dropping (or `.free()` from JS) removes the entry from the session registry and zeroizes the wrapped key/image_secret. `lock(handle)` remains available as the explicit early-cleanup path; `Drop` is the safety net."
  - `crates/relicario-wasm/src/session.rs` — no functional change required, but add a one-line module-level note above `pub fn remove(...)` explicitly stating that `Drop for SessionHandle` is the second caller of this function alongside `lock()`, so that a future reader removing `lock()` doesn't accidentally orphan the cleanup path.
- **Tests:**
  - Add a `#[wasm_bindgen_test]` (the `wasm-bindgen-test` crate is already in `[dev-dependencies]`, line 25 of `Cargo.toml`) under `crates/relicario-wasm/tests/session_drop.rs`. The test constructs a `SessionHandle` (via the public `unlock` path with synthetic JPEG + tiny `KdfParams` for fast key derivation, mirroring the existing native `session_tests::manifest_round_trip_via_handle` shape), records the underlying handle id via `handle.value()`, drops the handle, then asserts `session::with(id, |_| ()).is_none()`. The `session::with` symbol must be re-exported from `tests/` scope; if it isn't, expose `pub fn with_for_tests(handle: u32) -> bool` on the `session` module guarded by `#[cfg(test)]` so the test does not need to reach into thread-locals.
  - As a fallback / belt-and-suspenders, add a parallel native `#[test]` under the existing `mod session_tests` in `lib.rs` that constructs a `SessionHandle(session::insert(...))`, drops it explicitly with `drop(handle)`, then asserts `session::with(h, |_| ()).is_none()`. Native tests run on every `cargo test`; the wasm-bindgen-test runs only when someone invokes `wasm-pack test`, so the native variant is the one that catches regressions in CI.
- **Effort:** S
- **Depends on:** none

### Phase 2 — JS-side `.free()` audit + remove the swallow

- **Goal:** make the JS side stop hiding crypto-state-transition errors, and prove (via a recorded audit) that there are no other `.free()` callsites on `SessionHandle` (or on `EncryptedAttachment` / `TotpCode`) that bypass `wasm.lock(handle)` first.
- **Changes:**
  - `extension/src/service-worker/session.ts:24-28` — replace the body of `clearCurrent()` so that `current.free()` is called unconditionally (no `try`/`catch`) and `current` is set to `null` afterwards. The doc-block at the top of the file (lines 1-9) gets one extra paragraph noting that with `impl Drop` on the Rust side (Phase 1), `free()` now removes the registry entry — `lock(handle)` becomes a redundant early-cleanup option, but the SW currently does not call it. Document whether to keep `clearCurrent()` minimal (just `free()`) or add an explicit `wasm.lock(current.value)` immediately before `free()` for symmetry with the CLI's session lifecycle. Recommended: just `free()` post-Phase-1 — calling `lock` first is now belt-and-suspenders that adds a redundant HashMap remove. Document the rationale in the comment.
  - **Audit deliverable:** run `grep -rn "\.free\b" extension/src/` and record the result in the PR description. Today this returns exactly one match (`extension/src/service-worker/session.ts:26`), confirming the SW is the only surface that calls `.free()` on a wasm-bindgen handle. If a future surface (e.g. an offscreen document, a future setup-flow refactor) adds a second callsite, this PR's grep becomes the baseline that flags the new one.
  - **Lifecycle audit (no code change required, but record findings in PR description):** confirm every code path that constructs a `SessionHandle` via `unlock(...)` ultimately reaches `clearCurrent()`. The two paths are (a) explicit user lock via the popup `lock` action → `service-worker/router/popup-only.ts` `lock` handler → `clearCurrent()`, and (b) inactivity timer expiry → `service-worker/index.ts:51-58` session-expired callback → `clearCurrent()`. Both are already wired; this audit just records that the wiring is complete.
- **Tests:**
  - Vitest in `extension/`: add a small unit test in `extension/src/service-worker/__tests__/session.test.ts` (create if absent) that asserts `clearCurrent()` is a no-op when `current` is `null`, calls `setCurrent({ free: spy, value: 1 } as unknown as SessionHandle)` then `clearCurrent()`, and asserts `spy` was called exactly once and `getCurrent()` returns `null`. Use the existing `__stubs__/relicario_wasm.stub.ts` pattern for the handle shape if convenient.
  - Optionally extend the lifecycle test: a second case where `free()` throws — assert the exception now propagates out of `clearCurrent()` (regression coverage for the swallow removal).
- **Effort:** S
- **Depends on:** Phase 1 (the swallow removal is only safe once the Rust side actually does the cleanup work that `.free()` is now supposed to do)

### Phase 3 — `recovery_qr.rs` documentation pass

- **Goal:** raise `crates/relicario-core/src/recovery_qr.rs` to the documentation density of `crypto.rs` / `imgsecret.rs` / `backup.rs` / `tar_safe.rs` so a reader landing in the file does not assume it is out-of-the-documented-zone scratch code.
- **Changes:** all in `crates/relicario-core/src/recovery_qr.rs`:
  - **Module-level `//!` header** at the top of the file (above the `use` block on line 1). Three short paragraphs: (1) *what* the module produces (a 109-byte payload encoded as a QR v40 EcLevel::M SVG that, given the same passphrase, recovers the 32-byte image_secret); (2) *why the format is structured this way* — XChaCha20-Poly1305 wrapping over an Argon2id-derived "wrap key" derived from the recovery passphrase (`b"relicario-recovery-v1\0"` domain-separation prefix prevents the wrap key from ever colliding with a vault master key, even if the user reuses their vault passphrase here); (3) *parameter-pinning rationale* — `production_params()` deliberately re-states `KdfParams::default()`'s values rather than referencing them, because changing the default at the type level must not silently invalidate every recovery QR a user has printed.
  - **ASCII layout diagram** above the magic-constant block (lines 7-9):
    ```
    //  byte    field           length
    //  ------  --------------  ------
    //  0..4    MAGIC = "RREC"  4
    //  4..5    VERSION = 0x01  1
    //  5..37   kdf_salt        32   (random per QR)
    //  37..61  wrap_nonce      24   (random per QR)
    //  61..109 ciphertext      48   (32 image_secret + 16 AEAD tag)
    //  ------------------------------
    //  total                   109
    ```
    Pair the diagram with a `const _: () = assert!(PAYLOAD_LEN == 4 + 1 + 32 + 24 + 48);` static assertion so a future edit that changes the layout cannot drift from the documented total without a compile error.
  - **Doc-comments on the four public items:**
    - `RecoveryQrPayload` (line 11): one-paragraph summary; note that `as_bytes()` is the only accessor and that the bytes are an opaque sealed package — they should travel together (printing them as a QR is the supported transport, but a hex string also works for paranoid sneakernet).
    - `generate_recovery_qr` (line 45): documents inputs (passphrase, image_secret), output (the 109-byte payload), and the security properties (random kdf_salt + random wrap_nonce per call ⇒ two QRs from the same passphrase + image_secret are different bytes). Cross-reference `recovery_qr_to_svg` for rendering.
    - `unwrap_recovery_qr` (line 81): documents inputs/output, return-type rationale (`Zeroizing<[u8; 32]>` so the recovered image_secret zeroes when dropped), and the error vocabulary (`RecoveryQr` for format errors, `Decrypt` for AEAD failure / wrong passphrase — the deliberate non-distinguishing rejection from audit M4).
    - `recovery_qr_to_svg` (line 122): documents the rendering choice (QR v40, EcLevel::M, 140×140 minimum dimension) and the rationale (109 bytes fits comfortably in v40 at M, leaving recovery-error-correction headroom for a smudged print).
  - **`production_params` (line 32):** add a doc-comment explaining the deliberate divergence-tolerance from `KdfParams::default()` — wording: "Pinned at QR-format authoring time. `KdfParams::default()` may evolve as we re-tune Argon2 cost; this function MUST NOT change values that have ever shipped, because doing so silently invalidates every recovery QR printed under a previous parameter set." Either keep the function-with-comment shape, or per the synthesis suggestion, replace it with a `const RECOVERY_PRODUCTION_PARAMS: KdfParams = KdfParams { argon2_m: 65536, argon2_t: 3, argon2_p: 4 };` if `KdfParams` is `const`-constructible (it is — checked via `crypto.rs`). Either is acceptable; the `const` form is preferred because it makes the "this is not derived from defaults" property visible at the use site.
  - **Optional micro-cleanups while in the file** (cite explicitly so they aren't surprising in code review):
    - Replace the hand-counted `payload_bytes[5..37]`, `[37..61]`, `[61..109]` slice indices with named constants (`KDF_SALT_RANGE`, `WRAP_NONCE_RANGE`, `CIPHERTEXT_RANGE`) derived from the layout offsets. Improves diff-readability if the layout ever extends.
    - Add a one-line comment to `recovery_kdf_input` documenting that the length-prefix on `nfc_bytes` mirrors the same convention as `crypto::derive_master_key` (audit H1 in the design spec) — this is the connective tissue that explains "why is there a `(len as u64).to_be_bytes()` here too?"
- **Tests:**
  - No new test logic needed — the existing `tests/recovery_qr.rs` and the `RecoveryQr`/`Decrypt` round-trips in `tests/integration.rs` already pin behaviour.
  - The `const _: () = assert!(...)` static assertion is itself the contract test for the layout diagram.
- **Effort:** S
- **Depends on:** none

### Phase 4 — Relay launcher (queue.test.ts already done in `061facd`)

- **Goal:** confirm the test fix that already shipped, and bring the launcher script up to the four-role world.
- **Changes:**
  - `tools/relay/queue.test.ts` — **no change.** Confirmed during planning: line 54 is already `assert.ok(isRole("dev-c"));` and line 55 is already `assert.ok(!isRole("dev-d"));` (committed in `061facd`). This phase records the verification step in the PR description so a reviewer can see the file was checked.
  - `tools/relay/start.sh` — real changes:
    - Add a `DEV_C_PROMPT` discovery line at lines 30-33 alongside the existing `PM_PROMPT` / `DEV_A_PROMPT` / `DEV_B_PROMPT` definitions: `DEV_C_PROMPT="$(ls -t "$COORD_DIR"/*-dev-c-prompt.md 2>/dev/null | head -1 || echo "(none found)")"`.
    - Manual mode (`print_manual_instructions`, lines 35-51) — change "Open 3 new terminals" to "Open 4 new terminals" and add the `Terminal 4 (Dev C): cat '$DEV_C_PROMPT'` line.
    - tmux mode (`launch_tmux`, lines 53-69) — add a fourth `tmux new-window -t "$SESSION:" -n "dev-c" "cd '$REPO_ROOT' && claude"` line; update the summary print on line 61 from "4 windows: relay, pm, dev-a, dev-b" to "5 windows: relay, pm, dev-a, dev-b, dev-c"; add `Dev C: $DEV_C_PROMPT` to the prompts list (currently lines 64-66).
    - kitty mode (`launch_kitty`, lines 71-86) — add a fourth `kitty @ launch --type=tab --tab-title "Dev-C" --hold -- bash -l -i -c "cd '$REPO_ROOT' && claude"` line; update the summary print on line 81 from "3 windows (PM, Dev-A, Dev-B)" to "4 windows (PM, Dev-A, Dev-B, Dev-C)"; add `Dev C: $DEV_C_PROMPT` to the prompts list.
- **Tests:** none directly. The relay launcher has no automated tests; verification is "run `bash tools/relay/start.sh --manual` and confirm the banner mentions four terminals + the Dev-C prompt path; run `bash tools/relay/start.sh --kitty` on a kitty terminal and confirm a fourth tab opens." Both checks belong in the PR's manual-test plan.
- **Effort:** S
- **Depends on:** none

## Risks and mitigations

- **Adding `impl Drop` could change observable behaviour for any JS code that today holds two references to the same `SessionHandle` and frees one of them.** wasm-bindgen does not in fact let JS hold two refs to the same Rust struct without explicit cloning — the `SessionHandle` is moved into JS at construction and `.free()` consumes the handle pointer. The audit in Phase 2 confirmed only one `.free()` callsite under `extension/src/`. The CLI does not touch the WASM crate at all (Rust-side; the CLI uses `relicario-core` directly). So the realistic blast radius is exactly the one SW callsite this plan also touches. Mitigation: Phase 1's native test explicitly covers "drop the handle, registry is empty," and Phase 2's vitest covers "free is called exactly once."
- **Removing the `try { current.free() } catch { /* already freed */ }` swallow could expose latent bugs that were silently tolerated.** Specifically, if any code path today double-frees the handle (calls `clearCurrent()` twice without a `setCurrent()` in between), the catch is currently hiding it. Reading the SW lifecycle: `clearCurrent()` is called from (a) the `lock` message handler, (b) the inactivity-timer session-expired callback, and (c) potentially the unlock-failure cleanup path. The current `if (!current) return;` early-out at line 25 already guards the simplest double-free case (calling `clearCurrent()` twice in a row is safe because the second call no-ops). The only way the catch fires today is if something in WASM raises during `free()` — which post-Phase-1 should be impossible (Drop is infallible; `session::remove` returns a bool, not a Result). Mitigation: the swallow removal happens *after* Phase 1's Drop lands, so the catch should now have nothing to catch. If a regression test surfaces a real bug, the right fix is to find and fix the bug, not re-add the swallow.
- **The `recovery_qr.rs` static assertion (`const _: () = assert!(PAYLOAD_LEN == ...);`) requires Rust 1.79+** for `const` panic in test position. The workspace's `rust-toolchain.toml` (if present) or the `Cargo.toml` `rust-version` field needs to be checked; if the repo is on an older toolchain, fall back to a runtime `#[test] fn payload_len_matches_layout() { assert_eq!(PAYLOAD_LEN, 4 + 1 + 32 + 24 + 48); }` — same effect, slightly less elegant. Verify during implementation; not a blocker.
- **Phase 4's launcher change is untested by automation.** A typo in the kitty/tmux command lines won't surface until someone runs `start.sh --kitty` or `start.sh --tmux`. Mitigation: the PR description includes an explicit manual-test plan ("run all three modes and confirm 4 windows + Dev-C prompt resolution") and the change pattern is symmetric to the existing Dev-A/Dev-B lines, minimizing typo surface.
- **The `wasm-bindgen-test` variant in Phase 1 only runs under `wasm-pack test --node`, which is not part of the project's default `cargo test` cycle.** The native `#[test]` in `mod session_tests` is the one that protects against regressions in CI. The wasm-bindgen-test is included for symmetry and to exercise the actual WASM target's Drop behaviour — it should be invoked at least once during PR review and added to a follow-up CI workflow if the project's test infrastructure ever expands to wasm-pack.

## Out of scope

This plan deliberately does not address, and explicitly leaves to other plans or the P2/P3 tail:

- Plan B's CLI restructure work in its entirety (P1.2 main.rs split, P1.3 git-shell error UX, P1.10 parser migration to core, plus the CLI P2/P3 entries). These touch `crates/relicario-cli/` and are independent of this PR.
- Plan C's extension restructure work in its entirety (P1.4 setup.ts WASM extraction, P1.5 vault.ts split, P1.6 shared/state.ts typing, P1.9 SW router helper extraction, plus the extension P2/P3 entries). These touch `extension/src/` and are independent of this PR — except for the single SW file that Phase 2 of this plan touches, which is small and disjoint from Plan C's surfaces.
- The P2 WASM cleanups DEV-B flagged: redundant `need_key + with(...).unwrap()` double-lookup, `Vec<u8>` getter clones, the `wasm_*_recovery_qr` naming inconsistency, and the `device.rs` vs `session.rs` concurrency-primitive split. Each is independently correct and worth doing, but each is its own one-paragraph change and the PR is already covering its theme.
- DEV-C's other relay P2s: queue TTL/persistence/cap, the `tools/relay/call.py` and `call.ts` tracking decision, `server.ts` per-connection factory comment. None block this PR.
- The eight "Open architectural decisions" at the bottom of the synthesis (deliberate-Drop-omission, parser migration timing, bootstrap rule, `Lock` subcommand visibility, Task 12 status, call.py tracking, snake_case-vs-camelCase WASM naming, `.gitea_env_vars` gitignore). Most are user-judgement calls; this PR confirms #1 (the omission was not deliberate; the fix lands here) but takes no position on #2-#8.
- Anything touching the in-flight uncommitted v0.5.x work — manifests, glyphs, vault.css, relay tooling beyond `start.sh`. Those changes are on parallel branches and are explicitly hands-off for this PR.

## Done criteria

A reviewer can confirm this plan shipped by checking:

- [ ] `crates/relicario-wasm/src/lib.rs` contains `impl Drop for SessionHandle` whose body removes the registry entry.
- [ ] `crates/relicario-wasm/tests/session_drop.rs` (or equivalent native `#[test]` inside `mod session_tests`) asserts that dropping a `SessionHandle` removes its handle id from the session registry. `cargo test -p relicario-wasm` passes the new test.
- [ ] `extension/src/service-worker/session.ts` no longer contains `try { current.free(); } catch`. The body of `clearCurrent()` calls `current.free()` unconditionally, then sets `current = null`.
- [ ] `grep -rn "\.free\b" extension/src/` returns exactly one match (the SW callsite). PR description records this audit.
- [ ] `extension/src/service-worker/__tests__/session.test.ts` covers `clearCurrent()` with both the no-op (no current handle) and the propagating-throw cases. `npm test` from `extension/` passes.
- [ ] `crates/relicario-core/src/recovery_qr.rs` has a module-level `//!` header explaining format + domain-separation + parameter-pinning, an ASCII layout diagram next to the magic constants, doc-comments on `RecoveryQrPayload`, `generate_recovery_qr`, `unwrap_recovery_qr`, and `recovery_qr_to_svg`, plus either a `const RECOVERY_PRODUCTION_PARAMS` or a doc-commented `production_params()` explaining why it deliberately diverges from `KdfParams::default()`. `cargo test -p relicario-core` still passes.
- [ ] `tools/relay/queue.test.ts:54-55` verified to already match the new role union (no change required, recorded in PR description).
- [ ] `tools/relay/start.sh` discovers a `*-dev-c-prompt.md`, opens a fourth window/terminal in tmux and kitty modes, and the manual-mode banner mentions all four roles. `bash tools/relay/start.sh --manual` shows "Open 4 new terminals" and lists the Dev-C prompt path.