diff options
Diffstat (limited to 'scripts/docs_gates')
| -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 |
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"] |