Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37247 closed defect (bug) (fixed)

Sanitization needs to come after validation in WP_Customize_Manager

Reported by: schlessera's profile schlessera Owned by: westonruter's profile westonruter
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

In WP_Customize_Manager, the following methods validate after sanitizing:

  • post_value()
  • validate_setting_values()

Validation is checking whether the input is valid according to business rules. Sanitization is transforming the input to make it ready to be persisted.

The order in which they are used within the above methods, apart from just conceptually being the wrong way around, might cause a valid input to fail validation because the sanitization had already changed it.

Related to #37192

Attachments (2)

37247.0.4.diff (1.5 KB) - added by westonruter 8 years ago.
37247.0.5.diff (4.5 KB) - added by schlessera 8 years ago.
@westonruter 's fix + additional tests

Download all attachments as: .zip

Change History (8)

#1 @westonruter
8 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to westonruter
  • Status changed from new to accepted
  • Version set to trunk

@schlessera great observation. This is something I debated quite a bit, and I went with the current sanitize > validate scheme because the REST API was doing the same. Now that the order of operations is being changed in #37192, I agree that the Customizer should be aligned. Note, however, that the Customizer's implementation allowed for the sanitize callbacks to also return WP_Error instances, thus allowing the sanitize callbacks to also perform validation since the two are often very closely related. A sanitize callback can return a WP_Error in the case where the value being sanitized is “too far gone” to be recovered for the purposes of passing through and needs to be be flatly rejected (marked as invalid).

#2 @westonruter
8 years ago

  • Keywords has-patch added

@schlessera how does 37247.0.4.diff work for you?

@schlessera
8 years ago

@westonruter 's fix + additional tests

#3 @schlessera
8 years ago

  • Keywords has-unit-tests added

@westonruter Looks good to me.

I have taken your patch and added two tests. The tests fail without your patch, and succeed with your patch applied.

#4 @westonruter
8 years ago

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

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.

#5 @rachelbaker
8 years ago

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.

#6 @westonruter
8 years ago

In 38299:

Docs: Update outdated phpdoc for WP_Customize_Manager::validate_setting_values() to reflect changes in [37942].

Props dlh.
See #37247.
Fixes #37759.

Note: See TracTickets for help on using tickets.