NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpFoundation] Fix invalid ID not regenerated with native PHP file sessions#47130

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:4.4fromunknown repository

Aug 1, 2022

Merged

[HttpFoundation] Fix invalid ID not regenerated with native PHP file sessions#47130

nicolas-grekas merged 1 commit intosymfony:4.4fromunknown repository

Aug 1, 2022

Conversation

Copy link

ghost commentedJul 30, 2022•
edited by ghost
Loading

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#46993,#47126
License	MIT

Inside theSessionHandlerProxy class, the code defines$this->saveHandlerName to\ini_get('session.save_handler') when$handler is an instance of\SessionHandler.

symfony/src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/SessionHandlerProxy.php

Lines 24 to 25 in818d4dd

	$this->wrapper =$handlerinstanceof \SessionHandler;
	$this->saveHandlerName =$this->wrapper ?\ini_get('session.save_handler') :'user';

But inside theNativeSessionStorage class, the code create an instance ofStrictSessionHandler that doesn't inherit from\SessionHandler and is passed to theSessionHandlerProxy constructor.

symfony/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

Lines 422 to 424 in818d4dd

	}elseif (!$saveHandlerinstanceof AbstractProxy) {
	$saveHandler =newSessionHandlerProxy(newStrictSessionHandler(new \SessionHandler()));
	}

Therefore, we could create aisWrapper() method inside theStrictSessionHandler class to check if the wrapped handler is an internal PHP session handler (\SessionHandler), just likeAbstractProxy::isWrapper().

That's the only solution I have in mind right now.

carsonbot added Status: Needs Review Bug HttpFoundation labels

Jul 30, 2022

carsonbot added this to the4.4 milestone

Jul 30, 2022

ghost mentioned this pull request

Jul 30, 2022

[HttpFoundation] PHP Warning:SessionHandler::read(): Session ID is too long or contains illegal characters.#47126

Closed

derrabus reviewed

Jul 31, 2022

View reviewed changes

Copy link

Member

derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you for your PR. Would it be possible to add a test that reproduces the bug you're attempting to fix? I'd like to make sure we don't reintroduce it in the future.

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/StrictSessionHandler.php OutdatedShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/StrictSessionHandler.phpShow resolvedHide resolved

Copy link

Author

ghost commentedJul 31, 2022

Thank you for your PR. Would it be possible to add a test that reproduces the bug you're attempting to fix? I'd like to make sure we don't reintroduce it in the future.

@derrabus Unit test done here:657f020

nicolas-grekas reviewed

Aug 1, 2022

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, here are some final notes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/StrictSessionHandler.php OutdatedShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/SessionHandlerProxy.phpShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Proxy/SessionHandlerProxyTest.php OutdatedShow resolvedHide resolved

ghost commented

Aug 1, 2022

View reviewed changes

Copy link

Author

ghost left a comment•
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe we should use a more intuitive name for the method? LikeisInternalWrapper,wrapsInternalHandler,hasInternalHandler. What do you think@nicolas-grekas? In my opinion, this makes the method more understandable.

    /**     * Returns true if this handler wraps an internal PHP session save handler using \SessionHandler.     *     * @internal     */-   public function isWrapper(): bool+   public function isInternalWrapper(): bool    {        return $this->handler instanceof \SessionHandler;    }

    /**     * Returns true if this handler wraps an internal PHP session save handler using \SessionHandler.     *     * @internal     */-   public function isWrapper(): bool+   public function wrapsInternalHandler(): bool    {        return $this->handler instanceof \SessionHandler;    }

    /**     * Returns true if this handler wraps an internal PHP session save handler using \SessionHandler.     *     * @internal     */-   public function isWrapper(): bool+   public function hasInternalHandler(): bool    {        return $this->handler instanceof \SessionHandler;    }