Themers and Developers consider $title safe in node.tpl.php.
Since #1642070: Make use of entity labels in templates the variable $title was removed in favor of label as described here: http://drupal.org/node/1776718
The problem:
$title is still set, but is now insecure while it has been secure since 2007. As it will continue to work, developers will just continue using D7 templates, create code by old habits, etc. and such accidentally introduce XSS issues.
Reproduce:
Change $label to $title in node.tpl.php (or node.twig once basic node.twig is in with autoescape off) and use title of
<script>alert('xss');</script>
Suggested resolution (for this issue):
unset($variables[$title]);
or make secure via check_plain() after the node object has been flatted with code like this:
// Flatten the node entity's member fields.
$variables = array_merge((array) $node, $variables);
_This_ introduces the XSS and this is really difficult to see that a simple change from title to label could have such consequences.
Action
Lets fix it. (Shameless plug: With twig and autoescape enabled $title would automatically be secure ;-) )
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
chx CreditAttribution: chx commentedComment #3
BerdirThis has already been done for the taxonomy term template, it only makes sense to do it here as well.
Comment #4
xjmSee: #1811684: XSS: Bartik's node.tpl.php prone to XSS (prints $title)
Comment #5
BerdirChange is trivial, trying to see what breaks.
Comment #7
BerdirOk, let's see if I catched all of these wrong references.
Comment #9
BerdirHad also to fix calls to rdf_mapping. This should pass.
Comment #10
chx CreditAttribution: chx commentedGood to go.
Comment #11
webchickHm. This looks fine to me, but afaics we just did exactly the opposite of this over at #1272870: No semantics for nested comments / bad for screen-readers, with the justification that themers want the option of having fine-grained control over their variables in templates. We should figure out a unified approach we can standardize across all templates.
node.tpl.php's docs need to be updated to reflect these variables being removed, so needs work for that.
Comment #12
Berdir@webchick: These two issues don't conflict each other. The comment patch adds a number of explicit variables to the template, that is totally fine and not the problem here.
The problem here is the "ADD THEM ALL!!!"-approach of adding *everything* in $node to $variables, without making sure that it's safe for display.
Will check if the node.tpl.php templates need to be updated, it shouldn't be listing additional variables anyway.
Comment #13
BerdirUpdated the node.tpl.php documentation to refer to the corresponding $node->something properties instead of variables. Not sure what exactly happens with this documentation in the twig version.
I also copied over the documentation to bartik's node.tpl.php, there's a changed block at the end which I left like that, I think it doesn't hurt to have them in sync again and again, Twig is going to rip that out anyway, right?
Comment #14
xjmSo this is actually different from the approach I thought to take in this issue. I think that "safe" properties like these should be copied manually to the template, and just the blind extraction of everything from the object should go away.
Comment #15
Fabianx CreditAttribution: Fabianx commentedIn twig using $node->type is definitely easier as you just need to use {% if node.type %}, but for that you need to know that this is a node template.
I vote for copying variables manually wholesale for now and leave the preprocess cleanup to twig - when it happens.
I pinged jenlampton to comment here as well.
Comment #16
jenlamptonI agree with @Fabianx
As part of the change to Twig, we'll be removing all the fine-grained variables from the preprocess layer anyway, leaving them there would just be waste. Theme devs can get at all the info they need, at any level, securely, via
node.title
,node.author
,node.content.image.first.src
, etcOne other thing it may not be worth mentioning here, is that I'm also proposing we rename all the variables in all templates to be consistent with one another - so we may be getting rid of anything called 'label' in the node template. We should call it 'title' by the time it gets to a template. see #1825216: Name variables consistently across all templates (preprocess)
I also agree with @xjm that we certainly should not be encouraging theme devs to print anything insecure directly from the node object, by giving them something to cut & paste from the docs that will get them in trouble.
Let's just leave the existing variables for now and we can cleanup as part of the theme system / preprocess overhaul we'll be doing with Twig :)
Comment #17
BerdirSure, can do :)
I added all properties that are documented explicitly, I kept however the changes in the preprocess functions and did not add variables which are used there. Like langcode and rdf_mapping (there is quite a mess in rdf.module in regards to that, 50% of the call currently call it on the node and the others on variables).
No interdiff, that would probably be bigger than the patch anyway.
Comment #19
BerdirSo...
comment_count obviously doesn't exist if comment.module isn't enabled, but is documented in node.tpl.php. What do we want to do with that? define in comment_preprocess_node() and keep the documentation?
And readmore is actually not used anywhere, with the exception of poll.module, which sets it to FALSE because nobody told poor poll.module that there is no such thing anymore :) So that feels like an obvious thing that we can remove?
Comment #20
Fabianx CreditAttribution: Fabianx commentedcomment_count: I think I would define it to NULL by default and copy from $node if it exists. That way all variables are available always, but can be NULL.
( Looping with isset() might even be enough. )
readmore: Get rid of it then I think :-).
Comment #21
BerdirOk, make sure that all the documented properties exist and removed readmore.
Comment #22
Berdir#21: remove-copying-node-to-variables-1810710-21.patch queued for re-testing.
Comment #23
Fabianx CreditAttribution: Fabianx commentedThat is another good catch of the title property!
Looks all good to me ...
=> RTBC
Comment #24
webchickCommitted and pushed to 8.x. Thanks!
Comment #25.0
(not verified) CreditAttribution: commentedmarkup, reproduce