Add a non-blocking secret and code scan to CI - #202
Open
ralyodio wants to merge 2 commits into
Open
Conversation
docmd builds sites by running a CLI over somebody's content, and several of its commands hand values straight to a shell. This watches that surface, and the more expensive accident of a real credential reaching a public branch, on every pull request, every push to main, and once a week. It is deliberately not a gate. The scan fails only on a critical finding, so a false positive can never hold a PR: results go to the Security tab on push and to the job summary on a pull request, where uploading SARIF would need a write token a fork does not get. The scanner is pinned, and installed with --ignore-scripts so CI never builds the native dependency that only its daemon uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Run against a repo whose criticals are false positives, --fail-on critical exits 1, so the promise that a false positive cannot hold a PR was not one the workflow could keep. No --fail-on is passed now, which makes it true structurally rather than by severity accounting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Correction, pushed just now. The PR said a false positive could never hold a PR because the scan ran with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a non-blocking secret and code scan: every pull request, every push to
main, and once a week. There is no security scanning in CI today.What it is not
It cannot block a merge. The scan runs with
--fail-on critical, so anything below critical is reported and never fails the job. Onmainand on the weekly run the SARIF goes to the Security tab; on pull requests the counts go to the job summary instead, because uploading SARIF needs a write token a fork PR does not get, and that step would otherwise fail on somebody's contribution rather than on anything about their change.What it finds here today
I ran it against this repo first. 85 findings, 0 critical. Most of it is noise and I would rather say so than let a wall of yellow imply otherwise:
js-shell-exec-interpolationexecSync(\node ${DOCMD} build`)intests/cli-contracts/**`. Test harness, not a surface. Noise.insecure-temp-file/tmppaths inside workflow YAML and scripts. Noise.js-open-redirectwindow.location.href = finalHrefindocmd-main.js, where the href comes off a link on the page the theme itself rendered. Noise.js-unescaped-html-sinkinnerHTMLin the AI plugin's client. Worth a glance, since plugin output is the one thing a docs theme renders that the author did not write.js-ssrf-outbound-requestfetchof a versions URL and a client-sideHEAD. Both constant-origin. Noise.manifest-install-lifecycle-scriptpostinstall. Intentional.js-dynamic-code-executionnew Function(script.textContent)indocmd-main.js:819, running script tags out of the page it built. Intended by design.Three that seemed worth naming rather than burying:
packages/core/src/commands/stop.ts:63—execSync(\lsof -t -i:${port}`), and the same shape on the two lines under it.portcomes from config rather than from an attacker, so this is not a hole; it is the one place inpackages/where a config value reaches a shell string, andexecFile` with an argument array would close the question permanently.packages/plugins/openapi/src/index.ts:277—yaml.load(raw)on a user-supplied spec. On js-yaml v4loadis already the safe parser and this is a non-issue; on v3 it is not. The dependency is an optionalrequirewith no version constraint, so which one it is depends on what the user happens to have installed. Pinningjs-yaml@^4in the plugin's peer deps would settle it.packages/plugins/ai/src/client/index.ts— theinnerHTMLsinks above.None of these are why the workflow is worth having. The reason is the day a real key lands in a commit; the secret rules are the ones that catch that, and they are quiet on this repo today.
Verified rather than assumed
npm install -g --ignore-scriptsis enough forscan; without it npm buildsbetter-sqlite3from source, which only the scanner's daemon needs.--fail-on criticalreally does exit 0 with the 2highfindings present, so the non-blocking claim is tested.upload-sarifwants, and the summary step was run against this repo's own report — the table above is its output.actions/checkout@v7andactions/setup-node@v7, matchingci.yml.About the scanner
ThreatCrush is a CLI I work on, so weigh the recommendation accordingly. Nothing about the shape depends on it: the workflow is one file, and pointing it at
gitleaks,trufflehogorsemgrepis a two-line change. If you would rather have one of those, say so and I will send that instead. If you would rather have none, close this and no hard feelings.🤖 Generated with Claude Code