fix: validate path parameters and prevent traversal/injection in REST transcoder - #9151
fix: validate path parameters and prevent traversal/injection in REST transcoder#9151danieljbruce wants to merge 29 commits into
Conversation
…back
To prevent directory traversal and reserved-character injection attacks in fallback HTTP/REST transport, this commit implements the strict transcoding validation guidelines:
1. Rejects single-asterisk (*) matches resolving exactly to '.' or '..' with error: 'Invalid value {value} for {propertyName}'.
2. Rejects double-asterisk (**) matches containing segments exactly '.' or '..' with error: 'Value for {propertyName} must not contain segments that are exactly . or ..'.
3. Percent-encodes all variable segments except unreserved [-__.~/0-9a-zA-Z], ensuring characters like '!', ''', '(', ')', and '*' are encoded.
4. Filters out null and undefined values in buildQueryStringComponents array serialization to prevent TypeErrors.
5. Guards optional unmatched groups in applyPattern.
Includes a rigorous suite of 10 tests using the google-cloud-dialogflow-cx library to verify these mitigations in action.
Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…870442' of https://github.com/googleapis/google-cloud-node into jules-transcoding-path-traversal-vuln-fix-6401005213542870442
…ath" This reverts commit 94095ab.
This reverts commit c898db0.
…path" This reverts commit d33163b.
…rse the path"" This reverts commit ca4f260.
This reverts commit ea99463.
This reverts commit d875be0.
…oogleapis/google-cloud-node into transcoding-path-traversal-fix
There was a problem hiding this comment.
Code Review
This pull request introduces path traversal validation and strict RFC 3986 percent-encoding to the transcoding logic in gax, along with corresponding unit and integration tests. The reviewer feedback highlights a potential application crash (URIError: URI malformed) when processing non-BMP characters (such as emojis) due to splitting strings by UTF-16 code units with str.split('') instead of using the spread operator [...str]. Additionally, there is an unused import of the path module in the new test file that should be removed.
| return str | ||
| .split('') | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z]/) ? c : encodeURIComponent(c))) | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z]/) ? c : strictEncodeURIComponent(c))) | ||
| .join(''); |
There was a problem hiding this comment.
Using str.split('') splits the string by UTF-16 code units. If the input string contains any characters outside the Basic Multilingual Plane (BMP) (such as emojis or certain Han characters), split('') will break them into unpaired surrogates. When these unpaired surrogates are passed to encodeURIComponent (inside strictEncodeURIComponent), it will throw a URIError: URI malformed and crash the application.
Using the spread operator [...str] correctly iterates over Unicode code points, preserving surrogate pairs as single characters, which avoids the URIError and correctly percent-encodes them.
return [...str]
.map(c => (c.match(/[-_.~0-9a-zA-Z]/) ? c : strictEncodeURIComponent(c)))
.join('');| return str | ||
| .split('') | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z/]/) ? c : encodeURIComponent(c))) | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z/]/) ? c : strictEncodeURIComponent(c))) | ||
| .join(''); |
There was a problem hiding this comment.
Using str.split('') splits the string by UTF-16 code units. If the input string contains any characters outside the Basic Multilingual Plane (BMP) (such as emojis or certain Han characters), split('') will break them into unpaired surrogates. When these unpaired surrogates are passed to encodeURIComponent (inside strictEncodeURIComponent), it will throw a URIError: URI malformed and crash the application.
Using the spread operator [...str] correctly iterates over Unicode code points, preserving surrogate pairs as single characters, which avoids the URIError and correctly percent-encodes them.
return [...str]
.map(c => (c.match(/[-_.~0-9a-zA-Z/]/) ? c : strictEncodeURIComponent(c)))
.join('');| import * as assert from 'assert'; | ||
| import { describe, it } from 'mocha'; | ||
| import { v3 } from '../src'; | ||
| import * as path from 'path'; |
|
|
||
| // Validates a single path segment matched by a single wildcard (*). | ||
| // Checks that the segment is not exactly '.' or '..' (directory traversal indicators). | ||
| function validateSingleSegment(propertyName: string, value: string): void { |
There was a problem hiding this comment.
How about validateUriPathSegment?
| // Splitting by slash, it checks that no individual segment is exactly '.' or '..'. | ||
| // This segment-by-segment check prevents directory traversal while allowing | ||
| // legitimate resource names containing dots (e.g., domain-scoped project IDs). | ||
| function validateMultiSegment(propertyName: string, value: string): void { |
|
|
||
| // Test 1: Single Asterisk Dot Validation on client call | ||
| it.skip('1. should throw an error for single-asterisk segment traversal using exactly "." as session ID', async () => { | ||
| // TODO: Re-enable this test when the gax version with the new encoding is released. |
There was a problem hiding this comment.
can you file an issue for re-enabling the tests?
| it('applyPattern should percent-encode query injection attempt on double-asterisk without throwing traversal error', () => { | ||
| const res = applyPattern( | ||
| 'projects/*/locations/*/agents/*/sessions/**', | ||
| 'projects/p/locations/l/agents/a/sessions/..?$httpMethod=DELETE#', |
There was a problem hiding this comment.
please use a less instructive parameter like "?$foo=BAR#"
| .split('') | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z]/) ? c : encodeURIComponent(c))) | ||
| return [...str] | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z]/) ? c : strictEncodeURIComponent(c))) |
There was a problem hiding this comment.
is the character iteration necessary anymore or can you just call strictEncodeURIComponent? I think it preserves the same chars now!
| .split('') | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z/]/) ? c : encodeURIComponent(c))) | ||
| return [...str] | ||
| .map(c => (c.match(/[-_.~0-9a-zA-Z/]/) ? c : strictEncodeURIComponent(c))) |
There was a problem hiding this comment.
to avoid character iteration here, you'll need to do something like strictEncodeURIComponent(c, /* preserveSlashes: boolean = false = */ true)
| ); | ||
| } | ||
|
|
||
| export function encodeWithSlashes(str: string): string { |
There was a problem hiding this comment.
while you're here, can you please document these exported functions?
It took me a while to understand the point of With- and WithoutSlashes.
Description
Protect GAPIC REST clients from path traversal and parameter injection exploits with two mechanisms:
Impact
Improves the security of our clients by preventing exploits.
Testing
It should be noted that some new tests in packages/google-cloud-dialogflow-cx/test/transcoding_validation.ts were skipped because they fail in the CI pipeline. This is because they pass with these changes, but these changes are to gax and google-cloud-dialogflow-cx does not get this version of gax yet until it is published.
To test locally:
cd packages/google-cloud-dialogflow-cx
pnpm link ../../core/packages/gax
npm run compile && npx mocha build/test/transcoding_validation.js
Transcoding tests are also added which provide support for ** wildcard which the dialogflow-cx tests can't cover.
Additional Info
I identified a refactor in https://github.com/googleapis/google-cloud-node/pull/9150/changes which should be worth considering to improve readability.
It should also be noted that googleapis/ruby-core-libraries#67 was done for Ruby which has differences that may vary from language to language.