Option: Use sanitize=FALSE in token_replace and let drupal_set_title sanitize the title string. I think this is a fair option as drupal_set_title will check_plain for us, although hook_token implementations often run filter_xss as well...so please debate.

CommentFileSizeAuthor
#7 2052917-sanitize-title-once-7.patch518 bytesjpstrikesback
#1 2052917-sanitize-title-once.patch608 bytesjpstrikesback

Comments

jpstrikesback’s picture

jpstrikesback commented
StatusFileSize
new 2052917-sanitize-title-once.patch608 bytes

And the patch.

jpstrikesback’s picture

jpstrikesback commented
Status: Active » Needs review

Proper status

jpstrikesback’s picture

jpstrikesback commented
Version: 7.x-2.2 » 7.x-2.x-dev

And proper version...

greggles’s picture

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

An alternative would be to call drupal_set_title with the second parameter of PASS_THROUGH. Token should already be filtering things in a way to make them work for drupal_set_title.

Before committing anything, though, try it with javascript as the value of a token and see if the script executes. Try again with < as part of a title and see if it gets double escaped. If your filtering is right, there will be no double escaping and no script executing.

rballou’s picture

rballou commented
Status: Needs review » Needs work

This looks like it would be good to apply. The patch doesn't apply cleanly though, so if you can re-roll against dev I can then apply it.

I ended up deciding it was better to let drupal_set_title to check_plain since in the future there may be other things at play with the title if needed.

Thanks everyone

jpstrikesback’s picture

jpstrikesback commented

@greggles I would have done that but then a user could actually enter unsafe input and have it shown since you can enter more than tokens (even tho they'd likely need admin perm)

@rballou I'll re-roll shortly, cheers!

jpstrikesback’s picture

jpstrikesback commented
Status: Needs work » Needs review
StatusFileSize
new 2052917-sanitize-title-once-7.patch518 bytes

Here it is, I must have rolled that 1st one against something other than dev...

greggles’s picture

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

re #6 - good point ;) I haven't used the module and didn't look at code beyond this patch so my first paragraph of advice might be totally wrong.

jpstrikesback’s picture

jpstrikesback commented

All good, and thanks for the input regardless!

rballou’s picture

rballou commented
Status: Needs review » Fixed

Committed. Thanks everyone

Status: Fixed » Closed (fixed)

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