docs(10.1): add code review fix report
This commit is contained in:
Jakub Zych
@@ -0,0 +1,175 @@
|
||||
---
|
||||
phase: 10.1-runtime-admin-extension-point
|
||||
fixed_at: 2026-09-29T08:10:00Z
|
||||
review_path: .planning/phases/10.1-runtime-admin-extension-point/10.1-REVIEW.md
|
||||
iteration: 1
|
||||
findings_in_scope: 7
|
||||
fixed: 7
|
||||
skipped: 0
|
||||
status: all_fixed
|
||||
---
|
||||
|
||||
# Phase 10.1: Code Review Fix Report
|
||||
|
||||
**Fixed at:** 2026-09-29T08:10:00Z
|
||||
**Source review:** .planning/phases/10.1-runtime-admin-extension-point/10.1-REVIEW.md
|
||||
**Iteration:** 1
|
||||
|
||||
**Summary:**
|
||||
- Findings in scope: 7 (CR-01, WR-01 to WR-06; the Info findings were out of scope)
|
||||
- Fixed: 7
|
||||
- Skipped: 0
|
||||
|
||||
All fixes are in the framework repo (`summercms.go`). The application repo (`fonoteka.go`) needed no change: the Albums view model passes the deeper WR-03 guard and the Albums widget declares no `context`, so WR-06 does not affect it. Each fix has a regression test, and each test was confirmed to fail against the pre-fix code.
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### CR-01: A fractional, exponent or overflowing number for an integer column is a 500, not a validation error
|
||||
|
||||
**Status:** fixed
|
||||
**Files modified:** `modules/lagoon/fill.go`, `modules/lagoon/fill_test.go`, `modules/lagoon/README.md`, `modules/cabana/crud.go`, `modules/cabana/crud_test.go`, `modules/cabana/README.md`
|
||||
**Commit:** e60e697
|
||||
**Applied fix:** `lagoon.Fill` now returns a new exported `*lagoon.FillTypeError{Key, Err}` (with `Unwrap`) when `setField` cannot store a value. The error text is unchanged: `lagoon: fill <key>: <err>`. `CRUDService.save` maps it to a `ValidationError` on that field, `"The <key> field has an invalid value."`, which is a 422 `validation_failed`. Every other `Fill` failure, such as a model that is not a struct pointer, is still a `CapabilityError` (500).
|
||||
|
||||
I left the settings path unchanged. It still answers a Fill failure with a 422 on `body`, as it did before this fix.
|
||||
|
||||
**Tests:**
|
||||
- `TestFillTypeErrorNamesTheKey` covers a fraction, `1e21`, a bool into a string field and a string into a float field, and checks that a non-pointer model stays a plain error.
|
||||
- `TestCRUDFillTypeIsValidation` runs against real Postgres. Create with `year` set to `1977.5`, `1e21`, `99999999999999999999` and `"1977"` is a 422 on `details.year` and nothing is persisted. `{"name": true}` is a 422 on `name`. Update with `1977.5` is a 422 and the stored year is unchanged.
|
||||
|
||||
### WR-02: Plugin stylesheets stay enabled on views that are not controller views
|
||||
|
||||
**Status:** fixed: requires human verification (see the note under WR-01)
|
||||
**Files modified:** `admin/src/app/router.ts`, `admin/src/app/pluginAssets.ts` (comment), `admin/tests/app/router.test.ts`, `modules/boardwalk/dist/*` (rebuilt)
|
||||
**Commit:** 849a9ff
|
||||
**Applied fix:** A `router.afterEach` hook runs after every successful navigation and calls `activateStyles(routeControllerId(to))`. The new exported `routeControllerId` returns the controller id for the `list`, `create` and `record` routes and `''` for every other route. So the following screens disable every plugin link:
|
||||
- settings, login and not found;
|
||||
- a controller whose schema is still loading or failed to load.
|
||||
|
||||
**Tests:** in `router.test.ts`:
|
||||
- `routeControllerId` for every named route;
|
||||
- navigating to `/settings`, `/settings/mail`, `/nowhere` and to a controller with no styles disables every link;
|
||||
- a controller route enables only its own links.
|
||||
|
||||
### WR-01: A late schema response re-activates the previous controller's stylesheets over the current view
|
||||
|
||||
**Status:** fixed: requires human verification
|
||||
**Files modified:** `admin/src/app/pluginAssets.ts`, `admin/tests/app/pluginAssets.test.ts`, `admin/tests/smoke/extension.smoke.test.ts`, `modules/boardwalk/dist/*` (rebuilt)
|
||||
**Commit:** 5bbb0ad
|
||||
**Applied fix:** I used the review's alternative: the route decides, not the caller.
|
||||
- `activateStyles` is now called only by the router. It records the active controller.
|
||||
- `loadStyles` creates each new link with `disabled` set unless its owner is the active controller.
|
||||
- `loadControllerAssets` no longer activates anything.
|
||||
|
||||
A schema response that arrives after its view was left therefore adds its links disabled and leaves the current controller's links alone. No unmount guard is needed in `ListView` or `FormView`.
|
||||
|
||||
**Tests:**
|
||||
- A unit test in `pluginAssets.test.ts` covers a late response and links added while nothing is active.
|
||||
- A full-app smoke test opens list A with a deferred schema, navigates to B, then releases A's schema. B's link stays enabled and A's link is created disabled.
|
||||
|
||||
**Needs a browser check:** happy-dom only reflects the `disabled` attribute. Please confirm in a real browser that a `<link rel="stylesheet">` inserted with `disabled` set loads and applies once the router enables it. That is the spec'd behaviour (such a link counts as "explicitly enabled").
|
||||
|
||||
### WR-03: The partial view-model guard (T-10.1-09) is shallow and easy to bypass
|
||||
|
||||
**Status:** fixed
|
||||
**Files modified:** `modules/cabana/partial_render.go`, `modules/cabana/phase101_render_test.go`, `modules/cabana/README.md`, `modules/pact/capabilities.go` (doc comment)
|
||||
**Commit:** 7333f45
|
||||
**Applied fix:** `refusedViewModel` is now a two-part walk:
|
||||
- **Type walk (memoised):** it follows pointers, slices, arrays, map keys and values, all struct fields, and the result types of every exported method on both `T` and `*T`, whatever the method's arguments.
|
||||
- **Value walk:** it resolves interface-typed members such as `map[string]any`, `[]any` and `any` fields. It skips pointer, map and slice cycles and stops after 10,000 values; a view model larger than that is refused.
|
||||
|
||||
It refuses the following anywhere in that structure:
|
||||
- the controller's model;
|
||||
- any other GORM model: a struct with `TableName()`, any `gorm:` struct tag, `gorm.Model` or `gorm.DeletedAt`;
|
||||
- html/template trusted types.
|
||||
|
||||
**Tests:** the `view-model guard` subtest gains 14 refused cases:
|
||||
- the model wrapped in a struct;
|
||||
- the model inside `map[string]any`, `[]any` or an `any` field;
|
||||
- `template.HTML` inside `map[string]any`;
|
||||
- `*BackendUser`, a gorm-tagged struct, embedded `gorm.Model`, `gorm.DeletedAt`;
|
||||
- methods returning `template.HTML` (value receiver, pointer receiver, with an argument) and a method returning the model.
|
||||
|
||||
It also gains 7 accepted cases, which keep today's curated view models working:
|
||||
- the Albums-shaped stats struct;
|
||||
- a curated `map[string]any`;
|
||||
- a `time.Time` field;
|
||||
- a self-referencing struct, a cyclic map, a plain method, a nil pointer field.
|
||||
|
||||
The fonoteka Albums tests pass unchanged.
|
||||
|
||||
**Not done:**
|
||||
- A method that returns an interface is not called, so its run-time result is not checked. The README says so.
|
||||
- I did not apply the optional suggestion to restrict partial `class` values to a `summer-` prefix. It changes the sanitizer contract on both server and client, and the fonoteka `_stats.htm` markup, so it is wider than this finding.
|
||||
|
||||
### WR-04: `isJSONScalar` trusts `reflect.Kind`, not the JSON the value encodes to
|
||||
|
||||
**Status:** fixed
|
||||
**Files modified:** `modules/cabana/actions.go`, `modules/cabana/contracts.go`, `modules/cabana/crud_test.go`, `modules/cabana/phase101_actions_test.go`, `modules/cabana/README.md`
|
||||
**Commit:** 0bdb6eb
|
||||
**Applied fix:** `isJSONScalar` now marshals the value. It rejects a marshal error and any encoding that starts with `{` or `[`. `writeJSON` encodes into a buffer before `WriteHeader`. If encoding fails, it logs the error and writes a 500 with the generic `error` envelope (`Server error`). This applies to every `WriteData`, `WriteError` and `WriteErrorDetails` response.
|
||||
|
||||
**Tests:**
|
||||
- `TestIsJSONScalar` covers the accepted scalars and the rejected cases: NaN, infinity, slice, map, struct, a named string whose `MarshalJSON` returns an array, and a func.
|
||||
- `TestWriteJSONEncodeFailure` checks that an infinity in the body is a 500 with the generic envelope and that a normal body is byte-identical to before.
|
||||
- A `TestPhase101Actions` subtest has an action return `{name: <array-marshalling string>, active: NaN}`. The response is 200 with `fill: {}`.
|
||||
|
||||
### WR-05: Form schema shows widgets the admin is not allowed to run
|
||||
|
||||
**Status:** fixed
|
||||
**Files modified:** `modules/cabana/http.go`, `modules/cabana/phase101_actions_test.go`, `modules/cabana/README.md`
|
||||
**Commit:** 7b72bf4
|
||||
**Applied fix:** `formSchema` drops each `type: widget` field whose action is not registered or whose `Permissions` the principal fails. This is the same filtering `listSchema` applies to `toolbarActions`. The result is a new slice, so the cached compiled schema is never modified.
|
||||
|
||||
**Tests:** in the `TestPhase101Actions` permission subtest:
|
||||
- the `limited` admin, who has no `acme.demo.run`, gets a form schema with no widget and every other field;
|
||||
- the full admin still gets `lookup:lookup`.
|
||||
|
||||
### WR-06: The widget action route ignores the field's `context`
|
||||
|
||||
**Status:** fixed
|
||||
**Files modified:** `modules/cabana/actions.go`, `modules/cabana/phase101_render_test.go`, `modules/cabana/README.md`
|
||||
**Commit:** 719ed71
|
||||
**Applied fix:** `widgetAction` works out the form from the request: `create` when there is no `record_id`, `update` when there is one. It answers 404 `not_found` when `contextAllows(cc, field.Name, op)` refuses. The check runs before any record lookup, so the action never runs.
|
||||
|
||||
**Tests:** `TestPhase101WidgetContext` covers five cases:
|
||||
|
||||
| `context` | Request | Expected |
|
||||
|-----------|---------|----------|
|
||||
| `update` | no `record_id` | 404 |
|
||||
| `[update, preview]` | no `record_id` | 404 |
|
||||
| `create` | with `record_id` | 404 |
|
||||
| `create` | no `record_id` | 200 |
|
||||
| none | no `record_id` | 200 |
|
||||
|
||||
It also asserts that the action runs only in the 200 cases.
|
||||
|
||||
## Security review impact (10.1-SECURITY-REVIEW.md not edited)
|
||||
|
||||
- **T-10.1-09 (partial view models).** The review's mitigation text says `refusedViewModel` refuses the controller's model type and pointers or collections of it. After WR-03 the guard is broader. It refuses the controller's model or any GORM model nested anywhere in the view model, including behind interfaces. It also refuses trusted html/template types reached through fields or method results. Two limits remain:
|
||||
- a method that returns an interface is not checked;
|
||||
- the `class` values of partial nodes are not restricted to a prefix.
|
||||
|
||||
`TestPhase101PartialSanitizer/view-model_guard` stays the evidence test, and removal check RC-11 still applies.
|
||||
- **T-10.1-16 (plugin CSS isolation).** After WR-01 and WR-02, the route decides which stylesheets are enabled, not the view. Plugin CSS is enabled only while the router shows its controller's list, create or record route, and disabled on every other screen. This includes the time before a controller's schema arrives and after it fails to load. The residual-risk wording "while its controller is open" now matches the behaviour. Evidence: `tests/app/router.test.ts` ("plugin stylesheets follow the route") and `tests/smoke/extension.smoke.test.ts` (the late-response test).
|
||||
|
||||
## Verification
|
||||
|
||||
All gates ran in the **main checkout**. `workflow.use_worktrees` is `false`, so no worktree was created, and every result below can be reproduced from the current tree.
|
||||
|
||||
| Check | Result |
|
||||
|-------|--------|
|
||||
| `summercms.go`: `go vet ./... && go test ./...` | pass (all packages ok) |
|
||||
| `fonoteka.go`: `go vet` and `go test` over `./...`, `./plugins/golem15/user/...`, `./plugins/golem15/fonoteka/...` | pass (all packages ok, parity included) |
|
||||
| `npm --prefix admin run typecheck` | pass |
|
||||
| `npm --prefix admin test` | pass: 53 files, 681 tests (677 before these fixes) |
|
||||
| `scripts/check-admin-dist.sh` | pass after each SPA commit; each of 849a9ff and 5bbb0ad commits its own rebuilt `modules/boardwalk/dist` |
|
||||
| `scripts/check-phase10.1.sh --all` | pass: self-test, go, security, postgres, spa, openapi, dist, hygiene (phase10 and phase10.1), evidence files, evidence; "phase10.1 all passed" |
|
||||
|
||||
No OpenAPI or response shapes changed. `check-admin-openapi` passed inside the gate. Every identifier added to a README exists: I checked `lagoon.FillTypeError` with `go doc`.
|
||||
|
||||
---
|
||||
|
||||
_Fixed: 2026-09-29T08:10:00Z_
|
||||
_Fixer: Claude (gsd-code-fixer)_
|
||||
_Iteration: 1_
|
||||
Reference in New Issue
Block a user