docs: Improve permission checking documentation and examples #95
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @mohamed.khaled@9hdigital.com. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #95 +/- ##
============================================
+ Coverage 90.59% 94.20% +3.60%
============================================
Files 20 7 -13
Lines 1489 328 -1161
Branches 117 116 -1
============================================
- Hits 1349 309 -1040
+ Misses 140 19 -121
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I don't think we are really experiencing a security vulnerability here, because every ability handles permissions check individually during execution as discussed in #76 (comment). That being said, we can iterate on the handling because the run controller is unique, as its permissions check model differs for each ability. |
|
This PR needs a rebase to resolve conflicts after #94 landed. This would help to review the changes proposed in this PR. |
|
@gziolo on it |
… examples - Eliminate all manual permission checking examples that could lead to security vulnerabilities - Add advanced error handling section with specific error code handling - Update variable names for consistency across examples - Address @justlevine's feedback about documentation anti-patterns
Change permission check from loose to strict comparison in run_ability_permissions_check(). The previous pattern `if ( ! $ability->check_permission( $input ) )` would incorrectly pass WP_Error objects as truthy, potentially allowing unauthorized access. Fixed by using `if ( true !== $ability->check_permission( $input ) )` to properly handle both false and WP_Error return values.
The strict permission checking now correctly catches input validation failures in the permission check phase, returning 403 instead of allowing invalid requests to proceed to execution-specific validation. Updated 4 failing tests to expect 403 status and appropriate error codes: - test_resource_ability_requires_get - test_get_request_with_non_array_input - test_post_request_with_non_array_input - test_input_validation_failure_returns_error
Ref34t
force-pushed
the
feature/fix-documentation-clean
branch
from
9df7d76 to
bb906c3
Compare
Ref34t
changed the title
|
I've updated the PR title and description - you're right that this isn't a security vulnerability. The reason: even if the REST API permission check had the WP_Error truthiness issue, This update improves code correctness by using strict comparison ( |
|
I'm in the process of updating documentation in preparation for the 6.9 merge request in #117, so once that's merged, I'll come back here to check what still needs to be included. |
|
|
||
| $input = $this->get_input_from_request( $request ); | ||
| if ( ! $ability->check_permissions( $input ) ) { | ||
| if ( true !== $ability->check_permissions( $input ) ) { |
There was a problem hiding this comment.
Sharing relevant feedback that I initially mentioned in WordPress/wordpress-develop#9410 (comment): I don't think we should replace a contextually more specific error (from the actual permission callback) with a generic "Sorry you can't do this" error.
There was a problem hiding this comment.
Good catch! I've updated the code to preserve specific error messages from the permission callback instead of replacing them with a generic error.
The permission check now:
- Returns the
WP_Errordirectly ifcheck_permissions()returns one (preserves context) - Only falls back to the generic error if it returns
false
resolved here https://github.com/WordPress/abilities api/pull/95/commits/27b7b1a8215023b128cbcc55a27f51ac23e8774c
There was a problem hiding this comment.
Some minor documentation suggestions.
Also, the docs folder structure has been updated in #117, so you might have to fetch those updates.
Don't replace contextually specific errors from check_permissions() with generic 'Sorry, you are not allowed' error. If check_permissions() returns a WP_Error, preserve and return it directly. Only use generic error as fallback when permission check returns false.
5 participants
Summary
Changes Made
Documentation Improvements
REST API Controller Update
true !==) for consistency with execute() patternDiscussion Point
As @gziolo noted, this isn't a security vulnerability since each ability's
execute()method already performs comprehensive permission checking. The REST API controller creates a two-layer permission model:run_ability_permissions_check()execute()The controller's permission callback is architecturally unique because it dynamically checks permissions for any ability. We could consider simplifying by removing the separate permission callback and relying entirely on
execute()'s error handling.Open to feedback on this approach.
Test plan