PHP Regex Input Sanitization

Question

This helper class is supposed to validate the user input from a text based wifi login form.

The entire process is basically this:

I use the create_voucher function of this API passing voucher_duration to set the amount of time for which the voucher is going to be valid and $this->clean['name']." ".$this->clean['surname'] as a note so I can later identify which device belongs to which user.

The code returned by create_voucher is then sent to the passed e-mail-adress using php-mailer so the user can login and use the wifi.

I am particularly unhappy with my error handling and would like to know if you see any obvious ways to break the code or inject malicious code.

class sanitizer { public $clean; private $post=null; private $reg_email ='/^\S+@\S+\.\S+$/'; //Just some basic checking private $reg_name = '/^[\'\p{L} -]+[\n]?$/im'; //Allowing some wierd names private $reg_number='/^[[:digit:]]*$/im'; //A single integer no fuzz public function __construct($post){ $this->post=$post; } private function sanitize($key, $regex){ if (preg_match($regex, $this->post[$key])) { $this->clean[$key]= $this->post[$key]; } else { $this->clean[$key]=null; } } public function clean_up(){ if (isset($this->post['smt_sent'])) { if ($this->post['smt_sent']==1) { $this->sanitize('name', $this->reg_name); $this->sanitize('surname', $this->reg_name); $this->sanitize('voucher_duration', $this->reg_number); if ($this->post['voucher_duration'] > 0 && ($this->post['voucher_duration']/60 > 48)) { $this->clean['duration']=null; } $this->sanitize('email_own', $this->reg_email); $this->clean['smt_received']=1; $this->clean['error'] = false; //No errors yet foreach ($this->clean as $field) { //Loop trough each field if (!isset($field)) { $this->clean['error'] = true; //Yup there are errors } } return $this->clean; } } } } 

Answer 1

I think you need to think about inverting control here or you will find limited re-use in this class. Why would you pass a specifically-formatted POST request here to this class in order to execute validation vs. having calling code just reference methods on this class that execute the validation logic?

For example:

// somewhere outside class $valid = sanitizer::validateEmail($_POST['email_own']); 

By doing this you can make a class like this more re-usable, in that it is not built just to handle POST data validation for a single script.

---

There is a distinct difference between sanitization and validation. You seem to be using the concepts interchangeably. I would say this class is a validation class not a sanitization class.

---

From looking at the rules in your class, I actually question if there is really much value here, as really the only validation rule you have here that could not be better fulfilled by built-in PHP validation functionality is you name validation.

You could just as easily use filter_input() or filter_input_array() here. For example:

// not shown - checking of validation results $email = filter_input(INPUT_POST, 'email_own', FILTER_VALIDATE_EMAIL); $voucherDuration = filter_input(INPUT_POST, 'voucher_duration', FILTER_VALIDATE_INT); 

This might pretty much eliminate your need for this class altogether.

---

Some other code review thoughts:

• You should strive to use meaningful and specific names for classes, methods/functions, variables, etc. sanitizer as a class name here is very generic, especially since this class has a VERY specific use case that it supports. sanitize() as a method name incorrectly describes what is happening in this method, which is in fact a validation method. clean_up() as a method name doesn't seem to make sense. What are you "cleaning up"? Don't shorten variable names. Saving typing a few extra characters is trivial in comparison to having easily understandable code. For example, reg_* properties could be called regex_* to make the meaning much clearer, not just where defined, but also where used in the rest of the code.
• Speaking of the regexes. Should these be class constants? If you don't intend to have these items be mutable (which you don't in this case), don't allow them to be mutable.
• You have unnecessary code nesting/branching. In your clean_up() method you could easily combine the first two conditionals and you could invert the conditional to exit early when the condition isn't met, de-nesting the entire rest of the code in the method. In your sanitize() method you could remove the else block altogether by setting null as default value. As a rule of thumb, the more branching/nesting you have in your code, the more bug-prone it will be, so you should actively look to code it away.
• You need to be consistent in your method returns. Your clean_up() method doesn't return anything if the first two conditions aren't met.
• You should be consistent in where you write to data structures. You are breaking the logic up for where you write to $this->clean across two methods. Perhaps the sanitize method should not write to this array at all, but rather just perform the regex and return values.
• You have some stylistic problems you may want to address:
  • Classes should be named with first letter in uppercase by convention.
  • You use inconsistent spacing around assignment operators and flow-control structures.
  • Don't put comments at the end of lines where they are easily missed. Comments should go on line(s) before the lines they are applicable to. Also don't add unnecessary comments. Here your comments add no value in understanding the code.