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 &. 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:
token->replace()documentation states that The caller is responsible for calling \Drupal\Component\Utility\Html::escape() in case the $text was plain text.- token->replace() documentation states that the caller is responsible for choosing the right escaping / sanitization of the return string
- 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | simplewnews.token-replace.3031910-15.patch | 596 bytes | adamps |
| #10 | simplewnews.token-replace.3031910-10.patch | 10.67 KB | adamps |
| #5 | simplewnews.token-replace.3031910-5.patch | 8.26 KB | adamps |
| #6 | simplewnews.token-replace.ONLY_TESTS.3031910-5.patch | 1.94 KB | adamps |
Comments
Comment #2
adamps commentedComment #3
adamps commentedComment #5
adamps commentedComment #6
adamps commentedHere is a tests only patch that shows the problem
Comment #8
adamps commentedFailure was expected.
Comment #9
adamps commentedComment #10
adamps commentedUpdated 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.
Comment #11
adamps commentedComment #13
adamps commentedCommitting this to allow work on the next one. Don't worry I will revise it if anyone has concerns please leave a comment.
Comment #15
adamps commentedThe fix was backed out of 2.x but remains in 3.x. Here is a small adjustment to get links working correctly.
Comment #17
adamps commentedComment #19
thirstysix commentedBut, Still having the same issue with 3.x-dev
Comment #20
thirstysix commented