Performance enhancements for esc_url()

[markjaquith]

esc_url() gets used a lot on WordPress admin pages. Sometimes 100 times or more. Nacin did some KcacheGrind measurements that had it as 7% of some pages. We can speed it up.

Most of the grind comes from wp_kses_bad_protocol().

I had a thought that we're sort of going about things backwards. We're being very careful to exclude anything harmful — bad characters, bad protocols, duplicate fake-out protocols, etc. But almost 100% of the time, the URL going through it is a http/https URL that has completely normal characters in it. We can detect that really early, and bail.

I did some tests with this approach that showed a good time savings.

Quite obviously, there's no room to compromise on security, so we'll need to be watching unit test, and maybe even writing some new ones for good measure.

[nacin]

I think this would be great for 3.6.

[johnbillion]

Did you ever write a patch for this Mark?

[peterwilsoncc]

@schlessera assigning to you as requested ands putting on the 4.7 milestone. It can always be moved off if the security stuff requires additional testing.

[desrosj]

This has a solid plan from @markjaquith, needs a patch and some unit tests.

[peterwilsoncc]

@schlessera did you get a chance to look at this? It's been sitting around for a while so we can postpone if not.

thanks.

[schlessera]

@peterwilsoncc Sorry for not getting back to you sooner.

I had some time during a contributor day and made a first attempt at improving performance. Basically, I just add a regex in front of the bad protocol detection to return early for http(s) URLs. I'm sure there might be other performance enhancements at other (earlier) points in the code execution, but these will also have more potential impact on the security implications than what I've currently done.

With the change in place, the existing unit tests in group kses still pass successfully.

The regex I've taken is the @diegoperini one (with a slight modification to remove ftp) from the comparison here: ​https://mathiasbynens.be/demo/url-regex . It is pretty long and feature-complete, but due to the way the regex is evaluated, it is actually fast enough to provide a real improvement.

I've done some (not 100% scientific) benchmarks to compare the current version of esc_url from trunk against my modified version. For all benchmarks, both versions run on the same machine from within the same process, and only the relative change is stored, to get rid of fluctuations due to allocations of server resources as good as possible. You can see a table with the benchmark results here: ​https://docs.google.com/spreadsheets/d/1XK5lgAIDMlj6lQSTrelXdD5eh6wAqJuLMS1605iM618/edit?usp=sharing

The benchmark summary: In a normal use case, this change should speed up the esc_url function by 25-30%. In a best case scenario, the speed-up can be as high as 40%, while the absolute worst case scenario (basically, dealing with 100% bad protocol links) produces a slow-down of 2%, which I deem is acceptable given the very low probability of this ever being the case.

The test script I've used to run the benchmarks can be found here: ​https://gist.github.com/schlessera/d4dd4db71d59dfee6ee6c5b9a0ca8a33

[desrosj]

This patch still applies cleanly, but I am getting some unit test failures while testing.

[schlessera]

Refreshed the patch and made changes to fix broken tests

[schlessera]

The test failures were due to two issues:

• the protocol is normalized to lowercase by the full protocol check
• http needs to be rejected when only https is allowed, and vice versa.

Both problems should be solved.

[mukesh27]

Hi there!

@schlessera is this ticket is in your to-do list for the upcoming version?

The 22951.2.diff​ patch needs to update against the Trank version.

It's a good addition if we add this so we get some good page speed as esc_url is many places to validate URL.

dev-feedback added for more eyes on this ticket.

[mukesh27]

Patch updated against trunk version.

[mehulkaklotar]

I think this would be great for 6.2

[mukesh27]

As discuss in today's Performance bug scrub it. Let move this ticket in 6.2 for the visibility.

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

[mukesh27]

Thanks @markjaquith for the ticket!

​PR #3598 that working fine without any issue.

@desrosj @schlessera @azaozz Can you please take a look?

[schlessera]

@desrosj @schlessera @azaozz Can you please take a look?

I left a comment on the PR about turning the regex into a constant or static string to avoid having it be reconstructed multiple times per process.

[mukesh27]

I appreciate the review and advice, @schlessera. I have updated my PR because the changes make sense.

[azaozz]

Looking at the patches/PR, may be missing something but why the big regex?

The idea here is to short-circuit wp_kses_bad_protocol() when that function won't have anything to do. The purpose of this function is to strip "bad" protocols from the beginning of a string (presumably an URL). Seems it is mostly used to prevent things like javascript://alert(123) when the javascript:// protocol is not allowed. (Frankly I think rejecting a URL if it starts with a bad protocol would be better, but that's outside the scope here.)

Also wp_kses_bad_protocol_once() seems to be converting some HTML entities in the whole string. This is to better match the : char. See:

$string  = preg_replace( '/(&#0*58(?![;0-9])|&#x0*3a(?![;a-f0-9]))/i', '$1;', $string );
$string2 = preg_split( '/:|&#0*58;|&#x0*3a;|:/i', $string, 2 );

The proposed regex seem to be validating the whole URL (which is not intended here), and seems it may be missing the HTML entities. In addition thinking there was a problem with null characters that can "throw off" some regex (or some browsers?), so $string = wp_kses_no_null( $string ); should be at the top in all cases.

If we go with the original idea, the patch should probably be doing something like:

1. Remove null chars.
2. Check if https:// is allowed in $allowed_protocols. If yes, check if the string starts with https:// case-insensitive. If yes, short-circuit.
3. Check if http:// is allowed in $allowed_protocols. If yes, check if the string starts with http:// case-insensitive. If yes, short-circuit.

This will catch the most common use cases and will probably speed up wp_kses_bad_protocol() quite a bit. Don't think it needs to use regex at all. Seems stripos( $string, 'https://' ) === 0 would be enough, and a lot faster.

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

[mukesh27]

@azaozz thanks for the feedback. I have submitted second approach ​PR #3615. There is some unit tests failing due to string case that i'm checking.

The current version of esc_url from trunk and my modified version have been compared using benchmarks. To eliminate variations caused by server resource allocations as much as feasible, both versions of each benchmark are run on the same computer from within the same process, with only the relative change being recorded. The benchmark results are displayed in a table here: ​https://docs.google.com/spreadsheets/d/1eR80SuQkn1gZc4pfWE1D6enbYSlpRDqharfBtAkTLb0/edit?usp=sharing

The benchmark summary states that this improvement should speed up the esc_url function by 15% to 20% under typical use conditions. While the worst-case scenario (essentially, handling 100% broken protocol links) results in a slowdown of 1.2%, I consider this tolerable considering the extremely low likelihood that this will ever happen.

The test script I've used to run the benchmarks can be found here: ​https://gist.github.com/mukeshpanchal27/2f89f1fe0ed6c6ee091bf17d6e08bbb6 (I have edited/updated @schlessera code in this script)

@mukeshpanchal27 Thanks for the patch, ​https://github.com/WordPress/wordpress-develop/pull/3615/commits/20cca48f624bfb03cfefaa58811a5b087b12a44d looks good!

As you mentioned, some tests were failing because there was no lower case normalization of http(s). Don't think there are any security implications of that, but imho better to do it anyways (to keep the function output just as before).

Changed the code to do this in the fastest way possible (I think). Could you run the speed testing again if not a trouble. Would be good to compare with the previous results.

Thanks @azaozz for the feedback and changes.

I have re-run the analysis against both approaches, and below are the results:

Approach 1 use regex - ​PR #3598

• The test script I've used to run the benchmarks can be found here: ​https://gist.github.com/mukeshpanchal27/2f89f1fe0ed6c6ee091bf17d6e08bbb6
• The benchmark results are displayed in a table here: ​https://docs.google.com/spreadsheets/d/1eR80SuQkn1gZc4pfWE1D6enbYSlpRDqharfBtAkTLb0/edit#gid=0

Approach 2 - Current PR

• The test script I've used to run the benchmarks can be found here: ​https://gist.github.com/mukeshpanchal27/0d815c737a5c637d06beaf9d6ce93a0b
• The benchmark results are displayed in a table here: ​https://docs.google.com/spreadsheets/d/1eR80SuQkn1gZc4pfWE1D6enbYSlpRDqharfBtAkTLb0/edit#gid=243266386

The benchmark summary states that Approach 1 improvement should speed up the esc_url function by 2% to 3% under typical use conditions. I think we should implement some more performant functions for this PR to normalization of http(s) so we get some more performance improvement.

cc. @schlessera

I have re-run the analysis against both approaches

Thanks!

The benchmark summary states that Approach 1 improvement should speed up the esc_url function by 2% to 3%

Yea, this seems negligible. Thinking to get as close as possible to the "real picture" the results have to be weighted. The short-circuit here is only for http(s) because (the assumption is that) this matches most cases. A weighted results would probably be something like:

• 95% http://example.org/ and https://example.org/.
• 4.95% HTTP://EXAMPLE.ORG/ and HTTPS://EXAMPLE.ORG/ (his is probably even lower as the browsers normalize the URLs to lower case, but don't think it would matter that much).
• 0.05% everything else.

It is surprising to see that the big regex from ​https://github.com/WordPress/wordpress-develop/pull/3598 is actually faster than a strpos( $string, 'https://' ) and eventual strpos( $string, 'http://' ). That regex does a lot of needless things. Perhaps testing with longer strings (most URLs come with a path) would be better. The rest of the code in both patches seems the same, i.e. both call in_array( [protocol], $allowed_protocols, true ), etc.

Looking again at ​https://github.com/WordPress/wordpress-develop/pull/3598, it doesn't have the wp_kses_no_null( $string ) at the top which seems to be a regression. wp_kses_bad_protocol() is removing null chars from the string. Could that be the slow-down? Another thing there is returning early when the regex can't find a protocol. Don't think that's right. Also str_replace() may be unsafe (can replace more than needed). Will comment there.

Looked at this a bit more, specifically at how many URLs start with upper case HTTP. On the sites I have access to (not that many but still...), there were very few HTTP. Final count was 27 out of over 3000 URIs in post_content (nearly all URIs there are inserted by the editor and most are to uploaded images).

Of course, this is not representative, but thinking the upper case HTTP and HTTPS can probably be ignored here as not significant enough. This will simplify the shortcut code and make it even faster.

Also got curious to see if strpos() or 2x strpos() are faster than preg_match(). Did some quick testing and it seems they are :) The test setup ​is here. Typical results of 10,000 loops on my test server are:

Test regex:           3.438122 milliseconds.
Test strpos:          1.098487 milliseconds.
Test str_starts_with: 1.301403 milliseconds.

Using strpos() directly is fastest (no surprises) and the simple regex is slowest. These results are for 10,000 runs, so in reality all three are super fast (less than 0.001 millisecond). However since we want to speed things up, thinking that going with the fastest makes most sense. So the proposed code here would be:

function wp_kses_bad_protocol( $string, $allowed_protocols ) {
	$string = wp_kses_no_null( $string );

	// Short-circuit if the string starts with `https://` or `http://`. Most common cases.
	if (
		( 0 === strpos( $string, 'https://' ) && in_array( 'https', $allowed_protocols, true ) ) ||
		( 0 === strpos( $string, 'http://' ) && in_array( 'http', $allowed_protocols, true ) )
	) {
		return $string;
	}

	$iterations = 0;
	.............

Yea, this seems negligible. Thinking to get as close as possible to the "real picture" the results have to be weighted. The short-circuit here is only for http(s) because (the assumption is that) this matches most cases. A weighted results would probably be something like:

• 95% http://example.org/ and https://example.org/.
• 4.95% HTTP://EXAMPLE.ORG/ and HTTPS://EXAMPLE.ORG/ (his is probably even lower as the browsers normalize the URLs to lower case, but don't think it would matter that much).
• 0.05% everything else.

If we only consider standard urls (http and https), we have covered 95% of the URLs, which is fantastic. When the changes listed below are applied to the code, all of those urls see a performance boost of more than 30%.

function wp_kses_bad_protocol( $string, $allowed_protocols ) {
	$string = wp_kses_no_null( $string );

	// Short-circuit if the string starts with `https://` or `http://`. Most common cases.
	if (
		( 0 === strpos( $string, 'https://' ) && in_array( 'https', $allowed_protocols, true ) ) ||
		( 0 === strpos( $string, 'http://' ) && in_array( 'http', $allowed_protocols, true ) )
	) {
		return $string;
	}

	$iterations = 0;
	.............

This code looks solid to me, and I ran the analysis, and it performed better than the regex. The approx. The performance improvement is ~30% which is significant for standard URLs.

I post the result of strpos and str_starts_with here: ​https://github.com/WordPress/wordpress-develop/pull/3615#discussion_r1031073591 but for visibility, I would like to add it here also.

I ran the analysis for two functions in PHP 7.4 and 8.0 and the different is 1-2%.

• 7.4
  • strpos ~31%
  • str_starts_with ~30%
• PHP 8+
  • strpos ~32.5%
  • str_starts_with ~33.5%

As it improves performance now, improves performance further as more sites move to PHP 8+, it's easier to read, and Core has been changing existing instances of 0 === strpos() to str_starts_with(), I'm agreeing to use str_starts_with().

As it improves performance now, improves performance further as more sites move to PHP 8+, it's easier to read, and Core has been changing existing instances of 0 === strpos() to str_starts_with(), I'm agreeing to use str_starts_with().

Sounds good to me :)

[flixos90]

The approach from ​https://github.com/WordPress/wordpress-develop/pull/3615 looks reasonable and good to commit to me.

@markjaquith Since you originally opened this ticket (years ago), it would be great to get your feedback on that approach as well.

I'm happy to commit this, but we can let it wait for a bit more feedback first as there's still enough time until 6.2.

Note that the function parameter was renamed in ​https://github.com/WordPress/wordpress-develop/commit/a9afc8e36bf669ea1b5a5cb79602aae4e0ec613a, resulting in a merge conflict. But no worries, I will fix this conflict prior to committing, which I'm going to do shortly.

[flixos90]

In 55053:

Formatting: Improve performance of esc_url().

This changeset indirectly improves performance of the commonly used esc_url() function by optimizing the low-level function wp_kses_bad_protocol() for the by far most common scenarios, which are URLs using either the http or https protocol.

For this common scenario, the changeset now avoids the do while loop. While for a single call to the esc_url() function the performance wins are negligible, given that esc_url() is often called many times in one page load, they can add up, making this a worthwhile improvement.

Props mukesh27, schlessera, markjaquith, azaozz, spacedmonkey.
Fixes #22951.

Committed in https://core.trac.wordpress.org/changeset/55053.

Closing in favor of #3615, which was committed in https://core.trac.wordpress.org/changeset/55053.