diff options
| -rw-r--r-- | .github/workflows/docs-build.yml | 509 | ||||
| -rw-r--r-- | scripts/docs_gates/gates.py | 7 | ||||
| -rw-r--r-- | scripts/docs_gates/parity.py | 160 | ||||
| -rw-r--r-- | scripts/docs_gates/smoke.py | 44 | ||||
| -rw-r--r-- | scripts/docs_gates/test_gates.py | 18 | ||||
| -rw-r--r-- | scripts/docs_gates/test_parity.py | 270 | ||||
| -rw-r--r-- | scripts/docs_gates/test_smoke.py | 117 | ||||
| -rw-r--r-- | workers/.gitignore | 1 | ||||
| -rw-r--r-- | workers/apex/src/index.ts | 438 | ||||
| -rw-r--r-- | workers/apex/src/special.ts | 7 | ||||
| -rw-r--r-- | workers/apex/src/uagate.ts | 40 | ||||
| -rw-r--r-- | workers/apex/test/router.test.ts | 759 | ||||
| -rw-r--r-- | workers/apex/test/uagate.test.ts | 78 | ||||
| -rw-r--r-- | workers/apex/ua-policy.json | 2 |
14 files changed, 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": [] } |