New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Validation bypass for 'before' and 'after' rules when paired with 'date_format' rule. #24191

Merged
merged 2 commits into from May 21, 2018

Conversation

Projects
None yet
6 participants
@jonnsl
Contributor

jonnsl commented May 12, 2018

When using the validation rules 'before' and/or 'after' together with the 'date_format' rule, it is possible to have what should be considered invalid data be accepted by the validator as valid.

My understanding from reading the code is that the 'before' and 'after' rules, when used without de 'date_format' rule, will compare two fields relative to each other. e.g. given the rules:

$rules = ['start' => 'before:finish', 'finish' => 'after:start'];

The 'start' field must have a date which is less than the 'finish' field, and the 'finish' field must have a date which is greater than the 'start' field.

But, when the 'date_format' rule is also used, you can compare a field to another field or a specific date. And therein lies the problem. If you know what date the field is being compared to (You can easily guess that through the error message) and all fields from the request is being passed directly to the validator as data to be validated (This is quite common, Laravel itself does that in the 'ValidatesRequests' trait[1]). You can then send an extra field in the request with the same name as that date and a value of your choosing. The validator will then give preference to compare the field being validate relative to the bogus field, which can have an arbitrary date. (the tests added illustrate this more clearly)

I honestly have no idea how to fix this without breaking backwards compatibility, so this pull request only adds tests that should pass but are now failing.

[1]

$validator = $this->getValidationFactory()->make($request->all(), $validator);

Add more tests to Validator's before and after rules.
When the rules before and/or after are used together with the date_format rules, the validator should give preference to validate a given field relative to the fixed date specified in the rule, instead of comparing to a field with the same name as the specified date.
@tillkruss

This comment has been minimized.

Member

tillkruss commented May 13, 2018

We don't merge broken tests. Please re-submit with fix.

@tillkruss tillkruss closed this May 13, 2018

@laurencei

This comment has been minimized.

Member

laurencei commented May 14, 2018

@tillkruss - to be fair - the contribution documents explicity state he's done the right thing:

"Bug reports" may also be sent in the form of a pull request containing a failing test.

@tillkruss tillkruss reopened this May 14, 2018

@devcircus

This comment has been minimized.

Contributor

devcircus commented May 14, 2018

Maybe the docs just need to be reworded though because Taylor consistently says the same about not merging failing tests.

@jonnsl

This comment has been minimized.

Contributor

jonnsl commented May 14, 2018

@devcircus I didn't expect this to be merged. It's just bug report with tests attached. Tests are a better way of reporting a bug than simple creating a new issue.

@devcircus

This comment has been minimized.

Contributor

devcircus commented May 14, 2018

Sure but the common response to these PRs can be confusing. "We can't do anything with this" (though it may be true), doesn't quite reflect the suggestion that this is a legitimate way to report bugs. So maybe the docs could be clarified as to how these cases should be handled.

@TBlindaruk

This comment has been minimized.

Member

TBlindaruk commented May 14, 2018

Could we create an issue for this PR and create label for the issue like "development needed"?

@tillkruss

This comment has been minimized.

Member

tillkruss commented May 14, 2018

@taylorotwell: Thoughts?

Fix validation bypass for rules 'before' and 'after' when paired with…
… 'date_format'.

Now when when there is both a 'date_format' rule and a 'before' and/or 'after' rule,
the first parameter will be converted to a timestamp, and only if that fails, it will
be considered as a name for another field. This exactly the same behavior when there
isn't a date_format rule.

fixes #24191
@jonnsl

This comment has been minimized.

Contributor

jonnsl commented May 15, 2018

I think I've fixed it. Added another commit.

@taylorotwell taylorotwell reopened this May 21, 2018

@taylorotwell taylorotwell merged commit f1f2276 into laravel:5.5 May 21, 2018

1 of 2 checks passed

continuous-integration/styleci/pr The analysis is running
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jonnsl jonnsl deleted the jonnsl:validation-bypass branch May 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment