-
Notifications
You must be signed in to change notification settings - Fork 700
fix: validate path parameters and prevent traversal/injection in REST transcoder #9151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
23d2854
0e1f219
ebd4779
9759c50
70c928d
ae95e73
08d13b0
94095ab
d33163b
c898db0
ea99463
ca4f260
af94646
89c693f
3e46c57
fcc25cc
94131e5
e600bb3
db33841
2336449
38506c6
d875be0
5c6ad6d
23097b9
a967001
fe620b0
fe2c259
be19de3
1746c32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,6 +118,30 @@ export function deleteField(request: JSONObject, field: string): void { | |
| delete request[part]; | ||
| } | ||
|
|
||
| // 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 { | ||
| if (value === '.' || value === '..') { | ||
| throw new Error(`Invalid value ${value} for ${propertyName}`); | ||
| } | ||
| } | ||
|
|
||
| // Validates a multi-segment path matched by a double wildcard (**). | ||
| // 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and |
||
| if (value) { | ||
| // Split by slash and check for exact segment matches of '.' or '..' rather | ||
| // than using a simple string.includes('.') check. This avoids rejecting | ||
| // valid domain-scoped resource segments (e.g. projects/example.com:project-id). | ||
| const segments = value.split('/'); | ||
| if (segments.some(segment => segment === '.' || segment === '..')) { | ||
| throw new Error(`Value for ${propertyName} must not contain segments that are exactly . or ..`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export function buildQueryStringComponents( | ||
| request: JSONObject, | ||
| prefix = '', | ||
|
|
@@ -147,17 +171,26 @@ export function buildQueryStringComponents( | |
| return resultList; | ||
| } | ||
|
|
||
| // Strictly percent-encodes a character to comply with RFC 3986. | ||
| // This is necessary because encodeURIComponent natively encodes URL-unsafe | ||
| // characters like ?, #, $, &, +, etc., but preserves !, ', (, ), and *. | ||
| // To ensure strict compliance, we manually encode those preserved characters. | ||
| function strictEncodeURIComponent(str: string): string { | ||
| return encodeURIComponent(str).replace( | ||
| /[!'()*]/g, // Characters preserved by encodeURIComponent | ||
| character => '%' + character.charCodeAt(0).toString(16).toUpperCase() | ||
| ); | ||
| } | ||
|
|
||
| export function encodeWithSlashes(str: string): string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while you're here, can you please document these exported functions? It took me a while to understand the point of With- and WithoutSlashes. |
||
| return str | ||
| .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))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the character iteration necessary anymore or can you just call strictEncodeURIComponent? I think it preserves the same chars now! |
||
| .join(''); | ||
| } | ||
|
|
||
| export function encodeWithoutSlashes(str: string): string { | ||
| return str | ||
| .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))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to avoid character iteration here, you'll need to do something like |
||
| .join(''); | ||
| } | ||
|
|
||
|
|
@@ -168,8 +201,10 @@ function escapeRegExp(str: string) { | |
| export function applyPattern( | ||
| pattern: string, | ||
| fieldValue: string, | ||
| propertyName = 'resource', // Used to provide precise error messages when path validation fails | ||
| ): string | undefined { | ||
| if (!pattern || pattern === '*') { | ||
| validateSingleSegment(propertyName, fieldValue); | ||
| return encodeWithSlashes(fieldValue); | ||
| } | ||
|
|
||
|
|
@@ -186,10 +221,27 @@ export function applyPattern( | |
| '$', | ||
| ); | ||
|
|
||
| if (!fieldValue.match(regex)) { | ||
| const match = fieldValue.match(regex); | ||
| if (!match) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Identify the segments and wildcards in pattern to perform validation in order of appearance | ||
| const wildcards: string[] = pattern.match(/\*\*|\*/g) || []; | ||
|
|
||
| // Check the captured group values | ||
| for (let i = 1; i < match.length; i++) { | ||
| const groupVal = match[i]; | ||
| if (groupVal !== undefined && groupVal !== null) { | ||
| const wildcardType = wildcards[i - 1]; | ||
| if (wildcardType === '*') { | ||
| validateSingleSegment(propertyName, groupVal); | ||
| } else if (wildcardType === '**') { | ||
| validateMultiSegment(propertyName, groupVal); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return encodeWithoutSlashes(fieldValue); | ||
| } | ||
|
|
||
|
|
@@ -224,6 +276,7 @@ export function match( | |
| const appliedPattern = applyPattern( | ||
| pattern, | ||
| fieldValue === null ? 'null' : fieldValue!.toString(), | ||
| camelCasedField, | ||
| ); | ||
| if (appliedPattern === undefined) { | ||
| return undefined; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -370,6 +370,24 @@ describe('gRPC to HTTP transcoding', () => { | |
| ); | ||
| }); | ||
|
|
||
| it('should correctly handle Unicode surrogate pairs in encodeWithSlashes', () => { | ||
| // Emojis (like 😊) are surrogate pairs. | ||
| // They should be encoded successfully instead of throwing a URIError. | ||
| assert.strictEqual(encodeWithSlashes('😊'), '%F0%9F%98%8A'); | ||
| }); | ||
|
|
||
| it('should preserve unreserved characters while strictly percent-encoding all other characters in encodeWithSlashes', () => { | ||
| // Standard RFC unreserved characters: [-_.~0-9a-zA-Z] | ||
| const unreserved = 'abc-123_.~'; | ||
| assert.strictEqual(encodeWithSlashes(unreserved), unreserved); | ||
|
|
||
| // Reserved and special characters: should be percent encoded, including !\'()* | ||
| const specialChars = "!\'()*"; | ||
| const encoded = encodeWithSlashes(specialChars); | ||
| // ! -> %21, ' -> %27, ( -> %28, ) -> %29, * -> %2A | ||
| assert.strictEqual(encoded, '%21%27%28%29%2A'); | ||
| }); | ||
|
|
||
| it('encodeWithoutSlashes', () => { | ||
| assert.strictEqual(encodeWithoutSlashes('abcd'), 'abcd'); | ||
| assert.strictEqual( | ||
|
|
@@ -384,6 +402,12 @@ describe('gRPC to HTTP transcoding', () => { | |
| ); | ||
| }); | ||
|
|
||
| it('should correctly handle Unicode surrogate pairs in encodeWithoutSlashes', () => { | ||
| // Emojis (like 😊) are surrogate pairs. | ||
| // They should be encoded successfully instead of throwing a URIError. | ||
| assert.strictEqual(encodeWithoutSlashes('😊'), '%F0%9F%98%8A'); | ||
| }); | ||
|
|
||
| it('applyPattern', () => { | ||
| assert.strictEqual(applyPattern('*', 'test'), 'test'); | ||
| assert.strictEqual(applyPattern('test', 'test'), 'test'); | ||
|
|
@@ -411,6 +435,40 @@ describe('gRPC to HTTP transcoding', () => { | |
| ); | ||
| }); | ||
|
|
||
| it('applyPattern should throw an error for double-asterisk segment traversal containing segments that are exactly ".."', () => { | ||
| assert.throws(() => { | ||
| applyPattern( | ||
| 'projects/*/locations/*/agents/*/sessions/**', | ||
| 'projects/p/locations/l/agents/a/sessions/agents/../subagent', | ||
| 'session' | ||
| ); | ||
| }, /Value for session must not contain segments that are exactly \. or \.\./); | ||
| }); | ||
|
|
||
| it('applyPattern should throw an error for double-asterisk segment traversal containing segments that are exactly "."', () => { | ||
| assert.throws(() => { | ||
| applyPattern( | ||
| 'projects/*/locations/*/agents/*/sessions/**', | ||
| 'projects/p/locations/l/agents/a/sessions/agents/./subagent', | ||
| 'session' | ||
| ); | ||
| }, /Value for session must not contain segments that are exactly \. or \.\./); | ||
| }); | ||
|
|
||
| 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#', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use a less instructive parameter like "?$foo=BAR#" |
||
| 'session' | ||
| ); | ||
| assert.strictEqual(res, 'projects/p/locations/l/agents/a/sessions/..%3F%24httpMethod%3DDELETE%23'); | ||
| }); | ||
|
|
||
| it('applyPattern should handle optional unmatched groups gracefully without throwing TypeErrors', () => { | ||
| const res = applyPattern('projects/*', 'projects/p', 'session'); | ||
| assert.strictEqual(res, 'projects/p'); | ||
| }); | ||
|
|
||
| it('flattenObject', () => { | ||
| assert.deepStrictEqual(flattenObject({}), {}); | ||
| assert.deepStrictEqual(flattenObject({field: 'value'}), {field: 'value'}); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| // Copyright 2026 Google LLC | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // https://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| import * as assert from 'assert'; | ||
| import { describe, it } from 'mocha'; | ||
| import { v3 } from '../src'; | ||
|
|
||
| const sinon = require('sinon'); | ||
|
|
||
| describe('Dialogflow CX Fallback Transcoding and Path Traversal Prevention', () => { | ||
| let client: v3.SessionsClient; | ||
| let fetchStub: any; | ||
|
|
||
| beforeEach(() => { | ||
| client = new v3.SessionsClient({ | ||
| fallback: true, | ||
| credentials: { client_email: 'bogus@example.com', private_key: 'bogus' }, | ||
| projectId: 'bogus', | ||
| }); | ||
| fetchStub = sinon.stub().resolves({ | ||
| ok: true, | ||
| status: 200, | ||
| arrayBuffer: () => Promise.resolve(Buffer.from('{}')), | ||
| }); | ||
| client.auth.fetch = fetchStub; | ||
| }); | ||
|
|
||
| // 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you file an issue for re-enabling the tests?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. Already did here. |
||
| await client.initialize(); | ||
| await assert.rejects( | ||
| client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/.', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }), | ||
| /Invalid value \. for session/ | ||
| ); | ||
| }); | ||
|
|
||
| // Test 2: Single Asterisk Dot-Dot Validation on client call | ||
| it.skip('2. 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. | ||
| await client.initialize(); | ||
| await assert.rejects( | ||
| client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/..', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }), | ||
| /Invalid value \.\. for session/ | ||
| ); | ||
| }); | ||
|
|
||
|
|
||
|
|
||
| // Test 5: Standard Valid Path fallback REST call | ||
| it('5. should pass transcoding validation with a valid session path and construct the correct REST URL', async () => { | ||
| await client.initialize(); | ||
| await client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/valid-session-id', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }); | ||
| assert.strictEqual(fetchStub.callCount, 1); | ||
| const requestUrl = fetchStub.firstCall.args[0]; | ||
| assert.ok(requestUrl.includes('/v3/projects/p/locations/l/agents/a/sessions/valid-session-id:detectIntent')); | ||
| }); | ||
|
|
||
| // Test 6: Query Parameter Injection Prevention via percent-encoding | ||
| it('6. should protect against query parameter injection by percent-encoding "?" and "$" in the session ID', async () => { | ||
| await client.initialize(); | ||
| await client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/my-session?$httpMethod=DELETE#', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }); | ||
| assert.strictEqual(fetchStub.callCount, 1); | ||
| const requestUrl = fetchStub.firstCall.args[0]; | ||
| // "?" -> %3F, "$" -> %24, "=" -> %3D, "#" -> %23 | ||
| assert.ok(requestUrl.includes('my-session%3F%24httpMethod%3DDELETE%23')); | ||
| }); | ||
|
|
||
| // Test 7: Combined Path Traversal and Query Parameter Injection | ||
| it('7. should protect against path traversal and query injection by percent-encoding combined patterns', async () => { | ||
| // This request is permitted because the template uses * instead of ** | ||
| // * is supposed to match against exactly . or .. | ||
| // This is okay because we still percent encode the ? parameter. | ||
| // example: POST https://<location>-dialogflow.googleapis.com/v3/{session=projects/*/locations/*/agents/*/sessions/*}:detectIntent | ||
| await client.initialize(); | ||
| await client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/..?$httpMethod=DELETE#', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }); | ||
| assert.strictEqual(fetchStub.callCount, 1); | ||
| const requestUrl = fetchStub.firstCall.args[0]; | ||
| assert.ok(requestUrl.includes('/v3/projects/p/locations/l/agents/a/sessions/..%3F%24httpMethod%3DDELETE%23:detectIntent')); | ||
| }); | ||
|
|
||
| // Test 8: Combined Path Traversal (.) and Query Parameter Injection | ||
| it('8. should protect against path traversal and query injection by percent-encoding combined patterns using dot', async () => { | ||
| await client.initialize(); | ||
| await client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/.?$httpMethod=DELETE#', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }); | ||
| assert.strictEqual(fetchStub.callCount, 1); | ||
| const requestUrl = fetchStub.firstCall.args[0]; | ||
| assert.ok(requestUrl.includes('/v3/projects/p/locations/l/agents/a/sessions/.%3F%24httpMethod%3DDELETE%23:detectIntent')); | ||
| }); | ||
|
|
||
| // Test 9: Percent-encoding all other characters | ||
| it.skip('9. should percent-encode all other characters except unreserved ones', async () => { | ||
| // TODO: Re-enable this test when the gax version with the new encoding is released. | ||
| await client.initialize(); | ||
| await client.detectIntent({ | ||
| session: 'projects/p/locations/l/agents/a/sessions/ !@$&\'()*+,;=:%', | ||
| queryInput: { text: { text: 'hello' }, languageCode: 'en' }, | ||
| }); | ||
| assert.strictEqual(fetchStub.callCount, 1); | ||
| const requestUrl = fetchStub.firstCall.args[0]; | ||
| assert.ok(requestUrl.includes('/v3/projects/p/locations/l/agents/a/sessions/%20%21%40%24%26%27%28%29%2A%2B%2C%3B%3D%3A%25:detectIntent')); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
validateUriPathSegment?