spiral / framework

High-Performance PHP Framework
https://spiral.dev
MIT License
1.81k stars 88 forks source link

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

Closed krwu closed 1 year ago

krwu commented 4 years ago

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 commented 4 years ago

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).

wolfy-j commented 4 years ago

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.

krwu commented 4 years ago
  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.
wolfy-j commented 4 years ago

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.

krwu commented 4 years ago

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 commented 4 years ago

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).

wolfy-j commented 4 years ago

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

krwu commented 4 years ago

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.

wolfy-j commented 4 years ago

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 commented 4 years ago

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

krwu commented 4 years ago

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 commented 4 years ago

According to its sourcecode, Laravel uses: