golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(16.2): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-10-10 16:30:51 +02:00
parent 85a12e3296
commit 9899ac61dd
1 changed files with 294 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

294
.planning/phases/16.2-plugin-provided-field-types-and-the-blog-editor-formwidget/16.2-REVIEW.md Normal file
View File

@@ -0,0 +1,294 @@
---
phase: 16.2-plugin-provided-field-types-and-the-blog-editor-formwidget
reviewed: 2026-10-10T14:29:09Z
depth: standard
files_reviewed: 73
files_reviewed_list:
- admin/src/api/schema.d.ts
- admin/src/api/types.ts
- admin/src/app/fieldTypes.ts
- admin/src/app/pluginAssets.ts
- admin/src/components/form/FieldRenderer.vue
- admin/src/components/form/FormField.vue
- admin/src/components/form/FormGrid.vue
- admin/src/components/form/PreviewField.vue
- admin/src/components/form/fields/MLField.vue
- admin/src/components/form/fields/MLPluginField.vue
- admin/src/components/form/fields/PluginField.vue
- admin/src/components/form/formContext.ts
- admin/src/components/form/formState.ts
- admin/src/components/form/mlLocale.ts
- admin/src/components/form/registry.ts
- admin/src/components/relation/RelationChildModal.vue
- admin/src/components/shell/AppShell.vue
- admin/src/main.ts
- admin/src/views/FormView.vue
- admin/src/views/LoginView.vue
- modules/cabana/admin_openapi.go
- modules/cabana/contracts.go
- modules/cabana/crud.go
- modules/cabana/extension.go
- modules/cabana/field_ml.go
- modules/cabana/field_type.go
- modules/cabana/field_type_upload.go
- modules/cabana/form_schema.go
- modules/cabana/http.go
- modules/cabana/list_schema.go
- modules/cabana/query.go
- modules/cabana/registry.go
- modules/cabana/relation.go
- modules/cabana/relation_field.go
- modules/cabana/relation_form.go
- modules/cabana/schema_types.go
- modules/cabana/settings.go
- modules/cabana/tx_context.go
- modules/pact/capabilities.go
- modules/phrasebook/backend/lang/en/lang.yaml
- modules/phrasebook/backend/lang/pl/lang.yaml
- scripts/check-phase16.2.sh
- ../sm-journal-plugin/admin.go
- ../sm-journal-plugin/assets/src/blogeditor/blogeditor.css
- ../sm-journal-plugin/assets/src/blogeditor/element.ts
- ../sm-journal-plugin/assets/src/blogeditor/embed.ts
- ../sm-journal-plugin/assets/src/blogeditor/extensions.ts
- ../sm-journal-plugin/assets/src/blogeditor/index.ts
- ../sm-journal-plugin/assets/src/blogeditor/labels.ts
- ../sm-journal-plugin/assets/src/blogeditor/preview.ts
- ../sm-journal-plugin/assets/src/blogeditor/serializer.ts
- ../sm-journal-plugin/assets/src/blogeditor/toolbar.ts
- ../sm-journal-plugin/classes/blog_editor.go
- ../sm-journal-plugin/classes/format_html.go
- ../sm-journal-plugin/classes/media_store.go
- ../sm-journal-plugin/controllers/admin_registry.go
- ../sm-journal-plugin/controllers/api/media.go
- ../sm-journal-plugin/controllers/categories.go
- ../sm-journal-plugin/controllers/posts.go
- ../sm-journal-plugin/field_types.go
- ../sm-journal-plugin/go.mod
- ../sm-journal-plugin/lang/en/lang.yaml
- ../sm-journal-plugin/lang/pl/lang.yaml
- ../sm-journal-plugin/models/category/fields.yaml
- ../sm-journal-plugin/models/post/fields.yaml
- ../sm-journal-plugin/scripts/check-dist.sh
- ../sm-journal-plugin/vite.config.ts
- ../vue-journal-layer/app/assets/css/journal-base.css
- ../vue-journal-layer/app/components/Journal/CategoryHeader.vue
- ../vue-journal-layer/app/components/Journal/PostBody.vue
- ../vue-journal-layer/app/utils/embeds.ts
- ../vue-journal-layer/app/utils/htmlGate.ts
- ../vue-journal-layer/testing/expect-acme.txt
findings:
critical: 1
warning: 8
info: 8
total: 17
status: issues_found
---
# Phase 16.2: Code Review Report
**Reviewed:** 2026-10-10T14:29:09Z
**Depth:** standard
**Files Reviewed:** 73 (summercms.go diff 0754a6e..HEAD, sm-journal-plugin diff c60abc6..HEAD, vue-journal-layer diff 294a8d8..HEAD)
**Status:** issues_found
Paths starting with `../` are relative to the summercms.go repository root; they live in the sibling sm-journal-plugin and vue-journal-layer repositories.
## Summary
The review covered the new `pact.FormFieldType` API, cabana's compile, decode, untouched-skip, load, list and upload paths, the admin SPA host (PluginField, MLPluginField, the field-type loader, notices, View site), the Journal Blog Editor types (Decode/Load/ListValue/Upload, StoreMedia, the bluemonday extension and SanitizeHTMLReport), the TipTap custom element (schema, serializer, preview, toolbar), and the layer's sanitize-html mirror and embed expansion.
The security core holds up:
- Decode runs inside the save transaction for every written locale. The untouched skip compares against the value the form was served, so it cannot be used to store unsanitized bytes that are not already stored.
- SanitizeHTMLReport returns the input unchanged only when the token comparison matches.
- The layer expands embeds only after sanitizing, with a fixed `src` and a validated id.
- The preview iframe uses an empty sandbox, and stored HTML is parsed only into inert documents.
- Upload sniffing plus StoreMedia's extension list refuses SVG and HTML.
Problems found:
- **Blocker (data loss):** Journal `Load` serves an empty editor for a locale whose `content` is blank but whose `content_html` holds the body. The public API's `FormatTranslation` path supports that state. Any edit in that locale silently replaces the hidden translated body.
- **Upload route:** it skips the controller's create/update capability check and can leave orphaned public objects. The Journal storage key check is racy, so concurrent uploads can overwrite each other.
- **Live admin DOM:** the editor's attribute preservation is a deny-list (`on*` only). `id`, `name`, `style` and `srcset` from stored HTML reach the live admin DOM, which contradicts the threat-model claim.
- Other warnings: the markdown/HTML heuristic, the translatable Decode contract, the View site URL, and a phase gate that reports success without its data-loss regression tests.
## Critical Issues
### CR-01: Blog Editor hides a locale's existing `content_html` when its `content` is blank; editing that locale overwrites the hidden body
**File:** `../sm-journal-plugin/classes/blog_editor.go:204-209`
**Issue:** `loadBlogHTML` returns the stored text as-is when it is blank:
```go
if column == "" || LooksLikeHTML(text) || strings.TrimSpace(text) == "" {
return text, nil
}
```
`FormatTranslation` (`classes/format_html.go:292-316`) supports translations that carry only `content_html` with blank or missing `content`, for example API writes of a translated body. For such a locale, cabana's `hydratePluginFields` and `servedFieldValue` serve `""`. The administrator therefore sees an empty editor although the site renders a full translated post. The untouched skip protects only a save that leaves that locale empty. As soon as the administrator types anything there, for example believing no translation exists, `decodeOne` writes the new value. `FormAfterUpdate` (`controllers/posts.go:241-280`) then stores it as that locale's `content_html`, and the existing translated body is lost with no notice. This breaks D-07's intent ("a markdown post opens with its `content_html`"). The same applies to a host row whose `content` is blank but whose `content_html` is not.
**Fix:** When `htmlColumn` is set, serve the HTML column whenever `content` is blank, not only when it is markdown:
```go
text := storedText(in.Value)
column, _ := in.Options["htmlColumn"].(string)
if column == "" || (LooksLikeHTML(text) && strings.TrimSpace(text) != "") {
return text, nil
}
if in.Translated != nil {
html, found, err := in.Translated(column)
if err != nil {
return nil, err
}
if found && html != "" {
return html, nil
}
}
if strings.TrimSpace(text) == "" {
return text, nil
}
html, err := FormatHTML(text)
...
```
Add a test: a pl translation with only `content_html` is served as that HTML, an untouched save keeps it, and an edit writes both columns from the edited value.
## Warnings
### WR-01: Field upload route skips the controller's create/update capability check
**File:** `modules/cabana/field_type_upload.go:25-36, 80-81`
**Issue:** `fieldUpload` only runs `protect`, which checks the controller's required permissions. Unlike `create` and `update` (`http.go:831, 860`), it never calls `s.operationDeclared(w, r, cc, "create"|"update")`. Consequences:
- With `{id}` 0, any administrator holding the controller permission can write files to the public media bucket through a controller whose YAML declares no create capability (no toolbar `create`).
- The `id == 0` path never applies the controller's `FormExtendQuery` scope, so an administrator scoped to a subset of records can still upload.
- With `{id} > 0`, a record the administrator may view but whose form declares no update can still receive uploads.
**Fix:** Mirror the save routes before reading the body:
```go
op := "update"
if id == 0 {
op = "create"
}
if !s.operationDeclared(w, r, cc, op) {
return
}
```
Add `TestFieldUpload` cases for a controller without the `create` toolbar button (403 for id 0).
### WR-02: Stored upload objects are orphaned when the route refuses after Upload succeeded
**File:** `modules/cabana/field_type_upload.go:58-78, 83-99`
**Issue:** The type's `Upload` writes the object to the bucket first. Only then does `run` check that no second multipart part follows (`mr.NextPart()`), and that check returns 422. In the `id != 0` case, a later transaction error rolls back the DB transaction but not the blob write. The element's 60 s timeout (`element.ts:553-556`) also drops a late successful answer. Each case leaves a public, unreferenced object under `/journal/<folder>/` with no cleanup.
**Fix:** Before calling `Upload`, check that the stream has exactly one part. For example, buffer the part up to the cap, then read `NextPart` before calling `Upload`. Alternatively, add an optional `pact.FormFieldType.Discard(ctx, url)` that cabana calls when it refuses after a successful Upload. At minimum, document that a refused upload may leave the object behind.
### WR-03: Media key uniqueness is check-then-write; concurrent uploads of the same name overwrite each other
**File:** `../sm-journal-plugin/classes/media_store.go:79-85, 122-151`
**Issue:** `uniqueMediaKey` calls `bucket.Exists` and then `bucket.NewWriter(ctx, key, nil)` writes without a precondition. Two concurrent uploads of the same name to the same folder both see the key as free, and the second write replaces the first. One post then shows the other's image. Clipboard and drag-and-drop images commonly share names such as `image.png`, and `safeUploadName` maps every nameless file to `image.<ext>`, so collisions are likely. The plan-03 fix (T-16.2-16, "never overwrites") covers only the 999-suffix exhaustion case.
**Fix:** Write with `&blob.WriterOptions{IfNotExist: true}` (available in gocloud.dev v0.46) and retry with the next suffix on a `gcerrors.FailedPrecondition`. Alternatively, add a short random component to editor upload names.
### WR-04: `LooksLikeHTML` treats markdown that starts with a tag as HTML; the first edit stores raw markdown as HTML in both columns
**File:** `../sm-journal-plugin/classes/blog_editor.go:112-115, 207`
**Issue:** A markdown post whose first non-space character is `<` is served as raw markdown in the WYSIWYG editor, not as its `content_html`. Examples: a hero `<img …>`, a leading `<div>`, or `<!-- comment -->` followed by `## Heading` and `**bold**`. Any edit makes `preparePostWrite` (`controllers/posts.go:212-224`) copy that string into `content_html`. From then on the site renders literal `##` and `**` markers. This is silent corruption on first edit; it is listed as assumption A1, but no safeguard backs it.
**Fix:** Do not decide by the first character alone. For example, treat the value as HTML only when `content_html` is empty, equals `content`, or equals `SanitizeHTML(content)`; otherwise serve `content_html`. Add a fixture with a markdown post that starts with `<img>`.
### WR-05: Attribute preservation lets `id`, `name`, `style`, `srcset` and other stored attributes into the live admin DOM
**File:** `../sm-journal-plugin/assets/src/blogeditor/extensions.ts:100-125, 133-173`
**Issue:**
- `captureAttributes` and `renderCaptured` drop only `on*` names. Every other attribute of a paragraph, heading, list, table, cell, image, figure or link, and every attribute of a `span`, is rendered by ProseMirror into the contenteditable view in the admin document. This happens on the form and also on the read-only preview screen, which renders the element with `disabled`.
- Legacy imported HTML is never sanitized by an untouched save, so its attributes reach the admin DOM unchanged:
- `<img src=… name="querySelector">` clobbers `document.querySelector` (Document named properties override built-ins) and breaks the SPA while the post is open.
- `style="position:fixed;inset:0;z-index:99999;…"` can overlay the admin UI with attacker text (UI redress).
- `srcset` and `background` load remote resources.
- The policy also allows `id` globally, so any administrator can plant window-level named elements.
- Plan 02's statement "Stored HTML never reaches the live admin DOM" is inaccurate for attributes.
**Fix:** Switch to an allow-list that mirrors the server policy: `class`, `style` (filtered to the four allowed properties, as in htmlGate), `dir`, `lang`, `title`, `itemscope`/`itemtype`/`itemprop`, `data-message-*`, plus element-specific attributes. Keep any other attribute only as an opaque value on the node, and serialize it in the inert document without rendering it in the view. One way is to render `htmlAttrs` in the view through `addNodeView` or a `renderHTML` that filters by a view-safe list, while the serializer path emits everything. At minimum, drop `id`, `name`, `form`, `srcset`, `contenteditable`, `popover*` and `style` from the view rendering.
### WR-06: Translatable Decode can receive non-string values despite the documented pact contract
**File:** `modules/cabana/field_type.go:476-486`, `modules/pact/capabilities.go` (FormFieldValue doc: "it is a string for a translatable type")
**Issue:** `liftMLValues` validates only object-shaped submissions. A scalar submitted for a translatable plugin field (`null`, `3`, `true`) passes through `projectOperation` and reaches `ft.def.Decode` as `nil`/`float64`/`bool`. The published API promises a string, so a third-party type that does `in.Value.(string)` will panic, and the save fails as an unexpected 500. Journal guards against this; the framework contract does not.
**Fix:** Enforce the contract in `decodePluginFields` before calling `decodeOne` for a translatable default value:
```go
switch v := value.(type) {
case string:
case nil:
value = ""
default:
return pluginDecodeResult{}, &ValidationError{Details: fieldDetail(field.Name, "The "+field.Name+" field must be text or an object of locale codes to text.")}
}
```
Alternatively, change the pact doc to "any JSON scalar".
### WR-07: "View site" links to the admin's own origin, which is wrong for the headless deployment
**File:** `admin/src/components/shell/AppShell.vue:25-27, 40-48`
**Issue:** `siteURL = ${window.location.origin}/` assumes the public site and the admin share an origin. v1 is a headless backend with a separate Nuxt frontend. When the admin is served from an API or admin host, the link opens the Go binary's root (the SPA shell or a 404), not the site. WinterCMS's "View site" uses the configured application URL. The link attributes themselves are safe (`noopener noreferrer`, fixed origin).
**Fix:** Serve the site URL from configuration, for example an `app.url` or `backend.site_url` key exposed in `/navigation` or `/me`. Hide the link when the key is unset, or fall back to the origin only when it is explicitly configured as the same host. Update the module README and config docs in the same change.
### WR-08: The phase gate prints "Phase 16.2 gate passed" (exit 0) without running the data-loss regression suites
**File:** `scripts/check-phase16.2.sh:521-544`
**Issue:** The header calls the script fail-closed. Yet `--all` (the default mode) exits 0 and prints "Phase 16.2 gate passed" when Docker is unreachable. The Postgres stage holds `TestPostsAdminResaveKeepsHTML`, `TestPostsAdminSaveSanitizesHTML`, `TestCategoriesAdminContent`, `TestCategoriesAdminBlogEditor` and `TestFieldTypeSavePostgres`, which are this phase's D-03/D-10 acceptance tests. Plan 06 records that this host has no Docker, so the green gate was produced without them. A CI job or a later reader that trusts the exit code will treat the phase as verified.
**Fix:** Make the default fail closed. Exit non-zero, or at least print "Phase 16.2 gate INCOMPLETE" and exit 3, when Postgres did not run, unless `PHASE162_ALLOW_NO_POSTGRES=1` is set. This inverts the current `PHASE162_REQUIRE_POSTGRES` opt-in.
## Info
### IN-01: An embed placeholder that both policies accept may never expand
**File:** `../vue-journal-layer/app/utils/embeds.ts:41`
**Issue:** `EMBED_PLACEHOLDER` matches only `data-embed` before `data-id`, double-quoted, with an empty body. A placeholder written in the Source tab as `<figure data-id="…" data-embed="youtube"></figure>` passes the Go policy and the layer sanitizer. sanitize-html keeps attribute order, so it renders as an inert empty figure with no error anywhere. The editor's Embed node accepts either order (`extensions.ts:327-341`).
**Fix:** In the layer, match either attribute order (or parse the figure's attributes after sanitizing). Alternatively, have Decode normalize the placeholder's attribute order.
### IN-02: The expanded YouTube iframe has no `sandbox`, and the exported `/g` regex carries shared `lastIndex` state
**File:** `../vue-journal-layer/app/utils/embeds.ts:41, 56-60`
**Issue:** The iframe is fixed to youtube-nocookie with a validated id, which is fine, but it has no `sandbox` attribute as defence in depth. `EMBED_PLACEHOLDER` is an exported global regex; a consumer that calls `.test()` or `.exec()` on it gets stateful results.
**Fix:** Add `sandbox="allow-scripts allow-same-origin allow-presentation allow-popups"`. Export a factory or a non-global pattern and build the `/g` regex inside `expandEmbeds`.
### IN-03: Plugin aliases can shadow list column types, and ListValue is then silently ignored
**File:** `modules/cabana/field_type.go:104-109`, `modules/cabana/list_schema.go:22-24, 242-251`
**Issue:** `checkFieldTypeDef` refuses only `formFieldTypes`. A plugin alias `date`, `datetime` or `time` is accepted at boot, but a list column of that type resolves to the built-in column type and never calls the plugin's `ListValue`.
**Fix:** Also refuse aliases in `listColumnTypes`.
### IN-04: `crypto.randomUUID()` throws outside a secure context
**File:** `../sm-journal-plugin/assets/src/blogeditor/element.ts:551`
**Issue:** On an admin served over plain HTTP from a non-localhost host (a LAN dev box), `crypto.randomUUID` is undefined and every upload throws. The status line still says "uploading".
**Fix:** Fall back to `crypto.getRandomValues` or a counter for the request id.
### IN-05: content_html translations are skipped silently when the application is unavailable
**File:** `../sm-journal-plugin/controllers/posts.go:263-270`
**Issue:** `writeContentHTMLTranslations` returns nil when `c.application()` or the writer is nil, while cabana has already written the `content` translation through its own writer. The two columns then diverge with no error.
**Fix:** Return an error, which aborts the transaction, when a non-default write exists but no writer can be resolved.
### IN-06: Untouched detection cannot be byte-exact for legacy content with invalid UTF-8
**File:** `modules/cabana/field_type.go:641-651`
**Issue:** JSON transport replaces invalid UTF-8 with U+FFFD, so the echoed value never equals the served value under `sameJSON`. An untouched save then rewrites such a row with replacement characters, which breaks D-03 for those rows.
**Fix:** Document the limitation, or compare after normalizing the served value with `strings.ToValidUTF8(s, "<22>")`.
### IN-07: "Edit as HTML" selects the first identical block
**File:** `../sm-journal-plugin/assets/src/blogeditor/element.ts:524-532`
**Issue:** `textarea.value.indexOf(html)` selects the first occurrence when two raw HTML blocks are identical, so the wrong block is highlighted.
**Fix:** Pass the node position and map it to the block's source offset through `#blocks`.
### IN-08: Plugin-typed fields cannot declare `readOnly`/`hidden`/`disabled` in YAML
**File:** `modules/cabana/field_type.go:33-36`
**Issue:** `pluginFieldKeys` excludes general keys such as `readOnly` (and `checkFieldTypeDef` forbids options with those names), but `PluginField.vue:50` honours `field.readOnly`. A plugin field can therefore never be made read-only from fields.yaml.
**Fix:** Allow the general keys the SPA and server already understand (`readOnly`, `hidden`, `placeholder`, `cssClass`) for plugin fields, or document the restriction in `docs/backend/field-types.md`.
---
_Reviewed: 2026-10-10T14:29:09Z_
_Reviewer: Claude (msd-code-reviewer)_
_Depth: standard_