fix: require a durable OAuth token store for http/sse - #213
fix: require a durable OAuth token store for http/sse#213sriramveeraghanta wants to merge 1 commit into
Conversation
An in-memory token store is not a safe default for a transport that hands out OAuth credentials. The store holds the state *behind* the tokens clients already have — the DCR client registration and the JTI mappings — so losing it does not expire a session, it strands one. What the client sees after a restart is not a prompt to sign in again. The refresh grant answers invalid_grant, or invalid_client once the registration is gone too, and those are two of the three codes that make a compliant MCP client erase its stored credentials outright rather than retry. The user is logged out with nothing to distinguish it from a real revocation, and a `logger.warning` at boot is the only trace. So http/sse now refuse to start without a store that survives the process, and PLANE_ALLOW_EPHEMERAL_TOKEN_STORE is the way to accept the trade for local development. Two related fixes in the same path: REDIS_SSL reached the startup PING and the log line but never RedisStore itself. Against a TLS-only Redis that is the worst shape a misconfiguration can take: the eager PING succeeds, boot looks healthy, and every store operation afterwards dials plaintext and fails. The log even said ssl=True. The store was built once per get_oauth_mcp() call and __main__ calls it twice, so /http and /sse each held their own. Under Redis they happened to converge on one keyspace; under the in-memory store they never agreed at all. It is now built once and passed in, so the mounts share state by construction rather than by coincidence. Claude-Session: https://claude.ai/code/session_01GkDqALzi8u1FZcMybxqBoQ
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughHTTP/SSE startup now requires configured token storage by default. An explicit environment variable enables ephemeral in-memory storage. HTTP startup shares one token store across OAuth MCP instances. ChangesOAuth token storage
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes token storage and sharing behavior explicit for HTTP/SSE deployments, with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant HTTPStartup
participant TokenStoreBuilder
participant OAuthMCP
participant SharedTokenStore
HTTPStartup->>TokenStoreBuilder: Build one token store
TokenStoreBuilder-->>HTTPStartup: Return shared token store
HTTPStartup->>OAuthMCP: Create /http instance with shared storage
HTTPStartup->>OAuthMCP: Create SSE instance with shared storage
OAuthMCP->>SharedTokenStore: Resolve OAuth client registrations
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I reproduced a startup regression in the static-password Redis path on the current PR head ( From a clean checkout:
Result: 43 passed, 3 failed. All three failures come from passing One possible fix is to apply the TLS setting to a |
2 participants
Why
Users connecting Claude Code to the MCP server over OAuth get logged out after a
few hours. Investigating that turned up two independent causes; this PR closes
the second one and a bug next to it.
The token store holds the state behind the tokens clients already hold — the
DCR client registration and the JTI mappings. Losing it doesn't expire a
session, it strands one. The refresh grant then answers
invalid_grant, orinvalid_clientonce the registration is gone too, and those are two of thethree codes that make a compliant MCP client erase its stored credentials
rather than retry. The user is logged out with nothing to distinguish it from a
real revocation.
Reproduced against the pinned fastmcp 3.2.0 with a stub upstream:
REDIS_HOST/REDIS_PORTare optional today and set nowhere in the Dockerfile,README, or CI — so the in-memory fallback is what a default deployment gets,
announced only by a
logger.warningat boot.What changed
storage.py— the in-memory fallback is now opt-in, not a fallback.http/sse refuse to start without a store that survives the process. Set
PLANE_ALLOW_EPHEMERAL_TOKEN_STORE=trueto accept the trade locally.storage.py—REDIS_SSLnow reachesRedisStore. It reached the startupPING and the log line but not the store. Against a TLS-only Redis that is the
worst shape a misconfiguration can take: the eager PING succeeds, boot looks
healthy, the log says
ssl=True, and every store operation afterwards dialsplaintext and fails.
server.py/__main__.py— one store, shared. It was built once perget_oauth_mcp()call and__main__calls it twice, so/httpand/sseeachheld their own. Under Redis they happened to converge on one keyspace; under the
in-memory store they never agreed at all.
Deploy note
REDIS_HOST/REDIS_PORTaren'tset. That's the intent — but the env has to be set first. Startup now prints
either
Token store: Redis (...)or refuses outright, which also makes thefirst diagnostic question answerable from the boot log.
Tests
Same 18 pre-existing failures (all in
tests/tools/, unrelated to this path),+7 new tests, no regressions.
ruff checkandruff format --checkclean.The test that pinned the old fallback now pins the refusal. New coverage for the
opt-in, for non-truthy opt-in values, and for TLS reaching the store — that last
one was verified to fail without the fix.
Not in this PR
The primary cause is separate:
PlaneOAuthTokenVerifier.verify_tokenmakes twouncached Plane API calls per MCP request and turns any transient failure into a
401, which the client answers with a refresh, which
proxy.py:1268-1270reportsas
invalid_grant— same credential erasure, no restart required. That is whatfix-oauth-timeout-erroraddresses, and it should land too.https://claude.ai/code/session_01GkDqALzi8u1FZcMybxqBoQ
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests