feat: Redis-backed OAuth state + stateless transport for multi-instance support#15
Conversation
Add RedisOAuthProxy that stores OAuth transactions and authorization codes in Redis instead of in-memory Maps. This enables the OAuth flow to work across multiple server instances behind a load balancer. When REDIS_URL env var is set, the proxy uses Redis; otherwise falls back to the default in-memory OAuthProxy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stateless mode creates a fresh server/transport per request, eliminating the need for sticky sessions. Each instance can handle any request independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PLT-953 Redis-backed OAuth state for multi-instance MCP deployments
ProblemThe OAuth flow (authorize → callback → token exchange) stores state in in-memory Maps inside This was causing production 404 "Session not found" errors and failed OAuth flows. FixCreated
When
Branch
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Redis-backed OAuth state: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP Server (RedisOAuthProxy)
participant Redis
participant Provider as OAuth Provider (Upstream)
Client->>MCP: /authorize request
MCP->>MCP: validate params & PKCE
MCP->>Redis: store OAuthTransaction (with TTL)
MCP->>Provider: redirect to upstream authorize
Provider-->>Client: redirect back with code
Client->>MCP: /callback with code & state
MCP->>Redis: load OAuthTransaction
MCP->>Provider: exchange upstream code for tokens
Provider-->>MCP: return tokens
MCP->>Redis: store client authorization code (with TTL)
MCP->>Redis: delete transaction key
MCP-->>Client: redirect with client code & state
Client->>MCP: /token exchange (authorization_code + code_verifier)
MCP->>Redis: load client code
MCP->>MCP: validate PKCE (may delegate to parent in-memory map)
MCP->>Redis: delete client code (one-time use)
MCP-->>Client: return TokenResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/redis-oauth-proxy.ts (3)
253-255: Consider documenting Redis client lifecycle.The
destroy()method delegates to the parent but doesn't close the Redis connection. This is correct since the Redis client is shared and managed externally (insrc/index.ts). Consider adding a brief comment clarifying this intentional design.override destroy(): void { + // Redis client is shared and managed externally; not closed here super.destroy(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis-oauth-proxy.ts` around lines 253 - 255, Add a short inline comment to the override destroy() method in src/lib/redis-oauth-proxy.ts explaining that the method intentionally delegates to super.destroy() and does not close the Redis connection because the Redis client is shared and lifecycle-managed externally (referencing the shared client initialization in src/index.ts); mention that closing the client here would be unsafe to avoid confusion for future maintainers.
61-80: Fragile reliance on private OAuthProxy internals.Accessing TypeScript-private members via type casting (
_internal) is inherently fragile. Iffastmcprenames or restructures these internal methods (createTransaction,generateAuthorizationCode, etc.) in a minor version update, this code will break at runtime without compile-time warnings.Consider adding a version check or documenting the specific
fastmcpversion this was tested against:// Tested with fastmcp@3.33.0 - internal API may change in future versionsAlternatively, consider opening an issue with the
fastmcpmaintainers to expose stable APIs for custom storage backends.Also applies to: 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis-oauth-proxy.ts` around lines 61 - 80, This code relies on private internals of OAuthProxy via the OAuthProxyInternals cast and the _internal property (including createTransaction, generateAuthorizationCode, exchangeUpstreamCode, redirectToUpstream, clientCodes, config), which is fragile; update the implementation to add a runtime guard and documentation: detect and assert presence and expected types of the _internal members at startup (throw a clear error if missing or shape differs), log or throw a descriptive message that includes the fastmcp version, and add a short comment noting the tested fastmcp version (e.g., tested with fastmcp@X.Y.Z) and/or open an issue with fastmcp to request a stable public API; use these checks before calling createTransaction/generateAuthorizationCode/redirectToUpstream so the failure is explicit rather than a silent runtime break.
166-177: Add defensive check for missing codeData.If
generateAuthorizationCodesucceeds but somehow the code isn't in the Map (unlikely but possible in edge cases or future fastmcp changes), the authorization code won't be persisted to Redis. This would cause a silent failure where the callback succeeds but subsequent token exchange fails.🛡️ Suggested defensive handling
const clientCode = this._internal.generateAuthorizationCode( transaction, upstreamTokens, ); const codeData = this._internal.clientCodes.get(clientCode); - if (codeData) { + if (!codeData) { + throw new OAuthProxyError( + 'server_error', + 'Failed to generate authorization code', + ); + } const codeTtl = this._internal.config.authorizationCodeTtl || DEFAULT_CODE_TTL; await this.redis.set( `${CODE_PREFIX}${clientCode}`, serialize(codeData), 'EX', codeTtl, ); this._internal.clientCodes.delete(clientCode); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis-oauth-proxy.ts` around lines 166 - 177, Add a defensive check after reading from this._internal.clientCodes: if codeData is undefined, log a clear error including the clientCode (use this._internal.logger.error or equivalent) and throw an error (or return a rejected promise) so the caller of generateAuthorizationCode does not silently succeed without persisting the code; otherwise proceed with computing codeTtl (this._internal.config.authorizationCodeTtl || DEFAULT_CODE_TTL), call this.redis.set(`${CODE_PREFIX}${clientCode}`, serialize(codeData), 'EX', codeTtl), and delete the entry with this._internal.clientCodes.delete(clientCode).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 68-74: The "Redis connected" message is misleading because ioredis
connects lazily; update the logic around the redisClient creation: instead of
logging connection immediately after new Redis(...), attach a listener for the
Redis client's actual connection event (e.g., 'connect' or 'ready') and log
"Redis connected for OAuth state storage" inside that handler, or change the
immediate log to a neutral message like "Redis client instantiated for OAuth
state storage"; reference the redisClient variable and its 'on' event handler
used for 'error' to add the new 'connect'/'ready' handler or to alter the
existing log text.
---
Nitpick comments:
In `@src/lib/redis-oauth-proxy.ts`:
- Around line 253-255: Add a short inline comment to the override destroy()
method in src/lib/redis-oauth-proxy.ts explaining that the method intentionally
delegates to super.destroy() and does not close the Redis connection because the
Redis client is shared and lifecycle-managed externally (referencing the shared
client initialization in src/index.ts); mention that closing the client here
would be unsafe to avoid confusion for future maintainers.
- Around line 61-80: This code relies on private internals of OAuthProxy via the
OAuthProxyInternals cast and the _internal property (including
createTransaction, generateAuthorizationCode, exchangeUpstreamCode,
redirectToUpstream, clientCodes, config), which is fragile; update the
implementation to add a runtime guard and documentation: detect and assert
presence and expected types of the _internal members at startup (throw a clear
error if missing or shape differs), log or throw a descriptive message that
includes the fastmcp version, and add a short comment noting the tested fastmcp
version (e.g., tested with fastmcp@X.Y.Z) and/or open an issue with fastmcp to
request a stable public API; use these checks before calling
createTransaction/generateAuthorizationCode/redirectToUpstream so the failure is
explicit rather than a silent runtime break.
- Around line 166-177: Add a defensive check after reading from
this._internal.clientCodes: if codeData is undefined, log a clear error
including the clientCode (use this._internal.logger.error or equivalent) and
throw an error (or return a rejected promise) so the caller of
generateAuthorizationCode does not silently succeed without persisting the code;
otherwise proceed with computing codeTtl
(this._internal.config.authorizationCodeTtl || DEFAULT_CODE_TTL), call
this.redis.set(`${CODE_PREFIX}${clientCode}`, serialize(codeData), 'EX',
codeTtl), and delete the entry with
this._internal.clientCodes.delete(clientCode).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75812b3e-8aea-49c0-9fa5-a70581909a9a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/config.tssrc/index.tssrc/lib/redis-oauth-proxy.ts
ioredis connects lazily — logging immediately after instantiation is misleading. Move the log to the 'ready' event so it reflects actual connection status. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Enables the MCP server to run with multiple instances behind a load balancer by:
Redis-backed OAuth state — New
RedisOAuthProxyextendsOAuthProxyand stores the two pieces of OAuth flow state that need cross-instance sharing (transactions at/authorizeand authorization codes at/callback) in Redis with TTL-based expiry. The MCP client's token exchange can now land on any instance.Stateless HTTP transport — Enables FastMCP's
stateless: truehttpStream option. Each request creates a fresh server/transport, eliminating in-memoryactiveTransportstracking. No sticky sessions needed.Config
REDIS_URLenv var — when set, usesRedisOAuthProxy; otherwise falls back to default in-memoryOAuthProxy.Design notes
registerClientandhandleConsentare not overridden:registeredClientsMap is write-only (never read by other methods), and consent is disabled in our config.Datetypes.Test plan
max_instances> 1 in terraform)Closes PLT-953
🤖 Generated with Claude Code
Summary by CodeRabbit