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

Wrong header separator #23

Closed
peterthomassen opened this issue Feb 2, 2023 · 9 comments
Closed

Wrong header separator #23

peterthomassen opened this issue Feb 2, 2023 · 9 comments
Milestone

Comments

@peterthomassen
Copy link
Contributor

mail() documentation says:

additional_headers (optional) [...] Multiple extra headers should be separated with a CRLF (\r\n)

However, Mail_mail sets the header separator to PHP_EOL (if that constant is defined). On my system, this is \n (in contrast to what mail() expects).

As a result, on my system (Ubuntu 22.04, PHP 8.1), additional headers are not properly delivered: All but the first header are indented with a single space, making them a continuation of the previous header. Example message (as actually delivered):

To: recipient@local
Subject: Test subject
MIME-Version: 1.0
 Bcc: secret.recipient@local
 Sender: sender@local
 From: First Last <from@local>
 Content-Type: multipart/mixed;
 boundary="=_0a0ac24becceaae619c86104faa042c3"
Message-Id: <E1pNYm0-004IxB-D0@web.local>

This results in fully broken email display (MIME boundary does not work, Bcc header malformed and thus retained as plaintext, ...). -- Note that the first header (MIME-Version) does not have the space.

I cannot tell at which point (after calling mail()) these spaces get inserted. But I can tell that Mail_mail's practice of using PHP_EOL (\n) as a separator when passing additional_headers to mail() is in contradiction to the docs, which say that \r\n should be used.

And indeed when I override ->sep = "\r\n" on my Mail_mail instance, the problem goes away.

As things work as described in the mail() documentation, I think that the logic for setting sep is flawed, and \r\n should be used exclusively.

@till
Copy link
Member

till commented Feb 4, 2023

@peterthomassen Want to make a PR and add a test? What you say makes sense, more surprised no one hit this before.

@peterthomassen
Copy link
Contributor Author

Sure. I had not opened a PR because the original commit that added the PHP_EOL magic looked like there was a reason for it, and I wanted to start a discussion first. But if there's consensus, I can go ahead and create a PR.

@till
Copy link
Member

till commented Feb 4, 2023

The commit you linked seems to do it right. Did you mean a later one? I am on my phone and can't look.

@schengawegga
Copy link
Collaborator

@jparise has written, that we can't guarantee the use of \r\n on every system. It looks to me like he was not sure if any other system uses only \n in Mail headers. So I think, we should do some tests with a Linux and a windows system. @peterthomassen please make a PR and I will merge it into a branch, so we can test it on a bunch of system.

@till
Copy link
Member

till commented Feb 4, 2023

I was just confused this is even up for debate:

Each header field is logically a single line of characters comprising
the field name, the colon, and the field body. For convenience
however, and to deal with the 998/78 character limitations per line,
the field body portion of a header field can be split into a multiple
line representation; this is called "folding". The general rule is
that wherever this standard allows for folding white space (not
simply WSP characters), a CRLF may be inserted before any WSP.

(source) — CRLF is \r\n. 🤷🏼

@schengawegga
Copy link
Collaborator

@till 👍 thanks
@peterthomassen please send your PR

@alecpl
Copy link
Contributor

alecpl commented Mar 27, 2023

Someone should test whether this is gonna work with PHP < 8. There's been some changes regarding this over time, e.g. https://bugs.php.net/bug.php?id=47983

@schengawegga
Copy link
Collaborator

@alecpl Thanks for your input. I will test this, when i do some tests on different server OS.

@schengawegga schengawegga added this to the 1.6.0 milestone Aug 5, 2023
@Neustradamus
Copy link

To follow

peterthomassen added a commit to peterthomassen/Mail that referenced this issue Oct 31, 2023
peterthomassen added a commit to peterthomassen/Mail that referenced this issue Oct 31, 2023
@schengawegga schengawegga modified the milestones: 1.6.0, 2.0.0 Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants