WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#37192 closed defect (bug) (fixed)

Validate before sanitizing when processing REST Request arguments

Reported by: danielbachhuber Owned by: rachelbaker
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Currently, sanitization is called before validation. If you're using validation to check that an argument is of a particular type, the validation can fail if the sanitization callback transforms the type.

Instead, the validation callback should be called before the sanitization callback. It was a mistake that the current behavior is what it is, which we should correct before too long.

Attachments (2)

schlessera-37192-1.diff (2.4 KB) - added by schlessera 3 years ago.
Fix + test
37192.2.diff (2.4 KB) - added by rachelbaker 3 years ago.
Fix test and trim whitespace

Download all attachments as: .zip

Change History (15)

#1 @rachelbaker
3 years ago

  • Milestone changed from 4.6 to Future Release

Punting out of 4.6, can come back to this milestone with a patch.

@schlessera
3 years ago

Fix + test

#2 @schlessera
3 years ago

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed

I have written a test that failed with the currently committed behavior (first sanitizing, then validating), and then made the fix to first validate, and then sanitize, which makes the test succeed again.

This is my first patch submission, so please let me know if I missed something.

#3 @danielbachhuber
3 years ago

  • Milestone changed from Future Release to 4.6

Thanks @schlessera !

#4 @rachelbaker
3 years ago

  • Keywords commit added
  • Owner set to rachelbaker
  • Status changed from new to reviewing

#5 @rachelbaker
3 years ago

Let's get this in 4.6! I'll review asap.

#6 @westonruter
3 years ago

In 37942:

Customize: Reverse order of setting sanitization/validation, validating prior to sanitizing.

Reverses order where sanitization was being applied before validation originally in accordance with REST API logic.

Props westonruter, schlessera.
See #34893.
See #37192.
Fixes #37247.

@rachelbaker
3 years ago

Fix test and trim whitespace

#7 @rachelbaker
3 years ago

In 37192.2.diff I fixed the unit test that was returning a 404 instead of 200 due to mismatched namespace in the route registration.

#8 @rachelbaker
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 37943:

REST API: Reverse order of setting sanitization/validation, validating prior to sanitizing.

Fixes mistake in the current behavior, where the sanitization callback ran before the validation callback. Now the validation callback will run before the sanitization.

Props schlessera, rachelbaker.
See #37247.
Fixes #37192.

#9 @joehoyle
3 years ago

This slipped through the net, so only just noticed this. I wanted to note this was not a mistake at the time, and was a conscious decision, see my original proposal and PR for this here: https://github.com/WP-API/WP-API/pull/989

Though the terminology can be mixed, the basic through process was:

  1. Clean (sanitize) the input data to make sure it's of the type you expect (primarily a security measure)
  2. Check the value (cleaned) for "validity", meaning, is the value "draft" valid for the post status.

This is why it was first sanitize, then validate. Sanitize is a transformer to the expected data type, validate is essentially a bool check.

If you're using validation to check that an argument is of a particular type, the validation can fail if the sanitization callback transforms the type.

In the above case, the sanitization_callback would have incorrectly transformed the type to something that "validate_callback" was not unable to determine if the value was invalid. A good example of this is sanitize_email as that will actually return an empty string (or false I think) if a string that is not an email is passed, in which case the data then passed to validate_callback is empty. In this case, sanitize_email is just a _bad actor_ , that's why we special case it rest_sanitize_request_arg here: https://github.com/WP-API/WP-API/blob/8edf3fc7454ea7e774f4b7e290efdc51b0feac63/plugin.php#L395

This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.


3 years ago

#11 @schlessera
3 years ago

@joehoyle In this case, at least the naming is off.

Validation is what you do to make sure user input is valid. This includes checking accepted type, accepted content and matching context.

Sanitization is a transformation you do on data to make it safe for storing, to prevent stuff like SQL injection.

Disregarding the naming, though, it should also be obvious that you shouldn't have valid data that the user provided become invalid before doing the actual validation check.

#12 @ChopinBach
3 years ago

My interpretation of using sanitize_callback(), after starting to use the API, was that its intention was inline with what Joe pointed out.

  1. Clean (sanitize) the input data to make sure it's of the type you expect (primarily a security measure)

The API itself is using extremely loose validation. If we implement this change, (well, it is already implemented) we will potentially need to do a lot more logic in the validation, as we will be relying on loose type and other things.

Potentially, as Joe mentioned in slack, <script>alert('ZOMG Hacking you Hard')</script> would by default, the way the WP REST API plugin is currently configured, now be "valid" input, which would need to then be "sanitized". In its original form, sanitize_callback() acted as a fault tolerant white list filter. But because of it being named sanitize_callback(), we will now need to move some of that logic into the validation, or provide more robust validation that is not fault tolerant.

Overall, after thinking about it, this patch is probably good and potentially we should just slap all of the logic into validate callback, so there is not any semantic headaches in the future. I fail to see the harm of what the original implementation was; but I could definitely be wrong.

Maybe to make everyone happy, we can rename things into a three pronged approach of filter_callback(), validate_callback(), sanitize_callback().

Last edited 3 years ago by ChopinBach (previous) (diff)

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.