I am writing a custom module for Drupal 6 that programmatically creates a node using node_save().

The various custom node fields come from user input - is it necessary to manually escape these fields, or will node_save() take care of that for me?


If by escaping you mean using check_plain() or similar functions, then no; escaping is done when outputting something, not when saving it in the database, or in Drupal persistent variables.

Imagine what would happen if you call check_plain() on a node containing "&"; that character would be escaped to "&" that it would be converted in & when Drupal would show the node content. The result is that "&" would be shown as "&" and that is not what it would be expected.

In Writing secure code, you can read the following text:

Use check functions on output to prevent cross site scripting attacks

No piece of user-submitted content should ever be placed as-is into HTML.

  • Use check_plain() or theme('placeholder') for plain text.
  • Use check_markup() or filter_xss() for markup containing text.
  • Use the t() function with "@" or "%" placeholders to construct safe, translatable strings.

See how to handle text in a secure fashion for more details.

As you see, the page about writing secure code doesn't say to escape the text being saved, but only text being output, when the text has been entered from users, in a way or in another. An example of text entered from a user is the username or the signature associated to user accounts, the node title, or the node body.

About SQL injection, you just need to use placeholders in you queries; Drupal database functions escapes the content of the placeholders, which would not be possible if you write a query like the following one, for example:

$query = db_query("SELECT * FROM {node} WHERE title = '$title'");

The "Writing secure code" reports:

Use the database layer correctly. For example, never concatenate data directly into SQL queries, like this:

 db_query('SELECT foo FROM {table} t WHERE t.name = '. $_GET['user']); 

Instead, use proper argument substitution with db_query:

 db_query("SELECT foo FROM {table} t WHERE t.name = '%s' ", $_GET['user']); 

If you have to accommodate a variable number of arguments in your SQL, create an array of placeholders. Don't do this:

 db_query("SELECT t.s FROM {table} t WHERE t.field IN (%s)", $from_user); 

Instead, do this:

 $placeholders = implode(',', array_fill(0, count($from_user), "%d"));
 db_query("SELECT t.s FROM {table} t WHERE t.field IN ($placeholders)", $from_user); 
  • 1
    Sorry, I should have specified - I was talking about SQL injection. I will strip html tags when outputting it too. Aug 30 '11 at 19:57
  • The page on writing secure code doesn't, but the next page in the series does: drupal.org/node/101495 (handling user input with care) Aug 30 '11 at 20:11
  • That's the thing. I understand that using placeholders within db_query removes the risk, but I'm not using db_query - I'm using node_save(). Thanks for your answer, by the way :) Aug 30 '11 at 20:29
  • 1
    @John McCollum Yet, it's not about escaping the input of the user before saving it. The database API takes care of escaping the values, but the saved values are still what the user entered.
    – apaderno
    Aug 30 '11 at 20:29
  • 3
    node_save() doesn't need to escape the values because its code has been written following the points on how to write secure code. It's the database API that takes care of any security issues for SQL injection, and other problems; node_save() doesn't need to do anything extra.
    – apaderno
    Aug 30 '11 at 20:32

I quickly glanced through node_save and there is no escaping done on the $node object before drupal_write_record, drupal_write_record does no escaping either so even if I've overlooked something your best bet is to escape. Better safe than sorry!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.