feat(storage): Implement robust path validation and structured skip reporting#7546
feat(storage): Implement robust path validation and structured skip reporting#7546thiyaguk09 wants to merge 2 commits intogoogleapis:mainfrom
Conversation
thiyaguk09
commented
Mar 10, 2026
thiyaguk09
commented
- Adds protection against path traversal (../) using normalized path resolution.
- Prevents Windows-style drive letter injection while allowing GCS timestamps.
- Implements directory jail logic to ensure absolute-style paths are relative to destination.
- Preserves backward compatibility by returning an augmented DownloadResponse array.
- Automates recursive directory creation for validated nested files.
- Adds a comprehensive 13-scenario test suite for edge-case parity.
- Adds protection against path traversal (../) using normalized path resolution. - Prevents Windows-style drive letter injection while allowing GCS timestamps. - Implements directory jail logic to ensure absolute-style paths are relative to destination. - Preserves backward compatibility by returning an augmented DownloadResponse array. - Automates recursive directory creation for validated nested files. - Adds comprehensive 13-scenario test suite for edge-case parity.
product-auto-label
bot
added
the
api: storage
quirogas
added
the
priority: p2
| let files: File[] = []; | ||
|
|
||
| const baseDestination = path.resolve( | ||
| options.prefix || options.passthroughOptions?.destination || '.' |
There was a problem hiding this comment.
Should we prioritize options.passthroughOptions?.destination first before options.prefix?
|
|
||
| return Promise.all(promises); | ||
| const results = await Promise.all(promises); | ||
| return [...skippedFiles, ...results]; |
There was a problem hiding this comment.
Maintain the input order. Results should be placed in the array at the same index as their corresponding input file. Otherwise this could potentially be a breaking change.
| dest = path.join( | ||
| options.prefix || '', | ||
| passThroughOptionsCopy.destination || '', | ||
| file.name, | ||
| normalizedGcsName |
There was a problem hiding this comment.
Why are we combining prefix and destination here? , previously in line 580, you prioritised prefix over baseDestinations.
I think you need to only use baseDestination everywhere and combine it with the object name and do path traversal validations.
| if (options.stripPrefix) { | ||
| passThroughOptionsCopy.destination = file.name.replace(regex, ''); | ||
|
|
||
| const resolvedPath = path.resolve(baseDestination, dest); |
There was a problem hiding this comment.
If dest is an absolute path, path.Resolve will return dest as is. But that is not the behavior we want. If object name is /etc/passwd and if the user given options.passthroughOptions?.destination is /usr/downloads, the final path should be /usr/downloads/etc/passwd. Can you check if this is happening in this scenario?
3 participants
