S3 adapter should always use / separator independent of the host OS

[stancl]

Laravel Version

12.x

PHP Version

8.4

Database Driver & Version

irrelevant

Description

I got a bug report in my tenancy package, https://github.com/archtechx/tenancy/issues/1333

After reproducing it and not finding any logic on our end that would cause this, I tried to reproduce this without tenancy logic involved and managed to do so.

The bug is essentially that temporaryUrl() and possibly other methods (not confirmed) use the OS-specific DIRECTORY_SEPARATOR while S3 only supports /.

From a very brief source dive the cause seems to be these prefixPath() calls:

https://github.com/laravel/framework/blob/67286590127b8531b661a85e17c0a39d12030429/src/Illuminate/Filesystem/AwsS3V3Adapter.php#L80

Which use $this->prefixer, defined in the parent class and initialized as: https://github.com/laravel/framework/blob/67286590127b8531b661a85e17c0a39d12030429/src/Illuminate/Filesystem/FilesystemAdapter.php#L106-L108

The only thing I'm confused by is that these calls are abundant in the S3 adapter so the issue should exist in other cases too. It might be that this bug exists in other methods too, just wasn't noticed, or that S3 is tolerant of \ on some endpoints but not others?

Either way a quick fix should be initializing the prefixer in the S3 adapter's own constructor to make it use the / separator.

See the steps below for reproduction.

Steps To Reproduce

• Be on Windows, easiest way to repro outside of Windows is to probably just replace the DIRECTORY_SEPARATOR constant referenced above with a '\\' as a literal value
• Add 'root' => 'something', to the 's3' disk config in config/filesystems.php
• Do:

  Storage::disk('s3')->put('foo.txt', 'bar');
  Storage::disk('s3')->temporaryUrl('foo.txt', now()->addMinutes(10));
  

You'll get a URL containing %5C (hex for \) which makes the URL invalid since the path to the object should be using /.

[stancl]

Alternative solution to overriding the constructor, set filesystems.s3.directory_separator to '/' in laravel/laravel's default filesystems config file.

[github-actions[bot]]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[Kleppinger]

@stancl I've added an PR that got merged which fixes this issue.

[AhmedAlaa4611]

Hello @stancl and @crynobone

I think this should be closed in favor of: #57497 and #57534.

[JapSeyz]

as per my comment on https://github.com/laravel/framework/pull/57497#issuecomment-3450407488 - it broke every single prefixed media path. Hopefully #57534 is better.