Skip to content
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

Merged
merged 5 commits into from
Sep 17, 2024
Merged

Conversation

swissspidy
Copy link
Member

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 the targetHints property. Like WP_REST_Server::dispatch() it uses match_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() and match_request_to_handler(). So if the controller in question expects an int, it will get a string 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

  1. Install the Web Stories plugin
  2. Open the Web Stories editor
  3. See (or not see) a fatal error

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

@swissspidy swissspidy added [Type] Bug An existing feature does not function as intended REST API Interaction Related to REST API labels Sep 12, 2024
Copy link

github-actions bot commented Sep 12, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@swissspidy swissspidy added the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label Sep 12, 2024
@swissspidy
Copy link
Member Author

Adding the "No Core Sync Required" label solely because the core PR wasn't actually merged yet

Copy link

Flaky tests detected in a53c8b0.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10829903591
📝 Reported issues:

Copy link
Member

@Mamaduka Mamaduka left a 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.

@peterwilsoncc
Copy link
Contributor

@swissspidy swissspidy removed the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label Sep 13, 2024
@swissspidy
Copy link
Member Author

Weird workflow, clearly I am not accustomed to it. Done now.

@swissspidy
Copy link
Member Author

E2E failures are unrelated and happen because of a core change.

@TimothyBJacobs
Copy link
Member

I'm adding tests in WordPress/wordpress-develop#7139, is that properly testing what you were running into @swissspidy?

@swissspidy
Copy link
Member Author

I think so, yes, thanks @TimothyBJacobs!

@swissspidy swissspidy merged commit 6722990 into trunk Sep 17, 2024
61 checks passed
@swissspidy swissspidy deleted the fix/targethints branch September 17, 2024 20:31
@swissspidy
Copy link
Member Author

@Mamaduka do you think this warrants a minor release? if not, when is 19.3 happening?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants