Fix duplicate 'Content-Type' header prevents S/MIME encrypted HTML emails from being rendered correctly

[p7996619]

This fix is related to issue #25, but addresses the problem only for S/MIME emails for now. Tested with Outlook 2016, Roundcube, Contact Form 7 and WordPress notification mails.

[fabacab]

Thanks so much for these improvements. Please match the style of the existing code and I'll see about merging this soon. :)

[p7996619]

I changed now the way the charset is appendend, but this code (same as the regex) makes some assumptions:

• line endings are always \n which is not strictly RFC compliant, but probably fine in this case - see here: https://stackoverflow.com/a/4415687
• the last line is always followed by a line break
• charset is only append to the first occurrence of Content-Type

Tested it successfully with my setup, but there still may be edge cases where it could screw up the headers :/ I'd like to make it more robust, but this could get complicated and unfortunately I don't have much time right now.

[fabacab]

Tested it successfully with my setup, but there still may be edge cases where it could screw up the headers :/ I'd like to make it more robust, but this could get complicated and unfortunately I don't have much time right now.

Hmm. Well, it seems like maybe the better thing to do here is to split this PR into two smaller PRs.

1. One patch could be the addition of the newer application/pkcs-mime MIME Type to improve Roundcube compatibility, because this seems like a harmless change regardless, and
2. the second patch could be handling the charset issues.

However, I do wonder if the latter patch (the one for charset) would even be necessary. Can you describe why you're even adding this? I haven't received any reports of compatibility issues, and I'd prefer to do as little as possible to the contents of the mail messages. Downstream or upstream handlers are more likely to know more about the actual email contents than we do, anyways.

Would you have enough time to do at least just the first PR? If not, no worries, just let me know and I can chunk this up myself.

I appreciate your close attention to this plugin so far, regardless. :)

[p7996619]

Please excuse my delayed response, I was busy with uni the last few days.

I'm not exactly sure how to split a PR. Would it be enough to submit a new one from a separate branch containing only parts of the changes? Concerning charset issues - all changes I made are somewhat related to my current specific setup and intended usage. In this case I needed the utf-8 charset since Contact Form 7 doesn't add any charset specification to its mail headers and I'd like to be able to send special characters such as german umlauts. Additionally, utf-8 could solve some problems with similar characters from other languages. I probably could have overwritten parts of the form plugin, but I thought that there may be similar issues with other plugins too. I agree that it would be better to let existing implementations handle this kind of thing, but they seem to leave this header untouched. For me it seems to be an acceptable compromise to add the charset header if it wasn't specified at all. Ideally configurable in the settings of course.

Thanks for your patience ;)

[p7996619]

I reverted now the changes for adding the content type. Seems to be working fine now by simply adding the full content type string (Content-Type: text/html; charset=utf-8;) to the "custom headers" field in Contact Form 7. I must have overlooked something when trying this previously. Sorry for causing this unnecessary trouble.

[p7996619]

I did split the PR now and it should be ready to merge. Please let me know if something is missing.

[fabacab]

@githubuserx I'm finally returning to this thanks to the issue report in #34 (and a repreive from some very hectic personal concerns; please accept my apologies on the tardy reply).

In doing some additional research, I learned about a potentially much cleaner approach to deal with the Content-Type issue here. Please have a look at the patch in f79b98fb2ae1a53b790b1c93c8c4cd89e1701f7c and let me know if that version of the plugin resolves your compatibility issues with Roundcube? If so, it means the real issue was the fact that the Content-Type header's media type parameters were being discarded (by WordPress Core, frustratingly), rather than (merely?) an issue recognizing the older x-pkcs7-mime MIME subtype.

[p7996619]

It's all good, glad to have you back :)

Just tested the patch, but unfortunately without positive result. The outcome is the same as with the version on the develop branch. Your changes make sense, but at least in my case it doesn't solve the main issue with not being able to decrypt/display a message with text/html content type.

I think there are two main issues

1. Duplicate mail headers
2. Missing/wrong content types in the encrypted mail body

The openssl_pkcs7_encrypt() function prepends the headers given as parameter, but since the plaintext (and eventually ciphertext) also includes the same headers, at least the content type header is redundant and confuses PHPMailer. It is correct to pass the other header lines such as from or Reply-To to wp_mail(), but the content type is already given by PHP's method.

The structure would look like this: mail(header, header-pkcs7, encrypted(header, plaintext)) (also verified this by manually decrypting the body with openssl)

As the output of PHP's pkcs7 function is not directly sent via sendmail, the headers are additionally handled by wp_mail() and PHPMailer. That's were things go wrong. e.g. in my case these are the headers encrypt() passes on to the wp_mail() call:

From: Form <form@redacted>
Content-Type: text/html
X-WPCF7-Content-Type: text/html
Content-Type: text/html; charset=utf-8;
Reply-To: noreply@redacted
MIME-Version: 1.0
Content-Disposition: attachment; filename="smime.p7m"
Content-Type: application/x-pkcs7-mime; smime-type=enveloped-data; name="smime.p7m"
Content-Transfer-Encoding: base64

^ Notice the multiple Content-Type lines

wp_mail() then parses the content type header - https://core.trac.wordpress.org/browser/tags/4.9/src/wp-includes/pluggable.php#L276 and if it matches text/html, a flag is set on the PHPMailer instance: https://core.trac.wordpress.org/browser/tags/4.9/src/wp-includes/pluggable.php#L430

All content type headers (if multiple) are processed by wp_mail(), but once the isHTML flag is set, it's never unset again and hence PHPMailer treats the email as conventional/multipart html email, effectively ignoring and discarding the application/x-pkcs7-mime MIME type.

This results in the following headers after I've received the mail:

To: form@redacted
From: Form <form@redacted>
Reply-To: noreply@redacted
Subject: 
Message-ID: <e4c8d8f1b7791d4c8f6bff32c1bd7bff@redacted>
X-Mailer: WPMailSMTP/Mailer/smtp 1.3.3
MIME-Version: 1.0
Content-Disposition: attachment; filename="smime.p7m"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Content-Type: multipart/alternative;
    boundary="b1_e4c8d8f1b7791d4c8f6bff32c1bd7bff"
Content-Transfer-Encoding: 8bit

This is a multi-part message in MIME format.

--b1_e4c8d8f1b7791d4c8f6bff32c1bd7bff
Content-Type: text/plain; charset=us-ascii

MIISdwYJKoZIhvcNAQcDoII
...

The pkcs7 mime type is missing, therefore mail clients render the encrypted body as plaintext instead of decrypting it. The problem only occurs when sending HTML emails because only then the problematic flag in PHPMailer is set.

To remediate this, I would strictly remove all existing content-type headers from the array passed to openssl_pkcs7_encrypt(), since the original headers (including content type) are at this point already present in the mail body. - hence this PR ;)

Regarding #34: I had similar issues with non-ascii characters, which I fixed by adding another content type header containing charset=utf-8 to the "custom header field" in Contact Form 7. The important thing is, that the header with the correct content type and charset must be in the encrypted message body. The correct display of the message after decryption therefore depends on the headers specified by the plugin or code sending the email in the first place and is not modified by encrypting it.

Regarding #32: using the newer pkcs7 mime type is probably just a cosmetic change and not directly related to the content type issue. It does not hinder functionality in my experience. (I never decrypted emails in Roundcube, which would probably only work with some sort of plugin... but it does recognize emails as "encrypted" if the newer mime type is used)

[fabacab]

The problem only occurs when sending HTML emails because only then the problematic flag in PHPMailer is set.

Ah! Okay, thanks for reminding me of this context. So there are actually three, not two, different issues that I want to address independently:

1. Correct handling of text/plain email, i.e., not dealing with HTML anything.
2. Correct handling of HTML email.
3. Correct handling of charset parameters.

For the time being, I have avoided dealing with HTML email at all because, as stated in #25, I don't use anything except plain text emails. I also realize now that I misunderstood some of the original purpose of your patches, as well. Support for multipart MIME messages is a good thing to have, but I have wanted to hold off on merging support for that because I want to be really, really, really sure that plain text handling is working well and that I fully understand its processing pipeline. I clearly did not, until recently, because I had not traced things too deeply into PHPMailer.

Your last comment is super helpful and clarifying. So what I'll do is merge f79b98fb2ae1a53b790b1c93c8c4cd89e1701f7c, which seems to be the final necessary fix for actually unmolested plain text handling for both the charset media type parameter and the media type itself.

Then I'll come back to testing HTML-formatted emails. Given that the patch in f79b98fb2ae1a53b790b1c93c8c4cd89e1701f7c changes a few things about the logic regarding how we are handling the Content-Type header (most notably, we're using WordPress Core's filters now), would you feel okay rebasing your work on top of that change and making whatever changes are necessary to your branches so that I can take a look at them anew?

Thanks again for your time on this, I really appreciate your patience and thoroughness.

[p7996619]

Rebased both branches onto pkcs7, #32 probably needs refactoring. It works for plain text and HTML if charset was correctly specified beforehand, otherwise special characters would look like shown in #34.

[p7996619]

It's been a while, but I finally managed to get back to this. Last time I worked on it I managed to thoroughly confuse myself with branch naming, etc. which screwed up my git workflow - but I've sorted that out for now.

I also just checked if the committed code still makes sense, since I couldn't remember the details anymore. Maybe there is a better way to get the same result, but otherwise it should be what's needed to make HTML mails work and keep all original contents (incl. headers) unmodified.

If you don't have any objections it should be ready to merge.

[fabacab]

@githubuserx Hey again. :) Thanks for popping back up to this.

So, I confess: I was about to write back indicating that the HTML issue is resolved. And, indeed, sending S/MIME-encrypted HTML email does currently work. However, now I realize that I think I have misunderstood the issue you reported this whole time. You are not just trying to get "pure" HTML email to work (i.e., email sent as Content-Type: text/html) but rather specifically multi-part MIME emails that contain a part whose Content-Type is text/html. I should have noticed this a long time ago but did not, and I was so focused on ensuring that the underlying Content-Type handling was correct that it slipped my narrow attention.

Now that I'm taking another look at this as well, with fresh eyes, and I believe that the underlying Content-Type issue is resolved, I will have a closer look specifically at the multi-part MIME handling of HTML email and your patch.

Right off the cuff though, if I understand this PR correctly, one thing I would suggest is that you git rebase your patch atop the current develop branch rather than merging it. This will prevent duplicate commits from being recorded, and simplify the development history since your pull request contains only one unique commit.

[fabacab]

@githubuserx Thanks so much for the rebase. :) I have merged this PR into the remove-html-for-encrypt-only branch, which also includes a minor refactoring commit (0649fd1f543264f060cf20ed6eb978bc3d8caacb) that I'd like your opinion on, if you have the time. Specifically, that commit:

• Moves the filtering function to after the $plaintext is written to the $infile path. This was done to ensure that the plaintext message always contains all the original header lines, including Content-Type lines. This doesn't seem to break multi-part MIME or HTML email in my testing, and holds to the spirit of "molest the original data as little as possible." Let me know if this breaks anything for you.
• Following from the above, it removes the $header variable, since that's no longer needed and was also not referenced more than once in your original patch.
• Changes the filtering function to search for Content-Type: text/html instead of merely Content-Type:. This ensures that only text/html mail is affected, which if I understand correctly, is the one and only content type that needs to be altered, and again follows the "molest the original data as little as possible" attempts.

If that all looks good to you, I will merge this into the develop branch proper. :) Thanks again for your patience with me.

[p7996619]

Just tested your commit and everything works as expected! :+1: Thanks for improving it and taking the time.