Skip to content

fix(googleapis-common): validate and encode URI path parameters - #9162

Draft
danieljbruce wants to merge 1 commit into
mainfrom
fix/googleapis-common-path-traversal-7883256281869157031
Draft

fix(googleapis-common): validate and encode URI path parameters#9162
danieljbruce wants to merge 1 commit into
mainfrom
fix/googleapis-common-path-traversal-7883256281869157031

Conversation

@danieljbruce

Copy link
Copy Markdown
Contributor

Fixes path traversal and query injection security vulnerabilities in Apiary/Discovery clients by validating URI path parameters and encoding reserved path parameters in googleapis-common.


PR created automatically by Jules for task 7883256281869157031 started by @danieljbruce

Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@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 URI path validation and encoding helpers to prevent path traversal vulnerabilities in API requests, along with corresponding unit tests. The feedback suggests simplifying and optimizing the encodeWithSlashes and encodeWithoutSlashes helper functions by utilizing the built-in encodeURIComponent and splitting/joining on slashes, which improves both performance and readability.

Comment on lines +71 to +81
export function encodeWithSlashes(str: string): string {
return [...str]
.map(c => (c.match(/[-_.~0-9a-zA-Z]/) ? c : encodeURIComponent(c)))
.join('');
}

export function encodeWithoutSlashes(str: string): string {
return [...str]
.map(c => (c.match(/[-_.~0-9a-zA-Z/]/) ? c : encodeURIComponent(c)))
.join('');
}

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

The helper functions encodeWithSlashes and encodeWithoutSlashes can be significantly simplified and optimized:

  1. encodeWithSlashes is functionally identical to the built-in encodeURIComponent because all characters matched by the regex /[-_.~0-9a-zA-Z]/ are also preserved by encodeURIComponent, and any other characters (including !, *, ', (, )) are delegated to encodeURIComponent anyway. We can simply return encodeURIComponent(str) directly.
  2. encodeWithoutSlashes can be implemented much more efficiently by splitting the string by /, mapping each segment with encodeURIComponent, and joining them back with /. This avoids the overhead of spreading the string into a character array, executing a regex match on every single character, and joining them back.

This improves both readability and performance. Note that we pass encodeURIComponent directly to map instead of wrapping it in an arrow function (e.g., map(val => encodeURIComponent(val))) to avoid unnecessary closure allocations.

export function encodeWithSlashes(str: string): string {
  return encodeURIComponent(str);
}

export function encodeWithoutSlashes(str: string): string {
  return str.split('/').map(encodeURIComponent).join('/');
}
References
  1. Avoid wrapping methods in arrow functions for default cases to prevent unnecessary closure allocations and extra call stack frames.

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