avoid double unescaping #4447
avoid double unescaping #4447
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses path traversal vulnerabilities (PRISMA-2022-0393 and PRISMA-2022-0394) by preventing double URL decoding of path and wildcard parameters. The fix ensures that URL-encoded parameters are only decoded once, preventing attackers from using double-encoded sequences to bypass security checks.
Key Changes:
- Modified URL unescaping logic to only decode values containing
%or+characters, preventing unnecessary processing - Refactored
countParamsandcountSectionsto usebytes.Countwith zero-copy string-to-bytes conversion - Added comprehensive security test cases for both path parameters and wildcard parameters with various encoding scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| tree.go | Updated unescape logic to prevent double-decoding; refactored count functions to remove safeUint16 helper and use bytes operations |
| tree_test.go | Added TestSecureParameterHandling for security regression testing; removed unused imports (fmt, regexp); removed 16 existing test functions reducing overall test coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tree_test.go
Outdated
|
|
||
| // Triple encoding - should only decode once | ||
| {"/info/user%25252Fprofile", false, "/info/:user", Params{Param{Key: "user", Value: "user%252Fprofile"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree_test.go
Outdated
|
|
||
| // Mixed encoding - should only decode once | ||
| {"/info/%2Fuser%252Fprofile", false, "/info/:user", Params{Param{Key: "user", Value: "/user%2Fprofile"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
|
|
||
| // No encoding - should pass through unchanged | ||
| {"/info/user", false, "/info/:user", Params{Param{Key: "user", Value: "user"}}}, | ||
| }, unescape) |
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree_test.go
Outdated
| checkRequests(t, tree, testRequests{ | ||
| // Normal case - single encoding works as expected | ||
| {"/files/path%2Fto%2Ffile.txt", false, "/files/*filepath", Params{Param{Key: "filepath", Value: "/path/to/file.txt"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree_test.go
Outdated
|
|
||
| // Double encoding - should only decode once | ||
| {"/files/path%252Fto%252Ffile.txt", false, "/files/*filepath", Params{Param{Key: "filepath", Value: "/path%2Fto%2Ffile.txt"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree_test.go
Outdated
|
|
||
| // Double encoding - should only decode once | ||
| {"/info/user%252Fprofile", false, "/info/:user", Params{Param{Key: "user", Value: "user%2Fprofile"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree_test.go
Outdated
|
|
||
| // Triple encoding - should only decode once | ||
| {"/files/path%25252Fto%25252Ffile.txt", false, "/files/*filepath", Params{Param{Key: "filepath", Value: "/path%252Fto%252Ffile.txt"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree_test.go
Outdated
|
|
||
| // Mixed encoding - should only decode once | ||
| {"/files/%2Fpath%252Fto%2Ffile.txt", false, "/files/*filepath", Params{Param{Key: "filepath", Value: "//path%2Fto/file.txt"}}}, | ||
|
|
There was a problem hiding this comment.
[nitpick] Trailing whitespace detected. Consider removing the trailing whitespace for code cleanliness.
tree.go
Outdated
| if unescape { | ||
| if v, err := url.QueryUnescape(path); err == nil { | ||
| val = v | ||
| // Only unescape if the value contains percent-encoded characters or plus signs | ||
| // This prevents double unescaping and potential path traversal | ||
| if strings.ContainsAny(path, "%+") { | ||
| if v, err := url.QueryUnescape(path); err == nil { | ||
| val = v | ||
| } | ||
| } |
There was a problem hiding this comment.
The same unescape logic is duplicated in two places (param and catchAll cases). Consider extracting this into a helper function to reduce code duplication and make maintenance easier.
Suggested refactoring:
func unescapeValue(val string, unescape bool) string {
if unescape && strings.ContainsAny(val, "%+") {
if v, err := url.QueryUnescape(val); err == nil {
return v
}
}
return val
}Then use it in both locations:
val := unescapeValue(path[:end], unescape)
tree.go
Outdated
| func countSections(path string) uint16 { | ||
| return safeUint16(strings.Count(path, "/")) | ||
| s := bytesconv.StringToBytes(path) | ||
| return uint16(bytes.Count(s, strSlash)) |
There was a problem hiding this comment.
Similar to countParams, this function is now missing overflow protection that was previously provided by safeUint16.
If a path has more than 65535 slashes, the cast to uint16 will silently overflow, potentially causing incorrect behavior in the routing logic.
| return uint16(bytes.Count(s, strSlash)) | |
| count := bytes.Count(s, strSlash) | |
| if count > 0xFFFF { | |
| count = 0xFFFF | |
| } | |
| return uint16(count) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4447 +/- ##
==========================================
- Coverage 99.21% 96.36% -2.85%
==========================================
Files 42 44 +2
Lines 3182 2975 -207
==========================================
- Hits 3157 2867 -290
- Misses 17 87 +70
- Partials 8 21 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@appleboy Can you please help me understand the reason of these pre-check failures? |
|
@appleboy can you take a look please |
|
@Agnes-George Please revert the changes to |
This reverts commit a61c9c6.
|
@appleboy can you take a look please |
3 participants
this is to resolve - #4443
Pull Request Checklist
Please ensure your pull request meets the following requirements:
masterbranch.docs/doc.md.Thank you for contributing!