All security documentation

# Ledgerwire Security Review — 2026-06-10

**Reviewer:** code-reviewer subagent (Claude)
**Branch reviewed:** `feature/oauth-authorization-server` (diffed against `feature/quiltt-aggregator`)
**Scope:** Adversarial review of the REQ-047 OAuth 2.1 authorization layer — embedded authorization server (provider, consent, router, clients store), the four OAuth storage modules, dual-mode `/mcp` auth, dashboard "Connected apps", rate limiters, sweeper GC, and the phase-1 migrations.  Attack-the-design pass, not a line read.
**Method:** Read-only; no code changes during the review.  Full `pnpm test` run (green), `pnpm audit --prod --audit-level=high` (passes), and a runtime probe of better-sqlite3's default `foreign_keys` pragma.

Findings feed back into the PRD/TRD per project convention (see "Findings → backlog" at the end).  Fixes do NOT land in this review.

---

## Summary table

| # | Severity | Title | File:line |
|---|---|---|---|
| M1 | Medium | `foreign_keys` enforcement is implicit (better-sqlite3 default), never set, and the codebase comments assert the opposite | `src/storage/db.ts:385`, `src/web/mcp-bearer-auth.ts:262` |
| M2 | Medium | Refresh-token reuse detection is suppressible — scope/resource validation runs BEFORE rotation, so a reused token can avoid family revocation and acts as an existence oracle | `src/web/oauth/provider.ts:275-307` |
| M3 | Medium | New `@modelcontextprotocol/sdk@1.29.0` pin pulls 6 moderate transitive advisories into the prod tree (qs DoS reachable via the SDK body parsers; hono ×3) | `package.json`, `pnpm-lock.yaml` |
| L1 | Low | Consent transaction is a bearer capability not bound to the approving session (consent-phishing / fixation shape) | `src/web/oauth/consent.ts:257-318`, `provider.ts:151-171` |
| L2 | Low | SDK-mounted OAuth endpoints carry no explicit body-size limit (100kb default vs the 20kb global) | `src/web/oauth/router.ts:91-127` |
| L3 | Low | Wildcard CORS on `/token` and `/register` (SDK `cors()` default) | SDK `handlers/token.js`, `handlers/register.js` |
| L4 | Low | A legacy `mcp_tokens` value that happens to begin `lw_at_` is misrouted to the OAuth path (P ≈ 1.5e-11) | `src/web/mcp-bearer-auth.ts:242` |

No Critical or High findings.  The cross-user / open-redirect / PKCE / token-secrecy attack surfaces were probed and are sound (see "Verified sound" at the end).

---

## Medium findings

### M1 — `foreign_keys` enforcement is implicit and undocumented; the code asserts it is OFF

**Files:**
- `src/storage/db.ts:385` (`openDatabase` sets `journal_mode = WAL` but NO `foreign_keys` pragma)
- `src/web/mcp-bearer-auth.ts:262-263` (comment: "FK enforcement is OFF repo-wide, so this can genuinely happen after a user delete")
- Schema `REFERENCES users(id) ON DELETE CASCADE` clauses on `oauth_codes.user_id` (`db.ts:276`) and `oauth_tokens.user_id` (`db.ts:293`), plus the pre-existing `sessions` / `mcp_tokens` / `subscriptions` / `quiltt_connections` ones.

**Investigation (the phase-3 discrepancy, resolved definitively):** The phase-3 note reported that `db.ts` sets no `foreign_keys` pragma, yet a sweep test hit `FOREIGN KEY constraint failed` on `sessions.user_id`.  This is not a contradiction.  **better-sqlite3 turns `PRAGMA foreign_keys = ON` by default on every connection it opens** — unlike the bare SQLite C library and the `sqlite3` CLI, where the default is OFF.  Confirmed at runtime against the installed `better-sqlite3@9.6.0`:

```
const db = new Database(':memory:');
db.pragma('foreign_keys', {simple:true})   // => 1
// INSERT into a child table with a dangling user_id => "FOREIGN KEY constraint failed"
```

So FK enforcement IS active for the whole repo, including the two new OAuth tables' `ON DELETE CASCADE` clauses.  The real behaviour: deleting a `users` row cascades to delete that user's `oauth_codes` and `oauth_tokens` (good — clean tenant teardown), and you cannot insert an OAuth token/code referencing a non-existent user.

**Issue / impact:** The behaviour the system relies on is correct, but it is (a) **undocumented and implicit** — it depends entirely on a better-sqlite3 default that is not pinned anywhere in our code, and (b) **actively mis-stated** in `mcp-bearer-auth.ts:262`, whose 500 "token row outlived its user" branch is justified by a claim ("FK enforcement is OFF repo-wide") that is false.  The security-relevant fragility: the FK pragma is **per-connection** and is never asserted by us.  Any connection that does not inherit the better-sqlite3 default will silently NOT enforce the cascades — e.g. a Litestream restore verified with the `sqlite3` CLI, an ad-hoc ops query, a future driver swap, or a better-sqlite3 major-version change.  In that state, deleting a user would leave **orphaned, still-valid OAuth access tokens** pointing at a freed `user_id`, and `requireMcpAuth` would resolve them to a 500 (or, worse, if the id were later reused, to the wrong tenant).  A guarantee this load-bearing for tenant isolation should not rest on a library default.

**Recommendation:** Set `database.pragma("foreign_keys = ON")` explicitly in `openDatabase` immediately after the WAL pragma, so the guarantee is connection-independent and self-documenting.  Correct the comment in `mcp-bearer-auth.ts` to state that FK CASCADE is enforced (so the orphan-row branch is defensive-only).  Add a one-line regression test asserting `pragma('foreign_keys')===1` after `openDatabase`.

### M2 — Refresh-token reuse detection is suppressible; doubles as a refresh-token existence oracle

**File:** `src/web/oauth/provider.ts:264-345` (`exchangeRefreshToken`), specifically the ordering of the peek (`:275`), the scope-narrowing loop (`:280-296`, throws `InvalidScopeError` at `:291`), the resource check (`:298-305`, throws `InvalidTargetError` at `:302`), and the rotation call (`:307`).

**Issue:** `findRefreshToken` (peek) returns the row **regardless of `rotated_at` / `revoked_at` state**.  The provider then validates scope-narrowing and resource binding against that peeked row, and only afterwards calls `rotateRefreshToken`, which is the sole authority that detects reuse and revokes the family.  Because the validation runs first, a presentation of an already-rotated or revoked refresh token (the OAuth 2.1 theft signal) that is paired with an out-of-scope `scope` value or a mismatched `resource` throws `InvalidScopeError` / `InvalidTargetError` and returns **without ever calling `revokeFamily`**.

This breaks AC-047-3 ("presenting an already-rotated refresh token MUST revoke the entire token family") in two ways:

1. **Suppression of family revocation.** An attacker holding a stolen-but-already-rotated refresh token can keep presenting it with a deliberately out-of-scope scope to avoid tripping the family-wide revocation alarm, probing indefinitely.  (They gain no token from these requests, but the legitimate holder's family is never proactively killed, defeating the rotation tripwire.)
2. **Existence oracle.** The error code distinguishes credential states without consuming anything: an unknown / foreign-client refresh token folds to `null` → `invalid_grant`, whereas a *real* refresh token for the presenting client (even one that is rotated/revoked) paired with an obviously out-of-scope value returns `invalid_scope` (or `invalid_target` for a bad resource).  An attacker with a candidate token + `client_id` can therefore confirm "this is a genuine refresh token issued to this client" by observing `invalid_scope` vs `invalid_grant`, without rotating or revoking it.

**Recommendation:** Detect reuse FIRST.  Either (a) check `rotated_at`/`revoked_at` on the peeked row and call `revokeFamily` before any scope/resource validation, or (b) make `rotateRefreshToken` the first call and run scope/resource validation only on a successfully-rotated row (rolling back the rotation if validation then fails, or validating inside the same transaction).  The invariant should be: any presentation of a rotated/revoked refresh token revokes the family, irrespective of the requested scope or resource, and returns the same `invalid_grant` an unknown token gets.

### M3 — New SDK pin imports 6 moderate transitive advisories into the production tree

**Files:** `package.json`, `pnpm-lock.yaml` (commit `a056caf` — `chore(deps): pin @modelcontextprotocol/sdk to ^1.29.0`).

**Issue:** `pnpm audit --prod --audit-level=high` exits 0 (no high/critical), but `pnpm audit --prod` reports **6 moderate** advisories, all newly introduced on this branch via `@modelcontextprotocol/sdk@1.29.0`:

- **qs DoS** (GHSA-q8mj-m7cp-5q26, `qs >=6.11.1 <=6.15.1`, patched `>=6.15.2`) — reachable via `. > @modelcontextprotocol/sdk@1.29.0 > express@5.2.1 > body-parser@2.2.2 > qs@6.15.1` (8 paths).  This is on the request-parsing path of the very SDK auth handlers this feature mounts (`/authorize`, `/token`, `/register`, `/revoke` all call `express.urlencoded`/`express.json`).  A crafted body can throw inside `qs.stringify` for comma-format arrays.
- **hono ×3** (IP-restriction bypass for non-canonical IPv6; Set-Cookie injection via unsanitized sameSite/priority; JWT middleware accepts any Authorization scheme) — `hono <4.12.21`, patched `>=4.12.21`, via `@modelcontextprotocol/sdk@1.29.0 > @hono/node-server > hono@4.12.18` and a direct SDK edge.  hono is not on Ledgerwire's express request path, so live exposure is limited, but it is in the prod dependency tree.

**Impact:** The `--audit-level=high` pre-deploy gate (M12, 2026-05-17) passes, so this does not block deploy under current policy — but the OAuth branch measurably enlarges the moderate-CVE surface of the shipped runtime, and the qs DoS is on a reachable path.

**Recommendation:** Add a pnpm override to force `qs >= 6.15.2` (and `hono >= 4.12.21` if the SDK does not pull a patched line) for the SDK's transitive deps, or bump `@modelcontextprotocol/sdk` once an upstream release ships the patched ranges.  Re-run `pnpm audit --prod` to confirm a clean moderate count.  Consider tightening the pre-deploy gate to `--audit-level=moderate` for the prod tree now that the count is non-zero, with an explicit allowlist for any accepted advisory.

---

## Low findings

### L1 — Consent transaction is not bound to the approving session

**Files:** `src/web/oauth/provider.ts:151-171` (mint txn, 302 to `/oauth/consent?txn=RAW`), `src/web/oauth/consent.ts:257-318` (GET renders consent for any logged-in user holding the raw txn), `:325-416` (POST mints the code bound to `req.user`).

**Issue:** The raw transaction nonce is a 256-bit bearer capability delivered only to the browser that drove `/authorize`.  Any authenticated user who is induced to open `/oauth/consent?txn=<nonce>` and click Approve mints an authorization code bound to **their** account and delivered to the registering client's redirect_uri; the party who initiated `/authorize` holds the PKCE verifier and can complete the exchange.  This is the standard OAuth consent-phishing / session-fixation shape.  It is well-mitigated here: the POST requires an explicit human click, `requireSameOrigin` + `requireCsrfToken` block silent auto-submission, the consent screen names the client and the redirect host, and the copy warns "Only approve if you just connected Ledgerwire from Claude."  There is, however, no cryptographic binding between the session that initiated the authorize request and the session that approves it.

**Recommendation:** Accept as inherent-but-mitigated, or strengthen by recording the initiating signal (e.g. require that the approving session is the one that first GET-rendered the txn, or surface more provenance on the screen).  Given the data sensitivity (bank data), at minimum keep the explicit-approve + warning copy and ensure the warning is not weakened in future redesigns.

### L2 — No explicit body-size limit on SDK-mounted OAuth endpoints

**File:** `src/web/oauth/router.ts:91-127`.  The SDK `authorizationHandler` / `tokenHandler` / `revocationHandler` mount `express.urlencoded({extended:false})` and `clientRegistrationHandler` mounts `express.json()` with the **default 100kb** limit, versus the app-wide `20kb` set at `server.ts:2055`.  The consent POST correctly scopes its own `4kb` parser (`consent.ts:330`).  `/register` is unauthenticated (RFC 7591) and accepts up to 100kb JSON, bounded only by the 5/hr/IP + 20/day/IP limiters.

**Recommendation:** Pass a `limit` (e.g. 8-16kb) to the SDK handlers if the SDK exposes the option, or front them with a scoped `express.json`/`urlencoded` cap as the consent route does.  Low because the registration limiter caps volume.

### L3 — Wildcard CORS on `/token` and `/register`

The SDK `tokenHandler` and `clientRegistrationHandler` call `router.use(cors())` (i.e. `Access-Control-Allow-Origin: *`).  This is intentional and standard for a PKCE public-client token endpoint and open dynamic registration (browser-based MCP clients must reach them cross-origin); no credentials/cookies are involved on these endpoints.  Documented here so a future reviewer does not flag it as a regression.  No change recommended beyond awareness.

### L4 — `lw_at_` prefix collision with a random legacy bearer

**File:** `src/web/mcp-bearer-auth.ts:242`.  Dual-mode routing is by raw-token prefix.  Legacy `mcp_tokens` values are 43 base64url chars; the base64url alphabet includes `l`, `w`, `_`, `a`, `t`, so a legacy token could in principle begin `lw_at_` and be misrouted to the OAuth store (→ 401).  Probability ≈ (1/64)^6 ≈ 1.5e-11 per token; operationally irrelevant.  Documented for completeness.  Optional hardening: have the token mint reject/avoid legacy values colliding with reserved prefixes, or fall through to the legacy store on an OAuth-path miss (the latter would re-introduce fallback chaining, which the design deliberately avoids — so leaving as-is is the better tradeoff).

---

## Verified sound (probed, no finding)

- **Open redirect.** `redirect_uri` is exact-matched at `/authorize` (SDK `redirectUriMatches`: exact for non-loopback, RFC 8252 port-only relaxation for loopback) and re-validated at code exchange (`provider.ts:228` `redirectUri !== rec.redirectUri`).  Both the approve and deny consent paths redirect ONLY to the registration-validated `rec.redirectUri` (`consent.ts:356-416`); a missing/unparseable/expired/consumed txn renders a static error page, never a redirect.  The post-login `next` allowlist `^/oauth/consent\?txn=[A-Za-z0-9_-]{43}$` is anchored at both ends, applied via `isAllowedOauthNext` at BOTH `/auth/verify` (`server.ts:2371`) and `POST /login` (`dashboard.ts:386`), and resists URL-encoding (values are matched post-decode), backslashes (`\` ∉ charset), protocol-relative `//host` (2nd char must be `o`), and duplicate params (the `&` after 43 chars fails `$`).
- **PKCE.** S256 is the only reachable method — the SDK `RequestAuthorizationParamsSchema` requires `code_challenge_method: z.literal('S256')`; `plain` is rejected pre-redirect.  `skipLocalPkceValidation` is left unset, so the SDK verifies the challenge before `exchangeAuthorizationCode`.  `challengeForAuthorizationCode` returns consumed rows by design so that a replayed code reaches the `already_consumed` → family-revocation branch; cross-client codes die with the same `invalid_grant` (no oracle).
- **Token secrecy.** No raw `lw_at_` / `lw_rt_` / code / txn value is ever persisted (only sha256 hex) or logged — grepped the OAuth modules; provider/consent/storage log only `client_id`, `family_id`, `user_id`, `redirect_host`, and counts.  Lookups are indexed sha256-hash equality (preimage-resistant; timing not exploitable).
- **Code replay → family revocation.** Wired: `consumeCode` returns `already_consumed` → `provider.exchangeAuthorizationCode` calls `revokeFamily` (`provider.ts:204-217`).
- **Tenant isolation.** `revokeFamilyForUser` is `user_id`-scoped; the dashboard `DELETE /api/oauth-grants/:familyId` route uses it and returns 404 for a foreign/unknown family (no cross-user probe).  An OAuth token for user A attaches `req.mcpUser={id:A}` and cannot ride user B's MCP session — `http-transport.ts` enforces `entry.userId === mcpUser.id` (403 `session_owner_mismatch`).
- **Resource/audience binding.** Validated at `/authorize` (`InvalidTargetError` on mismatch), persisted on the txn → code → token, re-checked at the token endpoint and refresh, and re-checked at `/mcp` (`requireMcpAuth`: `record.resource !== canonicalResource` → 401).
- **Registration abuse.** `oauthRegisterIpHourLimiter` (5/hr) + `oauthRegisterIpDayLimiter` (20/day) + `clientCapGuard` (1000 active, 503 when full) are mounted in front of the SDK register handler (`router.ts:103-121`).  `isAllowedRedirectUri` rejects `http://localhost.evil.com` (hostname must equal `localhost`/`127.0.0.1` exactly), normalises mixed-case schemes via the URL parser, and `https://evil@good.com` is accepted as https but is then exact-matched against the registered value at authorize (userinfo included), so it is not an open redirect.
- **Flag-off integrity.** Mechanism (not just tests) confirmed: `registerOauthRoutes` is only called when `env.OAUTH_ENABLED && options?.db` (`server.ts:2042`); `/mcp` uses `requireMcpToken` (legacy, byte-identical `LEGACY_CHALLENGES`) when the flag is off and `requireMcpAuth` only when on; `DELETE /api/oauth-grants` mounts only when on; `buildOauthGrants` returns `[]` when off so the dashboard renders unchanged.  WWW-Authenticate legacy strings are unchanged when off (snapshot-pinned).
- **Mount-order / Stripe raw body.** OAuth routes mount before the global `express.json`/`urlencoded` (`server.ts:2042` vs `:2055`); the Stripe webhook's scoped `express.raw` is mounted earlier still (`:1288`) and is path-specific (`/webhooks/stripe`), so the OAuth `app.use('/token', …)` etc. cannot shadow it or alter its raw-body parsing.  (See L2 for the one parser nuance — size limits.)
- **Consent integrity.** POST stack is `urlencoded(4kb) → limiter → requireAuth → requireSameOrigin → requireCsrfToken`; the txn consume is a single atomic UPDATE guarded by `consumed_at IS NULL AND expires_at > now`; `client_name` and the redirect host are HTML-escaped (`logo_uri`/`client_uri` deliberately not rendered); the consent page runs under the strict default CSP with no inline JS; the `form-action` CSP override is applied only to the bodyless 302 redirect response and only widens `form-action` to the redirect origin (no other directive relaxed, no body served on that response).

---

## Test + dependency status (branch state reviewed)

- **`pnpm test`** — green.  237 test files (3 skipped), 2646 tests (2622 passed, 24 skipped — the skipped are the `SMOKE_TEST=1`-gated real-Teller/Quiltt suites).  (Note: the working tree contains `.claude/worktrees/*` copies that re-run a subset of suites; all pass.)
- **`pnpm audit --prod --audit-level=high`** — **exit 0** (gate passes).  `pnpm audit --prod` reports **6 moderate** advisories, all transitive via `@modelcontextprotocol/sdk@1.29.0` (qs DoS GHSA-q8mj-m7cp-5q26 ×several paths; hono <4.12.21 ×3; see M3).  Zero high/critical.

---

## Findings → backlog

Per the project convention that Medium+ findings feed into the specs rather than being fixed silently:

- **M1 → new TRD task (under REQ-047 / data-layer hardening).** "Set `PRAGMA foreign_keys = ON` explicitly in `openDatabase`; add a boot-time/regression assertion that the pragma is 1; correct the `mcp-bearer-auth.ts` comment that claims FK enforcement is off."  Also worth a one-line PRD note under the storage-invariant requirement (REQ-021 family) that referential integrity is an enforced guarantee, not incidental.
  **Status: Fixed on branch (commit `baa80bf`).** Explicit pragma in `openDatabase`, regression tests (`pragma === 1` on a fresh open; dangling-FK insert into `oauth_tokens` throws), comments corrected in `mcp-bearer-auth.ts` and `db.test.ts`.
- **M2 → REQ-047 AC amendment + TRD task.** Tighten AC-047-3 to read: "presenting an already-rotated or revoked refresh token MUST revoke the entire family BEFORE any scope/resource validation, and MUST return `invalid_grant` indistinguishable from an unknown token."  TRD task: reorder `exchangeRefreshToken` so reuse detection precedes scope/resource checks (or validate inside the rotation transaction), and add a test asserting that a rotated token + out-of-scope scope still revokes the family and returns `invalid_grant`.
  **Status: Fixed on branch (commit `683acc8`).** Reuse detection now precedes scope/resource validation; unknown / foreign-client / reused tokens all return an indistinguishable `invalid_grant`; `invalid_scope`/`invalid_target` only ever surface for a live, owned token; the `rotateRefreshToken` 'reuse' branch retained as a race backstop.  (The AC-047-3 wording amendment in the PRD/TRD remains a tracked doc task.)
- **M3 → TRD task (dependency hygiene; extends the M12 audit gate).** "Add pnpm overrides to force `qs >= 6.15.2` (and `hono >= 4.12.21`) under `@modelcontextprotocol/sdk`, or bump the SDK to a release carrying the patched ranges; re-run `pnpm audit --prod` to a clean moderate count.  Evaluate tightening the pre-deploy gate to `--audit-level=moderate` with an explicit advisory allowlist."

Lows (L1-L4) are recorded for awareness; none require a spec change, though L1's consent-screen warning copy should be treated as security-load-bearing in any future dashboard/consent redesign.