#297

static-analysis: run-scans.sh expands --include globs against the CWD, silently dropping an extension

Openluiscampos-evinkaopened 1mo ago

What

build_argv in plugins/static-analysis/skills/semgrep/scripts/run-scans.sh interpolates the include globs unquoted so they word-split. Unquoted expansion in bash is word splitting and pathname expansion, so the globs are expanded against the current working directory before they ever reach semgrep.

# Unquoted on purpose: includes is a space-separated glob list and must word-split here.
# shellcheck disable=SC2086
for g in $includes; do ARGV+=("--include=$g"); done

If the CWD happens to contain files matching one of the globs, that glob is replaced by the literal filenames found there — filenames that generally do not exist in the scan target. The patterns that match nothing survive, because the default nullglob/failglob behaviour leaves an unmatched pattern as-is. That asymmetry is what makes it hard to notice: some --include flags look right and one does not.

Reproduction

mkdir -p /tmp/repro/target && cd /tmp/repro
printf 'const x = 1;\n' > target/app.mjs
touch decoy-one.mjs decoy-two.mjs           # .mjs in the CWD, NOT in the target
printf '{"baseline":[],"javascript":["p/javascript"],"third_party":[]}\n' > rs.json

bash /path/to/run-scans.sh --target /tmp/repro/target --output-dir /tmp/repro/out \
  --mode run-all --rulesets rs.json --dry-run | grep -o '\-\-include=[^ ]*' | sort -u

Actual:

--include=*.cjs
--include=decoy-one.mjs      <-- from the CWD
--include=decoy-two.mjs      <-- from the CWD
--include=*.js
--include=*.jsx
--include=*.ts
--include=*.tsx

--include=*.mjs is gone. Expected: the six globs from includes_for, unexpanded.

Impact

Every language-scoped ruleset silently skips the extension whose glob got expanded. Running from a directory that holds a few .mjs files means p/javascript, p/nodejs and friends never look at any .mjs file in the target — and scans.json reports the run as successful, with a filesScanned count that looks plausible because the other extensions were still scanned.

The cross-language scans (p/security-audit, p/secrets, third-party repos) take no --include at all, so those still cover everything. That makes the gap easy to miss: the unscoped scans look complete and only the language-scoped ones are short.

It is CWD-dependent, which is presumably why CI does not catch it: run from a directory with no matching files and the globs pass through untouched.

Suggested fix

Disable pathname expansion around the loop, keeping the word splitting the comment asks for:

set -f
# shellcheck disable=SC2086
for g in $includes; do ARGV+=("--include=$g"); done
set +f

Verified against semgrep 1.176.1.

Activity

2
alexrelintex1mo ago

Additional impact: running from inside the target is the common case, and it evades coveredNothing.

The reproduction above uses decoy files outside the target, so the extension is dropped entirely. The more common path is running the scan from the root of the repo being scanned. There, the glob collapses to a root-level file that does exist in the target, so the ruleset still runs but only against that one file.

Observed on semgrep 1.176.0 with the CWD set to the target root, which contains docker-compose.yml:

"--include=docker-compose.yml"   (expected: "--include=*.yml")

p/github-actions then reported filesScanned: 1 (just docker-compose.yml) and never evaluated .github/workflows/*.yml. Because filesScanned is 1 rather than 0, coveredNothing stays empty, and scans.json reads as a complete, clean run. Any ruleset scoped only to YAML (p/github-actions, p/docker-compose, p/kubernetes) is exposed whenever a matching file sits at the root.

Confirming the suggested fix. Wrapping the loop in set -f / set +f fixed it. --dry-run now emits the literal "--include=*.yml" from both a CWD with an unrelated project.yml and the target root, and a rerun of p/github-actions scanned both workflow files (13 findings that had been missed). Happy to open a PR with the fix plus a regression test that runs --dry-run from a CWD containing a decoy match.

PMFreitass1mo ago

+1, reproduced on semgrep 1.177.0 at HEAD 0cc1c73, with a data point on how large the loss gets in a TypeScript project.

Running from the root of a Next.js repo, *.ts expanded to next-env.d.ts (the one .ts file at the root) and *.mjs to the two root .mjs configs. Every language-scoped ruleset for JavaScript (p/javascript, p/typescript, p/react, p/nodejs) then scanned 1 of 159 .ts files; .tsx was fine only because no .tsx sits at the root. scans.json showed all of them as succeeded, filesScanned: 73, coveredNothing empty. We only caught it by grouping paths.scanned by extension and comparing with git ls-files.

Rerunning with the working directory set to an empty directory (-w /tmp in the semgrep Docker image) restored full coverage (159 .ts, 69 .tsx, 7 .mjs). The set -f / set +f fix proposed above would have prevented it.

A cheap guard that would also catch any future variant: after the scans, compare per-extension counts in paths.scanned against the Step 1 file detection, and flag a language-scoped scan that saw far fewer files of an extension than exist.