Harden ETag regex to disregard newlines

WordPress · Dec 1, 2024 · 4456563 · 4456563
1 parent 7b01e50
commit 4456563
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 20 deletions.
diff --git a/plugins/optimization-detective/class-od-url-metric-group-collection.php b/plugins/optimization-detective/class-od-url-metric-group-collection.php
@@ -109,7 +109,7 @@ final class OD_URL_Metric_Group_Collection implements Countable, IteratorAggrega
 	 */
 	public function __construct( array $url_metrics, string $current_etag, array $breakpoints, int $sample_size, int $freshness_ttl ) {
 		// Set current ETag.
-		if ( 1 !== preg_match( '/^[a-f0-9]{32}$/', $current_etag ) ) {
+		if ( 1 !== preg_match( '/^[a-f0-9]{32}\z/', $current_etag ) ) {
 			throw new InvalidArgumentException(
 				esc_html(
 					sprintf(

diff --git a/plugins/optimization-detective/class-od-url-metric-group.php b/plugins/optimization-detective/class-od-url-metric-group.php
@@ -92,7 +92,7 @@ final class OD_URL_Metric_Group implements IteratorAggregate, Countable, JsonSer
 	 * @param int                            $maximum_viewport_width Maximum possible viewport width for the group. Must be greater than zero and the minimum viewport width.
 	 * @param int                            $sample_size            Sample size for the maximum number of viewports in a group between breakpoints.
 	 * @param int                            $freshness_ttl          Freshness age (TTL) for a given URL Metric.
-	 * @param OD_URL_Metric_Group_Collection $collection             Collection that this instance belongs to. Optional.
+	 * @param OD_URL_Metric_Group_Collection $collection             Collection that this instance belongs to.
 	 */
 	public function __construct( array $url_metrics, int $minimum_viewport_width, int $maximum_viewport_width, int $sample_size, int $freshness_ttl, OD_URL_Metric_Group_Collection $collection ) {
 		if ( $minimum_viewport_width < 0 ) {

diff --git a/plugins/optimization-detective/class-od-url-metric.php b/plugins/optimization-detective/class-od-url-metric.php
@@ -213,7 +213,7 @@ public static function get_json_schema(): array {
 				'etag'      => array(
 					'description' => __( 'The ETag for the URL Metric.', 'optimization-detective' ),
 					'type'        => 'string',
-					'pattern'     => '^[0-9a-f]{32}$',
+					'pattern'     => '^[0-9a-f]{32}\z',
 					'minLength'   => 32,
 					'maxLength'   => 32,
 					'required'    => false, // To be made required in a future release.

diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php
@@ -44,15 +44,15 @@ function od_register_endpoint(): void {
 			'type'        => 'string',
 			'description' => __( 'An MD5 hash of the query args.', 'optimization-detective' ),
 			'required'    => true,
-			'pattern'     => '^[0-9a-f]{32}$',
+			'pattern'     => '^[0-9a-f]{32}\z',
 			'minLength'   => 32,
 			'maxLength'   => 32,
 		),
 		'current_etag'        => array(
 			'type'        => 'string',
 			'description' => __( 'ETag for the current environment.', 'optimization-detective' ),
 			'required'    => true,
-			'pattern'     => '^[0-9a-f]{32}$',
+			'pattern'     => '^[0-9a-f]{32}\z',
 			'minLength'   => 32,
 			'maxLength'   => 32,
 		),
@@ -66,7 +66,7 @@ function od_register_endpoint(): void {
 			'type'              => 'string',
 			'description'       => __( 'HMAC originally computed by server required to authorize the request.', 'optimization-detective' ),
 			'required'          => true,
-			'pattern'           => '^[0-9a-f]+$',
+			'pattern'           => '^[0-9a-f]+\z',
 			'validate_callback' => static function ( string $hmac, WP_REST_Request $request ) {
 				if ( ! od_verify_url_metrics_storage_hmac( $hmac, $request['slug'], $request['current_etag'], $request['url'], $request['cache_purge_post_id'] ?? null ) ) {
 					return new WP_Error( 'invalid_hmac', __( 'URL Metrics HMAC verification failure.', 'optimization-detective' ) );

diff --git a/plugins/optimization-detective/tests/storage/test-data.php b/plugins/optimization-detective/tests/storage/test-data.php
@@ -282,7 +282,7 @@ public function test_od_get_url_metrics_slug(): void {
 		$second = od_get_url_metrics_slug( array( 'p' => 1 ) );
 		$this->assertNotEquals( $second, $first );
 		foreach ( array( $first, $second ) as $slug ) {
-			$this->assertMatchesRegularExpression( '/^[0-9a-f]{32}$/', $slug );
+			$this->assertMatchesRegularExpression( '/^[0-9a-f]{32}\z/', $slug );
 		}
 	}
 
@@ -328,7 +328,7 @@ public function test_od_get_url_metrics_storage_hmac_and_od_verify_url_metrics_s
 		list( $url, $slug, $cache_purge_post_id ) = $set_up();
 		$this->go_to( $url );
 		$hmac = od_get_url_metrics_storage_hmac( $slug, $url, $cache_purge_post_id );
-		$this->assertMatchesRegularExpression( '/^[0-9a-f]+$/', $hmac );
+		$this->assertMatchesRegularExpression( '/^[0-9a-f]+\z/', $hmac );
 		$this->assertTrue( od_verify_url_metrics_storage_hmac( $hmac, $slug, $url, $cache_purge_post_id ) );
 	}
 

diff --git a/plugins/optimization-detective/tests/storage/test-rest-api.php b/plugins/optimization-detective/tests/storage/test-rest-api.php
@@ -137,30 +137,36 @@ function ( OD_URL_Metric_Store_Request_Context $context ) use ( &$stored_context
 	 * @return array<string, mixed> Test data.
 	 */
 	public function data_provider_invalid_params(): array {
-		$valid_element = $this->get_valid_params()['elements'][0];
-		$current_etag  = md5( '' );
+		$valid_params  = $this->get_valid_params();
+		$valid_element = $valid_params['elements'][0];
 
 		return array_map(
-			function ( $params ) {
+			static function ( $params ) use ( $valid_params ) {
 				return array(
-					'params' => array_merge( $this->get_valid_params(), $params ),
+					'params' => array_merge( $valid_params, $params ),
 				);
 			},
 			array(
 				'bad_url'                                  => array(
 					'url' => 'bad://url',
 				),
+				'bad_current_etag1'                        => array(
+					'current_etag' => 'foo',
+				),
+				'bad_current_etag2'                        => array(
+					'current_etag' => $valid_params['current_etag'] . "\n",
+				),
 				'bad_slug'                                 => array(
 					'slug' => '<script>document.write("evil")</script>',
 				),
 				'bad_hmac'                                 => array(
 					'hmac' => 'not even a hash',
 				),
 				'invalid_hmac'                             => array(
-					'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array( 'different' => 'query vars' ) ), $current_etag, home_url( '/' ) ),
+					'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array( 'different' => 'query vars' ) ), $valid_params['current_etag'], home_url( '/' ) ),
 				),
 				'invalid_hmac_with_queried_object'         => array(
-					'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array() ), $current_etag, home_url( '/' ), 1 ),
+					'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array() ), $valid_params['current_etag'], home_url( '/' ), 1 ),
 				),
 				'invalid_viewport_type'                    => array(
 					'viewport' => '640x480',

diff --git a/plugins/optimization-detective/tests/test-class-od-url-metric.php b/plugins/optimization-detective/tests/test-class-od-url-metric.php
@@ -104,7 +104,7 @@ static function ( $value ) {
 				),
 				'error' => 'OD_URL_Metric[etag] must be at most 32 characters long.',
 			),
-			'bad_etag'                        => array(
+			'bad_etag1'                       => array(
 				'data'  => array(
 					'uuid'      => wp_generate_uuid4(),
 					'etag'      => 'd41d8cd98f00b204e9800998ecf8427$',
@@ -113,7 +113,18 @@ static function ( $value ) {
 					'timestamp' => microtime( true ),
 					'elements'  => array(),
 				),
-				'error' => 'OD_URL_Metric[etag] does not match pattern ^[0-9a-f]{32}$.',
+				'error' => 'OD_URL_Metric[etag] does not match pattern ^[0-9a-f]{32}\z.',
+			),
+			'bad_etag2'                       => array(
+				'data'  => array(
+					'uuid'      => wp_generate_uuid4(),
+					'etag'      => md5( '' ) . "\n",
+					'url'       => home_url( '/' ),
+					'viewport'  => $viewport,
+					'timestamp' => microtime( true ),
+					'elements'  => array(),
+				),
+				'error' => 'OD_URL_Metric[etag] must be at most 32 characters long.',
 			),
 			'missing_etag'                    => array(
 				'data' => array(
@@ -281,10 +292,7 @@ public function test_constructor( array $data, string $error = '' ): void {
 		$this->assertNull( $url_metric->get_group() );
 		$current_etag = md5( '' );
 		$collection   = new OD_URL_Metric_Group_Collection( array(), $current_etag, array(), 1, DAY_IN_SECONDS );
-		$groups       = iterator_to_array( $collection );
-		$this->assertCount( 1, $groups );
-		$this->assertInstanceOf( OD_URL_Metric_Group::class, $groups[0] );
-		$group = $groups[0];
+		$group        = $collection->get_first_group();
 		$url_metric->set_group( $group );
 		$this->assertSame( $group, $url_metric->get_group() );
 

diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php
@@ -86,6 +86,14 @@ public function data_provider_test_construction(): array {
 				'freshness_ttl' => HOUR_IN_SECONDS,
 				'exception'     => InvalidArgumentException::class,
 			),
+			'invalid_current_etag_bad2'  => array(
+				'url_metrics'   => array(),
+				'current_etag'  => md5( '' ) . PHP_EOL, // Note that /^[a-f0-9]{32}$/ would erroneously validate this. So the \z is required instead in /^[a-f0-9]{32}\z/.
+				'breakpoints'   => array( 400 ),
+				'sample_size'   => 3,
+				'freshness_ttl' => HOUR_IN_SECONDS,
+				'exception'     => InvalidArgumentException::class,
+			),
 			'invalid_url_metrics_bad'    => array(
 				'url_metrics'   => array( 'bad' ),
 				'current_etag'  => $current_etag,