docs(03): add code review report
This commit is contained in:
Jakub Zych
@@ -0,0 +1,190 @@
|
||||
---
|
||||
status: issues
|
||||
phase: 03-first-vertical-slice-genres-end-to-end
|
||||
depth: standard
|
||||
files_reviewed: 39
|
||||
files_reviewed_list:
|
||||
- bouncer/context.go
|
||||
- bouncer/jwt.go
|
||||
- bouncer/jwt_test.go
|
||||
- cmd/summer/main.go
|
||||
- examples/hello/hello_test.go
|
||||
- examples/hello/plugins/greeter/plugin.go
|
||||
- ../fonoteka.go/app/app.go
|
||||
- ../fonoteka.go/go.mod
|
||||
- ../fonoteka.go/main.go
|
||||
- ../fonoteka.go/parity/genre_integration_test.go
|
||||
- ../fonoteka.go/parity/genre_security_test.go
|
||||
- ../fonoteka.go/parity/genre_smoke_test.go
|
||||
- ../fonoteka.go/parity/genres_seed_test.go
|
||||
- ../fonoteka.go/parity/manifest.yaml
|
||||
- ../fonoteka.go/parity/parity_contract_test.go
|
||||
- ../fonoteka.go/parity/parity_test.go
|
||||
- ../fonoteka.go/plugins/golem15/fonoteka/active_collection.go
|
||||
- ../fonoteka.go/plugins/golem15/fonoteka/genre.go
|
||||
- ../fonoteka.go/plugins/golem15/fonoteka/genre_handler.go
|
||||
- ../fonoteka.go/plugins/golem15/fonoteka/plugin.go
|
||||
- ../fonoteka.go/plugins/golem15/user/plugin.go
|
||||
- ../fonoteka.go/README.md
|
||||
- go.mod
|
||||
- internal/build/build.go
|
||||
- lagoon/commands.go
|
||||
- lagoon/connection.go
|
||||
- lagoon/connection_test.go
|
||||
- lagoon/migrations.go
|
||||
- lagoon/migrations_test.go
|
||||
- lagoon/order.go
|
||||
- lagoon/order_test.go
|
||||
- lagoon/postgres_test.go
|
||||
- pact/capabilities.go
|
||||
- scripts/check-phase3.sh
|
||||
- surf/middleware_test.go
|
||||
- surf/params.go
|
||||
- surf/params_test.go
|
||||
- surf/router.go
|
||||
- surf/serve.go
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 7
|
||||
info: 4
|
||||
total: 11
|
||||
reviewed: 2026-09-17T18:45:00Z
|
||||
---
|
||||
|
||||
# Phase 3: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-09-17T18:45:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 39
|
||||
**Status:** issues
|
||||
|
||||
## Summary
|
||||
|
||||
The slice is a real boot path: shared pgx-stdlib `*sql.DB` under GORM, per-plugin gormigrate, named `jwt.auth` / `inv.must-change-password` on ServeMux, and `GET /_fonoteka/api/v1/genres` with grouped owner-OR-editor then AND active `collection_id`. HS256 is pinned (`WithValidMethods`, `alg:none` rejected), empty JWT secret fails `Boot`, missing middleware names fail Assemble, and CORS preflight short-circuits before named auth. Tenant counts are parameterized; `lagoon.OrderBy` only emits an allow-listed identifier. The recorded genres fixture stays unmodified; corpus passing increments only after replay.
|
||||
|
||||
No authentication bypass or SQL injection was found on the genre path. The defects that matter are in the new HTTP server and the two boot seams: `http.Server` has no timeouts, panic recovery can append a 500 body onto a committed response, `summer serve` drops `--addr`, and production `Activate` runs before the database is published while tests use `app.Handler` (Publish then Activate). PHP `JwtAuthenticate` maps unexpected lookup errors to 500; Go maps them to 401. Typed-param 404s are `net/http`'s HTML page.
|
||||
|
||||
Caled files not in SUMMARY frontmatter (`cmd/summer/runtime.go`, `fonoteka` `user.go` / `password.go` / `migrations.go`) were read to trace those paths.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: HTTP server has no header/body timeouts
|
||||
|
||||
**File:** `surf/serve.go:49`
|
||||
**Issue:** `ServeCommand` is the first production `http.Server` in the kernel. The struct sets only `Addr`, `Handler`, and `BaseContext`. `ReadHeaderTimeout` (and `ReadTimeout` / `WriteTimeout` / `IdleTimeout`) stay at zero. Go's default is "wait forever for the next header byte," which is the Slowloris hold-open. Graceful shutdown on SIGINT/SIGTERM is present; connection-level idle abuse is not.
|
||||
|
||||
Phase 6 owns CORS/rate-limit depth. Timeouts are serve hardening for this command, not that work.
|
||||
|
||||
**Fix:**
|
||||
|
||||
```go
|
||||
srv := &http.Server{
|
||||
Addr: addr,
|
||||
Handler: h,
|
||||
BaseContext: func(net.Listener) context.Context { return ctx },
|
||||
ReadHeaderTimeout: 10 * time.Second,
|
||||
ReadTimeout: 30 * time.Second,
|
||||
WriteTimeout: 30 * time.Second,
|
||||
IdleTimeout: 60 * time.Second,
|
||||
}
|
||||
```
|
||||
|
||||
### WR-02: Panic recovery writes a second body after headers are committed
|
||||
|
||||
**File:** `surf/router.go:290-300`
|
||||
**Issue:** `recoverJSON` always `WriteHeader(500)` and writes `{"error":true,"message":"Internal server error"}`. If the handler (or later middleware) already called `WriteHeader` / `Write` and then panics, `WriteHeader` is superfluous and `Write` **appends** to the existing body. A 200 JSON payload can become `{"data":[...]}{"error":true,...}` with status still 200. The panic test panics before any write, so this is untested.
|
||||
|
||||
**Fix:** Track whether the response started (a thin `http.ResponseWriter` wrapper with a `wrote` flag). On recover, if already written, log and return without a second body. If not, write the opaque 500 JSON.
|
||||
|
||||
### WR-03: `summer serve` cannot pass `--addr` to the app binary
|
||||
|
||||
**File:** `cmd/summer/main.go:38`, `cmd/summer/runtime.go:15-25`
|
||||
**Issue:** App `surf.ServeCommand` declares `--addr` (default `:8080`). The tool binary registers `delegateCommand("serve", ...)`, which has no flags and forwards only `in.Args()`. Cobra on `summer serve` therefore rejects `--addr` as unknown, or (if forced as a positional) never maps it onto the app flag. `migrate:rollback --plugin` was special-cased; `serve --addr` was not. Operators following the two-binary model cannot change the listen address without invoking `./bin/fonoteka serve` directly.
|
||||
|
||||
**Fix:** Declare the same `--addr` flag on the summer `serve` delegate and append `--addr`, value when set, matching `delegateRollbackCommand`. Or forward unknown flags via `cmd.Flags().Args()` / a dedicated passthrough.
|
||||
|
||||
### WR-04: Production boots plugins before the database is published; tests do not
|
||||
|
||||
**File:** `../fonoteka.go/main.go:31-37`, `surf/serve.go:35-43`, `../fonoteka.go/app/app.go:31-43`
|
||||
**Issue:** Generated `run` calls `party.Activate` with no `*sql.DB` / `*gorm.DB` on the backpack. `ServeCommand` (and `lagoon.withDB`) open, `Publish`, then `Assemble`. The in-process seam `app.Handler` does `lagoon.Use` + `Publish` **then** `Activate`. Current `Boot` implementations only check the JWT secret, so both paths work. Any later plugin that `Lookup[*gorm.DB]()` in `Boot` (Phase 5 models, Phase 7 user) will fail under `fonoteka serve` / `fonoteka migrate` while `app.Handler` tests stay green.
|
||||
|
||||
**Fix:** Publish the pool before `Activate` in generated `run` (open lazily in `Activate` is worse). Or generate `run` to match `app.Handler`: open, publish, activate, then register commands that reuse the published handles instead of `OpenFromApp` again. Add a test that boots the generated `run`/`ServeCommand` path, not only `app.Handler`.
|
||||
|
||||
### WR-05: User-lookup failures return 401, not PHP's 500
|
||||
|
||||
**File:** `bouncer/jwt.go:52-56`
|
||||
**Issue:** D-10 requires the same status and JSON as `JwtAuthenticate.php`. That middleware returns 401 for `UnauthorizedHttpException` and **500** `{"error":true,"message":"Authentication error"}` for unexpected errors (DB, config). Go uses `write401` for every `FindByID` error, including a missing GORM handle. A down database is indistinguishable from a bad token (401 vs 500). The string `"Authentication error"` matches; the status does not.
|
||||
|
||||
**Fix:**
|
||||
|
||||
```go
|
||||
user, err := users.FindByID(r.Context(), uint(id))
|
||||
if err != nil {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusInternalServerError)
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{"error": true, "message": "Authentication error"})
|
||||
return
|
||||
}
|
||||
```
|
||||
|
||||
Keep 401 for `user == nil` (`User not found`). Assert status 500 in the existing leak test when the provider returns an error.
|
||||
|
||||
### WR-06: Typed-param failures are HTML `404 page not found`
|
||||
|
||||
**File:** `surf/params.go:81-93`, `examples/hello/plugins/greeter/plugin.go:60-65`
|
||||
**Issue:** HTTP-01 / T-03-07 require malformed and unknown ids to 404. `constrain` and the greeter handler call `http.NotFound`, which writes `text/plain` `404 page not found`. PHP API 404s on `/_fonoteka/api/v1/.../{id}` are JSON envelopes. Genres has no path param, so the recorded fixture is unaffected; every later integer route inherits this body. Tests only assert status.
|
||||
|
||||
**Fix:** A small JSON 404 writer used by `constrain` and recommended to handlers:
|
||||
|
||||
```go
|
||||
func writeJSON404(w http.ResponseWriter) {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
_, _ = w.Write([]byte(`{"error":true,"message":"Not found"}`))
|
||||
}
|
||||
```
|
||||
|
||||
Keep status 404 for both malformed and unknown.
|
||||
|
||||
### WR-07: Phase 3 gate leaks the synthetic smoke server on failure
|
||||
|
||||
**File:** `scripts/check-phase3.sh:76-123`
|
||||
**Issue:** `cleanup_smoke` is defined but never `trap`ped. `set -e` plus a failing `parity:record` / `parity:replay` in the subshell exits the script before the success-path `cleanup_smoke` call. The background `python3` `serve_forever` stays bound to the ephemeral port. Phase 2 REVIEW IN-04 reported the same pattern in `check-phase2.sh`; this script copied it.
|
||||
|
||||
**Fix:** After defining `cleanup_smoke`, `trap cleanup_smoke EXIT` (and still call it on the happy path, or rely on the trap only).
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Two plugin-id lists can drift
|
||||
|
||||
**File:** `../fonoteka.go/app/app.go:18-19`, `../fonoteka.go/plugins.gen.go:11-14`
|
||||
**Issue:** `app.PluginIDs` is hand-maintained. Generated `main.PluginIDs` comes from `summer.yaml`. Tests and `app.Handler` use the former; `fonoteka serve` uses the latter. Adding a plugin and running `summer build` updates production without updating the test seam.
|
||||
|
||||
**Fix:** Share one generated list (blank-import friendly package, or `app.PluginIDs` written by `summer build`).
|
||||
|
||||
### IN-02: Bearer prefix is exact `Bearer `, not PHP's 6-character strip
|
||||
|
||||
**File:** `bouncer/jwt.go:86-96`
|
||||
**Issue:** PHP `AuthHeaders` takes `substr($header, strlen('bearer'))` then `trim` — case-insensitive in practice and tolerant of `bearer ` / `BEARER `. Go `CutPrefix(h, "Bearer ")` rejects those. The Nuxt fixture sends `Bearer {{jwt:alice}}`, so replay is green. A client that sends `bearer` will 401 `Token not provided` against Go and succeed against PHP.
|
||||
|
||||
**Fix:** Case-fold the scheme and accept any whitespace after it (`Authorization` RFC 6750). Keep the 401 body for missing/empty tokens.
|
||||
|
||||
### IN-03: Fallback active collection filters `kind=collection`; PHP `resolve()` does not
|
||||
|
||||
**File:** `../fonoteka.go/plugins/golem15/fonoteka/active_collection.go:119-126`
|
||||
**Issue:** PHP `ActiveCollectionResolver::resolve` fallback is `Collection::accessibleBy($user)->orderBy('id')->first()` with no kind filter (`switchTo` is the path that adds `kind = collection`). Go `firstAccessibleRealCollection` requires `kind=collection`. Plan 02 chose that on purpose (wishlist must not become the tenant). Stored wishlist context is still accepted (`findAccessibleCollection` has no kind filter), matching PHP `find()`. A user whose lowest accessible row is a wishlist gets different fallbacks on PHP vs Go. Seed/parity always persist a real collection, so the fixture does not catch this.
|
||||
|
||||
**Fix:** Keep the kind filter (it is the safer product rule) and add a Go test for "stored wishlist still counts wishlist albums; missing context skips wishlist IDs." Document the PHP `resolve()` divergence so Phase 12 does not "fix" it back.
|
||||
|
||||
### IN-04: `migrate` / `migrate:status` fail closed on an empty JWT secret
|
||||
|
||||
**File:** `../fonoteka.go/plugins/golem15/user/plugin.go:39-42`, `../fonoteka.go/main.go:31-36`
|
||||
**Issue:** `Activate` runs before any command. Empty `golem15.user.jwt.secret` fails `Boot`, so `fonoteka migrate` cannot apply DDL without a JWT secret. That matches D-11 fail-boot, but it couples migrations to an HTTP credential. README already exports both env vars together.
|
||||
|
||||
**Fix:** None required for v1 if the coupling is accepted. Optionally move the secret check into `Middlewares()` / `Assemble` so CLI migrate works with only `SUMMER_DATABASE__DSN`.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-09-17T18:45:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user