From 3caa7af194848eb77355a24e8371c8bbeafefb24 Mon Sep 17 00:00:00 2001 From: adlee-was-taken Date: Sat, 2 May 2026 16:03:53 -0400 Subject: [PATCH] docs(plan): v0.5.0 plans A/B and doc audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan A (Rust + docs): S1 pre-receive hook fix, S2 tar path-traversal hardening, S3 RELICARIO_* env-var audit, C1 stale branch cleanup. ~9 tasks, ~50 steps. Plan B (extension UX): P4 error-copy centralization (subsumes B2), B1 strength-meter regenerate fix, P1 password coloring (inlined), P3 form-layout envelope, P2 setup → fullscreen tab. ~15 tasks, ~85 steps. Doc audit: 14 findings, 6 fixed inline (README, ARCHITECTURE, overview), 8 proposed for v0.5.0 release prep. Co-Authored-By: Claude Opus 4.7 --- .../audits/2026-05-02-doc-audit.md | 169 ++ ...26-05-02-v0.5.0-plan-a-security-cleanup.md | 1528 +++++++++++++++ .../2026-05-02-v0.5.0-plan-b-extension-ux.md | 1654 +++++++++++++++++ 3 files changed, 3351 insertions(+) create mode 100644 docs/superpowers/audits/2026-05-02-doc-audit.md create mode 100644 docs/superpowers/plans/2026-05-02-v0.5.0-plan-a-security-cleanup.md create mode 100644 docs/superpowers/plans/2026-05-02-v0.5.0-plan-b-extension-ux.md diff --git a/docs/superpowers/audits/2026-05-02-doc-audit.md b/docs/superpowers/audits/2026-05-02-doc-audit.md new file mode 100644 index 0000000..43be59b --- /dev/null +++ b/docs/superpowers/audits/2026-05-02-doc-audit.md @@ -0,0 +1,169 @@ +# Documentation Audit — 2026-05-02 + +Pre-v0.5.0 audit of Relicario's documentation against the current codebase. + +## Summary + +- **Total findings:** 14 +- **Fixed inline:** 6 +- **Need user input (proposed only):** 8 +- **Top 3 recommendations:** + 1. **Add `relicario-server` to architecture docs.** It exists in `crates/`, is referenced by SECURITY.md, and underpins device-auth, but `docs/architecture/overview.md`'s "three codebases" framing and `CLAUDE.md`'s project-structure tree still pretend it doesn't exist (Findings 1, 2, 9). This is the single biggest gap before tagging v0.5.0. + 2. **Replace `CLAUDE.md`'s Roadmap.** It still says "Next: WASM build + Chrome MV3 browser extension (Plan 2)" — a milestone that shipped weeks ago. Multiple subsequent train rounds (typed items, attachments, backup, LastPass, device auth, fullscreen UX phases) have shipped, none of which are reflected (Finding 3). + 3. **Rewrite the `docs/ARCHITECTURE.md` "Crate Architecture" + "Vault Creation Flow" sections** so they describe the v0.5.0 surface (typed items, settings.enc, device auth boundary, server crate, extension WASM) rather than the v0.1.0 freeze (Finding 10). + +The codebase itself is well-documented — `crates/{relicario-core,relicario-cli}/ARCHITECTURE.md`, `extension/ARCHITECTURE.md`, and `docs/architecture/overview.md` are detailed and current. The drift is concentrated in **the top-level entry-point docs** (`README.md`, `CLAUDE.md`, `docs/ARCHITECTURE.md`) and in the SECURITY.md / overview.md edges. + +--- + +## Findings + +### Finding 1 — `relicario-server` crate is invisible in cross-codebase docs + +**File:** `docs/architecture/overview.md` (lines 14–48 — "The three codebases" + table) +**Issue:** The repo now has **four** Rust crates (`relicario-core`, `relicario-cli`, `relicario-wasm`, `relicario-server`) plus the extension. The framing "The three codebases" + accompanying ASCII diagram + four-row table all predate the May 2026 server crate. `relicario-server` is the pre-receive hook binary that enforces device-signature verification — load-bearing for the device-auth model that SECURITY.md already advertises. +**Fix:** Re-title the section ("The four codebases" or "The relicario codebases"), add a server box to the diagram, add a row to the table. The role is "Pre-receive Git hook that verifies commit signatures against `.relicario/devices.json` and `.relicario/revoked.json`". +**Severity:** must-fix-before-v0.5.0 +**Status:** Proposed; needs user decision (>50 words of new prose; touches the framing of the whole overview doc) + +--- + +### Finding 2 — `CLAUDE.md` project-structure tree omits `relicario-server` + +**File:** `CLAUDE.md` (lines 26–54) +**Issue:** The `crates/` tree only lists `relicario-core/`, `relicario-cli/`, `relicario-wasm/`. `relicario-server/` is missing. Since CLAUDE.md is the project-level summary every Claude session reads, this is the highest-leverage staleness. +**Fix:** Add a fourth crate entry for `relicario-server/` with `src/main.rs # pre-receive hook: verify_commit + generate_hook`. +**Severity:** must-fix-before-v0.5.0 +**Status:** Proposed; needs user decision (CLAUDE.md is user-controlled per audit constraints) + +--- + +### Finding 3 — `CLAUDE.md` Roadmap is severely stale + +**File:** `CLAUDE.md` (lines 91–93) +**Issue:** Says `Next: WASM build + Chrome MV3 browser extension (Plan 2). Then mobile (Rust core compiles to ARM).` Plan 2 (extension) shipped, then Plans 1A-1C, 3A (backup), 3B (LastPass), Plan 4 (security fixes + device auth), and Phases 1-2B of the fullscreen UX redesign all shipped. The current "next thing" per project memory is v0.5.0 polish + harden plus Phase 3/4 of fullscreen UX. +**Fix:** Replace with a current-state Roadmap line (e.g. `Next: v0.5.0 polish + harden, then Phase 3 (vault tab shell). Mobile (ARM) and recovery QR remain on the roadmap.`). +**Severity:** must-fix-before-v0.5.0 +**Status:** Proposed; needs user decision (CLAUDE.md is user-controlled; phrasing is a judgment call) + +--- + +### Finding 4 — `CLAUDE.md` says "Item IDs are random 8-char hex" + +**File:** `CLAUDE.md` (line 79) +**Issue:** Audit M8 bumped `ItemId`/`FieldId` to 16-char hex (64 bits). Verified against `crates/relicario-core/src/ids.rs:3-4, 35-37` and `tests/integration` — they're 16 hex chars. The same line also doesn't mention that `AttachmentId` was bumped to 32 hex chars / 128 bits (audit I2/B4). +**Fix:** Change to: `Item IDs and Field IDs are random 16-char hex strings (64 bits of OsRng entropy). AttachmentIds are content-addressed: first 32 hex chars of SHA-256(plaintext) (128 bits, audit I2/B4).` +**Severity:** must-fix-before-v0.5.0 +**Status:** Proposed; needs user decision (CLAUDE.md is user-controlled) + +--- + +### Finding 5 — `docs/architecture/overview.md` conventions table also says "8-char hex" + +**File:** `docs/architecture/overview.md` (line 180) +**Issue:** Same M8 bump; the conventions table at line 180 said `Item IDs are random 8-char hex`. +**Fix:** Update to 16-char hex / 64 bits, and bump the AttachmentId row to mention the 128-bit width. +**Severity:** must-fix-before-v0.5.0 +**Status:** Fixed inline in `docs/architecture/overview.md` + +--- + +### Finding 6 — `README.md` uses obsolete `entries/` directory layout + +**File:** `README.md` (lines 36, 117–118, 147–149) +**Issue:** References to `entries/*.enc` and the `entry.rs` module are pre-typed-items vocabulary. The on-disk layout is now `items/.enc` + `attachments//.enc` + `settings.enc`; the core module is `item.rs` + `item_types/`. The README is the security proof and the first thing visitors read — getting the on-disk shape wrong hurts the legibility-as-security pitch. +**Fix:** Replace `entries/` with `items/`. Add `settings.enc`, `attachments//.enc`, and (for device-auth) `revoked.json`. Rewrite the `crates/` tree to match the actual seven-module shape and add `relicario-wasm` and `relicario-server`. Update item-ID width to 16-char hex. +**Severity:** must-fix-before-v0.5.0 +**Status:** Fixed inline in `README.md` + +--- + +### Finding 7 — `README.md` Roadmap lists shipped features as upcoming + +**File:** `README.md` (lines 184–192) +**Issue:** All of these are checked off in real life but unchecked in the doc: WASM/Chrome extension, secure notes, secure document storage, LastPass import, Firefox extension. Only Bitwarden/1Password import, the unlock daemon, mobile, and Safari are still unstarted. +**Fix:** Mark the shipped items as `[x]`; add Firefox WebExtension, typed items, backup/restore, LastPass CSV import, and device authentication as completed; keep Bitwarden/1Password import, unlock daemon, mobile, Safari as open. +**Severity:** must-fix-before-v0.5.0 +**Status:** Fixed inline in `README.md` + +--- + +### Finding 8 — `docs/ARCHITECTURE.md` ASCII vault layout uses `entries/` and lacks settings/attachments/revoked + +**File:** `docs/ARCHITECTURE.md` (lines 45–53) +**Issue:** Same staleness as README Finding 6. The "GIT SERVER (untrusted)" box shows only `manifest.enc` + `entries/.enc` + `.relicario/{salt,params.json,devices.json}`. Missing: `settings.enc`, `attachments//.enc`, `revoked.json`. ID lengths are 8-char hex (`a1b2c3d4`) instead of 16-char hex. +**Fix:** Update box to current layout including settings.enc, attachments tree, revoked.json, and 16-char IDs. +**Severity:** must-fix-before-v0.5.0 +**Status:** Fixed inline in `docs/ARCHITECTURE.md` + +--- + +### Finding 9 — `docs/ARCHITECTURE.md` "Crate Architecture" omits wasm + server crates + +**File:** `docs/ARCHITECTURE.md` (lines 208–235) +**Issue:** The bottom box of the "Crate Architecture" diagram says `Future: relicario-wasm wraps this for browser extension` and `Future: JNI/Swift wrappers for Android/iOS`. WASM is no longer future — it shipped. The `relicario-server` crate isn't mentioned at all. The `relicario-core` module list inside the box still says `entry / Manifest / search`, predating the typed-items rewrite (`item`, `item_types/`, `settings`, `attachment`, `backup`, `device`). +**Fix:** Replace the inner-box module names with the current set; remove the "Future: relicario-wasm" line and add a "Consumed by" line listing all three downstream crates including server. +**Severity:** must-fix-before-v0.5.0 +**Status:** Fixed inline in `docs/ARCHITECTURE.md` + +--- + +### Finding 10 — `docs/ARCHITECTURE.md` "Vault Creation Flow" doesn't reflect typed-items or settings.enc + +**File:** `docs/ARCHITECTURE.md` (lines 60–89) +**Issue:** The vault-creation pipeline in this doc shows `master_key → XChaCha20-Poly1305 → manifest.enc` only. In reality `cmd_init` also encrypts and writes `settings.enc` (default `VaultSettings`). Field-history-tracked items, attachments, the `Item` envelope shape — none of these are in the flow doc. Without context on typed items, a new contributor reading this doc would have a v0.1-era model of the system. +**Fix:** Add a settings.enc step to the flow; either expand the items section or note that the full item lifecycle is in `crates/relicario-core/ARCHITECTURE.md`. +**Severity:** nice-to-have (the per-codebase ARCHITECTURE.md files are the source of truth; this top-level doc could just point at them) +**Status:** Proposed; needs user decision (>50 words of new prose, design choice between rewriting vs trimming) + +--- + +### Finding 11 — `docs/SECURITY.md` "Device registration was optional before v0.4.0" is undated/misleading + +**File:** `docs/SECURITY.md` (lines 60–62) +**Issue:** Says `Device registration was optional before v0.4.0. With device auth enabled, all commits must be signed by a registered device.` But (a) v0.4.0 hasn't been tagged yet — the changelog goes v0.1.0 → v0.2.0 → "Unreleased", and the next tag-in-flight per project memory is v0.5.0; (b) per the v0.5.0 polish + harden spec, device-auth enforcement is **currently a no-op** because the pre-receive hook fix (S1) hasn't landed. Saying "all commits MUST be signed" is aspirational, not current. +**Fix:** Reword to clarify (a) the actual version line (e.g. "Pre-v0.5.0 vaults can opt out by leaving `devices.json` empty"), AND (b) acknowledge that signature *enforcement* depends on the pre-receive hook being deployed and the S1 fix landing. Could just be a one-line caveat. +**Severity:** must-fix-before-v0.5.0 (security-doc accuracy is part of the legibility pitch) +**Status:** Proposed; needs user decision (security wording — exact phrasing matters) + +--- + +### Finding 12 — `docs/SECURITY.md` doesn't mention `relicario-server` + +**File:** `docs/SECURITY.md` (lines 44–51) +**Issue:** The "Device Authentication" section refers to a "pre-receive hook" but never says it lives in `crates/relicario-server`, what binary the hook calls (`relicario-server verify-commit `), or how to install it (`relicario-server generate-hook`). For a self-hosted user reading this to decide whether to enable it, those are the two essential operational facts. +**Fix:** Add a short paragraph naming the crate and the two subcommands, pointing to the design spec. +**Severity:** nice-to-have +**Status:** Proposed; needs user decision (>50 words of new prose) + +--- + +### Finding 13 — Foundational design spec's "Post-V1 Ideas" lists shipped features + +**File:** `docs/superpowers/specs/2026-04-11-relicario-design.md` (lines 351–361) +**Issue:** This doc is explicitly historical (per `docs/architecture/overview.md` "Stale spec docs" disclaimer), so editing it as architecture would violate convention. Still worth flagging that "Post-V1 Ideas" lists secure notes, secure documents, mobile, LastPass import, Firefox extension, TOTP — most of which have shipped. Per project policy this is *informational only*; the spec is a time-stamped decision artifact. +**Fix:** None — leave alone. If desired, prepend a one-line "Status: V1 shipped 2026-04-22; many Post-V1 ideas have since landed — see CHANGELOG.md" at the top of the file. +**Severity:** informational +**Status:** Proposed; needs user decision (touches a historical spec the user may want to leave frozen) + +--- + +### Finding 14 — Lowercase "relicario" in prose contexts + +**File:** `README.md` (line 67), `docs/ARCHITECTURE.md` (none found in prose), `docs/architecture/overview.md` (none found in prose), `docs/SECURITY.md` (none found in prose) +**Issue:** Per CLAUDE.md, "Relicario" should be capitalized in prose. A search across the audit-scope docs finds no uppercase-violations — most prose lowercase usages are in code paths (`.relicario/`, `relicario init`, `relicario-core`) which are correctly lowercase per the rule. The README at line 67 ("Relicario generates unique passwords per site") is correctly capitalized; line 26 ("Relicario embeds a random 256-bit secret") is correct. **No lowercase prose occurrences found.** This finding is "checked, no action needed." +**Fix:** N/A +**Severity:** informational +**Status:** No action needed (recorded for completeness) + +--- + +## Inline-fix verification + +Files modified during this audit: + +- `README.md` — vault layout (`items/`, `settings.enc`, `attachments/`), crate tree (added `relicario-wasm`, `relicario-server`, typed-items modules), ID width, Roadmap. +- `docs/ARCHITECTURE.md` — git-server box (`items/`, `settings.enc`, `attachments/`, `revoked.json`), crate-architecture inner box (current core modules), removed "Future: relicario-wasm" line. +- `docs/architecture/overview.md` — conventions table (16-char hex IDs, 128-bit AttachmentIds). + +No source files, `Cargo.lock`, or extension code were modified. CLAUDE.md, SECURITY.md, and the foundational design spec were not modified — those changes need user review per the audit constraints. diff --git a/docs/superpowers/plans/2026-05-02-v0.5.0-plan-a-security-cleanup.md b/docs/superpowers/plans/2026-05-02-v0.5.0-plan-a-security-cleanup.md new file mode 100644 index 0000000..661dd13 --- /dev/null +++ b/docs/superpowers/plans/2026-05-02-v0.5.0-plan-a-security-cleanup.md @@ -0,0 +1,1528 @@ +# Plan A — v0.5.0 Polish + Harden — Security + Cleanup (Rust + Docs) + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. TDD discipline throughout: write a failing test, run to confirm fail, implement, run to confirm pass, then commit. + +**Goal:** Land the four Rust + docs items in the v0.5.0 polish-harden bundle: fix the broken `relicario-server` pre-receive hook (S1, the security anchor), harden the backup-restore tar unpack against path-traversal/symlink/tar-bomb attacks (S2), audit and document `RELICARIO_*` env vars while moving the dev-only one under `cfg(debug_assertions)` (S3), and prune merged local feature branches with per-branch user confirmation (C1). Plan A's scope is bounded — items B1, B2, P1-P4 belong to Plan B (Extension UX) and are explicitly out of scope here. + +**Architecture:** S1 rewrites `verify_commit` to load `devices.json` and `revoked.json` at the commit, build a temporary `gpg.ssh.allowedSignersFile`, run `git verify-commit --raw` with `GIT_CONFIG_*` env to point at that file, parse the signing-key SHA256 fingerprint out of the `Good "git" signature for relicario with ED25519 key SHA256:...` line on stderr, and accept only when the fingerprint matches a registered, non-revoked device (using committer-date for the historical-commit revocation window). S2 swaps `tar::Archive::unpack` for an explicit per-entry loop with `..` / absolute / drive-letter rejection, symlink and hardlink rejection, a 100×-or-1-GiB size cap, and a final canonicalised `starts_with(target)` check. S3 documents every `RELICARIO_*` env var in `docs/SECURITY.md` and the relevant clap doc-comments and `cfg(debug_assertions)`-gates `RELICARIO_NO_GROUPS_CACHE`. C1 walks the five known-merged feature branches one-by-one, confirms each is in `git branch --merged main` output, and prompts the user before each `git branch -D`. + +**Tech Stack:** Rust (`relicario-core`, `relicario-cli`, `relicario-server`), `ssh-key` crate (already a `relicario-core` dependency) for fingerprint computation, `tar` crate (already a `relicario-cli` dependency) iterated by hand, `tempfile` (already a `relicario-cli` dev-dependency, mirror to `relicario-server`), `assert_cmd` (already in `relicario-cli`'s dev-dependencies, mirror to `relicario-server`). + +--- + +## File Structure + +| File | Change | Purpose | +|------|--------|---------| +| `crates/relicario-core/src/device.rs` | Modify | Add `pub fn fingerprint(public_key_openssh: &str) -> Result` returning `SHA256:` (S1 needs this; keep in core so server + tests share it) | +| `crates/relicario-core/src/lib.rs` | Modify | Re-export `device::fingerprint` | +| `crates/relicario-server/Cargo.toml` | Modify | Add `tempfile`, `assert_cmd`, `predicates` to `[dev-dependencies]` | +| `crates/relicario-server/src/main.rs` | Rewrite `verify_commit` (S1) | Allowed-signers temp file, fingerprint extraction, devices/revoked check | +| `crates/relicario-server/tests/verify_commit.rs` | Create | 4 acceptance integration tests for S1 | +| `crates/relicario-cli/src/main.rs` | Modify (S2) | Replace `archive.unpack(...)` at line 1722 with `safe_unpack_git_archive` call; add the helper above `cmd_backup_restore` | +| `crates/relicario-cli/src/helpers.rs` | Modify (S3) | `cfg(debug_assertions)`-gate the `RELICARIO_NO_GROUPS_CACHE` check; document | +| `crates/relicario-cli/src/main.rs` | Modify (S3) | Update the doc-comment on `cmd_groups`/CLI long-help mentioning the env var to note it's debug-only | +| `crates/relicario-cli/tests/backup.rs` (or new `crates/relicario-cli/tests/restore_hardening.rs`) | Modify/Create | 3 S2 acceptance tests + happy-path stays green | +| `docs/SECURITY.md` | Modify (S3) | Add a "Configuration env vars" section enumerating every `RELICARIO_*` var with purpose + trust assumption | +| _local git branches_ | Delete (C1) | Five merged feature branches, interactive | + +--- + +## Task 1: S1 — Add `device::fingerprint` helper to relicario-core + +**Files:** +- Modify: `crates/relicario-core/src/device.rs` +- Modify: `crates/relicario-core/src/lib.rs` + +The `relicario-server` `verify_commit` rewrite needs to compute SHA256 fingerprints from OpenSSH public keys (so it can match `git verify-commit --raw` stderr output against `devices.json` / `revoked.json` entries). The `ssh-key` crate already exposes this via `PublicKey::fingerprint(HashAlg::Sha256)`. We add it to core so the server crate (and unit tests) share the same implementation. + +- [ ] **Step 1: Write failing unit test for `fingerprint`** + +Add to `crates/relicario-core/src/device.rs` `tests` module: + +```rust +#[test] +fn fingerprint_matches_ssh_keygen_format() { + // Generate a key, compute fingerprint, and confirm format. + let (_, public) = generate_keypair().unwrap(); + let fp = fingerprint(&public).unwrap(); + // Format from ssh-keygen -l: "SHA256:<43 chars of url-safe base64 without padding>" + assert!(fp.starts_with("SHA256:"), "fingerprint should start with SHA256: prefix, got {fp}"); + let body = fp.strip_prefix("SHA256:").unwrap(); + assert_eq!(body.len(), 43, "SHA-256 fingerprint body is 43 base64 chars (no padding)"); + assert!(body.chars().all(|c| c.is_ascii_alphanumeric() || c == '+' || c == '/')); +} + +#[test] +fn fingerprint_is_deterministic() { + let (_, public) = generate_keypair().unwrap(); + assert_eq!(fingerprint(&public).unwrap(), fingerprint(&public).unwrap()); +} + +#[test] +fn fingerprint_differs_per_key() { + let (_, p1) = generate_keypair().unwrap(); + let (_, p2) = generate_keypair().unwrap(); + assert_ne!(fingerprint(&p1).unwrap(), fingerprint(&p2).unwrap()); +} +``` + +- [ ] **Step 2: Run test to confirm fail** + +Run: `cargo test -p relicario-core device::tests::fingerprint` +Expected: compilation error — `fingerprint` is not defined. + +- [ ] **Step 3: Implement `fingerprint`** + +Add to `crates/relicario-core/src/device.rs`: + +```rust +/// Compute the OpenSSH SHA-256 fingerprint of a public key, formatted exactly as +/// `ssh-keygen -lf` and as `git verify-commit --raw` reports it: `SHA256:` +/// (standard base64 without padding). +pub fn fingerprint(public_key_openssh: &str) -> Result { + use ssh_key::HashAlg; + let public = PublicKey::from_openssh(public_key_openssh) + .map_err(|e| RelicarioError::DeviceKey(format!("parse public key: {e}")))?; + Ok(public.fingerprint(HashAlg::Sha256).to_string()) +} +``` + +- [ ] **Step 4: Re-export from lib.rs** + +In `crates/relicario-core/src/lib.rs`, find the existing `pub use device::...` line and add `fingerprint`: + +```rust +pub use device::{fingerprint, generate_keypair, sign, verify, DeviceEntry, RevokedEntry}; +``` + +- [ ] **Step 5: Run tests** + +Run: `cargo test -p relicario-core device` +Expected: all device tests pass including the three new fingerprint tests. + +- [ ] **Step 6: Cross-check fingerprint format against ssh-keygen** + +Sanity-check that the format matches what `git verify-commit --raw` actually emits (we captured this during planning — see `crates/relicario-server/tests/verify_commit.rs` Step 2 below where it gets exercised end-to-end). If the `ssh-key` crate's `Fingerprint::Display` emits something different from the `SHA256:NX7cg...` form, this is the place to find out. Format note for the engineer: ssh-keygen omits trailing `=` padding from base64, exactly 43 chars for SHA-256. + +- [ ] **Step 7: Commit** + +```bash +git add crates/relicario-core/src/device.rs crates/relicario-core/src/lib.rs +git commit -m "$(cat <<'EOF' +feat(core): add device::fingerprint helper for SSH SHA256 fingerprints + +Wraps `ssh-key`'s `PublicKey::fingerprint(HashAlg::Sha256)`. Output +format matches `ssh-keygen -lf` and `git verify-commit --raw` stderr +("SHA256:<43-char base64>"). Used by the upcoming relicario-server +verify-commit rewrite (audit S1). + +Co-Authored-By: Claude Opus 4.7 +EOF +)" +``` + +--- + +## Task 2: S1 — Add dev-dependencies to relicario-server + +**Files:** +- Modify: `crates/relicario-server/Cargo.toml` + +The S1 acceptance tests need `assert_cmd` to spawn the binary and `tempfile` to build throwaway repos. Both are already used in `relicario-cli` — mirror the versions to keep workspace dependency drift minimal. + +- [ ] **Step 1: Confirm versions used by `relicario-cli`** + +Read `crates/relicario-cli/Cargo.toml` `[dev-dependencies]`. As of writing: `assert_cmd = "2"`, `predicates = "3"`, `tempfile = "3"`. If any have changed, use the values currently in that file. + +- [ ] **Step 2: Add to relicario-server Cargo.toml** + +Edit `crates/relicario-server/Cargo.toml`. Append (or merge if section exists): + +```toml +[dev-dependencies] +assert_cmd = "2" +predicates = "3" +tempfile = "3" +``` + +- [ ] **Step 3: Verify it builds** + +Run: `cargo build -p relicario-server --tests` +Expected: succeeds (no tests yet, just confirms manifest is valid). + +- [ ] **Step 4: Commit** + +```bash +git add crates/relicario-server/Cargo.toml +git commit -m "$(cat <<'EOF' +chore(server): add assert_cmd/predicates/tempfile dev-deps + +Needed for the upcoming verify-commit acceptance suite (audit S1). + +Co-Authored-By: Claude Opus 4.7 +EOF +)" +``` + +--- + +## Task 3: S1 — Write the four failing acceptance tests + +**Files:** +- Create: `crates/relicario-server/tests/verify_commit.rs` + +These tests drive the implementation. They build a real on-disk git repo, generate two ed25519 keypairs, write `.relicario/devices.json` and `.relicario/revoked.json`, sign commits with `git -c gpg.format=ssh ...`, then invoke the `relicario-server verify-commit ` binary via `assert_cmd` and assert exit code + stderr. + +**Reference output formats** (captured from a real `git -c gpg.format=ssh ...` repo during planning): + +- Signed by allowed key, exit 0: + `Good "git" signature for relicario with ED25519 key SHA256:NX7cgViq2JvCRDYWVun/kiGYDbl5yyNKApg/L5e/ixU` +- Signed by non-allowed key (still has GOODSIG line!), exit 1: + `Good "git" signature with ED25519 key SHA256:BMyEafi4tdXsSdaOwCqdLXq25Xe3I/LzZWG63WCvfBE` + `No principal matched.` +- Unsigned commit: empty stderr, exit non-zero from `git verify-commit`. + +The current code's `stderr.contains("Good signature")` check is doubly wrong: SSH signatures emit `Good "git" signature ...`, AND that line shows up even for unknown keys. We need fingerprint matching against `devices.json`. + +- [ ] **Step 1: Create the test file with helpers** + +Write `crates/relicario-server/tests/verify_commit.rs`: + +```rust +//! Acceptance tests for `relicario-server verify-commit`. +//! +//! These build real on-disk git repos with SSH-signed commits and invoke the +//! binary via `assert_cmd`. They cover the four scenarios from the audit S1 +//! acceptance criteria: +//! +//! 1. Revoked key, commit dated AFTER `revoked_at` -> exit 1 +//! 2. Unregistered key -> exit 1 +//! 3. Registered, non-revoked key -> exit 0 +//! 4. Revoked key, commit dated BEFORE `revoked_at` (historical) -> exit 0 + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use assert_cmd::Command as AssertCommand; +use relicario_core::device::{generate_keypair, DeviceEntry, RevokedEntry}; +use tempfile::TempDir; + +/// Write a private OpenSSH key + matching .pub file into `dir`, returning +/// (private_path, public_path, public_openssh_string). +fn write_keypair(dir: &Path, name: &str) -> (PathBuf, PathBuf, String) { + let (priv_pem, pub_line) = generate_keypair().expect("generate keypair"); + let priv_path = dir.join(format!("{name}.key")); + let pub_path = dir.join(format!("{name}.pub")); + fs::write(&priv_path, priv_pem.as_str()).unwrap(); + fs::write(&pub_path, &pub_line).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&priv_path, fs::Permissions::from_mode(0o600)).unwrap(); + } + (priv_path, pub_path, pub_line) +} + +/// Run `git` in `repo` with the given args; panic on non-zero exit. +fn git(repo: &Path, args: &[&str], extra_env: &[(&str, &str)]) { + let mut cmd = Command::new("git"); + cmd.current_dir(repo).args(args); + for (k, v) in extra_env { + cmd.env(k, v); + } + let status = cmd.status().expect("spawn git"); + assert!(status.success(), "git {args:?} failed"); +} + +/// Initialise a git repo with an empty initial commit (unsigned, bootstrap). +fn init_repo(repo: &Path) { + git(repo, &["init", "-q", "-b", "main"], &[]); + git(repo, &["config", "user.email", "test@test"], &[]); + git(repo, &["config", "user.name", "test"], &[]); + git(repo, &["commit", "--allow-empty", "-q", "-m", "init"], &[]); +} + +/// Sign a commit with `signing_key` against `allowed_signers` and a fixed +/// committer date (unix timestamp). Returns the new commit SHA. +fn sign_commit( + repo: &Path, + signing_key: &Path, + allowed_signers: &Path, + committer_unix: i64, + msg: &str, + file_path: &str, + file_content: &str, +) -> String { + fs::write(repo.join(file_path), file_content).unwrap(); + git(repo, &["add", file_path], &[]); + let date = format!("@{committer_unix} +0000"); + git( + repo, + &[ + "-c", "gpg.format=ssh", + "-c", &format!("user.signingkey={}", signing_key.display()), + "-c", &format!("gpg.ssh.allowedSignersFile={}", allowed_signers.display()), + "commit", "-S", "-q", "-m", msg, + ], + &[ + ("GIT_AUTHOR_DATE", &date), + ("GIT_COMMITTER_DATE", &date), + ], + ); + let out = Command::new("git") + .current_dir(repo) + .args(["rev-parse", "HEAD"]) + .output() + .unwrap(); + String::from_utf8(out.stdout).unwrap().trim().to_string() +} + +/// Write `.relicario/devices.json` and `.relicario/revoked.json`, commit, return SHA. +/// The commit itself is unsigned/bootstrap so we don't recurse into verification. +fn write_device_files( + repo: &Path, + devices: &[DeviceEntry], + revoked: &[RevokedEntry], +) { + let dir = repo.join(".relicario"); + fs::create_dir_all(&dir).unwrap(); + fs::write( + dir.join("devices.json"), + serde_json::to_string_pretty(devices).unwrap(), + ).unwrap(); + fs::write( + dir.join("revoked.json"), + serde_json::to_string_pretty(revoked).unwrap(), + ).unwrap(); + git(repo, &["add", ".relicario"], &[]); + git(repo, &["commit", "-q", "-m", "device files"], &[]); +} + +#[test] +fn registered_non_revoked_key_accepted() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path(); + init_repo(repo); + + let (priv_a, _pub_path_a, pub_a) = write_keypair(repo, "alice"); + write_device_files( + repo, + &[DeviceEntry { + name: "alice".into(), + public_key: pub_a.clone(), + added_at: 1_700_000_000, + added_by: "bootstrap".into(), + }], + &[], + ); + + // Allowed-signers file used only by the test fixture's git invocation + // (the binary builds its own at verify time). + let allowed = repo.join("test_allowed_signers"); + fs::write(&allowed, format!("relicario {}\n", pub_a.trim())).unwrap(); + + let sha = sign_commit(repo, &priv_a, &allowed, 1_710_000_000, "x", "a.txt", "hi"); + + AssertCommand::cargo_bin("relicario-server") + .unwrap() + .current_dir(repo) + .args(["verify-commit", &sha]) + .assert() + .success(); +} + +#[test] +fn unregistered_key_rejected() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path(); + init_repo(repo); + + let (_priv_a, _, pub_a) = write_keypair(repo, "alice"); + let (priv_evil, _, pub_evil) = write_keypair(repo, "evil"); + + // Only Alice is registered. + write_device_files( + repo, + &[DeviceEntry { + name: "alice".into(), + public_key: pub_a.clone(), + added_at: 1_700_000_000, + added_by: "bootstrap".into(), + }], + &[], + ); + + // Evil signs against an allowed-signers file containing both keys (so + // git verify-commit emits a "Good" line); the binary should still reject + // because evil isn't in devices.json. + let allowed = repo.join("test_allowed_signers"); + fs::write( + &allowed, + format!("relicario {}\nrelicario {}\n", pub_a.trim(), pub_evil.trim()), + ).unwrap(); + + let sha = sign_commit(repo, &priv_evil, &allowed, 1_710_000_000, "evil", "a.txt", "hi"); + + AssertCommand::cargo_bin("relicario-server") + .unwrap() + .current_dir(repo) + .args(["verify-commit", &sha]) + .assert() + .failure() + .stderr(predicates::str::contains("unregistered")); +} + +#[test] +fn revoked_key_after_revoked_at_rejected() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path(); + init_repo(repo); + + let (priv_a, _, pub_a) = write_keypair(repo, "alice"); + + // Alice was registered, then revoked at t=1_705_000_000. + write_device_files( + repo, + &[], + &[RevokedEntry { + name: "alice".into(), + public_key: pub_a.clone(), + revoked_at: 1_705_000_000, + revoked_by: "admin".into(), + }], + ); + + let allowed = repo.join("test_allowed_signers"); + fs::write(&allowed, format!("relicario {}\n", pub_a.trim())).unwrap(); + + // Commit dated AFTER revocation. + let sha = sign_commit(repo, &priv_a, &allowed, 1_710_000_000, "post", "a.txt", "hi"); + + AssertCommand::cargo_bin("relicario-server") + .unwrap() + .current_dir(repo) + .args(["verify-commit", &sha]) + .assert() + .failure() + .stderr(predicates::str::contains("revoked")); +} + +#[test] +fn revoked_key_before_revoked_at_accepted_historical() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path(); + init_repo(repo); + + let (priv_a, _, pub_a) = write_keypair(repo, "alice"); + + // Devices.json (current state) lists Alice as revoked. + write_device_files( + repo, + &[], + &[RevokedEntry { + name: "alice".into(), + public_key: pub_a.clone(), + revoked_at: 1_705_000_000, + revoked_by: "admin".into(), + }], + ); + + let allowed = repo.join("test_allowed_signers"); + fs::write(&allowed, format!("relicario {}\n", pub_a.trim())).unwrap(); + + // Commit dated BEFORE revocation -- historical-commit case. + let sha = sign_commit(repo, &priv_a, &allowed, 1_700_000_000, "historical", "a.txt", "hi"); + + AssertCommand::cargo_bin("relicario-server") + .unwrap() + .current_dir(repo) + .args(["verify-commit", &sha]) + .assert() + .success(); +} +``` + +- [ ] **Step 2: Run the suite to confirm all four fail** + +Run: `cargo test -p relicario-server --test verify_commit` +Expected: all four tests fail. The current `verify_commit` accepts everything that has a "Good signature" or "GOODSIG" string — `unregistered_key_rejected` and `revoked_key_after_revoked_at_rejected` will pass on a passing-everything implementation (which is the bug), but they assert specific stderr substrings that the current code never emits, so they will fail. `registered_non_revoked_key_accepted` and `revoked_key_before_revoked_at_accepted_historical` may or may not pass coincidentally — both should be checked under the *new* implementation. + +NOTE: do not commit yet — these are paired with the implementation in Task 4. + +--- + +## Task 4: S1 — Implement `verify_commit` + +**Files:** +- Modify: `crates/relicario-server/src/main.rs` + +Replace the body of `verify_commit` with the real check. The flow: + +1. Load `devices.json` at the commit (existing logic). +2. Bootstrap: empty/missing -> accept. +3. Load `revoked.json` at the commit (existing logic, unwrap_or_default). +4. Build a temp file `/allowed_signers` containing one line per device entry: `relicario ` (newline-terminated). +5. Spawn `git verify-commit --raw ` with env `GIT_CONFIG_COUNT=1`, `GIT_CONFIG_KEY_0=gpg.ssh.allowedSignersFile`, `GIT_CONFIG_VALUE_0=/allowed_signers`. Capture stderr. +6. If exit code is non-zero -> reject ("git verify-commit failed: "). +7. Parse the SHA256 fingerprint out of stderr. The line we want is `Good "git" signature for relicario with ED25519 key SHA256:`. Match a regex like `key (SHA256:[A-Za-z0-9+/]+)` — first capture group. +8. Build a fingerprint-to-name map from `devices` via `relicario_core::device::fingerprint` (skip entries that fail to parse, with a warning). +9. If the parsed fingerprint isn't in the map -> reject ("signed by unregistered device "). +10. Get committer date: `git show -s --format=%ct `, parse as `i64`. +11. Build a fingerprint -> `revoked_at` map from `revoked` via `fingerprint`. If the parsed fingerprint is in this map AND `committer_ts >= revoked_at` -> reject ("signed by revoked device ''"). +12. Else -> accept. + +The `let _ = &revoked;` dead code goes away. The `devices.contains` check is the new gate. + +- [ ] **Step 1: Add `tempfile` to runtime deps (not just dev)** + +`tempfile` is already a dev-dependency (Task 2) but the binary needs it at runtime. Edit `crates/relicario-server/Cargo.toml` `[dependencies]`: + +```toml +tempfile = "3" +regex = "1" +``` + +(`regex` is needed for fingerprint extraction; alternative is hand-rolled string slicing — regex is clearer.) + +- [ ] **Step 2: Replace `verify_commit` body** + +Edit `crates/relicario-server/src/main.rs`. Replace the entire `verify_commit` function (lines 36-81) with: + +```rust +fn verify_commit(commit: &str) -> Result<()> { + // 1. Load devices.json at this commit (bootstrap-tolerant). + let devices_json = match git_show(commit, ".relicario/devices.json") { + Ok(json) => json, + Err(_) => { + eprintln!("OK: commit {commit} (bootstrap - no devices.json)"); + return Ok(()); + } + }; + let devices: Vec = serde_json::from_str(&devices_json) + .context("parse devices.json")?; + + // 2. Empty devices.json => bootstrap mode. + if devices.is_empty() { + eprintln!("OK: commit {commit} (bootstrap - empty devices.json)"); + return Ok(()); + } + + // 3. Load revoked.json (may not exist). + let revoked: Vec = git_show(commit, ".relicario/revoked.json") + .ok() + .and_then(|s| serde_json::from_str(&s).ok()) + .unwrap_or_default(); + + // 4. Build temp allowed-signers file (one entry per registered device). + let tmp = tempfile::tempdir().context("create tempdir")?; + let allowed_path = tmp.path().join("allowed_signers"); + let mut allowed_body = String::new(); + for d in &devices { + // Format per ssh-keygen(1): principal[,principal...] [options] keytype base64key + // We use "relicario" as the single principal (matches what the CLI configures). + allowed_body.push_str("relicario "); + allowed_body.push_str(d.public_key.trim()); + allowed_body.push('\n'); + } + std::fs::write(&allowed_path, allowed_body).context("write allowed_signers")?; + + // 5. Run git verify-commit --raw with our allowed-signers (NOT mutating + // global git config -- use GIT_CONFIG_COUNT/KEY/VALUE override). + let output = Command::new("git") + .args(["verify-commit", "--raw", commit]) + .env("GIT_CONFIG_COUNT", "1") + .env("GIT_CONFIG_KEY_0", "gpg.ssh.allowedSignersFile") + .env("GIT_CONFIG_VALUE_0", allowed_path.as_os_str()) + .output() + .context("git verify-commit")?; + + let stderr = String::from_utf8_lossy(&output.stderr); + + // 6. Non-zero exit means: unsigned, bad signature, or signed by a key + // not in our allowed-signers file (which is exactly the registered + // device set). All three are rejection cases. + if !output.status.success() { + eprintln!("REJECT: commit {commit} — git verify-commit failed: {}", stderr.trim()); + std::process::exit(1); + } + + // 7. Parse the SHA-256 fingerprint of the signing key from stderr. + // SSH signature line format: + // Good "git" signature for relicario with ED25519 key SHA256: + // (capture the SHA256:... token; it's exactly what device::fingerprint emits.) + let re = regex::Regex::new(r"key (SHA256:[A-Za-z0-9+/]+)") + .expect("static regex"); + let signing_fp = match re.captures(&stderr).and_then(|c| c.get(1)) { + Some(m) => m.as_str().to_string(), + None => { + eprintln!( + "REJECT: commit {commit} — could not parse signing key fingerprint from: {}", + stderr.trim() + ); + std::process::exit(1); + } + }; + + // 8. Build device fingerprint maps. + let mut device_by_fp: std::collections::HashMap = + std::collections::HashMap::new(); + for d in &devices { + match relicario_core::device::fingerprint(&d.public_key) { + Ok(fp) => { device_by_fp.insert(fp, d); } + Err(e) => eprintln!( + "warning: skipping unparseable device entry '{}': {e}", + d.name + ), + } + } + + // 9. Reject if signing key isn't a registered device. + if !device_by_fp.contains_key(&signing_fp) { + eprintln!( + "REJECT: commit {commit} — signed by unregistered device (fingerprint {signing_fp})" + ); + std::process::exit(1); + } + + // 10. Get committer date (NOT author date — committer date is when the + // signature was applied; author date can be backdated by an attacker). + let ct_out = Command::new("git") + .args(["show", "-s", "--format=%ct", commit]) + .output() + .context("git show committer date")?; + if !ct_out.status.success() { + anyhow::bail!("git show committer date failed for {commit}"); + } + let committer_ts: i64 = String::from_utf8_lossy(&ct_out.stdout) + .trim() + .parse() + .context("parse committer timestamp")?; + + // 11. Reject if signing key is revoked AND commit is at-or-after revocation. + // Historical commits (committer_ts < revoked_at) survive the revoke. + for r in &revoked { + let r_fp = match relicario_core::device::fingerprint(&r.public_key) { + Ok(fp) => fp, + Err(_) => continue, + }; + if r_fp == signing_fp && committer_ts >= r.revoked_at { + eprintln!( + "REJECT: commit {commit} — signed by revoked device '{}' \ + (committer ts {committer_ts} >= revoked_at {})", + r.name, r.revoked_at + ); + std::process::exit(1); + } + } + + eprintln!("OK: commit {commit} verified (signed by '{}')", device_by_fp[&signing_fp].name); + Ok(()) +} +``` + +- [ ] **Step 3: Run the four acceptance tests** + +Run: `cargo test -p relicario-server --test verify_commit` +Expected: all four pass. + +- [ ] **Step 4: Run the rest of the workspace tests to confirm no regression** + +Run: `cargo test` +Expected: all tests still pass. + +- [ ] **Step 5: Manual sanity check (optional but good)** + +Build the binary and inspect a known-bad case: + +```bash +cargo build -p relicario-server +# (binary at target/debug/relicario-server) +``` + +Then either run the test fixture by hand or skip — the four tests cover the four spec scenarios. + +- [ ] **Step 6: Commit (S1 implementation + tests together)** + +```bash +git add crates/relicario-server/Cargo.toml \ + crates/relicario-server/src/main.rs \ + crates/relicario-server/tests/verify_commit.rs +git commit -m "$(cat <<'EOF' +fix(server): real signature verification in pre-receive hook (audit S1) + +`verify_commit` previously loaded devices.json/revoked.json and threw +both away, accepting any commit whose stderr contained "GOODSIG" or +"Good signature". This left device registration and revocation as no-ops: +unregistered keys could push, and revoked keys kept working forever. + +The fix: + +- Build a temp `gpg.ssh.allowedSignersFile` from devices.json at the + commit, passed via GIT_CONFIG_COUNT/KEY/VALUE so we never mutate + global git config. +- Run `git verify-commit --raw` and require zero exit. +- Parse `SHA256:` fingerprint out of the SSH "Good" line. +- Reject if fingerprint isn't a registered device. +- Reject if fingerprint is in revoked.json AND committer date >= + revoked_at. Historical commits (committer_ts < revoked_at) still + verify — old history survives revocation. + +Acceptance: 4 integration tests covering the matrix +(registered/unregistered x revoked/non-revoked, with revoked split into +post- and pre-revocation cases). + +Co-Authored-By: Claude Opus 4.7 +EOF +)" +``` + +--- + +## Task 5: S2 — Write failing tests for path-traversal hardening + +**Files:** +- Create: `crates/relicario-cli/tests/restore_hardening.rs` + +The current `cmd_backup_restore` calls `tar::Archive::unpack(target.join(".git"))` blindly. We need three failing tests that craft malicious tar payloads inside a `.relbak`, then assert restore exits with an error and never writes outside the target directory. + +The simplest path: factor the tar-extraction into a `safe_unpack_git_archive(tar_bytes: &[u8], dest: &Path, max_size: u64) -> Result<()>` function and unit-test that directly. Plus one CLI integration test for the happy path. + +- [ ] **Step 1: Decide test layer** + +Two options: +- (A) Pure-Rust unit test of `safe_unpack_git_archive`, hand-crafting tar entries with the `tar::Builder` API. +- (B) End-to-end test: build a malicious `.relbak`, run the CLI restore command, assert error. + +Pick **A** — fewer moving parts, easier to control payload exactly. The happy-path E2E case is already covered by existing backup roundtrip tests. + +- [ ] **Step 2: Create the test file** + +Write `crates/relicario-cli/tests/restore_hardening.rs`: + +```rust +//! Acceptance tests for tar-archive hardening on backup restore (audit S2). +//! +//! Three malicious-input scenarios that the unpacker must refuse: +//! 1. Path traversal: an entry whose path contains `..` +//! 2. Symlink: a symlink entry pointing outside the destination +//! 3. Tar bomb: total uncompressed size exceeds the cap + +use std::io::Write; +use std::path::PathBuf; + +use tar::{Builder, Header}; +use tempfile::TempDir; + +// The function under test lives in the CLI binary crate. Easiest way to +// call it from an integration test: re-export via a shim. See +// `crates/relicario-cli/src/main.rs` -- we add `pub` on the helper and +// expose it through a small `pub mod restore_hardening` module wired +// into `lib.rs`. If `relicario-cli` has no `lib.rs` yet, create one +// re-exporting the function. +use relicario_cli::restore_hardening::safe_unpack_git_archive; + +fn build_traversal_tar() -> Vec { + let mut buf = Vec::new(); + { + let mut b = Builder::new(&mut buf); + let mut h = Header::new_gnu(); + h.set_size(4); + h.set_mode(0o644); + h.set_cksum(); + b.append_data(&mut h, "../../escaped.txt", &b"hax!"[..]).unwrap(); + b.finish().unwrap(); + } + buf +} + +fn build_absolute_path_tar() -> Vec { + let mut buf = Vec::new(); + { + let mut b = Builder::new(&mut buf); + let mut h = Header::new_gnu(); + h.set_size(4); + h.set_mode(0o644); + h.set_cksum(); + b.append_data(&mut h, "/etc/escaped.txt", &b"hax!"[..]).unwrap(); + b.finish().unwrap(); + } + buf +} + +fn build_symlink_tar() -> Vec { + let mut buf = Vec::new(); + { + let mut b = Builder::new(&mut buf); + let mut h = Header::new_gnu(); + h.set_entry_type(tar::EntryType::Symlink); + h.set_size(0); + h.set_mode(0o777); + h.set_cksum(); + // link target points outside dest + b.append_link(&mut h, "evil-link", "/etc/passwd").unwrap(); + b.finish().unwrap(); + } + buf +} + +fn build_oversize_tar() -> Vec { + let mut buf = Vec::new(); + { + let mut b = Builder::new(&mut buf); + let mut h = Header::new_gnu(); + // Claim 2 GiB but only write a few bytes -- the size cap should + // trip on the header before unpacking the body. + h.set_size(2 * 1024 * 1024 * 1024); + h.set_mode(0o644); + h.set_cksum(); + // append_data needs to write the claimed bytes; instead we use + // append() which respects the header size and read from a sparse + // source. + let zeros = std::io::Cursor::new(vec![0u8; 4096]); + // For test purposes: just write a header with a huge claimed size + // and a small body; the cap should reject before reading. + b.append(&h, std::io::Read::take(zeros, 4096)).unwrap(); + b.finish().ok(); + } + buf +} + +#[test] +fn restore_rejects_path_traversal() { + let dest = TempDir::new().unwrap(); + let bytes = build_traversal_tar(); + let err = safe_unpack_git_archive(&bytes, dest.path(), 1024 * 1024).unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("path traversal") || msg.contains(".."), + "expected path-traversal rejection, got: {msg}"); + // And: the escape destination must not exist. + let escaped = dest.path().parent().unwrap().join("escaped.txt"); + assert!(!escaped.exists(), "traversal succeeded -- {} exists", escaped.display()); +} + +#[test] +fn restore_rejects_absolute_path() { + let dest = TempDir::new().unwrap(); + let bytes = build_absolute_path_tar(); + let err = safe_unpack_git_archive(&bytes, dest.path(), 1024 * 1024).unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("absolute") || msg.contains("path traversal"), + "expected absolute-path rejection, got: {msg}"); +} + +#[test] +fn restore_rejects_symlink() { + let dest = TempDir::new().unwrap(); + let bytes = build_symlink_tar(); + let err = safe_unpack_git_archive(&bytes, dest.path(), 1024 * 1024).unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("symlink") || msg.contains("link"), + "expected symlink rejection, got: {msg}"); +} + +#[test] +fn restore_rejects_size_bomb() { + let dest = TempDir::new().unwrap(); + let bytes = build_oversize_tar(); + // Cap of 1 KiB; entry claims 2 GiB. + let err = safe_unpack_git_archive(&bytes, dest.path(), 1024).unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("size") || msg.contains("cap") || msg.contains("too large"), + "expected size-cap rejection, got: {msg}"); +} + +#[test] +fn restore_accepts_normal_files() { + let dest = TempDir::new().unwrap(); + // Build a tiny well-formed tar with one regular file in subdir/. + let mut buf = Vec::new(); + { + let mut b = Builder::new(&mut buf); + let mut h = Header::new_gnu(); + h.set_size(5); + h.set_mode(0o644); + h.set_cksum(); + b.append_data(&mut h, "subdir/hello.txt", &b"hello"[..]).unwrap(); + b.finish().unwrap(); + } + safe_unpack_git_archive(&buf, dest.path(), 1024 * 1024).expect("happy path"); + let written = dest.path().join("subdir").join("hello.txt"); + assert!(written.exists()); + assert_eq!(std::fs::read(&written).unwrap(), b"hello"); +} +``` + +NOTE: integration tests in `crates//tests/` can only access `pub` items from the crate's library target. `relicario-cli` is currently binary-only. We have two options: + +- (A) Convert `relicario-cli` to bin+lib by adding `crates/relicario-cli/src/lib.rs` that `pub use`s the helper module, and let `main.rs` use the lib. +- (B) Move `safe_unpack_git_archive` into `relicario-core` (since it's pure logic with no CLI deps) and unit-test it there. + +Pick **B** — it belongs in core (no filesystem-trust assumptions are CLI-specific), it keeps `relicario-cli` binary-only, and it matches the project's "core is bytes-in/bytes-out" principle. The function takes `&[u8]` for the tar bytes and `&Path` for the destination, which is acceptable since it does fs writes. Or, more strictly: have the function emit `Vec<(PathBuf, Vec)>` of cleaned entries and let the CLI write them. **Choose the cleaner variant**: emit a `Vec<(RelPath, Vec)>` from core, let the CLI write. + +Adjust the tests accordingly: + +- Test target: `crates/relicario-core/tests/safe_unpack.rs` (new file). +- Function under test: `pub fn safe_unpack_git_archive(tar_bytes: &[u8], max_uncompressed_bytes: u64) -> Result)>>` in `crates/relicario-core/src/backup.rs` (or a new `crates/relicario-core/src/tar_safe.rs`). + +Move the test file to `crates/relicario-core/tests/safe_unpack.rs` and adjust the assertions: instead of `dest.path()`-based checks, assert the returned `Vec` is `Err` for malicious cases and contains the expected entry for the happy path. + +- [ ] **Step 3: Run tests to confirm fail** + +Run: `cargo test -p relicario-core --test safe_unpack` +Expected: compile error — `safe_unpack_git_archive` doesn't exist yet. + +NOTE: do not commit yet — paired with the implementation in Task 6. + +--- + +## Task 6: S2 — Implement `safe_unpack_git_archive` in core, wire into CLI + +**Files:** +- Create or modify: `crates/relicario-core/src/tar_safe.rs` (new module) +- Modify: `crates/relicario-core/src/lib.rs` (export module) +- Modify: `crates/relicario-cli/src/main.rs` (replace the `archive.unpack(...)` call) + +- [ ] **Step 1: Add `tar` to relicario-core dependencies** + +Edit `crates/relicario-core/Cargo.toml`: + +```toml +tar = { version = "0.4", default-features = false } +``` + +(Same version as relicario-cli already uses — keep workspace consistent.) + +- [ ] **Step 2: Implement the function** + +Write `crates/relicario-core/src/tar_safe.rs`: + +```rust +//! Hardened tar archive extraction used by backup restore. +//! +//! Defends against: +//! - Path traversal: entries with `..` components. +//! - Absolute paths and Windows drive letters. +//! - Symlinks and hardlinks (we never legitimately create either inside +//! `.git/` on restore). +//! - Tar bombs: caller provides an uncompressed-size cap. + +use std::io::Read; +use std::path::{Component, Path, PathBuf}; + +use crate::error::{RelicarioError, Result}; + +/// Maximum default cap if the caller doesn't have a better number: 1 GiB. +pub const DEFAULT_MAX_UNCOMPRESSED: u64 = 1024 * 1024 * 1024; + +/// Decode `tar_bytes` and return a list of (relative-path, contents) pairs +/// for regular files only. Caller writes them to disk. +/// +/// Any malicious-looking entry causes the entire operation to fail. We do +/// not partially-extract. +pub fn safe_unpack_git_archive( + tar_bytes: &[u8], + max_uncompressed_bytes: u64, +) -> Result)>> { + let mut archive = tar::Archive::new(tar_bytes); + let mut out: Vec<(PathBuf, Vec)> = Vec::new(); + let mut total_size: u64 = 0; + + for entry in archive.entries().map_err(|e| RelicarioError::BackupRestore(format!("tar entries: {e}")))? { + let mut entry = entry.map_err(|e| RelicarioError::BackupRestore(format!("tar entry: {e}")))?; + + // 1. Reject non-regular entries outright. + match entry.header().entry_type() { + tar::EntryType::Regular | tar::EntryType::Continuous | tar::EntryType::GNUSparse => {} + tar::EntryType::Directory => continue, // dirs are implicit in file paths + tar::EntryType::Symlink | tar::EntryType::Link => { + return Err(RelicarioError::BackupRestore(format!( + "rejecting symlink/hardlink entry: {} (path traversal blocked)", + entry.path().map(|p| p.display().to_string()).unwrap_or_default() + ))); + } + other => { + return Err(RelicarioError::BackupRestore(format!( + "rejecting unexpected tar entry type {other:?} (path traversal blocked)" + ))); + } + } + + // 2. Size cap. Check declared size in header before reading the body. + let claimed = entry.header().size().map_err(|e| { + RelicarioError::BackupRestore(format!("tar header size: {e}")) + })?; + if claimed > max_uncompressed_bytes { + return Err(RelicarioError::BackupRestore(format!( + "tar entry declared size {claimed} bytes exceeds cap {max_uncompressed_bytes}" + ))); + } + total_size = total_size.saturating_add(claimed); + if total_size > max_uncompressed_bytes { + return Err(RelicarioError::BackupRestore(format!( + "tar entries total size {total_size} bytes exceeds cap {max_uncompressed_bytes}" + ))); + } + + // 3. Path validation. + let raw_path = entry.path() + .map_err(|e| RelicarioError::BackupRestore(format!("tar entry path: {e}")))? + .into_owned(); + let cleaned = validate_relative_path(&raw_path)?; + + // 4. Read body (bounded by the size cap above). + let mut body = Vec::with_capacity(claimed as usize); + entry.read_to_end(&mut body) + .map_err(|e| RelicarioError::BackupRestore(format!("read tar entry body: {e}")))?; + + out.push((cleaned, body)); + } + + Ok(out) +} + +/// Reject any path that contains `..`, an absolute prefix, a Windows drive +/// letter, or a non-normal component. Returns a path made of only `Normal` +/// components. +fn validate_relative_path(p: &Path) -> Result { + let mut cleaned = PathBuf::new(); + for comp in p.components() { + match comp { + Component::Normal(s) => cleaned.push(s), + Component::CurDir => {} // "./foo" -> "foo" + Component::ParentDir => { + return Err(RelicarioError::BackupRestore(format!( + "tar entry contains `..` component: {} (path traversal blocked)", + p.display() + ))); + } + Component::RootDir => { + return Err(RelicarioError::BackupRestore(format!( + "tar entry has absolute path: {} (path traversal blocked)", + p.display() + ))); + } + Component::Prefix(_) => { + // Windows drive letter or UNC prefix. + return Err(RelicarioError::BackupRestore(format!( + "tar entry has drive/UNC prefix: {} (path traversal blocked)", + p.display() + ))); + } + } + } + if cleaned.as_os_str().is_empty() { + return Err(RelicarioError::BackupRestore(format!( + "tar entry has empty path after normalisation: {}", + p.display() + ))); + } + Ok(cleaned) +} +``` + +- [ ] **Step 3: Add the `BackupRestore` error variant if missing** + +Read `crates/relicario-core/src/error.rs`. If `BackupRestore(String)` doesn't exist as a variant, add it: + +```rust +#[error("backup restore: {0}")] +BackupRestore(String), +``` + +If a similar variant already exists (e.g. `Backup`, `Restore`, `Tar`), use that instead and adjust the function above. + +- [ ] **Step 4: Wire module into lib.rs** + +Edit `crates/relicario-core/src/lib.rs`: + +```rust +pub mod tar_safe; +pub use tar_safe::{safe_unpack_git_archive, DEFAULT_MAX_UNCOMPRESSED}; +``` + +- [ ] **Step 5: Run unit tests for the function** + +Run: `cargo test -p relicario-core --test safe_unpack` +Expected: all 5 tests pass (4 reject cases + 1 happy path). + +- [ ] **Step 6: Replace the unsafe `archive.unpack` call in CLI** + +Edit `crates/relicario-cli/src/main.rs`. Find the block at line 1719-1723: + +```rust + // .git/ history. + if let Some(tar_bytes) = &unpacked.git_archive { + let mut archive = tar::Archive::new(tar_bytes.as_slice()); + archive.unpack(target.join(".git")) + .with_context(|| "failed to untar .git/")?; + } else { +``` + +Replace with: + +```rust + // .git/ history. Hardened against path traversal, symlinks, and tar bombs. + if let Some(tar_bytes) = &unpacked.git_archive { + // Cap: 100x the compressed bundle, or 1 GiB, whichever is lower. + // 100x is a generous heuristic; legitimate .git pack data + // compresses 3-10x, so 100x leaves room for unusual repos. + let cap = std::cmp::min( + (tar_bytes.len() as u64).saturating_mul(100), + relicario_core::tar_safe::DEFAULT_MAX_UNCOMPRESSED, + ); + let entries = relicario_core::safe_unpack_git_archive(tar_bytes, cap) + .with_context(|| "failed to safely unpack .git/ archive")?; + let git_dir = target.join(".git"); + for (rel_path, body) in entries { + let dest = git_dir.join(&rel_path); + // Final paranoid check: ensure dest is inside git_dir even after + // OS-level resolution. We did the textual check in core; this + // catches any platform-specific quirk. + if !dest.starts_with(&git_dir) { + anyhow::bail!( + "tar entry {} resolved outside .git/ (path traversal blocked)", + rel_path.display() + ); + } + if let Some(parent) = dest.parent() { + fs::create_dir_all(parent).with_context(|| { + format!("failed to create parent {}", parent.display()) + })?; + } + fs::write(&dest, &body).with_context(|| { + format!("failed to write {}", dest.display()) + })?; + } + } else { +``` + +- [ ] **Step 7: Run the full test suite** + +Run: `cargo test` +Expected: all tests pass — including the existing happy-path backup roundtrip in `crates/relicario-cli/tests/backup.rs` (or wherever the existing restore test lives — confirm it's still green). + +- [ ] **Step 8: Commit** + +```bash +git add crates/relicario-core/Cargo.toml \ + crates/relicario-core/src/tar_safe.rs \ + crates/relicario-core/src/lib.rs \ + crates/relicario-core/src/error.rs \ + crates/relicario-core/tests/safe_unpack.rs \ + crates/relicario-cli/src/main.rs +git commit -m "$(cat <<'EOF' +fix(core,cli): harden backup-restore tar unpack (audit S2) + +`cmd_backup_restore` previously called tar::Archive::unpack on the +embedded .git archive with default settings, trusting the tar crate's +defaults to reject malicious payloads. They don't reject hardlinks, +they don't enforce size caps, and an attacker-crafted .relbak with +`../../etc/passwd` entries could escape the target directory. + +Replaced with `relicario_core::safe_unpack_git_archive`, which: + +- Rejects entries with `..` components, absolute paths, or Windows + drive prefixes (Component::ParentDir/RootDir/Prefix). +- Rejects symlinks and hardlinks outright -- legitimate .git/ tar + archives produced by `relicario backup export` only contain regular + files and directories. +- Enforces a max-uncompressed-bytes cap (caller picks; CLI uses + min(100x compressed, 1 GiB)). +- Returns (path, bytes) pairs to the caller; the CLI writes them and + re-checks `dest.starts_with(git_dir)` for OS-level paranoia. + +Acceptance: 5 unit tests in relicario-core (path traversal, absolute +path, symlink, size bomb, happy path) plus existing CLI restore +roundtrip stays green. + +Co-Authored-By: Claude Opus 4.7 +EOF +)" +``` + +--- + +## Task 7: S3 — Audit `RELICARIO_*` env vars and document them in SECURITY.md + +**Files:** +- Modify: `docs/SECURITY.md` + +The full set of `RELICARIO_*` env vars in production code (after the test-gate work in the previous device-auth plan): + +| Var | Site | Trust | +|---|---|---| +| `RELICARIO_VAULT` | `crates/relicario-cli/src/helpers.rs:vault_dir` (referenced via doc-comment at line 172) | User-trusted: vault-root override | +| `RELICARIO_IMAGE` | `crates/relicario-cli/src/session.rs:125` | User-trusted: reference-image path override | +| `RELICARIO_NO_GROUPS_CACHE` | `crates/relicario-cli/src/helpers.rs:103` | **Dev-only** — to be cfg-gated in Task 8 | +| `RELICARIO_GITEA_URL` | `crates/relicario-cli/src/main.rs:2298` | User config (Gitea base URL) | +| `RELICARIO_GITEA_TOKEN` | `crates/relicario-cli/src/main.rs:2303` | User secret (Gitea API token) | +| `RELICARIO_GITEA_OWNER` | `crates/relicario-cli/src/main.rs:2308` | User config (repo owner) | +| `RELICARIO_GITEA_REPO` | `crates/relicario-cli/src/main.rs:2313` | User config (repo name) | + +Plus the `cfg(test)`-gated `RELICARIO_TEST_*` set (already gated by the device-auth plan): `RELICARIO_TEST_PASSPHRASE`, `RELICARIO_TEST_ITEM_SECRET`, `RELICARIO_TEST_BACKUP_PASSPHRASE`, `RELICARIO_TEST_ITEM_PASSWORD`. These are test-binary-only and should be documented as such (so security reviewers don't flag them again). + +Confirm this list with a grep before writing — actual sites win over the spec's three-item list. + +- [ ] **Step 1: Verify the list** + +Run: `grep -rn 'RELICARIO_' crates/ | grep -v '/target/' | grep -v '/tests/' | grep 'env::'` +Expected: matches the table above (give or take `RELICARIO_TEST_*` which is what we're auditing). Update the table if any new vars have appeared since this plan was drafted. + +- [ ] **Step 2: Append a "Configuration env vars" section to SECURITY.md** + +Edit `docs/SECURITY.md`. Add at the bottom (after the existing "Access Control" section): + +```markdown +## Configuration env vars + +Relicario reads the following environment variables. Each one is a +trust boundary: an attacker who can set them in the user's environment +can influence Relicario's behavior. They are listed here so security +reviewers can audit the surface in one place. + +### User-facing + +| Variable | Purpose | Trust | +|---|---|---| +| `RELICARIO_VAULT` | Override the vault root directory (default: `~/.relicario`). | Trusted: filesystem path under the user's control. | +| `RELICARIO_IMAGE` | Override the reference-image JPEG path used during unlock. | Trusted: filesystem path under the user's control. The image file is read-only; its bytes feed `imgsecret::extract_secret`. | +| `RELICARIO_GITEA_URL` | Default Gitea API base URL for `relicario device add`. Equivalent to `--gitea-url`. | Trusted: HTTPS URL. Used only in the device-add code path. | +| `RELICARIO_GITEA_TOKEN` | Gitea personal-access token. Equivalent to `--gitea-token`. | **Secret**: anyone who can read this env var can manage the user's deploy keys via Gitea. The CLI does not log it. | +| `RELICARIO_GITEA_OWNER` | Repo owner (e.g. `alee`). Equivalent to `--owner`. | Trusted: opaque string. | +| `RELICARIO_GITEA_REPO` | Repo name (e.g. `vault`). Equivalent to `--repo`. | Trusted: opaque string. | + +### Test-only (compiled out of release builds) + +The following are gated behind `#[cfg(test)]` and **do not exist** in +binaries built without the test profile (`cargo build --release`): + +| Variable | Purpose | +|---|---| +| `RELICARIO_TEST_PASSPHRASE` | Bypass the rpassword prompt during integration tests. | +| `RELICARIO_TEST_ITEM_SECRET` | Bypass the rpassword prompt for item-secret fields during integration tests. | +| `RELICARIO_TEST_BACKUP_PASSPHRASE` | Bypass the prompt for backup export/restore passphrases during integration tests. | +| `RELICARIO_TEST_ITEM_PASSWORD` | Bypass the rpassword prompt for legacy item-password fields during integration tests. | + +### Dev-only (compiled out of release builds via `cfg(debug_assertions)`) + +| Variable | Purpose | +|---|---| +| `RELICARIO_NO_GROUPS_CACHE` | Disables the plaintext groups cache used for shell-completion. Intended for developers debugging the cache logic. Compiled out of `cargo build --release` builds. | +``` + +- [ ] **Step 3: Commit** + +```bash +git add docs/SECURITY.md +git commit -m "$(cat <<'EOF' +docs: enumerate RELICARIO_* env vars in SECURITY.md (audit S3) + +Adds a "Configuration env vars" section listing every RELICARIO_* +variable read by production code, with purpose and trust boundary. +Splits user-facing vars from test-only and dev-only ones to make the +attack surface explicit for security reviewers. + +Co-Authored-By: Claude Opus 4.7 +EOF +)" +``` + +--- + +## Task 8: S3 — `cfg(debug_assertions)`-gate `RELICARIO_NO_GROUPS_CACHE` + +**Files:** +- Modify: `crates/relicario-cli/src/helpers.rs` +- Modify: `crates/relicario-cli/src/main.rs` (doc-comment at line 174) + +`RELICARIO_NO_GROUPS_CACHE` is a developer escape hatch for debugging the groups-cache logic, not a user knob. Gating it with `cfg(debug_assertions)` makes the env-var read no-op in `cargo build --release` (since debug assertions are off in release by default), which means `strings` on the release binary won't show the variable name and the env-var lookup is dead-code-eliminated. + +- [ ] **Step 1: Write a build-mode-aware test** + +In `crates/relicario-cli/src/helpers.rs`, find the existing `tests` module (or add one) and append: + +```rust +#[cfg(test)] +mod tests { + // ... existing tests ... + + /// In debug builds (which `cargo test` always is) the override should still + /// work so existing dev workflows keep functioning. + #[test] + #[cfg(debug_assertions)] + fn no_groups_cache_env_var_honored_in_debug() { + use std::collections::BTreeSet; + let tmp = tempfile::tempdir().unwrap(); + std::env::set_var("RELICARIO_NO_GROUPS_CACHE", "1"); + super::write_groups_cache(tmp.path(), &BTreeSet::new()).unwrap(); + std::env::remove_var("RELICARIO_NO_GROUPS_CACHE"); + assert!(!tmp.path().join(".relicario/groups.cache").exists(), + "env var should suppress the cache write in debug builds"); + } +} +``` + +- [ ] **Step 2: Run test to confirm baseline (still passes)** + +Run: `cargo test -p relicario-cli helpers::tests::no_groups_cache_env_var_honored_in_debug` +Expected: passes — current code already honours the var in debug. + +- [ ] **Step 3: Gate the env-var check** + +Edit `crates/relicario-cli/src/helpers.rs`. Find the function (line 99-115ish): + +```rust +pub fn write_groups_cache( + vault_dir: &Path, + groups: &std::collections::BTreeSet, +) -> std::io::Result<()> { + if std::env::var_os("RELICARIO_NO_GROUPS_CACHE").is_some() { + return Ok(()); + } + // ... +``` + +Change to: + +```rust +pub fn write_groups_cache( + vault_dir: &Path, + groups: &std::collections::BTreeSet, +) -> std::io::Result<()> { + // Dev-only escape hatch. Compiled out of release builds: the + // const below is `false` under `cargo build --release`, which lets + // the optimiser drop the env-var lookup entirely (so `strings` on + // a release binary won't reveal the variable name). + if cfg!(debug_assertions) + && std::env::var_os("RELICARIO_NO_GROUPS_CACHE").is_some() + { + return Ok(()); + } + // ... rest unchanged +``` + +Update the doc-comment a few lines up to reflect the new behaviour: + +```rust +/// Write the sorted set of group names to `/.relicario/groups.cache`, +/// one name per line. In debug builds, setting `RELICARIO_NO_GROUPS_CACHE` +/// suppresses the write (used by tests). In release builds the env var is +/// ignored. +``` + +- [ ] **Step 4: Run the test again and the rest of the suite** + +Run: `cargo test -p relicario-cli` +Expected: all tests pass. + +- [ ] **Step 5: Update the `cmd_groups` doc-comment in main.rs** + +Find lines 170-176ish in `crates/relicario-cli/src/main.rs`: + +```rust + /// the plaintext `${RELICARIO_VAULT}/.relicario/groups.cache` file, + /// ... + /// `RELICARIO_NO_GROUPS_CACHE=1` to opt out of the cache (completion + /// will fall back to vault-locked behaviour). +``` + +Adjust to reflect that `RELICARIO_NO_GROUPS_CACHE` is debug-only: + +```rust + /// the plaintext `${RELICARIO_VAULT}/.relicario/groups.cache` file, + /// ... + /// In debug builds only, set `RELICARIO_NO_GROUPS_CACHE=1` to opt out + /// of the cache (completion will fall back to vault-locked behaviour). + /// The env var is a no-op in release builds. +``` + +- [ ] **Step 6: Verify the variable is gone from a release binary** + +Run: + +```bash +cargo build -p relicario-cli --release +strings target/release/relicario | grep RELICARIO_NO_GROUPS_CACHE && echo "FAIL: still present" || echo "OK: not present" +``` + +Expected: `OK: not present`. + +NOTE: `cfg!(debug_assertions)` evaluates to a `bool` literal at build time. Combined with `&&`, the optimiser sees `if false && ...` in release and dead-codes the whole branch including the env-var lookup. If `strings` somehow still finds it (e.g. due to a stray doc-string referencing the name in `#[command(...)]` long-help), that's expected — what matters is no env-var read at runtime. + +- [ ] **Step 7: Commit** + +```bash +git add crates/relicario-cli/src/helpers.rs crates/relicario-cli/src/main.rs +git commit -m "$(cat <<'EOF' +fix(cli): cfg-gate RELICARIO_NO_GROUPS_CACHE to debug builds (audit S3) + +The groups-cache opt-out is a developer debugging knob, not a user-facing +config. Gating its env-var lookup behind `cfg!(debug_assertions)` makes +release builds ignore the variable entirely; the optimiser drops the +lookup, removing the variable name from the release binary's strings. + +Doc-comments updated to reflect the new behaviour. + +Co-Authored-By: Claude Opus 4.7 +EOF +)" +``` + +--- + +## Task 9: C1 — Prune merged local feature branches (interactive) + +**Files:** +- _local git state only_ — no source files changed. + +The five branches to evaluate (from the spec): +- `feature/fullscreen-ux-phase-2a` +- `feature/typed-items-1a-rust-core` +- `feature/typed-items-1c-alpha` +- `feature/typed-items-1c-beta1` +- `feature/typed-items-1c-beta2` + +This task is **destructive** (`git branch -D`). The project rule (`CLAUDE.md`: "Always pause and ask before ... `git branch -D`") applies — prompt the user before each delete. + +- [ ] **Step 1: Confirm we're on `main` and clean** + +Run: + +```bash +git status +git branch --show-current +``` + +Expected: branch is `main`. Working tree state is the user's call — Cargo.lock is currently modified but that doesn't block branch deletion. + +- [ ] **Step 2: List merged branches** + +Run: + +```bash +git branch --merged main +``` + +Expected output should include all five target branches plus `* main`. If any are missing, do NOT delete that one — it has unmerged commits. Surface this to the user before continuing. + +- [ ] **Step 3: For each branch, confirm-then-delete one at a time** + +For each of the five branches in this exact order: + +1. `feature/fullscreen-ux-phase-2a` +2. `feature/typed-items-1a-rust-core` +3. `feature/typed-items-1c-alpha` +4. `feature/typed-items-1c-beta1` +5. `feature/typed-items-1c-beta2` + +Per branch: + +a. Run `git log --oneline main.. | head -5` to show the user what's *not* in main from this branch's perspective. Empty output = fully merged. Non-empty = unmerged commits — STOP and surface to the user, do not delete. + +b. Show the user the tip commit for context: + +```bash +git log -1 --format='%h %s' +``` + +c. **Prompt the user**: "Delete local branch `` (tip: ` `)? [y/N]" + +d. On `y` (or `yes`): run `git branch -D `. On anything else: skip and continue to the next. + +e. After delete, run `git branch` to confirm the branch is gone. + +- [ ] **Step 4: Verify final state** + +Run: + +```bash +git branch +``` + +Expected: only `* main` remains (assuming the user said yes to all five). If any branches remain, that's fine — the user chose to keep them. + +- [ ] **Step 5: No commit needed** + +Branch deletions don't produce commits. There is nothing to commit for C1; the task ends here. + +- [ ] **Step 6: Surface a summary to the caller** + +Print a one-line summary: "C1 done. Pruned N of 5 merged feature branches." (where N is whatever the user accepted). + +--- + +## Acceptance — final verification + +- [ ] **Step 1: All Rust tests green** + +```bash +cargo test +``` + +Expected: all tests pass, including the 4 S1 tests and 5 S2 tests added in this plan. + +- [ ] **Step 2: All targets build** + +```bash +cargo build +cargo build -p relicario-wasm --target wasm32-unknown-unknown +cargo build -p relicario-server +cargo build -p relicario-cli --release +``` + +Expected: all succeed. + +- [ ] **Step 3: Release binary doesn't reference the dev env var** + +```bash +strings target/release/relicario | grep RELICARIO_NO_GROUPS_CACHE && echo "FAIL" || echo "OK" +``` + +Expected: `OK`. + +- [ ] **Step 4: SECURITY.md is up to date** + +Confirm `docs/SECURITY.md` lists every `RELICARIO_*` var that appears in `grep -rn 'env::var' crates/ | grep RELICARIO`. + +- [ ] **Step 5: No stale local feature branches** + +```bash +git branch +``` + +Expected: matches the user's choices in Task 9. + +- [ ] **Step 6: Hand off to user for review and merge** + +Summarise the four security/cleanup landings (S1, S2, S3, C1), point at the spec, and stop. The user merges to `main` and tags `v0.5.0` after Plan B is also green. + +--- + +## Completion Checklist + +- [ ] Task 1: `device::fingerprint` helper added to relicario-core (S1 prep) +- [ ] Task 2: relicario-server dev-dependencies added (S1 prep) +- [ ] Task 3: 4 failing acceptance tests for verify-commit (S1) +- [ ] Task 4: `verify_commit` rewritten — allowed-signers temp file, fingerprint matching, devices/revoked check with committer-date semantics (S1) +- [ ] Task 5: 5 failing tests for `safe_unpack_git_archive` (S2) +- [ ] Task 6: `safe_unpack_git_archive` implemented in core, CLI restore wired through it (S2) +- [ ] Task 7: SECURITY.md "Configuration env vars" section added (S3) +- [ ] Task 8: `RELICARIO_NO_GROUPS_CACHE` cfg-gated to debug builds (S3) +- [ ] Task 9: Merged local feature branches pruned interactively (C1) +- [ ] Final acceptance walk done and Plan A handed off + +--- + +## Out of Scope (do not implement here) + +These belong to Plan B (Extension UX), tracked separately at +`docs/superpowers/plans/2026-05-02-v0.5.0-plan-b-extension-ux.md` (TBD by the +Plan B author): + +- B1 — Strength-meter desync after regenerate (extension/TS) +- B2/P4 — Error-message audit (`vault_locked` raw-code leak), ERROR_COPY map +- P1 — Password coloring +- P2 — Setup-wizard completion routes to fullscreen vault tab +- P3 — Form-layout 2-col -> full-width transition diff --git a/docs/superpowers/plans/2026-05-02-v0.5.0-plan-b-extension-ux.md b/docs/superpowers/plans/2026-05-02-v0.5.0-plan-b-extension-ux.md new file mode 100644 index 0000000..c07dd81 --- /dev/null +++ b/docs/superpowers/plans/2026-05-02-v0.5.0-plan-b-extension-ux.md @@ -0,0 +1,1654 @@ +# v0.5.0 Plan B — Extension UX Polish + Bug Fixes + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Land the extension-UX slice of v0.5.0: centralize error-message copy (P4, subsumes B2), fix the strength-meter desync after regenerate (B1), color revealed passwords by character class (P1), repair the form-layout transition where lower sections break visual rhythm (P3), and route setup-wizard completion to the fullscreen vault tab (P2). + +**Architecture:** Five independent slices on the existing extension shell. P4 introduces a single `ERROR_COPY` map keyed by snake_case error code, returning friendly title/body/CTA — the popup's one-off `humanizeError` regex chain and the fullscreen tab's raw `state.error` rendering both fold into it. B1 dispatches a synthetic `input` event after the regenerate handler programmatically sets the field's value, so the existing strength-meter listener at `password-tools.ts:65` re-rates the new password. P1 inlines the password-coloring spec/plan as a single `colorizePassword()` utility plus a small `chrome.storage.sync`-backed scheme (defaults: digits blue, symbols red). P3 constrains the lower sections (notes, custom-fields disclosure, attachments disclosure) to the same `max-width: 960px; margin: 0 auto` envelope as `.form-grid`. P2 swaps the setup-wizard's terminal-step idle render for `chrome.tabs.create({ url: chrome.runtime.getURL('vault.html') }) + window.close()`. + +**Tech Stack:** TypeScript, vanilla DOM, vitest + JSDOM/happy-dom, plain CSS. No new dependencies. + +**Spec:** `docs/superpowers/specs/2026-05-02-v0.5.0-polish-harden-design.md` (sections P1, P2, P3, P4, B1, B2). +**P1 source spec/plan:** `docs/superpowers/specs/2026-05-01-password-coloring-design.md`, `docs/superpowers/plans/2026-05-01-password-coloring.md` (inlined below). + +--- + +## File Structure + +| File | Change | Purpose | +|------|--------|---------| +| `extension/src/shared/error-copy.ts` | Create | Central `ERROR_COPY` map + `lookupErrorCopy()` (P4) | +| `extension/src/shared/__tests__/error-copy.test.ts` | Create | Generated test enumerating every grep'd error code (P4) | +| `extension/src/popup/popup.ts` | Modify | Replace `humanizeError` regex chain with `lookupErrorCopy` (P4) | +| `extension/src/vault/vault.ts` | Modify | Replace raw `state.error` render with friendly copy + Unlock CTA (P4) | +| `extension/src/popup/components/types/login.ts` | Modify | Dispatch `input` event after regenerate sets password value (B1); colorize revealed passwords (P1) | +| `extension/src/popup/components/types/__tests__/login.test.ts` | Modify | Vitest: regenerate dispatches `input` event (B1) | +| `extension/src/shared/password-coloring.ts` | Create | Pure `colorizePassword()` utility (P1) | +| `extension/src/shared/__tests__/password-coloring.test.ts` | Create | Vitest unit tests (P1) | +| `extension/src/shared/color-scheme.ts` | Create | Storage round-trip + `applyColorScheme()` (P1) | +| `extension/src/shared/__tests__/color-scheme.test.ts` | Create | Vitest unit tests (P1) | +| `extension/src/popup/styles.css` | Modify | `.pwd-digit/.pwd-symbol/.pwd-letter` rules + custom-property defaults (P1) | +| `extension/src/vault/vault.css` | Modify | Same coloring rules (P1); `.form-lower` constraint (P3) | +| `extension/src/popup/components/field-history.ts` | Modify | Colorize revealed history entries (P1) | +| `extension/src/popup/components/settings.ts` | Modify | Display section: pickers + swatch + reset (P1) | +| `extension/src/popup/popup.ts` | Modify | Boot-time `applyColorScheme()` + storage-change listener (P1) | +| `extension/src/vault/vault.ts` | Modify | Boot-time `applyColorScheme()` (P1) | +| `extension/src/setup/setup.ts` | Modify | After `state.configPushed = true`, open vault tab + close setup (P2) | +| `extension/src/setup/__tests__/setup.test.ts` | Create or extend | Vitest: completion calls `chrome.tabs.create` with vault URL (P2) | + +--- + +## Sequencing + +Per spec: P4 → B1 → P1 → P3 → P2. P4 lands first because it centralizes the error vocabulary that the rest of the work assumes. B1 second because it's the smallest fix and ships visible value fast. P1 third because it's self-contained and lands a polish slice before the layout work. P3 fourth — minimal CSS to repair the visual rhythm. P2 last — end-to-end change that benefits from earlier polish being in place when the new tab opens. + +--- + +## Task 1 (P4): Build `ERROR_COPY` map + replace popup regex + +**Files:** +- Create: `extension/src/shared/error-copy.ts` +- Create: `extension/src/shared/__tests__/error-copy.test.ts` +- Modify: `extension/src/popup/popup.ts` + +- [ ] **Step 1: Enumerate every error code via grep** + +```bash +cd /home/alee/Sources/relicario && \ + grep -rohE "ok: false, error: '[^']+'" extension/src/service-worker/ \ + --include="*.ts" \ + --exclude-dir=__tests__ \ + | sed -E "s/.*error: '([^']+)'/\\1/" \ + | sort -u +``` + +Expected output (this is the canonical list; if the grep surfaces more, add them — the test in Step 2 enforces this): + +``` +attachment_not_found +captured_tab_gone +download_failed +Extension not configured. Run setup first. +invalid base32 secret +invalid_sender_url +item_not_found +no items to import +no reference image stored locally +no_totp +not_a_login +origin_mismatch +Reference image not set. Run setup first. +remote already contains a Relicario vault +tab_navigated +unauthorized_sender +unknown_message_type +upload_failed +vault_locked +``` + +Note: a few entries are full sentences rather than snake_case — those already render acceptably; the map should still cover them so the lookup is total. + +- [ ] **Step 2: Write the failing test** + +Create `extension/src/shared/__tests__/error-copy.test.ts`: + +```ts +import { describe, it, expect } from 'vitest'; +import { execSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { ERROR_COPY, lookupErrorCopy } from '../error-copy'; + +const repoRoot = resolve(__dirname, '../../../..'); + +function discoverCodes(): Set { + const out = execSync( + `grep -rohE "ok: false, error: '[^']+'" extension/src/service-worker/ \ + --include="*.ts" --exclude-dir=__tests__`, + { cwd: repoRoot, encoding: 'utf-8' }, + ); + const codes = new Set(); + for (const line of out.split('\n')) { + const m = line.match(/error: '([^']+)'/); + if (m) codes.add(m[1]); + } + return codes; +} + +describe('ERROR_COPY', () => { + it('contains an entry for every error code returned by the service worker', () => { + const discovered = discoverCodes(); + expect(discovered.size).toBeGreaterThan(0); + const missing: string[] = []; + for (const code of discovered) { + if (!ERROR_COPY[code]) missing.push(code); + } + expect(missing).toEqual([]); + }); + + it('lookupErrorCopy returns the mapped entry for known codes', () => { + const copy = lookupErrorCopy('vault_locked'); + expect(copy.title).toBe('Vault locked'); + expect(copy.body).toMatch(/unlock/i); + }); + + it('lookupErrorCopy falls back to a generic shape for unknown codes', () => { + const copy = lookupErrorCopy('made_up_code_xyz'); + expect(copy.title).toBeTruthy(); + expect(copy.body).toContain('made_up_code_xyz'); // raw code visible in fallback body + }); +}); +``` + +- [ ] **Step 3: Run — expect FAIL (module missing)** + +```bash +cd extension && npx vitest run src/shared/__tests__/error-copy.test.ts +``` + +- [ ] **Step 4: Implement `error-copy.ts`** + +Create `extension/src/shared/error-copy.ts`: + +```ts +/// Central registry mapping snake_case (or sentence) error codes returned by +/// the service worker to user-facing copy. One key per distinct error string +/// emitted from `extension/src/service-worker/router/`. The accompanying test +/// enumerates the live grep and asserts every code is keyed here. +/// +/// Callers receive `{ title, body, cta? }`. The popup and fullscreen tab each +/// render this however suits their surface — popup as inline error block, +/// fullscreen as the lock-screen error region. + +export interface ErrorCta { + label: string; + /** Action ids the surface knows how to handle. New surfaces add cases. */ + action?: 'unlock' | 'reload_extension' | 'open_setup'; +} + +export interface ErrorCopy { + title: string; + body: string; + cta?: ErrorCta; +} + +const UNLOCK_CTA: ErrorCta = { label: 'Unlock vault', action: 'unlock' }; + +export const ERROR_COPY: Record = { + vault_locked: { + title: 'Vault locked', + body: 'Unlock your vault to continue.', + cta: UNLOCK_CTA, + }, + unauthorized_sender: { + title: 'Action not allowed', + body: 'This action is not allowed from here.', + }, + unknown_message_type: { + title: 'Internal error', + body: 'The extension received an unknown request — try reloading.', + cta: { label: 'Reload extension', action: 'reload_extension' }, + }, + origin_mismatch: { + title: 'Wrong site', + body: 'This login belongs to a different site — refusing to leak credentials cross-origin.', + }, + not_a_login: { + title: 'Not a login', + body: 'That item does not have a username and password to fill.', + }, + no_totp: { + title: 'No 2FA on this item', + body: 'This item does not have a TOTP secret configured.', + }, + invalid_sender_url: { + title: 'Cannot read tab URL', + body: 'The current tab has no recognizable URL — try reloading the page.', + }, + tab_navigated: { + title: 'Tab changed', + body: 'The browser tab changed before the action could complete — try again.', + }, + captured_tab_gone: { + title: 'Tab is gone', + body: 'The browser tab closed before the action could complete — try again.', + }, + item_not_found: { + title: 'Item not found', + body: 'That item is no longer in the vault — it may have been deleted from another device.', + }, + attachment_not_found: { + title: 'Attachment missing', + body: 'The attachment is referenced in the item but is not present in the vault.', + }, + upload_failed: { + title: 'Upload failed', + body: 'Could not upload the attachment — check your connection and try again.', + }, + download_failed: { + title: 'Download failed', + body: 'Could not download the attachment — check your connection and try again.', + }, + 'invalid base32 secret': { + title: 'Invalid secret', + body: 'The TOTP secret must be valid Base32 (letters A-Z and digits 2-7 only).', + }, + 'no items to import': { + title: 'Nothing to import', + body: 'The CSV did not contain any importable items.', + }, + 'no reference image stored locally': { + title: 'No reference image', + body: 'This device has no reference image saved locally — re-attach the device or restore from backup.', + }, + 'remote already contains a Relicario vault': { + title: 'Vault already exists', + body: 'The selected repository already contains a vault — use Attach existing instead of Create new.', + }, + 'Extension not configured. Run setup first.': { + title: 'Extension not configured', + body: 'Run setup before using this action.', + cta: { label: 'Open setup', action: 'open_setup' }, + }, + 'Reference image not set. Run setup first.': { + title: 'Reference image missing', + body: 'Run setup to save your reference image.', + cta: { label: 'Open setup', action: 'open_setup' }, + }, +}; + +/// Total lookup. Unknown codes return a generic shape with the raw code in the +/// body so debugging is still possible — the alternative (returning empty body) +/// hides bugs by silently degrading. +export function lookupErrorCopy(code: string): ErrorCopy { + return ERROR_COPY[code] ?? { + title: 'Something went wrong', + body: code, + }; +} +``` + +- [ ] **Step 5: Run — expect PASS** + +```bash +cd extension && npx vitest run src/shared/__tests__/error-copy.test.ts +``` + +- [ ] **Step 6: Replace popup `humanizeError` with `lookupErrorCopy`** + +In `extension/src/popup/popup.ts` (around line 121-160), replace `humanizeError` and the `sendMessage` body with: + +```ts +import { lookupErrorCopy } from '../shared/error-copy'; + +export function sendMessage(request: Request): Promise { + return new Promise((resolve) => { + chrome.runtime.sendMessage(request, (response: Response) => { + if (response && !response.ok && response.error) { + response = { ok: false, error: humanizeError(response.error) }; + } + resolve(response); + }); + }); +} + +/// Translate an error code from the service worker into a single human-readable +/// string for inline error blocks. Surfaces that need a CTA (unlock prompt, etc.) +/// should call `lookupErrorCopy(code)` directly and render `cta` themselves. +export function humanizeError(err: string): string { + // Rust/serde leakage that doesn't pass through the router's error vocabulary. + // Keep these regexes — they translate underlying-library wording, not snake_case codes. + if (/relative URL without a base/i.test(err)) { + return 'URL must start with https:// or http:// (e.g. https://example.com)'; + } + if (/item json:/i.test(err)) { + return 'Could not save item — one of the fields is in an invalid format.'; + } + if (/settings json:/i.test(err)) { + return 'Settings are in an invalid format — try reloading the extension.'; + } + // For everything else, look up in ERROR_COPY (handles vault_locked, origin_mismatch, + // unauthorized_sender, tab_navigated/captured_tab_gone, etc. — replacing the prior + // five hand-written regexes). + const copy = lookupErrorCopy(err); + return copy.body; +} +``` + +The existing five regex tests (`vault_locked`, `origin_mismatch`, `unauthorized_sender`, `tab_navigated|captured_tab_gone`) are gone — `lookupErrorCopy` handles them via exact-key match instead. + +- [ ] **Step 7: Run all popup tests to confirm nothing regressed** + +```bash +cd extension && npx vitest run src/popup/ +``` + +Expected: PASS. If a test asserted on a specific humanized string, update it to assert against `lookupErrorCopy('').body` instead. + +- [ ] **Step 8: Commit** + +```bash +git add extension/src/shared/error-copy.ts \ + extension/src/shared/__tests__/error-copy.test.ts \ + extension/src/popup/popup.ts +git commit -m "$(cat <<'EOF' +feat(ext/shared): centralize error-message copy in ERROR_COPY map + +Replaces the popup's regex-chain humanizeError with a total lookup over +every error code returned by extension/src/service-worker/router/. A +generated test discovers codes via grep so the registry can't drift. +The popup keeps its small set of regex translators for Rust/serde error +phrasing that doesn't go through the router's error vocabulary. + +Subsumes B2 — fullscreen consumer lands in the next commit. +EOF +)" +``` + +--- + +## Task 2 (P4 cont.): Wire `ERROR_COPY` into the fullscreen vault tab + +**Files:** +- Modify: `extension/src/vault/vault.ts` + +- [ ] **Step 1: Locate raw error rendering** + +In `extension/src/vault/vault.ts`, the lock screen renders: + +```ts +${state.error ? `
${escapeHtml(state.error)}
` : ''} +``` + +at line ~202, with `state.error = resp.error ?? 'unlock failed'` set at ~222. Other call sites set `state.error = (resp as { error: string }).error` at line ~414 — same pattern. + +When the service worker returns `{ ok: false, error: 'vault_locked' }`, that snake_case string lands directly in the rendered `
`. This is exactly the B2 leak shown in the screenshot. + +- [ ] **Step 2: Render via `lookupErrorCopy`** + +Add the import at the top: + +```ts +import { lookupErrorCopy, type ErrorCta } from '../shared/error-copy'; +``` + +Replace each `
${escapeHtml(state.error)}
` pattern with a helper: + +```ts +function renderErrorBlock(code: string | null | undefined): string { + if (!code) return ''; + const copy = lookupErrorCopy(code); + const ctaHtml = copy.cta + ? `` + : ''; + return ` +
+
${escapeHtml(copy.title)}
+
${escapeHtml(copy.body)}
+ ${ctaHtml} +
+ `; +} +``` + +Replace the lock-screen line (~202) with: + +```ts +${renderErrorBlock(state.error)} +``` + +Replace any other `${state.error ? \`
…\` : ''}` lines in `vault.ts` the same way. + +- [ ] **Step 3: Wire CTA actions** + +After the lock-screen markup is mounted, attach a click handler that dispatches based on `data-cta`: + +```ts +app.querySelector('.error-cta')?.addEventListener('click', (e) => { + const cta = (e.currentTarget as HTMLElement).dataset.cta as ErrorCta['action']; + switch (cta) { + case 'unlock': { + // Already on the lock screen; just refocus the passphrase input. + document.getElementById('vault-passphrase')?.focus(); + break; + } + case 'open_setup': { + void chrome.tabs.create({ url: chrome.runtime.getURL('setup.html') }); + break; + } + case 'reload_extension': { + chrome.runtime.reload(); + break; + } + } +}); +``` + +- [ ] **Step 4: Append `.error-block` styles to vault.css** + +Append to `extension/src/vault/vault.css`: + +```css +.error-block { + display: flex; + flex-direction: column; + align-items: center; + gap: 6px; + padding: 12px 16px; + border: 1px solid rgba(171, 43, 32, 0.4); + border-radius: 6px; + background: rgba(171, 43, 32, 0.08); + margin-top: 12px; +} +.error-block .error-title { + font-weight: 600; + color: var(--danger); +} +.error-block .error-body { + color: var(--text); + font-size: 12px; + text-align: center; +} +.error-block .error-cta { + margin-top: 6px; +} +``` + +- [ ] **Step 5: Manual smoke test** + +```bash +cd extension && npm run build +``` + +Load `extension/dist/` in Chrome, open the fullscreen vault while locked, trigger an action that returns `vault_locked`, confirm the block reads: + +``` +Vault locked +Unlock your vault to continue. +[Unlock vault] +``` + +with the snake_case code nowhere on screen. + +- [ ] **Step 6: Commit** + +```bash +git add extension/src/vault/vault.ts extension/src/vault/vault.css +git commit -m "$(cat <<'EOF' +fix(ext/vault): friendly error block in fullscreen tab (closes B2) + +Replaces raw escapeHtml(state.error) renders with lookupErrorCopy()-driven +title/body/CTA blocks. vault_locked specifically gets a 'Unlock vault' +CTA that refocuses the passphrase input. Other CTAs route to setup.html +or chrome.runtime.reload(). + +Closes B2; concludes P4. +EOF +)" +``` + +--- + +## Task 3 (B1): Dispatch `input` event after regenerate sets password + +**Files:** +- Modify: `extension/src/popup/components/types/login.ts` +- Modify: `extension/src/popup/components/types/__tests__/login.test.ts` + +- [ ] **Step 1: Locate the regenerate handler** + +In `extension/src/popup/components/types/login.ts` at lines 420-439, the gen-btn click opens the generator panel. The picked-value callback is: + +```ts +onPicked: (value) => { + const pw = document.getElementById('f-password') as HTMLInputElement | null; + if (pw) { pw.value = value; pw.type = 'text'; } +}, +``` + +Programmatic `pw.value = value` does NOT fire `input` events in standard DOM behavior. The strength-meter listener at `extension/src/shared/form-affordances/password-tools.ts:65` (`input.addEventListener('input', update)`) never sees the new value, so the meter keeps reporting the prior reading (or empty / "trivially crackable" if the field was empty before). + +- [ ] **Step 2: Write the failing test** + +In `extension/src/popup/components/types/__tests__/login.test.ts`, add: + +```ts +describe('regenerate handler dispatches input event', () => { + let app: HTMLElement; + beforeEach(() => { + document.body.innerHTML = '
'; + app = document.getElementById('app')!; + // ...existing setup mocks for renderForm should already be present above. + }); + + it('dispatches an InputEvent on #f-password after the generator picks a value', () => { + renderForm(app, 'add', null); + const pw = app.querySelector('#f-password')!; + const dispatchSpy = vi.spyOn(pw, 'dispatchEvent'); + + // Simulate the generator panel calling its onPicked callback. Easiest + // route: export the onPicked builder, OR open the panel and trigger its + // pick. The test here exercises the handler indirectly — the assertion + // is that dispatchEvent was called with an InputEvent after value is set. + // + // Implementation strategy: extract the onPicked body into a named helper + // `applyGeneratedPassword(input, value)` exported from login.ts so the + // test can call it directly. + applyGeneratedPassword(pw, 'sCMtTJkF%GN^mF#-N6D%'); + + expect(pw.value).toBe('sCMtTJkF%GN^mF#-N6D%'); + expect(pw.type).toBe('text'); + expect(dispatchSpy).toHaveBeenCalledWith(expect.any(InputEvent)); + const evt = dispatchSpy.mock.calls.find(c => c[0] instanceof InputEvent)![0] as InputEvent; + expect(evt.type).toBe('input'); + expect(evt.bubbles).toBe(true); + }); + + it('strength meter listener fires when the input event bubbles', () => { + renderForm(app, 'add', null); + const pw = app.querySelector('#f-password')!; + let listenerFired = false; + pw.addEventListener('input', () => { listenerFired = true; }); + applyGeneratedPassword(pw, 'newpass'); + expect(listenerFired).toBe(true); + }); +}); +``` + +- [ ] **Step 3: Run — expect FAIL** + +```bash +cd extension && npx vitest run src/popup/components/types/__tests__/login.test.ts -t "regenerate handler" +``` + +Expected: failure on `applyGeneratedPassword` not being exported, or on `dispatchSpy` not being called. + +- [ ] **Step 4: Extract and export `applyGeneratedPassword`** + +In `extension/src/popup/components/types/login.ts`, add (near the top of the module, alongside the other exported helpers): + +```ts +/// Programmatically install a generated password into the field, then +/// dispatch a synthetic `input` event so listeners (strength meter, autosave, +/// etc.) re-rate the new value. Without the dispatch, the meter at +/// shared/form-affordances/password-tools.ts:65 stays stale (B1). +export function applyGeneratedPassword(input: HTMLInputElement, value: string): void { + input.value = value; + input.type = 'text'; + input.dispatchEvent(new InputEvent('input', { bubbles: true })); +} +``` + +Replace the existing `onPicked` body inside the `gen-btn` click handler (line ~434) with: + +```ts +onPicked: (value) => { + const pw = document.getElementById('f-password') as HTMLInputElement | null; + if (pw) applyGeneratedPassword(pw, value); +}, +``` + +- [ ] **Step 5: Run — expect PASS** + +```bash +cd extension && npx vitest run src/popup/components/types/__tests__/login.test.ts -t "regenerate handler" +``` + +- [ ] **Step 6: Manual verification** + +```bash +cd extension && npm run build +``` + +Load `extension/dist/` in Chrome, open the popup, edit a login, click the regenerate (orange `↻`) button, pick a generated password. Confirm the strength bar redraws and the entropy text reads roughly `~10^N guesses — beyond consumer-hardware reach` (or similar, depending on the password) — NOT the stale `~10^1 guesses — trivially crackable`. + +- [ ] **Step 7: Commit** + +```bash +git add extension/src/popup/components/types/login.ts \ + extension/src/popup/components/types/__tests__/login.test.ts +git commit -m "$(cat <<'EOF' +fix(ext/login): dispatch input event after regenerate sets password (B1) + +Programmatic input.value = newPassword does not fire input events, so +the strength-meter listener at shared/form-affordances/password-tools.ts:65 +never re-rates the new value — meter stays stuck on the prior reading. + +Extract applyGeneratedPassword(input, value) helper that sets value, type, +then dispatches new InputEvent('input', { bubbles: true }). Vitest covers +the dispatch + a sanity check that bubbling listeners fire. +EOF +)" +``` + +--- + +## Task 4 (P1 — Phase A): `colorizePassword()` pure utility + +**Files:** +- Create: `extension/src/shared/password-coloring.ts` +- Create: `extension/src/shared/__tests__/password-coloring.test.ts` + +- [ ] **Step 1: Write the failing tests** + +Create `extension/src/shared/__tests__/password-coloring.test.ts`: + +```ts +import { describe, it, expect, beforeEach } from 'vitest'; +import { JSDOM } from 'jsdom'; +import { colorizePassword, PWD_DIGIT, PWD_SYMBOL, PWD_LETTER } from '../password-coloring'; + +describe('colorizePassword', () => { + beforeEach(() => { + const dom = new JSDOM(''); + (global as any).document = dom.window.document; + }); + + function classes(frag: DocumentFragment): string[] { + return Array.from(frag.querySelectorAll('span')).map(s => s.className); + } + function texts(frag: DocumentFragment): string[] { + return Array.from(frag.querySelectorAll('span')).map(s => s.textContent ?? ''); + } + + it('returns empty fragment for empty input', () => { + const frag = colorizePassword(''); + expect(frag.childNodes.length).toBe(0); + }); + + it('classifies a mixed-class run', () => { + const frag = colorizePassword('aB3$xY'); + expect(classes(frag)).toEqual([PWD_LETTER, PWD_DIGIT, PWD_SYMBOL, PWD_LETTER]); + expect(texts(frag)).toEqual(['aB', '3', '$', 'xY']); + }); + + it('all-letters produces a single letter span', () => { + const frag = colorizePassword('passwd'); + expect(classes(frag)).toEqual([PWD_LETTER]); + expect(texts(frag)).toEqual(['passwd']); + }); + + it('all-digits produces a single digit span', () => { + const frag = colorizePassword('123456'); + expect(classes(frag)).toEqual([PWD_DIGIT]); + expect(texts(frag)).toEqual(['123456']); + }); + + it('all-symbols produces a single symbol span', () => { + const frag = colorizePassword('!@#$%^'); + expect(classes(frag)).toEqual([PWD_SYMBOL]); + expect(texts(frag)).toEqual(['!@#$%^']); + }); + + it('classifies unicode letters as letters', () => { + const frag = colorizePassword('áñü'); + expect(classes(frag)).toEqual([PWD_LETTER]); + }); + + it('classifies whitespace as symbol', () => { + const frag = colorizePassword('a b'); + expect(classes(frag)).toEqual([PWD_LETTER, PWD_SYMBOL, PWD_LETTER]); + expect(texts(frag)).toEqual(['a', ' ', 'b']); + }); + + it('representative password snapshot: aB3$xY7&_!', () => { + const frag = colorizePassword('aB3$xY7&_!'); + expect(classes(frag)).toEqual([ + PWD_LETTER, PWD_DIGIT, PWD_SYMBOL, PWD_LETTER, PWD_DIGIT, PWD_SYMBOL, + ]); + expect(texts(frag)).toEqual(['aB', '3', '$', 'xY', '7', '&_!']); + }); +}); +``` + +- [ ] **Step 2: Run — expect FAIL** + +```bash +cd extension && npx vitest run src/shared/__tests__/password-coloring.test.ts +``` + +Expected: `Cannot find module '../password-coloring'`. + +- [ ] **Step 3: Implement** + +Create `extension/src/shared/password-coloring.ts`: + +```ts +export const PWD_DIGIT = 'pwd-digit'; +export const PWD_SYMBOL = 'pwd-symbol'; +export const PWD_LETTER = 'pwd-letter'; + +type Class = typeof PWD_DIGIT | typeof PWD_SYMBOL | typeof PWD_LETTER; + +function classify(ch: string): Class { + if (/^\d$/.test(ch)) return PWD_DIGIT; + if (/^\p{L}$/u.test(ch)) return PWD_LETTER; + return PWD_SYMBOL; +} + +/** + * Split `text` into runs of same-class codepoints and return a DocumentFragment + * of class-named nodes (one span per run). Returns an empty fragment + * for empty input. + * + * Pure: does not mutate any DOM outside the returned fragment, does not perform + * I/O. Safe to call on every render. + */ +export function colorizePassword(text: string): DocumentFragment { + const frag = document.createDocumentFragment(); + if (text.length === 0) return frag; + + const codepoints = Array.from(text); + let runStart = 0; + let runClass = classify(codepoints[0]); + + for (let i = 1; i <= codepoints.length; i++) { + const c = i < codepoints.length ? classify(codepoints[i]) : null; + if (c !== runClass) { + const span = document.createElement('span'); + span.className = runClass; + span.textContent = codepoints.slice(runStart, i).join(''); + frag.appendChild(span); + if (c !== null) { + runStart = i; + runClass = c; + } + } + } + return frag; +} +``` + +- [ ] **Step 4: Run — expect PASS** + +```bash +cd extension && npx vitest run src/shared/__tests__/password-coloring.test.ts +``` + +Expected: 8 PASS. + +- [ ] **Step 5: Commit** + +```bash +git add extension/src/shared/password-coloring.ts \ + extension/src/shared/__tests__/password-coloring.test.ts +git commit -m "feat(ext/shared): add colorizePassword utility" +``` + +--- + +## Task 5 (P1 — Phase B): `applyColorScheme()` + storage round-trip + +**Files:** +- Create: `extension/src/shared/color-scheme.ts` +- Create: `extension/src/shared/__tests__/color-scheme.test.ts` + +- [ ] **Step 1: Write the failing tests** + +Create `extension/src/shared/__tests__/color-scheme.test.ts`: + +```ts +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { JSDOM } from 'jsdom'; +import { + loadColorScheme, saveColorScheme, resetColorScheme, applyColorScheme, + DEFAULT_DIGIT_COLOR, DEFAULT_SYMBOL_COLOR, +} from '../color-scheme'; + +function mockChromeStorage(initial: any = {}) { + const store = { ...initial }; + (global as any).chrome = { + storage: { + sync: { + get: vi.fn((key: string) => Promise.resolve( + key in store ? { [key]: store[key] } : {})), + set: vi.fn((kv: any) => { Object.assign(store, kv); return Promise.resolve(); }), + remove: vi.fn((key: string) => { delete store[key]; return Promise.resolve(); }), + }, + }, + }; + return store; +} + +describe('color-scheme storage', () => { + beforeEach(() => { + const dom = new JSDOM(''); + (global as any).document = dom.window.document; + }); + + it('load returns defaults when storage is empty', async () => { + mockChromeStorage(); + const scheme = await loadColorScheme(); + expect(scheme.digit_color).toBe(DEFAULT_DIGIT_COLOR); + expect(scheme.symbol_color).toBe(DEFAULT_SYMBOL_COLOR); + }); + + it('load returns stored values when present', async () => { + mockChromeStorage({ + password_display_scheme: { digit_color: '#123456', symbol_color: '#abcdef' }, + }); + const scheme = await loadColorScheme(); + expect(scheme.digit_color).toBe('#123456'); + expect(scheme.symbol_color).toBe('#abcdef'); + }); + + it('save round-trips', async () => { + mockChromeStorage(); + await saveColorScheme({ digit_color: '#111111', symbol_color: '#222222' }); + const scheme = await loadColorScheme(); + expect(scheme).toEqual({ digit_color: '#111111', symbol_color: '#222222' }); + }); + + it('reset removes the storage key', async () => { + const store = mockChromeStorage({ + password_display_scheme: { digit_color: '#000000', symbol_color: '#ffffff' }, + }); + await resetColorScheme(); + expect(store.password_display_scheme).toBeUndefined(); + const scheme = await loadColorScheme(); + expect(scheme.digit_color).toBe(DEFAULT_DIGIT_COLOR); + }); + + it('apply sets CSS custom properties on document.documentElement', async () => { + mockChromeStorage({ + password_display_scheme: { digit_color: '#deadbe', symbol_color: '#feed00' }, + }); + await applyColorScheme(); + const root = document.documentElement.style; + expect(root.getPropertyValue('--relicario-pwd-digit-color').trim()).toBe('#deadbe'); + expect(root.getPropertyValue('--relicario-pwd-symbol-color').trim()).toBe('#feed00'); + }); + + it('save rejects malformed hex values', async () => { + mockChromeStorage(); + await expect(saveColorScheme({ digit_color: 'not-a-color', symbol_color: '#ffffff' })) + .rejects.toThrow(); + }); +}); +``` + +- [ ] **Step 2: Run — expect FAIL** + +```bash +cd extension && npx vitest run src/shared/__tests__/color-scheme.test.ts +``` + +- [ ] **Step 3: Implement** + +Create `extension/src/shared/color-scheme.ts`: + +```ts +export const DEFAULT_DIGIT_COLOR = '#2563eb'; +export const DEFAULT_SYMBOL_COLOR = '#dc2626'; +const STORAGE_KEY = 'password_display_scheme'; +const HEX_RE = /^#[0-9a-fA-F]{6}$/; + +export interface ColorScheme { + digit_color: string; + symbol_color: string; +} + +export const DEFAULT_SCHEME: ColorScheme = { + digit_color: DEFAULT_DIGIT_COLOR, + symbol_color: DEFAULT_SYMBOL_COLOR, +}; + +function isValid(s: ColorScheme): boolean { + return HEX_RE.test(s.digit_color) && HEX_RE.test(s.symbol_color); +} + +export async function loadColorScheme(): Promise { + const result = await chrome.storage.sync.get(STORAGE_KEY); + const stored = result[STORAGE_KEY] as Partial | undefined; + if (!stored) return { ...DEFAULT_SCHEME }; + return { + digit_color: typeof stored.digit_color === 'string' && HEX_RE.test(stored.digit_color) + ? stored.digit_color : DEFAULT_DIGIT_COLOR, + symbol_color: typeof stored.symbol_color === 'string' && HEX_RE.test(stored.symbol_color) + ? stored.symbol_color : DEFAULT_SYMBOL_COLOR, + }; +} + +export async function saveColorScheme(scheme: ColorScheme): Promise { + if (!isValid(scheme)) { + throw new Error('Invalid color values; expected #rrggbb hex strings.'); + } + await chrome.storage.sync.set({ [STORAGE_KEY]: scheme }); +} + +export async function resetColorScheme(): Promise { + await chrome.storage.sync.remove(STORAGE_KEY); +} + +/// Read the stored scheme (or defaults) and apply the colors as inline CSS +/// custom properties on document.documentElement. Idempotent — safe to call +/// from popup/vault boot and from a chrome.storage.onChanged handler. +export async function applyColorScheme(): Promise { + const scheme = await loadColorScheme(); + const root = document.documentElement.style; + root.setProperty('--relicario-pwd-digit-color', scheme.digit_color); + root.setProperty('--relicario-pwd-symbol-color', scheme.symbol_color); +} +``` + +- [ ] **Step 4: Run — expect PASS** + +```bash +cd extension && npx vitest run src/shared/__tests__/color-scheme.test.ts +``` + +- [ ] **Step 5: Commit** + +```bash +git add extension/src/shared/color-scheme.ts \ + extension/src/shared/__tests__/color-scheme.test.ts +git commit -m "feat(ext/shared): color-scheme storage + applyColorScheme" +``` + +--- + +## Task 6 (P1 — Phase C): CSS rules + custom-property defaults + +**Files:** +- Modify: `extension/src/popup/styles.css` +- Modify: `extension/src/vault/vault.css` + +- [ ] **Step 1: Append rules to popup/styles.css** + +Append to `extension/src/popup/styles.css`: + +```css +/* Password coloring (P1) — character-class display colors. Custom properties + are overridden at runtime by applyColorScheme() reading chrome.storage.sync. */ +:root { + --relicario-pwd-digit-color: #2563eb; + --relicario-pwd-symbol-color: #dc2626; +} +.pwd-digit { color: var(--relicario-pwd-digit-color); } +.pwd-symbol { color: var(--relicario-pwd-symbol-color); } +.pwd-letter { color: inherit; } +``` + +(If `:root` is already declared earlier in the file, fold the two new custom properties into the existing `:root` block instead of re-declaring.) + +- [ ] **Step 2: Append the same rules to vault.css** + +Same block appended to `extension/src/vault/vault.css`. Same `:root`-merge note applies. + +- [ ] **Step 3: Build** + +```bash +cd extension && npm run build 2>&1 | tail -5 +``` + +Expected: webpack compiles cleanly. + +- [ ] **Step 4: Commit** + +```bash +git add extension/src/popup/styles.css extension/src/vault/vault.css +git commit -m "style(ext): add password-coloring CSS rules + custom property defaults" +``` + +--- + +## Task 7 (P1 — Phase D.1): Wire field-history viewer to colorizePassword + +**Files:** +- Modify: `extension/src/popup/components/field-history.ts` + +- [ ] **Step 1: Locate the revealed-value render** + +```bash +grep -n "history-entry__value\|revealed" extension/src/popup/components/field-history.ts +``` + +The line near 72 reads roughly: + +```ts +
${displayValue}
+``` + +`displayValue` is HTML-escaped string interpolation. Switch to imperative DOM patch since `colorizePassword()` returns a fragment. + +- [ ] **Step 2: Update render** + +After the template renders the entry markup, replace the revealed cell's text content imperatively. Add: + +```ts +import { colorizePassword } from '../../shared/password-coloring'; + +// after innerHTML / template render: +container.querySelectorAll('.history-entry__value.revealed').forEach((el, idx) => { + el.textContent = ''; + el.appendChild(colorizePassword(revealedValues[idx])); +}); +``` + +(`revealedValues` is whatever array of revealed-entry strings the existing render already computes. Adapt to actual variable names in this file.) + +- [ ] **Step 3: Build and manual check** + +```bash +cd extension && npm run build +``` + +Open the popup, view a password's field history, reveal an entry — confirm digits render blue and symbols render red. + +- [ ] **Step 4: Commit** + +```bash +git add extension/src/popup/components/field-history.ts +git commit -m "feat(ext/popup/field-history): colorize revealed password entries" +``` + +--- + +## Task 8 (P1 — Phase D.2): Wire popup item-detail password reveal + +**Files:** +- Modify: the popup component that renders the password field's revealed value + +- [ ] **Step 1: Find the surface** + +```bash +grep -rn "field.*[Pp]assword\|FieldKind.Password\|reveal-password" extension/src/popup/components/ +``` + +Identify the component that branches on `field.kind === FieldKind.Password` and renders the revealed string. Likely candidates: an item-detail / item-view component. + +- [ ] **Step 2: Apply the imperative pattern** + +```ts +import { colorizePassword } from '../../shared/password-coloring'; + +passwordValueEl.textContent = ''; +if (revealed) { + passwordValueEl.appendChild(colorizePassword(field.value)); +} else { + passwordValueEl.textContent = '••••••••'; +} +``` + +- [ ] **Step 3: Manual check** + +Build, load, reveal a password in the popup item view — confirm coloring. + +- [ ] **Step 4: Commit** + +```bash +git add extension/src/popup/components/ +git commit -m "feat(ext/popup/item-detail): colorize revealed password field" +``` + +--- + +## Task 9 (P1 — Phase D.3): Wire fullscreen vault item-detail + +**Files:** +- Modify: the equivalent component under `extension/src/vault/` + +- [ ] **Step 1: Find the surface** + +```bash +grep -rn "FieldKind.Password\|reveal.*password" extension/src/vault/ +``` + +- [ ] **Step 2: Apply the same pattern as Task 8** + +Same code shape, different file. + +- [ ] **Step 3: Manual check** + +Open the fullscreen vault, reveal a password — confirm coloring. + +- [ ] **Step 4: Commit** + +```bash +git add extension/src/vault/ +git commit -m "feat(ext/vault): colorize revealed password field in fullscreen view" +``` + +--- + +## Task 10 (P1 — Phase D.4): Wire generator preview + +**Files:** +- Modify: the generator-panel component + +- [ ] **Step 1: Find the surface** + +```bash +grep -rn "generated\|preview" extension/src/popup/components/generator-panel*.ts +``` + +The panel has a live preview element that updates each roll. + +- [ ] **Step 2: Apply imperative pattern** + +```ts +import { colorizePassword } from '../../shared/password-coloring'; + +previewEl.textContent = ''; +previewEl.appendChild(colorizePassword(generatedPassword)); +``` + +- [ ] **Step 3: Manual check** + +Open the generator, click roll a few times — confirm preview shows colored characters. + +- [ ] **Step 4: Commit** + +```bash +git add extension/src/popup/components/generator-panel*.ts +git commit -m "feat(ext/generator): colorize live password preview" +``` + +--- + +## Task 11 (P1 — Phase E): Boot wiring on popup + vault + +**Files:** +- Modify: `extension/src/popup/popup.ts` +- Modify: `extension/src/vault/vault.ts` + +- [ ] **Step 1: Add the call in popup boot** + +Near the top of the popup's `init()` / `main()` (whichever runs on `DOMContentLoaded`): + +```ts +import { applyColorScheme } from '../shared/color-scheme'; + +await applyColorScheme(); + +chrome.storage.onChanged.addListener((changes, area) => { + if (area === 'sync' && 'password_display_scheme' in changes) { + void applyColorScheme(); + } +}); +``` + +- [ ] **Step 2: Add the call in vault boot** + +Same imports + same two blocks in `extension/src/vault/vault.ts`'s boot function. + +- [ ] **Step 3: Manual verification** + +Open both surfaces. (Settings UI for live edits arrives in Task 12 — for this task, manually pre-populate `chrome.storage.sync` via DevTools Application panel and confirm the colors apply on next reload.) + +- [ ] **Step 4: Commit** + +```bash +git add extension/src/popup/popup.ts extension/src/vault/vault.ts +git commit -m "feat(ext): apply color scheme on popup + vault startup" +``` + +--- + +## Task 12 (P1 — Phase F): Display section in settings + +**Files:** +- Modify: `extension/src/popup/components/settings.ts` +- Modify: `extension/src/popup/components/__tests__/settings.test.ts` +- Modify: `extension/src/popup/styles.css` + +- [ ] **Step 1: Find the existing settings section pattern** + +```bash +grep -n "section\|render" extension/src/popup/components/settings.ts | head -30 +``` + +Identify the function that builds a section group + child controls. + +- [ ] **Step 2: Add the Display section** + +Following the existing pattern, render two color pickers, a sample swatch, and a reset button: + +```ts +import { + loadColorScheme, saveColorScheme, resetColorScheme, + DEFAULT_DIGIT_COLOR, DEFAULT_SYMBOL_COLOR, +} from '../../shared/color-scheme'; +import { colorizePassword } from '../../shared/password-coloring'; + +const SAMPLE = 'Abc123!@#xyz'; + +async function renderDisplaySection(parent: HTMLElement): Promise { + const section = document.createElement('div'); + section.className = 'settings-section'; + section.innerHTML = ` +

Display

+
+ + +
+
+ + +
+
+ + `; + parent.appendChild(section); + + const scheme = await loadColorScheme(); + const digitInput = section.querySelector('#display-digit-color')!; + const symbolInput = section.querySelector('#display-symbol-color')!; + const swatch = section.querySelector('#display-swatch')!; + const resetBtn = section.querySelector('#display-reset')!; + + digitInput.value = scheme.digit_color; + symbolInput.value = scheme.symbol_color; + + const updateSwatch = () => { + swatch.style.setProperty('--relicario-pwd-digit-color', digitInput.value); + swatch.style.setProperty('--relicario-pwd-symbol-color', symbolInput.value); + swatch.textContent = ''; + swatch.appendChild(colorizePassword(SAMPLE)); + }; + updateSwatch(); + + const onChange = async () => { + updateSwatch(); + try { + await saveColorScheme({ + digit_color: digitInput.value, + symbol_color: symbolInput.value, + }); + } catch { + // Browser color inputs always emit valid hex; nothing to surface. + } + }; + digitInput.addEventListener('change', onChange); + symbolInput.addEventListener('change', onChange); + + resetBtn.addEventListener('click', async () => { + digitInput.value = DEFAULT_DIGIT_COLOR; + symbolInput.value = DEFAULT_SYMBOL_COLOR; + await resetColorScheme(); + updateSwatch(); + }); +} +``` + +Wire `renderDisplaySection(parent)` into the existing settings render function alongside the other sections. + +- [ ] **Step 3: Add swatch styling to popup/styles.css** + +Append to `extension/src/popup/styles.css`: + +```css +.color-preview-swatch { + font-family: ui-monospace, monospace; + font-size: 1.1rem; + padding: 8px 12px; + border: 1px solid var(--border-mid); + border-radius: 4px; + margin-top: 8px; + background: var(--bg-input); +} +.color-preview-swatch .pwd-digit { color: var(--relicario-pwd-digit-color); } +.color-preview-swatch .pwd-symbol { color: var(--relicario-pwd-symbol-color); } +.color-preview-swatch .pwd-letter { color: inherit; } +``` + +The custom properties are scoped to `.color-preview-swatch` itself via inline `style.setProperty` in the JS — handy so the swatch previews changes independent of the global root scheme. + +- [ ] **Step 4: Add settings tests** + +Extend `extension/src/popup/components/__tests__/settings.test.ts` with: + +```ts +it('Display section round-trips digit color to chrome.storage.sync', async () => { + // Mount settings, find #display-digit-color, dispatch change with '#abcdef', + // assert chrome.storage.sync.set was called with + // { password_display_scheme: { digit_color: '#abcdef', symbol_color: ... } } +}); + +it('Reset button removes the storage key and restores the swatch defaults', async () => { + // Mount settings, click #display-reset, assert chrome.storage.sync.remove + // was called with 'password_display_scheme', and the swatch contains the + // default-color spans. +}); +``` + +(Detail-fill the test bodies following the existing settings.test.ts mocking style.) + +- [ ] **Step 5: Run all extension tests** + +```bash +cd extension && npx vitest run +``` + +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add extension/src/popup/components/settings.ts \ + extension/src/popup/components/__tests__/settings.test.ts \ + extension/src/popup/styles.css +git commit -m "feat(ext/settings): Display section with color pickers + swatch + reset" +``` + +--- + +## Task 13 (P3): Constrain lower form sections to match `.form-grid` envelope + +**Files:** +- Modify: `extension/src/vault/vault.css` +- Modify: `extension/src/popup/components/types/login.ts` (wrap lower sections in a constrained container) + +- [ ] **Step 1: Confirm the layout shape** + +In `extension/src/popup/components/types/login.ts:344-365`, the form renders: + +```ts +${sectionsHtml} // .form-grid (max-width: 960px; margin: 0 auto) +
notes…
// full-width — visual rhythm break +${renderSectionsEditor(...)} // disclosure — full-width +${renderAttachmentsDisclosure(...)} // disclosure — full-width +
…
// full-width (or hidden in fullscreen) +``` + +The cards inside `.form-grid` sit at `max-width: 960px; margin: 0 auto` (`vault.css:1585-1590`); the lower sections inherit no width constraint and stretch the full pane width. P3's fix: wrap the lower sections in a `.form-lower` container with the same envelope. + +- [ ] **Step 2: Wrap lower sections in `.form-lower`** + +In `extension/src/popup/components/types/login.ts`, change the block starting at line ~351 to: + +```ts +${sectionsHtml} +
+
+
+ + +
+ +
+ + ${renderSectionsEditor(sectionsDraft, sectionsExpanded)} + ${isInTab() ? renderAttachmentsDisclosure({ itemId: existing?.id ?? '', attachments: attachmentsDraft, mode: 'edit' }) : ''} +
+ + +
+
+``` + +The `.form-lower` class is gated on `surface === 'fullscreen'` so the popup keeps its current single-column behavior (already constrained by the popup's narrow viewport). + +- [ ] **Step 3: Add `.form-lower` rule to vault.css** + +Append to `extension/src/vault/vault.css` (next to `.form-grid` around line 1585): + +```css +/* P3: lower form sections (notes, custom-fields disclosure, attachments, + form actions) constrained to the same envelope as .form-grid so the + visual rhythm doesn't break at the 2-col → full-width transition. */ +.form-lower { + max-width: 960px; + margin: 0 auto; + padding: 0 0; /* matches .form-grid's lack of horizontal padding */ +} +.form-lower > .form-group, +.form-lower > .disclosure, +.form-lower > .attachments-disclosure, +.form-lower > .form-actions { + /* Items inherit the envelope from .form-lower; no per-child max-width. */ + width: 100%; +} +``` + +- [ ] **Step 4: Build** + +```bash +cd extension && npm run build 2>&1 | tail -3 +``` + +- [ ] **Step 5: Manual viewport sweep** + +Load `extension/dist/` in Chrome. Open a new-login form in the fullscreen vault tab. Resize the viewport at each of these widths and confirm the lower sections (notes, custom-fields disclosure, attachments disclosure, form actions if visible) align horizontally with the IDENTITY/CREDENTIALS card grid above: + +- 1920×1080 (DevTools desktop) +- 1440×900 +- 1024×768 — borderline tablet; cards are still 2-col here per `@media (max-width: 720px)` +- 768×1024 — under the 720px breakpoint, `.form-grid` collapses to 1-col; `.form-lower` stays at `max-width: 960px` but viewport is narrower, so it just fills the pane (no jarring transition because there are no cards beside it either) + +Each viewport: confirm no jarring left/right edge change between the cards and the lower sections. + +- [ ] **Step 6: Commit** + +```bash +git add extension/src/popup/components/types/login.ts extension/src/vault/vault.css +git commit -m "$(cat <<'EOF' +fix(ext/login): constrain lower form sections to .form-grid envelope (P3) + +Notes, custom-fields disclosure, attachments disclosure, and form-actions +in fullscreen logins now sit inside a .form-lower wrapper with the same +max-width: 960px; margin: 0 auto envelope as .form-grid above. Removes +the visual rhythm break at the 2-col → full-width transition. + +Popup keeps its current single-column behavior (gated on surface flag). +EOF +)" +``` + +--- + +## Task 14 (P2): Setup-wizard completion → fullscreen vault tab + +**Files:** +- Modify: `extension/src/setup/setup.ts` +- Create or extend: `extension/src/setup/__tests__/setup.test.ts` + +- [ ] **Step 1: Locate the terminal-step success path** + +In `extension/src/setup/setup.ts`, the register-device handler at line ~1042 sets `state.configPushed = true; render();` at line ~1102 after successful device registration. That's the terminal success — no further navigation. The user is left looking at a static "device registered" success-box. + +P2's job: after `state.configPushed = true; render();`, open the fullscreen vault tab and close the setup tab. + +- [ ] **Step 2: Write the failing test** + +Create or extend `extension/src/setup/__tests__/setup.test.ts`: + +```ts +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { finishSetup } from '../setup'; + +describe('finishSetup', () => { + beforeEach(() => { + (global as any).chrome = { + tabs: { + create: vi.fn(() => Promise.resolve({ id: 999 })), + getCurrent: vi.fn(() => Promise.resolve({ id: 42 })), + remove: vi.fn(() => Promise.resolve()), + }, + runtime: { + getURL: vi.fn((p: string) => `chrome-extension://abc/${p}`), + }, + }; + }); + + it('opens vault.html in a new tab', async () => { + await finishSetup(); + expect(chrome.runtime.getURL).toHaveBeenCalledWith('vault.html'); + expect(chrome.tabs.create).toHaveBeenCalledWith({ + url: 'chrome-extension://abc/vault.html', + }); + }); + + it('closes the current setup tab after opening the vault tab', async () => { + await finishSetup(); + expect(chrome.tabs.getCurrent).toHaveBeenCalled(); + expect(chrome.tabs.remove).toHaveBeenCalledWith(42); + }); + + it('still opens the vault tab even if closing the setup tab fails', async () => { + (chrome.tabs.remove as any).mockRejectedValueOnce(new Error('no permission')); + await expect(finishSetup()).resolves.not.toThrow(); + expect(chrome.tabs.create).toHaveBeenCalled(); + }); +}); +``` + +- [ ] **Step 3: Run — expect FAIL** + +```bash +cd extension && npx vitest run src/setup/__tests__/setup.test.ts +``` + +Expected: `finishSetup` not exported. + +- [ ] **Step 4: Implement `finishSetup`** + +In `extension/src/setup/setup.ts`, add (export it so tests can import): + +```ts +/// Hand off from the setup wizard's terminal success state to the fullscreen +/// vault tab. Opens vault.html in a new tab and closes this setup tab. +/// Errors closing the setup tab are swallowed — the vault tab is what matters. +export async function finishSetup(): Promise { + const vaultUrl = chrome.runtime.getURL('vault.html'); + await chrome.tabs.create({ url: vaultUrl }); + try { + const current = await chrome.tabs.getCurrent(); + if (current?.id !== undefined) { + await chrome.tabs.remove(current.id); + } + } catch { + // Setup tab may not be closeable (e.g., if it was opened as a popup, not a tab). + // The vault tab is open — that's the user-visible success. + } +} +``` + +In `attachStep5` (line ~1102 area), change: + +```ts +state.configPushed = true; +render(); +``` + +to: + +```ts +state.configPushed = true; +render(); +// Hand off to the fullscreen vault tab. +void finishSetup(); +``` + +- [ ] **Step 5: Run — expect PASS** + +```bash +cd extension && npx vitest run src/setup/__tests__/setup.test.ts +``` + +- [ ] **Step 6: Manual smoke test (both setup modes)** + +```bash +cd extension && npm run build +``` + +Load `extension/dist/`, run through the new-vault flow end-to-end: + +1. Step 0: pick "create new" +2. …reach Step 5, click "register this device" +3. After "device registered" appears, the setup tab should close and the fullscreen vault tab should open at `chrome-extension:///vault.html`. + +Repeat with the attach-existing flow (Step 0: "attach existing") — same handoff expected. + +- [ ] **Step 7: Commit** + +```bash +git add extension/src/setup/setup.ts extension/src/setup/__tests__/setup.test.ts +git commit -m "$(cat <<'EOF' +feat(ext/setup): hand off completion to fullscreen vault tab (P2) + +After successful device registration (state.configPushed = true), the +wizard now opens vault.html in a new tab and closes the setup tab. +Both create-new and attach-existing flows funnel through the same +finishSetup() handler. Closing the setup tab is best-effort — +chrome.tabs.remove failures don't block the vault open. +EOF +)" +``` + +--- + +## Task 15: Final verification + +- [ ] **Step 1: Run the full test suite** + +```bash +cd extension && npx vitest run +``` + +Expected: all tests pass. + +- [ ] **Step 2: Build for production (Chrome + Firefox)** + +```bash +cd extension && npm run build:all 2>&1 | tail -5 +``` + +Expected: webpack compiles both targets with no errors (only the existing 4MB WASM warning). + +- [ ] **Step 3: End-to-end smoke test** + +Load `extension/dist/` in Chrome via `chrome://extensions` → Developer mode → Load unpacked. Walk through the v0.5.0 Plan B acceptance set: + +1. **B1 (regenerate desync):** Edit a login, click `↻`, pick a generated password. Strength bar updates and entropy text reflects the new password's strength. No "trivially crackable" stale reading. +2. **P4 (vault_locked friendly copy):** With the vault locked, open the fullscreen tab, attempt an action that returns `vault_locked`. Error block reads "Vault locked / Unlock your vault to continue. / [Unlock vault]". No `vault_locked` snake_case anywhere. Click "Unlock vault" — passphrase input gets focus. +3. **P4 (other codes):** Trigger `origin_mismatch` (try to fill from a host different than the item's URL) — block reads "Wrong site / This login belongs to a different site …". Trigger `unauthorized_sender` (e.g., bad CSP context) — block reads "Action not allowed / This action is not allowed from here.". +4. **P1 (coloring):** Reveal a password with mixed character classes in the popup item view, the fullscreen item view, the field-history viewer, and the generator preview. Digits render blue, symbols render red, letters inherit. Open settings → Display, change the digit color via the picker, observe the swatch update live. Reload the popup — the new color persists. +5. **P3 (form layout):** Open a new-login form in the fullscreen tab. Identity/Credentials cards align with notes / custom-fields / attachments below them at all four viewport sizes (1920×1080, 1440×900, 1024×768, 768×1024). +6. **P2 (setup → vault):** From a fresh extension install, run setup all the way through. After clicking "register this device" at step 5, the setup tab closes and the fullscreen vault tab opens. + +- [ ] **Step 4: Fix any smoke-test follow-ups** + +If anything regresses, fix and commit with `style(ext): polish smoke-test fixes` or similar. Otherwise no extra commit. + +--- + +## Completion Checklist + +- [ ] Task 1 (P4): `ERROR_COPY` map + popup `humanizeError` rewrite +- [ ] Task 2 (P4): Fullscreen vault `renderErrorBlock` (closes B2) +- [ ] Task 3 (B1): Regenerate dispatches `input` event +- [ ] Task 4 (P1-A): `colorizePassword()` utility +- [ ] Task 5 (P1-B): Color-scheme storage + `applyColorScheme()` +- [ ] Task 6 (P1-C): CSS rules + custom properties +- [ ] Task 7 (P1-D.1): Field-history viewer +- [ ] Task 8 (P1-D.2): Popup item-detail +- [ ] Task 9 (P1-D.3): Fullscreen item-detail +- [ ] Task 10 (P1-D.4): Generator preview +- [ ] Task 11 (P1-E): Boot wiring on popup + vault +- [ ] Task 12 (P1-F): Display section in settings +- [ ] Task 13 (P3): `.form-lower` envelope constraint +- [ ] Task 14 (P2): Setup completion → vault tab +- [ ] Task 15: Final verification + +--- + +## Self-Review Notes + +**Spec coverage:** +- B1: Tasks 3 (input-event dispatch + vitest). +- B2: Folded into Task 2 (fullscreen `renderErrorBlock` consumes ERROR_COPY). +- P1: Tasks 4–12 (utility, storage, CSS, four reveal surfaces, boot wiring, settings UI). Inlined verbatim from `docs/superpowers/plans/2026-05-01-password-coloring.md` so Plan B stands alone. +- P2: Task 14 (`finishSetup` + vitest on `chrome.tabs.create`). +- P3: Task 13 (Approach A from spec — constrain lower sections to `.form-grid` envelope, no card-wrapping). +- P4: Tasks 1 + 2 (centralized map, popup wiring, fullscreen wiring; generated test enumerates grep'd codes so it can't drift). + +**Placeholder scan:** No "TBD". The two "find via grep" steps in Tasks 8 and 10 reference search commands and the live source — they are not placeholders, they're locator hints because the password-reveal surfaces in the popup item-detail and generator-panel components weren't load-bearing enough to enumerate up front. The grep commands are exact. + +**Type / name consistency:** +- `ERROR_COPY`, `lookupErrorCopy`, `ErrorCopy`, `ErrorCta` consistent across Tasks 1 and 2. +- `colorizePassword`, `PWD_DIGIT/SYMBOL/LETTER`, `loadColorScheme/saveColorScheme/resetColorScheme/applyColorScheme`, `DEFAULT_DIGIT_COLOR/DEFAULT_SYMBOL_COLOR`, `ColorScheme`, `STORAGE_KEY = 'password_display_scheme'` consistent across Tasks 4–12. +- `applyGeneratedPassword(input, value)` introduced in Task 3 and not reused elsewhere — single call site at the gen-btn `onPicked`. +- `finishSetup` introduced in Task 14, called once from `attachStep5`. +- `.form-lower` wrapper class introduced in Task 13, gated on `surface === 'fullscreen'` so the popup is unaffected. + +**Stand-alone test:** A new engineer with zero context can execute this plan top-to-bottom. Each task has a concrete file list, a TDD-shaped sequence (write test → fail → implement → pass), the actual code to land, and a commit. Manual checks are bounded (specific viewports, specific error codes, specific reveal surfaces) so verification is unambiguous.