Fix the order of the CSRF and the private response listener

[ausi]

Q	A
Fixed issues	Fixes #2067

The problem in #2067 was the order of the CSRF and make-response-private listeners: First, the make-response-private listener kept the response public, then the CSRF listener added a cookie-header to remove the CSRF cookie. This “cookie deletion” was then stored in the HTTP cache and every subsequent access to this resource always resulted in the CSRF cookie to get removed.

I’m not sure what the “correct” priorities for them are, but I think both listeners should come very late. /cc @Toflar

ToDo

• Unit test to make sure the relative order is kept in the future.

[Toflar]

Nice find. Looks good to me. However, I think we need a comment and an extension test here to ensure the order of the listeners like we did for others that depend on the priority to prevent regressions.

/edit: didn't see your todo or maybe you added it later on :D

[ausi]

/edit: didn't see your todo or maybe you added it later on :D

Added it later ☺️

[ausi]

The order of all listeners is now:

Priority	Listener
1000	Contao\CoreBundle\EventListener\InitializeControllerListener
256	Contao\CoreBundle\EventListener\MergeHttpHeadersListener
10	FOS\HttpCacheBundle\EventListener\CacheControlListener
0	TrustedCookieResponseListener_c7f9b85
0	Lexik\Bundle\MaintenanceBundle\Listener\MaintenanceListener
0	Nelmio\SecurityBundle\EventListener\ContentTypeListener
0	Nelmio\SecurityBundle\EventListener\ReferrerPolicyListener
0	Contao\CoreBundle\EventListener\PreviewToolbarListener
0	Contao\CoreBundle\EventListener\StoreRefererListener
0	Symfony\Component\HttpKernel\EventListener\ResponseListener
0	Symfony\Component\HttpKernel\EventListener\SurrogateListener
0	Symfony\Component\HttpKernel\DataCollector\RequestDataCollector
0	Symfony\Component\Security\Http\RememberMe\ResponseListener
0	Nelmio\SecurityBundle\EventListener\ClickjackingListener
0	Nelmio\SecurityBundle\EventListener\ContentSecurityPolicyListener
0	Nelmio\SecurityBundle\EventListener\XssProtectionListener
0	FOS\HttpCacheBundle\EventListener\TagListener
0	Symfony\Component\Security\Http\Firewall\ContextListener
-98	Contao\CoreBundle\EventListener\CsrfTokenCookieSubscriber
-99	Contao\CoreBundle\EventListener\MakeResponsePrivateListener
-100	Symfony\Component\HttpKernel\EventListener\ProfilerListener
-128	Symfony\Bundle\WebProfilerBundle\EventListener\WebDebugToolbarListener
-255	Symfony\Component\HttpKernel\EventListener\DisallowRobotsIndexingListener
-255	Contao\CoreBundle\EventListener\SubrequestCacheSubscriber
-768	Contao\CoreBundle\EventListener\ClearSessionDataListener
-1000	Symfony\Component\HttpKernel\EventListener\SessionListener
-1024	Symfony\Component\HttpKernel\EventListener\StreamedResponseListener

As I don’t know what most of them do, I’m not sure if -99 and -98 is correct?

[ausi]

I think we need a comment and an extension test here to ensure the order of the listeners like we did for others that depend on the priority to prevent regressions.

Done.

[Toflar]

Imho the listeners should also come after ClearSessionDataListener as that one clears expired session data. The SubrequestCacheSubscriber can be ignored because it only handles subrequests. I think. /cc @aschempp

[ausi]

The SubrequestCacheSubscriber can be ignored because it only handles subrequests.

Sure? Because it checks the Cache-Control header which is a header we set in the make-private listener.

[m-vo]

Would it make sense to always leave gaps between the priorities?
So that I could in theory register my own listener to run in between? 🙂

[aschempp]

Without checking the code, I would assume the SubrequestCacheSubscriber merges cache headers from subrequests into the main response, so it affects all requests.

[ausi]

Imho the listeners should also come after ClearSessionDataListener as that one clears expired session data.

So, how about -896 (exactly between 768 and 1024) for MakeResponsePrivateListener and -832 (exactly between 768 and 896) for CsrfTokenCookieSubscriber?

[leofeyer]

Thank you @ausi.