Skip to content

Close the direct run.app door with a shared-secret origin gate - #11208

Open
MarkusNeusinger wants to merge 12 commits into
mainfrom
feat/origin-secret-gate
Open

Close the direct run.app door with a shared-secret origin gate#11208
MarkusNeusinger wants to merge 12 commits into
mainfrom
feat/origin-secret-gate

Conversation

@MarkusNeusinger

@MarkusNeusinger MarkusNeusinger commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • api/origin_gate.py requires the header the Cloudflare edge stamps, and refuses anything else with 403 before the request costs anything. The API stands on Cloud Run with ingress=all, so it answers on two addresses — api.anyplot.ai behind Cloudflare, and the raw *.run.app URL in front of nothing. Every edge measure (bot challenge, WAF, the cache that makes the max-age=300 reads free) was one URL away from being bypassed; api/request_context.py already documented callers doing it.
  • Unset means off. Nothing changes until ORIGIN_SECRET is set on the service, which is also the rollback. Local dev and the test suite never see the gate, so this can merge and deploy long before the Cloudflare rule or the secret exist.
  • /health reports origin_gateoff · off-seen · ok · missing · mismatch — for the request it was asked with, never the value. That is what turns the rollout into a measurement instead of a leap.
  • The apex Worker's source moves into infra/cloudflare/, because a Worker subrequest to a host in the same zone bypasses that zone's Transform Rules. It now stamps the header itself, deleting any inbound one first.
  • Transferred from the sibling repo kurrentschrift (PRs [line-basic] bokeh implementation #493, [box-basic] highcharts implementation #495), where this shipped and was measured live.
  • Known residual, deliberately out of scope: the app service also stands with ingress=all, and its nginx relays a crawler user agent through @seo_proxy to api.anyplot.ai, where the edge stamps the header legitimately — so the prerendered render stays reachable via the app's raw run.app URL. That is a second door on a second service (the request the API sees really did pass the edge), and closing it means gating anyplot-app or refusing to proxy for run.app hosts, which bot-serving-check.yml probes nightly. Named in api/origin_gate.py and docs/reference/api.md; own PR.

What is exempt, and why each one has to be

Exact paths, no prefixes:

Path Why
/health the deploy's pre-traffic smoke probes the candidate revision on its run.app tag URL, which by definition never passes the edge — gating it makes every deploy fail closed
/debug/cache/invalidate anyplot-specific. sync-postgres.yml posts here from a GitHub runner over the direct *.run.app URL on purpose — Cloudflare's bot challenge answers an unauthenticated curl POST against api.anyplot.ai with a 403 HTML page. The endpoint carries its own shared secret (CACHE_INVALIDATE_TOKEN, constant-time compared, 503 when unconfigured), so it is gated, just by a different lock
OPTIONS a browser cannot attach a custom header to a preflight

/seo-proxy/… is deliberately NOT exempt, though the sibling repo exempts it belt-and-braces. Copilot was right that it would be a real hole: those handlers query SpecRepository/ImplRepository on a cache miss or an unknown id, and any request with a recognized crawler user agent schedules an outbound Plausible event — so an exemption would leave the API's most expensive reads open on the direct URL, which is the cost this gate exists to refuse. The site's nginx already fetches those pages over https://api.anyplot.ai, so the path carries the header; step (b) of the rollout validates it end to end with a crawler user agent before anything is armed, and bot-serving-check.yml runs daily, so being wrong here is loud rather than silent.

The direct paths I checked (this is the part that does not transfer, and had to be re-derived for this repo):

  • api.anyplot.ai — SPA (VITE_API_URL), MCP clients, OG cards embedded cross-origin, and the site's nginx for @seo_proxy, @seo_proxy_python, /llms-full.txt, /sitemap.xml. All through the edge → the Transform Rule stamps them.
  • anyplot.ai/api/* — the Worker. Same-zone subrequest, so it must stamp for itself. Its /api/event Plausible passthrough is preserved untouched.
  • anyplot-api-…run.app — the Cloud Build smoke (now sends the header) and sync-postgres.yml (exempt path, above).
  • anyplot-app-…run.appbot-serving-check.yml hits the app origin, whose nginx then goes out through api.anyplot.ai. Unaffected.

Every header secret goes through one byte-wise comparator

secrets.compare_digest raises TypeError when either str holds a non-ASCII character, and a header value reaches the application latin-1-decoded straight from the wire. Comparing strings handed any unauthenticated caller a one-byte way to turn a cheap 401 or 403 into an unhandled, logged 500 (Copilot).

That was true of the gate — and of X-Admin-Token and X-Cache-Token, which matters more: /debug/cache/invalidate is exempt from the gate on the grounds that it has its own lock, and it is the one endpoint reachable on the direct run.app URL. So the fix is one comparator in api/secret_compare.py, used by all three call sites, rather than three separate patches: a comparator that is correct in two places out of three is exactly what nobody notices. It also refuses when either side is missing, so an unconfigured secret can never be satisfied by an absent header.

Pinned by tests including one that asserts the str comparison this replaced does raise on the same input, so the others cannot quietly stop measuring anything.

Two changes beyond the gate itself

The deploy step configures the revision additively — --update-secrets and --update-env-vars. Both --set- forms replace their whole set, so anything attached to the service out of band is stripped from every revision the pipeline creates. ORIGIN_SECRET is exactly that kind of binding — attached by hand to arm, removed by hand to roll back — and a secret-backed variable lives in the same revision environment as a literal one, so either flag was a way to silently disarm the gate on the next deploy (the second half found by Copilot, after the first fix). It cannot simply be listed in the flags instead: Cloud Run refuses a deploy naming a secret that does not exist, which would break every build until step (c) below. Two flags in the deploy step; everything else in api/cloudbuild.yaml is confined to the smoke step.

The analytics middleware moves inside CORSMiddleware. The gate has to be inside CORS (so its 403 carries the headers a browser needs to read it as a 403 rather than as an opaque network error) and outside the bot counter (so a refused request can never fire an outbound Plausible event — track_asset_fetch fires per request for anything with a crawler user agent, so a caller on the direct URL could otherwise turn each of its own refusals into one, unthrottled, at a third-party endpoint). In this repo the counter sat outside CORS, which makes those two mutually exclusive; moving it in resolves it. The cache-header middleware stays outside CORS, where its setdefault for the /og/ cards depends on being. api/main.py now carries the stack order and the reason for each position.

The only behavioural consequence of the move: a CORS preflight no longer reaches the counter. Preflights carry the browser's user agent, and both tracking functions return early unless detect_ai_agent classifies the UA, so nothing that was being counted stops being counted.

Rollout — in this order, and measured at each step

(a) Merge and deploy with the check off. Nothing to configure; ORIGIN_SECRET does not exist yet, gate_is_armed() is false, every path behaves exactly as today. Confirm with:

curl -s https://api.anyplot.ai/health   # expect "origin_gate":"off"

(b) Put the Transform Rule live and give the Worker its binding — then measure off-seen on EVERY path. Cloudflare dashboard: Rules → Transform Rules → Modify Request Header → set static X-Origin-Secret for http.host eq "api.anyplot.ai". Then deploy infra/cloudflare/anyplot-api-proxy.js with the ORIGIN_SECRET secret binding (procedure in infra/cloudflare/README.md). The gate is still off, so nothing can break; what this step buys is the evidence:

curl -s https://api.anyplot.ai/health   # must read "off-seen"
curl -s https://anyplot.ai/api/health   # must read "off-seen"  ← the Worker; this is the one that reads "off" if the binding is missing
curl -s https://<api-run-url>/health    # must stay "off" — that is the door being closed
# nginx rides on the first line's verdict; probe it end to end:
curl -s -A 'Mozilla/5.0 (compatible; Googlebot/2.1)' https://anyplot.ai/scatter-basic | head -5
curl -sI https://anyplot.ai/llms-full.txt

Do not proceed while any path that must keep working still reads off.

(c) Create the secret and arm the service. Both the Cloud Run runtime and the Cloud Build trigger use the same identity, 239660669828-compute@developer.gserviceaccount.com, so one grant covers the service and the smoke step's read.

# Never `echo` — it appends a newline. (The config strips whitespace as a
# second net, but the value should be right in the first place.)
gcloud secrets create ORIGIN_SECRET --project=anyplot --replication-policy=automatic
printf %s "<value>" | gcloud secrets versions add ORIGIN_SECRET --project=anyplot --data-file=-

gcloud secrets add-iam-policy-binding ORIGIN_SECRET --project=anyplot \
  --member="serviceAccount:239660669828-compute@developer.gserviceaccount.com" \
  --role="roles/secretmanager.secretAccessor"

# Same value into the Cloudflare Transform Rule and the Worker binding.

# Same value into the Cloudflare Transform Rule and the Worker binding.

Then arm the service. The full block is in docs/reference/api.md § Origin gate — it lives in the repository rather than in this description, because a procedure that exists only in a PR body is one nobody finds at 2 a.m. Three things in it are not obvious, each from a Copilot round:

  1. Refuse to act while a candidate revision is in flight. services update clones the service's latest template, not the serving one, and the pipeline deliberately leaves each build's smoked-but-unpromoted candidate as latest — so arming during a deploy would ship that build's image along with the gate, and naming the new revision precisely does not change which image it inherits. The block asserts latestReadyRevisionName == the revision serving 100% and stops otherwise.
  2. Pin the secret to a version number, never :latest. Cloud Run resolves a secret-backed variable when each instance starts, so with :latest a new secret version reaches new instances while older ones keep the old value — and since the edge stamps exactly one value, that shows up as intermittent 403s inside a single revision.
  3. Promote by name, never --to-latest — the same hazard as (1), and the reason api/cloudbuild.yaml refuses that flag.

Rotation gets its own paragraph there: the gate accepts exactly one value, so there is no overlap window. Roll back, rotate both sides, arm again on the new version number.

(d) Verify.

curl -s https://api.anyplot.ai/health   # "ok"
curl -s https://anyplot.ai/api/health   # "ok"
curl -s https://<api-run-url>/health    # "missing"  ← the door is now shut
curl -s -o /dev/null -w '%{http_code}\n' https://<api-run-url>/libraries   # 403
curl -s -o /dev/null -w '%{http_code}\n' https://api.anyplot.ai/libraries  # 200

Then walk the site once (gallery, a spec page, the stats page), fetch an OG card cross-origin, and let one sync-postgres run finish — its cache flush must still return 200.

(e) Rollback — one variable, no code change:

The same block as arming, with --remove-secrets=ORIGIN_SECRET in place of --update-secrets and a disarm- suffix — including the in-flight-candidate guard, which matters more here than when arming.

Removing the Worker binding is not a rollback: while the service is armed, that takes anyplot.ai/api/* down instead of freeing it. Roll back on the API side, always.

Follow-up this PR deliberately leaves open

/debug/cache/invalidate is exempt because sync-postgres.yml has no front door. The cleaner end state is for that workflow to send X-Origin-Secret from a repository secret, at which point the exemption can go. That needs a GitHub Actions secret plus a change to .github/workflows/sync-postgres.yml, which is out of this PR's scope — noted here so it is not lost.

Test plan

  • tests/unit/api/test_origin_gate.py — 57 tests: dormant by default (including with a wrong header), the armed gate across five header shapes and six methods, the exemption list both as live requests and as assertions on the list itself (including that /seo-proxy/… is refused), preflight and CORS-headers-on-the-403, the analytics middleware never firing on a refusal — for an asset path and a crawler page — all five /health verdicts, the non-ASCII header on all three secrets, and the trailing-newline strip on ORIGIN_SECRET and the other Secret-Manager-backed values.
  • uv run pytest tests/unit — 1818 passed, 1 skipped (pre-existing local skip: MonoLisa italic not cached).
  • uv run ruff check . / ruff format --check . — clean.
  • uv run --extra typecheck mypy api core — no issues in 37 source files.
  • api/cloudbuild.yaml parses; step ids unchanged (build-image, push-image, push-latest, deploy, smoke, promote, get-url).
  • Not verifiable before merge: the Cloud Build smoke's new lines and the Cloudflare side. The smoke is written so that a missing secret or a missing permission yields an empty value and bare probes — correct while the gate is off, loud at /libraries once it is on — and it accepts off/off-seen, so it cannot take the deploy pipeline down during the rollout or after a rollback.

Checklist

  • CHANGELOG.md updated under [Unreleased] — one ### Added entry for the gate, two ### Changed entries for the deploy flag and the middleware order.
  • Docs updated: docs/reference/api.md (new "Origin gate" section, /health response), docs/development.md (env table), docs/reference/repository.md and agentic/docs/project-guide.md (both repository maps get infra/), .env.example, and infra/cloudflare/README.md for the Worker and the measuring procedure.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke

The API stands on Cloud Run with ingress=all, so it answers on two addresses:
api.anyplot.ai, which Cloudflare proxies, and the raw *.run.app URL, which it
does not. Everything the edge enforces — the bot challenge, the WAF, the cache
that makes the max-age=300 reads free — was one URL away from being bypassed,
and api/request_context.py already documented callers doing it.

A Cloudflare Transform Rule stamps X-Origin-Secret onto every request it
proxies for the API host; api/origin_gate.py requires that header and refuses
anything else with 403 before the request costs anything. It is not
authentication — it says "you came through the front door", nothing about who
you are.

UNSET MEANS OFF, which is what makes this safe to ship first: the code can go
to production long before the rule and the secret exist, local dev and the test
suite never see the gate, and taking the variable off the service is the
rollback. /health reports the verdict for the request it was asked with — off,
off-seen, ok, missing, mismatch — never the value, so every route into the
service can be measured before the switch is thrown.

Exempt: /health (the deploy smoke reaches the candidate on its run.app tag URL,
which never passes the edge), /seo-proxy/… (belt and braces), OPTIONS (a
browser cannot attach a custom header to a preflight), and
/debug/cache/invalidate — sync-postgres.yml posts there over the direct URL by
design, because Cloudflare's bot challenge answers an unauthenticated curl POST
with a 403 HTML page; that endpoint carries its own constant-time token.

The apex Worker behind anyplot.ai/api/* gets its source into the repository at
infra/cloudflare/, because a Worker subrequest to a host in the same zone
bypasses that zone's Transform Rules — so it stamps the header itself, deleting
any inbound one first so a caller cannot supply it and corrupt an unarmed
/health probe. Its Plausible passthrough for /api/event is preserved.

Two changes beyond the gate. The deploy step attaches secrets with
--update-secrets instead of --set-secrets: the latter replaces the whole
binding set and would have stripped ORIGIN_SECRET, silently disarming the gate
on the next deploy. And the analytics middleware moves inside CORSMiddleware,
because the gate has to be inside CORS (so its 403 stays readable to a browser)
and outside the bot counter (so a refused request cannot fire an outbound
Plausible event), which in this stack was only possible with the counter inside
CORS. api/main.py now documents the whole order.

Transferred from the sibling repo kurrentschrift, where this shipped and was
measured live.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI balanced review requested due to automatic review settings September 2, 2026 21:16
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive cross-system rollout retains a DB-backed direct-origin bypass and needs configuration hardening and human operational review.

Pull request overview

Adds a Cloudflare-stamped shared-secret gate to prevent direct Cloud Run API access.

Changes:

  • Adds the origin-gate middleware, health diagnostics, configuration, and tests.
  • Updates Cloud Run deployment and Cloudflare Worker behavior.
  • Documents rollout, rollback, and infrastructure changes.
File summaries
File Description
.env.example Documents ORIGIN_SECRET.
CHANGELOG.md Records security and deployment changes.
api/cloudbuild.yaml Preserves and supplies the origin secret.
api/main.py Registers and orders the middleware.
api/origin_gate.py Implements the shared-secret gate.
api/routers/health.py Reports the gate verdict.
core/config.py Adds and normalizes secret configuration.
docs/development.md Documents the environment variable.
docs/reference/api.md Documents gate behavior and exemptions.
docs/reference/repository.md Adds the infrastructure directory.
infra/cloudflare/README.md Documents Worker deployment and rollout.
infra/cloudflare/anyplot-api-proxy.js Stamps the secret on Worker subrequests.
tests/unit/api/test_origin_gate.py Tests gate states, exemptions, and ordering.
Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 4
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread api/origin_gate.py Outdated
Comment thread core/config.py
Comment thread docs/reference/api.md Outdated
Comment thread docs/reference/repository.md
Copilot AI review requested due to automatic review settings September 2, 2026 21:21
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The SEO exemption retains a costly direct-origin bypass, and the documented Worker deployment exposes its API token through process arguments.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

api/origin_gate.py:75

  • The /seo-proxy exemption keeps a substantial direct-origin bypass open. A caller can hit arbitrary /seo-proxy/{spec_id} URLs on run.app, causing database lookups (api/routers/seo.py:1288-1296), and a spoofed crawler user agent also creates an outbound Plausible request (api/main.py:213-215, api/analytics.py:400-414) without passing Cloudflare's WAF or cache. This recreates the denial-of-wallet path the gate is intended to remove; require the origin header for this prefix after validating the nginx route during rollout, rather than exempting the entire SEO surface.
EXEMPT_PATHS = frozenset({"/health", "/seo-proxy", "/debug/cache/invalidate"})
EXEMPT_PREFIXES = ("/seo-proxy/",)

docs/reference/repository.md:137

  • Adding a top-level infra/ directory here leaves the corresponding directory structure in agentic/docs/project-guide.md:186-206 stale. The repository documentation contract requires structural project changes to update that agent guide too, so add infra/ and its Cloudflare role there in this PR.
├── infra/                             # Infrastructure that lives outside the code
│   └── cloudflare/                    # Source for the apex Worker + its rollout notes
  • Files reviewed: 13/13 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread infra/cloudflare/README.md Outdated
Copilot review, four findings, all applied.

1. The /seo-proxy exemption was a real bypass, not belt and braces. Those
   handlers are not uniformly cheap: a cache miss or an unknown id queries
   SpecRepository/ImplRepository, and any request with a recognized crawler
   user agent schedules an outbound Plausible event. Exempting them left the
   API's most expensive reads open on the direct run.app URL - exactly the
   cost the gate exists to refuse. The site's nginx already fetches those
   pages over api.anyplot.ai, so the path carries the header; the rollout
   validates it end to end with a crawler user agent before arming, and
   bot-serving-check.yml runs daily, so being wrong is loud. The exemption
   list is now exact paths only, no prefixes.

2. secrets.compare_digest raises TypeError when either str holds a non-ASCII
   character, and a header value reaches the middleware latin-1-decoded
   straight from the wire - so one byte >= 0x80 in X-Origin-Secret turned
   every refusal into an unhandled 500, an unauthenticated way to make the
   gate expensive. Both comparisons now encode first, which also covers a
   non-ASCII secret at the other end.

3. The /health example in docs/reference/api.md still showed version 0.2.0,
   as did the root endpoint two blocks above it; both now show the real
   version, with a note that the field tracks the installed package.

4. agentic/docs/project-guide.md's directory structure did not list the new
   infra/ tree. It does now, next to the same entry in
   docs/reference/repository.md.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 21:31
Copilot review: the Worker-deploy recipe passed the credential as
-H "Authorization: Bearer $CF_API_TOKEN", which publishes it in
/proc/<pid>/cmdline for the life of the request - to every process on the
machine, and for the token that authorises replacing this Worker. The
ORIGIN_SECRET handling in the same block already avoided exactly that.

It now goes in through curl --config on a process-substitution file
descriptor, with printf as a shell builtin, so the token reaches no argv at
all. Noted that this needs bash or zsh, with the 600-mode temporary file as
the fallback.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes production ingress security and manual Cloudflare and Cloud Run rollout behavior that cannot be fully validated from the repository alone.

Review details

Suppressed comments (1)

api/origin_gate.py:163

  • Once this middleware is armed, the existing break-glass contract in api/routers/debug.py:75-77 is incomplete: X-Admin-Token alone on the direct Cloud Run URL now receives this 403, and the new test confirms that both headers are required. Update that operator-facing guidance to require X-Origin-Secret as well, otherwise incident responders are directed to a path guaranteed to fail.
    async def __call__(self, scope, receive, send):
        if scope["type"] != "http" or not gate_is_armed() or is_exempt(scope["path"], scope["method"]):
            await self.app(scope, receive, send)
  • Files reviewed: 14/14 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread api/origin_gate.py
Comment thread core/config.py Outdated
Copilot AI review requested due to automatic review settings September 2, 2026 21:40
Copilot review: core/config.py's origin_secret docstring listed
/seo-proxy/... among the exempt paths, which is what it was before the
exemption was removed earlier in this PR. An operator reading only the
settings file could arm the gate expecting crawler traffic to be waved
through. It now names the two real exemptions and says explicitly that the
prerendered pages are not among them.

The PR description raised in the same review was already corrected before
this push; the review read the pre-edit body.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive cross-system rollout has unresolved secret-normalization and exempt-endpoint robustness issues.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

core/config.py:211

  • This validator changes the canonical ORIGIN_SECRET, but the new stampers use the raw Secret Manager value (api/cloudbuild.yaml:137-139 and infra/cloudflare/README.md:96-106). A space-padded value explicitly accepted by the new tests therefore makes the Worker/edge send a different value and makes the smoke report mismatch. Either reject surrounding whitespace here or normalize the secret identically before every header is stamped.

core/config.py:180

  • This docstring still says /seo-proxy/… is exempt, contradicting EXEMPT_PATHS and the updated API documentation. Remove it from the list so future maintainers do not reintroduce the bypass that this revision intentionally closed.
    Local dev and the test suite never set it. Exactly two paths stay exempt
    when it is set — `/health` and `/debug/cache/invalidate`; the prerendered
    `/seo-proxy/…` pages are NOT among them, since the site's nginx fetches
  • Files reviewed: 14/14 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread api/origin_gate.py
Copilot AI review requested due to automatic review settings September 2, 2026 21:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The deployment can still remove ORIGIN_SECRET, and two malformed-secret paths can cause production failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

infra/cloudflare/anyplot-api-proxy.js:24

  • origin_secret accepts arbitrary Unicode, but the Workers Headers.set API converts values to ByteString and throws for code points above U+00FF. A CJK or emoji secret that Cloud Run accepts will therefore make every anyplot.ai/api/* request fail in the Worker before reaching the API. Restrict and document the secret as a strong ASCII token (with a generation command), or apply one consistent transport encoding in the Worker and API.
    infra/cloudflare/README.md:123
  • Use the repository documentation's American spelling: “authorizes.”

api/origin_gate.py:80

  • This exemption leaves the same malformed-header failure that _matches avoids for the new gate. invalidate_cache still compares Starlette's Latin-1-decoded X-Cache-Token as str (api/routers/debug.py:487-492), so a raw byte ≥ 0x80 raises TypeError; because this exact path bypasses the origin gate, an unauthenticated direct-origin caller can force a logged 500 on every request. Compare bytes there too and add the corresponding raw-header regression test before relying on this endpoint as the alternate lock.
EXEMPT_PATHS = frozenset({"/health", "/debug/cache/invalidate"})
  • Files reviewed: 14/14 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread api/cloudbuild.yaml Outdated
Merge resolves the CHANGELOG conflict by keeping both Added entries (#11205
landed on main while this branch was open).

Copilot review, two findings.

1. /debug/cache/invalidate is exempt from the gate on the grounds that it has
   its own lock - so that lock has to be as cheap to fail as the gate is, and
   it was not: it compared the token as str, and it is the one endpoint
   reachable on the direct run.app URL, so a non-ASCII X-Cache-Token turned a
   401 into an unhandled, logged 500. X-Admin-Token had the same comparison.
   Rather than fixing three call sites separately, the byte-wise compare moves
   into api/secret_compare.py and all three use it; a comparator that is
   correct in two places out of three is exactly what nobody notices.

2. --set-env-vars is destructive over the revision environment, where a
   secret-backed variable lives alongside a literal one - so it was a second
   way for a pipeline deploy to drop the hand-attached ORIGIN_SECRET and
   disarm the gate, undoing what --update-secrets had just bought. Both flags
   are additive now.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 21:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security boundary depends on coordinated, live Cloudflare and Cloud Run configuration that cannot be verified from the repository alone.

Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

CHANGELOG only; both sides add entries under Added and both are kept.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 22:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive rollout can promote an unrelated unsmoked revision through its current --to-latest commands.

Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread api/origin_gate.py
Copilot review: the rollout used 'update-traffic --to-latest' for both arming
and rollback - the exact flag api/cloudbuild.yaml refuses, and for the same
reason. The deploy pipeline leaves each build's smoked-but-unpromoted
candidate as the latest revision, so --to-latest can promote a concurrent
build's image while the operator believes they are only turning the gate on
or off.

Both procedures now stamp a deterministic --revision-suffix on the service
update and promote exactly that revision with --to-revisions. The commands
move into docs/reference/api.md so the procedure outlives the PR
description, and the Worker README points at them instead of paraphrasing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 22:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive rollout can promote an untested image and produce inconsistent gate secrets across Cloud Run instances.

Review details

Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

api/origin_gate.py:9

  • The cited documentation at api/request_context.py:22-25 still says direct run.app callers can forge cf-connecting-ip. Once this gate is armed, that is no longer true for ordinary routes (and the exempt cache invalidation route does not use this helper). Update that docstring to describe the gate-off/legacy case so future security work does not treat a closed bypass as an active trust limitation.
    api/origin_gate.py:146
  • These new middleware methods omit the type hints required for all functions in this repository. Add ASGIApp, Scope, Receive, and Send annotations (including -> None) so the middleware participates in static checking instead of making mypy skip its request path.
    infra/cloudflare/README.md:64
  • The runbook never records the Transform Rule's reproducible configuration, although arming depends on it and this directory is intended to capture dashboard-only infrastructure. Document the host expression, Modify Request Header action, static X-Origin-Secret assignment, and shared-value requirement so restoring the Cloudflare setup from the repository cannot arm the API without its front door.
    infra/cloudflare/README.md:135
  • This initial deployment is intentionally performed before the gate is armed, so a correctly stamped Worker returns off-seen, not ok. The current expected value makes the documented rollout appear to fail at the exact checkpoint meant to validate it.
  • Files reviewed: 16/16 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread docs/reference/api.md Outdated
Comment thread docs/reference/api.md Outdated
… secret

Copilot review, two more on the same procedure, both correct.

1. 'services update' clones the service's LATEST template, not the serving
   one, and the deploy pipeline deliberately leaves each build's
   smoked-but-unpromoted candidate as latest. Arming during a deploy would
   therefore promote that build's image along with the gate - naming the new
   revision precisely does not change which image it inherits. The block now
   asserts that latestReadyRevisionName equals the revision serving 100% and
   refuses otherwise, the same shape of assertion the cloudbuild smoke makes
   about its own candidate tag.

2. ORIGIN_SECRET:latest floats. Cloud Run resolves a secret-backed variable
   when each instance starts, so a new version reaches new instances while
   older ones keep the old value - and because the edge stamps exactly one
   value, that surfaces as intermittent 403s inside a single revision. The
   binding is pinned to a resolved version number.

Rotation gets its own paragraph, because the gate accepts exactly one value
and there is no overlap window: roll back, rotate both sides, arm again on the
new number. The gate is off in between, which is the documented safe state.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 22:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security and deployment changes require human rollout validation, and the Worker documentation contains a contradictory pre-arm health expectation.

Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread infra/cloudflare/README.md Outdated
Copilot review: the Worker README told the operator to expect
'origin_gate':'ok' straight after deploying the Worker, which contradicts the
rollout order three sections above it - at that point the API gate is
deliberately still off, so a correctly stamped request answers 'off-seen'.
Read literally, the required pre-arm measurement looked like a failed
deployment, and the obvious reaction to it would have been to arm the gate
early.

The command now lists all three reachable verdicts with what each one means at
that moment, including that 'off' is the one that says the binding is missing
and arming would take the route down.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 22:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The app origin still relays gated requests, and the documented arm procedure can fail open or race a concurrent deployment.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

docs/reference/api.md:498

  • This safety check fails open if gcloud run services describe or the JSON parser fails. The here-string still gives read an empty line, so both variables become empty and test "" = "" succeeds; this block has no fail-fast wrapper. The subsequent update can then clone the candidate that this check was meant to exclude. Capture and validate the command status/output (or run the full procedure in a fail-fast subshell) before comparing nonempty revision names.
    docs/reference/api.md:518
  • The earlier equality check is subject to a deployment race: a Cloud Build can create a new no-traffic candidate after line 498 but before this command, and services update will then clone that new latest template. Naming the arm revision does not prevent it from inheriting and promoting the unsmoked image. Pin the update to the image digest captured from SERVING, introduce mutual exclusion with deployments, or verify the created revision against the captured serving revision before shifting traffic.
    api/origin_gate.py:146
  • Both new middleware methods are untyped, contrary to the repository requirement that all functions include type hints. Annotate the constructor with ASGIApp and -> None, and the call hook with Starlette's Scope, Receive, Send, and -> None; this also lets mypy check the raw ASGI boundary.
  • Files reviewed: 16/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread api/origin_gate.py
Copilot review, and it is correct as an observation: the app service also
stands with ingress=all, and its nginx relays a crawler user agent through
@seo_proxy to https://api.anyplot.ai, where the edge stamps the header
legitimately. So a caller can still reach the prerendered render and its DB
queries by asking the APP's raw run.app URL instead of the API's.

That is a second door on a second service, not a hole in this one - the
request this process sees really did come through the edge, so there is
nothing for this middleware to refuse. Closing it means gating the app service
or refusing to proxy for run.app hosts in app/nginx.conf, and
bot-serving-check.yml probes that exact flow on the app origin every night, so
it carries its own blast radius and belongs in its own PR.

Named in api/origin_gate.py and docs/reference/api.md rather than left for
someone to rediscover, because a gate whose limits are undocumented gets
trusted past them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 22:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The blanket OPTIONS exemption permits direct-origin analytics traffic, and the rollback procedure can become unavailable after a failed or concurrent deployment.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

api/origin_gate.py:108

  • The blanket OPTIONS exemption admits more than CORS preflights. CORSMiddleware only consumes an OPTIONS request when it includes Origin and Access-Control-Request-Method; a bare direct-origin OPTIONS /seo-proxy/legal reaches this branch, passes the gate, and then record_bot_fetch emits a Plausible event for a crawler user agent (api/main.py:211-215). This recreates the unthrottled third-party cost the middleware ordering is intended to prevent. Since CORS is already outside the gate, remove the method-wide exemption, or restrict it to an actual preflight by checking the required headers.
  • Files reviewed: 16/16 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread docs/reference/api.md Outdated
Comment thread api/routers/debug.py
…th headers

Copilot review, two findings.

1. The in-flight guard could have made the emergency rollback unavailable.
   A failed smoke skips the promote and nothing cleans the candidate up, so
   the failed revision stays latestReadyRevisionName indefinitely and
   SERVING == LATEST would never become true by waiting - a hard refusal
   there would deadlock disarming at exactly the moment it is needed, and the
   check was racy besides. The block now pins --image to the SERVING
   revision's image instead, which makes the operation correct whatever state
   the pipeline is in, and demotes the equality test to a warning that says
   what a mismatch still means (an unpromoted config change). A step 4 was
   added to confirm the result, since a warning nobody reads is not a
   safeguard.

2. require_admin's docstring still promised that X-Admin-Token alone gets an
   operator in over the direct URL. With the gate armed that call needs BOTH
   headers, because the origin gate answers first - and it answers 403, not
   401, which is the only hint an operator would get. Documented where they
   will read it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants