summaryrefslogtreecommitdiff
path: root/workers/apex/src
diff options
context:
space:
mode:
authorYuriy Andamasov <yuriy@vyos.io>2026-08-21 23:25:49 +0300
committerGitHub <noreply@github.com>2026-08-21 23:25:49 +0300
commit3a1c6c3020c4ed819616098a91be670719e290cc (patch)
tree758da979ad4f59d9f92988bede8bd241c3c6b3c8 /workers/apex/src
parent4a5950de1a4ad6e0a5390d1edb089f7139e7dfc5 (diff)
downloadvyos-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)
Diffstat (limited to 'workers/apex/src')
-rw-r--r--workers/apex/src/index.ts438
-rw-r--r--workers/apex/src/special.ts7
-rw-r--r--workers/apex/src/uagate.ts40
3 files changed, 461 insertions, 24 deletions
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
}