wp_create_user_request should sanitize the action_name using _wp_privacy_action_request_types

[garrett-eclipse]

Hello,

It was flagged by @birgire in #44721 that the wp_create_user_request would accept any action name.

The check against _wp_privacy_action_request_types found in _wp_personal_data_handle_actions should be moved into wp_create_user_request to check against invalid request actions. The check I'm speaking of;
​https://github.com/WordPress/wordpress-develop/blob/5.1.1/src/wp-admin/includes/user.php#L691-L698

As the wp_create_user_request is called directly after the check moving it into the function results in the same sanitization for _wp_personal_data_handle_actions while also sanitizing the other methods such as _wp_privacy_send_erasure_fulfillment_notification.

All the best

[garrett-eclipse]

Patch to move check for _wp_privacy_action_request_types into wp_create_user_request, along with updated unit tests to avoid failures

[garrett-eclipse]

Attached 46536.diff to move the check on _wp_privacy_action_request_types into wp_create_user_request to ensure more coverage of the check as it previously only covered _wp_personal_data_handle_actions and overlooked actions like _wp_privacy_send_erasure_fulfillment_notification.

To reduce and still distinguish the ! $action_name check I updated it to missing_action error and used it's invalid_action for the new check on _wp_privacy_action_request_types.

In order for the changes to pass existing unit tests I had to make the following adjustments;

• Replaced the original test_invalid_action in wpCreateUserRequest.php with test_missing_action to confirm the change to the ! $action_name error.
• Updated the test_invalid_action to confirm action names that don't pass the _wp_privacy_action_request_types check are caught
• Updated test_sanitized_action_name to use a unsanitized version of export_personal_data which passes the _wp_privacy_action_request_types error check.
• Updated wpSetUpBeforeClass inwpPrivacySendErasureFulfillmentNotification.php to use a valid action name remove_personal_data. NOTE: This test is also being addressed in #44721 so one or the other ticket will need a refresh once one is committed.
• Updated two additional invalid action names found in wpSendUserRequest.php

[garrett-eclipse]

Refreshed patch

[garrett-eclipse]

I've refreshed the patch in 46536.2.diff to remove the part of the patch already addressed in #44721 and fact the privacy portion found in user.php is now in privacy-tools.php due to #43895.

Everything is working nicely with my tests, @birgire does this address your original flag? Got time to test?

This is pretty well ready so moving into 5.6 for more eyes.

[birgire]

Thanks Garrett

This looks good. As I understand it, the original design was not to support any ad-hoc request type, I hope I understand that correctly, because of the existing _wp_privacy_action_request_types() checks.

I wonder if the original ticket number of the test_invalid_action() test case should be kept and the new ticket number added to it, like:

  * @ticket 44707
  * @ticket 46536
  */
  public function test_invalid_action() {

The action keys and their strings sounds rather general:

 WP_Error( 'invalid_action', __( 'Invalid action name.' ) );
 WP_Error( 'missing_action', __( 'Missing action name.' ) );

I also wonder if these strings would benefit from a translation context? (then in another ticket if needed)

[garrett-eclipse]

Refresh from @birgire's feedback

[garrett-eclipse]

Great feedback as always @birgire thank you.

Yes that's correct, the system is meant to only support the two ['export_personal_data', 'remove_personal_data'] you can't really get any others in currently without breaking core. In future we may revisit but would expect we'd do so via the _wp_privacy_action_request_types method as a single source for a filter and defaults. This change doesn't change any functionality aside from making things consistent and future-proof if we do want to add a filter at some point.

I've updated the patch in 46536.3.diff to ensure we have the original ticket number preserved on the test_invalid_action. Good catch.

As to expanding upon our action errors to be more specific, I'm 100% onboard. I thought maybe we used generic errors as they were re-used strings but searching that doesn't seem to be the case. I'll start a new ticket/patch, started here but found it was alot of strings so let's get this improvement in and iterate on the error strings separately.

Give it another once over if you don't mind and mark for committer review if you're happy.

[garrett-eclipse]

@birgire I opened and patched #51351, mind taking a look at it there and if it looks good we can move into 5.6.

[birgire]

thanks Garrett, this looks good to me, marking it with commit for now

[helen]

I have some tweaks I'd like to see here before commit, they are actually two different paths to take so open to any discussion.

Option 1 (my preference): we remove the if ( ! $action_name ) check entirely and just let empty-ish action names be considered invalid instead of differentiating as empty, and then also remove the corresponding missing test from the patch.

Option 2: change if ( ! $action_name ) to if ( empty( $action_name ) ) because the default value is an empty string, not a bool. Outcome is approximately the same, but seems more readable and precise to me. I'd love to hear more about how somebody would end up in a situation where they really need to differentiate between missing_action and invalid_action, and in that case, perhaps the display strings need to be more specific about what action because it's pretty generic but means different things in different contexts.

[garrett-eclipse]

Refresh to remove the unnecessary distinction between missing_action and invalid_action.

[garrett-eclipse]

Thanks for the review @helen, having some more time I delved back on this and agree option 1A is the way to go here, with the exception of keeping the unit test.

In 46536.4.diff I've refreshed the patch to drop the original if ( ! $action_name ) that presided prior to this ticket leaving the new if ( ! in_array( $action_name, _wp_privacy_action_request_types(), true ) ) conditional. Along with updating the unit test to use the correct invalid_action test result.

I preserved the unit test as this ticket seeks to change the existing behaviour that just checked if the action name is missing to also now check if there is a action name is it invalid. As these are two unique conditions we're satisfying with the single check in user.php I feel in our unit testing we should cover both these cases.

Let me know what you think?
Thanks

[helen]

In 49475:

Privacy: More precise checking of user request action names.

Props garrett-eclipse.
Fixes #46536.

[helen]

In 49476:

Remove accidentally duplicated code introduced in [49475].

See #46536.