Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add ability to limit request params #2088

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

amad commented Dec 21, 2012

Limit request params with safe hystack list to pass data directly to the active-record (helpful in update query)

Maybe it would be in validation lib !

Contributor

cryode commented Dec 24, 2012

I feel like this just promotes bad code. Ideally, you never want to pull direct, unmodified GET/POST data. xss_clean helps but isn't fool proof, and doesn't mean the data is in the proper format. Getting a bunch of data directly from GET/POST and into Active Record is not a good idea.

amad commented Dec 24, 2012

in most case you have purifiered request data after using Form Validation !

scenario:
User have this params in profile-edit form name and age
The Bad-User guess we have email field also in the db-table !
so edit request and POST you name, age, email

You validate name and age, clean them and going on ... (You are not expected to have another !)
then you put request array in your update query and user edit column that you doesn't want be edited ..

to prevent this scenario we have to define all indexes separately in query ..

$this->db->update('profile', array(
                                    'name' => $post['name'], 
                                    'age'  => $post['age']
                                ));

but when there is no worry about indexes ..

$this->db->update('profile', $post);

This is not mean that you doesn't check and validate data ! this mean you have not to redefine in query

Contributor

cryode commented Dec 24, 2012

I rarely include filtering or purification rules in my form validation, because it alters the inputted value. If my user experiences an error and their form is reloaded with their entered values, I want them to see exactly what they wrote (most of the time). And you don't want to rely on users doing what they're supposed to. You want to make things as secure as possible for even new users.

There's too many ways for things like this to go wrong.

  1. One of your update fields is not required. I submit nothing. Input::post() pulls in the correct NULL value because it does not exist, and tries to update database. DB does not allow for a NULL value because it has its own default. Result: DB error.
  2. None of your update fields are required. I submit a form with altered keys, none of which are expected by your application. Update then has NO data. Result: DB error.
  3. A user adds no input validation / purification. Could result in a lot of improper value errors (such as updating an INT column with a string).

This could be useful for devs who know what they're doing, but at the same time, I can achieve the exact same result without extending the core:

$post = [];

foreach (['name', 'age', 'birthday'] as $key)
{
    $post[$key] = $this->input->post($key);
}

It has good intentions, and can potentially be useful, but I see it causing more problems than it solves. CI is known for being newbie-friendly, and this teaches the wrong habits.

amad commented Dec 25, 2012

I'm agree with that
Yep we do it with this solution without changing the core
But the idea behind this commit is add this ability to the core
adding it to core without changing default functionality and usage is not bad idea and

This is improvement

$post = $this->input->post(NULL, TRUE, ['name', 'age', 'birthday']);

and also instead we can add new method to Input lib or add this ability to Form_validation
and handle that in new method ..
something like :

$post = $this->input->limit_post_bykey(['name', 'age', 'birthday'])->post(NULL, TRUE);
  • less calling post/get method
  • less defining new variable
  • less code
  • less memory and more speed ! somehow in scale ..
  • useful with wide usage

and be better if make it as new method instead of optional in post/get methods .. (less check for this option in post/get methods)

Contributor

narfbg commented Dec 25, 2012

Sorry, this isn't something that should go into CodeIgniter.

@narfbg narfbg closed this Dec 25, 2012

But as i think, it will be a great function ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment