Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Warn on actual alterations #63

Open
Chealer opened this issue Jan 12, 2018 · 9 comments
Open

Warn on actual alterations #63

Chealer opened this issue Jan 12, 2018 · 9 comments

Comments

@Chealer
Copy link

Chealer commented Jan 12, 2018

Our project is using Zend filters for security purposes. For example, we might want to ensure that a text input contains an integer using Zend\Filter\ToInt. filter() gives us the guarantee that the returned value is an integer. Unfortunately, it may quietly alter the input.

If the input is string "3" and the output is int 3, there is no problem.
But if the input is string "four" and the output is 0, there is a problem - either the wrong filter was used, or the UI should not have let "four" be entered.

To ensure that problems are detected, filter() should warn when an actual alteration is done, for example by triggering a PHP warning.

@weierophinney
Copy link
Member

zend-filter is not a validation library. It's purpose is to transform data and/or normalize data.

The proper place to do checks like this is within a validator.

If you are using this particular filter with a data set used within zend-inputfilter, what I'd do instead is use the validator Zend\Validator\Digits to ensure every value within the string is a numeric digit.

@Chealer
Copy link
Author

Chealer commented Jan 15, 2018

Thank you @weierophinney ,
We do want data normalization. If we have a string which contains "00", we do want to get a null int. What we do not want, however, is that even strings which are not a valid way to represent 0, such as "four", end up quietly converted to 0. If filter() returns 0 for such cases, we expect the call to trigger a warning.

Note that while I requested a warning to be triggered, I would find it a proper solution too to just refuse to guess a result and - for example - throw an exception. To phrase this differently, "don't quietly take a guess".

@froschdesign
Copy link
Member

@Chealer
The filter uses PHP's normal behaviour for conversion from string to numbers:

If the string starts with valid numeric data, this will be the value used. Otherwise, the value will be 0 (zero).

http://php.net/manual/en/language.types.string.php#language.types.string.conversion

If your use case doesn't allow this conversion, than add a validator in front.

@weierophinney
Copy link
Member

@Chealer Another route is to create a custom filter, if the current one does not implement the behavior you require.

Typically, we do not want our filters to raise exceptions. If they cannot filter the value, they should return the original value verbatim. So, in this case, I'd write a filter that checks to ensure the value is not malformed, and, if not, filters it, but otherwise returns it verbatim. Then use a validator to see if the value is valid for the domain.

Alternately, if you really want the exception and/or error, trigger it.

But we do not plan to implement such a feature in zend-filter, for the reasons stated above.

@Chealer
Copy link
Author

Chealer commented Jan 15, 2018

@froschdesign , I read ToInt's code and understand why it happens. If your point is that PHP itself already has a problematic behavior, I do not disagree.

It's not really that a specific use case doesn't allow such conversion. We use Zend filters to normalize inputs throughout the application. The designers must not have realized this problem, but this "filtering" is often quietly altering input and complicating debugging.

@Chealer
Copy link
Author

Chealer commented Jan 15, 2018

Thank you @weierophinney ,
I understand that we can work around this, but this requires some effort, and our philosophy leads us to contribute to our upstreams.

If there is no real solution in sight, I recommend mitigating through documentation.

@weierophinney
Copy link
Member

@Chealer We'd gladly accept documentation to cover this scenario. Docs are in the repository itself.

@froschdesign
Copy link
Member

@Chealer

If your point is that PHP itself already has a problematic behavior…

No, but we should document this behavior for the filter.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-filter; a new issue has been opened at laminas/laminas-filter#6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants