fix(security): prevent path traversal in S3 and Azure store providers#360
fix(security): prevent path traversal in S3 and Azure store providers#360heatxsink wants to merge 1 commit intorocketride-org:developfrom
Conversation
Add path traversal validation to S3Store._get_key() and AzureBlobStore._get_blob_name() using posixpath.normpath to resolve ../ sequences, then verifying the result stays within the configured prefix. This matches the existing protection in FilesystemStore._get_full_path(). Without this fix, a path containing ../../other-tenant/ could escape the per-tenant prefix and access other tenants' data on S3 or Azure.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Bug Summary
The S3 and Azure storage providers lack path traversal validation in their key/blob-name construction methods. Unlike
FilesystemStore, which properly usesPath.resolve()andPath.relative_to()to prevent../escapes, the cloud providers simply concatenate the configured prefix with the user-supplied path. Aproject_idorsourceparameter containing../../other-user/can escape the intended directory structure and access other tenants' data on S3/Azure.Affected Files and Lines
packages/ai/src/ai/account/store_providers/s3.py—S3Store._get_key()(lines 349-354)packages/ai/src/ai/account/store_providers/azure.py—AzureBlobStore._get_blob_name()(lines 341-346)Root Cause
Both methods perform only a backslash-to-forward-slash replacement and then concatenate the prefix with the path:
No normalization or boundary check is applied, so
../sequences in the path escape the prefix boundary.Reproduction Steps
tenant-A/data_get_key('../../tenant-B/secrets.txt')(or the Azure equivalent)tenant-A/data/../../tenant-B/secrets.txt, which S3/Azure resolves totenant-B/secrets.txt— cross-tenant accessStorageError('Path traversal detected: ...')Severity: HIGH
Cross-tenant data access in a multi-tenant cloud storage system. Any API endpoint that accepts user-supplied file paths and passes them through to the store providers is affected.
Fix Description
Added path traversal validation to both
_get_key()(S3) and_get_blob_name()(Azure):posixpath.normpath()to canonicalize the full key/blob-name (resolves../sequences)StorageErrorif traversal is detectedThis mirrors the approach used by
FilesystemStore._get_full_path(), which usesPath.resolve()+Path.relative_to()for the same purpose.Tests/Validation
Added
packages/ai/tests/ai/account/test_path_traversal.pywith 14 test cases (7 per provider) covering:../../escaping the prefix is rejected../escaping a shallow prefix is rejected..\\..\\) is rejected../(stays within prefix) is allowed#frontier-tower-hackathon