From cd076b881d0251d8f4125871c8000055bf7b1de0 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 9 Oct 2026 23:22:14 +0200 Subject: [PATCH] docs(16.1): add code review report --- .../16.1-REVIEW.md | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 .planning/phases/16.1-journal-admin-urgent-fixes-relation-search-list-state-post-f/16.1-REVIEW.md diff --git a/.planning/phases/16.1-journal-admin-urgent-fixes-relation-search-list-state-post-f/16.1-REVIEW.md b/.planning/phases/16.1-journal-admin-urgent-fixes-relation-search-list-state-post-f/16.1-REVIEW.md new file mode 100644 index 0000000..7a15425 --- /dev/null +++ b/.planning/phases/16.1-journal-admin-urgent-fixes-relation-search-list-state-post-f/16.1-REVIEW.md @@ -0,0 +1,168 @@ +--- +phase: 16.1-journal-admin-urgent-fixes-relation-search-list-state-post-f +reviewed: 2026-10-09T21:20:45Z +depth: standard +files_reviewed: 22 +files_reviewed_list: + - admin/src/state/listState.ts + - admin/src/state/useAuth.ts + - admin/src/state/useSidebar.ts + - admin/src/views/ListView.vue + - admin/tests/form/FormView.test.ts + - admin/tests/list/ListView.test.ts + - admin/tests/smoke/list.smoke.test.ts + - admin/tests/state/listState.test.ts + - admin/tests/state/useAuth.test.ts + - docs/backend/forms.md + - docs/backend/lists-and-filters.md + - modules/cabana/README.md + - modules/cabana/http.go + - modules/cabana/locale_order_test.go + - modules/cabana/query.go + - modules/cabana/query_relation_test.go + - scripts/check-phase10.sh + - ../sm-journal-plugin/README.md + - ../sm-journal-plugin/classes/format_html.go + - ../sm-journal-plugin/classes/format_html_test.go + - ../sm-journal-plugin/go.mod + - ../sm-journal-plugin/posts_admin_smoke_test.go +findings: + critical: 1 + warning: 5 + info: 3 + total: 9 +status: issues_found +--- + +# Phase 16.1: Code Review Report + +**Reviewed:** 2026-10-09T21:20:45Z +**Depth:** standard +**Files Reviewed:** 22 +**Status:** issues_found + +## Narrative Findings (AI reviewer) + +## Summary + +I reviewed the phase diffs: summercms.go `1ef21b0^..HEAD` and sm-journal-plugin `3d533d4..HEAD`. I checked the relation subquery SQL in `modules/cabana/query.go` against GORM's relationship model. Table, pivot and column names come only from the parsed schema, every identifier goes through `quotedIdent`/`qualifiedColumn`, and the search term is a bound parameter. I found no SQL injection path. The EXISTS correlation is correct for belongsTo, hasOne, hasMany and belongsToMany, including self-referencing relations. The `enabledContentLocales` reorder is correct, and so is the list-state key and validation logic. + +The blocking problem is in the journal plugin. Turning on `html.WithUnsafe()` makes the three-regex `rejectUnsafe` blocklist the only defence against stored XSS. That blocklist can be bypassed with ordinary HTML-parser behaviour, and I confirmed this by running the same goldmark config and regexes. The public Nuxt layer (`vue-journal-layer/app/utils/htmlGate.ts`) uses the same three regexes word for word, so it adds no defence in depth. Before this phase, goldmark replaced these payloads with ``. They now reach `content_html` and the public `v-html` sinks. + +## Critical Issues + +### CR-01: The rejectUnsafe gate can be bypassed now that raw HTML passes, so stored XSS reaches content_html (T-16.1-02 not met) + +**File:** `../sm-journal-plugin/classes/format_html.go:22,25-27,54-66` +**Issue:** With `goldmark.WithRendererOptions(html.WithUnsafe())`, goldmark copies raw HTML blocks verbatim. Inside a type-6 HTML block, which is any block starting with `
` or `

`, the content does not even have to be valid CommonMark tag syntax. With unsafe mode on, goldmark also stops applying its own `IsDangerousURL` filter. The remaining gate is a substring blocklist, and these payloads get past it. I verified each one against goldmark v1.8.6 with the exact engine options and regexes; every one returns `err == nil`: + +| Source passed to `FormatHTML` | Output (unchanged) | Why the gate misses it | +|---|---|---| +| `

` | same | `\son[a-z]+\s*=` needs whitespace before `on`; browsers accept a quoted value followed directly by the next attribute. Runs on page load. | +| `
\n\n
` | same | `/` is a valid attribute separator in HTML. Runs on page load. | +| `x` | same | The browser decodes the entity before parsing the URL. | +| `x` | same | Named entity. | +| `x` | same | Entity inside the scheme. | +| `x` | same | The URL parser strips tab and newline. | +| `
\nx\n
` | same | Same issue in the MathML/SVG namespace. | + +Non-script tags that are just as dangerous also pass: `` rewrites every relative URL on the page, `` is an open redirect, and `
` is a credential phish. The plan accepted only "inert tags such as style, footer or form" as residual risk, not executable script. + +**Impact:** All three write paths call `FormatHTML`: admin save at `controllers/posts.go:213`, `journal:import-posts` at `console/import_posts.go:185`, and the Journal API writes at `controllers/api/posts_helpers.go:511,567,768`. The result is served as `content_html` and rendered with `v-html` behind `gateJournalHtml`, which uses the same regexes. So any admin with the posts permission, or any imported file, can plant script that runs for every public visitor. If the admin SPA and the public site share an origin, it also runs for any signed-in superuser who opens the post. The admin cookie is HttpOnly but is still sent on same-origin API calls, so this is an escalation from author to full admin. `CheckHTML` for category `content` (`controllers/categories.go:75`) shares the same weak gate. That part predates this phase, but it is the same root cause. + +The README sentence added in this phase ("its gate still refuses ... event handlers and javascript, vbscript and data URLs on every write path") and the T-16.1-02 mitigation claim are both untrue as written. + +**Fix:** Replace the regex blocklist with a parser-based allowlist. Following the project's preference for established libraries, add `github.com/microcosm-cc/bluemonday` with a decision note and sanitize the rendered output rather than refusing it: +```go +var postPolicy = func() *bluemonday.Policy { + p := bluemonday.UGCPolicy() // drops script/iframe/object/embed/base/meta/form, on*, js/vbscript URLs + p.AllowAttrs("class").Globally() // keep imported class attributes (D-03 round-trip) + p.AllowElements("footer", "figure", "figcaption") + return p +}() + +func FormatHTML(src string) (string, error) { + var buf bytes.Buffer + if err := markdownEngine.Convert([]byte(strings.TrimSpace(src)), &buf); err != nil { + return "", fmt.Errorf("journal: markdown: %w", err) + } + return postPolicy.Sanitize(buf.String()), nil +} +``` +If a new dependency is not acceptable, use the parser already in the build: `golang.org/x/net/html`, at v0.58.0 in go.sum. Tokenize the output and refuse it when a start tag is not in an allowlist, when an attribute name starts with `on`, or when an `href`/`src`/`xlink:href`/`action`/`formaction` value, after entity decoding (the tokenizer does that) and after removing ASCII whitespace and control characters, starts with a scheme other than `http`, `https`, `mailto` or a relative path. Also port the fix to `vue-journal-layer/app/utils/htmlGate.ts`, for example DOMPurify on the client and server render, so the public layer actually provides defence in depth. Add every payload from the table above to `TestFormatHTMLRejectsUnsafeHTML`. + +## Warnings + +### WR-01: `data:` blocklist regex refuses ordinary Polish text "Data:" (and "metadata:"), so imports and saves fail + +**File:** `../sm-journal-plugin/classes/format_html.go:27` +**Issue:** `(?i)(?:javascript|vbscript|data):` has no word boundary and no attribute context, so it matches body text. In Polish, "Data:" is the usual label for a date ("Data: 12 maja 2024", "Data publikacji:"). I confirmed that `FormatHTML("

Data: 12 maja 2024

")` is refused, and so is any English text containing "metadata:" or "big data:". The admin save returns 422 on `content` (`controllers/posts.go:213-217`), and `journal:import-posts` rejects the post. This works against D-03's goal that imported HTML bodies survive a save. The regex existed before this phase, but the phase now routes all raw imported HTML through it. +**Fix:** Check URL schemes only inside URL-bearing attribute values. The parser-based gate from CR-01 does this. As a stop-gap, anchor the regex to an attribute value, for example `(?i)(?:href|src|action|formaction|xlink:href)\s*=\s*["']?\s*(?:javascript|vbscript|data):`. Treat that only as a stop-gap, since CR-01 shows the regex approach is not sufficient on its own. + +### WR-02: A stored list query that fails deterministically (non-422) locks the list until logout; on 422 the view shows only an error + +**File:** `admin/src/views/ListView.vue:141-150,165-184,420-424` +**Issue:** +1. Only a 422 clears the stored state. If a stored sort or filter makes the server fail deterministically with a 500, for example a relation sort whose `db.Joins(field)` collides with a join already added by `ListExtendQuery`, or a `relationSubquery` parse error, the list shows the `load_failed` alert. While `failed` is true, `DataTable`, including the sortable column headers, is not rendered (line 420 `v-if` / line 423 `v-else`), so the admin cannot clear the sort. Opening the bare list URL or the sidebar link restores the same stored query (line 171-176). The list stays broken until logout or until local storage is cleared by hand. +2. On a 422, storage is cleared, but the URL still holds the stale parameters and the user sees only the error alert. A restore that the user never asked for turns into a failed screen, and the user has to leave and come back to recover. +**Fix:** If the first load after a restore fails for any reason, drop the stored query and load the unfiltered list, rather than only clearing storage on a 422: +```ts +let restored = false +// in the watch, when restoring: +restored = true +void router.replace({ path: route.path, query: toListQuery(stored) }) + +// in loadList, after the generation check: +if (!result.data && (restored || result.response?.status === 422)) { + restored = false + writeListState(controllerId, {}) + void router.replace({ path: route.path, query: {} }) + return +} +restored = false +``` +Alternatively, keep `DataTable` headers rendered on failure so the sort can be reset. + +### WR-03: Stored list queries survive session expiry and are not scoped to an admin + +**File:** `admin/src/state/useAuth.ts:91-101`, `admin/main.ts:25-31`, `admin/src/state/listState.ts:24-26` +**Issue:** The key is `summer-admin.list:`, with no admin id. Only an explicit `logout()` calls `clearListStates()`. When the session ends any other way (token expiry or refresh failure goes through `onUnauthorized` → `clearUser()` in `main.ts:25-31`, or the user simply closes the tab), the stored searches stay. The next admin who signs in on that browser gets them restored into the URL and sent to the server. Search terms often hold personal data such as customer names or emails, and T-16.1-03 claims "logout clears them" as the mitigation. +**Fix:** Call `clearListStates()` from `clearUser()` so every session end clears them, or prefix the key with the signed-in admin's id (`summer-admin.list::`) and drop keys for other ids on login. + +### WR-04: The new relation SQL is only checked as DryRun strings and never runs against Postgres + +**File:** `modules/cabana/query_relation_test.go:133-146` (all relation tests); `modules/cabana/query.go:310-450` +**Issue:** Every new test opens `openDryRunDB` with `DryRun: true` and asserts on the SQL text. None of these constructs are ever executed: the correlated `EXISTS (SELECT 1 FROM ... JOIN ... ON ... WHERE ... AND LOWER(CAST(... AS TEXT)) LIKE ? ESCAPE '\')`, the `ORDER BY (SELECT string_agg(CAST(..) AS TEXT), ', ' ORDER BY CAST(..) AS TEXT)) FROM ... )` raw order column merged with the primary-key tiebreak, and the pivot alias correlation. The journal plugin has a testcontainers harness (`admin_harness_test.go`), but no test there runs a relation search or a to-many sort. A typo in operator precedence, alias scoping or the escape literal would pass CI and fail only in production. +**Fix:** Add a testcontainers-gated test in cabana, or in the journal admin harness. It should seed posts with zero, one and two categories (plus a soft-deleted category), run `?search=` on a belongsToMany, a hasMany and a belongsTo column, and run `?sort=categories&dir=desc`. Assert the returned ids and that `meta.total` equals the number of distinct posts. + +### WR-05: to-many sort key does not equal the displayed text, contradicting the code comment and docs + +**File:** `modules/cabana/query.go:314-324`, `modules/cabana/query.go:734-757`, `docs/backend/lists-and-filters.md:62` +**Issue:** The sort uses `string_agg(value, ', ' ORDER BY value)`, which sorts the values alphabetically. The displayed cell (`relatedSelectMany`) joins the preloaded records in whatever order `Preload` returns them, which has no ORDER BY (in practice pivot or primary-key order). A post with categories inserted as "Zebra", "Apple" shows "Zebra, Apple" but sorts as "Apple, Zebra", so the rows look out of order. The comment ("string_agg in the same order is the comma-joined value the list displays") and the docs sentence ("sorts by its comma-joined values, the text the list shows") are both wrong. +**Fix:** Make the display match the sort by sorting `values` in `relatedSelectMany` with `slices.Sort(values)` before joining. Alternatively, preload with the same order (`q.Preload(field, func(db *gorm.DB) *gorm.DB { return db.Order(column) })`) and correct the docs. Either way, add a test with related values in non-alphabetical order. + +## Info + +### IN-01: NULL placement for relation sorts differs from Winter + +**File:** `modules/cabana/query.go:318-332` +**Issue:** A record with no related rows sorts on NULL. PostgreSQL puts NULLs last for ASC and first for DESC, while Winter on MySQL does the opposite. Admin users will see records without categories at the opposite end from the PHP admin. +**Fix:** If parity matters, append `NULLS FIRST` for ASC and `NULLS LAST` for DESC to the raw order expression and to the to-one joined column. + +### IN-02: Relation search and sort validity is checked per request, not at compile time + +**File:** `modules/cabana/query.go:372-389,483-490` +**Issue:** A `searchable: true` column on a polymorphic relation, or with a `select` the related model lacks, compiles at boot but makes every search answer 422 ("is not a searchable relation column"). Combined with WR-02, the list-state 422 handling then clears storage each time. This is a misconfiguration the admin cannot fix and that only shows up at runtime. +**Fix:** Call `relationSubquery` for every searchable or sortable relation column during `CompileList`, or in the service constructor once the model is known, and fail boot, as the project already does for unknown column types. + +### IN-03: Two sources of truth for the main table name + +**File:** `modules/cabana/query.go:389` (`stmt.Schema.Table`) vs `modules/cabana/query.go:298,470,633-654` (`tableName(model)`) +**Issue:** The relation subquery correlates on GORM's parsed table name, but plain search columns, filters and the primary-key tiebreak qualify with `tableName(model)`. For a model without `TableName()`, that returns `""`, and `quotedIdent(db, "")` produces `""."col"`, which is invalid SQL. This predates the phase, but the new code now mixes the two sources in one statement. +**Fix:** Parse the model schema once in `ExecuteList` and pass `stmt.Schema.Table` to `applyListSearch`, `applyListFilters` and `applyListOrder`. + +--- + +_Reviewed: 2026-10-09T21:20:45Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_