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>
94 lines
6.4 KiB
Markdown
94 lines
6.4 KiB
Markdown
---
|
||
phase: 14.1-oauth-identities-and-fonoteka-me-routes
|
||
reviewed: 2026-10-05T19:45:00Z
|
||
depth: standard
|
||
files_reviewed: 14
|
||
files_reviewed_list:
|
||
- ../fonoteka.go/plugins/golem15/user/models/oauth_identity.go
|
||
- ../fonoteka.go/plugins/golem15/user/updates/202610050001_create_oauth_identities.go
|
||
- ../fonoteka.go/plugins/golem15/user/controllers/oauth_identities.go
|
||
- ../fonoteka.go/plugins/golem15/user/oauth_identities_test.go
|
||
- ../fonoteka.go/plugins/golem15/user/updates/oauth_identities_test.go
|
||
- ../fonoteka.go/plugins/golem15/user/lang/en/lang.yaml
|
||
- ../fonoteka.go/plugins/golem15/user/lang/pl/lang.yaml
|
||
- ../fonoteka.go/plugins/golem15/fonoteka/routes.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/phase08_coverage_test.go
|
||
- ../fonoteka.go/parity/parity_test.go
|
||
- ../fonoteka.go/parity/fonoteka_seed_test.go
|
||
- ../fonoteka.go/parity/fonoteka_reset.php
|
||
findings:
|
||
critical: 0
|
||
warning: 2
|
||
info: 3
|
||
total: 5
|
||
status: 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:**
|
||
```go
|
||
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 `Count`s identities for `user_id`, then `Delete`s 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_
|