Is it wise to write superglobal $_POST directly to a database?

Question

I would like to have a submit code that works with different forms like:

<?php 
 $type = $_GET['type'];
 if(isset($_SESSION['id']))
  handle_form();
 else 
  include 'form_'.$type.'.php'; // different fields based upon type
?>

So I was wondering if it was wise to loop through superglobal $_POST writing all keys and values to the database. Something like:

<?php 
 function handle_form() {
 $query = "";
 foreach($_POST as $key => $value) {
  $query .= mysql_real_escape_string($key)."='".mysql_real_escape_string($value)."' AND ";
 }
 mysql_query("UPDATE ".$_POST['type']." SET ".substr($query,0,-4)."WHERE `id` = $_SESSION['id']");
 }
?>

Or is this a very insecure approach for handling forms and is it better to hardcode all fields in the corresponding 'form_type.php'?

Building queries by concatenating strings coming from user input is never safe. What if `$_POST['id']` is set to "`0 OR id IS NOT NULL`"? — laurent, May 20 '12 at 12:37
Oke, I made a mistake while writing this snippet, in real the id comes from a session. (Will edit that.) But further I think it would be safe, because if somebody messes with the form a mysql_error will prevent the submission right? — Neograph734, May 20 '12 at 12:43

score 4 · Accepted Answer · answered May 20 '12 at 12:45

That is a really bad idea, because then any extra POST fields that don't correspond to columns would break the query. Your own code uses $_POST['type'] to determine which table you're updating, which would break as well unless you unset it beforehand.

Also, you are not doing any validation on the data that is actually being sent to the DB. Changing your own name/permissions to Admin? Sure. Changing another user's email to your own? Go ahead. Making your account balance +100,000$? No problem. Mark a product as "price $1"? Yup.

It would be a lot smarter to have some kind of Active Record. If you're writing a larger application, look into a framework such as Yii or Cake PHP, they come with such functionality built in.

Aside from that: Please stop writing new code with the ancient mysql_* functions. They are no longer maintained and community has begun the deprecation process. Instead you should learn about prepared statements and use either PDO or MySQLi. If you care to learn, here is a quite good PDO-related tutorial.

Oke, I could add the type to the session as well. As for the validation, it is for editing multiple textboxes on a user's page. So the user could mess up his own text. thanks for the info on query's :) — Neograph734, May 20 '12 at 12:51

score 2 · Answer 2 · edited May 23 '17 at 11:56

Everything that comes from an untrusted outside source is not safe to insert into the database or anywhere else because you risk Code Injection. In case of databases you risk SQL Injection. You have to sanitize the input. This applies for any of the Superglobals as well as for any other data you do not control, for instance a web service.

Escaping data with mysql_real_escape_string is the minimum you need to do but it does not protect you from all possible injection attacks. It's purely syntactic and cannot detect tampering with data values that are otherwise syntactically correct.

Apart from that you should not be using ext/mysql anymore. It is outdated and will be deprecated. Use ext/mysqli or PDO and use prepared statements.

is an sql injection still possible while using mysql_real_escape_string() ? — Neograph734, May 20 '12 at 12:45

score 0 · Answer 3 · answered May 20 '12 at 12:44

This is a potential nightmare. Never trust your users.

Each $_POST parameter should be examined as :

Is it part of the whitelist of accepted parameters ?
Has it the right datatype or value range ?

Then, please, do use Prepared Statements and not plain mysql_query() calls.

Is it wise to write superglobal $_POST directly to a database?

3 Answers3