Plugin Directory: Validate the required parameter for the bulk stats update endpoint#582
Conversation
…update endpoint. The endpoint didn't define any argument requirements, so requests missing the expected data would reach the callback and trigger a PHP warning when iterating over a null value. Adds a schema definition that validates the shape and types of the incoming data at the REST API layer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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. |
|
@dd32 Could I ask for a sanity check for these changes? It doesn't look like the two places calling this endpoint should be bothered by getting an error returned when This fixes: |
There was a problem hiding this comment.
Pull request overview
Adds REST API argument/schema validation to the internal Plugin Directory bulk stats update endpoint to ensure requests missing required data are rejected before reaching the callback (preventing warnings from iterating null).
Changes:
- Define
argsschema for/plugins/v1/update-stats, makingpluginsrequired. - Validate top-level and per-plugin stat payload types at the REST layer (object + typed known fields).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'active_installs' => array( 'type' => 'integer' ), | ||
| 'usage' => array( 'type' => 'object' ), |
There was a problem hiding this comment.
The usage field is only validated as an object, but sanitize_usage_numbers() assumes the values are numeric counts (it uses array_sum() and division). As-is, a request with non-numeric values under usage can still trigger warnings/TypeErrors inside the callback even though the REST layer is now validating. Consider tightening the schema for usage to validate the value types (e.g., constrain additionalProperties under usage to integers/numbers).
| 'usage' => array( 'type' => 'object' ), | |
| 'usage' => array( | |
| 'type' => 'object', | |
| 'additionalProperties' => array( | |
| 'type' => 'integer', | |
| ), | |
| ), |
| register_rest_route( 'plugins/v1', '/update-stats', array( | ||
| 'methods' => \WP_REST_Server::CREATABLE, | ||
| 'callback' => array( $this, 'bulk_update_stats' ), | ||
| 'permission_callback' => array( $this, 'permission_check_internal_api_bearer' ), | ||
| 'args' => array( | ||
| 'plugins' => array( | ||
| 'required' => true, | ||
| 'type' => 'object', |
There was a problem hiding this comment.
This change introduces REST-layer validation (required plugins param and schema), but there doesn’t appear to be a corresponding automated test to lock in the new behavior (e.g., missing plugins should return a REST validation error instead of reaching the callback). Since the repo already has API tests (including for internal-bearer endpoints), adding a test for /plugins/v1/update-stats would help prevent regressions.
|
@obenland I'm thinking this validation isn't needed, because the sender should only be sending valid data; Something is broken if it's sending invalid data.. Pentesters aren't responsible here surely, as they won't pass the permission callbacks to reach that far. I was going to update the error handler to understand rest-api requests and include a subset of the payload to diagnose this warning. |
|
Sounds good! |
Summary
/plugins/v1/update-statsendpoint didn't define any argument requirements, so requests missing thepluginsparameter would reach the callback and trigger a PHP warning when iterating over a null value.Test plan
pluginsobject still work as expectedpluginsreturn a REST API validation error instead of triggering a PHP warning🤖 Generated with Claude Code