[HttpKernel] Preserve security exceptions #27515
Conversation
|
should we actually ignore these ? These exceptions are handled by the exception listener registered by the firewall, so they should not reach the logging listener later. This looks like a priority issue instead. |
| @@ -121,6 +125,21 @@ public function load(array $configs, ContainerBuilder $container) | ||
| ->addTag('security.voter'); | ||
| } | ||
|
|
||
| public function process(ContainerBuilder $container) |
There was a problem hiding this comment.
I would rather put this compiler pass in a dedicated class.
We have lots of different passes in each of our bundles, and this logic is not more related to the DI extension than other passes (rather less)
| $request = $event->getRequest(); | ||
| do { | ||
| foreach ($this->silentIgnore as $class) { | ||
| if (is_a($exception, $class)) { |
| return; | ||
| } | ||
| } | ||
| } while (null !== $exception = $exception->getPrevious()); |
There was a problem hiding this comment.
use a different variable name for this loop, to avoid overriding the one containing the main exception
|
Just wondering: did #27516 change anything here? |
|
@nicolas-grekas yes :) @stof i'm hesitating between this approach and raising the security exception listener from 0 to 2048+1, edit: then again.. exceptions are edge-case-ish anyway.. so perhaps we can get away with it, along with a clear changelog entry. I do prefer the priority solution.. sure |
|
Shouldn't we revert #25366 instead? This breaks a behavior that was accepted. Maybe only keep the early collection, but when a listener takes over handling, I see no reason to not let it decide what to do with the exception, don't you think? Especially given the comments on #27440. |
|
status: needs work |
|
closing in favor of #27562 |
This PR introduces an approach that allows for skipping certain (origin) exception classes from being logged. In this case the security related ones.