Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions packages/actions/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ describe('built-in packs', () => {
// needs no credentials is a pack that can be installed fleet-wide without
// provisioning anything first.
expect(entry?.manifest.secrets).toHaveLength(0);
// Workflow plus the legacy-output converter it falls back to.
// One file. The legacy-output converter was removed in 1.7.0 once the
// pinned CLI could emit SARIF itself; every file a pack writes into
// somebody else's repository is surface their reviewer has to read.
expect(entry?.manifest.files.map((f) => f.destination)).toEqual([
'.github/workflows/threatcrush-scan.yml',
'.github/scripts/threatcrush-to-sarif.py',
]);
});

Expand Down Expand Up @@ -310,11 +311,13 @@ describe('built-in packs', () => {
inputs: {},
});
const content = result.files[0]?.content ?? '';
expect(content).toContain("grep -q -- '--format'");
expect(content).toContain('--format sarif --output threatcrush.sarif');
expect(content).toContain('if [ ! -s threatcrush.sarif ]; then');
// A CLI without --format takes the converter path rather than failing the
// repo out of being scanned at all.
expect(content).toContain('.github/scripts/threatcrush-to-sarif.py');
// The capability probe and the converter it guarded are both gone. The
// spec is pinned and the install refuses other bytes, so the interface is
// decided by the pack rather than discovered on the runner.
expect(content).not.toContain("grep -q -- '--format'");
expect(content).not.toContain('.github/scripts/threatcrush-to-sarif.py');
expect(content).toContain('this diff was NOT scanned');
// And the report must be fail-closed. Testing for status == "error" was
// fail-open: when the capability check fails the scan step is *skipped*,
Expand Down
65 changes: 31 additions & 34 deletions packages/actions/threatcrush-scan/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,40 +83,37 @@ print in SARIF order, which is file order, so the 50-row cap was decided by wher
a finding sat in the tree: a `high` in the last file scanned could be truncated
away while fifty `note`s from the first file printed in full.

## Two output paths, chosen up front

The workflow checks `threatcrush scan --help` for `--format` **before**
scanning, and picks accordingly:

| CLI | Path |
| --- | --- |
| Has `--format` | Native SARIF. Preferred; nothing is parsed. |
| Older | Runs the text scan and converts it with `.github/scripts/threatcrush-to-sarif.py`. |

The check happens up front because exit codes cannot tell the two failures
apart. The published `0.2.2` has no `--format`: the scan died with
`error: unknown option '--format'` and commander exited `1` — *the same code
the CLI uses for findings at or above `failOn`*. Read as a result, that
produced no SARIF, the empty-run fallback supplied one, and the PR comment
said **0 findings**. A green check on a repository that was never scanned.

The converter **fails closed**: if it cannot recognise the output it exits
non-zero and writes nothing, dumping what it saw. Emitting empty SARIF instead
would report "0 findings", which is indistinguishable from a clean scan.

Three details of the legacy format are load-bearing, and the converter is
tested against real captured output rather than assumption:

- Severity is bare for `CRITICAL`, bracketed for `[HIGH]`/`[MEDIUM]`/`[LOW]`.
One regex shape misses half the findings.
- `File:` paths are relative to the scan root, not the repository root.
Unprefixed, every finding resolves to nothing in the consumer's view.
- Whole-file findings report line `:0`; SARIF requires `startLine >= 1`.

**The legacy path is a stopgap, not a destination.** `0.2.2` is a secrets
scanner: it scores 12.9% against the testbed. Once a CLI with `--format` is
published the workflow switches to it automatically and coverage goes to
90.32%.
## One output path

The CLI emits SARIF itself. The workflow asks for it and nothing parses
anything:

```
threatcrush scan "$SCAN_PATH" --format sarif --output threatcrush.sarif
```

There used to be a second path — a capability probe on `--format`, and a
235-line Python converter that reconstructed findings by regex from the
terminal output when the probe said no. Both were removed in 1.7.0, because
the premise stopped holding: `threatcrushPackageSpec` pins an exact version
and the install step refuses any other bytes, so which interface the CLI has
is decided by the pack rather than discovered on the runner. The probe could
only ever answer yes.

Removing it is a security change more than a tidying one. The converter read a
*display* format, which is free to change between releases — the failure mode
being a silent undercount that still looks like a completed scan. And every
file a pack writes into somebody else's repository is surface a reviewer has
to read; this pack now installs one workflow and nothing else. That was a
direct ask from a maintainer reviewing the supply chain before merging.

The history is worth keeping, because it is the reason the exit-code handling
below is written the way it is. The published `0.2.2` had no `--format`: the
scan died with `error: unknown option '--format'` and commander exited `1` —
*the same code the CLI uses for findings at or above `failOn`*. Read as a
result, that produced no SARIF, the empty-run fallback supplied one, and the
comment said **0 findings**. A green check on a repository that was never
scanned.

## Exit codes are distinguished

Expand Down
9 changes: 1 addition & 8 deletions packages/actions/threatcrush-scan/sh1pt.actionpack.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ description: >-
Scans pull requests for hardcoded credentials, injection, SSRF, unsafe
deserialisation and dependency tampering, and uploads SARIF to the Security
tab.
version: 1.6.1
version: 1.7.0
publisher: profullstack
visibility: public
license: MIT
Expand Down Expand Up @@ -109,13 +109,6 @@ files:
- source: workflow.yml
destination: .github/workflows/threatcrush-scan.yml
mergeStrategy: replace-managed
# Compatibility shim for CLI versions older than native `--format sarif`.
# Unused once the installed CLI can emit SARIF itself — the workflow picks
# the native path whenever it is available — but shipping it means a repo is
# scanned today rather than waiting on a release.
- source: threatcrush-to-sarif.py
destination: .github/scripts/threatcrush-to-sarif.py
mergeStrategy: replace-managed
policies:
installMode: pull-request
managedComment: true
Expand Down
235 changes: 0 additions & 235 deletions packages/actions/threatcrush-scan/threatcrush-to-sarif.py

This file was deleted.

Loading
Loading