diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 651304e..e20bc38 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,10 +1,10 @@ -{"id":"forgejo-mcp-broker-q4x","title":"Phase 5c: idle reaper + Forgejo token rotation + child respawn","description":"Reaper (30s tick) applies idle timeout. Rotation (1-min tick) refreshes Forgejo tokens expiring \u003c2min, SIGTERMs child, respawns on next request (design.md §6). Token revocation tears down sessions.","acceptance_criteria":"Clock-injected tests: idle kill, rotation triggers respawn, revocation tears down sessions. Smoke test: 20 concurrent sessions for 10min with mid-test rotations.","status":"in_progress","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:18Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T15:27:46Z","started_at":"2026-04-27T15:27:46Z","dependencies":[{"issue_id":"forgejo-mcp-broker-q4x","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:31Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-q4x","depends_on_id":"forgejo-mcp-broker-t81","type":"blocks","created_at":"2026-04-24T17:45:31Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":2,"dependent_count":1,"comment_count":0} +{"id":"forgejo-mcp-broker-q4x","title":"Phase 5c: idle reaper + Forgejo token rotation + child respawn","description":"Reaper (30s tick) applies idle timeout. Rotation (1-min tick) refreshes Forgejo tokens expiring \u003c2min, SIGTERMs child, respawns on next request (design.md §6). Token revocation tears down sessions.","acceptance_criteria":"Clock-injected tests: idle kill, rotation triggers respawn, revocation tears down sessions. Smoke test: 20 concurrent sessions for 10min with mid-test rotations.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:18Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T15:32:44Z","started_at":"2026-04-27T15:27:46Z","closed_at":"2026-04-27T15:32:44Z","close_reason":"StartReaper shipped: idle eviction + Forgejo token rotation with atomic backend swap. Caught and fixed two -race issues (Done-watcher post-rotation; fakeSpawner test helper). 89.7% session coverage. Phase 5 complete.","dependencies":[{"issue_id":"forgejo-mcp-broker-q4x","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:31Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-q4x","depends_on_id":"forgejo-mcp-broker-t81","type":"blocks","created_at":"2026-04-24T17:45:31Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":2,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-ytw","title":"Phase 5b: bearer-token middleware on /mcp","description":"Middleware reads Authorization: Bearer \u003cmcp_token\u003e, resolves via store, attaches Forgejo access token to request context. 401 for missing/expired/revoked.","acceptance_criteria":"Table-driven tests: missing header, malformed, unknown token, expired, revoked, valid. Valid-token path puts Forgejo token on ctx via typed key.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:18Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T15:10:28Z","started_at":"2026-04-27T15:08:52Z","closed_at":"2026-04-27T15:10:28Z","close_reason":"Bearer middleware shipped: RequireBearer wraps protected handlers, looks up access_tokens by hash, rejects expired/revoked/unknown with RFC 6750 WWW-Authenticate. Session attached to ctx for downstream MCP endpoint use.","dependencies":[{"issue_id":"forgejo-mcp-broker-ytw","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:30Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-t81","title":"Phase 5a: session registry + spawn-on-initialize","description":"Map Mcp-Session-Id -\u003e supervisor.Child + user metadata. On first initialize for unknown sid, spawn forgejo-mcp with user's Forgejo token in env, bind to bridge. LastActive bumped per request.","acceptance_criteria":"Tests with fake supervisor + fake bridge cover: spawn-on-initialize, reuse for subsequent messages, unknown-sid returns 410, max-sessions cap enforced.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:17Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T15:24:32Z","started_at":"2026-04-27T15:11:43Z","closed_at":"2026-04-27T15:24:32Z","close_reason":"Session registry shipped: Mcp-Session-Id minting on initialize, sid reuse for follow-ups, 410 for unknown, 403 for sid-hijack, max-sessions cap with Retry-After, Done-channel reaping, graceful Stop. Decoupled from supervisor/bridge via SpawnFunc. 83.3% coverage.","dependencies":[{"issue_id":"forgejo-mcp-broker-t81","depends_on_id":"forgejo-mcp-broker-am1","type":"blocks","created_at":"2026-04-24T17:45:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t81","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:30Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t81","depends_on_id":"forgejo-mcp-broker-zuq","type":"blocks","created_at":"2026-04-24T17:45:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":3,"dependent_count":2,"comment_count":0} {"id":"forgejo-mcp-broker-xot","title":"Phase 4b: bridge integration test against real forgejo-mcp","description":"Drive the bridge with initialize -\u003e tools/list -\u003e tools/call get_forgejo_mcp_server_version against a real forgejo-mcp subprocess. Validates the opaque-pipe assumption.","acceptance_criteria":"Full handshake, tools/list returns expected set, tools/call returns a version string. Tagged as integration test if runtime exceeds 2s.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:16Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T14:28:39Z","started_at":"2026-04-27T14:10:04Z","closed_at":"2026-04-27T14:28:39Z","close_reason":"Bridge integration test passes against real forgejo-mcp 2.2.0: MCP handshake (initialize → notifications/initialized → tools/list → tools/call) round-trips through bridge cleanly. Fake Forgejo covers /api/v1/version and /api/v1/user probes. Phase 4 complete.","dependencies":[{"issue_id":"forgejo-mcp-broker-xot","depends_on_id":"forgejo-mcp-broker-am1","type":"blocks","created_at":"2026-04-24T17:45:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} {"id":"forgejo-mcp-broker-31t","title":"Phase 3b: supervisor stress tests (FD/goroutine/zombie leak detection)","description":"1000 spawn/stop cycles under -race. Verify no FD leak, no goroutine leak (go.uber.org/goleak), no zombies (wait4 returns ECHILD when idle).","acceptance_criteria":"Cycle test passes under -race. FD count stable within a small constant. goleak detects no extra goroutines after test.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:15Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T14:04:42Z","started_at":"2026-04-27T12:00:32Z","closed_at":"2026-04-27T14:04:42Z","close_reason":"Stress tests in place: 1000-cycle spawn/reap and 200-cycle Stop both clean under -race; FD/goroutine/zombie deltas all single-digit. Driver: /bin/true and /bin/cat (helper-process recursion at scale exposed an unrelated Go pidfd interaction). Supervisor now defensively closes pipe handles post-Wait.","dependencies":[{"issue_id":"forgejo-mcp-broker-31t","depends_on_id":"forgejo-mcp-broker-zuq","type":"blocks","created_at":"2026-04-24T17:45:26Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} {"id":"forgejo-mcp-broker-am1","title":"Phase 4a: internal/bridge JSON-RPC pipe + SSE writer","description":"Given a supervisor.Child: inbound HTTP JSON -\u003e newline-framed stdin; stdout lines -\u003e SSE frames. Handle client disconnect without killing the child.","acceptance_criteria":"Unit tests with mock Child that echoes: request/response round trip, multiple concurrent requests with correct id routing, client disconnect mid-stream.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:15Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T11:59:35Z","started_at":"2026-04-27T11:56:15Z","closed_at":"2026-04-27T11:59:35Z","close_reason":"Bridge shipped: per-id routing, SSE responses for request/reply messages, 204 for notifications, structured 4xx/5xx for malformed input. Decoupled from supervisor (takes pipes directly) for clean testing via io.Pipe. 90.0% coverage.","dependencies":[{"issue_id":"forgejo-mcp-broker-am1","depends_on_id":"forgejo-mcp-broker-zuq","type":"blocks","created_at":"2026-04-24T17:45:27Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":2,"comment_count":0} -{"id":"forgejo-mcp-broker-wgo","title":"Phase 2e: OAuth security review + attack-path tests","description":"Phase 2 exit gate. Review every handler for classic OAuth vulns (open redirect, code replay, mix-up, token leak in logs, host spoofing). Add at least one test per attack class. Update design.md §8 with findings.","acceptance_criteria":"Review checklist documented. Tests added for: PKCE mismatch, stale code, token absent from log attributes, bad redirect_uri, mismatched state, replay of used code.","status":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:14Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:45:14Z","dependencies":[{"issue_id":"forgejo-mcp-broker-wgo","depends_on_id":"forgejo-mcp-broker-b2o","type":"blocks","created_at":"2026-04-24T17:45:26Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-wgo","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:25Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":2,"dependent_count":0,"comment_count":0} +{"id":"forgejo-mcp-broker-wgo","title":"Phase 2e: OAuth security review + attack-path tests","description":"Phase 2 exit gate. Review every handler for classic OAuth vulns (open redirect, code replay, mix-up, token leak in logs, host spoofing). Add at least one test per attack class. Update design.md §8 with findings.","acceptance_criteria":"Review checklist documented. Tests added for: PKCE mismatch, stale code, token absent from log attributes, bad redirect_uri, mismatched state, replay of used code.","status":"in_progress","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:14Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T15:33:21Z","started_at":"2026-04-27T15:33:21Z","dependencies":[{"issue_id":"forgejo-mcp-broker-wgo","depends_on_id":"forgejo-mcp-broker-b2o","type":"blocks","created_at":"2026-04-24T17:45:26Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-wgo","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:25Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":2,"dependent_count":0,"comment_count":0} {"id":"forgejo-mcp-broker-zuq","title":"Phase 3a: internal/supervisor managed stdio subprocess","description":"Child type: Start, Stop(ctx) with SIGTERM -\u003e grace -\u003e SIGKILL, Wait+reap goroutine (no zombies), stderr drainer with prefix. Protocol-agnostic.","acceptance_criteria":"Unit tests against an echo-loop helper: round trip, graceful stop, kill-after-grace, child-exits-on-own detection, stderr capture. Manual spawn of real forgejo-mcp --transport stdio works.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:14Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T11:41:07Z","started_at":"2026-04-27T11:32:54Z","closed_at":"2026-04-27T11:41:07Z","close_reason":"internal/supervisor shipped: Start/Stop/Done/ExitErr/Pid, SIGTERM-\u003egrace-\u003eSIGKILL escalation, mandatory wait-and-reap. Test uses TestMain helper-process pattern; coverage 89.6% on the testable surface.","dependency_count":0,"dependent_count":3,"comment_count":0} {"id":"forgejo-mcp-broker-b2o","title":"Phase 2d: OAuth discovery endpoints (/.well-known/*)","description":"GET /.well-known/oauth-protected-resource and /.well-known/oauth-authorization-server. Issuer URLs MUST derive from cfg.PublicURL, never inbound headers (host-header attack defense per design.md §8).","acceptance_criteria":"Responses validate against RFC 8414/9728 shapes. Issuer URL sourced from config only. supported_scopes matches cfg.ForgejoOAuthScopes.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:13Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T15:08:19Z","started_at":"2026-04-27T15:06:24Z","closed_at":"2026-04-27T15:08:19Z","close_reason":"Discovery endpoints shipped: /.well-known/oauth-authorization-server (RFC 8414) and /.well-known/oauth-protected-resource (RFC 9728). All URLs derived from cfg.Issuer; explicit Host-header-spoofing test verifies attacker-supplied hosts don't leak into metadata.","dependencies":[{"issue_id":"forgejo-mcp-broker-b2o","depends_on_id":"forgejo-mcp-broker-pur","type":"blocks","created_at":"2026-04-24T17:45:25Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-b9i","title":"Phase 2b: internal/forgejo OAuth client","description":"Broker-side OAuth client for upstream Forgejo: authorize URL builder, code-to-token exchange, refresh_token grant, userinfo fetch, revoke. Used by AS callback and refresh machinery. Stateless; caller owns persistence.","acceptance_criteria":"Unit tests with httptest.Server fake Forgejo cover each grant plus error paths (wrong code, expired refresh, revoked token). No state persisted in this package.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T15:45:12Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-27T11:31:27Z","started_at":"2026-04-27T11:29:17Z","closed_at":"2026-04-27T11:31:27Z","close_reason":"internal/forgejo shipped: AuthorizeURL, ExchangeCode, Refresh, FetchUserInfo. Structured *forgejo.Error for OAuth failures (errors.As-friendly). 95.1% coverage. Stateless — caller owns persistence. Revocation deferred since upstream Forgejo lacks the endpoint.","dependency_count":0,"dependent_count":1,"comment_count":0} diff --git a/docs/design.md b/docs/design.md index 2dbd560..402f287 100644 --- a/docs/design.md +++ b/docs/design.md @@ -261,6 +261,8 @@ The broker itself terminates plain HTTP; Caddy handles TLS with Let's Encrypt. ## 8. Security +### 8.1 Posture + - **Public-URL authority.** Never derive issuer URLs from inbound headers — always from config. Publishing the wrong issuer allows an attacker to redirect flows to endpoints they control. - **PKCE required.** Reject authorize requests without `code_challenge`. Only `S256` method supported. - **Token storage.** Broker access/refresh tokens stored as SHA-256 hashes. Forgejo tokens stored cleartext (they must be usable for subprocess spawning); file permissions and optional encrypted volume mitigate at-rest risk. @@ -269,6 +271,30 @@ The broker itself terminates plain HTTP; Caddy handles TLS with Let's Encrypt. - **Audit log.** Structured log line per: client registration, authorize start, authorize callback success/failure, token issuance, token revocation, session spawn, session reap, child crash. Include `client_id`, `forgejo_username`, and session id. Do **not** log tokens. - **Dependencies.** Keep the dependency tree small and pinned. Review before adding any new dep. +### 8.2 Phase-2 attack-path review + +Performed at the end of phase 2 (issue `-wgo`). Each row is an attack class checked against the implementation; "covered by" points at the test that exercises the defence (or notes that it's untested). + +| # | Attack class | Defence | Covered by | +|---|---|---|---| +| 1 | Open redirect via crafted `redirect_uri` | Exact-match against the URIs registered with the client; bare equality, no prefix or pattern matching. | `TestAuthorize_MismatchedRedirectURI`, `TestToken_AuthCode_WrongRedirectURI` | +| 2 | Auth-code replay (use the same code twice) | Atomic `UPDATE auth_codes SET used_at=? WHERE used_at IS NULL` with rows-affected check; second use sees 0 affected rows and returns `invalid_grant`. | `TestToken_AuthCode_Reuse` | +| 3 | Auth-code substitution / mix-up across flows | PKCE S256: a code without the matching verifier can't be exchanged. `code_verifier` is required; `verifyPKCE` uses `subtle.ConstantTimeCompare`. | `TestToken_AuthCode_PKCEFails` | +| 4 | PKCE downgrade (request `plain` or omit) | `code_challenge_method` must equal `"S256"`; missing challenge or non-S256 method redirects to client with `error=invalid_request`. | `TestAuthorize_MissingPKCE_RedirectsWithError`, `TestAuthorize_NonS256_RedirectsWithError` | +| 5 | Issuer-URL spoofing via `Host` header | Discovery documents and redirect URIs derived from `cfg.Issuer`, never from the request. | `TestDiscovery_IssuerIgnoresHostHeader` | +| 6 | Refresh-token replay (concurrent or sequential reuse) | Atomic `UPDATE refresh_tokens SET revoked_at=? WHERE revoked_at IS NULL` with rows-affected check before minting a new pair — same single-shot pattern as the auth-code grant. **Fix landed in this review**; previous code read+validated, then unconditionally revoked, racing two simultaneous refreshes. | `TestToken_Refresh_ConcurrentReplay_OnlyOneSucceeds`, `TestToken_Refresh_RevokedToken` | +| 7 | Refresh expiry / revocation bypass | Bearer middleware checks `expires_at` and `revoked_at` on every request. Token rotation revokes old refresh on issue. | `TestRequireBearer_Expired/Revoked`, `TestToken_Refresh_RevokedToken`, `TestToken_Refresh_Expired` | +| 8 | Token leakage in audit logs | Handler logs use `slog.String("err", err.Error())` and structured fields (client_id, username, sid) — never the raw access/refresh token. Stored tokens are hashed. | Manual review; potential future test using a captured slog handler. | +| 9 | Session fixation via attacker-supplied sid | Sids are minted server-side via `crypto/rand`; the registry never accepts a client-supplied sid. | `TestServe_NewSession_MintsSidAndDispatches` | +| 10 | Sid hijack (legitimate sid reused under a different bearer) | Sids are bound to the bearer that minted them (`BrokerTokenHash`); cross-token access returns 403, not 410, so probing is distinguishable from "unknown sid". | `TestServe_TokenMismatch_403` | +| 11 | `redirect_uri` accepting code-execution schemes (`javascript:`, `data:`) | `validateRedirectURI` rejects these explicitly. Non-`https` schemes must be either loopback `http://` or a reverse-DNS private-use scheme per RFC 8252 §7.1. **Tightened in this review.** | `TestValidateRedirectURI` | +| 12 | DCR-driven storage flooding | None at the broker level yet — relies on Caddy rate-limit. Tracked as backlog item `-ttl`. | Not covered. | +| 13 | Missing PKCE verifier on `/token` | `code_verifier` required; missing → `invalid_request`. | `TestToken_AuthCode_MissingFields` | +| 14 | Authorization without `state` | The broker preserves and echoes the client's `state` on every redirect (success + error paths) so the client can run its own CSRF check. | `TestAuthorize_RedirectsToForgejo`, `TestAuthorize_MissingPKCE_RedirectsWithError` | +| 15 | Bearer-header parsing tricks (no scheme, empty token) | Middleware rejects missing header, wrong scheme, and empty token after `Bearer `. | `TestRequireBearer_NoHeader/WrongScheme/EmptyToken_401` | + +Items deferred to follow-up issues: rate limits (`-ttl`), `--token-fd` to avoid `/proc//environ` exposure (`-1n2`), AES-GCM at-rest encryption of Forgejo tokens (`-sd4`), structured-log assertion for the no-token-leakage check (no issue yet — file when wiring real audit logging). + ## 9. Scaling notes Single-instance design: diff --git a/internal/oauth/oauth.go b/internal/oauth/oauth.go index bf20574..e30c228 100644 --- a/internal/oauth/oauth.go +++ b/internal/oauth/oauth.go @@ -367,13 +367,37 @@ func validateRedirectURI(raw string) error { if err != nil { return fmt.Errorf("redirect_uri %q: %w", raw, err) } - if u.Scheme == "" { - return fmt.Errorf("redirect_uri %q: missing scheme", raw) + // RFC 6749 §3.1.2.1 requires absolute URIs. We further restrict to + // schemes we trust: + // - https: the production case + // - http: only for loopback hosts (local development) + // - private-use schemes per RFC 8252 §7.1 (e.g. claude://, com.foo://) + // Pseudo-schemes that allow code execution (javascript:, data:) are + // rejected to keep a future naive client from rendering an attacker- + // supplied URI as content. + scheme := u.Scheme + switch { + case scheme == "https": + return nil + case scheme == "http": + host := u.Hostname() + if host == "localhost" || host == "127.0.0.1" || host == "::1" { + return nil + } + return fmt.Errorf("redirect_uri %q: http only allowed for loopback hosts", raw) + case scheme == "javascript" || scheme == "data" || scheme == "": + return fmt.Errorf("redirect_uri %q: scheme %q is not allowed", raw, scheme) + default: + // Anything else: must be a private-use URI scheme that contains a + // dot (e.g. com.example.app:/) per RFC 8252 §7.1. Single-word + // schemes like "javascript" are caught above; this keeps the door + // open for legitimate mobile/desktop OAuth flows without a + // hardcoded allowlist. + if !strings.Contains(scheme, ".") { + return fmt.Errorf("redirect_uri %q: non-https scheme %q must be a reverse-DNS private-use scheme", raw, scheme) + } + return nil } - // RFC 6749 §3.1.2.1 requires absolute URIs; we additionally require - // http/https or claude.ai's documented custom scheme. Accept anything - // non-empty for now; tighten later if needed. - return nil } // ============================================================================ @@ -786,11 +810,26 @@ func (s *Server) tokenRefreshGrant(w http.ResponseWriter, r *http.Request) { return } - // Mint a new access token. Refresh-token rotation: also issue a new - // refresh token and revoke the old one. + // Atomically revoke the old refresh token. Two concurrent refresh + // requests with the same token would otherwise both pass the read + // above and each mint a fresh pair — quota duplication and a hint + // to a stolen-refresh attacker that the legitimate user is also + // active. Same single-shot pattern as the auth-code grant. + now := s.now().Unix() + res, err := s.store.DB().ExecContext(r.Context(), + `UPDATE refresh_tokens SET revoked_at = ? WHERE token_hash = ? AND revoked_at IS NULL`, + now, rtHash) + if err != nil { + writeOAuthError(w, http.StatusInternalServerError, "server_error", "") + return + } + if n, _ := res.RowsAffected(); n != 1 { + writeOAuthError(w, http.StatusBadRequest, "invalid_grant", "refresh token already used") + return + } + newAccess := secureToken(32) newRefresh := secureToken(32) - now := s.now().Unix() tx, err := s.store.DB().BeginTx(r.Context(), nil) if err != nil { @@ -820,18 +859,12 @@ func (s *Server) tokenRefreshGrant(w http.ResponseWriter, r *http.Request) { writeOAuthError(w, http.StatusInternalServerError, "server_error", "") return } - // Revoke the old refresh token (rotation per RFC 6749 §10.4). - if _, err := tx.ExecContext(r.Context(), - `UPDATE refresh_tokens SET revoked_at = ? WHERE token_hash = ?`, - now, rtHash); err != nil { - _ = tx.Rollback() - writeOAuthError(w, http.StatusInternalServerError, "server_error", "") - return - } + // Old refresh token already revoked above (atomic single-shot). if err := tx.Commit(); err != nil { writeOAuthError(w, http.StatusInternalServerError, "server_error", "") return } + _ = oldAccessHash // retained for potential future "revoke old access on refresh" tightening writeJSON(w, http.StatusOK, tokenResponse{ AccessToken: newAccess, diff --git a/internal/oauth/oauth_internal_test.go b/internal/oauth/oauth_internal_test.go index 2bd3055..d2cdee7 100644 --- a/internal/oauth/oauth_internal_test.go +++ b/internal/oauth/oauth_internal_test.go @@ -88,8 +88,14 @@ func TestValidateRedirectURI(t *testing.T) { ok bool }{ {"https", "https://app.example.com/cb", true}, - {"http_loopback", "http://localhost:1234/cb", true}, - {"custom_scheme", "claude://oauth/cb", true}, + {"http_loopback_localhost", "http://localhost:1234/cb", true}, + {"http_loopback_v4", "http://127.0.0.1/cb", true}, + {"http_loopback_v6", "http://[::1]/cb", true}, + {"http_non_loopback", "http://app.example.com/cb", false}, + {"reverse_dns_scheme", "com.example.claudeapp:/cb", true}, + {"single_word_scheme", "claude://oauth/cb", false}, + {"javascript_scheme", "javascript:alert(1)", false}, + {"data_scheme", "data:text/html,

hi

", false}, {"missing_scheme", "app.example.com/cb", false}, {"unparseable", "://no-scheme", false}, } diff --git a/internal/oauth/oauth_test.go b/internal/oauth/oauth_test.go index c2f0858..156a2e8 100644 --- a/internal/oauth/oauth_test.go +++ b/internal/oauth/oauth_test.go @@ -11,6 +11,7 @@ import ( "net/url" "path/filepath" "strings" + "sync" "sync/atomic" "testing" "time" @@ -861,6 +862,59 @@ func TestToken_Refresh_WrongClientID(t *testing.T) { } } +func TestToken_Refresh_ConcurrentReplay_OnlyOneSucceeds(t *testing.T) { + // Two goroutines race the same refresh token. Without an atomic + // single-shot UPDATE, both would mint a new pair. With it, exactly + // one succeeds. + fx := newFixture(t) + cid := fx.registerClient("https://app.example.com/cb") + tok := runFullFlow(t, fx, "https://app.example.com/cb", cid, "verifier-rfrace") + + form := url.Values{ + "grant_type": {"refresh_token"}, + "refresh_token": {tok.RefreshToken}, + "client_id": {cid}, + } + + type result struct { + status int + body string + } + results := make(chan result, 2) + var wg sync.WaitGroup + for i := 0; i < 2; i++ { + wg.Add(1) + go func() { + defer wg.Done() + resp, err := http.PostForm(fx.httpServer.URL+"/oauth/token", form) + if err != nil { + results <- result{status: 0, body: err.Error()} + return + } + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + results <- result{status: resp.StatusCode, body: string(body)} + }() + } + wg.Wait() + close(results) + + var ok, failed int + for r := range results { + switch r.status { + case http.StatusOK: + ok++ + case http.StatusBadRequest: + failed++ + default: + t.Errorf("unexpected status %d: %s", r.status, r.body) + } + } + if ok != 1 || failed != 1 { + t.Errorf("concurrent refresh outcome = (ok=%d, failed=%d), want (1, 1)", ok, failed) + } +} + func TestToken_Refresh_RevokedToken(t *testing.T) { fx := newFixture(t) cid := fx.registerClient("https://app.example.com/cb")