From fc84c15c32f8c9be70e8755d18bd742842fd6002 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 20:48:11 -0700 Subject: [PATCH 1/7] Announce synthetic traffic with X-Blog-Check on every live-check request 97.7% of the traffic reaching production is ours, and the only thing separating it from real visitors was a coincidence: the CI runner's curl and the VPS host's curl are byte-identical at 8.5.0, so user agent cannot tell our deploy gate from their smoke probe, and the CI half of the client address rotates every run. One runner image bump merges the two and nothing would report it. The host side captures the field already and their ci/smoke.sh sends vps/smoke. This is the other half. The value carries provenance rather than a boolean, /, so "which run produced this 404" is one query rather than a correlation across timestamps. Derived rather than configured: github/- under Actions, proxmox/manual elsewhere, and CHECK_TAG overrides both to name a purpose for a hand run. The run attempt is in the id deliberately, which is a refinement on the design as proposed. A re-run of a failed workflow keeps its GITHUB_RUN_ID and takes a new GITHUB_RUN_ATTEMPT, so the id alone merges a retry into the run it was retrying, and that is exactly the case someone reads the log to understand. It is two curl config files rather than one, and that is the part not to collapse later. The tag is unconditional; the Pangolin token is sent only to the origin it belongs to, because a redirect that one day points off-site must not mail the credential there. Folding them together would either give the tag that restriction for no reason or take it away from the token. Both are now assembled from the tag first, with the token appended where it is allowed, so every request is attributable including the off-site hop that deliberately carries no credential. Verified rather than assumed: the header is on the wire under curl -v, all three derivations produce the expected value, the full run still reports PASS - 1253 URLs honored, and two tagged requests were sent to production for the host side to confirm capture. CHECK_TAG is added to the env-docs gate's KNOBS and described in ENVIRONMENT.md, and the gate was watched failing on it before the row was written. Design agreed with the VPS agent in their section 31. Co-Authored-By: Claude Opus 5 (1M context) --- ENVIRONMENT.md | 1 + checks/check-env-docs.py | 9 ++++++- checks/check-live-urls.sh | 56 ++++++++++++++++++++++++++++++--------- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/ENVIRONMENT.md b/ENVIRONMENT.md index c9bfdd9..21b05d8 100644 --- a/ENVIRONMENT.md +++ b/ENVIRONMENT.md @@ -89,6 +89,7 @@ Set on the command line for one run rather than stored anywhere. | `NO_LINK_DEST=1` | full copy instead of hard-linking from the previous release | | `KEEP_RELEASES` | how many releases `make-release.sh` leaves behind | | `EXPECT_RELEASE` | the release id `check-live-urls.sh` requires the live site to report, which is what makes a rollback verifiable rather than merely exiting zero | +| `CHECK_TAG` | the `X-Blog-Check` provenance this run announces on every request, as `/`. Rarely set by hand: `check-live-urls.sh` derives `github/-` under Actions and `proxmox/manual` elsewhere. Set it to name a purpose for a hand run, as `proxmox/media-dev` | ## Two credentials to the VPS, and why they are separate diff --git a/checks/check-env-docs.py b/checks/check-env-docs.py index e2ed0f5..e22891d 100755 --- a/checks/check-env-docs.py +++ b/checks/check-env-docs.py @@ -40,7 +40,14 @@ # Set per invocation rather than stored, so they appear in no template and would otherwise # be invisible to this check. Listed here because the doc has a table for them, and a knob # nobody documented is the same failure as an undocumented file value. -KNOBS = {"ENV_FILE", "REQUIRE_BROTLI", "NO_LINK_DEST", "KEEP_RELEASES", "EXPECT_RELEASE"} +KNOBS = { + "ENV_FILE", + "REQUIRE_BROTLI", + "NO_LINK_DEST", + "KEEP_RELEASES", + "EXPECT_RELEASE", + "CHECK_TAG", +} # Names that look like configuration to the patterns above but are not. # ENVIRONMENT and RELEASE_ID are computed inside the workflow and passed down, and diff --git a/checks/check-live-urls.sh b/checks/check-live-urls.sh index 0bafda1..0930d1e 100755 --- a/checks/check-live-urls.sh +++ b/checks/check-live-urls.sh @@ -40,7 +40,33 @@ done FAILED="$(mktemp)" CURLERR="$(mktemp)" CURLRC="" -trap 'rm -f "$FAILED" "$CURLERR" ${CURLRC:+"$CURLRC"}' EXIT +CHECKRC="$(mktemp)" +trap 'rm -f "$FAILED" "$CURLERR" "$CHECKRC" ${CURLRC:+"$CURLRC"}' EXIT + +# Every request this script makes announces itself as synthetic, so the server's log can be +# filtered down to real visitors with one clause. Agreed with the host side, whose Traefik +# captures the field and whose own `ci/smoke.sh` already sends `vps/smoke`. +# +# The value carries provenance rather than a boolean, `/`, because "which run +# produced this 404" is then a one-line query against the log. +# +# The run attempt is part of the id deliberately. A re-run of a failed workflow keeps the +# same GITHUB_RUN_ID and gets a new GITHUB_RUN_ATTEMPT, so the id alone would merge a +# retried run into the run it was retrying, which is exactly the case someone reads the log +# to understand. +# +# It is forgeable and it gates nothing. Absence of the header is not proof of a human +# either: a scanner sends no header and neither does a forged request. It must never reach +# auth, rate limiting, robots handling, or caching. +if [ -z "${CHECK_TAG:-}" ]; then + if [ -n "${GITHUB_RUN_ID:-}" ]; then + CHECK_TAG="github/${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT:-1}" + else + CHECK_TAG="proxmox/manual" + fi +fi +printf 'header = "X-Blog-Check: %s"\n' "$CHECK_TAG" >"$CHECKRC" +echo "==> tagging requests X-Blog-Check: $CHECK_TAG" # A resource access token opens the proxy's auth gate. # It goes into a curl config file because bash cannot export an array to the parallel checks. @@ -58,14 +84,18 @@ elif [ -n "${PANGOLIN_ACCESS_TOKEN_ID:-}" ] || [ -n "${PANGOLIN_ACCESS_TOKEN:-}" fi # Assembled once here rather than per request, since it is the same for every call. -AUTH=() -[ -n "$CURLRC" ] && AUTH=(-K "$CURLRC") +# The check tag is unconditional and the token is not, which is why they are two files +# rather than one. Every request should be attributable; only a same-origin request may +# carry the credential, and folding them together would make the tag inherit that +# restriction for no reason, or the token lose it, depending on which way it was folded. +AUTH=(-K "$CHECKRC") +[ -n "$CURLRC" ] && AUTH+=(-K "$CURLRC") # Invoked indirectly, through `export -f` and the `xargs bash -c` calls below. # shellcheck disable=SC2329 check_render() { - local url="$1" code auth=() - [ -n "$CURLRC" ] && auth=(-K "$CURLRC") + local url="$1" code auth=(-K "$CHECKRC") + [ -n "$CURLRC" ] && auth+=(-K "$CURLRC") code=$(curl -s -o /dev/null -w '%{http_code}' --max-time 30 "${auth[@]}" "$BASE$url") [ "$code" = "200" ] || echo "render $url expected 200, got $code" >>"$FAILED" } @@ -81,8 +111,8 @@ check_render() { # arrived truncated to nothing, which still answers 200. Content type is asserted because a # server misconfigured into serving an error page for a missing asset answers 200 as well. check_media() { - local url="$1" code len type target auth=() target_auth=() - [ -n "$CURLRC" ] && auth=(-K "$CURLRC") + local url="$1" code len type target auth=(-K "$CHECKRC") target_auth=() + [ -n "$CURLRC" ] && auth+=(-K "$CURLRC") target="$BASE$url" target_auth=("${auth[@]}") # One hop is followed rather than passed to curl -L, because -L would carry the @@ -102,10 +132,10 @@ check_media() { # Same origin boundary as check_redirect, and for the same reason: a rule that one # day points off-site must not mail the token there. A bare prefix would also accept # a lookalike host registered as an attacker's subdomain. - target_auth=() + target_auth=(-K "$CHECKRC") if [ -n "$CURLRC" ]; then case "$target" in - "$BASE" | "$BASE"/*) target_auth=(-K "$CURLRC") ;; + "$BASE" | "$BASE"/*) target_auth+=(-K "$CURLRC") ;; esac fi ;; @@ -144,8 +174,8 @@ check_media() { # Invoked indirectly, the same way as check_render above. # shellcheck disable=SC2329 check_redirect() { - local url="$1" code dest dcode auth=() dest_auth=() - [ -n "$CURLRC" ] && auth=(-K "$CURLRC") + local url="$1" code dest dcode auth=(-K "$CHECKRC") dest_auth=(-K "$CHECKRC") + [ -n "$CURLRC" ] && auth+=(-K "$CURLRC") code=$(curl -s -o /dev/null -w '%{http_code}' --max-time 30 "${auth[@]}" "$BASE$url") case "$code" in 301 | 308) ;; @@ -162,7 +192,7 @@ check_redirect() { # starts with this one, such as a lookalike registered as an attacker's subdomain. if [ -n "$CURLRC" ]; then case "$dest" in - "$BASE" | "$BASE"/*) dest_auth=(-K "$CURLRC") ;; + "$BASE" | "$BASE"/*) dest_auth+=(-K "$CURLRC") ;; esac fi dcode=$(curl -s -o /dev/null -w '%{http_code}' --max-time 30 "${dest_auth[@]}" "$dest") @@ -174,7 +204,7 @@ check_redirect() { } export -f check_render check_redirect check_media -export BASE FAILED CURLRC +export BASE FAILED CURLRC CHECKRC echo "==> $BASE" From ee1e3931b637e7dda00373cee98c63e805282fd3 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 20:53:38 -0700 Subject: [PATCH 2/7] Validate what goes into the curl config, since a config is options not headers A curl config file is a list of directives rather than a list of headers, so a value interpolated into a quoted `header = "..."` line can leave it. A newline ends the line and starts a new directive; a double quote closes the string with the same effect. Either turns an override into "add a curl option nobody typed". CHECK_TAG takes a strict allowlist, because this repo defines its grammar: the `/` the design already states, so letters, digits, dot, underscore, hyphen and the separating slash. Anything else fails at entry. The Pangolin token values take a narrower rule, refusing only a quote or a newline, because the grammar of a credential belongs to its issuer and not to this script. Neither character is legal in an HTTP header value, so a token carrying one is a paste accident rather than a token. The failure names the variable and never echoes the value. Each guard was demonstrated failing rather than assumed: CHECK_TAG=$'proxmox/x\noutput = /tmp/pwned' exit 2 CHECK_TAG='proxmox/x" header = "X-Evil: 1' exit 2 CHECK_TAG='proxmox/media dev' exit 2 PANGOLIN_ACCESS_TOKEN_ID=$'a\nb' exit 2 And that exercise found one of my own: I had added a guard rejecting an empty CHECK_TAG, and it is unreachable. An empty value is already treated as unset and takes the derived default, which is the behaviour we want, so the guard could never fire and its message told the reader the opposite of what happens. Removed rather than left as a rule describing a case that cannot occur. Full run still reports PASS - 1253 URLs honored. shellcheck and shfmt clean. Found by Copilot review on #73. Co-Authored-By: Claude Opus 5 (1M context) --- checks/check-live-urls.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/checks/check-live-urls.sh b/checks/check-live-urls.sh index 0930d1e..fd4612c 100755 --- a/checks/check-live-urls.sh +++ b/checks/check-live-urls.sh @@ -65,6 +65,20 @@ if [ -z "${CHECK_TAG:-}" ]; then CHECK_TAG="proxmox/manual" fi fi +# Validated before it is written, because this lands in a curl config file and a curl config +# file is a list of options rather than a list of headers. A value carrying a newline ends the +# header line and starts a new directive, so an override could add an option nobody typed; a +# value carrying a double quote ends the quoted string with the same result. Neither is a +# legal HTTP header value either, so refusing both loses nothing. +# +# The grammar is the one the design already states, `/`, kept deliberately narrow: +# letters, digits, dot, underscore, hyphen, and the slash that separates the two halves. +case "$CHECK_TAG" in +*[!A-Za-z0-9._/-]*) + echo "FAIL CHECK_TAG may contain only letters, digits, and . _ - / -- got '$CHECK_TAG'" >&2 + exit 2 + ;; +esac printf 'header = "X-Blog-Check: %s"\n' "$CHECK_TAG" >"$CHECKRC" echo "==> tagging requests X-Blog-Check: $CHECK_TAG" @@ -72,6 +86,19 @@ echo "==> tagging requests X-Blog-Check: $CHECK_TAG" # It goes into a curl config file because bash cannot export an array to the parallel checks. # A command line is also world-readable in ps output, and this runs 1,245 of them. if [ -n "${PANGOLIN_ACCESS_TOKEN_ID:-}" ] && [ -n "${PANGOLIN_ACCESS_TOKEN:-}" ]; then + # Same hazard as CHECK_TAG above and the same reason, but a narrower rule, because the + # grammar of a credential is the issuer's to define and not this script's. Only the two + # characters that break out of a quoted config line are refused, and neither is legal in + # an HTTP header value, so a token containing one is a paste accident rather than a token. + # Reported without echoing the value, since it is a secret and the finding is its shape. + for name in PANGOLIN_ACCESS_TOKEN_ID PANGOLIN_ACCESS_TOKEN; do + case "${!name}" in + *'"'* | *$'\n'*) + echo "FAIL $name contains a quote or a newline, which cannot appear in an HTTP header value" >&2 + exit 2 + ;; + esac + done CURLRC="$(mktemp)" chmod 600 "$CURLRC" printf 'header = "P-Access-Token-Id: %s"\nheader = "P-Access-Token: %s"\n' \ From fc81267e866efc8f2f68ea3d0fff8ae67e443811 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 20:58:30 -0700 Subject: [PATCH 3/7] Enforce the tag shape the comment already claimed, rather than describing it The validation checked a character set and the comment above it said the grammar was /. Those are different rules, and the weaker one was the one running: `smoke`, `/smoke`, `proxmox/` and `a/b/c` all passed while reading as conforming. That matters because the shape is the whole point of provenance over a boolean. Grouping the log by source is only reliable if every tag has a source half, and a tag with no slash or three slashes breaks the query quietly rather than loudly. Now enforced: exactly one slash, both halves non-empty, from the same narrow character set. Each rule was demonstrated rejecting and the two legal shapes demonstrated passing: smoke must be / /smoke needs a non-empty half either side proxmox/ needs a non-empty half either side a/b/c takes exactly one / proxmox/media dev character set proxmox/media-dev accepted github/999-3 accepted Both derived defaults were re-checked against their own rule rather than assumed to satisfy it, which is the failure mode of adding a validator after the values it governs. ENVIRONMENT.md now states the shape is enforced instead of expected. Full run still PASS - 1253 URLs honored. Found by Copilot review on #73, as a suppressed comment. Co-Authored-By: Claude Opus 5 (1M context) --- ENVIRONMENT.md | 2 +- checks/check-live-urls.sh | 20 ++++++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/ENVIRONMENT.md b/ENVIRONMENT.md index 21b05d8..910f0c9 100644 --- a/ENVIRONMENT.md +++ b/ENVIRONMENT.md @@ -89,7 +89,7 @@ Set on the command line for one run rather than stored anywhere. | `NO_LINK_DEST=1` | full copy instead of hard-linking from the previous release | | `KEEP_RELEASES` | how many releases `make-release.sh` leaves behind | | `EXPECT_RELEASE` | the release id `check-live-urls.sh` requires the live site to report, which is what makes a rollback verifiable rather than merely exiting zero | -| `CHECK_TAG` | the `X-Blog-Check` provenance this run announces on every request, as `/`. Rarely set by hand: `check-live-urls.sh` derives `github/-` under Actions and `proxmox/manual` elsewhere. Set it to name a purpose for a hand run, as `proxmox/media-dev` | +| `CHECK_TAG` | the `X-Blog-Check` provenance this run announces on every request. **`/` is enforced, not merely expected**: exactly one `/`, both halves non-empty, and only letters, digits, `.`, `_`, `-`. Rarely set by hand, since `check-live-urls.sh` derives `github/-` under Actions and `proxmox/manual` elsewhere. Set it to name a purpose for a hand run, as `proxmox/media-dev` | ## Two credentials to the VPS, and why they are separate diff --git a/checks/check-live-urls.sh b/checks/check-live-urls.sh index fd4612c..99a82de 100755 --- a/checks/check-live-urls.sh +++ b/checks/check-live-urls.sh @@ -71,13 +71,29 @@ fi # value carrying a double quote ends the quoted string with the same result. Neither is a # legal HTTP header value either, so refusing both loses nothing. # -# The grammar is the one the design already states, `/`, kept deliberately narrow: -# letters, digits, dot, underscore, hyphen, and the slash that separates the two halves. +# The shape is enforced and not merely described, because the whole value of provenance over a +# boolean is that the log can be grouped by source, and `select(.tag | startswith("github/"))` +# is only reliable if every tag actually has a source half. A charset check alone would accept +# `smoke`, `/smoke` and `a/b/c`, each of which reads as conforming and breaks that query. +# Exactly one slash, both halves non-empty, from a deliberately narrow character set. case "$CHECK_TAG" in *[!A-Za-z0-9._/-]*) echo "FAIL CHECK_TAG may contain only letters, digits, and . _ - / -- got '$CHECK_TAG'" >&2 exit 2 ;; +*/*/*) + echo "FAIL CHECK_TAG takes exactly one / , as / -- got '$CHECK_TAG'" >&2 + exit 2 + ;; +/* | */) + echo "FAIL CHECK_TAG needs a non-empty half either side of the / -- got '$CHECK_TAG'" >&2 + exit 2 + ;; +*/*) ;; +*) + echo "FAIL CHECK_TAG must be / , such as proxmox/media-dev -- got '$CHECK_TAG'" >&2 + exit 2 + ;; esac printf 'header = "X-Blog-Check: %s"\n' "$CHECK_TAG" >"$CHECKRC" echo "==> tagging requests X-Blog-Check: $CHECK_TAG" From 84db125575f04954bf243b5811b101c5ead1e659 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 21:02:43 -0700 Subject: [PATCH 4/7] Rewrite the four failure messages so they read as prose A stray space before a comma, in `one / , as /` and in `/ , such as`, which reads as a typo in the one place a reader is already confused. It came from trying to keep a bare `/` from running into the punctuation after it. Fixed by naming the character instead of printing it: "takes exactly one slash" and "either side of the slash" have nothing to collide with. The example message loses the space and keeps the comma. The character-set message had the same collision in a different form, `. _ - / -- got`, where the slash ran into the separator. The set is quoted now rather than bare. All four read back as a user sees them: may contain only letters, digits, and the characters '. _ - /' -- got 'proxmox/media dev' takes exactly one slash, as / -- got 'a/b/c' needs a non-empty half either side of the slash -- got '/smoke' must be /, such as proxmox/media-dev -- got 'smoke' Found by Copilot review on #73, which also pointed out the second instance rather than only the first. Co-Authored-By: Claude Opus 5 (1M context) --- checks/check-live-urls.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/checks/check-live-urls.sh b/checks/check-live-urls.sh index 99a82de..5dc9077 100755 --- a/checks/check-live-urls.sh +++ b/checks/check-live-urls.sh @@ -78,20 +78,20 @@ fi # Exactly one slash, both halves non-empty, from a deliberately narrow character set. case "$CHECK_TAG" in *[!A-Za-z0-9._/-]*) - echo "FAIL CHECK_TAG may contain only letters, digits, and . _ - / -- got '$CHECK_TAG'" >&2 + echo "FAIL CHECK_TAG may contain only letters, digits, and the characters '. _ - /' -- got '$CHECK_TAG'" >&2 exit 2 ;; */*/*) - echo "FAIL CHECK_TAG takes exactly one / , as / -- got '$CHECK_TAG'" >&2 + echo "FAIL CHECK_TAG takes exactly one slash, as / -- got '$CHECK_TAG'" >&2 exit 2 ;; /* | */) - echo "FAIL CHECK_TAG needs a non-empty half either side of the / -- got '$CHECK_TAG'" >&2 + echo "FAIL CHECK_TAG needs a non-empty half either side of the slash -- got '$CHECK_TAG'" >&2 exit 2 ;; */*) ;; *) - echo "FAIL CHECK_TAG must be / , such as proxmox/media-dev -- got '$CHECK_TAG'" >&2 + echo "FAIL CHECK_TAG must be /, such as proxmox/media-dev -- got '$CHECK_TAG'" >&2 exit 2 ;; esac From cd95e28dc63e77e6d12cfbf925f8279fd4b6c12f Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 21:06:12 -0700 Subject: [PATCH 5/7] Say which characters apply to which part of the tag The ENVIRONMENT.md row required "exactly one /" and then said the value may contain "only letters, digits, . _ -", a set with no slash in it. Read strictly the two clauses contradict; read charitably the reader has to guess that the character set governs the halves rather than the whole. The row now says the slash is the separator and the only one allowed, and that each half is drawn from the character set. That is what the script enforces: the charset check permits a slash anywhere, and the two rules after it leave exactly one, in the separator position, so each half can only contain the rest. Found by Copilot review on #73, as a suppressed comment. Co-Authored-By: Claude Opus 5 (1M context) --- ENVIRONMENT.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ENVIRONMENT.md b/ENVIRONMENT.md index 910f0c9..700d172 100644 --- a/ENVIRONMENT.md +++ b/ENVIRONMENT.md @@ -89,7 +89,7 @@ Set on the command line for one run rather than stored anywhere. | `NO_LINK_DEST=1` | full copy instead of hard-linking from the previous release | | `KEEP_RELEASES` | how many releases `make-release.sh` leaves behind | | `EXPECT_RELEASE` | the release id `check-live-urls.sh` requires the live site to report, which is what makes a rollback verifiable rather than merely exiting zero | -| `CHECK_TAG` | the `X-Blog-Check` provenance this run announces on every request. **`/` is enforced, not merely expected**: exactly one `/`, both halves non-empty, and only letters, digits, `.`, `_`, `-`. Rarely set by hand, since `check-live-urls.sh` derives `github/-` under Actions and `proxmox/manual` elsewhere. Set it to name a purpose for a hand run, as `proxmox/media-dev` | +| `CHECK_TAG` | the `X-Blog-Check` provenance this run announces on every request. **`/` is enforced, not merely expected**: exactly one `/`, which is the separator and the only one allowed, with both halves non-empty and each drawn from letters, digits, `.`, `_`, `-`. Rarely set by hand, since `check-live-urls.sh` derives `github/-` under Actions and `proxmox/manual` elsewhere. Set it to name a purpose for a hand run, as `proxmox/media-dev` | ## Two credentials to the VPS, and why they are separate From 27cce353d23f6a3a21687d5a28c901f5e93c1fdc Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 21:10:44 -0700 Subject: [PATCH 6/7] Set globasciiranges rather than inheriting it, since the allowlist rests on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `A-Za-z0-9` in a shell pattern is a collation range, not an ASCII range, so what the allowlist actually permits depends on a shell option this script never set. It passes today because `globasciiranges` is on by default in bash 5.2, which makes the guarantee an accident of the build. Demonstrated rather than argued, under en_US.UTF-8: shopt -u globasciiranges aé -> ACCEPT aÉ -> ACCEPT shopt -s globasciiranges aé -> REJECT aÉ -> REJECT So a value containing an accented letter would have been written into the curl config on a shell where the option is off, while the comment above it called the set narrow. The consequence was never a config injection: neither a quote nor a newline can arrive this way, and those are the two characters that break out of a quoted config line. What was wrong is the claim. A validator that describes itself as strict has to be strict on its own terms rather than on the terms of whatever shell runs it. Set explicitly, and re-verified against the hostile case: with the option forced off beforehand, in a UTF-8 locale, `proxmox/café` is rejected. Full run still PASS - 1253 URLs honored. shellcheck and shfmt clean. Found by Copilot review on #73, as a suppressed comment. Co-Authored-By: Claude Opus 5 (1M context) --- checks/check-live-urls.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/checks/check-live-urls.sh b/checks/check-live-urls.sh index 5dc9077..dc6fb11 100755 --- a/checks/check-live-urls.sh +++ b/checks/check-live-urls.sh @@ -76,6 +76,13 @@ fi # is only reliable if every tag actually has a source half. A charset check alone would accept # `smoke`, `/smoke` and `a/b/c`, each of which reads as conforming and breaks that query. # Exactly one slash, both halves non-empty, from a deliberately narrow character set. +# +# The range `A-Za-z0-9` is collation-dependent, so the allowlist below is only ASCII-strict +# because `globasciiranges` happens to be on. Set explicitly rather than inherited, since a +# guarantee resting on a build default is not a guarantee. Demonstrated rather than assumed: +# with the option off, under en_US.UTF-8, `aé` and `aÉ` are both ACCEPTED by this pattern, +# and with it on they are rejected. +shopt -s globasciiranges case "$CHECK_TAG" in *[!A-Za-z0-9._/-]*) echo "FAIL CHECK_TAG may contain only letters, digits, and the characters '. _ - /' -- got '$CHECK_TAG'" >&2 From af22557109c85a01c9621e02fd2c97e963cb25c3 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 21:15:29 -0700 Subject: [PATCH 7/7] Reject CR in a token, and fail when globasciiranges cannot be set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, and the second is the more embarrassing. The token guard refused a quote and a newline and allowed a carriage return. Header injection is classically CRLF and a lone CR is enough on its own, so refusing LF while allowing CR leaves exactly the shape the guard exists for. Measured before fixing: a token containing \r was accepted. Both variables now reject all three, and the LF and quote cases were re-run to confirm nothing regressed. And the `shopt -s globasciiranges` added in the previous commit was itself unchecked. This script runs under `set -uo pipefail` and not `-e`, so on a shell without that option the command prints to stderr, returns 1, and execution steps straight over it — leaving the allowlist locale-dependent underneath a comment promising it is not. Which is the same defect the previous commit set out to fix, reintroduced one line below it: a guarantee resting on something nobody checked. `shopt` returns 1 on an unknown option name, verified, so the failure is now fatal and says why. Full run still PASS - 1253 URLs honored. shellcheck and shfmt clean. Both found by Copilot review on #73, as suppressed comments. Co-Authored-By: Claude Opus 5 (1M context) --- checks/check-live-urls.sh | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/checks/check-live-urls.sh b/checks/check-live-urls.sh index dc6fb11..e37f12d 100755 --- a/checks/check-live-urls.sh +++ b/checks/check-live-urls.sh @@ -82,7 +82,14 @@ fi # guarantee resting on a build default is not a guarantee. Demonstrated rather than assumed: # with the option off, under en_US.UTF-8, `aé` and `aÉ` are both ACCEPTED by this pattern, # and with it on they are rejected. -shopt -s globasciiranges +# Checked, because this script runs under `set -uo pipefail` and not `-e`, so an unsupported +# option would print to stderr, return 1, and be stepped straight over — leaving the +# validation locale-dependent underneath a comment promising it is not. `shopt` returns 1 on +# an unknown option name, which is what makes this testable rather than decorative. +shopt -s globasciiranges || { + echo "FAIL this shell does not support globasciiranges, so the character allowlist below would be locale-dependent" >&2 + exit 2 +} case "$CHECK_TAG" in *[!A-Za-z0-9._/-]*) echo "FAIL CHECK_TAG may contain only letters, digits, and the characters '. _ - /' -- got '$CHECK_TAG'" >&2 @@ -110,14 +117,18 @@ echo "==> tagging requests X-Blog-Check: $CHECK_TAG" # A command line is also world-readable in ps output, and this runs 1,245 of them. if [ -n "${PANGOLIN_ACCESS_TOKEN_ID:-}" ] && [ -n "${PANGOLIN_ACCESS_TOKEN:-}" ]; then # Same hazard as CHECK_TAG above and the same reason, but a narrower rule, because the - # grammar of a credential is the issuer's to define and not this script's. Only the two - # characters that break out of a quoted config line are refused, and neither is legal in - # an HTTP header value, so a token containing one is a paste accident rather than a token. + # grammar of a credential is the issuer's to define and not this script's. Only the + # characters that break out of a quoted config line are refused, and none is legal in an + # HTTP header value, so a token containing one is a paste accident rather than a token. # Reported without echoing the value, since it is a secret and the finding is its shape. + # + # Carriage return counts as a line ending here as much as newline does. Header injection + # is classically CRLF, and a lone CR is enough on its own, so refusing LF while allowing + # CR would leave the shape this guard exists for. for name in PANGOLIN_ACCESS_TOKEN_ID PANGOLIN_ACCESS_TOKEN; do case "${!name}" in - *'"'* | *$'\n'*) - echo "FAIL $name contains a quote or a newline, which cannot appear in an HTTP header value" >&2 + *'"'* | *$'\n'* | *$'\r'*) + echo "FAIL $name contains a quote, a newline, or a carriage return, none of which can appear in an HTTP header value" >&2 exit 2 ;; esac