Should you put $_POST data into variables?

Are there any benefits for putting POST data into variables and not working with them from the POST array values directly? (Using $age = $_POST[‘age’] instead of just $_POST[‘age’]). Kevin Yank recommends doing it in one of his books, but I do not know why. Using the POST array directly gives you an indication of where the variable came from, which provides an indicator that you might do something, like sanitize it. Plus, when using PDO, you don’t even have to sanitize it (for DB entry at least to prevent injections) as long you use prepared statements. The recommendation was from an older edition of one of Yank’s books where he did not use PDO, so maybe it is out dated now or there are other reasons I am missing?

One that springs to mind is quite minor - if you’re using the variable often, typing $age is a lot quicker than typing $_POST[‘age’] all the time.

1 Like

There is a major advantage - testability. Particular if your class methods receive $POST as an argument rather than work on the variable directly, it gives them the ability to easily work with a test that is providing a mock. Also, their functionality isn’t tied down to $_POST and could easily receive data from $GET at a later time (never use $REQUEST)

The biggest advantage is that as you validate or sanitize them during the move process you can be sure that the field you move them to is valid whereas the original $_POST field can contain anything. For5 $_POST since the user can enter anything you should validate what they entered and not just sanitize it.

Your assignment statements should always read something like:

$age = validateAge($_POST['age'];

Of course you need to sanitize values used with PDO (if you haven’t validated them) - otherwise you end up with a database full of meaningless junk.

Validating and sanitizing are not just for security, they are to make sure that the data is meaningful. If you don’t validate user input then they can enter anything at all and can fill your database with garbage.

In computer terms fields that haven’t been validated or sanitized are considered to be tainted the validation or sanitizing makes them untainted and that the naming convention used identifies which fields are which.

An alternative naming if you want to use values from an array throughout your code would be:

$valid['age'] = validateAge($_POST['age'];

Most books leave out this code from their examples because it would make the code four times longer and make the part of the code they are trying to discuss harder to see in amongst the validation part (which is always by far the largest part of any program).

3 Likes

These are all very good points, thanks.

I used to use a naming system where I named “cleaned” variables differently from unclean ones, but I stopped that once I started using PDO. I also used validation functions to change the names of variables, but naming variables based on whether they were sanitized for the DB vs validated by functions (made specifically for that variable’s content) became a bit tedious over time, especially for multiple validation functions on a single variable. (An example: $age might be validated by two separate functions, a function to validate general text (such as making sure no text contains two ‘\’) and a separate function to validate by age restrictions (such as making sure that $age is a number that falls between 18 and 100).)

You only need to distinguish between those that are not validated or sanitized($_POST, $_GET etc) and those that have been validated or sanitized. If you always validate or sanitize when moving values out of the tainted arrays then everything else will be untainted and it will be easy to tell one group from the other.

Just because you use PDO or switched sandwich fillings or replaced your car doesn’t change the need to validate or sanitize all inputs and to be able to tell the difference between variables that have from variables that haven’t.

1 Like

Just because you use PDO or switched sandwich fillings or replaced your car doesn’t change the need to validate or sanitize all inputs and to be able to tell the difference between variables that have from variables that haven’t.

I just nearly choked, laughing, when I read this - eating lunch while reading threads here. :smiley:

I used to use a naming system where I named “cleaned” variables
differently from unclean ones, but I stopped that once I started using
PDO.

Prepared statements prevent SQL injection reasonably well (there may be an exploit I’m unaware of), but even code working with PDO can be defeated if you’re careless, as the drupal team recently discovered. A section of the drupal SQL wrapper was written to automate the handling of “in” clauses - for example

SELECT * FROM customers WHERE name IN ( )

The number of parameters for the in clause varies from 2 and up. The function receives the in parameters as a list and relied on the keys to prepare the statement

SELECT * FROM customers WHERE name IN ( :name-0 :name-1, :name-2 ) 

Unfortunately this overlooked the fact that what PHP calls arrays other languages would call dictionaries or maps, because the keys of the array do not have to be numerical. Hence the code could be tricked at execution time into preparing a statement with an injection in it.

So PDO provides a layer of defense - but just that, a single layer. You should try to place as many layers of security in your code as you can.

Also, the protections in PDO against injection do not protect from cross site scripting attacks, CRF forgeries and a whole gaggle of other exploits. Sanitize your inputs!!

( Note - several PHP haters have posted the nature of the drupal exploit on blogs across the net so the details of this exploit will hardly be news to those inclined to research such things and exploit them. As a general rule, illustrating how an attack works publicly on this site or another is not recommended. In this particular case the attack is already too well known for this post to cause further harm. If you run drupal make sure it is kept upgraded. This particular bug affects all 7.x versions before 7.32. At the time of this writing the current version is 7.34 )

For more information on the fix see https://www.drupal.org/SA-CORE-2014-005

1 Like

I’m not sure I’m a fan of throwing the kitchen sink at the problem and hoping something sticks. If we have multiple layers of security, then what tends to happen is our security checks get spread around, and then there’s no longer any one place you can look to know if you’re secure or not.

Drupal’s code is a great example. After the fix, we can look at the code and know that $i will be a whole number and therefore safe. But what about $key? What sort of value is that? Is it safe to use in a SQL string? I’m guessing it’s been sanitized earlier in the program, but from the surrounding code, there’s no way to know that.

The bottom line rule for avoiding SQL injection is: Don’t use unknown input to build the SQL string – any part of the SQL string. Typically, we’d use user input for values, and that’s the part that prepared statements fixed. It let us use placeholders and send the values separately. But in Drupal’s case, they were – and still are, in the case of $key – using unknown input to make the placeholders themselves.

I’m not sure I’m a fan of throwing the kitchen sink at the problem
and hoping something sticks. If we have multiple layers of security,
then what tends to happen is our security checks get spread around, and
then there’s no longer any one place you can look to know if you’re
secure or not.

I learned a long time ago that it is impossible to stop idiots from being idiots. Blocking innocent mistakes is one thing, blocking every possible mistake the module programmers could make is impossible. Even if you do lock it down entirely, nothing stops an idiot from circumventing your lock down.

Drupal’s code is a great example. After the fix, we can look at the code and know that $i will be a whole number and therefore safe. But what about $key?
What sort of value is that? Is it safe to use in a SQL string? I’m
guessing it’s been sanitized earlier in the program, but from the
surrounding code, there’s no way to know that.

If finding out rather than spreading FUD is your goal a simple CTRL-F in sublime text is your friend. expandArguments is protected, so it can only be called from elsewhere in the class (this very sort of thing is why having protected and private methods is important). Looking at the calling function, $key has to be a field name set by the calling module, not by the user. If the calling module let’s the user pass field directives in, the author of that module is an idiot.

The bottom line rule for avoiding SQL injection is: Don’t use unknown input to build the SQL string – any
part of the SQL string. Typically, we’d use user input for values, and
that’s the part that prepared statements fixed. It let us use
placeholders and send the values separately. But in Drupal’s case, they
were – and still are, in the case of $key – using unknown input to make the placeholders themselves.

At what point does responsibility fall on the module author? I can see not thinking to filter the array keys of the in clause, but filtering the key arguments before invoking the library should be done. I suppose they could harden it further by requiring the field keys to be alpha characters only. But even then, at some point the end programmer has got to take some responsibility.

I don’t think I agree with that. Just like when we bind a value with PDO, we have the reasonable expectation that the value we bind could be literally anything and we’d still be safe from injection. Drupal’s database abstraction layer should provide the same kind of guarantee. One of it’s purposes, after all, is to enforce security checks. They shouldn’t make any assumptions about the data, especially when they use that data directly in the SQL string.

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.