diff --git a/docs/superpowers/specs/2026-05-02-v0.5.0-polish-harden-design.md b/docs/superpowers/specs/2026-05-02-v0.5.0-polish-harden-design.md new file mode 100644 index 0000000..e90944e --- /dev/null +++ b/docs/superpowers/specs/2026-05-02-v0.5.0-polish-harden-design.md @@ -0,0 +1,340 @@ +# v0.5.0 — Polish + Harden — Design + +Date: 2026-05-02 +Status: Draft + +## Overview + +v0.5.0 is a "polish + harden" bundle: a security-vulnerability fix, two +hardening follow-ups, two confirmed bugs, and four UX improvements. +No new functional features. Functional work — LastPass import, recovery +QR, Plan 1C-γ, Fullscreen Phases 3/4 — proceeds on parallel plans. + +The anchor item is a **HIGH-severity authentication bypass** in +`relicario-server`'s pre-receive hook (S1): both the registered-device +check and the revocation check are unimplemented. Until this lands, the +device-auth system is a no-op. + +## Goals + +1. Restore device-auth integrity: only registered, non-revoked signing + keys can push to the vault repo. +2. Close two minor hardening gaps surfaced during the security review. +3. Fix two user-visible bugs (strength-meter desync after generate, + raw error codes in the fullscreen tab). +4. Deliver four UX improvements that are polish-shaped, not feature-shaped. +5. Tag v0.5.0 with no known security vulnerabilities or visible-error-code + leaks. + +## Scope Map + +| Bucket | Item | Plan | +|---|---|---| +| Security | S1: Pre-receive hook fix | A | +| Security | S2: Tar archive path-traversal hardening | A | +| Security | S3: `RELICARIO_*` env-var audit | A | +| Cleanup | C1: Stale feature branch prune | A | +| Bugs | B1: Strength meter desync after regenerate | B | +| Bugs | B2: `vault_locked` raw error code in fullscreen tab | B (subsumed by P4) | +| UX | P1: Password coloring | B | +| UX | P2: Setup-wizard completion → fullscreen vault tab | B | +| UX | P3: Form-layout 2-col → full-width transition | B | +| UX | P4: Error-message audit (snake_case codes → friendly copy) | B | + +Two plans split by language/blast-radius: +- **Plan A** = Rust + docs (server, CLI, env-var audit, branch cleanup) +- **Plan B** = Extension UX (TypeScript + CSS) + +The plans share no files and can ship independently. + +--- + +## Plan A — Security + Cleanup + +### S1. Pre-Receive Hook Fix (anchor) + +**Problem.** `crates/relicario-server/src/main.rs:36-81`: `verify_commit` +loads `devices` and `revoked` from the repo at the commit, then drops +both on the floor. It runs `git verify-commit --raw` and accepts on +`GOODSIG` / `Good signature` regardless of which key produced the +signature. This means: + +- An attacker who never registered a device can push commits signed + with any GPG key that the server's keyring/allowed-signers happens + to trust (or none at all if the server has no SSH allowed-signers + configured). +- A revoked device's signing key continues to authenticate after + `relicario device revoke` is run — the `revoked.json` write is + cosmetic. + +**Fix.** Implement the verification logic per the design spec +(`docs/superpowers/specs/2026-05-02-device-authentication-design.md:284-300`): + +1. Build a temporary allowed-signers file from `devices.json` entries + loaded at the commit. Pass it to `git verify-commit` via + `GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=gpg.ssh.allowedSignersFile + GIT_CONFIG_VALUE_0=` so we don't mutate global git config. +2. Parse the signing-key fingerprint out of `git verify-commit --raw` + output (the fingerprint appears on the `Good "git" signature for … + with … key SHA256:…` line for SSH). +3. Reject if the fingerprint is not in `devices.json` at the commit. +4. Reject if the fingerprint is in `revoked.json` AND + `commit_timestamp >= revoked_at`. The historical-commit case + (timestamp < `revoked_at`) is allowed so old commits survive + revocation. +5. Use the commit's **committer date** (`GIT_COMMITTER_DATE`, + accessible via `git show -s --format=%ct`) — *not* the current wall + clock and not the author date — as `commit_timestamp`. Committer + date is when the signature was applied; author date is when work + was originally written and could be from before revocation even + for a malicious replay. + +**Acceptance.** +- An integration test that registers a device, revokes it, signs a + commit with the revoked key dated AFTER `revoked_at`, and confirms + the hook exits non-zero. +- An integration test that signs a commit with a key that was never + registered and confirms the hook exits non-zero. +- An integration test that signs a commit with a registered, non-revoked + key and confirms the hook exits zero. +- An integration test that signs a commit with a revoked key dated + BEFORE `revoked_at` (historical case) and confirms the hook exits zero. +- The dead `let _ = &revoked;` line is gone. + +### S2. Tar Archive Path-Traversal Hardening + +**Problem.** `crates/relicario-cli/src/main.rs:1722` unpacks the +bundled `git_archive` from a `.relbak` via `tar::Archive::unpack`, +trusting the `tar` crate's defaults. A malicious `.relbak` could +contain entries with `..` components or absolute paths. + +**Fix.** Iterate entries explicitly: + +- Reject any entry whose path contains a `..` component, an absolute + prefix, or a Windows drive letter. +- Reject symlinks and hardlinks (we never write either inside `.git/` + on restore). +- Cap total uncompressed size at 100× the compressed `git_archive` + size or 1 GiB, whichever is lower (defends against tar bombs). +- Write each entry under `target.join(".git")` only after the path + resolves *inside* that directory. + +**Acceptance.** +- Unit test with a hand-crafted tar containing `../../etc/passwd` — + restore bails with "path traversal blocked". +- Unit test with a symlink entry — restore bails. +- Unit test with a 1 TiB sparse entry — restore bails on the size cap. +- Existing valid-restore test still passes. + +### S3. `RELICARIO_*` Env-Var Audit + +**Problem.** Three env vars are not gated by `cfg(debug_assertions)`: +- `RELICARIO_IMAGE` (`crates/relicario-cli/src/session.rs:125`) — reference-image override path. +- `RELICARIO_NO_GROUPS_CACHE` (`crates/relicario-cli/src/helpers.rs:103`) — disables the groups cache. +- `RELICARIO_GITEA_URL` (`crates/relicario-cli/src/main.rs:2298`) — Gitea base URL fallback. + +Spot-check confirmed these look intentional, but they're undocumented +and `RELICARIO_NO_GROUPS_CACHE` is more of a dev escape hatch than +production config. + +**Fix.** +- Document each env var in `docs/SECURITY.md` and the relevant + `--help` text. +- Move `RELICARIO_NO_GROUPS_CACHE` under `cfg(debug_assertions)` — + it's a dev tool, not a user knob. +- Leave `RELICARIO_IMAGE` and `RELICARIO_GITEA_URL` as user-facing + config; audit each call site to confirm no untrusted-input-flows-into-path + cases. + +**Acceptance.** +- `docs/SECURITY.md` lists every `RELICARIO_*` env var with purpose + and trust assumption. +- `RELICARIO_NO_GROUPS_CACHE` is no-op in `cargo build --release`. + +### C1. Stale Feature Branch Prune + +**Problem.** Six local branches with no merged history beyond what's +already in main: `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`. + +**Fix.** Verify each is fully merged into main (`git branch --merged +main`), then delete locally only — no remote branch operations. Plan +execution will surface the list and prompt the user before running +`git branch -D` per branch (per project rule on git-destructive ops). + +**Acceptance.** `git branch` shows only `main` and active feature +branches. + +--- + +## Plan B — Extension UX + +### B1. Strength Meter Desync After Regenerate + +**Problem.** Confirmed in screenshot from 2026-05-02: clicking the +regenerate button on a login form's password field rerolls a +20-character mixed-class password (e.g., `sCMtTJkF%GN^mF#-N6D%`) but +the strength meter still reports `~10^1 guesses — trivially crackable`. +The rated string is stale — likely whatever was in the field before +the reroll, or empty. + +The meter listens to `input` events on the password field +(`extension/src/shared/form-affordances/password-tools.ts:65`). The +regenerate handler sets `input.value = newPassword` programmatically, +which **does not fire** `input` events in standard DOM behavior. + +**Fix.** In the regenerate handler (search for the orange-spinner +button click), after assigning `input.value`, dispatch an +`InputEvent('input', { bubbles: true })`. Confirm by re-rating +inside the handler if needed. + +**Acceptance.** +- Vitest: simulate regenerate click, assert that `input` event is + dispatched and meter calls `scheduleRate` with the new value. +- Manual: regenerate a password on the login form, confirm meter + jumps to "strong" with appropriate `guesses_log10`. + +### P4. Error-Message Audit (subsumes B2) + +**Problem.** Snake_case error codes leak straight into the fullscreen +tab and other surfaces: + +- `vault_locked` shown as a red string in the new-login form + (screenshot 2026-05-02). +- ~20 call sites in `extension/src/service-worker/router/` return + `{ ok: false, error: 'vault_locked' }` and similar. + +The popup has a one-off mapping at `extension/src/popup/popup.ts:147` +(`/vault_locked/i.test(err)` → unlock prompt), but the fullscreen tab +has no equivalent. + +**Fix.** +1. Build a single `ERROR_COPY` map keyed by error code, returning + `{ title: string; body: string; cta?: { label: string; action: () => void } }`. + Place at `extension/src/shared/error-copy.ts`. +2. Audit call sites in `extension/src/service-worker/router/` for all + error codes; ensure each has a mapping. +3. Replace the regex in `popup.ts:147` and the raw-error rendering in + the fullscreen tab with `lookupErrorCopy(code)`. +4. For `vault_locked` specifically: the CTA is "Unlock vault" → opens + the popup unlock view (or in the fullscreen tab, the inline unlock + form). + +**Acceptance.** +- No raw `snake_case` strings render in any UI surface for any error + return path. Verified by grep + manual trigger of each error code. +- `vault_locked` in the fullscreen tab shows friendly copy + an + "Unlock vault" button that works. +- Vitest: enumerate every distinct `error: '...'` string literal + found in `extension/src/service-worker/router/` via grep, assert + each is a key in `ERROR_COPY`. Generated test or build-time check + preferred over a static snapshot so it can't drift. + +### P1. Password Coloring + +**Problem / Fix.** Implement per existing spec at +`docs/superpowers/specs/2026-05-01-password-coloring-design.md` and plan +at `docs/superpowers/plans/2026-05-01-password-coloring.md`. No design +changes; pulled into v0.5.0 because it's polish-flavored and small. + +**Acceptance.** Per existing plan's acceptance criteria. + +### P2. Setup-Wizard Completion → Fullscreen Vault Tab + +**Problem.** Setup wizard currently routes to the popup item-list view +on completion. The user wants to land in the fullscreen vault tab — +the experience reads as more "real software" and avoids the sub-300px +popup width on first run. + +**Fix.** In the setup wizard's terminal step, after the vault is +created and the device is registered: + +1. Open the fullscreen vault tab via `chrome.tabs.create({ url: + chrome.runtime.getURL('vault.html') })`. +2. Close the setup tab (or the popup, depending on entry point). + +**Acceptance.** +- Manual: complete the new-vault setup flow; vault tab opens, setup + tab closes. +- Manual: complete the attach-existing flow; same behavior. +- Vitest: assert `chrome.tabs.create` is called with the vault URL on + successful setup completion. + +### P3. Form-Layout 2-col → Full-Width Transition + +**Problem.** Screenshot 2026-05-02 shows the new-login form in the +fullscreen tab: the IDENTITY and CREDENTIALS cards sit in a 2-column +grid with a max-width that stops well short of viewport edge, but the +Notes textarea, custom-fields disclosure, and attachments disclosure +below all stretch full-width. The visual rhythm breaks at the +transition — the lower sections look like a different page. + +**Fix.** Two candidate approaches; pick at implementation time: + +- **A.** Constrain the lower sections to the same max-width and + horizontal alignment as the cards. They become a third "row" in the + same column system. Simpler, less code. +- **B.** Wrap the lower sections in a card matching the upper cards' + visual treatment. Notes-as-card, custom-fields-as-card, + attachments-as-card. More work, more visual consistency, but might + feel heavier. + +Recommended: **A** for v0.5.0 — minimal CSS change, immediate +correctness. Revisit B in Phase 3 if the user still feels the layout +is off. + +**Acceptance.** +- The new-login form (and all item-type forms — login, secure note, + identity, card, key, document, totp) renders with consistent + horizontal alignment from top of form to bottom. +- Manual: viewport at 1920×1080, 1440×900, 1024×768, and 768×1024; + no jarring width transitions. + +--- + +## Testing Strategy + +| Layer | Tests | +|---|---| +| Rust unit (`relicario-server`) | S1 acceptance set (4 scenarios) | +| Rust unit (`relicario-core`) | S2 path-traversal scenarios (3 scenarios) | +| CLI integration | S2 happy-path restore unchanged | +| Vitest (extension) | B1 input-event dispatch; P4 ERROR_COPY snapshot; P2 tabs.create call | +| Manual | P3 viewport sweep; B2/P4 trigger each error code; S1 hook setup on a real Gitea instance; C1 branch cleanup confirmation | + +Existing tests stay green. No regression budget. + +## Out of Scope + +Listed for clarity; each tracked separately: + +- LastPass import (`docs/superpowers/plans/2026-04-29-relicario-lastpass-import.md`) +- Recovery QR + entropy floor (`docs/superpowers/plans/2026-05-01-recovery-qr-and-entropy-floor.md`) +- Plan 1C-γ (attachments + Document + trash UI) +- Fullscreen Phase 3 (shell) and Phase 4 (palette) +- Pre-v0.3.0 manual test walk (`docs/test-checklists/2026-04-27-pre-v0.3.0-audit.md`) — that's a v0.3.0 release gate, not a v0.5.0 item + +## Sequencing + +1. Plan A and Plan B can run in parallel (no shared files). +2. Inside Plan A: S1 first (unblocks the security claim), then S2, + then S3, then C1. +3. Inside Plan B: P4 first (unblocks B2 and surfaces all error codes + centrally), then B1, then P1, then P3, then P2 (P2 is end-to-end + and benefits from earlier polish). +4. Both plans merge to main; tag `v0.5.0` after both PRs are green. + +## Risks + +- **S1 SSH allowed-signers parsing.** `git verify-commit --raw` + output format for SSH signatures differs slightly from GPG; the + fingerprint-extraction regex needs unit coverage against real + output. Mitigation: capture sample outputs from a test repo and + check them into the test fixtures. +- **P3 layout regressions.** Constraining the lower sections may + affect the textarea behavior at small widths. Mitigation: viewport + sweep in acceptance. +- **C1 accidental deletion.** Plan execution must `git branch + --merged main` filter and prompt before each `-D`. The + no-destructive-without-asking project rule applies.