fix(httpclientpool): rebuild malformed Location URL #5902
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)pkg/protocols/http/httpclientpool/clientpool.go (3)
The addition of the
Enhance URL rebuild logic and error handling The current implementation has several areas for improvement:
Consider this enhanced implementation: // NOTE(dwisiswant0): rebuild request URL. See #5900.
if u := req.URL.String(); !isURLEncoded(u) {
parsed, err := urlutil.Parse(u)
if err != nil {
- return fmt.Errorf("%w: %w", ErrRebuildURL, err)
+ return fmt.Errorf("failed to rebuild URL %q: %w", u, err)
}
+
+ // Preserve original query parameters
+ if req.URL.RawQuery != "" {
+ parsed.URL.RawQuery = req.URL.RawQuery
+ }
+
+ // Validate the rebuilt URL
+ if parsed.URL.Scheme == "" || parsed.URL.Host == "" {
+ return fmt.Errorf("invalid URL after rebuild: missing scheme or host")
+ }
req.URL = parsed.URL
}
Improve URL encoding detection and consider package move The current implementation has limitations with partially encoded URLs and edge cases. Also, as noted in the comment, this utility function should be moved to Consider this enhanced implementation: -// isURLEncoded is an helper function to check if the URL is already encoded
-//
-// NOTE(dwisiswant0): shall we move this under `projectdiscovery/utils/urlutil`?
-func isURLEncoded(s string) bool {
- decoded, err := url.QueryUnescape(s)
- if err != nil {
- // If decoding fails, it may indicate a malformed URL/invalid encoding.
- return false
- }
-
- return decoded != s
+// isURLFullyEncoded checks if a URL string is properly encoded
+// It handles partial encoding and common edge cases
+func isURLFullyEncoded(s string) bool {
+ // Quick check for common encoded characters
+ if !strings.Contains(s, "%") {
+ return false
+ }
+
+ decoded, err := url.QueryUnescape(s)
+ if err != nil {
+ return false
+ }
+
+ // Check if re-encoding produces the same string
+ reEncoded := url.QueryEscape(decoded)
+ return s == reEncoded
}Consider:
Let's verify if there are any existing URL encoding utilities in the codebase: 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
pkg/protocols/http/httpclientpool/clientpool.go (1)
394-402: Consider additional validation and error handlingWhile the URL reconstruction logic addresses the core issue, consider these improvements:
- Validate that
parsed.URLis not nil before assignment- Consider preserving the original query parameters if present
- Add debug logging for URL reconstruction attempts
if u := req.URL.String(); !isURLEncoded(u) { parsed, err := urlutil.Parse(u) if err != nil { return fmt.Errorf("%w: %w", ErrRebuildURL, err) } + if parsed.URL == nil { + return fmt.Errorf("%w: nil URL after parsing", ErrRebuildURL) + } + // Preserve original query parameters if any + if req.URL.RawQuery != "" && parsed.URL.RawQuery == "" { + parsed.URL.RawQuery = req.URL.RawQuery + } req.URL = parsed.URL }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/protocols/http/httpclientpool/clientpool.go(4 hunks)pkg/protocols/http/httpclientpool/errors.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/protocols/http/httpclientpool/errors.go
🔇 Additional comments (1)
pkg/protocols/http/httpclientpool/clientpool.go (1)
394-402: Verify the fix with various URL patterns
While the implementation addresses the core issue of encoding spaces in URLs, let's verify it handles various URL patterns correctly.
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Implementation correctly handles URL encoding
The implementation in isURLEncoded and the URL rebuilding logic is sound and properly handles URL encoding:
isURLEncodedfunction correctly detects if a URL is already encoded by attempting to decode it and comparing with original- The URL rebuilding logic properly handles malformed URLs by:
- Checking if URL needs encoding
- Using
urlutil.Parseto properly parse and rebuild the URL - Returning appropriate errors if parsing fails
The implementation aligns with the fix's objectives and handles various URL patterns safely.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases and URL handling patterns in the codebase
# Look for existing URL handling test cases
echo "Searching for URL-related test cases..."
rg -g "**/*_test.go" "func Test.*URL" -A 5
# Look for other URL encoding/decoding patterns
echo "Searching for URL encoding patterns..."
ast-grep --pattern 'url.QueryEscape($_)'
# Look for redirect-related test cases
echo "Searching for redirect test cases..."
rg -g "**/*_test.go" "Location.*header" -A 5
Length of output: 2585
Script:
#!/bin/bash
# Let's check for URL encoding and redirection test cases more specifically
# Look for URL encoding/decoding tests and implementations
echo "Searching for URL encoding/decoding tests..."
rg -g "**/*_test.go" "isURLEncoded|urlutil.Parse" -A 5
# Check the implementation of isURLEncoded function
echo "Checking isURLEncoded implementation..."
rg "func isURLEncoded" -A 10
# Look for URL-related test cases in the clientpool package
echo "Searching for clientpool URL tests..."
rg -g "pkg/protocols/http/httpclientpool/*_test.go" "func Test" -A 5
# Check for URL handling in redirect scenarios
echo "Checking redirect handling..."
rg "Location.*=|Location.*:" -A 5
Length of output: 72085
Script:
#!/bin/bash
# Let's check the implementation and test coverage more specifically
# Check the implementation of isURLEncoded and related functions
echo "Checking URL encoding implementation..."
rg "isURLEncoded|urlutil.Parse" -A 10
# Look for URL-related test cases in the httpclientpool package
echo "Searching for URL-related tests in httpclientpool..."
fd -g "*_test.go" pkg/protocols/http/httpclientpool/ --exec cat {}
# Check for URL encoding/decoding in urlutil package
echo "Checking urlutil package implementation..."
fd -g "*.go" pkg/protocols/utils/urlutil/ --exec cat {}
Length of output: 43221
dwisiswant0
force-pushed
the
dwisiswant0/fix/httpclientpool/rebuild-malformed-location-URL
branch
from
2926a98 to
650bd67
Compare
Signed-off-by: Dwi Siswanto <[email protected]>
dwisiswant0
force-pushed
the
dwisiswant0/fix/httpclientpool/rebuild-malformed-location-URL
branch
from
650bd67 to
b4fe1df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/http/httpclientpool/clientpool.go(4 hunks)pkg/protocols/http/httpclientpool/errors.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/http/httpclientpool/errors.go
🔇 Additional comments (3)
pkg/protocols/http/httpclientpool/clientpool.go (3)
29-29: LGTM: Import addition is appropriate
The addition of the urlutil package is well-aligned with the PR's objective of improving URL handling.
407-418: Consider enhancing URL encoding detection
The current implementation has limitations with partially encoded URLs and edge cases. Also, as noted in the comment, this utility function could be moved to projectdiscovery/utils/urlutil.
Line range hint 382-403: Verify the fix handles spaces in URLs correctly
Let's ensure the changes properly handle the URL encoding issue described in #5900.
✅ Verification successful
Based on the code analysis, I can now provide a final response:
The URL encoding fix is properly implemented and handles spaces correctly
The implementation is sound because:
- The
isURLEncodedfunction correctly detects if a URL is already encoded by comparing the decoded and original strings - The fix rebuilds the URL only when necessary (when not already encoded)
- The implementation uses the standard Go URL parsing and encoding functions
- The error handling is comprehensive, returning
ErrRebuildURLif URL parsing fails
The changes properly address issue #5900 by:
- Checking if URL needs encoding before proceeding
- Using
urlutil.Parseto properly handle URL encoding - Only rebuilding the URL when necessary to avoid double-encoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases and real-world usage of URL encoding in redirects
# Look for test cases covering URL encoding in redirects
echo "Searching for relevant test cases..."
rg -l "Location.*%20|Location.* " --type go
# Look for URL encoding related code
echo "Searching for URL encoding handling..."
ast-grep --pattern 'url.QueryEscape($$$)'
# Look for redirect handling code
echo "Searching for redirect handling..."
rg "CheckRedirect|Location:" --type go
Length of output: 1144
Script:
#!/bin/bash
# Let's examine the issue #5900 and related code more thoroughly
# First, let's look at the isURLEncoded function implementation
echo "Checking isURLEncoded implementation..."
rg -A 10 "func isURLEncoded" --type go
# Look for URL encoding related test cases
echo "Checking URL encoding test cases..."
fd ".*_test.go" --exec rg -l "isURLEncoded|checkMaxRedirects"
# Check the actual implementation in the test files
echo "Examining test implementations..."
fd ".*_test.go" --exec rg -A 10 "TestCheckMaxRedirects|TestIsURLEncoded"
# Look at the urlutil.Parse implementation
echo "Checking urlutil.Parse implementation..."
rg -A 10 "func Parse" pkg/protocols/common/urlutil/
Length of output: 1279
Script:
#!/bin/bash
# Let's check the urlutil package location and implementation
# Find urlutil package
echo "Finding urlutil package..."
fd -t f -e go urlutil
# Check the implementation of Parse function
echo "Looking for Parse function implementation..."
rg -A 10 "func Parse" --type go
# Check for any test cases related to URL encoding and spaces
echo "Looking for URL encoding and space-related tests..."
rg -A 5 "test.*url.*space|test.*space.*url" --type go -i
# Look for the actual issue #5900 to understand the context
echo "Looking for references to issue #5900..."
rg "#5900" --type go
Length of output: 5162
Script:
#!/bin/bash
# Let's find and examine the urlutil package and its Parse function
# Find all Go files containing urlutil
echo "Finding files with urlutil..."
fd -e go --exec rg -l "urlutil"
# Look for the actual urlutil package implementation
echo "Looking for urlutil package implementation..."
rg -A 5 "package urlutil" --type go
# Check for URL parsing related functions in the codebase
echo "Looking for URL parsing functions..."
rg -A 5 "func.*URL" --type go
# Look for any test cases that verify URL handling
echo "Looking for URL-related test cases..."
rg -A 5 "Test.*URL" --type go
Length of output: 29227
| // NOTE(dwisiswant0): rebuild request URL. See #5900. | ||
| if u := req.URL.String(); !isURLEncoded(u) { | ||
| parsed, err := urlutil.Parse(u) | ||
| if err != nil { | ||
| return fmt.Errorf("%w: %w", ErrRebuildURL, err) | ||
| } | ||
|
|
||
| req.URL = parsed.URL | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and URL validation
The URL rebuilding logic could be improved in several ways:
- Add context to the error message
- Validate the rebuilt URL
- Consider potential side effects of direct URL modification
Consider this enhanced implementation:
// NOTE(dwisiswant0): rebuild request URL. See #5900.
if u := req.URL.String(); !isURLEncoded(u) {
parsed, err := urlutil.Parse(u)
if err != nil {
- return fmt.Errorf("%w: %w", ErrRebuildURL, err)
+ return fmt.Errorf("failed to rebuild URL %q: %w", u, ErrRebuildURL)
}
+
+ // Validate the rebuilt URL
+ if parsed.URL.Host == "" || parsed.URL.Scheme == "" {
+ return fmt.Errorf("invalid URL after rebuild %q: missing host or scheme", parsed.URL)
+ }
req.URL = parsed.URL
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NOTE(dwisiswant0): rebuild request URL. See #5900. | |
| if u := req.URL.String(); !isURLEncoded(u) { | |
| parsed, err := urlutil.Parse(u) | |
| if err != nil { | |
| return fmt.Errorf("%w: %w", ErrRebuildURL, err) | |
| } | |
| req.URL = parsed.URL | |
| } | |
| // NOTE(dwisiswant0): rebuild request URL. See #5900. | |
| if u := req.URL.String(); !isURLEncoded(u) { | |
| parsed, err := urlutil.Parse(u) | |
| if err != nil { | |
| return fmt.Errorf("failed to rebuild URL %q: %w", u, ErrRebuildURL) | |
| } | |
| // Validate the rebuilt URL | |
| if parsed.URL.Host == "" || parsed.URL.Scheme == "" { | |
| return fmt.Errorf("invalid URL after rebuild %q: missing host or scheme", parsed.URL) | |
| } | |
| req.URL = parsed.URL | |
| } |
|
|
||
| // isURLEncoded is an helper function to check if the URL is already encoded | ||
| // | ||
| // NOTE(dwisiswant0): shall we move this under `projectdiscovery/utils/urlutil`? |
ehsandeep
deleted the
dwisiswant0/fix/httpclientpool/rebuild-malformed-location-URL
branch
Proposed changes
Fix #5900
Checklist
Summary by CodeRabbit
New Features
Bug Fixes