Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95784
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <a13c876f-90b4-3020-2553-fd5a55f55997@gmail.com>
References: <CAGa2bXarw2jTj0yhXpywWNQOJLR2pdhfaqSdwj_btT2giEL15Q@mail.gmail.com>
 <CAGa2bXaA+uvh_wpRAHeiv+Xb-JOrTAkyoe3jzq3FVCEbfja_5w@mail.gmail.com>
 <232F1604-2211-4351-B830-EDC958A25D6D@strojny.net> <CAGa2bXb2y4afY28rwdcjgxUEkr41ZPMMdj4dUtMDVtJLBONAbA@mail.gmail.com>
 <2de35db0-9974-cc96-83dd-3d2dbd48f7f8@lsces.co.uk> <CAGa2bXYG1Or9+SGf3Rm4Yi5KUQ1mwnffTSSzuwARpy4bFfXB=w@mail.gmail.com>
 <5b72e9da-068a-bc79-82c2-f36f723f42bb@gmail.com> <CAGa2bXa+9raNXmg90rsfi+yt_9HMBtWXWORy3f_014Teg+CcBg@mail.gmail.com>
 <eaa51c8d-74b2-9ee8-da4a-045480b45993@gmail.com> <CAGa2bXYvJVkUAGVv7NypxYvkGQSTM00gDOVK+kb-OVYNkNVrVA@mail.gmail.com>
 <a13c876f-90b4-3020-2553-fd5a55f55997@gmail.com>
Date: Thu, 8 Sep 2016 19:44:52 +0900
Message-ID: <CAGa2bXYMjs+0gWd9vzLqTdn3oJHQmE_5Og=+VGHWmQCZJfGv_A@mail.gmail.com>
To: Rowan Collins <rowan.collins@gmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [RFC][VOTE] Add validation functions to filter module
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi Rowan,

On Thu, Sep 8, 2016 at 6:02 PM, Rowan Collins <rowan.collins@gmail.com> wrote:
> On 08/09/2016 09:28, Yasuo Ohgaki wrote:
>>
>> This might be the largest difference.
>>
>> To make something secure than it is now, adding additional security
>> layer is effective, not single location/code.
>
>
> My instinct is that this extra location would be a maintenance nightmare, as
> it would need to keep up with changes elsewhere in the application. In day
> to day use, what would make this extra security layer any more likely to be
> comprehensively maintained than the existing validation layer?

You are right and wrong.

WAF(Web application firewall) maintenance that is customized for
certain app is nightmare, I fully agree.

Dividing input validations and logic checks works. These two are
focusing on different objectives, format and logic. Finding
programmer's mistakes is a lot easier with software built-in input
validation, unlike WAF, because programmer knew what's the inputs.
UNIT tests help also.  That's the reasons why it works much better
than WAF approach.

>
>
>> Suppose we have validation module. You are suggesting something like
>>
>> $int = validate_int($var, $min, $max);
>> $bool = validate_bool($var, $allowed_bool_types);
>> // i.e. which type of bool 1/0, yes/no, on/off, true/false is allowed
>> // This isn't implemented. All of them are valid bools currently.
>> $str = validate_string($var, $min_len, $max_len);
>> $str = validate_string_encoding($var, $encoding);
>> $str = validate_string_chars($var, $allowed_chars);
>> $str = validate_string_regex($var, $regex);
>> $str = validate_string_degit($var, $min_len, $max_len);
>> $str = validate_string_callback($var, $callback);
>
>
> No, I'm suggesting something like:
>
> if (
>         ! validate_int($var, $min, $max)
>         || ! validate_bool($var, $allowed_bool_types)
>         || ! validate_string($var, $min_len, $max_len)
>         || ! validate_string_encoding($var, $encoding)
>         || ! validate_string_chars($var, $allowed_chars)
>         || ! validate_string_regex($var, $regex)
>         || ! validate_string_degit($var, $min_len, $max_len)
>         || ! $callback($var) // Note: no need to wrap this callback, it's
> just a boolean-returning function
> )
> {
> // ERROR
> }

My opinion is "it's better to separate this kind of _required_ format
check from logic" to make logic simpler and more maintainable.

Think of this format check as "forcing types". Even if you force DATE
type, you still might have to check logical errors in logic(model).
e.g. Reservation date for a service can only be set within certain
range.

If you're on the same boat as me. You don't have to return anything,
but catch exception. Above code would be

<?php
$validate_my_var = function($var) use ($callback) {
        validate_int($var, $min, $max);
        validate_bool($var, $allowed_bool_types);
        validate_string($var, $min_len, $max_len);
        validate_string_encoding($var, $encoding);
        validate_string_chars($var, $allowed_chars);
        validate_string_regex($var, $regex);
        validate_string_degit($var, $min_len, $max_len);
        $callback($var);
}

simply call it for a variable.
I would use array of definition as you described in previous mail, too.

$validation_def = [
   'my_var' => $validate_my_var,
   'some_var' => $validate_some_var,
   'another_var' => $validate_another_var,
];

function validate_array($input, $definition) {
  foreach($definition as $key => $func) {
     // Cannot handle "optional" rule well with this code.
     $func($input[$key]);
  }
}

try {
  validate_array($_POST, $validation_def);
} catch (InputValidationException $e) {
   // cleanup and die
}
?>

It seems a lot like proposed code with a lot more typing. IMO.
However, I'm OK with this kind of implementation, too.
i.e. Providing basic validate_*() functions.

Half implemented filter module validator problem remains, though.

>
> It was a deliberately narrow purpose as an example, but IMHO that's
> perfectly readable, and the focus of the module would be on creating a good
> set of validation rules, rather than worrying about how they're applied.
>
>
>> [...] Rule reuse and centralizing validation rule is easy.
>
>
> The language provides us plenty of ways to reuse code. For instance:
>
> function validate_my_field($var) {
>         return
>                 validate_int($var, $min, $max)
>                 && validate_bool($var, $allowed_bool_types)
>                 // etc

OK. I understood you prefer simple validation functions solution.

PHP is generic programming language, so most features could be
implemented by basic constructs.  Code reuse works with simple
functions. However, how about definition reuse? It cannot be reused
easily to generate JavaScript validation code, for example. If example
code made definition reusable, it would look more like proposed code.

We have working filter, and half implemented validator by filter
module. Leaving it as it is now is not good, IMHO. Why not finish
validator implementation and provide ready to use tool? Resulting code
would be similar to simple validation functions implementation,
somewhat at least, anyway.

What filter module is missing as validator currently are:

 - Whitelisting concept (Implemented)
 - Multiple rules for a variable (Implemented)
 - String rules (Implemented)
 - Optional rule (To be implemented. Refactoring is needed)

My PR implements above 3 out of 4 mandatory features to filter module
validation.

I still fail to see what's wrong for improving/finishing filter module
implementation and what's wrong in my improvement proposal.
Suggestions are appreciated always!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net