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 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>
