[Security] Fix Firewall ExceptionListener priority #23291
Conversation
664dd07 to
02c7351
Compare
8b7b22e to
370379f
Compare
|
I've just chatted with @chalasr about this. We broke slightly BC... as listeners with an identical priority are now registered in a different order than before. There are only 3 ways to fix this: A) This PR. It adds extra complexity, but truly re-adds the previous behavior. B) Changing the Firewall\ExceptionListener priority to a positive number. That's very risky: it will likely cause other BC breaks. C) Do nothing and expect the user to set their exception listener to a positive number. But going forward, this means that when a user creates an exception listener with no priority, they may break their security setup. That sucks :). I think we need @nicolas-grekas to tech review this, but I'm +1 on the concept. |
|
If we were to not do this, is there a way to change the declaration order in our XML files to restore the original one? |
|
Changing the order of the listeners declarations does not help. The firewall exception listener is registered at runtime (not a service). As said, changing priorities would probably cause more issues than it would solve. |
|
The right fix here is to increase the prirority of the firewall ExceptionListener (make it 1, so that it is still below other listeners registered with a priority). AFAIK, our promise for listeners having the same priority is that the order is totally undefined for them. The firewall was relying on an implementation detail here, and we should just make it defined properly. The Firewall expects its listener to run before the ExceptionListener from HttpKernel anyway (this is required for it to work), and priorities are the way to ensure that. We just had a broken usage of the dispatcher since years, which was hidden by the lazy registration previously (which actually means that projects not using our containerAwareEventDispatcher might have been affected before btw, so I would even consider fixing it in lower branches) |
|
I put a -1 vote on the current implementation adding more complexity in the registration process |
370379f to
8014b38
Compare
|
Fine to me, hope it doesn't cause other troubles. PR updated |
|
Thank you @chalasr. |
This PR was merged into the 3.3 branch. Discussion ---------- [Security] Fix Firewall ExceptionListener priority | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23253 | License | MIT | Doc PR | n/a When making EventDispatcher able to lazy load listeners, we stopped using `ContainerAwareEventDispatcher::addListenerService/addSubcriberService`, we use `EventDispatcher::addListener()` instead. This change makes that the order of listeners is different than before, because `ContainerAwareEventDispatcher` calls `addListener()` tardily so that factories are never stored in `EventDispatcher::$listeners`. Example diff due to the behavior change in 3.3 (registering an `AppBundle\ExceptionListener::doCatch()` exception listener in the fullstack): 3.2 ---- ```php array:5 0 => "Symfony\Component\Security\Http\Firewall\ExceptionListener::onKernelException" 1 => "AppBundle\ExceptionListener::doCatch" 2 => "Symfony\Component\HttpKernel\EventListener\ProfilerListener::onKernelException" 3 => "Symfony\Bundle\SwiftmailerBundle\EventListener\EmailSenderListener::onException" 4 => "Symfony\Component\HttpKernel\EventListener\ExceptionListener::onKernelException" ] ``` 3.3 ---- ```php array:5 [ 0 => "AppBundle\ExceptionListener::doCatch" 1 => "Symfony\Component\HttpKernel\EventListener\ProfilerListener::onKernelException" 2 => "Symfony\Bundle\SwiftmailerBundle\EventListener\EmailSenderListener::onException" 3 => "Symfony\Component\Security\Http\Firewall\ExceptionListener::onKernelException" 4 => "Symfony\Component\HttpKernel\EventListener\ExceptionListener::onKernelException" ] ``` (that is what breaks #23253, the lazy listener is called before the runtime firewall exception listener on dispatch). This fixes the order by increasing the security exception listener priority. Commits ------- 8014b38 [Security] Fix Firewall ExceptionListener priority
When making EventDispatcher able to lazy load listeners, we stopped using
ContainerAwareEventDispatcher::addListenerService/addSubcriberService, we useEventDispatcher::addListener()instead. This change makes that the order of listeners is different than before, becauseContainerAwareEventDispatchercallsaddListener()tardily so that factories are never stored inEventDispatcher::$listeners.Example diff due to the behavior change in 3.3 (registering an
AppBundle\ExceptionListener::doCatch()exception listener in the fullstack):3.2
3.3
(that is what breaks #23253, the lazy listener is called before the runtime firewall exception listener on dispatch).
This fixes the order by increasing the security exception listener priority.