fuel / fieldset

FuelPHP Framework - Form Builder library
MIT License
9 stars 5 forks source link

HTML escaping #25

Closed kenjis closed 10 years ago

kenjis commented 10 years ago

It seems there is no functionality to HTML-escaping when form building.

v1 has prep_value(): https://github.com/fuel/core/blob/1.8/develop/classes/form/instance.php#L627

emlynwest commented 10 years ago

This is something that should be done by the application, it can be added to a renderer if you want that functionality.

kenjis commented 10 years ago

All HTML forms made by this package have XSS vulnerability. Don't you provide standard way to prevent XSS?

emlynwest commented 10 years ago

This is not a security package though. It should be down to the app/framework/developer that implements the package.

kenjis commented 10 years ago

How does Fuel v2 provide a way to prevent XSS when users use this package?

emlynwest commented 10 years ago

That will be down to the (eventual) security package. Don't forget that fuel 2 is still a work in progress.

WanWizard commented 10 years ago

The basis of that package is already finished, it currently supports clean(), cleanUri(), striptags() and has CSRF functionality. It is used in Views and Presenters to encode the output...

kenjis commented 10 years ago

Thanks for your answers.

But my question is still the same: It seems there is no functionality in this Fieldset package to escape user input values with using Security package (or whatever). So when repopulating forms, attackers could do XSS.

In my opinion, Fieldset package uses Security class to escape user input values in generated HTML.

emlynwest commented 10 years ago

The current way that forms populate themselves will be removed in favour for being passed an array or having an Input object injected from the core.

WanWizard commented 10 years ago

Currently the Input object doesn't have any feature for escaping it's contents. And you don't want that, that would mean modified values.

If you want to work as loosly coupled as possible, I'd say accept an array, and add something like a getEscapedPost() method to Input, which returns an array with escaped input values, which can be used to inject the input data into Fieldset.

That does move the dependency on fuelphp/security to Input, but since that is part of Foundation, I can live with that.

emlynwest commented 10 years ago

Was what I was thinking. I'll add it to my todo.

kenjis commented 10 years ago

Wait, please.

If I want to populate modified user input, eg, trim(), how to do?

WanWizard commented 10 years ago

If you can pass an array or an instance of Input (basically, an instance of DataContainer), you can also fetch the input data in your controller, do whatever you want with it, then pass the resulting array?

kenjis commented 10 years ago

If I want to pass modified data to repopulate($data), I have to escape by myself before passing it? If I forget to escape, XSS could be done.

Is it right?

emlynwest commented 10 years ago

https://github.com/fuelphp/common/issues/8

kenjis commented 10 years ago

Thank you. XSS problem in this package almost resolved.

I believe "adding something like a getEscapedPost() method to Input" is bad design. Escaping is needs to output systems and specific to each output system. So I think escaping at the point of output is good design.