XSS: Static form fields are not properly escaped

[philno]

Hey, I was testing my laravel project for XSS vulnerabilities when I noticed that static form fields are not escaped.

How to reproduce / proof of concept: Using the default laravel auth system, register a new user and set <script>alert('danger');</script> as name. Add a new formbuilder form and add 'name' as a static field.

use Kris\LaravelFormBuilder\Form;

class EditUserForm extends Form
{
    public function buildForm()
    {
        $this
          ->add('name', 'static')
...

Create a simple view and controller for the form, set the user as model and the script will execute when you open the page (Tested with Chromium Version 55.0.2883.87 on linux mint).

To be clear, this is not a major security issue. You can and should prevent this by validating/sanitising the user input. However, static fields are pretty useful and escaping the data won't hurt performance while adding an additional layer of security.

[kristijanhusak]

@philno Yeah you are right, haven't really thought about it, for the same reason you mentioned (validating/sanitizing). If you have some free time, please create a PR.

[philno]

@kristijanhusak Where would be the appropiate place to escape the element value? I guess overriding StaticType#getValue?

e(parent::getValue($default)) should do the job. Using laravels 'e' escape helper function and the $default param of getValue...

Also, is it possible that this breaks anyones website? So, does the field need an option like 'escapeValue' or something?

[kristijanhusak]

@philno yeah that should do the trick.

Hopefully not, it will be in separate version anyway.

[beghelli]

hey guys, I don't think it makes sense to escape the values in PHP land. It seems odd to me to call getValue in PHP and have HTML encoded characters as part of the returned value.

I think it makes more sense to leave the responsibility to escape to the templates, where an encoded HTML character really makes sense. That's what happening in Laravel, I guess. If you use blade templates, the engine will escape the value when you call {{ $variableName }}. But the actual value in the PHP objects will not be escaped.

Anyway, I've sent a pull request for this, hope it helps :)