[HttpFoundation] [Session] Regenerate invalid session id #46249
Conversation
|
Hey! I think @BradJonesLLC has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
Link bug #45755 |
|
@nicolas-grekas Any idea about this? Thanks a lot in advance. Best regards |
|
I don't think changing a superglobal to accommodate some code is the correct approach. |
|
Thanks for your feedback. I understand your concern. I will try to find a better way! |
8c4b72b to
60154ce
Compare
|
@nicolas-grekas I propose a new way that does not involve mutating a superglobal: I use the native PHP functions to generate a new session ID. Therefore, session_start will work normally. Best regards |
|
NB: the Appveyor build failed with "Build execution time has reached the maximum allowed time for your plan (20 minutes)." I don't think this failure is relevant. I could not find how to retry the build. |
src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
Outdated
Show resolved
Hide resolved
60154ce to
14082d0
Compare
|
@nicolas-grekas thanks for your review, I applied the requested changes. I will attempt to write a test. Best regards |
14082d0 to
378dc13
Compare
378dc13 to
d8f84c7
Compare
|
@nicolas-grekas I added a test. It fails without my patch and passes with it. Best regards |
|
Thank you @peter17. |
|
After that update we got a problem in one of our projects regenerating the session on each page view. The new implemented regex fails each time because the newly created value in the session cookie is "wrong" too. The basic problem comes from the nelmio security plugin (signed_cookie feature). This feature extends the session cookie with a "." and an additional hash value. Especially the "." causes the regex to fail. I will create a ticket in the nelmio project - but leave this comment for others with similar problems |
|
@Flo-JB thanks for reporting this and for opening the issue in NelmioSecurityBundle What I don't understand is that the current issue was about avoiding an exception when |
|
I personally didn't notice any Symfony exception before. With the signd_cookie-feature enabled our session cookies get modified to s.th. like that: I didn't delve deeper in Nelmios code but I think they just append this verification hash after the session was started (In our case with a valid session_id) and put the modified value in the session cookie. Your check fails right now because you take the whole cookie contents (sessionid + separator + hash) without separating the session id out from that mixed value. |
|
This also causes issues for Drupal fwiw - https://www.drupal.org/project/drupal/issues/3285696 I'm not sure that the premise of this issue about which characters are allowed in a session ID or indeed used in a session ID is correct. See https://www.php.net/manual/en/function.session-id.php it says:
So we've limited the characters to the file session handler but we might not be using that. |
|
This gets even more interesting when the session ID as a cookie value - the legal characters there are, according to https://curl.se/rfc/cookie_spec.html, do not include a comma! |
…on id" to only validate when files session storage is used (alexpott) This PR was submitted for the 6.1 branch but it was squashed and merged into the 4.4 branch instead. Discussion ---------- [HttpFoundation] Update "[Session] Overwrite invalid session id" to only validate when files session storage is used | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fixes #46249 | License | MIT | Doc PR | - #46249 restricts the allowed characters in a session ID. Unfortunately this broke at least two open source projects the use the Symfony component. See nelmio/NelmioSecurityBundle#312 and https://www.drupal.org/project/drupal/issues/3285696 I think the change is not quite correct. It assumes that the valid characters in a session ID is consistent across all session handlers. It is not. See https://www.php.net/manual/en/function.session-id.php it says: >Depending on the session handler, not all characters are allowed within the session id. For example, the file session handler only allows characters in the range a-z A-Z 0-9 , (comma) and - (minus)! So we've limited the characters to the file session handler but we might not be using that. Commits ------- 12460fa [HttpFoundation] Update "[Session] Overwrite invalid session id" to only validate when files session storage is used
…ng or contains illegal characters (BrokenSourceCode) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [HttpFoundation] Prevent PHP Warning: Session ID is too long or contains illegal characters | Q | A | ------------- | --- | Branch? |4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #46777 | License | MIT This PR is intended to improve the changes made in the PR #46249 that doesn't check the max length of the session ID. To do this, I used the PHP ini directives below: - [`session.sid_length`](https://www.php.net/manual/en/session.configuration.php#ini.session.sid-length) (must be an integer between `22` and `256`) - [`session.sid_bits_per_character`](https://www.php.net/manual/en/session.configuration.php#ini.session.sid-bits-per-character) (must be an integer such as `4`, `5` or `6`) Commits ------- 8487950 [HttpFoundation] Prevent PHP Warning: Session ID is too long or contains illegal characters
Currently, having a PHPSESSID which does not match
/^[a-zA-Z0-9,\-]{1,123}$/(see https://www.php.net/manual/fr/function.session-start.php) will produce a php.WARNING and then a RuntimeException (please read #45755).I don't think there is a nice way to handle this so I propose to simply ignore invalid values.
With this PR, a value for PHPSESSID that does not match the regex will be ignored and a new session id will be generated. Then, the behavior will be the same as if no session existed, so a new session will be started and a new PHPSESSID will be defined.
It looks like Session storage is currently untested so I don't know how to test this...
Best regards