Store CAPTCHA images in the configured media storage (#33051) - #41167
Open
lbajsarowicz wants to merge 2 commits into
Open
Store CAPTCHA images in the configured media storage (#33051)#41167lbajsarowicz wants to merge 2 commits into
lbajsarowicz wants to merge 2 commits into
Conversation
CAPTCHA images were always written to the local media directory while the media base URL is served from remote storage, so the browser received a 404 and the CAPTCHA could not be solved. The same happened on multi-node setups. The image is now moved to the configured media storage after generation, and the cleanup cron reaps it from there.
Contributor
Author
|
@magento run all tests |
|
Hi @lbajsarowicz. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
The build fails on any PHPCS/PHPMD violation in a file the PR touches, so wrap the two over-long lines in di.xml and suppress the coupling warning that the new TargetDirectory dependency pushes over the threshold. Also corrects the import of Filesystem\Io\File, which read Magento\Captcha\Cron\Magento\Framework\Filesystem\Io\File - a class that does not exist. Only the constructor docblock referenced it; the signature uses the fully qualified name, so this was cosmetic.
Contributor
Author
|
@magento run Static Tests |
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.
Description
With remote storage enabled, the CAPTCHA image is never reachable: the browser gets a 404 and the CAPTCHA cannot be solved. The same happens on a multi-node setup without shared storage, where the node that generates the image is not the node that serves it.
Magento\Captcha\Helper\Data::getImgDir()hardcodes the local driver:so the PNG is written to the local pod's disk.
getImgUrl()however builds the URL from$store->getBaseUrl(DirectoryList::MEDIA), and with remote storage that path is proxied to the bucket (see thelocation /media/block innginx.conf.sample). The file and the URL point at two different places.Cron\DeleteExpiredImagespins the local driver in the same way, so it would also never reap anything written remotely.Fix
The image itself has to be rendered locally —
Laminas\Captcha\Image::generateImage()writes it with nativeimagepng(), which cannot go through a Magento driver at all. So simply droppingDriverPool::FILEdoes not work:AwsS3::getAbsolutePath()returns a bucket-relative path rather than a stream wrapper, andimagepng()would silently create a bogus local file.Instead a new
afterGenerateplugin onMagento\Captcha\Model\DefaultModelmoves the rendered image into the configured media storage. It compares the target media driver with the localFiledriver and returns untouched when they are the same, so behaviour is unchanged when remote storage is off. This is the same render-locally-then-rename()pattern the existingMagento\RemoteStorage\Plugin\Imageuses.The plugin depends only on
Magento\Framework\Filesystem\Directory\TargetDirectory, which is framework-level and is re-pointed at the remote filesystem byRemoteStorage/etc/di.xml, so no new inter-module dependency is introduced.Cron\DeleteExpiredImagesnow scans the target media directory as well, so remote objects are actually cleaned up. Its new constructor argument is optional with anObjectManagerfallback, so it is not a breaking change. No signature onCaptcha\Helper\DataorDefaultModelwas changed.Fixed Issues
Fixes #33051
Manual testing scenarios
remote_storageinapp/etc/env.php, AWS S3 driver) and make sure nginx proxies/media/to the bucket.captcha_delete_expired_imagesrun and confirm the objects are removed from the bucket.pub/media/captcha/<website>/.Questions or comments
Verified locally on 2.4-develop (Warden, PHP 8.3):
Magento/Captcha/Test/Unit/— 78 tests pass, including three new cases covering the drivers-equal no-op, the move when the drivers differ, and the missing-source-file case.Magento2and PHPStan level 1: clean.bin/magento dev:di:info Magento\Captcha\Model\DefaultModelthat the plugin is registered as anafterplugin ongenerate.DefaultModel\Interceptoris used, the image is written topub/media/captcha/base/, and the plugin correctly does nothing.Two things I could not verify here and would like a reviewer to confirm against a real bucket:
rename()across drivers is covered by unit tests only. The MFTFRemoteStorageAwsS3EnabledSuitewould be the natural home for end-to-end coverage.FileSystemExceptionout of block rendering on the login page. That matchesRemoteStorage\Plugin\Image, which also does not catch, but log-and-degrade is a defensible alternative here since the alternative is a broken login form. Happy to change it if maintainers prefer.Cost note, so it is not a surprise: this adds one remote PUT per CAPTCHA render (and one DELETE from the cron). The read is done by the browser directly from the bucket or CDN, so no PHP-side GET is added.
Contribution checklist