Fix has_permission() return type inconsistency (Issue #67) #76

Ref34t · 2025-09-14T09:54:48Z

Summary

Simplifies permission checking API and updates documentation to show proper is_wp_error() usage patterns for preventing WP_Error truthiness issues.

Problem Solved

The original has_permission() method could return either bool or WP_Error, creating a dangerous bug where WP_Error objects (which are truthy) could incorrectly pass permission checks in if statements.

Implementation

check_permissions($input, $skip_validation = false): New recommended method with clear bool|WP_Error return type
has_permission($input): Deprecated wrapper for backward compatibility
Updated documentation: Shows proper is_wp_error() usage patterns throughout
Streamlined API: Uses $skip_validation parameter for REST API permission checks

Testing

Existing comprehensive tests verify both methods work correctly:

Permission validation with proper error handling
Input validation integration with permission checks
Backward compatibility for deprecated has_permission() method

Fixes #67

github-actions · 2025-09-14T09:55:02Z

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @[email protected].

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.

Unlinked contributors: [email protected].

Co-authored-by: Ref34t <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: justlevine <[email protected]>
Co-authored-by: johnbillion <[email protected]>

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

justlevine · 2025-09-14T10:08:32Z

(@Ref34t keep #68 in mind (and #73) before you waste too much time because of failing tests - I'll give a real review one you tag ppl, just saw the rest errors and guessing there might be an unnecessary headache you dont need to care about)

Ref34t · 2025-09-14T10:34:30Z

@justlevine, thanks for keeping me aware of what is happening. You can review the fix itself now

justlevine

Aside from whether or not we need both a public checker function and a (also public) wrapper whose sole purpose is to coerce it to a bool, I think we need a better name for the function itself.

includes/abilities-api/class-wp-ability.php

justlevine · 2025-09-14T15:00:52Z

includes/abilities-api/class-wp-ability.php

 	 */
 	public function execute( $input = null ) {
-		$has_permissions = $this->has_permission( $input );
+		$has_permissions = $this->get_permission_status( $input );


Case in point to #76 (comment) - we're only using this internally while our only use of ::has_permission() is in a REST API endpoint (which long term should probably be an ability anyway

while our only use of ::has_permission() is in a REST API endpoint

Follow up: 👆(and not #73) is the reason why the tests are failing. WP_REST_Abilities_Run_Controller::run_ability_permissions_check() explicitly only checks against false and specifically allows the WP_Error through so invalid method errors are only shown to a user with the correct permissions when the callback is actually run:

https://github.com/ref34t/abilities-api/blob/8fcfcac9136dee73abb798bbaded0a21c17b5cd8/includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php#L93-L111

@Ref34t In the context of this PR, change ::run_ability_permissions_check() to if ( false === $ability->get_permission_status() (or whatever it's renamed to).

More broadly, I'm like 95% sure this leaked in from the A8C prior art and should be removed and punted to MCP Adapter, as "Resource" and "Tool" are not concepts of the Abilities API and don't belong in core. @emdashcodes / @gziolo please confirm, but either way it can be handled as part of #75.

More broadly, I'm like 95% sure this leaked in from the A8C prior art and should be removed and punted to MCP Adapter, as "Resource" and "Tool" are not concepts of the Abilities API and don't belong in core. @emdashcodes / @gziolo please confirm, but either way it can be handled as part of #75.

I noticed this when reviewing the PR, and I found it useful. I’m not 100% sure we should use this specific set of criteria to distinguish between GET and POST, but I didn’t have the bandwidth to start a larger conversation about the optimal approach as this seems more like an implementation detail. I think we could tap into Add property for marking Ability hints like destructive, read-only, idempotent to iron out the detection mechanism.

Overall, I’m tempted to borrow some of the solutions from MCP without using nomenclature and structure too tied this particular protocol.

Overall, I’m tempted to borrow some of the solutions from MCP without using nomenclature and structure too tied this particular protocol.

Yeah it's that latter part that's critical, and considering that MCP's value as a whole is doubtful, I would want a discussion of what a "resource" or "tool" is meant to be in our context, and whether it is inherently useful - if/when this gets revisited.

Either way, no longer part of this PR, so can be resolved.

includes/abilities-api/class-wp-ability.php

Ref34t · 2025-09-14T23:38:35Z

@justlevine Thanks for the feedback! Based on your comments and WP Core research, I’ve updated the PR to:
• Rename has_permission() → check_permissions() to avoid the misleading has_* boolean expectation.
• Standardize return types to true | false | WP_Error, matching Core patterns (like permissions_check()) and preventing WP_Error from being treated as true.
• Update execute() to always enforce permissions safely, centralizing logic and ensuring abilities can’t be bypassed.
• Align with Core standards while keeping developer usage simple and predictable.

These changes address #67, improve security, and follow Core conventions for REST and capability checks.

codecov · 2025-09-14T23:46:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.69%. Comparing base (a33e2be) to head (027c447).
⚠️ Report is 1 commits behind head on trunk.

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk      #76      +/-   ##
============================================
+ Coverage     85.64%   85.69%   +0.05%     
- Complexity      102      103       +1     
============================================
  Files            16       16              
  Lines           773      776       +3     
  Branches         86       86              
============================================
+ Hits            662      665       +3     
  Misses          111      111

Flag	Coverage Δ
javascript	`92.66% <ø> (ø)`
unit	`82.97% <100.00%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gziolo · 2025-09-15T09:03:52Z

You are exploring more advanced refactorings rather than a simple documentation update to account for possible incorrect usage. I left some other ideas to consider in #67 (comment) and #67 (comment).

includes/abilities-api/class-wp-ability.php

includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php

gziolo

I wanted to echo the feedback shared by @justlevine in #76 (comment). Can we limit this PR to renaming has_permission() to check_permission() with the included deprecation logic and documentation changes?

That part is ready to go.

Copilot

Pull Request Overview

Fixes a dangerous return type inconsistency in the has_permission() method that could cause WP_Error objects to incorrectly pass permission checks due to their truthiness. The solution introduces a new check_permission() method with clear return semantics and deprecates the old method for backward compatibility.

Introduces check_permission() method to replace problematic has_permission() with consistent API
Deprecates has_permission() while maintaining backward compatibility through delegation
Updates all method calls and documentation to use proper is_wp_error() checking patterns

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
includes/abilities-api/class-wp-ability.php	Adds new `check_permission()` method and deprecates `has_permission()`
includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php	Updates REST API to use new `check_permission()` method
tests/unit/abilities-api/wpRegisterAbility.php	Updates tests to use new method and adds deprecation test
docs/4.using-abilities.md	Updates documentation with proper `is_wp_error()` usage patterns
docs/2.getting-started.md	Updates getting started guide with correct error handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

includes/abilities-api/class-wp-ability.php

gziolo · 2025-09-26T12:42:10Z

@Ref34t, I see you are actively working on addressing feedback. I triggered review from GitHub Copilot to see if it catches anything. I'm still not sure whether check_permission or check_permissions would be a better choice. The part that triggered my thinking was documentation which uses plural everywhere.

Ref34t · 2025-09-26T12:44:53Z

@gziolo I had it before plural and now due to your last feedback I changed it again 😔 what do you think the final call here? 🤔

gziolo · 2025-09-26T12:52:36Z

The REST API uses permission_callback, so let's stick with the singular form, as it is also used for the ability. It looks like no further changes are necessary because in the docs I see the plural form when explaining the concept for REST API:

https://developer.wordpress.org/rest-api/extending-the-rest-api/adding-custom-endpoints/#permissions-callback

tests/unit/abilities-api/wpRegisterAbility.php

…per maintainer feedback

…on test

gziolo

All feedback addressed. It looks good to go from my angle. @justlevine how about you?

@jonathanbossenger, could you perform a quick sanity check on the docs changes?

justlevine · 2025-09-27T21:14:13Z

The REST API uses permission_callback, so let's stick with the singular form, as it is also used for the ability.

I'm sorry to be pedantic, but as I previously noted the REST API uses check_{*_}_permissions() plural, even though the registry arg is permission_callback. It's also more grammatically accurate when paired with the verb check_*.

I'm happy to do this myself in a follow-up PR if we feel bad about making @Ref34t keep swapping back-and-forth, but I do feel strongly about uniformity with core APIs.

justlevine

Looks good! Left some comments that have nothing to do with naming things.

justlevine · 2025-09-27T21:29:56Z

includes/abilities-api/class-wp-ability.php

 	 */
 	public function execute( $input = null ) {
-		$has_permissions = $this->has_permission( $input );
+		$has_permissions = $this->get_permission_status( $input );


Overall, I’m tempted to borrow some of the solutions from MCP without using nomenclature and structure too tied this particular protocol.

Yeah it's that latter part that's critical, and considering that MCP's value as a whole is doubtful, I would want a discussion of what a "resource" or "tool" is meant to be in our context, and whether it is inherently useful - if/when this gets revisited.

Either way, no longer part of this PR, so can be resolved.

justlevine · 2025-09-27T21:53:17Z

docs/2.getting-started.md

+    if ( $ability ) {
+        // Check permissions first - always use is_wp_error() to handle errors properly
+        $permission = $ability->check_permission();


I think this (and the previous example) is incorrect. By calling $ability->execute(), $ability->get_permission() gets called, there's rarely a reason to explicitly check permissions as a separate step.

if ( $ability ) { $site_title = $ability->execute(); // $site_title now holds the result of get_bloginfo('name') // error_log( 'Site Title: ' . $site_title ); } }

justlevine · 2025-09-27T22:01:51Z

docs/4.using-abilities.md

+
+```php
+// Method signature:
+// check_permission( $input = null )
+```


Is this intentional or did it leak?

justlevine · 2025-09-27T22:05:21Z

docs/4.using-abilities.md


-    // Check permission before execution
-    if ( $ability->has_permission( $input ) ) {
+    // Check permission before execution - always use is_wp_error() first


I know the doc in general uses a lot of unnecessary nesting, but I think this also make's it clearer than sandwiching the happy-path between two error states.

// Use a strict check to catch both false and WP_Error $permission = $ability->check_permission( $input ); if ( true === $permission ) { // Permission granted - safe to execute. $result = $ability->execute( $input ); if ( is_wp_error( $result ) ) { // Handle execution error echo 'Execution error: ' . $result->get_error_message(); } else { // Use $result if ( $result['success'] ) { echo 'Option updated successfully!'; echo 'Previous value: ' . $result['previous_value']; } } } else { // Don't leak permission errors to unauthenticated users. if ( is_wp_error( $permission ) ) { error_log( 'Permission check failed: ' . $permission->get_error_message() ); } echo 'You do not have permission to execute this ability.'; } }

Though imo we shouldn't be actively promoting check_permission() and instead have users fully handle execute()

$ability = wp_get_ability( 'my/ability' ); $result = $ability->execute( $input ) ; // If we want to only handle perm errors and not also invalid inputs etc. if ( is_wp_error() && 'ability_invalid_permissions' === $result->get_error_code() ) { // handle the error }

justlevine · 2025-09-27T22:07:26Z

includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php


 		$input = $this->get_input_from_request( $request );
-		if ( ! $ability->has_permission( $input ) ) {
+		if ( ! $ability->check_permission( $input ) ) {


Are we intentionally only handling false and saving the WP_Errors for ->execute()? cc @emdashcodes

Suggested change

if ( ! $ability->check_permission( $input ) ) {

if ( true !== $ability->check_permission( $input ) ) {

Let's fix it. I don't think it was intentional.

In my testing, the proposed code changes fail the result of 4 tests in the run controller. We would have to better handle the reason the request errored so it doesn't default to 403. It probably means, we would have to move some logic to run_ability_permissions_check.

An alternative would be to remove run_ability_permissions_check() (skip permission_callback) in the run controller, taking into account that run_ability (part of the callback) validates permissions anyway. It still would have to handle 403 when check_permissions() from the ability returns false.

Technically speaking, we are good. However, we can improve the implementation/documentation to avoid future misunderstandings.

…e-67

gziolo · 2025-09-30T07:59:14Z

I'm sorry to be pedantic, but as #76 (comment) the REST API uses check_{_}permissions() plural, even though the registry arg is permission_callback. It's also more grammatically accurate when paired with the verb check.

I'm not a native speaker, so I'm happy to follow your guidance here. I will land this PR and try address all the feedback for docs in another PR.

…e-67

gziolo · 2025-09-30T08:57:03Z

A follow-up #94 is ready for review. It addresses all feedback from @justlevine, but #76 (comment), which I think needs more thought.

Ref34t force-pushed the feature/fix-has-permission-return-type-issue-67 branch from fb27297 to 8fcfcac Compare September 14, 2025 10:38

justlevine mentioned this pull request Sep 14, 2025

Rename usage of has_permission() to account for a WP_Error #67

Closed

justlevine reviewed Sep 14, 2025

View reviewed changes

justlevine requested review from gziolo and johnbillion September 14, 2025 15:35

Ref34t requested a review from justlevine September 14, 2025 23:56

gziolo assigned Ref34t Sep 15, 2025

gziolo added the [Type] Bug Something isn't working label Sep 15, 2025

gziolo reviewed Sep 16, 2025

View reviewed changes

includes/abilities-api/class-wp-ability.php Outdated Show resolved Hide resolved

justlevine reviewed Sep 17, 2025

View reviewed changes

includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php Show resolved Hide resolved

gziolo requested changes Sep 23, 2025

View reviewed changes

Ref34t closed this Sep 26, 2025

Ref34t force-pushed the feature/fix-has-permission-return-type-issue-67 branch from 2f742bd to 4beab25 Compare September 26, 2025 12:29

Rename has_permission() to check_permission() with deprecation logic

c60032b

Ref34t reopened this Sep 26, 2025

Mohamed Khaled added 3 commits September 26, 2025 15:35

Update tests to use check_permission() and add deprecation test

0dba16e

Fix remaining has_permission test usage that was causing CI failure

209b541

Remove specific deprecation test that was causing CI failures

5aeab28

gziolo requested a review from Copilot September 26, 2025 12:40

Copilot AI reviewed Sep 26, 2025

View reviewed changes

includes/abilities-api/class-wp-ability.php Show resolved Hide resolved

includes/abilities-api/class-wp-ability.php Show resolved Hide resolved

includes/abilities-api/class-wp-ability.php Show resolved Hide resolved

Fix internal execute() method to use new check_permission() method

eb86cc7

Add test coverage for deprecated has_permission method

a44d683

gziolo reviewed Sep 26, 2025

View reviewed changes

tests/unit/abilities-api/wpRegisterAbility.php Outdated Show resolved Hide resolved

Mohamed Khaled added 2 commits September 26, 2025 16:00

Use WordPress test framework method for deprecated function coverage …

d032fae

…per maintainer feedback

Switch to @expectedDeprecated PHPDoc annotation for cleaner deprecati…

d55a0bb

…on test

gziolo approved these changes Sep 26, 2025

View reviewed changes

justlevine reviewed Sep 27, 2025

View reviewed changes

gziolo mentioned this pull request Sep 29, 2025

Prepare the branch for v0.2.0 release #90

Merged

Merge branch 'trunk' into feature/fix-has-permission-return-type-issu…

e8d93f3

…e-67

Merge branch 'trunk' into feature/fix-has-permission-return-type-issu…

027c447

…e-67

gziolo enabled auto-merge (squash) September 30, 2025 08:00

gziolo merged commit cbe8a59 into WordPress:trunk Sep 30, 2025
20 checks passed

Ref34t mentioned this pull request Sep 30, 2025

Fix documentation anti-patterns and REST API security vulnerability #93

Closed

3 tasks

gziolo mentioned this pull request Sep 30, 2025

Rename check_permission() to check_permissions() on WP_Ability #94

Merged

Ref34t mentioned this pull request Sep 30, 2025

docs: Improve permission checking documentation and examples #95

Open

3 tasks

	if ( ! $ability->check_permission( $input ) ) {
	if ( true !== $ability->check_permission( $input ) ) {

Fix has_permission() return type inconsistency (Issue #67) #76

Fix has_permission() return type inconsistency (Issue #67) #76

Uh oh!

Conversation

Ref34t commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem Solved

Implementation

Testing

Uh oh!

github-actions bot commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

justlevine commented Sep 14, 2025

Uh oh!

Ref34t commented Sep 14, 2025

Uh oh!

justlevine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ref34t commented Sep 14, 2025

Uh oh!

codecov bot commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gziolo commented Sep 15, 2025

Uh oh!

Uh oh!

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gziolo commented Sep 26, 2025

Uh oh!

Ref34t commented Sep 26, 2025

Uh oh!

gziolo commented Sep 26, 2025

Uh oh!

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

justlevine commented Sep 27, 2025

Uh oh!

justlevine left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justlevine Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justlevine Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Ref34t commented Sep 14, 2025 •

edited

Loading

github-actions bot commented Sep 14, 2025 •

edited

Loading

codecov bot commented Sep 14, 2025 •

edited

Loading

justlevine left a comment •

edited

Loading

justlevine Sep 27, 2025 •

edited

Loading

justlevine Sep 27, 2025 •

edited

Loading