Fix Opengrep differential scan re-reporting pre-existing findings as new (SEC-1975)

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

@@ -0,0 +1,27 @@
+name: Opengrep action tests
+
+# Regression tests for the Opengrep composite action's reporting logic —
+# specifically the stable-identity "new findings" diff in opengrep-report.sh
+# (SEC-1975). Pure bash + jq, no opengrep install needed.
+
+on:
+  pull_request:
+    paths:
+      - 'sast/opengrep/scripts/**'
+      - 'sast/opengrep/action.yml'
+      - 'sast/opengrep/tests/report-diff-test.sh'
+      - '.github/workflows/test-opengrep-action.yml'
+  merge_group:
+
+permissions:
+  contents: read
+
+jobs:
+  report-diff-tests:
+    name: opengrep/report-diff-tests
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+
+      - name: Run report-script diff tests
+        run: bash sast/opengrep/tests/report-diff-test.sh

sast/opengrep/README.md

@@ -1,6 +1,6 @@
 # opengrep
 
-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.
+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.
 
 ## Usage
 

sast/opengrep/action.yml

@@ -93,104 +93,81 @@ runs:
       env:
         BASE_SHA: ${{ inputs.baseline-sha }}
       run: |
-        # Fetch the base commit into the object store.
+        # Fetch the base commit (and its tree) so a worktree can be checked out
+        # at it for the baseline scan. --depth=1 is enough: we full-scan the base
+        # tree, we don't need history or a merge base.
         if ! git fetch --depth=1 origin "$BASE_SHA" 2>/dev/null; then
           echo "::warning::Failed to fetch baseline commit ${BASE_SHA} — all findings will be treated as new"
           echo "available=false" >> "$GITHUB_OUTPUT"
           exit 0
         fi
 
-        # --baseline-commit uses git diff --merge-base which requires the
-        # merge base to be reachable. A shallow clone (depth 1) hides the
-        # commit's parents, so deepen by 1 to expose them.
-        git fetch --deepen=1 origin 2>/dev/null || true
-
         echo "available=true" >> "$GITHUB_OUTPUT"
 
-    - name: Differential scan
+    - name: Scan
       shell: bash
       env:
         BASELINE_SHA: ${{ steps.baseline.outputs.available == 'true' && inputs.baseline-sha || '' }}
         ADDITIONAL_CONFIG: ${{ inputs.config }}
       run: |
-        SCAN_ARGS=(scan --no-rewrite-rule-ids --config "${GITHUB_ACTION_PATH}/rules/")
-        if [ -n "$ADDITIONAL_CONFIG" ]; then
-          SCAN_ARGS+=(--config "$ADDITIONAL_CONFIG")
+        set -uo pipefail
+
+        # We full-scan BOTH the head tree and the base tree with the SAME
+        # ruleset, then diff by stable identity in opengrep-report.sh — instead
+        # of opengrep's --baseline-commit, whose location/content fingerprint
+        # re-reports a pre-existing whole-file finding (e.g. missing-permissions
+        # over a job) as new whenever any line in the file is edited (SEC-1975).
+        RULES_DIR="${GITHUB_ACTION_PATH}/rules/"
+        # Resolve a local additional-config path to absolute so the baseline
+        # scan (run from a worktree in a different cwd) uses the identical rules.
+        if [ -n "$ADDITIONAL_CONFIG" ] && [ -e "$ADDITIONAL_CONFIG" ]; then
+          ADDITIONAL_CONFIG="$(realpath "$ADDITIONAL_CONFIG")"
         fi
-        SCAN_ARGS+=(--json --error)
-        if [ -n "$BASELINE_SHA" ]; then
-          SCAN_ARGS+=(--baseline-commit "$BASELINE_SHA")
-        fi
-        SCAN_ARGS+=(.)
-
-        set +e
-        opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/new-findings.json" 2>"${RUNNER_TEMP}/diff-scan.stderr"
-        exit_code=$?
-        set -e
-
-        # 0 = no new findings, 1 = new findings (with --error), 2+ = scan error.
-        case $exit_code in
-          0|1) ;;
-          *)
-            # --baseline-commit can fail in shallow clones. Fall back to a
-            # non-differential scan so the check still runs (all findings = new).
-            if [ -n "$BASELINE_SHA" ]; then
-              echo "::warning::Differential scan failed (exit ${exit_code}) — retrying without --baseline-commit (all findings will be treated as new)"
-              if [ -s "${RUNNER_TEMP}/diff-scan.stderr" ]; then
-                echo "::group::opengrep scan stderr (differential)"
-                cat "${RUNNER_TEMP}/diff-scan.stderr" >&2
-                echo "::endgroup::"
-              fi
-
-              SCAN_ARGS=(scan --no-rewrite-rule-ids --config "${GITHUB_ACTION_PATH}/rules/")
-              if [ -n "$ADDITIONAL_CONFIG" ]; then
-                SCAN_ARGS+=(--config "$ADDITIONAL_CONFIG")
-              fi
-              SCAN_ARGS+=(--json --error .)
-
-              set +e
-              opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/new-findings.json" 2>"${RUNNER_TEMP}/diff-scan.stderr"
-              exit_code=$?
-              set -e
-
-              case $exit_code in
-                0|1) ;;
-                *)
-                  echo "::group::opengrep scan stderr"
-                  cat "${RUNNER_TEMP}/diff-scan.stderr" >&2
-                  echo "::endgroup::"
-                  exit "$exit_code"
-                  ;;
-              esac
-            else
-              echo "::group::opengrep scan stderr"
-              cat "${RUNNER_TEMP}/diff-scan.stderr" >&2
-              echo "::endgroup::"
-              exit "$exit_code"
-            fi
-            ;;
-        esac
-
-    - name: Full scan
-      shell: bash
-      env:
-        ADDITIONAL_CONFIG: ${{ inputs.config }}
-      run: |
-        SCAN_ARGS=(scan --no-rewrite-rule-ids --config "${GITHUB_ACTION_PATH}/rules/")
+        # No --error: blocking is decided by the stable-identity diff in the
+        # report step, not by opengrep's exit code. Scanning '.' from each tree's
+        # own root keeps finding paths repo-relative and comparable across trees.
+        SCAN_ARGS=(scan --no-rewrite-rule-ids --config "$RULES_DIR")
         if [ -n "$ADDITIONAL_CONFIG" ]; then
           SCAN_ARGS+=(--config "$ADDITIONAL_CONFIG")
         fi
         SCAN_ARGS+=(--json .)
 
+        EMPTY='{"results":[],"errors":[],"paths":{"scanned":[]}}'
+
+        # ---- Head scan (authoritative: a real scan error fails the job) ----
         set +e
-        opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/all-findings.json" 2>"${RUNNER_TEMP}/full-scan.stderr"
-        exit_code=$?
+        opengrep "${SCAN_ARGS[@]}" > "${RUNNER_TEMP}/head-findings.json" 2>"${RUNNER_TEMP}/head-scan.stderr"
+        head_rc=$?
         set -e
+        if [ "$head_rc" -gt 1 ] || [ ! -s "${RUNNER_TEMP}/head-findings.json" ]; then
+          echo "::group::opengrep head scan stderr"
+          cat "${RUNNER_TEMP}/head-scan.stderr" >&2 || true
+          echo "::endgroup::"
+          echo "::error::opengrep scan failed (exit ${head_rc})"
+          exit "$([ "$head_rc" -gt 1 ] && echo "$head_rc" || echo 1)"
+        fi
 
-        # Informational only — warn on errors but don't fail the step.
-        if [ "$exit_code" -ne 0 ]; then
-          echo "::warning::Full scan failed (exit ${exit_code}) — total findings summary may be incomplete"
-          echo '{"results":[],"errors":[],"paths":{"scanned":[]}}' > "${RUNNER_TEMP}/all-findings.json"
+        # ---- Baseline scan (best-effort: any problem -> empty baseline, so
+        #      every head finding counts as new; a baseline issue never hides) -
+        printf '%s\n' "$EMPTY" > "${RUNNER_TEMP}/baseline-findings.json"
+        if [ -n "$BASELINE_SHA" ]; then
+          WORKTREE="${RUNNER_TEMP}/opengrep-baseline"
+          rm -rf "$WORKTREE"
+          if git worktree add --detach --force "$WORKTREE" "$BASELINE_SHA" >/dev/null 2>&1; then
+            set +e
+            ( cd "$WORKTREE" && opengrep "${SCAN_ARGS[@]}" ) \
+              > "${RUNNER_TEMP}/baseline-findings.json.tmp" 2>"${RUNNER_TEMP}/baseline-scan.stderr"
+            base_rc=$?
+            set -e
+            if [ "$base_rc" -le 1 ] && [ -s "${RUNNER_TEMP}/baseline-findings.json.tmp" ]; then
+              mv "${RUNNER_TEMP}/baseline-findings.json.tmp" "${RUNNER_TEMP}/baseline-findings.json"
+            else
+              echo "::warning::Baseline scan failed (exit ${base_rc}) — treating all findings as new"
+            fi
+            git worktree remove --force "$WORKTREE" >/dev/null 2>&1 || true
+          else
+            echo "::warning::Could not create baseline worktree at ${BASELINE_SHA} — treating all findings as new"
+          fi
         fi
 
     - name: Report
@@ -202,9 +179,10 @@ runs:
         PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
         PR_COMMENTS_ENABLED: ${{ inputs.pr-comments }}
       run: |
-        # Ensure files exist even if prior steps were skipped.
-        touch "${RUNNER_TEMP}/new-findings.json"
-        touch "${RUNNER_TEMP}/all-findings.json"
+        # Ensure files exist even if prior steps were skipped (missing/empty
+        # baseline => every head finding is treated as new).
+        touch "${RUNNER_TEMP}/baseline-findings.json"
+        touch "${RUNNER_TEMP}/head-findings.json"
         "$GITHUB_ACTION_PATH/scripts/opengrep-report.sh" \
-          "${RUNNER_TEMP}/new-findings.json" \
-          "${RUNNER_TEMP}/all-findings.json"
+          "${RUNNER_TEMP}/baseline-findings.json" \
+          "${RUNNER_TEMP}/head-findings.json"

sast/opengrep/scripts/opengrep-report.sh

@@ -3,16 +3,22 @@
 # with ::error:: annotations for new findings.
 #
 # Usage:
-#   opengrep-report.sh <new-findings.json> <all-findings.json>
+#   opengrep-report.sh <baseline-findings.json> <head-findings.json>
+#
+# "New" findings are computed here as the head full scan minus the baseline
+# full scan, keyed by stable identity (see compute_new_findings) — NOT by
+# opengrep's --baseline-commit, whose location/content fingerprint re-reports
+# pre-existing whole-file findings as new whenever the file is edited.
 #
 # Environment variables (set automatically by GitHub Actions):
 #   GITHUB_STEP_SUMMARY — path to the job summary file (falls back to stdout)
 #   GITHUB_OUTPUT       — path to expose step outputs
 #
 # Local testing:
-#   opengrep scan --config rules/ --json --error . > /tmp/new.json 2>/dev/null; true
-#   opengrep scan --config rules/ --json . > /tmp/all.json
-#   ./opengrep-report.sh /tmp/new.json /tmp/all.json
+#   opengrep scan --config rules/ --json . > /tmp/head.json
+#   git worktree add --detach /tmp/base <base-sha>
+#   ( cd /tmp/base && opengrep scan --config rules/ --json . ) > /tmp/baseline.json
+#   ./opengrep-report.sh /tmp/baseline.json /tmp/head.json
 #
 # Expected Opengrep JSON format (same as Semgrep):
 #   {
@@ -59,6 +65,77 @@ count_scanned() {
   jq '.paths.scanned | length' "$json_file" 2>/dev/null || echo 0
 }
 
+# jq program: head findings minus baseline findings.
+#
+# Identity is the STABLE key (check_id, path) plus a per-key COUNT — never the
+# line number or the matched text. Both of those shift when an unrelated line
+# *inside* a whole-block "absence" match (e.g. the missing-permissions rule,
+# which matches a whole job) is edited, which is exactly why opengrep's
+# --baseline-commit re-reported pre-existing findings as new on PRs that only
+# touched the file (SEC-1975).
+#
+# Per key, new count = max(0, head_count - baseline_count):
+#   - edit inside a pre-existing match -> count unchanged -> 0 new (no noise)
+#   - a genuinely new gap (new job/file/line) -> count rises -> still reported
+# When a key gained findings, the head findings least like the baseline
+# (content- and start-line-novel) are surfaced first, then capped at the delta,
+# so the annotation points at the genuinely new finding rather than an edited
+# pre-existing one.
+NEW_FINDINGS_JQ='
+($base[0].results // []) as $br
+| (.results // []) as $hr
+| ( $br
+    | group_by([.check_id, .path])
+    | map({ k: ([.[0].check_id, .[0].path] | @json),
+            n: length,
+            sigs: [.[].extra.lines],
+            lines: [.[].start.line] })
+    | map({ (.k): . }) | add // {} ) as $bmap
+| [ $hr
+    | group_by([.check_id, .path])[]
+    | ([.[0].check_id, .[0].path] | @json) as $k
+    | ($bmap[$k].n // 0) as $bn
+    | ($bmap[$k].sigs // []) as $bsigs
+    | ($bmap[$k].lines // []) as $blines
+    | ((length - $bn) | if . < 0 then 0 else . end) as $delta
+    | ( map(. + { _novel:
+            ( (if ([.extra.lines] - $bsigs) | length > 0 then 1 else 0 end)
+            + (if ([.start.line] - $blines) | length > 0 then 1 else 0 end) ) })
+        | sort_by(-._novel)
+        | .[0:$delta]
+        | map(del(._novel)) ) ]
+| add // []
+| { results: ., errors: [], paths: {} }
+'
+
+# compute_new_findings <baseline-json> <head-json> <out-json>
+# Write the new-findings JSON (a subset of the head results, preserved verbatim)
+# to <out-json>. A missing/empty baseline or a diff failure conservatively
+# treats every head finding as new, so a baseline problem never hides a finding.
+compute_new_findings() {
+  local baseline_json=$1 head_json=$2 out_json=$3
+
+  if [ ! -s "$head_json" ]; then
+    printf '%s\n' '{"results":[],"errors":[],"paths":{}}' > "$out_json"
+    return
+  fi
+
+  local base_json=$baseline_json cleanup=""
+  if [ ! -s "$baseline_json" ]; then
+    base_json=$(mktemp)
+    cleanup=$base_json
+    printf '%s\n' '{"results":[]}' > "$base_json"
+  fi
+
+  if ! jq --slurpfile base "$base_json" "$NEW_FINDINGS_JQ" "$head_json" \
+      > "$out_json" 2>/dev/null; then
+    cp "$head_json" "$out_json"
+  fi
+
+  [ -n "$cleanup" ] && rm -f "$cleanup"
+  return 0
+}
+
 # Emit ::error:: annotations for each finding (visible on PR diff).
 emit_annotations() {
   local json_file=$1
@@ -465,26 +542,35 @@ write_summary() {
 # ---------------------------------------------------------------------------
 
 main() {
-  local new_json="${1:?Usage: opengrep-report.sh <new-findings.json> <all-findings.json>}"
-  local all_json="${2:?Usage: opengrep-report.sh <new-findings.json> <all-findings.json>}"
+  local baseline_json="${1:?Usage: opengrep-report.sh <baseline-findings.json> <head-findings.json>}"
+  local head_json="${2:?Usage: opengrep-report.sh <baseline-findings.json> <head-findings.json>}"
 
   if ! command -v jq &>/dev/null; then
     echo "::error::jq is required but not installed — use a GitHub-hosted runner or install jq"
     exit 1
   fi
 
+  # New findings = head full scan minus baseline full scan, by stable identity.
+  local new_json
+  new_json=$(mktemp)
+  # Bake the path into the trap now (double quotes): the EXIT trap fires in the
+  # global scope where this local would be unbound under `set -u`.
+  # shellcheck disable=SC2064  # intentional: expand $new_json at definition time
+  trap "rm -f '$new_json'" EXIT
+  compute_new_findings "$baseline_json" "$head_json" "$new_json"
+
   local new_count all_count scanned_count
   new_count=$(count_findings "$new_json")
-  all_count=$(count_findings "$all_json")
-  scanned_count=$(count_scanned "$all_json")
+  all_count=$(count_findings "$head_json")
+  scanned_count=$(count_scanned "$head_json")
 
   # Emit ::error:: annotations for new findings (visible on PR diff).
   if [ "$new_count" -gt 0 ]; then
     emit_annotations "$new_json"
   fi
 
   # Write GitHub job summary.
-  write_summary "$new_json" "$all_json" "$new_count" "$all_count" "$scanned_count"
+  write_summary "$new_json" "$head_json" "$new_count" "$all_count" "$scanned_count"
 
   # Post inline PR review comments for new findings (best-effort — never
   # blocks outputs or the exit code, which are the authoritative signals).