First, the issues I have with your code:
-
eval
is very, very rarely needed, and extremely dangerous, use with caution. I've been developing in PHP for over 10 years, and never really encountered a situation that needed eval
. This is no exception. Eval is not required
- You're sanitizing the entire
$_POST
array. That's great, but there are special functions for doing that, like: filter_input_array
, array_filter
and many, many more... not to mention ready-made, open source projects and frameworks that already contain a solid request validation component.
- Always check the return values of functions, which you seem to be doing with
strstr
, but be weary of functions that return different types (like strstr
: it returns false
if the needle isn't found, but returns 0
if the needle is found at the start of the haystack string). Your if
statement might not work as intended.
- You're assuming
sanitize($value)
values will not contain any single quotes. Why? Because if they do, you'll end up with a syntax error in your evaled string
Be that as it may, you could easily write your code using variable variables and add a simple check not to step on existing variables in scope:
$sanitized = array_filter($_POST, 'sanitize');//call sanitize on all values
foreach ($sanitized as $key => $value)
{
if (!isset($$key) && strstr($key, 'removeFile') === false)
$$key = $value;
}
But really, $_POST
values belong together, they are part of the request, and should remain grouped... either in an array, or in an object of some sort. Don't assign each value to its own variable, because pretty soon you'll loose track of what variables are set and which are not. Using an unset variable creates that variable, assigning value null
, so what you have now makes for very error-prone code:
//request 1: POST => id=123&foo=bar
foreach ($sanitized as $k => $v)
$$k = $v;
$query = 'SELECT x, y, z FROM tbl WHERE id = ?';//using posted ID as value
$stmt = $db->prepare($query);
$stmt->execute(array($id));
All is well, because $id
was set, but never trust the network, don't assume that, just because $_POST
is set, all the keys will be set, and their values will be correct:
//request 2: POST => foo=bar&page=2
foreach ($sanitized as $k => $v)
$$k = $v;
$query = 'SELECT x, y, z FROM tbl WHERE id = ?';//using posted ID as value
$stmt = $db->prepare($query);
$stmt->execute(array($id));//id is null
Now we have a problem. This is just one example of how your code might cause issues. Imagine the script grows a bit, and look at this:
//request 3: POST => id=123&foo=bar&page=2
foreach ($sanitized as $k => $v)
$$k = $v;
//$id is 123, $foo is bar and $page = 2
$query = 'SELECT x, y, z FROM tbl WHERE id = ? LIMIT 10';//using posted ID as value
//a lot more code containing this statement:
$page = someFunc();
$log->write('someFunc returned log: '.$page);
//more code
$offset = 10*($page-1);//<-- page is not what we expected it to be
$query .= sprintf(' OFFSET %d', $offset);
$stmt = $db->prepare($query);
$stmt->execute(array($id));
Now this may seem far-fetched, and idiotic, but believe me: all of these things happen, more than I care to know. Adding some code that accidentally overwrites an existing variable that is used further down happens all the time. Especially in procedural code. Don't just blindly unpack an array. Keep that single variable, and use the keys to avoid:
- grey hair
- sudden, dramatic baldness
- loss of sanity
- bleeding ulcers
- In a work environment: catastrophic loss of data
- Sudden loss of job
- ... because code like this makes unicorns cry, and bronies will hunt you down