Is this way of handling POST and GET data a good practice?

Question

After reading this article, it made me wonder if this is actually a good practice for registering new users. Everyone with some PHP studied can see how it works, but I just feel repeating myself if I have to handle all the post data manually. I know it's not 'difficult' nor too long to do at once, but I think it could be handled in a better way on the long run if you implemented it something similar to this code. For example, to add a single field more you have to change a lot of code, doing copy/paste in the article, but here it's only one field more in the array $ValidFields. What do you think?

function registerUser()
{
// Only place (apart of the mysql table, obviously) to add new fields of all the script.
$ValidFields = array ("name","lastname","email","password");
$tablename="users";                  // If oop, this could be done in the __construct()
foreach ($_POST as $key => $value)
  if(in_array($key,$ValidFields))
    {
    $key=mysql_real_escape_string($key);
    if ($key=="password") $value=md5($value);
    else $value=mysql_real_escape_string($value);
    if (!$mysql)  // If there is nothing inside
      {
      $mysql="INSERT INTO ".$tablename." (".$key;
      $endmysql=") VALUES ('".$value."'";
      }
    else
      {
      $mysql.=", ".$key;
      $endmysql.=", '".$value."'";
      }
    }
$mysql=$mysql.$endmysql.")";
return $mysql;
}

Tested adding this code after the function

$_POST['name']="testname";
$_POST['lastname']="testlastname";
$_POST['email']="teste'mail";       // Checking MySQL injection (;
$_POST['password']="testpassword";
$_POST['CakePHP']="is_a_lie";       // "Hello world" is too mainstream
echo registerUser();

And the returned string is, effectively:

INSERT INTO users (name, lastname, email, password) VALUES ('testname', 'testlastname', 'teste\'mail', 'testpassword')

NOTE! I know that I should not use mysql_, this is only an illustrative script. There are many statements in php5 (PDO, MYSQLi, etc) that everyone should use. I'm focusing on scalability and performance. A similar process could be reproduced for creating the HTML form. Also, it should work similar with a class.

I'm just wondering why, in so many years PHP has been developed and in the 1 year something I've been studying it and searching for information online, I haven't seen any similar and maybe more efficient way of handling POST or GET data.

So, you're placing this into a function to sanitize the data? Sure, it speeds up the process, that's what a function is for. — Paul Dessert, Sep 04 '12 at 18:46
To sanitize and to insert all the data at once, without manually hardcoding each column (or being able to hardcode it only in a Config file) — Francisco Presencia, Sep 04 '12 at 18:48
Variables *should* be sanitized individually, in most cases. Only the programmer knows if he's expecting a string, a number, a date, or a block of HTML. If you have a very large number of user-provided data, you can name them with an array and loop through them that way. — Blazemonger, Sep 04 '12 at 18:49

score 2 · Accepted Answer · answered Sep 04 '12 at 18:44

I do not handle $_GET and $_POST at all. Instead I use parameter binding on my queries.

So my insert looks like this:

public function Insert( $table, array $bind )
  {
    $this->fetch = function( $sth, $obj )  { return $obj->lastID = $obj->lastInsertId(); };
    $this->sql = array();
    $this->bindings = array();

    $columns = implode( ", ", array_keys( $bind ) );
    $values  = ':' . implode( ", :", array_keys( $bind ) );

    foreach ( $bind as $column => $value )
      $this->bindings[] = array( 'binding' => $column, 'value' => $value );


    $this->sql['insert'] = "INSERT INTO " . $table . " (" . $columns . ")  VALUES (" . $values . ")";

    return $this;
  }

And the execute looks like this:

  public function Execute()
  {
    $sth = $this->prepare( implode( ' ', $this->sql ));
    if( !$sth )
      return false;

    foreach ( $this->bindings as $bind ) 
      if( $bind['binding'] ) {
        if( isset( $bind['type'] ))
          $sth->bindValue( ':' . $bind['binding'], $bind['value'], $bind['type'] );
        else
          $sth->bindValue( ':' . $bind['binding'], $bind['value'] );
      }

    if( $sth->execute() ) {
      $lambda = $this->fetch;
      return $lambda( $sth, $this );
    }
    else 
      return false;
  }

I posted in the question how I can do it, but I don't know the proper oop way. Now I'll study and learn from your code, thank you. — Francisco Presencia, Sep 04 '12 at 19:15

score 0 · Answer 2 · answered Sep 04 '12 at 19:14

0

This is absolutely not a good practice -- mysql_real_escape_string is ONLY safe for esaping data which is safely contained within single quotation marks. It cannot safely be used for any other purpose.

As a result, it is possible to inject malicious content into your query by setting a crafted POST key -- for instance, one could set

$_POST["name) SELECT CONCAT(name, CHAR(58), password) FROM users --"] = "";

to create one user in your database for every existing user, with a name indicating the password of the existing user. If your application displayed a list of users publicly, this would have the effect of making all users' passwords public. More nuanced attacks are also possible; this is just a simple example.

answered Sep 04 '12 at 19:14

I stated it's pseudo code, I should add md5() AT LEAST for the passwords plus some other fine-tuning. It can mislead some others, I will correct it. The question was about the general idea of handling the idea in this way. About the other bit, I'm not sure what you mean. Remember that the POST data I wrote is not there in the actual code. So isn't `$_POST['']` considered single quotation marks? – Francisco Presencia Sep 04 '12 at 19:18
No, I mean single quotation marks in the SQL query. You cannot safely use `mysql_real_escape_string` to escape non-string content like field names. – Sep 04 '12 at 19:52
With regard to POST data -- I mean that a malicious user could craft a POST request with a field with that name. There are very few restrictions on what a POST field can be called. – Sep 04 '12 at 19:53
I see! This is exactly the kind of vulnerability that I was looking for to make the code invalid. Can you think of any solution? Thanks! – Francisco Presencia Sep 04 '12 at 20:18
Don't store user input (such as `$_POST`) directly to your database. Make sure that SQL metadata, such as field names, can only be generated from constant strings within your application. – Sep 04 '12 at 20:37

Is this way of handling POST and GET data a good practice?

2 Answers2