Security hardening and bugs fixed

This page exists because the PR author kept a running log of every real bug this branch hit — packages/claude-mcp/BUGS.md — and it's unusually candid about which ones a test caught versus which ones only a person clicking through the product ever found. Six of the entries were found that second way. That ratio is itself useful signal about where this kind of system is fragile: OAuth, cookies and redirects fail in the browser, after the server has already returned a 200, and no amount of server-side testing sees that.

Business view

None of this changes what a doctor does. It's the reason the feature can be trusted to ship at all. Two categories of thing were specifically defended against:

• **A stolen or phished doctor login being used to hammer the system at

machine speed**, rather than the pace of a person clicking. The response isn't "make this impossible" — a valid login is a valid login — it's "bound how much damage it can do, and make it stop instantly once noticed."

• **A browser being tricked into sending an authenticated doctor somewhere

other than where they meant to go** — the open-redirect class of bug, which is exactly the kind of thing a login screen is a tempting target for.

Technical view

What a stolen token still cannot do

Spelled out directly in OAUTH-DESIGN.md's "Abuse and resource limits" section: an attacker holding a valid access token cannot write, alter, or delete anything (the database role has no such grant), cannot see more than that doctor already sees in clinic-web (permissions are inherited, not reinvented), and cannot reach a second clinic (the token names exactly one). The worst case is load and exposure of one clinic's data — never damage, never escalation to another tenant.

What it can still do, and the limits that bound it

The residual risk is volume — a token making far more requests than a human ever would. Multiple independent limits exist so no single one has to be perfect:

• Per-token rate limiting, implemented in

packages/claude-mcp/src/auth/rateLimit.ts and applied in index.ts:130-138 before a request reaches the database. Deliberately keyed on the token's hash, not the caller's IP — OAUTH-DESIGN.md explains why this is not optional: every clinic's traffic arrives from Anthropic's servers, so IP-based limiting would either do nothing (shared IPs) or let one abusive token throttle every other clinic sharing that IP.

• A five-connection cap on the tool pool (db/pool.ts) — even a fully

abused MCP process cannot take more than five connections from the production primary.

• A ten-second statement_timeout at the database-role level (see

Tenant isolation and the read-only role).

• A row cap and a maximum date range on every report tool — MAX_ROWS in

tools/types.ts, and the month-clamping in consumption.ts and supplierSpend.ts — so "give me everything since 2015" is refused rather than attempted.

• 1MB request body cap (app.ts:63).
• Instant revocation — because tokens are opaque strings hashed and

looked up fresh on every request (not self-verifying JWTs), deleting or revoking a row takes effect on the very next call. This is called out as "the actual stop button."

Errors are deliberately uninformative to an attacker

Two examples of the same principle, applied consistently::

• AuthenticationError (auth/types.ts:53-58) never distinguishes "no such

token" from "token expired" — both are the same 401. Telling an attacker which of their guesses was closer is free reconnaissance.

• POST /oauth/revoke always returns 200, even for a token that never

existed (router.js:417-424) — an endpoint that answered differently for real versus fake tokens would let an attacker discover valid tokens by trying them.

Five browser-level bugs, found by clicking, not by tests

From BUGS.md §6, condensed — each one only manifested in an actual browser, after the server had already done the right thing:

The rule the author distilled from all of it

BUGS.md closes with an explicit checklist before calling any tool in this package "done" — worth restating here because it's the clearest single summary of the engineering standard this PR holds itself to:

1. Any date compared in SQL against now() at time zone 'utc', never in JS.
2. Any enum array cast to ::text[] before calling .includes() on it.
3. Every comparison operator checked character-by-character against the

API's own helper function, with a test fixture sitting exactly on the boundary.

1. Every branch broken on purpose once, to confirm a test actually catches

it — "a test you have never seen fail is not evidence."

1. A parity test against the API's own query, for the same question.
2. Anything touching the browser clicked through by hand, in two tabs.