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

addCustomHeader corrupts header value #1854

Closed
clarkk opened this issue Oct 1, 2019 · 13 comments
Closed

addCustomHeader corrupts header value #1854

clarkk opened this issue Oct 1, 2019 · 13 comments

Comments

@clarkk
Copy link

clarkk commented Oct 1, 2019

addCustomHeader corrupts the header value

Original header
X-My-Header: document/supplier_order block/4431 id/12589 email/alias@mail.com log/21288

Encoded header (PHPMailer)
X-My-Header: =?us-ascii?Q?document/supplier=5Forder_block/4431_id/12589_email/alias@mail?= =?us-ascii?Q?.com_log/21288?=

Decoded header
X-My-Header: document/supplier_order_block/4431_id/12589_email/alias@mail.com_log/21288

The Original header and the decoded header are not identical. Spaces are translated to _ (underscore)

The header is decoded with mb_decode_mimeheader()

@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

That header is correctly Q-encoded according to RFC2047 section 4.2(2), so this looks like a bug in mb_mimedecode_header.

You could work around it by doing:

$decodedheader = mb_decode_mimeheader(str_replace('_', '=20', $header));

Though note that you should not do that blindly, as the header may not be Q-encoded, so check for that first.

@Synchro Synchro closed this as completed Oct 1, 2019
@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

Reported as a PHP bug.

@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

Someone in the PHP project confirmed the bug (quick work!), and also pointed out a better workaround - iconv_mime_decode performs the same function, but does not make this mistake. Obviously that does require the iconv extension.

@clarkk
Copy link
Author

clarkk commented Oct 1, 2019

iconv can also work differently depending on the system, so I try not to use functions which depend on iconv

@clarkk
Copy link
Author

clarkk commented Oct 1, 2019

but it would still be nice to pass an extra bool argument to addCustomHeader weather to disable the encoding

@clarkk
Copy link
Author

clarkk commented Oct 1, 2019

or at least check the value before encoding it if it contains "invalid" chars

@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

I think that's a pretty terrible idea. If you don't encode the headers when it's needed, you will have corrupted headers. I'm not doing that. You can always subclass if you want to break things on purpose.

@clarkk
Copy link
Author

clarkk commented Oct 1, 2019

What about checking if the header value contains any illegal chars before encoding?

@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

Have you tried reading the source? That's exactly what it does.

@clarkk
Copy link
Author

clarkk commented Oct 1, 2019

I have.. :)

The example X-My-Header: document/supplier_order block/4431 id/12589 email/alias@mail.com log/21288 contains no illegal chars but it is encoded anyway.. It may be a bit long but then you only have to break the header - not encode it

@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

Look at the encoder - it will apply Q encoding if the line contains non-ascii chars, or if its length means it needs folding (even if it only contains ascii). The reason for that is that the PHP mail() function can corrupt lines longer than 65 chars, even though SMTP is supposed to allow at least 76 (and up to 998). Q encoding the header value means that it can be folded safely even if the header value does not contain any spaces; it's not possible to fold a header without spaces except by using RFC2047 encoding. Look at the discussion around it in #1525, and the fix for it in #1840.

In short, if you don't encode the header correctly, you're asking for all kinds of other trouble. Avoiding encoding it on the basis that your decoder is broken isn't really a good reason.

@clarkk
Copy link
Author

clarkk commented Oct 1, 2019

Ok, you're the expert :)

The headers are just alot prettier when not encoded (readability)

@Synchro
Copy link
Member

Synchro commented Oct 1, 2019

🤷‍♂️ We're not here for pretty, we're here for functional and compliant!

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

2 participants