diff --git a/.planning/phases/32-seo-foundation/32-REVIEW.md b/.planning/phases/32-seo-foundation/32-REVIEW.md index 10812124..62fe2021 100644 --- a/.planning/phases/32-seo-foundation/32-REVIEW.md +++ b/.planning/phases/32-seo-foundation/32-REVIEW.md @@ -16,7 +16,7 @@ status: issues_found The scope (221 files across summercms.go, sm-journal-plugin and sm-seo-plugin) was split into three parallel reviewer lanes at standard depth; each lane's report follows verbatim, with finding IDs prefixed A-, B- and C-. Per-lane file lists are in each section's header block. -**Critical:** CR-A01 (extension `required:` field blocks every host save), CR-A02 (field-type upload ignores extension Permissions/Visible), CR-B01 (exact redirects with trailing slash or upper-case query never match at runtime), CR-C01 (widget host assigns `element` after `append`, so actions sent from `connectedCallback` are dropped: SERP preview and robots tester never load; orchestrator confirmed in `admin/src/components/form/fields/WidgetField.vue:161-162`). +**Critical:** CR-01 (CR-A01) (extension `required:` field blocks every host save), CR-02 (CR-A02) (field-type upload ignores extension Permissions/Visible), CR-03 (CR-B01) (exact redirects with trailing slash or upper-case query never match at runtime), CR-04 (CR-C01) (widget host assigns `element` after `append`, so actions sent from `connectedCallback` are dropped: SERP preview and robots tester never load; orchestrator confirmed in `admin/src/components/form/fields/WidgetField.vue:161-162`). --- @@ -52,8 +52,8 @@ Parts that hold up: - Invisible extensions are filtered consistently in the schema, show, save, widget-action and partial routes. Problems found: -- **Required extension fields (CR-A01).** A `required: true` field in an extension is turned into a model validation rule on the owner's record. That rule can never pass, so every save of the host form fails. -- **Upload route (CR-A02).** The plugin field-type upload route does not check extension visibility. Administrators who are not allowed to see an extension can still call its `Upload`. +- **Required extension fields (CR-01 (CR-A01)).** A `required: true` field in an extension is turned into a model validation rule on the owner's record. That rule can never pass, so every save of the host form fails. +- **Upload route (CR-02 (CR-A02)).** The plugin field-type upload route does not check extension visibility. Administrators who are not allowed to see an extension can still call its `Upload`. - **Record-event coverage and locale (warnings).** Some admin writes fire no event: hasMany link/unlink and the delete of a deferred child. `RecordSaved.Locale` is not a content locale. - **Journal MCP writes (warnings).** These can commit a slug change and fire no event, and they report a wrong `Old` value for derived `content_html` translations. @@ -61,7 +61,7 @@ Problems found: ### Critical Issues -### CR-A01: `required: true` on a form extension field makes every save of the host form fail (also for administrators who cannot see the extension) +### CR-01: [CR-A01] `required: true` on a form extension field makes every save of the host form fail (also for administrators who cannot see the extension) **File:** `modules/cabana/crud.go:1353-1361` (mergedRules), `modules/cabana/form_extensions.go:405-443` (compileExtensionKinds accepts `Required`) **Issue:** `compileFormExtensions` places extension fields into `cc.Form.Fields`. `mergedRules` loops over `cc.Form.Fields` and adds `required` for every `field.Required && valueFormField(field)` field, extension fields included. Then `lagoon.Validate` checks those rules against the owner's model. It reads the values through `virtualValuesForRules`/`valuesForRules`, which come from the model's columns. The extension field is not a column, and its value has already been lifted out of the body by `liftExtensionValues`, so the value is always nil and the rule always fails. I confirmed this in a scratch copy of the package. `parseExtensionDoc` accepts `required: true` with no error, and `mergedRules` returns `map[seo_title:required]` → `msgs=map[seo_title:[The seo title field is required.]]`. Nothing in `compileExtensionKinds` refuses or strips `Required`. The impact: @@ -88,7 +88,7 @@ if field.Required { ``` Add a Postgres test with a required extension field, saved by an administrator who sees the extension and by one who does not. -### CR-A02: Plugin field-type upload route ignores the extension's Permissions/Visible, so administrators without them can run its `Upload` +### CR-02: [CR-A02] Plugin field-type upload route ignores the extension's Permissions/Visible, so administrators without them can run its `Upload` **File:** `modules/cabana/field_type_upload.go:32-126` (`fieldUpload`), `uploadField` at line 130 **Issue:** The other per-field routes of an extension gate on `extensionVisible`: `widgetAction` (actions.go), `partial` (partial_render.go), `formSchema` (http.go) and save/show. `POST .../{id}/fields/{field}/upload` does not. `uploadField` finds the field in `cc.Form.Fields`, which includes the extension's fields, and checks only `contextAllows`. `protect()` checks only the host controller's permissions. So an administrator with access to, say, `acme.blog.posts` but without `golem15.seo.manage` can still run the SEO plugin's `seoimage` `Upload` (D-100). The plugin stores a file in the bucket, inside a transaction on the loaded record, with no admin UI showing it. This is exactly the access that `Permissions` and `Visible` are documented to deny ("Its widget actions and partials answer 404 for them"), and the docs never list the upload route as an exception. It is an authorization gap on a write path. @@ -111,7 +111,7 @@ Add the upload route to the permission tests in `form_extensions_postgres_test.g ### Warnings -### WR-A01: Admin writes that change or delete records fire no record event (hasMany link/unlink, deferred child unlink, deferred child commit) +### WR-01: [WR-A01] Admin writes that change or delete records fire no record event (hasMany link/unlink, deferred child unlink, deferred child commit) **File:** `modules/cabana/relation.go:1013` (linkRelated hasMany `Save(related)`), `modules/cabana/relation.go:1316` (unlinkRelated hasMany `Save(child)` with FK nulled + ClearOnChange), `modules/cabana/relation.go:1130` (unlinkDeferred `Delete(child)`), `modules/cabana/deferred.go` (commitDeferred binding children to the new parent) **Issue:** D-92 says "Admin, MCP and imports all go through cabana and emit it", and `pact.RecordSaved` says "The admin framework fires it for every admin write". The SEO plugin relies on it for 301s and for deleting entries with their record. Some admin paths still write records with no event: @@ -119,37 +119,37 @@ Add the upload route to the permission tests in `form_extensions_postgres_test.g - hasMany link and unlink rewrite the child's foreign key and clear its `ClearOnChange` columns (a parent change that can change the child's URL), with no `RecordSaved`. **Fix:** In `unlinkDeferred`, after the delete, call `EmitRecordDeleted(ctx, s.app, tx, pact.RecordDeleted{Controller: s.reg.controllerForRecord(child), Model: recordMorphType(tx, child), ID: recordIDText(child)})`. In linkRelated and unlinkRelated (hasMany), take a snapshot before `setModelColumn` and call `s.emitChildSaved(ctx, tx, child, false, changes)` after `Save`. Do the same for the hasMany children `commitDeferred` attaches. Alternatively, narrow the `pact.RecordSaved` doc and `docs/backend/admin-controllers.md` to the paths that actually fire. -### WR-A02: `RecordSaved.Locale` is the admin's UI language or the raw `Accept-Language` header, not a content locale, and reorder reports something different +### WR-02: [WR-A02] `RecordSaved.Locale` is the admin's UI language or the raw `Accept-Language` header, not a content locale, and reorder reports something different **File:** `modules/cabana/record_events.go:65`, `modules/cabana/relation_child.go:490`, `modules/cabana/reorder.go:318,337` **Issue:** `pact.RecordSaved.Locale` is documented as "the request's content locale". `emitRecordSaved` and `emitChildSaved` copy `towel.Locale(ctx)`. In the admin that value is set by `surf.Router.locale` to the raw `Accept-Language` header when no `LocaleResolver` is published, for example `pl-PL,pl;q=0.9,en-US;q=0.8`, or by `surf.LocaleFromPrincipal` to the administrator's `PreferredLocale` (the UI language). Journal's `requestLocale` has to parse exactly this raw header, which shows it happens. `reorderPost` instead sets `towel.WithLocale(ctx, schemaLocale(...))`, so reorder events carry a normalised UI code. Admin saves write the default content locale to the row and report the other locales per change, so the event-level Locale has no defined content meaning. A subscriber that keys on it (an SEO 301 per locale) will mis-key. **Fix:** Pick one meaning and enforce it at the emit site. For admin saves, set `Locale` to the default content locale (from `TranslationWriter.DefaultLocale`, or `""` without translate) and leave per-locale changes to `RecordChange.Locale`. At the very least, normalise with `primaryLanguage` the way `schemaLocale` does. Then document it in `pact/record_events.go`. -### WR-A03: A failing extension `Load` (or plugin-type `Load`) after a successful save rolls back the whole save +### WR-03: [WR-A03] A failing extension `Load` (or plugin-type `Load`) after a successful save rolls back the whole save **File:** `modules/cabana/crud.go:980`, `modules/cabana/form_extensions.go:1431-1435` **Issue:** `hydrateFormExtensions` runs as the last statement inside the save transaction closure, and its error is returned from it. The owner row, translations, deferred bindings and every extension's `Save` have all succeeded by then, but a read-side failure in third-party plugin code (an SEO `Load` hitting a missing row, a field type's `Load`) turns the save into a 500 and rolls everything back. `ShowRecord` likewise fails the whole host record when any one extension's `Load` fails. One contributing plugin can block editing of another plugin's records. **Fix:** Run the response hydration after the transaction commits, in a short read transaction, or log the error and omit that extension's keys from `result.Data` instead of returning it. Keep `Save` errors as the rollback path; hydration is presentation only. -### WR-A04: Journal API writes can commit changes (including a slug) and fire no RecordSaved +### WR-04: [WR-A04] Journal API writes can commit changes (including a slug) and fire no RecordSaved **File:** `../sm-journal-plugin/controllers/api/posts.go:223-236` (Store), `:281-294` (Update) **Issue:** The API handlers write without a transaction: `Save(post)`, then `applyPostAssocs` (categories, tags, and `syncTranslations`, which writes translated slugs), then `loadPostAssocs`. `cabana.EmitRecordSaved` is called only after all of them succeed. If `applyPostAssocs` or `loadPostAssocs` fails, the post row (and possibly some translations) is already committed but no event fires. A slug changed over MCP then gets no 301 (D-102), which is exactly what the change was for. **Fix:** Wrap the write in `lagoon.Transaction(ctx, db, ...)` and call `cabana.EmitRecordSaved(ctx, app, tx, ...)` inside it, so the event follows the commit and a failure rolls back the row too. If that is not possible, emit as soon as `Save` succeeds and emit again (or emit the translation changes) after `applyPostAssocs`. -### WR-A05: Journal `postSavedEvent` reports `Old: nil` for a translated `content_html` that existed, because the body is mutated between the snapshot and the event +### WR-05: [WR-A05] Journal `postSavedEvent` reports `Old: nil` for a translated `content_html` that existed, because the body is mutated between the snapshot and the event **File:** `../sm-journal-plugin/controllers/api/record_events.go:54,91`, `../sm-journal-plugin/controllers/api/posts_helpers.go:789` **Issue:** `snapshotPost` reads the old translations of the fields `submittedTranslations(body)` lists *before* the write. `syncTranslations` then calls `classes.FormatTranslation(attrs)` on the same `attrs` map inside `body`, which adds `attrs["content_html"]` whenever `content` is sent. `postSavedEvent` calls `submittedTranslations(body)` again, now sees `content_html`, finds no snapshot for it and reports `Old: nil`. Per `pact.RecordChange`, nil means "a translation that did not exist before", so the event is wrong for every update that sends a translated `content` without `content_html`. **Fix:** Compute the submitted set once, before the write, and pass it to both functions. Or include `content_html` in the snapshot whenever `content` is submitted (mirror `FormatTranslation`). Or copy the body's translation maps before `syncTranslations` mutates them. -### WR-A06: Tags auto-created by the Journal post API fire no RecordSaved although `journal-tag` is now a content type +### WR-06: [WR-A06] Tags auto-created by the Journal post API fire no RecordSaved although `journal-tag` is now a content type **File:** `../sm-journal-plugin/controllers/api/posts_helpers.go:732-770` (`resolveTagIDs` → `db.Create(&tag)`) **Issue:** D-102 adds the `journal-tag` content descriptor so that tags get SEO entries. The MCP post write creates new `models.Tag` rows on the fly and fires nothing for them. SEO and sitemap subscribers only learn about those tags from a later bulk job. -**Fix:** Collect the tags created in `resolveTagIDs` and emit `cabana.EmitRecordSaved(ctx, app, tx, pact.RecordSaved{Model: models.Tag{}.MorphName(), ID: ..., Created: true, Changes: ...})` for each, ideally inside the transaction suggested in WR-A04. +**Fix:** Collect the tags created in `resolveTagIDs` and emit `cabana.EmitRecordSaved(ctx, app, tx, pact.RecordSaved{Model: models.Tag{}.MorphName(), ID: ..., Created: true, Changes: ...})` for each, ideally inside the transaction suggested in WR-04 (WR-A04). -### WR-A07: `Hide` leaves the owner's relation-manager and partial endpoints of a hidden field reachable +### WR-07: [WR-A07] `Hide` leaves the owner's relation-manager and partial endpoints of a hidden field reachable **File:** `modules/cabana/form_extensions.go:624-641` (`dropHiddenFields`) **Issue:** `dropHiddenFields` removes a hidden field from `Writable`, `files`, `dates`, `permissions` and `FieldRelations`, but not from `cc.Relations` (relation-manager panels and their link, unlink and child CRUD routes, plus `commitDeferred`, which loops over `cc.Relations` and calls `contextAllows`; that returns true for a field no longer in the form) or from `cc.partials`/`cc.formPartials`. The pact doc says hidden fields "leave the schema, show and save", but writes through the relation routes of a hidden relation-manager field still go through, and the partial still renders. @@ -157,37 +157,37 @@ Add the upload route to the permission tests in `form_extensions_postgres_test.g ### Info -### IN-A01: Dead check in `BindWritableFields` +### IN-01: [IN-A01] Dead check in `BindWritableFields` **File:** `modules/cabana/crud.go:259-263` **Issue:** `BindWritableFields` runs in `compileRegistryWithTypes`, before `compileFormExtensions` fills `cc.extensionFields`, and again for relation child forms, which never have extensions. So `cc.extensionOf(field.Name) != nil` is always false there. The comment suggests it protects something it does not. **Fix:** Remove it, or move the guard to where it would matter and say so. -### IN-A02: Scoped API tokens never see an extension that declares no Permissions, and their posted values are dropped silently +### IN-02: [IN-A02] Scoped API tokens never see an extension that declares no Permissions, and their posted values are dropped silently **File:** `modules/cabana/form_extensions.go:1092-1097`, `modules/cabana/token_scopes.go:246-252` **Issue:** `Allows` returns false under a non-full token scope when `required` is empty. An extension without `Permissions` is therefore invisible to scoped MCP tokens, and a save from such a token drops the extension keys and still answers 200. That is fail-closed, but it is surprising and undocumented in `pact.AdminFormExtension`. **Fix:** Document it next to `Permissions`, or treat an empty `Permissions` as "inherit the host controller's access". -### IN-A03: `resolveExtensionLocales` wraps a nil error +### IN-03: [IN-A03] `resolveExtensionLocales` wraps a nil error **File:** `modules/cabana/form_extensions.go:1129-1131` **Issue:** When `DefaultLocale` returns `("", nil)`, `fmt.Errorf("cabana: default locale: %w", err)` produces `%!w()`. The same pattern exists in `hydrateMLRecord`. **Fix:** Use `errors.New("cabana: default locale is empty")` when `err == nil`. -### IN-A04: `tab: default` on an extension field is refused although `default` names the default tab elsewhere +### IN-04: [IN-A04] `tab: default` on an extension field is refused although `default` names the default tab elsewhere **File:** `modules/cabana/form_extensions.go:932-936` **Issue:** The docs say "`default` names the tab of fields without a `tab:`". Anchors and `after:` accept it, but a field with `tab: default` fails with "unknown tab" because `place` does not map it through `tabAnchorKey`. **Fix:** Normalise `field.Tab` with `tabAnchorKey` before the lookup, or say in the docs that a field omits `tab:` for the default tab. -### IN-A05: Journal `requestLocale` drops the region, so translated slugs in region-coded locales never match +### IN-05: [IN-A05] Journal `requestLocale` drops the region, so translated slugs in region-coded locales never match **File:** `../sm-journal-plugin/controllers/api/posts_helpers.go:806-819` **Issue:** `pt-BR` and `en_GB` become `pt` and `en`. If translate stores region codes, the translated-slug lookup in `findPostBySlugOrID` misses. **Fix:** Try the full normalised code first, then the primary language. -### IN-A06: The extension's client assets are served to every administrator of the host form +### IN-06: [IN-A06] The extension's client assets are served to every administrator of the host form **File:** `modules/cabana/form_extensions.go:508-543`, `modules/cabana/http.go:851` (`view.Assets = s.controllerAssets(cc)`) **Issue:** The plugin's `AdminClientAssets` JS/CSS is added to `cc.scripts`/`cc.styles` and loaded for every administrator, including those the extension is hidden from. This is not an authorization leak, because the scripts are already served, but it does run the plugin's script on forms where none of its elements exist. @@ -240,7 +240,7 @@ Main concerns: ### Critical Issues -### CR-B01: Exact redirects with a trailing slash or upper-case query characters never match at runtime +### CR-03: [CR-B01] Exact redirects with a trailing slash or upper-case query characters never match at runtime **File:** `../sm-seo-plugin/classes/redirects.go:119-137, 230-242, 277-289` **Issue:** `LoadRedirectSet`, used by the resolver (`resolver.go:375`), the 404 log's "resolved" state (`controllers/notfound.go:70,205`, `controllers/api/notfound.go:123`) and `ResolveNotFound` (`notfound.go:161`), loads exact rules on demand with @@ -266,25 +266,25 @@ A cleaner option is a stored `source_key TEXT` column, filled from `exactKey(r.S ### Warnings -### WR-B01: A query-specific exact redirect is silently shadowed by the path-only rule +### WR-08: [WR-B01] A query-specific exact redirect is silently shadowed by the path-only rule **File:** `../sm-seo-plugin/classes/redirects.go:130-137, 281-289` **Issue:** `exactKeys` returns `[path, path?query]`, and `Match` returns the first hit in that order. With `/products → /shop` and `/products?id=5 → /shop/item-5` both stored, `/products?id=5` always goes to `/shop`, so the more specific rule can never fire. `checkRedirectRules` does not report this: the probe's first hop is the other rule, so the `Hops[0].Rule.ID == r.ID` branches are skipped and the rule is saved without any notice. **Fix:** Try the `path?query` key before the path-only key in `Match`. Alternatively, have `CheckRedirect` refuse or warn when the probe of `r` is answered by a different rule (a "shadowed" problem). -### WR-B02: `AddressChanged` writes redirects without validation or the loop simulation +### WR-09: [WR-B02] `AddressChanged` writes redirects without validation or the loop simulation **File:** `../sm-seo-plugin/classes/addresses.go:58-120` **Issue:** Steps 1–4 use raw `tx.Create`, `tx.Save` and `tx.Delete` and never call `Problems()` or `CheckRedirect`. Only exact redirects whose source equals the new address are removed (step 1). Pattern and regex rules, case variants (`ignore_case`) and trailing-slash variants that match the new address survive. The new `old → new` 301 then forms a loop with them. For example, with a pattern `/a/* → /b/*` in place, a record moved from `/b/x` to `/a/x` gets `/b/x → /a/x → /b/x`. The resolver turns this into `not_found` for the record's live address. Re-pointing (step 3) can likewise close a cycle with pattern rules. Also, the descriptor URL is stored as an exact `source` unvalidated: an absolute or `//` URL from a descriptor would be stored and never match. **Fix:** After steps 1–4, run `s.CheckRedirect(ctx, tx, redirect)` and the same for every re-pointed rule. On `Loop`, either skip creating the 301 and log, or delete the conflicting shadowing rule. Validate `res.Old` and `res.New` with `Problems(tx)` before creating. -### WR-B03: The save-time loop check races with concurrent writes +### WR-10: [WR-B03] The save-time loop check races with concurrent writes **File:** `../sm-seo-plugin/classes/redirect_graph.go:168-177, 276-310`; `controllers/api/auth.go:172-180` **Issue:** `CheckRedirect` reads all rules inside the writer's transaction at READ COMMITTED with no lock. Two concurrent writes (two MCP agents, an import and an admin save, or a batch and a slug change) can each simulate without the other's uncommitted rule. For example, one creates `A → B` while the other creates `B → A`. Both commit, and the stored set contains the loop D-19 promises to refuse. The runtime guard turns it into a 404 for both addresses. **Fix:** Serialise redirect writes. Take `SELECT pg_advisory_xact_lock()` at the start of `SaveRedirect`, `DeleteRedirect`, `AddressChanged` and `runBatch` before `LoadRedirects`, or run those transactions at `SERIALIZABLE` and retry on 40001. -### WR-B04: Undo is not idempotent under concurrent requests +### WR-11: [WR-B04] Undo is not idempotent under concurrent requests **File:** `../sm-seo-plugin/classes/undo.go:71-83, 291-343` **Issue:** `Undo` reads the revision with `undone_at IS NULL` without locking. It then restores and marks the revision with `UPDATE ... WHERE id = ?`, with no `undone_at IS NULL` condition and no `RowsAffected` check. Two simultaneous undo calls (a double-click in the admin plus a retry, or an MCP agent retrying) can both pass the "already undone" check and both restore. For "deleted" subjects that means two re-created rules, redirects or 404 rows (or a unique-violation 500). In every case it writes two `via undo` history rows. `UndoBatch` has the same pattern. @@ -295,19 +295,19 @@ res := tx.Model(&models.Revision{}).Where("id = ? AND undone_at IS NULL", rev.ID if res.Error == nil && res.RowsAffected == 0 { return &UndoRefusal{Code: UndoDone, Key: "golem15.seo::lang.history.undo_done"} } ``` -### WR-B05: Public resolve and 404 report answer 500 for NUL or invalid UTF-8 paths +### WR-12: [WR-B05] Public resolve and 404 report answer 500 for NUL or invalid UTF-8 paths **File:** `../sm-seo-plugin/classes/resolver.go:198-219`; `classes/notfound.go:59-80`; `controllers/api/resolve.go:31-40` **Issue:** `parseTarget` and `NotFoundPath` return `u.Path`, which is percent-decoded. `/%00` or `/%FF` yields a path with a NUL or invalid UTF-8 byte. That path reaches Postgres as a text parameter (`lower(source) IN ?` in the redirect fetch, `url = ?` in `ReportNotFound`), and Postgres rejects it ("invalid byte sequence for encoding UTF8"). The anonymous resolve endpoint then answers 500 and writes a `log.Error` line per request: 600 per minute per IP, more with many IPs. The token-gated 404 report fails the same way for every such URL a visitor hits. **Fix:** In `parseTarget` and `NotFoundPath`, refuse a decoded path that is not `utf8.ValidString` or contains `\x00`, returning `ErrBadURL` / `ErrNotFoundURL` (422). Apply the same check to `rawQuery`. -### WR-B06: The runtime cycle log grows without bound and is keyed by attacker-controlled URLs +### WR-13: [WR-B06] The runtime cycle log grows without bound and is keyed by attacker-controlled URLs **File:** `../sm-seo-plugin/classes/redirects.go:529-537`; `classes/resolver.go:358, 385, 635` -**Issue:** `redirectHopsLogged` is a process-global `sync.Map` that is never evicted. Its key is `self = site + path?rawQuery`, and the raw query comes straight from the anonymous request (up to 2048 bytes). Once any looping or over-10-hop chain exists (which WR-B02 and WR-B03 make reachable), an unauthenticated client can request `/looping?x=1`, `?x=2`, … Each request stores a new entry and writes a new warning line. Memory grows without limit, and the log is flooded despite the "once per URL" intent. +**Issue:** `redirectHopsLogged` is a process-global `sync.Map` that is never evicted. Its key is `self = site + path?rawQuery`, and the raw query comes straight from the anonymous request (up to 2048 bytes). Once any looping or over-10-hop chain exists (which WR-09 (WR-B02) and WR-10 (WR-B03) make reachable), an unauthenticated client can request `/looping?x=1`, `?x=2`, … Each request stores a new entry and writes a new warning line. Memory grows without limit, and the log is flooded despite the "once per URL" intent. **Fix:** Key the de-duplication on the rule IDs of the cycle (or `npath` without the query), and bound it, for example with a small LRU or a time-based reset like `capWarned` in `notfound.go`. -### WR-B07: Export with a large `page` panics the handler +### WR-14: [WR-B07] Export with a large `page` panics the handler **File:** `../sm-seo-plugin/classes/transfer.go:159-168`; `controllers/api/transfer.go:88-89` **Issue:** `start := (page - 1) * perPage` overflows for large `page` values. `page=9223372036854777` gives `start = -9223372036854775616`, which passes the `start > len(rows)` check, and `rows[start:end]` panics with "slice bounds out of range". I reproduced this. It is reachable from `GET /_seo/api/v1/export?page=…` and from the `seo_export_csv` MCP tool for any holder of `golem15.seo.manage`. @@ -321,31 +321,31 @@ start := (page - 1) * perPage Apply the same clamp in `listParams` (`controllers/api/auth.go:201-216`), where the multiplication can overflow into a negative offset. -### WR-B08: Public throttle buckets key on the SSR server's IP, capping the whole site +### WR-15: [WR-B08] Public throttle buckets key on the SSR server's IP, capping the whole site **File:** `../sm-seo-plugin/routes.go:91-111` **Issue:** The README says the frontend server calls `/resolve` for every page it renders and reports every 404. Both calls are server-to-server, so `surf.ClientIP` resolves to the SSR host unless that host is configured as a trusted proxy and forwards `X-Forwarded-For`. `seo-resolve` (600/min) and `seo-notfound` (600/min) then become site-wide caps: about 10 rendered pages per second. Beyond that, resolve answers 429 and the frontend either fails or renders without the redirect decision and robots. Nothing in the plugin documents or enforces the trusted-proxy requirement. **Fix:** Let the frontend server authenticate resolve calls with the existing `notfound_token` (or a dedicated `resolve_token`) and exempt or raise the bucket for authenticated calls. At minimum, document in the README that the SSR host must be in the trusted proxies and must forward the visitor IP. -### WR-B09: Social image values bypass validation on every non-admin write path +### WR-16: [WR-B09] Social image values bypass validation on every non-admin write path **File:** `../sm-seo-plugin/classes/owner.go:129-150`; `classes/rules.go:33-52`; `classes/settings_write.go:104-105` **Issue:** The admin `seoimage` field type refuses anything but a path or an http(s) URL (`decodeImage` → `ValidImageURL`). `SetURLSEO` (API, MCP, batch `set_seo`, records import), `SaveRule` (API, MCP, batch, rules import) and `SaveSettings` (`default_og_image`) validate `canonical` and `robots` but not `og_image`. Values such as `javascript:alert(1)`, `data:…`, `//evil.example/x.png` or text with spaces are stored. `AbsoluteURL` (`urls.go:125-137`) passes any `IsAbs()` value through, so they come back from `/resolve` as `seo.og.image`. That value goes into `og:image` / `twitter:image` meta content, where crawlers and frontends trust it. **Fix:** Add `case column == "og_image" && v != "" && !ValidImageURL(v):` to `SetURLSEO`. Add the same check for `r.OGImage` in `SaveRule` and for `default_og_image` in `SaveSettings`, using the `golem15.seo::lang.image.url_invalid` problem. -### WR-B10: Invalid redirect status, match or query values are silently coerced on API, MCP and import +### WR-17: [WR-B10] Invalid redirect status, match or query values are silently coerced on API, MCP and import **File:** `../sm-seo-plugin/classes/batch.go:53-78`; `models/redirect.go:109-129` **Issue:** `RedirectFields.Apply` copies any status, match or query, and `Normalize` replaces unknown values with defaults. A client sending `"status": 308` or `307` gets a 301. `"match": "Regex"` or `"glob"` becomes `exact`, which then fails with a misleading `source_invalid`. `"query": "keep"` becomes `ignore`. All of this is answered with 201. The CSV import has the same behaviour for the `match` and `query` columns. An MCP agent cannot tell that its request was changed. **Fix:** In `RedirectFields.Apply` (or a `Validate` step before `Normalize` on API, batch and import paths), refuse values outside {301, 302, 410}, {exact, pattern, regex} and {ignore, pass} with a `ProblemsError` (`value_invalid`). Leave the defaulting only to fields the caller omitted. -### WR-B11: `set_url_seo`, batch `set_seo` and the records import accept any locale +### WR-18: [WR-B11] `set_url_seo`, batch `set_seo` and the records import accept any locale **File:** `../sm-seo-plugin/classes/owner.go:154-173`; `classes/entries.go:176-223`; `classes/transfer.go:536-568` **Issue:** `in.Locale` (or the CSV `locale` cell) overrides the matched locale without checking that the record has that locale. `SaveRecord` creates an entry for `"xx"` or `"EN "` or any string, with an empty URL (`described[locale]` is missing), and records a history row. The answer says `written_to: record`, but the resolver never reads that row. The SEO tab never shows it either, so the agent's change is lost while it is reported as success. **Fix:** In `SetURLSEO`, when `in.Locale` is set, require it to be one of `recordLocales(...)` (or the enabled locales from `Locales()`). Otherwise refuse with `locale_unknown` as a `ProblemsError`. -### WR-B12: Entry generation overwrites `entry.URL`, which can swallow an address change and its 301 +### WR-19: [WR-B12] Entry generation overwrites `entry.URL`, which can swallow an address change and its 301 **File:** `../sm-seo-plugin/classes/generate.go:111-131`; `listeners.go:46-70` **Issue:** Address-change detection (`AddressChanges`) depends on `entry.URL` holding the last address seen. `refreshEntry` sets `url` to the current descriptor URL unconditionally, and the job is queued after every settings save (`settings_write.go:292`) or from the CLI. If a record's slug changes and the job runs before the `RecordSaved` listener's transaction, the listener sees `entry.URL == url` and no 301 is created. This happens with an after-commit listener racing a worker, or with any write path that does not emit `RecordSaved`. The old URL then 404s with no trace in history. @@ -353,49 +353,49 @@ Apply the same clamp in `listParams` (`controllers/api/auth.go:201-216`), where ### Info -### IN-B01: The `regexCache` / `globCache` maps are never evicted +### IN-07: [IN-B01] The `regexCache` / `globCache` maps are never evicted **File:** `../sm-seo-plugin/classes/redirects.go:45-72`; `classes/urls.go:13-29` **Issue:** Every distinct source compiled during a save simulation, a dry-run batch, an import preview or a refused (looping) save stays cached for the life of the process. The input is admin/MCP-controlled (up to 512-character regexes), so this is bounded by trusted users, but a long-running MCP agent can grow it steadily. **Fix:** Cache only stored rules (key by rule ID plus `updated_at`), or use a bounded LRU. -### IN-B02: CSV `unescapeCell` changes legitimate values that start with an apostrophe +### IN-08: [IN-B02] CSV `unescapeCell` changes legitimate values that start with an apostrophe **File:** `../sm-seo-plugin/classes/csv.go:50-57` **Issue:** A stored title such as `'-Mix` is exported unescaped (it does not start with a formula character) and imported back as `-Mix`, because import strips any apostrophe followed by a formula character. Round-trips are therefore not lossless. **Fix:** In `escapeCell`, also prefix values that already start with `'` (so `'x` exports as `''x`), and strip exactly one apostrophe on import. -### IN-B03: The transfer token has no expiry or user binding, and the fallback key is per process +### IN-09: [IN-B03] The transfer token has no expiry or user binding, and the fallback key is per process **File:** `../sm-seo-plugin/classes/transfer.go:700-788` **Issue:** The apply token carries the file and a fingerprint but no issue time or admin ID, so it can be replayed by any SEO manager at any later time while the data is unchanged. Without an app key, `fallbackKey` is random per process, so with several instances behind a load balancer a preview and its apply land on different keys and fail with `token_invalid`. **Fix:** Add `iat` and `admin_id` to the token and check a TTL (for example 1 h) and the current principal on apply. Refuse to issue tokens when there is no app key, rather than using a per-process key. -### IN-B04: Uniqueness checks swallow query errors inside the save transaction +### IN-10: [IN-B04] Uniqueness checks swallow query errors inside the save transaction **File:** `../sm-seo-plugin/models/redirect.go:165-174`; `models/rule.go:110-119` **Issue:** `if err := q.Count(&count).Error; err == nil && count > 0` ignores errors. On Postgres a failed statement aborts the transaction, so the next statement fails with "current transaction is aborted", which hides the real cause. If the check is skipped, the unique constraint later surfaces as a 500 instead of `source_taken` / `url_taken`. **Fix:** Return the error, or map a unique-violation (23505) on create or save to the same problem key. -### IN-B05: `RenderTemplate` trims separators that belong to the content +### IN-11: [IN-B05] `RenderTemplate` trims separators that belong to the content **File:** `../sm-seo-plugin/classes/templates.go:29-40` **Issue:** `strings.Trim(out, " \t|-–—·:,")` runs on the whole rendered string. A template that is just `%title%`, or a title such as `Q&A:` or `-20% sale`, loses its own leading or trailing characters (`Q&A`, `20% sale`). **Fix:** Trim only the separators adjacent to placeholders that rendered empty. For example, render each placeholder with a sentinel, collapse `sentinel+separator` runs, then remove the sentinels. -### IN-B06: Body caps do not account for JSON escaping +### IN-12: [IN-B06] Body caps do not account for JSON escaping **File:** `../sm-seo-plugin/controllers/api/robots.go:103`; `controllers/api/transfer.go:105-112` **Issue:** `decode` truncates the body at the limit. A 500 KB robots.txt or a 900 KB CSV with many quotes or newlines exceeds the JSON body limit after escaping, so the caller gets a generic `422 The request body is invalid.` instead of `robots.too_large` or `413 too_large`. **Fix:** Detect truncation (read `limit+1` and compare) and answer 413 with the documented code, or size the limits from the worst-case escaped size. -### IN-B07: `/robots.txt` serves allow-all without the noindex header when the service is nil, even outside production +### IN-13: [IN-B07] `/robots.txt` serves allow-all without the noindex header when the service is nil, even outside production **File:** `../sm-seo-plugin/controllers/api/robots.go:20-46` **Issue:** When `service()` is nil (before boot), `staging` stays false and the body is `FallbackRobots` (allow all). A staging instance answering during startup gives crawlers an allow-all file, which contradicts D-22's "outside production always Disallow". **Fix:** When `svc == nil`, read the environment from the app config directly, or default to `NonProductionRobots` with `X-Robots-Tag: noindex, nofollow`. -### IN-B08: Self canonical and `url` are built from the decoded path +### IN-14: [IN-B08] Self canonical and `url` are built from the decoded path **File:** `../sm-seo-plugin/classes/resolver.go:358, 476-480` **Issue:** `self := site + path` uses `u.Path`, which is decoded. A page at `/a%3Fb` gets canonical `https://site/a?b` (a different URL), and spaces or non-ASCII characters end up unescaped in `canonical`, `og.url` and `url`. Location headers already use `escapePath`, so canonical and redirect forms disagree. @@ -451,7 +451,7 @@ The warnings cover: ### Critical Issues -### CR-C01: The SERP preview and the robots.txt tester deadlock: their first action is dropped by the host and `inflight` is never cleared +### CR-04: [CR-C01] The SERP preview and the robots.txt tester deadlock: their first action is dropped by the host and `inflight` is never cleared **File:** `../sm-seo-plugin/assets/src/elements/serp.ts:172-178, 217-224`; `../sm-seo-plugin/assets/src/elements/robotstester.ts:77-82, 110-117`. Root cause in the framework: `admin/src/components/form/fields/WidgetField.vue:82, 161-162`. @@ -502,7 +502,7 @@ Add a vitest case that mounts the element through a fake host with the same orde ### Warnings -### WR-C01: The social image preview accepts `/\host/...`, so the admin can load an image from an attacker's host +### WR-20: [WR-C01] The social image preview accepts `/\host/...`, so the admin can load an image from an attacker's host **File:** `../sm-seo-plugin/assets/src/elements/image.ts:6-8`. Server side: `../sm-seo-plugin/classes/field_types.go:157-163` (`ValidURLRef`, used by `ValidImageURL`). @@ -527,7 +527,7 @@ if strings.HasPrefix(v, "/") { } ``` -### WR-C02: The CSV export/import round trip changes cells that already start with an apostrophe followed by a formula character +### WR-21: [WR-C02] The CSV export/import round trip changes cells that already start with an apostrophe followed by a formula character **File:** `../sm-seo-plugin/classes/csv.go:43-57` (exercised by `classes/csv_test.go:60-70`; the claim is in `README.md:38`) @@ -550,7 +550,7 @@ func escapeCell(s string) string { Then add `{"/ap", "/x", "'=x"}` to the round-trip rows in `TestCSV`. -### WR-C03: The 404 "Open/Resolved" filter and the redirect "Chains" filter disagree with the runtime matcher +### WR-22: [WR-C03] The 404 "Open/Resolved" filter and the redirect "Chains" filter disagree with the runtime matcher **File:** `../sm-seo-plugin/models/notfound.go:44-61`, `../sm-seo-plugin/models/redirect.go:227-265`. The same scope backs MCP `list_not_found?status=` (`controllers/api/notfound.go:116`). @@ -572,7 +572,7 @@ The code comment admits the pattern/regex gap. The trailing-slash and query gaps Also document the remaining gap in the MCP tool's `jsonschema` text. -### WR-C04: The URL rule address is uncapped but sits in a btree UNIQUE index, so long values return 500 instead of 422 +### WR-23: [WR-C04] The URL rule address is uncapped but sits in a btree UNIQUE index, so long values return 500 instead of 422 **File:** `../sm-seo-plugin/models/rule.go:102-121`; `../sm-seo-plugin/updates/202610270005_create_golem15_seo_rules.go:28` @@ -592,7 +592,7 @@ if len(url) > MaxRuleURL || !strings.HasPrefix(url, "/") || ... { } ``` -### WR-C05: The robots element's grammar drifts from the Go grammar, and an unparseable value is silently wiped on the next edit +### WR-24: [WR-C05] The robots element's grammar drifts from the Go grammar, and an unparseable value is silently wiped on the next edit **File:** `../sm-seo-plugin/assets/src/elements/robots.ts:39-98, 136-145`; test claim in `tests/elements/robots.test.ts:75-79` @@ -615,13 +615,13 @@ Today the server normalises the values it writes, so this needs a value written - On a `null` parse, keep the raw value, render an "unrecognised value" note and disable the controls, or require an explicit reset, rather than showing "Inherit". - Add `unavailable_after:2027-02-30` to the TS malformed list. -### WR-C06: The element tests never exercise the host protocol; the tester and the image element have no tests +### WR-25: [WR-C06] The element tests never exercise the host protocol; the tester and the image element have no tests **File:** `../sm-seo-plugin/tests/elements/serp.test.ts:27-38`; no `robotstester.test.ts` or `image.test.ts` exists. **Issue:** - The `preview()` helper appends the element, checks that `{op:'context'}` was emitted, then dispatches a `summer-result` itself. It never sets or clears `busy`, never sets `state=error`, and never models the host dropping or answering asynchronously. -- The `inflight`/`queued`/`flush` logic in serp, robotstester and transfer is therefore untested, which is how CR-C01 shipped. +- The `inflight`/`queued`/`flush` logic in serp, robotstester and transfer is therefore untested, which is how CR-04 (CR-C01) shipped. - `golem15-seo-robotstester` (the queue, the `urlRequired` error, the context rebuild) and `golem15-seo-image` (upload id matching, error display, `safeImageURL`) have no tests at all. **Fix:** @@ -631,7 +631,7 @@ Today the server normalises the values it writes, so this needs a value written ### Info -### IN-C01: The regex length refusal puts an untranslated English phrase into the Polish UI +### IN-15: [IN-C01] The regex length refusal puts an untranslated English phrase into the Polish UI **File:** `../sm-seo-plugin/models/redirect.go:156` @@ -639,7 +639,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Add a `redirects.regex_too_long` phrase (en/pl) with `:max`, and use it here. -### IN-C02: A migration comment is stale +### IN-16: [IN-C02] A migration comment is stale **File:** `../sm-seo-plugin/updates/202610270003_create_golem15_seo_settings.go:8-11` @@ -647,7 +647,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Delete the sentence. -### IN-C03: The SERP preview's template rendering is not single-pass like the Go renderer +### IN-17: [IN-C03] The SERP preview's template rendering is not single-pass like the Go renderer **File:** `../sm-seo-plugin/assets/src/elements/serp.ts:44-53` @@ -655,7 +655,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Use one pass: `tpl.replace(/%(title|description|site)%/g, (_, k) => vars[k] ?? '')`. -### IN-C04: `plural()` can throw on an unusual `ui-locale` +### IN-18: [IN-C04] `plural()` can throw on an unusual `ui-locale` **File:** `../sm-seo-plugin/assets/src/elements/labels.ts:402-411` @@ -663,7 +663,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Format the count through the existing `num()` helper, which already catches. -### IN-C05: Two test names do not describe what they assert +### IN-19: [IN-C05] Two test names do not describe what they assert **File:** `../sm-seo-plugin/classes/owner_test.go:31`; `../sm-seo-plugin/classes/resolver_test.go:19-53` @@ -673,7 +673,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Rename the owner case. Either drop the synthetic chain loop or derive the chains from the resolver's actual candidate lists. -### IN-C06: The inner controls' accessible name is the machine field name +### IN-20: [IN-C06] The inner controls' accessible name is the machine field name **File:** `../sm-seo-plugin/assets/src/elements/text.ts:85-88`; `../sm-seo-plugin/assets/src/elements/robotstxt.ts:174-177` @@ -681,7 +681,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Have the host pass a translated `field-label` attribute and use that, or rely on `aria-labelledby` pointing at the host's label id. -### IN-C07: The README test instructions skip the vitest suite, and the test script hides an empty run +### IN-21: [IN-C07] The README test instructions skip the vitest suite, and the test script hides an empty run **File:** `../sm-seo-plugin/README.md:227-236`; `../sm-seo-plugin/package.json:11` @@ -695,7 +695,7 @@ Today the server normalises the values it writes, so this needs a value written - Drop `--passWithNoTests`. - Run `npm ci` unconditionally in `check-dist.sh`. -### IN-C08: Three partials are maintained twice +### IN-22: [IN-C08] Three partials are maintained twice **File:** `../sm-seo-plugin/extension/_seo_effective.htm` / `controllers/rules/_seo_effective.htm`; `extension/_seo_status.htm` / `controllers/rules/_seo_status.htm`; `extension/_type_placeholders.htm` / `controllers/settings/_placeholders.htm` @@ -703,7 +703,7 @@ Today the server normalises the values it writes, so this needs a value written **Fix:** Keep one copy and resolve the other path to it, or add a test that asserts the pairs stay equal. -### IN-C09: The `disabled` branches in the widget elements are dead, and would be broken if reached +### IN-23: [IN-C09] The `disabled` branches in the widget elements are dead, and would be broken if reached **File:** `../sm-seo-plugin/assets/src/elements/transfer.ts:265, 296, 378`; `robotstester.ts` `syncControls`; `styles.css:805-809`