WordPress.org

Make WordPress Core

#50620 closed defect (bug) (fixed)

REST API: regression after introducing changes how block renderer endpoint is now defined

Reported by: manooweb Owned by: TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: REST API Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Hello,

We developed a dynamic block with only boolean block attributes.
For this development we used WordPress 5.4.2 and everything works fine.

We are testing with the future version of WordPress 5.5 ( WordPress 5.5-beta1-48410 ) and we noticed a regression because sanitization of block attributes doesn't work anymore espacially for boolean attributes which is important to get the right value in PHP (true or false) and not a string.

Indeed there has been changes in the REST API block renderer endpoint definition introduced by this changeset [48069] to fix this ticket #48079.

As we can see in the endpoint schema we lost the 'properties' attributes which is used before to set the block attributes.

Everything is ok during the attributes validation which passes through the 'validate_callback' defined in the endpoint.

However during the REST API process we also pass through a sanitization step here https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/class-wp-rest-server.php#L965

and then here for block attributes
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api.php#L1864

We never pass through the condition because the 'properties' array is never defined at this point as it should be.

As every dynamic blocks need their own attributes, I don't understand how to set correctly this 'properties' array to get the values correctly sanitized.

Of course we found a workaround on our side directly in our 'render_callback' of our dynamic block function but I think it is not the best and right solution.

If you need further information, let me know.

Regards

Attachments (2)

50620.diff (1.3 KB) - added by manooweb 17 months ago.
Add sanitize_callback in block renderer endpoint
50620.1.diff (4.9 KB) - added by manooweb 17 months ago.
Add unit test

Download all attachments as: .zip

Change History (12)

#1 @manooweb
17 months ago

  • Summary changed from REST API: regression after introducing changes how block renderer endpoint is non defined to REST API: regression after introducing changes how block renderer endpoint is now defined

#2 @TimothyBlynJacobs
17 months ago

  • Keywords needs-patch needs-unit-tests good-first-bug added
  • Milestone changed from Awaiting Review to 5.5

Hi @manooweb,

Thanks for the ticket! That's a great catch. Do you want to work on a patch for it? We'd essentially want to duplicate our validate_callback that is set, but at the end call rest_sanitize_value_from_schema instead of the validate function.

#3 @manooweb
17 months ago

Hi Timothy,

You're welcome

Of course, I can try.
What do you mean excatly by duplicate

Create a sanitize_callback entry which duplicates the code of validate_callback except the call to rest_validate_value_from_schema replaced by a call to rest_sanitize_value_from_schema ?

#4 @manooweb
17 months ago

Timothy, I attached the patch. I tested manually on my WordPress environment and solve the issue on our dynamic block.

Do I also have to work on unit tests ?

@manooweb
17 months ago

Add sanitize_callback in block renderer endpoint

#5 @TimothyBlynJacobs
17 months ago

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

That looks great, thanks!

Do I also have to work on unit tests ?

You can work on the tests if you want to, but I can add them if you don't. If you are interested, this might be a helpful guide: https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/

#6 @SergeyBiryukov
17 months ago

  • Description modified (diff)

@manooweb
17 months ago

Add unit test

#7 @manooweb
17 months ago

Hi Timothy,
I've just added a new patch containing the fix and thus a unit test.

Have a nice week-end 😉

This ticket was mentioned in PR #400 on WordPress/wordpress-develop by TimothyBJacobs.


17 months ago

  • Keywords has-unit-tests added

#9 @TimothyBlynJacobs
17 months ago

Thanks for the tests @manooweb! Committing with some minor changes to the tests to make them more explicit.

#10 @TimothyBlynJacobs
17 months ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 48437:

REST API: Sanitize block renderer attributes.

In [48069] the Block Renderer was changed to register a single route for all dynamic blocks. Validation was dynamically applied based on the requested block, but sanitization was not. This commit adds the same sanitization back to the block attributes.

Props manooweb.
Fixes #50620. See #48079.

Note: See TracTickets for help on using tickets.