Fix nginx deny-all shadowing the /storage public-file allow-list - #228
Merged
Merged
Conversation
…w-list
location ^~ /storage { deny all; } used the ^~ prefix modifier, which
tells nginx to stop searching and skip all regex locations once this
prefix matches. That silently shadowed the regex location right below
it that was meant to allow public image/PDF files under /storage, so
every request under /storage was denied with a 403 regardless of the
allow-list, including Jetstream profile photos served from the public
disk.
Dropping ^~ lets nginx fall through to the regex allow-list for
requests that match an allowed extension, while everything else still
hits the deny-all fallback.
Adds a regression test that parses the committed nginx.conf (nginx
doesn't run under the PHPUnit suite) and asserts the deny rule no
longer uses ^~, following the existing pattern used for the CSP header
test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was broken
docker/nginx.confhad:The
^~prefix modifier tells nginx to stop searching for a matching location and skip evaluating any regex locations once this prefix location is the longest match. Since/storagematched^~first, the regex allow-list right below it was never reached, so every request under/storagewas denied with a 403 — including public files served from thepublicfilesystem disk, such as Jetstream profile photos (profile_photo_disk = publicinconfig/jetstream.php, served fromAPP_URL/storage).What changed
^~modifier from the/storagedeny-all location indocker/nginx.conf. Without it, nginx still evaluates the regex allow-list location for allowed extensions (jpg/jpeg/png/gif/ico/svg/pdf) before falling back to the deny-all rule for anything else under/storage.tests/Feature/NginxStorageAccessTest.php, which parses the committeddocker/nginx.conf(nginx doesn't run under the PHPUnit suite) and asserts the deny rule no longer uses^~while both the deny-all fallback and the extension allow-list still exist. This follows the same pattern as the existingNginxContentSecurityPolicyTest. Verified the new test fails against the pre-fix config and passes after the fix.Test plan
vendor/bin/pint --dirty— cleancomposer test(full PHPUnit suite) — 261 passed, 1 pre-existing skiptests/Feature/NginxStorageAccessTest.phpfails on the original^~ /storageconfig and passes after the fixFixes #139