Sins Against Drupal 1

This is the first is an ongoing series about ways Drupal can be badly misused. These are generally times someone tried to solve an otherwise interesting problem in just about the worst possible way. All of these will start with a description of the problem, how not to solve it, and then ideas about how to solve it well.

I present these at SC Drupal Users Group meetings from time to time as an entertaining way to discuss ways we can all improve our skills.

This first one was presented awhile ago now (Feb of 2015).

The Problem

The developer needed to support an existing JavaScript app with access to content in the form of Drupal nodes encoded in JSON. This is a key part of any headless Drupal project (this entire site was not headless, just one application), and in Drupal 7 and earlier there was no way to do this in core.

The Sinful Solution

Create a custom response handler within template.php that executes every time template.php is loaded.

The Code

During a routine code review of this site I found the following code in template.php:

...
function theme_name_preprocess_region(&amp;amp;$vars) {
if ($vars['region'] == 'header') {
$vars['classes_array'][] = 'clearfix';
}
}

if (isset($_POST['mode'])) {
if ($_POST['mode'] == 'get_fields_node') {
$node = node_load($_POST['id']);

$container = array();

$sliderCounter = count($node-&amp;gt;field_event_slider['und']);
for ($i = 0; $i &amp;lt; $sliderCounter; $i++) { $field_collection_id = $node-&amp;gt;field_event_slider['und'][$i]['value'];
$field_collection = entity_load('field_collection_item', array($node-&amp;gt;field_event_slider['und'][$i]['value']));

$currentCollectionItem = $field_collection[$field_collection_id];

if (isset($currentCollectionItem-&amp;gt;field_slider_image['und'][0]['uri'])) {
$container['slider'][$i]['src'] = file_create_url($currentCollectionItem-&amp;gt;field_slider_image['und'][0]['uri']);
}
if (isset($currentCollectionItem-&amp;gt;field_slider_caption['und'][0]['value'])) {
$container['slider'][$i]['caption'] = $currentCollectionItem-&amp;gt;field_slider_caption['und'][0]['value'];
}
}

$focusTid = $node-&amp;gt;field_event_type['und'][0]['tid'];
$eventTerm = taxonomy_term_load($focusTid);
$dateTid = $node-&amp;gt;field_event_date['und'][0]['tid'];
$dateTid = taxonomy_term_load($dateTid);

$container['nid'] = $node-&amp;gt;nid;
$container['focusTid'] = $focusTid;
$container['title'] = $node-&amp;gt;title;
$container['focus'] = $eventTerm-&amp;gt;name;
$container['date'] = $dateTid-&amp;gt;name;
$container['body'] = $node-&amp;gt;body['und'][0]['value'];

print json_encode($container);
die();
}

if ($_POST['mode'] == 'get_fields_focus') {
$focusTerm = taxonomy_term_load(substr($_POST['id'], 4));

$container['nid'] = $_POST['id'];
$container['title'] = $focusTerm-&amp;gt;name;
$container['body'] = $focusTerm-&amp;gt;description;

print json_encode($container);
die();
}
}
...

Notice that between the two functions is the random block of code wrapped in
if (isset($_POST['mode'])) {...}. So on every request that results in load the theme template.php is loaded and in addition to the normal parsing triggers a check to see if the page request was a POST that included a mode. It it was, we then proceed to load up a node, a couple taxonomy terms, and then encode them as JSON for response. The site sends the response and then unceremoniously dies().

Why is this so bad?

First, there is no parameter checking on the ID parameter: node_load($_POST[‘id’]). Anyone on the internet can load any node if they work out the ID. Since nodes are sequentially number, you could just start at 1 and your way up until they noticed that the site was sending the same useless response over and over. It also doesn’t send a 404 if the NID provided is invalid.

Second, no reasonable developer would expect you to hide a custom callback handler in template.php. It should be totally safe to load template.php without generating output under any condition (that should be true all non-tpl.php files).

Third, this code could run during any page request, not just the ones the application designer thought about. The request could have had Drupal do something relatively expensive before reaching this stage, and all that work was just wasted server resources – which creates an additional avenue for an attacker.

Fourth, Drupal has an exit function that actually does useful clean up and allows other modules to do the same. All that gets bypassed when you just die() midstream.

Finally, Drupal has tools to do all this. There was no reason to do this so badly.

Better Solutions

In Drupal 8 this is part of core. Enable Restful Web Services and optionally the RestUI module, and in a few minutes you can have this more or less out of the box.

In Drupal 7 are two modules that will do 90% of the work for us. If we really just want the raw node as JSON you could use Content as JSON. But often we want more control over field selection, and the option to pull in related content (which the developer in this case did, and used as his argument for the approach taken). Views Datasource gives us the power of Views to select the data and provides us a JSON (and a few other) display option.

Views Datasource based approach:

Install Views, ctools, and Views Datasource
Create your view and set the display format to JSON data document.
Pick your fields, set the path, and define the contextual filter:

From there save your view and you’re done. That’s all there is to it. No custom code to maintain, you get to rely on popular community tools to handle access checking and other security concerns, and you get multiple layers of caching.

So what happened?

This sin never saw the light of day.

At the time I encountered this “solution” I worked for a company that was asked review this site while it was still under development. Our code review gave the client a chance to go back to the developer and get a fix. The developer chose a more complicated solution than the Views one presented here (they defined a custom menu router with hook_menu() and moved much of this into the callback and added a few security checks) which was good enough for the project. But I still would have done it in views: it is much faster to develop, views plays nicely with Drupal caching to help improve performance, and is a straightforward approach is easy for a future developer to maintain.

Share your sins

I’m always looking for new material to include in this series. If you would like to submit a problem with a terrible solution, please remove any personally identifying information about the developer or where the code is running (the goal is not to embarrass individuals), post them as a gist (or a similar public code sharing tool), and leave me a comment here about the problem with a link to the code. I’ll do my best to come up with a reasonable solution and share it with SC DUG and then here.

If there are security issues in the code you want to share, please report those to the site owner before you tell anyone else so they can fix it. And please make sure no one could get from the code back to the site in case they ignore your advice.

This Week’s Drupal Fire Drill

This week many in the Drupal community lost a lot of sleep Tuesday night because the security team treated us to a warning about major security updates due out on Wednesday. Fortunately for many it wasn’t a crisis in the end, but it gave us all a chance to practice for the worst. Basically, it was like a fire drill in a elementary school: we got to prepare like there was a disaster, but since they wasn’t one we don’t really know how it would have gone if there was actually a fire. We haven’t had a stop-drop-and-roll type of emergency in a while, so it was a good refresher on how to handle a crisis.

For those who don’t know what I’m talking about here’s a quick review. At Cyberwoven, like many Drupal shops, we follow the Drupal Security twitter feed on one of our Slack channels so we saw this mid-afternoon Tuesday:

I read the PSA with images of Drupalgeddon dancing in my head:

There will be multiple releases of Drupal contributed modules on Wednesday July 13th 2016 16:00 UTC that will fix highly critical remote code execution vulnerabilities (risk scores up to 22/25).These contributed modules are used on between 1,000 and 10,000 sites. The Drupal Security Team urges you to reserve time for module updates at that time because exploits are expected to be developed within hours/days. Release announcements will appear at the standard announcement locations.

Drupal core is not affected. Not all sites will be affected. You should review the published advisories on July 13th 2016 to see if any modules you use are affected.

Oh that bold line up there wasn’t part of the original announcement. On Tuesday we didn’t have a sense of scale, were we talking about modules that everyone uses on almost every site (ctools came up more than once). It’s the one thing I wish the security team had done differently: given us that sense of scale.

I read all security postings, and make sure we take prompt steps to address them for clients as needed, but the potential here was that we’d have to update all 70+ sites in a few hours or less which is very different from your run-of-the-mill security update that often aren’t related to use cases and threat profiles for the majority of sites.

Here’s what we did next:

Tuesday

Took a minute to panic, complain, and joke about pending illnesses. This is actually a useful step because it allowed me to burn off some nervous energy and then to focus on the real work.
Pulled out the list of all active clients with Drupal sites, and doubled checked it for accuracy.
Made sure a developer had a working repo for all sites (70+). Since we had a couple people out of the office, and some projects had been reassigned recently to different developers, this was an important step to make sure no sites fell through the cracks during a rush to update them all.
Made sure we knew which were 6 sites, and which were 7 in case we are able to determine that 6 is also affected. Since I knew the announcement would likely skip D6, we needed to accept that we might have to take those sites offline for a time.
Made sure leadership knew that all developers may be busy start at 16:00 UTC on Wednesday. We didn’t actually cancel anything right away, but I didn’t want anyone surprised if we were all too busy posting and testing updates to worry about things like meetings.
Made sure complex projects were thought about ahead of time: sites with unusual setups or ongoing dev work that make sudden updates complex. For example we have one client that has 16 sites that all have an unusual set up, so we agreed who would handle those and made sure she was prepared.

Wednesday

First thing in the morning I saw the update to the notice that gave us a sense of scale and relaxed a little, but still made sure we were fully prepped.
Noon: the announcement of what modules were affected was released and a couple other developers and I immediately reviewed the releases. We relaxed once we determined none of our clients were using any of the modules listed.
I reviewed code from each of the three modules to see what the change was to look for ways to improve my own code to avoid similar errors.
Looked for ways to improve our response for the next time it’s not a drill.

Things would have been more exciting if we’d had to update our sites. Since we were prepared it was a matter of minutes for us to check that all our sites were secure. Each developer checked all the sites in their sandbox, and since I knew all sites were in someone’s sandbox that gave us 100% coverage without having to do lots of double checks.

I think it is too easy to look past doing the code review of modules we weren’t using but I find this kind of follow up really useful. Looking back on Drupalgeddon it’s amazing how much pain was caused by such a small error (16 characters are all that were needed to fix it). And by seeking to understand what went wrong you can look for places that you make similarly invalid assumptions.

If you read my post on making new mistakes, you also know I believe that looking for improvement is the most important detail (particularly when it turned out to be a drill, not a fire). Here’s my initial list of things to improve:

Have a system to automatically check every site for specific modules (this was already under development but will take a little while longer to complete).
Make sure at least two developers have a working sandbox for all projects at all times in case something comes out during a vacation.
Improve internal messaging about what to expect – template message and process. I tossed together some disjointed thoughts for account managers. But disjointed developer thinking does not make people feel like you’re on top of things.
Have better tracking of outliers: I completely missed that I had a demo site on Pantheon that did have the Coder module running. Since it was Pantheon they alerted me to this problem and had taken steps until I could do the update myself. But it would have been bad if that site had been someplace else and/or in production.
Make sure everyone one knows when the actual release is coming out, and what the outcome was. Several developers were hoping to get lunch before the updates, but hadn’t done so when the announcement came out (which could have been a problem if we’d ended up busy). And I spent the rest of the day answering one-off questions from the account team who wanted to know if the announcement had been bad news.

Ideally we’d come up with a method to automate security updates (maybe all updates), but that’s not totally straightforward. We have to worry about required patches, non-standard setups, automated testing, and other details. There has been discussion on the Pantheon power user’s mailing list, but every shop has a slightly different workflow (like the fact that we don’t use Pantheon much at all) so we’ll need to come up with a system that accommodates our system.

When Should I Update my Drupal Site to Drupal 8?

Last year Drupal 8 finally arrived, and brought the question that comes with every new release of Drupal: when should I update? New releases of Drupal mean two things: new features and cool new tools, and the retirement of an old version. We got the power and flexibility of Symfony and Drupal 6 sites are no longer getting community support. Unlike WordPress, which has well defined upgrade paths, each version of Drupal is a new adventure in upgrade pain. The more I watch people suffer with this pain, and the more I watch them try to find a way to do upgrades that preserve their site’s fundamental structure, the more I come to the conclusion that this pain is telling us something: we’re doing it wrong. Not because Drupal’s strategy is wrong, but because keeping all your content in the same structures is usually wrong. Drupal 8 should not make it easy for you to continue to use an old strategy, it should encourage us to update old assumptions.

Here is how I encourage everyone to view their choices:

If you have a Drupal 6 (or older) site you should update right now. Drupal 6 is no longer getting security updates so you are on borrowed time. But more importantly Drupal 8 is a better tool for the current state of the web than your Drupal 6 site. Most sites running on D6 reflect an online communications strategy that’s at least 4 or 5 years old. Those sites probably aren’t responsive, aren’t prepared to support apps, don’t have the right focus on social media and user engagement, and make assumptions about user behaviors that have evolved. Skip to Drupal 8: do not migrate these sites to Drupal 7. If there is a tool that is missing from Drupal 8 that your current site uses make sure you need it before complaining (or paying to have someone port it for you). Maybe that tool hasn’t been ported because it doesn’t make sense anymore. Some things are still missing, but lots of things are being rewritten differently because we have a better platform. The community is smarter than it was 5 or 10 years ago, and the platform is better, take the time to figure out why something hasn’t been ported: is it just no one has bothered, or has something better been built instead?

If you have a Drupal 7 site you should update when your web site no longer supports your work. This is actually the same advice I just gave, but without a few assumptions like “you need a secure site.” Many Drupal 7 sites have a lot of life left in them. A site you built today will be designed to meet the needs you have now, and the ones you foresee in the near future. Three years from now (when Drupal 7 is scheduled to lose support from the community) you will be operating on assumptions that have probably been wrong for at least two years. Every six months you should ask yourself: does my site reflect my online strategy, and is my strategy still working? If the answer is yes to both of those questions you are fine, if the answer is no to either – particularly the second – you should engage someone to help you update your strategy and rebuild your site.

I’ve been part of projects that failed in part because we tried to port a stale strategy and stale content to a fresh site. We broke the new site before it even launched. Don’t try to make Drupal 8 behave like your old site: embrace the change.