From 8812b21f19fdf8fe5614ba0c55b90e33ad00715a Mon Sep 17 00:00:00 2001 From: Martin Simon Date: Thu, 17 Sep 2026 13:24:43 +0200 Subject: [PATCH] Match action inputs on a word boundary, not as substrings The input-coverage check tested `f"inputs.{name}" not in piped`, so an input whose name is a prefix of another's reported as covered while never reaching the step: "path" is a substring of a piped "pathspec". Today's five names happen not to collide, so nothing is dead -- the guard was. Its own comment calls an unpiped input dead surface, and this is the check that was supposed to find one. Verified both directions: an injected unpiped `path` input now fails the lint, and passed it before. Also here: git diff --cached --quiet returns 128 on error, not just 0 and 1, and 2>/dev/null threw away the reason. On an unborn HEAD the suppressed message is "fatal: ambiguous argument 'HEAD'" and the script proceeded as though there were changes, failing one step later at rev-parse. The staging branch on PATHSPEC guarded nothing: both diffs below it pass the same possibly-empty array through `-- "${PATHSPEC[@]}"` with no guard, and an empty expansion after -- is what passing no pathspec means. Three usage strings that disagreed about [pathspec ...] are now one. The suite kept its scratch directories in three of six groups and left them in the other three, so every run leaked three; one scratch root exported as TMPDIR puts all six under the trap that already existed. Removed a fixture loop whose only case arm is empty, over a --data@ spelling the caller never produces, and two `|| true` under a harness that sets -u without -e and ends each group with its own exit. dependabot.yml claimed actions here are pinned to commit SHAs. The one action is actions/checkout@v7, first-party on a major tag deliberately. Verified: 13 assertions, lint-action.sh clean, shellcheck clean, actionlint clean, /tmp delta 0 (was 3). --- .github/dependabot.yml | 10 +++++++--- README.md | 1 + commit-branch.sh | 19 +++++++++---------- tests/lint-action.sh | 9 +++++++-- tests/run.sh | 16 +++++++++------- 5 files changed, 33 insertions(+), 22 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index f47954f..127e8e3 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,9 +1,13 @@ version: 2 updates: - # Actions are pinned to commit SHAs and the linters by image digest, which - # is what stops a moved tag changing what runs here. It is also what stops - # them updating on their own. + # The one action here is first-party and stays on a major tag deliberately: + # GitHub owns both the tag and the content a SHA would pin, so pinning moves + # trust from GitHub to GitHub. This entry is what keeps that tag current. + # + # The two linters ARE pinned, by image digest, because they are third party. + # They sit inside `run: docker run ...` blocks, which this ecosystem does not + # read, so those move by hand. - package-ecosystem: github-actions directory: / schedule: diff --git a/README.md b/README.md index 39cd981..1d421df 100644 --- a/README.md +++ b/README.md @@ -142,6 +142,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ``` + ## Buy us a coffee? If you feel like buying us a coffee (or a beer?), donations are welcome: diff --git a/commit-branch.sh b/commit-branch.sh index 9b16e2e..0ba6ffd 100755 --- a/commit-branch.sh +++ b/commit-branch.sh @@ -31,9 +31,10 @@ set -euo pipefail # called as x="$(f)" keeps running after a failure instead of aborting. shopt -s inherit_errexit -WORKDIR="${1:?usage: commit-branch.sh }" -BRANCH="${2:?usage: commit-branch.sh }" -MESSAGE="${3:?usage: commit-branch.sh [pathspec ...]}" +USAGE="usage: commit-branch.sh [pathspec ...]" +WORKDIR="${1:?$USAGE}" +BRANCH="${2:?$USAGE}" +MESSAGE="${3:?$USAGE}" shift 3 PATHSPEC=("$@") @@ -56,12 +57,10 @@ if ! git rev-parse --git-dir >/dev/null 2>&1; then exit 1 fi -# Stage so git decides what changed, not a directory walk. -if [ "${#PATHSPEC[@]}" -gt 0 ]; then - git add -A -- "${PATHSPEC[@]}" -else - git add -A -fi +# Stage so git decides what changed, not a directory walk. An empty PATHSPEC +# expands to nothing, which after -- means exactly what passing no pathspec +# means; both diffs below already rely on that. +git add -A -- "${PATHSPEC[@]}" # The oid of the commit this makes, for a caller that has to name it. Without # it a caller can only ask GitHub to resolve the branch by name afterwards, and # that is a race: a workflow dispatched seconds after this mutation can still @@ -76,7 +75,7 @@ emit_sha() { # oid printf 'sha=%s\n' "$1" >> "$GITHUB_OUTPUT" } -if git diff --cached --quiet HEAD -- "${PATHSPEC[@]}" 2>/dev/null; then +if git diff --cached --quiet HEAD -- "${PATHSPEC[@]}"; then log "nothing changed on $BRANCH" emit_sha "" exit 0 diff --git a/tests/lint-action.sh b/tests/lint-action.sh index b751118..1e94af8 100755 --- a/tests/lint-action.sh +++ b/tests/lint-action.sh @@ -21,7 +21,7 @@ body="$(mktemp)" trap 'rm -f "$body"' EXIT python3 - "$body" <<'PY' -import sys, yaml +import re, sys, yaml class Strict(yaml.SafeLoader): pass @@ -51,9 +51,14 @@ assert step["shell"] == "bash", "run: without shell: bash is a load-time error" # Compared on the env VALUES, not the names: an input is free to arrive under # a different variable (token -> GITHUB_TOKEN), so matching names would report # a gap that is not there. +# Matched on a word boundary. A plain substring test let an input whose name +# is a prefix of another's ("path" against a piped "pathspec") report as +# covered while never reaching the step -- the check passing on work it did +# not do, which is the one thing it exists to catch. declared = set(doc["inputs"]) piped = " ".join(str(v) for v in step.get("env", {}).values()) -missing = {name for name in declared if f"inputs.{name}" not in piped} +referenced = set(re.findall(r"inputs\.([A-Za-z0-9_-]+)", piped)) +missing = declared - referenced assert not missing, f"inputs never passed to the step: {sorted(missing)}" # Expressions belong in env:, never in the run body, so a value containing diff --git a/tests/run.sh b/tests/run.sh index e2d09cb..5e16cb6 100755 --- a/tests/run.sh +++ b/tests/run.sh @@ -23,8 +23,14 @@ fail=0 # Each group runs in a subshell, so a counter variable incremented inside one # never reaches this scope. The tally goes through a file instead, which is # the only way the assertion-count guard below can see anything at all. +# One scratch root for the whole suite, exported as TMPDIR so every mktemp +# below lands inside it. Three of the six groups cleaned up after themselves +# and three did not, and a group exiting early leaked whatever its own +# trailing rm would have removed. +TMPROOT="$(mktemp -d)" +export TMPDIR="$TMPROOT" TALLY="$(mktemp)" -trap 'rm -f "$TALLY"' EXIT +trap 'rm -rf "$TMPROOT"' EXIT ok() { printf ' ok %s\n' "$1"; echo ok >> "$TALLY"; } no() { printf ' FAIL %s\n %s\n' "$1" "$2"; echo no >> "$TALLY"; fail=$((fail + 1)); } @@ -60,7 +66,6 @@ echo "the signed-commit payload describes every change, deletions included" mkdir -p "$work/bin" cat > "$work/bin/curl" </dev/null 2>&1 || true + PATH="$work/bin:$PATH" "$ROOT/commit-branch.sh" "$repo" archive "test" >/dev/null 2>&1 if [ ! -f "$work/payload.json" ]; then no "the payload is built" "no payload captured" @@ -113,7 +118,7 @@ done echo '{"data":{"createCommitOnBranch":{"commit":{"oid":"cafebabecafebabe","signature":{"isValid":true,"state":"VALID"}}}}}' FAKE chmod +x "$work/bin/curl" - PATH="$work/bin:$PATH" "$ROOT/commit-branch.sh" "$repo" master "cuts" png/ >/dev/null 2>&1 || true + PATH="$work/bin:$PATH" "$ROOT/commit-branch.sh" "$repo" master "cuts" png/ >/dev/null 2>&1 if [ ! -f "$work/payload.json" ]; then no "a pathspec-scoped commit is built" "no payload captured" @@ -182,7 +187,6 @@ FAKE *) no "and it is labelled as the failure it is" "got [$out]" ;; esac - cd /; rm -rf "$work" exit $((fail > 0)) ) || fail=$((fail + 1)) @@ -224,7 +228,6 @@ FAKE >/dev/null 2>&1 && rc=0 || rc=$? eq "an unset GITHUB_OUTPUT is not a failure" "0" "${rc:-1}" - cd /; rm -rf "$work" exit $((fail > 0)) ) || fail=$((fail + 1)) @@ -256,7 +259,6 @@ FAKE # caller that tagged this would be naming an unsigned commit. eq "and nothing is handed to the caller" "" "$(cat "$out")" - cd /; rm -rf "$work" exit $((fail > 0)) ) || fail=$((fail + 1))