Skip to content

NativeSessionStorage::regenerate bug #8460

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
Bluestart83 opened this issue Jul 9, 2013 · 3 comments
Closed

NativeSessionStorage::regenerate bug #8460

Bluestart83 opened this issue Jul 9, 2013 · 3 comments
Labels
HttpFoundation

Comments

@Bluestart83
Copy link

This fix is about 6a15a3a
Also refer to: #7380

This fix is not valid because it generate another bug if _SESSION is not set! ($_SESSION is not defined)

I've modified code to this:

   $ret = session_regenerate_id($destroy);

    // workaround for https://bugs.php.net/bug.php?id=61470 as suggested by David Grudl
    session_write_close();
   $sessionSet = isset($_SESSION);
    if($sessionSet) {
        $backup = $_SESSION;
    }
    session_start();
    if($sessionSet) {
        $_SESSION = $backup;
    }

Please fix it.

The case happened for me when having no cookie/ PHPSESSID, using FOSUserBundle on http://BASE_URL/register/confirm/TOKEN

Thanks
Bluestart83 referenced this issue Jul 9, 2013
@fabpot
merged branch fabpot/session-regenerate-fix (PR #8270)
6a15a3a 
This PR was merged into the 2.2 branch.

Discussion
----------

[HttpFoundation] fixed issue with session_regenerate_id (closes #7380)

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #7380
| License       | MIT
| Doc PR        | n/a

Commits
-------

77f2aa8 [HttpFoundation] fixed issue with session_regenerate_id (closes #7380)
@lyrixx lyrixx mentioned this issue Jul 11, 2013
Closed
fabpot added a commit that referenced this issue Jul 15, 2013
@fabpot
merged branch lyrixx/fix-8460 (PR #8479)
579dbe2 
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes #8479).

Discussion
----------

 	[HttpFoundation] Fixed NativeSessionStorage:regenerate when does not exists

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #8460
| License       | MIT
| Doc PR        | -

**warning** I did not try to reproduce this bug, so I am not sure it fixes the issue. I just open a PR with the suggested patch. Tests for http foundation are OK. I am not sur it's the best way to fix this issue as I do not understand why symfony reach this code if $_SESSION is not set.

Commits
-------

f5d59fb [HttpFoundation] Fixed NativeSessionStorage:regenerate when  does not exists
@fabpot fabpot closed this as completed Jul 15, 2013
@advancingu advancingu mentioned this issue Jul 18, 2013
Closed
@Bluestart83
Copy link
Author

Please reopen the bug!
It's still not working as I tell to you this bug happen when validating a token http://ifbnserver/fr/register/confirm/VALIDATION_TOKEN with FOSUserBundle when no PHPSESSIONID cookie is set!!!
(friendsofsymfony/user-bundle", "version": "v1.3.2)

With your fix I have now:
"Notice: A session had already been started - ignoring session_start() "

If session is not already started do not start one.

The right fix is that is working for me:

$ret = session_regenerate_id($destroy);
if(isset($_SESSION)) {
// workaround for https://bugs.php.net/bug.php?id=61470 as suggested by David Grudl
session_write_close();

    $backup = $_SESSION;
    session_start();
    $_SESSION = $backup;

}

@fabpot fabpot reopened this Jul 22, 2013
@simonchrz
Copy link
Contributor

fixed in symfony 2.3.4 👍

@Tobion Tobion closed this as completed Aug 29, 2013
@lancergr lancergr mentioned this issue Sep 9, 2013
Closed
fabpot added a commit that referenced this issue Sep 13, 2013
@fabpot
merged branch lancergr/ticket_8460 (PR #8969)
1271a4c 
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes #8969).

Discussion
----------

[HttpFoundation] NativeSessionStorage regenerate

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #8460
| License       | MIT
| Doc PR        |

Since session_start is called by the regenerate function, then the 'started' flag of NativeSessionStorage have to be set to true. Otherwise, the variable $_SESSION is initiated and the exception "Failed to start the session: already started by PHP ($_SESSION is set)." is thrown.

This can be reproduced by clearing the session data (cookies) before authenticating with a method that does not require csrf (eg. using the confirmation link of FOSUserBundle).

Commits
-------

7a0eeb3 [HttpFoundation] NativeSessionStorage regenerate
@mark-newman
Copy link

For reference it's actually 2.3.5 that contains the workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @Tobion @mark-newman @simonchrz @Bluestart83