Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix cookie parsing from headers in server request factory using globals. #193

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

Xerkus
Copy link
Member

@Xerkus Xerkus commented Sep 10, 2024

PHP used to encode space in cookie values as + in violation of RFCs until it was fixed in PHP 7.4.
See https://bugs.php.net/bug.php?id=79174

We decode headers to read server request cookies when creating server request from globals. It was still converting + in the cookie value back to space.

This PR fixes our implementation to align it again with the supported PHP versions and RFCs.

PHP used to encode space as '+' in the cookie header values in violation
of the RFC. This was fixed in PHP 7.4. See
https://bugs.php.net/bug.php?id=79174

Our cookie header parsing continued to decode '+' into space. This fix
brings our implementation back into alignment with the supported PHP
versions.

Signed-off-by: Aleksei Khudiakov <aleksey@xerkus.pro>
Signed-off-by: Aleksei Khudiakov <aleksey@xerkus.pro>
@Xerkus Xerkus added the Bug Something isn't working label Sep 10, 2024
@Xerkus Xerkus added this to the 3.4.0 milestone Sep 10, 2024
@Xerkus Xerkus linked an issue Sep 10, 2024 that may be closed by this pull request
@Xerkus Xerkus requested a review from gsteel September 10, 2024 20:04
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

LGTM! My only concern here is that it's a behaviour change that is possibly unexpected, even though it's effectively a fix.

Can we get away with releasing this in a patch?

@Xerkus
Copy link
Member Author

Xerkus commented Sep 10, 2024

@gsteel You want to release it as 3.3.1?
We can do that but I was going to tag 3.4.0 anyway although phpunit 10 upgrade is a dev dependency and the whole thing can be easily reduced to a patch release.

@gsteel
Copy link
Member

gsteel commented Sep 10, 2024

I think a 3.4.0 release is fine - I was questioning the change needing to go into 4.0 as a possible BC break, or not as the case may be

@Xerkus
Copy link
Member Author

Xerkus commented Sep 10, 2024

There is no chance for BC break here.

@Xerkus Xerkus merged commit 6f63606 into laminas:3.4.x Sep 10, 2024
12 checks passed
@Xerkus Xerkus deleted the fix/cookie-parsing branch September 10, 2024 20:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plus signs in cookie data get converted to space.
2 participants