Security review — PR #22 (retroactive): 3 Medium / 3 Low / 1 Info

[pulkitpareek18]

Retroactive security review of PR #22 (Central API developer console — merged as 0c325fb, live at 0d1741d).

Discipline-debt clearance — PR #22 touched all four security-reviewer trigger surfaces (auth, crypto, audit, tenant boundaries) and merged without the subagent running, against the standing instruction in CLAUDE.md.

Full report (with reproduction steps + remediation for each finding) lives at qa-log/security-review-pr22.md.

Net assessment

Medium risk overall. No Critical findings. No tenant API key rotation required. Tenant scoping (the load-bearing security property) is correctly enforced; the tests/console-proxy.test.ts suite already covers A-01 + A-10 scoping. The actual issues are documentation drift, an enumeration channel, and an attribution gap on the audit log.

Findings (action items)

Medium — land this week

• F-1: Console JWT stored in localStorage; docs/threat_model.md A-09 claims "client memory" → either move to HttpOnly cookies OR update A-09 to match implementation. Run threat-model-update, file an ADR.
• F-2: Email enumeration on /api/console/signup → return uniform 202 regardless of email existence; emit verification email out-of-band.
• F-3: Console-initiated audit rows show actor_type='api_key' with actor_id=NULL → plumb actorType: 'console' + actorEmail through platform.ts and recordAuditEvent.

Low — land next week or fold into related work

• F-4: Add per-tenant write rate-limit (60/15min by req.console.tenantId) on POST/PATCH/DELETE in src/routes/console.ts.
• F-5: Add jti + aud='zeroauth-console' to console JWT; verify aud in verifyConsoleToken; Redis-backed jti revocation list.
• F-6: Validate ?limit= query param in 5 console handlers; reject with 400 invalid_limit instead of relying on sanitizeLimit downstream.

Informational

• F-7: Two console handlers use the error: field for human strings; convention is { error: '<machine_code>', message: '<human>' }.

Things checked + clean (8 items)

See report. Highlights: every SQL query in platform.ts is pg-parameterised + tenant-scoped; no dangerouslySetInnerHTML anywhere in dashboard; no JWT or password in log lines; Helmet CSP + trust proxy 1 correctly set behind Caddy.

Recommendations beyond findings

• Run threat-model-update skill to reconcile A-09 with shipped reality.
• Adopt zod via dep-add for the 8+ repeated if (!field) checks in console.ts.
• Add CSP report-uri (open item in threat model).

Subagent

Continuation reference: agentId acdae2de12c322caa — use SendMessage to resume the same security-reviewer session with follow-up questions.

🤖 Generated with Claude Code

[pulkitpareek18]

Resolution — 6 of 7 findings closed

All changes landed on dev as commit d187c77. 64 → 68 tests passing locally; typecheck clean; lint 0 errors.

Closed

• F-1 — docs/threat_model.md A-09 rewritten to document localStorage reality (the doc previously lied that the JWT "lives in client memory"). Includes pointer to the governance repo's authoritative dashboard.md component-level write-up, plus the open ADR for the future cookie migration.
• F-3 — actor_type='console' now plumbed through. Introduced AuditActor type in src/services/platform.ts; service functions createDevice / updateDevice / createTenantUser / updateTenantUser take an actor: AuditActor parameter instead of positional actorId. Console routes pass { type: 'console', id: tenantId, email: req.console.email }; v1 routes pass { type: 'api_key', id: apiKey.id }. Operator email lands in audit_events.metadata.actor_email.
• F-4 — New consoleWriteLimiter (60 writes / 15 min, keyed on req.console.tenantId) applied to all 6 authenticated console write routes.
• F-5 — Console JWTs now carry jti: <uuid v4> and aud: 'zeroauth-console'. verifyConsoleToken validates audience explicitly. Cross-audience tokens are rejected with 401. The jti is the seam for a future Redis-backed revocation list (separate ADR — not built today).
• F-6 — New parseLimit helper rejects non-integer, ≤0, or >1000 with 400 invalid_limit. Three new regression tests (non-integer, 0, 1001).
• F-7 — Two console handlers (/signup, /login) now use invalid_request machine code with the human string in message, matching codebase convention.

Left open

• F-2 — Email enumeration via /api/console/signup returning 409 for existing emails. Deferred — real fix needs email infrastructure we don't have yet. The byte-identical fix per the security review is "return 202 always + send verification email out-of-band." The interim option ("uniform 400 invalid_request") also leaks because fresh signups still return 201. Kept the 409 in place with an explanatory comment in src/routes/console.ts. Tracked as a hard subtask gated on ADR for email service adoption.

Will fold into related future work

• The cookie-vs-localStorage migration for the console JWT (the more aggressive F-1 remediation) — open as adr-index/ALL.md candidate 0006-console-jwt-cookie-vs-localstorage.md in the governance repo. Decision needed before first pilot SOW signing.
• Redis-backed jti revocation list — open as a separate ADR. Redis is already wired into docker-compose.yml but not yet used by app code.
• zod input validation across console.ts — recommendation Bump typescript from 5.9.3 to 6.0.3 in /dashboard #4 from the security review. Would land via the dep-add skill.
• CSP report-uri — recommendation Bump react and @types/react in /dashboard #5. Threat-model open item.

Closing the issue with F-2 explicitly broken out so it doesn't get lost.

🤖 Generated with Claude Code

[pulkitpareek18]

Closing: F-1, F-3, F-4, F-5, F-6, F-7 fixed in commit d187c77. F-2 carved out as #27 (gated on email infrastructure).