Skip to content

Commit

Permalink
Merge pull request #1637 from WordPress/fix/od-with-page-caching
Browse files Browse the repository at this point in the history 
Revise the use of nonces in requests to store a URL Metric and block cross-origin requests
  • Loading branch information
@westonruter
westonruter authored Nov 13, 2024
2 parents b20b43f + 545f0c3 commit e99b5b2
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 83 deletions.
9 changes: 3 additions & 6 deletions plugins/optimization-detective/detect.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,10 +239,9 @@ function extendElementData( xpath, properties ) {
* @param {number} args.maxViewportAspectRatio Maximum aspect ratio allowed for the viewport.
* @param {boolean} args.isDebug Whether to show debug messages.
* @param {string} args.restApiEndpoint URL for where to send the detection data.
* @param {string} args.restApiNonce Nonce for writing to the REST API.
* @param {string} args.currentUrl Current URL.
* @param {string} args.urlMetricSlug Slug for URL Metric.
* @param {string} args.urlMetricNonce Nonce for URL Metric storage.
* @param {string} args.urlMetricHMAC HMAC for URL Metric storage.
* @param {URLMetricGroupStatus[]} args.urlMetricGroupStatuses URL Metric group statuses.
* @param {number} args.storageLockTTL The TTL (in seconds) for the URL Metric storage lock.
* @param {string} args.webVitalsLibrarySrc The URL for the web-vitals library.
Expand All @@ -256,10 +255,9 @@ export default async function detect( {
isDebug,
extensionModuleUrls,
restApiEndpoint,
restApiNonce,
currentUrl,
urlMetricSlug,
urlMetricNonce,
urlMetricHMAC,
urlMetricGroupStatuses,
storageLockTTL,
webVitalsLibrarySrc,
Expand Down Expand Up @@ -549,9 +547,8 @@ export default async function detect( {
}

const url = new URL( restApiEndpoint );
url.searchParams.set( '_wpnonce', restApiNonce );
url.searchParams.set( 'slug', urlMetricSlug );
url.searchParams.set( 'nonce', urlMetricNonce );
url.searchParams.set( 'hmac', urlMetricHMAC );
navigator.sendBeacon(
url,
new Blob( [ JSON.stringify( urlMetric ) ], {
Expand Down
3 changes: 1 addition & 2 deletions plugins/optimization-detective/detection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,9 @@ function od_get_detection_script( string $slug, OD_URL_Metric_Group_Collection $
'isDebug' => WP_DEBUG,
'extensionModuleUrls' => $extension_module_urls,
'restApiEndpoint' => rest_url( OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ),
'restApiNonce' => wp_create_nonce( 'wp_rest' ),
'currentUrl' => $current_url,
'urlMetricSlug' => $slug,
'urlMetricNonce' => od_get_url_metrics_storage_nonce( $slug, $current_url ),
'urlMetricHMAC' => od_get_url_metrics_storage_hmac( $slug, $current_url ),
'urlMetricGroupStatuses' => array_map(
static function ( OD_URL_Metric_Group $group ): array {
return array(
Expand Down
35 changes: 18 additions & 17 deletions plugins/optimization-detective/storage/data.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,42 +141,43 @@ function od_get_url_metrics_slug( array $query_vars ): string {
}

/**
* Computes nonce for storing URL Metrics for a specific slug.
* 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
* @access private
*
* @see wp_create_nonce()
* @see od_verify_url_metrics_storage_nonce()
* @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 Nonce.
*
* @return string HMAC.
*/
function od_get_url_metrics_storage_nonce( string $slug, string $url ): string {
return wp_create_nonce( "store_url_metrics:$slug:$url" );
function od_get_url_metrics_storage_hmac( string $slug, string $url ): string {
$action = "store_url_metric:$slug:$url";
return wp_hash( $action, 'nonce' );
}

/**
* Verifies nonce for storing URL Metrics for a specific slug.
* Verifies HMAC for storing URL Metrics for a specific slug.
*
* @since 0.1.0
* @since n.e.x.t
* @access private
*
* @see wp_verify_nonce()
* @see od_get_url_metrics_storage_nonce()
* @see od_get_url_metrics_storage_hmac()
* @see od_get_url_metrics_slug()
*
* @param string $nonce Nonce.
* @param string $slug Slug (hash of normalized query vars).
* @param String $url URL.
* @return bool Whether the nonce is valid.
* @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_nonce( string $nonce, string $slug, string $url ): bool {
return (bool) wp_verify_nonce( $nonce, "store_url_metrics:$slug:$url" );
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 );
}

/**
Expand Down
47 changes: 39 additions & 8 deletions plugins/optimization-detective/storage/rest-api.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,22 @@
function od_register_endpoint(): void {

$args = array(
'slug' => 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 nonce argument, as it is provided as input
// with the 'url' argument to create the nonce by the server. which then is verified to match in the REST API request.
// 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.
),
'nonce' => array(
'hmac' => array(
'type' => 'string',
'description' => __( 'Nonce originally computed by server required to authorize the request.', 'optimization-detective' ),
'description' => __( 'HMAC originally computed by server required to authorize the request.', 'optimization-detective' ),
'required' => true,
'pattern' => '^[0-9a-f]+$',
'validate_callback' => static function ( string $nonce, WP_REST_Request $request ) {
if ( ! od_verify_url_metrics_storage_nonce( $nonce, $request->get_param( 'slug' ), $request->get_param( 'url' ) ) ) {
return new WP_Error( 'invalid_nonce', __( 'URL Metrics nonce verification failure.', 'optimization-detective' ) );
'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;
},
Expand Down Expand Up @@ -88,6 +88,27 @@ 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 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 {
// Strip out the port number since core does not account for it yet as noted in get_allowed_http_origins().
$origin = preg_replace( '/:\d+$/', '', $origin );
return '' !== is_allowed_http_origin( $origin );
}

/**
* Handles REST API request to store metrics.
*
Expand All @@ -100,6 +121,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(
Expand Down
50 changes: 9 additions & 41 deletions plugins/optimization-detective/tests/storage/test-data.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,49 +287,17 @@ public function test_od_get_url_metrics_slug(): void {
}

/**
* Test od_get_url_metrics_storage_nonce().
* Test od_get_url_metrics_storage_hmac() and od_verify_url_metrics_storage_hmac().
*
* @covers ::od_get_url_metrics_storage_nonce
* @covers ::od_verify_url_metrics_storage_nonce
* @covers ::od_get_url_metrics_storage_hmac
* @covers ::od_verify_url_metrics_storage_hmac
*/
public function test_od_get_url_metrics_storage_nonce_and_od_verify_url_metrics_storage_nonce(): void {
$user_id = self::factory()->user->create();

$nonce_life_actions = array();
add_filter(
'nonce_life',
static function ( int $life, string $action ) use ( &$nonce_life_actions ): int {
$nonce_life_actions[] = $action;
return $life;
},
10,
2
);

// Create first nonce for unauthenticated user.
$url = home_url( '/' );
$slug = od_get_url_metrics_slug( array() );
$nonce1 = od_get_url_metrics_storage_nonce( $slug, $url );
$this->assertMatchesRegularExpression( '/^[0-9a-f]{10}$/', $nonce1 );
$this->assertTrue( od_verify_url_metrics_storage_nonce( $nonce1, $slug, $url ) );
$this->assertCount( 2, $nonce_life_actions );

// Create second nonce for unauthenticated user.
$nonce2 = od_get_url_metrics_storage_nonce( $slug, $url );
$this->assertSame( $nonce1, $nonce2 );
$this->assertCount( 3, $nonce_life_actions );

// Create third nonce, this time for authenticated user.
wp_set_current_user( $user_id );
$nonce3 = od_get_url_metrics_storage_nonce( $slug, $url );
$this->assertNotEquals( $nonce3, $nonce2 );
$this->assertFalse( od_verify_url_metrics_storage_nonce( $nonce1, $slug, $url ) );
$this->assertTrue( od_verify_url_metrics_storage_nonce( $nonce3, $slug, $url ) );
$this->assertCount( 6, $nonce_life_actions );

foreach ( $nonce_life_actions as $nonce_life_action ) {
$this->assertSame( "store_url_metrics:{$slug}:{$url}", $nonce_life_action );
}
public function test_od_get_url_metrics_storage_hmac_and_od_verify_url_metrics_storage_hmac(): void {
$url = home_url( '/' );
$slug = od_get_url_metrics_slug( array() );
$hmac = od_get_url_metrics_storage_hmac( $slug, $url );
$this->assertMatchesRegularExpression( '/^[0-9a-f]+$/', $hmac );
$this->assertTrue( od_verify_url_metrics_storage_hmac( $hmac, $slug, $url ) );
}

/**
Expand Down
72 changes: 63 additions & 9 deletions plugins/optimization-detective/tests/storage/test-rest-api.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function ( OD_URL_Metric_Store_Request_Context $context ): void {
$this->assertSame( $valid_params['viewport']['width'], $url_metrics[0]->get_viewport_width() );

$expected_data = $valid_params;
unset( $expected_data['nonce'], $expected_data['slug'] );
unset( $expected_data['hmac'], $expected_data['slug'] );
$this->assertSame(
$expected_data,
wp_array_slice_assoc( $url_metrics[0]->jsonSerialize(), array_keys( $expected_data ) )
Expand Down Expand Up @@ -122,11 +122,11 @@ function ( $params ) {
'bad_slug' => array(
'slug' => '<script>document.write("evil")</script>',
),
'bad_nonce' => array(
'nonce' => 'not even a hash',
'bad_hmac' => array(
'hmac' => 'not even a hash',
),
'invalid_nonce' => array(
'nonce' => od_get_url_metrics_storage_nonce( od_get_url_metrics_slug( array( 'different' => 'query vars' ) ), home_url( '/' ) ),
'invalid_hmac' => array(
'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array( 'different' => 'query vars' ) ), home_url( '/' ) ),
),
'invalid_viewport_type' => array(
'viewport' => '640x480',
Expand Down Expand Up @@ -240,6 +240,58 @@ public function test_rest_request_bad_params( array $params ): void {
$this->assertSame( 0, did_action( 'od_url_metric_stored' ) );
}

/**
* Test sending data when no Origin request header is sent.
*
* @covers ::od_register_endpoint
* @covers ::od_handle_rest_request
* @covers ::od_is_allowed_http_origin
*/
public function test_rest_request_without_origin(): void {
$request = new WP_REST_Request( 'POST', self::ROUTE );
$request->set_body_params( $this->get_valid_params() ); // Valid and yet set as POST params and not as JSON body, so this is why it fails.
$response = rest_get_server()->dispatch( $request );
$this->assertSame( 403, $response->get_status(), 'Response: ' . wp_json_encode( $response ) );
$this->assertSame( 'rest_cross_origin_forbidden', $response->get_data()['code'], 'Response: ' . wp_json_encode( $response ) );
$this->assertSame( 0, did_action( 'od_url_metric_stored' ) );
}

/**
* Test sending data when a cross-domain Origin request header is sent.
*
* @covers ::od_register_endpoint
* @covers ::od_handle_rest_request
* @covers ::od_is_allowed_http_origin
*/
public function test_rest_request_cross_origin(): void {
$request = new WP_REST_Request( 'POST', self::ROUTE );
$request->set_header( 'Origin', 'https://cross-origin.example.com' );
$request->set_body_params( $this->get_valid_params() ); // Valid and yet set as POST params and not as JSON body, so this is why it fails.
$response = rest_get_server()->dispatch( $request );
$this->assertSame( 403, $response->get_status(), 'Response: ' . wp_json_encode( $response ) );
$this->assertSame( 'rest_cross_origin_forbidden', $response->get_data()['code'], 'Response: ' . wp_json_encode( $response ) );
$this->assertSame( 0, did_action( 'od_url_metric_stored' ) );
}

/**
* Test REST API request when 'home_url' is filtered.
*
* @covers ::od_register_endpoint
* @covers ::od_handle_rest_request
* @covers ::od_is_allowed_http_origin
*/
public function test_rest_request_origin_when_home_url_filtered(): void {
$request = $this->create_request( $this->get_valid_params() );
add_filter(
'home_url',
static function ( string $url ): string {
return trailingslashit( $url ) . 'home/en/?foo=bar#baz';
}
);
$response = rest_get_server()->dispatch( $request );
$this->assertSame( 200, $response->get_status() );
}

/**
* Test not sending JSON data.
*
Expand All @@ -248,6 +300,7 @@ public function test_rest_request_bad_params( array $params ): void {
*/
public function test_rest_request_not_json_data(): void {
$request = new WP_REST_Request( 'POST', self::ROUTE );
$request->set_header( 'Origin', home_url() );
$request->set_body_params( $this->get_valid_params() ); // Valid and yet set as POST params and not as JSON body, so this is why it fails.
$response = rest_get_server()->dispatch( $request );
$this->assertSame( 400, $response->get_status(), 'Response: ' . wp_json_encode( $response ) );
Expand Down Expand Up @@ -530,8 +583,8 @@ private function get_valid_params( array $extras = array() ): array {
unset( $data['timestamp'], $data['uuid'] ); // Since these are readonly.
$data = array_merge(
array(
'slug' => $slug,
'nonce' => od_get_url_metrics_storage_nonce( $slug, $data['url'] ),
'slug' => $slug,
'hmac' => od_get_url_metrics_storage_hmac( $slug, $data['url'] ),
),
$data
);
Expand Down Expand Up @@ -579,8 +632,9 @@ private function create_request( array $params ): WP_REST_Request {
*/
$request = new WP_REST_Request( 'POST', self::ROUTE );
$request->set_header( 'Content-Type', 'application/json' );
$request->set_query_params( wp_array_slice_assoc( $params, array( 'nonce', 'slug' ) ) );
unset( $params['nonce'], $params['slug'] );
$request->set_query_params( wp_array_slice_assoc( $params, array( 'hmac', 'slug' ) ) );
$request->set_header( 'Origin', home_url() );
unset( $params['hmac'], $params['slug'] );
$request->set_body( wp_json_encode( $params ) );
return $request;
}
Expand Down

0 comments on commit e99b5b2

Please sign in to comment.