Claude web OAuth connector - #17
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds OAuth 2.1 support for Claude Web/Mobile connectors to the Strava MCP server. The changes include implementing OAuth endpoints (metadata discovery, dynamic client registration, authorization, and token exchange), switching from serverless-express to AWS Lambda Web Adapter for SSE streaming, adding DynamoDB-backed OAuth storage, and integrating AWS Secrets Manager for credential management.
Changes:
- OAuth 2.1 endpoints with Dynamic Client Registration (DCR), PKCE support, and token management
- Migration from serverless-express to AWS Lambda Web Adapter for improved streaming support
- Three new DynamoDB tables for OAuth state management (clients, auth codes, tokens)
- AWS Secrets Manager integration for secure credential storage
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| template.yaml | Added DynamoDB tables, updated Lambda handler to run.sh, increased timeout to 900s, added Secrets Manager permissions, OAuth environment variables |
| src/oauth/utils.ts | Cryptographic utilities for PKCE (S256), token generation, and base64url encoding |
| src/oauth/store.ts | DynamoDB storage layer for OAuth clients, authorization codes, and tokens |
| src/oauth/server.ts | OAuth 2.1 server implementation with metadata, DCR, authorize, and token endpoints |
| src/lambda-web.ts | New Lambda entry point using AWS Lambda Web Adapter |
| src/index.ts | Refactored to use shared createApp function |
| src/app.ts | New shared application factory with MCP server, OAuth routes, and authentication middleware |
| src/config/secrets.ts | AWS Secrets Manager integration for loading Strava credentials |
| src/config/env.ts | Extended environment schema with OAuth configuration options |
| package.json | Added AWS SDK dependencies, removed serverless-express, updated scripts |
| run.sh | Shell script to launch Lambda Web Adapter server |
| docs/* | Updated documentation for OAuth authentication and deployment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| app.get('/authorize', async (req: Request, res: Response) => { | ||
| try { | ||
| const { | ||
| response_type, | ||
| client_id, | ||
| redirect_uri, | ||
| state, | ||
| code_challenge, | ||
| code_challenge_method, | ||
| scope, | ||
| } = req.query as Record<string, string | undefined>; | ||
|
|
||
| if (response_type !== 'code') { | ||
| return writeOAuthError(res, 400, 'unsupported_response_type'); | ||
| } | ||
|
|
||
| if (!client_id || !redirect_uri || !code_challenge || !code_challenge_method) { | ||
| return writeOAuthError(res, 400, 'invalid_request', 'Missing required parameters'); | ||
| } | ||
|
|
||
| if (code_challenge_method !== 'S256') { | ||
| return writeOAuthError(res, 400, 'invalid_request', 'Only S256 is supported'); | ||
| } | ||
|
|
||
| const client = await getClient(config.clientsTable, client_id); | ||
| if (!client) { | ||
| return writeOAuthError(res, 400, 'invalid_client', 'Unknown client'); | ||
| } | ||
|
|
||
| if (!client.redirect_uris.includes(redirect_uri)) { | ||
| return writeOAuthError(res, 400, 'invalid_redirect_uri', 'Redirect URI mismatch'); | ||
| } | ||
|
|
||
| if (!ensureRedirectAllowed(redirect_uri, config.allowedRedirectUris)) { | ||
| return writeOAuthError(res, 400, 'invalid_redirect_uri', 'Redirect URI not allowed'); | ||
| } | ||
|
|
||
| const now = Math.floor(Date.now() / 1000); | ||
| const code = generateToken(24); | ||
| const scopes = normalizeScopes(scope); | ||
|
|
||
| await putAuthCode(config.codesTable, { | ||
| code, | ||
| client_id, | ||
| redirect_uri, | ||
| code_challenge, | ||
| code_challenge_method: 'S256', | ||
| scopes, | ||
| expires_at: now + 300, | ||
| }); | ||
|
|
||
| await touchClient(config.clientsTable, client_id); | ||
|
|
||
| const redirectUrl = new URL(redirect_uri); | ||
| redirectUrl.searchParams.set('code', code); | ||
| if (state) { | ||
| redirectUrl.searchParams.set('state', state); | ||
| } | ||
|
|
||
| res.redirect(302, redirectUrl.toString()); |
There was a problem hiding this comment.
The /authorize endpoint automatically grants authorization without user consent. OAuth 2.0 requires explicit user consent before issuing an authorization code. There's no user interaction, authentication, or authorization decision being made. This endpoint should present a consent page to the user and only issue the authorization code after the user explicitly approves the request. Without user consent, any client with a valid client_id can obtain authorization codes.
| code_challenge, | ||
| code_challenge_method: 'S256', | ||
| scopes, | ||
| expires_at: now + 300, |
There was a problem hiding this comment.
Authorization codes are set to expire in 300 seconds (5 minutes). While OAuth 2.0 recommends short-lived authorization codes, 5 minutes is quite long. RFC 6749 recommends a maximum lifetime of 10 minutes but suggests authorization codes should be short-lived and expire quickly. Consider reducing this to 60-120 seconds to minimize the window for authorization code interception attacks.
| await putToken(config.tokensTable, { | ||
| ...storedToken, | ||
| access_token: accessToken, | ||
| access_expires_at: accessExpiresAt, | ||
| expires_at: storedToken.refresh_expires_at, | ||
| }); | ||
|
|
||
| await touchClient(config.clientsTable, clientId); | ||
|
|
||
| return res.json({ | ||
| access_token: accessToken, | ||
| token_type: 'Bearer', | ||
| expires_in: config.accessTokenTtlSeconds, | ||
| refresh_token: storedToken.refresh_token, | ||
| scope: storedToken.scopes.join(' '), | ||
| }); |
There was a problem hiding this comment.
The refresh token flow reuses the same refresh_token after generating a new access token. According to OAuth 2.0 security best practices and OAuth 2.1 requirements, refresh tokens should be rotated (a new refresh token should be issued) to limit the impact of token theft. Consider generating a new refresh token and invalidating the old one during token refresh to improve security.
| } else if (req.query.access_token) { | ||
| token = req.query.access_token as string; | ||
| } | ||
|
|
||
| if (!token && req.query.token) { | ||
| token = req.query.token as string; | ||
| } | ||
|
|
There was a problem hiding this comment.
Accepting OAuth access tokens via query parameters (access_token and token) is a security risk. Query parameters are logged in server logs, browser history, and can be leaked through Referer headers. OAuth 2.0 Security Best Current Practice (BCP) RFC 8252 explicitly discourages passing access tokens in query parameters. Access tokens should only be accepted via the Authorization header for OAuth flows.
| } else if (req.query.access_token) { | |
| token = req.query.access_token as string; | |
| } | |
| if (!token && req.query.token) { | |
| token = req.query.token as string; | |
| } | |
| } |
| } | ||
| } | ||
|
|
||
| if (!config.OAUTH_ENABLED && !config.AUTH_TOKEN) { |
There was a problem hiding this comment.
When both OAuth and AUTH_TOKEN are disabled/empty, the authentication middleware allows all requests to proceed (line 254-256). This creates an insecure configuration where the Lambda function is publicly accessible without any authentication. Consider requiring at least one authentication method to be configured, or add a warning/error if neither authentication method is enabled in production.
| if (!config.OAUTH_ENABLED && !config.AUTH_TOKEN) { | |
| if (!config.OAUTH_ENABLED && !config.AUTH_TOKEN) { | |
| if (process.env.NODE_ENV === 'production') { | |
| return res.status(500).json({ | |
| error: 'Server misconfiguration', | |
| message: 'No authentication method is configured', | |
| }); | |
| } | |
| // In non-production environments, allow requests when no auth is configured. |
| app.post('/register', async (req: Request, res: Response) => { | ||
| try { | ||
| const { redirect_uris, client_name } = req.body || {}; | ||
|
|
||
| if (!Array.isArray(redirect_uris) || redirect_uris.length === 0) { | ||
| return writeOAuthError(res, 400, 'invalid_client_metadata', 'redirect_uris is required'); | ||
| } | ||
|
|
||
| const invalid = redirect_uris.some( | ||
| (uri: string) => !ensureRedirectAllowed(uri, config.allowedRedirectUris) | ||
| ); | ||
|
|
||
| if (invalid) { | ||
| return writeOAuthError(res, 400, 'invalid_redirect_uri', 'redirect_uri not allowed'); | ||
| } | ||
|
|
||
| const clientId = generateToken(24); | ||
| const now = Math.floor(Date.now() / 1000); | ||
|
|
||
| await putClient(config.clientsTable, { | ||
| client_id: clientId, | ||
| redirect_uris, | ||
| client_name, | ||
| created_at: now, | ||
| }); | ||
|
|
||
| res.status(201).json({ | ||
| client_id: clientId, | ||
| client_id_issued_at: now, | ||
| token_endpoint_auth_method: 'none', | ||
| redirect_uris, | ||
| response_types: ['code'], | ||
| grant_types: ['authorization_code', 'refresh_token'], | ||
| }); | ||
| } catch (error) { | ||
| console.error('[OAuth] Registration error:', error); | ||
| writeOAuthError(res, 500, 'server_error', 'Failed to register client'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The /register endpoint (Dynamic Client Registration) is publicly accessible without authentication. This allows anyone to register OAuth clients, which could be abused for DoS attacks by filling up the DynamoDB table or for phishing attacks by registering malicious clients. Consider adding rate limiting, requiring API keys, or implementing some form of authentication for client registration to prevent abuse.
| return base64UrlEncode(randomBytes(bytes)); | ||
| } | ||
|
|
||
| export function isValidPkceS256(verifier: string, challenge: string): boolean { |
There was a problem hiding this comment.
The PKCE verifier validation only checks if the hash matches the challenge, but doesn't validate the verifier format. According to RFC 7636, the code verifier must be a string of 43-128 characters from the unreserved character set [A-Z], [a-z], [0-9], "-", ".", "_", "~". Add validation to ensure the verifier meets these requirements before attempting to verify it.
| export function isValidPkceS256(verifier: string, challenge: string): boolean { | |
| export function isValidPkceS256(verifier: string, challenge: string): boolean { | |
| // Validate PKCE code verifier according to RFC 7636: | |
| // - Length between 43 and 128 characters | |
| // - Characters limited to unreserved set: [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~" | |
| const length = verifier.length; | |
| if (length < 43 || length > 128) { | |
| return false; | |
| } | |
| if (!/^[A-Za-z0-9\-._~]+$/.test(verifier)) { | |
| return false; | |
| } |
| Effect: Allow | ||
| Action: | ||
| - secretsmanager:GetSecretValue | ||
| Resource: "*" |
There was a problem hiding this comment.
The Secrets Manager policy grants access to all secrets using a wildcard Resource ("*"). This is overly permissive and violates the principle of least privilege. The policy should be restricted to only the specific secret ARN that the function needs to access. Consider using a conditional resource specification, such as allowing access only when SecretsManagerArn is provided, or restrict to secrets with specific naming patterns or tags.
| Resource: "*" | |
| Resource: !Ref SecretsManagerArn |
| Description: Bearer token for authenticating MCP client requests (use a long random string) | ||
| NoEcho: true | ||
| MinLength: 32 | ||
| Default: "" |
There was a problem hiding this comment.
The MinLength constraint for AuthToken parameter was removed. The original constraint required AUTH_TOKEN to be at least 32 characters, which is a security best practice for bearer tokens. While the Default is now empty string to support OAuth-only deployments, removing the MinLength constraint means users could set weak tokens (like "test" or "123"). Consider adding a conditional validation that enforces MinLength when the value is non-empty, or validate this in the application code.
| Default: "" | |
| Default: "" | |
| AllowedPattern: '^$|.{32,}$' |
| const ALLOWED_REDIRECT_URIS = [ | ||
| 'https://claude.ai/api/mcp/auth_callback', | ||
| 'https://claude.com/api/mcp/auth_callback', | ||
| ]; | ||
|
|
There was a problem hiding this comment.
The allowed redirect URIs are hardcoded to only Claude-specific URLs. This prevents the OAuth implementation from being used for testing, local development, or with other OAuth clients that may have different redirect URIs. Consider making the allowed redirect URIs configurable through environment variables or allowing a broader set of redirect URIs for development/testing purposes (e.g., localhost URLs for development).
| const ALLOWED_REDIRECT_URIS = [ | |
| 'https://claude.ai/api/mcp/auth_callback', | |
| 'https://claude.com/api/mcp/auth_callback', | |
| ]; | |
| const DEFAULT_ALLOWED_REDIRECT_URIS = [ | |
| 'https://claude.ai/api/mcp/auth_callback', | |
| 'https://claude.com/api/mcp/auth_callback', | |
| ]; | |
| const envRedirectUris = process.env.ALLOWED_REDIRECT_URIS | |
| ? process.env.ALLOWED_REDIRECT_URIS.split(',').map((uri) => uri.trim()).filter(Boolean) | |
| : []; | |
| const ALLOWED_REDIRECT_URIS = | |
| envRedirectUris.length > 0 ? envRedirectUris : DEFAULT_ALLOWED_REDIRECT_URIS; |
de96efd to
c79443b
Compare
Summary
Testing