REST API: Check for a missing post before the comment status capability. - #12764
REST API: Check for a missing post before the comment status capability.#12764ramonjd wants to merge 2 commits into
Conversation
`WP_REST_Comments_Controller::create_item_permissions_check()` checked whether the user is allowed to set `status` before checking that a `post` was supplied at all. A request carrying a status but no post was therefore rejected as `rest_comment_invalid_status`, which points at the wrong parameter. Move the missing-post guard above the status capability check so the missing post is reported as such. Fixes #65761.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
🟡 Not ready to approve
The reordered guard also changes behaviour for type=note requests with status and no post, but there is no regression test covering that case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adjusts WP_REST_Comments_Controller::create_item_permissions_check() to validate that a post is provided before performing the capability check for the status parameter, ensuring the API returns an error that correctly points to the missing post parameter. It also adds a PHPUnit regression test to confirm the corrected error code for a user who cannot set status.
Changes:
- Reordered the “missing post” guard to run before the
statuscapability check in comment creation permissions. - Added a REST API PHPUnit test asserting
rest_comment_invalid_post_idwhenstatusis provided butpostis missing for a non-privileged user.
File summaries
| File | Description |
|---|---|
| src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php | Moves the missing-post check earlier so the REST API reports the correct parameter error. |
| tests/phpunit/tests/rest-api/rest-comments-controller.php | Adds regression coverage for the missing-post + status scenario (non-note). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Notes resolve the status capability against `edit_post` with the submitted post ID. With no post that becomes `current_user_can( 'edit_post', 0 )`, which `map_meta_cap()` denies for every role, so the missing post was reported as a status error for all callers including administrators. Cover the note variant so the guard order stays put. See #65761.
There was a problem hiding this comment.
🟢 Ready to approve
The change is a small, well-scoped reorder aligned with the Trac report and is backed by targeted new tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
tests/phpunit/tests/rest-api/rest-comments-controller.php:3847
- The docblock describes runtime behavior that is no longer true after this change: with the new early
empty( $request['post'] )guard, thestatuscapability check is never reached when no post is supplied, so it doesn't "fall back" tocurrent_user_can( 'edit_post', 0 )anymore. Reword to past/conditional tense to avoid misleading future readers.
* Without a post, the `status` capability check falls back to
* `current_user_can( 'edit_post', 0 )`, which no role can satisfy. An
* administrator is used here to show the missing post is reported even for a
* user holding every capability.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
andrewserong
left a comment
There was a problem hiding this comment.
Good catch, and thank you for the thorough testing instructions. This patch is testing well, with the expected error messages now for an author user, and the happy path for an admin unchanged.
LGTM!
What
WP_REST_Comments_Controller::create_item_permissions_check()checks whether the caller is allowed to set thestatusparameter before it checks that apostwas supplied at all. A request that carries astatusbut nopostis therefore rejected withrest_comment_invalid_status, "Sorry, you are not allowed to edit 'status' for comments", which points at the wrong parameter. The actual problem is the missing post.This moves the missing-post guard above the status capability check. It is a pure move of about seven lines; no logic changes.
Trac ticket: https://core.trac.wordpress.org/ticket/65761
Why the wrong error appears
The status check consults
moderate_commentsfor ordinary comments. An Administrator has it, so the check passes and execution reaches the missing-post guard, producing the correct error. A Subscriber, Contributor or Author does not, so it stops one guard early and reports the status problem instead. That asymmetry between roles is the bug.Behaviour changes
The reorder flips the reported error code in two situations. Both remain HTTP 403 and both remain rejected. Only the reason changes.
statusset, nopost, caller lacksmoderate_commentsrest_comment_invalid_statusrest_comment_invalid_post_idtype: note,statusset, nopost, any callerrest_comment_invalid_statusrest_comment_invalid_post_idThe second row is easy to miss. For a note,
$edit_capbecomesarray( 'edit_post', (int) $request['post'] ), which isarray( 'edit_post', 0 )when no post is supplied.map_meta_cap()returnsdo_not_allowforedit_postwheneverget_post()finds nothing, so that check failed for every caller, including Administrators, before this change.Because a response code changes, this belongs in a major release rather than a point release, hence 7.2.
Testing instructions
To check by hand, as an Author with a published post open in the block editor:
Still as an Author, check that you get the right "status" error when you do provide a post id:
Then do a happy path as an Admin in a post:
Tests