drinktracker/audit.md

# Security audit: MCP and OAuth 2.1 surfaces

Date: 2026-08-09
Auditor: independent model review, commissioned after the OAuth server shipped.

> **STATUS: all five findings were verified against the code and fixed.** Every
> claim below was independently confirmed before any change was made — there
> were no false positives, which is unusual for a cross-model review.
>
> The fixes and their regression tests are in the commit that follows this file.
> In short: refresh tokens now have durable per-generation lineage with an atomic
> consume (F1); consent is the only writer of grant scopes and codes are bound to
> a grant epoch (F2); every approval starts a clean epoch that destroys prior
> credentials (F3); loopback redirects match exactly apart from the port (F4);
> and bar counts are gated on `bar:read` (F5).
>
> Q1's advice was taken: lineage first, no refresh grace window for now.
> Q1 also corrected a claim made when the OAuth server shipped — reuse detection
> covered only one generation back, not the whole family. That was the single
> most valuable output of this audit.
>
> This document is kept as the record of what was reviewed. The findings below
> describe the code as it was, not as it is.

## Summary

The MCP surface has a strong overall tenant-isolation posture: all 22 tools pass through required bearer authentication and per-tool scope enforcement, their primary queries are scoped to the authenticated `userId`, foreign-key inputs are ownership-checked, and the bearer verifier checks token revocation, expiry, grant revocation, and account suspension from the database on every request. I found no cross-user row access and no unauthenticated path into an MCP tool. The OAuth implementation's main weakness is state lineage rather than basic PKCE: refresh rotation remembers only one prior hash and performs validation, access-token issuance, and rotation as separate state transitions. A thief can rotate a stolen refresh token twice so the legitimate client's later replay is no longer associated with the grant and does not revoke the attacker's token family. Fixing durable, atomic refresh-token lineage is the single most important change.

## Findings

### 1. High — Refresh-token replay detection forgets the token family

- **Severity:** High
- **Confidence:** Confirmed
- **Files:** `src/app/api/oauth/token/route.ts:152-170`, `src/app/api/oauth/token/route.ts:181-193`, `src/app/api/oauth/revoke/route.ts:28-47`, `src/lib/mcp/oauth.ts:243-255`, `prisma/schema.prisma:237-241`

Only `refreshHash` and the immediately preceding `refreshPrevHash` are retained. Every successful rotation overwrites the prior predecessor. A refresh token therefore identifies its grant for only one subsequent generation, not for the lifetime of the token family.

Concrete exploit path:

1. An attacker steals the grant's current refresh token `R0`.
2. The attacker sends `R0` to `POST /api/oauth/token` and receives `R1`; the row now stores `current=R1, previous=R0`.
3. Before the legitimate client refreshes, the attacker sends `R1` and receives `R2`; the row now stores `current=R2, previous=R1`. The only database reference to `R0` has been erased.
4. The legitimate client later sends `R0`. The query at `token/route.ts:152-159` finds no grant, returns `invalid_grant`, and never calls `revokeGrant`.
5. The attacker retains `R2` and can continue rotating it for the sliding 90-day lifetime. The event that should expose theft instead looks like an unknown token and leaves the attacker's family active.

The same loss of lineage defeats RFC 7009 revocation by token value. If the legitimate client posts the now-aged-out `R0` to `/api/oauth/revoke`, that endpoint cannot associate it with the grant, still returns its mandatory 200 response, and leaves `R2` active. Settings-based revocation by grant ID does still terminate the family.

There is also an atomicity failure in the same path. Two simultaneous requests with the same current token can both read it as current before either reaches `storeRefreshToken`. Both can issue token responses, and the last unconditional update at `oauth.ts:249-255` wins. Neither request is treated as replay. This also means the current zero-grace policy is not actually strict under concurrency.

**Specific fix:** Model refresh tokens as a lineage, for example an `OAuthRefreshToken` row per issued hash with `grantId`, generation, `usedAt`, and expiry. Retain consumed hashes until the family can no longer be used. In one database transaction, atomically change the presented current token from unused to used, create its successor, revoke/replace access tokens, and update grant activity. If the hash exists but is already consumed outside any configured grace interval, revoke the grant and all descendants. Use a guarded update/CAS, a row lock, or serializable transaction so only one non-grace exchange of a generation succeeds. An authenticated family/grant identifier inside the otherwise opaque token can make lookup efficient, but it does not remove the need to retain consumed generations.

This behavior falls short of [RFC 9700 section 4.14.2](https://www.rfc-editor.org/rfc/rfc9700.html#section-4.14.2), which describes retaining the relationship between rotated tokens so presentation of an invalidated family member can revoke the active token.

### 2. Medium — Stale code and refresh snapshots can restore broader scope than the latest consent

- **Severity:** Medium
- **Confidence:** Confirmed
- **Files:** `src/app/oauth/authorize/page.tsx:84-91`, `src/app/oauth/authorize/page.tsx:139-160`, `src/app/api/oauth/authorize/route.ts:68-88`, `src/app/api/oauth/token/route.ts:99-126`, `src/app/api/oauth/token/route.ts:152-185`, `src/lib/mcp/oauth.ts:217-231`

Authorization codes carry a scope snapshot, but they are bound only to `(client, user)`, not to a particular grant version or consent epoch. `exchangeCode` checks only that some current grant for that pair is active, then issues the code's stored scopes. `issueAccessToken` also writes those scopes back onto the standing grant. Refresh similarly uses a grant object read before issuance and writes that snapshot back.

Concrete exploit path:

1. A malicious public client has an existing broad grant and obtains an auto-approved broad authorization code `C-old` with its own valid PKCE challenge.
2. The user disconnects it, then reconnects the same client read-only. The approval upsert reuses the same grant row, stores the narrow scopes, and clears `revokedAt`.
3. Before `C-old`'s 60-second expiry, the client exchanges it with the correct verifier.
4. `exchangeCode` finds the newly active grant, mints an access token using `C-old`'s broad scopes, and `issueAccessToken` writes those broad scopes back to the grant. The client gets more privilege than the user's latest read-only consent.

A concurrent version does not require a stale code: a refresh request can read broad `grant.scopes`, a narrower consent can be stored, and the refresh can then mint and write the stale broad snapshot. Two different authorization codes can likewise be exchanged in an order chosen by the client, with the last exchange redefining the standing grant.

No path was found that invents an unsupported scope or grants a scope the user never approved at any point. The defect is that an older approval can override the latest effective consent.

**Specific fix:** Make the consent endpoint the only writer of standing grant scopes; remove the scope update from `issueAccessToken`. Add a monotonically increasing consent/grant version and bind every authorization code to the grant ID and version that authorized it. At exchange, require the same active version and require the current grant to cover every code scope; otherwise return `invalid_grant`. Read refresh scopes and atomically rotate the token inside the same transaction so a stale pre-consent snapshot cannot be written back.

### 3. Medium — An in-flight exchange can repopulate a revoked grant and later revive a stolen credential

- **Severity:** Medium
- **Confidence:** Confirmed
- **Files:** `src/app/api/oauth/token/route.ts:99-135`, `src/app/api/oauth/token/route.ts:146-193`, `src/lib/mcp/oauth.ts:212-275`, `src/app/api/oauth/authorize/route.ts:68-74`, `src/lib/mcp/auth.ts:58-66`

Grant validation, access-token issuance, refresh-token storage, and grant revocation are separate transactions. Neither `issueAccessToken` nor `storeRefreshToken` conditionally requires `revokedAt` to remain null. Re-consent then clears only `revokedAt` and changes scopes; it does not clear refresh state or invalidate access tokens that may have been created after the revocation transaction scanned them.

Concrete exploit path:

1. A malicious client or refresh-token thief starts a valid refresh request and passes the active-grant checks.
2. The user disconnects the client. `revokeGrant` marks the grant revoked, clears the two stored refresh hashes, and revokes access tokens visible to that transaction.
3. The already in-flight request resumes. It can create an unrevoked access-token row and/or unconditionally write its newly returned refresh hash onto the now-revoked grant.
4. While `revokedAt` remains set, `verifyMcpToken` correctly rejects the access token and refresh rejects the grant. There is no immediate bypass; this check was traced and is why the finding is not rated High.
5. If the user later reconnects the same client, the approval upsert sets `revokedAt: null` without resetting those raced credentials. The attacker's refresh token becomes usable again for up to 90 days; a raced access token also becomes usable if its one-hour lifetime has not elapsed.

There is a related non-racing state inconsistency. If a grant already has a refresh token and a later authorization code omits `offline_access`, `exchangeCode` skips `storeRefreshToken` but does not clear the existing refresh hashes. The grant scopes can therefore say that offline access is absent while the old refresh token remains accepted. The refresh handler never checks for `offline_access` and returns another refresh token on every successful use. Thus a previously issued refresh credential can continue persistent access after the standing scope array no longer contains the permission that authorized it.

**Specific fix:** Serialize refresh/code issuance against revocation on the grant row and make the active-grant predicate part of the write, not an earlier read. A revoke should advance a grant epoch. Every token/code write should be conditional on the epoch it read, so it fails if revocation won the race. Re-consenting a revoked grant should atomically start a clean epoch: clear all refresh-token state and revoke/delete every old access token before setting the grant active. Also clear refresh state whenever the effective grant drops `offline_access`, and reject refresh grants whose current scope array lacks it. Creating a new grant row is viable if codes are bound to its exact ID/version.

### 4. Medium — Loopback redirect matching ignores every component except scheme, host, and path

- **Severity:** Medium
- **Confidence:** Suspected
- **Files:** `src/lib/mcp/oauth.ts:137-152`, `src/lib/mcp/authorize-request.ts:65-81`

For loopback redirects, `redirectUriMatches` intentionally ignores the port, but it also ignores the query string, fragment, username, and password. A requested loopback URI can therefore differ from its registered value in security-relevant components and still become the actual redirect target.

Concrete conditional exploit path: suppose a legitimate native client's registered loopback callback uses a query parameter to select or forward the post-callback destination. An attacker starts authorization with that legitimate public `client_id`, an attacker-controlled PKCE challenge, and the same loopback scheme/host/path but changes the query to an attacker destination. The server accepts it. If the victim has a covering standing grant, authorization is automatic; otherwise the legitimate client name is shown. A callback handler that forwards the altered query and authorization response can deliver the code to the attacker, who knows the verifier and exchanges it for the victim's token.

The comparison defect is confirmed, but exploitability is marked **Suspected** because the database was deliberately not queried and the external clients' loopback callback behavior is not in this repository. I could not establish that a currently registered callback forwards attacker-controlled query data.

**Specific fix:** For a supported loopback URI, compare the complete registered and requested URI after removing only the port. Query, fragment, path, userinfo, scheme, and host must otherwise match exactly; reject a requested fragment even if the registered URI has none. Apply the same rule at authorization and token exchange. [RFC 9700 section 2.1](https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1) requires exact redirect matching except for the native-loopback port exception and identifies authorization-code leakage as the consequence of loose matching.

### 5. Low — `drinks:read` discloses bar and recipe counts

- **Severity:** Low
- **Confidence:** Confirmed
- **Files:** `src/lib/mcp/tools/drinks.ts:368-400`, `src/lib/mcp/tools/drinks.ts:417-436`, `src/lib/mcp/oauth.ts:27-32`

`get_collection_stats` requires only `drinks:read`, but queries and returns the number of non-empty bar items and saved recipes. The advertised scope descriptions put bar inventory and recipes behind `bar:read`.

Concrete exploit path: a client requests and receives only `drinks:read`, then calls `get_collection_stats`. It receives `barItemCount`, `recipeCount`, and the corresponding text despite the user never granting `bar:read`. This is same-user metadata rather than row contents, so the impact is Low.

**Specific fix:** Remove the bar/recipe counts from this tool, return them only when `caller.scopes` includes `bar:read`, or require both `drinks:read` and `bar:read` for the entire tool. If more cross-axis tools are expected, extend `ToolSpec` to accept an array of required scopes rather than relying on ad hoc checks.

## Material negative results

- **No cross-tenant MCP access found.** All 22 tools are registered through `defineTool`; all 22 declare a scope. Reads use `caller.userId`, creates set it server-side, and updates/deletes first resolve ownership. `rate_drink` verifies the supplied drink belongs to the caller, and `save_recipe` does the same for `sourceDrinkId`.
- **No unauthenticated MCP operation found.** `withMcpAuth` is configured with `required: true`; the verifier accepts only the MCP token shape; no route calls `auth()` as a fallback; cookies cannot authenticate `/api/mcp`. `OPTIONS` exposes no data or state change.
- **Revocation, expiry, and suspension are checked live.** `verifyMcpToken` checks the token row, access-token expiry, OAuth grant revocation, and the joined user's current status on every request. There is no token cache. Finding 3 requires a later re-consent because the live grant check correctly blocks the raced token while the grant remains revoked.
- **PKCE and same-code concurrency are sound.** S256 is mandatory, verifier syntax is constrained, comparison is timing-safe, and the guarded code consume permits only one exchange of a given code. The consume-before-validation ordering can burn a code or let a bad verifier revoke its grant, but that is availability/DoS and is outside the stated threat model.
- **Dynamic registration did not expose SSRF or client secrets.** Registration performs no outbound fetch, generates the client ID server-side, accepts only public clients, and restricts redirects to HTTPS or loopback. Untrusted client metadata is rendered as escaped text in the reviewed UI.
- **Settings management is tenant-scoped.** PAT and OAuth-grant listing/revocation use the cookie-authenticated user's ID in their reads and writes.
- **The deliberate wildcard MCP CORS policy does not combine with cookie auth.** No `Access-Control-Allow-Credentials` header or session-cookie authentication path was found.

## Q1. Should refresh-token replay have a grace window?

Zero grace is security-correct but operationally harsh; it is not required to be zero by the OAuth security BCP. [RFC 9700 section 4.14.2](https://www.rfc-editor.org/rfc/rfc9700.html#section-4.14.2) requires public clients to use sender-constrained refresh tokens or rotation with retained family relationships and describes family revocation when an invalidated token reveals reuse. It does not prescribe a grace duration.

Mature implementations expose both choices. [Auth0 offers a configurable rotation overlap](https://dev.auth0.com/docs/secure/tokens/refresh-tokens/configure-refresh-token-rotation): its default leeway is disabled, but a configured window allows only the immediately previous generation and older generations still trigger family invalidation. [Okta defaults to a 30-second grace period configurable from 0 to 60 seconds](https://developer.okta.com/docs/guides/refresh-tokens/main/#grace-period-for-token-rotation), explicitly to handle a lost response or poor network.

For this app, the present zero-grace behavior is defensible if forced reauthorization after a lost response is acceptable. A short 5–10 second grace window would be a reasonable reliability choice, limited to the immediately preceding generation; any older generation or a replay outside the window should revoke the family. Do **not** add grace to the current implementation first. Finding 1 means old generations are forgotten, and simultaneous current-token requests are not atomically resolved. Implement durable lineage and atomic rotation, then choose grace. For robust retry semantics, either retain enough protected successor state to return the same result idempotently or use generation-aware rotation that cannot let late responses overwrite newer client state.

One correction to the question's premise: the implementation does not revoke the grant whenever a previous token is “ever” presented. It recognizes only the immediately preceding hash until the next successful rotation overwrites it.

## Q2. Can a token receive more scope than the user consented to?

**Normal, non-racing path:** no problem found. `validateAuthorizeRequest` parses the requested scopes; explicit approval writes the same array to both grant and code; auto-approval requires `coversScopes(existing.scopes, requested)` and puts only the requested subset on the code; access-token creation removes only the `offline_access` marker. Unsupported scopes are never written to an access token.

**Across consent changes and concurrency:** yes, a token can exceed the user's latest effective consent, as detailed in Finding 2. A broad code from an older consent can be exchanged after the same grant row is revoked and reactivated with narrow scopes. A refresh that read broad scopes before the consent change can also issue and write those scopes afterward. The code/refresh never creates a scope the user did not historically approve, but it can override a later narrowing or revoke-and-reconsent decision. The root causes are the absence of a consent epoch and `issueAccessToken` treating token issuance as authority to rewrite standing consent.

Two concurrent exchanges of different valid codes can both succeed and whichever token issuance writes last determines the grant's stored scopes. That does not by itself invent scope, but it makes request ordering—not the latest consent action—the final authority.

`offline_access` has a separate persistence defect rather than an access-token-scope escalation: removing it from the effective grant does not clear a previously issued refresh token, and refresh does not require the marker. That path is covered in Finding 3.

## Q3. Is consume-before-grant/PKCE safe, and what happens under concurrency?

The exact ordering is:

| Stage | Code behavior | Security/concurrency result |
|---|---|---|
| 1 | Hash lookup loads the code plus client and user snapshot (`token/route.ts:65-71`). | No lock is held; expiry is not accepted here. |
| 2 | `updateMany` sets `consumedAt` only where it is null and `expiresAt` is still in the future (`:74-87`). | For the **same code**, exactly one concurrent request gets `count=1`; all others fail. Expiry between lookup and consume also fails closed. |
| 3 | Client ID and redirect are checked (`:90-97`). | A wrong request burns the code but gets no token. This is availability, not token theft. |
| 4 | The current grant is loaded by `(client row ID, user ID)` and must be active (`:99-105`). | A missing or already-revoked grant fails. The code is already consumed. |
| 5 | PKCE is verified (`:107-116`). | A mismatch cannot mint a token and revokes the current grant. The code remains consumed. |
| 6 | The user snapshot's status is checked (`:118-120`). | A suspended account gets no token; MCP also rechecks live status on use. |
| 7 | `issueAccessToken` revokes visible access tokens, creates one, and updates grant scopes/activity in a transaction (`oauth.ts:212-233`). | The transaction does not condition on the grant still being active or on a grant version. A concurrent revoke can win without preventing this row from being created, although live grant verification blocks it while revoked. |
| 8 | If requested, the first refresh token is stored in a separate write (`token/route.ts:129-134`). | A revoke can interleave before this write; re-consent can later revive the raced state (Finding 3). |

Direct answers:

- **Can a code outlive its grant?** Yes in the logical authorization sense. `OAuthAuthCode` has foreign keys to client and user, not to `OAuthGrant`, and soft revocation does not consume/delete codes. A code cannot exchange while the grant is revoked, but if the same `(client,user)` row is reactivated before the code's 60-second expiry, the old code becomes usable again. It can carry stale scopes (Finding 2).
- **Can a revoked grant be resurrected?** `exchangeCode` and `issueAccessToken` do not directly clear `revokedAt`, and `verifyMcpToken` checks the grant, so a concurrent exchange does not immediately bypass a grant that stays revoked. The approval upsert intentionally reactivates the row. Because that reactivation has no epoch/reset, it can also reactivate pre-revocation codes or credentials written by an in-flight request (Findings 2 and 3).
- **Can two concurrent exchanges both succeed?** Two exchanges of the **same authorization code cannot both succeed** because of the guarded consume. Two different valid codes for the same grant can both succeed. Two concurrent refresh requests using the same current refresh token can also both succeed because validation and rotation are not one guarded transaction (Finding 1).

Consume-before-PKCE is therefore not the source of a duplicate same-code mint. Its security cost is code burning and grant revocation on a bad verifier; under the stated exclusion of DoS, that ordering is acceptable. The material problems are the missing grant/consent version and the non-atomic writes after the consume.

## Coverage

| Requested file | Coverage | Result |
|---|---|---|
| `src/lib/mcp/oauth.ts` | Fully reviewed | Findings 1–4; PKCE primitive itself clean. |
| `src/app/api/oauth/token/route.ts` | Fully reviewed | Findings 1–3; same-code consume is concurrency-safe. |
| `src/lib/mcp/authorize-request.ts` | Fully reviewed | Fatal-before-redirect validation order is sound; Finding 4 inherits the loopback matcher. |
| `src/app/oauth/authorize/page.tsx` | Fully reviewed | Auto-approve correctly uses `coversScopes`; stale-consent issue is Finding 2. |
| `src/app/api/oauth/authorize/route.ts` | Fully reviewed | Cookie auth and request revalidation are present; Findings 2–3 concern grant reuse/versioning. |
| `src/lib/mcp/auth.ts` | Fully reviewed | Per-request token, expiry, grant, and user-status checks are sound. |
| `src/app/api/mcp/route.ts` | Fully reviewed | Bearer auth is required; no cookie fallback; CORS decision is safe under current wiring. |
| `src/app/api/oauth/register/route.ts` | Fully reviewed | No SSRF or secret storage found; redirect registration restrictions present. |
| `src/lib/mcp/tools/bar.ts` | Fully reviewed | No IDOR or scope bypass found. |
| `src/lib/mcp/tools/chatgpt.ts` | Fully reviewed | No IDOR or scope bypass found. |
| `src/lib/mcp/tools/drinks.ts` | Fully reviewed | No IDOR; Finding 5 is a cross-scope count disclosure. |
| `src/lib/mcp/tools/meta.ts` | Fully reviewed | No IDOR or scope bypass found. |
| `src/lib/mcp/tools/recipes.ts` | Fully reviewed | `sourceDrinkId` ownership is checked; no IDOR found. |
| `src/lib/mcp/tools/wishlist.ts` | Fully reviewed | No IDOR or scope bypass found. |
| `src/lib/mcp/context.ts` | Fully reviewed | Missing auth fails closed; scope check is exact. |
| `src/lib/mcp/define.ts` | Fully reviewed | Every tool goes through auth/scope/error plumbing. |
| `src/lib/mcp/schemas.ts` | Fully reviewed | Image URL omitted; IDs and write schemas traced to validators. |
| `src/lib/auth.ts` | Fully reviewed | `/oauth/authorize` is not public; JWT revalidates active user. |
| `prisma/schema.prisma` | Fully reviewed | OAuth uniqueness/cascades and all related user-owned relationships traced. |
| `src/app/api/settings/mcp-tokens/route.ts` | Fully reviewed | User-scoped listing/creation; no token secret stored. |
| `src/app/api/settings/mcp-tokens/[id]/route.ts` | Fully reviewed | Guarded user-scoped revocation. |
| `src/app/api/settings/mcp-grants/route.ts` | Fully reviewed | User-scoped listing. |
| `src/app/api/settings/mcp-grants/[id]/route.ts` | Fully reviewed | User-scoped lookup before grant revocation. |

Supporting paths fully reviewed: `src/app/api/oauth/revoke/route.ts`, all three OAuth/protected-resource discovery routes under `src/app/.well-known`, `src/middleware.ts`, `src/lib/authz.ts`, `src/components/oauth/consent-form.tsx`, `src/lib/validators.ts`, `src/lib/mcp/{config,tokens,server,audit}.ts`, and the relevant `mcp-handler` authentication wrapper. Relevant ownership invariants were cross-checked in the existing ratings and recipes API routes. The `mcp-handler` package as a whole and unrelated application features were not audited.

Validation: `./node_modules/.bin/tsc --noEmit --incremental false` completed successfully. The application was not run, and no database or service was contacted.