Set sortOrder on the FrontControllerInterface plugin - #42
Open
lbajsarowicz wants to merge 1 commit into
Open
Conversation
The Shield plugin on Magento\Framework\App\FrontControllerInterface had no sortOrder. Since Magento 2.4.1 PluginList::_sort() treats a missing sortOrder as PHP_INT_MIN, so nothing could be configured to run before Shield, not even with a negative value. Set sortOrder="100" and document the ordering and the page cache interaction in the README.
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.
Problem
Shield registers an around-plugin on
Magento\Framework\App\FrontControllerInterfaceinetc/di.xmlwithout asortOrder:In current
magento/magento2(2.4-develop), plugin ordering is decided byMagento\Framework\Interception\PluginList\PluginList::_sort():A plugin with no
sortOrderfalls back toPHP_INT_MIN, which means it always sorts first. No other plugin on the same interface can be configured to run before it, even with a negativesortOrder, since any finite number is still greater thanPHP_INT_MIN.This
PHP_INT_MINfallback was introduced in commit06f21adaf7d289c94f0a065fcf521f53c5c298f9("MC-31618: Move static config to files - PLUGIN_LIST", 2020-06-10). It is present in tag2.4.1and later, but not in2.4.0(tagged 2020-07-20). Before that commit,_sort()treated a missingsortOrderas0on both sides (or used the other item'ssortOrderwhen only one side had one), so a plugin withoutsortOrdersorted in the middle of the pack, not first.Core plugins already registered on
FrontControllerInterfaceshow the practical effect:Magento_PageCache'sBuiltinPlugin,VarnishPlugin, andRegisterFormKeyFromCookieall have nosortOrder(so they also sort first, alongside Shield, in registration order), whileMagento_Deploy'sconfigHashplugin usessortOrder="50". Without asortOrderon Shield's own plugin, any future core or third-party plugin that needs to run ahead of Shield (say, to short-circuit the request before Shield evaluates it) has no way to do so through configuration.Change
Added
sortOrder="100"to thesansec_shieldplugin entry inetc/di.xml:Also added a "Plugin order" section to
README.mdexplaining the ordering and its interaction with the built-in full page cache.Why 100
100 is higher than
Magento_Deploy'sconfigHash(sortOrder="50"), so that plugin still runs before Shield, unchanged from today. It leaves room below Shield (1-99) for plugins that need to run before it, and room above (101+) for plugins that should run after. It keeps Shield out of the "no sortOrder" group, which under the current sort implementation is unconfigurable and always runs first regardless of any explicit value anyone else sets.Behaviour with page cache
Magento_PageCache'sBuiltinPluginhas nosortOrder, so it still sorts before Shield'ssortOrder="100"(nosortOrderisPHP_INT_MIN, lower than any explicit value). When a request is a full page cache hit,BuiltinPlugin::aroundDispatch()returns the cached response directly without calling$proceed, so Shield's plugin never runs for that request.This is intended behavior. Full page cache entries are keyed by request URL (and vary-cookie state), so a URL carrying a malicious payload is, by construction, not a cache hit: it wasn't cached before and won't match an existing cache entry. Such a request always falls through to
$proceed, reaching Shield like any other new request. Only requests for URLs that were already served safely can be served from cache without going through Shield again.Testing
--fail-on-warningandxmllintonetc/*.xmlboth passed.magento/magento2(upstream/2.4-develop) thatPluginList::_sort()currently uses thePHP_INT_MINfallback, and traced its introduction to commit06f21adaf7d289c94f0a065fcf521f53c5c298f9, confirming the pre-2.4.1 behavior treated a missingsortOrderas0.sortOrdervalues of the other core plugins onFrontControllerInterface(Magento_PageCache'sBuiltinPlugin,VarnishPlugin,RegisterFormKeyFromCookiewith none;Magento_Deploy'sconfigHashat50) to pick a value for Shield that does not change their relative order.