-
Notifications
You must be signed in to change notification settings - Fork 26
docs: Improve permission checking documentation and examples #95
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
base: trunk
Are you sure you want to change the base?
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 85.69% 84.92% -0.78%
Complexity 103 103
============================================
Files 16 16
Lines 776 776
Branches 86 85 -1
============================================
- Hits 665 659 -6
- Misses 111 117 +6
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
9df7d76
to
bb906c3
Compare
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 ( |
docs/4.using-abilities.md
Outdated
Once abilities are registered, they can be retrieved and executed using global functions from the Abilities API. | ||
|
||
## Getting a Specific Ability (`wp_get_ability`) | ||
**Getting a Specific Ability (`wp_get_ability`)** |
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.
I've been known to misread diffs on mobile 🫣 but if all these reverted to **
, then it's a leak from #96 and should be changed back to semantic
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.
resolved 0fd3448
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