ci: scan pull requests for credentials and injection with ThreatCrush - #2993
ci: scan pull requests for credentials and injection with ThreatCrush#2993ralyodio wants to merge 6 commits into
Conversation
Greptile SummaryAdds a report-only ThreatCrush pull-request workflow with SARIF upload, job-summary and PR-comment reporting, plus a compatibility converter for CLI releases without native SARIF.
Confidence Score: 4/5The compatibility parser’s fail-open undercounting path should be fixed before merging; dependency pinning and module-style cleanup are also recommended. A completed legacy scan can declare findings that the parser silently drops, after which the workflow publishes valid-looking partial or empty SARIF instead of treating the scan as unrecognized. Files Needing Attention: .github/scripts/threatcrush-to-sarif.py, .github/workflows/threatcrush-scan.yml
|
| Filename | Overview |
|---|---|
| .github/scripts/threatcrush-to-sarif.py | Adds legacy-output SARIF conversion, but its completion check can accept and emit incomplete results when declared findings are not parsed. |
| .github/workflows/threatcrush-scan.yml | Adds the complete scanning and reporting workflow; mutable executable dependencies and one repository-style violation require attention. |
Prompt To Fix All With AI
### Issue 1
.github/scripts/threatcrush-to-sarif.py:67-69
**Footer count permits partial results**
When an older CLI retains the completion footer but emits a missing, reordered, truncated, or renamed finding field, `parse` silently drops that record without comparing the declared count to the parsed count. The converter then publishes partial or empty SARIF, causing the PR report to undercount findings or report none and making a future `failOn` threshold evaluate only the parsed subset.
### Issue 2
.github/workflows/threatcrush-scan.yml:31
**Mutable code holds write permissions**
The job executes `@profullstack/threatcrush@latest` and mutable major-version action tags while holding pull-request and security-event write permissions. Pinning these dependencies to immutable reviewed versions prevents a changed or compromised release from modifying PR comments or security-event data with the job token.
**How this was verified:** The workflow grants both write scopes before executing the mutable npm package and action references in the same job.
### Issue 3
.github/workflows/threatcrush-scan.yml:224
**Inline script violates module convention**
The new GitHub Script block uses `require('fs')`, contrary to the repository rule requiring top-level imports and avoiding `require()`. This introduces a module-style exception that requires special handling during future maintenance.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "ci: add the ThreatCrush security scan wo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| lines = ANSI.sub("", text).splitlines() | ||
| if not any(FOOTER.match(line) for line in lines): | ||
| raise Unrecognised("no scan-completion footer found") |
There was a problem hiding this comment.
When an older CLI retains the completion footer but emits a missing, reordered, truncated, or renamed finding field, parse silently drops that record without comparing the declared count to the parsed count. The converter then publishes partial or empty SARIF, causing the PR report to undercount findings or report none and making a future failOn threshold evaluate only the parsed subset.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/scripts/threatcrush-to-sarif.py
Line: 67-69
Comment:
**Footer count permits partial results**
When an older CLI retains the completion footer but emits a missing, reordered, truncated, or renamed finding field, `parse` silently drops that record without comparing the declared count to the parsed count. The converter then publishes partial or empty SARIF, causing the PR report to undercount findings or report none and making a future `failOn` threshold evaluate only the parsed subset.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| - name: Install ThreatCrush | ||
| run: | | ||
| for attempt in 1 2 3; do | ||
| if npm install -g "@profullstack/threatcrush@latest"; then |
There was a problem hiding this comment.
The job executes @profullstack/threatcrush@latest and mutable major-version action tags while holding pull-request and security-event write permissions. Pinning these dependencies to immutable reviewed versions prevents a changed or compromised release from modifying PR comments or security-event data with the job token.
How this was verified: The workflow grants both write scopes before executing the mutable npm package and action references in the same job.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/threatcrush-scan.yml
Line: 31
Comment:
**Mutable code holds write permissions**
The job executes `@profullstack/threatcrush@latest` and mutable major-version action tags while holding pull-request and security-event write permissions. Pinning these dependencies to immutable reviewed versions prevents a changed or compromised release from modifying PR comments or security-event data with the job token.
**How this was verified:** The workflow grants both write scopes before executing the mutable npm package and action references in the same job.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| lines.append("No findings.") | ||
|
|
||
| with open(os.environ["RUNNER_TEMP"] + "/threatcrush-comment.md", "w") as handle: | ||
| handle.write("\n".join(lines) + "\n") |
There was a problem hiding this comment.
The new GitHub Script block uses require('fs'), contrary to the repository rule requiring top-level imports and avoiding require(). This introduces a module-style exception that requires special handling during future maintenance.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/threatcrush-scan.yml
Line: 224
Comment:
**Inline script violates module convention**
The new GitHub Script block uses `require('fs')`, contrary to the repository rule requiring top-level imports and avoiding `require()`. This introduces a module-style exception that requires special handling during future maintenance.
**Context Used:** AGENTS.md ([source](https://github.com/generalaction/emdash/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Signed-off-by: Anthony Ettinger <anthony@chovy.com>
Signed-off-by: Anthony Ettinger <anthony@chovy.com>
Signed-off-by: Anthony Ettinger <anthony@chovy.com>
Signed-off-by: Anthony Ettinger <anthony@chovy.com>
1 participant
Adds a pull-request workflow that scans the diff for hardcoded credentials,
injection, SSRF and unsafe deserialisation. Results go to the Security tab as
SARIF and to a comment on the pull request.
It is report-only.
failOnis empty, so it annotates and never fails a build.A repository with pre-existing findings should get a report on its first install,
not a blocked pull request — a gate that fires on everything gets switched off
within a day. Tighten it to
critical,highin the workflow once any backlog istriaged.
.github/workflows/threatcrush-scan.yml— the workflow.github/scripts/threatcrush-to-sarif.py— a compatibility shim for CLI versionsolder than native SARIF output; unused once the installed CLI can emit it itself
Permissions are least-privilege (
contents: read,pull-requests: write,security-events: write). It runs onpull_request, notpull_request_target,so contributor code never executes with your secrets in scope. The SARIF upload
is
continue-on-errorand degrades quietly where code scanning is unavailable.Disclosure: I maintain ThreatCrush.
It is free and MIT, and the workflow installs it from npm — nothing here phones
home. If this is not something you want, closing it is the right answer, and I
will not send another.