New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request Data Sanitization #186

Closed
johannesschobel opened this Issue Jul 1, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@johannesschobel
Copy link
Contributor

johannesschobel commented Jul 1, 2017

Hey all..

I found a problem when "sanitzing" the data of a Request. However, I would like to point out, that is not an issue with apiato itself, but rather a problem with Laravel and/or PHP itself.

In order to illustrate the problem, please take a look at the following class: \App\Containers\User\Actions\UpdateUserAction. This class prepares the data from the request in order to update a User entity. Thereby, an array is created and all data from the request is mapped to this $data array.
Afterwards, empty values are removed from this array, by calling

$userData = array_filter($userData);

However, and that is the problem at this point, this will not only remove all "empty" values (e.g., null) but rather remove all false values as well. The value is then removed from the array, and later on, not updated within the database. This way, it is not possible to set any value to false - which is quite bad ;)

In my current project i have developed a sanitizeData method that allows to sanitize the data accordingly. I would like to create a PR for this "issue" (i would like to emphasize again, that it is not an apiato related issue!). However, apiato could be "clever enough" to provide an appropriate solution for this ;)

What do you think?
Cheers,
Johannes

@johannesschobel

This comment has been minimized.

Copy link
Contributor

johannesschobel commented Jul 1, 2017

Hm, I just realized that I should add a few comments on the array_filter approach..

If you use PATCH for (partially) updating an entity, you would need to check, which fields are submitted to the API.. Usually, this results in a lot of IF-statements.. For example

// ...
if($request->has('data.name')) {
   $data['name'] = $request->input('data.name');
}
if($request->has('data.description')) {
   $data['description'] = $request->input('data.description');
}
// ...

At the end of this IF-statement madness the $data only contains the values to be updated. Easy..
However, if you would like to reduce all these if-statements, you usually would do something like this:

$data = [
   'name' => $request->input('data.name'),
   'description' => $request->input('data.description'),
   // ...
];
$data = array_filter($data);

The array_filter() method removes all "empty" fields from the array. However, PHP has some weird interpretation of "empty", as this would remove false, null, '' (empty string) and so on as well! Clearly, not what you want..

Therefore, we need some easy-to-use sanitization of the request data - I have it already implemented!
Please let me know what you think about this "problem"

@Mahmoudz

This comment has been minimized.

Copy link
Member

Mahmoudz commented Jul 2, 2017

@johannesschobel Hi I agree on the existing of the problem if you have a solution of any kind that doesn't affect anything else, go ahead and submit a PR to check it, would love to merge it. Best,

@johannesschobel

This comment has been minimized.

Copy link
Contributor

johannesschobel commented Jul 3, 2017

i will contribute.. Code is already existing, just need to clean it up, write a doc and submit it as PR.. will do it later this day..

@johannesschobel

This comment has been minimized.

Copy link
Contributor

johannesschobel commented Jul 3, 2017

added the PR #187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment