Harden managed-mode OAuth and session authorization - #2
Merged
Merged
Conversation
An authenticated AES-256-GCM envelope, keyed from a server secret via HKDF,
for values the server hands out but must be able to read back: OAuth access
and refresh tokens, and client registrations.
The envelope carries its own expiry and is bound to a kind ('access',
'refresh', 'client') through the AEAD additional data, so a value sealed as
one kind can never be opened as another. Because it is authenticated and
self-describing, no server-side store is needed: the values survive a restart
and work across instances that share the secret.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
Addresses an external security report against managed mode v1.5.2 (ST-01,
ST-02). Managed mode only; the selfhosted default has no OAuth endpoints and
no per-request auth, and is unchanged.
ST-02 — the OAuth bridge handed out the user's raw SeaTable API token and
enforced none of its bindings:
* /register generated a client_id and discarded the registration, so
/authorize had nothing to check a callback against. The client_id is now
a sealed envelope carrying the client's name and its redirect_uris; a
client_id we did not issue cannot be opened, and the callback list inside
it cannot be edited.
* redirect_uri was compared only when the token request happened to send
it -- omitting the field skipped the check. It is now required.
* PKCE was verified only when the authorization request had supplied a
challenge. S256 is now mandatory and 'plain' is gone, including from the
advertised metadata.
* The authorization code was not bound to a client. It now is, and the
exchanging client must match.
* access_token and refresh_token were the raw SeaTable API token, byte for
byte. They are now sealed envelopes -- one hour for access, fourteen days
for a rotating refresh token -- and the API token never leaves the
process. resolveAccessToken() unseals it server-side.
* The refresh grant echoed back whatever it was given. It now rejects any
value it did not issue.
With open dynamic registration, "registered client" is not a trust
statement: an attacker can register honestly. So the callback policy asks a
different question -- does the code leave the user's machine? Loopback and
private-use app schemes (cursor://, vscode://) stay local and pass without
friction; a remote https destination that is not curated is still allowed,
but only after the user acknowledges where their token is about to be sent.
That acknowledgement is read from our own form body and requires
Sec-Fetch-Site: same-origin, so neither the entry link nor a foreign
auto-submit can skip it. The consent screen now leads with the destination
and marks the application name as self-reported, because it is chosen by
whoever registered the client and cannot be verified.
ST-01 — after initialization, POST/GET/DELETE /mcp authorized on the
mcp-session-id header alone. A request with no token, an invalid token, or
another account's token was routed to the session owner's client. Every
request now carries a credential, it is validated, and it must resolve to the
identity that created the session (401 / 403). The session ID is a routing
value and is logged only as a fingerprint. The positive validation cache
drops from five minutes to one, which is the window in which a revoked token
still passes.
Also in this change:
* The OAuth endpoints bypassed the rate limiter entirely, leaving
POST /authorize usable as an unthrottled oracle for testing SeaTable API
tokens -- and forwarding every attempt to the SeaTable backend. Now 30
requests/min per IP across the OAuth endpoints, 10/min for token
submissions.
* Roughly half of all real authorization attempts failed with a bare
"Invalid API token". The token is now trimmed before use, and on failure
the server distinguishes an account API token from a base API token and
says which one is needed and where to find it.
* The logs could not answer the report's forensic questions: no client IP,
no callback destination, no link between an issued code and its
exchange, and several rejection paths logged nothing at all. Every OAuth
event now carries ip, clientName and the callback origin; a derived flow
id pairs the authorization with its exchange; no rejection path is
silent. Neither the code, nor a prefix of it, nor the API token is ever
written to the log.
BREAKING CHANGE: managed mode requires SEATABLE_TOKEN_SECRET (min. 32 chars,
stable across restarts) and refuses to start without it. Existing OAuth
clients must register and authorize again: their stored client_id is not a
sealed envelope and will be rejected. Clients that omit redirect_uri at the
token endpoint, or that do not use PKCE S256, no longer complete the flow.
Raw SeaTable API tokens continue to be accepted as bearer credentials.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
Covers SEATABLE_TOKEN_SECRET, the callback policy and its confirmation step, SEATABLE_OAUTH_TRUSTED_REDIRECT_HOSTS, and the per-request credential rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
Trivy fails the build on HIGH/CRITICAL findings, and six npm packages had accumulated advisories since the 1.5.2 image was built in March. All resolve within their existing semver ranges, so package.json is unchanged: axios 1.13.6 -> 1.19.0 (direct; prototype pollution, DoS) fast-uri 3.1.0 -> 3.1.6 (host confusion, policy bypass) form-data 4.0.5 -> 4.0.6 (CRLF field override) hono 4.12.4 -> 4.13.4 (CORS reflects any origin) ip-address 10.1.0 -> 10.5.0 (SSRF via parsing inconsistency) path-to-regexp 8.3.0 -> 8.4.2 (regex DoS) vite and vitest still carry advisories but are devDependencies; the image runs npm prune --production, so they are not shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
The registration log carried only clientName. When a real client authorized against the staging instance, the log could not answer whether the callback it used matched the one it registered -- which is exactly what /authorize decides on, and the only way to tell whether the RFC 8252 loopback port carve-out was exercised or merely present. Both fields come from an unauthenticated caller, so the list is capped at five entries of 120 characters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
Only failed refreshes were logged, so the healthy case was invisible. That matters now: 1.5.2 returned no expires_in and clients treated the token as permanent, while an access token now lives one hour. A client that does not renew cleanly would prompt its user for the API token every hour -- and without this line, that would only surface as user complaints. The refresh token itself is never logged; the client is identified by a truncated digest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
SEATABLE_ACCESS_TOKEN_TTL, in seconds, default 3600, range 30..2592000. One hour is a compromise between "a stolen token expires soon" and "the user is not asked for their API token again", and the right value depends on how real clients behave. 1.5.2 returned no expires_in at all, so whether ChatGPT and Claude renew silently is still unknown; a short value makes that observable in minutes instead of an hour. The refresh token is never issued shorter-lived than the access token, so a long access lifetime cannot silently invert the relationship. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
Registration logged full redirect_uris while code issuance logged only the origin, so the two lines could not be compared -- and the origin is the wrong half: for an incident the question is where the code was actually delivered, path included. All callback fields now carry the full destination, capped at 200 characters because the value comes from an unauthenticated caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the findings from the external security report against managed mode v1.5.2 (ST-01, ST-02). Supersedes #1, which identified the same
redirect_uriweakness on 3 July but landed a narrower fix; itsSEATABLE_OAUTH_TRUSTED_REDIRECT_HOSTSidea is carried over here with different semantics (friction, not a gate).Managed mode only. The selfhosted default has no OAuth endpoints and no per-request auth, and is unchanged.
ST-02 — OAuth token theft
The bridge handed out the user's raw SeaTable API token as both
access_tokenandrefresh_token, and enforced none of its bindings:/registerdiscarded the registrationclient_idis a sealed envelope carrying the client's name and itsredirect_urisclient_id/redirect_uriunchecked at/authorizeredirect_uriat/tokenskipped the comparisonplainremoved from code and metadataCallback policy. With open dynamic registration, "registered client" is not a trust statement — an attacker registers honestly. So the question asked is whether the code leaves the user's machine:
cursor://,vscode://)httpson a curated hosthttpselsewherehttp,javascript:,data:,file:,blob:The acknowledgement is read from our own form body only and requires
Sec-Fetch-Site: same-origin, so neither the entry link nor a foreign auto-submit can skip it. The consent screen leads with the destination and marks the application name as self-reported — it is chosen by whoever registered the client.ST-01 — session ID accepted as sole credential
After initialization,
POST/GET/DELETE /mcpauthorized on themcp-session-idheader alone; no token, an invalid token, or another account's token all reached the session owner's client. Every request now carries a credential, it is validated, and it must resolve to the identity that created the session (401 / 403). Session IDs are logged as a fingerprint only. The positive validation cache drops from 5 min to 1 min.Also in scope
handleMcpRequest.POST /authorizewas an unthrottled oracle for testing SeaTable API tokens, forwarding every attempt to the SeaTable backend. Now 30/min per IP, 10/min for token submissions.ip,clientNameand the callback origin; a derivedflowid pairs an authorization with its exchange; no rejection path is silent. Neither the code, nor a prefix of it, nor the API token is ever logged.Breaking
SEATABLE_TOKEN_SECRET(min. 32 chars, stable across restarts) and refuses to start without it.client_idis not a sealed envelope.redirect_uriat the token endpoint, or that do not use PKCE S256, no longer complete the flow.Verification
284 tests, 46 of them new and written test-first against the report's own attack sequences — each was demonstrated red before the fix. Coverage lives in
tests/oauthProvider.security.spec.ts,oauthRedirectPolicy.spec.ts,oauthRateLimit.spec.ts,oauthTokenInput.spec.ts,oauthObservability.spec.ts,managedSessionAuth.spec.tsandtokenCipher.spec.ts. Several pre-existing tests had pinned the vulnerable behaviour as expected (refresh_token grant returns same token,access_token === raw token) and were rewritten deliberately.Additionally verified end to end against a real SeaTable 6.2.12: the account-vs-base token probe (200/401 vs 200/403), the full register → authorize → exchange flow, all three acknowledgement bypasses, and every ST-01 negative control including a cross-identity 403.
Not covered: the real ChatGPT / Claude clients. That needs a staging run before merge — in particular whether their registered
redirect_urimatches the one they authorize with, and whether they recover cleanly from a rejected staleclient_id.Not addressed
The webhook SSRF from the same report (ST-03) lives in the SeaTable server, not here.
🤖 Generated with Claude Code
https://claude.ai/code/session_014k5RGUNUDegp7Fhsotwiyi