Files
worldmonitor/todos/083-complete-p2-timing-unsafe-key-comparison.md
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

3.7 KiB

status, priority, issue_id, tags, dependencies
status priority issue_id tags dependencies
complete p2 083
code-review
security
oauth
timing-attack

Non-constant-time API key comparison enables timing oracle attack

Problem Statement

Both api/oauth/token.js (validateSecret) and api/mcp.ts (direct key path) use Array.includes() for API key validation. JavaScript === exits on the first mismatching byte, creating a timing side-channel. Over enough requests, an attacker can enumerate valid key prefixes character by character. The OAuth token endpoint is the most exposed surface since it validates the full raw API key with no caching.

Findings

  • api/oauth/token.js:47return validKeys.includes(secret);
  • api/mcp.ts:449if (!validKeys.includes(candidateKey))
  • Security sentinel: "JavaScript's === operator exits on the first mismatching byte. For a known partial key prefix, an attacker can run thousands of requests and measure response time differentials to enumerate valid key prefixes character by character."
  • Rate limit (10 req/min per IP) slows but does not prevent: Vercel edge runs globally, a distributed attacker sources from many IPs
  • Fix is crypto.timingSafeEqual on Uint8Array-encoded key bytes (available in Web Crypto API on edge runtimes)

Proposed Solutions

function timingSafeIncludes(candidateKey, validKeys) {
  if (!candidateKey) return false;
  const enc = new TextEncoder();
  const candidate = enc.encode(candidateKey);
  return validKeys.some(k => {
    const valid = enc.encode(k);
    if (valid.length !== candidate.length) return false;
    return crypto.subtle.timingSafeEqual(valid, candidate);
  });
}

Note: crypto.subtle.timingSafeEqual is available in the Web Crypto API (edge runtimes). Node's crypto.timingSafeEqual is not available in Vercel edge.

Pros: Eliminates timing oracle. Cryptographically sound. Cons: Requires encoding keys to Uint8Array before comparison. Slightly more code. Effort: Small (1 helper function, 2 call sites) Risk: Low


Option 2: Use Web Crypto HMAC comparison

Compute HMAC-SHA256(key, nonce) and compare digests. More complex, similar result.

Pros: Industry standard for constant-time comparison. Cons: More complex than timingSafeEqual, requires a nonce. Effort: Medium Risk: Low (but overkill vs Option 1)


Option 3: Accept risk (defer)

The rate limiter and short key length (typical API keys are random high-entropy strings) reduce practical attack feasibility significantly. Accept the risk and document it.

Pros: No code change. Cons: Leaves a known timing oracle on a credential endpoint. Effort: 0 Risk: Medium (latent)

Option 1. crypto.subtle.timingSafeEqual is available on Vercel edge runtime. The fix is a single helper function replacing two Array.includes calls.

Technical Details

Affected files:

  • api/oauth/token.js:45-49 — replace validateSecret
  • api/mcp.ts:448-450 — replace inline validKeys.includes
  • Potentially extract to api/_api-key.js as isValidApiKey(k) (see todo #085)

Acceptance Criteria

  • No Array.includes used for API key comparison in token.js or mcp.ts
  • Constant-time comparison used for all key validation
  • Edge runtime compatibility verified (no Node.js crypto module)
  • Existing tests still pass

Work Log

2026-03-28 — Code Review Discovery

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

Actions:

  • Security sentinel flagged as H-2 (HIGH)
  • Both key validation call sites identified
  • crypto.subtle.timingSafeEqual confirmed available on Web Crypto API (edge compatible)