diff options
| author | Yuriy Andamasov <yuriy@vyos.io> | 2026-08-21 23:25:49 +0300 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-08-21 23:25:49 +0300 |
| commit | 3a1c6c3020c4ed819616098a91be670719e290cc (patch) | |
| tree | 758da979ad4f59d9f92988bede8bd241c3c6b3c8 | |
| parent | 4a5950de1a4ad6e0a5390d1edb089f7139e7dfc5 (diff) | |
| download | vyos-documentation-3a1c6c3020c4ed819616098a91be670719e290cc.tar.gz vyos-documentation-3a1c6c3020c4ed819616098a91be670719e290cc.zip | |
fix: IS-572: correct verified defects in the Cloudflare Workers docs pipeline (#2209)
* fix: IS-572: correct verified defects in the Cloudflare Workers docs pipeline
Fixes raised against the LTS port PR but landing on rolling first, so the
backport runs in the right direction, plus one defect found while verifying
the nightly canary sweep.
- apex PDF: a 206 is now only answered to a request that actually carried a
Range header, and every R2Range shape is normalized to concrete bounds.
R2 reports a whole-object range on un-ranged gets, so keying the 206 off
obj.range alone made every plain GET of the 1.3 PDF a 206.
- apex PDF: a failed If-Match now maps to 412, not 304.
- apex sitemap index: entries derive their origin from the request, so the
canary index no longer points at production.
- UA policy: drop the dead Google-Extended entry (a robots.txt token, never
a UA) and log Applebot-Extended while still allowing Applebot, via a
most-specific-match precedence between the allow and log lists.
- docs-build: replace the check_head guard's exit 78 (neutral only in the
retired Actions v1 runtime) with an output gating every deploy/promote
step, so a superseded run ends green-and-skipped instead of red.
- smoke/parity: read CF Access service-token credentials from the
environment instead of argv, and pass them via env: in both workflows.
- smoke: stop asserting X-Docs-Build on the PDF probe — apex's R2 fallback
path legitimately never sets it, making the probe unpassable for 1.3.
- gates/smoke: strip critical-pages lines before testing for comments.
- parity: fetch each sitemap once, honouring the _SCHEME override.
Advances: IS-572
* fix: IS-572: harden check_head guard against a failed ls-remote
Phase-0 CodeRabbit findings on the new guard, both valid:
- set -euo pipefail + an explicit emptiness check, so a failed git ls-remote
cannot leave $remote empty and be misread as 'branch moved' (cut exits 0
on empty input) — that would silently skip a deploy that should have run.
- pass github.ref_name and github.sha through env vars rather than
interpolating them into the shell command.
Advances: IS-572
* fix: IS-572: address adversarial + CodeRabbit review on the CF docs gates
Answers the seven adversarial-review findings on vyos/vyos-documentation#2209,
five of which GitHub CodeRabbit independently reproduced on review 4946100889.
Range handling (apex Worker) — the reported defect was real but mis-diagnosed.
Probing a real R2 binding under vitest-pool-workers shows R2 never returns the
inverted/zero-length range the review predicted: for the Headers form this
Worker uses, R2 answers every unsatisfiable, malformed, multi-range and
unknown-unit Range by IGNORING it and returning the complete object with
range={offset:0,length:size} — indistinguishable from a satisfied whole-object
range, and it does not throw. (Only the object-literal form throws, code 10039,
which this Worker cannot reach.) Trusting obj.range therefore answered
`Range: bytes=99-` with 206 + `Content-Range: bytes 0-8/9` and the whole body,
so a client resuming mid-download would silently corrupt its file. Range intent
is now re-derived from the client's own header per RFC 9110 §14.1.2:
unsatisfiable → 416 + `Content-Range: bytes */size`, ignored → 200 with the
complete body, satisfiable → 206 as before. resolveRange() additionally clamps
its length at zero so its documented contract holds for every R2Range input.
Preconditions: reorder to RFC 9110 §13.2.2's MUST-ordered precedence
(If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since) so
`If-Match: "old"` + `If-None-Match: "new"` yields 412 rather than 304, and gate
304 on GET/HEAD per §13.1.2 so a conditional POST cannot be answered 304.
UA gate: a log match now wins a contest against an allow match unless the allow
entry strictly contains the log entry, so `GPTBot/1.0 DuckDuckBot` emits its
ua-log event instead of being swallowed by the longer allow needle. Applebot /
Applebot-Extended verdicts are unchanged and now pinned by tests.
parity.py: restore the exact-200 sitemap check (_OPENER returns for any 2xx, so
a 204 yielded an empty corpus and the gate passed having probed nothing) and
route the sitemap request through a single credential-attaching constructor, so
--sitemap-host against the Access-gated canary no longer 403s on every sitemap.
Secrets: --access-id/--access-secret removed from both gates in favour of the
paired env vars. An argv value is readable from the process table and captured
by `set -x`; no call site used the flags. smoke.py reads --critical-list via
Path.read_text() rather than leaking a file handle.
Tests: 115 -> 138 worker tests, 46 -> 50 gate tests. Every new behavioural test
was verified to fail against the pre-fix source.
Advances: IS-572
* test: IS-572: compare the forbidden UA token case-insensitively
bestMatch() lowercases every ua-policy.json entry before matching, so a
"google-extended" entry would be functionally identical to the "Google-Extended"
token this guard exists to keep out. toContain() compares primitives by strict
equality, so the lowercase variant passed the assertion and would have quietly
restored the entry.
Lowercase both sides so the assertion matches the matcher's own case semantics.
Verified by injecting a lowercase "google-extended" into the log list: the test
now fails, and passed before this change.
Advances: IS-572
* fix: IS-572: scope the CF Access token per host; correct range + precondition semantics
Round-3 adversarial review findings. Each fix has a test that fails against 5b4f40f9.
parity.py sent the CF Access service token to every host the run touched, which
pre-cutover includes --sitemap-host docs.vyos.io — still served by ReadTheDocs.
Credentials are now modelled as an Access record bound to --probe-host, and
build_request() attaches them per destination rather than per run, so the token
reaches only the host it belongs to while the post-cutover same-host
configuration stays credentialed.
The apex Worker's range handling assumed its own regex agreed with R2's parser.
It does not: R2 accepts ASCII space only, this one accepts \s, so a
tab-separated Range parsed here, was ignored there, and produced a 206 carrying
the whole object in answer to a request for three bytes. classifyRangeHeader now
returns the concrete bounds it derived and the Worker checks them against the
bytes R2 actually returned before promising a 206 — any parser divergence
degrades to a 200 instead of corrupting a resumed download. Range positions are
compared as digit strings, so specs above 2^53 no longer collapse into each
other, and Range is ignored outright on every method but GET (§14.2).
If-Range was silently dropped: R2Conditional carries no such validator, so a
stale range request was answered with bytes of the new representation under a
206. It is now evaluated against the object's own ETag, with a failed validator
serving the complete representation.
Preconditions were inferred from which headers were present, which cannot be
right in both directions — If-Match satisfied plus If-None-Match satisfied owed
a 304 and got a 412. The conditionals are filtered to those §13.2.2 applies to
this request before R2 sees them, and a body-less result is resolved by
evaluating the validators in §13.2.2 order.
Also drops the UA gate's narrow-allow carve-out. It compared the two matched
policy entries rather than the UA's token structure, so a request-controlled UA
quoting both tokens bought itself an allow. No pair in ua-policy.json used it.
🤖 Generated by [robots](https://vyos.io)
* fix: IS-572: cancel abandoned R2 body streams; drop stale --access-* comments
CodeRabbit round-3 follow-up.
R2 hands back a body on two paths whose response carries none, and dropping the
reference leaves the stream open holding its connection until GC. CodeRabbit
flagged the If-Range re-read; the larger instance is the 416, where R2 answers an
unsatisfiable Range with the COMPLETE object — 29.2 MiB for the 1.3 PDF — and the
response sends none of it. Both now go through discardBody(), which cancels so
the transfer is aborted rather than drained, and swallows failures because the
response on those paths is already decided.
Keeping the get-then-re-read rather than switching to head()-then-get(): a head()
would avoid opening the slice stream, but it would add a round-trip to the path
where If-Range MATCHES — the normal resumed download — to tidy the rare path
where it does not, and it would open a TOCTOU window that the current shape does
not have, since here the object whose validator was checked is the very response
served.
The R2 test mock returned string bodies, which is why this class of bug was
invisible to it. Bodies are now real ReadableStreams that record their own
cancellation, so the three new tests observe the actual stream lifecycle: the 416
cancels the whole object, the stale If-Range cancels the discarded slice, and a
served body is never cancelled.
Also corrects two comments that still described --access-id/--access-secret as a
manual fallback; the flags were removed in round 2 and argparse rejects them. A
repo-wide sweep found no other prose describing the removed interface. The
docs-build.yml change is one comment line inside an env: block; check_head is
untouched.
🤖 Generated by [robots](https://vyos.io)
* fix: IS-572: compare Access origins; ignore conditionals per method
Round-4 adversarial review findings.
parity.py: _authority() compared (hostname, port) verbatim, so `p.invalid`,
`p.invalid:443` and the trailing-dot FQDN form were three different origins.
Post-cutover both --sitemap-host and --probe-host name the same Access-gated
host; spelling the default port out on either cost the sitemap fetch its token
and 403'd it, reverting the same-host keeper case. Compare normalized origins
instead: lowercased host, single trailing dot stripped, the scheme's default
port folded to None (bare arguments read under _SCHEME). IDN/punycode
equivalence stays out of scope -- both hosts are ASCII literals passed by CI.
apex worker: applicablePreconditions() filtered conditionals by validator
applicability but not by method, so RFC 9110 13.2.1 -- "a server MUST ignore
the conditional request header fields ... such as CONNECT, OPTIONS, or TRACE"
-- was unmet: an OPTIONS carrying a stale If-Match was refused 412 where the
same request without it succeeded. Take the method and return an empty set for
those three; R2 is then given nothing to evaluate.
apex worker: the stale-If-Range re-read was a bare get(), dropping the other
preconditions, so a key rewritten between the two reads was served under a 200
though the request's If-Match excluded it. The legacy snapshot deploy's
force_pdf_refresh input does rewrite this key. Carry the same onlyIf into the
re-read and resolve a body-less result through preconditionStatus() on the new
object's validators.
Advances: IS-572
* fix: IS-572: scope the Access token by origin, not authority; hide the secret from repr
Round-5 CodeRabbit findings, both on the CF Access credential primitive in
scripts/docs_gates/parity.py.
_authority() returned (hostname, port) and dropped the scheme, so an https
credential also applied to the plaintext http URL of the same name --
Access("p.invalid", ...).applies_to("http://p.invalid/") was True, and
build_request() would then have attached CF-Access-Client-Id/-Secret to a
cleartext request. `http://p.invalid:80/` is the sharp case: 80 folds to None
under http, matching the https-scoped ("p.invalid", None) exactly.
Not reachable in current callers: every URL in the module is built from
_SCHEME and both workflow invocations pass bare hosts, so a single run is
single-scheme and the http value exists only for the test monkeypatch. Fixed
as a contract defect regardless -- applies_to() documents itself as comparing
ORIGINs, an origin is scheme+host+port, and over-broad credential scope is the
failure mode this primitive already produced once (round 2, token sent to
docs.vyos.io). Return (scheme, host, port), resolving a bare argument's scheme
under _SCHEME as before, and fold the default port against the origin's own
scheme so http :80 and https :443 stay distinct rather than collapsing
together. The four origin-normalization cases and the :8443 denial are
unchanged.
Access.client_secret is now dataclasses.field(repr=False). The default
dataclass repr rendered every field, so a failed assertion, a debug print or
any exception interpolating an Access would have put the service token into CI
output, which is durable and world-readable for this repo. The client id stays
in repr: it names which token without being the credential, and dropping it
would make a scoping failure much harder to diagnose. The secret remains
readable as a field, which is the only way it is ever consumed.
Swept the rest of scripts/docs_gates/ for the same exposure: smoke.Probe holds
no credential fields, smoke passes the id/secret as plain locals that reach no
output path, and the str(e) texts recorded in parity-report.json and the
SMOKE-FAIL lines carry urllib's code/reason/URL, never request headers.
Two tests added, both verified failing against the pre-fix source.
Advances: IS-572
🤖 Generated by [robots](https://vyos.io)
* fix: IS-572: roll back to the newest deployment's 100%-traffic version id
`wrangler deployments list --json` sorts ASCENDING by created_on
(wrangler 4.123.0 versionsDeploymentsListHandler), so `.[0]` selected the
OLDEST deployment, not the current one the stale comment claimed. It also
took the deployment `id`, but `wrangler rollback` resolves its positional
through fetchVersion() and needs a VERSION id.
Select newest-first by created_on and take the version_id serving 100% of
traffic, walking to the next-newest deployment when traffic is split across
versions -- mirroring wrangler's own fetchDefaultRollbackVersionId(), minus
its .shift() (it picks after deploying; we capture before). Non-array or
unparseable output now fails safe to an empty id instead of a wrong one.
Rewrite the stale comment to describe the actual JSON shape, note in the
consumer that rollback_id is a version id, and add --message for rollback
audit context (supported at 4.123.0; --yes supplies it non-interactively).
Advances: IS-572
🤖 Generated by [robots](https://vyos.io)
* fix: IS-572: re-check the branch tip after smoke; harden rollback capture
Two production-safety fixes in docs-build.yml.
1. TOCTOU on the check_head guard. It resolved the branch tip ONCE, before the
candidate deploy and before the smoke gate (480s deadline), then gated
PROMOTE, the post-promote probe and the registry pointer publish on that
minutes-old answer. A push landing in that window left the run promoting a
superseded SHA to production and publishing a pointer naming it.
`cancel-in-progress: false` does not cover this: it makes the SECOND run
queue and skip, while the run already past the check sails on.
Add a second guard after smoke, immediately before the first
production-facing step, and move PROMOTE + probe + pointer publish onto its
output. The probe must move with PROMOTE: left on the first check it would
run after a skipped promote, see production serving the previous SHA, read
that as a promote failure and roll back a production nobody touched.
A branch that moved mid-run ends GREEN-and-skipped, matching the convention
the exit-78 redesign established: production and the registry are untouched,
and the queued run promotes the new tip, so there is nothing for an operator
to act on. The residual check-to-deploy window is one step boundary; no
compare-and-swap exists on the Cloudflare side to close it entirely.
2. The rollback-capture bootstrap branch swallowed every failure. `2>/dev/null`
plus a bare `if` could not tell "Worker does not exist yet" from auth,
network or malformed output, so a transient failure yielded an empty
rollback_id, logged "nothing to roll back to" and promoted with the
auto-rollback silently disarmed while the run looked healthy.
Only a genuine not-found may now produce an empty id. wrangler 4.123.0 exits
1 on every error path and writes nothing machine-readable to stdout when the
command fails, so the discriminator is the API error code it renders into
stderr: 10007/10090, the pair its own isWorkerNotFoundError() uses. Auth
carries 9106/10000 and connectivity failures carry no code, so neither can be
mistaken for bootstrap. Anything unmatched fails the step. Non-array output
now fails instead of being coerced to []; an empty list and a split-traffic
list are reported distinctly (notice / warning) rather than sharing one
ambiguous message with the failure paths.
The code pair was read out of the shipped 4.123.0 bundle, not observed
against a live missing Worker; the comment says so and asks for re-verification
on wrangler bumps.
Also corrects two comments the change falsifies: the concurrency note claiming
queued runs are covered by "the check_head guard", and the exit-78 note claiming
promote/registry gate on the first check.
Advances: IS-572
🤖 Generated by [robots](https://vyos.io)
* fix: IS-572: run the post-promote probe whenever the deploy succeeded
PROMOTE ran `wrangler deploy` and then the hostname purge, the tarball and
three `r2 object put` calls in one step, while the post-promote probe named no
status function in its `if:` and so carried an implicit success(). A transient
purge 5xx, a tar error or one failed upload therefore failed PROMOTE, skipped
the probe entirely, and left the new version LIVE on the production Worker,
never verified and never rolled back.
Split the step at the deploy. Everything fallible after it moves to a new
`promote_publish` step gated on `steps.promote.outcome == 'success'`, and the
probe becomes the verification finalizer: `!cancelled() && steps.promote.outcome
== 'success'`. `!cancelled()` counts as a status function, which is what
suppresses the implicit success() and lets the probe run after a failed
purge/upload; it is not `always()`, because firing an auto-rollback while an
operator tears the run down is the wrong reflex. Gating on PROMOTE's outcome
rather than re-reading check_head_2 is strictly stronger: 'skipped' on the
moved-branch path, 'success' only once the production deploy completed. The
registry-pointer publish keeps its bare condition, whose implicit success() is
now load-bearing — it is what stops a pointer from naming a rolled-back
generation.
Rollback-target capture no longer reaches backwards. The old jq flattened
`versions[]` across ALL deployments and took the first 100%-traffic id found
scanning newest→oldest, so a current deployment deliberately serving A/B at
90/10 resolved to an older deployment's version — and a probe failure would
"roll back" to it and destroy the split. It now inspects only the current
deployment and refuses otherwise. The verdict is discriminated inside jq so a
malformed record fails red instead of masquerading as split traffic:
`select(.percentage == 100) | .version_id` on `{"percentage":100}` yielded null,
`null // empty` yielded empty, and empty was read as "split" — an unparseable
API response promoting with the auto-rollback silently disarmed. Non-numeric
`percentage`, empty/absent `versions` and unsortable `created_on` are rejected
for the same reason.
The hostname purge is retried once. It is the one post-deploy failure with an
asymmetric knock-on: the probe measures the edge, so a purge that never lands
makes a healthy deploy look like a failed promote.
Finally, the apex PDF range-divergence fallback no longer answers with a 206
carrying bounds the client never requested (RFC 9110 §14.4). It discards the
slice and fails 503, keeping the r2-range-divergence log line that is the actual
alarm. Unreachable under today's workerd, which ignores multi-range.
Advances: IS-572
🤖 Generated by [robots](https://vyos.io)
* ci: IS-572: bind DOCS_CF_LIVE via env: in the purge/registry step
The purge branch expanded ${{ vars.DOCS_CF_LIVE }} directly into the shell
body while the other three consumers of the same value bind it through env:
(PDF carry-forward, candidate reset, post-promote probe). zizmor flags the
inline form as template expansion; the value is repo-controlled so it is not
exploitable, but the env form removes the class and matches the surrounding
steps.
* ci: IS-572: validate DOCS_CF_LIVE, pin wrangler, bound rollback percentages
Three Major findings from CodeRabbit review 4971096766, all outside-diff.
DOCS_CF_LIVE failed open. Four steps branch on it — the SKIP_PDF carry-forward
staleness check, the candidate-reset staleness check, the production hostname
purge and the post-promote probe — and every one compares against the literal
string "true", so an unset or misspelled repository variable silently took the
branch that skips the check, skips the purge and skips the probe, after which
the registry pointer published an unpurged, never-verified deployment on a green
run. Validate the value once at the top of the only job that reads it, so all
four comparisons are safe by construction instead of each re-deriving the guard.
`npx wrangler` in the previous-build metadata fetch ran before `npm ci`, so on
the normal (non-SKIP_PDF) path it downloaded whatever version the registry
served rather than the lockfile-pinned 4.123.0 every later step uses. Move the
install ahead of the first wrangler invocation and make it unconditional; the
inline `npm ci` in the SKIP_PDF branch and the conditional install step after
the check_head guard are both redundant once it is.
The rollback-target jq required `percentage` to be numeric but not in range, so
a current deployment reporting both 100 and 101 yielded exactly one 100%-traffic
match and classified as a determinate `ok` — arming a rollback with a version
selected from data already known to be nonsense. Out-of-range now routes to the
malformed path, never to `split:`.
Advances: IS-572
* ci: IS-572: validate rollback_id at capture, bind it via env at the finalizer
rollback_id originates from Cloudflare's API (`wrangler deployments list
--json`), so it is untrusted text. It was written to $GITHUB_OUTPUT
unvalidated and then reached shell source through a template expansion at
three sites in the post-promote finalizer.
The $GITHUB_OUTPUT write is the sharper of the two exposures: a value
containing a newline injects arbitrary ADDITIONAL step outputs regardless
of how carefully the consumer quotes, because no shell is involved. Validate
at capture — the only place that can stop it.
The character class is deliberately wider than the observed format. wrangler
4.123.0 performs no client-side validation of a version id (it interpolates
the string straight into the API URL path), so the canonical UUID shape is
what Cloudflare's bundled SDK documents rather than something the CLI
enforces. Hard-failing on anything but a UUID would turn a Cloudflare format
change into a blocked promote; hard-failing outside [A-Za-z0-9._-] cannot,
and still certainly excludes newlines, quotes, whitespace and every shell
metacharacter. A safe-but-non-UUID value warns instead of erroring.
Empty stays legitimate — it is the no-target state of all three benign paths
(first deploy, no deployments, split traffic) and the finalizer's -z branch
depends on it. A malformed non-empty value fails loudly, matching how the
surrounding capture already treats malformed API data.
At the finalizer, ROLLBACK_ID is bound through the step's env: mapping and
the three sites use "$ROLLBACK_ID", so the value is never substituted into
the shell source. The -z semantics are unchanged.
Advances: IS-572
* ci: IS-572: verify probe status, reconcile a failed pointer publish
Two merge blockers from the pre-merge adversarial gate on d079a2c9.
1. The post-promote probe accepted an error response as verification.
`withDocsHeaders` sets X-Docs-Build unconditionally, so error responses
carry it too; the probe read only that header and never the status line,
so a production answering 500 with the new SHA satisfied the match and
the registry pointer published for a generation serving errors.
The probe now parses the status line and requires an exact 200 alongside
the built SHA. Parsing resets on every status line, so 1xx interim
responses contribute nothing and only the final response block is read;
duplicate X-Docs-Build headers with identical values are accepted while
conflicting ones surface as "<conflicting>" and can never match. Requests
are bounded with --max-time 20 and the pipeline runs under pipefail so a
transport failure is distinguishable from a missing header. Early-attempt
non-200s and transport failures stay retryable — only the final attempt's
verdict decides. Every unverified outcome still rolls back, but the three
causes now carry distinguishable diagnoses.
--fail is deliberately not used: it collapses 4xx/5xx into curl exit 22
and discards the status line, which is the value needed to tell a broken
new version apart from a transport failure.
2. Production and the registry could diverge permanently. The round-8
promote/publish split correctly decoupled a publish failure from the
probe, but added no convergence action for the case where the probe then
succeeds: production served the new SHA while latest.json still named the
old one, with nothing to reconcile it. latest.json is the source of truth
for the candidate-reset staleness check and the SKIP_PDF carry-forward, so
both would later restore a generation production no longer serves.
A reconciliation step now runs when the promote and probe both succeeded
but the pointer did not publish. It converges FORWARD — re-uploading the
sha-scoped generation and only then publishing the pointer, so the pointer
can never name an incomplete generation — and on failure leaves the
pointer untouched and reports the divergence by name with both SHAs and a
copy-pasteable manual repair. Gating on the probe's success keeps it off
the rollback paths entirely, where production and the pointer already
agree. The pointer step's load-bearing implicit success() is unchanged.
Refs: IS-572
* ci: IS-572: drop registry-pointer reconciliation, keep the probe fix
Scope reduction. The `Reconcile registry pointer after a failed publish`
step introduced in the previous commit is removed; the post-promote probe
rewrite in that same commit stays.
Removing the step returns registry-divergence behaviour to what `rolling`
has today: when `promote_publish` fails after a successful deploy, the
pointer step's load-bearing implicit success() skips it, and production can
end up serving a SHA that `$slug/latest.json` does not name. That path is
now documented at the `Publish registry pointer` step and tracked in
https://vyos.dev/T9237 rather than repaired here.
Also reverts the DOCS_CF_LIVE header comment to "four later steps", which
restores its internal agreement with the "four comparisons downstream"
sentence in the same block.
Advances: IS-572
🤖 Generated by [robots](https://vyos.io)
| -rw-r--r-- | .github/workflows/docs-build.yml | 509 | ||||
| -rw-r--r-- | .github/workflows/docs-canary-qa.yml | 21 | ||||
| -rw-r--r-- | scripts/docs_gates/gates.py | 7 | ||||
| -rw-r--r-- | scripts/docs_gates/parity.py | 160 | ||||
| -rw-r--r-- | scripts/docs_gates/smoke.py | 44 | ||||
| -rw-r--r-- | scripts/docs_gates/test_gates.py | 18 | ||||
| -rw-r--r-- | scripts/docs_gates/test_parity.py | 270 | ||||
| -rw-r--r-- | scripts/docs_gates/test_smoke.py | 117 | ||||
| -rw-r--r-- | workers/apex/src/index.ts | 438 | ||||
| -rw-r--r-- | workers/apex/src/special.ts | 7 | ||||
| -rw-r--r-- | workers/apex/src/uagate.ts | 40 | ||||
| -rw-r--r-- | workers/apex/test/router.test.ts | 759 | ||||
| -rw-r--r-- | workers/apex/test/uagate.test.ts | 78 | ||||
| -rw-r--r-- | workers/apex/ua-policy.json | 2 |
14 files changed, 2363 insertions, 107 deletions
diff --git a/.github/workflows/docs-build.yml b/.github/workflows/docs-build.yml index bdb9e41d..8855b8d9 100644 --- a/.github/workflows/docs-build.yml +++ b/.github/workflows/docs-build.yml @@ -13,8 +13,12 @@ on: concurrency: group: docs-build-${{ github.ref_name }} # false (not true): a cancel mid-promote would leave production unverified and the - # registry pointer stale — queued runs are safe because the check_head guard (§7.1) - # skips any run whose SHA is no longer the branch tip. + # registry pointer stale. Queueing instead is safe because BOTH check_head guards + # (§7.1) skip a run whose SHA is no longer the branch tip — the first before the + # candidate deploy, the second after the smoke gate and immediately before promote. + # A single up-front check was NOT enough: it says nothing about the run that is + # already past it, and the candidate deploy plus the smoke gate (480s deadline) give + # a push minutes of room to land in between. cancel-in-progress: false permissions: @@ -40,6 +44,37 @@ jobs: echo "pdf=$(jq -r --arg s "$(echo "$entry" | jq -r .slug)" \ '.versions[] | select(.slug==$s) | .pdf // empty' workers/versions.json)" >> "$GITHUB_OUTPUT" + # DOCS_CF_LIVE is the pre/post-cutover switch, and four later steps in this job + # branch on it: the SKIP_PDF carry-forward staleness check, the candidate-reset + # staleness check, the production hostname purge, and the post-promote probe. + # Every one of those comparisons is written against the literal string "true", so + # ANY other value — an unset variable, "True", "yes", a typo — silently takes the + # branch that skips the check, skips the purge and skips the probe, after which the + # registry pointer publishes an unpurged, never-verified deployment on a green run. + # Validate the value ONCE, here, at the top of the only job that reads it: the four + # comparisons downstream are then safe by construction rather than each re-deriving + # the same three-line guard. Placed before the docker build so a misconfigured + # variable costs seconds, not a full build. A future job that reads this variable + # needs its own gate — this one covers build-deploy only. + - name: Validate DOCS_CF_LIVE + env: + DOCS_CF_LIVE: ${{ vars.DOCS_CF_LIVE }} + run: | + set -eu + case "$DOCS_CF_LIVE" in + true|false) + echo "DOCS_CF_LIVE=$DOCS_CF_LIVE" + ;; + "") + echo "::error::repository variable DOCS_CF_LIVE is unset or empty — it must be set to exactly 'true' or 'false'" + exit 1 + ;; + *) + echo "::error::repository variable DOCS_CF_LIVE is '$DOCS_CF_LIVE' — it must be exactly 'true' or 'false' (lowercase, no quotes)" + exit 1 + ;; + esac + # Build image in-workflow from docker/Dockerfile (plan v4.1: digest pin dropped — # no published ghcr.io image exists; the checked-out commit IS the pin, and the # buildx GHA cache makes repeat builds cheap). @@ -117,6 +152,16 @@ jobs: cp -r docs/_build/html/. "dist/assets/en/$slug/" npx --yes pagefind@1.5.2 --site "dist/assets/en/$slug" + # Must precede the FIRST `npx wrangler` invocation in this job. Without + # workers/node_modules, `npx` silently downloads whatever wrangler the registry + # serves today, so the SKIP_PDF carry-forward and the previous-build metadata fetch + # below would run on an unpinned version while every later step — including the + # rollback path — runs the lockfile-pinned one. Unconditional on purpose: the + # metadata fetch below runs on every build, long before the check_head guard that + # used to gate this install. + - name: Install workers deps + run: cd workers && npm ci + - name: PDF into artifact (build or registry carry-forward) env: CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} @@ -130,7 +175,7 @@ jobs: # §5 + §7.1.4: SKIP_PDF refuses to run when registry is stale vs production. # Registry layout is pointer-indirected (§7.1.4 atomicity): resolve # $slug/latest.json → sha, then read the sha-scoped generation. - cd workers && npm ci && npx wrangler r2 object get "$REGISTRY_BUCKET/$slug/latest.json" --file /tmp/latest.json --remote && cd .. + cd workers && npx wrangler r2 object get "$REGISTRY_BUCKET/$slug/latest.json" --file /tmp/latest.json --remote && cd .. reg_sha=$(jq -r .sha /tmp/latest.json) if [ "$DOCS_CF_LIVE" = "true" ]; then prod_sha=$(curl -sI "https://docs.vyos.io/en/$slug/" | tr -d '\r' | awk -F': ' 'tolower($1)=="x-docs-build"{print $2}') @@ -191,16 +236,37 @@ jobs: --versions workers/versions.json \ $( [ -f previous-meta.json ] && echo --previous-meta previous-meta.json ) + # Exit 78 was the "neutral" status ONLY in the deprecated Actions v1 runtime. On the + # current runtime any non-zero exit fails the step and the job, so this guard's own + # "skipping deploy" message was a lie: every legitimate branch-moved race — the exact + # situation `cancel-in-progress: false` deliberately creates — surfaced as a red run. + # That erodes the guard's value as an alarm. Publish an output instead and gate the + # deploy steps on it, so a superseded run ends green-and-skipped. This is the FIRST + # of two checkpoints: it covers the candidate deploy and the smoke gate, which touch + # nothing production-facing. PROMOTE, the post-promote probe and the registry pointer + # publish gate on the post-smoke re-check further down, NOT on this output. - name: check_head guard (§7.1) + id: check_head + env: + REF_NAME: ${{ github.ref_name }} + BUILT_SHA: ${{ github.sha }} run: | - set -eu - remote=$(git ls-remote origin "refs/heads/${{ github.ref_name }}" | cut -f1) - [ "$remote" = "${{ github.sha }}" ] || { echo "branch moved — skipping deploy"; exit 78; } - - - name: Install workers deps - run: cd workers && npm ci + set -euo pipefail + remote=$(git ls-remote origin "refs/heads/$REF_NAME" | cut -f1) + # pipefail AND the emptiness check are both needed: `cut` exits 0 on empty input, so + # a failed ls-remote would otherwise leave $remote empty and be misread as "branch + # moved" — silently skipping a deploy that should have run. An unresolvable tip is + # an infrastructure failure, not a skip signal, so it fails the job loudly. + [ -n "$remote" ] || { echo "::error::could not resolve the remote tip of $REF_NAME"; exit 1; } + if [ "$remote" = "$BUILT_SHA" ]; then + echo "current=true" >> "$GITHUB_OUTPUT" + else + echo "current=false" >> "$GITHUB_OUTPUT" + echo "::notice::branch moved (tip is now $remote, this run built $BUILT_SHA) — skipping deploy, promote and registry steps" + fi - name: Deploy CANDIDATE + if: steps.check_head.outputs.current == 'true' env: CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} @@ -213,6 +279,16 @@ jobs: - name: Pre-traffic smoke via canary apex (§7.1.2) id: smoke + if: steps.check_head.outputs.current == 'true' + env: + # CF Access service token goes through the environment, NEVER through argv: a + # secret on the command line is readable from the process table for the life of + # the process and is captured verbatim by `set -x` traces and crash dumps. + # These environment variables are the ONLY supported way to pass the token: the + # --access-id / --access-secret flags were removed, and argparse now rejects them + # outright rather than ignoring them and leaving every probe to 403. + CF_ACCESS_CLIENT_ID: ${{ secrets.CF_ACCESS_CLIENT_ID }} + CF_ACCESS_CLIENT_SECRET: ${{ secrets.CF_ACCESS_CLIENT_SECRET }} run: | set -eu # NOTE: never use the GHA "cond AND format(...)" expression trick for optional @@ -227,12 +303,10 @@ jobs: python -m scripts.docs_gates.smoke \ --host docs-next.vyos.io --slug '${{ steps.matrix.outputs.slug }}' \ --expect-sha '${{ github.sha }}' \ - --access-id '${{ secrets.CF_ACCESS_CLIENT_ID }}' \ - --access-secret '${{ secrets.CF_ACCESS_CLIENT_SECRET }}' \ $pdf_arg - name: Candidate reset on smoke failure (§7.1.3) - if: failure() && steps.smoke.conclusion == 'failure' + if: failure() && steps.check_head.outputs.current == 'true' && steps.smoke.conclusion == 'failure' env: CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} @@ -257,8 +331,68 @@ jobs: --name '${{ steps.matrix.outputs.worker }}-candidate' \ --var DOCS_BUILD_SHA:"$reg_sha" --var DOCS_ENV:canary - - name: PROMOTE (capture rollback id → deploy → purge → registry, §7.1.4) + # TOCTOU re-check. The guard above evaluated the tip ONCE, before the candidate + # deploy and before the smoke gate (which alone budgets a 480s deadline), so a push + # landing in that window left this run promoting a superseded SHA to production and + # publishing a registry pointer naming it. `cancel-in-progress: false` does not help: + # it makes the SECOND run queue and skip, while the run already past the first check + # sails on. Re-resolve the tip now that smoke has passed, immediately before the + # first step that touches production, and gate PROMOTE on THIS output instead of the + # first one. The steps downstream of PROMOTE chain off PROMOTE's own `outcome` + # rather than reading this output again — a strictly stronger predicate, since + # `outcome` is 'skipped' exactly when this check says the branch moved AND is + # 'success' only once the production deploy has actually completed. + # The probe in particular must move with PROMOTE: gated on the first check it would + # still run after a skipped promote, see production serving the (correct) previous + # SHA, read that as a promote failure and roll back a production nobody touched. + # Residual window: this check → `wrangler deploy` is a single step boundary, seconds + # rather than minutes. It is not zero — Cloudflare offers no compare-and-swap on + # deploy — so the guard narrows the race, it does not eliminate it. + # The candidate deployed above needs no cleanup on the moved-branch path: candidate + # Workers carry no route, and the queued run for the new tip overwrites the candidate + # with its own deploy. + # Moved branch ends the run GREEN-and-skipped, matching the convention the first + # guard's redesign established (see the exit-78 note above): a legitimate branch race + # is not an operator-actionable failure, production and the registry pointer are + # untouched, and the queued run promotes the new tip. Failing red here would + # reintroduce exactly the false alarm that redesign removed. + - name: check_head re-guard after smoke (§7.1) + id: check_head_2 + if: steps.check_head.outputs.current == 'true' + env: + REF_NAME: ${{ github.ref_name }} + BUILT_SHA: ${{ github.sha }} + run: | + set -euo pipefail + remote=$(git ls-remote origin "refs/heads/$REF_NAME" | cut -f1) + # Same hardening as the first guard, and for the same reason: `cut` exits 0 on + # empty input, so pipefail AND the emptiness check are both needed — otherwise a + # failed ls-remote leaves $remote empty and is misread as "branch moved", turning + # an infrastructure failure into a silent skip of a promote that should have run. + [ -n "$remote" ] || { echo "::error::could not resolve the remote tip of $REF_NAME"; exit 1; } + if [ "$remote" = "$BUILT_SHA" ]; then + echo "current=true" >> "$GITHUB_OUTPUT" + else + echo "current=false" >> "$GITHUB_OUTPUT" + echo "::notice::branch moved during build/smoke (tip is now $remote, this run built $BUILT_SHA) — skipping promote, post-promote probe and registry pointer publish" + fi + + # This step ENDS at `wrangler deploy`. Everything that used to follow it in the same + # step — the hostname purge, the tarball, the three registry uploads — moved to the + # next step, because a failure in any of them failed THIS step, and the post-promote + # probe carried an implicit success() (its `if:` named no status function), so it was + # skipped. A transient purge 5xx or one failed upload therefore left the new version + # LIVE on the production Worker, never probed and never rolled back. + # The split is preferred over flagging `deployed=true` from inside the shell: the + # invariant then lives in the `if:` conditions where it is auditable statically, and + # nothing depends on outputs written by a step that went on to fail. + # Residual window it does NOT close: if `wrangler deploy` itself errors AFTER the + # deployment has taken effect, this step fails, the probe is skipped and production + # is unverified. Cloudflare offers no way to distinguish that from a deploy that + # never landed; the run is red either way, so it is an operator-visible state. + - name: PROMOTE (capture rollback id → deploy, §7.1.4) id: promote + if: steps.check_head_2.outputs.current == 'true' env: CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} @@ -268,28 +402,221 @@ jobs: slug='${{ steps.matrix.outputs.slug }}' worker='${{ steps.matrix.outputs.worker }}' cd workers - # Rollback target: verify this JSON shape on the FIRST real deploy — - # `npx wrangler deployments list --name "$worker" --json | jq .` — and adjust - # the jq path if wrangler's output differs (their JSON shape has churned across - # majors; the ID needed is the CURRENT deployment's, i.e. the newest entry). + # Rollback target, captured BEFORE the deploy below replaces production. + # `wrangler deployments list --json` (verified against wrangler 4.123.0's + # versionsDeploymentsListHandler) dumps the raw deployment objects sorted + # ASCENDING by created_on, so the CURRENT deployment is the LAST entry — NOT + # `.[0]`. Each entry looks like: + # {"id":"<deployment-id>","created_on":"<iso8601>","author_email":…, + # "versions":[{"version_id":"<version-id>","percentage":100}, …]} + # `wrangler rollback` takes a VERSION id (it resolves its positional through + # fetchVersion() and redeploys that version at 100%), never a deployment id, so + # we extract versions[].version_id — not the deployment's own `id`. + # SPLIT TRAFFIC: a deployment may spread traffic over several versions, and one + # with no single 100% version is not a safe rollback target. That is a state to + # REPORT, not to route around — see the extraction below. The reference point is + # wrangler's own fetchDefaultRollbackVersionId() minus its .shift(): wrangler + # picks AFTER deploying so it must skip the current deployment, whereas we + # capture BEFORE, so for us the newest entry is the good one we want back. # FIRST-DEPLOY BOOTSTRAP: the production Worker does not exist before the very - # first promote — `deployments list` fails/returns empty. That is a valid state: - # rollback_id stays empty, the deploy CREATES the Worker, and the post-promote - # step knows an empty id means "nothing to roll back to". - if deps=$(npx wrangler deployments list --name "$worker" --json 2>/dev/null); then - rollback_id=$(echo "$deps" | jq -r '.[0].id // empty') + # first promote, so `deployments list` fails. That is a valid state: rollback_id + # stays empty, the deploy CREATES the Worker, and the post-promote step knows an + # empty id means "nothing to roll back to". EVERY OTHER failure of that command — + # auth, network, an unparseable payload — must fail the step LOUDLY: an empty + # rollback_id promotes with the auto-rollback silently disarmed while the run + # still looks healthy, which is strictly worse than a red build. + # + # Telling the two apart cannot use the exit status: in wrangler 4.123.0 every + # error path ends in a single uncaught throw, so the process exits 1 regardless of + # cause, and a failed command writes nothing machine-readable to stdout. The only + # discriminator is the Cloudflare API error code wrangler renders into its stderr + # note as `... [code: NNNNN]`. Codes 10007 (script not found) and 10090 (legacy + # environment not found) are exactly the pair wrangler's own isWorkerNotFoundError() + # treats as "this Worker does not exist"; authentication failures carry 9106/10000 + # (its AUTHENTICATION_ERROR_CODES) and connectivity failures never reach the API + # so they carry no code at all — neither can be mistaken for the bootstrap case. + # The code was read out of the shipped 4.123.0 bundle, NOT observed against a live + # missing Worker, so re-verify this signature whenever wrangler is bumped; an + # unrecognised future spelling costs one loud red run, never a silent disarm. + # The note is esbuild-formatted (ANSI colour, wrapped to the terminal width), so + # strip escapes and collapse whitespace before matching. + rollback_id="" + list_err=$(mktemp) + if deps=$(npx wrangler deployments list --name "$worker" --json 2>"$list_err"); then + # Validate the shape up front and fail on anything else, rather than coercing a + # non-array to [] and reporting "no rollback target" for malformed output. + if ! printf '%s' "$deps" | jq -e 'type == "array"' >/dev/null 2>&1; then + echo "::error::wrangler deployments list --json for $worker did not return a JSON array — refusing to promote with the rollback target undetermined" + exit 1 + fi + # ONLY THE CURRENT (newest) DEPLOYMENT is a legitimate rollback target. An + # earlier spelling of this expression flattened `versions[]` across ALL + # deployments and took the first 100%-traffic id found scanning newest→oldest, + # so it reached BACKWARDS: a current deployment deliberately serving A/B at + # 90/10 resolved to some older deployment's 100% version, and a probe failure + # would then "roll back" to it and destroy the split that was set on purpose. + # Take the current deployment's single 100%-traffic version, or refuse. + # + # The verdict is discriminated inside jq and reported out here so that a + # MALFORMED record fails the step instead of masquerading as a determinate + # state. It used to masquerade: `select(.percentage == 100) | .version_id` on + # `{"percentage":100}` yields null, `null // empty` yields empty, and empty was + # read as "split traffic" — i.e. an unparseable API response promoted with the + # auto-rollback silently disarmed. The three good states stay distinguishable: + # no deployments at all → notice; a real traffic split on the CURRENT + # deployment → loud warning + promote unprotected; anything unparseable → red. + # + # `percentage` is required to be NUMERIC for the same reason: the API returns + # numbers, and a missing field or a string "100" slips past a `== 100` test, so + # a deployment that is actually at 100% would be reported as split and promote + # with the rollback disarmed. A percentage this step cannot read leaves it + # unable to tell "100%" from "a split", and guessing wrong destroys live + # traffic — so it fails red rather than guess. It must also be IN RANGE: a + # current deployment reporting both 100 and 101 yields exactly one + # 100%-traffic match, so the numeric check alone would classify plainly + # impossible data as a determinate "ok" and arm a rollback selected from it. + # Out-of-range therefore routes to the malformed path, never to "split:". + # + # Validation scope: `created_on` is checked on EVERY record because it decides + # which record is "current", so one unusable value corrupts the selection. + # `versions` / `version_id` are checked on the current record only — it is the + # only record this step reads, and failing on an older record's shape would + # block promotes over data with no bearing on the rollback target. + verdict=$(printf '%s' "$deps" | jq -r ' + def bad($why): "malformed:" + $why; + if length == 0 then "empty:" + elif any(.[]; type != "object") then + bad("a deployment entry is not an object") + elif any(.[]; (.created_on | type) != "string") then + bad("a deployment entry has no string created_on to order by") + else + (sort_by(.created_on) | last) as $cur + | if ($cur.versions | type) != "array" then + bad("the current deployment has no versions array") + elif ($cur.versions | length) == 0 then + bad("the current deployment has an empty versions array") + elif any($cur.versions[]; type != "object") then + bad("the current deployment has a non-object versions entry") + elif any($cur.versions[]; (.percentage | type) != "number") then + bad("the current deployment has a versions entry with a non-numeric percentage") + elif any($cur.versions[]; .percentage < 0 or .percentage > 100) then + bad("the current deployment has a versions entry with an out-of-range percentage") + else + [$cur.versions[] | select(.percentage == 100)] as $full + | if ($full | length) == 0 then "split:" + elif ($full | length) > 1 then + bad("the current deployment reports more than one 100%-traffic version") + elif (($full[0].version_id | type) != "string") or ($full[0].version_id == "") then + bad("the current deployment 100%-traffic version has no usable version_id") + else "ok:" + $full[0].version_id + end + end + end + ') || { echo "::error::could not evaluate the deployments payload for $worker — refusing to promote with the rollback target undetermined"; exit 1; } + case "${verdict%%:*}" in + ok) + rollback_id=${verdict#*:} + echo "rollback target for $worker: version $rollback_id" + ;; + empty) + echo "::notice::$worker has no deployments yet — nothing to roll back to; rollback disabled for this run" + ;; + split) + # Determinate state, not a swallowed failure: `wrangler rollback` takes one + # version id, so a split-traffic deployment has no single-version target to + # offer it. Warn loudly and promote unprotected rather than block a promote + # on a traffic split someone set deliberately. + echo "::warning::$worker's current deployment has no single 100%-traffic version (split traffic) — no safe rollback target; rollback disabled for this run" + ;; + *) + echo "::error::wrangler deployments list --json for $worker returned a record this step cannot trust (${verdict#*:}) — refusing to promote with the rollback target undetermined" + exit 1 + ;; + esac else - rollback_id="" - echo "first deploy for $worker — no prior deployment; rollback disabled for this run" + cat "$list_err" >&2 + esc=$(printf '\033') + if sed "s/${esc}\\[[0-9;]*m//g" "$list_err" | tr -s '[:space:]' ' ' \ + | grep -qE '\[code: (10007|10090)\]'; then + echo "::notice::production Worker $worker does not exist yet (API code 10007/10090) — first-deploy bootstrap; rollback disabled for this run" + else + echo "::error::wrangler deployments list --name $worker failed for a reason other than 'Worker does not exist' (see stderr above) — refusing to promote with the auto-rollback disarmed" + exit 1 + fi + fi + rm -f "$list_err" + # rollback_id came out of Cloudflare's API (`wrangler deployments list --json`), + # so it is untrusted text, and this is the last point before it becomes a step + # output. Two distinct exposures, only one of which careful quoting can close: + # * a NEWLINE in the value injects arbitrary ADDITIONAL step outputs into + # $GITHUB_OUTPUT. No shell is involved, so no amount of care at the consumer + # end helps; this check is the only place that can stop it. + # * shell metacharacters at the consumers, handled separately by passing the + # value through `env:` instead of a template expansion (see the finalizer). + # The accepted character class is deliberately WIDER than the observed format. + # wrangler 4.123.0 performs no client-side validation of a version id — it + # interpolates the string straight into the API URL path — so the canonical + # UUID shape below is what Cloudflare's own bundled SDK documents, not something + # the CLI enforces. Hard-failing on anything but a UUID would turn a Cloudflare + # format change into a blocked promote; hard-failing on anything outside + # [A-Za-z0-9._-] cannot, and still certainly excludes newlines, quotes, + # whitespace and every shell metacharacter. A non-UUID that is within the safe + # set is therefore a warning, not an error. + # EMPTY STAYS LEGITIMATE: it is the no-target state of all three benign paths + # above (first deploy, no deployments, split traffic) and the finalizer's -z + # branch depends on it. The malformed value is never echoed back. + if [ -n "$rollback_id" ]; then + case "$rollback_id" in + *[!A-Za-z0-9._-]*) + echo "::error::wrangler deployments list --json for $worker returned a version_id containing characters no Worker version id may contain — refusing to promote with an untrustworthy rollback target" + exit 1 + ;; + esac + if [ "${#rollback_id}" -gt 128 ]; then + echo "::error::wrangler deployments list --json for $worker returned a ${#rollback_id}-character version_id (max 128) — refusing to promote with an untrustworthy rollback target" + exit 1 + fi + if ! printf '%s' "$rollback_id" \ + | grep -qE '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$'; then + echo "::warning::$worker's rollback target '$rollback_id' is within the safe character set but is not the canonical UUID shape Cloudflare documents for version_id — accepted, but worth a look" + fi fi echo "rollback_id=$rollback_id" >> "$GITHUB_OUTPUT" npx wrangler deploy --config branch/wrangler.$(case "$slug" in rolling) echo rolling;; 1.5) echo v15;; 1.4) echo v14;; esac).jsonc \ --name "$worker" --var DOCS_BUILD_SHA:'${{ github.sha }}' --var DOCS_ENV:production - if [ "${{ vars.DOCS_CF_LIVE }}" = "true" ]; then - # hostname-scoped purge (§3.3; all-plans since 2025-04) + + # Gated on PROMOTE's OUTCOME, not on check_head_2 directly: `outcome` is 'skipped' + # exactly when the branch moved (PROMOTE carries the check_head_2 condition and its + # own implicit success()), and 'success' only once the production deploy completed. + # A failure in this step no longer suppresses the probe below — that is the whole + # point of the split. + - name: Purge + registry generation upload (§7.1.4) + id: promote_publish + if: steps.promote.outcome == 'success' + env: + CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} + CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} + CF_ZONE_ID: ${{ vars.CF_ZONE_ID_VYOS_IO }} + DOCS_CF_LIVE: ${{ vars.DOCS_CF_LIVE }} + run: | + set -eu + slug='${{ steps.matrix.outputs.slug }}' + sha='${{ github.sha }}' + # hostname-scoped purge (§3.3; all-plans since 2025-04), retried once for the same + # reason the registry uploads below are. This purge is the one post-deploy failure + # whose knock-on effect is asymmetric: the probe measures the EDGE, so a purge that + # never lands leaves the edge serving the previous generation and the probe reads + # that as a failed promote — rolling back a deploy that was in fact fine. One retry + # removes the transient-5xx bulk of that class at no cost. What remains (a purge + # API that is genuinely down) still ends in a rollback, deliberately: see the probe + # step's note. + purge_hostname() { curl -sf -X POST "https://api.cloudflare.com/client/v4/zones/$CF_ZONE_ID/purge_cache" \ -H "Authorization: Bearer $CLOUDFLARE_API_TOKEN" -H "Content-Type: application/json" \ --data '{"hosts":["docs.vyos.io"]}' + } + if [ "$DOCS_CF_LIVE" = "true" ]; then + purge_hostname || { sleep 5; purge_hostname; } else echo "DOCS_CF_LIVE=false — pre-cutover: skipping production hostname purge" fi @@ -302,9 +629,8 @@ jobs: # a half-written generation (e.g. new tar.zst, still-old meta.json) mid upload. # Uploading under $slug/<sha>/* first — a key no prior generation ever reused — # keeps readers of the OLD pointer seeing a full, untouched old generation. - cd .. && tar --zstd -cf lastgood.tar.zst -C dist/assets . + tar --zstd -cf lastgood.tar.zst -C dist/assets . page_count=$(find "dist/assets/en/$slug" -name '*.html' | wc -l | tr -d ' ') - sha='${{ github.sha }}' printf '{"sha":"%s","page_count":%s}' "$sha" "$page_count" > lastgood.meta.json printf '{"sha":"%s"}' "$sha" > latest.json cd workers @@ -318,48 +644,153 @@ jobs: exit 1 fi + # THE VERIFICATION FINALIZER. Once `wrangler deploy` has succeeded, production has + # changed and this step must run whatever happens afterwards — so the condition is + # deliberately NOT the bare `steps.check_head_2...` form, which would pick up an + # implicit success() and be skipped by a failed purge or a failed registry upload, + # leaving production live and unverified. + # `!cancelled()` and not `always()`: on a cancelled run the deploy may not even have + # finished, and firing an auto-rollback while the operator is tearing the run down is + # the wrong reflex. `!cancelled()` counts as a status function, so it also suppresses + # the implicit success() — which is exactly what lets this run after a failed + # promote_publish. + # `steps.promote.outcome == 'success'` keeps both older guarantees intact: 'skipped' + # on the moved-branch path (so the probe cannot see production serving the correct + # previous SHA and read it as a promote failure), and never 'success' unless the + # rollback_id below was captured by a step that ran to completion. + # + # DELIBERATE: reaching here after a FAILED purge can roll back a deploy that was + # itself fine — the Worker took the new version, the edge kept serving the old one, + # and this probe measures the edge. That is the intended trade. The alternative, + # exiting without a rollback whenever the purge failed, leaves an UNVERIFIED version + # live on production, which is precisely the state this whole gate exists to prevent; + # a rollback instead returns production to the last version known to serve correctly, + # which is also what the stale edge is already serving, and the run is red either way + # so an operator looks at it. The transient half of that class is retried away in the + # purge step; what is left is a purge API that is down, and a known-good production + # is the right place to be sitting while it is. - name: Post-promote probe + auto-rollback (§7.1.5) + id: probe + if: ${{ !cancelled() && steps.promote.outcome == 'success' }} env: + # API-derived (Cloudflare `deployments list`), so it is bound here rather than + # expanded into the script text below: a template expansion is textual + # substitution into the shell source, an env var is not. Validated at capture. + ROLLBACK_ID: ${{ steps.promote.outputs.rollback_id }} CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} CF_ZONE_ID: ${{ vars.CF_ZONE_ID_VYOS_IO }} DOCS_CF_LIVE: ${{ vars.DOCS_CF_LIVE }} run: | set -eu + # pipefail: probe_once below is a pipeline whose FIRST command is the curl that can + # fail. Without it the pipeline's status is awk's, and a transport failure would be + # indistinguishable from a missing header. + set -o pipefail if [ "$DOCS_CF_LIVE" != "true" ]; then echo "DOCS_CF_LIVE=false — pre-cutover: docs.vyos.io still serves RTD; skipping production probe" exit 0 fi slug='${{ steps.matrix.outputs.slug }}' + # VERIFICATION REQUIRES BOTH A SUCCESSFUL STATUS AND THE BUILT SHA. `withDocsHeaders` + # (workers/branch/src/index.ts) sets X-Docs-Build UNCONDITIONALLY — error responses + # carry it too, which that function itself acknowledges by special-casing + # `status >= 400` for Cache-Control. Matching the header alone therefore accepted a + # production answering 500 with the new SHA as a verified promote, after which the + # registry pointer published for a generation that was serving errors. + # + # `--fail` is deliberately NOT used: it collapses every 4xx/5xx into curl exit 22 and + # discards the status line — the one value needed to tell a broken new version (500) + # apart from a transport failure. Parsing the status explicitly supersedes it. + # + # probe_once prints "<status> <sha>" for the FINAL response block. Resetting on every + # status line means 1xx interim responses (e.g. Cloudflare Early Hints) contribute + # nothing — only the last block's headers are read. Duplicate X-Docs-Build headers + # with IDENTICAL values state the same fact and are accepted; CONFLICTING values are + # untrustworthy and surface as "<conflicting>", which can never equal the built SHA. + probe_once() { + curl -sSI --max-time 20 "https://docs.vyos.io/en/$slug/" \ + | tr -d '\r' \ + | awk ' + /^HTTP\// { code = $2; n = 0; v = ""; next } + tolower($0) ~ /^x-docs-build:/ { + val = $0 + sub(/^[^:]*:[ \t]*/, "", val) + sub(/[ \t]+$/, "", val) + n++ + if (n == 1) { v = val } else if (val != v) { v = "<conflicting>" } + } + END { printf "%s %s\n", (code == "" ? "<no-status>" : code), (n == 0 ? "<absent>" : v) } + ' + } # Retry loop: the purge above is asynchronous, so an immediate probe can still # observe a stale edge response and trigger a false rollback. Poll up to 6 times - # (60s budget) before concluding the promote genuinely failed. - got="" + # (60s budget) before concluding the promote genuinely failed. A non-200 or a + # transport failure on an EARLY attempt is retried for the same reason a stale SHA + # is — only the FINAL attempt's verdict decides. + code="<no-status>" + got="<absent>" + verified=false for attempt in 1 2 3 4 5 6; do - got=$(curl -sI "https://docs.vyos.io/en/$slug/" | tr -d '\r' | awk -F': ' 'tolower($1)=="x-docs-build"{print $2}') - if [ "$got" = '${{ github.sha }}' ]; then + if out=$(probe_once); then + code=${out%% *} + got=${out#* } + else + # curl itself failed (DNS, TLS, connection refused, --max-time exceeded). + code="<transport-error>" + got="<none>" + fi + if [ "$code" = "200" ] && [ "$got" = '${{ github.sha }}' ]; then + verified=true break fi if [ "$attempt" -lt 6 ]; then - echo "::notice::post-promote probe attempt $attempt/6 saw stale edge (got $got) — retrying in 10s" + echo "::notice::post-promote probe attempt $attempt/6 unverified (status $code, X-Docs-Build $got) — retrying in 10s" sleep 10 fi done - if [ "$got" != '${{ github.sha }}' ]; then - if [ -z '${{ steps.promote.outputs.rollback_id }}' ]; then - echo "::error::post-promote probe failed (got $got) on FIRST deploy — no prior version to roll back to; investigate manually" + if [ "$verified" != "true" ]; then + # Same action for every unverified outcome — a rollback to the last version known + # to serve correctly — but distinguishable diagnoses, because "200 with the wrong + # SHA" (stale edge / promote never took) and "500 with the right SHA" (the new + # version is live and broken) point an operator at completely different causes. + if [ "$code" = "200" ]; then + reason="production answered 200 but served X-Docs-Build $got, not ${{ github.sha }}" + elif [ "$code" = "<transport-error>" ]; then + reason="production could not be reached at all after 6 attempts (see curl stderr above)" + else + reason="production answered status $code carrying X-Docs-Build $got — the deployed version is live but not serving successfully" + fi + if [ -z "$ROLLBACK_ID" ]; then + echo "::error::post-promote probe failed ($reason) but PROMOTE captured no rollback target (first deploy, or no 100%-traffic version) — investigate manually" exit 1 fi - echo "::error::post-promote probe failed (got $got) — rolling back" - cd workers && npx wrangler rollback '${{ steps.promote.outputs.rollback_id }}' \ - --name '${{ steps.matrix.outputs.worker }}' --yes + # rollback_id is a Worker VERSION id (see the PROMOTE capture above); + # `wrangler rollback <version-id>` redeploys that version at 100% traffic. + echo "::error::post-promote probe failed ($reason) — rolling back to version $ROLLBACK_ID" + cd workers && npx wrangler rollback "$ROLLBACK_ID" \ + --name '${{ steps.matrix.outputs.worker }}' \ + --message 'auto-rollback: docs-build post-promote probe failed' --yes curl -sf -X POST "https://api.cloudflare.com/client/v4/zones/$CF_ZONE_ID/purge_cache" \ -H "Authorization: Bearer $CLOUDFLARE_API_TOKEN" -H "Content-Type: application/json" \ --data '{"hosts":["docs.vyos.io"]}' exit 1 fi + # The IMPLICIT success() on this condition is load-bearing and must stay implicit: + # naming no status function is what makes this step skip whenever ANY earlier step + # failed — the probe (so a rolled-back generation never becomes the pointer target) + # and the purge/upload step (so a pointer never names a generation that was not + # fully uploaded). Adding always()/!cancelled() here would publish over a rollback. + # + # KNOWN GAP: because that success() is implicit, a failed `promote_publish` skips + # this step even when the probe went on to verify production serving this SHA — and + # a terminal failure here has the same effect. Either way production and + # $slug/latest.json disagree, and nothing in this job reconciles them. Tracked in + # https://vyos.dev/T9237. - name: Publish registry pointer (§7.1.4) + id: publish_pointer + if: steps.check_head_2.outputs.current == 'true' env: CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN_DOCS }} CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} diff --git a/.github/workflows/docs-canary-qa.yml b/.github/workflows/docs-canary-qa.yml index 7f3be7c0..59fc5849 100644 --- a/.github/workflows/docs-canary-qa.yml +++ b/.github/workflows/docs-canary-qa.yml @@ -11,6 +11,15 @@ jobs: steps: - uses: actions/checkout@v4 - name: Per-entry sweep (one page + PDF per versions.json entry, §3.4 gate c) + env: + # CF Access service token goes through the environment, NEVER through argv: a secret + # on the command line is readable from the process table for the life of the process + # and is captured verbatim by `set -x` traces and crash dumps. These environment + # variables are the ONLY supported way to pass the token: the --access-id / + # --access-secret flags were removed, and argparse now rejects them outright + # rather than ignoring them and leaving every probe to 403. + CF_ACCESS_CLIENT_ID: ${{ secrets.CF_ACCESS_CLIENT_ID }} + CF_ACCESS_CLIENT_SECRET: ${{ secrets.CF_ACCESS_CLIENT_SECRET }} run: | set -eu # Capture-first (not pipe/process-substitution-into-while): a malformed @@ -23,9 +32,7 @@ jobs: while read -r v; do slug=$(echo "$v" | jq -r .slug); pdf=$(echo "$v" | jq -r '.pdf // empty') if ! python -m scripts.docs_gates.smoke --host docs-next.vyos.io --slug "$slug" \ - --expect-sha SKIP --access-id '${{ secrets.CF_ACCESS_CLIENT_ID }}' \ - --access-secret '${{ secrets.CF_ACCESS_CLIENT_SECRET }}' \ - ${pdf:+--pdf "$pdf"}; then + --expect-sha SKIP ${pdf:+--pdf "$pdf"}; then echo "sweep-fail=$slug" | tee -a "$GITHUB_STEP_SUMMARY" fail=1 fi @@ -33,15 +40,17 @@ jobs: exit $fail # a broken advertised version MUST fail nightly QA (§3.4 gate c) # --expect-sha SKIP = presence-only assert; implemented in smoke.docs_build_ok (Task 3.2). - name: URL parity vs RTD (pre-cutover truth) + env: + # Same argv-secret rule as the sweep step above; parity.py reads these by default. + CF_ACCESS_CLIENT_ID: ${{ secrets.CF_ACCESS_CLIENT_ID }} + CF_ACCESS_CLIENT_SECRET: ${{ secrets.CF_ACCESS_CLIENT_SECRET }} run: | # --slugs pinned to CF-built versions: 1.3/1.2 have no RTD sitemaps (§15a.5); # legacy parity is the snapshot repo's crawl-inventory job (defense in depth # alongside the matching parity.py DEFAULT_SLUGS) python -m scripts.docs_gates.parity \ --sitemap-host docs.vyos.io --probe-host docs-next.vyos.io \ - --slugs rolling,1.5,1.4 \ - --access-id '${{ secrets.CF_ACCESS_CLIENT_ID }}' \ - --access-secret '${{ secrets.CF_ACCESS_CLIENT_SECRET }}' + --slugs rolling,1.5,1.4 - uses: actions/upload-artifact@v4 if: always() with: { name: parity-report, path: parity-report.json } diff --git a/scripts/docs_gates/gates.py b/scripts/docs_gates/gates.py index 73f4b23e..7b5cbf1d 100644 --- a/scripts/docs_gates/gates.py +++ b/scripts/docs_gates/gates.py @@ -84,8 +84,11 @@ def main() -> int: ap.add_argument("--critical-list", type=Path, default=Path("scripts/docs_gates/critical-pages.txt")) a = ap.parse_args() - critical = [line.strip() for line in a.critical_list.read_text().splitlines() - if line.strip() and not line.startswith("#")] + # Strip BEFORE the comment test: an indented " # note" line is a comment, not a page + # every deployable build must contain — the unstripped test turned it into a live entry, + # and a comment can never exist as a file, so it would block the deploy as a missing page. + lines = (line.strip() for line in a.critical_list.read_text().splitlines()) + critical = [line for line in lines if line and not line.startswith("#")] return run(a.artifact, a.slug, a.versions, a.previous_meta, critical) diff --git a/scripts/docs_gates/parity.py b/scripts/docs_gates/parity.py index 08225f5a..c30536db 100644 --- a/scripts/docs_gates/parity.py +++ b/scripts/docs_gates/parity.py @@ -9,9 +9,12 @@ Location for alias rows). from __future__ import annotations import argparse +import dataclasses import json +import os import re import sys +import urllib.parse import urllib.request from pathlib import Path @@ -58,11 +61,107 @@ _OPENER = urllib.request.build_opener(_NoRedirect) _SCHEME = "https" -def fetch(host: str, path: str, access: tuple[str, str] | None, method: str = "HEAD"): - req = urllib.request.Request(f"{_SCHEME}://{host}{path}", method=method) - if access: - req.add_header("CF-Access-Client-Id", access[0]) - req.add_header("CF-Access-Client-Secret", access[1]) +# The port each scheme already implies, so `host` and `host:443` are not two origins. +_DEFAULT_PORTS = {"https": 443, "http": 80} + + +def _authority(value: str) -> tuple[str, str | None, int | None]: + """The normalized ORIGIN of a full URL or of a bare `host[:port]` argument. + + An origin is scheme + host + port, and all three are returned: a credential scoped to an + https host must not match a plaintext http URL. Dropping the scheme made + `Access("p.invalid", ...)` apply to `http://p.invalid/`, so the ONE choke point that + decides whether to attach the service token would have attached it to a cleartext + request. Nothing constructs such a URL today — every URL in this module is built from + _SCHEME, so a single run is single-scheme — but the scope of a credential should not + depend on that staying true. + + Two spellings of one origin still have to compare equal, because the two sides of this + comparison come from different places: one is a URL this module built, the other is + whatever an operator typed after --probe-host. Comparing (hostname, port) verbatim made + `p.invalid` and `p.invalid:443` distinct, so spelling the default port out cost the + credential its own scope — post-cutover, where --sitemap-host and --probe-host name the + same Access-gated host, that silently 403'd every sitemap fetch and the sweep then + reported an empty corpus as a pass. Normalized here: + + * case — `scheme` and `hostname` are already lowercased by urlsplit; kept explicit for + the reader. + * the root label's trailing dot — `p.invalid.` names the same host as `p.invalid`. + * the scheme's default port → None, so `:443` under https (or `:80` under http) is not + a separate authority. Folded against the origin's OWN scheme, so http `:80` and + https `:443` stay the distinct origins they are. + * a bare argument carries no scheme, so it is read under _SCHEME — the scheme every + URL in this module is built with. + + Deliberately NOT normalized: IDN/punycode equivalence (`ünïcode.example` against its + `xn--` form). Both hosts here are ASCII literals passed by CI, idna encoding carries its + own failure modes, and the safe direction for a credential-scoping test is to leave a + Unicode spelling not matching its punycode one rather than to guess an equivalence. + """ + parts = urllib.parse.urlsplit(value if "://" in value else f"//{value}") + scheme = (parts.scheme or _SCHEME).lower() + host = parts.hostname.lower() if parts.hostname else None + if host and host.endswith("."): + host = host[:-1] + port = parts.port + if port is not None and port == _DEFAULT_PORTS.get(scheme): + port = None + return scheme, host, port + + +@dataclasses.dataclass(frozen=True) +class Access: + """A CF Access service token BOUND TO THE ONE HOST it may be presented to. + + The binding is the point. This run talks to two hosts that are not the same party: + --probe-host is our Access-gated canary, while --sitemap-host is (pre-cutover) + docs.vyos.io, still served by ReadTheDocs. Credentials modelled as a bare + (id, secret) tuple carry no notion of destination, so a single `if access:` test in + the request builder sent our service token to BOTH — handing it to a third party on + every nightly sitemap fetch. Pairing the secret with its host makes the destination + check part of the credential rather than a rule each call site has to remember. + """ + + host: str + client_id: str + # repr=False: the default dataclass repr renders every field, so a failed assertion, a + # debug print or any exception that interpolates an Access would put the service token + # verbatim into CI logs — which are durable and, for this repo, world-readable. The id + # stays: it names WHICH token without being the credential, and losing it would make a + # scoping failure much harder to read. Secret is fetched via the attribute, never shown. + client_secret: str = dataclasses.field(repr=False) + + def applies_to(self, url: str) -> bool: + """True only for a URL whose ORIGIN is this credential's host (see _authority).""" + return _authority(url) == _authority(self.host) + + +def build_request(url: str, access: Access | None, + method: str = "HEAD") -> urllib.request.Request: + """The ONE place that attaches CF Access credentials to a request. + + Every outbound request in this module goes through here, and the attach decision is + made PER DESTINATION, never per run. Two failure modes meet at this function and only + a host-scoped single choke point closes both: + + * Credential leak. The sitemap host and the probe host are different parties + pre-cutover; an unscoped `if access:` mailed our service token to ReadTheDocs + once a night. `Access.applies_to()` makes that structurally impossible. + * Split-brain. The sitemap fetch used to build its own bare Request, so pointing + --sitemap-host at the Access-gated canary 403'd every sitemap while the probe + requests worked. Post-cutover both flags name the same host, and because the + scoping test is on the URL rather than on which caller asked, that configuration + still gets credentialed sitemap fetches with no extra wiring. + """ + req = urllib.request.Request(url, method=method) + if access is not None and access.applies_to(url): + req.add_header("CF-Access-Client-Id", access.client_id) + req.add_header("CF-Access-Client-Secret", access.client_secret) + return req + + +def fetch(host: str, path: str, access: Access | None, method: str = "HEAD"): + req = build_request(f"{_SCHEME}://{host}{path}", access, method) try: with _OPENER.open(req, timeout=30) as r: return r.status, r.headers.get("Location") @@ -77,22 +176,55 @@ def main() -> int: ap.add_argument("--sitemap-host", required=True) ap.add_argument("--probe-host", required=True) ap.add_argument("--slugs", default=DEFAULT_SLUGS) - ap.add_argument("--access-id") - ap.add_argument("--access-secret") ap.add_argument("--report", type=Path, default=Path("parity-report.json")) a = ap.parse_args() - access = (a.access_id, a.access_secret) if a.access_id else None + # CF Access service-token credentials are read ONLY from the environment. They were + # also accepted as --access-id/--access-secret flags; that is removed rather than + # merely discouraged, because a value passed in argv is readable from the process table + # for the lifetime of the process and is captured verbatim by `set -x` traces, crash + # dumps and CI process listings. No call site used the flags (both workflows export the + # env vars), so there is nothing to migrate and no ergonomic loss worth the exposure. + # Access itself stays OPTIONAL: the sitemap host may be a public origin needing no token. + access_id = os.environ.get("CF_ACCESS_CLIENT_ID", "") + access_secret = os.environ.get("CF_ACCESS_CLIENT_SECRET", "") + if bool(access_id) != bool(access_secret): + # Half a service token is never usable — every probe would 403 and the run would + # report a wholly misleading "parity broken". Names only, never the values. + print("CF Access needs BOTH an id and a secret, or neither " + "(CF_ACCESS_CLIENT_ID, CF_ACCESS_CLIENT_SECRET)", file=sys.stderr) + return 2 + # Bound to the PROBE host, and to nothing else. --probe-host is the host we own and + # gate with Access; --sitemap-host is whatever currently publishes the truth sitemaps, + # which pre-cutover is ReadTheDocs. Should the two flags name the same host — the + # post-cutover configuration — build_request() credentials the sitemap fetch too, + # because the test is on the destination and not on the call site. + access = Access(a.probe_host, access_id, access_secret) if access_id else None failures: list[dict] = [] checked = 0 for slug in a.slugs.split(","): - status, _ = fetch(a.sitemap_host, f"/en/{slug}/sitemap.xml", None, "GET") - if status != 200: - failures.append({"path": f"/en/{slug}/sitemap.xml", "reason": f"sitemap {status}"}) - continue + # ONE request per sitemap. This used to probe the status with fetch() and then fetch + # the whole document a second time — two full GETs of a multi-thousand-URL sitemap per + # slug — and the body fetch hard-coded "https://", so the _SCHEME override (the hook + # the tests use to drive this path against a local plain-HTTP server) was ignored. + # Two things the single-call rewrite must NOT lose: + # 1. The discarded pre-check asserted status == 200 exactly. _OPENER raises + # HTTPError for non-2xx (3xx included — it refuses to follow redirects), but it + # RETURNS normally for any other 2xx, so a sitemap answering 204/206 would yield + # an empty corpus and the gate would pass having probed nothing. The explicit + # status check below restores that strictness. + # 2. CF Access credentials WHEN — and only when — the sitemap host is the host the + # token belongs to. A bare Request here 403'd a --sitemap-host pointed at the + # Access-gated canary; an unconditionally credentialed one posted the token to + # ReadTheDocs. build_request() decides per destination and settles both. try: - with urllib.request.urlopen(f"https://{a.sitemap_host}/en/{slug}/sitemap.xml", - timeout=30) as r: + with _OPENER.open(build_request( + f"{_SCHEME}://{a.sitemap_host}/en/{slug}/sitemap.xml", access, "GET"), + timeout=30) as r: + if r.status != 200: + failures.append({"path": f"/en/{slug}/sitemap.xml", + "reason": f"sitemap status {r.status}"}) + continue urls = urls_from_sitemap(r.read().decode()) except Exception as e: # noqa: BLE001 — record per-slug, keep sweeping; report ALWAYS written failures.append({"path": f"/en/{slug}/sitemap.xml", diff --git a/scripts/docs_gates/smoke.py b/scripts/docs_gates/smoke.py index 770e93e3..6c526d98 100644 --- a/scripts/docs_gates/smoke.py +++ b/scripts/docs_gates/smoke.py @@ -15,9 +15,11 @@ from __future__ import annotations import argparse import dataclasses import json +import os import sys import time import urllib.request +from pathlib import Path APEX_PATHS = ["/versions.json", "/healthz", "/robots.txt", "/sitemap.xml"] SEARCH_MOUNT_MARKER = 'id="vyos-search"' @@ -73,7 +75,14 @@ def probe_plan(slug: str, pdf: str | None, critical: list[str]) -> list[Probe]: plan = [Probe(f"/en/{slug}/{rel}", 200, True, False) for rel in ["index.html", *critical]] plan.append(Probe(f"/en/{slug}/pagefind/pagefind.js", 200, True, False)) if pdf: - plan.append(Probe(pdf, 200, True, False)) + # assert_docs_build=False: the PDF is the ONE content path that can legitimately be + # answered by the apex Worker instead of a branch content Worker. 1.3's PDF (29.2 MiB) + # exceeds the 25 MiB static-asset cap, so apex serves it straight from R2 (spec §5) and + # that response carries only etag / accept-ranges / content-type / content-length — + # X-Docs-Build is a content-Worker header apex never sets on it. Asserting it made the + # probe structurally unpassable for 1.3 (observed nightly: "detail=status+docs-build"). + # The build SHA is still gated for this version: every HTML probe above asserts it. + plan.append(Probe(pdf, 200, False, False)) plan.append(Probe(f"/en/{slug}/definitely-missing-page-xyz.html", 404, False, False)) plan += [Probe(p, 200, False, True) for p in APEX_PATHS] plan[0].assert_search_mount = True # plan[0] is always /en/<slug>/index.html @@ -192,14 +201,35 @@ def main() -> int: ap.add_argument("--host", required=True) ap.add_argument("--slug", required=True) ap.add_argument("--expect-sha", required=True) - ap.add_argument("--access-id", required=True) - ap.add_argument("--access-secret", required=True) ap.add_argument("--pdf", default=None) - ap.add_argument("--critical-list", default="scripts/docs_gates/critical-pages.txt") + ap.add_argument("--critical-list", type=Path, + default=Path("scripts/docs_gates/critical-pages.txt")) a = ap.parse_args() - critical = [line.strip() for line in open(a.critical_list).read().splitlines() - if line.strip() and not line.startswith("#")] - return run(a.host, a.slug, a.expect_sha, a.access_id, a.access_secret, a.pdf, critical) + # CF Access service-token credentials are read ONLY from the environment. They were also + # accepted as --access-id/--access-secret flags; that is removed rather than merely + # discouraged, because a value passed in argv publishes it in the process command line — + # readable from the process table for the lifetime of the process, and captured verbatim + # by `set -x` shell traces, crash dumps and process-listing tooling. No call site used + # the flags (docs-build.yml and docs-canary-qa.yml both export the env vars), so there is + # nothing to migrate. Neither value nor its length is ever echoed. + access_id = os.environ.get("CF_ACCESS_CLIENT_ID", "") + access_secret = os.environ.get("CF_ACCESS_CLIENT_SECRET", "") + missing = [name for name, value in ( + ("CF_ACCESS_CLIENT_ID", access_id), + ("CF_ACCESS_CLIENT_SECRET", access_secret)) if not value] + if missing: + # The canary host is Access-gated, so an empty credential would turn every probe into + # an indistinguishable 403 — fail loudly on the cause instead. Names only, no values. + print(f"missing CF Access credentials: {', '.join(missing)}", file=sys.stderr) + return 2 + # Strip BEFORE the comment test: an indented " # note" line is a comment, not a page that + # every deployable build must contain (it would fail the probe as a missing critical page). + # read_text() (rather than a bare open().read()) closes the handle deterministically, + # matching gates.py; the bare form leaked the descriptor until GC on any interpreter + # without CPython's refcounting. + lines = (line.strip() for line in a.critical_list.read_text().splitlines()) + critical = [line for line in lines if line and not line.startswith("#")] + return run(a.host, a.slug, a.expect_sha, access_id, access_secret, a.pdf, critical) if __name__ == "__main__": diff --git a/scripts/docs_gates/test_gates.py b/scripts/docs_gates/test_gates.py index a30837ea..e65be3af 100644 --- a/scripts/docs_gates/test_gates.py +++ b/scripts/docs_gates/test_gates.py @@ -1,5 +1,7 @@ import json +import sys from pathlib import Path + import pytest from scripts.docs_gates import gates @@ -102,3 +104,19 @@ def test_fail_when_declared_pdf_missing(artifact: Path, versions: Path): rc = gates.run(artifact=artifact, slug="rolling", versions=versions, previous_meta=None, critical=["index.html"]) assert rc == 1 + + +def test_critical_list_strips_before_testing_for_comments(monkeypatch, tmp_path, artifact): + # An INDENTED comment used to survive the `line.startswith("#")` test (applied to the + # UNSTRIPPED line) and become a live critical-page entry. A comment can never exist as a + # file, so it would block every deploy with "critical page missing: en/rolling/ # ...". + crit = tmp_path / "critical.txt" + crit.write_text("# leading comment\n # indented comment\n\n index.html \n") + seen: list[str] = [] + monkeypatch.setattr(gates, "run", + lambda art, slug, versions, prev, critical: seen.extend(critical) or 0) + monkeypatch.setattr(sys, "argv", [ + "gates", "--artifact", str(artifact), "--slug", "rolling", + "--versions", str(versions_arg(tmp_path)), "--critical-list", str(crit)]) + assert gates.main() == 0 + assert seen == ["index.html"] diff --git a/scripts/docs_gates/test_parity.py b/scripts/docs_gates/test_parity.py index 76057d70..053eb79f 100644 --- a/scripts/docs_gates/test_parity.py +++ b/scripts/docs_gates/test_parity.py @@ -1,6 +1,9 @@ import json import sys import urllib.error +import urllib.request + +import pytest from scripts.docs_gates import parity from scripts.docs_gates.conftest import REDIRECT_LOCATION, REDIRECT_PATH @@ -61,7 +64,7 @@ def test_main_always_writes_report_on_transport_errors(tmp_path, monkeypatch): def _boom(*a, **k): raise urllib.error.URLError("timed out") - monkeypatch.setattr(parity.urllib.request, "urlopen", _boom) + monkeypatch.setattr(parity._OPENER, "open", _boom) monkeypatch.setattr(sys, "argv", ["parity", "--sitemap-host", "sitemap.invalid", "--probe-host", "probe.invalid", "--report", str(report)]) @@ -70,3 +73,268 @@ def test_main_always_writes_report_on_transport_errors(tmp_path, monkeypatch): data = json.loads(report.read_text()) assert data["failures"] # report written despite transport errors assert any("sitemap" in f["reason"] for f in data["failures"]) + + +# --- CF Access credentials come from the ENVIRONMENT ONLY. The --access-id/--access-secret +# flags were REMOVED: an argv-passed secret is readable from the process table and captured +# by `set -x` traces. Access stays OPTIONAL here — the sitemap host may be public — but HALF +# a service token is never usable, so an id/secret mismatch is rejected outright. --- + +def _parity_argv(monkeypatch, tmp_path, *extra): + monkeypatch.setattr(sys, "argv", ["parity", "--sitemap-host", "s.invalid", + "--probe-host", "p.invalid", "--slugs", "rolling", + "--report", str(tmp_path / "r.json"), *extra]) + + +def test_access_credentials_default_from_environment(monkeypatch, tmp_path): + _parity_argv(monkeypatch, tmp_path) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + seen: list[parity.Access | None] = [] + + def _probe(host, path, access, method="HEAD"): + seen.append(access) + return 200, None + + monkeypatch.setattr(parity, "fetch", _probe) + monkeypatch.setattr(parity._OPENER, "open", + lambda *a, **k: _sitemap_response("<urlset></urlset>")) + parity.main() + # scoped to --probe-host, which is the only host the token may ever be presented to + assert parity.Access("p.invalid", "env-id", "env-secret") in seen + + +def test_half_a_service_token_is_rejected(monkeypatch, tmp_path, capsys): + _parity_argv(monkeypatch, tmp_path) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "only-an-id") + monkeypatch.delenv("CF_ACCESS_CLIENT_SECRET", raising=False) + monkeypatch.setattr(parity, "fetch", lambda *a, **k: (_ for _ in ()).throw( + AssertionError("must not probe with half a token"))) + assert parity.main() == 2 + assert "CF_ACCESS_CLIENT_SECRET" in capsys.readouterr().err + + +def test_secret_bearing_flags_are_rejected_not_silently_ignored(monkeypatch, tmp_path): + # The flags are GONE, not deprecated. argparse must reject them outright so an operator + # reaching for the old muscle-memory invocation gets an error instead of a run that + # silently ignores the credential they passed and then 403s on every probe. + for flag, value in (("--access-id", "an-id"), ("--access-secret", "a-secret")): + _parity_argv(monkeypatch, tmp_path, flag, value) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + with pytest.raises(SystemExit) as exc: + parity.main() + assert exc.value.code == 2 + + +# --- The sitemap used to be fetched TWICE per slug (a status probe via fetch(), then the +# body via a second GET) and the body fetch hard-coded "https://", ignoring _SCHEME. --- + +class _CountingSitemap: + """Records the Request objects the opener is handed, so a test can assert both the URL + (once per slug, honouring _SCHEME) and the CF Access headers actually attached to it.""" + + def __init__(self, status: int = 200) -> None: + self.requests: list[urllib.request.Request] = [] + self.status = status + + @property + def urls(self) -> list[str]: + return [r.full_url for r in self.requests] + + def __call__(self, req, *a, **k): + self.requests.append(req) + return _sitemap_response( + '<urlset><url><loc>http://h/en/rolling/a.html</loc></url></urlset>', + status=self.status) + + +def _sitemap_response(body: str, status: int = 200): + class _R: + def __init__(self): + self.status = status + + def read(self): + return body.encode() + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + return _R() + + +def test_sitemap_fetched_once_per_slug_and_honours_the_scheme_override(monkeypatch, tmp_path): + _parity_argv(monkeypatch, tmp_path) + monkeypatch.delenv("CF_ACCESS_CLIENT_ID", raising=False) + monkeypatch.delenv("CF_ACCESS_CLIENT_SECRET", raising=False) + monkeypatch.setattr(parity, "_SCHEME", "http") + counter = _CountingSitemap() + monkeypatch.setattr(parity._OPENER, "open", counter) + monkeypatch.setattr(parity, "fetch", lambda *a, **k: (200, None)) + parity.main() + assert counter.urls == ["http://s.invalid/en/rolling/sitemap.xml"] # once, and NOT https + + +_ACCESS_HEADERS = ("Cf-access-client-id", "Cf-access-client-secret") # urllib capitalises + + +def test_sitemap_host_that_is_not_the_probe_host_gets_NO_access_headers(monkeypatch, tmp_path): + # THE credential-scoping assertion, and the inverse of what this test used to demand. + # Pre-cutover the two flags name different parties: --sitemap-host is docs.vyos.io, + # still served by ReadTheDocs, while --probe-host is our Access-gated canary. Crediting + # every outbound request "because the run holds a token" handed our CF Access service + # token to a host we do not control, once every night. + _parity_argv(monkeypatch, tmp_path) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + counter = _CountingSitemap() + monkeypatch.setattr(parity._OPENER, "open", counter) + monkeypatch.setattr(parity, "fetch", lambda *a, **k: (200, None)) + parity.main() + assert len(counter.requests) == 1 + req = counter.requests[0] + assert req.full_url.startswith("https://s.invalid/") # the third-party host + for header in _ACCESS_HEADERS: + assert req.get_header(header) is None + + +def test_sitemap_host_equal_to_the_probe_host_IS_credentialed(monkeypatch, tmp_path): + # The other direction, and the reason the scoping lives inside build_request() rather + # than at each call site: post-cutover both flags name the same Access-gated host and + # that sitemap fetch must still carry the token. A bare Request here (the shape before + # round 2) 403'd every sitemap, and the sweep then reported an empty corpus as a pass. + monkeypatch.setattr(sys, "argv", ["parity", "--sitemap-host", "p.invalid", + "--probe-host", "p.invalid", "--slugs", "rolling", + "--report", str(tmp_path / "r.json")]) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + counter = _CountingSitemap() + monkeypatch.setattr(parity._OPENER, "open", counter) + monkeypatch.setattr(parity, "fetch", lambda *a, **k: (200, None)) + parity.main() + req = counter.requests[0] + assert req.get_header("Cf-access-client-id") == "env-id" + assert req.get_header("Cf-access-client-secret") == "env-secret" + + +def test_build_request_attaches_the_token_to_its_own_host_and_to_nothing_else(): + # build_request() in isolation: one Access object, many destinations. + access = parity.Access("p.invalid", "an-id", "a-secret") + own = parity.build_request("https://P.Invalid/en/rolling/", access) # case-insensitive + assert own.get_header("Cf-access-client-id") == "an-id" + assert own.get_header("Cf-access-client-secret") == "a-secret" + for other in ("https://s.invalid/en/rolling/", # a different host entirely + "https://p.invalid.evil.example/en/", # suffix-extended lookalike + "https://notp.invalid/en/", # prefix-extended lookalike + "https://p.invalid:8443/en/rolling/"): # same name, different authority + for header in _ACCESS_HEADERS: + assert parity.build_request(other, access).get_header(header) is None + for header in _ACCESS_HEADERS: # no token configured at all + assert parity.build_request("https://p.invalid/", None).get_header(header) is None + + +# --- The scoping test compares ORIGINS, not spellings. `p.invalid` and `p.invalid:443` are +# the same HTTPS origin, and so is the trailing-dot FQDN form; comparing (hostname, port) +# verbatim made all three distinct. The case that matters is post-cutover, where BOTH flags +# name the same host: write either one with an explicit `:443` and the sitemap fetch silently +# lost its token and 403'd — reverting the keeper case two tests up. --- + +def test_equivalent_spellings_of_one_origin_all_get_the_token(): + for host, url in (("p.invalid", "https://p.invalid:443/en/rolling/"), # default port explicit + ("p.invalid:443", "https://p.invalid/en/rolling/"), # ...and the reverse + ("p.invalid:443", "https://p.invalid:443/en/"), # explicit on both + ("p.invalid.", "https://p.invalid/en/rolling/"), # trailing-dot FQDN + ("p.invalid", "https://p.invalid./en/rolling/"), # ...and the reverse + ("P.INVALID.:443", "https://p.invalid/en/")): # every axis at once + req = parity.build_request(url, parity.Access(host, "an-id", "a-secret")) + assert req.get_header("Cf-access-client-id") == "an-id", f"{host} vs {url}" + assert req.get_header("Cf-access-client-secret") == "a-secret", f"{host} vs {url}" + + +def test_normalization_does_not_widen_the_scope_to_a_different_origin(): + # The inverse pin: normalizing the default port and the trailing dot must not smear the + # comparison into matching anything else. A non-default port stays a distinct origin in + # BOTH directions, and a trailing dot on a lookalike is still a lookalike. + for host, url in (("p.invalid", "https://s.invalid:443/en/"), # different host, :443 + ("p.invalid:8443", "https://p.invalid/en/"), # non-default on the cred + ("p.invalid", "https://p.invalid:8443/en/"), # non-default on the URL + ("p.invalid.", "https://p.invalid.evil.example./en/")): # dotted lookalike + for header in _ACCESS_HEADERS: + req = parity.build_request(url, parity.Access(host, "an-id", "a-secret")) + assert req.get_header(header) is None, f"{host} vs {url}" + + +def test_the_default_port_that_normalizes_is_the_one_for_the_scheme_in_use(monkeypatch): + # A bare `host[:port]` argument carries no scheme, so the default it is compared against + # is the scheme every URL in this module is built with (_SCHEME) — not a hard-coded 443. + # Under the http override the tests use, 80 is the default and 443 is a real distinct port. + monkeypatch.setattr(parity, "_SCHEME", "http") + token = parity.Access("p.invalid:80", "an-id", "a-secret") + assert parity.build_request("http://p.invalid/en/", token).get_header( + "Cf-access-client-id") == "an-id" + assert parity.build_request("http://p.invalid:443/en/", token).get_header( + "Cf-access-client-id") is None + + +def test_probe_host_written_with_an_explicit_port_still_credentials_its_own_sitemap( + monkeypatch, tmp_path): + # The end-to-end shape of the bug: post-cutover both flags name the same host, but one + # of them spells the default port out. Before origin normalization the sitemap request + # went out bare, 403'd behind Access, and the sweep reported an empty corpus as a pass. + monkeypatch.setattr(sys, "argv", ["parity", "--sitemap-host", "p.invalid", + "--probe-host", "p.invalid:443", "--slugs", "rolling", + "--report", str(tmp_path / "r.json")]) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + counter = _CountingSitemap() + monkeypatch.setattr(parity._OPENER, "open", counter) + monkeypatch.setattr(parity, "fetch", lambda *a, **k: (200, None)) + parity.main() + req = counter.requests[0] + assert req.get_header("Cf-access-client-id") == "env-id" + assert req.get_header("Cf-access-client-secret") == "env-secret" + + +def test_a_plaintext_http_url_never_gets_an_https_scoped_token(): + # An ORIGIN is scheme + host + port. Comparing only (host, port) left the transport out + # of the credential's scope, so a token bound to an https host also applied to the + # cleartext http URL of the same name — the choke point would have attached the service + # token to a request that puts it on the wire in plaintext. `http://p.invalid:80/` is the + # sharp case: 80 folds to None under http, so the authority-only comparison matched the + # https-scoped ("p.invalid", None) exactly. + access = parity.Access("p.invalid", "an-id", "a-secret") # bare host → _SCHEME (https) + for url in ("http://p.invalid/en/rolling/", "http://p.invalid:80/en/rolling/"): + for header in _ACCESS_HEADERS: + assert parity.build_request(url, access).get_header(header) is None, url + # control, same test: its own scheme still gets the token + assert parity.build_request("https://p.invalid/en/rolling/", access).get_header( + "Cf-access-client-id") == "an-id" + + +def test_the_service_token_is_not_rendered_by_repr(): + # The default dataclass repr renders every field. A failed assertion, a debug print or an + # exception that interpolates an Access would then put the token into CI output, which is + # durable. str() delegates to __repr__, so it covers f-string interpolation too. + access = parity.Access("p.invalid", "an-id", "sekrit-must-not-be-rendered") + for rendered in (repr(access), str(access), f"{access}"): + assert "sekrit-must-not-be-rendered" not in rendered + assert access.client_secret == "sekrit-must-not-be-rendered" # still readable as a field + assert "an-id" in repr(access) # the id is NOT the credential; keep it for diagnosis + + +def test_non_200_sitemap_is_a_failure_not_an_empty_corpus(monkeypatch, tmp_path): + # _OPENER only raises for non-2xx. A sitemap answering 204 (or any other 2xx) returned + # normally with an empty/irrelevant body, so the corpus came back empty and the parity + # gate PASSED having probed nothing at all — the exact silent-degrade the discarded + # exact-200 pre-check existed to prevent. + _parity_argv(monkeypatch, tmp_path) + monkeypatch.delenv("CF_ACCESS_CLIENT_ID", raising=False) + monkeypatch.delenv("CF_ACCESS_CLIENT_SECRET", raising=False) + report = tmp_path / "r.json" + monkeypatch.setattr(parity._OPENER, "open", _CountingSitemap(status=204)) + monkeypatch.setattr(parity, "fetch", lambda *a, **k: (200, None)) + assert parity.main() == 1 + data = json.loads(report.read_text()) + assert any(f["reason"] == "sitemap status 204" for f in data["failures"]) diff --git a/scripts/docs_gates/test_smoke.py b/scripts/docs_gates/test_smoke.py index a2654dcf..790f953d 100644 --- a/scripts/docs_gates/test_smoke.py +++ b/scripts/docs_gates/test_smoke.py @@ -1,11 +1,14 @@ from __future__ import annotations import io +import sys import urllib.error import urllib.request from email.message import Message from urllib.parse import urlsplit +import pytest + from scripts.docs_gates import smoke from scripts.docs_gates.conftest import REDIRECT_LOCATION, REDIRECT_PATH @@ -364,3 +367,117 @@ def test_inter_round_sleep_capped_to_remaining_budget(monkeypatch): assert len(sleeps) == 1 assert 0 < sleeps[0] <= smoke.DEADLINE_SECONDS # capped to the ~10s remaining budget assert sleeps[0] < smoke.RETRY_SLEEP_SECONDS # NOT the full 30s sleep + + +# --- The PDF probe was structurally unpassable: it asserted X-Docs-Build, but 1.3's PDF is +# served by the APEX Worker straight from R2 (spec §5, 29.2 MiB > the 25 MiB asset cap) and +# that path sets only etag / accept-ranges / content-type / content-length. Observed nightly: +# "/en/1.3/vyos-documentation.pdf: status=206 docs-build=None detail=status+docs-build". --- + +def test_pdf_probe_does_not_assert_docs_build(): + plan = smoke.probe_plan("1.3", pdf="/en/1.3/vyos-documentation.pdf", critical=["index.html"]) + pdf = next(p for p in plan if p.path.endswith(".pdf")) + assert pdf.assert_docs_build is False # apex's R2 path legitimately never sets it + assert pdf.assert_apex_build is False # nor does the content Worker set X-Apex-Build + # ...but the build SHA is still gated for this version, via the HTML probes: + assert next(p for p in plan if p.path.endswith("/index.html")).assert_docs_build is True + + +def test_pdf_probe_still_demands_an_exact_200(): + # The 206 seen alongside the docs-build failure was an apex defect (a 206 answered to a + # request carrying no Range header), fixed in workers/apex/src/index.ts — NOT something + # this gate should learn to tolerate. + plan = smoke.probe_plan("1.3", pdf="/en/1.3/vyos-documentation.pdf", critical=[]) + assert next(p for p in plan if p.path.endswith(".pdf")).expect_status == 200 + + +# --- CF Access credentials: env by default (argv publishes secrets to the process table), +# flags as a manual fallback, and an empty value is rejected rather than sent as a blank +# header (every probe would then 403 and the report would blame the wrong thing). --- + +def _argv(monkeypatch, tmp_path, *extra): + crit = tmp_path / "critical.txt" + crit.write_text("index.html\n") + monkeypatch.setattr(sys, "argv", ["smoke", "--host", "h", "--slug", "rolling", + "--expect-sha", "SKIP", + "--critical-list", str(crit), *extra]) + return crit + + +def test_access_credentials_default_from_environment(monkeypatch, tmp_path): + _argv(monkeypatch, tmp_path) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + seen: dict[str, str] = {} + monkeypatch.setattr(smoke, "run", + lambda host, slug, sha, aid, asec, pdf, critical: + seen.update(id=aid, secret=asec) or 0) + assert smoke.main() == 0 + assert seen == {"id": "env-id", "secret": "env-secret"} + + +def test_secret_bearing_flags_are_rejected_not_silently_ignored(monkeypatch, tmp_path): + # --access-id/--access-secret are GONE, not deprecated: an argv-passed secret is readable + # from the process table and captured verbatim by `set -x` traces. argparse must reject + # them so the old muscle-memory invocation errors out instead of silently ignoring the + # credential the operator passed and then 403ing on every probe. + for flag, value in (("--access-id", "an-id"), ("--access-secret", "a-secret")): + _argv(monkeypatch, tmp_path, flag, value) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "env-id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "env-secret") + monkeypatch.setattr(smoke, "run", lambda *a, **k: pytest.fail("must not probe")) + with pytest.raises(SystemExit) as exc: + smoke.main() + assert exc.value.code == 2 + + +def test_critical_list_is_read_through_a_path_without_resource_warnings(monkeypatch, tmp_path): + # `open(a.critical_list).read()` left the descriptor to be closed by GC; --critical-list + # is now `type=Path` and read via Path.read_text(), which closes deterministically. + # HONEST SCOPE: this is not a strict regression test for the close itself — CPython's + # refcounting also closes the bare-open form immediately, so no ResourceWarning fires + # either way and this test passes against the pre-fix source (verified). What it DOES + # pin is the argparse `type=Path` change (a str would have no .read_text()) plus + # warning-free reading on interpreters without refcounting, e.g. PyPy, where the + # bare-open form genuinely leaks until GC. + import warnings + + crit = _argv(monkeypatch, tmp_path) + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "sec") + seen: list[str] = [] + monkeypatch.setattr(smoke, "run", + lambda host, slug, sha, aid, asec, pdf, critical: + seen.extend(critical) or 0) + with warnings.catch_warnings(): + warnings.simplefilter("error", ResourceWarning) + assert smoke.main() == 0 + assert seen == ["index.html"] + assert crit.exists() + + +def test_missing_access_credentials_fail_loudly_without_probing(monkeypatch, tmp_path, capsys): + _argv(monkeypatch, tmp_path) + monkeypatch.delenv("CF_ACCESS_CLIENT_ID", raising=False) + monkeypatch.delenv("CF_ACCESS_CLIENT_SECRET", raising=False) + monkeypatch.setattr(smoke, "run", lambda *a, **k: pytest.fail("must not probe")) + assert smoke.main() == 2 + err = capsys.readouterr().err + assert "CF_ACCESS_CLIENT_ID" in err and "CF_ACCESS_CLIENT_SECRET" in err + + +def test_critical_list_strips_before_testing_for_comments(monkeypatch, tmp_path): + # An INDENTED comment used to survive the `line.startswith("#")` test (applied to the + # unstripped line) and become a live critical page — which can never exist as a file. + crit = tmp_path / "critical.txt" + crit.write_text("# leading comment\n # indented comment\n\n cli.html \n") + monkeypatch.setenv("CF_ACCESS_CLIENT_ID", "id") + monkeypatch.setenv("CF_ACCESS_CLIENT_SECRET", "sec") + monkeypatch.setattr(sys, "argv", ["smoke", "--host", "h", "--slug", "rolling", + "--expect-sha", "SKIP", "--critical-list", str(crit)]) + seen: list[str] = [] + monkeypatch.setattr(smoke, "run", + lambda host, slug, sha, aid, asec, pdf, critical: + seen.extend(critical) or 0) + assert smoke.main() == 0 + assert seen == ["cli.html"] diff --git a/workers/apex/src/index.ts b/workers/apex/src/index.ts index 278794e2..f97b4bc5 100644 --- a/workers/apex/src/index.ts +++ b/workers/apex/src/index.ts @@ -47,6 +47,263 @@ function apexHeaders(resp: Response, env: ApexEnv, cacheClass: string = DEFAULT_ return out; } +// R2's `R2Range` is a three-shape union — `{offset, length?}`, `{length}` (offset implicitly 0) +// and `{suffix}` (the trailing N bytes) — so `"offset" in range` is NOT a safe way to read it: +// the two offset-less shapes would fall through to the full-object 200 branch and be served +// with a Content-Length claiming the whole object while the body held only a slice. workerd is +// observed to normalize every shape to `{offset, length}` before it reaches us, but the type +// admits the others, so resolve all three to concrete byte bounds, clamped to the object size. +// Exported for direct unit testing. +export function resolveRange( + range: { offset?: number; length?: number; suffix?: number }, + size: number, +): { start: number; length: number } { + if (typeof range.suffix === "number") { + const suffix = Math.min(Math.max(range.suffix, 0), size); // a suffix past the start is the whole object + return { start: size - suffix, length: suffix }; + } + const start = Math.min(Math.max(range.offset ?? 0, 0), size); + // The trailing Math.max(_, 0) keeps the documented "clamped to the object size" contract + // total: without it a negative `range.length` would pass straight through Math.min and + // yield a negative length (and so a negative Content-Length). A real R2 binding cannot + // produce that — see classifyRangeHeader's note on observed R2 behaviour — but this + // function is exported and unit-tested as a standalone utility over the R2Range union, so + // it should not have a documented invariant its own signature can violate. Deliberately + // NOT guarding non-finite inputs: NaN bounds are unreachable from the binding and the + // guard would be untestable-in-anger dead weight. + const length = Math.max(Math.min(range.length ?? size - start, size - start), 0); + return { start, length }; +} + +// A single `bytes=` range-spec. Anything with a comma is a multi-range and deliberately +// fails to match. The whitespace class is `\s`, which is DELIBERATELY wider than the ` ` +// (ASCII space) that R2's own parser accepts — see classifyRangeHeader's contract note on +// why the two grammars are allowed to disagree. +const SINGLE_BYTE_RANGE = /^\s*bytes\s*=\s*(\d*)\s*-\s*(\d*)\s*$/i; + +/** + * Numeric comparison of two non-empty digit strings, without going through Number(). + * + * A range-spec's positions are unbounded digit strings, and Number() silently rounds + * anything above 2^53: `Number("9007199254740993") === Number("9007199254740992")`, which + * collapsed `bytes=9007199254740993-9007199254740992` — an invalid spec (last < first) + * that §14.1.2 says to IGNORE, so 200 — into an apparently-valid one that then read as + * unsatisfiable and answered 416. Comparing normalised digit strings by length and then + * lexically is exact at every magnitude. + */ +function cmpDigits(a: string, b: string): number { + const x = a.replace(/^0+(?=\d)/, ""); + const y = b.replace(/^0+(?=\d)/, ""); + if (x.length !== y.length) return x.length - y.length; + return x < y ? -1 : x > y ? 1 : 0; +} + +export type RangeIntent = + | { kind: "ignored" } + | { kind: "unsatisfiable" } + | { kind: "single"; start: number; length: number }; + +/** + * What the client's Range header ASKS FOR, judged against the representation length. + * + * This exists because R2 does not tell us. Probed against a real R2 binding under + * vitest-pool-workers, `get(key, {range: <Headers>})` signals "I ignored your Range" by + * returning the WHOLE object with `range = {offset: 0, length: size}` — the byte-for-byte + * same shape it returns for a legitimately-satisfied whole-object range like `bytes=0-`. + * It does this for every unsatisfiable spec (`bytes=10-` / `bytes=99-` / `bytes=-0` on a + * 10-byte object), every malformed one (`bytes=abc`, `bytes=-`, `bytes=5-2`), multi-ranges + * (`bytes=0-1,4-5`) and unknown units (`items=0-5`). It does NOT throw for any of them and + * it never returns a zero/negative length except for a genuinely zero-length object. + * (The object-literal form `get(key, {range: {offset: 99}})` DOES throw + * "The requested range is not satisfiable (10039)" — but this Worker passes Headers, so + * that path is unreachable here.) + * + * Trusting `obj.range` alone therefore answered `Range: bytes=99-` with + * `206 + Content-Range: bytes 0-9/10` and the FULL body — a 206 that does not correspond to + * the request (RFC 9110 §15.3.7). That is actively dangerous for the resuming downloader + * this range forwarding exists to serve: a client resuming at byte 99 would append bytes + * 0-9 to its partial file and silently corrupt it. Re-deriving intent from the client's own + * header is the only way to separate the three cases. + * + * Satisfiability follows RFC 9110 §14.1.2 verbatim: an int-range is satisfiable iff + * first-pos < length; a suffix-range iff suffix-length is non-zero (so on a zero-length + * representation, a non-zero suffix-range is the ONLY satisfiable form). An invalid spec + * (last-pos < first-pos) MUST be ignored rather than rejected, hence "ignored", not + * "unsatisfiable". + * + * The `single` verdict carries the CONCRETE byte bounds the client asked for, clamped the + * way §14.1.2 clamps them. That is what makes this classifier safe to disagree with R2's + * parser. The two grammars are not identical and cannot be kept identical: R2's accepts + * only ASCII space around the tokens (miniflare's `/^ *(\d+)? *- *(\d+)? *$/`) while this + * one accepts `\s`, so `Range: bytes=2<TAB>-<TAB>4` parses here and is ignored there. When + * a caller compares these bounds against the bytes R2 actually handed back, any such + * divergence — this one, or the next one a parser change introduces — degrades to a plain + * 200 instead of a 206 whose Content-Range describes a body the client did not ask for. + * Chasing byte-for-byte grammar parity would put the guarantee back in the hands of two + * regexes staying in sync, which is the coupling that produced the bug. + */ +export function classifyRangeHeader(header: string, size: number): RangeIntent { + const m = SINGLE_BYTE_RANGE.exec(header); + if (!m) return { kind: "ignored" }; // multi-range, unknown unit, or unparseable + const [, firstRaw, lastRaw] = m; + if (firstRaw === "") { + if (lastRaw === "") return { kind: "ignored" }; // bare "bytes=-" is malformed + // §14.1.2: suffix-length 0 is unsatisfiable; a suffix past the start is the whole object. + if (cmpDigits(lastRaw, "0") <= 0) return { kind: "unsatisfiable" }; + const suffix = Math.min(Number(lastRaw), size); + return { kind: "single", start: size - suffix, length: suffix }; + } + // §14.1.2: an invalid spec (last-pos < first-pos) is ignored, not rejected. + if (lastRaw !== "" && cmpDigits(lastRaw, firstRaw) < 0) return { kind: "ignored" }; + if (cmpDigits(firstRaw, String(size)) >= 0) return { kind: "unsatisfiable" }; + const first = Number(firstRaw); // < size, so within safe-integer range + const last = lastRaw === "" ? size - 1 : Math.min(Number(lastRaw), size - 1); + return { kind: "single", start: first, length: last - first + 1 }; +} + +/** A quoted entity-tag list (`"a", W/"b"`) or `*`, compared per RFC 9110 §8.8.3.2. */ +function etagListMatches(list: string, etag: string, compare: "strong" | "weak"): boolean { + const items = list.split(",").map((s) => s.trim()).filter((s) => s !== ""); + if (items.includes("*")) return true; // "*" matches iff a representation exists — one does + const weaken = (t: string) => t.replace(/^W\//, ""); + // Strong comparison: neither side may be weak (§8.8.3.2). + if (compare === "strong" && etag.startsWith("W/")) return false; + return items.some((t) => + compare === "strong" ? t === etag : weaken(t) === weaken(etag), + ); +} + +/** + * `uploaded <= <HTTP-date>`, at seconds granularity, or null when the date is unparseable + * (§13.1.3: an invalid date MUST be ignored, which callers map to "precondition passes"). + * Seconds granularity matches R2's own comparison, which the Headers form of `onlyIf` + * selects — evaluating at millisecond precision here would disagree with the binding that + * produced the failure we are trying to name. + */ +function uploadedAtOrBefore(uploaded: Date | undefined, httpDate: string): boolean | null { + const at = Date.parse(httpDate); + if (Number.isNaN(at) || !uploaded) return null; + return Math.floor(uploaded.getTime() / 1000) <= Math.floor(at / 1000); +} + +/** + * The conditional headers R2 is allowed to see, filtered to those RFC 9110 §13.2.2 says + * actually apply to THIS request. + * + * R2 ANDs together every validator it is handed; §13.2.2 instead defines a precedence in + * which a lower-ranked validator is not evaluated at all. Forwarding `request.headers` + * wholesale therefore let R2 fail a request on a validator the RFC says to ignore — most + * visibly `If-Modified-Since` on a non-GET/HEAD method, which §13.2.2 step 4 does not + * evaluate, but which R2 evaluated anyway and answered with a body-less object that this + * Worker could only turn into a 412. Filtering at the source means a body-less result now + * always corresponds to a precondition that genuinely applies. + * + * The METHOD decides this before any header does. §13.2.1: "a server MUST ignore the + * conditional request header fields defined by this specification when received with a + * request method that does not involve the selection or modification of a selected + * representation, such as CONNECT, OPTIONS, or TRACE." Filtering by validator applicability + * alone still handed those methods' conditionals to R2, so an OPTIONS carrying a stale + * `If-Match` a client had left lying around was refused 412 where the same request without + * it succeeded. Returning an empty set here is what "ignore" means at this layer: R2 is + * given nothing to evaluate, so it cannot answer body-less, so preconditionStatus() — which + * is only ever reached from a body-less result — is unreachable for these methods too. + */ +function applicablePreconditions(h: Headers, method: string): Headers { + const out = new Headers(); + if (method === "OPTIONS" || method === "TRACE" || method === "CONNECT") return out; + const isGetOrHead = method === "GET" || method === "HEAD"; + const ifMatch = h.get("if-match"); + const ifNoneMatch = h.get("if-none-match"); + if (ifMatch !== null) out.set("if-match", ifMatch); + else { + const ius = h.get("if-unmodified-since"); // §13.2.2 step 2: only when If-Match is absent + if (ius !== null) out.set("if-unmodified-since", ius); + } + if (ifNoneMatch !== null) out.set("if-none-match", ifNoneMatch); + else if (isGetOrHead) { + const ims = h.get("if-modified-since"); // step 4: only when If-None-Match is absent, GET/HEAD only + if (ims !== null) out.set("if-modified-since", ims); + } + return out; +} + +/** + * Which status a FAILED `onlyIf` owes the client, decided by re-evaluating the request's + * conditionals against the object's own validators in RFC 9110 §13.2.2 order. + * + * R2 reports THAT a precondition failed and never WHICH one. Inferring from header + * PRESENCE cannot be right in both directions, which is how `If-Match: "x"` + + * `If-None-Match: "x"` on a matching object — If-Match satisfied, If-None-Match failed, + * so §13.1.2 owes a 304 — came back 412 purely because an If-Match header was present. + * Evaluating the validators removes the guess: presence selects which check runs, the + * comparison decides the answer. + */ +export function preconditionStatus( + h: Headers, isGetOrHead: boolean, etag: string, uploaded: Date | undefined, +): 304 | 412 { + const ifMatch = h.get("if-match"); + if (ifMatch !== null) { + if (!etagListMatches(ifMatch, etag, "strong")) return 412; // §13.2.2 step 1 + } else { + const ius = h.get("if-unmodified-since"); // step 2 + if (ius !== null && uploadedAtOrBefore(uploaded, ius) === false) return 412; + } + const ifNoneMatch = h.get("if-none-match"); + if (ifNoneMatch !== null) { + // §13.1.2: a failed If-None-Match is 304 for GET/HEAD and 412 for every other method. + if (etagListMatches(ifNoneMatch, etag, "weak")) return isGetOrHead ? 304 : 412; + } else if (isGetOrHead) { + const ims = h.get("if-modified-since"); // step 4 + if (ims !== null && uploadedAtOrBefore(uploaded, ims) === true) return 304; + } + // R2 refused for a reason this evaluation could not reproduce (a validator comparison + // that differs at the margins, say). 412 is the safe answer: a 304 would assert a cache + // validity we have not established. + return 412; +} + +/** + * RFC 9110 §13.1.5 If-Range: does the client's validator still describe this object? + * + * R2 cannot answer this — its `R2Conditional` carries only etagMatches / + * etagDoesNotMatch / uploadedBefore / uploadedAfter, so an `If-Range` in the forwarded + * Headers is silently dropped and the range is applied unconditionally. For an object + * whose ETag has moved on, that answered `Range: bytes=100-` + `If-Range: "old"` with + * bytes 100+ of the NEW representation under a 206 — a resuming downloader then appends + * the new tail to its old prefix and silently corrupts the file, which is the precise + * failure this range forwarding exists to avoid. + * + * §13.1.5 requires a STRONG validator, so a weak entity-tag never matches. The date form + * likewise never matches here: it must compare against Last-Modified, and this Worker + * does not emit one, so no client can hold a date validator for this resource that we + * could honour — treating it as a mismatch (serve the complete representation) is both + * correct and the safe direction. + */ +function ifRangeMatches(value: string, etag: string): boolean { + const v = value.trim(); + if (!v.startsWith('"')) return false; // weak tag or HTTP-date → not a strong match + return etagListMatches(v, etag, "strong"); +} + +/** + * Abandon a body stream this Worker has decided not to send. + * + * R2 hands back a body on paths whose response carries none. An unsatisfiable Range gets + * the COMPLETE object (29.2 MiB for the 1.3 PDF) and is answered 416 with a null body; a + * stale `If-Range` gets the sliced range and is answered from a re-read. Dropping the + * reference leaves the stream open until GC collects it, holding the connection; cancelling + * releases it now and aborts the transfer rather than draining it. Failures are swallowed — + * this is cleanup on a path whose response is already decided, and a stream that is already + * closed or errored is exactly the state we wanted. + */ +async function discardBody(body: ReadableStream | null | undefined): Promise<void> { + try { + await body?.cancel(); + } catch { + /* already closed or errored — nothing left to release */ + } +} + async function themed(env: ApexEnv, status: 404 | 503): Promise<Response> { const page = await env.ASSETS.fetch(new Request(`https://apex.internal/${status}.html`)); return apexHeaders(new Response(page.body, { status, headers: { "content-type": "text/html; charset=utf-8" } }), env); @@ -78,14 +335,24 @@ export default { console.log(JSON.stringify({ event: "binding-missing", binding: "DOCS_PDFS" })); return themed(env, 503); } + const method = request.method.toUpperCase(); + const isGetOrHead = method === "GET" || method === "HEAD"; + // §14.2: "GET is the only method for which range handling is defined" — a Range on + // any other method MUST be ignored. Reading it as null here suppresses the whole + // partial-content path in one place: R2 is never asked to slice, and the 206/416 + // branches below are unreachable. Gating only the R2 forward would leave the + // response side still seeing a Range header and answering a HEAD or a POST with a + // 416 or a Content-Range. + const rangeHeader = method === "GET" ? request.headers.get("range") : null; + const onlyIf = applicablePreconditions(request.headers, method); + let raw: R2ObjectBody | R2Object | null; try { - // Forward Range + conditional (If-None-Match/If-Match/If-Modified-Since) headers - // straight through to R2 so a resumed download or a client with a fresh cached - // copy doesn't have to re-pull the full 29.2 MiB object. + // Forward Range + the APPLICABLE conditionals so a resumed download or a client + // with a fresh cached copy doesn't have to re-pull the full 29.2 MiB object. raw = await bucket.get(pdfVersion.pdf_r2_key!, { - range: request.headers, - onlyIf: request.headers, + ...(rangeHeader !== null ? { range: request.headers } : {}), + onlyIf, }); } catch (e) { console.log(JSON.stringify({ event: "binding-error", binding: "DOCS_PDFS", error: String(e) })); @@ -102,25 +369,160 @@ export default { "accept-ranges": "bytes", }; - // A satisfied onlyIf precondition (e.g. If-None-Match matched the R2 object's current - // ETag) makes R2 hand back a body-less R2Object — just the validators, no content. + // A FAILED onlyIf precondition makes R2 hand back a body-less R2Object — just the + // validators, no content — and never says which validator failed. Because `onlyIf` + // was filtered to the conditionals §13.2.2 actually applies to this request, a + // body-less result here always means a precondition that genuinely applies failed; + // preconditionStatus() re-evaluates them against the object's own validators, in + // §13.2.2 order, to decide between 304 and 412. if (!("body" in raw) || !raw.body) { - return apexHeaders(new Response(null, { status: 304, headers: pdfHeaders }), env, pdfCacheClass); + const status = preconditionStatus( + request.headers, isGetOrHead, raw.httpEtag, raw.uploaded); + return apexHeaders(new Response(null, { status, headers: pdfHeaders }), env, pdfCacheClass); } - const obj = raw as R2ObjectBody; + let obj = raw as R2ObjectBody; pdfHeaders["content-type"] = "application/pdf"; - // A satisfied Range request — R2 echoes the actually-served byte range on `obj.range`; - // its absence means either no Range header was sent or R2 served the full object. - const range = obj.range; - if (range && "offset" in range) { - const start = range.offset ?? 0; - const length = range.length ?? obj.size - start; - pdfHeaders["content-range"] = `bytes ${start}-${start + length - 1}/${obj.size}`; - pdfHeaders["content-length"] = String(length); - return apexHeaders(new Response(obj.body, { status: 206, headers: pdfHeaders }), env, pdfCacheClass); + // §13.1.5 If-Range, which R2 cannot evaluate (see ifRangeMatches). A failed validator + // means the client's partial copy is stale, so the Range is ignored ENTIRELY and the + // complete representation is served — including for a spec that would otherwise be + // unsatisfiable, since the 416 branch below must not fire on a range we have decided + // not to honour. R2 has already applied the range at this point, so the whole object + // has to be re-read; that costs one extra R2 read on the rare stale-resume path and + // nothing at all on the common one. + // + // The re-read carries the SAME `onlyIf`. §13.2.1 requires preconditions to hold for the + // representation ultimately selected, and the two reads need not see one object: a bare + // re-get answered `Range` + stale `If-Range` + `If-Match: "A"` with a 200 carrying + // object B, whose ETag the client had explicitly excluded, whenever the key was + // rewritten in between. That rewrite is not hypothetical — the legacy snapshot repo's + // deploy workflow re-uploads this exact key on its `force_pdf_refresh` input. Re-sending + // the conditionals ties the verdict to the bytes actually served, because R2 evaluates + // `onlyIf` against the very object it returns; the body-less outcome that produces is + // not a gap in this path but the correct answer, resolved by preconditionStatus() + // exactly as on the first read. When no conditionals were sent, `onlyIf` is empty and a + // body-less result cannot occur, so the common path is untouched. + // + // A head() before the get() would also expose the validators, and would avoid opening + // this slice stream at all — but it would put a second round-trip on the path where + // If-Range MATCHES, which is the normal resumed download, in exchange for tidying the + // rare one where it does not. It would also widen the window this shape keeps narrow: + // the object whose validators decide the verdict is the one R2 returns from the same + // call. So the get-then-re-read stays, and the slice we are abandoning is cancelled + // rather than left to GC. + const ifRange = rangeHeader !== null ? request.headers.get("if-range") : null; + let rangeApplies = rangeHeader !== null; + if (ifRange !== null && !ifRangeMatches(ifRange, obj.httpEtag)) { + rangeApplies = false; + await discardBody(obj.body); + let full: R2ObjectBody | R2Object | null; + try { + full = await bucket.get(pdfVersion.pdf_r2_key!, { onlyIf }); + } catch (e) { + console.log(JSON.stringify({ event: "binding-error", binding: "DOCS_PDFS", error: String(e) })); + return themed(env, 503); + } + if (!full) return themed(env, 404); // deleted between the two reads + if (!("body" in full) || !full.body) { + // The key was rewritten between the two reads and the request's preconditions do + // not hold for the new representation. Same treatment as a first-read failure, on + // the new object's validators — never the old ones, which describe a representation + // this response is not about. + const status = preconditionStatus( + request.headers, isGetOrHead, full.httpEtag, full.uploaded); + return apexHeaders(new Response(null, { + status, headers: { etag: full.httpEtag, "accept-ranges": "bytes" }, + }), env, pdfCacheClass); + } + obj = full as R2ObjectBody; + pdfHeaders.etag = obj.httpEtag; } + // A satisfied Range request. R2 echoes the actually-served byte range on `obj.range` — + // but it does so for FULL gets too: against a real R2 binding under workerd, a get() + // whose forwarded Headers carry NO Range header still comes back with + // `range = {offset: 0, length: obj.size}`. Keying the 206 off `obj.range` alone therefore + // turned every plain GET of the 1.3 PDF into a 206 — which is exactly what the nightly + // canary sweep observed (`/en/1.3/vyos-documentation.pdf: status=206`) — and RFC 9110 + // §15.3.7 only permits a 206 in answer to a request that actually carried a Range header. + // So: gate on the REQUEST first, then normalize whatever shape R2 handed back — and + // then CHECK that the two agree before promising a 206, because "R2 sliced it" and + // "R2 handed back everything" are the same shape on the wire. + // + // What R2 actually handed back, as concrete bounds. `obj.range` is absent only if a + // binding declines to report one, in which case the body is the complete object. + const actual = obj.range + ? resolveRange(obj.range, obj.size) + : { start: 0, length: obj.size }; + const servedWhole = actual.start === 0 && actual.length === obj.size; + + if (rangeApplies && rangeHeader !== null) { + const intent = classifyRangeHeader(rangeHeader, obj.size); + if (intent.kind === "unsatisfiable") { + // §14.2: "the server SHOULD send a 416"; §15.5.17: a 416 to a byte-range request + // SHOULD carry `Content-Range: bytes */<complete-length>`. Deliberately no + // content-type — there is no PDF payload on this response. 416 is >= 400 so + // apexHeaders() forces no-store, which is right: the verdict depends on the + // request's Range header and the cache key does not include it. + // R2 answers an unsatisfiable Range with the COMPLETE object, so the body being + // dropped here is the whole 29.2 MiB one — the largest abandoned stream on any + // path through this handler. + await discardBody(obj.body); + return apexHeaders( + new Response(null, { + status: 416, + headers: { etag: pdfHeaders.etag, "accept-ranges": "bytes", + "content-range": `bytes */${obj.size}` }, + }), + env, + pdfCacheClass, + ); + } + // A 206 is owed only when the bytes R2 selected are the bytes the client asked for. + // `length === 0` means a zero-length representation (the only way R2 yields it) — + // e.g. a non-zero suffix-range, which §14.1.2 calls satisfiable, against an empty + // object. No valid Content-Range exists for an empty selection (§14.4 forbids a + // last-pos below the first-pos), so a 206 is unrepresentable. Fall through to the + // 200: §15.5.17's own note records that servers are free to ignore Range and answer + // with the complete representation, which for an empty object is exactly this body. + if (intent.kind === "single" && intent.length > 0 && + actual.start === intent.start && actual.length === intent.length) { + pdfHeaders["content-range"] = + `bytes ${intent.start}-${intent.start + intent.length - 1}/${obj.size}`; + pdfHeaders["content-length"] = String(intent.length); + return apexHeaders(new Response(obj.body, { status: 206, headers: pdfHeaders }), env, pdfCacheClass); + } + // Otherwise no 206 is owed: either the spec was one §14.1.2 says to ignore + // (multi-range / malformed / unknown unit), or R2 declined a spec that this parser + // accepted — the grammar divergence classifyRangeHeader documents. Both leave R2 + // having returned the complete object, so fall through to the 200 below. + } + + // Serve what R2 actually handed back, described truthfully. `servedWhole` is the + // normal case and the only one a 200 can describe; a partial body under a 200 would + // ship a Content-Length that contradicts it. A partial body reaching HERE — past the + // agreement check above — means R2 sliced to bounds the request did not ask for, and + // there is no honest success response left: a 200 would misstate the length, and a + // 206 would answer with a range the client never requested, which §14.4 does not + // permit (Content-Range on a 206 describes the selected range, and no range was + // selected). This used to ship that illegal 206. + // + // So: drop the slice and fail. Re-reading the object for a clean 200 is the other + // option Codex offered, but it means a second conditional read with its own + // object-rewritten-between-reads handling — a copy of the If-Range block above, or a + // refactor of that live and currently-correct path — bought for a branch that cannot + // execute under today's workerd (round 3 established empirically that the Headers + // form ignores multi-range and returns the whole object). The log line is the part + // that earns its keep: it is the alarm that R2's range semantics have moved, and it + // is what would justify writing that re-read for real. + if (!servedWhole) { + console.log(JSON.stringify({ + event: "r2-range-divergence", path: url.pathname, size: obj.size, + served: `${actual.start}+${actual.length}`, requested: rangeHeader ?? null, + })); + await discardBody(obj.body); + return themed(env, 503); + } pdfHeaders["content-length"] = String(obj.size); return apexHeaders(new Response(obj.body, { status: 200, headers: pdfHeaders }), env, pdfCacheClass); } diff --git a/workers/apex/src/special.ts b/workers/apex/src/special.ts index f1a843f0..15e693b1 100644 --- a/workers/apex/src/special.ts +++ b/workers/apex/src/special.ts @@ -26,8 +26,13 @@ export async function specialPathFor( }); if (p === "/sitemap.xml") { + // Origin comes from the REQUEST, never a hard-coded production hostname: this same Worker + // also serves the canary origin (docs-next.vyos.io, DOCS_ENV=canary), and a canary sitemap + // index whose entries pointed at docs.vyos.io would send any checker that follows it + // straight to production — a candidate tree with broken or missing per-version sitemaps + // would then pass its own sitemap check by silently grading production instead of itself. const entries = m.versions - .map((v) => `<sitemap><loc>https://docs.vyos.io/en/${v.slug}/sitemap.xml</loc></sitemap>`) + .map((v) => `<sitemap><loc>${url.origin}/en/${v.slug}/sitemap.xml</loc></sitemap>`) .join(""); return new Response( `<?xml version="1.0" encoding="UTF-8"?><sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">${entries}</sitemapindex>`, diff --git a/workers/apex/src/uagate.ts b/workers/apex/src/uagate.ts index 822a7277..4a407e51 100644 --- a/workers/apex/src/uagate.ts +++ b/workers/apex/src/uagate.ts @@ -6,13 +6,43 @@ export interface UaPolicy { export type UaVerdict = "allow" | "block" | "log"; +/** + * The LONGEST entry in `list` occurring in the (already-lowercased) UA, lowercased, or null. + * Longest rather than first-hit so the containment test in uaVerdict() compares against the + * most specific entry a multi-token UA matched, not an arbitrary earlier one. + */ +function bestMatch(lowerUa: string, list: string[]): string | null { + return list.reduce<string | null>((best, entry) => { + const needle = entry.toLowerCase(); + if (!lowerUa.includes(needle)) return best; + return best === null || needle.length > best.length ? needle : best; + }, null); +} + export function uaVerdict(ua: string, policy: UaPolicy): UaVerdict { - const hit = (list: string[]) => list.some((n) => ua.toLowerCase().includes(n.toLowerCase())); + const lowerUa = ua.toLowerCase(); // Explicit blocks take precedence — a request-controlled UA string that spoofs an // allow-listed substring (e.g. "Googlebot EvilScraper") must not be able to bypass a // block entry just by also matching the allow list. - if (hit(policy.block)) return "block"; - if (hit(policy.allow)) return "allow"; - if (hit(policy.log)) return "log"; - return "allow"; // fail-open default + if (bestMatch(lowerUa, policy.block) !== null) return "block"; + + // A log match WINS over any competing allow match, unconditionally. `log` is a telemetry + // verdict, not a denial (the request is served either way), so resolving a contest the + // wrong way is asymmetric: choosing `allow` loses the ua-log event permanently, while + // choosing `log` costs one log line. A UA presenting BOTH an allow token and a log token + // (e.g. "GPTBot/1.0 DuckDuckBot") is exactly the shape worth recording. + // + // There used to be a carve-out here: a matched allow entry that strictly CONTAINED the + // matched log entry won, so a policy could express a narrow allow exception inside a + // broader log entry (log "Foo", allow "Foo-Search"). It is gone, for two reasons. It was + // spoofable — containment was tested between the two matched ENTRIES, never against the + // UA's own token structure, so a caller writing "Bytespider/2.0 Bytespider-Search/1.0" + // matched both entries as independent tokens and bought itself `allow`, and the UA + // string is entirely request-controlled. And it bought nothing: no entry pair in + // ua-policy.json takes that branch. The pair the shipped policy does depend on runs the + // OTHER way — Apple ships "Applebot" (search, allow) and "Applebot-Extended" (AI + // training, log), where the log entry is the longer one, so there is no containment and + // log wins regardless. Losing the carve-out costs a future narrow-allow vendor variant + // nothing worse than being logged as well as served. + return bestMatch(lowerUa, policy.log) === null ? "allow" : "log"; // unknown UAs fail open } diff --git a/workers/apex/test/router.test.ts b/workers/apex/test/router.test.ts index 3ba1867c..b5f965fc 100644 --- a/workers/apex/test/router.test.ts +++ b/workers/apex/test/router.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from "vitest"; -import worker from "../src/index"; +import worker, { resolveRange, classifyRangeHeader } from "../src/index"; function makeEnv(overrides: Record<string, unknown> = {}) { const html = (body: string, status = 200) => @@ -126,6 +126,21 @@ describe("apex router (§3.2 order)", () => { vi.resetModules(); } }); + it("/sitemap.xml index entries use the REQUEST origin, not a hard-coded docs.vyos.io", async () => { + // This Worker serves the canary origin too. A canary sitemap index pointing at production + // would send any checker that follows it to docs.vyos.io, so a candidate tree with broken + // per-version sitemaps would pass by silently grading production instead of itself. + const canary = await get("/sitemap.xml"); + const canaryBody = await canary.text(); + expect(canaryBody).toContain("<loc>https://docs-next.vyos.io/en/rolling/sitemap.xml</loc>"); + expect(canaryBody).not.toContain("docs.vyos.io"); + + const prod = await worker.fetch( + new Request("https://docs.vyos.io/sitemap.xml", { headers: { "user-agent": "vitest" } }), + makeEnv({ DOCS_ENV: "production" }), + ); + expect(await prod.text()).toContain("<loc>https://docs.vyos.io/en/1.5/sitemap.xml</loc>"); + }); it("/llms.txt with missing default binding → 503, never 404", async () => { const env = makeEnv({ DOCS_ROLLING: undefined }); expect((await get("/llms.txt", env)).status).toBe(503); @@ -188,9 +203,31 @@ describe("apex router (§3.2 order)", () => { // satisfied byte range on `.range`) behavior closely enough to exercise index.ts's // handling of both without needing the real R2 binding. const ETAG = '"pdf-etag-1"'; + const UPLOADED = new Date("2026-01-15T10:00:00Z"); + const BEFORE_UPLOAD = "Wed, 14 Jan 2026 10:00:00 GMT"; + const AFTER_UPLOAD = "Fri, 16 Jan 2026 10:00:00 GMT"; + // R2 hands back a ReadableStream, not a string, and the difference is exactly what makes + // an abandoned body observable: a stream the Worker neither sends nor cancels stays open + // holding its connection. A string-bodied mock cannot see that class of bug at all, so + // bodies here are real streams that record their own cancellation. `highWaterMark: 0` + // keeps `pull` from running until something actually reads, so a stream cancelled before + // any read still reaches its `cancel()` algorithm rather than being already closed. + function bodyStream(text: string, sink?: { cancelled: string[] }) { + return new ReadableStream({ + pull(c) { + c.enqueue(new TextEncoder().encode(text)); + c.close(); + }, + cancel() { + sink?.cancelled.push(text); + }, + }, { highWaterMark: 0 }); + } + function r2Env( - objects: Record<string, { body: string; etag?: string }>, + objects: Record<string, { body: string; etag?: string; uploaded?: Date }>, overrides: Record<string, unknown> = {}, + sink?: { cancelled: string[] }, ) { return makeEnv({ DOCS_PDFS: { @@ -200,25 +237,100 @@ describe("apex router (§3.2 order)", () => { const etag = hit.etag ?? ETAG; const size = hit.body.length; + const uploaded = hit.uploaded ?? UPLOADED; + const secs = (d: number) => Math.floor(d / 1000); // R2 compares at seconds granularity + + // R2 returns a body-less R2Object whenever an onlyIf precondition FAILS, and it + // never says which one did — that ambiguity is exactly what index.ts has to + // resolve by re-evaluating the request's conditionals against these validators. + // R2 ANDs every validator it is handed and knows nothing about the request + // METHOD; index.ts is what filters the set down to the ones RFC 9110 §13.2.2 + // says apply, so this mock deliberately evaluates whatever it is given. + const bodyless = { httpEtag: etag, size, uploaded }; const ifNoneMatch = options?.onlyIf?.get?.("if-none-match"); - if (ifNoneMatch && ifNoneMatch === etag) { - return { httpEtag: etag, size }; // R2Object, no `body` — precondition matched + if (ifNoneMatch && (ifNoneMatch === "*" || ifNoneMatch.split(",").some( + (t) => t.trim().replace(/^W\//, "") === etag.replace(/^W\//, "")))) { + return bodyless; // If-None-Match matched → "not modified" + } + const ifMatch = options?.onlyIf?.get?.("if-match"); + if (ifMatch && ifMatch !== "*" && !ifMatch.split(",").some( + (t) => t.trim() === etag)) { + return bodyless; // If-Match failed → precondition failed + } + const ifUnmodifiedSince = options?.onlyIf?.get?.("if-unmodified-since"); + if (ifUnmodifiedSince && secs(uploaded.getTime()) > secs(Date.parse(ifUnmodifiedSince))) { + return bodyless; // object is newer than the client's copy } + const ifModifiedSince = options?.onlyIf?.get?.("if-modified-since"); + if (ifModifiedSince && secs(uploaded.getTime()) <= secs(Date.parse(ifModifiedSince))) { + return bodyless; // not modified since the client's copy + } + + // The whole-object result. R2 returns this shape for a plain un-ranged get AND + // — critically — for every Range header it declines to honour. Both verified + // against a real R2 binding under @cloudflare/vitest-pool-workers. + const whole = { + httpEtag: etag, size, uploaded, body: bodyStream(hit.body, sink), + range: { offset: 0, length: size }, + }; const rangeHeader = options?.range?.get?.("range"); - const m = rangeHeader ? /^bytes=(\d+)-(\d+)$/.exec(rangeHeader) : null; - if (m) { - const offset = Number(m[1]); - const length = Number(m[2]) - offset + 1; + if (!rangeHeader) return whole; + + // Range parsing mirrors R2's OWN grammar rather than being merely "strict": + // miniflare src/workers/shared/range.ts uses /^ *bytes *=/i for the prefix and + // /^ *(\d+)? *- *(\d+)? *$/ per comma-separated spec — ASCII SPACE ONLY, never + // \s. index.ts's classifier accepts \s, so the two grammars genuinely disagree + // on a tab. Reproducing R2's grammar here is what makes the tab-separated-Range + // test a real divergence rather than an artefact of a lazily-strict mock. + const prefix = / *bytes *=/i.exec(rangeHeader); + if (!prefix || prefix.index !== 0) return whole; // unknown unit → ignored + const specs = rangeHeader.substring(prefix[0].length).split(","); + if (specs.length !== 1) return whole; // multi-range → ignored + const m = /^ *(\d+)? *- *(\d+)? *$/.exec(specs[0]); + if (!m) return whole; // unparseable (a tab lands here, exactly as in R2) + const [, startRaw, endRaw] = m; + + if (startRaw !== undefined && endRaw !== undefined) { + const offset = Number(startRaw); + const last = Number(endRaw); + // Observed R2: an int-range with first-pos >= size, or an invalid spec with + // last < first, is IGNORED — R2 hands back the complete object rather than + // throwing or returning a zero-length range. (`bytes=10-20` and `bytes=5-2` + // on a 10-byte object both returned `{offset: 0, length: 10}` + the full body.) + if (offset >= size || last < offset) return whole; + const length = Math.min(last, size - 1) - offset + 1; // last-pos clamps to EOF return { - httpEtag: etag, - size, - body: hit.body.slice(offset, offset + length), + httpEtag: etag, size, uploaded, + body: bodyStream(hit.body.slice(offset, offset + length), sink), range: { offset, length }, }; } - - return { httpEtag: etag, size, body: hit.body }; + if (startRaw !== undefined) { // open-ended `bytes=5-` + const offset = Number(startRaw); + if (offset >= size) return whole; // unsatisfiable → ignored + return { + httpEtag: etag, size, uploaded, + body: bodyStream(hit.body.slice(offset), sink), + range: { offset, length: size - offset }, + }; + } + if (endRaw !== undefined) { + // Suffix form. R2Range is a three-shape union and this arm deliberately + // returns the RAW `{suffix}` shape rather than pre-normalizing to + // `{offset, length}` — that is what exercises index.ts's resolveRange(). + // (workerd itself normalizes, but the type admits this shape.) + const n = Number(endRaw); + // miniflare: a suffix >= length yields no ranges, and `bytes=-0` is skipped — + // both leave R2 serving the complete object rather than rejecting. + if (n === 0 || n >= size) return whole; + return { + httpEtag: etag, size, uploaded, + body: bodyStream(hit.body.slice(size - n), sink), + range: { suffix: n }, + }; + } + return whole; // bare `bytes=-` }, } as unknown as R2Bucket, ...overrides, @@ -330,5 +442,626 @@ describe("apex router (§3.2 order)", () => { expect(r.status).toBe(200); expect(await r.text()).toBe("legacy:/en/1.2/vyos-documentation.pdf"); }); + + // --- Regression: a plain GET must never answer 206. R2 reports a whole-object `range` + // on un-ranged gets, so keying the 206 off `obj.range` alone made EVERY plain GET of the + // 1.3 PDF a 206 with a Content-Range — which is what the nightly canary sweep observed + // ("/en/1.3/vyos-documentation.pdf: status=206") and what RFC 9110 §15.3.7 forbids. --- + + it("plain GET (no Range header) → 200, never 206, even though R2 echoes a whole-object range", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await get("/en/1.3/vyos-documentation.pdf", env); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("suffix Range (bytes=-4) → 206 with the last 4 bytes and a matching Content-Range/Length", async () => { + // The `{suffix}` R2Range shape used to miss the `"offset" in range` guard entirely and + // fall through to the 200 branch, where content-length claimed the WHOLE object size + // while the body held only the tail — a corrupt download for any resumed fetch. + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, // length 9 + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=-4" }, + }), + env, + ); + expect(r.status).toBe(206); + expect(await r.text()).toBe("YTES"); + expect(r.headers.get("content-range")).toBe("bytes 5-8/9"); + expect(r.headers.get("content-length")).toBe("4"); + }); + + it("failed If-Match → 412, not 304 (a 304 would tell the client its stale copy is current)", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", "if-match": '"some-other-etag"' }, + }), + env, + ); + expect(r.status).toBe(412); + expect(await r.text()).toBe(""); + // 412 is an error response, so the §3.3 precedence forces no-store over the PDF class. + expect(r.headers.get("Cache-Control")).toBe("no-store"); + }); + + // --- RFC 9110 §13.2.2 precondition PRECEDENCE. R2 reports THAT an onlyIf precondition + // failed, never WHICH one, so index.ts re-derives it from the request's own headers. + // Testing the not-modified family first got the ordering backwards. --- + + async function conditional(headers: Record<string, string>, method = "GET") { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + return worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method, + headers: { "user-agent": "vitest", ...headers }, + }), + env, + ); + } + + it("If-None-Match alone (matching) → 304", async () => { + expect((await conditional({ "if-none-match": '"pdf-etag-1"' })).status).toBe(304); + }); + + it("If-Match + If-None-Match together → 412: If-Match takes strict precedence", async () => { + // The failing precondition here is If-Match. Answering 304 (because If-None-Match is + // also present) would tell the client its stale copy is still current, when the + // higher-precedence check it asked for actually failed. §13.2.2 steps 1 and 3. + const r = await conditional({ + "if-match": '"stale-etag"', + "if-none-match": '"pdf-etag-1"', + }); + expect(r.status).toBe(412); + expect(r.headers.get("Cache-Control")).toBe("no-store"); + }); + + it("If-Unmodified-Since → 412, never 304 (§13.2.2 step 2 outranks the 304 family)", async () => { + const r = await conditional({ + "if-unmodified-since": "Wed, 01 Jan 2020 00:00:00 GMT", + "if-none-match": '"pdf-etag-1"', + }); + expect(r.status).toBe(412); + }); + + it("a failed If-None-Match on a non-GET/HEAD method → 412, not 304", async () => { + // §13.1.2: on a false If-None-Match the origin MUST answer "304 ... if the request + // method is GET or HEAD or 412 ... for all other request methods". Nothing upstream + // restricts the method, so this path is reachable and 304 would be an invalid answer. + const r = await conditional({ "if-none-match": '"pdf-etag-1"' }, "POST"); + expect(r.status).toBe(412); + }); + + it("HEAD keeps the 304 (it is one of the two methods §13.1.2 allows it for)", async () => { + expect((await conditional({ "if-none-match": '"pdf-etag-1"' }, "HEAD")).status).toBe(304); + }); + + // --- RFC 9110 §14.1.2 / §15.5.17 range satisfiability. R2 signals "I ignored your + // Range" by returning the WHOLE object — the same shape as a satisfied whole-object + // range — so index.ts re-derives intent from the client's own header. --- + + async function ranged(rangeHeader: string, body = "PDF-BYTES") { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body } }, + { DOCS_ENV: "production" }, + ); + return worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: rangeHeader }, + }), + env, + ); + } + + it("Range past EOF → 416 + Content-Range: bytes */size, NOT a 206 serving the whole body", async () => { + // The pre-fix path trusted obj.range, and R2 answers an unsatisfiable range with the + // complete object — so this returned `206 Content-Range: bytes 0-8/9` plus all 9 + // bytes. A client resuming at byte 99 would have appended bytes 0-8 to its partial + // file and silently corrupted the download. + const r = await ranged("bytes=99-"); // body is 9 bytes + expect(r.status).toBe(416); + expect(r.headers.get("content-range")).toBe("bytes */9"); + expect(await r.text()).toBe(""); + expect(r.headers.get("Cache-Control")).toBe("no-store"); // 416 >= 400 + }); + + it("Range starting exactly at EOF → 416 (§14.1.2: satisfiable iff first-pos < length)", async () => { + const r = await ranged("bytes=9-"); + expect(r.status).toBe(416); + expect(r.headers.get("content-range")).toBe("bytes */9"); + }); + + it("closed Range wholly past EOF → 416", async () => { + const r = await ranged("bytes=20-30"); + expect(r.status).toBe(416); + }); + + it("suffix-length 0 → 416 (§14.1.2 names it unsatisfiable)", async () => { + const r = await ranged("bytes=-0"); + expect(r.status).toBe(416); + expect(r.headers.get("content-range")).toBe("bytes */9"); + }); + + it("multi-range → 200 with the complete body, not a single-range 206 that misdescribes it", async () => { + // R2 ignores multi-ranges and returns the whole object. Stamping + // `Content-Range: bytes 0-8/9` on it would claim a single partial covering + // everything, in answer to a request for two disjoint sub-ranges. + const r = await ranged("bytes=0-1,4-5"); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("malformed Range → 200 with the complete body (§14.1.2: invalid spec is ignored)", async () => { + for (const bad of ["bytes=abc", "bytes=-", "bytes=5-2", "items=0-5"]) { + const r = await ranged(bad); + expect(r.status, `Range: ${bad}`).toBe(200); + expect(r.headers.get("content-range"), `Range: ${bad}`).toBeNull(); + } + }); + + it("a satisfiable whole-object Range still gets a real 206", async () => { + // The 416/200 guards must not swallow the legitimate case: `bytes=0-` IS satisfiable + // (first-pos 0 < 9), so it keeps its 206 even though the payload is the whole object. + const r = await ranged("bytes=0-"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 0-8/9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + // --- The classifier's grammar and R2's grammar are NOT the same grammar, and the + // Worker no longer assumes they are: it checks the bounds it derived against the bytes + // R2 actually returned before promising a 206. --- + + it("tab-separated Range → 200 with the whole body, never a 206 for bytes nobody sliced", async () => { + // R2 parses ranges with ASCII space only (/^ *bytes *=/i + /^ *(\d+)? *- *(\d+)? *$/); + // the classifier's \s also accepts a tab. So this header says "single, bytes 2-4" + // here and "unparseable, serve everything" to R2 — and trusting the classifier alone + // shipped `206 Content-Range: bytes 0-8/9` carrying all 9 bytes in answer to a + // request for 3. Same lying-206 class as the unsatisfiable case above. + const r = await ranged("bytes=2\t-\t4"); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("LEADING whitespace is stripped before either parser sees it, so only INNER whitespace diverges", async () => { + // Worth pinning because it bounds the divergence surface. `Headers` strips the + // optional whitespace around a field value (RFC 9110 §5.5), so "\tbytes=2-4" arrives + // as "bytes=2-4" and both grammars accept it — the 206 here is correct, not a + // regression. Only whitespace INSIDE the value (the test above) can reach the two + // parsers intact and be read differently by them. + const r = await ranged("\tbytes=2-4"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 2-4/9"); + }); + + it("space-separated Range stays a 206 — R2 accepts spaces, so the bounds still agree", async () => { + // The degrade must be driven by actual disagreement, not by giving up on whitespace. + const r = await ranged("bytes = 2-4"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 2-4/9"); + expect(await r.text()).toBe("F-B"); + }); + + it("positions above 2^53: an invalid spec is ignored (200), not read as unsatisfiable (416)", async () => { + // Number() rounds 9007199254740993 down to ...992, so `last < first` read as false and + // this invalid spec was promoted to "unsatisfiable" → 416. §14.1.2 says ignore it. + const r = await ranged("bytes=9007199254740993-9007199254740992"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("a partial body the Worker did not ask for is a 503, never a 206 for bytes nobody requested", async () => { + // Belt and braces for a future R2 whose grammar accepts something this classifier + // calls "ignored". The body in hand is a slice of bounds the request never named: + // a 200 would ship Content-Length: 9 over 3 bytes, and a 206 would carry + // `Content-Range: bytes 2-4/9` in answer to `bytes=0-1,4-5` — a selected range the + // client did not select, which §14.4 does not permit. Neither is honest, so the + // divergence is surfaced as a failure (plus the r2-range-divergence log line) rather + // than dressed up as a success. Unreachable under today's workerd, which ignores + // multi-range and returns the whole object. + const env = makeEnv({ + DOCS_ENV: "production", + DOCS_PDFS: { + get: async () => ({ + httpEtag: ETAG, size: 9, uploaded: UPLOADED, + body: "F-B", range: { offset: 2, length: 3 }, + }), + } as unknown as R2Bucket, + }); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=0-1,4-5" }, // classifier: ignored + }), + env, + ); + expect(r.status).toBe(503); + expect(r.headers.get("content-range")).toBeNull(); + }); + + // --- §14.2: "GET is the only method for which range handling is defined." --- + + it("HEAD + Range → 200, no Content-Range: Range is ignored on every method but GET", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method: "HEAD", + headers: { "user-agent": "vitest", range: "bytes=0-3" }, + }), + env, + ); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + }); + + it("POST + an unsatisfiable Range → 200, not 416: the header is ignored, not judged", async () => { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method: "POST", + headers: { "user-agent": "vitest", range: "bytes=99-" }, + }), + env, + ); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + }); + + // --- §13.2.1: "a server MUST ignore the conditional request header fields defined by + // this specification when received with a request method that does not involve the + // selection or modification of a selected representation, such as CONNECT, OPTIONS, or + // TRACE." Only OPTIONS is testable through worker.fetch() — TRACE and CONNECT are + // forbidden methods in the fetch spec and `new Request` refuses to construct them. --- + + it("OPTIONS ignores conditionals entirely: neither a failing nor a matching one is judged", async () => { + // A stale If-Match reached R2 as an onlyIf, came back body-less, and this Worker had + // no reading of that but 412 — so an OPTIONS carrying a conditional a client had left + // lying around was refused where the same request without it succeeded. + const options = (headers: Record<string, string>) => worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + method: "OPTIONS", + headers: { "user-agent": "vitest", ...headers }, + }), + r2Env({ "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }), + ); + expect((await options({ "if-match": '"stale-etag"' })).status).toBe(200); // not 412 + expect((await options({ "if-unmodified-since": BEFORE_UPLOAD })).status).toBe(200); // not 412 + expect((await options({ "if-none-match": ETAG })).status).toBe(200); // not 304/412 + }); + + // --- RFC 9110 §13.1.5 If-Range. R2's R2Conditional carries only etagMatches / + // etagDoesNotMatch / uploadedBefore / uploadedAfter, so an If-Range in the forwarded + // Headers is silently DROPPED and the range applied unconditionally. --- + + async function withIfRange(ifRange: string, rangeHeader: string) { + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, + ); + return worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: rangeHeader, "if-range": ifRange }, + }), + env, + ); + } + + // --- Abandoned body streams. R2 hands back a body on paths whose response carries + // none; a stream that is neither sent nor cancelled holds its connection until GC. --- + + it("an unsatisfiable Range cancels the whole-object body it answers 416 without", async () => { + // The largest abandoned stream on any path here: R2 answers an unsatisfiable Range + // with the COMPLETE object, which for the 1.3 PDF is 29.2 MiB, and the 416 sends none + // of it. Cancelling aborts the transfer rather than draining or leaking it. + const sink = { cancelled: [] as string[] }; + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, sink, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=99-" }, + }), + env, + ); + expect(r.status).toBe(416); + expect(sink.cancelled).toEqual(["PDF-BYTES"]); + }); + + it("a stale If-Range cancels the sliced body it discards before re-reading", async () => { + const sink = { cancelled: [] as string[] }; + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, sink, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=4-", "if-range": '"stale-etag"' }, + }), + env, + ); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + expect(sink.cancelled).toEqual(["BYTES"]); // the abandoned slice, not the served body + }); + + it("a served body is NEVER cancelled — the cleanup must not reach the response path", async () => { + const sink = { cancelled: [] as string[] }; + const env = r2Env( + { "legacy/1.3/vyos-documentation.pdf": { body: "PDF-BYTES" } }, + { DOCS_ENV: "production" }, sink, + ); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { "user-agent": "vitest", range: "bytes=4-" }, // satisfiable, honoured + }), + env, + ); + expect(r.status).toBe(206); + expect(await r.text()).toBe("BYTES"); + expect(sink.cancelled).toEqual([]); + }); + + it("If-Range matching the current ETag → the range is honoured, 206", async () => { + const r = await withIfRange(ETAG, "bytes=4-"); + expect(r.status).toBe(206); + expect(r.headers.get("content-range")).toBe("bytes 4-8/9"); + expect(await r.text()).toBe("BYTES"); + }); + + it("If-Range naming a STALE ETag → 200 with the complete new representation", async () => { + // The corruption case. R2 cannot evaluate If-Range, so it applied the range anyway and + // this returned bytes 4+ of the NEW object under a 206 — a resuming downloader then + // appends the new tail to its old prefix and silently produces a broken PDF. §13.1.5 + // requires the failed validator to yield the complete representation instead. + const r = await withIfRange('"stale-etag"', "bytes=4-"); + expect(r.status).toBe(200); + expect(r.headers.get("content-range")).toBeNull(); + expect(r.headers.get("content-length")).toBe("9"); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("a stale If-Range wins over an unsatisfiable spec → 200, not 416", async () => { + // Ordering matters: a Range being ignored entirely (§13.1.5) is decided before + // satisfiability (§14.1.2) is ever judged, so no 416 may escape here. + const r = await withIfRange('"stale-etag"', "bytes=99-"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("a WEAK If-Range validator never matches (§13.1.5 requires a strong one)", async () => { + const r = await withIfRange('W/"pdf-etag-1"', "bytes=4-"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("an HTTP-date If-Range never matches — this Worker emits no Last-Modified to compare against", async () => { + const r = await withIfRange(AFTER_UPLOAD, "bytes=4-"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("an object rewritten between the two If-Range reads is still judged against the request's preconditions", async () => { + // §13.2.1 requires preconditions to hold for the representation ULTIMATELY SELECTED. + // The re-read after a stale If-Range was a BARE get(), dropping every other + // precondition the request carried, so: If-Match passes on read 1, the key is + // rewritten, and the bare read 2 then answered 200 with the very representation the + // client's If-Match excluded. The key does get rewritten — `force_pdf_refresh: true` + // in the legacy snapshot repo's deploy workflow re-uploads it. + const KEY = "legacy/1.3/vyos-documentation.pdf"; + const objects: Record<string, { body: string; etag?: string }> = { + [KEY]: { body: "PDF-BYTES", etag: ETAG }, + }; + // Borrow the shared mock, then wrap it so the object changes BETWEEN the two reads. + type Bucket = { get: (key: string, options?: unknown) => Promise<unknown> }; + const inner = (r2Env(objects) as unknown as { DOCS_PDFS: Bucket }).DOCS_PDFS; + let reads = 0; + const env = r2Env(objects, { + DOCS_ENV: "production", + DOCS_PDFS: { + get: async (key: string, options?: unknown) => { + const result = await inner.get(key, options); + if (++reads === 1) objects[key].etag = '"pdf-etag-2"'; // rewritten mid-flight + return result; + }, + }, + }); + const r = await worker.fetch( + new Request("https://docs-next.vyos.io/en/1.3/vyos-documentation.pdf", { + headers: { + "user-agent": "vitest", + range: "bytes=4-", + "if-range": '"stale-etag"', // fails → forces the whole-object re-read + "if-match": ETAG, // satisfied on read 1, violated by read 2's object + }, + }), + env, + ); + expect(reads).toBe(2); + expect(r.status).toBe(412); + expect(await r.text()).toBe(""); + // The validator reported is the one belonging to the object the verdict was reached on. + expect(r.headers.get("etag")).toBe('"pdf-etag-2"'); + expect(r.headers.get("content-type")).not.toBe("application/pdf"); + }); + + // --- §13.2.2 preconditions, decided by EVALUATING the validators rather than by + // guessing from which headers are present. --- + + it("If-Match satisfied + If-None-Match satisfied on a GET → 304, not 412", async () => { + // The inverse of the If-Match-fails case above, and the one presence-inference got + // wrong: If-Match matches (so step 1 passes) while If-None-Match also matches (so + // step 3 FAILS) — §13.1.2 owes a 304. Seeing an If-Match header at all returned 412. + const r = await conditional({ + "if-match": '"pdf-etag-1"', + "if-none-match": '"pdf-etag-1"', + }); + expect(r.status).toBe(304); + expect(await r.text()).toBe(""); + }); + + it("If-Modified-Since alone on a non-GET/HEAD → 200: §13.2.2 step 4 never evaluates it", async () => { + // R2 ANDs every validator it is handed and knows nothing about the method, so + // forwarding the raw headers made it fail the request on a validator the RFC says to + // ignore — and the only answer left was a 412. Filtering the conditionals down to the + // applicable set means the request simply proceeds. + const r = await conditional({ "if-modified-since": AFTER_UPLOAD }, "POST"); + expect(r.status).toBe(200); + expect(await r.text()).toBe("PDF-BYTES"); + }); + + it("If-Modified-Since alone on a GET IS evaluated → 304", async () => { + // The other side of the filter: dropping the header for non-GET must not drop it here. + expect((await conditional({ "if-modified-since": AFTER_UPLOAD })).status).toBe(304); + }); + + it("a satisfied If-Unmodified-Since serves the object; a failed one is 412", async () => { + expect((await conditional({ "if-unmodified-since": AFTER_UPLOAD })).status).toBe(200); + expect((await conditional({ "if-unmodified-since": BEFORE_UPLOAD })).status).toBe(412); + }); + + it("If-Match: * matches any existing representation (§13.1.1)", async () => { + expect((await conditional({ "if-match": "*" })).status).toBe(200); + }); + + it("If-None-Match: * on an existing representation fails → 304 on a GET", async () => { + expect((await conditional({ "if-none-match": "*" })).status).toBe(304); + }); + + it("If-None-Match matches WEAKLY (§13.1.2 mandates the weak comparison)", async () => { + // A weak tag from the client must still match a strong stored tag, or every + // revalidation from a cache that weakened the tag re-downloads 29.2 MiB. + expect((await conditional({ "if-none-match": 'W/"pdf-etag-1"' })).status).toBe(304); + }); + + it("If-None-Match honours a comma-separated tag list", async () => { + const r = await conditional({ "if-none-match": '"other", "pdf-etag-1"' }); + expect(r.status).toBe(304); + }); + + it("If-Match compares STRONGLY: a weak tag from the client never satisfies it", async () => { + expect((await conditional({ "if-match": 'W/"pdf-etag-1"' })).status).toBe(412); + }); + }); + + describe("resolveRange (R2Range is a three-shape union)", () => { + it("resolves offset+length, length-only, and suffix forms, clamped to the object size", () => { + expect(resolveRange({ offset: 2, length: 3 }, 10)).toEqual({ start: 2, length: 3 }); + expect(resolveRange({ offset: 4 }, 10)).toEqual({ start: 4, length: 6 }); // to end of object + expect(resolveRange({ length: 4 }, 10)).toEqual({ start: 0, length: 4 }); // offset defaults to 0 + expect(resolveRange({ suffix: 3 }, 10)).toEqual({ start: 7, length: 3 }); // trailing bytes + expect(resolveRange({ suffix: 99 }, 10)).toEqual({ start: 0, length: 10 }); // suffix past start clamps + expect(resolveRange({ offset: 8, length: 99 }, 10)).toEqual({ start: 8, length: 2 }); // length clamps + }); + + it("never returns a negative length, so Content-Length can never go negative", () => { + // A real R2 binding cannot produce these, but resolveRange is exported and its + // contract says "clamped to the object size" — that must hold for every input the + // R2Range type admits, not just the ones observed in practice. + expect(resolveRange({ offset: 0, length: -5 }, 10)).toEqual({ start: 0, length: 0 }); + expect(resolveRange({ offset: 10, length: 5 }, 10)).toEqual({ start: 10, length: 0 }); + expect(resolveRange({ offset: 99 }, 10)).toEqual({ start: 10, length: 0 }); + expect(resolveRange({ suffix: -3 }, 10)).toEqual({ start: 10, length: 0 }); + expect(resolveRange({ offset: 0, length: 0 }, 0)).toEqual({ start: 0, length: 0 }); + }); + }); + + describe("classifyRangeHeader (§14.1.2 satisfiability, re-derived from the client's header)", () => { + // R2 cannot tell us: it answers unsatisfiable, malformed, multi-range and unknown-unit + // Range headers identically — with the complete object — which is also exactly what a + // satisfied whole-object range looks like. Verified against a real R2 binding. + it("single satisfiable byte ranges → single, with the concrete bounds they select", () => { + // The bounds are the point: they are what the Worker compares against the bytes R2 + // actually returned before it will promise a 206. + const cases: Array<[string, number, number]> = [ + ["bytes=0-", 0, 10], ["bytes=5-", 5, 5], ["bytes=0-0", 0, 1], + ["bytes=-3", 7, 3], ["bytes=0-9", 0, 10], + ["bytes=5-99", 5, 5], // last-pos clamps to EOF + ["bytes=9-", 9, 1], + ["bytes=-99", 0, 10], // suffix past the start is the whole object + ]; + for (const [h, start, length] of cases) { + expect(classifyRangeHeader(h, 10), h).toEqual({ kind: "single", start, length }); + } + }); + + it("unsatisfiable ranges → unsatisfiable", () => { + for (const h of ["bytes=10-", "bytes=99-", "bytes=10-20", "bytes=-0"]) { + // first-pos >= length, or suffix-length 0 + expect(classifyRangeHeader(h, 10), h).toEqual({ kind: "unsatisfiable" }); + } + }); + + it("multi-range, malformed and unknown-unit → ignored (§14.1.2: an invalid spec is ignored)", () => { + for (const h of ["bytes=0-1,4-5", "bytes=abc", "bytes=-", "items=0-5", "bytes=5-2", ""]) { + expect(classifyRangeHeader(h, 10), h).toEqual({ kind: "ignored" }); + } + }); + + it("tolerates the case and whitespace variation R2 itself accepts", () => { + // R2 honours all three of these, so misreading them as "ignored" would downgrade a + // legitimate 206 to a 200. + expect(classifyRangeHeader("BYTES=0-5", 10)).toEqual({ kind: "single", start: 0, length: 6 }); + expect(classifyRangeHeader("bytes = 0-5", 10)).toEqual({ kind: "single", start: 0, length: 6 }); + expect(classifyRangeHeader("bytes=0-5 ", 10)).toEqual({ kind: "single", start: 0, length: 6 }); + }); + + it("zero-length representation: only a non-zero suffix-range is satisfiable", () => { + // §14.1.2 states this case explicitly. `bytes=0-` fails first-pos < length (0 < 0). + expect(classifyRangeHeader("bytes=0-", 0)).toEqual({ kind: "unsatisfiable" }); + // Satisfiable, but it selects zero bytes — no Content-Range can describe an empty + // selection (§14.4), so the caller's `length > 0` guard sends it to a 200. + expect(classifyRangeHeader("bytes=-5", 0)).toEqual({ kind: "single", start: 0, length: 0 }); + }); + + it("positions above 2^53 compare exactly — an invalid spec stays ignored, not 416", () => { + // Number() rounds both of these to 9007199254740992, so `last < first` read as false + // and the spec was promoted from "invalid, ignore it" (§14.1.2 → 200) to + // "unsatisfiable" (→ 416). Digit-string comparison is exact at any magnitude. + expect(classifyRangeHeader("bytes=9007199254740993-9007199254740992", 10)) + .toEqual({ kind: "ignored" }); + // ...while a genuinely huge first-pos is still unsatisfiable. + expect(classifyRangeHeader("bytes=9007199254740993-", 10)).toEqual({ kind: "unsatisfiable" }); + // Leading zeros normalise rather than inflating the digit count. + expect(classifyRangeHeader("bytes=00000005-00000002", 10)).toEqual({ kind: "ignored" }); + expect(classifyRangeHeader("bytes=0000000002-0000000005", 10)) + .toEqual({ kind: "single", start: 2, length: 4 }); + }); + + it("accepts whitespace R2's own parser rejects — the divergence the bounds check absorbs", () => { + // R2 parses ranges with `/^ *bytes *=/i` + `/^ *(\d+)? *- *(\d+)? *$/` (ASCII space + // only; miniflare src/workers/shared/range.ts). This classifier's `\s` accepts a tab + // too, so the two grammars genuinely disagree here. That is tolerated by design: the + // Worker checks these bounds against the bytes R2 returned, so a spec R2 declined + // degrades to a 200 rather than to a 206 describing a body nobody asked for. The + // end-to-end proof is the "tab-separated Range" test below. + expect(classifyRangeHeader("bytes=2\t-\t4", 10)).toEqual({ kind: "single", start: 2, length: 3 }); + expect(classifyRangeHeader("\tbytes=2-4", 10)).toEqual({ kind: "single", start: 2, length: 3 }); + }); }); }); diff --git a/workers/apex/test/uagate.test.ts b/workers/apex/test/uagate.test.ts index f4c16c66..1989f847 100644 --- a/workers/apex/test/uagate.test.ts +++ b/workers/apex/test/uagate.test.ts @@ -17,8 +17,86 @@ describe("UA gate (§3.2.1) — ships log-only for AI crawlers", () => { it("unknown UA → allow (fail-open for humans)", () => { expect(uaVerdict("Mozilla/5.0 (X11; Linux x86_64) Firefox/128.0", policy)).toBe("allow"); }); + it("Applebot is allowed but Applebot-Extended is logged — most-specific match wins", () => { + // Apple's AI-training crawler token CONTAINS the search crawler's, so plain + // substring matching with a fixed allow-before-log precedence let the allow entry + // swallow it: the AI crawler was allowed AND never logged, unlike every other AI + // crawler in the log list. + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot/0.1; +http://www.apple.com/go/applebot)", policy)).toBe("allow"); + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot-Extended/0.1)", policy)).toBe("log"); + }); + it("Google-Extended is not a UA token — it must not sit in the UA policy at all", () => { + // Google-Extended is a robots.txt user-agent control token; it never appears in a + // User-Agent header, so an entry for it could only ever be dead weight. + // Compared case-INSENSITIVELY on both sides: bestMatch() lowercases every policy entry + // before matching, so "google-extended" would be functionally identical to the token + // this guard exists to keep out — but toContain() compares primitives by strict + // equality, so a lowercase variant would sail past a case-sensitive assertion and + // quietly restore the entry. Match the matcher's own case semantics. + const entries = [...policy.allow, ...policy.log, ...policy.block].map((e) => e.toLowerCase()); + expect(entries).not.toContain("google-extended"); + }); it("block takes precedence over allow on a UA matching both lists", () => { const dualMatch = { ...policy, allow: ["Googlebot"], block: ["Googlebot EvilScraper"] }; expect(uaVerdict("Mozilla/5.0 (compatible; Googlebot EvilScraper/1.0)", dualMatch)).toBe("block"); }); + + // --- allow-vs-log contests. Pinned verdicts for the four UAs that distinguish every + // candidate rule, so a future tweak to the precedence cannot silently drop telemetry. --- + + it("a UA carrying BOTH a log token and a longer allow token is logged, not allowed", () => { + // "GPTBot/1.0 DuckDuckBot" matches allow "DuckDuckBot" (11 chars) and log "GPTBot" (6). + // Under the longest-match rule the longer ALLOW needle won and the ua-log event never + // fired; under the original allow-before-log rule it also won. A UA presenting two + // different crawlers' tokens is precisely the shape worth recording, and `log` costs + // nothing but a log line — the request is served either way. + expect(uaVerdict("GPTBot/1.0 DuckDuckBot", policy)).toBe("log"); + }); + + it("pinned verdicts for the four discriminating UAs", () => { + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot/0.1)", policy)).toBe("allow"); + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot-Extended/0.1)", policy)).toBe("log"); + expect(uaVerdict("GPTBot/1.0 DuckDuckBot", policy)).toBe("log"); + expect(uaVerdict("Mozilla/5.0 (compatible; Googlebot/2.1)", policy)).toBe("allow"); + }); + + it("a narrow allow entry no longer overrides a matched log entry — log wins outright", () => { + // This branch used to return "allow" when the matched allow entry strictly CONTAINED + // the matched log entry, so a policy could carve a narrow allow out of a broad log + // entry. Removed as spoofable (see the next test). Both rows are now "log", which is + // the safe verdict — the request is still served either way; only telemetry differs. + const carveOut = { allow: ["Bytespider-Search"], log: ["Bytespider"], block: [] }; + expect(uaVerdict("Bytespider-Search/1.0", carveOut)).toBe("log"); + expect(uaVerdict("Bytespider/1.0", carveOut)).toBe("log"); + }); + + it("the removed carve-out was spoofable by quoting both tokens independently", () => { + // The concrete bypass. Containment was tested between the two matched ENTRIES, never + // against the UA's own token structure, so a request-controlled string naming both + // tokens separately matched allow "Bytespider-Search" and log "Bytespider", satisfied + // the containment test, and bought the AI crawler an `allow`. It must be logged. + const carveOut = { allow: ["Bytespider-Search"], log: ["Bytespider"], block: [] }; + expect(uaVerdict("Bytespider/2.0 Bytespider-Search/1.0", carveOut)).toBe("log"); + }); + + it("dropping the carve-out leaves every SHIPPED-policy verdict unchanged", () => { + // The vendor pair the shipped policy actually depends on runs the other way round: log + // "Applebot-Extended" is LONGER than allow "Applebot", so the allow entry never + // contained the log entry and log already won. No pair in ua-policy.json took the + // removed branch, so its removal is behaviour-preserving for what we ship. + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot/0.1)", policy)).toBe("allow"); + expect(uaVerdict("Mozilla/5.0 (compatible; Applebot-Extended/0.1)", policy)).toBe("log"); + }); + + it("an entry present in BOTH lists resolves to log, not allow", () => { + // Equality is not containment. Listing the same token twice is an authoring error, and + // `log` is the resolution that cannot lose data. + const contradictory = { allow: ["CCBot"], log: ["CCBot"], block: [] }; + expect(uaVerdict("CCBot/2.0", contradictory)).toBe("log"); + }); + + it("block still short-circuits ahead of the allow-vs-log contest", () => { + const all3 = { allow: ["DuckDuckBot"], log: ["GPTBot"], block: ["EvilScraper"] }; + expect(uaVerdict("GPTBot/1.0 DuckDuckBot EvilScraper", all3)).toBe("block"); + }); }); diff --git a/workers/apex/ua-policy.json b/workers/apex/ua-policy.json index 4a3534eb..f0021be5 100644 --- a/workers/apex/ua-policy.json +++ b/workers/apex/ua-policy.json @@ -1,5 +1,5 @@ { "allow": ["Googlebot", "bingbot", "DuckDuckBot", "YandexBot", "Applebot", "UptimeRobot"], - "log": ["GPTBot", "CCBot", "ClaudeBot", "Google-Extended", "Bytespider", "PerplexityBot", "meta-externalagent"], + "log": ["GPTBot", "CCBot", "ClaudeBot", "Applebot-Extended", "Bytespider", "PerplexityBot", "meta-externalagent"], "block": [] } |