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.

Sign up for GitHub

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

Jump to bottom

Prevent speculatively loading links to the uploads, content, plugins, template, or stylesheet directories #1167

Merged
merged 4 commits into from
Apr 29, 2024
Merged

Prevent speculatively loading links to the uploads, content, plugins, template, or stylesheet directories #1167

merged 4 commits into from
Apr 29, 2024

Conversation

westonruter
Copy link
Member

This was brought up by @AntonyXSI in #1157:

3. Excluding links that contain "." or file extensions i.e to prevent a user from downloading large PDFs or images potentially bottlenecking bandwidth. An example would be images being wrapped in a link to the original uncropped image. Hovering over images in a gallery for example could result in lots of large unoptimized images being downloaded unnecessarily in the background. As images will begin to render almost instantly prerendering images, pdfs etc may not be as beneficial as prerendering pages. I'm also not sure what happens if .exe,.pdf or other large files are prefetched that would normally be downloaded once clicked? Does Chrome drop the request or continue the download from the same prefetch request?

See Chrome's behavior for speculative loading of an image and a PDF: #1157 (comment).

In this PR, the uploads directory is excluded from speculative loading. The uploads directory path is added to the base href excludes.

Before After
image image

What if the uploads directory is pointing to a CDN instead? Well, it will be ignored anyway because the speculation rules are configured to only do same-origin speculative loads:

performance/plugins/speculation-rules/helper.php

Lines 83 to 91 in cb5a37e

$rules = array(
array(
'source' => 'document',
'where' => array(
'and' => array(
// Include any URLs within the same site.
array(
'href_matches' => $prefixer->prefix_path_pattern( '/*' ),
),

So they will be excluded anyway. (The same goes for a site with a site customizing the site_url to point to another domain, cf. #1164). But what if someone wants to do cross-origin speculative loads? That could be made possible with #1156 where the href_matches is changed from /* to instead be *://*/*. Nevertheless, it seems Chrome does not currently support this:

image

Nevertheless, the above was specifically for prerendering. If I try instead with the mode set to prefetch, I get different results:

image

So it can work with prefetch in some situations.

We should keep that in mind for the future.

@westonruter westonruter added the [Type] Enhancement A suggestion for improvement of an existing feature label Apr 19, 2024
@westonruter westonruter added this to the speculation-rules n.e.x.t milestone Apr 19, 2024
@westonruter westonruter requested a review from felixarntz as a code owner April 19, 2024 20:03
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 19, 2024 •
edited
Loading

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: tunetheweb <[email protected]>
Co-authored-by: adamsilverstein <[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.

@westonruter westonruter requested a review from tunetheweb April 19, 2024 20:04
@westonruter westonruter mentioned this pull request Apr 24, 2024
Closed
@felixarntz
Copy link
Member

@westonruter Would it make sense to instead prevent speculative loading of any links to something within wp-content? Uploads is probably most important, but I'm not sure we'd want to speculatively load anything bundled in a plugin or theme either, as it wouldn't be a regular WordPress URL.

@westonruter
Copy link
Member Author

@felixarntz yeah, good point. That should be added as another entry specific to wp-content since the uploads directory can be configured to point somewhere else.

@westonruter
Copy link
Member Author

For completeness, the following URLs should be excluded: WP_CONTENT_URL, WP_PLUGIN_URL, get_stylesheet_directory_uri(), and get_template_directory_uri(). All of these can be overridden from their defaults.

@westonruter westonruter changed the title Prevent speculatively loading links to the uploads directory Prevent speculatively loading links to the uploads, content, plugins, template, or stylesheet directories Apr 25, 2024
adamsilverstein
adamsilverstein approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Nice work!

tunetheweb
tunetheweb approved these changes Apr 29, 2024
View reviewed changes
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

@westonruter westonruter merged commit 0410600 into trunk Apr 29, 2024
25 checks passed
@westonruter westonruter deleted the update/uploads-dir-excluded-from-speculative-loading branch April 29, 2024 15:02
@westonruter
Copy link
Member Author

Build for testing: speculation-rules.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamsilverstein adamsilverstein adamsilverstein approved these changes

@tunetheweb tunetheweb tunetheweb approved these changes

@felixarntz felixarntz Awaiting requested review from felixarntz felixarntz is a code owner

Assignees
No one assigned
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Milestone
speculation-rules 1.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@westonruter @felixarntz @adamsilverstein @tunetheweb