summercms/.planning/phases/08-oauth2-1-authorization-server/08-SECURITY-REVIEW.md

---
phase: 08
slug: oauth2-1-authorization-server
status: verified
threats_total: 11
threats_closed: 11
threats_open: 0
accepted_risks: 0
asvs_level: 1
created: 2026-09-24
verified: 2026-09-24
reviewer: gsd-executor (08-10 Task 2, self-performed -- see Reviewer Note)
---

# Phase 8 — Security Review

> Direct standard-library OAuth 2.1 authorization server (DCR, S256 PKCE,
> authorization-code and rotating-refresh grants, connected-app revocation,
> operator client command, MCP `/me` bootstrap). Every T-08 threat locked in
> 08-10-PLAN.md's threat model is mapped below to executed, named Go test
> evidence. Unmapped IDs would be a review gap, not an accepted risk; none
> exist.

**Date:** 2026-09-24
**Scope:** Plans 08-01 through 08-10 (implementation, coverage, and this
plan's own gap-closure Task 1 additions), all of `wristband` (summercms.go)
and the OAuth-touching surface of `fonoteka.go` (`plugins/golem15/fonoteka`,
its `classes/auth`, `console`, `updates`, `controllers/api` subpackages, and
`parity`).
**Repos grepped:** `summercms.go` and `fonoteka.go` (excluding `.planning/`
and `vendor/`).

## Reviewer Note

08-CONTEXT.md D-04 and 08-10-PLAN.md Task 2 call for an independent
`gsd-security-auditor` agent pass. This executor's available tool set for
this run does not include a Task/Agent spawning tool, so per the plan's own
fallback instruction ("if you cannot spawn one, perform the review yourself
... and say so explicitly in the review's frontmatter and in your checkpoint
return") this review was performed directly by the 08-10 executor: every
threat below is closed with source citations and named tests re-executed
during this review (not merely inherited unverified from earlier plans'
own claims), and one new HIGH-relevant gap (T-08-SURFACE-adjacent: the
generic manual-token-delete route did not cascade-revoke an OAuth refresh
lineage) was found and fixed as part of this same review pass (see
`fonoteka.go` commit `fix(08-10): cascade OAuth refresh-token lineage revoke
through the manual token-delete route`). The 08-10 checkpoint return states
this plainly so the human approver can weigh it accordingly.

---

## Verdict Summary

The register contains **11 total threats: 11 closed, 0 open, 0 accepted
risks**. This verdict follows this review's own 2026-09-24 gate run: `go
vet ./...` and `go test ./...` (both repos, all workspace modules) passed,
`go test -race ./...` passed on the touched `plugins/golem15/fonoteka`
package, and every test cited below was re-confirmed to exist and pass by
name (not assumed from prior plans' summaries).

---

## Trust Boundaries

| Boundary | Description | Data Crossing |
|----------|-------------|----------------|
| untrusted connector → `/oauth/mcp/authorize` | query-only PKCE/scope/redirect parameters from an unauthenticated caller | `client_id`, `redirect_uri`, `code_challenge`, `scope`, `resource`, `state` |
| untrusted connector → `/oauth/mcp/token` | client authentication plus code/refresh secrets, form or Basic | `client_id`/`client_secret`, `code`, `code_verifier`, `refresh_token` |
| untrusted connector → `/oauth/mcp/register` | self-declared RFC 7591 client metadata, bounded to 64 KiB before decode | `redirect_uris`, `client_name`, `token_endpoint_auth_method` |
| JWT-authenticated user → consent/connected-apps | pending-request handle and submitted scopes; collection ids are never read from the caller | `request_id`, `scopes[]` (never `collection_ids` from the client) |
| wristband ↔ app store (`OAuthStore`) | transactional code/refresh/client row access with `FOR UPDATE` locks | code/refresh/client rows, row locks |
| app store → `ApiTokenManager` (`AccessTokenIssuer`) | mints the actual bearer secret, stamps `oauth_client_id` | minted `inv_` secret, scopes, collection ids |
| manual token-delete route → OAuth refresh lineage | a generic non-OAuth-aware route must still fully revoke an OAuth-issued token's lineage | `ApiToken.ID`, linked `OAuthRefreshToken` row |
| gate script working directory → stdout/stderr | scripted MCP client and shell stage output must never carry a live secret past cleanup | redacted log lines, temp state files |

---

## Threat Register

| Threat ID | Category | Component | Disposition | Proof |
|-----------|----------|-----------|--------------|-------|
| T-08-PKCE | Spoofing/Elevation | authorize/exchange | mitigate | `wristband/authorize_test.go:TestPKCEChallengeMethodMustBeS256`; `wristband/authorize_test.go:TestPKCEChallengeLengthBounds`; `wristband/token_test.go:TestTokenWrongVerifierIsInvalidGrant`; `wristband/phase08_coverage_test.go:TestTokenPlainPKCEIsRejectedEvenWhenVerifierEqualsChallenge`; `plugins/golem15/fonoteka/classes/auth/oauth_token_issuer_test.go:TestOAuthCodeExchangeWrongVerifierIsInvalidGrantAndMintsNothing` |
| T-08-CODE-REPLAY | Spoofing | code transaction | mitigate | `wristband/token_test.go:TestTokenCodeSequentialReplayIsInvalidGrantSecondTime`; `wristband/token_test.go:TestTokenCodeConcurrentReplayHasExactlyOneWinner`; `plugins/golem15/fonoteka/classes/auth/oauth_token_issuer_test.go:TestOAuthCodeExchangeConcurrentReplayHasExactlyOneWinner` (real Postgres `FOR UPDATE`, `oauth_store.go:132`) |
| T-08-REFRESH-REPLAY | Spoofing/Elevation | refresh transaction | mitigate | `wristband/token_test.go:TestRefreshConcurrentReplayHasExactlyOneWinner`; `plugins/golem15/fonoteka/classes/auth/oauth_store_test.go:TestOAuthRefreshConcurrentReplayHasExactlyOneWinner`; `plugins/golem15/fonoteka/classes/auth/oauth_store_test.go:TestOAuthRefreshReplayRevokesLineageAndBothAccessTokens`; `plugins/golem15/fonoteka/phase08_coverage_test.go:TestOAuthRevocationRotatedPredecessorIsDeadAfterConnectedAppRevoke` |
| T-08-OPEN-REDIRECT | Spoofing/Disclosure | redirect construction | mitigate | `wristband/authorize_test.go:TestAuthorizeUnknownClientReturnsLocal400NoLocation`; `TestAuthorizeUnregisteredRedirectURIReturnsLocal400NoLocation`; `TestAuthorizeTrailingSlashMismatchIsUnregistered`; `TestOrderedRedirectQueryEncodingIsRFC3986`; `TestOrderedRedirectAppendsToExistingQuery` |
| T-08-SECRET-TIMING | Information Disclosure | crypto/client auth | mitigate | source `wristband/crypto.go:33` `constantEqual` uses `crypto/subtle.ConstantTimeCompare`, called from `wristband/token.go:197` (client secret) and `:210` (PKCE `s256Challenge`); `wristband/token_test.go:TestTokenConfidentialClientWrongSecretIsInvalidClient` |
| T-08-SCOPE-CEILING | Elevation | authorize/consent/refresh | mitigate | `wristband/authorize_test.go:TestAuthorizeCeilingTruncatesAiWithoutError`; `plugins/golem15/fonoteka/oauth_connect_test.go:TestOAuthConsentScopeCeilingViaShow`; `plugins/golem15/fonoteka/phase08_coverage_test.go:TestOAuthConsentIntersectionGrantsOnlyOriginallyRequestedScopes`; `plugins/golem15/fonoteka/oauth_connect_test.go:TestOAuthConsentEmptySubmittedScopeIntersectionIs422` |
| T-08-CROSS-USER | Elevation | consent/connected apps | mitigate | `wristband/consent_test.go:TestPendingRequestForeignOwnerIsNotFound`; `plugins/golem15/fonoteka/oauth_connect_test.go:TestOAuthConsentCrossUserIsNotFound`; `plugins/golem15/fonoteka/oauth_lifecycle_test.go:TestConnectedAppsRevokeForeignAndMissingAndManualShareExact404` |
| T-08-REQUEST-LEAK | Information Disclosure | logs/fixtures/output | mitigate | `plugins/golem15/fonoteka/phase08_coverage_test.go:TestOAuthConsentShowExactKeySet` (positive allow-list, no `collection_id(s)`/`collections` key); `scripts/check-phase8.sh`'s `redact_phase8` helper (7 call sites) and `stage_secret_scan`; grep of `wristband` and the OAuth-touching `fonoteka.go` packages for `fmt.Print*`/`log.*`/`slog.*` returns no matches |
| T-08-DCR-FLOOD | Denial of Service | register | mitigate | `wristband/registration_test.go:TestRegisterOversizedBody` (64 KiB, D-21); `TestRegisterCapReached` (200-client cap); `TestRegisterSweepsStaleUnconsentedButKeepsArtisanClients`; source `plugin.go`'s `fonoteka-oauth-register` bucket (30/min per IP) applied via `throttle:fonoteka-oauth-register`, confirmed mounted by `plugins/golem15/fonoteka/oauth_registration_test.go:TestOAuthRawRegistrationSurface` |
| T-08-SURFACE | Elevation | route/MCP boundary | mitigate | `plugins/golem15/fonoteka/routes_isolation_test.go:TestFullRouteTableAuthGroupMutualExclusivity`; `plugins/golem15/fonoteka/phase08_coverage_test.go:TestOAuthTokenSurfaceIsolationCoverage` (30 named subtests); `plugins/golem15/fonoteka/oauth_metadata_test.go:TestOAuthMetadataRouteIsolation`; the manual-token-delete cascade gap this review found is now closed (`controllers/api/token_api_controller.go` `Destroy`, GREEN evidence `plugins/golem15/fonoteka/phase08_coverage_test.go:TestOAuthRevocationManualTokenRouteKillsRefreshChain`) |
| T-08-SC | Tampering | package supply chain | mitigate | `git log --oneline -- go.mod go.sum` in both `summercms.go` and `fonoteka.go` (root and `plugins/golem15/fonoteka`) shows no commit since Phase 5/7 respectively touched dependency files; Phase 8 adds zero new packages (D-01: direct standard-library port, no zitadel/oidc) |

*Status: 11 closed / 0 open. All dispositions are `mitigate`; no accept rows this phase.*

---

## Findings by Threat

### T-08-PKCE — PKCE bypass

- **Source:** `wristband/authorize.go` (challenge method/length validation at
  authorize time), `wristband/token.go:203-210` `verifyPkce` (method must be
  exactly `S256`, comparison via `constantEqual`).
- **Test evidence:** `TestPKCEChallengeMethodMustBeS256` (authorize rejects
  `plain` as `invalid_request`, preserving `state`/`iss`); `TestPKCEChallengeLengthBounds`
  (43/128-char RFC 7636 bounds, `too-short`/`too-long`/`empty` subtests);
  `TestTokenWrongVerifierIsInvalidGrant` (exchange-time mismatch);
  `TestTokenPlainPKCEIsRejectedEvenWhenVerifierEqualsChallenge` (defense in
  depth: even a directly-seeded `plain`-method code row with
  verifier==challenge is rejected at exchange, proving `verifyPkce` never
  falls back to a naive string compare); `TestOAuthCodeExchangeWrongVerifierIsInvalidGrantAndMintsNothing`
  (real Postgres, zero tokens minted on mismatch).
- **Disposition:** closed / mitigate.

### T-08-CODE-REPLAY — authorization code replay

- **Source:** `plugins/golem15/fonoteka/classes/auth/oauth_store.go:132`
  `ByCodeHashForUpdate` takes a `FOR UPDATE` row lock before the exchange
  transaction reads/marks the code used.
- **Test evidence:** `TestTokenCodeSequentialReplayIsInvalidGrantSecondTime`
  (second exchange of the same code is `invalid_grant`);
  `TestTokenCodeConcurrentReplayHasExactlyOneWinner` (in-memory backend,
  two racing goroutines, exactly one 200); `TestOAuthCodeExchangeConcurrentReplayHasExactlyOneWinner`
  (real Postgres row lock, exactly one persisted access token across two
  concurrent exchanges).
- **Disposition:** closed / mitigate.

### T-08-REFRESH-REPLAY — refresh token replay and lineage kill

- **Source:** `oauth_store.go:243` `RevokeLineage` walks forward through
  `RotatedToID`, revoking each visited refresh row and its linked
  `ApiToken`, committed inside the same transaction as the replay detection
  before `invalid_grant` is returned (commit-then-report, not report-then-commit).
- **Test evidence:** `TestRefreshConcurrentReplayHasExactlyOneWinner`;
  `TestOAuthRefreshConcurrentReplayHasExactlyOneWinner` (real Postgres);
  `TestOAuthRefreshReplayRevokesLineageAndBothAccessTokens` (both the
  original and rotated access tokens die on replay of the spent
  predecessor); `TestOAuthRevocationRotatedPredecessorIsDeadAfterConnectedAppRevoke`
  (08-10 new: the reverse direction — revoking the *current* token after a
  rotation also kills the old, already-rotated predecessor the moment it is
  presented, because a rotated row is treated as spent).
- **Disposition:** closed / mitigate.

### T-08-OPEN-REDIRECT — authorize redirect construction

- **Source:** `wristband/authorize.go` validates `client_id` and
  `redirect_uri` (exact registered-URI match, no trailing-slash coercion)
  before any redirect is possible; unknown-client/unregistered-URI failures
  are local `text/plain` 400s with **no** `Location` header, never a
  redirect to an attacker-controlled or guessed URI. Later validation
  failures (bad scope, resource mismatch) redirect only to the
  already-validated registered URI, with RFC 3986 query encoding.
- **Test evidence:** `TestAuthorizeUnknownClientReturnsLocal400NoLocation`;
  `TestAuthorizeUnregisteredRedirectURIReturnsLocal400NoLocation`;
  `TestAuthorizeTrailingSlashMismatchIsUnregistered` (a trailing-slash
  variant of a registered URI is treated as unregistered, not silently
  accepted — closes a classic open-redirect bypass); `TestOrderedRedirectQueryEncodingIsRFC3986`;
  `TestOrderedRedirectAppendsToExistingQuery`.
- **Disposition:** closed / mitigate.

### T-08-SECRET-TIMING — client secret and PKCE timing side channel

- **Source:** `wristband/crypto.go:31-34` `constantEqual` wraps
  `crypto/subtle.ConstantTimeCompare`; every secret/challenge comparison in
  `wristband/token.go` (`:197` client secret, `:210` PKCE) goes through it,
  never a bare `==` or `strings.Compare`.
- **Grep:** `grep -n "==.*Secret\|Secret.*==" wristband/*.go` (excluding
  tests) finds no direct string-equality secret comparison; the only
  comparisons are `constantEqual` calls.
- **Test evidence:** `TestTokenConfidentialClientWrongSecretIsInvalidClient`
  (exact byte body, `WWW-Authenticate: Basic realm="OAuth"`,
  `Cache-Control: no-cache, private`).
- **Disposition:** closed / mitigate.

### T-08-SCOPE-CEILING — scope escalation via consent or ceiling bypass

- **Source:** `wristband/authorize.go` truncates requested scope to the
  client's `ScopeCeiling` at authorize time; `oauth_consent_controller.go`
  `ConsentStore` computes `granted := intersectStrings(submitted,
  clientRequested)` where `clientRequested` is already ceiling-truncated —
  submitting a scope the client never requested (even one within
  `MintableScopes`) cannot enter the grant.
- **Test evidence:** `TestAuthorizeCeilingTruncatesAiWithoutError` (authorize-time
  truncation); `TestOAuthConsentScopeCeilingViaShow` (ceiling reflected at
  consent show time); `TestOAuthConsentIntersectionGrantsOnlyOriginallyRequestedScopes`
  (08-10 new: submitting `["read","ai"]` against a pending request that
  only ever asked `read` grants `read` only — proves the intersection is
  against the *originally pending* scope set, not just `MintableScopes`);
  `TestOAuthConsentEmptySubmittedScopeIntersectionIs422` (empty grant is a
  422, never a 200 with a zero-scope token).
- **Disposition:** closed / mitigate.

### T-08-CROSS-USER — cross-user consent/connected-app access

- **Source:** Pending-request lookup and connected-app queries are always
  scoped `WHERE user_id = ?` (or an equivalent ownership predicate); a
  foreign or missing id is the identical 404, never a distinguishable 403.
- **Test evidence:** `TestPendingRequestForeignOwnerIsNotFound`;
  `TestOAuthConsentCrossUserIsNotFound`; `TestConnectedAppsRevokeForeignAndMissingAndManualShareExact404`
  (foreign, missing, and manual-token ids all produce the byte-identical
  `{"error":"Token not found"}` 404).
- **Disposition:** closed / mitigate.

### T-08-REQUEST-LEAK — request_id/secret leakage in logs, fixtures, or output

- **Source:** No `fmt.Print*`/`log.*`/`slog.*` call anywhere in `wristband`
  or the OAuth-touching `fonoteka.go` packages references a request_id,
  code, secret, or verifier (confirmed by direct grep, zero matches).
  `ConsentShow`'s response is a positive field allow-list
  (`client_name`/`redirect_host`/`scopes_requested`/`collection_name`/`expires_at`),
  never a serialized model. `scripts/check-phase8.sh`'s `redact_phase8`
  helper strips `client_secret=`, `code_verifier=`, `refresh_token=`,
  `access_token`, `Authorization: Bearer/Basic`, and `inv_*` patterns before
  any stage's diagnostic output reaches stdout/stderr (7 call sites across
  the gate script), and `stage_secret_scan` additionally scans the gate's
  own working directory for credential-shaped strings after the run.
- **Test evidence:** `TestOAuthConsentShowExactKeySet` (08-10 new: asserts
  the exact 5-key set and the explicit absence of
  `collections`/`collection_id`/`collection_ids`).
- **Disposition:** closed / mitigate.

### T-08-DCR-FLOOD — dynamic client registration flooding

- **Source:** `wristband/register.go:67` bounds the request body to
  `Options.RegisterMaxBodyBytes` (64 KiB default, D-21) via
  `http.MaxBytesReader` before JSON decoding; `CreateWithCap` enforces the
  200-client cap and the unconsented-client sweep inside one transaction
  (T-08-DCR-FLOOD); `plugin.go`'s `fonoteka-oauth-register` bucket applies a
  30/minute per-IP throttle at the route layer.
- **Test evidence:** `TestRegisterOversizedBody` (exactly one byte past 64
  KiB is rejected with the endpoint's normal `invalid_client_metadata`
  response, not a generic error); `TestRegisterCapReached` (201st
  registration attempt is `Registration temporarily unavailable`);
  `TestRegisterSweepsStaleUnconsentedButKeepsArtisanClients` (stale
  DCR-origin rows are swept, artisan-issued rows with `RegistrationIP ==
  nil` never are); `TestOAuthRawRegistrationSurface` (confirms
  `throttle:fonoteka-oauth-register` is the register route's sole
  middleware in the real assembled table).
- **Disposition:** closed / mitigate.

### T-08-SURFACE — route/MCP boundary elevation

- **Source:** `surf.BuildRouter`'s real assembled route table is the source
  of truth (not a hand-built fixture) for every isolation assertion;
  `routes.go`'s `GroupRaw` mount for the four RFC endpoints refuses house
  middleware at build time (Phase 6 `T-06-11`).
- **Test evidence:** `TestFullRouteTableAuthGroupMutualExclusivity`
  (jwt/token groups never share middleware); `TestOAuthTokenSurfaceIsolationCoverage`
  (08-10 new: 30 named subtests, one per `TokenSurfaceIsolationTest.php`
  method — real assertions for the oauth/mcp raw surface, `/me/locale`, and
  the token surface's exact current route set; honestly-labeled
  deferred-scope assertions for route families Phase 8 does not port, see
  08-PHP-TEST-MAP.md's Deferred-scope notice); `TestOAuthMetadataRouteIsolation`
  (no `oauth` guard registered, closing the Phase 6 D-09/D-10 reservation).
- **Finding (this review, closed):** `controllers/api/token_api_controller.go`'s
  generic `Destroy` handler (mounted at `/_fonoteka/api/v1/tokens/{id}`)
  called `auth.RevokeToken`, which only stamps `revoked_at` on the
  `ApiToken` row. Deleting an OAuth-issued token through this route (rather
  than the dedicated `/oauth/connected-apps/{id}` route) left its linked
  `OAuthRefreshToken` row alive and rotatable — a real gap relative to PHP's
  single canonical revoke path (`security/OAuthRevocationTest.php::test_manual_token_route_on_oauth_token_kills_refresh_chain`).
  **Fixed** in this review by routing `Destroy` through the same
  `wristband.Server.Revoke` cascade `ConnectedAppsDestroy` already uses;
  for a manual (non-OAuth) token the lineage lookup finds nothing and the
  behavior is unchanged. GREEN evidence: `TestOAuthRevocationManualTokenRouteKillsRefreshChain`.
- **Disposition:** closed / mitigate.

### T-08-SC — OAuth package supply chain

- **Source/Rationale:** `git log --oneline -- go.mod go.sum` in
  `summercms.go` shows the last dependency-file change was Phase 7's bcrypt
  addition (`8fcaff7`), untouched since; the same check in `fonoteka.go`
  (root and `plugins/golem15/fonoteka`) shows the last change was Phase 3
  and Phase 5 respectively. Phase 8's entire OAuth surface (D-01: direct
  standard-library port — `crypto/rand`, `crypto/sha256`, `crypto/subtle`,
  `net/url`, `encoding/base64`) adds zero new third-party packages to
  either repository. No package-legitimacy audit is triggered because there
  is nothing new to audit.
- **Disposition:** closed / mitigate.

---

## Credential / bearer / secret logging grep

```
rg -n 'fmt\.Print|log\.(Print|Fatal)|slog\.' summercms.go/wristband \
   fonoteka.go/plugins/golem15/fonoteka/classes/auth \
   fonoteka.go/plugins/golem15/fonoteka/controllers/api \
   --glob '!*_test.go'
```

No matches. The only place a secret-shaped value is intentionally printed is
`fonoteka.go/plugins/golem15/fonoteka/console/oauth_client.go`'s one-time
`client_secret=` line on client creation (D-19's documented, non-recoverable
console output — matches PHP's `IssueOAuthClient` command exactly, and
`--list` never prints it, per `TestOAuthClientCommandListPrintsClientIDAndURIsNeverTheSecret`).

## Route-table isolation source check

```
grep -n "GroupRaw" fonoteka.go/plugins/golem15/fonoteka/routes.go
```

The four RFC endpoints (`/.well-known/oauth-authorization-server`,
`/oauth/mcp/authorize`, `/oauth/mcp/register`, `/oauth/mcp/token`) are the
only routes inside the `GroupRaw` block; `surf.BuildRouter` refuses house
middleware on a raw group at build time (Phase 6 `T-06-11`,
`surf/routetable_test.go:TestRawGroupHouseMiddlewareRefusedAtBuild`).

---

## Post-Review Verification Gates

Gate run performed as part of this review (2026-09-24), re-executed after
the `token_api_controller.go` fix:

- `summercms.go`: `go vet ./...` — pass; `go test ./...` — pass (all 20
  packages, including `wristband`).
- `fonoteka.go` (root workspace module): `go vet ./...` — pass; `go test
  ./...` — pass.
- `fonoteka.go/plugins/golem15/fonoteka`: `go vet ./...` — pass; `go test
  ./...` — pass (all 9 packages); `go test . -race` — pass.
- `fonoteka.go/plugins/golem15/user`: `go vet ./...` — pass; `go test
  ./...` — pass.
- Evidence assertion: every `file:TestName` cited in the Threat Register
  above was individually re-run (`go test -run '^<Name>$'`) and confirmed
  passing during this review, not taken on faith from prior plans'
  summaries.

Full two-repository `go test -race ./...` and the complete
`scripts/check-phase8.sh` gate (parity/corpus, secret scan, UI harness, real
unchanged fonoteka-mcp lifecycle) are 08-10 Task 3's sole execution, per
08-CONTEXT.md D-14 and this phase's Validation Strategy.

---

## Accepted Risks Log

None this phase. All 11 threats close as `mitigate`.

---

## Security Audit Trail

| Audit Date | Threats Total | Closed | Open | Run By |
|------------|---------------|--------|------|--------|
| 2026-09-24 | 11 | 11 | 0 | gsd-executor (08-10 Task 2, self-performed per Reviewer Note; found and fixed one real gap — see T-08-SURFACE finding) |