Do not decode a contact message twice

Looks like this changed but still worth fixing, it prevents to theme those mails as HTML. Not sure what exactly is going to happen with this, but this might help.

Needs manual testing of the output with and without a HTML mail formatter/sender.

That had some unrelated changes.

Oh, there is another call to checkPlain() missed that. but we also need to get rid of the double-conversion in the patch.

Here is a patch that also removes the checkPlain() call.
I can't find any other occurrence of the MailFormatHelper::htmlToText() function, The @todo about improving htmlToText is probably still valid, just a little out of place without a single htmlToText() call in the class.
The MailFormatHelper::htmlToText() calls in the test can be removed (without the test failing), but I'd like some feedback about that first, don't want to post 20 different patches :D.

Tested locally as well, everything seems to work just fine.

checkPlain() will be gone soon anyway and #markup is filtered with Xss::adminTagList. If we want more restrictive filtering, you have to add #allowed_tags = {TAGS GO HERE}.

But I don't know which tags should be allowed in such a message?

What we basically need/want to test is how this behaves in combination with the default plain text mail plugin and with one that supports HTML.

So install and configure https://github.com/webflo/drupal-swiftmailer, then send a contact mail and check how it looks. Is the HTML in there, is it escaped, ...

I'd guess this also needs a reroll

I think this is already been fixed in #2559605: Convert SafeMarkup::checkPlain() in render arrays to #plain_text. It uses the '#plain_text' key instead of the '#markup' like it did before.
The content-type is always plain-text (even though HTML might be selected). Is that the expected behaviour? I tested it without any patches, emails sent with the core and the swift mailer plugin (configured either as plain-text or HTML) all show up in the same way:

Tested it with real emails. Yes, there is a new issue, the old issue seems resolved by the change in #2559605: Convert SafeMarkup::checkPlain() in render arrays to #plain_text but causes emails to be always sent as plain-text.

I sent the HTML email with the following content:

This is a more <b>specific</b> test, I think the <a href="http://d8.dev">emails</a> use markdown for their markup.

<i>This text is italic</i>

This is <sup>superscripted</sup> text.

I received this:

The contact message body is one thing. That actually should be escaped because you *are* sending a plain text mail.

To send a HTML mail, you must set up something like swiftmailer. The HTML that I'd expect then is the one from the field templates for example.

In Simplenews, we have a test mail plugin and a test that simulates an HTML mail.

We should bring that into core and test a contact message, that just like SimplenewsSourceTest::testSendHTML() explicitly puts special characters like > and ' into mail subject and body and then make sure that it is printed as expected for both plain text (converted according to what we expect) and HTML (escaped exactly once for body). Subject is always plain.

Alright, I set up everything correctly this time, I think. The message is formatted now (at least the content I entered).

Source Content:

This is a more <b>specific</b> test, I think the <a href="http://d8.dev">emails</a> are formatted now.

<i>This text is italic</i>

This is <sup>superscripted</sup> text.

The email in my inbox:

The incorrect formatting for the unsubscribe link is an issue for simplenews I guess.

For the test-porting:

Extend testSendPersonalContactMessage() in ContactPersonalTest with a message that uses HTML formatting (which requires a test system like simplenews, see HTMLTestingMailSystem). That's what I'll be doing now, I'll post the patch tomorrow which extends the test.

Alright, here is a patch that extends test coverage with HTML formatting for contact messages. It uses the testing mail sender from simplenews (removed references specific for simplenews and fixed the documentation [which is copy/paste in simplenews]). I changed the test old test slightly, adding the special characters to subject and body. This requires me to split the mail body on line breaks to extract the message.

I'll take a look at the old patch, but I think that one is useless by now.

1. +++ b/core/modules/contact/src/Plugin/Mail/HTMLTestingMailSystem.php
   @@ -0,0 +1,46 @@
   + */
   +class HTMLTestingMailSystem implements MailInterface {
   

   I would move this this next to TestMailCollector and also name it accordingly.

2. +++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
   @@ -103,6 +105,38 @@ function testSendPersonalContactMessage() {
   +    $message['message[0][value]'] = "This <i>is</i> a more <b>specific</b> <sup>test</sup>, I think the <a href='http://d8.dev'>emails</a> are formatted now.";
   

   As said above, the body itself should not be formatted, only the other part of the mail. I'll have to test this myself.

Moved the test, still passing even though it shouldn't (according to your second point in #20).

Straight reroll against 8.1.x, we already have tests so removed the "Need tests" tag and it's 8.1.x target so also removed "Needs beta evaluation" tag as well.

Applying the 2223967-23.patch from #23 failed for Drupal 8.1.8:

patching file `composer.lock'
Hunk #1 FAILED at 4.
Hunk #16 FAILED at 680.
[...]
35 out of 78 hunks FAILED -- saving rejects to composer.lock.rej
patching file `core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php'
patching file `core/modules/contact/src/Tests/ContactPersonalTest.php'

Reroll of the previous patch #23

1. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php
   @@ -0,0 +1,46 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Mail\Plugin\Mail\HTMLTestMailCollector.
   + */
   

   Remoe this docblock . this is not needed

2. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php
   @@ -0,0 +1,46 @@
   +    $message['body'] = implode("\n\n", $message['body']);
   

   use PHP_EOL

I see here only the test but nothing seems fixed.

1. diff --git a/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php b/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php
   ...
   +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php
   ...
   +class HTMLTestMailCollector implements MailInterface {
   

   s/HTMLTestMailCollector/HtmlTestMailCollector. Better TestHtmlMailCollector?

2. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php
   @@ -0,0 +1,46 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Mail\Plugin\Mail\HTMLTestMailCollector.
   + */
   

   Should be removed.

3. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/HTMLTestMailCollector.php
   @@ -0,0 +1,46 @@
   +  public function mail(array $message) {
   

   This seem identical to \Drupal\Core\Mail\Plugin\Mail\TestMailCollector::mail(). Why not extending TestMailCollector to reuse the code?

4. +++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
   @@ -83,7 +83,9 @@ function testSendPersonalContactMessage() {
   +    $message_captured = trim($message_exploded[9] . ' ' . $message_exploded[10]);
   +    $this->assertTrue(strcmp($message_captured, $message['message[0][value]']) !== FALSE, 'Message body is in sent message.');
   

   Probably this deserves a comment, explaining what are we testing here.

Attempting to address #31.1, #31.2, #32.1, #32.2, #32.3

Please have a look

Still to do #32.4

This applied cleanly on 8.2.x, but no longer applies on 8.3 for me. Not positive why yet, if I get it solved I'll post more information or an updated patch.

I hate to remove work that was done in the previous patch, but it seems core/modules/contact/src/Tests/ContactPersonalTest.php or an equivalent test file no longer exists. Simply removing the portion of the previous patch that modified that test makes it apply cleanly again in 8.3.

Here's a patch that handles that, it's identical otherwise. I didn't hide the old patch yet, because that test might still belong somewhere, but I just needed a patch that applies for now.

#38 is not correct reroll please reroll the #34 agian.

Re-rolled #34 (and threw in an interdiff from #38 for good measure).

Correct test failures.
Fixed coding standards errors.

Correct test failure.

All of the patches after #7 seem to be test-only changes, and don't jive with the actual bug fix described in the IS.

Reroll of #46

I changed two things in the patch:

1. Fixed the deprecation notices in test: Replaced getUsername() with getDisplayName(). I picked getDisplayName() as the human-centric replacement instead of getAccountName() as the database-centric function. In default installations, both return the same string.
2. Changed the order of subject and message back.

I've addressed #48 by adding missing changes from #7 to patch from #52.
@Gogowitsch I've reverted your issue description changes because I don't think we should separate the original proposed resolution to another issue.

So I originally reported this problem as a security issue and we could not agree if it is a security issue or not if Drupal allows sending emails with unescapsed XSS (that email clients _should_ block). I used Mailhog when I discovered this which is a stupid email client/collector for sure. https://security.stackexchange.com/questions/12568/is-e-mail-a-direct-ve...

@berdir realised that this issue is in the public issue queue for 6 years so we decided to close the security issue and go public with my patch. This was my original report, just to give you more context what is happening:

This module has an XSS vulnerability.

You can see this vulnerability by:
1. Enabling the module and the Swiftmailer module
2. Configuring the Swiftmailer module on /admin/config/swiftmailer/messages to use HTML message format and DO NOT respect the provided email format. In other words, always send out HTML mails.
3. Register a new user and go to its Contact form, ex: /user/2/contact
4. Type <script>alert('xss');</script> as message body and click on "Send message".

When the addressee opens the email the XSS gets executed because it was not properly escaped by the Contact module. Precisely, it was double escaped. Even if \Drupal\contact\MessageViewBuilder::view() tries to sanitize the string by calling \Drupal\Core\Mail\MailFormatHelper::htmlToText(), when this method gets called, the XSS string is already escaped by the field API. Calling \Drupal\Core\Mail\MailFormatHelper::htmlToText() just re-enables the XSS vulnerability. (The called Xss:filter() ignores the HTML escaped XSS string.)

Swiftmailer trusts the generated message body by the Contact module because of this: https://git.drupalcode.org/project/swiftmailer/blob/8.x-1.0-beta2/src/Pl...

The issue could be also reproduced with the STMP module if the HTML email sending is also enforced.

My patch takes a different direction that the last one, because I think the MailFormatHelper::htmlToText() should still be called if the input is a raw string and not an implementation of the MarkupInterface.

One clarification on this point:

we could not agree if it is a security issue or not

The Security Team is as agreed as we get that it is not a security issue and can be fixed in public.

Yes, that is right. I have got a full explanation.

I don't see why you'd want to make it dynamic and change the direction of this issue at this point. I think it is a safe assumption at that point that non-Markup html strings are going to be converted later on. Swiftmailer would either do that or apply a basic format, although there's some ongoing discussions about that in the swiftmailer issue queue.

People have been using the existing fix for years and it has explicit test coverage.

People have been using the existing fix for years and it has explicit test coverage.

So if it is stable then why it has not been merged yet?

I think it is a safe assumption at that point that non-Markup html strings are going to be converted later on.

I can also live with that, because the mailer service should decide how it handles the outgoing message. Including the transformation/sanitization of the content. But I think there should be a better/explicit guidance about how mailer implementation should handle this situation and that would solve this problem. When I bumpped into this I remember I was not sure which companent should I blame or patch and based on some digging it seemed reasonable to keep the sanitization in Contact module, just make it work better or similar how others work.

I have just found this public issue after I raised another security issue - @mxr576 we think alike! OK fine, it is been agreed to solve this in public but I fully agree with the idea:

So if it is stable then why it has not been merged yet?

I strongly support to commit this ASAP and backport to D8.

I think it is a safe assumption at that point that non-Markup html strings are going to be converted later on.

I can also live with that

Sounds like we have a plan we can all agree on, so I have hidden patch #57 and scheduled patch #53 for retest on D9.1. PhpMail calls MailFormatHelper::htmlToText() without checking instanceof MarkupInterface. Personally I find that a bit strange, but we can't really change it now. So to be consistent the contact module must never convert.

But I think there should be a better/explicit guidance about how mailer implementation should handle this situation and that would solve this problem

Yes I agree with you @mxr576. However this would be a separate issue I think. We all want this patch to be committed ASAP so let's not try and add any changes to it:-)

We all want this patch to be committed ASAP so let's not try and add any changes to it:-)

We all want this patch a real fix to be committed ASAP =]

Since #53 there is a TestHtmlMailCollector in Drupal core provided by mail_html_test module for example and I would also reconsider the need for those changes that has happened in testSendPersonalContactMessage().

It was not a coincidence that the test in #57 was much lightweight than in #53.

It was not a coincidence that the test in #57 was much lightweight than in #53.

Sure it can make sense to improve the patch if there are good reasons. I would say that the key points are

1. include an interdiff
2. explain why you made each change (which you did a bit in #64 but not so much in #57)
3. keep MessageViewBuilder as it was in #53

In any case #53 fails on D9 so it needs work

Re-rolled patch from #53 for 9.1.x.

I have updated the patch to fix the failed test case.

Thanks @sja112

In #64 @mxr576 pointed out

Since #53 there is a TestHtmlMailCollector in Drupal core provided by mail_html_test

So I propose that we need to fix this patch to use the existing one, rather than adding a duplicate one.

@adamps thanks for pointing out the issue. I missed this. I have updated the patch.

Updated patch to fix the failing tests.

Great looks good to me.

I would be really great if the issue summary can be updated:

• To describe the problem better - check_plain() doesn't exist anymore
• Describe the possible BC implications of making this change
• Describe the security implications of making this change

One thing that would be good to know is if another other mailer is relying on this code for security? Another thought is that if we're agreeing that it is the mailer's responsibility to ensure the message body is safe then we need to make this extremely explicit.

1. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -108,6 +108,36 @@ public function testSendPersonalContactMessage() {
   +    $this->assertEqual($mail['to'], $this->contactUser->getEmail());
   +    $this->assertEqual($mail['from'], $this->config('system.site')->get('mail'));
   +    $this->assertEqual($mail['reply-to'], $this->webUser->getEmail());
   +    $this->assertEqual($mail['key'], 'user_mail');
   +    $this->assertEqual($mail['subject'], $subject);
   

   Let's use the PHPunit method here $this->assertEquals()

2. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -108,6 +108,36 @@ public function testSendPersonalContactMessage() {
   +    $this->assertTrue(strpos($mail['body'], 'Hello ' . $variables['@recipient-name']) !== FALSE, 'Recipient name is in sent message.');
   +    $this->assertTrue(strpos($mail['body'], $this->webUser->getDisplayName()) !== FALSE, 'Sender name is in sent message.');
   +    $this->assertTrue(strcmp(PlainTextOutput::renderFromHtml($message_captured), trim($message['message[0][value]'])) !== FALSE, 'Message was found in mail.');
   

   $this>assertStringContainsString() and there's no need for a message.

I am working on it.

Even href shouldn't be like this

<a href='http://d8.dev'>emails</a>
and Please elaborate in comments about this

$message_exploded = explode(PHP_EOL, $mail['body']);
+    $message_captured = trim($message_exploded[8]) . ' ' . trim($message_exploded[9]) . ' ' . trim($message_exploded[10]);

updated as per comment #74

1. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -80,20 +83,17 @@ public function testSendPersonalContactMessage() {
   -    $this->assertEqual($mail['to'], $this->contactUser->getEmail());
   -    $this->assertEqual($mail['from'], $this->config('system.site')->get('mail'));
   -    $this->assertEqual($mail['reply-to'], $this->webUser->getEmail());
   -    $this->assertEqual($mail['key'], 'user_mail');
   +    $this->assertEquals($mail['to'], $this->contactUser->getEmail());
   +    $this->assertEquals($mail['from'], $this->config('system.site')->get('mail'));
   +    $this->assertEquals($mail['reply-to'], $this->webUser->getEmail());
   +    $this->assertEquals($mail['key'], 'user_mail');
   ...
   -    $this->assertEqual($mail['subject'], $subject, 'Subject is in sent message.');
   -    $this->assertStringContainsString('Hello ' . $variables['@recipient-name'], $mail['body'], 'Recipient name is in sent message.');
   -    $this->assertStringContainsString($this->webUser->getDisplayName(), $mail['body'], 'Sender name is in sent message.');
   -    $this->assertStringContainsString($message['message[0][value]'], $mail['body'], 'Message body is in sent message.');
   +    $this->assertEquals($mail['subject'], $subject, 'Subject is in sent message.');
   

   These changes are not part of the change. And are out-of-scope. Yes it's a bit odd because the same test has two styles but eventually assertEqual() will be removed so adding more makes more future work.

   Also test assertions unrelated to this change are being removed here.

2. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -108,6 +108,36 @@ public function testSendPersonalContactMessage() {
   +    // Assert mail content.
   +    $this->assertTrue(strpos($mail['body'], 'Hello ' . $variables['@recipient-name']) !== FALSE, 'Recipient name is in sent message.');
   +    $this->assertTrue(strpos($mail['body'], $this->webUser->getDisplayName()) !== FALSE, 'Sender name is in sent message.');
   +    $this->assertTrue(strcmp(PlainTextOutput::renderFromHtml($message_captured), trim($message['message[0][value]'])) !== FALSE, 'Message was found in mail.');
   

   These should be assertStringContainsString()

Thanks @alexpott.

I updated the issue summary and fixed the title: the problem is that we are encoding twice rather the decoding twice.

Describe the possible BC implications of making this change

My feeling is that we are not changing an API; rather we are fixing code to match the existing API. (True the API is poorly defined, but we can deduce what it needs to be through logical analysis and examination of existing code.) So there isn't a direct BC problem. It's true that some sites might be relying on the current behaviour. It's likely not very common and I see no way to avoid it. See the IS for more discussion.

Describe the security implications of making this change. One thing that would be good to know is if another other mailer is relying on this code for security

This change is strictly positive for security. It solves some security risks and doesn't introduce new ones. The previous issue title was misleading here.

Another thought is that if we're agreeing that it is the mailer's responsibility to ensure the message body is safe then we need to make this extremely explicit.

I became maintainer for simplenews and then swiftmailer and for sure I can confirm that the mail API is lightly documented and hard to understand how to use accurately. It's a way bigger problem that just this one aspect though so I'm not sure if it's practical to consider it as part of this issue. Currently the entire API is specified in about 20 lines of code and only 2-3 lines per element, some elements not even mentioned. Could we raise a separate issue to cover this?

It solves some security risks and doesn't introduce new ones.

What security risk does this solve? That's the bit I don't understand yet. As far as I can see this change results in html now being outputted by the 'mail' view mode of the contact message. Any code that was relying on this now has to expect potentially un-safe HTML there now. This looks like increasing risks, not lowering them.

Is it worth considering to add an html-mail view mode and use that instead. And deprecate the mail view mode mode in favour of this new one?

@alexpott I'm happy to have a chat if that would be easier as I fear I am just repeating myself. I already tried to explain in the IS and I'm unsure how to explain much better, however I will try.

You do an amazing job to commit so many Core issues to help the community and I guess that means you likely spend little time in Contrib. Although Core doesn't support sending HTML mails they are working in Contrib. Swiftmailer module is installed on 37000 D8 sites. Modules such as Simplenews (18000 D8 sites) send messages that contain HTML. It all works with the current interfaces. I agree it's not very well documented currently how to do it. However in the case of unclear documentation then it makes sense to accept the widespread current usage. In any case, as far as I can see, the solution proposed in this patch is the only way that works as I tried to explain in the IS. The current code is clearly bugged as per point 2 of the IS problem/motivation.

What security risk does this solve?

I tried to explain that in the IS. The security risk is when using a Contrib mail plugin that generates HTML mails. An anonymous user can send contact form messages containing arbitrary unsafe HTML. The user types a message containing HTML; the contact module encodes it to SafeMarkup, then MessageViewBuilder decodes it again. However because the decode occurred in #post_render the output is still an implementation of SafeMarkup. This is a severe violation of the SafeMarkup interface as the markup is entirely unfiltered. Hence @mxr576 and I independently raised this as security issue but the security team cleared it for a public issue.

Any code that was relying on this now has to expect potentially un-safe HTML there now.

In comparison to the previous paragraph the HTML output is safe - it is a valid implementation of MarkupInterface.

I agree that a mail plug-in code could be missing the call to MailFormatHelper::htmlToText(). However if so then

• They wrote their code without looking at the reference implementation PhpMail
• The code is already broken if used with other Contrib modules such as SwiftMailer

Is it worth considering to add an html-mail view mode and use that instead. And deprecate the mail view mode mode in favour of this new one?

I don't really follow how that strategy would work - which view mode would actually be used by contact_mail()? A new view mode sounds like a lot of work for every site that uses the contact module.

@shaktik Still, comments are pending to implement.

New patch that address all the comments from #74, #76, #79.

I fixed the tests which wasn't testing anything in the original patch (it failed to send the message then made some asserts on the message from the first part of the test).

Another thought is that if we're agreeing that it is the mailer's responsibility to ensure the message body is safe then we need to make this extremely explicit.

Good idea. I have improved the comment on MailInterface::format().

Good news for removing an worries about back-compatibility: the interface definition for $message refers to hook_mail() which is very clear that the sender can pass plain text or markup. Therefore it seems there is no doubt that the plug-in must handle either. We are not changing the interface, we are simply improving the documentation.

Here's a version that applies on Drupal 8.9. I am not sure if it applies on 9.x at this point, or if #88 does for that matter, so I'll just leave this here for now without changing the status or hiding any other patches.

I think the patch is good, and I will RTBC if we can clarify a few things.

Basically I think the documentation introduced is great, but should be even more verbose.

1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
   @@ -15,12 +15,16 @@ interface MailInterface {
   -   * are converted to plain-text by the Drupal\Core\Mail\Plugin\Mail\PhpMail
   

   Elsewhere in this issue this is called the "reference implementation". Maybe it's usfeul to keep referring to it.

2. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
   @@ -15,12 +15,16 @@ interface MailInterface {
   +   * converted, joined and wrapped. The string can be either plain text or
   

   The meaning of "converted, joined and wrapped" is not clear.

3. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
   @@ -15,12 +15,16 @@ interface MailInterface {
   +   * be returned in $message['plain']. Each line must be converted if it
   +   * doesn't match the desired output.  You can use
   

   I'm not clear what "converted" and "desired output" mean here.

4. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
   @@ -15,12 +15,16 @@ interface MailInterface {
   +   * \Drupal\Core\Mail\MailFormatHelper::htmlToText() to convert to plain text
   +   * or \Drupal\Component\Utility\Html\Html::Escape() to convert to HTML.
   

   Presumably whether you can use these depends on what format your original text is?

5. The IS says in "remaining tasks":
   

   Raise a follow-on issue because PhpMail converts incorrectly: it runs MailFormatHelper::htmlToText() even if the line is already a plain text string. This causes corruption of any message that resembles HTML (for example a user account mail containing a < character).

   I'd like to see this issue created before RTBC, and a note on how it relates to this issue. Does anything about the changes here make that issue a worse problem?

Thanks @jonathanshaw. Here is another patch with an improved comment.

Regarding 5. I will raise it. No the problem has not become worse. There is only a small relationship between the two issues: this issue improves the interface comment and so makes it more obvious that PhpMail does not obey the interface.

Great thanks. I raised #3174760: Mails resembling HTML are corrupted

Committed/pushed to 9.1.x and backported to 9.0.x. Needs a re-roll for 8.9.x

Great many thanks @catch

RTBC++

Committed 05083ea and pushed to 8.9.x. Thanks!

Great many thanks @catch

The formating of contact messages is kind of broken now. Note multiple new lines and weird indentation.

admin (http://localhost/d9/docroot/user/1) sent a message using the contact
form at http://localhost/d9/docroot/contact.



      Message
                Test