If I use node_save() to create a new node, do I need to manually escape user input?

Question

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?

Answer 1

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:

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

Instead, use proper argument substitution with db_query:

<?php
 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:

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

Instead, do this:

<?php
 $placeholders = implode(',', array_fill(0, count($from_user), "%d"));
 db_query("SELECT t.s FROM {table} t WHERE t.field IN ($placeholders)", $from_user); 
?>

Answer 2

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!