Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Fix Firewall ExceptionListener priority#23291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
664dd07 to02c7351Compare8b7b22e to370379fCompareweaverryan commentedJun 26, 2017
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. |
nicolas-grekas commentedJun 28, 2017
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? |
chalasr commentedJun 28, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
stof commentedJun 28, 2017
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) |
stof commentedJun 28, 2017
I put a -1 vote on the current implementation adding more complexity in the registration process |
chalasr commentedJun 28, 2017
Fine to me, hope it doesn't cause other troubles. PR updated |
fabpot commentedJul 3, 2017
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/aWhen 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----```phparray: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----```phparray: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
Uh oh!
There was an error while loading.Please reload this page.
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.