fix(storage): Implement path containment to prevent traversal attacks

[thiyaguk09]

This pull request significantly enhances the security of the downloadToFile method by implementing comprehensive path validation. The changes prevent directory traversal attacks, ensuring that files can only be written to designated locations and mitigating the risk of malicious file overwrites or unauthorized access to the file system. This is achieved through checks for relative path components, absolute paths, and Windows drive specifications, complemented by new and updated test cases.

Highlights

• Path Containment Implementation: Implemented robust path containment logic within the downloadToFile() method to prevent directory traversal attacks.
• Comprehensive Path Validation: The downloadToFile() method now normalizes path separators and explicitly checks for '..' segments, paths starting with '/', and Windows drive letters (e.g., 'C:').
• Security Exception Handling: A RuntimeException is thrown with a clear message if an invalid or malicious path is detected during the download operation.
• New Test Cases for Traversal: Added new system and unit tests to specifically verify that both relative ('..') and absolute path traversal attempts are correctly blocked.
• Test Case Refinement: Updated existing tests to use relative file paths instead of __DIR__ or php://temp, aligning with the new security checks and preserving the original intent of verifying file system interactions.

Changelog

• Storage/src/StorageObject.php
  • Introduced path validation logic within downloadToFile to prevent directory traversal.
  • Normalized path separators from \ to / for consistent checking.
  • Added checks for .. as a distinct path segment, paths starting with /, and paths beginning with a Windows drive letter (e.g., C:).
  • Configured to throw a RuntimeException with a specific message upon detection of a disallowed path.
• Storage/tests/System/ManageObjectsTest.php
  • Modified testDownloadsToFileShouldNotCreateFileWhenObjectNotFound to use a simple relative filename for the download path.
  • Added testDownloadsToFileShouldBlockRelativeTraversal to confirm .. in paths triggers the expected RuntimeException.
  • Added testDownloadsToFileShouldBlockAbsolutePath to confirm absolute paths trigger the expected RuntimeException.
• Storage/tests/Unit/StorageObjectTest.php
  • Modified testDownloadsToFileShouldNotCreateFileWhenObjectNotFound to use a simple relative filename for the download path.
  • Added testDownloadsToFileShouldBlockRelativeTraversal to confirm .. in paths triggers the expected RuntimeException.
  • Added testDownloadsToFileShouldBlockAbsolutePath to confirm absolute paths trigger the expected RuntimeException.

[bshaffer]

Great job! Some minor suggestions is all I have

[bshaffer]

great job @thiyaguk09 ! thank you

[acoulton]

Unfortunately this change has broken BC for us across a number of projects, where we can already guarantee that $path is safe from user injection.

I think it does not actually provide the benefit it claims, and in some cases would reduce security. IMO it should be reverted - please see #8690 for full writeup.