Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harden validation of user-submitted LCP background image URL #1713

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 2, 2024

Fixes #1584

Accepting the LCP background image URL from the untrusted client is dangerous as it could lead to abuse. The JSON Schema to enforce the validity of the URL Schema with such a background image URL is not robust enough to ensure that the URL is safe and that it actually references a valid image file. Therefore, a new od_store_url_metric_validity filter is introduced in the REST API endpoint. After the user-supplied URL Metric has been validated according to the JSON Schema, the instance is passed as the second argument to this filter. Callbacks can then return false or a non-empty WP_Error to block the URL Metric from being stored, which is implemented here to ensure that the supplied background image is valid.

@westonruter westonruter added [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) labels Dec 2, 2024
@westonruter westonruter added this to the image-prioritizer n.e.x.t milestone Dec 2, 2024
* @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema.
* @return bool|WP_Error Validity. Valid if true or a WP_Error without any errors, or invalid otherwise.
*/
function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Strict_URL_Metric $url_metric ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying thread from #1697 (comment):

@adamsilverstein:

is part of the goal here to validate that the stored path actually returns an image? Is this something that could be checked on the client side?

@westonruter:

The goal is to validate that the URL is valid. Since a malicious client could submit a URL to point to anything (e.g. even some 3P tracker URL), it's important to validate that (1) it's actually an image, (2) it's on a safe host.

}

$r = wp_safe_remote_head(
$data['url'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from #1697 (comment):

The wp_safe_remote_head() function passes the supplied URL through wp_http_validate_url(), but that doesn't necessarily mean the request is safe. I'm thinking there should perhaps be an allowlist of hosts that this is allowed to connect to. Maybe we query for the first image attachment and pass it into wp_get_attachment_image_url() to then extract the hostname from the image. This would be better than having to try to come up with a list of all image CDNs that are used by plugins.

The list of allowed hosts could also be sent to the frontend so as to avoid sending the invalid URL in the first place. See related todo:

// TODO: There should to be a way to pass additional args into the module. Perhaps extensionModuleUrls should be a mapping of URLs to args.
if ( extension.initialize instanceof Function ) {
const initializePromise = extension.initialize( {
isDebug,
webVitalsLibrarySrc,
} );

Then again, would be a shape to continuously reject all of the URL Metrics in the case that an unexpected host is being used for a given background image. In that case, perhaps we should allow OD_URL_Metric to be mutable, so in the case where an invalid host is provided this callback could instead do:

$url_metric->set( 'lcpElementExternalBackgroundImage', null );

or rather

$url_metric->unset( 'lcpElementExternalBackgroundImage' );

When mutating an instance of OD_URL_Metric it should re-trigger the validation so that the mutation would cause the JSON Schema to be made invalid that it would throw an exception.

Also, whenever mutation happens then it should call a new $this->group->clear_cache() method which empties out $group->result_cache and which would in turn call $collection->clear_cache().

So this od_store_url_metric_validity filter could be used in two ways:

  1. A filter callback could return false or a non-empty WP_Error instance to block the URL Metric from being stored at all.
  2. A filter callback could mutate the supplied OD_URL_Metric object to apply custom sanitizations or validation constraints which then would end up getting persisted in storage.

* @param bool|WP_Error $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise.
* @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema.
*/
$validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying from #1697 (comment):

To compliment https://github.com/WordPress/performance/pull/1697/files#r1859381128, this would allow a filter callback to always have access to whatever the original data was, regardless of any changes which have been made by other callbacks to the $url_metric object. This would be analogous to the second $postarr argument to the wp_insert_post_data filter, after the filtered $data arg.

Suggested change
$validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric );
* @param bool|WP_Error $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise.
* @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema.
* @param array<string, mixed> $url_metric_data Original URL Metric data before any mutations.
*/
$validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric, $url_metric->jsonSerialize() );

Base automatically changed from add/external-bg-preload to trunk December 11, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) [Plugin] Optimization Detective Issues for the Optimization Detective plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image Prioritizer should be able to store the LCP (background) image URL for prioritization
1 participant