feat(test): add fuzz tests for attacker-reachable code paths and nightly CI#665
Merged
Conversation
…tly CI Add 12 new fuzz tests targeting security-relevant parsing code: JSON-RPC entry point, encrypted session establishment, PAKE handshake, MQTT message handling, advanced args parsing, mapping matchers, MQTT path/protocol parsing, ZapLink response parsing, run handler, and RPC ID unmarshaling. Expand `task fuzz` from 6 to 36 targets (17 previously missing existing tests + 12 new) using a continue-on-failure loop so all targets run even when one crashes. Add nightly CI workflow (.github/workflows/fuzz.yml) that runs all fuzz targets for 2 minutes each, uploads crash corpus as artifacts, and creates a GitHub issue on failure. Fix invalid UTF-8 passthrough bugs found by fuzzing: - virtualpath.ParseVirtualPathStr: reject invalid UTF-8 in ID/name - helpers.getPathExt/getPathBase/getPathDir: reject invalid UTF-8 input - helpers.DecodeURIIfNeeded/FilenameFromPath: reject invalid UTF-8 input - tags.extractSpecialPatterns: reject invalid UTF-8 filenames - models.RPCID.UnmarshalJSON: reject empty input (lossy round-trip) Include fuzz corpus regression entries for all fixed bugs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new GitHub Actions fuzzing workflow and expands in-repo fuzz infrastructure: many new Go fuzz tests across API, middleware, helpers, readers, service, and zapscript packages; UTF-8 validation hardening in path/URI helpers; new fuzz corpus seeds; and Taskfile/CI/task docs updates to run the extended fuzz targets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/zapscript/advargs/advargs_fuzz_test.go (1)
81-98: Consider: Verify parsed values match, not just error presence.The determinism check only compares
(err == nil)between runs. For stronger guarantees, consider also verifying that successfully parsed structs have identical field values. This would catch non-deterministic parsing that produces different results on success.💡 Optional: Add value comparison for successful parses
+import "reflect" + // Determinism: same input must produce same result for each dest type var launchArgs2 zapscript.LaunchRandomArgs err1b := Parse(raw, &launchArgs2, ctx) if (err1 == nil) != (err1b == nil) { t.Errorf("non-deterministic error for LaunchRandomArgs with key=%q value=%q", key, value) } +if err1 == nil && err1b == nil && !reflect.DeepEqual(launchArgs, launchArgs2) { + t.Errorf("non-deterministic result for LaunchRandomArgs with key=%q value=%q", key, value) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zapscript/advargs/advargs_fuzz_test.go` around lines 81 - 98, The determinism check only compares error presence; update the tests so when Parse succeeds for a type (e.g., when err1 == nil and err1b == nil for LaunchRandomArgs, or similarly for PlaylistArgs and GlobalArgs) you also deep-compare the two parsed structs (launchArgs vs launchArgs2, playlistArgs vs playlistArgs2, globalArgs vs globalArgs2) and fail the test if they differ; use a Go deep-equal check (or reflect.DeepEqual) to compare fields and include the key/value in the error message for identification.pkg/zapscript/zaplinks_fuzz_test.go (1)
106-110: Consider comparingTrustedslice for complete determinism validation.The determinism check compares
ZapScriptandAuthbut not theTrustedslice. While JSON slice parsing is typically deterministic, adding a length/content check would provide complete coverage.♻️ Optional: Add Trusted slice comparison
if err == nil && err2 == nil { if wk.ZapScript != wk2.ZapScript || wk.Auth != wk2.Auth { t.Errorf("non-deterministic result for body %q", body) } + if len(wk.Trusted) != len(wk2.Trusted) { + t.Errorf("non-deterministic Trusted length for body %q", body) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zapscript/zaplinks_fuzz_test.go` around lines 106 - 110, The determinism check currently compares wk.ZapScript and wk.Auth but omits wk.Trusted; update the test inside the fuzz loop to also validate wk.Trusted vs wk2.Trusted by checking lengths and element-wise equality (or use reflect.DeepEqual) so the condition flags non-deterministic results when Trusted differs; reference the wk.Trusted and wk2.Trusted fields in the same if-block that checks ZapScript/Auth and include a clear failure message identifying Trusted mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Taskfile.dist.yml`:
- Around line 97-108: Remove the duplicate fuzz test named
FuzzParseVirtualPathStr from pkg/helpers/uris_fuzz_test.go (the one that targets
the virtualpath implementation) so the virtualpath function is only fuzzed by
pkg/helpers/virtualpath/virtualpath_fuzz_test.go; delete the test function
FuzzParseVirtualPathStr and its seed data in that file and update any references
(e.g., Taskfile entries listing "FuzzParseVirtualPathStr ./pkg/helpers") to
point only to the virtualpath fuzz test or remove the redundant listing so there
is a single canonical fuzz target for the virtualpath package.
---
Nitpick comments:
In `@pkg/zapscript/advargs/advargs_fuzz_test.go`:
- Around line 81-98: The determinism check only compares error presence; update
the tests so when Parse succeeds for a type (e.g., when err1 == nil and err1b ==
nil for LaunchRandomArgs, or similarly for PlaylistArgs and GlobalArgs) you also
deep-compare the two parsed structs (launchArgs vs launchArgs2, playlistArgs vs
playlistArgs2, globalArgs vs globalArgs2) and fail the test if they differ; use
a Go deep-equal check (or reflect.DeepEqual) to compare fields and include the
key/value in the error message for identification.
In `@pkg/zapscript/zaplinks_fuzz_test.go`:
- Around line 106-110: The determinism check currently compares wk.ZapScript and
wk.Auth but omits wk.Trusted; update the test inside the fuzz loop to also
validate wk.Trusted vs wk2.Trusted by checking lengths and element-wise equality
(or use reflect.DeepEqual) so the condition flags non-deterministic results when
Trusted differs; reference the wk.Trusted and wk2.Trusted fields in the same
if-block that checks ZapScript/Auth and include a clear failure message
identifying Trusted mismatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 919c117d-0ea2-47bb-b0df-ab7110c71ee4
📒 Files selected for processing (26)
.github/workflows/fuzz.ymlAGENTS.mdTaskfile.dist.ymlpkg/api/methods/run_fuzz_test.gopkg/api/middleware/encryption_fuzz_test.gopkg/api/models/rpcid.gopkg/api/models/rpcid_fuzz_test.gopkg/api/pairing_fuzz_test.gopkg/api/server_fuzz_test.gopkg/database/tags/filename_parser.gopkg/database/tags/testdata/fuzz/FuzzExtractSpecialPatterns/98fce631eb9c5dd5pkg/helpers/paths.gopkg/helpers/testdata/fuzz/FuzzDecodeURIIfNeeded/ba0160f680fd7fcbpkg/helpers/testdata/fuzz/FuzzFilenameFromPath/5f644fdcef1c73a8pkg/helpers/testdata/fuzz/FuzzGetPathBase/718f6ecdbb5c1f5bpkg/helpers/testdata/fuzz/FuzzGetPathDir/e40f64e0e1794357pkg/helpers/testdata/fuzz/FuzzGetPathExt/bf289923489eb280pkg/helpers/testdata/fuzz/FuzzParseVirtualPathStr/a6d9e8e6ad9ae59fpkg/helpers/uris.gopkg/helpers/uris_fuzz_test.gopkg/helpers/virtualpath/virtualpath.gopkg/readers/mqtt/common_fuzz_test.gopkg/readers/mqtt/mqtt_fuzz_test.gopkg/service/mappings_fuzz_test.gopkg/zapscript/advargs/advargs_fuzz_test.gopkg/zapscript/zaplinks_fuzz_test.go
…ecks Remove redundant FuzzParseVirtualPathStr from pkg/helpers (canonical copy lives in pkg/helpers/virtualpath), move its corpus file, and update Taskfile + docs. Add reflect.DeepEqual struct comparison to advargs fuzz determinism checks and Trusted field comparison to zaplinks fuzz test.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/testing/FUZZ_TESTING.md (1)
71-80:⚠️ Potential issue | 🟡 MinorKeep all
FuzzParseVirtualPathStrexamples on the same package path.Line 71 is correct, but Lines 74, 77, and 80 still point to
./pkg/helpers/. For this target, the examples should stay on./pkg/helpers/virtualpath/to avoid copy/paste failures.📘 Suggested doc fix
-# Time-boxed fuzzing (30 seconds) -go test -fuzz=FuzzParseVirtualPathStr -fuzztime=30s ./pkg/helpers/ +# Time-boxed fuzzing (30 seconds) +go test -fuzz=FuzzParseVirtualPathStr -fuzztime=30s ./pkg/helpers/virtualpath/ -# Run with more parallel workers (8 cores) -go test -fuzz=FuzzParseVirtualPathStr -parallel=8 ./pkg/helpers/ +# Run with more parallel workers (8 cores) +go test -fuzz=FuzzParseVirtualPathStr -parallel=8 ./pkg/helpers/virtualpath/ -# Disable minimization for faster fuzzing -go test -fuzz=FuzzParseVirtualPathStr -fuzzminimizetime=0 ./pkg/helpers/ +# Disable minimization for faster fuzzing +go test -fuzz=FuzzParseVirtualPathStr -fuzzminimizetime=0 ./pkg/helpers/virtualpath/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testing/FUZZ_TESTING.md` around lines 71 - 80, Update the fuzzing examples so all invocations of the FuzzParseVirtualPathStr target use the exact package path ./pkg/helpers/virtualpath/ instead of ./pkg/helpers/; specifically adjust the three commands that currently reference ./pkg/helpers/ (the -fuzztime, -parallel, and -fuzzminimizetime examples) to point to ./pkg/helpers/virtualpath/ so they match the first example and the actual fuzz target FuzzParseVirtualPathStr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/testing/FUZZ_TESTING.md`:
- Around line 71-80: Update the fuzzing examples so all invocations of the
FuzzParseVirtualPathStr target use the exact package path
./pkg/helpers/virtualpath/ instead of ./pkg/helpers/; specifically adjust the
three commands that currently reference ./pkg/helpers/ (the -fuzztime,
-parallel, and -fuzzminimizetime examples) to point to
./pkg/helpers/virtualpath/ so they match the first example and the actual fuzz
target FuzzParseVirtualPathStr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b0a0c52-e036-48d1-b8bb-5dd28828a97a
📒 Files selected for processing (7)
Taskfile.dist.ymlpkg/helpers/uris_fuzz_test.gopkg/helpers/virtualpath/testdata/fuzz/FuzzParseVirtualPathStr/a6d9e8e6ad9ae59fpkg/testing/FUZZ_TESTING.mdpkg/testing/README.mdpkg/zapscript/advargs/advargs_fuzz_test.gopkg/zapscript/zaplinks_fuzz_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/testing/README.md
- pkg/helpers/virtualpath/testdata/fuzz/FuzzParseVirtualPathStr/a6d9e8e6ad9ae59f
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/helpers/uris_fuzz_test.go
- Taskfile.dist.yml
- pkg/zapscript/advargs/advargs_fuzz_test.go
- pkg/zapscript/zaplinks_fuzz_test.go
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
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.
Summary
task fuzzfrom 6 to 36 targets with continue-on-failure loopSummary by CodeRabbit
New Features
Testing
Bug Fixes
Documentation