-
Notifications
You must be signed in to change notification settings - Fork 88
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
Problem with latest Symfony update and signed_cookie feature #312
Comments
@peter17 sure - in this specific project all cookies are included by setting the value to ['*']. I don't see a good generic solution right now without breaking the signing meachism (for example: storing the hash in a separate cookie file instead of the same one) |
I am thinking of an ugly solution that might work: on src/EventListener/SignedCookieListener.php replace the following block:
by
I believe it would work because I think this happens before the session_start() call of the NativeSessionStorage. If it didn't then an exception would have been thrown by NativeSessionStorage before I made my fix there. @Flo-JB Do you have a minimum code to reproduce the issue? Or can you try this on your project? I don't have time now to set up a project and test this... Thanks! |
@peter17 With that fix the cookie will be rewritten two times on each page call (surely not the perfect solution) but I think better than the other options. Another alternative would be more complex - I think of an overridable session_id check service instead of a hardcoded one which could be injected to NativeSessionStorage and could be replaced by the NelmioSecurityBundle. Big thanks for investigating this issue |
Good! I'd like the opinion of a maintainer of NelmioSecurityBundle before I make a PR with this change. Maybe they have better ideas or can comment the code above? Thanks in advance |
…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
After the latest Symfony update with this pull symfony/symfony#46249 Symfony refreshes the session on each page view because the implemented regex filter from the pull reequest fails because of the "." stored in the session cookie with signed_cookie feature enabled.
It would be easier to fix this one on Symfony side but maybe they keep the restricted check and an update in this library is needed...
The text was updated successfully, but these errors were encountered: