Welcome Guest, Not a member yet? Register   Sign In
Bug in validation library
#1

[eluser]Donny Kurnia[/eluser]
Today I found that prep_for_form in validation not working as expected. After debugging the validation, I found out the culprit. Here is my patch (note: the revision is my project's revision, not the CI's revision):

Code:
Index: system/libraries/Validation.php
===================================================================
--- system/libraries/Validation.php    (revision 225)
+++ system/libraries/Validation.php    (working copy)
@@ -734,7 +734,7 @@
             return $data;
         }
        
-        if ($this->_safe_form_data == FALSE OR $data == '')
+        if ($this->_safe_form_data == TRUE OR $data == '')
         {
             return $data;
         }

I wonder why the CI put
Code:
$this->_safe_form_data == FALSE
in there instead of TRUE? The original validation code could be read as: if it not safe (safe == false), then return original $data. Is it typo or the developer has it's own reason?
#2

[eluser]Derek Allard[/eluser]
Thanks for reporting Donny. Can you please list the validation/prepping rules you were using? Also, how are you creating your form text input, and how are you inserting text into it?
#3

[eluser]Derek Allard[/eluser]
I also notice you've labeled this bug as "critical". Have you found a security implication here, or something that could compromise an entire server, or bring down CodeIgniter? What makes it "critical" in your eyes Donny, I just want to be sure I'm not missing anything here.
#4

[eluser]Donny Kurnia[/eluser]
I use Oracle database and use AdoDB to insert data to database, so all escaping handled by Adodb. The problem is that the data contain " cannot displayed correctly in the form.

I'm inserting this data:

Code:
Sample " Data

Then when I see it in the database, it stored as:

Code:
Sample " Data

Then when I edit this data, the input field on my edit page just display:

Code:
Sample

When I view the page source, it display this:
Code:
<input name="data" id="data" value="Sample " Data" type="text" maxlength="255" />

Then I do the change that I post earlier. The data now stored as:
Code:
Sample " Data
and the edit page source is now correct:
Code:
<input name="data" id="data" value="Sample " Data" type="text" maxlength="255" />

This is my validation rules :
Code:
$rules['data'] = 'trim|xss_clean|prep_for_form|max_length[255]|required';
$this->validation->set_rules($rules);

What I mean by critical is the logic of the validation code.

The original code read as:
If the _safe_form_data == false (which mean that it not save) then return original data.

So, the str_replace code below the if's will never be called, and the prep_for_form will not have effect. I know this because it always got into if section, and the str_replace never called by validation.

I'm sorry if the critical label is not appropiate for this bug.

Edited:
To display input field in edit page, I use the old way:
Code:
<input name="data" id="data" value="<?=$data_content->fields['DATA']?>" type="text" maxlength="255" />
I know that I should use form_prep() to correctly display quote, but because I didn't use form_prep(), I discover the prep_for_form bug.
#5

[eluser]Donny Kurnia[/eluser]
Derek, today I'm revisit the Validation library.

After I change
Code:
if ($this->_safe_form_data == FALSE OR $data == '')
to become
Code:
if ($this->_safe_form_data == TRUE OR $data == '')
, all of validation value whether it have prep_for_form rule or not will be prepped.
So I start to dig the library to find out how prep_for_form work.
I stumble upon this code:
Code:
function set_fields($data = '', $field = '')
    {    
        if ($data == '')
        {
            if (count($this->_fields) == 0)
            {
                return FALSE;
            }
        }
        else
        {
            if ( ! is_array($data))
            {
                $data = array($data => $field);
            }
            
            if (count($data) > 0)
            {
                $this->_fields = $data;
            }
        }        
            
        foreach($this->_fields as $key => $val)
        {
            $this->$key = ( ! isset($_POST[$key])) ? '' : $this->prep_for_form($_POST[$key]);
            
            $error = $key.'_error';
            if ( ! isset($this->$error))
            {
                $this->$error = '';
            }
        }        
    }

and this one in run() function

Code:
if ($total_errors > 0)
        {
            $this->_safe_form_data = TRUE;
        }
        
        $this->set_fields();
I tried to understand it, so correct me if I'm wrong.
1. When I call $this->validation->run() and there is error in the form ($total_error > 0), the private variable $_safe_form_data will be set to TRUE, otherwise it will be FALSE as it's initial value.
2. Function set_fields() will be called. In here, function prep_for_form will be called if $_POST[$key] is isset.
3. In prep_for_form() original code:
Code:
function prep_for_form($data = '')
    {
        if (is_array($data))
        {
            foreach ($data as $key => $val)
            {
                $data[$key] = $this->prep_for_form($val);
            }
            
            return $data;
        }
        
        if ($this->_safe_form_data == FALSE OR $data == '')
        {
            return $data;
        }

        return str_replace(array("'", '"', '<', '>'), array("'", "&quot;", '&lt;', '&gt;'), stripslashes($data));
    }
the $this->_safe_form_data will be checked if it FALSE or not. So, the prep_for_form() will only take effect if $this->_safe_form_data is TRUE and $data != '', in this case it mean that there is error occured when validation run() function is called.

This mean that whatever the validation rule is, if the validation run() result is error free, prep_for_form will never take effect. If there is error, prep_for_form will always run() even there is no prep_for_form rule in it.

So I'm suggestion that there should be another function for automatic prep_for_form that will be called when the validation have error, and prep_for_form for the validation rule. Or it should never be called automatically when the error occured.

In my application, I use prep_for_form in rules and expect it run only in the field that have rules with it, and never run automatically when the form have error in validation.
#6

[eluser]onejaguar[/eluser]
"prep_for_form" is indeed redundant; it will automatically be run on all fields if a validation error occurs and will not be run in there are no errors, even if explicitly told to run in the rules.

The "form_prep" helper function will do what you want; running in all cases. e.g.:
Code:
$this->load->helper('form');
$rules['data'] = 'trim|xss_clean|form_prep|max_length[255]|required';

Rather than changing functionality, the user manual should be updated to remove “prep_for_form” as an option in the validation rules and it should state that if there are errors, special characters are converted so that HTML data can be shown in a form field without breaking it.
#7

[eluser]Donny Kurnia[/eluser]
[quote author="onejaguar" date="1222315090"]"prep_for_form" is indeed redundant; it will automatically be run on all fields if a validation error occurs and will not be run in there are no errors, even if explicitly told to run in the rules.

The "form_prep" helper function will do what you want; running in all cases. e.g.:
Code:
$this->load->helper('form');
$rules['data'] = 'trim|xss_clean|form_prep|max_length[255]|required';

Rather than changing functionality, the user manual should be updated to remove “prep_for_form” as an option in the validation rules and it should state that if there are errors, special characters are converted so that HTML data can be shown in a form field without breaking it.[/quote]

I didn't think to use form_prep in the rules. Thanks for the info Smile

I hope the documentation get updated for the next CI release.




Theme © iAndrew 2016 - Forum software by © MyBB