Skip to content
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

[HttpFoundation] Update "[Session] Overwrite invalid session id" to only validate when files session storage is used #46678

Merged
merged 1 commit into from Jun 19, 2022

Conversation

alexpott
Copy link
Contributor

@alexpott alexpott commented Jun 15, 2022

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.

@carsonbot carsonbot added this to the 6.1 milestone Jun 15, 2022
@carsonbot carsonbot changed the title Revert "[HttpFoundation] [Session] Overwrite invalid session id" [HttpFoundation] Revert " [Session] Overwrite invalid session id" Jun 15, 2022
@alexpott
Copy link
Contributor Author

Also I think the original fix is a bit awkward because it results in session data loss - see the fail test runs on https://www.drupal.org/project/drupal/issues/3285696.

It's possible for a session handler to have it's own method for generating IDs - see SessionIdInterface and there are no restrictions about the characters that can return. What is valid is determined by how the session is stored. The a-z A-Z 0-9 , (comma) and - (minus) and length restrictions only exist for PHP's built in file session storage.

@alexpott
Copy link
Contributor Author

Slightly different approach prompted by @nicolas-grekas - targeting just the NativeFileSessionHandler or if somehow PHP's file session storage is being used.

@alexpott alexpott changed the title [HttpFoundation] Revert " [Session] Overwrite invalid session id" [HttpFoundation] Update "[Session] Overwrite invalid session id" to only validate when files session storage is used Jun 15, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.1, 4.4 Jun 19, 2022
…nly validate when files session storage is used
@nicolas-grekas
Copy link
Member

Thank you @alexpott.

@nicolas-grekas nicolas-grekas merged commit 07fa911 into symfony:4.4 Jun 19, 2022
@jevrard
Copy link

jevrard commented Jul 8, 2022

I use native PHP storage and default PHP save handler with configuration like this :

# framework.yml
framework:
    session:
        handler_id: null
        storage_factory_id: session.storage.factory.native
# php.ini
session.save_handler = files

Unfortunately, the getSaveHandlerNameof the SessionHandlerProxy will always return user, so an invalid session ID is not replaced by new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants