diff --git a/ci/selftest.sh b/ci/selftest.sh index 9b0ee7f..03762a5 100755 --- a/ci/selftest.sh +++ b/ci/selftest.sh @@ -251,7 +251,11 @@ identity_row() { # $1 = RED|green (want), $2 = label, $3 = line to plant return fi row_out=$(./ci/static.sh 2>&1 || true) - if printf '%s' "$row_out" | grep -q 'sets a git identity that outlives it'; then + # The stable half of the message. The wider phrasing moved from "A ci/ script + # sets …" to "A tracked file writes …" when the scan was widened, and these + # rows all reported NOT FLAGGED while the check was working perfectly — an + # assertion on a log line is coupled to its wording. + if printf '%s' "$row_out" | grep -q 'git identity that outlives it'; then got=RED else got=green @@ -279,12 +283,42 @@ identity_row green 'other a non-identity key' "$gc core.autocrlf false" identity_row green 'prose a commented-out mention' "# $gc user.name \"x\" is the fault" cp "$work/build-zip.orig" ci/build-zip.sh +# A violation OUTSIDE ci/*.sh. This is the row the old scope missed entirely: +# a workflow step writes an identity into the runner's clone just as +# permanently, and the glob the check started with could not see it. +cp "$work/build-zip.orig" ci/build-zip.sh +wf=.github/workflows/selftest-identity.yml +printf 'name: x\njobs:\n y:\n steps:\n - run: %s user.email "x@y"\n' "$gc" > "$wf" +git add -N "$wf" >/dev/null # untracked files are invisible to git ls-files +wf_out=$(./ci/static.sh 2>&1 || true) +printf '%s' "$wf_out" | grep -q 'writes a git identity that outlives it' \ + && ok "flagged a write outside ci/ (a workflow step)" \ + || bad "NOT FLAGGED: a workflow step writing an identity — the scan is still scoped to ci/" +rm -f "$wf" +git rm --cached -q --ignore-unmatch "$wf" >/dev/null 2>&1 + +# 🔴 A drift check on the FORM, and only that: `if … | xargs -0 grep …; then` +# has a false green in it, because xargs exits 123 when any batch found nothing +# and `if` reads 123 as no-match. It is invisible while the list fits one exec, +# which is why nothing here can measure it — this tree's 48 files are one batch. +# So this asserts the shape is gone rather than that the bug is absent, and it +# cannot tell you the replacement is correct. scan_tracked greps per file and +# reads its verdict from the output, which is what makes batching irrelevant. +# ^[^#]* — the first version of this matched the COMMENT in static.sh that +# quotes the bad form in order to explain it, which is the stale-docblock trap +# in its own right: the paragraph documenting a rule is text the rule applies to. +if grep -nE '^[^#]*(if|elif).*\| *xargs.*grep' ci/static.sh; then + bad "a check above consumes xargs' exit status — 123 on a partial match reads as clean" +else + ok "no check in ci/static.sh consumes an xargs exit status" +fi + # And the passing line must carry a number: "every script is clean" reads the # same over zero files as over eleven. clean_out=$(./ci/static.sh 2>&1 || true) -printf '%s' "$clean_out" | grep -qE "^ [0-9]+ scripts in ci/ leave the clone's git identity alone" \ - && ok "the git-identity check reports how many scripts it read" \ - || bad "the git-identity check printed no count — it cannot tell 11 scripts from 0" +printf '%s' "$clean_out" | grep -qE "^ [0-9]+ tracked files leave the clone's git identity alone" \ + && ok "the git-identity check reports how many files it read" \ + || bad "the git-identity check printed no count — it cannot tell 48 files from 0" # The fixtures are lines that SET an identity. If any of them executed rather # than being read, this clone now has one — which would also mean every verdict diff --git a/ci/static.sh b/ci/static.sh index baf4cd3..c63514f 100755 --- a/ci/static.sh +++ b/ci/static.sh @@ -19,6 +19,51 @@ shipped_files() { ./ci/dist-files.sh | grep '\.php$' || true } +# 🔴 scan_tracked: the only way this file greps a set of tracked files. +# +# The form it replaces — `if git ls-files -z … | xargs -0 grep -nE …; then` — +# has a false GREEN in it. `xargs` exits 123 when ANY of its invocations exited +# 1-125, and `grep` exits 1 on no-match, so a run where one batch matched and +# another did not exits 123 and `if` reads that as clean. Measured: +# +# one batch, a match xargs exit 0 -> caught +# one batch, no match xargs exit 123 -> clean, correct +# two batches, one match xargs exit 123 -> CLEAN, and it printed the hit +# +# Harmless only while the whole list fits in one exec. git ls-files hands over +# enough paths to split the moment a tree grows or a path gets long, and then +# the check passes over a violation it has already printed. Nothing about the +# output says which case you are in. Reported by the Nolimit Security session +# 2026-09-28, who hit it after widening their own scan from 13 files to 102. +# +# So: no xargs, and the verdict is the OUTPUT rather than any exit status. The +# loop also yields the count for free, which is the other thing every one of +# these checks was missing — "none" over zero files and "none" over nineteen +# are the same green. +# +# Sets: scan_hits (matches, one "file:line:text" per line), scan_files (how +# many files were read). A grep failure (exit > 1) is reported, never swallowed. +scan_tracked() { # $1 = -E|-P, $2 = pattern, rest = git pathspecs + local mode=$1 pattern=$2 f m rc + shift 2 + scan_hits='' + scan_files=0 + # -z: git QUOTES a non-ASCII path, and the quoted form is not a path that + # exists, so a line-based read silently skips the file it meant to check. + while IFS= read -r -d '' f; do + [ -n "$f" ] || continue + scan_files=$((scan_files + 1)) + m=$(grep -n "$mode" -e "$pattern" -- "$f") + rc=$? + if [ "$rc" -gt 1 ]; then + note_fail "grep failed on $f (exit $rc) — this scan did not examine it." + elif [ "$rc" = 0 ]; then + scan_hits="${scan_hits}$(printf '%s\n' "$m" | sed "s|^|$f:|") +" + fi + done < <(git ls-files -z "$@") +} + ci_step "PHP syntax" # The header promises a minimum PHP version; without it in the loop a piece of # newer syntax passes here and fatals on the customer's server instead. @@ -73,10 +118,14 @@ else fi ci_step "no debug leftovers" -if git ls-files -z '*.php' | xargs -0 grep -nE '\b(var_dump|print_r|dd)[[:space:]]*\(' ; then +scan_tracked -E '\b(var_dump|print_r|dd)[[:space:]]*\(' '*.php' +if [ "$scan_files" -lt 1 ]; then + note_fail "The debug-leftover scan read no PHP files at all, so it proved nothing." +elif [ -n "$scan_hits" ]; then + printf '%s' "$scan_hits" note_fail "Debug functions must not ship." else - echo " none" + echo " none in $scan_files tracked PHP files" fi ci_step "no short open tags, no closing ?>" @@ -84,9 +133,14 @@ before=$fails # With short_open_tag off the code is printed to the visitor as text instead of # being run; whitespace after a closing tag is sent to the browser and breaks # headers and redirects. -if git ls-files -z '*.php' | xargs -0 grep -nP '<\?(?!php|=|xml)' ; then +scan_tracked -P '<\?(?!php|=|xml)' '*.php' +if [ "$scan_files" -lt 1 ]; then + note_fail "The open-tag scan read no PHP files at all, so it proved nothing." +elif [ -n "$scan_hits" ]; then + printf '%s' "$scan_hits" note_fail "Use " ] \ && { ci_annotate error "$f" 1 "Remove the trailing ?> at the end of the file."; fails=$((fails + 1)); } done < <(git ls-files -z '*.php') -[ "$fails" = "$before" ] && echo " clean" +[ "$fails" = "$before" ] && echo " $open_tag_files tracked PHP files, no short tags and no trailing ?>" ci_step "text domain" # --min is deliberately far below the real count. It is not an assertion about @@ -307,7 +361,7 @@ elif [ "$fails" = "$before" ]; then echo " $secrets_scanned tracked files read, nothing that looks like a key or a private key" fi -ci_step "no ci/ script configures the clone it was handed" +ci_step "no tracked file configures the clone it was handed" # `git config user.name` writes .git/config and OUTLIVES the script. A helper # that sets an identity before committing therefore renames every later commit # in that working copy, including a release made by a person days afterwards @@ -352,14 +406,25 @@ ci_step "no ci/ script configures the clone it was handed" # # The rule: a script may configure ITSELF, never the working copy it was # handed. Reported by the Nolimit Security session 2026-09-25. -scripts_scanned=$(git ls-files 'ci/*.sh' | wc -l) -if [ "$scripts_scanned" -lt 1 ]; then - note_fail "The git-identity scan found no ci/*.sh files at all, so it proved nothing." -elif git ls-files -z 'ci/*.sh' \ - | xargs -0 grep -nE '^[^#]*git config([[:space:]]+--(local|global|system))*[[:space:]]+user\.[a-zA-Z]+[[:space:]]+[^[:space:])]'; then - note_fail "A ci/ script sets a git identity that outlives it. Scope it to the one commit with 'git -c user.name=… commit', which does not write .git/config." +# +# Writing the fault in prose inside a tracked file will trip this, by design. +# To name it in documentation without going red, write the key as user. — +# the pattern needs [a-zA-Z] after the dot, so an angle-bracket placeholder is +# prose and a real key is a violation. +# Every tracked file, not just ci/*.sh. A workflow step, a hook, a Makefile or +# a docs snippet somebody pastes can write an identity just as permanently, and +# `ci/*.sh` was scope chosen from where the first instance happened to live — +# the same "what are your checks pointed at" question as the contrast check. +# Nolimit Security widened theirs from 13 files to 102 after the thing that +# actually ran on this box turned out to be outside that glob. +scan_tracked -E '^[^#]*git config([[:space:]]+--(local|global|system))*[[:space:]]+user\.[a-zA-Z]+[[:space:]]+[^[:space:])]' . +if [ "$scan_files" -lt 1 ]; then + note_fail "The git-identity scan read no tracked files at all, so it proved nothing." +elif [ -n "$scan_hits" ]; then + printf '%s' "$scan_hits" + note_fail "A tracked file writes a git identity that outlives it. Scope it to the one command with 'git -c user.name=… commit', which does not write .git/config." else - echo " $scripts_scanned scripts in ci/ leave the clone's git identity alone" + echo " $scan_files tracked files leave the clone's git identity alone" fi ci_step "no placeholders left in the plugin header"