Add native Go fuzzing coverage#227
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds native Go fuzz testing across the repository: a scheduled CI workflow, a Makefile target and shell script to run fuzz targets, documentation updates, fuzz tests for npm/pnpm/yarn lockfile parsers, SBOM codec, plugin path sanitizers, and SDK JSON handling, plus a small plugin path validation fix. ChangesNative Go Fuzzing Infrastructure
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Cron as Scheduled Trigger
participant Workflow as Fuzz Workflow
participant Make as make fuzz
participant Script as run-fuzz.sh
participant GoTest as go test -fuzz
Cron->>Workflow: nightly trigger / manual dispatch
Workflow->>Make: make fuzz FUZZTIME=2m
Make->>Script: scripts/run-fuzz.sh
Script->>GoTest: run each fuzz target (npm, pnpm, yarn, sbom, plugin, sdk)
GoTest-->>Workflow: pass/fail results
Workflow->>Workflow: upload testdata/fuzz artifacts on failure
Sequence diagram(s) (compact metadata)
🐰 A rabbit hops through fuzzed terrain, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bomly Diff SummaryCompared Overview
Dependency ChangesSummary: 4 added, 0 changed, 0 removed. Added Dependencies
Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy FindingsSummary: 1 introduced, 0 persisted, 0 resolved. Introduced Findings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/detectors/node/npm/npm_lockfile_fuzz_test.go (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared fuzz-validation helper instead of copy-pasting across packages.
maxFuzzInputSizeandrequireFuzzGraphValidare duplicated verbatim in the npm, pnpm, and yarn fuzz test files. Since the codebase already favors shared test helpers (e.g.testutil.BuildGoBinary()), consider moving this graph-invariant validation into a small shared test helper package (e.g.internal/detectors/node/nodetestor similar) so all three ecosystems share one implementation and future changes to the invariant only need to happen once.♻️ Suggested consolidation
// internal/detectors/node/nodetest/fuzz.go package nodetest const MaxFuzzInputSize = 1 << 20 func RequireFuzzGraphValid(t *testing.T, graph *sdk.Graph) { t.Helper() // ... same body ... }Then in each fuzz test file:
-const maxFuzzInputSize = 1 << 20 - func FuzzDepGraphFromNPMLockfile(f *testing.F) { ... - requireFuzzGraphValid(t, graph) + nodetest.RequireFuzzGraphValid(t, graph) } - -func requireFuzzGraphValid(t *testing.T, graph *sdk.Graph) { ... }Also applies to: 39-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/detectors/node/npm/npm_lockfile_fuzz_test.go` at line 11, The fuzz-test validation logic is duplicated across the npm, pnpm, and yarn node detector tests, including the max input size constant and requireFuzzGraphValid helper. Move this shared invariant-checking code into a small common test helper package such as nodetest, and have npm_lockfile_fuzz_test and the sibling fuzz tests call that shared helper instead of keeping local copies. Keep the shared symbols named clearly, like MaxFuzzInputSize and RequireFuzzGraphValid, so future invariant changes happen in one place.sdk/fuzz_test.go (1)
46-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRound-trip fuzz tests don't verify data fidelity, only that re-marshal/unmarshal succeeds.
Both
FuzzGraphJSONandFuzzPackageRegistryJSONmarshal the decoded value and unmarshal it again, but never compare the round-tripped value against the original decoded value. This means these targets can't catch a class of bug fuzzing is specifically good at finding: silent data loss/corruption during marshal/unmarshal (e.g., a dropped edge, alteredPURL, or mutated field) that doesn't produce an error.Consider asserting structural/value equality between the original and round-tripped graph/registry (e.g., compare node/edge counts and IDs, or use
reflect.DeepEqualif the types support it deterministically).♻️ Example addition for FuzzGraphJSON
var roundTrip Graph if err := json.Unmarshal(encoded, &roundTrip); err != nil { t.Fatalf("round-trip graph JSON does not unmarshal: %v", err) } + requireFuzzGraphValid(t, &roundTrip) + if roundTrip.Size() != graph.Size() { + t.Fatalf("round-trip graph size mismatch: got %d want %d", roundTrip.Size(), graph.Size()) + }Also applies to: 75-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/fuzz_test.go` around lines 46 - 64, The fuzz targets in FuzzGraphJSON and FuzzPackageRegistryJSON only verify that marshal/unmarshal succeeds, so they miss silent data corruption. After the round-trip in these functions, compare the decoded value against the original decoded value using a deterministic structural/value check (for example, DeepEqual or explicit comparisons of counts/IDs/fields for Graph and the registry type). Keep the existing unmarshal/marshal error checks, but add an assertion that the round-tripped object matches the pre-মarshal object so fuzzing can catch data loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/detectors/node/npm/npm_lockfile_fuzz_test.go`:
- Line 11: The fuzz-test validation logic is duplicated across the npm, pnpm,
and yarn node detector tests, including the max input size constant and
requireFuzzGraphValid helper. Move this shared invariant-checking code into a
small common test helper package such as nodetest, and have
npm_lockfile_fuzz_test and the sibling fuzz tests call that shared helper
instead of keeping local copies. Keep the shared symbols named clearly, like
MaxFuzzInputSize and RequireFuzzGraphValid, so future invariant changes happen
in one place.
In `@sdk/fuzz_test.go`:
- Around line 46-64: The fuzz targets in FuzzGraphJSON and
FuzzPackageRegistryJSON only verify that marshal/unmarshal succeeds, so they
miss silent data corruption. After the round-trip in these functions, compare
the decoded value against the original decoded value using a deterministic
structural/value check (for example, DeepEqual or explicit comparisons of
counts/IDs/fields for Graph and the registry type). Keep the existing
unmarshal/marshal error checks, but add an assertion that the round-tripped
object matches the pre-মarshal object so fuzzing can catch data loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ad009a0-ab1e-45b8-b383-e9309a3830a4
📒 Files selected for processing (12)
.github/workflows/fuzz.ymlCONTRIBUTING.mdMakefiledev-docs/CI.mdinternal/detectors/node/npm/npm_lockfile_fuzz_test.gointernal/detectors/node/pnpm/pnpm_lockfile_fuzz_test.gointernal/detectors/node/yarn/yarn_lockfile_fuzz_test.gointernal/plugin/path_fuzz_test.gointernal/plugin/types.gointernal/sbom/codec_fuzz_test.goscripts/run-fuzz.shsdk/fuzz_test.go
1 participant
Summary
make fuzzand an explicit fuzz target runner with configurableFUZZTIME".\\"cleaned to"."and was acceptedValidation
go test ./internal/detectors/node/npm ./internal/detectors/node/pnpm ./internal/detectors/node/yarn ./internal/sbom ./sdk ./internal/pluginmake fuzz FUZZTIME=5sgo test ./internal/detectors/gradle ./internal/detectors/maven ./internal/detectors/sbtafter transient Java readiness timeoutsmake testgit diff --checkmake fmt-checkNote: the first full
make testrun hit transient Java readiness timeouts in Gradle/Maven/sbt detector tests; those packages passed on immediate rerun, and the subsequent fullmake testpassed.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation