fix(fuzz): prevent path mutation across sequential Rebuild calls (#6398)#7253
Conversation
…jectdiscovery#6398) retryablehttp.Request.Clone() shares the URL pointer, so when Rebuild() calls UpdateRelPath() on the cloned request it mutates q.req.URL.Path. On subsequent Rebuild() calls the original path segments are wrong, causing corrupted fuzz requests (especially visible with numeric path parts like /user/55/profile). Snapshot the original path during Parse() and use the snapshot in Rebuild() instead of reading from the mutable request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/fuzz/component/path_test.go (1)
83-124: Good regression test; consider adding explicit mutation check.The test correctly simulates single-mode fuzzing and validates that all three segments (including the numeric
55) are properly fuzzed. The use ofrequire.Containshandles Go's non-deterministic map iteration order.However, the test name promises "RebuildDoesNotMutateOriginal" but only verifies fuzzing works—it doesn't explicitly assert the original wasn't mutated. Consider adding a direct verification for stronger coverage.
📝 Optional: Add explicit mutation check
// All three segments must have been fuzzed require.Len(t, fuzzedPaths, 3, "expected 3 fuzzed paths for 3 segments") + // Verify original request path was not mutated + require.Equal(t, "/user/55/profile", req.URL.Path, "original request path should not be mutated") + // Verify that each segment was individually fuzzed require.Contains(t, fuzzedPaths, "/user FUZZED/55/profile")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/component/path_test.go` around lines 83 - 124, Test name promises the original Path isn't mutated but currently only checks fuzzed outputs; capture the original segment values before fuzzing (using path.Iterate into a snapshot map or reading from req), run the existing fuzzing loop, then assert the snapshot still equals the current path state by re-iterating path.Iterate or calling path.Parse(req) and comparing segment values; reference TestPathComponent_RebuildDoesNotMutateOriginal, the path variable, segments map, SetValue, Rebuild and Iterate to locate where to take the snapshot and where to compare afterward.pkg/fuzz/component/path.go (1)
144-149: Clone implementation is correct; consider documenting context choice.The Clone method correctly propagates
originalPathand clones the value. Usingcontext.Background()is consistent with Rebuild() (line 136), but it discards any context attached to the original request (e.g., deadlines, tracing). If this is intentional for isolation purposes, a brief comment would clarify the design choice.📝 Optional: Add clarifying comment
// Clones current state to a new component func (q *Path) Clone() Component { return &Path{ value: q.value.Clone(), originalPath: q.originalPath, + // Use Background context for isolation; cloned components + // operate independently of the original request's lifecycle. req: q.req.Clone(context.Background()), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/component/path.go` around lines 144 - 149, Add a brief comment above Path.Clone explaining the deliberate use of context.Background() when calling q.req.Clone(context.Background()), referencing the Path.Clone method and the Rebuild() behavior for consistency; state that this intentionally discards any caller deadlines/tracing to ensure isolation (or alternatively note how to preserve the original request context if that was intended), so future readers understand the tradeoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/fuzz/component/path_test.go`:
- Around line 83-124: Test name promises the original Path isn't mutated but
currently only checks fuzzed outputs; capture the original segment values before
fuzzing (using path.Iterate into a snapshot map or reading from req), run the
existing fuzzing loop, then assert the snapshot still equals the current path
state by re-iterating path.Iterate or calling path.Parse(req) and comparing
segment values; reference TestPathComponent_RebuildDoesNotMutateOriginal, the
path variable, segments map, SetValue, Rebuild and Iterate to locate where to
take the snapshot and where to compare afterward.
In `@pkg/fuzz/component/path.go`:
- Around line 144-149: Add a brief comment above Path.Clone explaining the
deliberate use of context.Background() when calling
q.req.Clone(context.Background()), referencing the Path.Clone method and the
Rebuild() behavior for consistency; state that this intentionally discards any
caller deadlines/tracing to ensure isolation (or alternatively note how to
preserve the original request context if that was intended), so future readers
understand the tradeoff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a6dd940-a73c-40d7-b03d-a51678676f63
📒 Files selected for processing (2)
pkg/fuzz/component/path.gopkg/fuzz/component/path_test.go
Made-with: Cursor # Conflicts: # pkg/fuzz/component/path.go
Neo - PR Security ReviewCaution Review could not be completed Review could not be completed. Please retry with Suggestion: Try again with Comment |
|
/claim #6398 |
2 participants
Summary
Fixes #6398 — Fuzzing skips numeric path parts due to shared state mutation across sequential
Rebuildcalls.Problem
When fuzzing URLs with numeric path segments (e.g.
/api/v2/users), the path parts slice was being mutated in place during iteration. SubsequentRebuildcalls would see the already-modified path, causing numeric segments to be skipped.Fix
Clone the path parts slice before mutation in
Rebuildto prevent cross-call contamination.Testing
Added unit tests covering numeric path parts fuzzing to verify all segments are properly fuzzed.
/claim #6398
Summary by CodeRabbit
Bug Fixes
Tests