Skip to content

Add a static check for detectors that don't set SecretParts#4913

Open
mcastorina wants to merge 1 commit intosecret-parts-rename-draftfrom
polecat/rust-moae37t5
Open

Add a static check for detectors that don't set SecretParts#4913
mcastorina wants to merge 1 commit intosecret-parts-rename-draftfrom
polecat/rust-moae37t5

Conversation

@mcastorina
Copy link
Copy Markdown
Contributor

@mcastorina mcastorina commented Apr 22, 2026

Summary

Adds a small static-analysis tool under hack/checksecretparts/ that scans pkg/detectors/ and warns about detector packages that construct detectors.Result values without ever populating the new SecretParts field.

It's wired into the Lint workflow as warning-only (continue-on-error: true): findings show up in CI but don't block merges. The intent is to land the check before every detector is migrated — otherwise the ~900 unmigrated packages would fail every unrelated PR.

Stacked on #4911 (the rename that introduces SecretParts); that needs to land first.

Why

SecretParts replaces the old AnalysisInfo field and is expected to be populated by every detector going forward. Today only ~11 detectors set it. Rather than wait for a big-bang migration, this PR adds the enforcement mechanism first so each incremental migration PR gets immediate feedback, and so the flip from warning to hard-fail becomes a one-line workflow change once the migration is complete.

What it does

  • For each directory under pkg/detectors/, finds composite literals of the form detectors.Result{...} or &detectors.Result{...} in non-test .go files.
  • If the package never mentions SecretParts in any non-test source (neither in the literal nor in a later x.SecretParts = ... assignment), emits one warning per construction site.
  • Ignores _test.go files on both sides — some tests zero the field for cmp comparisons (e.g. pkg/detectors/gitlab/v1/gitlab_integration_test.go) and those references would otherwise mask real findings.

No new module dependencies — just go/ast and go/parser.

How to run

# Warning mode (default), exit 0 even with findings:
go run ./hack/checksecretparts

# Fail mode, exit 1 on findings — for use after the full migration:
go run ./hack/checksecretparts -fail

# Scoped to specific packages:
go run ./hack/checksecretparts ./pkg/detectors/aws ./pkg/detectors/github

Full instructions for flipping warning → hard-fail are in hack/checksecretparts/README.md.

Verification

  • go test ./hack/checksecretparts/ — 8 cases covering missing-field, literal-populated, later-assigned, pointer literal, multiple sites, test-file-only references (must not suppress), and no-op packages.
  • Against the current tree: 825 findings. Spot-checked that every detector already known to populate SecretParts (postmark, databricks, figma, ngrok, monday, airbrake, digitalocean, datadog, tableau) is correctly exempted.
  • go vet ./... and go build ./... are clean.

Out of scope

  • Populating SecretParts on any real detector — that's a separate migration.
  • Flipping the check to hard-fail — to be done in a one-liner follow-up once migration completes.
  • Deeper dataflow (per-code-path verification). The package-level heuristic is deliberately coarse; it's enough to surface unmigrated detectors and doesn't risk false positives on the migration PRs themselves.

Note

Low Risk
Low risk: adds a standalone static-analysis helper and a non-gating CI job, with no runtime code-path changes to detectors themselves.

Overview
Introduces hack/checksecretparts, a small Go-based static analysis tool that scans pkg/detectors for detectors.Result{...} / &detectors.Result{...} constructions in non-test files and warns when the package never references SecretParts.

Wires the tool into the Lint GitHub Actions workflow as a warning-only job (continue-on-error: true), with docs and unit tests to support later flipping to a hard-fail mode.

Reviewed by Cursor Bugbot for commit 9843976. Bugbot is set up for automated code reviews on this repo. Configure here.

Introduces a small Go tool under hack/checksecretparts that finds
detector packages which construct detectors.Result without populating
the new SecretParts field.

The check runs in CI as warning-only (continue-on-error) so the ~900
unmigrated detectors don't block unrelated PRs while they're being
migrated; it can be flipped to a hard failure by dropping
continue-on-error and passing -fail once all detectors populate the
field.

Covers composite and pointer literals, ignores test files on both
sides (construction and reference), and suppresses findings for any
package that mentions SecretParts anywhere in its non-test source so
that detectors setting the field via later assignment (x.SecretParts
= ...) are not flagged.
@mcastorina mcastorina requested a review from a team as a code owner April 22, 2026 19:46
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9843976. Configure here.

}

if len(findings) > 0 {
fmt.Fprintf(os.Stderr, "checksecretparts: %d package(s) construct detectors.Result without SecretParts\n", len(findings))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary message miscounts findings as packages

Low Severity

The summary line reports len(findings) as the number of "package(s)", but findings contains one entry per construction site, not per package. A single package with multiple detectors.Result{} literals produces multiple Finding values (confirmed by the "multiple construction sites reported individually" test case yielding wantLen: 2). This makes the summary overcount packages — e.g. 825 findings might come from far fewer distinct packages.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9843976. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant