Undefined Index - Security Risk vs Performance vs Code Bloat

Question

I'm collaborating on a project where the REST APIs basically break in development mode because it has a more include error reporting policy. Here's a typical line in this project:

public function someAction() {
   // Returns a map of params => values sent with HTTP req
   $params = $this->getParams();

   // This key may not exist --+
   //                          |
   //                          v
   $someField = $params['someField'] ?: 'default value';
   $someField = $this->sanitizeInput($someField);

   // ...
}

As a result, the JSON response in dev mode will often be littered with PHP: Notice: Undefined Index warnings, which will break the JSON output string.

My questions

• What exactly is the security risk (if any) in assuming that a variable has been initialized, particularly when pulling it from $_GET or $_POST?
• Would it be worth the trouble to go through and wrap every access to some assumed array key with isset() or array_key_exists()?
• I've added isset() around individual keys that raise undef index warnings under certain actions throughout the app, but the code looks super bloated now...

Answer 1

The issue with ignoring errors like this is exactly what you have found - debuggings becomes a huge pain, and pottential real bugs get dismissed as "normal behaviour".

However, as with any other time in programming, if you find yourself writting the same code over and over, you probably need to write an abstraction.

In your case, you can add an additional method to the class, as well as getParams (which presumably just returns the contents of $_REQUEST), add a getParam() method:

function getParam($key, $default=null)
{
    return isset($_REQUEST[$key])? $_REQUEST[$key] : $default;
}

Then your calling code becomes:

$someField = $this->getParam('someField', 'default value');

EDIT you could also add the sanitation call into this method as well:

function getParam($key, $default=null)
{
    return isset($_REQUEST[$key])? $this->sanitizeInput($_REQUEST[$key]) : $default;
}

Reducing your calling code even more. Now you not only have proper error free code, but you have reduced your calling code from three lines:

$params = $this->getParams();

// This key may not exist --+
//                          |
//                          v
$someField = $params['someField'] ?: 'default value';
$someField = $this->sanitizeInput($someField);

To one:

$someField = $this->getParam('someField', 'default value');