Repository navigation
test(fuzz): add dynamic-analysis fuzzing gate for manifestedit and webhook - #188
Merged
Merged
Conversation
…bhook Add Go fuzzing as the OpenSSF Best Practices dynamic-analysis mechanism, plus `task fuzz` / `task fuzz-smoke` wrappers (run under -race) and CONTRIBUTING docs. Two targets assert the no-panic robustness invariant against hostile input: - FuzzManifestEdit (internal/git/manifestedit): the YAML split/decode/index and decide/apply/patch/delete surface. - FuzzDecodeEventList (internal/webhook): the untrusted audit ServeHTTP decode path plus the admission commandObjectUID JSON probe. Both survive 1M+ generated inputs with no failure. Convergence stays covered by TestConvergence_Corpus on realistic manifests — a fuzzer can only synthesize a "desired" object by parsing arbitrary YAML, which yields type-ambiguous shapes the typed Kubernetes API never produces, so convergence there tests the harness, not the code (see docs/design/dynamic-analysis-fuzzing-plan.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Go fuzz tests ( ChangesFuzzing infrastructure and docs
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Fuzzer
participant FuzzManifestEdit
participant fuzzExerciseEdit
participant PatchDocument
participant FuzzDecodeEventList
participant AuditHandler
Fuzzer->>FuzzManifestEdit: fuzz input bytes
FuzzManifestEdit->>fuzzExerciseEdit: build & sanitize document
fuzzExerciseEdit->>PatchDocument: patch with ordered YAML
PatchDocument-->>FuzzManifestEdit: no panic
Fuzzer->>FuzzDecodeEventList: fuzz input bytes
FuzzDecodeEventList->>AuditHandler: POST /audit-webhook
AuditHandler-->>FuzzDecodeEventList: HTTP status 1xx-5xx
Related PRs: None identified. Suggested labels: documentation, testing, ci Suggested reviewers: None identified. 🐰 A fuzzer hops through bytes so strange, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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: 2
🧹 Nitpick comments (2)
internal/git/manifestedit/fuzz_test.go (1)
86-107: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant
splitDocumentscall.
splitDocuments(string(data))is already computed once per fuzz input in thef.Fuzzloop (line 70);fuzzExerciseEditrecomputes it again for everyidx, tripling the split work per input. Since this runs on every fuzz iteration (potentially millions), consider passing the already-splitdocsslice in.♻️ Proposed refactor
-func fuzzExerciseEdit(content []byte, idx int) { - docs := splitDocuments(string(content)) - if idx < 0 || idx >= len(docs) { - return - } +func fuzzExerciseEdit(content []byte, idx int, docs []splitDoc) { + if idx < 0 || idx >= len(docs) { + return + }(and update the call site to pass the slice already produced by
splitDocumentsin thef.Fuzzclosure — adjustsplitDocto the actual element type returned bysplitDocuments.)🤖 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/git/manifestedit/fuzz_test.go` around lines 86 - 107, The fuzz helper is re-splitting the input documents unnecessarily, which adds repeated work on every iteration. Update fuzzExerciseEdit to accept the already computed docs slice from the f.Fuzz closure instead of calling splitDocuments(string(content)) internally, and adjust the call site to pass that slice through; use the splitDocuments and fuzzExerciseEdit symbols to find the refactor point.Taskfile-build.yml (1)
168-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate loop logic between
fuzz-smokeandfuzz.Both tasks run the identical per-target loop, differing only in the fuzztime variable and description. Consider factoring the loop into a shared internal task parameterized by a time var to avoid drift if the invocation flags change later.
♻️ Proposed consolidation
+ _fuzz-run: + internal: true + requires: + vars: + - FUZZTIME + cmds: + - | + set -eu + for entry in {{.FUZZ_TARGETS}}; do + pkg="${entry%%:*}"; target="${entry##*:}" + echo "==> fuzz ${target} (${pkg}) for {{.FUZZTIME}}" + go test "${pkg}" -run '^$' -fuzz="^${target}$" -fuzztime={{.FUZZTIME}} -race + done + fuzz-smoke: desc: Short fuzzing smoke — briefly active-fuzz each target under the race detector cmds: - - | - set -eu - for entry in {{.FUZZ_TARGETS}}; do - pkg="${entry%%:*}"; target="${entry##*:}" - echo "==> fuzz-smoke ${target} (${pkg}) for {{.FUZZ_SMOKE_TIME}}" - go test "${pkg}" -run '^$' -fuzz="^${target}$" -fuzztime={{.FUZZ_SMOKE_TIME}} -race - done + - task: _fuzz-run + vars: + FUZZTIME: '{{.FUZZ_SMOKE_TIME}}' fuzz: desc: >- Release-time dynamic analysis — longer fuzz discovery run per target under the race detector. Crash reproducers land under <pkg>/testdata/fuzz/<Target>/ (commit them). See docs/design/dynamic-analysis-fuzzing-plan.md. cmds: - - | - set -eu - for entry in {{.FUZZ_TARGETS}}; do - pkg="${entry%%:*}"; target="${entry##*:}" - echo "==> fuzz ${target} (${pkg}) for {{.FUZZ_TIME}}" - go test "${pkg}" -run '^$' -fuzz="^${target}$" -fuzztime={{.FUZZ_TIME}} -race - done + - task: _fuzz-run + vars: + FUZZTIME: '{{.FUZZ_TIME}}'🤖 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 `@Taskfile-build.yml` around lines 168 - 192, The per-target fuzz loop is duplicated in fuzz-smoke and fuzz, which risks drift if the Go test invocation changes. Factor the shared logic into one internal task or reusable command in Taskfile-build.yml, and parameterize only the fuzz time value used by fuzz-smoke and fuzz. Keep the existing behavior in the fuzz-smoke and fuzz task definitions, but have both call the shared helper that handles the entry parsing, logging, and go test invocation.
🤖 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.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 72-76: Soften the wording in the CONTRIBUTING guidance for the
fuzzed parsing paths: the current text overstates coverage by saying hostile
input can “never” crash the controller. Rephrase the description around the
current fuzz targets in internal/git/manifestedit and internal/webhook to say
they reduce crash risk on those untrusted/semi-structured input paths, while
keeping the note that task test replays fuzz seeds and crash reproducers as
regression cases.
In `@docs/design/dynamic-analysis-fuzzing-plan.md`:
- Around line 21-42: Recheck the badge-level mapping in the
dynamic-analysis-fuzzing-plan section and adjust the explanation around
dynamic_analysis, enable_assertions, and dynamic_analysis_fixed so it matches
the Passing badge rules. The current text incorrectly treats the 80%
branch-coverage threshold as a blocker for Passing; update the wording to state
that dynamic_analysis and enable_assertions are only suggested for Passing,
while the coverage threshold applies to higher badge levels, and keep the
discussion focused on why fuzzing is the chosen path.
---
Nitpick comments:
In `@internal/git/manifestedit/fuzz_test.go`:
- Around line 86-107: The fuzz helper is re-splitting the input documents
unnecessarily, which adds repeated work on every iteration. Update
fuzzExerciseEdit to accept the already computed docs slice from the f.Fuzz
closure instead of calling splitDocuments(string(content)) internally, and
adjust the call site to pass that slice through; use the splitDocuments and
fuzzExerciseEdit symbols to find the refactor point.
In `@Taskfile-build.yml`:
- Around line 168-192: The per-target fuzz loop is duplicated in fuzz-smoke and
fuzz, which risks drift if the Go test invocation changes. Factor the shared
logic into one internal task or reusable command in Taskfile-build.yml, and
parameterize only the fuzz time value used by fuzz-smoke and fuzz. Keep the
existing behavior in the fuzz-smoke and fuzz task definitions, but have both
call the shared helper that handles the entry parsing, logging, and go test
invocation.
🪄 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 Plus
Run ID: 1e6bac87-8057-43f8-b8dd-dc1d835976d4
📒 Files selected for processing (5)
CONTRIBUTING.mdTaskfile-build.ymldocs/design/dynamic-analysis-fuzzing-plan.mdinternal/git/manifestedit/fuzz_test.gointernal/webhook/fuzz_test.go
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
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.
What
Adds Go fuzzing as the project's dynamic-analysis mechanism, so hostile or
malformed input to the parsing surfaces can never crash the controller — and so
the OpenSSF Best Practices Badge dynamic-analysis criteria can be answered
Met with a real, running gate rather than a claim.
Two fuzz targets, each asserting the no-panic robustness invariant against
arbitrary input:
FuzzManifestEdit(internal/git/manifestedit) — the hand-rolled YAMLsplit / block-scalar / decode / index and decide-apply-patch-delete surface.
FuzzDecodeEventList(internal/webhook) — the network-facing auditServeHTTPdecode path (untrusted JSONEventList, bounded by anio.LimitReader) plus the admissioncommandObjectUIDJSON probe.Both survive 1M+ generated inputs with no failure.
Plus:
task fuzz(release-time discovery, per target) andtask fuzz-smoke(shortsmoke) wrappers in
Taskfile-build.yml, both run under the race detector.CONTRIBUTING.md.docs/design/dynamic-analysis-fuzzing-plan.md— the plan and the copy-readybadge answers.
Why no-panic, not convergence
The plan originally proposed asserting manifest-edit convergence. That is the
wrong invariant for a fuzzer: convergence only holds for a canonical desired
object, and in production the desired state always comes from the typed
Kubernetes API. A fuzzer can only synthesize a "desired" by parsing arbitrary
YAML, which yields type-ambiguous shapes the real system never produces (e.g.
apiVersion: 00→ a number;data: {0: 000000}→ a numeric ConfigMap value), soevery "non-convergence" it found was a harness artifact, not a product bug.
Convergence stays covered by
TestConvergence_Corpuson realistic manifests;no-panic is the universal property worth fuzzing. Full rationale in the plan doc.
Badge answers this unlocks
task fuzz).-race).testdata/fuzz/…,replayed by
task test).CI wiring
None required. A plain
go test(viatask test) already replays thef.Addseeds and any committed crash reproducer as ordinary regression cases, so this
adds no CI job.
task fuzzis deliberately not wired into required CI —active fuzzing is nondeterministic and belongs in the release checklist, not the
everyday green gate.
Validation
task fmt/task vet/task lint(0 issues)task test— coverage held at 73.9% baselinetask fuzz-smoke— both targets, under-racetask test-e2e— 53 passed, 0 failed, 8 skipped🤖 Generated with Claude Code
Summary by CodeRabbit