Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

User input data sanitation #42

Closed
joshhornby opened this issue Apr 6, 2016 · 23 comments
Closed

User input data sanitation #42

joshhornby opened this issue Apr 6, 2016 · 23 comments
Labels

Comments

@joshhornby
Copy link

Data sanitation in Laravel feels like a second class citizen, (http://www.easylaravelbook.com/blog/2015/03/31/sanitizing-input-using-laravel-5-form-requests/) unlike other frameworks:

I think it would be nice to pull something like https://github.com/daylerees/sanitizer into the core and work on a native way to include sanitation to form requests.

cc @daylerees

@sileence
Copy link

sileence commented Apr 6, 2016

I've been thinking about this for a while. I'd be happy to contribute part of this feature if @taylorotwell and the team approve this.

@tomschlick
Copy link

Form requests used to have a sanatize() method on them. It wasn't ever used and was eventually removed but I think that would be the best place to put this kind of logic.

From that we could add a few methods to get the sanitized or the raw input directly from the FR object.

@daylerees
Copy link

Happy to update the sanitizer repo to the latest FW version if there's a demand for that in the core and it makes things easier.

@brayniverse
Copy link

If there was a sanitize() method on the FormRequest class we could quite easily perform our own sanitisation either using plain old PHP or a library like @daylerees's Sanitizer. If you do it anywhere else, like a controller, you can sanitise as normal.

I like the idea but is there much need for a sanitisation library to be integrated into the core? Would it be more beneficial than using your own?

@gocanto
Copy link

gocanto commented Apr 6, 2016

I support the idea, and to be honest, I was not aware of this pkg. thanks for ur work @daylerees@taylorotwell again!

@tomschlick
Copy link

What syntax would you all prefer?

I think keeping it consistent with the rules method would be beneficial...

Example:

public function sanitize()
{
    return [
        'name' => 'trim|lowercase|striptags|customfunctionhere',
        ];
}

So that structure would turn around and use the format that @daylerees package implements behind the scenes.

We might want to also allow for a closure instead of the pipe separated string so that you wouldn't have to register new functions for one-off custom things.

Thoughts?

@daylerees
Copy link

If I remember right (it's been a while) I based the format of the sanitization rules on that of the validator. The whole pattern really. It's a bit old now, but would be easy enough to make it match up with the frameworks validator again. It's not a complicated piece of software.

@tomschlick
Copy link

Yup, reading through the source now and it looks like you support closures and everything else I pointed out.

I guess the real question is if the framework would write it's own implementation or use the package directly. I know Taylor is sensitive to how many dependencies core utilizes in composer...

@daylerees
Copy link

Well if it's a desired feature, I'm happy to do anything I can to help out. 👍

@JosephSilber
Copy link
Member

JosephSilber commented Apr 8, 2016

Form requests used to have a sanatize() method on them. It wasn't ever used and was eventually removed but I think that would be the best place to put this kind of logic.

It never shipped, so saying "it wasn't ever used" is kind of a misnomer. It was originally merged in, but was later removed before Laravel 5 was actually released.

As I've written here, the reason Taylor removed it is because he wanted the values in the request to actually be replaced with the new sanitized data (simply calling the replace method on the request object is far from enough).

I think it would be nice to pull something like https://github.com/daylerees/sanitizer into the core and work on a native way to include sanitation to form requests.

There's no need for that. As you can see from the original sanitization implementation, Laravel does not have to ship its own sanitizer in order to support request input sanitization.

OTOH, it would be nice for Laravel to ship with a sanitizer 😛

@tomschlick
Copy link

It never shipped, so saying "it wasn't ever used" is kind of a misnomer. It was originally merged in, but was later removed before Laravel 5 was actually release.

I had been working with 5 a few months before the final release so thats probably why I remember it in there for so long.

As I've written here, the reason Taylor removed it is because he wanted the values in the request to actually be replaced with the new sanitized data (simply calling the replace method on the request object is far from enough).

I think its important that if this feature was implemented that we have access to the raw data as well.

@joshhornby
Copy link
Author

@JosephSilber In my opinion a santizer goes hand in hand with validation. What's the point in one with out the other? That's why I believe it should be in the core. Also I'm sure Taylor could sprinkle some Laravel magic around the sanitizer.

@gcphost
Copy link

gcphost commented Aug 1, 2016

I had been using @daylerees sanitizer with a custom validation filter but after looking into it more I decided to just make a package using my own methods as seen here:

https://github.com/Askedio/laravel-validator-filter

I'm inline with @joshhornby that sanitization goes hand-in-hand with validation, so a 'filter' option on the Validator that would modify the request and validators data is what I thought would work best.

@joshhornby
Copy link
Author

@taylorotwell What is your opinion on this?

@vedmant
Copy link

vedmant commented Apr 14, 2017

Yeah, I was expecting Laravel should have something like this, for now I'm checking packages like https://github.com/Waavi/Sanitizer or https://github.com/daylerees/sanitizer . However both of them don't support array sanitizing, similar https://laravel.com/docs/5.4/validation#validating-arrays which is essential for my project. It would be really nice to have flexible Validator style Sanitizer.

@CristianLlanos
Copy link

I agree. I believe it'd be amazing to have a built-in sanitizer. I mean sanitizing data "the Lavarel way". It might be easy to use replace, but it doesn't feel Laravelish, so to speak.

@brayniverse
Copy link

@CristianLlanos how do you currently sanitize your data, can you give an example? What do you think is wrong with the current way you do it, and what would you change to make it easier?

@taylorotwell
Copy link
Member

What kind of sanitization are people actually doing?

@vedmant
Copy link

vedmant commented Sep 17, 2017

@taylorotwell In my project I had to trim all strings (in sub arrays as well), convert dates (in sub arrays), I had amounts coming from requests in format with commas (like 1,000,000.00) where I had to remove commas, make some strings uppercase. Here is how I use sanitizer:

return Sanitizer::make([
    '*'             => 'trim', // Trim all data
    'document'      => 'strtoupper|preg_replace:/[^A-Z0-9]/::{{ VALUE }}', // Keeps only A-Z and 0-9
    'name'          => 'strtoupper|preg_replace:/[^a-zA-Z0-9 !#@&()+-]/::{{ VALUE }}', // Keeps only list of allowed chars
    'address1'      => 'strtoupper|preg_replace:/[^a-zA-Z0-9 !#@&()+-]/::{{ VALUE }}',
    'address2'      => 'strtoupper|preg_replace:/[^a-zA-Z0-9 !#@&()+-]/::{{ VALUE }}',
    'city'          => 'strtoupper|preg_replace:/[^a-zA-Z0-9 !#@&()+-]/::{{ VALUE }}',
    'amount'        => 'str_replace:,::{{ VALUE }}', // Removes commas
    'tax1'          => 'str_replace:,::{{ VALUE }}',
    'tax2'          => 'str_replace:,::{{ VALUE }}',
    'rows.*.amount' => 'str_replace:,::{{ VALUE }}',
    'rows.*.tax'    => 'str_replace:,::{{ VALUE }}',
])->sanitize($data);

@gcphost
Copy link

gcphost commented Sep 17, 2017

We use trim, str_slug, strtolower, and a bunch of custom filters for things like phone number style, cleaning inputs and removing: phone numbers, emails, profanity, links, and long strings.

@taylorotwell
Copy link
Member

Trimming is handled internally now. I would be open to looking at other ideas for sanitization if people have ideas.

@arcanedev-maroc
Copy link

arcanedev-maroc commented Mar 17, 2018

Hi @taylorotwell (and fellow artisans), i've question about the FormRequest object.

How to apply sanitization if the validator passes, if fails, skip the sanitization ?

I know there is prepareForValidation() method works as before hook but i'm searching for after validation hook.

Something like prepareAfterValidation() or afterValidation() at the end of this method.

I can create a PR in 5.6 branch (or 5.5) if it's OK

Best regards 👍

UPDATE:

About the actual sanitization that i'm using:

BEFORE HOOK

protected function prepareForValidation()
{
    $this->merge($this->sanitize());
}

protected function sanitize()
{
    $data = $this->all(); // or $this->only([...])
    $rules = [...];

    return Sanitizer::process($data, $rules);
}

AFTER HOOK

I create a custom method called getValidatedData() with same process as sanitize() method above and use it like User::create($request->getValidatedData()).

@surgiie
Copy link

surgiie commented Aug 12, 2019

@taylorotwell @vedmant

I know this is a closed issue but I wish I would of came across this post about 2 years ago. I wrote a package that does some input formatting using laravels validation syntax which simply utilizes the parsing code provided by the ValidationRuleParserclass. That and thanks to the Arr class, dot notation is supported for associative arrays. Hopefully it’s useful to someone:

https://github.com/collab-corp/formatter

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests