Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95167
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: <CAGa2bXa78fkZw8gtepmBDu+VDwHW_WiDDv65=zzgLTFqYxF+DA@mail.gmail.com>
References: <CAGa2bXarw2jTj0yhXpywWNQOJLR2pdhfaqSdwj_btT2giEL15Q@mail.gmail.com>
 <CAGa2bXadVagTwBzEym9PZROmXu3SKxg=huCAQNK3k1-jZeG_pg@mail.gmail.com>
 <CAGa2bXY-vmEgEFrE0OxQPSx=FKHeHdCayrDXxtTkHUVaJbV+-A@mail.gmail.com>
 <CA+kxMuRiOBQpmTeKqNyV8rX0GKCLrYixi--y5TcYUkdqpT746w@mail.gmail.com> <CAGa2bXa78fkZw8gtepmBDu+VDwHW_WiDDv65=zzgLTFqYxF+DA@mail.gmail.com>
Date: Mon, 15 Aug 2016 12:35:56 +0900
Message-ID: <CAGa2bXbOeWgk4xDLkBL4RDR95-Lnwagw2M-kgX8vE2P1k3PQiA@mail.gmail.com>
To: Dan Ackroyd <danack@basereality.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Re: [RFC][VOTE] Add validation functions to filter module
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

On Mon, Aug 15, 2016 at 11:15 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Dan,
>
> Thank you for sharing idea!
>
>
> On Mon, Aug 15, 2016 at 10:25 AM, Dan Ackroyd <danack@basereality.com> wrote:
>>
>> On 15 August 2016 at 01:53, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>>
>>> One more usual request.
>>> Please describe reason(s) why you object proposal.
>>
>>
>> I'm not entirely sure why you ask for reasons when people vote no. The
>> reasons are almost always the same as the reasons given before the
>> voting starts.
>
> Without feedback, there is no clue which way should go or improve.
> I didn't realize some of your idea and I think your feedback is great.
>
>>
>> But for posterity:
>>
>> i) Validation error messages need to specify what is wrong.....which
>> is bespoke to the application. Which is a reason why validation code
>> belongs in userland.
>
> When exception is enabled, offensive key name is written in exception message.
>
> When exception is disabled, your statement is true. This could be improved.
> Good feedback.
>
>>
>> ii) Validation error message need to be in the correct language for an
>> application. It is not a good approach for people to be trying to
>> match strings emitted by internal code and trying to convert them to
>> the correct language.
>
> It seems there is misunderstanding.
> These new functions are intended for "secure coding input validation" that
> should never fail. It means something unexpected in input data that
> cannot/shouldn't keep program running. Why do you need to parse
> message?
>
> All needed info, filter name, key and value, is in exception message and
> exception object, BTW.
>
> This one is good feedback, too.
> I appreciate better error message suggestions.
>
>>
>> iii) The argument that it needs to be fast could be applied to
>> anything and everything, and so is bogus. The RFC doesn't even show
>> that userland implementations are slow enought to be a concern.
>
> I thought I don't have to have example of userland implementation, so
> it's good feedback also.
>
> Typical OO implementation uses number of setters to define validation rules.
> In addition, it validates validation rule is OK for it. e.g. It will
> check input data type at least. Setters and validation rule validation
> makes execution slower obviously.
>
> One may optimize validation rules to plain array (like I do).
> In this case, performance is could be better than previously mentioned
> validators do all in the production environment.
>
> I also thought the performance issue is not much important because
> there is no PHP feature to compare. All of us knew PHP function call
> overheads are relatively large and proposed almost all in C implementation
> would be faster than userland.
>
>>
>> iv) The RFC makes an assumption that programs should exit when validation fails.
>>
>> "Input data validation should accept only valid and possible inputs.
>> If not, reject it and terminate program."
>>
>> and the code example:
>>
>>> catch (FilterValidateException $e) {
>>>    var_dump($e->getMessage());
>>>    die('Invalid input detected!'); // Should terminate execution when input validation fails
>>> }
>>
>> This assumption is bogus.
>>
>> Any program that accepts data from users should provide useful error
>> messages when the data is wrong with someting as simple as a string
>> being too long.
>
> There is misunderstanding on this.
> As I wrote explicitly in the RFC, input validation and user input
> mistakes must be handled differently.
>
> "The input validation (or think it as assertion or requirement) error"
> that this RFC is dealing, is should never happen conditions (or think
> it as contract should never fail).
>
> The point of having the input validation is accept only inputs that
> program expects and can work correctly. Accepting unexpected
> data that program cannot work correctly is pointless.
>
> Don't misunderstood me. I'm not saying "You should reject user input mistakes".
> "User input mistakes" and "input validation error" is totally different error.
>
>>
>> v) I don't like the current filter functions, and recommend people
>> avoid using them. Adding to them with an even harder to use API is the
>> wrong way to go.
>
> I didn't recommend it either because it could not be used for input
> validation easily, escaping or sanitization could be done for
> dedicated API.
>
> Having new module is one of my idea also. However, I realized many of
> filter module codes could be reused after investigation. That's the
> reason why I added to filter module. I also named new functions to
> have "validate_*" prefix, rather than "filter_*" to emphasis it's
> for validations. I renamed them to "filter_*" to comply CODING_STANDARDS.
>
> This feedback is great because I'm worrying about the same thing.
> Please feedback this kind of things during discussion so that I can
> do something on issues.
>
> Thank you for comments.
> I think it's very helpful for improvements!


Hi Dan,

I don't mind suspend vote for a while to resolve issues if there
should be changes in the RFC.  I also don't mind adding missing
features, e.g. helpful error messages when exception is disabled, to
my todo list. BTW, I'll document basic idea of secure coding and
emphasize how it should be done, so misuse would be few.

It seems you aren't objecting the idea itself.
Could you give more feedback what's missing to change your vote?

Thank you!

--
Yasuo Ohgaki
yohgaki@ohgaki.net