Opened 5 years ago
Last modified 4 hours ago
#52886 reopened enhancement
Update esc_url to allow for specifying an https:// as default protocol
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 6.9 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Formatting | Keywords: | has-patch has-unit-tests has-dev-note dev-feedback |
| Focuses: | Cc: |
Description (last modified by )
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: GB30100
The usage could then be:
esc_url( $url, null, 'display', 'https://' );
Attachments (3)
Change History (32)
#1
@
5 years ago
- Keywords has-patch needs-unit-tests added
- Milestone changed from Awaiting Review to 5.8
#3
@
5 years 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
@
5 years 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.
#6
@
5 years 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.
5 years ago
#8
@
5 years 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.
#9
@
4 years 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 | */ |
| 4314 | function 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:
- still appears to meet the aims of the ticket without requiring a change to existing
esc_url()calls. - still allows developers to force
http://orhttps://if their call requires it. - seems to remove the redundancy @SergeyBiryukov raised.
- doesn't assume that
http://should be the default protocol. - will mean that
esc_url()automatically adapts when a website is updated to supporthttps://.
Questions
- If
wp_is_using_https()isn't appropriate because we only need to test one of the URLs, would it bewp_is_home_url_using_https()Docs orwp_is_site_url_using_https()Docs?
- If
wp_is_using_https()isn't appropriate because it fails to sufficiently check thehttps://status for this issue, wouldwp_is_https_supported()Docs be more appropriate? Or is there another that stands out as appropriate?
#10
follow-up:
↓ 11
@
4 years 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
@
4 years 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/mkazideally it would by default returnhttpsbutesc_url()will returnhttp.
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.
This ticket was mentioned in Slack in #core-editor by ndiego. View the logs.
3 years ago
#16
@
5 months ago
- Description modified (diff)
@pcarvalho suggested using the first allowed $protocols value (in the existing argument).
The following code would change the fallback scheme to https:// only if
esc_url()oresc_url_raw()includes an array as the second argument and- 'https' is the first value in that array.
$scheme = ( is_array( $protocols ) && 'https' === reset( $protocols ) ) ? 'https://' : 'http://';
$url = $scheme . $url;
Results:
echo esc_url( 'example.org' ); // http://example.org
echo esc_url( 'http-first.example.org', array( 'http', 'https' ) ); // http://http-first.example.org
echo esc_url( 'https-first.example.org', array( 'https', 'http' ) ); // https://https-first.example.org
I also tried to change the default with a filter (kses_allowed_protocols), though supporting that could be unnecessarily complex. If customizing the order of the wp_allowed_protocols() array would not cause problems elsewhere, then esc_url() might check whether 'https' is the first value in either the $protocols argument or the allowed protocols array:
$scheme = 'http://';
$allowed_protocols = wp_allowed_protocols();
if ( is_array( $allowed_protocols ) && in_array( 'https', $allowed_protocols )
&& ( is_array( $protocols ) && 'https' === reset( $protocols ) || ( ! is_array( $protocols ) && 'https' === reset( $allowed_protocols ) ) ) ) {
$scheme = 'https://';
}
$url = $scheme . $url;
Results:
echo esc_url( 'example.org' ); // https://example.org
echo esc_url( 'http-first.example.org', array( 'http', 'https' ) ); // http://http-first.example.org
echo esc_url( 'https-first.example.org', array( 'https', 'http' ) ); // https://https-first.example.org
This ticket was mentioned in PR #9864 on WordPress/wordpress-develop by @sabernhardt.
2 months ago
#17
Prepends https:// instead of http:// when 'https' is the first value in the $protocols array.
@sabernhardt commented on PR #9864:
2 months ago
#18
Note that this changes the test_protocol() unit test from r33851 to reflect a change in behavior. With WordPress 6.8, the esc_url function would return an empty string when $protocols only contains 'https', but this proposed change would return a URL in that case. I also included a few more cases.
#20
@
2 months ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 60734:
@johnbillion commented on PR #9864:
2 months ago
#21
#22
@
11 days ago
I created a draft for the dev note based on the commit message and included an example.
This ticket was mentioned in PR #10532 on WordPress/wordpress-develop by @peterwilsoncc.
5 days ago
#24
Duplicates since 6.9.0 annotation in esc_url() in the wrapper functions.
Trac ticket: https://core.trac.wordpress.org/ticket/52886
#25
@
5 days ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening this for PR#10532 to duplicate the @since 6.9.0 annotation on esc_url for the wrapper functions esc_url_raw() and sanitize_url().
It's a very minor docs only change.
#27
@
5 days ago
- Keywords dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for merging [61273] to 6.9 pending another committers sign-off.
#28
@
5 hours ago
Reopening for merging [61273] to 6.9 pending another committers sign-off.
Just to confirm, should we update the PHPDoc for deprecated functions as well? If so, I think we may also need to update the PHPDoc for the clean_url() function.
Thanks for the patch!
Just noting that the
@sincetag should be in the function DocBlock, after@since 2.8.0, and should follow this format for consistency: