Skip to content

Commit d9fbf1e

Browse files
ralyodioclaude
andcommitted
ci: scope the ThreatCrush PR comment to the files the PR changes
The scan runs against the whole tree and the comment reported the whole tree, so every pull request got the same table regardless of what it touched. #408 changes five files under the games code and drew 53 rows about `install.sh`, the service worker, the DNS client and the Moshpit SQL helpers — nothing it went near. A comment that says the same thing on every PR says nothing. Reviewers learn to scroll past it, and the finding that *is* theirs scrolls past with it. The tree is still scanned in full and the full SARIF still goes to the Security tab, so coverage is unchanged; only what the comment talks about is narrower. The findings outside the diff are counted and pointed at rather than dropped, so "no findings" can never be misread as "the repository is clean". Scoping fails *open*: if the changed-file list cannot be read, every finding is shown and the header says so. That is the opposite of the rest of this workflow, which treats an unknown as NOT RUN — the risk there is claiming a clean scan, and the risk here is hiding a real finding behind an empty scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 09fc528 commit d9fbf1e

1 file changed

Lines changed: 86 additions & 2 deletions

File tree

.github/workflows/threatcrush-scan.yml

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,42 @@ jobs:
6868
echo "::notice::CLI $(threatcrush --version 2>/dev/null || echo unknown) predates --format; converting terminal output instead."
6969
fi
7070
71+
# Which files does this pull request actually touch?
72+
#
73+
# The scan below still covers the whole tree, and the whole tree still
74+
# goes to the Security tab — coverage is unchanged. This list exists only
75+
# to decide what the PR *comment* talks about. A comment that reports
76+
# every finding in the repository says the same thing on every pull
77+
# request, which is the same as saying nothing: a five-file change to the
78+
# games code drew a 53-row table about `install.sh`, the service worker
79+
# and the DNS client, none of which it went near. Reviewers learn to
80+
# scroll past it, and the one finding that *is* theirs scrolls past with
81+
# it.
82+
#
83+
# Paginated deliberately: `listFiles` caps at 100 per page, and a
84+
# truncated list here would silently mark real findings as out-of-scope.
85+
- name: Collect the PR's changed files
86+
id: changed
87+
uses: actions/github-script@v9
88+
with:
89+
script: |
90+
const files = await github.paginate(github.rest.pulls.listFiles, {
91+
owner: context.repo.owner,
92+
repo: context.repo.repo,
93+
pull_number: context.issue.number,
94+
per_page: 100,
95+
});
96+
// Removed files cannot hold a finding; keeping them would only
97+
// widen the set with paths the scan never saw.
98+
const paths = files
99+
.filter((f) => f.status !== 'removed')
100+
.map((f) => f.filename);
101+
require('fs').writeFileSync(
102+
`${process.env.RUNNER_TEMP}/changed-files.txt`,
103+
paths.join('\n'),
104+
);
105+
core.info(`${paths.length} changed file(s) in this PR`);
106+
71107
- name: Scan
72108
id: scan
73109
run: |
@@ -170,6 +206,29 @@ jobs:
170206
results = None
171207
print(f"::warning::could not read SARIF: {err}")
172208
209+
# The comment reports on this pull request. Anything outside it is
210+
# counted and pointed at, never tabulated: it is not news, it did not
211+
# arrive with this branch, and nobody reviewing this diff can act on
212+
# it here.
213+
try:
214+
with open(os.environ["RUNNER_TEMP"] + "/changed-files.txt") as handle:
215+
changed = {line.strip() for line in handle if line.strip()}
216+
except Exception as err:
217+
# Fail *open* on purpose, and only here. Everywhere else in this
218+
# workflow an unknown is treated as "not scanned", because
219+
# claiming a clean scan is the dangerous direction. This one is
220+
# the opposite: if the changed-file list is missing we cannot
221+
# tell which findings are in scope, and the safe move is to show
222+
# all of them rather than hide a real one behind an empty set.
223+
changed = None
224+
print(f"::warning::could not read changed-file list: {err}")
225+
226+
def in_scope(result):
227+
if changed is None:
228+
return True
229+
uri = result["locations"][0]["physicalLocation"]["artifactLocation"]["uri"]
230+
return uri.lstrip("./") in changed
231+
173232
lines = ["## ThreatCrush Security Scan", ""]
174233
175234
# Fail closed: render findings only on positive evidence that a scan
@@ -188,13 +247,25 @@ jobs:
188247
"This is not a clean result. See the job log.",
189248
]
190249
else:
250+
scanned = results
251+
results = [r for r in scanned if in_scope(r)]
252+
elsewhere = len(scanned) - len(results)
253+
191254
counts = {"error": 0, "warning": 0, "note": 0}
192255
for result in results:
193256
level = result.get("level", "warning")
194257
if level in counts:
195258
counts[level] += 1
196259
197-
lines.append(f"**{len(results)}** finding(s)")
260+
# Say which of the two things this number is. On the fail-open
261+
# path it is every finding in the tree, and calling that "in the
262+
# files this PR changes" would be a plain falsehood — the one
263+
# claim this comment must never make is a narrower scope than it
264+
# actually looked at.
265+
scope = "in the files this PR changes" if changed is not None else (
266+
"repository-wide — the changed-file list was unavailable, so nothing could be scoped out"
267+
)
268+
lines.append(f"**{len(results)}** finding(s) {scope}")
198269
lines.append("")
199270
200271
if results:
@@ -222,7 +293,20 @@ jobs:
222293
lines += ["", f"_…and {len(results) - 50} more. Full results in the Security tab._"]
223294
lines += ["", "Snippets are redacted; ThreatCrush never prints matched credential material."]
224295
else:
225-
lines.append("No findings.")
296+
lines.append(f"No findings {scope}.")
297+
298+
# Counted, not hidden. The whole tree was scanned and the whole
299+
# tree is in the Security tab; this line is what says so, so that
300+
# "no findings" above can never be misread as "the repository is
301+
# clean". Rendering these rows here instead would put the same
302+
# unchanged wall of pre-existing findings on every pull request.
303+
if elsewhere:
304+
lines += [
305+
"",
306+
f"_{elsewhere} further finding(s) elsewhere in the repository are not "
307+
"shown: they are pre-existing and untouched by this PR. "
308+
"The full tree was scanned — see the Security tab._",
309+
]
226310
227311
with open(os.environ["RUNNER_TEMP"] + "/threatcrush-comment.md", "w") as handle:
228312
handle.write("\n".join(lines) + "\n")

0 commit comments

Comments
 (0)