4

I'm letting users update their name with this code.

    $dbh = connect();
    $q = $dbh->prepare('UPDATE Users SET username=:name WHERE User_ID=:id LIMIT 1'); 
    $q->bindParam(":id", $loggedInUser->user_id, PDO::PARAM_INT);
    $q->bindParam(":name", $_GET['name'], PDO::PARAM_STR);
    $q->execute();

A) is this enough to sanitize information? b) when I put HTML tags in there like <b>name</b> it actually shows up in bold on my site! Is there an option where I can have PDO strip out all HTML?

onteria_
  • 68,181
  • 7
  • 71
  • 64
Nikki
  • 79
  • 1
  • 8

3 Answers3

2

Looks reasonably sound. I would suggest using POST instead of GET for destructive / manipulative operations though. You're far less likely to suffer from CSRF attacks if you stick to POST data though it does not make you totally immune.

If you do not actually want users to enter HTML into the name field, don't worry about filtering data on the way into the database. Escape it on the way out via htmlspecialchars() or htmlentities().

I've always stood by the idea that data should go into the database as raw as possible.

Edit: Almost forgot, make sure the expected values in $_GET / $_POST actually exist before attempting to use them, eg

if (isset($_POST['name'])) {
    // now use it
Phil
  • 157,677
  • 23
  • 242
  • 245
  • Which one is safer? (Don't really care about speed/resources) – Nikki May 23 '11 at 23:50
  • 1
    @Nikki Which "what" is safer? `htmlspecialchars()` vs `htmlentities()`? I suggest you read the manual and determine which one best suits your purpose. – Phil May 23 '11 at 23:51
  • Thanks - htmlentities is the one I will go with as it does ALL html, not just special characters. – Nikki May 23 '11 at 23:55
  • I know I'm 12 years late to the party, but I do htmlentities($variable, "ENT_COMPAT"). I also do it going INTO the database because I won't want any unwanted sql functions being triggered during the insert/update/whatever I'm doing. – jpgerb Mar 16 '23 at 20:33
  • @jpgerb that's not possible with parameter binding. Please see [this answer](https://stackoverflow.com/a/38411974/283366) for why encoding data in the DB might not be a good idea – Phil Mar 16 '23 at 22:28
1

A) Read manual:

The parameters to prepared statements don't need to be quoted; the driver automatically handles this. If an application exclusively uses prepared statements, the developer can be sure that no SQL injection will occur (however, if other portions of the query are being built up with unescaped input, SQL injection is still possible).

B) Never trust to user's data. Use htmlspecialchars in output.

C) Use $_POST and tokens for queries, which will change any data, to avoid CSRF.

OZ_
  • 12,492
  • 7
  • 50
  • 68
  • 2
    Using POST will ***not*** avoid CSRF. – Alix Axel May 24 '11 at 00:39
  • 1
    @Alix Axel Using POST will allow to avoid most of CSRF attacks. And using POST with tokens will help to avoid them all :) – OZ_ May 24 '11 at 00:42
  • 1
    Using GET with tokens will also avoid any CSRF attack. Using POST (without tokens) will only help avoid CSRF attacks from people that don't know how to POST. I think you should rephrase your last point, since it is a critical issue. – Alix Axel May 24 '11 at 01:29
0

Never trust user input! At a minimum, wrap the $_GET['name'] in a sanitizing function, like mysql_real_escape_string() to prevent SQL Injection attacks. And then when you're outputting user-supplied data, make sure to wrap it in htmlspecialchars() to prevent Cross-Site Scripting (XSS) attacks.

Dan
  • 2,157
  • 20
  • 24
  • I thought PDO would take care of escaping the data when you prepare the statements like I did above. Is htmlspecialchars enough to sanitize against all XSS? – Nikki May 23 '11 at 23:45
  • -1 for `mysql_real_escape_string()`, using bound parameters is much better. +1 for `htmlspecialchars()` though – Phil May 23 '11 at 23:46
  • @Nikki - yes prepared statements and bound parameters are better than any attempt at sanitizing text. I agree with Phil about the -1/+1 – Stephen P May 23 '11 at 23:51