-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Fix GH-8086: Introduce mail.mixed_lf_and_crlf INI #10191
Conversation
Seems sensible, just as a reminder to update the UPGRADING document and open an issue on doc-en (or PR) to document this. |
We probably also want to counter-act #7907, i.e. use the ini setting and a variable instead of the And while I'm not strictly against this change, we need to be very careful wrt. security (especially if we want to target PHP-8.1 or PHP-8.2). There are header injection checks which may only assume CRLF (e.g. 37962c6). |
When this INI option is enabled, it reverts the line separator for headers and message to LF which was a non conformant behavior in PHP 7. It is done because some non conformant MTAs fail to parse CRLF line separator for headers and body. This is used for mail and mb_send_mail functions.
90c7755
to
d3175a7
Compare
Makes sense - done.
I don't really plan to change imap as it is for people who should understand MTAs and can probably handle this just fine. Unless I'm missing something and the current change already impacts it somehow? |
I guess that imap stuff was a bad example. I'm not really concerned about possibly having some cases where CRLF may slip through (we could still improve if necessary), but rather that there might be internal handling which would not properly detect header injection due to LF only. From a quick look, it seems that Regarding which branch to target: @ramsey, @patrickallaert, what do you think about targeting PHP-8.1? |
I chose PHP-8.2 as a base branch because I thought that it might get confusing if INI is available when added to some later version in 8.1. But if others and mainly RM's think that 8.1 is fine I would be cool with that ofc. |
Is this likely to land soon? |
I am just working on |
I plan to merge it tomorrow to 8.2+. |
OK, thanks. I don't think I have made any conflicting changes on |
Merged in cc931af |
When this INI option is enabled, it reverts the line separator for headers and message to LF which was a non conformant behavior in PHP 7. It is done because some non conformant MTAs fail to parse CRLF line separator for headers and body.
This targets PHP 8.2 even though as for some it might seem more like a feature because it requires new INI. On the other side this might have been omission when the bug fix was introduced in PHP 8.0 as it wasn't realized that there are many non conformant MTAs. Also seems like this is an issue for many users.