Skip to content

Disallow invalid characters in session.name #27246

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
Merged
merged 1 commit into from
May 17, 2018
Merged

Disallow invalid characters in session.name #27246

merged 1 commit into from
May 17, 2018

Conversation

ostrolucky
Copy link
Contributor

@ostrolucky ostrolucky commented May 12, 2018 •
edited
Loading

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27023
License MIT
Doc PR

PHP saves cookie with correct name, but upon deserialization to
$_COOKIE, it replaces "." characters with "_".

This is probably also reason why \SessionHandler is not able to find
a session.

https://harrybailey.com/2009/04/dots-arent-allowed-in-php-cookie-names/
https://bugs.php.net/bug.php?id=75883

@ostrolucky ostrolucky force-pushed the session-cookie-dots branch from 10cbf6c to 1548744 Compare May 12, 2018 19:29
@nicolas-grekas
Copy link
Member

Good idea.
I'd suggest to be bullet proof using parse_str for normalization:
php > parse_str('a.b', $a);
php > print_r($a);
Array
(
[a_b] =>
)

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 12, 2018
@ostrolucky
Copy link
Contributor Author

ostrolucky commented May 12, 2018 •
edited
Loading

Not sure what do you mean. Can you be more specific? Should I change this validation approach to just normalization approach? Frankly, I would like that more, so I don't have to do that by myself in userspace.

Some cases are also handled internally by php's setcookie

ErrorException:
Warning: Cookie names cannot contain any of the following '=,; \t\r\n\013\014'

  at vendor\symfony\http-foundation\Session\Storage\Handler\AbstractSessionHandler.php:148

@nicolas-grekas
Copy link
Member

Instead of checking for some chars (there are more that are turned to an underscore), I was thinking about doing the check like this:
parse_str($name, $parsedName);
if (array($name => '') !== $parsedName) => the name is invalid

nicolas-grekas
nicolas-grekas reviewed May 13, 2018
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

return implode('&', array_keys($parsed)) !== (string) $v;
})
->thenInvalid('Session name %s contains illegal character(s)')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing double quotes around %s
about the implode, this means & is valid in a session name? how strange is that :)
but OK, that's because cookie are not encoded using URL queries.
This also means we should exclude ;, isn't it?

Copy link
Contributor Author

@ostrolucky ostrolucky May 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config quotes it by type itself, if I did it, it would be double quoted. Yes, & is valid. No need to exclude ; and further complicate this closure, rest of the chars are handled by setcookie call. As long as developer is warned, it's all good.

Got any pointers why appveyor fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appveryor fails for some reason on ' char even though it's valid... I've just removed it from test case

@ostrolucky ostrolucky changed the title Disallow "." in session.name Disallow invalid characters in session.name May 13, 2018
@ostrolucky ostrolucky force-pushed the session-cookie-dots branch 2 times, most recently from abc19c2 to 56646ad Compare May 13, 2018 21:45
@ostrolucky
Disallow illegal characters like "." in session.name
16ebb43 
PHP saves cookie with correct name, but upon deserialization to
$_COOKIE, it replaces some characters, e.g. "." becomes "_".

This is probably also reason why \SessionHandler is not able to find
a session.

https://harrybailey.com/2009/04/dots-arent-allowed-in-php-cookie-names/
https://bugs.php.net/bug.php?id=75883
@ostrolucky ostrolucky force-pushed the session-cookie-dots branch from 56646ad to 16ebb43 Compare May 13, 2018 21:57
@fabpot
Copy link
Member

fabpot commented May 17, 2018

Thank you @ostrolucky.

@fabpot fabpot merged commit 16ebb43 into symfony:2.7 May 17, 2018
fabpot added a commit that referenced this pull request May 17, 2018
@fabpot
bug #27246 Disallow invalid characters in session.name (ostrolucky)
e98ce72 
This PR was merged into the 2.7 branch.

Discussion
----------

Disallow invalid characters in session.name

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

PHP saves cookie with correct name, but upon deserialization to
`$_COOKIE`, it replaces "." characters with "_".

This is probably also reason why \SessionHandler is not able to find
a session.

https://harrybailey.com/2009/04/dots-arent-allowed-in-php-cookie-names/
https://bugs.php.net/bug.php?id=75883

Commits
-------

16ebb43 Disallow illegal characters like "." in session.name
This was referenced May 21, 2018
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
4 participants
@ostrolucky @nicolas-grekas @fabpot @carsonbot