docs(17): add code review fix report

.planning/phases/17-blog-companions/17-REVIEW-FIX.md

@@ -0,0 +1,156 @@
+---
+phase: 17-blog-companions
+fixed_at: 2026-10-12T22:10:00Z
+review_path: .planning/phases/17-blog-companions/17-REVIEW.md
+iteration: 1
+findings_in_scope: 8
+fixed: 8
+skipped: 0
+status: all_fixed
+---
+
+# Phase 17: Code Review Fix Report
+
+**Fixed at:** 2026-10-12T22:10:00Z
+**Source review:** .planning/phases/17-blog-companions/17-REVIEW.md
+**Iteration:** 1
+
+**Summary:**
+- Findings in scope: 8 (CR-01, WR-01 to WR-07; the Info findings are out of scope)
+- Fixed: 8
+- Skipped: 0
+
+Before each fix I checked the finding against the current source. Every finding still applied. Quick task 261012-s3g had not touched any of the affected code paths.
+
+Paths outside `summercms.go` are relative to `/media/nvme/dev/golem15/summercms.io/summercms/`. The plugin fixes are committed in each plugin's own repository (branch `main`). They are not pushed. The host app's submodule pointers (`sm-grzybyfunkcjonalne-app/plugins/golem15/{sitemap,menu,journal}`) still need a bump, in a separate commit after the plugin commits are pushed (see Follow-ups).
+
+## Fixed Issues
+
+### CR-01: Sitemap writes admin-supplied `javascript:` URLs into `<loc>` (stored XSS)
+
+**Files modified:**
+- `summercms.go`: `modules/signpost/signpost.go`, `modules/signpost/signpost_test.go`, `modules/signpost/README.md`, `docs/plugins/menu-item-types.md`
+- `sm-sitemap-plugin`: `classes/urlto.go`, `classes/generator.go`, `controllers/definitions.go`, `console/import_winter.go`, `console/import.go`, `lang/en/lang.yaml`, `lang/pl/lang.yaml`, `README.md`, plus tests in `classes/urlto_test.go`, `classes/generator_test.go`, `controllers/definitions_test.go` and `import_test.go`
+
+**Commits:** `964f1a7` (summercms.go), `c42bc26` (sm-sitemap-plugin). The admin smoke coverage of the row refusals is in `433202b`.
+
+**Applied fix:**
+- **Save check.** The definitions controller has new `FormBeforeCreate` and `FormBeforeUpdate` hooks. On each row they refuse:
+  - a `url` that `classes.AllowedItemURL` rejects (`data.<i>.url`, `errors.url_scheme`);
+  - a `urlPattern` that `signpost.ValidItemPattern` rejects (`data.<i>.urlPattern`, `errors.url_pattern`).
+- **Output filter.** `Generator.addItem` drops any `loc` or alternate that does not come out of `Url::to` as `http`/`https`. This covers rows stored before the fix, imports and registry results.
+- **Imports.**
+  - `sitemap:import-winter` drops url items with a disallowed URL, drops page items whose `--page-url` is disallowed, and clears a disallowed `urlPattern`. Each case gets a report line.
+  - `sitemap:import` refuses an entry with a disallowed URL.
+- **Framework.** `signpost.Registry.Pattern` now ignores an item override that is not a path: it must start with a single `/`, with no `//` or `/\` prefix, no surrounding whitespace and no control characters. The next layer then applies. The new helper `signpost.ValidItemPattern` is documented in the README and in `docs/plugins/menu-item-types.md`.
+- **Design choice.** The review's `allowedSitemapURL` would also have refused relative paths such as `about`. That would reject PHP data that worked, so the rule I applied is wider: a relative or `/` path, or an absolute `http`/`https` URL. Any other scheme is refused, and so are protocol-relative `//host` and `/\host`, a bare `#fragment`, surrounding whitespace and control characters. `//evil.test/x` cannot leave the site, because `Url::to` puts it under the root host.
+
+### WR-01: Moving a menu entry to another menu orphans its subtree; a parent from another menu overrides the chosen menu
+
+**Files modified:** `sm-menu-plugin`: `models/menu_entry.go`, `controllers/menuentries.go`, `lang/en/lang.yaml`, `lang/pl/lang.yaml`, `README.md`, `menu_smoke_test.go`
+**Commit:** `7255d34` (sm-menu-plugin)
+**Status:** fixed: requires human verification (logic change)
+**Applied fix:**
+- **Parent from another menu.** `MenuEntry.BeforeSave` still gives an entry with a parent its parent's menu, which keeps PHP parity for non-admin paths. When the save named a different menu, it now records that menu (`MenuEntry.RequestedMenuID`). `checkPlacement` then refuses the save with 422 on `parent` (`errors.parent_menu`), so the admin's menu choice is no longer lost without a message.
+- **Subtree move.** `FormAfterUpdate` moves every descendant into the entry's menu, inside the save transaction. It uses a recursive CTE with `UNION`, so a looping chain cannot loop the query.
+- **UI text.** The `menu_comment` help text explains both behaviours.
+- **Design choice.** I chose refusal over scoping the parent options, because `RelationExtendOptionsQuery` gets no record context.
+
+### WR-02: Reorder page sends overlapping saves; out-of-order completion leaves an older tree
+
+**Files modified:** `summercms.go`: `admin/src/views/ReorderView.vue`, `admin/tests/views/ReorderView.test.ts`, `docs/backend/lists-and-filters.md`
+**Commits:** `e1ac983`, plus `27bb13b` (dist rebuild)
+**Status:** fixed: requires human verification (logic change)
+**Applied fix:**
+- **One save at a time.** Only one save is in flight. A save requested meanwhile is queued as a snapshot (tree, partition and load generation) and sent after the first one settles.
+- **Stale responses.** A response whose generation is older than the page's current one leaves `tree` and `savedTree` alone. Both a reload and a partition switch start a new generation.
+- **Failures.** A failed save does not restore the old tree if a later save is waiting or a move is still in its 400 ms debounce, because that later save carries the whole tree. A 409 drops a queued save of the same set and reloads.
+- **Tests.** Two new specs cover a failure with a queued move and a late response after a partition switch. Both fail against the old code.
+
+### WR-03: A registry menu entry or sitemap row saved without a reference passes validation and then disappears
+
+**Files modified:**
+- `sm-menu-plugin`: `controllers/menuentries.go`, `lang/en/lang.yaml`, `lang/pl/lang.yaml`, `README.md`, `menu_smoke_test.go`
+- `sm-sitemap-plugin`: `controllers/definitions.go`, `README.md`, `sitemap_smoke_test.go`
+
+**Commits:** `64215f2` (sm-menu-plugin), `433202b` (sm-sitemap-plugin)
+
+**Applied fix:**
+- **Menu entries.** `checkEntry` refuses an entry whose registry type declares `References` but has an empty reference (`reference`, `errors.reference_required`).
+- **Sitemap rows.** The definitions controller refuses the same case per row (`data.<i>.reference`). It reuses the PHP phrase `golem15.sitemap::lang.item.reference_required`.
+- **Not changed.** `cabana.amongOptions` is left as it is. Required-ness is per type, which the plugins know and cabana does not.
+
+### WR-04: `journal-post` resolves unpublished posts, exposing drafts on the anonymous menu API and in sitemap.xml
+
+**Files modified:** `sm-journal-plugin`: `classes/menu_item_types.go`, `menu_item_types_test.go`, `menu_item_types_smoke_test.go`, `README.md`
+**Commit:** `31d901c` (sm-journal-plugin)
+**Applied fix:** `resolvePost` now applies `publishedPosts(db)`: the post must be published and have a publish date in the past. A draft or future post resolves to nothing.
+- **Design choice.** `postReferences` is unchanged and still lists drafts. Editors can wire a menu entry or sitemap row before a post goes live, and it appears once the post is published.
+- **Tests.** The unit and smoke tests now pin that drafts and future posts resolve to nil.
+
+### WR-05: `/sitemap.xml` is anonymous, unthrottled and resolves every registry item on each request
+
+**Files modified:** `sm-sitemap-plugin`: `routes.go`, `plugin.go`, `config/config.yaml`, `README.md`, `sitemap_smoke_test.go`
+**Commit:** `cf5c40e` (sm-sitemap-plugin)
+
+**Applied fix:**
+- **Throttle.** `/sitemap.xml` is behind the new surf bucket `sitemap-xml` (`sitemap.BucketSitemap`, `Plugin.Buckets`): 60 requests a minute per client IP, behind trusted proxies. `/sitemap.xsl` stays unthrottled.
+- **Cache.** The generated XML is cached in memory for `golem15.sitemap.cache_seconds` seconds (`sitemap.CacheConfigKey`): 60 by default, `0` turns it off, a negative value fails the boot.
+  - The cache key is a digest of the root URL, the stored definition items and the settings. A save of the definition or settings is therefore served at once, with no invalidation hooks. Registry content (a new or changed post) shows once the entry expires.
+  - Only one generation runs at a time, and concurrent misses wait for it.
+  - The cache holds at most 16 entries and is emptied when full, which bounds memory when `app.url` is unset and the Host header varies.
+- **Design choice.** I picked a short TTL keyed on the stored data over explicit invalidation. It is exact for definition and settings saves and needs no model hooks.
+- **Not done.** I did not cache the manifest by mtime: it is read only on a cache miss now.
+- **Tests.** The smoke harness boots with the cache off, because the existing tests change listeners and registry content between requests. A new test, `TestSitemapCacheAndThrottle`, covers a cache hit, a save served at once and the 429 on request 61.
+
+### WR-06: The menu importer skips the URL, target and same-menu-parent rules that admin saves enforce
+
+**Files modified:** `sm-menu-plugin`: `console/import_winter.go`, `menu_api_test.go`, `README.md`
+**Commit:** `9c0f833` (sm-menu-plugin)
+**Applied fix:** `mapEntries` makes each kept entry pass the admin rules through `adminRules`. Each change gets its own report line:
+- a URL refused by `classes.AllowedURL` is trimmed if the only problem is surrounding whitespace, and cleared otherwise;
+- a target outside `_self`/`_blank` is cleared;
+- an entry whose parent is in another menu becomes a top-level entry of its own menu, keeping its own children.
+
+**Design choice:** I cleared and re-parented instead of dropping, so no imported entry or subtree is lost.
+
+### WR-07: Opening a singleton settings entry in a new tab lands on a list route that answers 404
+
+**Files modified:** `summercms.go`: `admin/src/views/ListView.vue`, `admin/src/views/SettingsIndexView.vue`, `admin/tests/form/SingletonForm.test.ts`, `docs/backend/admin-controllers.md`, `docs/backend/settings.md`
+**Commits:** `9d7c21d`, `d9d6b26` (keeps the test name `check-phase17.sh` requires), plus `27bb13b` (dist rebuild)
+**Applied fix:**
+- **Settings index.** It no longer intercepts clicks or probes `/singleton`. Every controller link is a plain RouterLink to the list route.
+- **List view.** When the list request answers 404, ListView asks `GET …/singleton`. If that answers an id, it replaces the route with the record's form; while it waits, the loading state shows and no error flashes. If not, the 404 stands as before.
+- **Effect.** A click, a new tab and a bookmark all open the singleton's record. Non-singleton links make no extra request.
+- **Design choice.** I chose the admin-only alternative from the review over a new `SettingsEntry` flag. It needs no API, swag or OpenAPI change.
+
+## Verification
+
+All gates ran in the main checkout (`workflow.use_worktrees: false`, so no worktree was created). The plugin suites ran in their sibling checkouts. Docker was reachable, so the testcontainers suites ran.
+
+| Gate | Result |
+|------|--------|
+| `go vet ./...` (summercms.go) | pass |
+| `go test ./...` (summercms.go, with Docker) | pass (40 packages) |
+| `go test ./...` in sm-sitemap-plugin, sm-menu-plugin, sm-journal-plugin (with Docker) | pass |
+| `npm --prefix admin run typecheck` | pass |
+| `npm --prefix admin run test` | pass (88 files, 1330 tests) |
+| `npm --prefix admin run test:browser` | pass (7 tests) |
+| `scripts/check-admin-dist.sh` | pass (after the dist rebuild `27bb13b`) |
+| `scripts/check-admin-openapi.sh --check` | pass (inside the `check-phase17.sh` docs stage; no swag annotations changed) |
+| `go test ./cmd/summer -run TestDocsTree` | pass |
+| `scripts/check-phase17.sh --all` | pass: self-test, go, plugins, admin, layers, docs and postgres (Docker) |
+
+The host app (`sm-grzybyfunkcjonalne-app`) builds its plugins from its submodule checkouts. Its `check-phase17.sh` stages therefore ran against the old plugin commits plus the new framework. The `signpost` change is backward compatible.
+
+## Follow-ups
+
+- **Push and bump.** Push `sm-sitemap-plugin` (`c42bc26`, `433202b`, `cf5c40e`), `sm-menu-plugin` (`7255d34`, `64215f2`, `9c0f833`) and `sm-journal-plugin` (`31d901c`). Then bump the three submodule pointers in `sm-grzybyfunkcjonalne-app`, as a separate commit.
+- **nginx throttle.** The host's nginx could also add `limit_req` for `/sitemap.xml`. That is deploy config and is not changed here.
+- **Gate coverage.** The new tests are not yet listed in `check-phase17.sh`'s required-name arrays, so the gate runs them without requiring them by name: `TestSitemapCacheAndThrottle`, `TestOnlyHTTPLocs`, `TestAllowedItemURL`, `TestCheckItems`, `TestValidItemPattern` and the new ReorderView/SingletonForm specs. Add them in a planning or gate commit if wanted.
+
+---
+
+_Fixed: 2026-10-12T22:10:00Z_
+_Fixer: Claude (msd-code-fixer)_
+_Iteration: 1_