fix(fuzz): use OrderedMap for deterministic path segment iteration#6940
fix(fuzz): use OrderedMap for deterministic path segment iteration#6940jpirstin wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
Path.Parse() used a plain Go map for storing path segments, which has
non-deterministic iteration order. This caused fuzzing templates to
intermittently skip numeric path parts (e.g., /user/55/profile would
sometimes not fuzz the '55' segment).
Fix: Replace map[string]interface{} with mapsutil.OrderedMap to guarantee
insertion-order iteration, matching the pattern already used by the Cookie
component.
Also fix Rebuild() to use the KV.Get() accessor instead of directly
accessing the .Map field, which is nil when using OrderedMap.
Fixes projectdiscovery#6398
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughReplaces the plain map for path segments with an ordered map using 1-based numeric keys, updates parsing and Rebuild to use the ordered structure (with type-checked replacements and trailing-slash preservation), and adds tests verifying deterministic ordering and trailing-slash behavior plus test refactors to use 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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)
111-157: Mixed assertion style — consider aligning withrequireused inTestPathComponent_DeterministicOrder.
TestPathComponent_SQLInjectionusesif err != nil { t.Fatal }/ bareif newReq.Path != ...checks, while the newTestPathComponent_DeterministicOrderusesrequire.*. Aligning styles improves readability and prevents missing follow-on checks if a fatal error is encountered mid-test.♻️ Proposed style unification
func TestPathComponent_SQLInjection(t *testing.T) { path := NewPath() req, err := retryablehttp.NewRequest(http.MethodGet, "https://example.com/user/55/profile", nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) found, err := path.Parse(req) - if err != nil { - t.Fatal(err) - } - if !found { - t.Fatal("expected path to be found") - } + require.NoError(t, err) + require.True(t, found) // ... err = path.Iterate(func(key string, value interface{}) error { // ... return nil }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) newReq, err := path.Rebuild() - if err != nil { - t.Fatal(err) - } - if newReq.Path != "/user/55 OR True/profile" { - t.Fatalf("expected path to be '/user/55 OR True/profile', got '%s'", newReq.Path) - } + require.NoError(t, err) + require.Equal(t, "/user/55 OR True/profile", newReq.Path) }🤖 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 111 - 157, The test TestPathComponent_SQLInjection mixes t.Fatal/if checks with bare comparisons; switch to the same require.* style used in TestPathComponent_DeterministicOrder to improve consistency and fail-fast behavior: replace err checks (e.g., after retryablehttp.NewRequest, path.Parse, path.Iterate, path.Rebuild) with require.NoError calls and replace the final equality assertion comparing newReq.Path with require.Equal (or require.Equalf) so all assertions use testify/require and reference the TestPathComponent_SQLInjection function and the variables req, path, newReq to locate and update the checks.pkg/fuzz/component/path.go (1)
95-126: Trailing slash is silently dropped duringRebuild.
Parseskips all empty segments (including trailing ones from a path like/user/55/).Rebuilddoes the same, so a request for/user/55/will be rebuilt as/user/55, dropping the trailing slash. This is likely pre-existing behavior carried forward from the old implementation, but it should be confirmed as intentional since it can alter the semantics of the request (some servers treat/resource/and/resourcedifferently).🤖 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 - 126, Rebuild currently drops trailing empty segments because the loop skips empty segments; after processing segments in the Rebuild logic (the loop that iterates originalSplitted and uses q.value.parsed.Get with keys like strconv.Itoa(segmentIndex)), detect if the original path had a trailing slash (i.e., len(originalSplitted) > 1 && originalSplitted[len(originalSplitted)-1] == "") and if so append an empty segment ("" ) to rebuiltSegments so joining will preserve the trailing slash; keep the existing behavior of skipping intermediate empty segments but ensure the leading empty segment is preserved as already handled.
🤖 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 111-157: The test TestPathComponent_SQLInjection mixes t.Fatal/if
checks with bare comparisons; switch to the same require.* style used in
TestPathComponent_DeterministicOrder to improve consistency and fail-fast
behavior: replace err checks (e.g., after retryablehttp.NewRequest, path.Parse,
path.Iterate, path.Rebuild) with require.NoError calls and replace the final
equality assertion comparing newReq.Path with require.Equal (or require.Equalf)
so all assertions use testify/require and reference the
TestPathComponent_SQLInjection function and the variables req, path, newReq to
locate and update the checks.
In `@pkg/fuzz/component/path.go`:
- Around line 95-126: Rebuild currently drops trailing empty segments because
the loop skips empty segments; after processing segments in the Rebuild logic
(the loop that iterates originalSplitted and uses q.value.parsed.Get with keys
like strconv.Itoa(segmentIndex)), detect if the original path had a trailing
slash (i.e., len(originalSplitted) > 1 &&
originalSplitted[len(originalSplitted)-1] == "") and if so append an empty
segment ("" ) to rebuiltSegments so joining will preserve the trailing slash;
keep the existing behavior of skipping intermediate empty segments but ensure
the leading empty segment is preserved as already handled.
|
Friendly ping — any bandwidth to review this one? It fixes non-deterministic path segment ordering in the fuzzer's query parameter handler. Happy to address any feedback. |
|
CodeRabbit review is now complete (pass). Wondering if a maintainer could take a look when you get a chance — this is a targeted fix for non-deterministic OrderedMap iteration in path segment parsing with a regression test included. No changes to public API. Appreciate your time. |
…lash in path Rebuild - TestPathComponent_SQLInjection: replace t.Fatal/if checks with require.NoError, require.True, require.Equal to match the style used in TestPathComponent_DeterministicOrder - path.go Rebuild: append empty trailing segment when original path ends with '/' so the rebuilt path also ends with '/' (servers may treat /resource/ vs /resource differently) - Add TestPathComponent_TrailingSlash regression test
|
Friendly ping — this PR has been open for 10 days and CodeRabbit review completed with no actionable comments. Happy to address any maintainer feedback or rebase if needed. Thanks for your time! |
1 participant
Proposed Changes
Fixes #6398
Path.Parse()used a plain Gomap[string]interface{}for storing path segments, which has non-deterministic iteration order. This caused fuzzing templates to intermittently skip numeric path parts (e.g.,/user/55/profilewould sometimes not fuzz the55segment).Root Cause
Go maps iterate in random order by design. When the path segments were stored in a plain map with string keys
"1","2","3", the iteration order was non-deterministic, causing some segments to be skipped during fuzzing.Fix
map[string]interface{}withmapsutil.OrderedMapinPath.Parse()to guarantee insertion-order iteration. This matches the pattern already used by theCookiecomponent.Rebuild()to useKV.Get()accessor instead of directly accessing the.Mapfield, which isnilwhen usingOrderedMap.Test Results (run just now, rebased on latest dev)
The
TestPathComponent_DeterministicOrdertest runs 50 iterations to verify keys always come back in insertion order["1", "2", "3"]with values["user", "55", "profile"].Files Changed
pkg/fuzz/component/path.go— Replace map with OrderedMap, fix Rebuild accessorpkg/fuzz/component/path_test.go— Add deterministic order + SQLi injection testsChecklist
devbranch/claim #6398
Summary by CodeRabbit
Bug Fixes
Tests
Refactor