mirror of
https://github.com/koala73/worldmonitor.git
synced 2026-04-25 17:14:57 +02:00
14a31c4283
* 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>
111 lines
4.4 KiB
Markdown
111 lines
4.4 KiB
Markdown
---
|
|
status: complete
|
|
priority: p2
|
|
issue_id: "085"
|
|
tags: [code-review, quality, oauth]
|
|
dependencies: []
|
|
---
|
|
|
|
# `api/oauth/token.js` duplicates 4 patterns already in existing helpers
|
|
|
|
## Problem Statement
|
|
|
|
`api/oauth/token.js` re-implements four utilities that already exist in the `api/` helper modules: `getClientIp`, key validation, `jsonResponse`, and CORS headers. This creates maintenance drift — if any of these patterns need to change (CF header priority, allowed CORS headers, key validation logic), `token.js` will not be updated.
|
|
|
|
## Findings
|
|
|
|
**1. `getClientIp` (token.js:36-43) duplicates `_rate-limit.js`**
|
|
|
|
`_rate-limit.js` has an identical function (same header priority: cf-connecting-ip → x-real-ip → x-forwarded-for → 0.0.0.0). If CF header priority changes again (it already changed once per MEMORY.md PR #1241), two files will diverge.
|
|
|
|
**2. `validateSecret` (token.js:45-49) duplicates key parsing from `_api-key.js` and `mcp.ts`**
|
|
|
|
`(process.env.WORLDMONITOR_VALID_KEYS || '').split(',').filter(Boolean).includes(key)` now exists in 3 files. A fourth copy if direct-key path in `mcp.ts` also has it inline (line 448).
|
|
|
|
**3. `jsonResp` + `CORS_HEADERS` (token.js:7-18) duplicates `_json-response.js` + `_cors.js`**
|
|
|
|
`_json-response.js` exports `jsonResponse(body, status, headers)` with the same behavior. `_cors.js` exports `getPublicCorsHeaders()`. The local `CORS_HEADERS` hardcodes `Allow-Headers: Content-Type, Authorization` while the shared module includes `X-WorldMonitor-Key, X-Widget-Key, X-Pro-Key` too. This creates a silent CORS header divergence.
|
|
|
|
**4. `storeToken` raw pipeline fetch duplicates Upstash write pattern**
|
|
|
|
`_upstash-json.js` handles GET. There is no shared `writeJsonToUpstash`. `storeToken` owns the raw pipeline fetch including env-var guards, error handling, and `results[0]?.result === 'OK'` parsing. A second write path from another endpoint cannot reuse this.
|
|
|
|
## Proposed Solutions
|
|
|
|
### Option 1: Import from existing helpers (recommended)
|
|
|
|
```js
|
|
// Replace local getClientIp:
|
|
import { getClientIp } from '../_rate-limit.js'; // requires export on _rate-limit.js
|
|
|
|
// Replace validateSecret:
|
|
import { isValidApiKey } from '../_api-key.js'; // requires new named export
|
|
|
|
// Replace jsonResp + CORS_HEADERS:
|
|
import { getPublicCorsHeaders } from '../_cors.js';
|
|
import { jsonResponse } from '../_json-response.js';
|
|
|
|
// Keep storeToken as-is or extract writeJsonToUpstash to _upstash-json.js
|
|
```
|
|
|
|
**Pros:** Single source of truth for all patterns. ~25 LOC removed from token.js.
|
|
**Cons:** Requires small changes to 2 helper files (export `getClientIp`, add `isValidApiKey`).
|
|
**Effort:** Small-Medium
|
|
**Risk:** Low
|
|
|
|
---
|
|
|
|
### Option 2: Accept local copies, add doc comment explaining why
|
|
|
|
Add a comment: `// Note: local copy of _rate-limit.js:getClientIp to avoid import across api/oauth/ subdirectory.`
|
|
|
|
**Pros:** No helper changes needed.
|
|
**Cons:** Drift risk remains. Comment-based coupling is weaker than import-based coupling.
|
|
**Effort:** Tiny
|
|
**Risk:** Low (but doesn't fix the problem)
|
|
|
|
---
|
|
|
|
### Option 3: Extract all to `api/_oauth-utils.js`
|
|
|
|
Create a dedicated util file for OAuth-specific patterns.
|
|
|
|
**Pros:** Clean separation.
|
|
**Cons:** Overkill — the patterns already exist in shared helpers.
|
|
**Effort:** Small
|
|
**Risk:** Low
|
|
|
|
## Recommended Action
|
|
|
|
Option 1. The import path `../` works since `oauth/token.js` is in `api/oauth/`. Export `getClientIp` from `_rate-limit.js` and `isValidApiKey` from `_api-key.js`. Both are trivial changes.
|
|
|
|
## Technical Details
|
|
|
|
**Affected files:**
|
|
- `api/oauth/token.js` — remove 4 duplicated helpers, add imports
|
|
- `api/_rate-limit.js` — export `getClientIp`
|
|
- `api/_api-key.js` — export `isValidApiKey(k: string): boolean`
|
|
- Optionally: `api/_upstash-json.js` — add `writeJsonToUpstash(key, value, ttlSeconds)`
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] `getClientIp` used from one location
|
|
- [ ] Key validation logic used from one location
|
|
- [ ] CORS headers for token endpoint come from `_cors.js`
|
|
- [ ] `jsonResponse` from `_json-response.js`
|
|
- [ ] No duplicated patterns across `api/` helpers
|
|
- [ ] All existing tests pass
|
|
|
|
## Work Log
|
|
|
|
### 2026-03-28 — Code Review Discovery
|
|
|
|
**By:** Claude Code (compound-engineering:ce-review)
|
|
|
|
**Actions:**
|
|
|
|
- Simplicity reviewer and architecture strategist both flagged
|
|
- getClientIp: exact copy confirmed by code comparison
|
|
- validateSecret pattern: appears in 3+ locations now
|
|
- CORS divergence: `token.js` misses `X-WorldMonitor-Key` from allow-headers vs `_cors.js`
|