From f0492b7555446ba9d0e83120ebcef98f46a341de Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Mon, 13 Jul 2026 13:29:07 -0400 Subject: [PATCH] feat(scanner): auto-resolve + merge suppressions when --suppressions absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit review.sh only applied suppressions when handed an explicit --suppressions FILE, so only the pre-push hook resolved them. Every other entry point (the Open SWE daily-report automation, nightly sweep, on-demand/CI, agent runs) called review.sh without it and therefore suppressed nothing, re-surfacing every already-adjudicated false positive as HIGH. When --suppressions is not passed, resolve by repo basename and MERGE both suppression locations (machine-level first, wins id collisions): - machine-level: ${SH_SECURITY_SUPPRESSIONS_DIR:-~/.config/sea-haven/security-review}//suppressions.json - repo-local: /.security-review/suppressions.json An explicit --suppressions still overrides, so the hook and existing callers are unaffected. Degrades gracefully off-Mac (repo-local only); fail-safe on an unparseable file (suppresses nothing → blocks). SECURITY (/sh-security-review, 2026-07-13): fan-out + proof-or-kill confirmed one HIGH — the git-tracked repo-local suppressions.json lets anyone who can commit to a scanned repo suppress a real finding and PASS an automated run (verified by an actual exploit run; same posture nightly_sweep already had). ACCEPTED-RISK per Adam on the condition that the automated scanners only ever target trusted repos (no unreviewed untrusted contributions). Documented in the auto-resolve block, README trust-model note, and a hard warning in sweep-targets.txt. Four other candidates downgraded to low/pre-existing. Verified: machine-level and repo-local both auto-resolve and suppress; explicit --suppressions override still blocks; simulated off-Mac host keeps repo-local and correctly re-blocks machine-level-only FPs. --- README.md | 22 +++++++++++++++++++--- review.sh | 39 +++++++++++++++++++++++++++++++++++++++ sweep-targets.txt | 11 +++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a887b78..1ae68e7 100644 --- a/README.md +++ b/README.md @@ -40,13 +40,29 @@ The global mode sets `git config --global core.hooksPath ~/.config/git/hooks`. S its own local `core.hooksPath` overrides the global hook — install per-repo there. See memory `reference_global_security_review_hook`. -**Suppressing a false positive.** A written justification is required and is surfaced in the report. The -hooks resolve a suppressions file in this order: +**Suppressing a false positive.** A written justification is required and is surfaced in the report. +When no `--suppressions FILE` is passed, `review.sh` auto-resolves and **merges** suppressions from +two locations (an explicit `--suppressions` still overrides both): 1. **Machine-level (preferred), kept out of repo history:** `${SH_SECURITY_SUPPRESSIONS_DIR:-~/.config/sea-haven/security-review}//suppressions.json` (override the base dir with `SH_SECURITY_SUPPRESSIONS_DIR`). Keeps a suppression from becoming a permanent in-history "ignore." -2. **Repo-local fallback:** `/.security-review/suppressions.json` (used only if no machine-level file exists). +2. **Repo-local:** `/.security-review/suppressions.json` (tracked; travels with the repo). + +Both files' `.suppressions[]` are concatenated (machine-level first, so it wins any id collision). +This means every entry point — the pre-push hook, the nightly sweep, on-demand/CI runs, and the +Open SWE daily-report automation — resolves suppressions identically. **Portability note:** hosts that +cannot see the Mac's `~/.config` (the Open SWE automation, the R720 VM) get only the tracked repo-local +file, so a suppression that must be honored off-Mac has to live repo-local. Fail-safe: if a file is +present but unparseable, nothing is suppressed (the gate blocks). + +> **⚠️ Trust model — automated scanners must target TRUSTED repos only.** The repo-local +> `.security-review/suppressions.json` is git-tracked, so anyone who can land a commit in a scanned +> repo can suppress a real finding by committing it alongside the vuln (scanner IDs are deterministic). +> `/sh-security-review` confirmed this as a HIGH gate-bypass; it is **accepted** on the condition that +> the automated scanners (Open SWE daily report, nightly sweep) only ever scan repos that do **not** +> merge untrusted contributions without human review. See `sweep-targets.txt`. Relaxing that scoping +> requires a trust check first (signed / CODEOWNERS-verified repo-local suppressions). Same JSON either place: `{"suppressions":[{"id":"","justification":"…"}]}`. Caveat: machine-level files are keyed by **repo basename**, so two repos sharing a name collide — fine for the diff --git a/review.sh b/review.sh index 2314db5..1bf282e 100755 --- a/review.sh +++ b/review.sh @@ -32,6 +32,45 @@ TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT ALL="$TMP/all.json"; echo '[]' > "$ALL" add() { jq --argjson add "$1" '. + $add' "$ALL" > "$ALL.t" && mv "$ALL.t" "$ALL"; } +# --- Auto-resolve suppressions when --suppressions was not passed --- +# Mirrors the pre-push hook's resolution, but MERGES machine-level + repo-local +# (the hook uses XOR: first match wins) and degrades gracefully. On hosts without +# the machine-level dir — the Open SWE daily-report automation and the R720 VM, +# which clone repos but cannot see ~/.config on Adam's Mac — only the tracked +# repo-local file is used, so suppressions travel inside the cloned repo. On the +# Mac both files are merged. An explicit --suppressions FILE still overrides this, +# so the hook and any existing caller are unaffected. Fail-safe: if resolution or +# the merge fails, SUPPRESSIONS stays empty and the gate suppresses nothing (blocks). +# +# SECURITY — ACCEPTED RISK (Adam, 2026-07-13, /sh-security-review confirmed HIGH): +# The repo-local .security-review/suppressions.json is GIT-TRACKED, so anyone who +# can land a commit in a scanned repo can ship a real finding together with a +# suppression for it (scanner IDs are deterministic/precomputable) and make an +# AUTOMATED bare-review.sh run PASS. This is the same trust posture nightly_sweep +# already used. It is ACCEPTED on the condition that the automated scanners +# (Open SWE daily report, nightly sweep) only ever target TRUSTED repos — no repo +# that merges untrusted contributions without human review may be added to their +# target lists. See sweep-targets.txt and memory reference_global_security_review_hook. +# Do NOT relax that scoping without adding a trust check (signed/CODEOWNERS-verified +# repo-local suppressions). +if [ -z "$SUPPRESSIONS" ]; then + _repo_root="$(git -C "$TARGET" rev-parse --show-toplevel 2>/dev/null || echo "$TARGET")" + _machine_sup="${SH_SECURITY_SUPPRESSIONS_DIR:-$HOME/.config/sea-haven/security-review}/$(basename "$_repo_root")/suppressions.json" + _repo_sup="$_repo_root/.security-review/suppressions.json" + _sup_files=() + [ -f "$_machine_sup" ] && _sup_files+=("$_machine_sup") # machine-level first → wins id collisions + [ -f "$_repo_sup" ] && _sup_files+=("$_repo_sup") + if [ "${#_sup_files[@]}" -gt 0 ]; then + _merged="$TMP/suppressions.merged.json" + if jq -s '{suppressions: (map(.suppressions // []) | add)}' "${_sup_files[@]}" > "$_merged" 2>/dev/null; then + SUPPRESSIONS="$_merged" + echo "== Suppressions auto-resolved: ${_sup_files[*]} ==" >&2 + else + echo " [WARN] suppression file(s) present but unparseable; suppressing nothing: ${_sup_files[*]}" >&2 + fi + fi +fi + # Map a scope list into find paths under TARGET (default: whole target). scope_paths() { if [ -n "$SCOPE" ]; then for d in $SCOPE; do [ -e "$TARGET/$d" ] && echo "$TARGET/$d"; done diff --git a/sweep-targets.txt b/sweep-targets.txt index 9be820e..c17f2e4 100644 --- a/sweep-targets.txt +++ b/sweep-targets.txt @@ -2,6 +2,17 @@ # Lines starting with '#' and blank lines are ignored. ~ is expanded. # Override at runtime with the TARGETS env var (space-separated paths). # +# ┌─ SECURITY — TRUSTED REPOS ONLY (accepted-risk condition, 2026-07-13) ─────────┐ +# │ review.sh auto-loads each scanned repo's GIT-TRACKED │ +# │ .security-review/suppressions.json. A contributor who can land a commit can │ +# │ therefore suppress a real finding (ship the vuln + its own suppression) and │ +# │ make the automated run PASS — /sh-security-review confirmed this as HIGH and │ +# │ it was ACCEPTED only on the condition that this list (and any target list the │ +# │ Open SWE daily-report automation uses) contains ONLY trusted repos. Do NOT │ +# │ add a repo that merges untrusted contributions without human review. The same │ +# │ rule applies to the Open SWE daily automation's target set — keep it in sync. │ +# └───────────────────────────────────────────────────────────────────────────────┘ +# # NOTE: the testbed canary corpus is ALWAYS scanned by nightly_sweep.sh as the # anti-complacency check; do NOT list it here (it is handled separately). #