docs(14.2): add code review fix report

2026-10-12 12:23:09 +02:00
parent a082a0583d
commit fb0d051f90
1 changed files with 150 additions and 0 deletions
--- a/.planning/phases/14.2-local-fonoteka-frontend-on-local-summercms-backend/14.2-REVIEW-FIX.md
+++ b/.planning/phases/14.2-local-fonoteka-frontend-on-local-summercms-backend/14.2-REVIEW-FIX.md
@@ -0,0 +1,150 @@
+---
+phase: 14.2-local-fonoteka-frontend-on-local-summercms-backend
+fixed_at: 2026-10-12T10:20:00Z
+review_path: .planning/phases/14.2-local-fonoteka-frontend-on-local-summercms-backend/14.2-REVIEW.md
+iteration: 1
+findings_in_scope: 9
+fixed: 9
+skipped: 0
+status: all_fixed
+---
+
+# Phase 14.2: Code Review Fix Report
+
+**Fixed at:** 2026-10-12T10:20:00Z
+**Source review:** .planning/phases/14.2-local-fonoteka-frontend-on-local-summercms-backend/14.2-REVIEW.md
+**Iteration:** 1
+
+**Summary:**
+- Findings in scope: 9 (WR-01 to WR-09; Info findings were out of scope)
+- Fixed: 9
+- Skipped: 0
+
+Commits went to the repository that owns each file. Repositories: `summercms.go` (framework), `fonoteka.go` (app), `vue-fonoteka-app`. `workflow.use_worktrees` is `false`, so every edit and commit was made in the main checkouts.
+
+## Fixed Issues
+
+### WR-01: Access log writes share and invitation tokens for 405 and 404 responses on token routes
+
+**Files modified:** `modules/surf/accesslog.go`, `modules/surf/accesslog_test.go`, `modules/surf/README.md`, `docs/services/logging.md`; fonoteka.go `README.md`
+**Commits:** summercms.go `fbcfaaf`; fonoteka.go `560bb3e`
+**Applied fix:** The request log no longer logs the URL path, not even for an unmatched request. It records `method`, `pattern` (empty when unmatched), `status` and `duration_ms`. New test cases: `POST /blog/posts/<secret>` (405) and `/blog/posts/<secret>/` (404). Both assert that the secret and the path are absent. The docs and the fonoteka README say that the Go log never shows a path.
+
+### WR-02: 499 remap hides real server errors whenever the client has gone away
+
+**Files modified:** `modules/surf/accesslog.go`, `modules/surf/router.go`, `modules/surf/accesslog_test.go`, `modules/surf/README.md`, `docs/services/logging.md`, `scripts/check-phase14.2.sh`
+**Commit:** summercms.go `c30145e`
+**Status:** fixed: requires human verification (logic change)
+**Applied fix:** A 5xx is logged as 499 only when both of these hold:
+- The request context had already ended with `context.Canceled` when the status was written. `statusRecorder` checks this at the first `WriteHeader`/`Write`.
+- The handler did not panic. `recoverJSON` and `recoverBare` now mark the access entry through `markPanicked`.
+
+A 5xx written before the client went away keeps its real status. A recovered panic is always 500. Every record of a request whose client went away carries `client_gone=true`. The handler's own error is not visible to the log. The heuristic "cancelled before the status was written" therefore stands in for "the error is the cancellation". A real error that happens after cancellation and does not panic would still log 499. That case needs a cancelled context, so it is narrow.
+
+Tests cover seven cases:
+- cancel, then 5xx
+- live 5xx
+- 5xx, then cancel
+- 200, then cancel
+- `recoverJSON` 5xx after cancel
+- panic after cancel, through `recoverJSON`
+- panic after cancel, through `recoverBare`
+
+A gate self-test plants `status=500 ... client_gone=true` and checks that `scan_go_log` refuses it.
+
+### WR-03: Uploads route serves the stored content type verbatim on the app origin (stored XSS via text/html)
+
+**Files modified:** `modules/surf/uploads.go`, `modules/surf/uploads_test.go`, `modules/surf/README.md`, `docs/services/storage.md`, `docs/setup/configuration.md`
+**Commit:** summercms.go `41a03b9`
+**Applied fix:** Every uploads response now carries `X-Content-Type-Options: nosniff` and `Content-Security-Policy: default-src 'none'; img-src 'self'; media-src 'self'; sandbox`. A successful response whose type is not a raster image (jpeg, png, gif, webp, avif) gets `Content-Disposition: attachment`, on GET and HEAD. That covers HTML, SVG, PDF, text and a missing type. Error responses get no disposition. The disposition is decided when the status is written, after `attach.StaticHandlerPublic` set the type.
+
+Tests:
+- `TestUploadsResponseHeaders` (no Docker) runs a type matrix on GET and HEAD.
+- `TestUploadsMount` (Docker) now stores a public `text/html` blob and asserts it is served as an attachment, while the png stays inline.
+
+Checked live: a seeded `.jpg` cover is served through Nuxt as `200 image/jpeg` with the CSP and no disposition. The docs' "nosniff prevents HTML" claim is corrected.
+
+### WR-04: `storage.uploads.public_path_prefix` is not validated
+
+**Files modified:** `modules/surf/uploads.go`, `modules/surf/uploads_test.go`, `modules/surf/README.md`, `docs/services/storage.md`
+**Commit:** summercms.go `01d2548`
+**Applied fix:** A new function, `uploadsPrefix`, validates the prefix when `storage.uploads.serve` is on. It refuses:
+- a missing leading slash
+- a scheme or host
+- `//`
+- `{` or `}`
+- `?` or `#`
+- whitespace or a backslash
+- an empty, `.` or `..` segment
+- a bare `/`
+
+The error names `storage.uploads.public_path_prefix` and says "absolute URL path". Test cases include `storage/uploads`, `storage/app/uploads/public`, `https://cdn.example.com/uploads` and `//cdn...`. Valid prefixes with a trailing slash or surrounding spaces pass. A CDN prefix also passes while `serve` is off.
+
+### WR-05: `fonoteka:dev-seed` takes over existing accounts and tokens behind a single environment-name gate
+
+**Files modified:** fonoteka.go `plugins/golem15/fonoteka/console/dev_seed.go`, `console/dev_seed_test.go`, `dev_marker.go`, `dev_marker_test.go`, `plugin.go`, `scripts/dev-stack.sh`, `.env.dev.example`, `README.md`; summercms.go `scripts/check-phase14.2.sh`
+**Commits:** fonoteka.go `389a059`; summercms.go `8b835f3`
+**Status:** fixed: requires human verification (behaviour change: `dev` is no longer accepted)
+**Applied fix:**
+- **Existing user:** a user with `E2E_EMAIL` and no `dev-seed-mcp` API token row (live or revoked) is one the seed did not create. The seed now refuses it before writing anything, unless `DEV_SEED_RESET_EXISTING=1`. The seed creates the user and the token in one transaction, so the token reliably marks seeded users. dev-stack.sh does not set the opt-in. The local demo user already carries the marker, so `up` reported `user: unchanged`.
+- **Fixed token:** a `DEV_SEED_API_TOKEN` row owned by another user is refused and never reassigned. The error names the token id, never the secret. The update path no longer writes `user_id`.
+- **Environment:** `console.DevEnvironment` accepts `local` only. The seed, the marker route (which shares it) and dev-stack.sh `load_env` all refuse `dev`. It is documented as a common name for a shared server. The running stack was already `env: local`.
+- **Tests:** the new `TestDevSeedRefusesForeignRows` covers a foreign user (opt-in values "", 0, yes, true are refused; password and must_change_password are untouched; the counts do not change; `=1` takes the user over and later runs need no opt-in) and a token row owned by another user. `TestDevSeedRefusesProduction` and `TestDevMarkerRoute` now expect `dev` to be refused. The gate's `CONSOLE_REQUIRE` lists the new test.
+
+The reviewer also suggested a seed-side check that the DSN host is loopback. It was not added: dev-stack.sh already enforces it, and the guidance for this fix did not require it.
+
+### WR-06: Minted API token secret persisted to a group/world-readable file and echoed into Playwright output
+
+**Files modified:** fonoteka.go `scripts/dev-stack.sh`, `README.md`; vue-fonoteka-app `tests/e2e/helpers/runTinker.ts`, `tests/contracts/e2e-backend-switch.test.mjs`; summercms.go `scripts/check-phase14.2.sh`
+**Commits:** fonoteka.go `a5527e9`; vue-fonoteka-app `70eba9e`; summercms.go `bb5c2d3`
+**Applied fix:**
+- **dev-stack.sh:** `seed()` removes `seed.out`, then writes the seed output to it under `umask 077` (mode 600; the old file was 664 and `tee` would have kept that mode). The terminal gets the output through `redact_seed`, which replaces `created <secret> (shown once)`. A new `redact-seed` subcommand exposes the filter.
+- **runTinker.ts:** the seed runs with `stdio: ['ignore', 'pipe', 'inherit']`, and stdout is echoed through the exported `redactSeedOutput`. The secret is also scrubbed from the thrown error's stdout and message.
+- **Tests:** the contract test plants a secret in a fake seed and asserts it never reaches stdout or stderr (it failed before the fix). The gate self-test pipes a planted token line through `redact-seed`, and `--e2e` refuses unless `seed.out` has mode 600. After the restart it was 600.
+
+### WR-07: Phase gate `--e2e` can verify a stale binary
+
+**Files modified:** fonoteka.go `scripts/dev-stack.sh`, `README.md`; summercms.go `scripts/check-phase14.2.sh`
+**Commits:** fonoteka.go `45ee468`; summercms.go `3159bc1`
+**Applied fix:**
+- **Build stamp:** `up` creates `build.stamp.pending` before `summer build`. Once Go answers, it moves the file to `tmp/dev-stack/build.stamp`, so the stamp's mtime is the moment the build started. `down` removes the stamp.
+- **`fresh` subcommand:** refuses a missing stamp, or any file of the app or framework checkout newer than the stamp. It ignores .git, node_modules, testdata, .planning, .claude, .msd, tmp, storage, bin, parity, the framework's admin, docs and scripts, `*.md`, `*_test.go` and `*.golden`.
+- **Already-up stack:** `up` on a stack that is already running warns when it is stale.
+- **Gate:** `--e2e` restarts a running stack that `fresh` refuses (`down`, then `up --detach`, left running) and requires `fresh` to pass after `up`. The self-test plants an app and framework tree with five cases: clean, ignored files, a stale framework source, a stale app source and no stamp.
+
+### WR-08: go.log 5xx scan passes when the access log is off
+
+**Files modified:** summercms.go `scripts/check-phase14.2.sh`; fonoteka.go `scripts/dev-stack.sh`, `README.md`
+**Commits:** summercms.go `1fb3fb9`, `f70a0a5`, `a082a05`; fonoteka.go `261d9bc`
+**Applied fix:**
+- **Scan:** `scan_go_log FILE OFFSET` still refuses a 5xx or a search-failure line anywhere in the file. It now also refuses unless the part written during this run has an `http request` record. `--e2e` takes the offset after `up`/`fresh`.
+- **dev-stack.sh:** `check_keys` defaults `SUMMER_HTTP__ACCESS_LOG` and `SUMMER_STORAGE__UPLOADS__SERVE` to `true` when they are unset, and refuses any other value.
+- **Self-tests:** no records, an empty log, records only before the offset, records after the offset, a long log, and planted env files with the request log or the uploads route off.
+- **Follow-up `a082a05`:** the first version (`tail | grep -q` under `pipefail`) wrongly refused a long go.log that did have records: grep stopped at the first match and tail died of SIGPIPE. The gate run found it. The check now counts matches, and a 20000-line self-test case covers it.
+
+### WR-09: Realtime spec accepts any publication on the channel, including another spec's
+
+**Files modified:** vue-fonoteka-app `tests/e2e/realtime.spec.ts`
+**Commit:** vue-fonoteka-app `4d8d525`
+**Status:** fixed: requires human verification (test logic)
+**Applied fix:** The spec takes a snapshot of `received.length` before `createAlbum`. The assertion then accepts only later frames on the channel whose `push.pub.data.payload.id` equals the created album's id. Both backends publish the `{event, payload, timestamp}` envelope (UAT Finding C; Go `AlbumBroadcastPayload`). It passed in the gate run. It was not shown failing before the fix, because the false pass depends on timing across parallel specs.
+
+## Verification
+
+Gates ran in the main checkouts of all three repositories (no worktree), with Go on 127.0.0.1:8423 and Nuxt on :3119.
+
+- After the Go changes, the stack was restarted with `scripts/dev-stack.sh down` and then `up --detach` in fonoteka.go. The seed showed every entity `unchanged` and the token line `unchanged (secret not shown)`. The new `build.stamp` was written and `fresh` passed. Nuxt was warmed up with requests to /en, /en/albums, /en/login and /pl.
+- **First `--all`:** every stage up to guard passed, then e2e failed. `auth.setup` timed out waiting to leave /login: the backend answered `POST /login` 200 and the post-login API calls 200, so this was Nuxt compiling the first post-login page cold, not a code failure. `--e2e` alone then passed with 17 expected and nothing unexpected, skipped or flaky.
+- **Second `--all`:** e2e failed on the WR-08 SIGPIPE bug described above. It was fixed in `a082a05`.
+- **Final `bash scripts/check-phase14.2.sh --all`:** every stage passed, from self-test, go (vet and full `go test ./...` with Docker in summercms.go, the fonoteka.go root and the fonoteka plugin) and docs, through parity (175/175), contracts (10/10 blocking) and guard, to e2e (17/0/0/0).
+- **Report-only results:** specs 19 expected and 22 unexpected; `pnpm test:contracts` fails 1 of 289. Both are the same as before the fixes (earlier reports `e2e-20261011T213918Z`/`T215650Z`; `missing-catalog` is in deferred-items).
+
+Last gate line: `Phase 14.2 gate passed`
+
+The stack is still running (`dev-stack.sh status`: marker on Go and through :3119).
+
+---
+
+_Fixed: 2026-10-12T10:20:00Z_
+_Fixer: Claude (msd-code-fixer)_
+_Iteration: 1_