fix(fuzz): stabilize path fuzzing for numeric segments#7152
fix(fuzz): stabilize path fuzzing for numeric segments#7152a638011 wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Comment |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIntroduces an immutable originalPath snapshot in the Path component, switches path segments/values to ordered maps with 1-based indexing to preserve insertion order and empty segments, deep-copies request URLs on Clone/Rebuild, and changes execWithInput to use the actual (possibly substituted) parameter value for frequency checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/fuzz/component/path.go (2)
95-123:⚠️ Potential issue | 🟠 MajorRebuild still collapses empty path segments.
Using
originalPathfixes ordering, but thecontinueon empty segments still rewrites paths like/a//b/into/a/band drops trailing slashes. That changes the target route, so path fuzzing is still not layout-preserving for these inputs.Suggested fix
for i := 1; i < len(originalSplitted); i++ { originalSegment := originalSplitted[i] if originalSegment == "" { - // Skip empty segments - continue + rebuiltSegments = append(rebuiltSegments, "") + continue } // Check if we have a replacement for this segment key := strconv.Itoa(segmentIndex)🤖 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 95 - 123, The rebuild loop currently skips empty segments (the `continue`), which collapses consecutive slashes and drops trailing slash; instead preserve empty segments by removing the `continue`: when originalSegment == "" append an empty string to rebuiltSegments (so `/a//b/` becomes `["", "a", "", "b", ""]`) and still advance segmentIndex and the rest of the logic so q.value.parsed.Get(strconv.Itoa(segmentIndex)) stays in sync; update the loop that references originalSplitted, rebuiltSegments, segmentIndex, and q.value.parsed.Get to append "" for empty segments rather than skipping them.
138-143:⚠️ Potential issue | 🟠 MajorEnsure
Rebuild()doesn't mutate the base request by deep-copying the URL before mutation.The current implementation clones the request but may still mutate
q.reqifretryablehttp.Request.Clone()shares the underlying URL pointer. The testTestPathComponent_RebuildUsesOriginalPathSnapshot(lines 151-171) only verifies that the rebuilt path is correct, but doesn't assert that the original request remains unchanged afterRebuild()completes. Add an assertion to verifyreq.Pathstays unchanged after rebuilding, and consider deep-copying the URL struct before callingUpdateRelPath()to guarantee immutability of the original request object.🤖 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 138 - 143, The Rebuild() implementation currently clones q.req but may still share the underlying URL; fix by deep-copying the URL struct on the cloned request before calling UpdateRelPath — e.g., after cloned := q.req.Clone(...), allocate a new url.URL, copy *cloned.URL into it and assign cloned.URL = newURL so UpdateRelPath cannot mutate q.req; preserve the existing RawPath fallback behavior (setting cloned.RawPath = rebuiltPath on error). Also update the TestPathComponent_RebuildUsesOriginalPathSnapshot test to assert that the original req.Path (or original q.req.URL.Path) remains unchanged after calling Rebuild().
🧹 Nitpick comments (1)
pkg/fuzz/parts_frequency_test.go (1)
34-41: Use the same target when seeding frequency state.This only proves the old collision if both calls normalize to the same target. Marking
"99"against/blog/99/postmakes the test depend onnormalizeTarget()stripping the path. Seeding withbaseReq.String()keeps the regression focused on the parameter-key change itself.Suggested test tightening
- // Mark a different numeric path segment as frequent on the same target. - // With the old code, both values collided on the raw index key "2". - targetReq, err := retryablehttp.NewRequest(http.MethodGet, "https://example.com/blog/99/post", nil) - require.NoError(t, err) - rule.options.FuzzParamsFrequency.MarkParameter("99", targetReq.String(), rule.options.TemplateID) - baseReq, err := retryablehttp.NewRequest(http.MethodGet, "https://example.com/user/55/profile", nil) require.NoError(t, err) + // Mark a different numeric path segment as frequent on the same target. + // With the old code, both values collided on the raw index key "2". + rule.options.FuzzParamsFrequency.MarkParameter("99", baseReq.String(), rule.options.TemplateID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/parts_frequency_test.go` around lines 34 - 41, The test currently marks the frequency using targetReq.String(), which makes the assertion depend on normalizeTarget() behavior; instead create both requests (baseReq and targetReq) and seed the frequency state using the same target string as the later check by calling rule.options.FuzzParamsFrequency.MarkParameter("99", baseReq.String(), rule.options.TemplateID) (ensure baseReq is constructed before you call MarkParameter), so the test isolates the parameter-key collision logic rather than path normalization—refer to targetReq, baseReq, MarkParameter and normalizeTarget() when locating the change.
🤖 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/fuzz/component/path.go`:
- Around line 95-123: The rebuild loop currently skips empty segments (the
`continue`), which collapses consecutive slashes and drops trailing slash;
instead preserve empty segments by removing the `continue`: when originalSegment
== "" append an empty string to rebuiltSegments (so `/a//b/` becomes `["", "a",
"", "b", ""]`) and still advance segmentIndex and the rest of the logic so
q.value.parsed.Get(strconv.Itoa(segmentIndex)) stays in sync; update the loop
that references originalSplitted, rebuiltSegments, segmentIndex, and
q.value.parsed.Get to append "" for empty segments rather than skipping them.
- Around line 138-143: The Rebuild() implementation currently clones q.req but
may still share the underlying URL; fix by deep-copying the URL struct on the
cloned request before calling UpdateRelPath — e.g., after cloned :=
q.req.Clone(...), allocate a new url.URL, copy *cloned.URL into it and assign
cloned.URL = newURL so UpdateRelPath cannot mutate q.req; preserve the existing
RawPath fallback behavior (setting cloned.RawPath = rebuiltPath on error). Also
update the TestPathComponent_RebuildUsesOriginalPathSnapshot test to assert that
the original req.Path (or original q.req.URL.Path) remains unchanged after
calling Rebuild().
---
Nitpick comments:
In `@pkg/fuzz/parts_frequency_test.go`:
- Around line 34-41: The test currently marks the frequency using
targetReq.String(), which makes the assertion depend on normalizeTarget()
behavior; instead create both requests (baseReq and targetReq) and seed the
frequency state using the same target string as the later check by calling
rule.options.FuzzParamsFrequency.MarkParameter("99", baseReq.String(),
rule.options.TemplateID) (ensure baseReq is constructed before you call
MarkParameter), so the test isolates the parameter-key collision logic rather
than path normalization—refer to targetReq, baseReq, MarkParameter and
normalizeTarget() when locating the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44e328bb-dd93-4163-8301-d1ebe8d34640
📒 Files selected for processing (4)
pkg/fuzz/component/path.gopkg/fuzz/component/path_test.gopkg/fuzz/parts.gopkg/fuzz/parts_frequency_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/fuzz/component/path.go`:
- Around line 118-121: The rebuild currently treats an explicit empty
replacement as "missing" because of the newValue != "" check; change the logic
in the rebuild loop (the branch using q.value.parsed.Get(key).(string),
rebuiltSegments and originalSegment) to only fall back to originalSegment when
the key is absent (i.e., when the type assertion/ok is false). In other words,
remove the newValue != "" guard so that when ok is true you append newValue even
if it's an empty string, and only append originalSegment when ok is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e348f6a-0511-4daa-9242-e4cb46d2f4a2
📒 Files selected for processing (2)
pkg/fuzz/component/path.gopkg/fuzz/component/path_test.go
…y tests - Remove newValue != "" guard to support explicit empty string replacements - Clarify URL deep-copy with intermediate variable for better readability - Add explicit assertions that original request path remains unchanged after Rebuild - Addresses CodeRabbit review feedback on PR projectdiscovery#7152
1 participant
Proposed Changes
Fixes #6398 — path-based fuzzing still skips numeric segments on
devunder some runs.This keeps the fix minimal and addresses the remaining root causes together:
Deterministic path segment iteration
Path.Parse()used a plain Go map, so segment iteration order was unstable./user/55/profileis always iterated asuser,55,profile.Preserve the original path across rebuilds
retryablehttp.Request.Clone()shares the underlying URL pointer, soUpdateRelPath()mutates the original request path too.Frequency tracking uses the effective path parameter
"2") instead of the effective path value ("55").actualParameterso different numeric segments do not collide on the same path position.Proof
Reproduced on current
dev:Path.Parse()+Iterate()on/user/55/profileproduced different orders across runs, including:[1=user 2=55 3=profile][2=55 3=profile 1=user][3=profile 1=user 2=55]Path.Rebuild()mutated the original request path after cloning:/user/55/profile/user/55/profile OR TrueFocused regression tests added:
All passed locally.
Summary
pkg/fuzz/component/path.goChecklist
dev/claim #6398
Summary by CodeRabbit
New Features
Bug Fixes
Tests