diff --git a/.planning/phases/19.3-framework-transport-and-infrastructure-for-wavepath/19.3-REVIEW.md b/.planning/phases/19.3-framework-transport-and-infrastructure-for-wavepath/19.3-REVIEW.md new file mode 100644 index 0000000..6bd56f4 --- /dev/null +++ b/.planning/phases/19.3-framework-transport-and-infrastructure-for-wavepath/19.3-REVIEW.md @@ -0,0 +1,254 @@ +--- +phase: 19.3-framework-transport-and-infrastructure-for-wavepath +reviewed: 2026-10-15T18:16:47Z +depth: standard +files_reviewed: 35 +files_reviewed_list: + - go.mod + - internal/build/build.go + - internal/build/manifest.go + - modules/bouncer/cookie.go + - modules/bouncer/turnstile.go + - modules/cabana/reorder.go + - modules/cooler/cooler.go + - modules/cooler/memory.go + - modules/cooler/ratelimit.go + - modules/cooler/redis.go + - modules/cooler/rediscache.go + - modules/cooler/sitecache.go + - modules/lagoon/attach/bucket.go + - modules/lagoon/attach/diskext.go + - modules/lagoon/attach/s3_off.go + - modules/lagoon/attach/s3_on.go + - modules/lagoon/attach/store.go + - modules/lagoon/attach/thumb.go + - modules/pact/capabilities.go + - modules/surf/apidocs.go + - modules/surf/cors.go + - modules/surf/router.go + - modules/surf/serve.go + - scripts/check-phase19.3.sh + - modules/cooler/memory_test.go + - modules/cooler/install_test.go + - modules/cooler/ratelimit_test.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/controllers/site_cache.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-faq-plugin/controllers/faqs.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/site_cache.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/menus.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/menuentries.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-menu-plugin/controllers/admin_registry.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/classes/site_cache.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/controllers/site_cache.go + - /media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin/plugin.go + - /media/nvme/dev/golem15/summercms.io/summercms/vue-faq-layer/server/lib/siteCachePurge.ts + - /media/nvme/dev/golem15/summercms.io/summercms/vue-faq-layer/server/api/__site__/cache/purge.post.ts +findings: + critical: 1 + warning: 9 + info: 11 + total: 21 +status: issues_found +--- + +# Phase 19.3: Code Review Report + +**Reviewed:** 2026-10-15T18:16:47Z +**Depth:** standard +**Files Reviewed:** 35 (framework sources, the plugins' purge wiring, the shared Nuxt purge route; tests got a lighter pass) +**Status:** issues_found + +## Narrative Findings (AI reviewer) + +## Summary + +The phase is careful in most of the places the brief asked about. The Redis Lua script is atomic and matches `surf.MemoryStore`: a missing key or one without a TTL opens the window, a denied attempt does not increment, and INCR keeps the TTL. Keys are namespaced `rl:` / `cache:`, and Flush escapes SCAN glob characters. The S3 driver is linked only under `summercms_s3`, and an untagged binary refuses `s3://` at boot. Build tags are validated by regex and passed as a single exec argument. The api-docs template escapes the title and spec URL, and the assets carry SRI. The purge token is compared in constant time over sha256 digests. Every cabana write path that reaches the plugins' hooks (`crud.go:134`, `reorder.go:319`) now runs in `lagoon.Transaction`, so after-commit semantics hold. + +The defects are at the edges between modules: + +- The `cors:` middleware runs in declaration order. Responses from the guard, throttle, Turnstile, body limit or panic recovery therefore carry no CORS headers, and the browser cannot read them (CR-01). +- The site-cache purge runs synchronously inside the admin request after commit, and bulk deletes repeat it per row. +- The purge client follows redirects with the bearer token attached. +- The shipped Nuxt route returns 404 to every proxied request, so the https mode that the Go side advertises cannot be used. +- Several hardening gaps remain in credentialed CORS validation, cookie Secure inference, the api-docs CSP and Turnstile misconfiguration visibility. + +Already logged in `deferred-items.md` and not counted again: S3 object metadata keeps the sniffed `text/html` / `text/javascript` content type; Redis outage latency in `Cache.Remember` plus go-redis stderr logging; the unreachable "in transaction but no purger" plugin branch; the gofmt and consumer-name items. + +## Critical Issues + +### CR-01: `cors:` headers are missing from every response produced outside the profile middleware (401, 403, 413, 422, 429, 500) + +**File:** `modules/surf/router.go:416-466`, `modules/surf/cors.go:255-262` +**Issue:** `wrap` composes route middleware so that `rt.middleware[0]` is outermost (`for i := len-1; i >= 0; i-- { h = mw(h) }`). It then puts `bodyLimit`, `r.locale` and `recoverJSON`/`recoverBare` outside all of them. `profileMiddleware` only sets `Access-Control-Allow-Origin` when the request reaches it. With the ordering the docs suggest (`docs/services/routing.md:265`: "lists `cors:spa` among its middleware names, next to its guard"), a route such as `[]string{"auth", "throttle:60,1", "turnstile:open", "cors:spa"}` answers: +- a guard refusal (401, expired token), +- a throttle denial (429), +- a Turnstile refusal (422/503), +- a body-limit trip (413, always outside), and +- a recovered panic (500, always outside) + +all without ACAO, ACAC or `Vary: Origin`. The browser turns each of these into an opaque network error. A credentialed SPA, which is the use case profiles exist for, can no longer see the 401 that should trigger its cookie refresh, or the 422 that should re-show the Turnstile widget. The global `pathScopedCORS` wrapper does not have this problem because it runs before the mux. +**Fix:** Apply the profile outside every other route layer, whatever position it has in the list. In `wrap`, take the `cors:` entry out of the loop and wrap it last, after `recoverJSON` and before `recordPattern`: +```go +profile, _ := routeCORSProfile(rt) // already validated in addCORSPreflights +// ... loop skips names with base "cors" ... +if rt.raw { h = recoverBare(h) } else { h = recoverJSON(h) } +if profile != "" { + h = r.corsProfiles[profile].profileMiddleware(h) +} +return recordPattern(rt.method+" "+rt.path, h), nil +``` +Add a test that sends a cross-origin request to a `cors:spa` route behind a refusing guard and asserts that the 401 carries `Access-Control-Allow-Origin`. + +## Warnings + +### WR-01: The site-cache purge runs synchronously in the admin request, and bulk delete multiplies it + +**File:** `modules/cooler/sitecache.go:123-195`, `modules/lagoon/transaction.go:86,194-205`, `modules/cabana/crud.go:1025-1040` +**Issue:** `lagoon.Transaction` runs after-commit callbacks inline (`runAfterCommit` after `gdb.Transaction` returns). `PurgeSiteCacheAfterCommit` registers `p.Purge`, which is a blocking HTTP POST with a 2 s bound. Every admin save therefore waits for the Nuxt site. When the site is down or slow, each save takes up to 2 s longer. `BulkDelete` calls `deleteRecord` per row, and the FAQ, menu and menu-entry hooks each register a purge, so deleting N rows sends N sequential full-cache purges after commit. With N=50 and a hung site that is about 100 s, which is past typical server write timeouts. The client then sees a failed request for a delete that has committed. Each purge also wipes the whole `nitro:routes` cache (`siteCachePurge.ts:62-71`), so after the first purge the remaining N-1 do nothing. +**Fix:** Coalesce per transaction and send asynchronously. One option is a purger method that marks "purge pending" in the after-commit buffer once per transaction (dedupe on the `*SiteCachePurger`). Another is to make `Purge` enqueue onto a single background worker with a small debounce, for example: +```go +func (p *SiteCachePurger) Purge(ctx context.Context, kind, slug string) { + if p == nil { return } + select { case p.queue <- siteCachePurgeBody{kind, slug}: default: /* one already pending */ } +} +``` +Document that the purge is fire-and-forget. + +### WR-02: The purge client follows redirects and forwards the bearer token, including https-to-http on the same host + +**File:** `modules/cooler/sitecache.go:93,148` +**Issue:** `&http.Client{}` uses the default redirect policy. Go forwards `Authorization` on a redirect when the destination hostname equals the original or is a subdomain of it, and it ignores the scheme and port. A purge URL `https://site.example.com/api/__site__/cache/purge` that answers 301/302/307/308 to `http://site.example.com/...` (a common proxy misconfiguration, or a trailing-slash redirect behind a TLS terminator) therefore sends the token in clear. `NewSiteCachePurgerFor` (lines 87-89) exists to prevent exactly that. A 301/302 also turns the POST into a GET, so the purge silently does nothing. +**Fix:** +```go +client: &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }}, +``` +A 3xx then logs as `site cache purge failed status=30x`. + +### WR-03: The shipped purge route refuses every proxied request, so the https/remote mode the Go side advertises cannot work + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/vue-faq-layer/server/api/__site__/cache/purge.post.ts:109-121`, `modules/cooler/sitecache.go:60-63,87-89` +**Issue:** On the Go side, `site_cache.purge_url` accepts any https host ("use https for any other host"). The route that all three layers ship returns 404 whenever `X-Forwarded-For`, `X-Real-IP` or `Forwarded` is present. A remote https endpoint is reached through a TLS-terminating proxy almost by definition, and nginx, Caddy and Traefik add those headers by default. The configuration therefore passes boot validation and fails every save with `status=404`. Operators get a warning per save and no explanation. The two halves of one feature disagree about the threat model. +**Fix:** Pick one model. Either restrict `purge_url` to loopback hosts in `NewSiteCachePurgerFor` (drop the https/remote allowance and its docs), or let the route accept proxied requests when an explicit runtime flag is set (for example `siteCache.allowProxied`), relying on the bearer token alone, and document the pairing in `docs/services/caching.md`. + +### WR-04: Credentialed CORS validation only refuses a bare `*`; a wildcard entry or an unanchored pattern still trusts arbitrary origins with credentials + +**File:** `modules/surf/cors.go:45-58,79-88,120-137` +**Issue:** `validateCORSConfig` rejects `supports_credentials: true` only when `allowed_origins` contains exactly `"*"`. These still pass: +- `allowed_origins: ["https://*"]` or `["*://*"]`, which `wildcardOriginPattern` turns into `^https://.*$` and which trusts every https origin with credentials; +- `allowed_origins_patterns: ["https://.*\\.example\\.com"]` (delimiters stripped, as the error message tells PHP users to do, but without `^`/`$`), which `MatchString` treats as a substring match, so `https://x.example.com.attacker.net` is reflected with `Access-Control-Allow-Credentials: true`. + +The phase made "credentials with `*` is a boot error" a deliberate hardening step (02-SUMMARY), and these are the same exposure under a different spelling. +**Fix:** When `SupportsCredentials` is true: +- require every `allowed_origins_patterns` entry to start with `^` and end with `$` (or anchor them yourself with `^(?:` + p + `)$` in `newCORSPolicy`); +- refuse wildcard origins whose host part is only `*` (for example, require at least one literal label after the `*`, as in `https://*.example.com`). + +Add these cases to `TestCORSCredentialsWildcardRefused`. + +### WR-05: In production, the cookie's Secure flag is inferred false without an error when `app.url` is http or unset + +**File:** `modules/bouncer/cookie.go:100-107,116-121` +**Issue:** The production guard only refuses an *explicit* `auth.cookie.secure: false`. When the key is unset, `cookieSecure` derives Secure from `app.url`. If `app.url` is missing, mistyped or `http://` (common when TLS terminates at a proxy and `APP_URL` was copied from a dev `.env`), production writes the httpOnly auth token cookie without `Secure`, and nothing errors or warns. `SameSite=None` is caught; Lax/Strict are not. +**Fix:** In the production environment, treat an inferred `secure=false` like an explicit one: +```go +if !secure && strings.EqualFold(strings.TrimSpace(src.Environment()), "production") { + return cfg, errors.New("bouncer: auth.cookie.secure resolves to false in production (app.url is not https); set app.url to https or auth.cookie.secure: true") +} +``` +At the least, log a warning naming `app.url`. + +### WR-06: The api-docs CSP allowlists all of jsDelivr for scripts, which cancels the nonce + +**File:** `modules/surf/apidocs.go:207` +**Issue:** `script-src 'nonce-…' https://cdn.jsdelivr.net` lets any script from jsDelivr execute, and that includes every npm package and every GitHub repo (`/gh//`). Any HTML injection on the page (Swagger UI renders `description` markdown from the spec, and Swagger UI has a history of DOM XSS) can load attacker code from jsDelivr without a nonce. The SRI attributes protect only the three tags that carry them. The policy also has no `default-src`, `style-src` or `connect-src`. +**Fix:** Add the nonce to the three external `