From 6a5d2636149770038325a69428e78f0021fed706 Mon Sep 17 00:00:00 2001 From: Remigiusz Antczak Date: Sat, 24 Oct 2026 10:19:32 +0200 Subject: [PATCH] review_checks: fail closed on scanner output, guard dash filenames, batch semgrep Scanner JSON that is missing, empty, malformed or the wrong shape now produces a "not run" line instead of reading as zero findings, a semgrep `errors` array is reported as a partial scan, and crash exit codes from gitleaks, osv-scanner, semgrep, shellcheck, ruff, pyflakes, tsc and go vet are surfaced rather than swallowed (findings codes such as shellcheck 1 or osv-scanner 1 are not crashes). Changed files reach the linters, gitleaks and osv-scanner as ./path so a leading-dash name is never parsed as an option, the gitleaks report is cleared before every run, and semgrep runs once per 200 changed files after `--` instead of once per file. SKILL.md routes the whole-app security reference lazily. Tests: 32/32 (19 in test-review-checks-security-folds.sh, 13 existing) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_014QNFFPWVcPUbeuvwM5fk2Z --- .claude/skills/deep-code-review/SKILL.md | 2 +- .../deep-code-review/scripts/review_checks.sh | 76 ++++++++---- changelog.d/security-folds.md | 4 + .../review-checks-security-folds.sh | 2 + scripts/test-review-checks-security-folds.sh | 108 ++++++++++++++++++ 5 files changed, 166 insertions(+), 26 deletions(-) create mode 100644 changelog.d/security-folds.md create mode 100644 scripts/ci-tests.d/review-checks-security-folds.sh create mode 100755 scripts/test-review-checks-security-folds.sh diff --git a/.claude/skills/deep-code-review/SKILL.md b/.claude/skills/deep-code-review/SKILL.md index 3f6ab3fe3..ddda1c5f0 100644 --- a/.claude/skills/deep-code-review/SKILL.md +++ b/.claude/skills/deep-code-review/SKILL.md @@ -21,6 +21,6 @@ You are a senior reviewer hunting defects that a change introduces. For a small 4. One finding per distinct mechanism, most severe first, at most 8. 5. Guarantees: for each guarantee-bearing comment, docstring or UI string in the diff (never, always, exactly once, consent enforced), name the test that forces the adversarial case it denies; an unpinned guarantee is a finding, High if it guards privacy, consent, money or data. -Whole-app security audit or review (not a diff): read `references/security-review-mode.md` first. +Whole-app security audit (not a diff): read only `references/security-review-mode.md`; it routes the rest. OUTPUT. Final message: the severity-ranked findings, each as `file:line`, severity, mechanism and failing input in one or two sentences. Report template: `references/full-map-report.md`. diff --git a/.claude/skills/deep-code-review/scripts/review_checks.sh b/.claude/skills/deep-code-review/scripts/review_checks.sh index a6d0f7d3d..e213083ae 100755 --- a/.claude/skills/deep-code-review/scripts/review_checks.sh +++ b/.claude/skills/deep-code-review/scripts/review_checks.sh @@ -28,8 +28,8 @@ case "$tmo" in ''|*[!0-9]*) echo "review_checks: --timeout must be an integer" > git rev-parse --verify -q "$base^{commit}" >/dev/null || { echo "review_checks: unresolvable base '$base'" >&2; exit 2; } cd "$(git rev-parse --show-toplevel)" -raw=$(mktemp "${TMPDIR:-/tmp}/review-raw.XXXXXX") out=$(mktemp "${TMPDIR:-/tmp}/review-out.XXXXXX") -trap 'rm -f "$raw" "$out"' EXIT +raw=$(mktemp "${TMPDIR:-/tmp}/review-raw.XXXXXX") out=$(mktemp "${TMPDIR:-/tmp}/review-out.XXXXXX") tsv=$(mktemp "${TMPDIR:-/tmp}/review-tsv.XXXXXX") +trap 'rm -f "$raw" "$out" "$tsv"' EXIT files=$(git diff --name-only --diff-filter=ACMR "$base"...HEAD) rc=0 @@ -53,18 +53,21 @@ run_one() { case "$rc" in 124|142) skip "$tool" "timeout ${tmo}s"; return 1 ;; esac return 0 } +# scan_ok TOOL [OKRC...]: false (with a not-run line) when the last run_one exit code is not 0 or an OKRC (findings), i.e. a crash. +scan_ok() { local t=$1 c; shift; for c in 0 "$@"; do [ "$rc" = "$c" ] && return 0; done; skip "$t" "exit $rc"; return 1; } # Findings parsed from $out by a sed script (TSV columns) land via `add`. sh_files=$(changed '\.(sh|bash)$') if [ -n "$sh_files" ]; then if have shellcheck; then while IFS= read -r f; do - run_one shellcheck shellcheck -f gcc "$f" || continue + run_one shellcheck shellcheck -f gcc "./$f" || continue + scan_ok shellcheck 1 || continue sed -nE 's/^.*:([0-9]+):[0-9]+: [a-z]+: (.*) \[(SC[0-9]+)\]$/\1\t\3\t\2/p' "$out" | while IFS=$'\t' read -r l r m; do add shellcheck "$f" "$l" "$r" "$m"; done done <<<"$sh_files" fi while IFS= read -r f; do - run_one bash-n bash -n "$f" || continue + run_one bash-n bash -n "./$f" || continue [ "$rc" = 0 ] || add bash-n "$f" "$(sed -nE 's/.*line ([0-9]+):.*/\1/p' "$out" | head -n1)" syntax "$(head -n1 "$out")" done <<<"$sh_files" fi @@ -73,7 +76,7 @@ py_files=$(changed '\.py$') if [ -n "$py_files" ]; then if have python3; then while IFS= read -r f; do - run_one py_compile python3 -c 'import sys;compile(open(sys.argv[1],"rb").read(),sys.argv[1],"exec")' "$f" || continue + run_one py_compile python3 -c 'import sys;compile(open(sys.argv[1],"rb").read(),sys.argv[1],"exec")' "./$f" || continue [ "$rc" = 0 ] || add py_compile "$f" "$(sed -nE 's/.*[(,] ?line ([0-9]+).*/\1/p' "$out" | head -n1)" SyntaxError "$(tail -n1 "$out")" done <<<"$py_files" fi @@ -84,7 +87,8 @@ if [ -n "$py_files" ]; then else skip "ruff/pyflakes" "not installed"; ln=; fi if [ -n "$ln" ]; then while IFS= read -r f; do - run_one "$ln" "${lint[@]}" "$f" || continue + run_one "$ln" "${lint[@]}" "./$f" || continue + scan_ok "$ln" 1 || continue sed -nE 's/^.*:([0-9]+):[0-9]+:? ([A-Z]+[0-9]+ )?(.*)$/\1\t\2\t\3/p' "$out" | while IFS=$'\t' read -r l r m; do add "$ln" "$f" "$l" "${r% }" "$m"; done done <<<"$py_files" @@ -94,20 +98,20 @@ fi js_files=$(changed '\.(js|mjs|cjs)$') if [ -n "$js_files" ] && have node; then while IFS= read -r f; do - run_one node-check node --check "$f" || continue + run_one node-check node --check "./$f" || continue [ "$rc" = 0 ] || add node-check "$f" "$(sed -nE 's/^.*:([0-9]+)$/\1/p' "$out" | head -n1)" SyntaxError "$(grep -m1 'Error' "$out" || head -n1 "$out")" done <<<"$js_files" fi if [ -n "$(changed '\.(ts|tsx)$')" ] && [ -f tsconfig.json ] && have tsc; then - if run_one tsc tsc --noEmit -p tsconfig.json; then + if run_one tsc tsc --noEmit -p tsconfig.json && scan_ok tsc 1 2; then sed -nE 's/^(.*)\(([0-9]+),[0-9]+\): error (TS[0-9]+): (.*)$/\1\t\2\t\3\t\4/p' "$out" | while IFS=$'\t' read -r f l r m; do add tsc "$f" "$l" "$r" "$m"; done fi fi if [ -n "$(changed '\.go$')" ] && [ -f go.mod ] && have go; then - if run_one go-vet go vet ./...; then + if run_one go-vet go vet ./... && scan_ok go-vet 1; then sed -nE 's/^([^#[:space:]][^:]*\.go):([0-9]+):[0-9]+: (.*)$/\1\t\2\t\3/p' "$out" | while IFS=$'\t' read -r f l m; do add go-vet "$f" "$l" vet "$m"; done fi @@ -116,42 +120,55 @@ fi json_files=$(changed '\.json$') if [ -n "$json_files" ] && have jq; then while IFS= read -r f; do - run_one jq jq empty "$f" || continue + run_one jq jq empty "./$f" || continue [ "$rc" = 0 ] || add jq "$f" "$(sed -nE 's/.*line ([0-9]+).*/\1/p' "$out" | head -n1)" invalid-json "$(head -n1 "$out")" done <<<"$json_files" fi # Scanner JSON (file $2, may have log noise before the JSON) of kind $1 -> TSV: file, line, rule, message. +# Fails closed: missing, empty or malformed JSON exits non-zero instead of reading as "no findings". sjson() { - python3 - "$1" "$2" <<'PY' 2>/dev/null || true + python3 - "$1" "$2" <<'PY' import json, sys kind = sys.argv[1] t = open(sys.argv[2], encoding="utf-8", errors="replace").read() i = min([x for x in (t.find("{"), t.find("[")) if x >= 0], default=-1) -d = json.JSONDecoder().raw_decode(t[i:])[0] if i >= 0 else [] +if i < 0: + sys.exit("sjson: no JSON in scanner output") +d = json.JSONDecoder().raw_decode(t[i:])[0] +# Wrong-shape JSON must not read as clean: gitleaks is a list, semgrep and osv an object with a "results" list. +if kind == "gitleaks": + if not isinstance(d, list): + sys.exit("sjson: gitleaks report is not a list") +elif not isinstance(d, dict) or not isinstance(d.get("results"), list): + sys.exit(f"sjson: {kind} output has no results list") rows = [] if kind == "gitleaks": # Secret/Match fields are deliberately never read rows = [(r.get("File"), r.get("StartLine"), r.get("RuleID"), r.get("Description") or "secret detected") for r in d] elif kind == "semgrep": - rows = [(r["path"], r["start"]["line"], r.get("check_id"), (r.get("extra") or {}).get("message", "")) for r in d.get("results") or []] + rows = [(r["path"], r["start"]["line"], r.get("check_id"), (r.get("extra") or {}).get("message", "")) for r in d["results"]] else: - for r in d.get("results") or []: + for r in d["results"]: for p in r.get("packages") or []: pk = p.get("package") or {} rows += [(r["source"]["path"], 0, v.get("id"), f"{pk.get('name')}@{pk.get('version')}: {v.get('summary', '')}") for v in p.get("vulnerabilities") or []] for row in rows: print("\t".join(str(x).replace("\t", " ").replace("\n", " ") for x in row)) +sys.exit(3 if kind == "semgrep" and d.get("errors") else 0) # 3 = rows printed, but the scan was partial PY } if have gitleaks; then - rep=$(mktemp "${TMPDIR:-/tmp}/review-gl.XXXXXX"); trap 'rm -f "$raw" "$out" "$rep"' EXIT + rep=$(mktemp "${TMPDIR:-/tmp}/review-gl.XXXXXX"); trap 'rm -f "$raw" "$out" "$tsv" "$rep"' EXIT while IFS= read -r f; do [ -f "$f" ] || continue - run_one gitleaks gitleaks detect --no-git --no-banner --redact --exit-code 0 -f json -r "$rep" -s "$f" || continue - sjson gitleaks "$rep" | while IFS=$'\t' read -r _ l r m; do add gitleaks "$f" "$l" "$r" "$m"; done - : >"$rep" + : >"$rep" # a stale report from the previous file must never be read for this one + run_one gitleaks gitleaks detect --no-git --no-banner --redact --exit-code 0 -f json -r "$rep" -s "./$f" || continue + scan_ok gitleaks || continue + [ -s "$rep" ] || echo '[]' >"$rep" # gitleaks may leave an empty report for a clean file + sjson gitleaks "$rep" >"$tsv" || { skip gitleaks "unparseable report for $f"; continue; } + while IFS=$'\t' read -r _ l r m; do add gitleaks "$f" "$l" "$r" "$m"; done <"$tsv" done <<<"$files" fi @@ -160,19 +177,28 @@ if [ -z "$lock_files" ]; then : elif [ "${REVIEW_OSV:-}" != 1 ]; then skip osv-scanner "REVIEW_OSV not set; osv-scanner queries the OSV API" elif have osv-scanner; then while IFS= read -r f; do - run_one osv-scanner osv-scanner --format json -L "$f" || continue - sjson osv "$out" | while IFS=$'\t' read -r _ l r m; do add osv-scanner "$f" "$l" "$r" "$m"; done + run_one osv-scanner osv-scanner --format json -L "./$f" || continue + scan_ok osv-scanner 1 || continue # exit 1 = vulnerabilities found + sjson osv "$out" >"$tsv" || { skip osv-scanner "unparseable output for $f"; continue; } + while IFS=$'\t' read -r _ l r m; do add osv-scanner "$f" "$l" "$r" "$m"; done <"$tsv" done <<<"$lock_files" fi if [ -z "${REVIEW_SEMGREP_CONFIG:-}" ]; then skip semgrep "REVIEW_SEMGREP_CONFIG not set; --config auto is never used" elif [ ! -e "$REVIEW_SEMGREP_CONFIG" ]; then skip semgrep "REVIEW_SEMGREP_CONFIG must be an existing local path" elif have semgrep; then - while IFS= read -r f; do - [ -f "$f" ] || continue - run_one semgrep semgrep --config "$REVIEW_SEMGREP_CONFIG" --json --quiet --metrics=off --disable-version-check "$f" || continue - sjson semgrep "$out" | while IFS=$'\t' read -r _ l r m; do add semgrep "$f" "$l" "$r" "$m"; done - done <<<"$files" + sg=() + while IFS= read -r f; do [ -f "$f" ] && sg+=("$f"); done <<<"$files" + # One call per 200 files: typically a single run, but bounded argv and a timeout that is not shared by everything. + for ((i = 0; i < ${#sg[@]}; i += 200)); do + run_one semgrep semgrep --config "$REVIEW_SEMGREP_CONFIG" --json --quiet --metrics=off --disable-version-check -- "${sg[@]:i:200}" || continue + scan_ok semgrep || continue + e=0; sjson semgrep "$out" >"$tsv" || e=$? + if [ "$e" = 0 ] || [ "$e" = 3 ]; then + while IFS=$'\t' read -r p l r m; do add semgrep "$p" "$l" "$r" "$m"; done <"$tsv" + [ "$e" = 0 ] || skip semgrep "partial scan: semgrep reported errors" + else skip semgrep "unparseable output"; fi + done fi if [ "$tests" = 1 ]; then diff --git a/changelog.d/security-folds.md b/changelog.d/security-folds.md new file mode 100644 index 000000000..302e783e3 --- /dev/null +++ b/changelog.d/security-folds.md @@ -0,0 +1,4 @@ +### Fixed +- `review_checks.sh` scanners now fail closed: malformed, empty, missing or wrong-shape scanner JSON, a semgrep `errors` array (partial scan) and crash exit codes from gitleaks, osv-scanner, semgrep, shellcheck, ruff, pyflakes, tsc and go vet print a `not run` line instead of reading as "no findings". Changed files reach the linters, gitleaks and osv-scanner as `./path` so a leading-dash filename is never read as an option, the gitleaks report is cleared before every run, and semgrep runs once per 200 changed files after `--`. +- `SKILL.md` routes the whole-app security review reference lazily ("read only", it routes the rest) instead of "first". +- size-budget-raise: .claude/skills/deep-code-review/SKILL.md 2884→2893 lazy whole-app security route wording diff --git a/scripts/ci-tests.d/review-checks-security-folds.sh b/scripts/ci-tests.d/review-checks-security-folds.sh new file mode 100644 index 000000000..0c3dff753 --- /dev/null +++ b/scripts/ci-tests.d/review-checks-security-folds.sh @@ -0,0 +1,2 @@ +# semgrep batching, fail-closed scanner JSON and dash-filename tests for review_checks.sh. +bash scripts/test-review-checks-security-folds.sh diff --git a/scripts/test-review-checks-security-folds.sh b/scripts/test-review-checks-security-folds.sh new file mode 100755 index 000000000..3cd29d1c4 --- /dev/null +++ b/scripts/test-review-checks-security-folds.sh @@ -0,0 +1,108 @@ +#!/usr/bin/env bash +# Regression tests for review_checks.sh folds: malformed, empty or wrong-shape scanner JSON, scanner/linter crash +# codes and semgrep `errors` fail closed (a not-run line, never a silent clean); leading-dash filenames are never +# read as options; semgrep runs once for a normal changed-file set. REVIEW_CHECKS overrides the script under test +# (used to prove each check fails on the parent commit). +# shellcheck disable=SC2015,SC2119,SC2120 +set -euo pipefail +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +RC="${REVIEW_CHECKS:-$ROOT/.claude/skills/deep-code-review/scripts/review_checks.sh}" +T=$(mktemp -d "${TMPDIR:-/tmp}/dcr-test-rcfolds.XXXXXX"); trap 'rm -rf "$T"' EXIT +fail=0 +ok() { printf 'ok %s\n' "$1"; } +bad() { printf 'FAIL %s\n' "$1"; fail=1; } + +mkdir "$T/stub" "$T/repo" +cat >"$T/stub/semgrep" <<'S' +#!/usr/bin/env bash +echo "semgrep $*" >>"$SG_LOG" +[ -z "${SG_EXIT:-}" ] || exit "$SG_EXIT" +if [ -n "${SG_OUT:-}" ]; then echo "$SG_OUT"; else echo '{"results":[]}'; fi +S +cat >"$T/stub/gitleaks" <<'S' +#!/usr/bin/env bash +echo "gitleaks $*" >>"$GL_LOG" +while [ $# -gt 0 ]; do [ "$1" = -r ] && rep=$2; shift; done +[ -z "${GL_EXIT:-}" ] || exit "$GL_EXIT" +if [ -n "${GL_FIRST_BAD:-}" ]; then # garbage on the first call only, nothing written afterwards + [ -e "$GL_FIRST_BAD" ] && exit 0 + : >"$GL_FIRST_BAD"; printf 'not json' >"$rep"; exit 0 +fi +printf '%s' "${GL_OUT-[]}" >"$rep" +S +cat >"$T/stub/osv-scanner" <<'S' +#!/usr/bin/env bash +echo "osv-scanner $*" >>"$OSV_LOG" +echo "${OSV_OUT-{\"results\":[]\}}" +exit "${OSV_EXIT:-0}" +S +for tool in shellcheck ruff node; do + cat >"$T/stub/$tool" <>"\$TOOL_LOG" +exit "\${EXIT_$tool:-0}" +S +done +chmod +x "$T/stub"/* + +cd "$T/repo" +git init -q . && git -c user.email=t@example.com -c user.name=t commit -q --allow-empty -m base +git branch base +printf 'a\n' >a.txt +printf 'b\n' >b.txt +printf '#!/bin/sh\necho hi\n' >./-v.sh +printf 'x = 1\n' >./-p.py +printf '1;\n' >./-n.js +printf '{}\n' >./-j.json +printf '{}\n' >package-lock.json +git add -- . && git -c user.email=t@example.com -c user.name=t commit -q -m files +export SG_LOG="$T/sg.log" GL_LOG="$T/gl.log" OSV_LOG="$T/osv.log" TOOL_LOG="$T/tool.log" REVIEW_SEMGREP_CONFIG="$T/rules.yml" +echo "rules: []" >"$REVIEW_SEMGREP_CONFIG" +: >"$SG_LOG"; : >"$GL_LOG"; : >"$OSV_LOG"; : >"$TOOL_LOG" +run() { PATH="$T/stub:$PATH" REVIEW_OSV=1 bash "$RC" --base base "$@" 2>"$T/err" >"$T/out"; } +errhas() { grep -qF -- "$1" "$T/err"; } +findings() { jq --arg t "$1" '[.[] | select(.tool==$t)] | length' "$T/out"; } + +# Dash filenames: bash -n and jq would read them as options; every tool gets ./ or --. +run && ok "exit 0" || bad "exit 0" +jq -e 'any(.[]; .tool=="bash-n" or .tool=="jq")' "$T/out" >/dev/null && bad "dash file not read as option" || ok "dash file not read as option" +grep -q -- '-s ./-v.sh' "$GL_LOG" && ok "gitleaks gets ./ path" || bad "gitleaks gets ./ path" +grep -q -- '-L ./package-lock.json' "$OSV_LOG" && ok "osv-scanner gets ./ path" || bad "osv-scanner gets ./ path" +grep -q 'shellcheck .* ./-v.sh' "$TOOL_LOG" && grep -q 'ruff .* ./-p.py' "$TOOL_LOG" && grep -q 'node --check ./-n.js' "$TOOL_LOG" && ok "linters get ./ path" || bad "linters get ./ path" +# One semgrep call for the changed files, targets after --. +[ "$(wc -l <"$SG_LOG")" -eq 1 ] && grep -q -- ' -- .*a.txt' "$SG_LOG" && grep -q -- 'b.txt' "$SG_LOG" && ok "semgrep batched once" || bad "semgrep batched once" + +# Malformed scanner output: not-run line, no findings, still exit 0. +SG_OUT='not json' GL_OUT='not json' OSV_OUT='not json' run && ok "bad json exit 0" || bad "bad json exit 0" +errhas 'not run: semgrep (unparseable output)' && errhas 'not run: gitleaks (unparseable report' && errhas 'not run: osv-scanner (unparseable output' && ok "bad json fails closed" || bad "bad json fails closed" +[ "$(findings semgrep)$(findings gitleaks)$(findings osv-scanner)" = 000 ] && ok "bad json no findings" || bad "bad json no findings" + +# Wrong-shape JSON (valid JSON, missing results list) is not "clean". +SG_OUT='{"x":1}' OSV_OUT='{"results":null}' GL_OUT='{"x":1}' run +errhas 'not run: semgrep (unparseable output)' && errhas 'not run: osv-scanner (unparseable output' && errhas 'not run: gitleaks (unparseable report' && ok "wrong-shape json fails closed" || bad "wrong-shape json fails closed" + +# gitleaks: an empty report for a clean file is [], not an error. +GL_OUT='' run +errhas 'gitleaks (unparseable' && bad "empty gitleaks report is clean" || ok "empty gitleaks report is clean" + +# A bad report for one file must not leak into the next file's read. +GL_FIRST_BAD="$T/gl-first" run +[ "$(grep -c 'not run: gitleaks (unparseable report' "$T/err")" -eq 1 ] && ok "stale gitleaks report cleared" || bad "stale gitleaks report cleared" +rm -f "$T/gl-first" + +# semgrep `errors` => partial-scan line, findings still reported. +SG_OUT='{"results":[{"check_id":"r","path":"a.txt","start":{"line":1},"extra":{"message":"m"}}],"errors":[{"message":"boom"}]}' run +errhas 'not run: semgrep (partial scan' && [ "$(findings semgrep)" = 1 ] && ok "semgrep errors surfaced" || bad "semgrep errors surfaced" + +# Crash codes surfaced; osv exit 1 (vulnerabilities found) is not a crash. +SG_EXIT=3 run && ok "crash exit 0" || bad "crash exit 0" +errhas 'not run: semgrep (exit 3)' && ok "semgrep crash surfaced" || bad "semgrep crash surfaced" +GL_EXIT=2 run +errhas 'not run: gitleaks (exit 2)' && ok "gitleaks crash surfaced" || bad "gitleaks crash surfaced" +OSV_EXIT=127 run +errhas 'not run: osv-scanner (exit 127)' && ok "osv crash surfaced" || bad "osv crash surfaced" +OSV_EXIT=1 OSV_OUT='{"results":[{"source":{"path":"x"},"packages":[{"package":{"name":"p","version":"1"},"vulnerabilities":[{"id":"GHSA-x","summary":"s"}]}]}]}' run +errhas 'not run: osv-scanner' && bad "osv exit 1 is findings" || [ "$(findings osv-scanner)" = 1 ] && ok "osv exit 1 is findings" || bad "osv exit 1 is findings" +EXIT_shellcheck=2 EXIT_ruff=2 run +errhas 'not run: shellcheck (exit 2)' && errhas 'not run: ruff (exit 2)' && ok "linter crash surfaced" || bad "linter crash surfaced" +exit "$fail"