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

Disallow invalid characters in session.name #27246

Merged
merged 1 commit into from May 17, 2018

Conversation

ostrolucky
Copy link
Contributor

@ostrolucky ostrolucky commented May 12, 2018

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

@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

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


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

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
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
@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
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
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