summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYuriy Andamasov <yuriy@vyos.io>2026-08-21 23:43:26 +0300
committerYuriy Andamasov <yuriy@vyos.io>2026-08-21 23:43:26 +0300
commitbeec730d3743687482c6516dec8c15cc2bcea63b (patch)
treed0f2ab96c5b547b493fc517e42b95f37801ce48f
parent0f69d0846fef313f23c84d7dff8b5ae8e24ec04c (diff)
downloadvyos-documentation-claude/cf-port-circinus.tar.gz
vyos-documentation-claude/cf-port-circinus.zip
ci: IS-572: re-sync ported Cloudflare Workers pipeline files with rollingclaude/cf-port-circinus
The category-1 files in this port are byte-identical copies from `rolling`. `rolling` has since moved: [vyos-documentation#2209](https://github.com/vyos/vyos-documentation/pull/2209) merged as `3a1c6c30`, thirteen rounds of hardening on exactly these files. Re-take all 14 category-1 paths from `origin/rolling` via `git checkout origin/rolling -- <paths>`, so byte-identity holds by construction rather than by hand-editing: .github/workflows/docs-build.yml scripts/docs_gates/{gates,parity,smoke,test_gates,test_parity,test_smoke}.py workers/.gitignore workers/apex/src/{index,special,uagate}.ts workers/apex/test/{router,uagate}.test.ts workers/apex/ua-policy.json Thirteen of the fourteen carry [vyos-documentation#2209](https://github.com/vyos/vyos-documentation/pull/2209) exactly β€” the pre-change tree was byte-identical to `3a1c6c30^` for those paths. `workers/.gitignore` additionally picks up the one-line `test-results/` entry from [vyos-documentation#2212](https://github.com/vyos/vyos-documentation/pull/2212); inert on circinus, since only the deliberately-unported `apex-deploy.yml` writes that directory. Deliberate exclusions are unchanged: `docs-canary-qa.yml` (cron runs on the default branch only, so it is not ported even though [vyos-documentation#2209](https://github.com/vyos/vyos-documentation/pull/2209) touched it on `rolling`), `apex-deploy.yml`, and the `docs-preview-*` workflows. `docs/conf.py` stays hand-merged and circinus-specific, with its ReadTheDocs fallback intact. πŸ€– Generated by [robots](https://vyos.io)
-rw-r--r--.github/workflows/docs-build.yml509
-rw-r--r--scripts/docs_gates/gates.py7
-rw-r--r--scripts/docs_gates/parity.py160
-rw-r--r--scripts/docs_gates/smoke.py44
-rw-r--r--scripts/docs_gates/test_gates.py18
-rw-r--r--scripts/docs_gates/test_parity.py270
-rw-r--r--scripts/docs_gates/test_smoke.py117
-rw-r--r--workers/.gitignore1
-rw-r--r--workers/apex/src/index.ts438
-rw-r--r--workers/apex/src/special.ts7
-rw-r--r--workers/apex/src/uagate.ts40
-rw-r--r--workers/apex/test/router.test.ts759
-rw-r--r--workers/apex/test/uagate.test.ts78
-rw-r--r--workers/apex/ua-policy.json2
14 files changed, 2349 insertions, 101 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/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/.gitignore b/workers/.gitignore
index ef5b8080..4b240bc1 100644
--- a/workers/.gitignore
+++ b/workers/.gitignore
@@ -1,3 +1,4 @@
node_modules/
.wrangler/
dist/
+test-results/
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": []
}