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

Merged
merged 29 commits into from
Dec 17, 2024
Merged
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
57a35e4
Revert "Remove od_store_url_metric_validity filter to be re-added in …
westonruter Dec 2, 2024
57df740
Use 'status' key instead of 'code'
westonruter Dec 2, 2024
1d46d83
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 11, 2024
9de74b3
Add clear_cache() method to OD_URL_Metric_Group
westonruter Dec 11, 2024
ad1d9ea
Add ability to unset an extended property on OD_URL_Metric and OD_Ele…
westonruter Dec 12, 2024
8fba89a
Suppress erroneous IDE warnings
westonruter Dec 12, 2024
8f2af87
Unset lcpElementExternalBackgroundImage if URL is invalid
westonruter Dec 13, 2024
06f0fea
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 13, 2024
42005e6
Improve docs and tidiness
westonruter Dec 13, 2024
0d584b7
Add tests for od_url_metric_storage_validity filter
westonruter Dec 13, 2024
e40b3f1
Fix typo in readme
westonruter Dec 13, 2024
d73f8ca
Scaffold new tests for Image Prioritizer
westonruter Dec 15, 2024
31fc8ac
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 15, 2024
18553ee
Add missing access private tags
westonruter Dec 15, 2024
b7b1a47
Add tests for various validity conditions for external BG images
westonruter Dec 15, 2024
5373233
Fix handling of invalid external BG image and add tests
westonruter Dec 15, 2024
bcefd69
Avoid preloading background images larger than 2MB
westonruter Dec 15, 2024
6005f4a
Update readme to relate features of Image Prioritizer
westonruter Dec 15, 2024
250f094
Replace validity filter with sanitization filter; unset unset()
westonruter Dec 16, 2024
f74f4f4
Rename filter
westonruter Dec 16, 2024
6856026
Further optimize WP_Query
westonruter Dec 16, 2024
e1d0ac9
Improve translatability of error message
westonruter Dec 16, 2024
b4e693c
Update readme with links to where the performance features are implem…
westonruter Dec 17, 2024
42c3de8
Merge branch 'trunk' into add/external-bg-preload-validation
westonruter Dec 17, 2024
d4c9f40
Eliminate od_store_url_metric_data filter in favor of reusing rest_re…
westonruter Dec 17, 2024
f8a00b4
Remove todo
westonruter Dec 17, 2024
4a8dc16
Account for route matching being case insensitive
westonruter Dec 17, 2024
ba14c36
Improve function description and further trim route
westonruter Dec 17, 2024
5ab7fd1
Add test case for route ending in newline
westonruter Dec 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improve function description and further trim route
westonruter committed Dec 17, 2024

Verified

This commit was signed with the committer’s verified signature. The key has expired.
westonruter Weston Ruter
commit ba14c364c76754e855c42bd0fa20e7318f74ede7
6 changes: 3 additions & 3 deletions plugins/image-prioritizer/helper.php
Original file line number Diff line number Diff line change
@@ -264,7 +264,7 @@ static function ( $host ) {
}

/**
* Filters the response before executing any REST API callbacks.
* Sanitizes the lcpElementExternalBackgroundImage property from the request URL Metric storage request.
*
* This removes the lcpElementExternalBackgroundImage from the URL Metric prior to it being stored if the background
* image URL is not valid. Removal of the property is preferable to invalidating the entire URL Metric because then
@@ -288,8 +288,8 @@ function image_prioritizer_filter_rest_request_before_callbacks( $response, arra
if (
$request->get_method() !== 'POST'
||
// The strtolower() is due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match.
OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== strtolower( trim( $request->get_route(), '/' ) )
// The strtolower() is due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match.
OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== trim( strtolower( trim( $request->get_route(), '/' ) ) )
Copy link
Member Author

Choose a reason for hiding this comment

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

By chance I had the thought of checking whether the route was case sensitive. It wasn't. So this would have been a vulnerability. I want to double check how $request->get_route() gets populated to see if there could potentially be any other ways to make a request to the endpoint and yet bypass this condition here.

Copy link
Member Author

@westonruter westonruter Dec 17, 2024

Choose a reason for hiding this comment

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

Indeed, I found that if you tried making a request without pretty permalinks and added %0A to the end of the rest_route query parameter:

http://localhost:8888/index.php?rest_route=/optimization-detective/v1/url-metrics:store%0A

Then the route is parsed as "/optimization-detective/v1/url-metrics:store\n" and this matches here in \WP_REST_Server::match_request_to_handler() on this line:

$match = preg_match( '@^' . $route . '$@i', $path, $matches );

This is because $ matches a newline. In reality this should be using \z instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test case to explicitly check for this: 5ab7fd1

) {
return $response;
}