26 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 08-oauth2-1-authorization-server | 2026-09-23T23:13:14Z | standard | 62 |
|
|
issues_found |
Phase 08: Code Review Report
Reviewed: 2026-09-23T23:13:14Z Depth: standard Files Reviewed: 62 Status: issues_found
Summary
Reviewed the OAuth 2.1 authorization-server port: every non-test Go source in wristband/ and the fonoteka plugin's OAuth surface (store adapter, controllers, plugin wiring, routes, console command, migration), plus the bonfire/tide framework changes, the gate scripts, and a skim of tests and fixtures. Each protocol path was compared line by line against the PHP originals in /media/nvme/dev/golem15/fonoteka/plugins/golem15/fonoteka (OAuthCodeManager.php, OAuthTokenController.php, OAuthAuthorizeController.php, OAuthConsentController.php, ConnectedAppController.php, OAuthRegisterController.php, IssueOAuthClient.php, OAuthClient.php), and the 08-SECURITY-REVIEW.md claims were checked against code.
The core protocol mechanics hold up: PKCE S256 with constant-time compare, row-locked single-use code exchange, row-locked refresh rotation with commit-then-report lineage kill, exact-match redirect validation before any redirect, RFC 3986 ordered query encoding, owner-scoped consent and connected-app lookups, advisory-locked DCR cap, and bounded registration bodies all match the PHP behaviour and the threat register. No finding rises to Critical.
The real defects cluster in three places: (1) the new bonfire Repeatable flag is wired as a Cobra StringSlice, which CSV-splits values on commas, so the operator command cannot register a redirect URI containing a comma (verified empirically with pflag); (2) several silent parity divergences from the PHP byte contract that the fixtures do not cover (scope ordering in consent/token responses, empty-array scope ceiling semantics, display-time control-character stripping, validation-failure bodies on consent/deny); and (3) configuration/robustness gaps in plugin.go (two declared config keys are never read; an unset app.url produces an empty issuer with no fail-closed check). A cross-transaction check-then-act in consent issuance mirrors PHP but is trivially fixable here.
Warnings
WR-01: bonfire Repeatable flags use Cobra StringSlice, which splits every value on commas
File: bonfire/root.go:66-80, bonfire/command.go:81-90
Issue: Repeatable flags are registered with cmd.Flags().StringSlice/StringSliceP and read back with GetStringSlice. pflag's StringSlice is CSV-parsed: a single occurrence --redirect-uri=https://a.example/cb?x=1,2 yields ["https://a.example/cb?x=1", "2"] (verified by running pflag v1.0.10 directly). The command's only two consumers are redirect URIs (which may legally contain commas in query or path) and scopes. For fonoteka:oauth-client, a comma-bearing redirect URI can therefore never be registered: the split fragment 2 fails RejectRedirectURI with "Redirect URI is not a valid URL: 2". The doc comment on Flag.Repeatable promises "ordered, multi-occurrence" semantics without mentioning CSV splitting, and none of the bonfire tests (output_test.go:257-360) pass a comma-bearing value, so the split is undetected. Quoting rules of the CSV reader also apply (a value containing " is mangled).
Fix: Use the array variant, which never splits:
// bonfire/root.go
if flag.Shorthand != "" {
cmd.Flags().StringArrayP(flag.Name, flag.Shorthand, def, flag.Description)
} else {
cmd.Flags().StringArray(flag.Name, def, flag.Description)
}
// bonfire/command.go
vals, err := in.cmd.Flags().GetStringArray(name)
Add a bonfire test with --redirect-uri=https://a.example/cb?x=1,2 asserting a single element.
WR-02: authorize.go treats an empty (non-nil) scope ceiling as "nothing allowed"; PHP and the store contract say "no ceiling"
File: wristband/authorize.go:99, wristband/stores.go:29, ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store.go:333
Issue: stores.go:29 documents ScopeCeiling []string // nil/empty means no ceiling, and PHP OAuthClient::ceilingScopes() returns null for both NULL and [] (is_array($ceiling) && $ceiling !== [] ? ... : null). authorize.go:99 checks only client.ScopeCeiling != nil. lagoon.Jsonable.Get() returns the decoded slice as-is, so a scope_ceiling column holding [] decodes to a non-nil empty slice, the ceiling branch runs, dataScopes is empty for every request, and every /oauth/mcp/authorize call for that client redirects with invalid_scope "requested scope is outside this client's ceiling". Today the Go writers (clientRecordToModel with Valid: rec.ScopeCeiling != nil, the console command returning nil for no --scope) avoid storing [], so the failure is latent; but the contract comment, the PHP reference and the code disagree, and any future writer or manual SQL that stores [] locks the client out of authorization. Fail-closed, so not Critical.
Fix:
// wristband/authorize.go
if len(client.ScopeCeiling) > 0 {
and add an authorize test seeding ScopeCeiling: []string{} that expects the ceiling to be a no-op.
WR-03: display-time control-character stripping was removed, but two data sources never had capture-time stripping
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/oauth_consent_controller.go:270-280, ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/connected_app_controller.go:93-109, ../fonoteka.go/plugins/golem15/fonoteka/console/oauth_client.go:94,271-279
Issue: PHP OAuthConsentController::untrustedName and ConnectedAppController::serializeApp both apply preg_replace('/[\x00-\x1F\x7F]/u', '', ...) at display time. The Go port moved that filter to DCR capture time (wristband/register.go:105) and reduced untrustedName to a 120-rune truncation, on the stated assumption that "control characters were already stripped once at DCR capture time". That assumption is false for (a) operator-issued clients: console/oauth_client.go:94 stores oauthClientTruncateName(name, 255) without any control-character filter (wristband.stripControlChars is unexported and not reachable from the command), so a name containing ESC/CR/NUL from a shell argument is persisted verbatim; and (b) every client row created by the PHP application before the port, which never stripped at capture. For both, client_name on the consent screen and in GET /oauth/connected-apps now carries raw control bytes where PHP emitted a cleaned string: a byte-contract divergence on the exact field the phishing-surface defense targets.
Fix: Restore the display-time filter (cheap, idempotent) in untrustedName:
func untrustedName(name string) string {
cleaned := strings.Map(func(r rune) rune {
if r <= 0x1F || r == 0x7F { return -1 }
return r
}, name)
r := []rune(cleaned)
if len(r) > 120 { return string(r[:120]) }
return cleaned
}
and/or export wristband.StripControlChars and apply it in the console command next to IssueClientCredentials.
WR-04: access_token_ttl_seconds and refresh_token_ttl_seconds are declared in config.yaml but never read
File: ../fonoteka.go/plugins/golem15/fonoteka/plugin.go:95-114, ../fonoteka.go/plugins/golem15/fonoteka/config/config.yaml:7-8
Issue: config.yaml publishes oauth.access_token_ttl_seconds and oauth.refresh_token_ttl_seconds under golem15.fonoteka.oauth.*, and the D-03 comment in Boot says the config "overrides ... are layered on top". Boot reads dcr_client_cap, dcr_unconsented_sweep_seconds, register_max_body_bytes, resource, pending_request_ttl_seconds and code_ttl_seconds only. Options.AccessTokenTTL and Options.RefreshTokenTTL always stay at DefaultOptions(). An operator shortening the refresh lifetime (a security control) or the access lifetime via config or SUMMER_GOLEM15__FONOTEKA__OAUTH__REFRESH_TOKEN_TTL_SECONDS sees no effect and no error.
Fix:
if v := app.Config.Int("golem15.fonoteka.oauth.access_token_ttl_seconds"); v > 0 {
oauthOpts.AccessTokenTTL = time.Duration(v) * time.Second
}
if v := app.Config.Int("golem15.fonoteka.oauth.refresh_token_ttl_seconds"); v > 0 {
oauthOpts.RefreshTokenTTL = time.Duration(v) * time.Second
}
plus a boot test asserting both overrides reach Options (the existing plugin_boot_test.go pattern).
WR-05: an unset app.url silently yields an empty issuer and relative endpoint URLs
File: ../fonoteka.go/plugins/golem15/fonoteka/plugin.go:89-94, wristband/server.go:163-178
Issue: issuer is derived from app.Config.String("app.url") with no validation. With app.url unset (or app.Config == nil) the server boots normally, GET /.well-known/oauth-authorization-server advertises "issuer":"" and endpoints like "/oauth/mcp/authorize", every iss parameter in authorize/consent redirects is empty (RFC 9207 mix-up defense degraded to a no-op), and the /connect handoff Location becomes a relative /connect?request=.... server.go:20-28 documents Issuer as the one option the caller must set, but nothing enforces it. A misconfigured deployment fails at the first MCP client instead of at boot.
Fix: Fail closed in Boot:
if issuer == "" {
return errors.New("golem15.fonoteka: app.url must be set; the OAuth issuer cannot be empty")
}
(or have wristband.NewServer return an error when opts.Issuer == "").
WR-06: consent issuance/denial is a check-then-act across two transactions with unguarded UPDATEs
File: wristband/consent.go:74-101,106-122,130-167, ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store.go:149-167,180-183
Issue: IssueCode and DenyPending call lookupOwnedPending (its own WithinTx, plain Take, no FOR UPDATE) and then open a second WithinTx to MarkIssued/MarkUsed. MarkIssued is UPDATE ... WHERE id = ? with no request_id IS NOT NULL AND code_hash IS NULL AND used_at IS NULL guard; MarkUsed likewise. Two concurrent POSTs by the owner (double-click on Allow, or Allow racing Deny, or two tabs) both pass the lookup: the second MarkIssued overwrites code_hash/scopes/collection_ids of an already-issued code, both callers receive a redirect_to carrying a code but only the last-written one is redeemable; a Deny landing after an Allow stamps used_at on the issued code so the client's exchange then fails invalid_grant. PHP has the same race (no lock in pendingFor), so this is not a parity regression and the outcome is fail-closed, but the port already has the transactional seam (Tx) that makes it a one-line fix, and ConsentStore additionally runs the lookup three times per request (PendingRequest at :114, then IssueCode's own).
Fix: Move the lookup into the mutating transaction with a row lock and make the UPDATE conditional:
// consent.go IssueCode: single WithinTx: ByRequestIDForUpdate -> validate -> MarkIssued -> MarkConsented
// oauth_store.go MarkIssued:
res := t.db.WithContext(ctx).Model(&models.OAuthAuthCode{}).
Where("id = ? AND request_id IS NOT NULL AND code_hash IS NULL AND used_at IS NULL", id).
UpdateColumns(...)
if res.Error != nil { return res.Error }
if res.RowsAffected == 0 { return wristband.ErrPendingNotFound }
WR-07: consent scope ordering is canonicalised; PHP preserves request/submission order, which reaches the token scope string
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/oauth_consent_controller.go:69,123-124,216-245
Issue: PHP show() returns array_intersect($pending->scopes, MINTABLE_SCOPES), i.e. the pending row's scopes in the order the client sent them on /authorize (scope=write read → ["write","read"]), and store() computes array_intersect($data['scopes'], $clientRequested, MINTABLE), i.e. in the submitted order. Those granted scopes are persisted by issueCode and later serialised as the /oauth/mcp/token scope field (implode(' ', ...)) and as scopes in /oauth/connected-apps. The Go port replaces both with canonicalMintableIntersection / intersectStrings(submitted, clientRequested) which always emit read → write → ai order (citing 08-UI-SPEC.md). For any client that requests scopes in non-canonical order, or any consent submission in non-canonical order, scopes_requested, the token scope string and the connected-apps scopes array differ byte-for-byte from PHP. PHP also preserves duplicates (scope=read read), Go de-duplicates. The recorded fixtures only exercise canonical order, so the harness cannot see this. The phase rule is "do not improve response shapes during the port".
Fix: Port array_intersect semantics: iterate the first argument's order.
func intersectPreservingFirst(a, allowed []string) []string {
set := map[string]bool{}
for _, s := range allowed { set[s] = true }
out := make([]string, 0, len(a))
for _, s := range a { if set[s] { out = append(out, s) } }
return out
}
// show: scopes_requested = intersectPreservingFirst(view.ScopesRequested, auth.MintableScopes)
// store: granted = intersectPreservingFirst(submitted, clientRequested)
If the canonical order is a deliberate deviation, record it as a decision note and add a parity fixture with scope=write read.
WR-08: consent/deny validation-failure bodies diverge from PHP for malformed JSON and for deny
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/oauth_consent_controller.go:89-93,108-111,161-176
Issue: The port correctly reproduces (from a live recording) that a failed $request->validate() in OAuthConsentController::store crashes to Winter's HTML 500 page. Two adjacent cases do not follow the same PHP path: (a) ConsentStore returns the house opaque JSON 500 ({"error":true,"message":"Internal server error"}, wire.WriteOpaque500) when readJSON fails on a syntactically invalid body, but Laravel's Request::json() swallows a JSON syntax error into an empty bag, so PHP hits the same required failure and returns the Winter HTML page; (b) ConsentDeny uses the identical bare $request->validate(['request_id' => 'required|string']) in PHP, which crashes to the same Winter page on a missing/invalid request_id, yet the Go ConsentDeny (:173-176) returns the house 422 {"error":"Validation failed","errors":...} JSON. No fixture records either case (the deny fixture only covers the 404), so this is inferred from source, but both are deterministic divergences on a route whose recorded contract is precisely "basic-shape failure crashes".
Fix: In ConsentStore, treat a readJSON error as an empty field set (fall through to the required failure and the Winter page). In ConsentDeny, replace writeValidation(w, errs) with writeOAuthConsentWinterErrorPage(w), or record a live PHP fixture for both cases and match whatever it shows.
Info
IN-01: DCR sweep/cap run after body validation; PHP runs them first and counts revoked clients
File: wristband/register.go:57-153, ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store.go:75-93
Issue: PHP order is isJson → sweep → cap check (OAuthClient::count(), including revoked rows) → parse redirect_uris → .... Go is isJson → decode → parse everything → sweep+cap. Observable differences: at cap, an invalid body gets the parse error in Go but "Registration temporarily unavailable" in PHP; an invalid registration attempt no longer sweeps stale clients; and the cap counts only unrevoked rows (D-03 explicitly chose "unrevoked", so that part is decided). Only the ordering is undocumented.
Fix: Either move the SweepUnconsented+count ahead of body parsing (keeping the create in the same transaction), or note the ordering change in the decision log.
IN-02: RevokeLineage walks forward only; PHP lineageIds walks predecessors too
File: ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store.go:243-277
Issue: PHP collects predecessors via where('rotated_to_id', $current->id) and successors via rotated_to_id, stamping revoked_at on all of them. Go follows RotatedToID forward only, so after a connected-app revoke of the current token, already-rotated predecessors keep revoked_at IS NULL. Presenting one later is still rejected (rotated ⇒ replay ⇒ RevokeLineage from there), and their access tokens were revoked at rotation time, so the API surface is unchanged; the difference is persisted audit state and the stores.go comment ("every row it was rotated to") documenting the narrower behaviour as intended.
Fix: Add the backward walk (WHERE rotated_to_id = ? loop) before the forward walk, or keep as-is and note the audit-state divergence.
IN-03: /oauth/mcp/token runs two DELETE sweeps before client authentication, on unindexed columns
File: wristband/token.go:96-105, ../fonoteka.go/plugins/golem15/fonoteka/updates/21_oauth_schema_correction.go:35-43
Issue: D-17 adds an expiry sweep PHP lacks. It runs for any request with a syntactically valid grant_type, before authenticateClient, so unauthenticated callers (30/min/IP) drive DELETE ... WHERE expires_at < now on both tables per request; the migration adds no index on expires_at, so each is a sequential scan. A sweep failure also turns an otherwise-valid grant into an opaque 500. Not a correctness bug; flagged because it couples an unauthenticated path to write load.
Fix: Either move the sweep after successful client authentication, or index expires_at on both tables in the migration, or run the sweep opportunistically (e.g. once per N seconds via s.now()).
IN-04: typed JSON decoding in Register changes PHP's error bodies for loosely-typed metadata
File: wristband/register.go:26-32,68-76
Issue: redirect_uris []string / client_name string etc. mean a body like {"redirect_uris":"https://x"}, {"redirect_uris":[1]} or {"client_name":123,...} fails Decode and returns invalid_client_metadata "Request must be application/json". PHP returns invalid_redirect_uri "redirect_uris is required", "redirect_uris must be an array of URI strings" and, for a numeric client_name, a 201 with "MCP client". D-21 documents collapsing malformed/oversized bodies to the endpoint-native error, which covers syntax errors, but not well-formed JSON with unexpected types.
Fix: Decode into map[string]any (or json.RawMessage fields) and port PHP's per-field type checks to keep the exact error bodies, or record the decision.
IN-05: serializeToken swallows the collections query error
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/token_api_controller.go:168
Issue: _ = gdb.Select("id", "name").Where("id IN ?", ids).Find(&rows).Error silently renders collections: [] on a DB error, so a transient failure produces a 200 with wrong data rather than a 500. Also lacks WithContext(r.Context()).
Fix: Return the error to the caller (serializeToken(...) (map[string]any, error)) and map it to writeOpaque500.
IN-06: MeToken discards its app parameter with _ = app
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/me_token_controller.go:22
Issue: The handler takes app *backpack.App only to keep the constructor shape uniform and immediately discards it. Harmless, but it is a dead parameter masked to silence vet.
Fix: Either drop the parameter (func MeToken() http.HandlerFunc) or use deps-style resolution if the handler is expected to need app later.
IN-07: Boot nil-checks app once and then dereferences it unconditionally
File: ../fonoteka.go/plugins/golem15/fonoteka/plugin.go:68,81,128
Issue: if app != nil && app.Events != nil at :68 suggests a nil app is tolerated, but :81 (app.Config) and :128 (app.Lookup) dereference it unconditionally and would panic. The guard is misleading.
Fix: Return an error early (if app == nil { return errors.New("fonoteka: nil app") }) and drop the partial guard.
IN-08: clientIP hand-parses RemoteAddr and ignores the app's trusted-proxy configuration
File: wristband/register.go:316-332
Issue: The host/port split is hand-rolled (LastIndex(":") with a ] heuristic) where net.SplitHostPort is exact, and it uses the raw RemoteAddr while the same plugin keys its throttles through surf.ClientIP(r, trusted). Behind the production reverse proxy every DCR row records the proxy's address. Only "non-nil" matters for the sweep today, so functionally harmless; PHP recorded $request->ip() (trusted-proxy aware).
Fix: host, _, err := net.SplitHostPort(r.RemoteAddr); consider an Options.ClientIP func(*http.Request) string seam so the app can pass surf.ClientIP.
IN-09: RevokeLineage has no cycle guard
File: ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store.go:246-276
Issue: The for loop follows rotated_to_id until nil. MarkRotated always points at a freshly created row, so a cycle cannot arise through the code, but a corrupted or hand-edited row would spin the transaction forever holding FOR UPDATE locks. PHP has the same unguarded loop.
Fix: Track visited ids (seen := map[uint]bool{}) and return an error on revisit.
IN-10: gate script nits: literal %q in an echo, and two different SUMMER_APP__KEY values across migrate and serve
File: scripts/check-phase8.sh:76,263-277
Issue: :76 is a bash echo, not printf, so the message prints a literal %q. :263-277 generate SUMMER_APP__KEY twice with independent /dev/urandom reads; the migrate step and the serve step therefore run under different app keys. Harmless for a disposable empty database today, but if any migration (e.g. 11_secrets_slice.go credential encryption) ever writes key-derived ciphertext, the served process could not read it.
Fix: Replace %q with '$stage'; hoist SUMMER_APP__KEY="$(head -c32 /dev/urandom | base64)" above both subshells.
IN-11: /oauth/mcp/authorize creates a pending row per unauthenticated hit with no throttle
File: ../fonoteka.go/plugins/golem15/fonoteka/routes.go:71-76, wristband/authorize.go:146-164
Issue: Any caller who knows a valid client_id and one of its registered redirect URIs (both are public knowledge for DCR clients) can insert one oauth_auth_codes row per request; register and token are throttled, authorize is not. PHP's routes.php is identical (no throttle on authorize), so this is parity, and the D-17 sweep at /token and /register bounds growth to the 600 s pending TTL. Recorded for the threat register: T-08-DCR-FLOOD covers only /register.
Fix: Optional: add a fonoteka-oauth-authorize bucket keyed by IP (the metadata route can stay middleware-free), and note it as a deliberate deviation if adopted.
Reviewed: 2026-09-23T23:13:14Z Reviewer: Claude (gsd-code-reviewer) Depth: standard