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

phase, slug, status, threats_total, threats_closed, threats_open, accepted_risks, asvs_level, created, verified, reviewer

phase	slug	status	threats_total	threats_closed	threats_open	accepted_risks	asvs_level	created	verified	reviewer
08	oauth2-1-authorization-server	verified	11	11	0	0	1	2026-09-24	2026-09-24	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)