From 03d0781c39a547b611e34b3c42caad754ae1cce0 Mon Sep 17 00:00:00 2001 From: adlee-was-taken Date: Wed, 6 May 2026 18:36:53 -0400 Subject: [PATCH] fix(ext): unswallow free() errors in SW session.clearCurrent + vitest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 added impl Drop for SessionHandle on the Rust side so .free() now actually removes the SESSIONS registry entry. The JS-side try { current.free() } catch { /* already freed */ } swallow was hiding the fact that .free() wasn't doing the cleanup at all; post-Phase-1 it has to go so failures surface instead of being lost. .free() callsite audit: exactly one match under extension/src/ — the SW session.ts line this commit edits. Lifecycle audit: clearCurrent() is reached via (a) popup lock → router popup-only.ts and (b) session-timer expiry → service-worker/index.ts. Refs: docs/superpowers/specs/2026-05-04-security-polish-design.md (Phase 2) Refs: docs/superpowers/reviews/2026-05-04-architecture-review.md (P1.1, DEV-C P2 service-worker) Co-Authored-By: Claude Opus 4.7 --- .../service-worker/__tests__/session.test.ts | 37 +++++++++++++++++++ extension/src/service-worker/session.ts | 8 +++- 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 extension/src/service-worker/__tests__/session.test.ts diff --git a/extension/src/service-worker/__tests__/session.test.ts b/extension/src/service-worker/__tests__/session.test.ts new file mode 100644 index 0000000..e83f5d4 --- /dev/null +++ b/extension/src/service-worker/__tests__/session.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; +import type { SessionHandle } from '../../../wasm/relicario_wasm'; +import { clearCurrent, getCurrent, setCurrent } from '../session'; + +describe('session', () => { + beforeEach(() => { + // Reset module-scope `current` between tests. Overwrite first with a + // benign no-throw fake so a prior test's throwing handle can't escape. + setCurrent({ free: vi.fn(), value: 0 } as unknown as SessionHandle); + clearCurrent(); + }); + + it('clearCurrent() is a no-op when no handle is set', () => { + expect(() => clearCurrent()).not.toThrow(); + expect(getCurrent()).toBeNull(); + }); + + it('clearCurrent() calls free() exactly once and clears current', () => { + const free = vi.fn(); + setCurrent({ free, value: 1 } as unknown as SessionHandle); + + clearCurrent(); + expect(free).toHaveBeenCalledTimes(1); + expect(getCurrent()).toBeNull(); + + clearCurrent(); + expect(free).toHaveBeenCalledTimes(1); + expect(getCurrent()).toBeNull(); + }); + + it('clearCurrent() propagates exceptions from free()', () => { + const free = vi.fn(() => { throw new Error('boom'); }); + setCurrent({ free, value: 2 } as unknown as SessionHandle); + + expect(() => clearCurrent()).toThrow(/boom/); + }); +}); diff --git a/extension/src/service-worker/session.ts b/extension/src/service-worker/session.ts index 89d0992..4859646 100644 --- a/extension/src/service-worker/session.ts +++ b/extension/src/service-worker/session.ts @@ -7,6 +7,12 @@ /// Future multi-vault (β+) would replace `current` with /// `Map` and thread `vaultId` through every /// handler. Deliberate α simplicity — not an oversight. +/// +/// As of Phase 1 of the security-polish series, `impl Drop for SessionHandle` +/// on the Rust side makes `.free()` the meaningful cleanup call: it removes +/// the entry from the SESSIONS registry and zeroizes `master_key` and +/// `image_secret`. Calling `wasm.lock(handle.value)` before `.free()` would +/// be redundant belt-and-suspenders; this module intentionally does not. import type { SessionHandle } from '../../wasm/relicario_wasm'; @@ -23,6 +29,6 @@ export function requireCurrent(): SessionHandle { export function clearCurrent(): void { if (!current) return; - try { current.free(); } catch { /* already freed */ } + current.free(); current = null; }