To reproduce:

  • Create a post with an apostrophe in the title
  • Subscribe to the post, make a comment
  • Since [comment:node:title] is in the subject pattern, the title will be check_plain'ed and the subject will contain ' instead of the apostrophe.
CommentFileSizeAuthor
#9 1096360_double_escape_9.patch1.09 KBgreggles
#5 comment_notify_html.patch1.87 KBberenddeboer
#4 comment_notify_html.patch1 KBberenddeboer
#1 1096360.patch1.16 KBgrendzy

Comments

grendzy’s picture

grendzy commented
Status: Active » Needs review
StatusFileSize
new 1096360.patch1.16 KB

Here's one idea. We could also consider letting token sanitize and then running decode_entities().

Other questions:
-- Do we need to protect against header injection, or does drupal_mail do this for us?
-- What happens if unfiltered HTML is passed to a module like mimemail?

berenddeboer’s picture

berenddeboer commented
Status: Needs review » Needs work

Grendzy, with this patch people can now post javascript and every piece of HTML. So this is clearly wrong. You will need to sanitise per the input format.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

@berenddeboer: Isn't that up to the Mail system itself? Core does a filter_xss() within drupal_html_to_text().

berenddeboer’s picture

berenddeboer commented
StatusFileSize
new comment_notify_html.patch1 KB

Here's another patch, that sanitises a body according to its format.

berenddeboer’s picture

berenddeboer commented
StatusFileSize
new comment_notify_html.patch1.87 KB

Sorry guys, bad patch file. This one should be correct.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Given the hardcoded LANGUAGE_NONE I wonder if this works on multilingual sites - any comments?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I also tend toward Dave's perspective in #3.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Needs work

We need to use 'sanitize' => FALSE and it is up to the mail system to filter output. So +1 for patch in #1.

See:
_user_mail_text()
drupal_html_to_text()

For the concerns about header injection, anything run in token_replace() cannot alter message headers at all.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 1096360_double_escape_9.patch1.09 KB

Well, this no longer applied after #952072: [comment:unsubscribe-url] is not working (and a few other token issues) but it was of course easy to re-roll.

One last look by other folks before I commit?

grendzy’s picture

grendzy commented
Status: Needs review » Reviewed & tested by the community

looks good - I double checked on the header injection, and mime_header_encode() will automatically encode any newlines contained in the replacement value.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Reviewed & tested by the community » Fixed

Thanks grendzy, Dave Reid for the work on this.

Now committed - http://drupalcode.org/project/comment_notify.git/commit/1364ed8

Status: Fixed » Closed (fixed)

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