Make WordPress Core

Opened 10 months ago

Closed 4 months ago

Last modified 2 weeks ago

#64311 closed enhancement (fixed)

Add filters for input and output validation

Reported by: priethor Owned by: priethor
Priority: normal Milestone: 7.1
Component: Abilities API Version: 6.9
Severity: normal Keywords: has-patch has-unit-tests has-dev-note
Cc: Focuses:

Description

The Abilities API currently validates ability input and output against JSON Schema using WordPress's built-in rest_validate_value_from_schema(), which supports only a subset of JSON Schema Draft 4 (aligning with WordPress core). This approach is reliable for core compatibility but limits extenders to dated JSON Schema features, missing newer ones like $ref references for composability and reusability, and not keyword to exclude patterns.

Developers extending the Abilities API that need more expressive schema validation have no way to override the default validator without forking or monkey-patching.

Proposed Solution

Introduce two hooks to allow custom validation:

/**
 * Filters the input validation result for an ability.
 *
 * @since 7.0.0
 *
 * @param true|WP_Error $is_valid Validation result (true or WP_Error).
 * @param mixed         $input    The input being validated.
 * @param string        $name     The ability name.
 */
apply_filters( 'wp_ability_validate_input', $is_valid, $input, $name );

/**
 * Filters the output validation result for an ability.
 *
 * @since 7.0.0
 *
 * @param true|WP_Error $is_valid Validation result (true or WP_Error).
 * @param mixed         $output   The output being validated.
 * @param string        $name     The ability name.
 */
apply_filters( 'wp_ability_validate_output', $is_valid, $output, $name );

Change History (26)

This ticket was mentioned in PR #10557 on WordPress/wordpress-develop by @priethor.


10 months ago
#1

  • Keywords has-unit-tests added

#2 @priethor
10 months ago

  • Keywords has-unit-tests removed
  • Summary Abilities API: add filters for input and ouput validationAbilities API: add filters for input and output validation

#3 @priethor
10 months ago

  • Keywords has-unit-tests added

#4 @juanmaguitar
7 months ago

  • Milestone 7.0Future Release

Because of the lack of activity in the last 3 months and due to the Beta1 freeze happening in 24hrs I'm punting this to "Future Release"

#5 @gziolo
6 months ago

  • Milestone Future Release7.1

Worth noting that similar extensibility was explored earlier in https://github.com/WordPress/abilities-api/pull/37, which proposed ability_input_schema and ability_output_schema filters. However, the filters proposed here are more powerful — they operate on the validation result rather than the schema itself. Schema-level filtering can already happen at registration time, whereas these hooks give developers control over the validation logic, which is the actual constraint point for supporting newer JSON Schema features.

That same PR also proposed ability_permission_result and ability_execute_result filters. It might be worth considering whether those should be part of the parallel effort as well, to provide a complete set of extensibility points across the ability lifecycle.

I'd be happy to see this proposal included in WordPress 7.1.

#6 @JeffPaul
6 months ago

  • Keywords abilities added

@westonruter commented on PR #10557:


6 months ago
#7

One thing to address: the add_filter() calls in the new tests use anonymous closures and are never cleaned up with remove_filter(). Since PHPUnit runs tests in the same process, filters added in one test can leak into subsequent tests and silently affect results. Each test should store the closure reference and remove it after assertions, e.g.:

@gziolo Actually, this isn't a concern because the hooks get reset after each test is run, regardless of whether the tests run in a separate processor not.

In set_up:

https://github.com/WordPress/wordpress-develop/blob/4d3b0b9ab132dfcf83cfb66f2939eba176b2584d/tests/phpunit/includes/abstract-testcase.php#L117-L119

In tear_down:

https://github.com/WordPress/wordpress-develop/blob/4d3b0b9ab132dfcf83cfb66f2939eba176b2584d/tests/phpunit/includes/abstract-testcase.php#L228

@gziolo commented on PR #10557:


6 months ago
#8

@westonruter, thank you so much for pointing me to that logic. That’s perfect. I’m glad it existed as this is the very well designed default behavior 👍

I intend to land this PR as soon as 7.1 cycle starts.

#9 @gziolo
6 months ago

I proactively followed up with #64989 to expand filtering in other aspects of the execution lifecycle for individual abilities.

#10 @desrosj
5 months ago

  • Component AIAbilities API

Moving tickets related to the Abilities API to a new sub-component.

#11 @desrosj
4 months ago

  • Keywords abilities removed

Removing abilities and abilities-api custom keywords. This is now indicated by the Abilities API component.

#12 @desrosj
4 months ago

  • Keywords has-patch, has-unit-tests → has-patch has-unit-tests
  • Summary Abilities API: add filters for input and output validationAdd filters for input and output validation

@gziolo commented on PR #10557:


4 months ago
#13

Merged the latest changes from trunk and resolved the conflicts. Since trunk introduced the new execution lifecycle filters (wp_ability_normalize_input, wp_ability_permission_result, wp_pre_execute_ability, wp_ability_execute_result), the only real conflict was in the test file — resolved by keeping both the upstream pipeline tests and this PR's input/output validation filter tests.

Also addressed the review feedback:

  • Corrected the @since tags for wp_ability_validate_input and wp_ability_validate_output to 7.1.0.
  • Refactored validate_input() and validate_output() to return early instead of reassigning $validity, while preserving the hardening that coerces a false filter return into a WP_Error.

Validation passing locally: PHPCS, PHP compatibility, PHPStan, and the abilities-api tests (57 tests, 89 assertions).

@gziolo commented on PR #10557:


4 months ago
#14

Note for a follow-up: as decided in https://github.com/WordPress/wordpress-develop/pull/10557#discussion_r2575710801, esc_html() was removed from $this->name in the input/output validation error messages (the entities would be wrong when rendered into JSON). The same reasoning applies to the other error messages in WP_Ability that still wrap $this->name in esc_html() (ability_invalid_permission_callback, ability_invalid_execute_callback, ability_callback_exception, and the permission denial in execute()). Since those messages predate this PR, it'd make sense to address them in a separate follow-up commit rather than expand this PR's scope.

cc @westonruter

#15 @gziolo
4 months ago

  • Resolutionfixed
  • Status assignedclosed

In 62398:

Abilities API: Add filters for input and output validation

Introduce the wp_ability_validate_input and wp_ability_validate_output filters so developers can layer custom validation on top of the default JSON
Schema checks, either augmenting an existing WP_Error or rejecting otherwise valid data.

Props priethor, gziolo, westonruter, enej.
Fixes #64311.

#16 @gziolo
4 months ago

  • Keywords needs-dev-note added

This ticket was mentioned in PR #11917 on WordPress/wordpress-develop by @gziolo.


4 months ago
#17

Follow-up to https://github.com/WordPress/wordpress-develop/pull/10557.

Preserves custom REST response statuses returned by wp_ability_validate_input filter errors. The REST run controller already preserves custom statuses from wp_ability_normalize_input; this applies the same behavior to validation errors by only defaulting to 400 when the WP_Error does not already include a status value.

Testing:

  • npm run test:php -- --filter Tests_REST_API_WpRestAbilitiesV1RunController
  • npm run test:php -- --group abilities-api

This ticket was mentioned in PR #11918 on WordPress/wordpress-develop by @gziolo.


4 months ago
#18

## Description

WP_Ability renders the ability name into several WP_Error messages that are commonly serialized to JSON (e.g. through the REST API), where HTML entities produced by esc_html() would be incorrect. This removes esc_html() from the ability name in those messages:

  • ability_callback_exception
  • ability_invalid_permission_callback
  • ability_invalid_execute_callback
  • ability_invalid_permissions

This aligns them with validate_input() / validate_output(), which already pass $this->name unescaped (see the decision in https://github.com/WordPress/wordpress-develop/pull/10557#discussion_r2575710801).

### Why this is safe

Ability names are pattern-validated at registration to ^[a-z0-9-]+\/[a-z0-9-]+$, so they can never contain HTML special characters — esc_html() on the name is a no-op in every case. The change is purely about correctness/consistency for JSON-bound messages.

### What is intentionally left unchanged

  • The _doing_it_wrong() calls render into HTML admin output, so they keep esc_html() (including <code>-wrapped values and the arbitrary $property_name).
  • The arbitrary exception text ($e->getMessage()) and permission error text ($has_permissions->get_error_message()) keep esc_html(), since those can contain arbitrary content.

## Testing

  • tests/phpunit/tests/abilities-api/wpAbility.php passes (57 tests, 89 assertions).
  • PHPCS, PHP compatibility, and PHPStan all pass.

---

Trac ticket: https://core.trac.wordpress.org/ticket/64311

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request.

#19 @gziolo
4 months ago

In 62399:

EST API: Preserve custom status for ability validation errors

he REST run controller for the Abilities API already preserves a custom
HTTP status returned by wp_ability_normalize_input filter errors. Apply
the same behavior to validation errors from wp_ability_validate_input:
a WP_Error is only defaulted to a 400 status when it does not already
include one.

The shared defaulting logic is extracted into a new private
ensure_error_status() helper and reused for both normalization and
validation errors.

Follow-up to [62398].
See #64311.

@gziolo commented on PR #11917:


4 months ago
#20

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 62397
GitHub commit: https://github.com/WordPress/wordpress-develop/commit/1d51ae58054360b749e8f5714d0071e2c2a6318d

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

#22 @gziolo
4 months ago

In 62401:

Abilities API: Stop HTML-escaping the ability name in WP_Error messages

Ability names are pattern-validated at registration, so esc_html() on them is always a no-op. Drop it from the WP_Error messages, which are commonly serialized to JSON where the escaping would be incorrect.

Follow-up to [62398].
See #64311.

@gziolo commented on PR #11918:


4 months ago
#23

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 62401
GitHub commit: https://github.com/WordPress/wordpress-develop/commit/4ddc905a5ea4be6971ef7d8a4fed227f3b0a2635

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

This ticket was mentioned in Slack in #core-ai by gziolo. View the logs.


4 months ago

#25 @milana_cap
6 weeks ago

  • Keywords has-dev-note added; needs-dev-note removed

#26 @gziolo
2 weeks ago

In 63391:

Abilities API: Add missing @since 7.1.0 changelog entries

Document the 7.1 changes to existing docblocks that had no changelog line: the public meta argument, the wp_ability_validate_input and wp_ability_validate_output filters, exception handling in invoke_callback(), and the new schema property and collection parameters in the REST list controller.

Follow-up to [62238], [62398], [62420], [62548], [62729], [62737].

Props khokansardar.
See #65058, #64311, #64990, #65568.

Note: See TracTickets for help on using tickets.