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 13 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
- Implemented "jail" logic using path.resolve to prevent traversal. - Neutralized leading slashes in GCS object names. - Pre-allocated results array to maintain 1:1 input/output index parity. - Added automatic recursive directory creation for nested local paths. - Fixed prioritization of destination options in downloadManyFiles.
- Reordered security checks to catch illegal drive letters before path resolution. - Fixed SkipReason mismatch (Illegal Character vs Path Traversal) on Windows. - Ensured absolute Windows paths are neutralized before traversal validation.
…fety Isolated async loop variables to fix path leakage, decoupled prefix from destination logic, and added cross-platform traversal checks for both forward and backslashes.
Updated Parity Check tests with platform-conditional logic to handle OS-specific backslash resolution.
| if (options.skipIfExists && existsSync(resolvedPath)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Can you assign an empty result here as well with a skip reason? If you continue, you'll be leaving a hole in the final result array.
There was a problem hiding this comment.
I have updated the logic to assign a skipped result with SkipReason.ALREADY_EXISTS to that index before continuing. This ensures the finalResults array remains fully populated and perfectly aligned with the input file array.
| * // - "file2.txt" (with the contents from my-bucket.file2.txt) | ||
| * const response = await transferManager.downloadManyFiles('test-folder'); | ||
| * // All files with GCS prefix of 'test-folder' have been downloaded. | ||
| * ``` |
There was a problem hiding this comment.
Can you add more notes here about the change in behavior. Previously we were returning only files which actually get downloaed. Previously the input request files size and returned results need not match but now the return result is exactly equal to input request files size. Also communicate the traversal vulnerability changes using a few comments here.
There was a problem hiding this comment.
I have updated the JSDoc.
|
|
||
| export type DownloadResponse = [Buffer]; | ||
|
|
||
| export type DownloadResponseWithStatus = [Buffer] & { |
There was a problem hiding this comment.
QQ. This doesn't cause a breaking change to the existing customers because the return type is being changed? The existing users can still continue using this utility as is?
There was a problem hiding this comment.
It is not a breaking change. The return type remains an array at its core, so existing code using index-based access or destructuring (e.g., [buffer]) will continue to work. We are simply using a TypeScript intersection to attach additive metadata like skipped and reason to that array object.
5 participants
