Skip to content

fix(api_core): improve rest path validation#17753

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

fix(api_core): improve rest path validation#17753
daniel-sanche wants to merge 14 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.

Raises:
ValueError: If a wildcard within a structurally valid value violates path traversal rules.
"""
err = ValueError(

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.

#NIT
I do not know how often _extract_and_validate_wildcards gets called. IF it is regular occurrence we will be executing the f-string and ValueError creation every single time we call the function, even when we don't have an error condition.

If we call the function often AND errors are expected to be rare then a factory function (def create_err_msg()) to generate an err message only when we raise (a la: raise create_err_msg()) could be useful.

# 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)):

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.

#PREFERENCE/QUESTION:
Could we benefit from changing the order of operations?

If the REST API is a standard web service, 99.9% of incoming requests will be normal path segments (like "api", "v1", "users", "12345"). They will rarely be "." or "..".

This means our first check (the tuple check) will "miss" 99.9% of the time, forcing Python to execute the _validate_multi_segment_value anyway.

We can bypass the tuple lookup entirely by rewriting the logic. Since we know that _validate_multi_segment_value(val) already natively returns False for dots ("." OR ".."), we can drop the tuple check entirely to save a few cycles on every normal path

# Optimized for the 99.9% use-case (normal path segments)

if val and not _validate_multi_segment_value(val):

Why this is faster for REST routing:

Old Way (Normal Path): Checks tuple (Misses a lot), Checks if val (True), then Runs the function

New Way (Normal Path): Checks val (True) then Runs the Function.

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?

positional = match.group("positional")
template = match.group("template")

if positional == "*":

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.

#PREFERENCE: Above we define constants that can be applied to the return values for each of these conditions. I recommend we use them:

# Segment expressions used for validating paths against a template.
_SINGLE_SEGMENT_PATTERN = r"([^/]+)"
_MULTI_SEGMENT_PATTERN = r"(.+)"

("a/b/../..", False),
("a/b/../../..", False),
(".", False),
("..", False),

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.

#PREFERENCE:

I recommend some additional checks to cover some more edge cases:

Suggested change
("..", False),
("..", False),
("", False),
("/", False),
("//", False),
("a/../../b", False),
("../..", False),

),
("projects/{project=**}", "projects/(.+)", ("**",)),
("projects/{project=locations/*}", "projects/locations/([^/]+)", ("*",)),
("projects/*/locations/**", "projects/([^/]+)/locations/(.+)", ("*", "**")),

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: What if the template is just "projects/my-project" with no wildcards at all? It should return the literal escaped and an empty tuple of wildcard types. Do you think we should add the sanity check to ensure the regex parser doesn't choke on plain strings?

("projects/my-project", "projects/my-project", ()),

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