fix(scan): read context before deciding severity - #154
Merged
Conversation
Three structural false-positive causes, measured against mulgadc/spinifex —
an AWS-compatible control plane of 2,439 Go files that scored 280 findings and
zero true positives at 0.11.2, the worst result recorded so far.
None of them is a bad pattern, which is why none had been fixed by tightening
one. The construct is always genuinely present: `InsecureSkipVerify: true`
really does disable TLS verification and `"/tmp/test-wal"` really is a
predictable path. What was missing is the context that decides whether anyone
should care, and a finding nobody should care about, reported at high, is what
makes a scanner unpitchable.
1. A bare `//nolint:gosec` was ignored. `FOREIGN_CREDENTIAL` insisted the
annotation name `G101` as well as the linter, which is not how Go projects
write it — nine of spinifex's twenty-one `InsecureSkipVerify` lines carry
the bare form with a written reason, and every one was re-reported at high.
Naming the linter is still required, so `//nolint` and `//nolint:errcheck`
still count for nothing.
2. Only the credential rules read `isTestPath`. The code rules never did, so
104 findings reported at full severity from inside a test — scratch paths in
table-driven fixtures, `exec.Command("sh", "-c", "exit 42")` in a
process-supervision test, self-signed certificates in the e2e harness. The
path set also missed `harness/` and `testutils/`, both plain Go directories
with no `_test.go` suffix on any file.
3. `secret-generic-credential` matched identifier names rather than values.
`ErrExpiredToken = "ExpiredToken"` is an AWS API error code; `pathToken =
"/latest/api/token"` is a URL. Both repeat their own key, which
`isTestFixtureValue` already knew how to spot and only ever did in tests —
the reasoning was never about tests, so it is lifted out as
`describesItsOwnKey`. The sharpest case was `cmd/rds-agent/engine.go`, where
the literal flagged as the credential is the name of a psql bind parameter
on the most carefully written line in the file.
Also: a credential in a `placeholder` attribute is the example text an empty
input shows, not data — two of spinifex's nine criticals.
Nothing is dropped. Every one of these is a heuristic about where code lives or
what it is named, and heuristics of that kind may inform a severity and never a
verdict, so the finding stays counted, printed, in the SARIF and caught by
`--fail-on low`. On spinifex the count is 275 before and after; what moves is
critical 9 → 7 medium 104 → 14
high 44 → 5 low 118 → 249
leaving 12 findings above low where there were 53.
Detection is unchanged. Against profullstack/malware-test-prs the count holds
at 130 with all 43 criticals intact; the only movement is four
`secret-generic-credential` findings on scaffolding lines
(`secret: "top-secret"`) that are not the scored sinks in their own fixtures.
Deliberately not fixed, and documented where it would have gone: spinifex's
remaining seven criticals are `ASIA1A2B3C4D5E6F7890` in its API docs. A rule
exempting typed sequences was written and removed again — `false-positives.test.ts`
pins `AKIA1234567890ABCDEF` as a key that must never be exempted, and the two
values are the same shape. Quiet documentation is not worth a scanner that
skips the shapes its own corpus is built from.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan63 finding(s) HIGH/CRITICAL: 4 | MEDIUM: 52 | LOW: 7
…and 13 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
gitleaks and semgrep both read the comment explaining why a typed-sequence exemption cannot be had, found the two literals it names, and blocked. Written as a prefix and a body now, and the test password is assembled from parts — the convention `foreign-suppression.test.ts` already established for exactly this reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gitleaks reads every ref, so rewriting the lines in the tree does not clear the commit that introduced them. Same remedy as the four fingerprints above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Three structural false-positive causes, measured against
mulgadc/spinifex — an AWS-compatible
control plane of 2,439 Go files that scored 280 findings and zero true
positives at 0.11.2, the worst result recorded so far.
None of them is a bad pattern, which is why none had been fixed by tightening
one. The construct is always genuinely present:
InsecureSkipVerify: truereally does disable TLS verification, and
"/tmp/test-wal"really is apredictable path. What was missing is the context that decides whether anyone
should care.
The three causes
1. A bare
//nolint:gosecwas ignored.FOREIGN_CREDENTIALinsisted theannotation name
G101as well as the linter, which is not how Go projectswrite it. Nine of spinifex's twenty-one
InsecureSkipVerifylines carry thebare form with a written reason beside it, and every one was re-reported at
high:
Naming the linter is still required, so
//nolintand//nolint:errcheckstill count for nothing. The old docstring's argument was right about bare
//nolintand was being applied to the wrong half of the annotation.2. Only the credential rules read
isTestPath. The code rules never did,so 104 findings reported at full severity from inside a test — scratch paths in
table-driven fixtures,
exec.Command("sh", "-c", "exit 42")in aprocess-supervision test, self-signed certificates in the e2e harness. The path
set also missed
harness/andtestutils/, both plain Go directories with no_test.gosuffix on any file.3.
secret-generic-credentialmatched identifier names, not values.Each repeats its own key, which
isTestFixtureValuealready knew how to spotand only ever did inside tests. The reasoning was never about tests, so it is
lifted out as
describesItsOwnKeyand softens everywhere.The sharpest case is
cmd/rds-agent/engine.go, where the literal flagged asthe credential is the name of a psql bind parameter — on a line that reads the
password from the environment via
\getenv, suppresses statement logging threeways, and redacts the password out of its own error strings. The scanner was
flagging the most careful line in the file.
Also: a credential in a
placeholderattribute is the example text an emptyinput shows, not data — two of spinifex's nine criticals.
Nothing is dropped
Every one of these is a heuristic about where code lives or what it is named,
and heuristics of that kind may inform a severity and never a verdict. A
softened finding stays counted, printed, in the SARIF, and caught by
--fail-on low.On spinifex the count is 275 before and after. What moves is severity:
131 findings changed severity — 104 by test path, 13 by a foreign
nolint, 11by echoing their own key, 2 by a placeholder attribute, 1 by documentation.
That leaves 12 findings above low where there were 53.
Detection is unchanged
Against
profullstack/malware-test-prsthe count holds at 130 with all 43criticals intact. Its scored fixtures live in
vulns/, not a test path. Theonly movement is four
secret-generic-credentialfindings on scaffolding lines—
secret: "top-secret"andexpected_token = "valid-token-123"— neither ofwhich is the scored sink in its own fixture (CWE-475 and CWE-480, both code
rules, untouched).
313 tests pass, 15 of them new, every one built from a line copied out of
spinifex.
tsc --noEmitis clean.Deliberately not fixed
Spinifex's remaining seven criticals are
ASIA1A2B3C4D5E6F7890in its APIdocumentation. A rule exempting typed sequences — digits running 1234567890,
letters running ABCDEF — was written, measured and removed again:
false-positives.test.tspinsAKIA1234567890ABCDEFas a key that must neverbe exempted however it is named, and the two values are the same shape.
Quiet documentation is not worth a scanner that skips the shapes its own
corpus is built from. The dead end is documented in
secret-rules.tsso thenext person does not spend the afternoon rediscovering it.
🤖 Generated with Claude Code