It always seemed weird to me#57372 added a stateless check in theContextListener because it would be an error to have it running in a stateless context, but now this error is hidden.

Wouldn’t it be better to remove said check?

Ping@VincentLanglet

@MatTheCat Before the change in#57372, Symfony will try to fetch the token from the session and refresh the user (even if the request is stateless). The problem now is that the change only updated thekernel.request event listener and left thekernel.response event listener as is. Due to the absence of thekernel.request event listener, no token is set and thekernel.response event listener therefore removes the session.

Maybe another option would be to revert the change made to theContextListener in#57372 and instead return false in thesupports method for stateless requests? This would have the same result that both event listeners would no longer do anything.

It always seemed weird to me#57372 added a stateless check in theContextListener because it would be an error to have it running in a stateless context, but now this error is hidden.

When I tried to debug#57372, I got error because the session was used and it wasn't voluntary. I don't remember having done anything to use the ContextListener.

Wouldn’t it be better to remove said check?

symfony/src/Symfony/Component/Security/Http/Firewall/ContextListener.php

Line 98 inf3e0c81

$session = !$request->attributes->getBoolean('_stateless') &&$request->hasPreviousSession() &&$request->hasSession() ?$request->getSession() :null;

Either the check should be kept and some others fixes are required, either something else need to be find in order to not trigger this listener on stateless request.

Maybe another option would be to revert the change made to the ContextListener in#57372 and instead return false in the supports method for stateless requests? This would have the same result that both event listeners would no longer do anything.

I like this idea@micheh.

I don't remember having done anything to use the ContextListener.

Your firewall was stateful, which didn't match the route configured as stateless.

AFAIU the point of stateless requests was to warn you if you were using the session, not to stop Symfony from using it.

Your firewall was stateful, which didn't match the route configured as stateless.

I don't think so, I never declared a route as stateless, I just had firewall stateful (and route stateful) or firewall stateless (and route stateless).

AFAIU the point of stateless requests was to warn you if you were using the session, not to stop Symfony from using it.

I don't think so, the check done in the AbstractSessionListener

That's why there was already previous fix as#54742 or#51350.

When the request is stateless, IMHO the session is supposed to never be used. Which lead some questions like#57502 (comment) about throwing an exception inrequest::getSession for stateless requests...

I never declared a route as stateless, I just had firewall stateful (and route stateful) or firewall stateless (and route stateless).

Then I don’t see how you could get an error coming from theContextListener 🤔

the check done in the AbstractSessionListener doesn't check if the session was use by Symfony or the developer.

I don’t think there is a need for that. But, as a user, you can trigger session usage by Symfony: like when you have a stateful firewall, theContextListener will always run. On the other hand, as a user you cannot do anything against the WebProfilerBundle using the session or not.

In the first case I’d prefer to be warned because the session was used because of me.
In the second, the WebProfilerBundle should be updated to work, the request being configured stateless or not.

Considering every case as the second means you end up hiding developers’ mistakes, which I don’t think is wise 🤷‍♂️

I never declared a route as stateless, I just had firewall stateful (and route stateful) or firewall stateless (and route stateless).

Then I don’t see how you could get an error coming from theContextListener 🤔

Honestly I dunno either, I just tried and didn't reproduce but I don't remember about the time I debug this and I surely had a reason because I didn't add the stateless check on everyhasSession call. But still the usecase of micheh should be valid no ? A stateless request is possible on a stateful firewall.

Then, wouldn't the best to have the following methodsupports in the ContextListener

     public function supports(Request $request): ?bool    {        return $request->attributes->get('_stateless') ? false : null;    }

WDYT about updating your PR@micheh ?

A stateless request is possible on a stateful firewall.

Well, no 😅

A stateful firewall will (de)serialize the current user using the session. If you're saying a request matching this firewall is stateless, you either misconfigured the request, or the firewall.

A stateless request is possible on a stateful firewall.

Well, no 😅

A stateful firewall will (de)serialize the current user using the session. If you're saying a request matching this firewall is stateless, you either misconfigured the request, or the firewall.

Indeed, my bad.

Anyway, I agree the stateless check should be reverted and then I'll see if have session usage.
If not it's perfect, if yes and it's coming from symfony, the support method might need to be updated.

Should this PR be updated or do you want to open another PR@MatTheCat ?

I guess this PR can be updated since its goal stays the same?

A stateful firewall will (de)serialize the current user using the session. If you're saying a request matching this firewall is stateless, you either misconfigured the request, or the firewall.

I disagree. It should be possible to have some stateless routes in a stateful firewall. Routes can configure a stateless boolean option in order to declare that the session shouldn't be used when matching a request. This includes fetching the user from the session, which theContextListener does. The option exists for this reason on the route itself to allow stateless routes in a stateful firewall.

If you revert the changes in#57372 it will throw an exception, because the listener will try to fetch the user from the session, which is not allowed in a stateless route. With the change in#57372 and in this PR, it will prevent theContextListener to use the session, which means there is no more exception. But it will still throw an exception if you use the session yourself, because this logic is in theAbstractSessionListener that remains unchanged. Therefore it wont hide developer mistakes, but update Symfony to respect the stateless setting correctly in theContextListener.

If you revert the changes in#57372 it will throw an exception, because the listener will try to fetch the user from the session, which is not allowed in a stateless route. With the change in#57372 and in this PR, it will prevent theContextListener to use the session, which means there is no more exception. But it will still throw an exception if you use the session yourself, because this logic is in theAbstractSessionListener that remains unchanged. Therefore it wont hide developer mistakes, but update Symfony to respect the stateless setting correctly in theContextListener.

Then wouldn't it better to return false in the ContextListener::support method when the request is stateless ?

Well it all boils down if you considerstateless as a check or as a directive.

The blog post which introduced it reads

Routes can now configure astateless boolean option. If set totrue, they declare that session won't be used during the handling of the request.

Note that it is writtendeclare, notforce.

Following your suggestion, there wouldn’t be any difference between a stateful and stateless firewall. Feels weird to me (also, it probably would be a new feature more than a bugfix 🤔).

(I’m unsubscribing from this thread because we’re running in circles.)

I agree with@MatTheCat about the intention of the_stateless attribute. The changes in this PR and in#57372 are changing the semantics of the attribute.

Stateful and Lazy firewalls should be compatible with_stateless routes BTW.

I agree with@MatTheCat about the intention of the_stateless attribute. The changes in this PR and in#57372 are changing the semantics of the attribute.

Then should#57372 be reverted@nicolas-grekas ?

Yes, we should consider reverting indeed. And reconsidering the original issue from this more-accurate angle.

-1 for this PR. If a firewall is configured as being stateful, the ContextListener should always run (as that's exactly what a stateful firewall is about).
The right fix is to revert the change done in the ContextListener inhttps://github.com/symfony/symfony/pull/57372/files (other changes in the profiler code are fine, as the web debug toolbar needs to be compatible with stateless requests as it is a debug tool running on all pages)

If a route is explicitly configured as being stateless, it must either not perform any security checks (to never trigger lazy authentication) or perform it using a stateless firewall. Otherwise it cannot be stateless.

#58002@nicolas-grekas@stof

(other changes in the profiler code are fine, as the web debug toolbar needs to be compatible with stateless requests as it is a debug tool running on all pages)

If a route is explicitly configured as being stateless, it must either not perform any security checks (to never trigger lazy authentication) or perform it using a stateless firewall. Otherwise it cannot be stateless.

Then#57852 would still be useful IMHO

Then#57852 would still be useful IMHO

I don't think so.

Removing the security check in your controller because you marked the request as stateless does not make any sense. If your controller needs a security check, it should perform it. And then, if the security check involved using the session, you will get an exception (in debug mode) or a warning in logs telling you that you marked a request as stateless while it is not, which means a mistake in your project (and then, the right fix is either to change your firewall to be stateless or to allow the request to be stateful).

I don't see a lot of use cases for explicit checking on$request->isStateless() outside debug tools like the profiler (and custom collectors have no reason to read the session as we already cover this with the core collector), the dedicated check in the session listener and the special logic we have (in the core as well) for the smart default of that state for stateless firewalls.
cases where you mark a request as stateless or stateful explicitly will likely need to know it is done through an attribute anyway as the standard way to do that is by putting the information in the route definition (relying on the route defaults to declare the attribute).

I would even say that introducingRequest::isStateless() could push developers towards the wrong path for all cases where they arenot expected to check it.

Then#57852 would still be useful IMHO

I don't think so.

Removing the security check in your controller because you marked the request as stateless does not make any sense. [...]

I don't see a lot of use cases for explicit checking on$request->isStateless() outside debug tools like the profiler [...]

I would even say that introducingRequest::isStateless() could push developers towards the wrong path for all cases where they arenot expected to check it.

I personally have multiple use-cases in our code base:

• When looking at the flash messages doc (cfhttps://symfony.com/doc/current/session.html#flash-messages), call to{% for message in app.flashes('notice') %} is done in thebase.html.twig template. When used by a stateless request it add a usage of the session. Since having two base templates is not really convenient, it's better to have astateless check inside the twig template before looking for flashes. (Or better symfony could add this check inside his code maybe)

• We're having a service which add the session id to theMonolog\LogRecord and this should be skipped when the request is stateless or it will add a session usage.

• We're using some listenersonKernelResponse/onKernelRequest which are doing multiple things and one of these things is setting/getting a value in the session. So we've added a stateless check to skip this step for stateless requests but keep the rest of the logic.

Stateful and Lazy firewalls should be compatible with_stateless routes BTW.

Currently they are not compatible becauseTokenStorage::getToken() is called in some event listeners, which will instruct the ContextListener to try to fetch the user from the session (resulting in an exception if the user is available in the session). When the debug mode is disabled, this will also refresh the user and if the session is not valid anymore for some reason (e.g. timeout, roles changed etc.), the ContextListener will remove the session and turn an otherwise stateless request/response into a stateful one.

-1 for this PR. If a firewall is configured as being stateful, the ContextListener should always run (as that's exactly what a stateful firewall is about). The right fix is to revert the change done in the ContextListener inhttps://github.com/symfony/symfony/pull/57372/files (other changes in the profiler code are fine, as the web debug toolbar needs to be compatible with stateless requests as it is a debug tool running on all pages)

If a route is explicitly configured as being stateless, it must either not perform any security checks (to never trigger lazy authentication) or perform it using a stateless firewall. Otherwise it cannot be stateless.

TokenStorage::getToken() is not necessarily used only in security checks. For example, the SecurityDataCollector will try to get the token.

Third party event listeners like Sentry also check if a token is available in order to enrich the logs with user information (if it is avaialble). The event listener of the Blameable Doctrine Extension also fetches the token on each request.

In those event listeners it would be possible to only callgetToken() if the request isnot stateless. But then again, authenticators in stateless firewalls may also set the token. Should every consumer ofTokenStorage::getToken() have the knowledge when this method can be called without an exception/warning?

There is a service calledsecurity.untracked_token_storage for use cases like Sentry's. I guess this is what should be used in most if not all these situations. To be confirmed of course.

Fixed by#58002