eb84825724
WR-01 is fixed; WR-02 and the three info findings stay open. The phase goal is 8/8 with corpus 175/175/0. Co-authored-by: Cursor <cursoragent@cursor.com>
6.4 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 14.1-oauth-identities-and-fonoteka-me-routes | 2026-10-05T19:45:00Z | standard | 14 |
|
|
issues_found |
Phase 14.1: Code Review Report
Reviewed: 2026-10-05T19:45:00Z Depth: standard Files Reviewed: 14 Status: issues_found
Summary
Phase 14.1's production path is largely aligned with D-01–D-12: last-method 409 is identity-count-only (D-06), missing/foreign/unknown DELETE share one host WriteWinterHTTPError (HTTP-01), GET list is an explicit {provider, linked_at} map, identity routes sit on the JWT group only with unconstrained {provider} and DELETE throttle:10,1, and /me emits collection_ids JSON null while scopes stay [] (D-09). Two warnings remain: Hidden() for profile_data is not backed by json:"-" (and TestHiddenNeverMarshals is currently red), and last-method 409 is a non-transactional count-then-delete.
No BLOCKER (critical) findings on the listed HTTP contracts.
Warnings
WR-01: Hidden profile_data is not json:"-"; DATA-07 registry test is red
File: ../fonoteka.go/plugins/golem15/user/models/oauth_identity.go:15-29
Issue: AccessToken and RefreshToken correctly carry json:"-". ProfileData is listed in Hidden() but has only a GORM tag. lagoon.Jsonable has no MarshalJSON, so encoding/json of an OAuthIdentity emits exported Data / Valid / NullOnEmpty under the key ProfileData (emails and other profile PII). Index itself is safe (explicit two-key map, D-05), and TestOAuthIdentitiesIndexDoesNotLeakEncryptedTokensOrProfileData passes on that path.
The project's HasHidden backstop does not. plugins/golem15/fonoteka/classes/hidden_marshal_test.go still has expectedUserModels = 7. Confirmed this review: go test ./plugins/golem15/fonoteka/classes -run TestHiddenNeverMarshals fails with user models.All() = 8, want 7. After bumping the count, assertHiddenTagged would fail on profile_data, and populateHidden cannot set a Jsonable field.
Phase 02's go test ./plugins/golem15/fonoteka does not include the classes package, so this stayed hidden from the plan verify command.
Fix:
ProfileData lagoon.Jsonable[map[string]any] `gorm:"column:profile_data" json:"-"`
Bump expectedUserModels to 8 and extend setHiddenSentinel (or skip Jsonable populate once json:"-" is present). Re-run TestHiddenNeverMarshals.
WR-02: Last-method 409 is a non-transactional count-then-delete
File: ../fonoteka.go/plugins/golem15/user/controllers/oauth_identities.go:81-98
Issue: Destroy loads the row, then Counts identities for user_id, then Deletes if n != 1. Two concurrent DELETEs of two remaining providers can both observe n == 2 and both delete, leaving zero identities. D-06 is fail-closed lockout; D-13 already records that a social-only account cannot sign in on Go. throttle:10,1 does not serialize in-flight requests. Sequential unit tests (204 sibling, EN/PL 409, 409-with-password) cannot catch this.
Fix: Run Find + Count + Delete in one transaction with SELECT … FOR UPDATE on the caller's identity rows (or a single DELETE … WHERE user_id = ? AND provider = ? AND (SELECT count(*) …) > 1 that returns 0 rows → 409). Keep the password flag unused.
Info
IN-01: Winter 404 unit tests inject a stub, not host WriteWinterHTTPError
File: ../fonoteka.go/plugins/golem15/user/oauth_identities_test.go:155-180
Issue: Missing, foreign, and unknown DELETE all call writeOAuthIdentityNotFound. The host (routes.go:241-245) injects api.WriteWinterHTTPError, whose HTML does not include the path — production bodies are indistinguishable. Plugin tests prove that property only against a constant stub (writeWinter404 / winter404Body), not the embedded winter_404.html + app.url Origin. A future WriteNotFound that branched on r.URL would still pass these tests.
Fix: Optional: point Destroy tests at api.WriteWinterHTTPError with a config app.url of http://127.0.0.1:8423 so they share bytes with the recorded DELETE fixture.
IN-02: MeToken D-09 empty-but-Valid CollectionIDs is untested
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/me_token_controller.go:36-38
Issue: The handler correctly keeps collection_ids as a nil any (JSON null) unless Valid && len(Get()) > 0, and uses wire.Slice only for scopes. TestMeTokenHandlerNilScopesSerializeAsEmptyArrayAndCollectionIDsAsJSONNull uses a zero ApiToken (Valid == false). mintUnrestrictedParityToken also seeds invalid Jsonable. The Valid && empty slice branch is implemented and unused by tests.
Fix: Add a case with CollectionIDs: lagoon.Jsonable[[]uint]{Data: []uint{}, Valid: true} and require "collection_ids":null still, not [].
IN-03: Index Find decrypts Encrypted token columns it never returns
File: ../fonoteka.go/plugins/golem15/user/controllers/oauth_identities.go:31-48
Issue: Find(&rows) scans access_token / refresh_token, so lagoon.Encrypted.Scan decrypts plaintext into process memory for a list that only emits provider and linked_at. Not a JSON leak (json:"-" on those fields; explicit map). GORM debug logging of the struct is redacted (String / GoString / MarshalJSON → [redacted]).
Fix: Select("provider", "linked_at") (or omit the encrypted columns) on the Index query.
Reviewed: 2026-10-05T19:45:00Z Reviewer: the agent (gsd-code-reviewer) Depth: standard