Structured review of every OAuth/auth handler against the standard
attack catalog. Findings table added to design.md §8.2.
Two real issues found and fixed:
- Refresh-token replay race: tokenRefreshGrant read the row, validated
it, then minted a new pair before unconditionally revoking the old
refresh. Two concurrent /token requests with the same refresh would
both pass validation and both mint a fresh pair — token-quota
duplication and a hint to a stolen-refresh attacker. Fixed with the
same atomic UPDATE rows-affected pattern already used for auth-code
single-use. New TestToken_Refresh_ConcurrentReplay_OnlyOneSucceeds
races two goroutines and verifies exactly one wins.
- Permissive redirect_uri schemes: validateRedirectURI accepted any
non-empty scheme, including javascript: and data:. Tightened to
require https, http for loopback only, or a reverse-DNS private-use
scheme per RFC 8252 §7.1. TestValidateRedirectURI updated to cover
each variant including the rejected javascript:/data: cases.
Items deferred to backlog (already filed):
- Rate limits on /oauth/register and /oauth/token (-ttl)
- --token-fd to close the /proc/<pid>/environ window (-1n2)
- AES-GCM at-rest encryption of Forgejo tokens (-sd4)
Closes forgejo-mcp-broker-wgo. Phase 2 complete.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds internal/session.Registry, the MCP session glue that maps
Mcp-Session-Id to a running forgejo-mcp child + bridge.
Lifecycle:
- First /mcp POST without Mcp-Session-Id: SpawnFunc creates a backend
(in production: supervisor.Start + bridge.New); registry mints a
192-bit hex session id, attaches it to the response header, and
dispatches the request to the new backend.
- Subsequent POSTs with the header dispatch to the existing backend.
- Unknown sids → 410 Gone (per MCP guidance, so clients re-initialise
instead of retrying forever).
- Sids are bound to the OAuth token that minted them: a different
bearer probing a stolen sid gets 403, distinct from "your token is
bad" (401) and "sid unknown" (410).
Cleanup:
- When backend.Done closes (child exited on its own — crash, OOM,
user-driven shutdown), a goroutine reaps the entry.
- Stop tears every session down on broker shutdown. The 30s idle
reaper and Forgejo token rotation come in 5c.
The Registry is decoupled from supervisor and bridge via SpawnFunc, so
tests don't need to fork real processes — they hand the registry a fake
that returns a controllable Backend. Also added oauth.ContextWithSession
so the session tests can inject an oauth.Session into request contexts
without standing up the full bearer-middleware chain.
Tests: 83.3% coverage. Cover spawn-on-initialize, sid reuse, unknown
sid, max-session cap with Retry-After, no-auth-context guard, sid
hijack defense (token mismatch → 403), Done-channel reaping, and
graceful Stop.
Closes forgejo-mcp-broker-t81.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds Authenticator.RequireBearer — http middleware that gates downstream
handlers on a valid broker access token.
Lookup path:
1. Read Authorization: Bearer <token> header.
2. SHA-256 the token, query access_tokens by token_hash.
3. Reject expired or revoked rows.
4. Build a Session (client_id, forgejo user info, upstream token,
scopes) and attach to r.Context() under a typed key.
Downstream handlers (the MCP endpoint shipping in 5a) read the
upstream Forgejo token via SessionFromContext to spawn forgejo-mcp
subprocesses scoped to the right user.
Failures emit 401 with an RFC 6750 §3 WWW-Authenticate header carrying
distinct error codes (invalid_request for missing/malformed headers,
invalid_token with reason=expired/revoked/unknown for token problems).
The body stays empty so a confused browser doesn't render auth errors;
all detail rides in the header where compliant clients look for it.
Tests: 90.9% on RequireBearer, 91.7% on lookupSession. Covers valid
token, missing/wrong-scheme/empty Authorization, unknown token,
expired token (clock-advanced past AccessTokenTTL), revoked token (via
the public /oauth/revoke endpoint).
Closes forgejo-mcp-broker-ytw.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds RFC 8414 (oauth-authorization-server) and RFC 9728 (oauth-
protected-resource) metadata documents.
Both URLs are derived from cfg.Issuer at construction time, never from
inbound request headers. Test TestDiscovery_IssuerIgnoresHostHeader
explicitly probes this — a malicious Host: evil.example.com value must
not leak into the published metadata. Defense against the OAuth
metadata-spoofing class starts at the discovery layer.
Capabilities published reflect the actual OAuth surface:
- response_types_supported = ["code"]
- grant_types_supported = ["authorization_code", "refresh_token"]
- code_challenge_methods_supported = ["S256"] (PKCE only, no plain)
- token_endpoint_auth_methods_supported = ["none"] (PKCE-only public clients)
Protected-resource metadata advertises /mcp as the resource; phase 5
will mount the gated MCP endpoint there.
Closes forgejo-mcp-broker-b2o.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements internal/oauth, the broker's OAuth 2.1 AS surface that
Claude.ai (and other MCP clients) talk to. User authentication is
delegated to upstream Forgejo via internal/forgejo.
Endpoints:
POST /oauth/register — RFC 7591 dynamic client registration
GET /oauth/authorize — RFC 6749 + 7636 PKCE (S256 only)
GET /oauth/callback — Forgejo redirects back here after consent
POST /oauth/token — authorization_code + refresh_token grants
POST /oauth/revoke — RFC 7009
Security model:
- PKCE required, S256 only — plain method rejected per OAuth 2.1
- Every broker-issued access/refresh token stored as hex(sha256(plain));
plaintext leaves the broker exactly once in the /token response body
- Refresh-token rotation: each refresh issues a new token pair and
revokes the old refresh (RFC 6749 §10.4)
- Auth-code single-use enforced atomically via UPDATE...WHERE used_at IS
NULL with rows-affected check, blocking the concurrent-replay race
- Issuer URL sourced from cfg.Issuer at construction time, never from
inbound headers — prevents host-header injection on /.well-known
metadata that ships in 2d
- redirect_uri must match a registered URI exactly (no prefix/wildcard)
Pending-authorization state (between /authorize and /callback) lives in
an in-memory sync.Map with a 1-minute reaper goroutine. A broker restart
drops them, forcing the user to re-authorize — acceptable trade-off
versus introducing a fifth table.
Tests: 81.0% coverage with ~20 cases across happy paths, every required-
field error, PKCE failure, code-replay, refresh expiry/revocation,
client-id and redirect-uri mismatches, Forgejo-side errors, and the
reaper logic itself (internal test).
Closes forgejo-mcp-broker-pur. The OAuth keystone is in place; phase 2c
unblocks discovery (2d) and security review (2e), and combined with the
existing supervisor + bridge it unblocks the session glue work in
phase 5.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
