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

Use CRLF for header separation, except sendmail, fixes #23 #24

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

peterthomassen
Copy link
Contributor

No description provided.

@schengawegga
Copy link
Collaborator

@peterthomassen Thanks for your PR. Can you see differences between PHP8.1 and PHP7.4 on your server?

@till You quoted the RFC description in the issue #23. According to the RFC, \r\n is always required. In this PR, there is still a difference between windows OS and other systems. The only difference between this PR and the old code, is that the PHP_EOL isn´t used anymore. I think, i have to test this PR on a windows and on a linux server, to check, whats the exact difference between PHP_EOL, "\n" and "\r\n", on each system. Maybe we can only provide "\r\n" on every system, like the RFC says?

My idea is, there is a problem with the PHP_EOL constant on that system constellation (Ubuntu 22.04, PHP 8.1). That could be explain, why this error did not occur earlier. I will test this in the next few days, but i have to setup a test system first. Maybe this could take a few days.

@peterthomassen
Copy link
Contributor Author

@peterthomassen Thanks for your PR. Can you see differences between PHP8.1 and PHP7.4 on your server?

Unfortunately, I'm not in a position to test PHP 7.4.

@till You quoted the RFC description in the issue #23. According to the RFC, \r\n is always required. In this PR, there is still a difference between windows OS and other systems. The only difference between this PR and the old code, is that the PHP_EOL isn´t used anymore. I think, i have to test this PR on a windows and on a linux server, to check, whats the exact difference between PHP_EOL, "\n" and "\r\n", on each system. Maybe we can only provide "\r\n" on every system, like the RFC says?

The RFC quote is about using CRLF for folding headers (i.e. as a separator for multi-line headers). That's different from what we're discussing, which is the separator between headers (i.e. what's right before the field name).

Because that quote doesn't apply there, I didn't generally set "\r\n" in this PR, but only where it is known to make problems (= the case I encountered). I don't know which separator between headers one should use on Windows, but it seems the answer does not follow from the quote in #23 (comment).

@schengawegga
Copy link
Collaborator

There is a note at the parameter "additional headers" from the mail() function:

If messages are not received, try using a LF (\n) only. Some Unix mail transfer agents (most notably » qmail) replace LF by CRLF automatically (which leads to doubling CR if CRLF is used). This should be a last resort, as it does not comply with » RFC 2822.

That sound like the problem that @peterthomassen has.
@till what do you think?

Maybe, we should make the CRLF editable via the parameters of the Mail\mail method, instead of changing the general if statement?

@peterthomassen
Copy link
Contributor Author

That sound like the problem that @peterthomassen has.

I have the opposite problem.

@schengawegga
Copy link
Collaborator

@peterthomassen ok. i´m very tired at the moment. But i will check this in the next few days, and give a feedback. Thank you for your contribution.

@schengawegga
Copy link
Collaborator

@peterthomassen sorry for the long time. but i have to get some things done before i can have a look at this projects again. So i read the issue #23 and this PR again, and still be confused. You´ve written, that the problem occurs on your ubuntu system, because the PHP_EOL was \n. But in this PR, it would be still \n on your ubuntu. So i don´t understand, why this PR solves your problem? Is this PR really solving your problem on your ubuntu system?

@peterthomassen
Copy link
Contributor Author

peterthomassen commented Mar 26, 2023

No problem about the delay. :-)

Oops! You are right, the PR is buggy! I'm glad you caught my mistake. I had misread the strpos(PHP_OS, 'WIN') check and thought it checks for true instead of false.

My intention was that the separator should be set to "\r\n" even when PHP_EOL is "\n".

I am using a different fix (did not touch the library, but instead I set $mail->sep = "\r\n" on my instance, which does fix the bug).

So it seems like both on Windows and not on Windows, "\r\n" would be the right behavior. This is also in line with the mail() documentation, which says:

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

This is OS-independent. In fact, I had wondered before what the Windows treatment is about, but I had largely ignored it because I don't know much about Windows. But it now seems that on either OS, the same handling is in order.

If you agree, I'm happy to fix the PR.

@schengawegga
Copy link
Collaborator

@peterthomassen thanks for your reply. i am happy about your mistake. i was struggeling about understanding the issue. ;-)
So as i see, we can delete this whole section about the line-feed in Mail_mail, because in mail.php the $sep was initialized with \r\n, so if we delete this whole section, we always have \r\n in Mail_mail headers. But i want to check this in the fields on linux an windows servers, to understand and reproduce the issue and the solution by deleting the whole section.

@jparise @till what do you think about this solution? alternatively or additionaly we can implement getter and setter for the $sep, to set it back manually, if the problem occurs after deleting the whole section.

@schengawegga schengawegga added this to the 1.5.1 milestone Mar 26, 2023
@schengawegga
Copy link
Collaborator

The RFC 822 shows CRLF too. So I think, we can delete the whole section and we are compliant with PHP mail() and the RFC 822

@till
Copy link
Member

till commented Mar 27, 2023

@schengawegga Yeah, as I said I am more surprised it took this long for someone to file a bug.

@schengawegga
Copy link
Collaborator

i took a look on the actual RFC5322. The CRLF has not changed and is still RFC compliance. So i will do a change of this CR to delete the whole section and implement a possibility to set the $sep manually, if this will occur any error in existing implementations. I am wondering, that it took so long to file this bug, too. Maybe it is not very common to use the Mail_mail Method of this class. In my applications, i only use Mail_SMTP. We will see.

@peterthomassen
Copy link
Contributor Author

... and I am wondering about the motivation for the commit that introduced the behavior we're now removing.

But perhaps that was conditional on circumstances that have changed since then, so the archaeology is hard to do now. (Or perhaps we're missing something ...)

@schengawegga
Copy link
Collaborator

schengawegga commented Mar 27, 2023

@peterthomassen yes, that's why i'm not taking the decision lightly either. And want to do some tests on different server OS.
@jparise Maybe you can say something to the reasons of this commit 30263f5?

@schengawegga
Copy link
Collaborator

schengawegga commented Mar 27, 2023

I think, i found the reason of this commit in a note on PHP mail():

Note:
If messages are not received, try using a LF (\n) only. Some Unix mail transfer agents (most notably » qmail) replace LF by CRLF automatically (which leads to doubling CR if CRLF is used). This should be a last resort, as it does not comply with » RFC 2822.

So we can not know, wich MTA is used and we also can not know, if this MTA replaces LF with CRLF automatically.
Maybe it can be the reason, why this bug was not addressed until know.

Maybe at some implementations of Mail_mail the use of LF is the correct one for the installed MTA.

So i think, we can not delete this section without running in trouble with other implementations.
I will think about a solution to set the $sepmanually with new setters and getter and overwrite der automatic detection, if set manually.
To give the option to force $sep to the needed value, if the automatic detection is the wrong one.

@peterthomassen @till @jparise @alecpl What do you think about this solution?

@peterthomassen
Copy link
Contributor Author

I have no preference which way this is handled, as I already found my own fix. :-)

@jparise
Copy link
Member

jparise commented Mar 27, 2023

I'm afraid I don't recall any additional context beyond what I recorded in the commit message, and it's certainly possible things in the execution environment have changed over the last 20 years. 😉

The qmail detail is interesting because I was definitely using qmail-based systems at the time. For example, I administrated the Horde mailing lists using ezmlm.

In general, I would make the default behavior RFC-compliant and allow for site-level configuration to change the $sep value at runtime.

@schengawegga
Copy link
Collaborator

Thanks for your answers. I think i found a good solution. Since PHP7.2 the mail() supports headers as an array.

Version Description
7.2.0 The additional_headers parameter now also accepts an array.

Giving the headers as an array to mail() should fix this behavior automaticly, i hope ;-)

So i will give the headers array in Mail_mail directly to mail on PHP7.2 and above and add setter and getter to change $sep at runtime if needed.

@peterthomassen
Copy link
Contributor Author

Sounds like a great plan!

@schengawegga schengawegga removed this from the 1.5.1 milestone Mar 28, 2023
@schengawegga
Copy link
Collaborator

@peterthomassen
@jparise
@till

So, after a few tries, and a lot of thinking and practicing a lot ways of getters, setters and giving the headers array directly to the mail() function, i stopped developing.

Giving the headers array directly to the mail() function is very complicated, because not only the Mail_mail uses prepareHeaders. And if i only give the headers Array to the mail() function, all other checks in prepareHeaders will not be done anymore.

But then i found out that the solution is already there and i think thats the reason, why this bug not be filed after such a long time.

Here is the solution code from my test script:

$mail = Mail::factory('mail');

$mail->sep = "\r\n";

$headers = array(
    'From' => 'localman@localhost',
    'Subject' => 'DingDongDay',
    'X-your-Header' => 'I am spring',
    'X-my-Header' => 'BOOOIIINNNNGGG',
);

$body = "You are my sunshine.";

var_dump($mail->send('mamamia@localhost',$headers, $body));

You can set the separator $this->sep directly, because it is a public variable in the class.
And if you set it directly, it is not been overwritten by the automatic OS detection in the constructor, because you set it after the constructor has been called.

So the solution should be almost there, but a little bit hidden.

@peterthomassen can you tell me, if this solution works for you as well?

@schengawegga
Copy link
Collaborator

@peterthomassen HAHA. I`m sorry. This is the trouble i get into it, when i am developing at 00:15am. That was your solution already. So ignore my question, please. ;-)

@schengawegga
Copy link
Collaborator

@jparise @till @alecpl
nevertheless, i think this solution is intentional. Why else should this variable be public if it should not be explicitly changeable?
If this should not intentional behavior, the variable should be protected.
Sure, it is not a good practice to set it directly, and not over a setter, wich can be used to sanitize the input.
But i do not know, what separators somebody uses rightnow.
If i protect that variable now and implement a setter, wich only allows \r\n or \n, i don´t know if i will crash someones actual scripting.

What do you think about all that?
I´am very struggeling at the moment with this issue.

@jparise
Copy link
Member

jparise commented Apr 2, 2023

@jparise @till @alecpl nevertheless, i think this solution is intentional. Why else should this variable be public if it should not be explicitly changeable?

Yes, I think I did that intentionally. $sep can be both read and (re)set by callers.

@schengawegga
Copy link
Collaborator

@jparise Thanks. That´s what i thought. So we can forget the idea to implement getter and setter, because it is already there.
But i think, we should delete the behavior of automatic changing the separator depending on the OS, to make the process RFC compliant. If there is any other separator needed, the user can set this separator via the $sep variable.`

With this change, we also will not crash any script, wich has been set the $sep already with a value, we don´t know.

When we make this change, we have to write a comment, wich explains this behavior well and give a clear hint to set a separator manually, if needed.

Is there a way of giving the callers a hint of this change after Update via composer or log files?
We can raise a E_USER_NOTICE via trigger_error()?

What do you think about that plan?

@jparise
Copy link
Member

jparise commented Apr 3, 2023

@jparise Thanks. That´s what i thought. So we can forget the idea to implement getter and setter, because it is already there. But i think, we should delete the behavior of automatic changing the separator depending on the OS, to make the process RFC compliant. If there is any other separator needed, the user can set this separator via the $sep variable.`

With this change, we also will not crash any script, wich has been set the $sep already with a value, we don´t know.

When we make this change, we have to write a comment, wich explains this behavior well and give a clear hint to set a separator manually, if needed.

Is there a way of giving the callers a hint of this change after Update via composer or log files? We can raise a E_USER_NOTICE via trigger_error()?

What do you think about that plan?

I think your proposal will only change the default behavior for Windows users. I assume that's a smaller percentage of the total user base. We could address this backward incompatibility by releasing the change under a new major version number. I don't know if there's a more helpful (non-annoying) way to signal the affected users at runtime.

@schengawegga
Copy link
Collaborator

@jparise Thanks. That´s what i thought. So we can forget the idea to implement getter and setter, because it is already there. But i think, we should delete the behavior of automatic changing the separator depending on the OS, to make the process RFC compliant. If there is any other separator needed, the user can set this separator via the $sep variable.With this change, we also will not crash any script, wich has been set the$sepalready with a value, we don´t know. When we make this change, we have to write a comment, wich explains this behavior well and give a clear hint to set a separator manually, if needed. Is there a way of giving the callers a hint of this change after Update via composer or log files? We can raise aE_USER_NOTICEviatrigger_error()`?
What do you think about that plan?

I think your proposal will only change the default behavior for Windows users. I assume that's a smaller percentage of the total user base. We could address this backward incompatibility by releasing the change under a new major version number. I don't know if there's a more helpful (non-annoying) way to signal the affected users at runtime.

No, my proposal will change the default behavior for Non-Windows user, like Linux or Mac, because we set \r\n as static default, wich is now the default for Windows users, after the automatic OS detection runs through. All other OS user will get \n after the automation. So the change will affect the most of the users, i assume.

@jparise
Copy link
Member

jparise commented Apr 11, 2023

I think your proposal will only change the default behavior for Windows users. I assume that's a smaller percentage of the total user base. We could address this backward incompatibility by releasing the change under a new major version number. I don't know if there's a more helpful (non-annoying) way to signal the affected users at runtime.

No, my proposal will change the default behavior for Non-Windows user, like Linux or Mac, because we set \r\n as static default, wich is now the default for Windows users, after the automatic OS detection runs through. All other OS user will get \n after the automation. So the change will affect the most of the users, i assume.

Ah, I read the strpos(PHP_OS, 'WIN') === false backwards.

I think this is a situation where it would be good to break backwards compatibility to achieve the correct default behavior, but I do think it probably warrants a semver bump, and I'm afraid I don't have a good sense for how disruptive that would be to the user case (if at all).

@schengawegga
Copy link
Collaborator

schengawegga commented Apr 11, 2023

It is possible, that the impact of this change will not as big as mentioned. Because it seems that many MTA will replace \n into \r\n automatically. I tested it on my windows xampp Maschine, and sendmail/mail() will replace that on it’s own. So I will do more tests with Linux systems and several MTA to get a feeling, how deep the impact really is. My feeling at the moment is, that the most users will not notice any difference, because the \n was replaced automatically into \r\n. So if this behavior will be confirmed, we should change the script to be RFC compliant. At the same time, we should trigger a user notice, on every system, wich is using a non windows system and still using \n, so that the user can read the solution in the protocol.

@jparise what do you think about that solution?

@jparise
Copy link
Member

jparise commented Apr 11, 2023

Got it.

At the same time, we should trigger a user notice, on every system, wich is using a non windows system and still using \n, so that the user can read the solution in the protocol.

What would that condition look like in practice? After the default is changed, it would only trigger for people who have overridden $sep?

@schengawegga schengawegga added this to the 1.6.0 milestone Aug 5, 2023
@schengawegga schengawegga changed the base branch from master to v1.6.0 October 20, 2023 22:02
@schengawegga
Copy link
Collaborator

Got it.

At the same time, we should trigger a user notice, on every system, wich is using a non windows system and still using \n, so that the user can read the solution in the protocol.

What would that condition look like in practice? After the default is changed, it would only trigger for people who have overridden $sep?

yes

@schengawegga schengawegga changed the base branch from v1.6.0 to master October 29, 2023 21:29
@aydun
Copy link

aydun commented Oct 31, 2023

Yeah, as I said I am more surprised it took this long for someone to file a bug.

It seems php's mail() was more lenient in php7 and handled the \n separated headers but php8 is stricter (determined by a quick test script run under php7 and php8 and comparing results).

The mail documentation is clear that it should be \r\n.

PHP_EOL is always defined in current versions of php. The if defined part looks to be cautious programming in case of an old version and if not defined just sets the separator as PHP_EOL would - ie \r\n on Windows, otherwise \n. So on current versions this reduces to $sep = PHP_EOL and therefore it produces the right setting on Windows. But the problem is observed on Linux systems where this is \n.

The code comment justification is about passing command line arguments - but that does not happen (code). There seems no reason to change $this->sep at all in Mail/mail.php. Just leave it as defined in mail.php

@schengawegga
Copy link
Collaborator

@aydun thanks for your reply and the tests you've done.

I found a behavior of postfix replacing \n by \r\n around twelve years ago: https://serverfault.com/a/325513

Maybe this would be the reason of this if statement, and maybe postfix fixed this behavior already.

So, I am with you to delete this if statement, leave the initial value of $this->sep = '\r\n' and release a new major version, to mark it as backwards incompatible change.

@jparise @peterthomassen @ashnazg @aydun do you agree with that?

@aydun
Copy link

aydun commented Oct 31, 2023

@jparise @peterthomassen @ashnazg @aydun do you agree with that?

@schengawegga - Yes, that makes sense to me.

@peterthomassen
Copy link
Contributor Author

peterthomassen commented Oct 31, 2023

I'm fine with that.

I'd propose to add the E_USER_NOTICE only in cases where behavior changed. The behavior change is when, according to the old code, the default was overwritten with PHP_EOL, which happened when PHP_EOL != "\r\n". I therefore replaced that code block with a notice that is triggered under this condition.

(The old code also branches depending on whether PHP_EOL is defined. However, it's been defined since PHP 5.0.2, and this module requires PHP 5.2.1. For the purposes of this module, that means that it's always defined, and the condition is not needed.)

@schengawegga
Copy link
Collaborator

I'm fine with that.

I'd propose to add the E_USER_NOTICE only in cases where behavior changed. The behavior change is when, according to the old code, the default was overwritten with PHP_EOL, which happened when PHP_EOL != "\r\n". I therefore replaced that code block with a notice that is triggered under this condition.

(The old code also branches depending on whether PHP_EOL is defined. However, it's been defined since PHP 5.0.2, and this module requires PHP 5.2.1. For the purposes of this module, that means that it's always defined, and the condition is not needed.)

Thanks for the commit. But I see some problems with the E_USER_NOTICE, because PHP_EOL will be \n on all Linux systems. So this will trigger the user notice on all Linux systems, after this release.

Which will be the most of the systems this package runs on. So this notice will spam the error log on every Linux system at every single mail transmission.

I think a description in the changelog and the new major release version 2.0.0 is enough to inform the users.

@peterthomassen
Copy link
Contributor Author

I think a description in the changelog and the new major release version 2.0.0 is enough to inform the users.

I agree. I changed the code accordingly.

@schengawegga
Copy link
Collaborator

@peterthomassen Thank you for this commit. Can you do this in sendmail.php, too? code

@schengawegga schengawegga changed the base branch from master to 2.0.0 November 1, 2023 16:09
@schengawegga schengawegga modified the milestones: 1.6.0, 2.0.0 Nov 1, 2023
@peterthomassen
Copy link
Contributor Author

peterthomassen commented Nov 1, 2023

I sure can, but do we know this is the right thing to do?

The code comment says:

        /*
         * Because we need to pass message headers to the sendmail program on
         * the commandline, we can't guarantee the use of the standard "\r\n"
         * separator.  Instead, we use the system's native line separator.
         */

Does this argument no longer hold? (It seems to me that this is a different argument than in mail.php.)

@schengawegga
Copy link
Collaborator

@peterthomassen good point. i will think about that.

@schengawegga
Copy link
Collaborator

I sure can, but do we know this is the right thing to do?

The code comment says:

        /*
         * Because we need to pass message headers to the sendmail program on
         * the commandline, we can't guarantee the use of the standard "\r\n"
         * separator.  Instead, we use the system's native line separator.
         */

Does this argument no longer hold? (It seems to me that this is a different argument than in mail.php.)

You're right. We have to leave this, because it's passed via the command line. So the OS line separator is necessary. Thank you and all contributers for your efforts. I will merge this in the new 2.0.0 branch.

@schengawegga schengawegga merged commit 99f9333 into pear:2.0.0 Nov 1, 2023
@aydun aydun mentioned this pull request Jan 15, 2024
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

Successfully merging this pull request may close these issues.

5 participants