Skip to content

fix(api_core): improve rest path validation#17753

Open
daniel-sanche wants to merge 22 commits into
mainfrom
fix_rest
Open

fix(api_core): improve rest path validation#17753
daniel-sanche wants to merge 22 commits into
mainfrom
fix_rest

Conversation

@daniel-sanche

Copy link
Copy Markdown
Contributor

Context: b/532254612

@daniel-sanche
daniel-sanche requested a review from a team as a code owner July 17, 2026 00:06

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/google-api-core/google/api_core/path_template.py Outdated
Comment thread packages/google-api-core/tests/unit/test_path_template.py
@daniel-sanche daniel-sanche changed the title fix: improve rest path validation fix(api_core): improve rest path validation Jul 17, 2026

@chalmerlowe chalmerlowe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments and suggestions.

Comment thread packages/google-api-core/google/api_core/path_template.py Outdated
Comment thread packages/google-api-core/google/api_core/path_template.py Outdated
return

# Multi-segment templates ("**") must represent at least one valid, non-escaped segment.
if template_str == "**":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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
        return

What 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
        return

What 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 err

What 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?

@daniel-sanche daniel-sanche Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to capture some of this in the docstrings, but let me know if you have suggestions for how to word this better

Comment thread packages/google-api-core/google/api_core/path_template.py
Comment thread packages/google-api-core/tests/unit/test_path_template.py
Comment thread packages/google-api-core/tests/unit/test_path_template.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants