CODEWITHSUNDEEP
phpperformancepostscalability
Francisco Presencia
Francisco Presencia

Reputation: 8860

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

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.

Upvotes: 1

Views: 482

Answers (2)

user149341
user149341

Reputation:

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.

Upvotes: 0

JvdBerg
JvdBerg

Reputation: 21856

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;
  }

Upvotes: 2

Related Questions