CODEWITHSUNDEEP
phpsql-injection
Humpton
Humpton

Reputation: 1529

Using $_GET & $_POST

Maybe I'm doing something wrong right from the beginning, and if so I'll work on that too...

I have a menu item that as part of the URL passes an Event ID#. In my specific case it takes the user to an information page for that Event. Then there is a button that lets them sign up for the Event. Click and they're signed up for the Event and taken back to the same information page which now says they're signed up and lets them see a few extra things.

The first time they hit the page I use a $_GET to figure out the Event ID# which is then passed back to the page if they click the sign up button as a hidden input field. But this time I need to use $_POST to figure out the Event ID#. So the code in my query has a boolean part which looks like 

SELECT stuff FROM ... WHERE eventID = ($_GET["ID"] ? $_GET["ID"] : $_POST["ID"])

It works, but it just feels like it can be done better...

Upvotes: 3

Views: 742

Answers (5)

hbw
hbw

Reputation: 15760

  1. Sanitize your database inputs. mysql_real_escape_string() is a quick and easy way to achieve this (or, if the ID is always numeric, you could just use intval()). Don't be like the school who fell for Bobby Tables.
  2. If you're not sure which request method was used, use the $_REQUEST superglobal, which contains both GET and POST variables (example: $_REQUEST['ID']). I normally don't use $_REQUEST, since I like to be clear about where my data comes from, but this would be a perfect situation to use it.

As Nick Presta pointed out, $_REQUEST also includes cookie variables, and as a matter of fact, the default* order of precedence for name conflicts is $_COOKIE, $_POST, and then $_GET. In light of this, before you plug data into the query, you could either do what you're doing now, or use $_SERVER['REQUEST_METHOD'] instead:

// You can use mysql_real_escape_string() instead if you want
$id = ($_SERVER['REQUEST_METHOD'] == 'POST') ? intval($_POST['id']) : intval($_GET['id']);

Also, as outis noted, keep in mind that you have the option of using prepared statements instead of just raw SQL queries.

* — The ordering is configurable via the variables_order configuration directive, as Stewart mentioned in the comments.

Upvotes: 8

staticsan
staticsan

Reputation: 30555

You should separate the selection of the provided ID from where it gets put into the database. Put another way, as soon as the page knows whether it's been called via GET or POST it will know the correct super-global to retrieve the ID from. In fact, there may be some pages that will always be called with POST and others always with GET.

If it truly doesn't matter which it gets the ID from, use $_REQUEST.

Upvotes: 0

Nathan Kleyn
Nathan Kleyn

Reputation: 5143

For simplicities sake, combining previous suggestions for you:

"SELECT stuff FROM ... WHERE eventID = '" . mysql_real_escape_string($_REQUEST['id']) . "';"

And for the record, don't use double quotes for the array key unless you are going to include any variables or escapes that need to be parsed, it takes longer to process. For example:

$_POST['id'] and not $_POST["id"] unless you're doing something like $_POST["post_$id"]

Hope that helps.

Upvotes: 2

Dan McGrath
Dan McGrath

Reputation: 42038

As well as htw's answer, if the Event ID's available shown to users are based on permissions, make sure you revalidate that they are allowed that particular ID.
Just because it is in a hidden field, doesn't mean they can't change it.

Upvotes: 1

Sev
Sev

Reputation: 15727

It surely can be done better. First of all, sanitize user input before querying the database like you have. You have opened yourself up to SQL Injection.

Upvotes: 3

Related Questions