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] Do not swallow trailing = in cookie value #51819

Merged

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Oct 2, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #51814
License MIT

cc @pschultz as you opened the bug report

@OskarStark OskarStark self-assigned this Oct 2, 2023
@carsonbot carsonbot added this to the 5.4 milestone Oct 2, 2023
@OskarStark OskarStark changed the title [HttpFoundation] Do not swallow trailing equal sign in cookie value [HttpFoundation] Do not swallow trailing = in cookie value Oct 2, 2023
@OskarStark
Copy link
Contributor Author

Or should this be fixed inside HeaderUtils::split() method?

@stof
Copy link
Member

stof commented Oct 3, 2023

I guess we have similar bugs in other places using HeaderUtils::split with multiple separator (i.e. all of them). That's because HeaderUtils considers that all separators are list separators. But = is a key-value separator.

@OskarStark
Copy link
Contributor Author

So do you have a proposal on where to fix this?

@stof
Copy link
Member

stof commented Oct 3, 2023

Well, HeaderUtils should be the place to fix it, but not sure we can fix it with the existing signature (unless we hardcode a special behavior for =).

@nicolas-grekas
Copy link
Member

The bug is clearly in HeaderUtils, see

[[['foo_cookie', 'foo=='], ['expires', 'Tue, 22-Sep-2020 06:27:09 GMT'], ['path', '/']], 'foo_cookie=foo==; expires=Tue, 22-Sep-2020 06:27:09 GMT; path=/', ';='],

foo_cookie=foo== is correctly parsed while foo_cookie=foo= isn't

@OskarStark
Copy link
Contributor Author

Will check on Monday 👍🏻

@OskarStark OskarStark force-pushed the bug/cookie-trailing-equal-sign branch from 7cf16b0 to 360e0ad Compare October 9, 2023 09:47
@OskarStark
Copy link
Contributor Author

Review please

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

The whole logic is opaque to me 😓

src/Symfony/Component/HttpFoundation/HeaderUtils.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/HeaderUtils.php Outdated Show resolved Hide resolved
@OskarStark OskarStark force-pushed the bug/cookie-trailing-equal-sign branch from f2d7a47 to ed51a41 Compare October 13, 2023 10:51
@nicolas-grekas nicolas-grekas force-pushed the bug/cookie-trailing-equal-sign branch 2 times, most recently from 469f98b to 59d6acf Compare October 17, 2023 09:23
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I rewrote the implementation, it now makes sense, at least to me :)
AND, it handles more edge cases.

@OskarStark
Copy link
Contributor Author

Thank you Nicolas!! 👏

@@ -46,13 +46,15 @@ public static function provideHeaderToSplit(): array

[[['foo_cookie', 'foo=1&bar=2&baz=3'], ['expires', 'Tue, 22-Sep-2020 06:27:09 GMT'], ['path', '/']], 'foo_cookie=foo=1&bar=2&baz=3; expires=Tue, 22-Sep-2020 06:27:09 GMT; path=/', ';='],
[[['foo_cookie', 'foo=='], ['expires', 'Tue, 22-Sep-2020 06:27:09 GMT'], ['path', '/']], 'foo_cookie=foo==; expires=Tue, 22-Sep-2020 06:27:09 GMT; path=/', ';='],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
[[['foo_cookie', 'foo=='], ['expires', 'Tue, 22-Sep-2020 06:27:09 GMT'], ['path', '/']], 'foo_cookie=foo==; expires=Tue, 22-Sep-2020 06:27:09 GMT; path=/', ';='],
[[['foo_cookie', 'foo==='], ['expires', 'Tue, 22-Sep-2020 06:27:09 GMT'], ['path', '/']], 'foo_cookie=foo===; expires=Tue, 22-Sep-2020 06:27:09 GMT; path=/', ';='],
[[['foo_cookie', 'foo=='], ['expires', 'Tue, 22-Sep-2020 06:27:09 GMT'], ['path', '/']], 'foo_cookie=foo==; expires=Tue, 22-Sep-2020 06:27:09 GMT; path=/', ';='],

Does this pass as well?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@nicolas-grekas
Copy link
Member

Thank you @OskarStark.

@nicolas-grekas nicolas-grekas merged commit a0df509 into symfony:5.4 Oct 17, 2023
7 of 11 checks passed
@OskarStark OskarStark deleted the bug/cookie-trailing-equal-sign branch October 17, 2023 11:42
@OskarStark
Copy link
Contributor Author

Thank you, as you did the reimplementation 👍

@fabpot fabpot mentioned this pull request Oct 21, 2023
@fabpot fabpot mentioned this pull request Oct 29, 2023
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