The Side Effect of Using Return Type To Handle Errors

I came across a curious little side effect of the issue I just posted earlier:

public function action()
{
   $id = filter_input(INPUT_POST, 'id', FILTER_SANITIZE_STRING);
	
   if(is_bool($id))
   {
      die('invalid id');
   }

   if(isset($_POST['accept']))
   {
      $this->acceptId($id);
   }
   else if(isset($_POST['reject']))
   {
      $this->rejectId($id);
   }
}

This is obviously going to do something when a POST has occured, but the method was invoked each and every time. The reason why it works? If there is no variable named ‘id’ POSTed, filter_input returns null. And null is not boolean, so the test passes. But as it’s not a POST request, neither of the two other if-tests are true, so the code silently passes through (id can contain characters here, so the filter isn’t used to just get integers).

If you’re not going to throw an error when the parameter is missing, the test is actually completely useless. This bug had hidden itself within the usage of the type to test for fault instead of actually checking the value, and did not creep out before I rewrote the if-test.

Leave a Reply

Your email address will not be published. Required fields are marked *