Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automagically flagging requests as stateless breaks valid session access #50715

Open
derrabus opened this issue Jun 20, 2023 · 12 comments
Open

Comments

@derrabus
Copy link
Member

Symfony version(s) affected

6.3.0

Description

There are already several comments on #48044 that indicate that that change is a BC break.

The main problem is that flagging a firewall stateless has never meant that each request that passed that firewall is meant to be stateless. It was merely a way to tell Symfony not to persist the authentication token in the session. The application could still have valid reasons to access the session later on. @tucksaun mentioned CSRF checks and flash messages as examples.

My problem is that #48044 changes the semantics of the firewall attribute stateless. This might be a surprise for applications that suddenly raise exceptions on routes that worked perfectly fine with Symfony 6.2. However, this exception is not raised with production settings as far as I can tell, so it's "only" the local dev environment that breaks.

I think that this behavior should've been opt-in, maybe with a deprecation layer.

How to reproduce

Declare a stateless firewall and access the session from a controller behind that firewall on a route with no explicit stateless setting.

Possible Solution

Revert #48044 or make that functionality opt-in.

Additional Context

In my case, my firewall was declared stateless because Symfony was not supposed to manage the session of the application. This is part of a typical temporary authenticator I use when migrating legacy applications to Symfony: I can make Symfony aware of the authenticated user without migrating the login form. If I wouldn't declare the firewall stateless, Symfony would remember the authenticated token which would break the logout mechanism of the legacy app.

I "solved" this in my project by declaring an event listener:

#[AsEventListener(priority: 1024)]
final class StatelessListener
{
    public function __invoke(RequestEvent $event): void
    {
        $event->getRequest()->attributes->set('_stateless', false);
    }
}

But that only works because we don't make use of that stateless route flag at all at the moment.

@stof
Copy link
Member

stof commented Jun 20, 2023

But that only works because we don't make use of that stateless route flag at all at the moment.

You could make your solution compatible with that by running your listener between the RouterListener (priority 32) and the FirewallListener (priority 8) instead of using priority 1024. This way, you could check whether the route already added this attribute in the request.

@leofeyer
Copy link
Contributor

It was merely a way to tell Symfony not to persist the authentication token in the session. The application could still have valid reasons to access the session later on.

I second this. IMHO the exception should only be thrown if the code tries to access any authentication related session data (e.g. the token) and not otherwise. Or is the idea that there must never be a session if a firewall or route is declared stateless?

@stof
Copy link
Member

stof commented Aug 24, 2023

When a request is declared as stateless, there must not be any usage of the session during the handling of that request. That's the whole point.
A request can actually have 3 states:

  • declared as not stateless
  • declared as stateless
  • no declaration on the request (this is treated as not stateless as far as the requirement is enforced, but this unconfigured state is relevant for other parts).

If a route declares the stateless flag, it declares that state for the request

When a firewall is marked as stateless, it has 2 impacts:

  • the ContextListener using the session to store the authentication token is disabled (meaning that your authenticator must be a stateless one able to authenticate each incoming request instead of relying on a previous request to a specific check path like form_login)
  • if the stateless state of the request is unknown at the time of executing the firewall, it will mark the request as stateless (making it easy for the common use case where parts of the app covered by a stateless firewall must be fully stateless)

So if you need a stateless firewall without making the request fully stateless (because you use the session for something else), the solution is to explicitly mark the request as not stateless before the firewall runs (see my previous comment)

@fritzmg
Copy link
Contributor

fritzmg commented Aug 24, 2023

  • if the stateless state of the request is unknown at the time of executing the firewall, it will mark the request as stateless (making it easy for the common use case where parts of the app covered by a stateless firewall must be fully stateless)

So if you need a stateless firewall without making the request fully stateless (because you use the session for something else), the solution is to explicitly mark the request as not stateless before the firewall runs (see my previous comment)

But that is the BC break. Being able to define a request (or route) as stateless is fine, but a stateless firewall should not automatically assume that all requests are stateless (only the authentication is).

@leofeyer
Copy link
Contributor

leofeyer commented Aug 24, 2023

When a request is declared as stateless, there must not be any usage of the session during the handling of that request.

@stof Thanks for clearing that up. 👍

@VincentLanglet
Copy link
Contributor

In my case, I get the warning

Session was used while the request was declared stateless.

when someone call a route /api/routeNotFound.

It renders the 404 not found page which use the session (for CSP nonce for instance).

Some advice were to put stateless: false on the concerned route. But how do we do when it's not a route, but an error page generated by Symfony. Shouldn't this route be ignored by the "automatically set stateless" PR ?

Is there a way to configure different error page for different firewall ?

@derrabus
Copy link
Member Author

If your error pages unconditionally access your session, none of your routes is stateless. 🤷🏻‍♂️

@javaDeveloperKid
Copy link
Contributor

javaDeveloperKid commented Nov 14, 2023

I've always thought that stateless firewall turns off session (you're not able to use session at all during that request). I have a flashback of the blog post that presented this as a performance feature (if stateless means there is no session then let's remove it)

@stof
Copy link
Member

stof commented Nov 14, 2023

@javaDeveloperKid Marking a firewall as stateless means that the firewall itself will not use the session anymore.
Then, we have an additional smart behavior based on that about marking the request as stateless which I explained in #50715 (comment).
Once a request is marked as stateless, the framework ensures that nothing used the session during the request handling so that it actually is stateless.
If your template uses the session to read flash messages, the template will prevent marking things as stateless.

@pkly
Copy link

pkly commented Mar 11, 2024

Hello, I just wanted to drop in and say that this broke my 5.4 -> 6.4 upgrade. Some of the tests started crashing because of the changed behavior and then another test started behaving weirdly when removing credentials for the next request (token stored in session).

Anyway yeah, I'd like this behavior to be optional if possible.

@tacman
Copy link
Contributor

tacman commented Mar 21, 2024

I'm getting the error when making a DELETE request to API platform. But it appears to actually do the delete (in the database).

I do have the symfony/security-bundle installed, but it's really a pretty generic app, symfony new --webapp (Symfony 7), then installing api platform and the symfony tools for security (e.g. make:user).

Any suggestions on how to debug this or configuring it differently?

curl 'https://127.0.0.1:8012/api/reactions/66'   -X 'DELETE'  | jq
{
  "@id": "/api/errors/500",
  "@type": "hydra:Error",
  "title": "An error occurred",
  "detail": "Session was used while the request was declared stateless.",
  "status": 500,
  "type": "/errors/500",
  "trace": [
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/event-dispatcher/Debug/WrappedListener.php",
      "line": 116,
      "function": "onKernelResponse",
      "class": "Symfony\\Component\\HttpKernel\\EventListener\\AbstractSessionListener",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/event-dispatcher/EventDispatcher.php",
      "line": 206,
      "function": "__invoke",
      "class": "Symfony\\Component\\EventDispatcher\\Debug\\WrappedListener",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/event-dispatcher/EventDispatcher.php",
      "line": 56,
      "function": "callListeners",
      "class": "Symfony\\Component\\EventDispatcher\\EventDispatcher",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php",
      "line": 127,
      "function": "dispatch",
      "class": "Symfony\\Component\\EventDispatcher\\EventDispatcher",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/http-kernel/HttpKernel.php",
      "line": 211,
      "function": "dispatch",
      "class": "Symfony\\Component\\EventDispatcher\\Debug\\TraceableEventDispatcher",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/http-kernel/HttpKernel.php",
      "line": 199,
      "function": "filterResponse",
      "class": "Symfony\\Component\\HttpKernel\\HttpKernel",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/http-kernel/HttpKernel.php",
      "line": 76,
      "function": "handleRaw",
      "class": "Symfony\\Component\\HttpKernel\\HttpKernel",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/http-kernel/Kernel.php",
      "line": 185,
      "function": "handle",
      "class": "Symfony\\Component\\HttpKernel\\HttpKernel",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/symfony/runtime/Runner/Symfony/HttpKernelRunner.php",
      "line": 35,
      "function": "handle",
      "class": "Symfony\\Component\\HttpKernel\\Kernel",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/vendor/autoload_runtime.php",
      "line": 29,
      "function": "run",
      "class": "Symfony\\Component\\Runtime\\Runner\\Symfony\\HttpKernelRunner",
      "type": "->"
    },
    {
      "file": "/home/tac/g/sites/pulse/public/index.php",
      "line": 5,
      "function": "require_once"
    }
  ],
  "hydra:title": "An error occurred",
  "hydra:description": "Session was used while the request was declared stateless."
}

@WubbleWobble
Copy link

WubbleWobble commented Apr 11, 2024

But that is the BC break. Being able to define a request (or route) as stateless is fine, but a stateless firewall should not automatically assume that all requests are stateless (only the authentication is).

Yes - this is an annoyance. We're using Google's IAP for authentication which works by passing us signed headers, and so whilst the authentication is stateless, the pages being served are anything but (CSRF tokens, flash bags etc).

On the plus side, at least I now know that I can fix it by marking a crap-ton of routes as stateless: false :)

(Edit: Actually can just mark all of the routes in question as not stateless en-masse via our attributes.yaml, so it's an easy fix!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants