Vault cleanup: dead ctEqual, and a PrincipalKind union #145

New issue

Open

opened 2026-09-12 19:32:58 +02:00 by lz · 0 comments

lz commented

2026-09-12 19:32:58 +02:00

Owner

Two small items from the /pr-review pass over feat/mcp. Neither is urgent; both are cheap.

1. `ctEqual` is dead code

crypto/vault.ts:39. Grepped: the only callers are its own tests (crypto/vault.test.ts). It was written for the verifier check, but even on main that check used pt.equals(VERIFIER_PLAINTEXT), not ctEqual — so it has been dead since before envelope encryption, not because of it.

feat/mcp retires the verifier entirely, so its docstring ("Constant-time compare for verifier checks etc.") now names a concept that no longer exists.

Either delete it plus its three tests (~5 + ~12 lines), or keep it and fix the comment. Deleting is my preference — a constant-time compare is easy to re-add correctly, and a dead one invites someone to reach for it in a context where timingSafeEqual's length-leak matters.

Same file, unrelated: callerSid in rotate-passphrase/+server.ts is accepted, void-ed, and documented as "for future use". Also pre-existing.

2. `principal_kind` / `principal_id` are bare `string`

auth/vault-envelope.ts and VaultKeysTable in db/schema.ts. The schema comment already names the intended vocabulary ('master' | 'user' | 'token') but the type does not enforce it, so a typo compiles clean.

Failure mode if one ever slipped through: principalExists returns false → isSetupComplete false → /api/state says needs-setup → operator is routed to /setup → new DEK over a live vault. Same endpoint as #143, different cause.

A union type makes it a compile error. This was deliberately deferred when the table landed (one principal, no second caller yet); worth revisiting when the second principal arrives — i.e. alongside #140.

Two small items from the `/pr-review` pass over `feat/mcp`. Neither is urgent; both are cheap. ## 1. `ctEqual` is dead code `crypto/vault.ts:39`. Grepped: the only callers are its own tests (`crypto/vault.test.ts`). It was written for the verifier check, but even on `main` that check used `pt.equals(VERIFIER_PLAINTEXT)`, not `ctEqual` — so it has been dead since before envelope encryption, not because of it. `feat/mcp` retires the verifier entirely, so its docstring ("Constant-time compare for verifier checks etc.") now names a concept that no longer exists. Either delete it plus its three tests (~5 + ~12 lines), or keep it and fix the comment. Deleting is my preference — a constant-time compare is easy to re-add correctly, and a dead one invites someone to reach for it in a context where `timingSafeEqual`'s length-leak matters. Same file, unrelated: `callerSid` in `rotate-passphrase/+server.ts` is accepted, `void`-ed, and documented as "for future use". Also pre-existing. ## 2. `principal_kind` / `principal_id` are bare `string` `auth/vault-envelope.ts` and `VaultKeysTable` in `db/schema.ts`. The schema comment already names the intended vocabulary (`'master' | 'user' | 'token'`) but the type does not enforce it, so a typo compiles clean. Failure mode if one ever slipped through: `principalExists` returns false → `isSetupComplete` false → `/api/state` says `needs-setup` → operator is routed to `/setup` → new DEK over a live vault. Same endpoint as #143, different cause. A union type makes it a compile error. This was deliberately deferred when the table landed (one principal, no second caller yet); worth revisiting when the second principal arrives — i.e. alongside #140.

lz referenced this issue

2026-09-12 19:35:15 +02:00

Envelope-encrypt the vault: one DEK, per-principal wrapped keys #146

lz referenced this issue

2026-09-15 18:25:48 +02:00

Slice 2: machine principal, create_workspace and start_workspace #159