diff --git a/core/common/src/service-object.ts b/core/common/src/service-object.ts index 798193813f76..7874d819bc13 100644 --- a/core/common/src/service-object.ts +++ b/core/common/src/service-object.ts @@ -563,17 +563,25 @@ class ServiceObject extends EventEmitter { const uriComponents = [this.baseUrl, this.id || '', reqOpts.uri]; if (isAbsoluteUrl) { - uriComponents.splice(0, uriComponents.indexOf(reqOpts.uri)); + const url = new URL(reqOpts.uri); + const encodedPath = util.encodeURIPath(url.pathname); + url.pathname = encodedPath; + let res = url.toString(); + if (!reqOpts.uri.endsWith('/') && res.endsWith('/')) { + res = res.slice(0, -1); + } + reqOpts.uri = res; + } else { + reqOpts.uri = uriComponents + .filter(x => x!.trim()) // Limit to non-empty strings. + .map(uriComponent => { + const trimSlashesRegex = /^\/*|\/*$/g; + const trimmed = uriComponent!.replace(trimSlashesRegex, ''); + return util.encodeURIPath(trimmed); + }) + .join('/'); } - reqOpts.uri = uriComponents - .filter(x => x!.trim()) // Limit to non-empty strings. - .map(uriComponent => { - const trimSlashesRegex = /^\/*|\/*$/g; - return uriComponent!.replace(trimSlashesRegex, ''); - }) - .join('/'); - const childInterceptors = (arrify as unknown as (arg1: any) => [])( reqOpts.interceptors_!, ); diff --git a/core/common/src/service.ts b/core/common/src/service.ts index d0a179242467..a07a18a33376 100644 --- a/core/common/src/service.ts +++ b/core/common/src/service.ts @@ -212,20 +212,28 @@ export class Service { uriComponents.push(reqOpts.uri); if (isAbsoluteUrl) { - uriComponents.splice(0, uriComponents.indexOf(reqOpts.uri)); + const url = new URL(reqOpts.uri); + const encodedPath = util.encodeURIPath(url.pathname); + url.pathname = encodedPath; + let res = url.toString(); + if (!reqOpts.uri.endsWith('/') && res.endsWith('/')) { + res = res.slice(0, -1); + } + reqOpts.uri = res; + } else { + reqOpts.uri = uriComponents + .map(uriComponent => { + const trimSlashesRegex = /^\/*|\/*$/g; + const trimmed = uriComponent.replace(trimSlashesRegex, ''); + return util.encodeURIPath(trimmed); + }) + .join('/') + // Some URIs have colon separators. + // Bad: https://.../projects/:list + // Good: https://.../projects:list + .replace(/\/:/g, ':'); } - reqOpts.uri = uriComponents - .map(uriComponent => { - const trimSlashesRegex = /^\/*|\/*$/g; - return uriComponent.replace(trimSlashesRegex, ''); - }) - .join('/') - // Some URIs have colon separators. - // Bad: https://.../projects/:list - // Good: https://.../projects:list - .replace(/\/:/g, ':'); - const requestInterceptors = this.getRequestInterceptors(); (arrify as unknown as (arg1: any) => any[])(reqOpts.interceptors_!).forEach( diff --git a/core/common/src/util.ts b/core/common/src/util.ts index 322e6cfee37a..c36d0858e458 100644 --- a/core/common/src/util.ts +++ b/core/common/src/util.ts @@ -913,6 +913,10 @@ export class Util { return dup; } + encodeWithSlashes = encodeWithSlashes; + encodeWithoutSlashes = encodeWithoutSlashes; + encodeURIPath = encodeURIPath; + /** * Decorate the options about to be made in a request. * @@ -1024,5 +1028,50 @@ class ProgressStream extends Transform { } } +export function encodeWithSlashes(str: string, propertyName = 'resource ID field'): string { + const segments = str.split('/'); + for (const segment of segments) { + if (segment === '.' || segment === '..') { + throw new Error( + `Value for ${propertyName} must not contain segments that are exactly . or .. .`, + ); + } + } + return encodeURIComponent(str) + .replace(/%2F/gi, '/') + .replace(/[!'()*]/g, c => '%' + c.charCodeAt(0).toString(16).toUpperCase()); +} + +export function encodeWithoutSlashes(str: string, propertyName = 'resource ID field'): string { + if (str === '.' || str === '..') { + throw new Error(`Invalid value ${str} for ${propertyName}.`); + } + return encodeURIComponent(str) + .replace(/[!'()*]/g, c => '%' + c.charCodeAt(0).toString(16).toUpperCase()); +} + +export function encodeURIPath(uri: string): string { + const parts = uri.split('/'); + return parts + .map(part => { + if (part === '') { + return ''; + } + if (part.includes(':')) { + const subparts = part.split(':'); + return subparts + .map(subpart => { + if (subpart === '') { + return ''; + } + return encodeWithoutSlashes(subpart, 'path segment'); + }) + .join(':'); + } + return encodeWithoutSlashes(part, 'path segment'); + }) + .join('/'); +} + const util = new Util(); export {util}; diff --git a/core/common/test/util.ts b/core/common/test/util.ts index 6c018afd4d3d..3348b13448a9 100644 --- a/core/common/test/util.ts +++ b/core/common/test/util.ts @@ -1903,6 +1903,58 @@ describe('common/util', () => { }); }); + describe('encodeWithSlashes & encodeWithoutSlashes', () => { + it('encodeWithSlashes should allow valid path segments and encode special characters', () => { + assert.strictEqual( + util.encodeWithSlashes('foo/bar-123_~.baz'), + 'foo/bar-123_~.baz', + ); + assert.strictEqual( + util.encodeWithSlashes('foo/bar baz'), + 'foo/bar%20baz', + ); + }); + + it('encodeWithSlashes should throw if any segment is . or ..', () => { + assert.throws(() => { + util.encodeWithSlashes('foo/./bar', 'testField'); + }, /Value for testField must not contain segments that are exactly \. or \.\. \./); + + assert.throws(() => { + util.encodeWithSlashes('foo/../bar', 'testField'); + }, /Value for testField must not contain segments that are exactly \. or \.\. \./); + }); + + it('encodeWithoutSlashes should allow valid characters and encode slashes and special characters', () => { + assert.strictEqual( + util.encodeWithoutSlashes('foo-123_~.baz'), + 'foo-123_~.baz', + ); + assert.strictEqual( + util.encodeWithoutSlashes('foo/bar'), + 'foo%2Fbar', + ); + assert.strictEqual( + util.encodeWithoutSlashes('photo_😀.png'), + 'photo_%F0%9F%98%80.png', + ); + assert.strictEqual( + util.encodeWithoutSlashes('test*file!'), + 'test%2Afile%21', + ); + }); + + it('encodeWithoutSlashes should throw if the value is . or ..', () => { + assert.throws(() => { + util.encodeWithoutSlashes('.', 'testField'); + }, /Invalid value \. for testField\./); + + assert.throws(() => { + util.encodeWithoutSlashes('..', 'testField'); + }, /Invalid value \.\. for testField\./); + }); + }); + describe('maybeOptionsOrCallback', () => { it('should allow passing just a callback', () => { const optionsOrCallback = () => {}; diff --git a/handwritten/storage/src/nodejs-common/service-object.ts b/handwritten/storage/src/nodejs-common/service-object.ts index 842ada149abc..fc6de84a415b 100644 --- a/handwritten/storage/src/nodejs-common/service-object.ts +++ b/handwritten/storage/src/nodejs-common/service-object.ts @@ -562,17 +562,25 @@ class ServiceObject extends EventEmitter { const uriComponents = [this.baseUrl, this.id || '', reqOpts.uri]; if (isAbsoluteUrl) { - uriComponents.splice(0, uriComponents.indexOf(reqOpts.uri)); + const url = new URL(reqOpts.uri); + const encodedPath = util.encodeURIPath(url.pathname); + url.pathname = encodedPath; + let res = url.toString(); + if (!reqOpts.uri.endsWith('/') && res.endsWith('/')) { + res = res.slice(0, -1); + } + reqOpts.uri = res; + } else { + reqOpts.uri = uriComponents + .filter(x => x!.trim()) // Limit to non-empty strings. + .map(uriComponent => { + const trimSlashesRegex = /^\/*|\/*$/g; + const trimmed = uriComponent!.replace(trimSlashesRegex, ''); + return util.encodeURIPath(trimmed); + }) + .join('/'); } - reqOpts.uri = uriComponents - .filter(x => x!.trim()) // Limit to non-empty strings. - .map(uriComponent => { - const trimSlashesRegex = /^\/*|\/*$/g; - return uriComponent!.replace(trimSlashesRegex, ''); - }) - .join('/'); - const childInterceptors = Array.isArray(reqOpts.interceptors_) ? reqOpts.interceptors_ : []; diff --git a/handwritten/storage/src/nodejs-common/service.ts b/handwritten/storage/src/nodejs-common/service.ts index 9173a38f73d7..e12afee5ed56 100644 --- a/handwritten/storage/src/nodejs-common/service.ts +++ b/handwritten/storage/src/nodejs-common/service.ts @@ -234,20 +234,28 @@ export class Service { uriComponents.push(reqOpts.uri); if (isAbsoluteUrl) { - uriComponents.splice(0, uriComponents.indexOf(reqOpts.uri)); + const url = new URL(reqOpts.uri); + const encodedPath = util.encodeURIPath(url.pathname); + url.pathname = encodedPath; + let res = url.toString(); + if (!reqOpts.uri.endsWith('/') && res.endsWith('/')) { + res = res.slice(0, -1); + } + reqOpts.uri = res; + } else { + reqOpts.uri = uriComponents + .map(uriComponent => { + const trimSlashesRegex = /^\/*|\/*$/g; + const trimmed = uriComponent.replace(trimSlashesRegex, ''); + return util.encodeURIPath(trimmed); + }) + .join('/') + // Some URIs have colon separators. + // Bad: https://.../projects/:list + // Good: https://.../projects:list + .replace(/\/:/g, ':'); } - reqOpts.uri = uriComponents - .map(uriComponent => { - const trimSlashesRegex = /^\/*|\/*$/g; - return uriComponent.replace(trimSlashesRegex, ''); - }) - .join('/') - // Some URIs have colon separators. - // Bad: https://.../projects/:list - // Good: https://.../projects:list - .replace(/\/:/g, ':'); - const requestInterceptors = this.getRequestInterceptors(); const interceptorArray = Array.isArray(reqOpts.interceptors_) ? reqOpts.interceptors_ diff --git a/handwritten/storage/src/nodejs-common/util.ts b/handwritten/storage/src/nodejs-common/util.ts index e6c4db98b095..b2dfc0dee8f2 100644 --- a/handwritten/storage/src/nodejs-common/util.ts +++ b/handwritten/storage/src/nodejs-common/util.ts @@ -1041,6 +1041,10 @@ export class Util { : [optionsOrCallback as T, cb as C]; } + encodeWithSlashes = encodeWithSlashes; + encodeWithoutSlashes = encodeWithoutSlashes; + encodeURIPath = encodeURIPath; + _getDefaultHeaders(gcclGcsCmd?: string) { const headers = { 'User-Agent': getUserAgentString(), @@ -1072,5 +1076,50 @@ class ProgressStream extends Transform { } } +export function encodeWithSlashes(str: string, propertyName = 'resource ID field'): string { + const segments = str.split('/'); + for (const segment of segments) { + if (segment === '.' || segment === '..') { + throw new Error( + `Value for ${propertyName} must not contain segments that are exactly . or .. .`, + ); + } + } + return encodeURIComponent(str) + .replace(/%2F/gi, '/') + .replace(/[!'()*]/g, c => '%' + c.charCodeAt(0).toString(16).toUpperCase()); +} + +export function encodeWithoutSlashes(str: string, propertyName = 'resource ID field'): string { + if (str === '.' || str === '..') { + throw new Error(`Invalid value ${str} for ${propertyName}.`); + } + return encodeURIComponent(str) + .replace(/[!'()*]/g, c => '%' + c.charCodeAt(0).toString(16).toUpperCase()); +} + +export function encodeURIPath(uri: string): string { + const parts = uri.split('/'); + return parts + .map(part => { + if (part === '') { + return ''; + } + if (part.includes(':')) { + const subparts = part.split(':'); + return subparts + .map(subpart => { + if (subpart === '') { + return ''; + } + return encodeWithoutSlashes(subpart, 'path segment'); + }) + .join(':'); + } + return encodeWithoutSlashes(part, 'path segment'); + }) + .join('/'); +} + const util = new Util(); export {util}; diff --git a/handwritten/storage/test/bucket.ts b/handwritten/storage/test/bucket.ts index 23874839e1a2..2e289b9f6429 100644 --- a/handwritten/storage/test/bucket.ts +++ b/handwritten/storage/test/bucket.ts @@ -1878,6 +1878,28 @@ describe('Bucket', () => { }); }); + describe('security - URI encoding and path traversal protection', () => { + it('should throw error when bucket name or object path segment is dot or dot-dot', () => { + assert.throws(() => { + const invalidBucket = new Bucket(STORAGE, '..'); + invalidBucket.getMetadata(assert.ifError); + }, /Invalid value \.\. for path segment\./); + }); + + it('should percent-encode query parameter injection payload in file name', done => { + const maliciousFileName = 'file_name?\\$httpMethod=DELETE#'; + const file = bucket.file(maliciousFileName); + + bucket.request = (reqOpts: DecorateRequestOptions) => { + assert(reqOpts.uri.includes('file_name%3F%24httpMethod%3DDELETE%23')); + assert(!reqOpts.uri.includes('?$httpMethod=DELETE#')); + done(); + }; + + file.getMetadata(assert.ifError); + }); + }); + describe('file', () => { const FILE_NAME = 'remote-file-name.jpg'; let file: FakeFile;