Abilities: Strip internal schema keywords from abilities REST responses#11451
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 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. |
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. |
|
Client side clean up at WordPress/gutenberg#77029. |
There was a problem hiding this comment.
Pull request overview
Updates the Abilities REST API responses to remove WordPress-internal (non–JSON Schema) keywords from input_schema/output_schema so that client-side JSON Schema validators don’t fail.
Changes:
- Add recursive schema-keyword stripping in
WP_REST_Abilities_V1_List_Controller::prepare_item_for_response(). - Add PHPUnit coverage to ensure internal schema keywords are removed from ability REST responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/wp-includes/rest-api/endpoints/class-wp-rest-abilities-v1-list-controller.php |
Adds recursive filtering of schema arrays before returning them in REST responses. |
tests/phpunit/tests/rest-api/wpRestAbilitiesV1ListController.php |
Adds a test ensuring internal schema keywords are stripped from input_schema/output_schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private function strip_internal_schema_keywords( array $schema ): array { | ||
| $allowed_keywords = rest_get_allowed_schema_keywords(); | ||
| $allowed_keywords[] = 'required'; | ||
| $allowed_keywords = array_flip( $allowed_keywords ); | ||
|
|
||
| $schema = array_intersect_key( $schema, $allowed_keywords ); | ||
|
|
There was a problem hiding this comment.
strip_internal_schema_keywords() currently filters schemas using rest_get_allowed_schema_keywords(), which is a WordPress-supported subset and does not include several standard JSON Schema draft-04 keywords (e.g. $schema, $ref, allOf, not, definitions). As a result, any ability schema that legitimately uses those keywords will have them removed from REST responses, changing the meaning of the schema and potentially breaking existing consumers. If the intent is only to remove WordPress-internal keys like sanitize_callback/validate_callback/arg_options, consider switching to a targeted denylist for those internal keys, or expanding the allowlist to cover the JSON Schema keywords you want to preserve (and ensuring recursion covers those keywords too).
| private function strip_internal_schema_keywords( array $schema ): array { | ||
| $allowed_keywords = rest_get_allowed_schema_keywords(); | ||
| $allowed_keywords[] = 'required'; | ||
| $allowed_keywords = array_flip( $allowed_keywords ); | ||
|
|
||
| $schema = array_intersect_key( $schema, $allowed_keywords ); | ||
|
|
||
| // Sub-schema maps: keys are user-defined, values are sub-schemas. | ||
| foreach ( array( 'properties', 'patternProperties' ) as $keyword ) { | ||
| if ( isset( $schema[ $keyword ] ) && is_array( $schema[ $keyword ] ) ) { | ||
| foreach ( $schema[ $keyword ] as $key => $child_schema ) { | ||
| if ( is_array( $child_schema ) ) { | ||
| $schema[ $keyword ][ $key ] = $this->strip_internal_schema_keywords( $child_schema ); | ||
| } |
There was a problem hiding this comment.
strip_internal_schema_keywords() rebuilds and flips the allowed-keywords array on every recursive call. For deep or repetitive schemas this adds avoidable overhead. Consider computing the flipped allowlist once (e.g., in the top-level call or as a static/private property) and passing it into the recursive calls to keep the per-node work to just filtering + traversal.
| $data = $response->get_data(); | ||
|
|
There was a problem hiding this comment.
The test does not assert that the REST request succeeded (e.g. status 200) before indexing into $data['input_schema']['properties']['content']. If the endpoint returns an error (or an unexpected payload), this can produce undefined-index notices that obscure the real failure. Add an explicit status assertion (and/or assertArrayHasKey checks) before accessing nested keys.
| $data = $response->get_data(); | |
| $this->assertEquals( 200, $response->get_status() ); | |
| $data = $response->get_data(); | |
| $this->assertIsArray( $data ); | |
| $this->assertArrayHasKey( 'input_schema', $data ); | |
| $this->assertIsArray( $data['input_schema'] ); | |
| $this->assertArrayHasKey( 'properties', $data['input_schema'] ); | |
| $this->assertIsArray( $data['input_schema']['properties'] ); | |
| $this->assertArrayHasKey( 'content', $data['input_schema']['properties'] ); | |
| $this->assertIsArray( $data['input_schema']['properties']['content'] ); | |
| $this->assertArrayHasKey( 'output_schema', $data ); | |
| $this->assertIsArray( $data['output_schema'] ); |
8871bf0 to
34a0291
Compare
There was a problem hiding this comment.
The fix is correct and well-tested. One clarification and a note for future work.
The description says this uses "the same approach (rest_get_allowed_schema_keywords() + array_intersect_key()) that WP_REST_Server::get_data_for_route() already uses." That's not quite right — get_data_for_route() uses an allowlist (array_intersect_key keeping only known-good keywords), while this PR uses a denylist (array_diff_key removing 3 specific internal keywords). The denylist is the correct choice here because rest_get_allowed_schema_keywords() is missing valid JSON Schema keywords (allOf, not, definitions, dependencies, additionalItems) that would get silently stripped with the allowlist approach. Worth updating the description to avoid confusion for future readers.
Looking ahead, the recursive internal-keyword stripping logic is generic and not abilities-specific. In a future release, it could be extracted into a shared utility, alongside updating the set of allowed schema keywords to cover the full JSON Schema spec so an allowlist approach becomes viable. That's out of scope for an RC fix though, and it's being tracked more broadly in Abilities API: Add schema compiler for AI tool calling compatibility.
Implementation correctly traverses all JSON Schema sub-schema locations and tests cover both flat and deeply nested schemas across all keyword types.
Ability `input_schema` and `output_schema` may contain WordPress-internal properties like `sanitize_callback`, `validate_callback`, and `arg_options` that are not valid JSON Schema keywords. These cause client-side JSON Schema validators to fail. Strip non-standard keywords recursively using the same allowlist approach (`rest_get_allowed_schema_keywords()`) that `WP_REST_Server::get_data_for_route()` already uses for endpoint arguments.
…eywords. Use a targeted denylist of the three known WordPress-internal keys (sanitize_callback, validate_callback, arg_options) instead of an allowlist based on rest_get_allowed_schema_keywords(). The allowlist approach was too aggressive — it stripped legitimate JSON Schema keywords like $schema, $ref, allOf, not, and definitions that are valid but not in WordPress's supported subset.
The blind recursion into all array values would incorrectly strip denylist keys from data-holding keywords like `default` and `enum`. Restrict recursion to known sub-schema locations: properties, patternProperties, definitions, items, additionalProperties, additionalItems, not, anyOf, oneOf, allOf.
…ing test coverage. Defines INTERNAL_SCHEMA_KEYWORDS as an associative array so array_flip() is no longer called on every recursive invocation. Adds test coverage for additionalItems, definitions, and tuple-style items sub-schema locations.
… stripping. Add wp_is_numeric_array() guard to the dependencies handler so property-dependency arrays (numeric arrays of strings) are explicitly skipped rather than relying on array_diff_key being a no-op.
5291a90 to
e89f03c
Compare
src/wp-includes/rest-api/endpoints/class-wp-rest-abilities-v1-list-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-abilities-v1-list-controller.php
Outdated
Show resolved
Hide resolved
…list-controller.php Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
Track ticket: https://core.trac.wordpress.org/ticket/65035
Ability
input_schemaandoutput_schemamay contain WordPress-internal properties likesanitize_callback,validate_callback, andarg_optionsthat are not valid JSON Schema keywords. These cause client-side JSON Schema validators to fail.This adds recursive stripping of internal keywords in
prepare_item_for_response(), using a denylist approach (array_diff_keyremoving specific internal keywords defined inWP_REST_Abilities_Controller::INTERNAL_SCHEMA_KEYWORDS). This differs from the allowlist approach (rest_get_allowed_schema_keywords()+array_intersect_key()) used inWP_REST_Server::get_data_for_route()— the denylist is the correct choice here becauserest_get_allowed_schema_keywords()is missing valid JSON Schema keywords (allOf,not,definitions,dependencies,additionalItems) that would get silently stripped with the allowlist approach.Related Gutenberg PR: WordPress/gutenberg#77029