NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpFoundation] Do not swallow trailing`=` in cookie value#51819

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.4fromOskarStark:bug/cookie-trailing-equal-sign

Oct 17, 2023

Merged

[HttpFoundation] Do not swallow trailing`=` in cookie value#51819

nicolas-grekas merged 1 commit intosymfony:5.4fromOskarStark:bug/cookie-trailing-equal-sign

Oct 17, 2023

Conversation

Copy link

Contributor

OskarStark commentedOct 2, 2023•
edited
Loading

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 added Bug HttpFoundation labels

Oct 2, 2023

OskarStark self-assigned this

Oct 2, 2023

carsonbot added the Status: Needs Review label

Oct 2, 2023

carsonbot added this to the5.4 milestone

Oct 2, 2023

OskarStark requested review fromnicolas-grekas andstof

October 2, 2023 21:13

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

Copy link

ContributorAuthor

OskarStark commentedOct 3, 2023

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

Copy link

Member

stof commentedOct 3, 2023

I guess we have similar bugs in other places usingHeaderUtils::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.

Copy link

ContributorAuthor

OskarStark commentedOct 3, 2023

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

Copy link

Member

stof commentedOct 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=).

OskarStark added the Status: Needs Work label

Oct 3, 2023

Copy link

Member

nicolas-grekas commentedOct 6, 2023

The bug is clearly in HeaderUtils, see

symfony/src/Symfony/Component/HttpFoundation/Tests/HeaderUtilsTest.php

Line 48 inadcd3d0

[[['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 whilefoo_cookie=foo= isn't

Copy link

ContributorAuthor

OskarStark commentedOct 6, 2023

Will check on Monday 👍🏻

OskarStark force-pushed thebug/cookie-trailing-equal-sign branch from7cf16b0 to360e0adCompare

October 9, 2023 09:47

Copy link

ContributorAuthor

OskarStark commentedOct 9, 2023

Review please

nicolas-grekas reviewed

Oct 12, 2023

View reviewed changes

Copy link

Member

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 OutdatedShow resolvedHide resolved

OskarStark force-pushed thebug/cookie-trailing-equal-sign branch fromf2d7a47 toed51a41Compare

October 13, 2023 10:51

nicolas-grekas force-pushed thebug/cookie-trailing-equal-sign branch 2 times, most recently from469f98b to59d6acfCompare

October 17, 2023 09:23

nicolas-grekas approved these changes

Oct 17, 2023

View reviewed changes

Copy link

Member

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.

carsonbot added Status: Reviewed and removed Status: Needs Review Status: Needs Work labels

Oct 17, 2023

[HttpFoundation] Do not swallow trailing= in cookie value

77ca01c

nicolas-grekas force-pushed thebug/cookie-trailing-equal-sign branch from59d6acf to77ca01cCompare

October 17, 2023 09:25

Copy link

ContributorAuthor

OskarStark commentedOct 17, 2023

Thank you Nicolas!! 👏

OskarStark commented

Oct 17, 2023

View reviewed changes

src/Symfony/Component/HttpFoundation/Tests/HeaderUtilsTest.php

		[['foo bar'],'"foo" "bar"',','],

		[[['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

ContributorAuthor

OskarStarkOct 17, 2023

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=/',';='],