From 138631fe51ff8fa243e0bdbdaa1fdac9d4553cd0 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 12 Oct 2026 21:49:38 +0200 Subject: [PATCH] docs(17): add code review fix report --- .../17-blog-companions/17-REVIEW-FIX.md | 156 ++++++++++++++++++ 1 file changed, 156 insertions(+) create mode 100644 .planning/phases/17-blog-companions/17-REVIEW-FIX.md diff --git a/.planning/phases/17-blog-companions/17-REVIEW-FIX.md b/.planning/phases/17-blog-companions/17-REVIEW-FIX.md new file mode 100644 index 0000000..c273830 --- /dev/null +++ b/.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 `` (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..url`, `errors.url_scheme`); + - a `urlPattern` that `signpost.ValidItemPattern` rejects (`data..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..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_