10000 [HttpFoundation] Fix TypeError on null `$_SESSION` in `NativeSessionStorage::save()` by chalasr · Pull Request #46808 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Fix TypeError on null $_SESSION in NativeSessionStorage::save() #46808

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
Merged
merged 1 commit into from
Jul 6, 2022
Merged

[HttpFoundation] Fix TypeError on null $_SESSION in NativeSessionStorage::save() #46808

merged 1 commit into from
Jul 6, 2022

Conversation

chalasr
Copy link
Member
@chalasr chalasr commented Jun 29, 2022 •
edited
Loading
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

When sending concurrent requests via ajax async to a route pointing to a controller requiring an authenticated user through a stateful - session-based - firewall that calls SessionInterface::save(), it happens that $_SESSION is null under some conditions which causes the following error on PHP 8.1:

Exception 'TypeError' with message 'array_keys(): Argument #1 ($array) must be of type array, null given' in /app/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php:246
…app/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php (246)
…age::save called at /app/vendor/symfony/http-foundation/Session/Session.php (198)

The issue prevents me from upgrading to PHP 8.1 in a project I'm working on with @jwage.

@carsonbot carsonbot added this to the 4.4 milestone Jun 29, 2022
@chalasr
Copy link
Member Author
chalasr commented Jun 29, 2022

Build failures unrelated.

@jwage
Copy link
Contributor
jwage commented Jun 29, 2022

Thanks. This will definitely fix the issue for me but I am not sure why $_SESSION is null and I can't reproduce it. I only see the error stacktrace in newrelic. I think it was always potentially null sometimes prior to PHP 8.x too and array_keys() just changed to no longer accept null. I am going to keep trying to reproduce it in my app.

@jwage
Copy link
Contributor
jwage commented Jun 29, 2022 •
edited
Loading

I finally figured out how to reproduce this. If you delete the session cookie PHPSESSID and then hit an action that calls $session->save() on the very first line of the action, you will get the following:

Screen Shot 2022-06-29 at 2 57 51 PM

This error is different in my development environment but in production $_SESSION would be null and would error when it gets passed to array_keys() on line 246 in NativeSessionStorage.php.

The session does get successfully recreated from the remember me cookie, but $_SESSION is still null on that first request in the controller and when you send a 2nd request everything works correctly.

@jwage
Copy link
Contributor
jwage commented Jun 29, 2022 •
edited
Loading

Just for some added context. This was happening before in PHP 7.4 too, but it only started throwing an exception in PHP 8. In PHP 7.4 it was only a warning hence why I didn't see this error show up in production NewRelic.

$ php7.4 test.php
PHP Warning:  array_keys() expects parameter 1 to be array, null given in /data/repositories/traderspost/test.php on line 3

$ php8.1 test.php
PHP Fatal error:  Uncaught TypeError: array_keys(): Argument #1 ($array) must be of type array, null given in /data/repositories/traderspost/test.php:3
Stack trace:
#0 /data/repositories/traderspost/test.php(3): array_keys()
#1 {main}
  thrown in /data/repositories/traderspost/test.php on line 3

@jwage
Copy link
Contributor
jwage commented Jun 29, 2022 •
edited
Loading

Now that I understand the problem a little more, I am not sure if the above patch is the right fix. When you use remember me cookie and session cookie expires, remember me cookie should recreate the session which it does, but it seems like it happens at the end of the request lifecycle so anything that calls SessionInterface::save() before $_SESSION is recreated will fail. Should something be reinstantiating $_SESSION earlier in the request lifecycle?

As a temporary workaround in my app, I am testing this. It does fix the issue but I am not sure yet if this will have any negative side effects:

<?php

declare(strict_types=1);

namespace App\EventListener\Session;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class FixSessionGlobalVariableSubscriber implements EventSubscriberInterface
{
    /**
     * @return array<string, string>
     */
    public static function getSubscribedEvents(): array
    {
        return [KernelEvents::REQUEST => 'onKernelRequest'];
    }

    public function onKernelRequest(RequestEvent $event): void
    {
        if (! $event->isMainRequest()) {
            return;
        }

        if (! isset($_SESSION)) {
            $_SESSION = [];
        }
    }
}

@chalasr
Copy link
Member Author
chalasr commented Jul 4, 2022 •
edited
Loading

@jwage Unless I'm mistaken the issue is only with save() and should be solved with the proposed patch. Apart from that, I'd say it's not Symfony's job to initialize the $_SESSION superglobal (which should never be used directly in userland applications anyway).

@jwage
Copy link
Contributor
jwage commented Jul 5, 2022

@chalasr that makes sense then. Thanks!

@nicolas-grekas
Copy link
Member

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit 2670125 into symfony:4.4 Jul 6, 2022
@chalasr chalasr deleted the handle-null-session branch July 6, 2022 14:22
This was referenced Jul 29, 2022
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug HttpFoundation Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @jwage @nicolas-grekas @derrabus @carsonbot
0