@sroze Yes, I realized that after examining the code more closely and so I edited my comment. Still, cross-referencing the code with PHP's implementation may be a good thing to do to prevent regression in behaviour. I'm not sure how new the Cookie code is, and the code that is currently there is not actually used to generate the final cookie at the moment, so it may be incomplete/untested?

@ThomHurks here is the native implem:
https://github.com/php/php-src/blob/master/ext/standard/head.c#L84

everything looks fine to me.

@nicolas-grekas Not the case, there are some differences that I can spot:

• Symfony does not check the value ofvalue against the characters=,; \t\r\n\013\014 whereas PHP does. Symfony only checks it againstname and PHP checks it againsname andvalue.
• When a cookie needs to be deleted, Symfony sets themax-age to31536001 and theexpiry tonow - max-age but PHP just passes inexpiry = 1 andmax-age = 0.
• PHP checks if the expiry date has a year greater than 9999.
• Symfony alsourlencode'sname, I don't see PHP do that.
• Symfony usesurlencode for thename vsrawurlencode for thevalue, not sure why that difference is there.

I'd like to propose a plan:

• @sroze would you like to submit a testing infra for this? Could be heavily inspired byAbstractSessionHandlerTest on 3.4
• @ThomHurks would you like to submit a separate PR on 3.3 to align the behavior with native PHP?

@nicolas-grekas I will likely have time tomorrow to create a PR.

@sroze@ThomHurks any progress?

Regardingspectre attack andsuggested mitigations should this be considered as a critical security fix?

I don't think it has anything to do with the attacks you've mentioned. I did not get time to get ahead with this so if anybody would like until I get back to it, you're welcome 😉

@sroze I quote the part fromhttps://www.chromium.org/Home/chromium-security/ssca that seems to imply this is actually relevant:

Where possible, prevent cookies from entering the renderer process' memory by using the SameSite and HTTPOnly cookie attributes, and by avoiding reading from document.cookie.

moving to the 3.4 milestone as the last bugfix release for 3.3 was published today

Back on here, this PR now contains a second commit provided by@cvilleger, thank to him for working on it.

The Response + Cookie classes are now functionnally tested, and their behavior is a bit more aligned to PHP native one:

Symfony does not check the value of value against the characters =,; \t\r\n\013\014 whereas PHP does. Symfony only checks it against name and PHP checks it agains name and value.

It does for the name, but doesn't care for the value. HTTP allows anything so no reason to restrict like PHP.

When a cookie needs to be deleted, Symfony sets the max-age to 31536001 and the expiry to now - max-age but PHP just passes in expiry = 1 and max-age = 0.

Aligned

PHP checks if the expiry date has a year greater than 9999.

I don't see any reason, kept as is (and tested).

Symfony also urlencode's name, I don't see PHP do that.

PHP should encode. Symfony does, that's OK.

Symfony uses urlencode for the name vs rawurlencode for the value, not sure why that difference is there.

Unrelated to the PHP vs Symfony delta, kept as is.

Status: needs review

Thank you@nicolas-grekas.

Added the RFC document：rfc6265
The Set-Cookie Header：
https://tools.ietf.org/html/rfc6265#section-5.2.2

@nicolas-grekas you said:

Symfony also urlencode's name, I don't see PHP do that.
PHP should encode. Symfony does, that's OK.

This might be correct from a spec point of view, but FYI upgrading our app broke cookies with names "foo:bar:baz" as the : gets encoded and CloudFront's cookie whitelist does not recognize the encodedfoo%3Abar as being the same cookie asfoo:bar so the cookie gets dropped before reaching our servers. We could work around it but I wonder if this should be considered a regression or not.

Also a related issue, clearCookie does not allow clearing cookies without urlencoding, so a cookie set without urlencoding can not be cleared.

@Seldaek I think we can fix these. WOuld you mind opening an issue?