summaryrefslogtreecommitdiff
path: root/scripts/docs_gates
diff options
context:
space:
mode:
authorYuriy Andamasov <yuriy@vyos.io>2026-08-21 23:25:49 +0300
committerGitHub <noreply@github.com>2026-08-21 23:25:49 +0300
commit3a1c6c3020c4ed819616098a91be670719e290cc (patch)
tree758da979ad4f59d9f92988bede8bd241c3c6b3c8 /scripts/docs_gates
parent4a5950de1a4ad6e0a5390d1edb089f7139e7dfc5 (diff)
downloadvyos-documentation-3a1c6c3020c4ed819616098a91be670719e290cc.tar.gz
vyos-documentation-3a1c6c3020c4ed819616098a91be670719e290cc.zip
fix: IS-572: correct verified defects in the Cloudflare Workers docs pipeline (#2209)
* fix: IS-572: correct verified defects in the Cloudflare Workers docs pipeline Fixes raised against the LTS port PR but landing on rolling first, so the backport runs in the right direction, plus one defect found while verifying the nightly canary sweep. - apex PDF: a 206 is now only answered to a request that actually carried a Range header, and every R2Range shape is normalized to concrete bounds. R2 reports a whole-object range on un-ranged gets, so keying the 206 off obj.range alone made every plain GET of the 1.3 PDF a 206. - apex PDF: a failed If-Match now maps to 412, not 304. - apex sitemap index: entries derive their origin from the request, so the canary index no longer points at production. - UA policy: drop the dead Google-Extended entry (a robots.txt token, never a UA) and log Applebot-Extended while still allowing Applebot, via a most-specific-match precedence between the allow and log lists. - docs-build: replace the check_head guard's exit 78 (neutral only in the retired Actions v1 runtime) with an output gating every deploy/promote step, so a superseded run ends green-and-skipped instead of red. - smoke/parity: read CF Access service-token credentials from the environment instead of argv, and pass them via env: in both workflows. - smoke: stop asserting X-Docs-Build on the PDF probe — apex's R2 fallback path legitimately never sets it, making the probe unpassable for 1.3. - gates/smoke: strip critical-pages lines before testing for comments. - parity: fetch each sitemap once, honouring the _SCHEME override. Advances: IS-572 * fix: IS-572: harden check_head guard against a failed ls-remote Phase-0 CodeRabbit findings on the new guard, both valid: - set -euo pipefail + an explicit emptiness check, so a failed git ls-remote cannot leave $remote empty and be misread as 'branch moved' (cut exits 0 on empty input) — that would silently skip a deploy that should have run. - pass github.ref_name and github.sha through env vars rather than interpolating them into the shell command. Advances: IS-572 * fix: IS-572: address adversarial + CodeRabbit review on the CF docs gates Answers the seven adversarial-review findings on vyos/vyos-documentation#2209, five of which GitHub CodeRabbit independently reproduced on review 4946100889. Range handling (apex Worker) — the reported defect was real but mis-diagnosed. Probing a real R2 binding under vitest-pool-workers shows R2 never returns the inverted/zero-length range the review predicted: for the Headers form this Worker uses, R2 answers every unsatisfiable, malformed, multi-range and unknown-unit Range by IGNORING it and returning the complete object with range={offset:0,length:size} — indistinguishable from a satisfied whole-object range, and it does not throw. (Only the object-literal form throws, code 10039, which this Worker cannot reach.) Trusting obj.range therefore answered `Range: bytes=99-` with 206 + `Content-Range: bytes 0-8/9` and the whole body, so a client resuming mid-download would silently corrupt its file. Range intent is now re-derived from the client's own header per RFC 9110 §14.1.2: unsatisfiable → 416 + `Content-Range: bytes */size`, ignored → 200 with the complete body, satisfiable → 206 as before. resolveRange() additionally clamps its length at zero so its documented contract holds for every R2Range input. Preconditions: reorder to RFC 9110 §13.2.2's MUST-ordered precedence (If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since) so `If-Match: "old"` + `If-None-Match: "new"` yields 412 rather than 304, and gate 304 on GET/HEAD per §13.1.2 so a conditional POST cannot be answered 304. UA gate: a log match now wins a contest against an allow match unless the allow entry strictly contains the log entry, so `GPTBot/1.0 DuckDuckBot` emits its ua-log event instead of being swallowed by the longer allow needle. Applebot / Applebot-Extended verdicts are unchanged and now pinned by tests. parity.py: restore the exact-200 sitemap check (_OPENER returns for any 2xx, so a 204 yielded an empty corpus and the gate passed having probed nothing) and route the sitemap request through a single credential-attaching constructor, so --sitemap-host against the Access-gated canary no longer 403s on every sitemap. Secrets: --access-id/--access-secret removed from both gates in favour of the paired env vars. An argv value is readable from the process table and captured by `set -x`; no call site used the flags. smoke.py reads --critical-list via Path.read_text() rather than leaking a file handle. Tests: 115 -> 138 worker tests, 46 -> 50 gate tests. Every new behavioural test was verified to fail against the pre-fix source. Advances: IS-572 * test: IS-572: compare the forbidden UA token case-insensitively bestMatch() lowercases every ua-policy.json entry before matching, so a "google-extended" entry would be functionally identical to the "Google-Extended" token this guard exists to keep out. toContain() compares primitives by strict equality, so the lowercase variant passed the assertion and would have quietly restored the entry. Lowercase both sides so the assertion matches the matcher's own case semantics. Verified by injecting a lowercase "google-extended" into the log list: the test now fails, and passed before this change. Advances: IS-572 * fix: IS-572: scope the CF Access token per host; correct range + precondition semantics Round-3 adversarial review findings. Each fix has a test that fails against 5b4f40f9. parity.py sent the CF Access service token to every host the run touched, which pre-cutover includes --sitemap-host docs.vyos.io — still served by ReadTheDocs. Credentials are now modelled as an Access record bound to --probe-host, and build_request() attaches them per destination rather than per run, so the token reaches only the host it belongs to while the post-cutover same-host configuration stays credentialed. The apex Worker's range handling assumed its own regex agreed with R2's parser. It does not: R2 accepts ASCII space only, this one accepts \s, so a tab-separated Range parsed here, was ignored there, and produced a 206 carrying the whole object in answer to a request for three bytes. classifyRangeHeader now returns the concrete bounds it derived and the Worker checks them against the bytes R2 actually returned before promising a 206 — any parser divergence degrades to a 200 instead of corrupting a resumed download. Range positions are compared as digit strings, so specs above 2^53 no longer collapse into each other, and Range is ignored outright on every method but GET (§14.2). If-Range was silently dropped: R2Conditional carries no such validator, so a stale range request was answered with bytes of the new representation under a 206. It is now evaluated against the object's own ETag, with a failed validator serving the complete representation. Preconditions were inferred from which headers were present, which cannot be right in both directions — If-Match satisfied plus If-None-Match satisfied owed a 304 and got a 412. The conditionals are filtered to those §13.2.2 applies to this request before R2 sees them, and a body-less result is resolved by evaluating the validators in §13.2.2 order. Also drops the UA gate's narrow-allow carve-out. It compared the two matched policy entries rather than the UA's token structure, so a request-controlled UA quoting both tokens bought itself an allow. No pair in ua-policy.json used it. 🤖 Generated by [robots](https://vyos.io) * fix: IS-572: cancel abandoned R2 body streams; drop stale --access-* comments CodeRabbit round-3 follow-up. R2 hands back a body on two paths whose response carries none, and dropping the reference leaves the stream open holding its connection until GC. CodeRabbit flagged the If-Range re-read; the larger instance is the 416, where R2 answers an unsatisfiable Range with the COMPLETE object — 29.2 MiB for the 1.3 PDF — and the response sends none of it. Both now go through discardBody(), which cancels so the transfer is aborted rather than drained, and swallows failures because the response on those paths is already decided. Keeping the get-then-re-read rather than switching to head()-then-get(): a head() would avoid opening the slice stream, but it would add a round-trip to the path where If-Range MATCHES — the normal resumed download — to tidy the rare path where it does not, and it would open a TOCTOU window that the current shape does not have, since here the object whose validator was checked is the very response served. The R2 test mock returned string bodies, which is why this class of bug was invisible to it. Bodies are now real ReadableStreams that record their own cancellation, so the three new tests observe the actual stream lifecycle: the 416 cancels the whole object, the stale If-Range cancels the discarded slice, and a served body is never cancelled. Also corrects two comments that still described --access-id/--access-secret as a manual fallback; the flags were removed in round 2 and argparse rejects them. A repo-wide sweep found no other prose describing the removed interface. The docs-build.yml change is one comment line inside an env: block; check_head is untouched. 🤖 Generated by [robots](https://vyos.io) * fix: IS-572: compare Access origins; ignore conditionals per method Round-4 adversarial review findings. parity.py: _authority() compared (hostname, port) verbatim, so `p.invalid`, `p.invalid:443` and the trailing-dot FQDN form were three different origins. Post-cutover both --sitemap-host and --probe-host name the same Access-gated host; spelling the default port out on either cost the sitemap fetch its token and 403'd it, reverting the same-host keeper case. Compare normalized origins instead: lowercased host, single trailing dot stripped, the scheme's default port folded to None (bare arguments read under _SCHEME). IDN/punycode equivalence stays out of scope -- both hosts are ASCII literals passed by CI. apex worker: applicablePreconditions() filtered conditionals by validator applicability but not by method, so RFC 9110 13.2.1 -- "a server MUST ignore the conditional request header fields ... such as CONNECT, OPTIONS, or TRACE" -- was unmet: an OPTIONS carrying a stale If-Match was refused 412 where the same request without it succeeded. Take the method and return an empty set for those three; R2 is then given nothing to evaluate. apex worker: the stale-If-Range re-read was a bare get(), dropping the other preconditions, so a key rewritten between the two reads was served under a 200 though the request's If-Match excluded it. The legacy snapshot deploy's force_pdf_refresh input does rewrite this key. Carry the same onlyIf into the re-read and resolve a body-less result through preconditionStatus() on the new object's validators. Advances: IS-572 * fix: IS-572: scope the Access token by origin, not authority; hide the secret from repr Round-5 CodeRabbit findings, both on the CF Access credential primitive in scripts/docs_gates/parity.py. _authority() returned (hostname, port) and dropped the scheme, so an https credential also applied to the plaintext http URL of the same name -- Access("p.invalid", ...).applies_to("http://p.invalid/") was True, and build_request() would then have attached CF-Access-Client-Id/-Secret to a cleartext request. `http://p.invalid:80/` is the sharp case: 80 folds to None under http, matching the https-scoped ("p.invalid", None) exactly. Not reachable in current callers: every URL in the module is built from _SCHEME and both workflow invocations pass bare hosts, so a single run is single-scheme and the http value exists only for the test monkeypatch. Fixed as a contract defect regardless -- applies_to() documents itself as comparing ORIGINs, an origin is scheme+host+port, and over-broad credential scope is the failure mode this primitive already produced once (round 2, token sent to docs.vyos.io). Return (scheme, host, port), resolving a bare argument's scheme under _SCHEME as before, and fold the default port against the origin's own scheme so http :80 and https :443 stay distinct rather than collapsing together. The four origin-normalization cases and the :8443 denial are unchanged. Access.client_secret is now dataclasses.field(repr=False). The default dataclass repr rendered every field, so a failed assertion, a debug print or any exception interpolating an Access would have put the service token into CI output, which is durable and world-readable for this repo. The client id stays in repr: it names which token without being the credential, and dropping it would make a scoping failure much harder to diagnose. The secret remains readable as a field, which is the only way it is ever consumed. Swept the rest of scripts/docs_gates/ for the same exposure: smoke.Probe holds no credential fields, smoke passes the id/secret as plain locals that reach no output path, and the str(e) texts recorded in parity-report.json and the SMOKE-FAIL lines carry urllib's code/reason/URL, never request headers. Two tests added, both verified failing against the pre-fix source. Advances: IS-572 🤖 Generated by [robots](https://vyos.io) * fix: IS-572: roll back to the newest deployment's 100%-traffic version id `wrangler deployments list --json` sorts ASCENDING by created_on (wrangler 4.123.0 versionsDeploymentsListHandler), so `.[0]` selected the OLDEST deployment, not the current one the stale comment claimed. It also took the deployment `id`, but `wrangler rollback` resolves its positional through fetchVersion() and needs a VERSION id. Select newest-first by created_on and take the version_id serving 100% of traffic, walking to the next-newest deployment when traffic is split across versions -- mirroring wrangler's own fetchDefaultRollbackVersionId(), minus its .shift() (it picks after deploying; we capture before). Non-array or unparseable output now fails safe to an empty id instead of a wrong one. Rewrite the stale comment to describe the actual JSON shape, note in the consumer that rollback_id is a version id, and add --message for rollback audit context (supported at 4.123.0; --yes supplies it non-interactively). Advances: IS-572 🤖 Generated by [robots](https://vyos.io) * fix: IS-572: re-check the branch tip after smoke; harden rollback capture Two production-safety fixes in docs-build.yml. 1. TOCTOU on the check_head guard. It resolved the branch tip ONCE, before the candidate deploy and before the smoke gate (480s deadline), then gated PROMOTE, the post-promote probe and the registry pointer publish on that minutes-old answer. A push landing in that window left the run promoting a superseded SHA to production and publishing a pointer naming it. `cancel-in-progress: false` does not cover this: it makes the SECOND run queue and skip, while the run already past the check sails on. Add a second guard after smoke, immediately before the first production-facing step, and move PROMOTE + probe + pointer publish onto its output. The probe must move with PROMOTE: left on the first check it would run after a skipped promote, see production serving the previous SHA, read that as a promote failure and roll back a production nobody touched. A branch that moved mid-run ends GREEN-and-skipped, matching the convention the exit-78 redesign established: production and the registry are untouched, and the queued run promotes the new tip, so there is nothing for an operator to act on. The residual check-to-deploy window is one step boundary; no compare-and-swap exists on the Cloudflare side to close it entirely. 2. The rollback-capture bootstrap branch swallowed every failure. `2>/dev/null` plus a bare `if` could not tell "Worker does not exist yet" from auth, network or malformed output, so a transient failure yielded an empty rollback_id, logged "nothing to roll back to" and promoted with the auto-rollback silently disarmed while the run looked healthy. Only a genuine not-found may now produce an empty id. wrangler 4.123.0 exits 1 on every error path and writes nothing machine-readable to stdout when the command fails, so the discriminator is the API error code it renders into stderr: 10007/10090, the pair its own isWorkerNotFoundError() uses. Auth carries 9106/10000 and connectivity failures carry no code, so neither can be mistaken for bootstrap. Anything unmatched fails the step. Non-array output now fails instead of being coerced to []; an empty list and a split-traffic list are reported distinctly (notice / warning) rather than sharing one ambiguous message with the failure paths. The code pair was read out of the shipped 4.123.0 bundle, not observed against a live missing Worker; the comment says so and asks for re-verification on wrangler bumps. Also corrects two comments the change falsifies: the concurrency note claiming queued runs are covered by "the check_head guard", and the exit-78 note claiming promote/registry gate on the first check. Advances: IS-572 🤖 Generated by [robots](https://vyos.io) * fix: IS-572: run the post-promote probe whenever the deploy succeeded PROMOTE ran `wrangler deploy` and then the hostname purge, the tarball and three `r2 object put` calls in one step, while the post-promote probe named no status function in its `if:` and so carried an implicit success(). A transient purge 5xx, a tar error or one failed upload therefore failed PROMOTE, skipped the probe entirely, and left the new version LIVE on the production Worker, never verified and never rolled back. Split the step at the deploy. Everything fallible after it moves to a new `promote_publish` step gated on `steps.promote.outcome == 'success'`, and the probe becomes the verification finalizer: `!cancelled() && steps.promote.outcome == 'success'`. `!cancelled()` counts as a status function, which is what suppresses the implicit success() and lets the probe run after a failed purge/upload; it is not `always()`, because firing an auto-rollback while an operator tears the run down is the wrong reflex. Gating on PROMOTE's outcome rather than re-reading check_head_2 is strictly stronger: 'skipped' on the moved-branch path, 'success' only once the production deploy completed. The registry-pointer publish keeps its bare condition, whose implicit success() is now load-bearing — it is what stops a pointer from naming a rolled-back generation. Rollback-target capture no longer reaches backwards. The old jq flattened `versions[]` across ALL deployments and took the first 100%-traffic id found scanning newest→oldest, so a current deployment deliberately serving A/B at 90/10 resolved to an older deployment's version — and a probe failure would "roll back" to it and destroy the split. It now inspects only the current deployment and refuses otherwise. The verdict is discriminated inside jq so a malformed record fails red instead of masquerading as split traffic: `select(.percentage == 100) | .version_id` on `{"percentage":100}` yielded null, `null // empty` yielded empty, and empty was read as "split" — an unparseable API response promoting with the auto-rollback silently disarmed. Non-numeric `percentage`, empty/absent `versions` and unsortable `created_on` are rejected for the same reason. The hostname purge is retried once. It is the one post-deploy failure with an asymmetric knock-on: the probe measures the edge, so a purge that never lands makes a healthy deploy look like a failed promote. Finally, the apex PDF range-divergence fallback no longer answers with a 206 carrying bounds the client never requested (RFC 9110 §14.4). It discards the slice and fails 503, keeping the r2-range-divergence log line that is the actual alarm. Unreachable under today's workerd, which ignores multi-range. Advances: IS-572 🤖 Generated by [robots](https://vyos.io) * ci: IS-572: bind DOCS_CF_LIVE via env: in the purge/registry step The purge branch expanded ${{ vars.DOCS_CF_LIVE }} directly into the shell body while the other three consumers of the same value bind it through env: (PDF carry-forward, candidate reset, post-promote probe). zizmor flags the inline form as template expansion; the value is repo-controlled so it is not exploitable, but the env form removes the class and matches the surrounding steps. * ci: IS-572: validate DOCS_CF_LIVE, pin wrangler, bound rollback percentages Three Major findings from CodeRabbit review 4971096766, all outside-diff. DOCS_CF_LIVE failed open. Four steps branch on it — the SKIP_PDF carry-forward staleness check, the candidate-reset staleness check, the production hostname purge and the post-promote probe — and every one compares against the literal string "true", so an unset or misspelled repository variable silently took the branch that skips the check, skips the purge and skips the probe, after which the registry pointer published an unpurged, never-verified deployment on a green run. Validate the value once at the top of the only job that reads it, so all four comparisons are safe by construction instead of each re-deriving the guard. `npx wrangler` in the previous-build metadata fetch ran before `npm ci`, so on the normal (non-SKIP_PDF) path it downloaded whatever version the registry served rather than the lockfile-pinned 4.123.0 every later step uses. Move the install ahead of the first wrangler invocation and make it unconditional; the inline `npm ci` in the SKIP_PDF branch and the conditional install step after the check_head guard are both redundant once it is. The rollback-target jq required `percentage` to be numeric but not in range, so a current deployment reporting both 100 and 101 yielded exactly one 100%-traffic match and classified as a determinate `ok` — arming a rollback with a version selected from data already known to be nonsense. Out-of-range now routes to the malformed path, never to `split:`. Advances: IS-572 * ci: IS-572: validate rollback_id at capture, bind it via env at the finalizer rollback_id originates from Cloudflare's API (`wrangler deployments list --json`), so it is untrusted text. It was written to $GITHUB_OUTPUT unvalidated and then reached shell source through a template expansion at three sites in the post-promote finalizer. The $GITHUB_OUTPUT write is the sharper of the two exposures: a value containing a newline injects arbitrary ADDITIONAL step outputs regardless of how carefully the consumer quotes, because no shell is involved. Validate at capture — the only place that can stop it. The character class is deliberately wider than the observed format. wrangler 4.123.0 performs no client-side validation of a version id (it interpolates the string straight into the API URL path), so the canonical UUID shape is what Cloudflare's bundled SDK documents rather than something the CLI enforces. Hard-failing on anything but a UUID would turn a Cloudflare format change into a blocked promote; hard-failing outside [A-Za-z0-9._-] cannot, and still certainly excludes newlines, quotes, whitespace and every shell metacharacter. A safe-but-non-UUID value warns instead of erroring. Empty stays legitimate — it is the no-target state of all three benign paths (first deploy, no deployments, split traffic) and the finalizer's -z branch depends on it. A malformed non-empty value fails loudly, matching how the surrounding capture already treats malformed API data. At the finalizer, ROLLBACK_ID is bound through the step's env: mapping and the three sites use "$ROLLBACK_ID", so the value is never substituted into the shell source. The -z semantics are unchanged. Advances: IS-572 * ci: IS-572: verify probe status, reconcile a failed pointer publish Two merge blockers from the pre-merge adversarial gate on d079a2c9. 1. The post-promote probe accepted an error response as verification. `withDocsHeaders` sets X-Docs-Build unconditionally, so error responses carry it too; the probe read only that header and never the status line, so a production answering 500 with the new SHA satisfied the match and the registry pointer published for a generation serving errors. The probe now parses the status line and requires an exact 200 alongside the built SHA. Parsing resets on every status line, so 1xx interim responses contribute nothing and only the final response block is read; duplicate X-Docs-Build headers with identical values are accepted while conflicting ones surface as "<conflicting>" and can never match. Requests are bounded with --max-time 20 and the pipeline runs under pipefail so a transport failure is distinguishable from a missing header. Early-attempt non-200s and transport failures stay retryable — only the final attempt's verdict decides. Every unverified outcome still rolls back, but the three causes now carry distinguishable diagnoses. --fail is deliberately not used: it collapses 4xx/5xx into curl exit 22 and discards the status line, which is the value needed to tell a broken new version apart from a transport failure. 2. Production and the registry could diverge permanently. The round-8 promote/publish split correctly decoupled a publish failure from the probe, but added no convergence action for the case where the probe then succeeds: production served the new SHA while latest.json still named the old one, with nothing to reconcile it. latest.json is the source of truth for the candidate-reset staleness check and the SKIP_PDF carry-forward, so both would later restore a generation production no longer serves. A reconciliation step now runs when the promote and probe both succeeded but the pointer did not publish. It converges FORWARD — re-uploading the sha-scoped generation and only then publishing the pointer, so the pointer can never name an incomplete generation — and on failure leaves the pointer untouched and reports the divergence by name with both SHAs and a copy-pasteable manual repair. Gating on the probe's success keeps it off the rollback paths entirely, where production and the pointer already agree. The pointer step's load-bearing implicit success() is unchanged. Refs: IS-572 * ci: IS-572: drop registry-pointer reconciliation, keep the probe fix Scope reduction. The `Reconcile registry pointer after a failed publish` step introduced in the previous commit is removed; the post-promote probe rewrite in that same commit stays. Removing the step returns registry-divergence behaviour to what `rolling` has today: when `promote_publish` fails after a successful deploy, the pointer step's load-bearing implicit success() skips it, and production can end up serving a SHA that `$slug/latest.json` does not name. That path is now documented at the `Publish registry pointer` step and tracked in https://vyos.dev/T9237 rather than repaired here. Also reverts the DOCS_CF_LIVE header comment to "four later steps", which restores its internal agreement with the "four comparisons downstream" sentence in the same block. Advances: IS-572 🤖 Generated by [robots](https://vyos.io)
Diffstat (limited to 'scripts/docs_gates')
-rw-r--r--scripts/docs_gates/gates.py7
-rw-r--r--scripts/docs_gates/parity.py160
-rw-r--r--scripts/docs_gates/smoke.py44
-rw-r--r--scripts/docs_gates/test_gates.py18
-rw-r--r--scripts/docs_gates/test_parity.py270
-rw-r--r--scripts/docs_gates/test_smoke.py117
6 files changed, 592 insertions, 24 deletions
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"]