8000 [HttpKernel] AbstractSessionListener saves session for stateless request · Issue #50958 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] AbstractSessionListener saves session for stateless request #50958

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.

Sign up for GitHub

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

Jump to bottom
Closed
gndk opened this issue Jul 12, 2023 · 6 comments
Closed

[HttpKernel] AbstractSessionListener saves session for stateless request #50958

gndk opened this issue Jul 12, 2023 · 6 comments
Labels
Bug HttpKernel Status: Needs Review

Comments

@gndk
Copy link
Contributor
gndk commented Jul 12, 2023 •
edited
Loading

Symfony version(s) affected

6.3.1

Description

I've been seeing some session_write_close(): Failed to write session data with "Symfony\Component\HttpFoundation\Session\Storage\Handler\RedisSessionHandler" handler warnings in my Sentry for a specific route.

This is probably caused by Redis sessions being non-locking (fingers crossed for #4976).

So I set this route to stateless: true, but it still happens.

This route receives an AJAX request during a "normal" pageview. It does not itself use the session.

But as it is an AJAX request, the request includes the session cookie, which is probably why AbstractSessionListener.php#L95 and AbstractSessionListener.php#L108 evaluate to true.

I think this case might have been missed when _stateless was introduced in #35732.

How to reproduce

I wrote this test case for SessionListenerTest, which I believe should pass.

Currently, it fails with Symfony\Component\HttpFoundation\Session\Session::save() was not expected to be called..

public function testSessionNotSavedForStatelessRequest()
{
    $session = $this->createMock(Session::class);
    $session->expects($this->once())->method('isStarted')->willReturn(true);
    $session->expects($this->once())->method('getUsageIndex')->willReturn(0);

    $session->expects($this->never())->method('save');

    $listener = new SessionListener(new Container(), false);
    $kernel = $this->createMock(HttpKernelInterface::class);

    $request = new Request();
    $request->setSession($session);
    $request->attributes->set('_stateless', true);

    $listener->onKernelResponse(new ResponseEvent($kernel, $request, HttpKernelInterface::MAIN_REQUEST, new Response()));
}

Possible Solution

Don't save the session if the request is stateless.

The whole check for stateless request could be moved to the top of the listener.

I don't understand why the session should be saved in a stateless request, but there is a test case for it in SessionListenerTest.php#L785.

Maybe the exception/warning for using the session in a stateless request should be thrown without actually saving the session?

$session = $event->getRequest()->getSession();

if ($event->getRequest()->attributes->get('_stateless', false)) {
    if ($session->getUsageIndex() !== 0) {
        if ($this->debug) {
            throw new UnexpectedSessionUsageException('Session was used while the request was declared stateless.');
        }

        if ($this->container->has('logger')) {
            $this->container->get('logger')->warning('Session was used while the request was declared stateless.');
        }
    }

    return;
}

if ($session->isStarted()) {
    // ...

    $session->save();

    // ...
}

Additional Context

ErrorException: Warning: session_write_close(): Failed to write session data with "Symfony\Component\HttpFoundation\Session\Storage\Handler\RedisSessionHandler" handler
#19 /vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php(259): Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::save
#18 /vendor/symfony/http-foundation/Session/Session.php(171): Symfony\Component\HttpFoundation\Session\Session::save
#17 /vendor/symfony/http-kernel/EventListener/AbstractSessionListener.php(134): Symfony\Component\HttpKernel\EventListener\AbstractSessionListener::onKernelResponse
#16 /vendor/symfony/event-dispatcher/EventDispatcher.php(260): Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}
#15 /vendor/symfony/event-dispatcher/EventDispatcher.php(220): Symfony\Component\EventDispatcher\EventDispatcher::callListeners
#14 /vendor/symfony/event-dispatcher/EventDispatcher.php(56): Symfony\Component\EventDispatcher\EventDispatcher::dispatch
#13 /vendor/symfony/http-kernel/HttpKernel.php(199): Symfony\Component\HttpKernel\HttpKernel::filterResponse
#12 /vendor/symfony/http-kernel/HttpKernel.php(187): Symfony\Component\HttpKernel\HttpKernel::handleRaw
#11 /vendor/symfony/http-kernel/HttpKernel.php(74): Symfony\Component\HttpKernel\HttpKernel::handle
#10 /vendor/symfony/http-kernel/Kernel.php(197): Symfony\Component\HttpKernel\Kernel::handle
#9 /vendor/symfony/http-kernel/HttpCache/SubRequestHandler.php(86): Symfony\Component\HttpKernel\HttpCache\SubRequestHandler::handle
#8 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(473): Symfony\Component\HttpKernel\HttpCache\HttpCache::forward
#7 /vendor/symfony/framework-bundle/HttpCache/HttpCache.php(68): Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache::forward
#6 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(273): Symfony\Component\HttpKernel\HttpCache\HttpCache::pass
#5 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(287): Symfony\Component\HttpKernel\HttpCache\HttpCache::invalidate
#4 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(210): Symfony\Component\HttpKernel\HttpCache\HttpCache::handle
#3 /vendor/symfony/http-kernel/Kernel.php(188): Symfony\Component\HttpKernel\Kernel::handle
#2 /vendor/symfony/runtime/Runner/Symfony/HttpKernelRunner.php(35): Symfony\Component\Runtime\Runner\Symfony\HttpKernelRunner::run
#1 /vendor/autoload_runtime.php(29): require_once
#0 /public/index.php(7): null
@gndk gndk added the Bug label Jul 12, 2023
@MatTheCat
Copy link
Contributor

You may have misunderstood stateless routes: they won't prevent the session being used, but will warn you if so: https://symfony.com/doc/current/routing.html#stateless-routes

@gndk
Copy link
Contributor Author
gndk commented Jul 13, 2023 •
edited
Loading

No, I understand that. But I'm not using the session, neither is Symfony, as you can see from the stack trace. So why does it need to be saved and how else can I prevent that? Why does the warning/exception come after instead of before the save?

@MatTheCat
Copy link
Contributor

From https://symfony.com/doc/current/session.html

Sessions are only started if you read or write from it.

The stack trace shows that Session::save is called, which only happens if the session has been started, which means you read or wrote from it.

@gndk
Copy link
Contributor Author
gndk commented Jul 13, 2023 •
edited
Loading

From https://symfony.com/doc/current/session.html

Sessions are only started if you read or write from it.

The stack trace shows that Session::save is called, which only happens if the session has been started, which means you read or wrote from it.

I didn't read or write to the session. This is the whole controller (for Sentry JS tunnel). It dispatches async message through SQS messenger.

#[Route(
    path: '/api/sentry',
    name: RouteName::API_SENTRY_TUNNEL,
    methods: ['POST'],
    stateless: true,
)]
#[IsGranted('PUBLIC_ACCESS')]
public function __invoke(Request $request): Response
{
    $this->messageBus->dispatch(new SendSentryEvent($request->getContent()));

    return new Response();
}

Maybe #[IsGranted('PUBLIC_ACCESS')] is the problem.. 🤔

@MatTheCat
Copy link
Contributor

Maybe #[IsGranted('PUBLIC_ACCESS')] is the problem.. 🤔

Pretty sure this is the case if your route is under a stateful firewall (because the token will be fetched from the session).
The profiler exposes session usages in its “request” panel so you may be able to confirm.

@gndk
Copy link
Contributor Author
gndk commented Jul 13, 2023 •
edited
Loading

Maybe #[IsGranted('PUBLIC_ACCESS')] is the problem.. 🤔

Pretty sure this is the case if your route is under a stateful firewall (because the token will be fetched from the session). The profiler exposes session usages in its “request” panel so you may be able to confirm.

I had a suspicion it could be related to the firewall...

That solved it.

I added an additional stateless firewall for my /api routes:

'api' => [
    'stateless' => true,
    'pattern' => '^/api',
],

That wasn't enough though. I have one event subscriber that still called the session, because $request->hasSession()) evaluates to true in the AJAX request with session cookie.

I added another check there, and now it's working without the "session used on stateless route" warning:

if ($this->security->getFirewallConfig($request)?->isStateless() ?? false) {
    return;
}

Thanks for the help @MatTheCat!

@gndk gndk closed this as completed Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@MatTheCat @carsonbot @gndk
0