fix(api_core): improve rest path validation#17753
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces path traversal validation and percent-encoding for variables within path templates in google/api_core/path_template.py, along with corresponding unit tests. The feedback highlights a potential security bypass in _extract_and_validate_wildcards where path traversal sequences (like projects/..) could bypass validation if the value does not match the sub-template or if the template is single-segment. It is recommended to apply _validate_multi_segment_value to these edge cases and add tests to verify that these bypasses are successfully blocked.
chalmerlowe
left a comment
There was a problem hiding this comment.
Some comments and suggestions.
| return | ||
|
|
||
| # Multi-segment templates ("**") must represent at least one valid, non-escaped segment. | ||
| if template_str == "**": |
There was a problem hiding this comment.
#QUESTION
There is a subtle inconsistency in how we handle empty strings between the two if statements and the else statement. Is this intentional?
Snippet 1: Handling Single Segments (*)
This block checks if the template is for a single segment (like *).
# Single-segment templates (None or "*") cannot match exactly "." or ".."
# and cannot have multi-segment paths resolving to 0 segments.
if template_str is None or template_str == "*":
if val in (".", "..") or (val and not _validate_multi_segment_value(val)):
raise err
returnWhat happens if val is an empty string ("")?
The check val in (".", "..") is False.
Then it checks (val and not _validate_multi_segment_value(val)). Since val is "" (which is "falsy" in Python), the val and part evaluates to False.
The whole condition immediately evaluates to False, so it bypasses the error and returns successfully. An empty string is allowed here.
Snippet 2: Handling Bare Multi-Segments (**)
This block checks if the template is a bare double star (like **).
# Multi-segment templates ("**") must represent at least one valid, non-escaped segment.
if template_str == "**":
if not _validate_multi_segment_value(val):
raise err
returnWhat happens if val is an empty string ("")?
There is no val and guard here. It calls _validate_multi_segment_value("") directly.
If we trace _validate_multi_segment_value(""), it splits the empty string into segments, ends up with 0 leftover segments, and returns False.
So not _validate_multi_segment_value("") becomes not False which is True.
So this snippet raises an error. An empty string is rejected here.
Snippet 3: Falling back for Complex Templates
This block is at the very end of the function, handling cases where the value didn't match the expected complex pattern (e.g. something like projects/{project}/locations/{location}).
else:
# For values that don't match the pattern, ensure the value doesn't
# resolve to 0 segments (e.g. "projects/..").
if val and not _validate_multi_segment_value(val):
raise errWhat happens if val is an empty string ("")?
Just like Snippet 1, we have a val and guard here.
Since val is "" (falsy), the condition val and ... is False.
It bypasses the error. An empty string is allowed here.
Why this is inconsistent:
If a user passes an empty string "" as the value:
- If the template is
*(Snippet 1), it passes. - If the template is
**(Snippet 2), it fails. - If the template is a complex pattern (Snippet 3), it passes.
Is this difference in behavior intentional?
There was a problem hiding this comment.
This block is at the very end of the function, handling cases where the value didn't match the expected complex pattern (e.g. something like projects/{project}/locations/{location}).
The high-level answer to your question is that these functions are checking for security issues, not doing generic validation against the templates.The actual validation against the regex happens later in the stack, in the validate() step.
The reason we have two phases is that an API can provide multiple templates to check against (e.g. parent=(projects/** | cluster/**). So if we encounter an input that doesn't line up with a template in the validate() step, we just go through the list and find another template to match. We don't want to raise an exception unless we have to.
But the code is in this PR is checking for security issues. If we find any input that "breaks free" from any template, we want to reject it immediately by raising an exception, and don't keep looking for other inputs that could accept it
The * and ** segment have different security implications, that are detailed in the doc I sent you. Basically, if we encounter an empty string for *, we can keep going and let validate() decide whether to accept the input or continue to the next template. But if we encounter a ** segment that resolves to an empty path, we need to treat that as a potential security issue
There was a problem hiding this comment.
I tried to capture some of this in the docstrings, but let me know if you have suggestions for how to word this better
Co-authored-by: Chalmer Lowe <chalmerlowe@google.com>
Context: b/532254612