Open ichilton opened 12 years ago
I've submitted a pull request here: https://github.com/fuel/oil/pull/133
Ok, it just occurred to me that this should probably go into 1.2/develop so re-issuing the pull request.
Here we go - same commit, but in 1.2/develop: https://github.com/fuel/oil/pull/134
Do you have an example of an attack?
Discusion about this on IRC: http://scrp.at/bxm
@philsturgeon I don't actually - I couldn't understand why it was done like that and realised it was a problem.
It's mainly to avoid the XSS filtering screwing up any useful characters that may go in the title, as a & ended up double-encoded on output.
Yeah, that's what I thought.
I think a better fix would be to change the template so it outputs: Form::input('title', Input::post('title', isset($post) ? e($post->title) : ''), array('class' => 'span6'));
What do you think?
Hmm - I just tried to exploit it and it didn't work.
I put this is a test
lol - Github XSS filter instead of escape then :)
All Form methods escapes values with prep_value(), doesn't it?
@WanWizard said it wasn't, but it must be I guess?
I was refering to data passed to a view, and you were asking about input (which I assumed was about the Input class, so POST data, not input form fields).
The different form methods will prep the value passed when the form "prep_value" setting is true (in your config/form.php), and the attribute "dont_prep" isn't set to true. prep_value() does the same as passing data to a view, it escapes it.
So indeed it's not a good idea to pass data to a view with escaping, and then use Form methods in a view (unless you've set "prep_value" to false in your config).
But if you use $post->title
outside of Form methods in a view, XSS could be done.
So is it better the default of fuelphp is with escaping? I mean default by secure.
Security::htmlentites default is not double-encoding. So most users will not get troubled.
Personally I find it confusing that the Form methods escape, while it is best practice to escape everything passed to the view. So a form method prepping should be the exception, not the rule.
But I assume it has to do with the fact that until now, database objects have to be passed to the view raw as they are read-only and can not be escaped. So for those not using ORM (or array conversions), prepping is probably a good thing.
This is a very confusing issue. Is this still a problem, and if so, can someone tell me exactly what the issue is?
Hi,
I've found an XSS security problem with the oil generated admin screens.
If you do: oil generate admin posts title:string slug:string summary:text body:text user_id:int
The generated code uses: $this->template->set_global('post', $post, false);
Setting false as the 3rd parameter means that it's not filtered.
But then it's used as follows - and Form::input doesn't escape the values either: Form::input('title', Input::post('title', isset($post) ? $post->title : ''), array('class' => 'span6'));
Thanks,
Ian