-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Target Hints: Add missing param sanitization #65280
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Adding the "No Core Sync Required" label solely because the core PR wasn't actually merged yet |
Flaky tests detected in a53c8b0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10829903591
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm that this fixes the fatal error with the Web Stories plugin.
The unit test coverage would be nice to have, but it is probably not a blocker for the Gutenberg plugin fix if we want to ship a minor release as soon as possible.
@swissspidy Are you able to add a link to this PR to https://github.com/WordPress/gutenberg/blob/trunk/backport-changelog/6.7/7139.md |
Weird workflow, clearly I am not accustomed to it. Done now. |
E2E failures are unrelated and happen because of a core change. |
I'm adding tests in WordPress/wordpress-develop#7139, is that properly testing what you were running into @swissspidy? |
I think so, yes, thanks @TimothyBJacobs! |
@Mamaduka do you think this warrants a minor release? if not, when is 19.3 happening? |
What?
This fixes a serious bug in the
Gutenberg_REST_Server
class which is intended for 6.7 merge. This bug causes fatal errors for plugins like Web Stories.#64504 updated
WP_REST_Sever
to add thetargetHints
property. LikeWP_REST_Server::dispatch()
it usesmatch_request_to_handler()
to get the correct handler and then "sends" the correct headers.The problem is that it does not sanitize the request params it gathered from
WP_REST_Request::from_url()
andmatch_request_to_handler()
. So if the controller in question expects anint
, it will get astring
and if it has strict typing, that leads to fatal errors.Note: This might warrant a 19.2.1 release.
Why?
Bugs are bad!
How?
Fixes the bug by validating and sanitizing the params.
I don't have the capacity right now to add tests, so if someone else can do it, that would be great.
Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A