Improve URL handling in Optimization Detective #1043

westonruter · 2024-03-10T12:54:24Z

Compute the current URL server-side instead of letting client specify the URL in JavaScript. Previously there was no validation that the supplied URL is actually valid or that it corresponds to the server-computed slug from the normalized query vars. True the URL is not used except as metadata, but it was not ideal that proper validation was not being performed. Now the validity of the supplied URL is guaranteed as it is used alongside the slug in the computation of the nonce.
Each stored URL metric now includes the original URL for which the data was gathered, being stored alongside the viewport, timestamp, and elements. The URL is also stored as the post_title of the od-url-metrics post so there is apparently duplication with the URLs stored in the post_content. However, this may not be the case since the URL metrics being stored the post are grouped because they share the same normalized query vars. Each of the URL metrics have have a slightly different actual URL (although they should share the same canonical URL). Storing the original URL with each URL metric can assist with debugging issues with query var normalization.
Removes JSON pretty-printing of post_content for URL Metrics posts.
Removed TODO for adding a version identifier for the stored URL metrics since strict schema validation now achieves this by dropping entries which no longer match the schema.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

github-actions · 2024-03-10T12:54:39Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

….com/WordPress/performance into update/url-handling

…om/WordPress/performance into update/url-handling

westonruter · 2024-03-18T21:57:32Z

plugins/optimization-detective/storage/data.php

+ *
+ * @return string Current URL.
+ */
+function od_get_current_url(): string {


Inspired by the same function in the AMP plugin: https://github.com/ampproject/amp-wp/blob/5708e19c60cf0f88ce522349fba955266b6892f6/includes/amp-helper-functions.php#L617-L662

felixarntz

@westonruter LGTM, just a few small questions.

felixarntz · 2024-03-19T16:56:01Z

plugins/optimization-detective/storage/data.php

+	if ( isset( $_SERVER['REQUEST_URI'] ) ) {
+		// phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
+		$current_url .= ltrim( wp_unslash( $_SERVER['REQUEST_URI'] ), '/' );
+	}


Thinking about my favorite edge-case of sites within a subdirectory, this works as expected, right? I guess since $_SERVER['REQUEST_URI'] would include the full path, including the part that's potentially part of home_url(), this should be fine. Just want to make sure since the function doesn't otherwise consider a path that may be part of home_url() already.

Yes, REQUEST_URI should include the full path including the subdirectory root path. Otherwise, this function here only uses home_url() to obtain the default scheme, host, and port. The path is not used.

felixarntz · 2024-03-19T16:56:22Z

plugins/optimization-detective/storage/data.php

+		// phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
+		$current_url .= ltrim( wp_unslash( $_SERVER['REQUEST_URI'] ), '/' );
+	}
+	return esc_url_raw( $current_url );


Why does this use esc_url_raw()?

Well, humm, since potentially malicious data is present in REQUEST_URI. Perhaps this is overkill, but it has worked well in the AMP plugin.

I would think this is already handled by $wpdb->prepare() when storing the value. But in any case, doesn't hurt I guess.

felixarntz · 2024-03-19T16:59:27Z

plugins/optimization-detective/storage/post-type.php

@@ -190,7 +190,7 @@ static function ( OD_URL_Metric $url_metric ): array {
 			},
 			$group_collection->get_flattened_url_metrics()
 		),
-		JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES // TODO: No need for pretty-printing.
+		JSON_UNESCAPED_SLASHES // No need for escaped slashes since not printed to frontend.


Completely unrelated question: Why is this relevant? Do frontend slashes have to be escaped? I'm asking since the Speculation Rules plugin JSON output contains backslashes which Domenic suggested to remove in my Speculation Rules post draft code example. I'm confused about whether those should or shouldn't be there. 🤔

Good question. The slashes should always be present when the JSON is inline in HTML, as this prevents malicious strings from breaking out of the containing script. For example, if someone somehow got </script> into the data being serialized to JSON, this would prematurely terminate the script element, whereas if slashes are escaped, then <\/script> would not.

In the context of post_content here where the JSON is never being printed to HTML, there is no need for this escaping.

Er, so that should probably be restored to Speculation Rules 😊

I see, thanks for explaining. I believe Speculation Rules already does this correctly then, feel free to check. I was referring to the Make Core post draft I've been working on.

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images no milestone PRs that do not have a defined milestone for release [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Mar 10, 2024

westonruter added 3 commits March 10, 2024 08:22

Move 'url' field to core URL Metric schema

7f170e6

Move URL computation server-side

7a93aad

Remove unnecessary JSON pretty-printing

56f2ebe

westonruter force-pushed the update/url-handling branch from 02aab4d to 56f2ebe Compare March 10, 2024 15:22

This was referenced Mar 11, 2024

Optimization Detective: Server-applied optimizations informed by client-side detection #869

Closed

Introduce ILO_URL_Metric to encapsulate core data structure #988

Merged

Merge branch 'update/ilo-to-optimization-detective' of https://github…

b259404

….com/WordPress/performance into update/url-handling

westonruter mentioned this pull request Mar 14, 2024

Implement garbage collection of URL metrics including at uninstallation #1055

Merged

Base automatically changed from update/ilo-to-optimization-detective to feature/image-loading-optimization March 18, 2024 21:54

felixarntz self-requested a review as a code owner March 18, 2024 21:54

Merge branch 'feature/image-loading-optimization' of https://github.c…

b6095dd

…om/WordPress/performance into update/url-handling

westonruter commented Mar 18, 2024

View reviewed changes

felixarntz approved these changes Mar 19, 2024

View reviewed changes

westonruter merged commit 18d36fe into feature/image-loading-optimization Mar 19, 2024
48 checks passed

westonruter deleted the update/url-handling branch March 19, 2024 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve URL handling in Optimization Detective #1043

Improve URL handling in Optimization Detective #1043

westonruter commented Mar 10, 2024 •

edited

Loading

github-actions bot commented Mar 10, 2024 •

edited

Loading

westonruter Mar 18, 2024

felixarntz left a comment

felixarntz Mar 19, 2024

westonruter Mar 19, 2024

felixarntz Mar 19, 2024

westonruter Mar 19, 2024

felixarntz Mar 19, 2024

felixarntz Mar 19, 2024

westonruter Mar 19, 2024

westonruter Mar 19, 2024

felixarntz Mar 19, 2024

Improve URL handling in Optimization Detective #1043

Improve URL handling in Optimization Detective #1043

Conversation

westonruter commented Mar 10, 2024 • edited Loading

Checklist

github-actions bot commented Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Mar 10, 2024 •

edited

Loading

github-actions bot commented Mar 10, 2024 •

edited

Loading