olemd/forgejo-mcp-broker
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

sec(oauth): phase-2 attack-path review (forgejo-mcp-broker-wgo)

Browse source 
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>
This commit is contained in:
Ole-Morten Duesund 2026-04-27 17:37:00 +02:00
commit 8369ec2cc7
5 changed files with 140 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

26
docs/design.md
View file

@ -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/<pid>/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: