Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assets return 403 with 3.7.1 #1143

Closed
joepc74 opened this issue Aug 28, 2023 · 13 comments
Closed

assets return 403 with 3.7.1 #1143

joepc74 opened this issue Aug 28, 2023 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@joepc74
Copy link

joepc74 commented Aug 28, 2023

Bug description

Tenancy assets return 403.
This code:
<img src="{{ asset('logo.svg') }}" height="37" />
Works with 3.7, and with 2.7.1 return 403 Forbidden.

With favicon same issue:

Return 403.

Steps to reproduce

Blade code: <img src="{{ asset('logo.svg') }}" height="37" />
Direct url: http://localhost.test/tenancy/assets/logo.svg
Image dir: storage\tenantleon\app\public

Expected behavior

Show assets.

Laravel version

8.2

stancl/tenancy version

3.7.1

@joepc74 joepc74 added the bug Something isn't working label Aug 28, 2023
@stancl
Copy link
Member

stancl commented Aug 28, 2023

That would be related to this change: 4af70d3

What operating system are you on?

@joepc74
Copy link
Author

joepc74 commented Aug 28, 2023

That would be related to this change: 4af70d3

What operating system are you on?

Windows 11 with Xampp

@stancl
Copy link
Member

stancl commented Aug 28, 2023

The change we made uses realpath() to convert paths strings to absolute paths which we then validate as subdirectories of the storage_path().

The function also returns false if the path doesn't exist. I think that might be what's happening here, though in that case you'd likely get 404 before and get 403 now.

I tested how realpath() works on Windows and it seems to handle both \ and / just fine, so I'm not sure what part of the change would make this behave incorrectly on Windows.

@joepc74
Copy link
Author

joepc74 commented Aug 28, 2023

The change we made uses realpath() to convert paths strings to absolute paths which we then validate as subdirectories of the storage_path().

The function also returns false if the path doesn't exist. I think that might be what's happening here, though in that case you'd likely get 404 before and get 403 now.

I tested how realpath() works on Windows and it seems to handle both \ and / just fine, so I'm not sure what part of the change would make this behave incorrectly on Windows.

realpath("{$allowedRoot}/{$path}") ----> C:\xampp\htdocs\avata\storage\tenantleon\app\public\logo.svg
$allowedRoot ----> C:\xampp\htdocs\avata\storage/tenantleon\app/public

@stancl
Copy link
Member

stancl commented Aug 28, 2023

Ah, so it seems that the startsWith() fails since realpath() returns paths with \ while storage_path() returns the path with /. Thanks for providing those strings, I'll try to push a fix today

@stancl
Copy link
Member

stancl commented Aug 28, 2023

If you want, you can test if making this change would fix the problem for you:

- startsWith($allowedRoot))
+ startsWith(realpath($allowedRoot)))

@joepc74
Copy link
Author

joepc74 commented Aug 29, 2023

Works.

@ibarrasantiago
Copy link

I have the same issue and that change fixes it.

@xahmedtaha
Copy link

Any ETA on when this fix will be pushed? Should I make a pull request? @stancl

@stancl
Copy link
Member

stancl commented Sep 1, 2023

Just to confirm — is everyone who's experiencing this on Windows?

@xahmedtaha
Copy link

xahmedtaha commented Sep 1, 2023

Yes, with the exact same issue because of the different slashes /, \

@stancl
Copy link
Member

stancl commented Sep 2, 2023

Cool, thanks for the feedback here. Working on a refactor that implements this in a better way. realpath() is a bit of a pain since it can return false when the path doesn't exist, so I'm adding more logic to handle every possible case, as well as test every possible case while always returning 404 (no more 403) to avoid leaking any information (e.g. the existence of a file outside the storage root).

@stancl stancl closed this as completed in caf2267 Sep 2, 2023
@stancl
Copy link
Member

stancl commented Sep 2, 2023

Fixed in v3.7.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants