Make WordPress Core

Opened 7 years ago

Closed 20 months ago

Last modified 18 months ago

#40426 closed defect (bug) (fixed)

Remove dns-prefetch of s.w.org domain

Reported by: joelhardi's profile joelhardi Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: minor Version: 4.8
Component: Emoji Keywords: has-patch has-unit-tests commit assigned-for-commit
Focuses: privacy Cc:

Description

DNS prefetch for the emoji CDN in general-template.php is causing the link tag <link rel='dns-prefetch' href='//s.w.org' /> to be generated on public-facing URLs such as the login/registration page wp-login.php.

This may trigger the user-agent to make a DNS lookup of this domain name which would leak information (for instance) to a rogue DNS resolver. This is a security/privacy concern because it enables discovery or profiling of browsing behavior by anyone operating an upstream DNS resolver.

There does not seem to be a compelling reason to request a dns-prefetch of any hardcoded domain names (even those owned by Automattic) -- I don't believe it is desirable for WordPress core to trigger any unnecessary external network activity by default. An HTTP request to a third-party domain would be a more obvious example, but even a DNS lookup of a FQDN can be used for tracking.

An alternate remedy would be to conditionally insert this <link> tag only when external assets from s.w.org are required, but given the questionable value that dns-prefetch of a CDN domain provides in normal environments (browser, system or local DNS resolver is likely to have already cached this DNS lookup, so dns-prefetch does nothing in the vast majority of page loads) I'd suggest it's simpler to simply remove the hardcoded prefetch insertion. Less code is better in this case.

Please see patch. This is also a net performance improvement since it reduces page size and server processing, but that's secondary.

Plugins, themes or optional components such as emoji support can still trigger resource hints such as prefetch independently, but hardcoding URLs into general-template.php seems like a hack.

Attachments (2)

general-template.php.diff (599 bytes) - added by joelhardi 7 years ago.
patch to remove hard-coded dns-prefetch of s.w.org domain name
40426.diff (653 bytes) - added by sabernhardt 3 years ago.
refreshed patch

Download all attachments as: .zip

Change History (16)

@joelhardi
7 years ago

patch to remove hard-coded dns-prefetch of s.w.org domain name

#2 @superpoincare
7 years ago

Another related issue is that if you self host emoji (no CDN), using add_filter to change baseUrl and svgUrl to folders in your own domain, then the code adds a line to dns-prefetch to your own domain.

So if your website domain is example.com, you see a line in the HTML output:

<link rel='dns-prefetch' href='//example.com' />

This isn't needed.

#3 @jhabdas
7 years ago

  • Severity changed from normal to minor
  • Type changed from enhancement to defect (bug)

I'd suggest it's simpler to simply remove the hardcoded prefetch insertion. Less code is better in this case.

Simply put: A page which contains a DNS prefetch to a domain it does not call externally should be considered a bug. I too question the value of dns-prefetch and, in this case, it seems to remove more value than it adds and I'd like to see it removed so I do not need to hack core to optimize page loads (yes, even dead code, even dead code).

#4 @garrett-eclipse
5 years ago

Related - #44001
*If emoji scripts gets internalized due to privacy concerns then the need to prefetch s.w.org will be gone.

@sabernhardt
3 years ago

refreshed patch

#5 @sabernhardt
3 years ago

  • Component changed from General to Script Loader
  • Keywords has-patch added

I considered making the filter default an empty string (or boolean false), but that wouldn't solve the situation with self-hosted emoji. So I simply refreshed the first patch.

For a temporary fix, I have used the filter to remove the link tag in a plugin:

add_filter( 'emoji_svg_url', '__return_false' );

This ticket was mentioned in PR #3055 on WordPress/wordpress-develop by sabernhardt.


21 months ago
#6

  • Keywords has-unit-tests added
  1. Remove emoji_svg_url prefetch from wp_resource_hints().
  2. Update unit tests in Tests_General_wpResourceHints (tests/phpunit/tests/general/wpResourceHints.php): remove test_should_have_defaults_on_frontend() entirely and remove s.w.org prefetch from expected output of ten other test functions.

Trac ticket: https://core.trac.wordpress.org/ticket/40426

#7 @sabernhardt
21 months ago

  • Component changed from Script Loader to Emoji
  • Focuses privacy added
  • Milestone changed from Awaiting Review to 6.1

mukeshpanchal27 commented on PR #3055:


21 months ago
#8

Thanks, @sabernhardt for the PR. It looks good to me. @SergeyBiryukov Can you please review it when you get a chance?

#9 @SergeyBiryukov
21 months ago

  • Keywords commit added

Some history here:

Looks like concerns were raised on both tickets about whether it's appropriate to include this for every WordPress site.

@johnbillion in comment:53:ticket:34292:

Including a default preconnect entry for s.w.org seems unjustified. This means every single visitor (with a supporting browser) to every single WordPress site in the world will be performing a preconnect to s.w.org, purely to save a few ms in case an emoji is used that's not supported by the browser.
Does this really justify a preconnect to s.w.org on every site?

@dd32 in comment:3:ticket:37387:

I'm not sure myself if it should be in the list at all, a fallback emoji loading a few ms later isn't going to be the end of the world, and browsers are getting better at supporting everything already.
I'd say the vast majority of WordPress sites don't actually have any Emoji's on them, so a few ms in those cases would be fine.

I tend to agree with these sentiments. Since there is also a privacy concern as outlined in this ticket, it seems better to not include dns-prefetch for s.w.org by default. A plugin could use the wp_resource_hints filter for that.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


20 months ago

#11 @audrasjb
20 months ago

  • Keywords assigned-for-commit added
  • Owner set to audrasjb
  • Status changed from new to accepted

Looks good to go. Self assigning for commit as no one owns this ticket right now.

#12 @audrasjb
20 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 53904:

Script Loader: Remove default DNS prefetch entry for s.w.org.

A default DNS prefetch entry for s.w.org was previously included to save a few ms in case an emoji is used that is not supported by the browser. It appears this use case doesn't justify a prefetch to s.w.org on every WordPress website.

This changesets removes emoji_svg_url prefetch from wp_resource_hints(). It also updates unit tests in Tests_General_wpResourceHints by removing test_should_have_defaults_on_frontend() entirely and removing s.w.org prefetch from expected output of ten other test functions.

Plugin authors can use the wp_resource_hints filter if they need to re-add the DNS prefetch entry for s.w.org.

Follow-up to [37920], [38122].

Props joelhardi, superpoincare, jhabdas, garrett-eclipse, sabernhardt, SergeyBiryukov.
Fixes #40426.
See #37387.

#14 @peterwilsoncc
18 months ago

@mukesh27 added to props via make/core per comment #8 via prbot.

Note: See TracTickets for help on using tickets.