fix(security): swap script-src 'unsafe-inline' for a per-request CSP nonce - #11220
Conversation
The hash version was built, measured and rejected a day earlier (#11213): Cloudflare JavaScript Detections injects an inline script at the edge whose body carries a per-response ray id, so it has no hash that can be listed, and a hash-only script-src blocked exactly that one script. Cloudflare documents the way out — it copies a nonce out of the response header onto the script it injects — and that is what this ships. nginx mints the nonce from $request_id (16 random bytes as 32 hex digits, a subset of the CSP nonce charset), sends it in the policy and stamps the same value onto every <script> tag with sub_filter, at server level in both server blocks because four different locations can end up serving the shell. Three things keep the two halves from drifting apart: the shell is no-store and sub_filter clears ETag and Last-Modified on its own, so a nonced body can never be replayed under a fresh header; index.html is excluded from build-time precompression, because gzip_static would serve an unstamped .gz; and the deploy smoke refuses to promote a candidate whose script tags do not carry the nonce from their own response header. 'strict-dynamic' stays out: it drops 'self' for scripts, and the built shell links its chunks with <link rel="modulepreload"> — link elements the stamp does not touch and trust propagation does not cover. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🟡 Changes recommended
Some shell routes remain cacheable, and several safeguards and rollback instructions do not enforce their stated guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces inline-script CSP allowances with per-request nginx nonces and adds deployment safeguards.
Changes:
- Stamps CSP nonces onto shell scripts and prevents precompressed shell delivery.
- Adds policy regression tests and deployment smoke checks.
- Documents deployment rollback procedures.
File summaries
| File | Description |
|---|---|
app/nginx.conf |
Stamps per-request nonces. |
app/security-headers.conf |
Enforces nonce-based CSP. |
app/vite.config.ts |
Excludes the shell from precompression. |
app/cloudbuild.yaml |
Adds nonce deployment checks. |
app/index.html |
Adjusts a nonce-sensitive comment. |
tests/unit/api/test_csp_policy.py |
Adds CSP configuration guards. |
agentic/docs/project-guide.md |
Documents rollback procedures. |
changelog.d/csp-nonce.md |
Records the security change. |
Review details
Suppressed comments (2)
tests/unit/api/test_csp_policy.py:260
- This guard encodes the wrong failure mode. Vite's external module entry is itself a
<script>and receives the nonce, so it remains trusted under'strict-dynamic'; if a modulepreload hint is refused, the entry still executes and fetches its imports later, causing a performance regression rather than a blank SPA. Requiring an arbitrary<linksubstitution would reject a valid policy and does not prove the preload is authorized; remove or rework this guard and the matching policy explanation.
assert re.search(r"sub_filter\s+'<link", conf), (
"script-src carries 'strict-dynamic', which drops 'self' for scripts, but "
"app/nginx.conf still stamps only <script> tags. Vite's <link "
'rel="modulepreload"> chunk hints would be blocked and the SPA would never '
agentic/docs/project-guide.md:1088
- This rollback guarantee expires after the next frontend deployment: at that point the previous revision also serves the nonce policy, not
'unsafe-inline'. The procedure should identify a recorded pre-nonce revision or another durable known-good target rather than assuming the immediately previous revision always predates this change.
image and only reveals itself in production. The current example is the CSP nonce path in
`app/security-headers.conf`: if Cloudflare ever stops stamping its edge-injected script
with the nonce it reads from our response header, the previous revision still serves the
`'unsafe-inline'` policy byte for byte.
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Four findings, all real, and one of them a bug the local probes had already walked past: * python.anyplot.ai/<spec> and /<spec>/<library> serve the shell as a FILE through `try_files /index.html =404`, inside their own location and without the internal redirect that would reach `location = /index.html`. Measured: those two routes answered with no Cache-Control at all while the main host sent `no-store` — a nonced body that a client may keep. Both now send the shell's Cache-Control and re-include the header snippet. * The deploy smoke counted `nonce=` attributes instead of comparing them, so a tag carrying a DIFFERENT nonce passed while the browser would refuse it. Reproduced against a local nginx stamping `$connection_requests$msec`: the old form saw "0 tags without a nonce", the new one fails with "0 of 7 tags carry the header's nonce". * The rollback pointed at "the previous revision", which stops being the pre-nonce one after the next deploy. It now says to pick by creation time, and says what the lever becomes when no such revision is left. * The API service is anyplot-api, not anyplot-backend. Also corrected two claims rather than defending them: 16 random bytes are exactly 128 bits and CSP recommends rather than requires them, and 'strict-dynamic' would cost the modulepreload hints — a console full of violations and a slower first paint — not the whole SPA, because the entry module is itself a nonced <script>. `test_the_shell_is_never_stored` now finds every location that answers with the shell, by reading the try_files argument positions rather than the one exact match it happened to know about. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
Four locations serve the shell, and the assertion said three while its message named four. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
There was a problem hiding this comment.
🟡 Changes recommended
The deployment probe does not request compression, so it cannot detect the precompressed-shell failure it claims to prevent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot's second round found the probe blind to the failure it was written
for. Plain curl sends no Accept-Encoding, so `gzip_static` never reaches for
the `.gz` — and a precompressed shell coming back is the single most likely
way to lose the stamp. Reproduced against a local nginx with an index.html.gz
planted in the docroot:
plain curl 7 stamped tags → the smoke passed
curl --compressed 0 stamped tags → what a browser gets
`--compressed` on that one fetch closes it; the probe now fails with
"0 of 7 <script> tag(s) carry the header's nonce" against the same planted
file and passes on the real build.
Also: the rollback in project-guide.md is a numbered procedure with one action
per step, per the repository's Google-style convention, instead of two
commands separated by comments inside one block. And the changelog records the
python.anyplot.ai Cache-Control gap as its own Fixed entry — it predates this
PR (those routes never sent one) and is worth its own line.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
There was a problem hiding this comment.
🔵 Needs a closer look
The strict-dynamic guard incorrectly treats nonce-stamped modulepreload links as CSP-authorized.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
app/security-headers.conf:85
- The proposed remediation does not work: CSP nonce matching does not authorize
<link rel="modulepreload">, and'strict-dynamic'trust propagation applies to scripts created by a trusted script, not parser-created link elements. Wideningsub_filterto<link>would therefore still leave these preload requests blocked; document a tested redesign or removal of the preload hints instead.
tests/unit/api/test_csp_policy.py:174 - This repeats the entropy overstatement already corrected in
security-headers.conf: CSP recommends at least 128 bits; it does not define 128 bits as a ceiling. Keep the test rationale consistent with the policy documentation.
This issue also appears on line 302 of the same file.
tests/unit/api/test_csp_policy.py:306
- This assertion codifies the same ineffective fix: it passes when any
<link>stamp exists, although nonce attributes do not authorize modulepreload links under CSP. Reject'strict-dynamic'while Vite emits parser-created modulepreloads, or test a proven preload-policy redesign instead.
assert re.search(r"sub_filter\s+'<link", conf), (
"script-src carries 'strict-dynamic', which drops 'self' for scripts, but "
"app/nginx.conf still stamps only <script> tags — so Vite's <link "
'rel="modulepreload"> chunk hints are refused and every chunk waits for the '
"entry module to ask for it. Stamp <link> too, or drop the keyword."
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…loor Review round three raised two points about text, not behaviour, and they go opposite ways. Taken: the test docstring still called 128 bits "the ceiling of what the spec asks for" after security-headers.conf had already been corrected. CSP RECOMMENDS at least 128 bits; both places now say so in the same words. Not taken: that a nonce cannot authorize a `<link rel="modulepreload">`, and that the strict-dynamic guard therefore encodes an ineffective fix. A preload request carries its link element's nonce as cryptographic nonce metadata and a nonce-source in script-src matches it — which is exactly why React #26781, Next.js #64091, Vite #9719 and Rails #53794 all carry the same report and the same fix, "put the nonce on the preload link". Widening the stamp to <link> is the remedy, and the guard stands. Both files now name that evidence so the question does not have to be re-litigated by the next reader. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
|
Round three left no threads, and its three suppressed notes split two ways. Taken — the entropy wording. Not taken — "a nonce does not authorize A preload request carries its link element's nonce as the request's cryptographic nonce metadata, and a
If the nonce did nothing for preload links, none of those fixes would work. So widening the Nothing about the shipped policy changes either way — State: all checks green on every round, all seven review threads replied to and resolved, no open threads. Ready when you are. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes enforced CSP and production deployment behavior, and the precompression test remains overly permissive.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tests/unit/api/test_csp_policy.py:250
- This guard only checks whether the text after
excludecontainsindex, so patterns such as/index\.js$/or/not-index\.html$/still pass even though Vite would emitindex.html.gz. Match the actualindex.htmlexclusion (or inspect the built outputs) so the test enforces the invariant it documents.
agentic/docs/project-guide.md:1101 - Repository prose follows the Google style rules referenced by
.github/copilot-instructions.md:10, which prohibit all-caps emphasis. Use sentence case here instead ofNOT.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Two last review notes, both fair. The precompression guard looked for "index" anywhere after `exclude`, so `/index\.js$/` and even `/not-index\.html$/` would have satisfied it while `dist/index.html.gz` was written all the same. It now matches the pattern, and the four cases that distinguish those are checked. And the rollback section says "deliberately not built" in sentence case, per the Google style rules the repository follows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
There was a problem hiding this comment.
🟡 Changes recommended
The precompression test and deployment nonce probe both permit false-positive validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
Round four, both findings real and both about a check that could pass on
something invalid.
The smoke extracted the nonce with `nonce-[0-9a-f]*`, which matches a bare
`nonce-`, and `test -n` calls that non-empty. A policy with an empty nonce and
`nonce=""` on every tag would then have agreed with itself all the way through
the probe while the browser blocked every script. It now requires the 32 hex
digits nginx's $request_id always is:
old extraction on a `'nonce-'` header: ['nonce-'] -> passed
new extraction: [] -> fails
And the precompression guard now RUNS the declared exclude pattern against
"index.html" instead of reading it for the word "index". Two rounds found the
reading version accepting patterns that exclude nothing — /index\.js$/,
/not-index\.html$/, and /foo/index\.html$/ — which is a sign the approach was
wrong rather than the spelling. Eight cases checked, including those three and
a flagged /INDEX\.HTML$/i.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
There was a problem hiding this comment.
🔵 Needs a closer look
The edge-dependent security rollout needs human validation, and its server-scope regression guard remains incomplete.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tests/unit/api/test_csp_policy.py:231
- This does not actually enforce server-level placement: each full server block also contains nested location includes, so moving the stamp into only
location = /index.htmlstill satisfies both searches while the Python in-place shell routes remain unstamped. Restrict the assertions to top-level server directives so this regression cannot pass the unit test (the deploy smoke only probes the main/route).
changelog.d/csp-nonce.md:23 - This change adds one additional curl invocation—the existing shell probe was already present—so “two extra curls” overstates the deploy cost recorded in the changelog.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Round five, no threads, two suppressed notes and both worth taking.
`test_every_server_block_stamps_the_nonce` searched the whole server block,
nested locations included, so moving the stamp into `location = /index.html`
would have satisfied it — the exact regression it exists to refuse, and the
deploy smoke would not have caught it either since it probes `/` on the main
host. `_server_level_of` now cuts every nested `location { … }` out first.
Mutation-checked: with the stamp moved into that one location, the test fails
with "these server blocks do not stamp the CSP nonce at server level".
And the changelog said "two extra curls" where the nonce probe adds one.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive delivery change depends on Cloudflare’s production-only nonce injection behavior and requires final live verification.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…1221) ## Summary - **The second door has a gate now, and it ships switched off.** `api/origin_gate.py` closed the API service's door and wrote the app service's door down as the thing it could not close from its own side: `anyplot-app` stands with `ingress=all`, serves the whole site from `https://anyplot-app-r3tvmejsmq-ez.a.run.app` with no bot challenge, no WAF and no rate limit, and relays any crawler user agent through `@seo_proxy` to `https://api.anyplot.ai` — where the edge stamps the API's secret *legitimately*, so the API gate cannot tell. `app/origin-gate.conf.template` is the nginx half of the same mechanism: same secret, same five verdicts, `ORIGIN_GATE` unset = off. - **Nothing is armed by merging this.** The image defaults `ORIGIN_GATE=off`, the service declares no environment variables at all, and `/_health` already reports `X-Origin-Gate` — so every route into the container can be measured before anything is switched on. The runbook is in `infra/cloudflare/README.md` § "The site's own origin", summarised at the bottom here. - **Three callers reach that origin without the edge and each now stamps its own header**, plus one that turned out to be a live production path nobody had connected to the gate: the apex Worker's `/api/event`. ## The mechanism, in three sentences `app/origin-gate.conf.template` renders into `/etc/nginx/conf.d/00-origin-gate.conf` at container start, via the entrypoint script `20-envsubst-on-templates.sh` that `nginxinc/nginx-unprivileged:alpine` already ships — no start script of ours, and `NGINX_ENVSUBST_FILTER=^ORIGIN_` keeps envsubst away from `$host` and `$uri`. Seven `map` blocks turn the presented header into `$origin_gate_status` (`off` · `off-seen` · `ok` · `missing` · `mismatch`, the API's own vocabulary) and `$origin_gate_deny`; every server block in `app/nginx.conf` carries `if ($origin_gate_deny) { return 421; }` at server level, which runs in the server-rewrite phase and therefore covers every location the block has and every location added later. The secret is written in exactly one map key, `app/nginx.conf` never names `$http_x_origin_secret` at all, and every `proxy_pass` clears the header so no upstream is ever handed it — three rules with a test each, plus a CI smoke that greps the refusal page and the container log for the value. ## The finding the brief asked for: `Host` would have worked, and still cannot be the mechanism `api/origin_gate.py` guessed that `$host` at this origin might be the `run.app` name for *all* traffic, which would have settled the question. It is not: | | | |---|---| | `gcloud beta run domain-mappings list --region=europe-west4 --project=anyplot` | `anyplot.ai → anyplot-app`, `www.anyplot.ai → anyplot-app`, `api.anyplot.ai → anyplot-api` | They are **Cloud Run domain mappings**, so Cloudflare forwards the original `Host` and `$host` really does distinguish the edge from the raw URL — and the value cannot be spoofed, because Google's frontend answers a foreign Host on a `run.app` address with its own 404 before the container is reached (measured in #11208). A Host rule still cannot be the gate. `bot-serving-check.yml` probes this exact origin with crawler user agents *because* Cloudflare 403s GitHub-runner IPs even for a UA-spoofed Googlebot; it cannot spoof the Host either, and any exception keyed on something it could present instead — a header it invents, a user agent — is public with this repository. The exception has to be the shared secret. Once the workflow carries the secret, the Host rule buys nothing the header does not, so it is not built. ## Every hostname this container serves The Transform Rule has to cover all of them, or arming locks out the visitors it protects. | Hostname | Reaches the container via | Transform Rule | |---|---|---| | `anyplot.ai` | Cloud Run domain mapping, proxied | **required** | | `www.anyplot.ai` | Cloud Run domain mapping, proxied — it **serves the site**, it does not redirect (`curl -sI https://www.anyplot.ai/` → 200, no `Location`) | **required** | | `anyplot.ai/api/event` | the apex Worker, `fetch(request)` back to this origin | none — the **Worker** stamps it (see below) | | `python.anyplot.ai` | nothing today: it is a `server_name` in `app/nginx.conf` with **no DNS record and no domain mapping** (`curl` → `Could not resolve host`, checked 2026-09-04) | add it the day the DNS record is added; the block is gated already | | `anyplot-app-r3tvmejsmq-ez.a.run.app` | direct | none — this is the door being closed | | `anyplot-app-239660669828.europe-west4.run.app` | direct, the service's second URL | none, same door | | `candidate---anyplot-app-r3tvmejsmq-ez.a.run.app` | direct, the pre-traffic tag URL | none — the smoke sends the header itself | The rule must be a **Set**, not an Add: a caller supplying its own `X-Origin-Secret` has to have it replaced. ## Every legitimate direct caller, and what each now sends | Caller | Now sends | Notes | |---|---|---| | `app/cloudbuild.yaml` pre-traffic smoke | `X-Origin-Secret` on every probe, read from Secret Manager **inside the step** | Not `availableSecrets`, which resolves at build start and would fail every build until the secret exists — the same reasoning `api/cloudbuild.yaml` already carries. It also asks `/_health` for the verdict **before** any content probe, so a wired-up-wrong secret is reported as itself instead of as a mystifying 403 on the home page. | | `.github/workflows/bot-serving-check.yml` | the header from the `ORIGIN_SECRET` repository secret, on all ~36 probes | Reads `/_health` first: `missing` and `mismatch` are hard failures with a message naming the secret; `off` is a warning; an absent header is a warning naming deploy lag. Without that, an armed gate plus a missing secret would open an incident saying "every crawler page is broken". | | the apex Worker, `/api/event` | stamps from its own `ORIGIN_SECRET` binding | **The one that would have broken production.** `anyplot.ai/api/event` is the only path under the Worker's route that goes to the *site's* origin instead of the API host, and a Worker subrequest to a host in the same zone skips that zone's Transform Rules — the exact finding `infra/cloudflare/README.md` exists for, biting a second time. Arming without this answers **every Plausible pageview on the site** with a 403, quietly. | | Cloud Run startup probe | nothing, and needs nothing | `gcloud run services describe` → `startupProbe: tcpSocket: port 8080`, `failureThreshold 1`. Not an HTTP probe, so no exemption. | | IndexNow (`indexnow-submit.yml`) + Bing's key verification | nothing, and needs nothing | Both fetch `https://anyplot.ai/<key>.txt`, i.e. through the edge. | | PageSpeed Insights (audit harness) | nothing, and needs nothing | Audits `https://anyplot.ai/...`, through the edge. | Also checked and empty: `gcloud monitoring uptime list-configs --project=anyplot` → `Listed 0 items.` No Lighthouse CI workflow exists. The `ORIGIN_SECRET` repository secret **already exists** (created 2026-09-03 for the API rollout, used by `sync-postgres.yml`), and the build service account `239660669828-compute@developer.gserviceaccount.com` — the account **both** triggers run as — **already holds** `roles/secretmanager.secretAccessor` on it. So no grant and no new secret are needed; the runbook only confirms them. ## Correcting a claim from #11220 That PR's description recorded, as the reason a Cloud Run env-var switch was not built: > `/etc/nginx` is read-only in `nginxinc/nginx-unprivileged` (checked: no `chown`/`chmod g+w` on it, final `USER 101`), so the image's own `NGINX_ENVSUBST_OUTPUT_DIR` mechanism bails out with "not writable" and renders nothing. That reading came from the wrong file in the image's repository. `nginxinc/nginx-unprivileged:alpine` is `FROM nginxinc/nginx-unprivileged:1.31.5-alpine-slim`, and `mainline/alpine/Dockerfile` — the variant that adds the modules — genuinely contains no such lines, because they are in the base it inherits from. `mainline/alpine-slim/Dockerfile`: ```dockerfile # nginx user must own the cache and etc directory to write cache and tweak the nginx config && chown -R $UID:0 /var/cache/nginx \ && chmod -R g+w /var/cache/nginx \ && chown -R $UID:0 /etc/nginx \ && chmod -R g+w /etc/nginx ``` `/etc/nginx/conf.d` is **owned by uid 101**, which is the uid the container runs as. Now confirmed live rather than read: the new CI job's log carries `20-envsubst-on-templates.sh: Running envsubst on /etc/nginx/templates/00-origin-gate.conf.template to /etc/nginx/conf.d/00-origin-gate.conf`. #11220 was right that nothing could exercise it before it mattered — which is what that job is for, and it earned its keep on the first run (below). ## Verification **CI builds the app image and runs the gate against it.** `app/Dockerfile` was hadolinted but never *built* before Cloud Build, i.e. after the merge — and what it produces is not a program that fails to import but an nginx whose config is rendered at container start. The new `app-image` job in `ci-image.yml` runs the real image three ways: | | | |---|---| | gate off | `/` 200; `/_health` → `X-Origin-Gate: off`; with any header → `off-seen` | | armed | `/` 403 bare, 403 with a wrong secret, 200 with the right one; `/_health` still 200 (exempt) and reporting `missing` / `mismatch` / `ok` | | armed, no secret | 403 with no header **and** with an empty one — the tagged map keys, proven | | always | the 403 is the gate's own page; the secret appears in neither that page nor `docker logs`; `nginx -t` validates the rendered config (`nginx -T` would print the secret into the CI log, so it is never run) | **It failed on its first run, with a defect nothing else here could have found.** With the gate armed: ``` nginx: [emerg] could not build map_hash, you should increase map_hash_bucket_size: 64 ``` nginx cannot hash a `map` key longer than one bucket, and the default bucket is the processor's cache line — 64 bytes. The tagged key is `presented:` plus the whole secret, so a 32-byte secret written as hex is 74 characters. **With the gate off the container starts perfectly**, because the key is short then; the failure would have appeared at the exact moment of arming and nowhere earlier. Cloud Run would have kept the previous revision serving, so it would have been a safe failure rather than an outage — but it would have been a failure in the middle of the one procedure this whole PR exists to make undramatic. The template now sets `map_hash_bucket_size 512`, the smoke uses a production-length 64-character secret so the ceiling stays exercised, and a test pins the directive. **The rendered config, parsed by nginx's own grammar.** No Docker in this environment, so `crossplane` (nginx's own config parser) was run over the template rendered three ways and assembled into the same `http {}` context the container has: ``` gate on, secret set → status: ok undefined variables: [] gate off, secret empty → status: ok undefined variables: [] ← the shipping state gate on, secret empty → status: ok undefined variables: [] ← fail-closed ``` Worth noting for calibration: crossplane parses, it does not build hash tables, so it passed the `map_hash` defect cleanly. A text check cannot replace a running container, which is the argument for the CI job. **Static guards**, `tests/unit/api/test_app_origin_gate.py` (14 cases), next door to the API gate's own tests: every server block gated and each with its own `error_page`/`@origin_denied`; the gate before the trailing-slash rewrite; `app/nginx.conf` never naming `$http_x_origin_secret`; every `$origin_gate_*` it reads defined by the template; the secret written once and **tagged**; the hash bucket raised; no `proxy_pass` without clearing the header; the exemption exactly `/_health`; both `/_health` blocks reporting the verdict and re-including the header snippet; the Worker stamping and deleting-before-stamping on `/api/event`; the smoke and the monitor carrying the header; the Dockerfile's three lines. **Local gates:** `uv run pytest tests/unit tests/integration` — 1988 passed. `ruff check .`, `ruff format --check .`, `mypy api core` clean. `uv run python -m tools.changelog check --base origin/main` — 7 fragments well-formed. **No `/verify-frontend` run**, and the reason is worth stating rather than skipping: the changed behaviour is nginx's, there is no SPA change in the diff (no file under `app/src`, no TypeScript at all), and the flow cannot be driven from a browser without the container. The container smoke above is that loop, and the deploy's pre-traffic smoke is the second one. ## Review round Four findings, three of them defects, all applied; threads answered and resolved. - **The gate header was forwarded to every upstream.** The sharpest one. nginx passes incoming request headers to a proxied server by default, so once the Transform Rule stamps this host, `/js/script.js` and `/api/event` would have handed the shared secret to **plausible.io** — and with it the API service's key, since both take the same value. Fixed wider than reported: the rule is now that the header is consumed by this server and never forwarded, so *every* `proxy_pass` in both blocks clears it, and a test refuses a location that proxies without doing so. - **Arming was written as two flags.** This service pins traffic to a named revision (`app/cloudbuild.yaml` promotes with `--to-revisions=<name>=100`), so `gcloud run services update` alone creates an armed revision that serves nothing — and step (e) would have read `off` on a path that carries the header. The runbook is now the API's own block, copied rather than paraphrased: refuse while a Cloud Build is in flight, pin the **serving** image rather than the latest template, `--revision-suffix`, then `update-traffic --to-revisions=…=100`. Rolling back is its own block that looks nothing up. - **`ORIGIN_SECRET:latest` instead of a pinned version.** Cloud Run resolves a secret-backed variable when each *instance* starts, so a rotation reaches new instances while older ones keep the old value — intermittent 403s inside one revision. The block resolves the newest ENABLED version number and refuses if there is none. - **`ORIGIN_GATE=ON` also arms**, because a plain `map` key is matched without regard to case. Kept, and the documentation corrected instead: nobody sets that variable to `ON` without meaning to arm, and the other failure direction — an operator who armed the gate, was told nothing, and still has an open origin — is the one worth avoiding. One more, from CI rather than review: hadolint **DL3064** reads the ENV variable *name* and warns that a secret may be baked into the image. The value is the empty string, and it is declared for the opposite reason — so a service supplying no secret renders a config that refuses everyone rather than one nginx cannot parse. Renaming would silence the rule and break a four-place contract (Secret Manager, the API service, the Worker binding, the repository secret), so the exception sits on an `ENV` instruction of its own with its reason beside it, per the repository's own hadolint convention, and `ci-image.yml`'s claim that `app/Dockerfile` needs no exceptions is updated. ## Two decisions worth reviewing - **`return 421`, not `return 403`.** An `error_page 403` sends the refusal back through the server-rewrite phase, where it hits the same `if` again and loses the custom page. A distinct internal code also keeps this trick apart from the crawler's `418`. nginx generates a 421 of its own only for a coalesced HTTP/2 connection with a mismatched authority, which cannot happen here — Cloud Run speaks HTTP/1.1 to this container (`ports: name: http1`). - **A custom 403 page instead of nginx's stock one.** The stock body prints the exact nginx version to anyone knocking on the raw origin. The refusal is a named location, entered without re-running the server-rewrite phase, and it carries `X-Origin-Gate` so a half-applied rotation says `mismatch` instead of just failing. - **The map comparison is case-insensitive and not constant-time**, unlike `api/secret_compare.py`. nginx lowercases a `map` source before hashing and offers no constant-time primitive. Both are written down at the top of the template; against a random 256-bit secret the answer to both is the entropy, not the comparison. ## Rollout — for the main session Steps (a) and (b) are safe on their own and can sit for days. **The arm and the rollback are full blocks, not one-liners** — they live in `infra/cloudflare/README.md` § "Arming, in full" and § "Rolling back", and mirror the API's. ```bash # (a) merge + deploy. Nothing is armed. curl -sI https://anyplot.ai/_health | grep -i x-origin-gate # expect: off # (b) widen the Transform Rule to anyplot.ai AND www.anyplot.ai (dashboard → # Rules → Transform Rules → Modify Request Header, "Set"), and redeploy the # Worker so its /api/event branch stamps too. Then measure EVERY path — # each must read off-seen before anything is armed: curl -sI https://anyplot.ai/_health | grep -i x-origin-gate curl -sI https://www.anyplot.ai/_health | grep -i x-origin-gate curl -si -X POST -A 'Googlebot' https://anyplot.ai/api/event -d '{}' | grep -i -e '^HTTP' -e x-origin-gate curl -sI https://anyplot-app-r3tvmejsmq-ez.a.run.app/_health | grep -i x-origin-gate # stays off # (c) confirm the two callers (both already exist — nothing to create): gh secret list --repo MarkusNeusinger/anyplot | grep ORIGIN_SECRET gcloud secrets get-iam-policy ORIGIN_SECRET --project=anyplot # the compute SA has secretAccessor gh workflow run bot-serving-check.yml --repo MarkusNeusinger/anyplot # expect "origin gate: off-seen" # (d) arm — infra/cloudflare/README.md § "Arming, in full". In outline: # refuse while a Cloud Build is in flight; resolve the SERVING revision's # image; resolve the newest ENABLED secret VERSION (never :latest); # services update --image=… --update-secrets=ORIGIN_SECRET=ORIGIN_SECRET:$VERSION # --update-env-vars=ORIGIN_GATE=on --revision-suffix="arm-<stamp>"; # services update-traffic --to-revisions="anyplot-app-arm-<stamp>=100". # (e) verify: curl -sI https://anyplot.ai/_health | grep -i x-origin-gate # ok curl -s -o /dev/null -w '%{http_code}\n' https://anyplot.ai/ # 200 curl -s -o /dev/null -w '%{http_code}\n' https://anyplot-app-r3tvmejsmq-ez.a.run.app/ # 403 curl -s -A 'Mozilla/5.0 (compatible; Googlebot/2.1)' https://anyplot.ai/scatter-basic | grep canonical curl -si -X POST -A 'Googlebot' https://anyplot.ai/api/event -d '{}' | head -1 # 202, not 403 gh workflow run bot-serving-check.yml --repo MarkusNeusinger/anyplot # green gcloud run services describe anyplot-app --project=anyplot --region=europe-west4 --format="value(status.traffic)" # (f) rollback — infra/cloudflare/README.md § "Rolling back". Same shape, no # lookups: pin the serving image, --remove-env-vars=ORIGIN_GATE, # --revision-suffix="disarm-<stamp>", promote that revision by name. ``` **Rotation** now touches **five** copies of one value: Secret Manager, the API service, the app service, the Worker binding and the GitHub repository secret. Roll back, rotate, arm again; the gate is off in between, which is the documented safe state. Note also the secret's length ceiling: the map key is `presented:` plus the secret and the bucket is 512 bytes, so a secret past roughly 500 characters would keep the armed revision from ever becoming ready. ## Plan N/A — audit item A30, decided directly. ## Test plan - [x] CI `app-image`: the real image passes the gate matrix — off / armed / armed-with-no-secret, the exempt path, each verdict, the refusal page, and the secret in neither the page nor the logs - [x] `crossplane` parses the rendered config in all three states inside the container's own `http {}`; no undefined variables - [x] `uv run pytest tests/unit tests/integration` — 1988 passed, 14 of them new in `tests/unit/api/test_app_origin_gate.py` - [x] `uv run ruff check .` + `ruff format --check .` + `mypy api core` — clean - [x] `uv run python -m tools.changelog check --base origin/main` — fragment well-formed - [x] the base image's entrypoint, template directory and `/etc/nginx` ownership verified against `nginxinc/docker-nginx-unprivileged` `mainline/alpine-slim/Dockerfile`, then confirmed live in the CI job's entrypoint log - [x] `gcloud run services describe anyplot-app` — no env vars today, `startupProbe` is `tcpSocket` (no HTTP probe to exempt) - [ ] After merge, before arming: every path in step (b) of the rollout reads `off-seen` - [ ] After arming: step (e), including `anyplot.ai/api/event` answering 202 rather than 403 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3 --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
The open item #11213 left behind, closed the way that PR said it had to be: a nonce, not a hash.
Why a nonce is the only thing that can work here
app/index.htmlships three executable inline scripts. A FOURTH arrives after nginx — Cloudflare JavaScript Detections injects one into every HTML response at the edge, and its body carries a per-response ray id and timestamp:A body that differs per response has no hash to list, which is what #11213 measured and why the hash policy was not shipped. Turning JavaScript Detections off is not available either: the Free plan rejects
enable_js=falsewhile Bot Fight Mode is on.Verified against Cloudflare's current docs (JavaScript Detections, read 2026-09-04, and the bots reference page, last updated 2026-08-26):
Two conditions come with it and both hold here: the nonce must arrive in the response header ("JavaScript Detections is not supported with
nonceset via<meta>tags"), and/cdn-cgi/challenge-platform/must be reachable —script-src 'self'covers it, same-origin. Two side effects the docs name are already true of this setup: JSD stripsETagfrom injected responses (we clear it anyway, below), and it skips injection entirely onCache-Control: no-transform, which we do not send.The mechanism
$request_id— nginx's own 16 random bytes rendered as 32 hex digits. Hex is a subset of thebase64-valuecharset the CSP nonce grammar accepts, and 128 bits is at the ceiling of what the spec asks for.app/security-headers.conf):script-src 'self' 'nonce-$request_id' https://cdn.jsdelivr.net.'unsafe-inline'is gone, not kept beside it — a browser ignores it the moment a nonce appears, so the pair is the strict policy wearing a permissive label.app/nginx.conf):sub_filter '<script' '<script nonce="$request_id"'withsub_filter_once off, at server level in both server blocks. Not per-location, because four locations can end up serving the shell — the exact= /index.html, the SPA fallback, and in thepython.anyplot.aiblock two regex routes whosetry_files /index.html =404serves the file in place, with no internal redirect to re-run location matching. A per-location stamp is a stamp missing from whichever one is forgotten, failing on those routes alone.app/vite.config.ts):index.htmlis excluded from both compression plugins.gzip_static onwould hand outindex.html.gzbyte for byte andsub_filter— which only ever sees uncompressed bodies — would silently skip the stamp while the header still demanded it. The hashed chunks keep their.gz/.br; the shell is 11 kB and nginx gzips it on the fly.no-store— the exact= /index.htmlin each server block and the two python-host spec routes, which the review caught serving a nonced shell with noCache-Controlat all (see below) — andsub_filterclearsLast-ModifiedandETagon its own whenever it rewrites a body. Without that, a 304 would replace the stored headers with a fresh nonce while the stored body still carried the old one — the classic nonce-plus-cache failure. Verified below: neither validator is present.'strict-dynamic'is deliberately not adopted. The reason is narrower than it first looks, and the review sharpened it: the keyword makes the browser ignore'self'for scripts, but the entry module is itself a nonced<script>, so it runs and the imports it fetches run with it. What breaks is one layer up —yarn buildlinks every chunk from the shell with<link rel="modulepreload">, and a link element is neither something trust propagation reaches nor something a<script-only stamp touches. Those hints are refused, which costs a console full of violations and a slower first paint, not a blank page. A cost with nothing bought, since the chunks are fingerprinted files under'self'already. So the chunks stay on'self', and a test requires the stamp to be widened to<link>by whoever adopts the keyword.Local verification, behind the real config
nginx 1.24 with
http_sub_module, runningapp/nginx.confitself (only the include path, docroot and port rewritten) over a realyarn build:sub_filtercomes fromngx_http_sub_module, which is not compiled in by default, so it is worth having checked rather than assumed:nginxinc/nginx-unprivileged:alpineinstalls the prebuilt nginx package from nginx.org's Alpine repository, and that binary's own configure line carries--with-http_sub_module(unpackednginx-1.31.5-r1.apkand read the string out ofusr/sbin/nginx). Had it been missing, nginx would have refused to start on "unknown directive" — caught by the candidate, but only there.The one cosmetic finding on the way:
sub_filteris a literal string match, so the HTML comment that documented the Eruda loader with the wordsPlain <script>came back carrying a straynonce=inside a comment. Harmless, but confusing in exactly the artefact anyone debugging CSP reads, so the comment now describes the tag instead of spelling it.Guards, so this cannot rot quietly
tests/unit/api/test_csp_policy.py— the hash test is retired with the hashes (they guarded a reserve that no longer exists), and six checks take its place. Every one was mutation-checked: each mutation below was applied to a copy and the named test was confirmed to fail.$connectiontest_the_header_and_the_stamp_name_the_same_variablesub_filterremoved from the python server blocktest_every_server_block_stamps_the_noncesub_filter_once offdroppedexcluderemovedtest_the_shell_is_never_precompressed'unsafe-inline'put back beside the noncetest_script_src_never_mixes_unsafe_inline_with_a_nonce_or_hashtest_the_policy_takes_its_nonce_from_a_per_request_variable'strict-dynamic'added without widening the stamptest_strict_dynamic_would_have_to_widen_the_stamp_to_the_module_preloadsno-storeremoved from the exact= /index.htmltest_the_shell_is_never_storedno-storeremoved from ONE python-host spec routetry_filesThat exercise found a real defect in the first draft:
header_nonce_variable()read the nonce with a regex over the whole conf file, andsecurity-headers.confquotes'nonce-…'in its own prose — so with the nonce deleted from the actual directive the test still "found" one and passed. It now reads the parsed directive.And the deploy smoke refuses to promote without it.
app/cloudbuild.yamlnow fetches the candidate's shell, pulls the nonce out of itsContent-Security-Policyheader, and requires that every<script>tag carries that exact value. This is the failure with no other symptom: the page still arrives, still has its<div id="root">, still passes every existing probe — and runs no inline script at all.That probe was run four ways against local nginx, using the step's own bash with Cloud Build's
$$resolved. Against the config in this PR:against the same config with the two
sub_filterdirectives deleted:against a config that stamps a different variable than the header — the case review round one showed the probe waving through, because it counted
nonce=attributes instead of comparing them:and with an
index.html.gzplanted back into the docroot, which is round two's finding and the one that mattered most: plaincurlsends noAccept-Encoding, sogzip_staticnever reaches for the.gzand the probe was blind to the exact regression it exists for.With
--compressedon that fetch it fails on the planted file (0 of 7 … nonce-ffb48a58…, exit 1) and passes once the.gzis gone.Rollback
Under a minute, no rebuild, back to today's policy byte for byte:
Pick the target by creation time, not by position — "the previous revision" is the pre-nonce one only until the next frontend deploy, which the review flagged. Once no pre-nonce revision is left, the lever is a revert PR through the normal pipeline; acceptable, because by then the live question has long been answered. Written down in
agentic/docs/project-guide.md§ Rollback and, in short form, at the directive it protects inapp/security-headers.conf.A Cloud Run env var switch was considered and deliberately not built, and that is a deviation from the brief worth stating plainly. nginx cannot read the process environment from its configuration, so an env-var switch needs a startup templating step —
envsubstinto a writable path, and/etc/nginxis read-only innginxinc/nginx-unprivileged(checked: nochown/chmod g+won it, finalUSER 101), so the image's ownNGINX_ENVSUBST_OUTPUT_DIRmechanism bails out with "not writable" and renders nothing. The alternative is a hand-rolled entrypoint writing config into/tmp. Either way its failure mode is "the container does not start", and there is no Docker in this environment and no CI job that builds this image, so nothing could exercise it before it mattered. The traffic split has the same latency, needs no new machinery, and the deploy pipeline proves the previous revision healthy on every build.What must still be measured in production, and what it would mean
Whether Cloudflare actually stamps the nonce is not observable from here — it depends on the CSP header the edge sees from the origin, so a local proxy cannot fake it. After merge and deploy, on
https://anyplot.aiin a real browser: the JSD script present and carrying the nonce, zero CSP violations on the landing page and a plot page, Plausible events still firing, Bot Fight Mode still active.One thing to look for specifically. The injected script creates a hidden
about:blankiframe and inserts a second inline script into that document:An
about:blankdocument inherits its embedder's CSP, so that inner script needs the nonce too — and there is an open Cloudflare community report that JSD does not propagate the nonce into that frame. If the outer script is nonced and only the inner one is refused, that is an upstream gap in a defence-in-depth signal and not a site regression; if the outer script is refused too, Cloudflare is not honouring the nonce at all, and the rollback above is the answer.Review rounds
Six rounds. Twelve findings taken, one rejected with sources, the last round clean (0 comments, 0 open threads).
no-storegap below; the smoke countingnonce=instead of comparing it; the rollback's positional "previous revision";anyplot-backend→anyplot-api; and the 128-bit overstatement.gzip_staticnever reached for a.gzand it could not see the precompressed-shell regression it exists for — the sharpest catch of the six, reproduced above. Plus the rollback as a numbered procedure per the repository's Google style.<link rel="modulepreload">— see the comment thread; four frameworks ship precisely that fix.nonce-[0-9a-f]*accepted a barenonce-thattest -ncalls non-empty, so an empty nonce would have agreed withnonce=""tags all the way through; and the precompression guard now RUNS the declared pattern againstindex.htmlinstead of reading it for the word "index", after three near-misses in two rounds showed the reading approach was the wrong shape.test_every_server_block_stamps_the_noncesearched the whole block, nested locations included — so moving the stamp intolocation = /index.htmlwould have satisfied the guard that exists to refuse exactly that, and the smoke wouldn't have caught it either since it probes/on the main host. It now cuts nested locations out first. Plus "two extra curls" where the probe adds one.The one worth naming in full is round one's, a genuine hole the local probes had walked straight past:
python.anyplot.ai/<spec>and/<spec>/<library>serve the shell throughtry_files /index.html =404, which treats the shell as a FILE and answers inside that location — no internal redirect, solocation = /index.htmland itsno-storeare never reached. Measured before the fix:A nonced shell a client may keep. Both routes now send the shell's
Cache-Controland re-includesecurity-headers.confbeside it (anadd_headerof their own would otherwise have dropped the entire inherited set — verified CSP, HSTS and X-Frame-Options are still on those responses). And the test no longer looks forlocation = /index.htmlby name: it derives the shell locations from thetry_filesargument positions, finds all four, and fails if any one losesno-store.Verification:
pytest tests/unit tests/integration— 1974 passed.ruff check,ruff format --check,mypy api coreclean.yarn type-check,yarn lint,yarn fm:checkclean,yarn test— 626 passed in 70 files.🤖 Generated with Claude Code
https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3