Skip to content

fix(api-common): validate path template variables against dot segments - #14641

Open
blakeli0 wants to merge 1 commit into
googleapis:mainfrom
blakeli0:fix/path-template-dot-segments
Open

blakeli0 wants to merge 1 commit into
googleapis:mainfrom
blakeli0:fix/path-template-dot-segments

Conversation

@blakeli0

Copy link
Copy Markdown
Contributor

Validates URI path template variable segments during PathTemplate.instantiate to prevent unintended dot-segment ("." and "..") path traversal and ensures strict percent-encoding of non-unreserved URI characters:

  • Reject "." and ".." segments in single/multi-segment wildcard (*) bindings (ValidationException: "Invalid value %s for %s.").
  • Reject any "/"-delimited segment equal to "." or ".." in path wildcard (**) bindings (ValidationException: "Value for %s must not contain segments that are exactly . or ..").
  • Encode path variable segments using PercentEscaper("-_.~", false) so all characters outside [-_.~0-9a-zA-Z] are percent-encoded (e.g., spaces as %20, ? as %3F, # as %23, % as %25).

@blakeli0
blakeli0 requested a review from a team as a code owner October 12, 2026 18:11

@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 replaces URLEncoder with Guava's PercentEscaper for URL encoding and introduces validation to reject path segments that are exactly '.' or '..' during template instantiation. It also adds comprehensive unit tests to verify these validation rules. The review feedback suggests optimizing the hasPathWildcard helper method by breaking out of the loop early once a path wildcard is found.

Comment on lines +1216 to +1230
private static boolean hasPathWildcard(ListIterator<Segment> iterator) {
int start = iterator.nextIndex();
boolean found = false;
while (iterator.hasNext()) {
Segment seg = iterator.next();
if (seg.kind() == SegmentKind.END_BINDING) {
break;
}
if (seg.kind() == SegmentKind.PATH_WILDCARD) {
found = true;
}
}
restore(iterator, start);
return found;
}

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.

medium

Once a PATH_WILDCARD segment is found, we can break out of the loop early. Since restore(iterator, start) is called at the end of the method, the iterator's position will be correctly restored regardless of how far we advanced it. This avoids unnecessary iterations over the remaining segments of the binding.

Suggested change
private static boolean hasPathWildcard(ListIterator<Segment> iterator) {
int start = iterator.nextIndex();
boolean found = false;
while (iterator.hasNext()) {
Segment seg = iterator.next();
if (seg.kind() == SegmentKind.END_BINDING) {
break;
}
if (seg.kind() == SegmentKind.PATH_WILDCARD) {
found = true;
}
}
restore(iterator, start);
return found;
}
private static boolean hasPathWildcard(ListIterator<Segment> iterator) {
int start = iterator.nextIndex();
boolean found = false;
while (iterator.hasNext()) {
Segment seg = iterator.next();
if (seg.kind() == SegmentKind.END_BINDING) {
break;
}
if (seg.kind() == SegmentKind.PATH_WILDCARD) {
found = true;
break;
}
}
restore(iterator, start);
return found;
}

This branch has not been deployed

No deployments
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.

1 participant