framework icon indicating copy to clipboard operation
framework copied to clipboard

[validation] Validation should treat all input values as string to validate

Open krwu opened this issue 5 years ago • 12 comments

Demo code:

public function index(Validation\ValidationInterface $validation)
{
    $validator = $validation->validate(
        $this->request->data->all(),
        [
            'name' => 'string',
            'age' => 'int',
            'birthday' => 'datetime::valid',
            'subscribe' => 'bool',
            "balance" => 'double'
        ]
    );
    return $validator->getErrors();
}

Testing:

curl -X POST -d 'name=foo&age=30&birthday=1990-02-05&subscribe=true&balance=38.5' http://localhost:8080

Output:

{
  "age": "The condition `is_int` was not met.",
  "subscribe": "Not a valid boolean.",
  "balance": "The condition `is_double` was not met."
}

Let's try it again with spiral/filter(which relies on spiral/validation):

Demo code:

class MyFilter extends Filter
{
    protected const SCHEMA = [
        'name' => 'data:name',
        'age' => 'data:age',
        'birthday' => 'data:birthday',
        'subscribe' => 'data:subscribe',
        'balance' => 'data:balance',
    ];

    protected const VALIDATES = [
        'name' => 'string',
        'age' => 'int',
        'birthday' => 'datetime::valid',
        'subscribe' => 'bool',
        "balance" => 'double',
    ];

    protected const SETTERS = [];
}
public function index(MyFilter $f)
{
    return [
        'errors' => $f->getErrors(),
        'fileds' => $f->getFields()
    ];
}

Testing

curl -X POST -d 'name=foo&age=30&birthday=1990-02-05&subscribe=true&balance=38.5' http://localhost:8080

Output

{
  "errors": {
    "age": "The condition `is_int` was not met.",
    "subscribe": "Not a valid boolean.",
    "balance":"The condition `is_double` was not met."
  },
  "fileds": {
    "name": "foo",
    "age": "30",
    "birthday": "1990-02-05",
    "subscribe": "true"
  }
}

Add SETTERS:

protected const SETTERS = [
        'age' => 'intval',
        'subscribe' => 'boolval',
        'balance' => 'doubleval',
    ];

Now it seems work:

{
  "errors": [],
  "fileds": {
    "name": "foo",
    "age": 30,
    "birthday": "1990-02-05",
    "subscribe": true,
    "balance": 38.5
  }
}

But how about user input is invalid:

curl -X POST -d 'name=foo&age=bar&birthday=nothing&subscribe=blabla&balance=foolyou' http://localhost:8080

The result will be:

{
  "errors": {
    "birthday": "Not a valid date."
  },
  "fileds": {
    "name": "foo",
    "age": 0,
    "birthday": "nothing",
    "subscribe": true,
    "balance": 0
  }
}

invalid age, subscribe, balance are all passed the check, but become wrong values.

Even worse when user input "false" for boolean parameters:

curl -X POST -d 'subscribe=false' http://localhost:8080
{
    "errors":[],
    "fileds":{
        "subscribe":true
    }
}

false becomes true.

In web applications, every value from user input is "string". A validator should firstly validate user input, then cast valid values to its type.

krwu avatar Apr 05 '20 15:04 krwu

The problem is:

If you don't use SETTERS in filters, both validator and filter will throw errors to correct values even user input correct values. But if you use SETTERS to cast type before validation, filter and validator can not do what we need them to: check user input, throw errors. They will pass the check, and give you some wrong values(all invalid values become the valid default values).

krwu avatar Apr 05 '20 15:04 krwu

What about type::notNull, most of SETTERS will cast the value to null in case of error. Yes, you are going to get "this value is required" but the validation itself will be pretty safe.

wolfy-j avatar Apr 07 '20 09:04 wolfy-j

  1. most of the cast (intval, doubleval) will cast invalid values to 0, boolval will cast 'true', 'false' to true.
  2. When user input an invalid value, you should tell him "your input is not a bool/integer", not "some field should not be null", because he DID input something, not null.

krwu avatar Apr 07 '20 09:04 krwu

OK,

  1. sounds we have to introduce the configuration for SETTERS and the number of type conversion functions to deal with that.

  2. This one is a bit more tricky. It means that we should always have a correct pair of setters and validation rules... We will try to address it in next major updates for the filters.

wolfy-j avatar Apr 07 '20 09:04 wolfy-j

I think it's not a problem for users from Symfony. They may have used to such kind of validation rules. However, it may confuse users from Laravel and other frameworks. It depends on the design concepts of Spiral.

krwu avatar Apr 07 '20 09:04 krwu

What do you think is wrong with filter_var, so you give up using it instead of is_function and ctype_function?

For example, $isInt = filter_var($value, FILTER_VALIDATE_INT) !== false) and $isBool = filter_var($value, FILTER_VALIDATE_BOOLEAN) !== false).

krwu avatar Apr 07 '20 10:04 krwu

Nothing wrong with it. It's more the question when to apply these values and how to filter data.

wolfy-j avatar Apr 07 '20 10:04 wolfy-j

I think the main question is where the values from. If the source is another system, component or inner code, it's very nice to check type strict. If the main source of values is HTTP requests, we have known their types are always "string", why don't we validate them as "string" firstly, then we do SETTERS or GETTERS after they have passed the validations.

Validation is for values from the user, not for values that have been cast by us.

krwu avatar Apr 07 '20 10:04 krwu

We have built the filters to work not only in HTTP env. We use them for console commands and other domain layers.

For example, the filter can get values from an active InputInterface scope, you can check the FilterBootloader. Furthermore, some filter values can be objects (entities) so it's def not always strings.

wolfy-j avatar Apr 07 '20 13:04 wolfy-j

I'm not a user of laravel validation, but does it assume that every value scalar string?

wolfy-j avatar Apr 07 '20 13:04 wolfy-j

I'm not a user of laravel validation, but does it assume that every value scalar string?

You can see it's validation docs here.

you can do validation like this:

public function store(Request $request)
{
    $validatedData = $request->validate([
        'title' => 'required|unique:posts|max:255',
        'category_id' => 'required|integer|exists:category,id',
        'someInteger' => 'digits:30',
        'someBool' => 'bool',
        'body' => 'required',
    ]);

    // The blog post is valid...
}

You can also create Form Reques (like Filter in Spiral) to validate and filter parameters.

To the bool rule, "true", "false", “1”, "0" and 1, 0 are valid. To the digits:length, 'integerornumeric` rules, numbers and numbers in string format are valid.

In spiral, this controller action:

public function open(int $id) {}

does not match http://localhost:8080/open/223, but this does:

public function open(string $id) {}

In Laravel, all these actions can match http://localhost:8080/open/223:

// match int
public function open(int $id) {}

// match string
public function open(string $id) {}

// match model, auto query from database with ORM
public function open(User $user) {}

krwu avatar Apr 07 '20 13:04 krwu

According to its sourcecode, Laravel uses:

  • filter_var for integer, ip, ipv4, ipv6 rules.
  • in_array($value, [[true, false, 0, 1, '0', '1']], true) for boolean rule.

krwu avatar Apr 07 '20 13:04 krwu