docs(17): add code review report
This commit is contained in:
406
.planning/phases/17-blog-companions/17-REVIEW.md
Normal file
406
.planning/phases/17-blog-companions/17-REVIEW.md
Normal file
@@ -0,0 +1,406 @@
|
||||
---
|
||||
phase: 17-blog-companions
|
||||
reviewed: 2026-10-12T12:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 111
|
||||
files_reviewed_list:
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/package.json
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/api/types.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/app/router.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/DropdownField.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/RepeaterField.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/RichEditorField.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/formContext.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/FormErrorBanner.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/FormField.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/FormGrid.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/formState.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/PreviewField.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/registry.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/richeditor/editor.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/richeditor/html.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/richeditor/LinkPopover.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/richeditor/lossy.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/reorder/context.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/reorder/ReorderBranch.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/reorder/tree.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/FormView.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/ListView.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/ReorderView.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/SettingsIndexView.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/go.mod
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/actions.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/admin_openapi.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/contracts.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/crud.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_dependent.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_repeater.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_richeditor.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/form_schema.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/list_schema.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/registry.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/relation_form.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/reorder.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/schema_types.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/settings.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/singleton.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/pact/capabilities.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/party/registry.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/phrasebook/backend/lang/en/lang.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/phrasebook/backend/lang/pl/lang.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/seaglass/seaglass.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/signpost/signpost.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/scripts/check-phase17.sh
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/console/import_winter.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/controllers/api/faqs.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/controllers/faqs.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/models/faq/fields.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/models/faq.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/plugin.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/routes.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/scripts/winter-export.sql
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/updates/202610110001_create_golem15_faq_faqs.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/config/http.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/config/menu_items.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/deploy/nginx/grzybyfunkcjonalne-staging.conf
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/.gitmodules
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/go.mod
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/go.work
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/plugins.gen.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/scripts/check-deploy.sh
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/scripts/import-companions.sh
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/scripts/migrate.sh
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/scripts/smoke.sh
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/scripts/urls.txt
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/summer.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/vue-grzybyfunkcjonalne-app/app/components/FaqSection.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/vue-grzybyfunkcjonalne-app/app/components/SiteHeader.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-grzybyfunkcjonalne-app/vue-grzybyfunkcjonalne-app/nuxt.config.ts
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/classes/menu_item_types.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/go.mod
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/lang/en/lang.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/lang/pl/lang.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/menu_item_types.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/classes/links.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/classes/tree.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/console/import_winter.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/api/menus.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/helpers.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/menuentries/config_reorder.yaml
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/menuentries.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/models/menu_entry.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/models/menu.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/plugin.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/routes.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/scripts/winter-export-entries.sql
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/scripts/winter-export-menus.sql
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/updates/202610110003_add_url_pattern_to_golem15_menu_menu_entries.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/admin.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/events.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/generator.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/manifest.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/rewrite.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/serialize.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/tiers.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/classes/urlto.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/console/import_winter.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/controllers/definitions.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/plugin.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/routes.go
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/vue-faq-layer/app/components/Faq/Block.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/vue-faq-layer/app/components/Faq/Item.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/vue-faq-layer/app/components/Faq/List.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/vue-menu-layer/app/components/Menu/NavItem.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/vue-menu-layer/app/components/Menu/Nav.vue
|
||||
- /media/nvme/dev/golem15/summercms.io/summercms/vue-menu-layer/app/utils/menuLinks.ts
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 7
|
||||
info: 9
|
||||
total: 17
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 17: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-10-12T12:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 111
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
I reviewed the framework pieces (seaglass, signpost, the pact contracts, cabana richeditor/repeater/dependsOn/reorder/singleton), the admin SPA, the four plugins (Journal menu item types, sitemap, menu, FAQ), the host app's wiring and scripts, and the two Nuxt layers. Paths outside `summercms.go` are given relative to `/media/nvme/dev/golem15/summercms.io/summercms/`.
|
||||
|
||||
Most of the security work holds up under tracing:
|
||||
- The seaglass byte-stable shortcut only returns the raw input when it tokenizes the same as the bluemonday output. I could not find a bypass with entity-encoded schemes, attributes on end tags, raw-text elements or slash-separated attributes.
|
||||
- The admin `displayHtml` unwraps unknown elements, so the text of raw-text elements is re-escaped.
|
||||
- The FAQ API sanitizes on output, so imported rows are served clean.
|
||||
- The menu API and the menu layer both apply the scheme allowlist.
|
||||
- The reorder save locks the scoped set and refuses a set mismatch, cycles and foreign parents.
|
||||
- The shell scripts quote their variables and validate their arguments.
|
||||
|
||||
**The one critical gap is the sitemap.** It never checks URL schemes. A `javascript:` URL that passes Laravel's `isValidUrl` port is written verbatim into `<loc>`, and the stylesheet renders `<loc>` as a clickable `<a href>`. That page is on the public site origin, which is also the origin of the cookie-authenticated admin.
|
||||
|
||||
The warnings cover:
|
||||
- Menu tree integrity when an entry changes menu.
|
||||
- A save race on the reorder page.
|
||||
- Registry entries saved without a reference, which then vanish silently.
|
||||
- Draft posts exposed through `journal-post`.
|
||||
- An unthrottled, expensive anonymous `/sitemap.xml`.
|
||||
- The menu importer skipping the rules that admin saves enforce.
|
||||
- A broken new-tab path for singleton settings links.
|
||||
|
||||
Documented deferred items (for example protocol-relative menu URLs, phrase keys in hook validation errors, the translate cookie) are not repeated here.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Sitemap writes admin-supplied `javascript:` URLs into `<loc>`, which sitemap.xsl renders as a clickable link on the public origin (stored XSS)
|
||||
|
||||
**File:**
|
||||
- `sm-sitemap-plugin/classes/urlto.go:17-71`: `validURLPrefix` and `filterValidateURL`.
|
||||
- `sm-sitemap-plugin/classes/generator.go:204-205, 231`: `addItem`.
|
||||
- `sm-sitemap-plugin/models/definition/fields.yaml`: the `url` and `urlPattern` row fields.
|
||||
- `sm-sitemap-plugin/controllers/definitions.go`: no save hook.
|
||||
- `summercms.go/modules/signpost/signpost.go:156-159`: the item pattern override is unvalidated.
|
||||
|
||||
**Issue:**
|
||||
- **Nothing checks the scheme.** A sitemap `url` item is stored as typed; the definitions controller has no `FormBefore*` hook and the repeater only coerces strings.
|
||||
- **The URL survives unchanged.** `URLTo` keeps any value `filterValidateURL` accepts: any scheme followed by a non-empty host. I ran the shipped `urlto.go` on `javascript://%0aalert(1)%0a//x` and it came back byte-identical. Browsers execute that URL: `//` starts a JS comment, `%0a` ends it, `alert(1)` runs and the trailing `//x` is a comment.
|
||||
- **It is written raw and rendered as a link.** The URL goes unescaped (only XML-escaped) into `<loc>`. `assets/sitemap.xsl` renders `<a href="{$loc}">` (lines 92, 143 and 192 for alternates).
|
||||
- **It runs on the site origin.** nginx serves `/sitemap.xml` on the site origin, and `/gfadmin`'s cookie session lives on that origin. The CSP there is Report-Only. A click on that row by any visitor, or by a superuser, runs script with the admin session.
|
||||
- **There are other ways in.** Item `urlPattern` is passed by `signpost.Registry.Pattern` without the "starts with /" check that host overrides get, so a registry row's pattern can produce the same URL. `sitemap:import-winter` copies `url` items without any check (`console/import_winter.go:351-354`).
|
||||
- **The menu plugin is not affected.** It filters its URLs through `AllowedURL`; the sitemap has no equivalent.
|
||||
|
||||
**Fix:** Validate on save and filter on output.
|
||||
```go
|
||||
// sm-sitemap-plugin/controllers/definitions.go
|
||||
func (definitionsAdminController) FormBeforeUpdate(_ context.Context, model any) error {
|
||||
def, ok := model.(*models.Definition)
|
||||
if !ok {
|
||||
return nil
|
||||
}
|
||||
details := map[string]any{}
|
||||
for i, item := range def.Items() {
|
||||
if u := models.Text(item.URL); u != "" && !allowedSitemapURL(u) {
|
||||
details[fmt.Sprintf("data.%d.url", i)] = []string{"golem15.sitemap::lang.errors.url_scheme"}
|
||||
}
|
||||
if p := models.Text(item.URLPattern); p != "" && (!strings.HasPrefix(p, "/") || strings.HasPrefix(p, "//")) {
|
||||
details[fmt.Sprintf("data.%d.urlPattern", i)] = []string{"golem15.sitemap::lang.errors.url_pattern"}
|
||||
}
|
||||
}
|
||||
if len(details) > 0 {
|
||||
return &cabana.ValidationError{Details: details}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// allowedSitemapURL: a path, or an absolute http(s) URL; no control characters.
|
||||
func allowedSitemapURL(u string) bool {
|
||||
if strings.TrimSpace(u) != u || strings.ContainsFunc(u, func(r rune) bool { return r < 0x20 || r == 0x7f }) {
|
||||
return false
|
||||
}
|
||||
l := strings.ToLower(u)
|
||||
return (strings.HasPrefix(u, "/") && !strings.HasPrefix(u, "//")) || strings.HasPrefix(l, "http://") || strings.HasPrefix(l, "https://")
|
||||
}
|
||||
```
|
||||
Also add these:
|
||||
- In `Generator.addItem`, drop any computed `loc` whose scheme is not `http` or `https`. This covers imports and registry results.
|
||||
- Apply the same check in `sitemap:import-winter`.
|
||||
- In `signpost.Registry.Pattern`, only accept an item override that starts with a single `/`.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Moving a menu entry to another menu orphans its subtree, and a parent from another menu silently overrides the chosen menu
|
||||
|
||||
**File:**
|
||||
- `sm-menu-plugin/models/menu_entry.go:93-101`: `BeforeSave`.
|
||||
- `sm-menu-plugin/controllers/menuentries.go:274-283` and `404-436`: `RelationExtendOptionsQuery` and `checkPlacement`.
|
||||
|
||||
**Issue:**
|
||||
- **The subtree stays behind.** Changing the `menu` field of an entry that has children updates only that row's `menu_id`. Its descendants keep the old `menu_id` with a `parent_id` that now points into another menu.
|
||||
- **The public API drops them silently.** `classes.BuildTree` treats an entry whose parent is not among the menu's entries as unreachable, so the whole subtree disappears from `GET /_menu/api/v1/menus/{code}` with no error. The reorder page of the old menu shows them as roots instead, so the two views disagree.
|
||||
- **The parent wins over the chosen menu.** The `parent` dropdown offers entries of every menu. Picking a parent from menu B while the `menu` field says A makes `BeforeSave` overwrite `menu_id` with B, and the admin's menu choice is lost without a message. `checkPlacement` only checks cycles.
|
||||
|
||||
**Fix:**
|
||||
- In `FormAfterUpdate`, refuse a parent whose `menu_id` differs from the submitted menu (or scope the `parent` options to the entry's menu).
|
||||
- When `menu_id` changes, move the whole subtree inside the save transaction:
|
||||
```go
|
||||
// after the row write, inside cabana.TxFromContext(ctx)
|
||||
db.Exec(`WITH RECURSIVE sub(id) AS (
|
||||
SELECT id FROM golem15_menu_menu_entries WHERE parent_id = ?
|
||||
UNION ALL SELECT e.id FROM golem15_menu_menu_entries e JOIN sub ON e.parent_id = sub.id)
|
||||
UPDATE golem15_menu_menu_entries SET menu_id = ? WHERE id IN (SELECT id FROM sub)`, entry.ID, entry.MenuID)
|
||||
```
|
||||
|
||||
### WR-02: Reorder page sends overlapping saves; out-of-order completion leaves the database on an older tree than the screen shows
|
||||
|
||||
**File:** `summercms.go/admin/src/views/ReorderView.vue:109-143` (`save`, `scheduleSave`), `282-286` (`onPartition`)
|
||||
|
||||
**Issue:**
|
||||
- **Saves overlap.** `scheduleSave` debounces by 400 ms but never waits for an in-flight POST. Two saves can be in flight at once.
|
||||
- **The older tree can win.** The server serializes them with `FOR UPDATE`, but in arrival order, not send order. If request 1 (older tree) commits after request 2, the database keeps the older order while the UI shows the newer one. Each request carries the full node set, so neither gets a 409.
|
||||
- **A failure throws away later moves.** If the earlier save fails, `tree.value = cloneTree(savedTree)` discards moves the user made after it was sent.
|
||||
- **A partition switch can poison `savedTree`.** `flush()` starts a save for partition A and the route change then loads partition B. If A's POST resolves after B's load, `savedTree = sent` sets it to A's tree. A later failed save in B then "restores" A's nodes onto the B page.
|
||||
|
||||
**Fix:**
|
||||
- Keep only one save in flight. If a move happens while it is pending, mark the page dirty and send the latest tree once the first save settles.
|
||||
- Tie each save to a generation and the partition. Ignore a response, success or failure, whose generation is older than the current one, or whose partition differs from `partition.value`.
|
||||
```ts
|
||||
let inFlight: Promise<void> | null = null
|
||||
let pending = false
|
||||
async function save(): Promise<void> {
|
||||
if (inFlight) { pending = true; return }
|
||||
const gen = generation, part = partition.value?.value ?? null
|
||||
inFlight = doSave(gen, part).finally(() => { inFlight = null; if (pending) { pending = false; void save() } })
|
||||
}
|
||||
```
|
||||
|
||||
### WR-03: A registry menu entry or sitemap row saved without a reference passes validation and then silently disappears
|
||||
|
||||
**File:**
|
||||
- `summercms.go/modules/cabana/field_dependent.go:353-357`: `amongOptions` accepts null.
|
||||
- `sm-menu-plugin/controllers/menuentries.go:367-388`: `checkEntry`.
|
||||
- `sm-sitemap-plugin/controllers/definitions.go`.
|
||||
|
||||
**Issue:**
|
||||
- **Null passes the recheck.** It treats null as always valid ("required is checked by the rules"), and neither plugin makes `reference` required for types that take one.
|
||||
- **The entry saves cleanly.** A `journal-post`, `journal-category` or `category-journal-posts` entry saved with no reference gives no error.
|
||||
- **Then it vanishes.** At serve time, `resolvePost` and `resolveCategory` return nil for an empty reference. `BuildTree` then drops the entry and its stored children with only a warn log, and the sitemap skips the row. The editor sees a saved entry that never appears on the site.
|
||||
|
||||
**Fix:** Require a reference when the selected type declares `References`:
|
||||
```go
|
||||
if t, known := c.registry().Type(entry.Type); known && t.References != nil && models.Text(entry.Reference) == "" {
|
||||
details["reference"] = []string{"golem15.menu::lang.errors.reference_required"}
|
||||
}
|
||||
```
|
||||
Add the same per-row check (`data.<i>.reference`) to the sitemap definitions controller.
|
||||
|
||||
### WR-04: `journal-post` resolves unpublished posts, exposing draft titles and URLs on the anonymous menu API and in sitemap.xml
|
||||
|
||||
**File:** `sm-journal-plugin/classes/menu_item_types.go:187-210` (`resolvePost`, "without a published filter")
|
||||
|
||||
**Issue:**
|
||||
- **Drafts resolve.** `resolvePost` loads the post by id with no published or date filter.
|
||||
- **The exposure is public.** A menu entry or sitemap row pointing at a draft or future-dated post publishes that post's title and slug. The menu API (`GET /_menu/api/v1/menus/{code}`, anonymous, CORS `*`) returns them, and `/sitemap.xml` lists the URL for search engines. The Journal public API itself 404s on the same URL.
|
||||
- **It is easy to trigger.** `postReferences` also lists every post, drafts included, so picking a draft in the admin is easy. The doc comment presents this as PHP parity, but the leak goes through a new anonymous JSON API that PHP did not have.
|
||||
|
||||
**Fix:** In `resolvePost`, apply `publishedPosts(db)` (keep `postReferences` unfiltered if editors need to pre-wire menus), so an unpublished post resolves to nil:
|
||||
```go
|
||||
if err := publishedPosts(db).Where("id = ?", id).Limit(1).Find(&posts).Error; err != nil {
|
||||
```
|
||||
|
||||
### WR-05: `/sitemap.xml` is anonymous, unthrottled and resolves every registry item on each request
|
||||
|
||||
**File:** `sm-sitemap-plugin/routes.go:27-33`, `47-87`; `sm-sitemap-plugin/classes/generator.go:185-193`
|
||||
|
||||
**Issue:**
|
||||
- **There is no throttle.** The menu and FAQ APIs sit behind a 120/min bucket; the sitemap route is mounted outside any throttle group, and nginx has no `limit_req` for it.
|
||||
- **Each request is expensive.** It reloads the definition and settings, runs every registry `Resolve` (all published posts, all categories, translation rows, first-category joins) and re-reads the manifest file from disk. Nothing is cached.
|
||||
- **So it is an amplification target.** Any anonymous client can tie up database connections and CPU with a GET loop.
|
||||
|
||||
**Fix:**
|
||||
- Put the route behind a `surf` throttle bucket, as the menu and FAQ APIs are.
|
||||
- Cache the generated XML for a short TTL keyed by the root URL, invalidated on definition or settings save. Cache the manifest by mtime.
|
||||
|
||||
### WR-06: The menu importer skips the URL, target and same-menu-parent rules that admin saves enforce
|
||||
|
||||
**File:** `sm-menu-plugin/console/import_winter.go:237-325` (`mapEntries`), `194-204` (raw `SkipHooks` inserts)
|
||||
|
||||
**Issue:**
|
||||
- **No URL or target checks.** `mapEntries` never calls `classes.AllowedURL` or `classes.AllowedTarget`. A PHP entry with a `javascript:` URL, a URL with surrounding spaces, or a `_parent` target is copied as-is.
|
||||
- **No same-menu check.** It does not check that an entry's parent is in the same menu.
|
||||
- **The damage shows up later.** The API's `allowed()` hides a bad URL as text, so this is not XSS, but there are two consequences:
|
||||
- The imported rows break later admin edits, because `checkEntry` refuses the row's own URL on the first unrelated save.
|
||||
- A cross-menu parent makes the entry vanish from the public tree (see WR-01), even though the import reported it as kept.
|
||||
|
||||
**Fix:** Inside the `mapEntries` loop, give each of these cases a report line and drop it, or keep it with an explicit warning:
|
||||
- a URL that fails `classes.AllowedURL` (or clear it);
|
||||
- a target outside `_self`/`_blank` (normalize it to `nil`);
|
||||
- a parent whose `MenuID` differs from the entry's.
|
||||
|
||||
### WR-07: Opening a singleton settings entry in a new tab lands on a list route that answers 404
|
||||
|
||||
**File:** `summercms.go/admin/src/views/SettingsIndexView.vue:20-44, 66-86`
|
||||
|
||||
**Issue:**
|
||||
- **Only a plain left click works.** It is intercepted and resolved through `GET …/singleton`.
|
||||
- **Other clicks get a broken page.** For a middle-click, Ctrl/Cmd-click or "open in new tab", the code returns early, and the anchor's `href` is `settingsPath(entry)`: the controller's list route. For a singleton controller (the sitemap definition), `cabana.list` answers 404 through `singletonRefuses`, so the new tab shows a broken list.
|
||||
- **Every link pays an extra request.** Every non-singleton controller link now makes an extra `/singleton` request that 404s before navigating.
|
||||
|
||||
**Fix:** Carry a singleton flag on the settings entry, or resolve singleton ids when `settingsGroups` loads, so `href` is already `/{vendor}/{plugin}/{controller}/{id}`. Alternatively, have ListView redirect to the record when the list answers 404 for a schema with `singleton: true`.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: FAQ slugs and menu codes are not validated against the format the public APIs accept
|
||||
|
||||
**File:** `sm-faq-plugin/models/faq.go:272-277`, `sm-faq-plugin/controllers/api/faqs.go:22`, `sm-menu-plugin/models/menu.go:199-204`, `sm-menu-plugin/controllers/api/menus.go:21`
|
||||
**Issue:**
|
||||
- **The admin and importers accept any non-empty slug or code.** The APIs only serve `^[a-z0-9][a-z0-9\-]*$` (FAQ) and `^[A-Za-z0-9_-]+$` (menu).
|
||||
- **Such records are unreachable.** A FAQ slug with an underscore or capital letter (possible through `faq:import-winter` or a manual edit) is permanently 404 without any admin-side error.
|
||||
- **Duplicates go unnoticed.** Duplicate (slug, locale) FAQ rows are also allowed; the API silently serves the lowest id.
|
||||
|
||||
**Fix:** Add a `regex:` rule matching the API pattern, plus a (slug, locale_id) uniqueness check, to the FAQ and Menu rules, and report non-matching rows in the importers.
|
||||
|
||||
### IN-02: menu_items.yaml comment misstates the Journal default patterns
|
||||
|
||||
**File:** `sm-grzybyfunkcjonalne-app/config/menu_items.yaml:24-26`
|
||||
**Issue:** The comment says the Journal defaults are `/blog/post/:slug` and `/blog/category/:slug`. The code defaults are `/blog/:slug` and `/blog/category/:slug` (`sm-journal-plugin/classes/menu_item_types.go:44-45`), so the `journal-post` override is a no-op.
|
||||
**Fix:** Correct the comment.
|
||||
|
||||
### IN-03: The seaglass policy lets stored HTML reuse any site `class` and (from the UGC base) `id`
|
||||
|
||||
**File:** `summercms.go/modules/seaglass/seaglass.go:34`
|
||||
**Issue:**
|
||||
- **Any class is allowed.** `class` is allowed globally with any token, and bluemonday's UGC standard attributes add `id`.
|
||||
- **The site's styling and ids can be reused.** FAQ HTML rendered by `v-html` on the public site can borrow its classes (overlay or `skip-link` styles) and its ids (`main`, `faq`, the menu's ids) for UI redress or DOM clobbering by a FAQ editor.
|
||||
- **The impact is low,** since editors are trusted, but the policy is broader than the editor can produce.
|
||||
|
||||
**Fix:** Restrict `class` to a prefix (for example `^(wn-|rich-)[\w-]+$` tokens), or to the classes imported content actually uses, and drop `id` unless footnotes need it.
|
||||
|
||||
### IN-04: The list's reorder link stringifies a multi-value filter and ignores the toolbar
|
||||
|
||||
**File:** `summercms.go/admin/src/views/ListView.vue:84-93, 423-426`
|
||||
**Issue:**
|
||||
- **Multi-value filters give a bad partition.** When the menu filter holds several ids, `String(value)` yields `"1,2"`. ReorderView then discards it and opens the first partition instead of the filtered one.
|
||||
- **The toolbar is ignored.** The Reorder button is shown whenever `schema.reorder` is set, even when `toolbar.buttons` omits `reorder`.
|
||||
|
||||
**Fix:** Pass a partition only for a single scalar id. Show the button only when `toolbarButtons` includes `reorder`.
|
||||
|
||||
### IN-05: sitemap:import-winter counts an item mapped to "" as both remapped and dropped
|
||||
|
||||
**File:** `sm-sitemap-plugin/console/import_winter.go:341-349`
|
||||
**Issue:** `Remapped++` runs before the `to == ""` drop branch, so the summary over-counts.
|
||||
**Fix:** Check `to == ""` before incrementing `Remapped`, as `sm-menu-plugin/console/import_winter.go:252-256` does.
|
||||
|
||||
### IN-06: The singleton route creates a row on a GET
|
||||
|
||||
**File:** `summercms.go/modules/cabana/singleton.go:49-110`, `http.go` (route registered without `requireAjax`)
|
||||
**Issue:** `SingletonRecord` is first-or-create, so a GET has a write side effect without the CSRF/ajax gate the write routes use. The write is idempotent and only creates an empty definition, but it breaks the safe-method convention.
|
||||
**Fix:** Wrap the route with `requireAjax`, or document the exception.
|
||||
|
||||
### IN-07: The frontend_url rewrite inserts the setting unescaped into XML
|
||||
|
||||
**File:** `sm-sitemap-plugin/classes/rewrite.go:276-299`
|
||||
**Issue:** The `frontend_url` text is substituted into already-escaped `<loc>`, `href="…"` and the stylesheet PI without escaping. A value containing `&`, `<` or `"` (admin setting) produces malformed XML or breaks out of the attribute.
|
||||
**Fix:** Reject such characters on the settings save, or apply `EscapeText` and `EscapeAttr` to `frontend` per context before replacing.
|
||||
|
||||
### IN-08: A reorder sort value outside the int4 range is a 500, not a 422
|
||||
|
||||
**File:** `summercms.go/modules/cabana/reorder.go:207-211, 309-314`
|
||||
**Issue:** `AdminReorderNodeInput.SortOrder` is `int64` and is written unchecked into `INTEGER` sort columns. An out-of-range value is a Postgres error, so the client gets a logged 500.
|
||||
**Fix:** In `checkReorderNodes`, refuse `SortOrder` outside `[math.MinInt32, math.MaxInt32]` with a 422.
|
||||
|
||||
### IN-09: Repeater row errors stay at their old index after a row is moved or removed
|
||||
|
||||
**File:** `summercms.go/admin/src/components/form/fields/RepeaterField.vue:127-139, 199-245`
|
||||
**Issue:** Errors and notices are keyed `field.index.nested` and are not re-keyed or cleared when `move` or `remove` changes the row order. After a 422, moving a row shows its errors on the row that now holds that index.
|
||||
**Fix:** Call `feedback.clearError` for the affected row prefixes on move and remove, or re-key the entries alongside `keys`.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-12T12:00:00Z_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user