Skip to content

Exec-bit guard's prefix classifier is a blacklist, so every unlisted separator spelling fails open #3692

Description

@devantler

🤖 Generated by the Agentic Engineer

Evidence

scripts/guard-tracked-exec-bit.sh decides whether a .sh path sits in a command position by extracting the text in front of it and walking that prefix as whitespace-separated words. Four review rounds on #3667 have each found one more spelling that defeats it:

round found by spelling mechanism
1 Codex echo ready && ./scripts/x.sh classifier broke on echo before reaching &&
1b testing round 1's fix ./scripts/a.sh && bash ./scripts/b.sh greedy grep -o swallowed a.sh into one occurrence
2 CodeRabbit echo ready; scripts/x.sh prefix word is ready;, separator arm matches a separate ; only
3 CodeRabbit echo ready| scripts/x.sh, echo ready&& scripts/x.sh, echo ready;scripts/x.sh same glue on |/&&; the third has no whitespace before the path, so extraction produces nothing at all

Every one is a fail-open: a directly-execed script tracked 100644 goes unchecked whenever another invocation satisfies the anti-vacuity check. Each individual fix was correct and each was verified by ablation — the problem is that the enumeration does not converge.

Why the obvious fix is not available

The natural response — normalise shell separators across the scan before extraction — is unsafe here, and measurably so. The scanned corpus is *.sh, *.yaml, *.yml under scripts/ and .github/, and 140 files contain a literal |. Among them is the guard's own

readonly SCRIPT_PATH_RE='(\.github|scripts)/[A-Za-z0-9_./-]+\.sh'

Splitting on | rewrites that to (\.github | scripts)/…, which both breaks real matches and manufactures new apparent command positions. The same hazard applies to any regex alternation, YAML block scalar, or quoted string in the corpus, and to redirections such as 2>&1 if & is normalised. This is why the current &&/|| split is restricted to whitespace-delimited occurrences — the restriction is deliberate, and it is exactly what rounds 2 and 3 exploit.

The actual defect

The design is a blacklist of non-command prefixes: extract permissively, then discard an occurrence when a prefix word is recognised as something that does not exec. Anything unrecognised is treated as "not a command position", so every unlisted spelling fails open.

Repeated rounds differing only in syntax are a finding about the check's direction, not about the individual spellings.

Expected behaviour

Invert it, so the unlisted case fails closed:

  • Decide command position from a whitelist of shapes the guard can positively prove are not direct execs (an interpreter, a recognised argument position), and treat anything it cannot classify as a direct invocation requiring the bit — or as an explicit "cannot classify" error naming the line.
  • Either way the residual becomes a false positive that surfaces loudly at the offending line, rather than a silent miss. That is the correct direction for a guard whose whole purpose is to stop a silent CI break, and it matches the paved-road rule that a check should fail with the fix.

The cost to weigh: a false positive fails every PR and merge-group run, so the inversion needs the same conservation check the incremental fixes used — the committed tree currently reports 22 directly-invoked scripts and must still report exactly those, with no additions, before the inversion can land.

Tokenising the corpus the way a shell would (rather than by textual separator splitting) is the other credible route, and is the only one that satisfies "preserve quoted text" properly. It is a larger change than a regex.

Acceptance criteria

  • Fixtures for every spelling in the table above, each failing against the pre-inversion guard.
  • A fixture for a quoted separator (echo "a; b" && scripts/x.sh) and for a redirection (cmd 2>&1 scripts/x.sh), proving the normalisation does not manufacture a command position.
  • The committed tree still reports exactly the same 22 directly-invoked scripts.
  • An unclassifiable line produces a named error, not a silent pass.

Rough size

Medium. This is a redesign of the prefix decision, not a regex change, and it should land on its own with its fixtures rather than on top of #3667.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions