ci: scan pull requests for credentials and injection with ThreatCrush

[ralyodio]

Adds one workflow. On each pull request it scans the checked-out repository for
hardcoded credentials, injection, SSRF and unsafe deserialisation.

• .github/workflows/threatcrush-scan.yml

Read-only. permissions: is contents: read and nothing else. Findings go
to the job summary and a SARIF artifact — no pull request comment, no Security
tab upload, no write scope requested. Those two steps are not disabled in the
file, they are absent from it. This is also the shape that keeps working on fork
pull requests, where GitHub downgrades GITHUB_TOKEN to read-only.

Report-only. failOn is empty, so findings never fail the build. An install
or scan failure does fail the job: a scanner that reports clean when it did not
run is worse than no scanner.

Pre-existing findings. The report leads with findings in the files the pull
request changes and folds the rest of the repository behind a <details>
summary, so an existing backlog is visible without being posted at the author of
an unrelated change. Anything intentional can be excluded with a
.threatcrushignore or a // threatcrush-disable-next-line <rule-id> comment.

Scope: it scans the whole checked-out repository, not only the diff.

Supply chain. Pinned to @profullstack/threatcrush@0.11.2; the tarball is
hashed and checked against a value in the workflow before install (npm view it
yourself), installed with --ignore-scripts, actions pinned to commit SHAs, and
it runs on pull_request rather than pull_request_target.

Asked first in #6.

Disclosure: I maintain ThreatCrush;
MIT and free. Written with AI assistance. Closing this is a fine answer and I
will not send another.

[alexanderradahl]

Thanks for the disclosure and for being careful about pull_request, persist-credentials: false, action SHA pins, --ignore-scripts, and the npm tarball integrity check. I independently fetched @profullstack/threatcrush@0.11.2; the published SHA-512 matches the value in this workflow, and I also ran the pinned scanner against the current main tree in a disposable, scrubbed environment.

I like the idea of a lightweight report-only security check, but I don't want to merge this version as-is. Please narrow the first integration before we run/approve it:

1. Remove the compatibility converter and use 0.11.2's native SARIF path. The exact version pinned here already advertises scan --format sarif --output ...; I verified it directly. That makes the 235-line terminal-output parser plus interface-detection branch dead compatibility surface for the only version this workflow can install. Because the dependency is exact-pinned, an upgrade is already an explicit review event. I'd rather fail on an unexpected CLI interface than own a parser for human-readable output.

2. Make the first version read-only. Please start with permissions: contents: read, a job summary, and optionally a SARIF artifact. Drop pull-requests: write, the PR-comment mutation, security-events: write, and the Security-tab upload from the initial PR. Those write scopes are currently requested unconditionally even though the workflow is described as report-only. They also behave differently on fork PRs: GitHub normally downgrades GITHUB_TOKEN writes for fork-triggered pull_request workflows, so the richer outputs are least reliable on the contributor PRs where this is most useful. Once the signal quality is proven, we can evaluate a separate minimal reporting step. GitHub's token behavior is documented here: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions

3. Establish a useful baseline before wiring this into every PR. On current main, the pinned scanner reports 6 findings:

   • two js-uninitialized-buffer warnings on Buffer.allocUnsafe in the PTY ring, where the returned buffer is filled from retained ring bytes before use;
   • one environment-serialization warning in the stub MCP server's deliberate envdump test;
   • three low-severity synthetic credential findings in tests/fixtures.

   These are useful scanner-quality data, but as-is the repo begins with a permanently noisy report. Please either propose narrow suppressions/baseline handling for these known intentional cases, or explain how the workflow will distinguish new findings from the accepted baseline without hiding real regressions.

4. Keep the project-owned CI surface much smaller. A 382-line workflow + 235-line converter is a lot of security-sensitive code for a report-only scanner in this repository. With native SARIF, no comments, and no Security-tab mutation, this should be able to become a small auditable workflow: checkout without persisted credentials → verify/install the exact tarball → run native SARIF scan → summarize/upload artifact → fail only if the scanner itself did not run.

I have not approved the first-time-contributor workflow run yet because the workflow itself is the thing under review. If you can reduce it to that read-only/native-SARIF shape, I'll re-review the diff and then run it.

[ralyodio]

Thank you — this was a careful review and every one of the four points was the
pack's problem rather than this repository's. All four are addressed; the branch
is updated.

1. Converter gone, native SARIF only. You read it correctly: 0.11.2
advertises scan --format sarif --output and the 235-line terminal-output
parser could only ever have run against a version this workflow cannot install.
It is deleted, along with the interface-detection branch that selected it. The
workflow now installs one file and nothing else.

I did not replace the probe with an assertion, because the check you already
liked covers it. A CLI without --format writes no SARIF, and the step treats a
missing SARIF file as a hard failure that says the diff was not scanned
rather than reporting zero findings. That is the same "fail on an unexpected CLI
interface" outcome, decided by evidence rather than by asking, and it is the
behaviour that failed closed on the exit-code collision in the first place
(unknown option and "findings at or above --fail-on" are both exit 1).

2. Read-only. permissions: is now contents: read and nothing else. The
Security-tab upload and the PR-comment step are removed from the rendered
file, not left in it behind a false condition — a disabled upload still asks
you to read an upload and take on trust that the guard is right. Findings go to
the job summary and a SARIF artifact, neither of which needs a write scope.

Your point about fork PRs is the one that actually changed my mind about the
default. The two write-scoped outputs are exactly the ones GitHub downgrades on
fork-triggered pull_request runs, so the richest reporting was least reliable
precisely where an external scan is most useful. That is now the documented
reason the read-only build exists, rather than a fallback for the cautious.

3. Baseline. I reproduced your six findings exactly, with the pinned version,
against 182fd94. Two answers, because the six are not one thing:

Two of them are my bugs, not yours. Both js-uninitialized-buffer hits are
false positives and I am fixing the rule rather than asking you to suppress it:

• bridge.mjs:1386 — Buffer.allocUnsafe(length) is fully written before it
  escapes; firstLen plus the wrap-around copy covers [0, length) in every
  branch.
• bridge.mjs:1361 — the ring is only ever read through copy, which clamps to
  retained = min(total, capacity), so no unwritten byte is reachable.

That is the rule failing to model "allocUnsafe then filled before use", which is
the whole legitimate reason to call it. It belongs in the scanner.

The other four are intentional and are what suppressions are for. 0.11.2
already supports a .threatcrushignore at the scan root, --exclude, and inline
// threatcrush-disable-next-line <rule-id> reason comments. For the envdump
fixture that is one line above it; the three synthetic credentials in
tests/ are already emitted at note, and can take the same treatment. I did
not commit any of these — narrowing your tree is your call, not something to
arrive in an unsolicited PR.

And on distinguishing new from baseline: the workflow already does this, and I
should have said so up front. It derives the PR's own diff from
HEAD^1..HEAD on the merge ref (no API call, no token, hence fetch-depth: 2),
leads the report with findings in the files the PR touches, and folds the rest of
the repository into one <details> line with its severity counts. Nothing is
hidden and nothing is baselined away. On a PR touching bridge.mjs today it
renders as two findings up top and "4 pre-existing finding(s) elsewhere in the
repository" behind the fold. If the merge ref is missing (conflicted PR) it
reports everything unscoped and says so, rather than scoping to the wrong set.

4. Size. 286 lines in one file, down from 382 + 235. The shape is the one you
described: checkout without persisted credentials → verify and install the exact
tarball → native SARIF scan → summarise and upload the artifact → fail only if
the scanner did not run.

Two things I did not do, so they are not surprises. The scan still covers the
whole tree rather than the diff — only the report is scoped — because a
credential three directories away is still committed. And failOn is still
empty, so nothing here can fail your build except the scanner failing to run.

I have not re-requested review; the workflow run is still yours to approve or
not, and closing this is still a fine answer.

[ralyodio]

Following up on point 3 — the js-uninitialized-buffer false positives are fixed
at the source: profullstack/threatcrush#156.

The rule was a bare regex on Buffer.allocUnsafe(, so it fired on every call.
Filling the buffer yourself is the whole reason that API exists over
Buffer.alloc, which is why there was no way to silence your two occurrences
without silencing the rule everywhere. It now exonerates an allocation that is
written into before it escapes, bound to the name the allocation was assigned
to — so an unrelated .copy( below an escaping allocUnsafe still does not
excuse it.

Verified end to end against 182fd94: 6 findings → 4. Both bridge.mjs hits
are gone, the other four are unchanged, and an allocation that escapes unwritten
still flags.

The four that remain are all in tests/ and all intentional, so the baseline
question is now narrower than when you raised it: one envdump fixture and three
synthetic credentials, already emitted at note. Whether to annotate them is
entirely your call — a one-line // threatcrush-disable-next-line <rule-id> <reason> above each, or a .threatcrushignore. I am not going to send that as a
PR; narrowing your tree is yours to decide.

Worth being straight about the trade I made: the guard cannot prove the write
covers the whole buffer, so a genuine partial fill would now slip through.
Proving coverage needs range analysis the engine does not do, and the alternative
was a rule that fires on every correct use and gets turned off — which catches
that partial write in exactly the same number of cases, namely none. It is
documented in the code rather than left for the next person to discover.

This does not change the workflow in this PR; the pinned version won't pick it up
until I cut a release and bump the pin, and I would rather you review the workflow
on its own terms than have it move under you. Happy to bump it here once it lands
if you would prefer to start from a clean report.

Thanks again — this was the most useful review this workflow has had, and two of
the four points were bugs on my side that nobody else had caught.

[alexanderradahl]

Thanks — the revision fixes the four things from my first review: the converter is gone, permissions are now contents: read, reporting is native SARIF/read-only, and the PR-vs-backlog split is much easier to audit. I also reproduced the top-level @profullstack/threatcrush@0.11.2 tarball integrity again.

I found three remaining trust-boundary issues before I’m comfortable approving the first-time workflow run:

1. The top-level tarball hash does not pin the code that will actually execute. 0.11.2 has 11 semver-ranged runtime dependencies (^...) and its published tarball contains no npm-shrinkwrap.json. npm install -g --ignore-scripts <verified-tarball> therefore resolves/fetches a mutable transitive tree after the verified tarball step. --ignore-scripts is good, but when threatcrush runs it executes those resolved modules. For a security scanner, the “exact bytes reviewed” claim needs to extend to the runtime tree. Please either ship/use a publishable shrinkwrap in the pinned CLI release, or add a small pre-execution check that canonicalizes the resolved package-lock/dependency tree (name/version/integrity) and compares it to a maintainer-pinned digest before invoking the CLI. npm specifically documents npm-shrinkwrap.json as the publishable lockfile intended for globally installed CLI/apps.

2. Keep scanner output outside the untrusted checkout. --output threatcrush.sarif writes to a path the PR itself can pre-create as a file/symlink, and the later Python/artifact steps consume the same checkout-controlled path. You already made the right argument for putting the npm tarball in RUNNER_TEMP; please do the same for SARIF, e.g. ${RUNNER_TEMP}/threatcrush.sarif, and have the report/artifact read that exact path. The checkout should be scan input, not storage for trusted scan output.

3. Changed-file classification is currently bypassable by ordinary Git filenames. git diff --name-only ... writes newline-delimited names, then Python does .read().split(). A finding in path with space.js is emitted by 0.11.2 with URI path with space.js, but the changed set becomes {path, with, space.js}, so it is incorrectly folded into “pre-existing elsewhere.” I reproduced this locally. Please use NUL-delimited paths (git diff --name-only -z) and parse with split(b"\\0")/os.fsdecode. While there, Markdown-escape the repo-controlled URI before putting it into the job summary so filenames containing |, backticks, or newlines cannot forge the shape of the security report.

I’m deliberately not approving run 31997099518 yet: the workflow itself is still the artifact under review. Once those three are closed, I’m happy to re-review the current head and run it.

Also: nice catch/fix upstream on the two allocUnsafe false positives. Getting the scanner from 6 to 4 intentional fixture findings is exactly the kind of signal-quality improvement I wanted from this exercise.

[ralyodio]

Thanks. All three are fixed in bafd335.

1. The runtime tree, not just the top-level tarball

You're right, and it was the real hole: the hash pinned one package, not the code that executes. 0.11.2 declares eleven ^-ranged runtime dependencies and publishes no npm-shrinkwrap.json, so npm install -g --ignore-scripts <verified tarball> resolved a mutable tree after the verified step, and that tree is what does the scanning. --ignore-scripts covers install time and says nothing about run time.

I took your second option, and went one step past detection: the tree is now pinned rather than resolved and then checked.

• .github/threatcrush/package-lock.json pins all 209 packages, each with an exact version and an integrity hash. Install is npm ci, which installs exactly that set and verifies every tarball against the hash it was pinned with. Nothing resolves at run time, so there is no drift to detect and no unrelated PR going red because a transitive dependency cut a patch release.
• .github/threatcrush/tree-digest.mjs is the pre-execution check you described. It canonicalises the lockfile to sorted name@version integrity lines and sha256s them, and asserts that against TREE_DIGEST in the workflow before npm ci runs. It also asserts the top-level entry still equals the sha512-8N3j… you verified twice, because that is the one value a reviewer can confirm straight from the registry. An entry anywhere in the tree with no integrity is fatal, not a warning.

The digest is taken over the tree, not the file, so npm reformatting or reordering the lockfile does not move it; changing what executes does. The practical effect is that editing a single integrity hash fails the job until TREE_DIGEST is edited too, and that is one high-contrast line in a diff.

Verified against tampered copies of the lockfile:

transitive hash flipped   -> expected sha256-7b55f654…  received sha256-7a54fc93…   exit 1
top-level version bumped  -> lockfile pins @profullstack/threatcrush@0.11.3,
                             workflow expects 0.11.2                                exit 1
integrity field deleted   -> no integrity pinned for node_modules/blessed           exit 1
TREE_DIGEST unset         -> refusing to run an unpinned dependency tree            exit 1

On the shrinkwrap: it belongs upstream and I'll ship npm-shrinkwrap.json in the next ThreatCrush release. I deliberately did not do that as part of this PR, because it would mean re-pinning you to a version you have not reviewed and a tarball you have not independently verified. When it lands, this lockfile becomes a copy of the shipped shrinkwrap rather than a thing this repo maintains.

2. Scan output outside the untrusted checkout

Fixed, and the same argument applied to the install directory.

• SARIF is written to ${RUNNER_TEMP}/threatcrush/threatcrush.sarif, and the report step and the artifact upload read that exact path. --output threatcrush.sarif resolved inside the checkout, which the PR controls and can ship with that path already present as a file, a directory, or a symlink pointing elsewhere.
• The CLI installs into ${RUNNER_TEMP}/threatcrush/cli too. Beyond the same boundary argument, npm ci inside the checkout would drop 209 packages into the tree under review and the scan would then walk them.
• The changed-file list moved there as well. The three paths are declared once in the job's env: block, so there is no path literal repeated across steps to drift.

3. Changed-file classification

Fixed. git diff --name-only -z, parsed with split(b"\0") and os.fsdecode, so a name that is not valid UTF-8 compares equal rather than raising. I reproduced your case first: 0.11.2 emits the URI as path with space.js, .read().split() produced {path, with, space.js}, and the finding was filed under "pre-existing elsewhere".

Repo-controlled URIs and rule IDs are now escaped before going into the summary. &, backtick, | and < become entities and control characters become visible \xNN escapes, so a filename cannot close the code span it sits in, open a column, or start a row. Escaped rather than dropped, because a silently mangled path is its own reporting bug. A file named a.js|\nHIGH | `rce` | `fake.js`:1 renders as one cell:

| LOW | `x` | `a.js|\x0aHIGH | `rce` | `fake.js`:1`:1 |

Evidence

You have not approved a workflow run and I am not asking you to before you have read the diff, so I ran the job locally instead: every run: block executed in order against a synthesized refs/pull/N/merge (two parents, base first), with RUNNER_TEMP, GITHUB_OUTPUT, GITHUB_PATH and GITHUB_STEP_SUMMARY wired up. Verify passed, npm ci installed 209 packages, the scan wrote SARIF to RUNNER_TEMP, and the summary rendered:

**0** finding(s) in the 4 file(s) this pull request changes.
Nothing in the files this pull request changes.

<details><summary>6 pre-existing finding(s) elsewhere in the repository — MEDIUM: 3 | LOW: 3</summary>

Adding a file named demo path with space.mjs with a synthetic key to the same merge ref put it where it belongs, which is the before/after for point 3:

**1** finding(s) in the 5 file(s) this pull request changes.
| HIGH | `secret-openai-key` | `demo path with space.mjs`:1 |

The 6/3/3 backlog is the pinned scanner against current main and matches what you measured, so the added files introduce no new findings of their own.

On size, since you raised it

Honest accounting: the workflow went 286 to 325 lines, plus a 112-line helper. The lockfile is 2,534 lines, but it is generated data with a one-command regeneration recipe in the workflow comment, not security-sensitive logic to audit line by line, and the single TREE_DIGEST constant exists so you never have to read it. I think that trade is the right one against an unpinned 209-package tree executing on every PR, but it is your repository and your call. If you would rather not carry the lockfile, the alternative is the resolve-then-compare-digest version you originally described, which is about 15 lines and no lockfile, at the cost of the job going red whenever any transitive dependency publishes.

Closing this is still a fine answer.

_{Written with AI assistance.}