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, 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 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 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: 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, 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.