Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 4 of 4
  1. #1
    New to the CF scene
    Join Date
    Sep 2019
    Posts
    4
    Thanks
    0
    Thanked 0 Times in 0 Posts

    PHP Looping through $_POST Fields Securely

    Hi,

    I'm working on a new development project and I'm really starting to put into practice things I've been learning. I have a form with a number of fields, and to avoid repetition I'm using the following construct:

    PHP Code:
    if(!filter_has_var(INPUT_POST"username")) {          
            
    error_check();         
        } else {
            foreach(
    $_POST as $item=>$x) {                
                if(empty(
    $x)) {                             
                    echo 
    "Please complete all fields.";
                    die();                                    
                } else {
                    
    $fields[] = $x;                       
                };
            }; 
    This gives me an array containing the field values. My plan is to then run a loop over $fields to perform validation and sanitisation of the data, before extracting each element into a separate variable. However, reading up on the extract() function in the manual it says you shouldn't extract user input into variables this way, as it can pose a security risk.

    I was just wondering if this is still true, even though you're sanitising and validating the data before doing the extraction and, if so, is there a better way of doing this without manually entering the validation/sanitisation code for every field/variable?

    I want to avoid this:
    PHP Code:
    $username filter_var($_POST['username'], FILTER_SANITIZE_STRING);
    $emailaddr filter_var($)POST['email'], FILTER_SANITIZE_EMAIL);
    etc... 
    Any hints would be greatly appreciated.

  2. #2
    Senior Coder deathshadow's Avatar
    Join Date
    Feb 2016
    Location
    Keene, NH
    Posts
    3,738
    Thanks
    5
    Thanked 536 Times in 522 Posts
    My problem with "extracting to each variable" is the creation of "variables for nothing". The technique also runs the risk of blindly trusting that the $_POST data is complete, exists, and other issues from client-side that just can't be trusted.

    Particularly when each filter_var result is unique to the usage case -- you wouldn't want the same 'sanitizing' for HTML output as you would an e-mail or a database.

    Blindly looping $_POST or even a subset of it is the type of thing that gets dev's in trouble. It happened with SMF over a decade ago where they were blindly trusting that the INDEXES into $_POST they were getting back from the form -- the names in the form -- would be correct/valid. End result was a hacker adding their own name'd input to manipulate database values that shouldn't have even had access client-side.

    The way I like to handle this is to combine my form generation, form output, and form handling to operate off a single master array. For example, let's use a simple contact form:

    Code:
    $contactForm = [
    	'formId' => 'contact',
    	'action' => 'contact.php',
    	'method' => 'post',
    	'submitText' => 'Submit',
    	'fields' => [
    		'name' => [
    			'label' => 'Your Name',
    			'type' => 'text',
    			'required' => true
    		],
    		'e-mail' => [
    			'label' => 'E-Mail Address',
    			'type' => 'email',
    			'required' => true
    		],
    		'title' => [
    			'label' => 'Message Title',
    			'type' => 'text',
    			'required' => true
    		],
    		'message' => [
    			'label' => 'Your Message',
    			'type' => 'textarea',
    			'required' => true
    		]
    	]
    ];
    My template then uses this array to build the form... AND my validation uses the 'fields' part to also validate. This way we are only validating / processing files we KNOW are supposed to exist.

    A slimmed down version of such validation going something like this:

    Code:
    function keyTrue($index, $array) {
    	return
    		array_key_exists($index, $array) &&
    		$array[$index] == true;
    } // keyTrue
    
    function isValidEmail($address) {
    	if (filter_var($address, FILTER_VALIDATE_EMAIL) == FALSE)
    		return false;
    		
    	/* explode out local and domain */
    	list($local, $domain) = explode('@', $address);
    	
    	$localLength = strlen($local);
    	$domainLength = strlen($domain);
    	
    	return (
    		/* check for proper lengths */
    		($localLength > 0 && $localLength < 65) &&
    		($domainLength > 3 && $domainLength < 256) &&
    		(
    			checkdnsrr($domain, 'MX') ||
    			checkdnsrr($domain, 'A')
    		)
    	);
    } // isValidEmail
    
    function formValidate($formData) {
    	$errors = [];
    	foreach ($formData['fields'] as $name => $value) {
    		if (array_key_exists($name, $_POST)) {
    			if (
    				keyTrue('required', $value) &&
    				empty($_POST[$name])
    			) $errors[$name] = 'This field is required';
    			else {
    				switch ($value['type']) {
    					case 'email':
    						if (!isValidEmail($_POST[$name]))
    							$errors[$name] = 'You did not enter a valid E-mail address';
    						break;
    				}
    			}
    		} else if (
    			keyTrue('required', $value)
    		) $errors[$name] = 'This field is required';
    	}
    	return $errors;
    } // formValidate
    returning an array of errors you can use when/if you spit the form back at the user to add the messages to each field.

    NEVER trust things like http 'name' indexes from user side to be correct, to even exist, etc etc.

    Whilst filter_var is pretty silly these days... since HTML output you should be using htmlspecialchars, and for database there is ZERO reason to waste time manually sanitizing when we have prepare/execute. Only thing to check is field validity and see how I have that "switch" statement? Add other validations there or next to it. Same for e-mail values, where the message body is relatively benign if you force as plaintext (HTML has zero damned business in e-mail in the first place) and for header values the only things one should really need to be stripping is /r/n
    “There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies and the other way is to make it so complicated that there are no obvious deficiencies.” – C.A.R. Hoare, The 1980 ACM Turing Award Lecture
    http://www.cutcodedown.com

  3. #3
    Senior Coder CFMaBiSmAd's Avatar
    Join Date
    Oct 2006
    Location
    Denver, Colorado USA
    Posts
    4,297
    Thanks
    3
    Thanked 561 Times in 546 Posts
    Create an array that holds a definition of the expected form fields, with the main array index being the field name, then loop over this defining array to access and process the form data. Store an array of information for each field containing a label/name, any validation steps, and any dynamic processing (crud). As you loop over this defining array, you would 'dynamically' call the validation steps for each input, storing any validation errors in a php array, with the error array index being the field name.

    into a separate variable
    Don't create separate variables at all. The submitted data is already a perfectly fine variable, an array variable. Just directly use the elements in the array by operating on it as a set of data in the rest of the code.
    Finding out HOW to do something is called research, i.e. keep searching until you find the answer. After you attempt to do something and cannot solve a problem with it yourself, would be when you ask others for help.

  4. #4
    New to the CF scene
    Join Date
    Sep 2019
    Posts
    4
    Thanks
    0
    Thanked 0 Times in 0 Posts
    Hi,

    Thanks for this guys. Your answers were very thorough and well presented; I particularly like the use of an array to handle the entire form. I've changed my approach accordingly.


 

Tags for this Thread

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •