Revise the use of nonces in requests to store a URL Metric and block cross-origin requests #1637
Conversation
westonruter
added
[Type] Bug
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
westonruter
changed the title
westonruter
force-pushed
the
fix/od-with-page-caching
branch
from
5f64507 to
68c8fc5
Compare
westonruter
force-pushed
the
fix/od-with-page-caching
branch
from
68c8fc5 to
167c935
Compare
… fix/od-with-page-caching
There was a problem hiding this comment.
@westonruter This looks great! Just some minor feedback, which would be good to address, but no blockers.
| * @since 0.1.0 | ||
| * @since n.e.x.t Renamed from od_get_url_metrics_storage_nonce(). |
There was a problem hiding this comment.
Better to consider this an entirely new function. I would argue it wasn't "just" renamed, the purpose was also changed as it now returns a different kind of value.
There was a problem hiding this comment.
Sure. Done in 9d6e807
| * @since 0.1.0 | ||
| * @since n.e.x.t Renamed from od_verify_url_metrics_storage_nonce(). |
There was a problem hiding this comment.
See above, I'd prefer we consider this a new function.
There was a problem hiding this comment.
Sure. Done in 9d6e807
| * @param string $origin Origin to check. | ||
| * @return bool Whether the origin is allowed. | ||
| */ | ||
| function od_is_allowed_http_origin( string $origin ): bool { |
There was a problem hiding this comment.
Why can't we use is_allowed_http_origin() directly instead of this function?
If it's just about the port support, I'd say that's an edge case so then I think we should at least use the original function anytime there's no port, and the custom logic only if there is one.
I'd also say we could simplify it: If there is a port in the passed $origin, simply remove it for the comparison and call is_allowed_http_origin() with that. It's not really that important to validate that the port also matches IMO, especially because of how much of an edge case that is.
There was a problem hiding this comment.
Well, unit tests run the home being localhost:8889 so it's not an edge case from that perspective.
I've gone ahead and just stripped out the port number in 545f0c3.
There was a problem hiding this comment.
Great, I think that simplifies the implementation, and validating the same port number is probably not super important (for localhost sites there's probably not too much of a security implication anyway).
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
This is the first PR to address #1496.
wp_restnonce when making POST requests to store a URL Metric. Since the endpoint does not require authentication, the nonce adds no value.Originrequest header so that it must matchhome_url()in order for the URL Metric storage POST request to be accepted. The WordPress REST API by default allows for cross-origin requests, but this makes no sense specifically for storing a URL Metric since the client must by definition be on the frontend of the site.wp_create_nonce()to compute the HMAC to validate that the provided query-varsslugactually corresponds to the providedurlin the request (that is, the URL of the current page). There is no need to usewp_create_nonce()here, since there does not need to be an expiration and it does not need to be tied to the current logged-in user.By eliminating the use of nonces, clients will not have issues when submitting a URL Metric for storage when a page cache is holding onto the rendered page for longer than a day.