From 9f6b5a5706623eb022b06009a513f14d3cece103 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Tue, 29 Sep 2026 03:29:41 +0200 Subject: [PATCH] docs(10.1): add code review report --- .../10.1-REVIEW.md | 327 ++++++++++++++++++ 1 file changed, 327 insertions(+) create mode 100644 .planning/phases/10.1-runtime-admin-extension-point/10.1-REVIEW.md diff --git a/.planning/phases/10.1-runtime-admin-extension-point/10.1-REVIEW.md b/.planning/phases/10.1-runtime-admin-extension-point/10.1-REVIEW.md new file mode 100644 index 0000000..2051e79 --- /dev/null +++ b/.planning/phases/10.1-runtime-admin-extension-point/10.1-REVIEW.md @@ -0,0 +1,327 @@ +--- +phase: 10.1-runtime-admin-extension-point +reviewed: 2026-09-29T01:27:56Z +depth: standard +files_reviewed: 99 +files_reviewed_list: + - admin/openapi/admin.json + - admin/src/api/schema.d.ts + - admin/src/api/types.ts + - admin/src/app/pluginAssets.ts + - admin/src/components/form/fields/PartialField.vue + - admin/src/components/form/fields/WidgetField.vue + - admin/src/components/form/formContext.ts + - admin/src/components/form/FormField.vue + - admin/src/components/form/registry.ts + - admin/src/components/list/ListToolbar.vue + - admin/src/components/partial/PartialHost.vue + - admin/src/components/partial/partialNodes.ts + - admin/src/components/ui/ExtensionFailure.vue + - admin/src/styles/main.css + - admin/src/views/FormView.vue + - admin/src/views/ListView.vue + - admin/tests/app/pluginAssets.test.ts + - admin/tests/fixtures/extension.form-schema.json + - admin/tests/fixtures/extension.list-schema.json + - admin/tests/fixtures/extension.partial.json + - admin/tests/fixtures/lang.json + - admin/tests/fixtures/settings.json + - admin/tests/fixtures/typed.ts + - admin/tests/fixtures/widgets.form-schema.json + - admin/tests/fixtures/widgets.list-schema.json + - admin/tests/form/FormField.test.ts + - admin/tests/form/formState.test.ts + - admin/tests/form/FormView.test.ts + - admin/tests/form/PartialField.test.ts + - admin/tests/form/registry.test.ts + - admin/tests/form/WidgetField.test.ts + - admin/tests/list/ListToolbar.test.ts + - admin/tests/list/ListView.test.ts + - admin/tests/list/PartialHost.test.ts + - admin/tests/smoke/extension.smoke.test.ts + - admin/vite.config.ts + - ../fonoteka.go/plugins/golem15/fonoteka/admin_albums_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin_collections_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin_phase09_security_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin_phase101_albums_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin_phase101_smoke_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin_phase10_controllers_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/admin_phase10_copy_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/assets/css/albums.css + - ../fonoteka.go/plugins/golem15/fonoteka/assets/js/discogs-lookup.js + - ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums_admin_controller.go + - ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums/config_list.yaml + - ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums/_stats.htm + - ../fonoteka.go/plugins/golem15/fonoteka/lang/en/lang.yaml + - ../fonoteka.go/plugins/golem15/fonoteka/lang/pl/lang.yaml + - ../fonoteka.go/plugins/golem15/fonoteka/models/album/fields.yaml + - go.mod + - modules/boardwalk/boardwalk.go + - modules/boardwalk/boardwalk_test.go + - modules/boardwalk/README.md + - modules/cabana/actions.go + - modules/cabana/admin_openapi.go + - modules/cabana/contracts.go + - modules/cabana/extension.go + - modules/cabana/form_schema.go + - modules/cabana/form_schema_test.go + - modules/cabana/http.go + - modules/cabana/list_schema.go + - modules/cabana/list_schema_test.go + - modules/cabana/messages.go + - modules/cabana/openapi_conformance_test.go + - modules/cabana/partial_render.go + - modules/cabana/phase101_actions_test.go + - modules/cabana/phase101_assets_test.go + - modules/cabana/phase101_render_test.go + - modules/cabana/phase101_schema_test.go + - modules/cabana/phase10_coverage_test.go + - modules/cabana/phase10_csrf_test.go + - modules/cabana/plugin_assets.go + - modules/cabana/README.md + - modules/cabana/registry.go + - modules/cabana/schema_types.go + - modules/cabana/security_coverage_test.go + - modules/cabana/settings.go + - modules/cabana/testdata/extension/assets/css/gadgets.css + - modules/cabana/testdata/extension/assets/js/lookup.js + - modules/cabana/testdata/extension/controllers/gadgets/config_form.yaml + - modules/cabana/testdata/extension/controllers/gadgets/config_list.yaml + - modules/cabana/testdata/extension/controllers/gadgets/_stats.htm + - modules/cabana/testdata/extension/controllers/gadgets/_summary.htm + - modules/cabana/testdata/extension/lang/en/lang.yaml + - modules/cabana/testdata/extension/lang/pl/lang.yaml + - modules/cabana/testdata/extension/models/gadget/columns.yaml + - modules/cabana/testdata/extension/models/gadget/fields.yaml + - modules/lagoon/fill.go + - modules/lagoon/fill_test.go + - modules/lagoon/README.md + - modules/pact/capabilities.go + - modules/pact/README.md + - modules/phrasebook/backend/lang/en/lang.yaml + - modules/phrasebook/backend/lang/pl/lang.yaml + - scripts/check-phase10.1.sh + - scripts/check-phase10.sh +findings: + critical: 1 + warning: 6 + info: 7 + total: 14 +status: issues_found +--- + +# Phase 10.1: Code Review Report + +**Reviewed:** 2026-09-29T01:27:56Z +**Depth:** standard +**Files Reviewed:** 99 +**Status:** issues_found + +## Summary + +I read every production file in scope in full: the cabana extension, actions, assets, partial renderer and schema compilers, the route table, pact, boardwalk, lagoon, the SPA loader, partial host, widget, views and toolbar, the fonoteka Albums controller, template and JS, and both gate scripts. Tests and fixtures were only checked for reliability problems. I also traced the callers each change depends on: `CRUDService.save` for `lagoon.Fill`, `writeCRUDError` and `writeJSON`, the router `Where` constraints, and `requireAjax`/`csrfSafe`. + +The main security controls hold up: + +- **Plugin assets.** Lookup is by exact key, and a miss falls through to boardwalk, which runs `path.Clean`. I checked with a real `net/http.ServeMux` probe: `..%2f` and `%2f` segments unescape into `{file...}` as `../../x.js`, which matches no key. +- **Partial sanitizer.** The server allowlist, the client `h()` rebuild and the one-leading-slash URL rule are consistent. No scheme or protocol-relative bypass was found. +- **CSRF.** Both new POST routes go through `requireAjax`. +- **Record scoping.** Record ids are scoped through `FormExtendQuery`. + +The defects are elsewhere: + +- **CR-01.** The new `lagoon.Fill` `json.Number` conversion turns ordinary admin input (a fractional or exponent number in an integer column, such as the new Albums `year` field) into an HTTP 500 instead of a 422. +- **Stylesheet isolation (T-10.1-16).** The SPA's per-controller stylesheet isolation can be switched to the wrong controller by a late response, and it never deactivates on non-controller views. +- **View-model guard (T-10.1-09).** The guard is much shallower than the security review states. +- **Fill values.** The scalar check on action fills trusts `reflect.Kind` rather than what the value actually encodes to. + +## Critical Issues + +### CR-01: A fractional, exponent or overflowing number for an integer column is a 500, not a validation error + +**File:** `modules/lagoon/fill.go:163-203`, `modules/cabana/crud.go:327-329`, `admin/src/components/form/fields/NumberField.vue:12-20` +**Issue:** `convertNumber` correctly refuses `json.Number("1977.5")`, `"1e+21"` and out-of-range values for int and uint destinations. However, the only admin save path, `CRUDService.save`, maps every `lagoon.Fill` error to `&CapabilityError{}`, and `writeCRUDError` answers that with a generic 500. `CapabilityError.Error()` also logs the misleading "missing Fill/Validate capability". + +The SPA's `NumberField` sends `Number(raw)` with `inputmode="decimal"`, so typing `1977.5` into the Albums `year` field this phase added produces a "server error" toast with no field message. The Album `between:1889,2100` rule never runs. The same applies to a string sent for an integer column, for example when a widget action returns `"year": "1977"`, which becomes more likely once Phase 14 maps Discogs data. + +The settings path (`settings.go:149`) already maps the same failure to a 422, so the two write paths now disagree. The commit message for c3efbc3 says "a fraction or an overflow is an error", but the error it produces is the wrong kind. +**Fix:** Have `Fill` return a typed, per-key conversion error and map it to a 422 on the field: +```go +// lagoon +type FillTypeError struct{ Key string; Err error } +func (e *FillTypeError) Error() string { return "lagoon: fill " + e.Key + ": " + e.Err.Error() } +// in Fill: +if err := setField(field, val); err != nil { + return &FillTypeError{Key: key, Err: err} +} + +// cabana/crud.go save(): +if err := lagoon.Fill(target, fillAllowed(cc, target, op), projected, false); err != nil { + var typed *lagoon.FillTypeError + if errors.As(err, &typed) { + return &ValidationError{Details: map[string]any{typed.Key: []string{"The " + typed.Key + " field has an invalid value."}}} + } + return &CapabilityError{ControllerID: controllerID(cc)} +} +``` +Add a cabana test that PUTs `{"year": 1977.5}` and `{"year": 1e21}` and expects a 422 with `details.year`. + +## Warnings + +### WR-01: A late schema response re-activates the previous controller's stylesheets over the current view + +**File:** `admin/src/views/ListView.vue:123-129`, `admin/src/views/FormView.vue:146-159`, `admin/src/app/pluginAssets.ts:95-115` +**Issue:** Both views call `loadControllerAssets(controllerId, …)` after `await api.GET(schema)` without checking that the view is still mounted. `loadControllerAssets` calls `activateStyles(controllerId)`, which disables every link not owned by that controller. + +Sequence: open list A, then navigate quickly to list B. B's schema resolves first and activates B's styles. A's slower response then resolves on the unmounted A instance, enables A's CSS and disables B's while B is on screen. That defeats the T-10.1-16 mitigation ("plugin CSS never styles a view it was not written for"). +**Fix:** Guard on unmount or on a generation counter before touching global asset state: +```ts +let alive = true +onBeforeUnmount(() => { alive = false }) +// in load(): +if (schema.value && alive) { + void loadControllerAssets(controllerId, schema.value.assets) +} +``` +Alternatively, make `activateStyles` take the current route's controller id from the router rather than from the caller. + +### WR-02: Plugin stylesheets stay enabled on views that are not controller views + +**File:** `admin/src/app/pluginAssets.ts:94-99` (callers: only `ListView.vue:128` and `FormView.vue:159`) +**Issue:** `activateStyles` runs only when a list or form schema arrives. `SettingsFormView`, `SettingsIndexView`, `NotFoundView` and `LoginView` (after logout) never deactivate anything. The last controller's plugin CSS, which is global and unscoped, keeps styling those screens. + +The same happens while a new controller's schema is loading, and permanently if its schema request fails (`schemaFailed`), because `loadControllerAssets` is then never called. The security review's residual-risk text ("while its controller is open") does not match this behaviour. +**Fix:** Deactivate on every route change and re-enable only when a controller view resolves: +```ts +// router.ts +router.beforeEach(() => { activateStyles('') }) // '' owns nothing -> every plugin link disabled +``` +Add a test that navigating to settings disables all `[data-summer-controller]` links. + +### WR-03: The partial view-model guard (T-10.1-09) is shallow and easy to bypass + +**File:** `modules/cabana/partial_render.go:264-316` +**Issue:** `refusedViewModel` compares only the top-level `baseType(vm)` with the controller's `NewRecord()` type. It inspects struct field types only when looking for trusted template types. None of the following are caught, and each passes silently: +- `struct{ Album *models.Album }` or `map[string]any{"album": album}`, where the raw GORM model is nested inside a wrapper. +- Any other GORM model, for example a backend user row, since only the controller's own model type is compared. +- A method on the view model that returns `template.HTML`. html/template calls methods such as `{{ .Data.Banner }}`, and their `template.HTML` result is emitted unescaped. `carriesTrustedContent` walks fields and never method sets. + +The last case is the dangerous one. The sanitizer still applies, but it permits `class` on every element and same-origin `a[href]`. If a method passes record data through, a value that non-admin collection users write through the Nuxt app or the MCP server can inject allowlisted markup into the admin: overlay links using the admin's own utility classes (`fixed inset-0 z-50`), or a link to any same-origin path. The security review marks T-10.1-09 as mitigated and says the guard refuses the model "at the type level". That does not hold for these cases. +**Fix:** Recurse the model check through struct fields, pointers, slices and maps. Refuse any type implementing `schema.Tabler` or carrying `gorm.Model`/`gorm.DeletedAt`, rather than only the controller's model. Refuse types whose method set, for exported methods with no arguments, returns a trusted template type: +```go +func refusedType(t, model reflect.Type, seen map[reflect.Type]bool) bool { + t = baseType(t) + if t == nil || seen[t] { return false } + seen[t] = true + if t == model || trustedTemplateTypes[t] { return true } + for _, tt := range []reflect.Type{t, reflect.PointerTo(t)} { + for i := 0; i < tt.NumMethod(); i++ { + if m := tt.Method(i).Type; m.NumOut() >= 1 && trustedTemplateTypes[m.Out(0)] { return true } + } + } + if t.Kind() == reflect.Struct { + for i := 0; i < t.NumField(); i++ { + if refusedType(t.Field(i).Type, model, seen) { return true } + } + } + return false +} +``` +Also consider limiting `class` values on partial nodes to a `summer-` prefix on both server and client, so partial content cannot reuse the admin's layout utilities. + +### WR-04: `isJSONScalar` trusts `reflect.Kind`, not the JSON the value encodes to + +**File:** `modules/cabana/actions.go:207-243`, `modules/cabana/contracts.go:190-194` +**Issue:** `onlyFillScalars` accepts any value whose kind is string, bool or numeric. Two cases break the "fill holds only scalars" contract: +1. A named scalar type with a `MarshalJSON` method, for example `type Tags string` that marshals to `["a","b"]`, passes the check but encodes as an array or object. The SPA then patches a non-scalar into a scalar field. +2. `math.NaN()` or `±Inf` in `result.Fill`, for example a Phase 14 Discogs price computation, passes as `reflect.Float64`. `writeJSON` has already called `WriteHeader(200)` before `json.Encoder.Encode` fails, so the client receives a 200 with an empty or truncated body. openapi-fetch then throws, and the admin sees a generic failure with nothing logged. +**Fix:** Decide scalar-ness by encoding each value: +```go +func isJSONScalar(v any) bool { + raw, err := json.Marshal(v) + if err != nil || len(raw) == 0 { + return false + } + switch raw[0] { + case '{', '[': + return false + } + return true +} +``` +Also marshal the envelope into a buffer before `WriteHeader` in `writeJSON`, so an encode failure becomes a logged 500. + +### WR-05: Form schema shows widgets the admin is not allowed to run + +**File:** `modules/cabana/http.go:511-528` (compare with `listSchema` at `http.go:542-551`) +**Issue:** `listSchema` removes `toolbarActions` the principal may not run. `formSchema` sends every `type: widget` field unchanged, even when the principal fails `action.Permissions`. An admin holding the controller permission but not the action permission gets a working-looking button that always returns 403 with a "forbidden" toast. This is the same D-12 permission filtering the toolbar applies, applied inconsistently. It also reveals the name of an action the admin cannot use. +**Fix:** In `formSchema`, after `Localize`, drop or disable widget fields whose `cc.Actions[field.Action].Permissions` the principal fails: +```go +principal, _ := bouncer.User(r.Context()) +kept := view.Fields[:0] +for _, f := range view.Fields { + if f.Type == "widget" && !Allows(principal, cc.Actions[f.Action].Permissions) { + continue + } + kept = append(kept, f) +} +view.Fields = kept +``` +Add a permission-matrix case asserting that the widget is absent from the form schema. + +### WR-06: The widget action route ignores the field's `context` + +**File:** `modules/cabana/actions.go:24-62`, `widgetField` at `actions.go:148-158` +**Issue:** The SPA hides a widget declared with `context: update` on the create form (`FormView.vue` `contextAllows`), but the server runs its action for any request. A direct POST without `record_id` runs an update-only action with `Record == nil`. A plugin action written for the update form, and so assuming a record, will nil-dereference into a 500 or act without one. The save path enforces context (`crud.go` `contextAllows`), so the widget route is inconsistent with it. +**Fix:** In `widgetAction`, derive the operation from `in.RecordID` (`create` when nil, otherwise `update`). Answer 404 when the field's context does not allow that operation, reusing `contextAllows(cc, field.Name, op)`. + +## Info + +### IN-01: The widget-tag collision mitigation (T-10.1-11) only checks YAML + +**File:** `modules/cabana/extension.go:241-252`, `../fonoteka.go/plugins/golem15/fonoteka/assets/js/discogs-lookup.js:61-63` +**Issue:** The `{vendor}-{plugin}-` prefix is checked on the tag named in `fields.yaml`, not on what scripts define. Scripts also persist for the life of the SPA: after visiting plugin B's list, B's JS stays loaded on A's form. Any loaded script can therefore `customElements.define('golem15-fonoteka-discogs-lookup', …)` first. The Albums script then silently skips its own `define` because of the `customElements.get(TAG) === undefined` guard. Plugin JS is trusted code, but the review presents T-10.1-11 as a mitigation. +**Fix:** Document this as advisory in the review. Have plugin scripts throw, rather than silently skip, when their tag is already defined by another constructor. + +### IN-02: A toolbar action accepts `{"values": null}` + +**File:** `modules/cabana/actions.go:83` +**Issue:** The documented contract is that the body must be exactly `{}`. `{"values": null}` and `{"record_id": null}` decode to nil and pass. This is harmless but does not match the README and SUMMARY statement. +**Fix:** Decode into `map[string]json.RawMessage` and require `len(m) == 0`. + +### IN-03: The OpenAPI annotations omit the 500 responses the new routes return + +**File:** `modules/cabana/admin_openapi.go` (AdminWidgetAction, AdminToolbarAction, AdminPartial) +**Issue:** An action error, a render failure, a size cap or a refused view model all return 500. None of the three operations document `@Failure 500`, and the partial route does not document the 422 path. +**Fix:** Add `@Failure 500 {object} ErrorEnvelope` to all three routes and regenerate `admin.json` and `schema.d.ts`. + +### IN-04: The gate's partial-template hygiene only scans `*/controllers/*/_*.htm` + +**File:** `scripts/check-phase10.1.sh:191` +**Issue:** Partials resolve under `ConfigDir()`, which can be any directory in the plugin, so a template outside `controllers/` escapes the `