From fb0d051f90715275878f513a6e20e912d75ad403 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 12 Oct 2026 12:23:09 +0200 Subject: [PATCH] docs(14.2): add code review fix report --- .../14.2-REVIEW-FIX.md | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 .planning/phases/14.2-local-fonoteka-frontend-on-local-summercms-backend/14.2-REVIEW-FIX.md diff --git 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 new file mode 100644 index 0000000..3899ba2 --- /dev/null +++ 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/` (405) and `/blog/posts//` (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 (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_