fix: Validate path parameters against path traversal and parameter injection - #3236
fix: Validate path parameters against path traversal and parameter injection#3236quartzmo wants to merge 5 commits into
Conversation
…jection Validates discovery-based path parameters in the request builder before template expansion. Rejects '?' and '#' in reserved path parameters (+ and #) as a primary control to prevent parameter injection (since reserved expansions bypass URL-escaping). Rejects '.' and '..' segments in all path parameters to prevent path traversal.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation for path parameters in RequestBuilder to prevent path traversal exploits and query/fragment injections, along with corresponding unit tests. A critical security issue was identified in the path traversal validation logic: splitting the parameter value by '/' before unescaping allows a bypass (e.g., '..%2f..' is not split, but unescapes to '../..'). It is recommended to unescape the entire parameter value first before splitting it into segments.
efevans
left a comment
efevans
left a comment
There was a problem hiding this comment.
Same as the in the gapic library, I'd prefer if we could ignore my initial request, and move back to string.split in this case to keep the implementation simpler and somewhat at parity with the gapic. I think it's harder to justify in this case since this is all new code whereas the gapic was changing a method that was already splitting, but I think it should be okay given the KTLO-stance towards discovery libs.
| } | ||
| // Unescape the entire value first to prevent bypasses using URL-encoded slashes (e.g. %2f). | ||
| string unescapedVal = Uri.UnescapeDataString(val); | ||
| bool isReserved = op == "+" || op == "#"; |
There was a problem hiding this comment.
In the ruby implementation implementation here there's a check on non-reserved parameters for slashes, do we need that here?
2 participants
Validates discovery-based path parameters in the request builder before template expansion.
Rejects '?' and '#' in reserved path parameters (+ and #) as a primary control to prevent parameter injection (since reserved expansions bypass URL-escaping).
Rejects '.' and '..' segments in all path parameters to prevent path traversal. Added unit tests for validation.
b/529889305