All security documentation

# Ledgerwire Security Review — 2026-05-17

**Reviewer:** code-reviewer subagent (Claude)
**Branch reviewed:** `feature/trd-2026-001-teller-mcp-server` @ `807d717`
**Scope:** Full security review (auth, crypto, webhooks, MCP transport, SQL, logging, secrets, input validation, authorization, deps, hardening).
**Method:** Read-only; no code changes during the review pass.

This document is the canonical reference used by subsequent fix-implementation agents.
Fixes land on branch `feature/security-fixes`.

---

## Summary table

| # | Severity | Title | File:line |
|---|---|---|---|
| C1 | Critical | Cross-user data exposure via `get_reconnect_link` | `src/storage/tokens.ts:268`, `src/teller/aggregator.ts:244`, `src/mcp/tools/get-reconnect-link.ts:37` |
| C2 | Critical | No webhook timestamp replay-window check | `src/web/server.ts:152` |
| H1 | High | Session IDs stored plaintext at rest | `src/storage/sessions.ts:42` |
| H2 | High | `markDisconnected` not scoped by `user_id` | `src/storage/tokens.ts:328` |
| H3 | High | Master-key handling vs misleading "short lifetime" comment | `src/storage/tokens.ts:119` |
| H4 | High | No rate limiting on magic-link issuance | `src/web/server.ts:435`, `dashboard.ts:234` |
| H5 | High | No rate limiting on `/mcp` bearer endpoint | `src/web/mcp-bearer-auth.ts:90` |
| H6 | High | Webhook diagnostic logging leaks body + secret metadata | `src/web/server.ts:204` |
| H7 | High | No CSRF defence on `POST /enrollments` | `src/web/server.ts:613` |
| H8 | High | No CSRF defence on MCP-token mint/revoke endpoints | `src/web/dashboard.ts:338` |
| H9 | High | Plaintext Teller access token embedded in reconnect URLs (emails + MCP responses) | `src/teller/aggregator.ts:244` |
| M1 | Medium | No security headers (helmet) | all routes |
| M2 | Medium | No `app.set('trust proxy')` | `src/web/server.ts:693` |
| M3 | Medium | No explicit body-size limits | `src/web/server.ts:254,400` |
| M4 | Medium | `decodeURIComponent` cookie fallback silently catches | `src/web/auth-middleware.ts:62` |
| M5 | Medium | Magic-link response time leaks signal (Resend awaited) | `src/web/server.ts:435` |
| M6 | Medium | AES-GCM ciphertext not bound to enrollment context (no AAD) | `src/storage/encryption.ts:120` |
| M7 | Medium | `MCP_STDIO=1` not guarded in production | `src/index.ts:80`, `src/mcp/server.ts:276` |
| M8 | Medium | mTLS private key lives in `process.env` for process lifetime | `src/teller/transport.ts:65` |
| M9 | Medium | Webhook idempotency stamp + side-effects not transactional | `src/web/server.ts:333` |
| M10 | Medium | No webhook timestamp max-skew check (paired with C2) | same as C2 |
| M11 | Medium | esbuild/vite moderate CVEs in dev deps | `package.json`/lockfile |
| M12 | Medium | No CI dep-audit step | CI config |
| M13 | Medium | Server boots without `TELLER_WEBHOOK_SECRET` (warns only) | `src/config/env.ts:225`, `src/web/server.ts:153` |
| M14 | Medium | Origin allowlist — verify `Origin: null` handling | `src/web/mcp-bearer-auth.ts:241` |

Plus ~15-20 Low items (5 representative ones documented at end).

---

## Critical findings

### C1 — Horizontal privilege escalation via `get_reconnect_link`

**Files:**
- `src/storage/tokens.ts:268-290` (`getToken`)
- `src/teller/aggregator.ts:244-262` (`getReconnectLink`)
- `src/mcp/tools/get-reconnect-link.ts:37-76` (tool handler)

**Issue:** `getToken` resolves rows by `enrollment_ref` alone — no `user_id` predicate. `getReconnectLink` then decrypts the access token (under the row's own `user_id` via `resolveKey`) and embeds it as `?accessToken=…` in the returned URL. The MCP tool handler does not check that the resolved row's `user_id` equals the bearer-authenticated `deps.userId`.

**Exploit:** Attacker authenticated as user B who learns or guesses user A's 13-char base32 `enrollment_ref` (≈64 bits — unguessable by pure brute force, but reachable via screenshots, support transcripts, log leaks) calls `get_reconnect_link({ enrollment_ref: "<A's ref>" })` and receives a URL containing the decrypted plaintext Teller access token for victim A's bank. That token can be used directly against Teller's API independent of Ledgerwire.

**Fix:** Scope `getToken` by `user_id` (`AND user_id = ?`); change `AccountAggregator.getReconnectLink` signature to `(userId, enrollmentRef)`; in the tool handler only proceed if `record.user_id === deps.userId`. See also H9 — the URL itself should also not embed the plaintext token.

### C2 — No webhook timestamp replay-window check

**File:** `src/web/server.ts:152-221` (`verifyTellerSignature`)

**Issue:** Parses `t=…` but never compares to current clock. HMAC is valid forever (secret rarely rotates); idempotency bounded only by `webhook_events` row presence which has no TTL.

**Exploit:** Captured valid `enrollment.disconnected` webhook (via past breach of `webhook_events`, log leak, MITM-pre-TLS scenario) can be replayed indefinitely once the idempotency row is purged. Marks arbitrary enrollments disconnected and triggers bogus reconnect emails (data integrity + phishing — the email URL carries a live access token).

**Fix:** Reject any signed body whose `t=` is more than ±5 minutes off `Date.now()` (Stripe's standard tolerance).

---

## High findings

### H1 — Session IDs stored plaintext at rest

**File:** `src/storage/sessions.ts:42-70`; table definition in `src/storage/db.ts:66-73`.

**Issue:** Session IDs (`randomBytes(32).base64url`) are stored verbatim as PK in `sessions(id …)`. Magic-link tokens and MCP bearer tokens are correctly SHA-256-hashed; sessions break this invariant.

**Exploit:** Read-only DB compromise (SQLi elsewhere, backup leak, volume snapshot, dev-laptop theft) yields live 30-day session cookies for every signed-in user.

**Fix:** Store `sha256(sessionId)` as PK; hash inputs in `findSessionById`. Migration: invalidate active sessions on deploy (one-time logout) or maintain a `legacy_id` column for a grace period.

### H2 — `markDisconnected` not scoped by `user_id`

**File:** `src/storage/tokens.ts:328-343`

**Issue:** `UPDATE tokens SET status='disconnected' WHERE enrollment_id = ?` — no `user_id`. Schema PK is `(user_id, enrollment_id)`, which permits duplicates of `enrollment_id` across users.

**Exploit:** Theoretical/low likelihood today, but defence-in-depth failure. Bundle this fix with C1 (same query family).

**Fix:** `WHERE user_id = ? AND enrollment_id = ?`; resolve `user_id` via the webhook event's enrollment row before mutating.

### H3 — Master-key handling vs misleading comment

**File:** `src/storage/tokens.ts:119-129` (`resolveKey`)

**Issue:** Code comment claims master key isn't cached "to keep secret lifetime short," but `env.TOKEN_ENCRYPTION_KEY` lives forever in `process.env`; the derived buffer is not zeroed; Node has no reliable buffer-wipe primitive. The comment misrepresents actual behaviour and the code pays per-request CPU cost for no real benefit.

**Fix:** Either cache per-`user_id` HKDF subkey at module scope, or rewrite the comment to honestly describe the threat model. Move to KMS-style design for full mitigation (out of scope here).

### H4 — No rate limiting on magic-link issuance

**Files:** `src/web/server.ts:435-502`, `src/web/dashboard.ts:234-289`.

**Issue:** `POST /auth/request` (and `POST /login`) has no per-IP, per-email, or global throttle. Each call writes a row, calls Resend, and pollutes the from-domain reputation.

**Exploit:** Email-bomb any third-party address; exhaust Resend quota; fill `magic_links` / `users` tables; inflate operator's bill.

**Fix:** Per-IP token bucket (5/min, 20/hour) and per-email cooldown (1/min) before write/send.

### H5 — No rate limiting on `/mcp` bearer endpoint

**File:** `src/web/mcp-bearer-auth.ts:90-144`

**Issue:** No per-IP / per-token-prefix throttle. A leaked bearer has no rate cap until manually revoked.

**Fix:** Per-IP and per-token rate limit; alert on sustained 401.

### H6 — Webhook diagnostic logging leaks body + secret metadata

**File:** `src/web/server.ts:204-217`

**Issue:** On HMAC mismatch logs `bodyPreview: rawBody.slice(0, 120)`, `secretLen`, and `secretPreview` (first 4 + last 2 chars of secret). Allows unauthenticated log-injection (120 attacker-controlled bytes into prod log stream) and reduces brute-force search space for the webhook secret if logs leak.

**Fix:** Drop `bodyPreview`, `secretPreview`, `secretLen`. Keep boolean `matched` + timestamp.

### H7 — No CSRF on `POST /enrollments`

**File:** `src/web/server.ts:613-652`, browser caller at `src/web/templates.ts:1107-1118`

**Issue:** Cookie-only auth (`requireAuth`), no CSRF token, no `Origin`/`Referer` check. `SameSite=Lax` blocks background fetch CSRF but not top-level form POSTs. Attacker can trick signed-in user into POSTing the attacker's Teller enrollment into the victim's account.

**Fix:** Either `SameSite=Strict` on session cookie, double-submit CSRF token, or `Origin === PUBLIC_URL` whitelist.

### H8 — No CSRF on MCP-token mint/revoke endpoints

**File:** `src/web/dashboard.ts:338-410`

**Issue:** Same class as H7 for `POST /api/mcp-tokens` and `DELETE /api/mcp-tokens/:hash`. Token hash is rendered in the dashboard HTML (visible in any response-disclosure attack).

**Fix:** Same options as H7. Bundle the fix.

### H9 — Plaintext Teller token in reconnect URLs

**File:** `src/teller/aggregator.ts:244-262`

**Issue:** Reconnect URL contains `?accessToken=<plaintext>`. URL is included in disconnect emails (forever-in-inbox + Resend "Sent" archive) and returned via JSON-RPC to AI clients which persist conversation history.

**Fix:** Server-side `/reconnect/:nonce` that 302-redirects to the Teller URL; never serialise the raw token through MCP responses or emails. Use a single-use nonce table (`reconnect_nonces`) with short TTL (~10 min) keyed by `(user_id, enrollment_ref)`.

---

## Medium findings

### M1 — No security headers (helmet)

Add helmet with HSTS, X-Content-Type-Options=nosniff, X-Frame-Options=DENY, Referrer-Policy=strict-origin-when-cross-origin, Permissions-Policy, and a CSP scoped per-route (tight on `/dashboard`, allowing `cdn.teller.io` on `/connect`).

### M2 — Missing `trust proxy`

`src/web/server.ts:693-702`. Without `app.set('trust proxy', 1)`, `req.ip` returns the Fly proxy address. Any rate limiter (H4/H5) collapses to a single key. Fly is one hop — set to `1`.

### M3 — No explicit body-size limits

Default 100kb is permissive. Set `express.json({limit:'20kb'})` and `express.raw({type:'application/json', limit:'64kb'})`.

### M4 — `decodeURIComponent` silently falls back

`src/web/auth-middleware.ts:62-69`. Dead code today (base64url never percent-encodes), but a silent catch obscures intent. Document or reject on decode failure.

### M5 — Magic-link response time leaks signal

`/auth/request` awaits `sendEmail`. Resend response timing varies between accepted/rejected addresses — measurable enumeration channel despite uniform 200.

**Fix:** Fire-and-forget the send; respond within a constant ~10ms window. Or `Promise.race` with a fixed-minimum delay.

### M6 — AES-GCM not bound to enrollment context

`src/storage/encryption.ts:120-153`. Per-user HKDF prevents cross-user ciphertext swap, but **within a user** an attacker with DB write could swap one enrollment's ciphertext into another and the auth tag still verifies.

**Fix:** `setAAD(Buffer.from(\`${user_id}:${enrollment_id}\`))` on both cipher and decipher. Bump `key_version` and migrate eagerly.

### M7 — `MCP_STDIO=1` not guarded in prod

`src/index.ts:80-88`, `src/mcp/server.ts:276-287`. Stdio path resolves all queries to `env.USER_ID` (default `default_user`) bypassing per-bearer scoping. Hard to actually exploit on Fly (no stdio), but should be explicit.

**Fix:** Throw at boot if `NODE_ENV === 'production' && MCP_STDIO`.

### M8 — mTLS private key in `process.env` for process lifetime

`src/teller/transport.ts:65-73`. Standard for Node + Fly secrets; document rotation runbook.

### M9 — Webhook side-effects not transactional

`src/web/server.ts:333-393`. Crash between `recordWebhookEvent` returning `true` and `markDisconnected` running silently drops the redelivered webhook.

**Fix:** Single transaction wrapping idempotency stamp + mutations; or two-stamp approach (`received_at` then `processed_at`).

### M10 — Paired with C2.

### M11 — Dev-dep CVEs

`esbuild <= 0.24.2` (GHSA-67mh-4wv8-2f99), `vite <= 6.4.1` (GHSA-4w7w-66w2-5vf9). Both via vitest. Dev-only impact (vitest not shipped) but exposes dev server to malicious LAN peer during `pnpm test`.

**Fix:** `pnpm update vitest` to a release that pulls vite ≥ 6.4.2.

### M12 — No CI dep-audit

Add `pnpm audit --prod --audit-level=high` to CI scripts.

**Status (2026-05-17, commit pending):** `package.json` now exposes a
`pnpm audit` script (`"audit": "pnpm audit --prod --audit-level=high"`).
No GitHub Actions workflow exists in this repo yet, so the script must be
invoked manually before each deploy until CI is set up.  Pre-deploy
checklist: run `pnpm audit` and require a clean exit (0) before
`fly deploy`.  When CI lands, add a `pnpm run audit` step on every PR.

### M13 — Boots without `TELLER_WEBHOOK_SECRET`

`src/config/env.ts:225-229`. Refuse to boot in `NODE_ENV=production` without it (zod `superRefine`).

### M14 — Origin allowlist + `Origin: null`

`src/web/mcp-bearer-auth.ts:241-268`. Verify `Origin: null` is rejected (probably already correct — `allow.has("null")` is false). Add a test.

---

## Low (representative)

- **L1.** `escapeHtml` doesn't escape backtick (`` ` ``); fine for current usage, breaks if a value is ever interpolated inside a `<script>` template literal. `src/web/templates.ts:55-62`.
- **L2.** `connect.html` interpolates `TELLER_APP_ID`/`TELLER_ENV` inside JS single-quoted literals with no escape. Operator-controlled today, but XSS-shaped pattern. `src/web/connect.html:24-25`, `src/web/templates.ts:1080-1083`.
- **L3.** 30-day session fixed at issue time; no inactivity shortening, no rotation on privilege change. `src/storage/sessions.ts:22-26`.
- **L4.** `magic_links` table grows on every request (incl. unknown emails). Compounded with H4 = unauthenticated unbounded-growth vector. `src/services/magic-link.ts:75-91`.
- **L5.** `app.use(express.static(...))` registered without cache-control. `src/web/dashboard.ts:207`.

Other Low categories noted but not enumerated: missing graceful-shutdown drain (in-flight MCP cut off), session map in MCP transport never expires (`src/mcp/http-transport.ts:50`), CSP retrofit blocked by inline template scripts, no SRI on Teller CDN script.

---

## What's done well

Above-average overall posture for solo-built code; the multi-user refactor introduced the C1/H9/H2 cluster (cross-user authorization) and H1 (session-storage invariant), which are the only critical-class regressions.

- Crypto primitives correct: HKDF + AES-GCM + CSPRNG, SHA-256 hashed magic-link and MCP tokens, constant-time HMAC compare, per-user key derivation, key-version migration story
- HMAC verified over raw body **before** JSON.parse; multi-`v1=` rotation support; `timingSafeEqual`
- Idempotency on webhooks implemented (gap: M9 transactionality)
- MCP transport ordering correct: bearer → origin → session-owner match; `WWW-Authenticate` headers proper
- SQL uniformly parameterised — no concat/template-literal queries found
- HTML templates funnel every interpolated value through `escapeHtml`/`escapeHtmlAttr`
- Logging unusually disciplined: pino redact + `LONG_SECRET_RE` regex + debug-only token URL paths
- No `Math.random`; no string-concat SQL anywhere
- Magic-link consumption atomic via UPDATE-with-predicate
- Tool dispatch threads `deps.userId` correctly **except** for the C1 hole

---

## Fix priority (recommended sequencing)

1. **Ship today:** C1 + H9 + H2 (one cohesive change — cross-user data exposure)
2. **This week:** C2 (replay window), H1 (session hash), H6 (log scrub), H4+H5+M2 (rate limit + trust proxy)
3. **Before public launch:** H7+H8 (CSRF), M1 (helmet), rest of Medium