Use `$wpdb` functions for insert/update.

[boonebgorges]

Concatenating MySQL query strings is dangerous. You are using sanitize_title_for_query() pretty extensively to avoid SQL injection. This is generally effective, but there are two significant problems with it:

1. You need to remember to do it. I didn't see any places where you'd forgotten, but often the sanitization takes place in a totally separate file (like with $current_user_id, which you're trusting from the $current_user global. This makes it very hard to keep track of whether you're missing something.
2. sanitize_title_for_query() is a very restrictive function. Anything passed through this function is sanitized using sanitize_title_with_dashes(), which strips everything but [a-z0-9_-]. In other words: URL-safe characters. Sometimes this is what you want. But sometimes it's way too conservative. If all you want to do is sanitize against SQL injection, $wpdb->prepare( '...foo = %s...', $foo ) is enough.

In the attached changeset, I showed a few examples of how to use the $wpdb insert() and update() helpers. They do all of the SQL concatenation and sanitization for you - you just provide the values and the placeholders. (More documentation: http://codex.wordpress.org/Class_Reference/wpdb#INSERT_row)

If you don't like these helpers, and still want to concatenate INSERT and UPDATE queries - to send to your log file, for example - at the very least you should be using $wpdb->prepare in every case:

// Always
$query = $wpdb->prepare( "INSERT INTO $table (user_id,foo,bar) VALUES (%d,%s,%s)" ), $user_id, $foo, $bar );

// Never
$query = "INSERT INTO $table (user_id,foo,bar) VALUES '" . $user_id . "', '" . $foo . "', '" . $bar . "'";

[boonebgorges]

(There are other places in the plugin that need a similar treatment - I just picked a couple for demonstration.)

[neelakansha85]

I do remember that I was running into some issues related to the $wpdb insert() and update() helpers due to their restrictive forms. (Probably could have also been my lack of understanding of the insert() and update() fields at that point)

Additionally I wanted to keep the user input totally restrictive by allowing only values that the system should accept such as for $site_address_bucket and $site_address where I wanted to allow only [a-z0-9] for I concatenate them with a -. But I do agree with you that it did give me a hard time trying to keep a check of using sanitization everywhere required. I will rewrite all the sql queries using insert() and update() to see if it works out this time. Thanks for pointing that out.

[boonebgorges]

Yeah. I totally understand the need to validate user input by removing invalid characters.

But it's generally helpful to separate this kind of validation from the task of sanitization, ie prevention against MySQL injection. So, go ahead and run things through sanitize_title() - this will limit the character set as you need. But by always using $wpdb->prepare(), or the insert/update helpers, you will be covered against injection no matter what kind of data validation you're doing.

On 02/05/16 10:22, Neel Shah wrote:

I do remember that I was running into some issues related to the $wpdb insert() and update() helpers due to their restrictive forms. (Probably could have also been my lack of understanding of the insert() and update() fields at that point)

Additionally I wanted to keep the user input totally restrictive by allowing only values that the system should accept such as for $site_address_bucket and $site_address https://github.com/neelakansha85/nsd-site-setup-wizard/blob/master/admin/step2_process.php#L13-L17 where I wanted to allow only [a-z0-9] for I concatenate them with a |-|. But I do agree with you that it did give me a hard time trying to keep a check of using sanitization everywhere required. I will rewrite all the sql queries using insert() and update() to see if it works out this time. Thanks for pointing that out.

— Reply to this email directly or view it on GitHub https://github.com/neelakansha85/nsd-site-setup-wizard/pull/10#issuecomment-180427555.

[neelakansha85]

I have updated the code with more $wpdb->prepare() and will continue to do so with next versions.