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

Exclude _wpnonce URLs in speculation rules #1143

Merged
merged 2 commits into from Apr 16, 2024

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Apr 15, 2024

This is a sub-PR of #1142.

Fixes #1141

Before After
Screenshot 2024-04-15 14 49 27 Screenshot 2024-04-15 14 49 33

The URLPattern used to exclude URLs with _wpnonce is /*\?*(^|&)_wpnonce=*. Take special note that the second * wildcard only matches chars in the query segment. Additionally, the ^ anchors the match to the beginning of the search segment, and not to the beginning of the entire string. (Remember: URLPattern is not regex.) This means that only the _wpnonce query parameter will matched, whether it is the first query parameter or not. It also won't incorrectly match other_wpnonce.

The following URLs were tested to be properly included and excluded:

URL Status URLPattern Tester
http://example.com/foo/?_wpnonce=1234 Excluded Test
http://example.com/?_wpnonce=1234 Excluded Test
http://example.com/foo/?amp=1&_wpnonce=1234 Excluded Test
http://example.com/?amp=1&_wpnonce=1234 Excluded Test
http://example.com/foo/?_wpnonce=1234&amp=1 Excluded Test
http://example.com/?_wpnonce=1234&amp=1 Excluded Test
http://example.com/?my_wpnonce=1234 Not excluded (because not _wpnonce) Test
http://example.com/?p=1#?_wpnonce=1234 Not excluded (because in URL fragment) Test
http://example.com/_wpnonce=1234/ Not excluded (because in path) Test

@westonruter westonruter added [Type] Bug An existing feature is broken [Plugin] Speculative Loading Issues for the Speculative Loading plugin (formerly Speculation Rules) labels Apr 15, 2024
@westonruter westonruter added this to the speculation-rules n.e.x.t milestone Apr 15, 2024
Copy link
Contributor

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

github-actions bot commented Apr 15, 2024

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 <westonruter@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: tunetheweb <tunetheweb@git.wordpress.org>

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

@tunetheweb
Copy link
Contributor

Nice thorough set of tests btw. Anyway we can add them to the plugin test suite so if someone in future isn't as robust as you, it's still covered?

@westonruter
Copy link
Member Author

Anyway we can add them to the plugin test suite so if someone in future isn't as robust as you, it's still covered?

That's a good idea. I've opened #1144 to explore that.

Base automatically changed from fix/woocommerce-add-to-cart-preloading to trunk April 16, 2024 06:19
@swissspidy swissspidy merged commit 205bda3 into trunk Apr 16, 2024
22 checks passed
@swissspidy swissspidy deleted the fix/woocomerce-remove-from-cart branch April 16, 2024 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Speculative Loading Issues for the Speculative Loading plugin (formerly Speculation Rules) [Type] Bug An existing feature is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WooCommerce "Remove from cart" links are prefetched/preloaded in Speculative Loading
3 participants