Fix the flaky test testDoDownloadDirectory_PathTraversalProtection

[sharatchandrag]

Summary
Add path-traversal protection to doDownloadDirectory and re-enable disabled test

Description

• Add path-traversal protection in GcpBlobStore#doDownloadDirectory: each blob's post-strip destination is resolved against a canonicalized safeTargetDir = targetDir.toAbsolutePath().normalize() and rejected if it escapes via Path#startsWith (segment-based, not lexical).
• Rejected blobs are reported as FailedBlobDownload entries with a SecurityException("Blocked path traversal for blob: <name>") and are filtered before TransferManager.downloadBlobs(...), so attacker-controlled keys never reach the GCS transfer pipeline.
• Folder-marker handling and the early-return on empty blobInfos are preserved; stripPrefix semantics continue to match TransferManager.setStripPrefix.
• Re-enable GcpBlobStoreTest#testDoDownloadDirectory_PathTraversalProtection (was @Disabled("Failing because of temp directory permission")). The original test was unfixable as written — it verified Blob#downloadTo (never called by the implementation) and did not stub TransferManager. Rewritten to assert the real contract: malicious blob is in failedTransfers with a SecurityException, destination is outside tempDir, and mockTransferManager.downloadBlobs(...) is never invoked.
• No public API change; legitimate keys (including ones with .. segments that stay within the target directory) are unaffected. GcpBlobStoreTest passes (300/300).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.98%. Comparing base (b0d51ad) to head (2069a8b).

Files with missing lines	Patch %	Lines
.../salesforce/multicloudj/blob/gcp/GcpBlobStore.java	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #422      +/-   ##
============================================
+ Coverage     81.97%   81.98%   +0.01%     
  Complexity      646      646              
============================================
  Files           198      198              
  Lines         12802    12815      +13     
  Branches       1695     1698       +3     
============================================
+ Hits          10495    10507      +12     
  Misses         1567     1567              
- Partials        740      741       +1     

Flag	Coverage Δ
unittests	81.98% <93.75%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[iamabhilaksh]

Thanks for putting this together!
Could you shed a little more light on the context behind this? It would be really helpful to understand the exact root cause, how this specific implementation fixes it, and the steps you took to verify it. Thanks!

[sharatchandrag]

@iamabhilaksh Please note:
Context: (Aws and Ali sdk handles this but in case of Gcp we need to handle this)
When downloading a directory of blobs, we need to check where each file would land. A blob with a malicious key like prefix/../../../etc/passwd could escape the target folder and write files to unintended locations on the filesystem.

How the fix works: Before downloading each blob, we now check — "does this file's destination path actually live inside the intended target folder?" If not, we skip it and record it as a failed download with a security error.

Example:
Target folder: /tmp/downloads
Malicious blob key: prefix/../../../etc/passwd → resolves to the root directory /etc/passwd → ❌ blocked
Normal blob key: prefix/subdir/file.txt → resolves to /tmp/downloads/subdir/file.txt → ✅ allowed

[SriramGuduri]

LGTM

[iamabhilaksh]

Great work! The path-traversal protection is secure and properly implemented.

I have one minor nit regarding consistency:

In the traversal check, you compute the failed destination using safeTargetDir (which is absolute). However, in the TransferManager failure-handling block later on, you use targetDir (which could be relative, depending on user input).

Suggestion: Use safeTargetDir in both places so API consumers receive a consistent, absolute path format for all FailedBlobDownload errors.

Diff

- Path destination = targetDir.resolve(relative).normalize();
+ Path destination = safeTargetDir.resolve(relative).normalize();

Other than that, this looks excellent! 🚀

[iamabhilaksh]

Suggestion: There's no test for the prefix=null path. When prefix is null, stripPrefix
is "", so relative = name (the raw blob key), and destination = safeTargetDir.resolve(name) .normalize(). A blob named "../../../etc/passwd" should still be blocked. Worth adding a
test to confirm the protection holds when no prefix is specified?

[sharatchandrag]

Yes, will take this edge case in the next iteration

[iamabhilaksh]

Suggestion: The test only covers the all-blocked case (single malicious blob, blobInfos
ends up empty). There's no test for the mixed case — one malicious + one legitimate blob —
where we'd want to assert that downloadBlobs IS invoked with exactly the legitimate blob
while the malicious one stays in failedTransfers. That's the load-bearing scenario:
path-traversal protection working alongside normal downloads. Could we add a second test
case for that path?

[sharatchandrag]

Yes this is good point, will take up this in the next iteration

[iamabhilaksh]

Question: Path#startsWith is a segment-based check, not a lexical prefix match, so it
correctly blocks safeTargetDir/../sibling after normalize() resolves it. One edge case:
if safeTargetDir itself normalizes to / on an unusual mount (e.g. a container root), then
every path starts with it and the check becomes a no-op. Is that worth a comment, or do we
consider that out-of-scope for this fix?

[sharatchandrag]

This is out of scope usecase for us, its unsual for the users to pass the root directory as the download directory and also most systems will reject such downloads

[iamabhilaksh]

nit : Use StringUtils.EMPTY instead.

final String stripPrefix = prefix != null ? prefix : StringUtils.EMPTY;

[iamabhilaksh]

Left some minor comments. Rest LGTM 👍

[SriramGuduri]

LGTM — clean, correct path-traversal protection using the resolve-normalize-startsWith pattern. Malicious blobs are filtered before reaching TransferManager.

Nitpicks (non-blocking):

1. GcpBlobStoreTest.java — consider using assertInstanceOf(SecurityException.class, failure.getException()) (JUnit 5.8+) instead of assertTrue(x instanceof Y) for clearer failure messages.

2. GcpBlobStore.java — minor: stripPrefix and the path-traversal check both derive from the same request field. Just a note to keep these in sync if the prefix semantics ever diverge.

[LihaoLiuXs]

The prefix strip here does not match how the GCS SDK computes the destination, so the guard validates a path the SDK will not use — and it rejects legitimate blobs.

In google-cloud-storage 2.62.0 (the version pinned in blob-gcp/pom.xml), TransferManagerUtils.createDestPath computes the destination as
config.getDownloadDirectory().resolve(blob.getName().replaceFirst(config.getStripPrefix(), "")).
String#replaceFirst treats the strip prefix as a regex, while line 1043 strips it as a literal prefix. The two agree only when the prefix contains no regex metacharacters.

Concrete case — prefixToDownload = "backup(1)/", blob key backup(1)/../evil.txt:

• SDK: replaceFirst("backup(1)/", "") matches nothing (that pattern means backup1/), so it writes <dir>/backup(1)/../evil.txt → <dir>/evil.txt, safely inside the target directory.
• This code: strips literally → ../evil.txt → resolves to <parent>/evil.txt → the blob is failed with SecurityException.

A download that works today starts failing, which contradicts "legitimate keys (including ones with .. segments that stay within the target directory) are unaffected". The same mismatch makes the reported destination wrong.

Suggested fix: make both sides strip identically, e.g. setStripPrefix(Pattern.quote(prefix)) at line 1067 — getStripPrefix() is consumed only by createDestPath, and quoting also avoids PatternSyntaxException for prefixes such as data[2024/.