fix(threatcrush-scan): drop install scripts and the checkout token - #957
Merged
Conversation
…kout token
SonarQube fails the pack's own workflow on a repository that runs it,
which is a poor look for a security scan:
githubactions:S6505 omitting --ignore-scripts allows lifecycle scripts
to run during package installation
githubactions:S8543 using dependencies without locking resolved versions
Both are fair. The job holds `pull-requests: write` and
`security-events: write`, and it was installing a floating `@latest` with
lifecycle scripts enabled — so an npm publish by anyone in the CLI's
dependency tree ran arbitrary code inside a write-scoped job on a
stranger's runner, with a checkout credential still in .git/config.
- install with --ignore-scripts. The CLI declares no install hook, and
`scan` was verified to run correctly from an --ignore-scripts install.
- default to an exact version rather than @latest. Whoever installs the
pack renders the version current at the time; the repository upgrades
when it decides to.
- persist-credentials: false on checkout. Nothing in this job pushes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 13, 2026
#952 pinned threatcrushPackageSpec to 0.11.0 and bumped the manifest to 1.2.0 while this branch was open, with a better reason than the one here: the `workspace:` protocol slip in 0.7.0/0.7.1 is what @latest actually cost, and that is worth more in the file than a rule number. Both sides reached the same default, so the manifest and README go back to master's wording verbatim — which also un-conflicts the branch. What is left is the half master still does not have: --ignore-scripts on the install, and persist-credentials: false on checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
ThreatCrush Security Scan310 finding(s) HIGH/CRITICAL: 24 | MEDIUM: 50 | LOW: 236
…and 260 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
changed the title
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
1 participant
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.
The
threatcrush-scanpack fails security review on the repositories it is installed into — a bad look for any workflow, a fatal one for a workflow whose subject is security.SonarCloud's quality gate on NeptuneHub/AudioMuse-AI#856 failed with a C security rating on new code, both findings on the install line:
CodeRabbit reached the same conclusion independently on chrisleekr/binance-trading-bot#732, scoring the PR 🟡 Moderate because it "gives an unpinned scanner access to a write-scoped job and retained checkout credentials".
S8543 is already fixed on master — #952 pinned
threatcrushPackageSpecto0.11.0while this branch was open, with a better reason than the one this branch originally carried: theworkspace:protocol slip in 0.7.0/0.7.1 is what@latestactually cost. Both sides reached the same default, so the manifest and README here are now byte-identical to master's, which also un-conflicts the branch.What is left, which master does not have
The job holds
pull-requests: writeandsecurity-events: write, and it still runs the install with lifecycle scripts enabled and a checkout credential in.git/config.--ignore-scriptson the install (S6505). A lifecycle script is arbitrary code from the dependency tree; the CLI declares no install hook of its own, so this costs nothing it needs.persist-credentials: falseon checkout. Nothing in this job pushes, so the token should not sit in the working tree while a network-installed scanner reads a pull request diff.Verified
--ignore-scriptsskipsbetter-sqlite3's native install hook, so this is the change that could plausibly have broken the scan. It does not:Both YAML files still parse, and the rendered workflow carries the changes through:
Rendered into AudioMuse-AI#856, SonarCloud went from 2 MAJOR vulnerabilities to 0 open issues and the gate passes.
Consumer side is profullstack/threatcrush#121; wider discussion of the 0-for-6 first batch is profullstack/threatcrush#120.
🤖 Generated with Claude Code