search

benedmunds / CodeIgniter-Ion-Auth

Simple and Lightweight Auth System for CodeIgniter
http://benedmunds.com/ion_auth/
MIT License
2.35k stars 1.14k forks source link

Edit User fails to update Groups in Codeigniter 3 #683

Closed pacificcoders closed 9 years ago

pacificcoders commented 9 years ago

Issue: When editing user as an admin, selecting or deselecting groups is ignored. Cause: Isolated it to xss_clean. Workaround: Change: $this->form_validation->set_rules('groups', $this->lang->line('edit_user_validation_groups_label'), 'xss_clean'); To: $this->form_validation->set_rules('groups', $this->lang->line('edit_user_validation_groups_label')); Haven't had time to investigate if it's a Codeigniter 3 xss issue related to passing arrays in general. However, thought I would flag it in case anyone else was having the same problem.

benedmunds commented 9 years ago

I don't see what the issue would be from checking the CI 3 source... This is the second time this has been reported. Have you seen this before @narfbg ?

narfbg commented 9 years ago

I've seen it, although in a different context ... Here's a somewhat related issue: bcit-ci/CodeIgniter#1136

It's nothing unexpected really, xss_clean() may strip some characters if they match a certain pattern. That's why it should only be used on output, not input.

benedmunds commented 9 years ago

Makes sense, thanks!

benedmunds commented 9 years ago

This has been fixed with 2e342ec6a8fcfb891e9ec7a6ac36d60270aa0696

pacificcoders commented 9 years ago

Confirmed. Works like a charm. Thanks.

avenirer commented 9 years ago

Please @benedmunds revert the changes.

The problem was that when you did the set_rules, you didn't pass the field as array. It should have been:

$this->form_validation->set_rules('groups[]', $this->lang->line('edit_user_validation_groups_label'), 'xss_clean');

benedmunds commented 9 years ago

I believe its a CI2/3 difference here. Either way XSS shouldnt be ran on input.

avenirer commented 9 years ago

Actually you are mistaken here. Is not a difference between CI2 and CI3. Here is the manual for CI2: https://ellislab.com/codeigniter/user-guide/libraries/form_validation.html#arraysasfields

And also you didn't resolve the problem, you just made it worse, because now there is no more verification for the groups[] field. Which... is a vulnerability. Although I think that at least regarding security and login systems is good to have xss_clean, the sanest thing to do at that line would have been to do: $this->form_validation->set_rules('groups[]', $this->lang->line('edit_user_validation_groups_label'), 'integer'); ...and not delete it entirely (because,in the end, you are inserting it in the database).

benedmunds commented 9 years ago

Im not sure it matters at all. The only time you'll hit a non-integer is if someone is manually sending through post data or fucks up the logic. And in that case the model is typecasting to int and the DB only takes int. Plus the modal of course handles the escaping. So I dont see an issue here.