Revise the use of nonces in requests to store a URL Metric and block cross-origin requests #1637
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,42 +141,45 @@ function od_get_url_metrics_slug( array $query_vars ): string { | |
| } | ||
|
|
||
| /** | ||
| * Computes HMAC for storing URL Metrics for a specific slug. | ||
| * | ||
| * This is used in the REST API to authenticate the storage of new URL Metrics from a given URL. | ||
| * | ||
| * @since 0.1.0 | ||
| * @since n.e.x.t Renamed from od_get_url_metrics_storage_nonce(). | ||
| * @access private | ||
| * | ||
| * @see od_verify_url_metrics_storage_hmac() | ||
| * @see od_get_url_metrics_slug() | ||
| * | ||
| * @param string $slug Slug (hash of normalized query vars). | ||
| * @param string $url URL. | ||
| * | ||
| * @return string HMAC. | ||
| */ | ||
| function od_get_url_metrics_storage_hmac( string $slug, string $url ): string { | ||
| $action = "store_url_metric:$slug:$url"; | ||
| return wp_hash( $action, 'nonce' ); | ||
| } | ||
|
|
||
| /** | ||
| * Verifies HMAC for storing URL Metrics for a specific slug. | ||
| * | ||
| * @since 0.1.0 | ||
| * @since n.e.x.t Renamed from od_verify_url_metrics_storage_nonce(). | ||
|
There was a problem hiding this comment. See above, I'd prefer we consider this a new function. There was a problem hiding this comment. Sure. Done in 9d6e807 |
||
| * @access private | ||
| * | ||
| * @see od_get_url_metrics_storage_hmac() | ||
| * @see od_get_url_metrics_slug() | ||
| * | ||
| * @param string $hmac HMAC. | ||
| * @param string $slug Slug (hash of normalized query vars). | ||
| * @param String $url URL. | ||
| * | ||
| * @return bool Whether the HMAC is valid. | ||
| */ | ||
| function od_verify_url_metrics_storage_hmac( string $hmac, string $slug, string $url ): bool { | ||
| return hash_equals( od_get_url_metrics_storage_hmac( $slug, $url ), $hmac ); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,22 +38,22 @@ | |
| function od_register_endpoint(): void { | ||
|
|
||
| $args = array( | ||
| 'slug' => array( | ||
| 'type' => 'string', | ||
| 'description' => __( 'An MD5 hash of the query args.', 'optimization-detective' ), | ||
| 'required' => true, | ||
| 'pattern' => '^[0-9a-f]{32}$', | ||
| // This is further validated via the validate_callback for the 'hmac' parameter, as it is provided as input | ||
| // with the 'url' argument to create the HMAC by the server. which then is verified to match in the REST API request. | ||
| ), | ||
| 'hmac' => array( | ||
| 'type' => 'string', | ||
| 'description' => __( 'HMAC originally computed by server required to authorize the request.', 'optimization-detective' ), | ||
| 'required' => true, | ||
| 'pattern' => '^[0-9a-f]+$', | ||
| 'validate_callback' => static function ( string $hmac, WP_REST_Request $request ) { | ||
| if ( ! od_verify_url_metrics_storage_hmac( $hmac, $request->get_param( 'slug' ), $request->get_param( 'url' ) ) ) { | ||
| return new WP_Error( 'invalid_hmac', __( 'URL Metrics HMAC verification failure.', 'optimization-detective' ) ); | ||
| } | ||
| return true; | ||
| }, | ||
|
|
@@ -88,6 +88,42 @@ function od_register_endpoint(): void { | |
| } | ||
| add_action( 'rest_api_init', 'od_register_endpoint' ); | ||
|
|
||
| /** | ||
| * Determines if the HTTP origin is an authorized one. | ||
| * | ||
| * Note that `is_allowed_http_origin()` is not used directly because the underlying `get_allowed_http_origins()` does | ||
| * not account for the URL port (although there is a to-do comment committed in core to address this). Additionally, | ||
| * the `is_allowed_http_origin()` function in core for some reason returns a string rather than a boolean. | ||
| * | ||
| * @since n.e.x.t | ||
| * @access private | ||
| * | ||
| * @see get_allowed_http_origins() | ||
| * @see is_allowed_http_origin() | ||
| * | ||
| * @param string $origin Origin to check. | ||
| * @return bool Whether the origin is allowed. | ||
| */ | ||
| function od_is_allowed_http_origin( string $origin ): bool { | ||
|
There was a problem hiding this comment. Why can't we use If it's just about the port support, I'd say that's an edge case so then I think we should at least use the original function anytime there's no port, and the custom logic only if there is one. I'd also say we could simplify it: If there is a port in the passed There was a problem hiding this comment. Well, unit tests run the home being I've gone ahead and just stripped out the port number in 545f0c3. There was a problem hiding this comment. Great, I think that simplifies the implementation, and validating the same port number is probably not super important (for |
||
| $allowed_origins = get_allowed_http_origins(); | ||
| $home_url_port = wp_parse_url( home_url(), PHP_URL_PORT ); | ||
|
|
||
| // Append the home URL's port to the allowed origins if they lack a port number. | ||
| if ( is_int( $home_url_port ) ) { | ||
| $allowed_origins = array_map( | ||
| static function ( string $allowed_origin ) use ( $home_url_port ): string { | ||
| if ( null === wp_parse_url( $allowed_origin, PHP_URL_PORT ) ) { | ||
| $allowed_origin .= ':' . (string) $home_url_port; | ||
| } | ||
| return $allowed_origin; | ||
| }, | ||
| $allowed_origins | ||
| ); | ||
| } | ||
|
|
||
| return in_array( $origin, $allowed_origins, true ); | ||
| } | ||
|
|
||
| /** | ||
| * Handles REST API request to store metrics. | ||
| * | ||
|
|
@@ -100,6 +136,16 @@ function od_register_endpoint(): void { | |
| * @return WP_REST_Response|WP_Error Response. | ||
| */ | ||
| function od_handle_rest_request( WP_REST_Request $request ) { | ||
| // Block cross-origin storage requests since by definition URL Metrics data can only be sourced from the frontend of the site. | ||
| $origin = $request->get_header( 'origin' ); | ||
| if ( null === $origin || ! od_is_allowed_http_origin( $origin ) ) { | ||
| return new WP_Error( | ||
| 'rest_cross_origin_forbidden', | ||
| __( 'Cross-origin requests are not allowed for this endpoint.', 'optimization-detective' ), | ||
| array( 'status' => 403 ) | ||
| ); | ||
| } | ||
|
|
||
| $post = OD_URL_Metrics_Post_Type::get_post( $request->get_param( 'slug' ) ); | ||
|
|
||
| $url_metric_group_collection = new OD_URL_Metric_Group_Collection( | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Better to consider this an entirely new function. I would argue it wasn't "just" renamed, the purpose was also changed as it now returns a different kind of value.
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.
Sure. Done in 9d6e807