Problem description

Any subscribe mails with a subject or body that looks like HTML will be corrupted. In particular any < character will cause words after it to be removed from the mail. The problem is that the Drupal mail system expects both subject and body to be escaped markup even for a plain text mail and this module doesn't do that.

If the site uses a mail plug-in capable of sending HTML mail (e.g. Swiftmailer) then there is a different problem. Any HTML entities in token replacements will be double-escaped, so that & will be shown as &amp;. The problem is that the token substitution automatically escapes the token replacements but not the main text; the result is not marked as safe markup so Swiftmailer escapes it.

For example see MailBuilder::buildSubscribeMail

    $message['subject'] = $this->token->replace($message['subject'], $context, array('sanitize' => FALSE));
    $message['body'][] = $this->token->replace($body, $context, array('sanitize' => FALSE));

Problems:

  1. token->replace() documentation states that The caller is responsible for calling \Drupal\Component\Utility\Html::escape() in case the $text was plain text.
  2. token->replace() documentation states that the caller is responsible for choosing the right escaping / sanitization of the return string
  3. token->replace() does not have a sanitize option in D8

See #2580723: Fix token system confusion, with new function Token::replacePlain() for discussion on how token->replace() is inadequate.

Proposed resolution

Create a new functions simplenews_token_replace_subject() and simplenews_token_replace_body() that are wrappers to token->replace() with the required escaping and sanitizing.

Concerns

Possibly some sites have actually got HTML in the body field of confirmation mails. They could have some mechanism to run this HTML through a text filter in the mail plugin. This fix will break that scenario.

However this scenario is fragile/hacky - it's not really the right way to do it. Arguably it's not really supported and it just happened to work. The right way to do this is #3003811: Add an option to send HTML format opt-in / subscribe mails and add a "format" setting onto the page at /admin/config/services/simplenews/settings/subscription.

Comments

AdamPS created an issue. See original summary.

adamps’s picture

Issue summary: View changes
adamps’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Priority: Major » Normal
Status: Active » Needs review
StatusFileSize
new6.08 KB

Status: Needs review » Needs work

The last submitted patch, 3: simplewnews.token-replace.3031910-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

Status: Needs work » Needs review
StatusFileSize
new8.26 KB
adamps’s picture

Here is a tests only patch that shows the problem

Status: Needs review » Needs work

The last submitted patch, 6: simplewnews.token-replace.ONLY_TESTS.3031910-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Plan to commit

Failure was expected.

adamps’s picture

adamps’s picture

StatusFileSize
new10.67 KB

Updated version with formatting. This gets ready for #3003811: Add an option to send HTML format opt-in / subscribe mails which I will fix after this one. It will all be tested with the upcoming new swiftmailer version.

adamps’s picture

Title: Tokens are not correctly escaped in subscribe mails » Corruption of subscribe mails that look like HTML
Issue summary: View changes

  • AdamPS committed 894dada on 8.x-2.x
    Issue #3031910 by AdamPS: Corruption of subscribe mails that look like...
adamps’s picture

Status: Needs review » Fixed
Issue tags: -Plan to commit

Committing this to allow work on the next one. Don't worry I will revise it if anyone has concerns please leave a comment.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

adamps’s picture

Version: 8.x-2.x-dev » 3.x-dev
Status: Closed (fixed) » Needs review
StatusFileSize
new596 bytes

The fix was backed out of 2.x but remains in 3.x. Here is a small adjustment to get links working correctly.

  • AdamPS committed 3baab32 on 3.x
    Issue #3031910 by AdamPS: Corruption of subscribe mails that look like...
adamps’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

thirstysix’s picture

But, Still having the same issue with 3.x-dev

<p>We have received a request for the following subscription changes for <a href="mailto:xxxxxxxxxxxx">xxxxxxx@xxxxxx.com</a> at <a href="http://localhost/xxxxxxxxxxxx/:">http://localhost/xxxxxxxxx/:</a></p>
<p> - &lt;p&gt;Subscribe to Default newsletter&lt;/p&gt;</p>
<p>To confirm please use the link below.</p>
<p><a href="http://localhost/xxxxxxxxxxxxxxxxx">http://localhost/xxxxxxxxxxxxxxxxxxxxxx…</a></p>
thirstysix’s picture