8000 [HttpFoundation] Fix invalid ID not regenerated with native PHP file sessions · Pull Request #47130 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

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 Aug 1, 2022
Merged

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

merged 1 commit into from Aug 1, 2022

Conversation

@ghost
Copy link
@ghost ghost commented Jul 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 the SessionHandlerProxy 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 in 818d4dd

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

But inside the NativeSessionStorage class, the code create an instance of StrictSessionHandler that doesn't inherit from \SessionHandler and is passed to the SessionHandlerProxy constructor.

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

Lines 422 to 424 in 818d4dd

} elseif (!$saveHandler instanceof AbstractProxy) {
$saveHandler = new SessionHandlerProxy(new StrictSessionHandler(new \SessionHandler()));
}

Therefore, we could create a isWrapper() method inside the StrictSessionHandler class to check if the wrapped handler is an internal PHP session handler (\SessionHandler), just like AbstractProxy::isWrapper().

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

@carsonbot carsonbot added this to the 4.4 milestone Jul 30, 2022
@ghost ghost mentioned this pull request Jul 30, 2022
Closed
derrabus
derrabus reviewed Jul 31, 2022
View reviewed changes
Copy link
Member
@derrabus 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.

@ghost
Copy link
Author
ghost commented Jul 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
nicolas-grekas reviewed Aug 1, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas 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

ghost
ghost commented Aug 1, 2022
View reviewed changes
Copy link
Author
@ghost 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? Like isInternalWrapper, 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;
    }

nicolas-grekas
nicolas-grekas approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas 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.

I'm fine with isWrapper: this is what AbstractProxy uses already for the same thing

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 1, 2022

Thank you @brokensourcecode.

@nicolas-grekas nicolas-grekas merged commit 6c0f7da into symfony:4.4 Aug 1, 2022
This was referenced Aug 26, 2022
Merged
Merged
Merged
Merged
@fabpot fabpot mentioned this pull request Sep 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

3 participants

@nicolas-grekas @derrabus @carsonbot
0