Drupal Custom Templating Security Issues

Question

I'm mainly a Wordpress guy and am trying to learn the ropes of Drupal 7. My question relates to templating best practices and security concerns. I am working with extremely complex designs (yeah designers right!?) so my markup needs to be clean and just right which I have found Drupal makes extremely difficult with the large hierarchy of template files and functions. Basically the workflow I have found that has been working for me is to override the output of specific content types that I need really specialized markup for at the node level.

So for instance : node--custom-content-type.tpl.php

Like I said I am a wordpress guy and am used to being able to run a database query, grab the exact field values that I want and use them however I want. I have been kpr or printing out the $variables array, studying what it contains, and grabbing values directly like so:

$link = $variables['field_link'][0]['url'];
$link_title = $variables['field_link'][0]['title'];
$target = $variables['field_link'][0]['attributes']['target'];
$text = $variables['field_main_text'][0]['safe_value'];

And then echo'ing out and using the variables in the markup exactly as I'd like:

<article class="getstarted-wrapper">
    <a id="tocollege" target="<?php print_r($target); ?>" title="<?php print_r($link_title); ?>" href="<?php print_r($link); ?>"><img src="/sites/all/themes/amped/images/visiticon.png" /></a>
    <a id="mapcollege" target="_blank" title="View Location In Google Maps" href="<?php echo $maplink; ?>"><img src="/sites/all/themes/amped/images/mapicon.png" /></a>
    <div class="getstarted-top" style="background:<?php print_r($bg);  ?>;">
        <figure>
            <img title="<?php print_r($auth_title);  ?>" alt="<?php print_r($auth_alt); ?>" src="<?php print_r($auth_img); ?>" />
        </figure>
    </div><!--getstarted-top-->
    <div class="getstarted-bottom">
        <p><?php print_r($text); ?></p>
        <a target="<?php print_r($target); ?>" title="<?php print_r($link_title); ?>" href="<?php print_r($link); ?>">Get Started</a>
        <span>This will take you to <?php print_r($college_name);  ?></span>
    </div><!--getstarted-bottom-->  
</article><!--getstarted-wrapper-->

I am wondering how this process matches up against best practices, what am I doing wrong, what am I doing right, and more importantly what are my security risks and how can I avoid them??

score 1 · Accepted Answer · answered Jul 10 '13 at 20:48

Every time you output a plain text string (ie anything that is not deliberately markup) into an HTML page, you need to escape it.

In plain PHP templates that is typically done with the htmlspecialchars() function. Drupal offers check_plain() as a short-cut, although not a very short one. You can define your own shorter cut to reduce the pain:

function h($s) {
    echo htmlspecialchars($s, ENT_QUOTES, 'utf-8');
}

<a id="tocollege" target="<?php h($target); ?>" title="<?php h($link_title); ?>" href="<?php h($link); ?>">...

(I'm not sure what the use of print_r was for—this is traditionally used for producing readable structured-object output for debugging, but that format of output isn't generally what you want in a production web page, and in your example it was only being used for strings, where it makes no difference anyway.)

Thanks for the response, my only other question (not sure if you are the one to ask this or not) is about how this workflow matches up against Drupal best practices? Is there a better way of accomplishing the same tasks etc? — Brendan Moore, Jul 11 '13 at 19:37
I'm not that familiar with Drupal, but a quick look at a bunch of templates seemed to show that being completely vulnerable to XSS is absolutely the industry standard. :-( — bobince, Jul 11 '13 at 19:46

score 1 · Answer 2 · answered Jul 13 '13 at 22:22

The correct approach with Drupal is to sanitize user input on output. Since Drupal has multiple modes of output (not just HTML) it's improper to sanitize on input, so when outputting HTML you can use Drupal's check_plain() function as bobince suggests. check_plain is one of several filter functions available for use, see https://drupal.org/node/28984 for more.

If you are overriding the output and accessing theme variables it is correct that the best practice to run check_plain (or other filter functions) yourself. If it's node properties then you can also use the 'safe' properties as described on the link above.

Drupal Custom Templating Security Issues

2 Answers2