mirror of https://github.com/koala73/worldmonitor.git synced 2026-04-25 17:14:57 +02:00

Files

Elie Habib 14a31c4283 feat(mcp): OAuth 2.0 Authorization Server for claude.ai connector (#2418 )

* feat(mcp): add OAuth 2.0 Authorization Server for claude.ai connector

Implements spec-compliant MCP authentication so claude.ai's remote connector
(which requires OAuth Client ID + Secret, no custom headers) can authenticate.

- public/.well-known/oauth-authorization-server: RFC 8414 discovery document
- api/oauth/token.js: client_credentials grant, issues UUID Bearer token in Redis TTL 3600s
- api/_oauth-token.js: resolveApiKeyFromBearer() looks up token in Redis
- api/mcp.ts: 3-tier auth (Bearer OAuth first, then ?key=, then X-WorldMonitor-Key);
  switch to getPublicCorsHeaders; surface error messages in catch
- vercel.json: rewrite /oauth/token, exclude oauth from SPA, CORS headers
- tests: update SPA no-cache pattern

Supersedes PR #2417. Usage: URL=worldmonitor.app/mcp, Client ID=worldmonitor, Client Secret=<API key>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs: fix markdown lint in OAuth plan (blank lines around lists)

* fix(oauth): address all P1+P2 code review findings for MCP OAuth endpoint

- Add per-IP rate limiting (10 req/min) to /oauth/token via Upstash slidingWindow
- Return HTTP 401 + WWW-Authenticate header when Bearer token is invalid/expired
- Add Cache-Control: no-store + Pragma: no-cache to token response (RFC 6749 §5.1)
- Simplify _oauth-token.js to delegate to readJsonFromUpstash (removes duplicated Redis boilerplate)
- Remove dead code from token.js: parseBasicAuth, JSON body path, clientId/issuedAt fields
- Add Content-Type: application/json header for /.well-known/oauth-authorization-server
- Remove response_types_supported (only applies to authorization endpoint, not client_credentials)

Closes: todos 075, 076, 077, 078, 079

🤖 Generated with claude-sonnet-4-6 via Claude Code (https://claude.ai/claude-code) + Compound Engineering v2.40.0

Co-Authored-By: claude-sonnet-4-6 (200K context) <noreply@anthropic.com>

* chore(review): fresh review findings — todos 081-086, mark 075/077/078/079 complete

* fix(mcp): remove ?key= URL param auth + mask internal errors

- Remove ?key= query param auth path — API keys in URLs appear in
  Vercel/CF access logs, browser history, Referer headers. OAuth
  client_credentials (same PR) already covers clients that cannot
  set custom headers. Only two auth paths remain: Bearer OAuth and
  X-WorldMonitor-Key header.

- Revert err.message disclosure: catch block was accidentally exposing
  internal service URLs/IPs via err.message. Restore original hardcoded
  string, add console.error for server-side visibility.

Resolves: todos 081, 082

* fix(oauth): resolve all P2/P3 review findings (todos 076, 080, 083-086)

- 076: no-credentials path in mcp.ts now returns HTTP 401 + WWW-Authenticate instead of rpcError (200)
- 080: store key fingerprint (sha256 first 16 hex chars) in Redis, not plaintext key
- 083: replace Array.includes() with timingSafeIncludes() (constant-time HMAC comparison) in token.js and mcp.ts
- 084: resolveApiKeyFromBearer uses direct fetch that throws on Redis errors (500 not 401 on infra failure)
- 085: token.js imports getClientIp, getPublicCorsHeaders, jsonResponse from shared helpers; removes local duplicates
- 086: mcp.ts auth chain restructured to check Bearer header first, passes token string to resolveApiKeyFromBearer (eliminates double header read + unconditional await)

* test(mcp): update auth test to expect HTTP 401 for missing credentials

Align with todo 076 fix: no-credentials path now returns 401 + WWW-Authenticate
instead of JSON-RPC 200 response. Also asserts WWW-Authenticate header presence.

* chore: mark todos 076, 080, 083-086 complete

* fix(mcp): harden OAuth error paths and fix rate limit cross-user collision

- Wrap resolveApiKeyFromBearer() in try/catch in mcp.ts; Redis/network
  errors now return 503 + Retry-After: 5 instead of crashing the handler
- Wrap storeToken() fetch in try/catch in oauth/token.js; network errors
  return false so the existing if (!stored) path returns 500 cleanly
- Re-key token endpoint rate limit by sha256(clientSecret).slice(0,8)
  instead of IP; prevents cross-user 429s when callers share Anthropic's
  shared outbound IPs (Claude remote MCP connector)

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

2026-03-28 14:53:32 +04:00

4.3 KiB

Raw Permalink Blame History

status, priority, issue_id, tags, dependencies

status

priority

issue_id

Store API key hash (not plaintext) in Redis OAuth token entries

Problem Statement

api/oauth/token.js stores the raw client_secret (= the actual WorldMonitor API key) verbatim in Redis under oauth:token:<uuid>. Anyone who gains read access to Upstash (misconfigured token, support incident, future ACL issue) gets all live API keys in plaintext. Redis is now a second authoritative secret store alongside WORLDMONITOR_VALID_KEYS.

Findings

api/oauth/token.js:33 — value = JSON.stringify({ apiKey, ... }) stores raw API key
api/_oauth-token.js:24 — return entry?.apiKey ?? null returns the raw key directly
api/mcp.ts:431 — uses bearerApiKey directly for rate limiting and downstream calls
Security agent C-1: "Anyone who dumps or scans Redis gets all live API keys in plaintext"
After todo #078 simplification: Redis stores plain string apiKey — still plaintext

Proposed Solutions

Option 1: Store SHA-256 hash of key, re-validate on lookup

Approach:

On token issuance: redis.set('oauth:token:<uuid>', sha256(apiKey))

On Bearer resolution: resolveApiKeyFromBearer returns the hash. Then mcp.ts compares sha256(candidate) against stored value, and validates against WORLDMONITOR_VALID_KEYS using Array.includes (or constant-time comparison).

Actually — the simpler version: store sha256(apiKey) in Redis. On lookup, return the hash. In mcp.ts, compare sha256(candidateKey) against the hash for all valid keys in WORLDMONITOR_VALID_KEYS. If any match, the key is valid.

Pros:

Redis compromise exposes hashes, not live API keys
Defense-in-depth

Cons:

Adds crypto.subtle.digest calls (available in Edge runtime)
Slightly more complex lookup: hash comparison instead of direct string match
Breaking change to stored token format (need migration or versioned format)

Effort: 2-3 hours

Risk: Medium (changing auth critical path)

Option 2: Store a stable key ID (non-reversible label)

Approach: Generate a deterministic short ID from each key (e.g., first 8 chars of SHA-256 hex). Store only this as the token value. On lookup, compute the same ID for each candidate in WORLDMONITOR_VALID_KEYS and find the matching one.

Pros:

Simpler than storing full hash
Redis only exposes a partial fingerprint

Cons:

Still requires iterating WORLDMONITOR_VALID_KEYS on every Bearer lookup

Effort: 2 hours

Risk: Medium

Option 3: Accept the current design (defer)

Approach: Keep raw key storage but document the trust boundary. Rotate API keys immediately on any Upstash access incident.

Pros: No code change Cons: Violates least-privilege principle; Redis breach = all active sessions compromised

Effort: 0

Risk: High (latent)

Recommended Action

Implement Option 2 (key ID/fingerprint). The lookup path is simpler and Redis compromise exposes only partial fingerprints. Block on todo #078 (simplification) since that changes the storage format.

Technical Details

Affected files:

api/oauth/token.js — change stored value from raw key to key fingerprint
api/_oauth-token.js — return fingerprint; update lookup logic in mcp.ts
api/mcp.ts — match fingerprint against computed fingerprints of valid keys

Resources

PR: #2418
Security finding: C-1 (security-sentinel agent)
Architecture note: Architecture-strategist confirmed no-risk for current design but flagged Redis trust boundary

Acceptance Criteria

Redis oauth:token:<uuid> value does not contain raw API key
Bearer token resolution still correctly identifies the originating API key
Upstash dump of oauth:token:* keys reveals no plaintext API keys
Full auth flow still works end-to-end

Work Log

2026-03-28 — Code Review Discovery

By: Claude Code (compound-engineering:ce-review)

Actions:

Security sentinel flagged as CRITICAL (C-1)
Architecture strategist confirmed acceptable design for now but noted trust boundary gap
Marked P2 (not P1) because: current API keys have short TTLs in practice, Upstash access is tightly controlled, and todo #078 must land first to establish the storage format

4.3 KiB Raw Permalink Blame History