WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 9 hours ago

#52886 new enhancement

Update esc_url to allow for specifying an https:// as default protocol

Reported by: mkaz Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch needs-dev-note has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

If no protocol is specified for esc_url the function will automatically prepend the http:// protocol. This is likely now the wrong assumption, but potentially can break backwards compatibility if changed, since developers may rely on this.

So this change proposes an additional parameter to the function to specify a default protocol, keeping the old default but now allowing for one to ask for https://

This came up in this ticket: https://github.com/WordPress/gutenberg/pull/30100

The usage could then be:

esc_url( $url, null, 'display', 'https://' );

Attachments (3)

52886.diff (2.2 KB) - added by mkaz 6 months ago.
52886.1.diff (2.3 KB) - added by audrasjb 5 months ago.
52886.2.diff (2.9 KB) - added by mkaz 4 months ago.
Unit test added.

Download all attachments as: .zip

Change History (15)

@mkaz
6 months ago

#1 @rachelbaker
6 months ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.8

#2 @SergeyBiryukov
6 months ago

Thanks for the patch!

Just noting that the @since tag should be in the function DocBlock, after @since 2.8.0, and should follow this format for consistency:

@since 5.8.0 Added the `$default_protocol` parameter.

@audrasjb
5 months ago

#3 @audrasjb
5 months ago

  • Keywords needs-dev-note added

In 52886.1.diff:

  • Add @since mention
  • Few coding standards fixes (whitespaces inside docblocks)

Question: shouldn't we also add the new parameter in the clean_url filter at the bottom of the function? Not sure it's really needed, but I prefer to ask :)

#4 @SergeyBiryukov
5 months ago

The usage could then be:

esc_url( $url, null, 'display', 'https://' );

Looking at this again, I wonder if it's straightforward enough in practice, as it would require passing four parameters to every esc_url() call that might be missing the protocol and should default to https://. If the site is known to use https://, that seems redundant.

Perhaps this should be a filter instead, like esc_url_default_protocol? That way it could only be specified once.

Version 0, edited 5 months ago by SergeyBiryukov (next)

#5 @SergeyBiryukov
5 months ago

  • Description modified (diff)

#6 @mkaz
5 months ago

Thanks for the follow up on this one, it had fallen off my radar.

My potential concern with a filter is that it would apply to all esc_url() calls.

The use case we had was to implement on the Block Editor for the social links, but if we did it as a filter and once it got to core it the filter would be implemented for everyone which is the same as default to https:// everywhere.

Also, as a filter someone else could override and then it would not pick the right URL for say https://twitter.com/

@audrasjb I'm not sure it makes sense to pass in to the clean_url filter, I can't think of a scenario where it would help.

My other thought was should we change to esc_url_raw to also support defaulting to https://

This ticket was mentioned in Slack in #core-editor by mkaz. View the logs.


4 months ago

#8 @desrosj
4 months ago

  • Milestone changed from 5.8 to Future Release

As we're at feature freeze for 5.8 and this one still needs unit tests, I'm going to punt. Please do continue to discuss and feel free to move back into a numbered milestone once tests are able to be added.

@mkaz
4 months ago

Unit test added.

#9 @costdev
3 weeks ago

@SergeyBiryukov

Looking at this again, I wonder if it's straightforward enough in practice, as it would require passing > four parameters to every esc_url() call that might be missing the protocol and should default to
https://. If the site is known to use https://, that seems redundant.

In this case, would wp_is_using_https()Docs be appropriate regarding "known to use https://" as it tests the home and site URLs?

If so, an adjustment could be made to the latest patch:

Line 
4291<?php
4292
4293/**
4294 * Checks and cleans a URL.
4295 *
4296 * A number of characters are removed from the URL. If the URL is for displaying
4297 * (the default behaviour) ampersands are also replaced. The {@see 'clean_url'} filter
4298 * is applied to the returned cleaned URL.
4299 *
4300 * @since 2.8.0
4301 * @since 5.8.0 Added the `$default_protocol` parameter.
4302 *
4303 * @param string   $url              The URL to be cleaned.
4304 * @param string[] $protocols        Optional. An array of acceptable protocols.
4305 *                                   Defaults to return value of wp_allowed_protocols().
4306 * @param string   $_context         Private. Use esc_url_raw() for database usage.
4307 * @param string   $default_protocol Optional. Use to specify a different default protocol.
4308 *                                   Defaults to http:// or https:// based on the result
4309 *                                   of wp_is_using_https().
4310 * @return string The cleaned URL after the {@see 'clean_url'} filter is applied.
4311 *                An empty string is returned if `$url` specifies a protocol other than
4312 *                those in `$protocols`, or if `$url` contains an empty string.
4313 */
4314function esc_url( $url, $protocols = null, $_context = 'display', $default_protocol = '' ) {
4315        $original_url = $url;
4316
4317        if ( '' === $default_protocol ) {
4318                $default_protocol = ( wp_is_using_https() ) ? 'https://' : 'http://';
4319        }
4320
4321        if ( '' === $url ) {
4322                return $url;
4323        }
4324
4325        $url = str_replace( ' ', '%20', ltrim( $url ) );
4326        $url = preg_replace( '|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\[\]\\x80-\\xff]|i', '', $url );
4327
4328        if ( '' === $url ) {
4329                return $url;
4330        }
4331
4332        if ( 0 !== stripos( $url, 'mailto:' ) ) {
4333                $strip = array( '%0d', '%0a', '%0D', '%0A' );
4334                $url   = _deep_replace( $strip, $url );
4335        }
4336
4337        $url = str_replace( ';//', '://', $url );
4338        /*
4339         * If the URL doesn't appear to contain a scheme, we presume
4340         * it needs http:// prepended (unless it's a relative link
4341         * starting with /, # or ?, or a PHP file).
4342         * Since 5.8, it uses $default_protocol which, if empty, will use
4343         * http:// or https:// based on the result of wp_is_using_https().
4344         */
4345        if ( strpos( $url, ':' ) === false && ! in_array( $url[0], array( '/', '#', '?' ), true ) &&
4346                ! preg_match( '/^[a-z0-9-]+?\.php/i', $url ) ) {
4347                $url = $default_protocol . $url;
4348        }
4349
4350        // Replace ampersands and single quotes only when displaying.
4351

This adjustment:

  1. still appears to meet the aims of the ticket without requiring a change to existing esc_url() calls.
  2. still allows developers to force http:// or https:// if their call requires it.
  3. seems to remove the redundancy @SergeyBiryukov raised.
  4. doesn't assume that http:// should be the default protocol.
  5. will mean that esc_url() automatically adapts when a website is updated to support https://.

Questions

  1. If wp_is_using_https() isn't appropriate because we only need to test one of the URLs, would it be wp_is_home_url_using_https() Docs or wp_is_site_url_using_https() Docs?
  1. If wp_is_using_https() isn't appropriate because it fails to sufficiently check the https:// status for this issue, would wp_is_https_supported() Docs be more appropriate? Or is there another that stands out as appropriate?
Last edited 3 weeks ago by costdev (previous) (diff)

#10 follow-up: @mkaz
3 weeks ago

In this case, would wp_is_using_https()Docs be appropriate regarding "known to use https://" as it tests the home and site URLs?

No, unfortunately that wouldn't solve the case I'm looking for, that function only works for the current site while the esc_url() function can be passed any arbitrary URL which it won't know if it is SSL or not.

The example is a Twitter profile link passed in like: twitter.com/mkaz ideally it would by default return https but esc_url() will return http.

#11 in reply to: ↑ 10 @costdev
3 weeks ago

Replying to mkaz:

In this case, would wp_is_using_https()Docs be appropriate regarding "known to use https://" as it tests the home and site URLs?

No, unfortunately that wouldn't solve the case I'm looking for, that function only works for the current site while the esc_url() function can be passed any arbitrary URL which it won't know if it is SSL or not.

The example is a Twitter profile link passed in like: twitter.com/mkaz ideally it would by default return https but esc_url() will return http.


True! I believe I misinterpreted if the site is known to use https to refer to the current site rather than the remote site. Your patch resolves the ticket's issue without any BC breaks and I agree that esc_url_raw() should be updated to support a $default_protocol as well.

#12 @costdev
9 hours ago

  • Keywords has-unit-tests added; needs-unit-tests removed
Note: See TracTickets for help on using tickets.