golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(08): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-09-24 01:16:02 +02:00
parent 95e8ccde3d
commit 310a4cd7a8
1 changed files with 281 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

281
.planning/phases/08-oauth2-1-authorization-server/08-REVIEW.md Normal file
View File

@@ -0,0 +1,281 @@
---
phase: 08-oauth2-1-authorization-server
reviewed: 2026-09-23T23:13:14Z
depth: standard
files_reviewed: 62
files_reviewed_list:
- bonfire/command.go
- bonfire/output_test.go
- bonfire/root.go
- ../fonoteka.go/parity/fixtures/mcp/mcp-lifecycle.yaml
- ../fonoteka.go/parity/fixtures/routes/GET___fonoteka_api_v1_oauth_connected-apps_jwt.yaml
- ../fonoteka.go/parity/fixtures/routes/POST___fonoteka_api_v1_oauth_consent_jwt.yaml
- ../fonoteka.go/parity/manifest.yaml
- ../fonoteka.go/parity/migrate_test.go
- ../fonoteka.go/parity/oauth_audit_test.go
- ../fonoteka.go/parity/oauth_flow_test.go
- ../fonoteka.go/parity/parity_contract_test.go
- ../fonoteka.go/parity/parity_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/api_token_manager.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_store_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/oauth_token_issuer_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/phase08_coverage_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/config/config.yaml
- ../fonoteka.go/plugins/golem15/fonoteka/console/oauth_client.go
- ../fonoteka.go/plugins/golem15/fonoteka/console/oauth_client_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/console/phase08_coverage_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/connected_app_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/connected_app_controller_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/me_token_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/me_token_controller_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/oauth_consent_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/oauth_consent_controller_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/token_api_controller.go
- ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/winter_error_page.html
- ../fonoteka.go/plugins/golem15/fonoteka/models/oauth_auth_code.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/oauth_client.go
- ../fonoteka.go/plugins/golem15/fonoteka/oauth_authorize_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/oauth_connect_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/oauth_lifecycle_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/oauth_metadata_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/oauth_registration_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/oauth_tools_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/phase08_coverage_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/plugin.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes_isolation_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/updates/21_oauth_schema_correction.go
- ../fonoteka.go/plugins/golem15/fonoteka/updates/oauth_schema_correction_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/updates/phase08_coverage_test.go
- ../fonoteka.go/plugins/golem15/user/console_test.go
- scripts/check-phase8-mcp-client.mjs
- scripts/check-phase8-red.sh
- scripts/check-phase8.sh
- scripts/check-phase8-ui.mjs
- tide/normalize.go
- wristband/authorize.go
- wristband/authorize_test.go
- wristband/client_issue.go
- wristband/consent.go
- wristband/consent_test.go
- wristband/crypto.go
- wristband/phase08_coverage_test.go
- wristband/redirect_html.go
- wristband/register.go
- wristband/registration_test.go
- wristband/server.go
- wristband/server_test.go
- wristband/stores.go
- wristband/token.go
- wristband/token_test.go
findings:
critical: 0
warning: 8
info: 11
total: 19
status: 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:
```go
// 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:**
```go
// 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`:
```go
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:**
```go
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`:
```go
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:
```go
// 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.
```go
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_