fix: whitelist the admin/logo media prefix so the logos load with remote storage - #5
Open
paales wants to merge 1 commit into
Open
fix: whitelist the admin/logo media prefix so the logos load with remote storage#5paales wants to merge 1 commit into
paales wants to merge 1 commit into
Conversation
Both logos upload correctly with remote storage enabled -- the file lands in
the bucket and core_config_data stores the basename -- but they cannot be
served.
HeaderPlugin renders <media base url>/admin/logo/custom/{login,menu}/<file>.
With remote storage that path does not exist on the web server's filesystem,
so nginx/Apache falls through to pub/get.php. Magento\MediaStorage\App\Media
serves only paths whose prefix appears in
system/media_storage_configuration/allowed_resources; core ships catalog,
wysiwyg, logo, tmp and friends, but nothing matching admin/... The result is a
hard 404 on both logos while the files sit happily in the bucket.
Register the admin/logo prefix from the module's own config.xml. The whitelist
is prefix-matched with stripos($resource, $allowed) === 0, so a single entry
covers both custom/login and custom/menu and nothing else under media/admin/.
Inert on a local filesystem: the web server serves those paths directly and
get.php is never reached.
Verified on Magento 2.4.9 / Mage-OS 3.2.0 with remote_storage.driver=aws-s3
(OVH S3): both logo URLs go from 404 to 200 image/png with this change.
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
1 participant
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.
Written by Claude Code:
Problem
On an installation with remote storage enabled (S3/Azure/GCS), both admin logos upload fine but cannot be displayed.
The upload half is already remote-storage-safe —
Magento_RemoteStorageprefersMagento\Framework\FilesystemtocustomRemoteFilesystem(which coversDirectoryList::MEDIA) andMagento\Framework\File\UploadertoMagento\RemoteStorage\Model\File\Uploader— so the file lands in the bucket andcore_config_datagets the bare basename, exactly as intended.Serving it is where it breaks.
HeaderPluginrenders<media base url>/admin/logo/custom/login/<file>. That path does not exist on the web server's filesystem, so nginx'stry_files $uri $uri/ /get.php$is_args$argsfalls through topub/get.php.get.php— andMagento\MediaStorage\App\Mediabehind it — serves only paths whose prefix is listed insystem/media_storage_configuration/allowed_resources. Core shipscatalog,wysiwyg,logo,tmp, … but nothing that matchesadmin/…, soisAllowed()returns false and both logos hard-404 while the files sit happily in the bucket.Note
logois on the core list, but the check isstripos($resource, $allowedResource) === 0— foradmin/logo/custom/login/x.pngthat returns6, not0, so it does not match.Fix
Register the
admin/logoprefix from the module's ownetc/config.xml. Because the check is a prefix match, one entry covers bothcustom/loginandcustom/menuand nothing else undermedia/admin/.The entry is inert on a local filesystem — the web server serves those paths directly and
get.phpis never reached — so this is safe for every installation, not just remote-storage ones.Verification
Magento 2.4.9 / Mage-OS 3.2.0,
remote_storage.driver = aws-s3(OVH S3), logos uploaded through the admin:/media/admin/logo/custom/login/logo-horizontal.pngimage/png/media/admin/logo/custom/menu/logo-vertical.pngimage/pngConfirmed on every web pod after a rollout, with no other changes in play.
One caveat worth knowing when testing:
pub/get.phpcaches the whitelist invar/resource_config.jsonforsystem/media_storage_configuration/configuration_update_timeseconds (3600 by default), andcache:flushdoes not touch that file — so an already-warm node can keep 404ing until it expires or the file is removed.Also added a short Remote Storage section to the README.