6

Possible Duplicate:
The ultimate clean/secure function

I was informed in another thread that this bit of code was pretty useless:

function getPost($s) {
        if (array_key_exists($s, $_POST))
            return mysql_real_escape_string(htmlspecialchars($_POST[$s]));
        else return false;
    }


    function getGet($s) {
        if (array_key_exists($s, $_GET))
            return mysql_real_escape_string(htmlspecialchars($_GET[$s]));
        else return false;
    }

Can anybody help understand why and how I can make it better please? Links or references are welcome also.

Just trying to always improve :)

Community
  • 1
  • 1
jribeiro
  • 3,387
  • 8
  • 43
  • 70
  • 2
    See [The ultimate clean/secure function](http://stackoverflow.com/questions/4223980/the-ultimate-clean-secure-function) bottom line: wholesale sanitation is useless. Sanitize data when it's being used, with the correct method for that use. (There's nothing speaking against the basic idea of putting the sanitation into a function, though, to write less code.) – Pekka Feb 25 '12 at 17:04

2 Answers2

11

Well, it's bad for the same way magic_quotes_gpc is bad. It's magic and will escape everything, whether you want it to or not. Instead, handle the escaping where it's used, and you can change things without any problem. So:

function post($key) {
    if(array_key_exists($key, $_POST)) {
        return $_POST[$key];
    }

    return false;
}

And do your escaping where it's needed. Otherwise, things can look strange, and unescaping them will defeat the point. Consider this; I input my last name, O'Hara, in a textbox. You want to echo it back, but you fetch it using getPost. Here's what I get back:

O\'Hara

Did you htmlspecialchars it again? Well, then I get:

O\'ara

or something. This happens to me a lot and it's incredibly annoying - please don't do it.

Ry-
  • 218,210
  • 55
  • 464
  • 476
  • Hmmm i don't use it in all POST values. It's just for re-use when really needed. Anyway great answer. Thanks – jribeiro Feb 25 '12 at 17:13
8

I wouldn't say useless, just a bit misguided. You should do the escaping immediately before you use it in the context it needs to be escaped for. For example, if you want to send the value back to the browser you might do this:

echo htmlspecialchars($_GET['name']);

But if you want to send it to the database you might do this:

mysql_query(... 'INSERT INTO users VALUES ("'.mysql_real_escape_string($_GET['name']).'")');

With your method you are fixed in what you can do with it. If you do this:

echo getGet('name');

You are going to print out a MySQL escaped string rather than the actual name.

spencercw
  • 3,320
  • 15
  • 20