Skip to content

Session ID changing with custom session handler in PHP 8.2.0RC3 #9668

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
campbell-m opened this issue Oct 4, 2022 · 8 comments
Closed

Session ID changing with custom session handler in PHP 8.2.0RC3 #9668

campbell-m opened this issue Oct 4, 2022 · 8 comments

Comments

@campbell-m
Copy link

Description

I'm afraid I can't yet reproduce a minimal test case after many hours of trying. However in the meantime I thought I'd report what I'm seeing anyway as we're getting close to the PHP 8.2.0 release date and in case someone can recognise what might be going wrong.

My application uses a custom session handler implementing the SessionHandlerInterface to store sessions in the database. It has worked fine with PHP 8.0, 8.1 and all the alpha, beta and RC versions up to and including RC2. Then when I installed RC3, sessions suddenly stopped working and the value of $_SESSION on one page was not the same as what had been written on the previous page. Adding some debugging I can see that the problem is that in RC3 the value of $id passed to read() when the session starts is not the same as that passed to write() when the previous page exits. However in RC2, with the exact same code, the two $id values are identical.

Any ideas?

PHP Version

PHP 8.2.0RC3

Operating System

Windows 11 IIS

@cmb69
Copy link
Member

cmb69 commented Oct 4, 2022

I assume this is closely related to #9583, possibly a duplicate. Would be great if you could check @Girgias's patch (#9583), although prebuilt Windows binaries are not available.

@campbell-m
Copy link
Author

Many thanks! I don't think I have the ability or time to build some Windows binaries. However I can confirm that adding

public function validateId($key){return true;}

to my custom session handler solves the problem. And also, now that I have seen the documentation for validateId(), I can produce a minimal test case by adding

ini_set('session.use_strict_mode', '1');
session_name('TESTING');

to my test. So it looks very much like a duplicate.

Are you interested in a test program, or is it not worth it? And should my session handler have a proper implementation of validateId(), or is that not required in theory and I should just wait for RC4?

campbell-m added a commit to meeting-room-booking-system/mrbs-code that referenced this issue Oct 4, 2022
@campbell-m
Copy link
Author

I've found another problem, which may be another symptom of the same bug. I get the same problem (ie session_id changing each time) even if I set a custom session handler and then immediately reset it to the default session handler. See the test code below. Note that the custom session handler has a validateId() method in order to circumvent the previous manifestation of this bug. With PHP 8.2.0RC2 the program behaves as expected and gives error log output such as

[05-Oct-2022 14:26:34 UTC] MySessionHandler::__construct
[05-Oct-2022 14:26:34 UTC] 09ebe2053a3db0620049c84e85751fbc
[05-Oct-2022 14:26:34 UTC] MySessionHandler::__construct
[05-Oct-2022 14:26:34 UTC] 09ebe2053a3db0620049c84e85751fbc
[05-Oct-2022 14:26:34 UTC] MySessionHandler::__construct
[05-Oct-2022 14:26:34 UTC] 09ebe2053a3db0620049c84e85751fbc
...

But with PHP 8.2.0RC3 the session id changes each time:

[05-Oct-2022 14:24:09 UTC] MySessionHandler::__construct
[05-Oct-2022 14:24:09 UTC] 4bd0b6ad45192984434cec1715fe0f41
[05-Oct-2022 14:24:09 UTC] MySessionHandler::__construct
[05-Oct-2022 14:24:09 UTC] 5d77fcfe55c1ad5b1ccb882a3941ebab
[05-Oct-2022 14:24:09 UTC] MySessionHandler::__construct
[05-Oct-2022 14:24:09 UTC] 71bbc8fe517b6bd48f0f0ab4eae479bb

Here is the test code (apologies for the infinite loop):

<?php

error_reporting(-1);

class MySessionHandler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface
{
  public function __construct()
  {
    error_log(__METHOD__);
  }

  public function open($path, $name): bool
  {
    error_log(__METHOD__ . " $path $name");
    return true;
  }

  public function close(): bool
  {
    return true;
  }

  #[\ReturnTypeWillChange]
  public function read($id)
  {
    error_log(__METHOD__ . " $id");
    return 'foo|s:3:"bar";';
  }

  public function write($id, $data): bool
  {
    error_log(__METHOD__ . " $id $data");
    return true;
  }

  public function destroy($id): bool
  {
    error_log(__METHOD__ . " $id");
    return true;
  }

  #[\ReturnTypeWillChange]
  public function gc($max_lifetime)
  {
    error_log(__METHOD__ . " $max_lifetime");
    return true;
  }
  
  public function validateId($key) : bool
  {
    error_log(__METHOD__ . " $key");
    return true;
  }
  
  public function updateTimestamp($id, $data) : bool
  {
    error_log(__METHOD__ . " $id $data");
    return true;
  }

}

$handler = new MySessionHandler();
session_set_save_handler($handler, true);

$handler = new SessionHandler();
session_set_save_handler($handler, true);

ini_set('session.use_strict_mode', '1');
session_name('TESTING');
session_start();

error_log(session_id());

header("Location: " . $_SERVER['PHP_SELF']);

@warenzema
Copy link

@campbell-m I originally opened #9583, and what you are seeing matches with what is expected when Girgias attempted to fix the problem by simply making validateId always return false. It passed tests, so they thought it was fine, but apparently test coverage is poor.
I replied that their changed introduced a new bug (which is what you've encountered), and they re-fixed it. (See my last comment on #9583 )
Based on the timing of his commits, my guess is that what has happened is that RC3 has the incorrect fix, and the (hopefully) real fix won't be available until RC4.

@warenzema
Copy link

@campbell-m
Ok, I just confirmed that only the wrong-fix is present in RC3

php-src/ext/session/session.c

Lines 1070 to 1074 in e5ac246

/* Dummy PS module function */
/* We consider any ID valid, so we return FAILURE to indicate that a session doesn't exist */
PHPAPI zend_result php_session_validate_sid(PS_VALIDATE_SID_ARGS) {
return FAILURE;
}

Note how 1073 is FAILURE, but it is changed back to SUCCESS in the real-fix here: Girgias@1f81a3b

@campbell-m
Copy link
Author

@warenzema
Thanks. I will wait for RC4 and retest.

@cmb69
Copy link
Member

cmb69 commented Oct 6, 2022

Indeed, that fix will only be in the next RCs (8.0.25RC1, 8.1.12RC1 and 8.2.0RC4).

Anyhow, I've double-checked that #9638 fixes @campbell-m's test script, so closing as duplicate of #9583.

@campbell-m
Copy link
Author

I can confirm that RC4 fixes the test script above.

@cmb69 cmb69 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants