Skip to content

Commit 1ddf447

Browse files
picatzpicatz
authored
Fix Opengrep differential scan re-reporting pre-existing findings as new (SEC-1975) (#26)
Opengrep's --baseline-commit classifies a finding as new/old via a location- and content-sensitive fingerprint of the matched range. Whole-file "absence" rules (e.g. missing-permissions, which matches a whole job) span a large range, so editing any line inside it (such as pinning an action SHA) changes the fingerprint and the pre-existing finding is mis-reported as new. This blocked the SHA-pinning campaign and any workflow edit on repos lacking an explicit permissions: block (SEC-1975). Replace --baseline-commit with a stable-identity diff: full-scan both the head tree and the base tree (checked out in a git worktree) with the same ruleset, then in opengrep-report.sh treat a finding as new only when its (rule, file) key gained findings — new count = max(0, head_count - baseline_count), never keyed on line number or matched text. Edits inside a pre-existing match no longer re-report it; a genuinely new gap (added job/file/line) still is, and still blocks. A missing/failed baseline scan conservatively treats every head finding as new so a baseline problem never hides a finding.
1 parent 5a0bc36 commit 1ddf447

5 files changed

Lines changed: 292 additions & 91 deletions

File tree

‎.github/workflows/test-opengrep-action.yml‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
name: Opengrep action tests
2+
3+
# Regression tests for the Opengrep composite action's reporting logic —
4+
# specifically the stable-identity "new findings" diff in opengrep-report.sh
5+
# (SEC-1975). Pure bash + jq, no opengrep install needed.
6+
7+
on:
8+
pull_request:
9+
paths:
10+
- 'sast/opengrep/scripts/**'
11+
- 'sast/opengrep/action.yml'
12+
- 'sast/opengrep/tests/report-diff-test.sh'
13+
- '.github/workflows/test-opengrep-action.yml'
14+
merge_group:
15+
16+
permissions:
17+
contents: read
18+
19+
jobs:
20+
report-diff-tests:
21+
name: opengrep/report-diff-tests
22+
runs-on: ubuntu-latest
23+
steps:
24+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
25+
26+
- name: Run report-script diff tests
27+
run: bash sast/opengrep/tests/report-diff-test.sh

‎sast/opengrep/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# opengrep
22

3-
Differential SAST with [Opengrep](https://opengrep.dev) for pull requests. Scans the current branch and compares against the base branch using `--baseline-commit`, **failing only on newly introduced findings**. Pre-existing findings are reported in the job summary but don't block the PR.
3+
Differential SAST with [Opengrep](https://opengrep.dev) for pull requests. Full-scans both the PR head and the base branch, then **fails only on newly introduced findings** — a finding is "new" only if its `(rule, file)` had fewer findings on the base. Pre-existing findings are reported in the job summary but don't block the PR. Identifying new findings by stable identity rather than by line/content fingerprint means editing a file (e.g. pinning an action SHA) does not re-report a pre-existing whole-file finding such as a missing `permissions:` block.
44

55
## Usage
66

‎sast/opengrep/action.yml‎

Lines changed: 62 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -93,104 +93,84 @@ runs:
9393
env:
9494
BASE_SHA: ${{ inputs.baseline-sha }}
9595
run: |
96-
# Fetch the base commit into the object store.
96+
# Fetch the base commit (and its tree) so a worktree can be checked out
97+
# at it for the baseline scan. --depth=1 is enough: we full-scan the base
98+
# tree, we don't need history or a merge base.
9799
if ! git fetch --depth=1 origin "$BASE_SHA" 2>/dev/null; then
98100
echo "::warning::Failed to fetch baseline commit ${BASE_SHA} — all findings will be treated as new"
99101
echo "available=false" >> "$GITHUB_OUTPUT"
100102
exit 0
101103
fi
102104
103-
# --baseline-commit uses git diff --merge-base which requires the
104-
# merge base to be reachable. A shallow clone (depth 1) hides the
105-
# commit's parents, so deepen by 1 to expose them.
106-
git fetch --deepen=1 origin 2>/dev/null || true
107-
108105
echo "available=true" >> "$GITHUB_OUTPUT"
109106
110-
- name: Differential scan
107+
- name: Scan
111108
shell: bash
112109
env:
113110
BASELINE_SHA: ${{ steps.baseline.outputs.available == 'true' && inputs.baseline-sha || '' }}
114111
ADDITIONAL_CONFIG: ${{ inputs.config }}
115112
run: |
116-
SCAN_ARGS=(scan --no-rewrite-rule-ids --config "${GITHUB_ACTION_PATH}/rules/")
117-
if [ -n "$ADDITIONAL_CONFIG" ]; then
118-
SCAN_ARGS+=(--config "$ADDITIONAL_CONFIG")
113+
set -uo pipefail
114+
115+
# We full-scan BOTH the head tree and the base tree with the SAME
116+
# ruleset, then diff by stable identity in opengrep-report.sh — instead
117+
# of opengrep's --baseline-commit, whose location/content fingerprint
118+
# re-reports a pre-existing whole-file finding (e.g. missing-permissions
119+
# over a job) as new whenever any line in the file is edited (SEC-1975).
120+
RULES_DIR="${GITHUB_ACTION_PATH}/rules/"
121+
# Resolve a local additional-config path to absolute so the baseline
122+
# scan (run from a worktree in a different cwd) uses the identical rules.
123+
if [ -n "$ADDITIONAL_CONFIG" ] && [ -e "$ADDITIONAL_CONFIG" ]; then
124+
ADDITIONAL_CONFIG="$(realpath "$ADDITIONAL_CONFIG")"
119125
fi
120-
SCAN_ARGS+=(--json --error)
121-
if [ -n "$BASELINE_SHA" ]; then
122-
SCAN_ARGS+=(--baseline-commit "$BASELINE_SHA")
123-
fi
124-
SCAN_ARGS+=(.)
125-
126-
set +e
127-
opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/new-findings.json" 2>"${RUNNER_TEMP}/diff-scan.stderr"
128-
exit_code=$?
129-
set -e
130-
131-
# 0 = no new findings, 1 = new findings (with --error), 2+ = scan error.
132-
case $exit_code in
133-
0|1) ;;
134-
*)
135-
# --baseline-commit can fail in shallow clones. Fall back to a
136-
# non-differential scan so the check still runs (all findings = new).
137-
if [ -n "$BASELINE_SHA" ]; then
138-
echo "::warning::Differential scan failed (exit ${exit_code}) — retrying without --baseline-commit (all findings will be treated as new)"
139-
if [ -s "${RUNNER_TEMP}/diff-scan.stderr" ]; then
140-
echo "::group::opengrep scan stderr (differential)"
141-
cat "${RUNNER_TEMP}/diff-scan.stderr" >&2
142-
echo "::endgroup::"
143-
fi
144-
145-
SCAN_ARGS=(scan --no-rewrite-rule-ids --config "${GITHUB_ACTION_PATH}/rules/")
146-
if [ -n "$ADDITIONAL_CONFIG" ]; then
147-
SCAN_ARGS+=(--config "$ADDITIONAL_CONFIG")
148-
fi
149-
SCAN_ARGS+=(--json --error .)
150-
151-
set +e
152-
opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/new-findings.json" 2>"${RUNNER_TEMP}/diff-scan.stderr"
153-
exit_code=$?
154-
set -e
155-
156-
case $exit_code in
157-
0|1) ;;
158-
*)
159-
echo "::group::opengrep scan stderr"
160-
cat "${RUNNER_TEMP}/diff-scan.stderr" >&2
161-
echo "::endgroup::"
162-
exit "$exit_code"
163-
;;
164-
esac
165-
else
166-
echo "::group::opengrep scan stderr"
167-
cat "${RUNNER_TEMP}/diff-scan.stderr" >&2
168-
echo "::endgroup::"
169-
exit "$exit_code"
170-
fi
171-
;;
172-
esac
173-
174-
- name: Full scan
175-
shell: bash
176-
env:
177-
ADDITIONAL_CONFIG: ${{ inputs.config }}
178-
run: |
179-
SCAN_ARGS=(scan --no-rewrite-rule-ids --config "${GITHUB_ACTION_PATH}/rules/")
126+
# No --error: blocking is decided by the stable-identity diff in the
127+
# report step, not by opengrep's exit code. Scanning '.' from each tree's
128+
# own root keeps finding paths repo-relative and comparable across trees.
129+
SCAN_ARGS=(scan --no-rewrite-rule-ids --config "$RULES_DIR")
180130
if [ -n "$ADDITIONAL_CONFIG" ]; then
181131
SCAN_ARGS+=(--config "$ADDITIONAL_CONFIG")
182132
fi
183133
SCAN_ARGS+=(--json .)
184134
135+
EMPTY='{"results":[],"errors":[],"paths":{"scanned":[]}}'
136+
137+
# ---- Head scan (authoritative: a real scan error fails the job) ----
185138
set +e
186-
opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/all-findings.json" 2>"${RUNNER_TEMP}/full-scan.stderr"
187-
exit_code=$?
139+
opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/head-findings.json" 2>"${RUNNER_TEMP}/head-scan.stderr"
140+
head_rc=$?
188141
set -e
142+
if [ "$head_rc" -gt 1 ] || [ ! -s "${RUNNER_TEMP}/head-findings.json" ]; then
143+
echo "::group::opengrep head scan stderr"
144+
cat "${RUNNER_TEMP}/head-scan.stderr" >&2 || true
145+
echo "::endgroup::"
146+
echo "::error::opengrep scan failed (exit ${head_rc})"
147+
exit "$([ "$head_rc" -gt 1 ] && echo "$head_rc" || echo 1)"
148+
fi
189149
190-
# Informational only — warn on errors but don't fail the step.
191-
if [ "$exit_code" -ne 0 ]; then
192-
echo "::warning::Full scan failed (exit ${exit_code}) — total findings summary may be incomplete"
193-
echo '{"results":[],"errors":[],"paths":{"scanned":[]}}' > "${RUNNER_TEMP}/all-findings.json"
150+
# ---- Baseline scan (best-effort: any problem -> empty baseline, so
151+
# every head finding counts as new; a baseline issue never hides) -
152+
printf '%s\n' "$EMPTY" > "${RUNNER_TEMP}/baseline-findings.json"
153+
if [ -n "$BASELINE_SHA" ]; then
154+
WORKTREE="${RUNNER_TEMP}/opengrep-baseline"
155+
rm -rf "$WORKTREE"
156+
if git worktree add --detach --force "$WORKTREE" "$BASELINE_SHA" >/dev/null 2>&1; then
157+
set +e
158+
( cd "$WORKTREE" && opengrep "${SCAN_ARGS[@]}" ) \
159+
> "${RUNNER_TEMP}/baseline-findings.json.tmp" 2>"${RUNNER_TEMP}/baseline-scan.stderr"
160+
base_rc=$?
161+
set -e
162+
if [ "$base_rc" -le 1 ] && [ -s "${RUNNER_TEMP}/baseline-findings.json.tmp" ]; then
163+
mv "${RUNNER_TEMP}/baseline-findings.json.tmp" "${RUNNER_TEMP}/baseline-findings.json"
164+
else
165+
echo "::warning::Baseline scan failed (exit ${base_rc}) — treating all findings as new"
166+
fi
167+
# No-op on the success path (mv consumed it); drops the partial file
168+
# a failed/errored scan would otherwise leave behind.
169+
rm -f "${RUNNER_TEMP}/baseline-findings.json.tmp"
170+
git worktree remove --force "$WORKTREE" >/dev/null 2>&1 || true
171+
else
172+
echo "::warning::Could not create baseline worktree at ${BASELINE_SHA} — treating all findings as new"
173+
fi
194174
fi
195175
196176
- name: Report
@@ -202,9 +182,10 @@ runs:
202182
PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
203183
PR_COMMENTS_ENABLED: ${{ inputs.pr-comments }}
204184
run: |
205-
# Ensure files exist even if prior steps were skipped.
206-
touch "${RUNNER_TEMP}/new-findings.json"
207-
touch "${RUNNER_TEMP}/all-findings.json"
185+
# Ensure files exist even if prior steps were skipped (missing/empty
186+
# baseline => every head finding is treated as new).
187+
touch "${RUNNER_TEMP}/baseline-findings.json"
188+
touch "${RUNNER_TEMP}/head-findings.json"
208189
"$GITHUB_ACTION_PATH/scripts/opengrep-report.sh" \
209-
"${RUNNER_TEMP}/new-findings.json" \
210-
"${RUNNER_TEMP}/all-findings.json"
190+
"${RUNNER_TEMP}/baseline-findings.json" \
191+
"${RUNNER_TEMP}/head-findings.json"

‎sast/opengrep/scripts/opengrep-report.sh‎

Lines changed: 95 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,22 @@
33
# with ::error:: annotations for new findings.
44
#
55
# Usage:
6-
# opengrep-report.sh <new-findings.json> <all-findings.json>
6+
# opengrep-report.sh <baseline-findings.json> <head-findings.json>
7+
#
8+
# "New" findings are computed here as the head full scan minus the baseline
9+
# full scan, keyed by stable identity (see compute_new_findings) — NOT by
10+
# opengrep's --baseline-commit, whose location/content fingerprint re-reports
11+
# pre-existing whole-file findings as new whenever the file is edited.
712
#
813
# Environment variables (set automatically by GitHub Actions):
914
# GITHUB_STEP_SUMMARY — path to the job summary file (falls back to stdout)
1015
# GITHUB_OUTPUT — path to expose step outputs
1116
#
1217
# Local testing:
13-
# opengrep scan --config rules/ --json --error . > /tmp/new.json 2>/dev/null; true
14-
# opengrep scan --config rules/ --json . > /tmp/all.json
15-
# ./opengrep-report.sh /tmp/new.json /tmp/all.json
18+
# opengrep scan --config rules/ --json . > /tmp/head.json
19+
# git worktree add --detach /tmp/base <base-sha>
20+
# ( cd /tmp/base && opengrep scan --config rules/ --json . ) > /tmp/baseline.json
21+
# ./opengrep-report.sh /tmp/baseline.json /tmp/head.json
1622
#
1723
# Expected Opengrep JSON format (same as Semgrep):
1824
# {
@@ -59,6 +65,77 @@ count_scanned() {
5965
jq '.paths.scanned | length' "$json_file" 2>/dev/null || echo 0
6066
}
6167

68+
# jq program: head findings minus baseline findings.
69+
#
70+
# Identity is the STABLE key (check_id, path) plus a per-key COUNT — never the
71+
# line number or the matched text. Both of those shift when an unrelated line
72+
# *inside* a whole-block "absence" match (e.g. the missing-permissions rule,
73+
# which matches a whole job) is edited, which is exactly why opengrep's
74+
# --baseline-commit re-reported pre-existing findings as new on PRs that only
75+
# touched the file (SEC-1975).
76+
#
77+
# Per key, new count = max(0, head_count - baseline_count):
78+
# - edit inside a pre-existing match -> count unchanged -> 0 new (no noise)
79+
# - a genuinely new gap (new job/file/line) -> count rises -> still reported
80+
# When a key gained findings, the head findings least like the baseline
81+
# (content- and start-line-novel) are surfaced first, then capped at the delta,
82+
# so the annotation points at the genuinely new finding rather than an edited
83+
# pre-existing one.
84+
NEW_FINDINGS_JQ='
85+
($base[0].results // []) as $br
86+
| (.results // []) as $hr
87+
| ( $br
88+
| group_by([.check_id, .path])
89+
| map({ k: ([.[0].check_id, .[0].path] | @json),
90+
n: length,
91+
sigs: [.[].extra.lines],
92+
lines: [.[].start.line] })
93+
| map({ (.k): . }) | add // {} ) as $bmap
94+
| [ $hr
95+
| group_by([.check_id, .path])[]
96+
| ([.[0].check_id, .[0].path] | @json) as $k
97+
| ($bmap[$k].n // 0) as $bn
98+
| ($bmap[$k].sigs // []) as $bsigs
99+
| ($bmap[$k].lines // []) as $blines
100+
| ((length - $bn) | if . < 0 then 0 else . end) as $delta
101+
| ( map(. + { _novel:
102+
( (if ([.extra.lines] - $bsigs) | length > 0 then 1 else 0 end)
103+
+ (if ([.start.line] - $blines) | length > 0 then 1 else 0 end) ) })
104+
| sort_by(-._novel)
105+
| .[0:$delta]
106+
| map(del(._novel)) ) ]
107+
| add // []
108+
| { results: ., errors: [], paths: {} }
109+
'
110+
111+
# compute_new_findings <baseline-json> <head-json> <out-json>
112+
# Write the new-findings JSON (a subset of the head results, preserved verbatim)
113+
# to <out-json>. A missing/empty baseline or a diff failure conservatively
114+
# treats every head finding as new, so a baseline problem never hides a finding.
115+
compute_new_findings() {
116+
local baseline_json=$1 head_json=$2 out_json=$3
117+
118+
if [ ! -s "$head_json" ]; then
119+
printf '%s\n' '{"results":[],"errors":[],"paths":{}}' > "$out_json"
120+
return
121+
fi
122+
123+
local base_json=$baseline_json cleanup=""
124+
if [ ! -s "$baseline_json" ]; then
125+
base_json=$(mktemp)
126+
cleanup=$base_json
127+
printf '%s\n' '{"results":[]}' > "$base_json"
128+
fi
129+
130+
if ! jq --slurpfile base "$base_json" "$NEW_FINDINGS_JQ" "$head_json" \
131+
> "$out_json" 2>/dev/null; then
132+
cp "$head_json" "$out_json"
133+
fi
134+
135+
[ -n "$cleanup" ] && rm -f "$cleanup"
136+
return 0
137+
}
138+
62139
# Emit ::error:: annotations for each finding (visible on PR diff).
63140
emit_annotations() {
64141
local json_file=$1
@@ -465,26 +542,35 @@ write_summary() {
465542
# ---------------------------------------------------------------------------
466543

467544
main() {
468-
local new_json="${1:?Usage: opengrep-report.sh <new-findings.json> <all-findings.json>}"
469-
local all_json="${2:?Usage: opengrep-report.sh <new-findings.json> <all-findings.json>}"
545+
local baseline_json="${1:?Usage: opengrep-report.sh <baseline-findings.json> <head-findings.json>}"
546+
local head_json="${2:?Usage: opengrep-report.sh <baseline-findings.json> <head-findings.json>}"
470547

471548
if ! command -v jq &>/dev/null; then
472549
echo "::error::jq is required but not installed — use a GitHub-hosted runner or install jq"
473550
exit 1
474551
fi
475552

553+
# New findings = head full scan minus baseline full scan, by stable identity.
554+
local new_json
555+
new_json=$(mktemp)
556+
# Bake the path into the trap now (double quotes): the EXIT trap fires in the
557+
# global scope where this local would be unbound under `set -u`.
558+
# shellcheck disable=SC2064 # intentional: expand $new_json at definition time
559+
trap "rm -f '$new_json'" EXIT
560+
compute_new_findings "$baseline_json" "$head_json" "$new_json"
561+
476562
local new_count all_count scanned_count
477563
new_count=$(count_findings "$new_json")
478-
all_count=$(count_findings "$all_json")
479-
scanned_count=$(count_scanned "$all_json")
564+
all_count=$(count_findings "$head_json")
565+
scanned_count=$(count_scanned "$head_json")
480566

481567
# Emit ::error:: annotations for new findings (visible on PR diff).
482568
if [ "$new_count" -gt 0 ]; then
483569
emit_annotations "$new_json"
484570
fi
485571

486572
# Write GitHub job summary.
487-
write_summary "$new_json" "$all_json" "$new_count" "$all_count" "$scanned_count"
573+
write_summary "$new_json" "$head_json" "$new_count" "$all_count" "$scanned_count"
488574

489575
# Post inline PR review comments for new findings (best-effort — never
490576
# blocks outputs or the exit code, which are the authoritative signals).

0 commit comments

Comments
 (0)