Make WordPress Core

Opened 2 years ago

Closed 6 months ago

Last modified 4 hours ago

#56160 closed enhancement (duplicate)

Deprecate wp_sanitize_redirect

Reported by: malthert's profile malthert Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.3
Component: Security Keywords:
Focuses: Cc:

Description

All places that currently use it are better served with esc_url_raw and there seems to be no correct usage of it anywhere (most plugins use it where esc_url_raw should be used instead).

I'm happy to provide a PR, I just need to get some heads up about how the deprecation process works in WP exactly.

Change History (10)

#1 @costdev
2 years ago

  • Keywords 2nd-opinion added
  • Version changed from trunk to 2.3

Hi @malthert, thanks for opening this ticket. I've left a few thoughts below.


[53455] changed esc_url_raw() calls to sanitize_url() as this is now the recommended function for sanitizing a URL as of WordPress 6.1.


All places that currently use it are better served with esc_url_raw and there seems to be no correct usage of it anywhere (most plugins use it where esc_url_raw should be used instead).

For discussion, posterity, and for making committers' lives easier should this get support, could you provide your thoughts on why esc_url_raw() serves some of these usages better, or where it's more appropriate than wp_sanitize_redirect()?


An impact analysis is an important part of any deprecation. I'll kick it off with some numbers:


I've added the 2nd-opinion keyword to draw attention to this ticket.

#2 @malthert
2 years ago

WP core uses it only in 3 functions:

  • wp_redirect
  • wp_safe_redirect
  • wp_validate_redirect

https://developer.wordpress.org/reference/functions/esc_url_raw/
States:

The resulting URL is safe to use in database queries, redirects and HTTP requests.

So if esc_url_raw is safe to use in (= sanitize), then there is no need for wp_sanitize_redirect, as esc_url_raw already does this.
If esc_url_raw is NOT safe for sanitizing data in redirects, this needs to be updated in docs to highlight wp_sanitize_redirect has to be used.

Additionally, it's unclear what the difference between the use cases for sanitize_url and wp_sanitize_redirect are?

This ticket was mentioned in PR #6024 on WordPress/wordpress-develop by @kkmuffme.


8 months ago
#3

  • Keywords has-patch added

sanitize_url() description says:

Sanitizes a URL for database or redirect usage.

Therefore a separate function does not make sense, as it creates inconsistent behavior.

#4 @kkmuffme
8 months ago

I want to take over this ticket and created a PR to alias the functions like what happened to esc_url_raw with sanitize_url in WP 6.1. I didn't mark it as deprecated though to keep the external (plugins,...) minimal.

I didn't look at the internals - if these functions actually did the same. In case they did not, the docs for sanitize_url and esc_url_raw should be updated instead to clarify that they're NOT safe for redirects.

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


8 months ago

#6 @swissspidy
8 months ago

  • Keywords needs-refresh added

The current PR breaks existing tests for wp_sanitize_redirect and wp_validate_redirect so clearly the behavior is not identical.

#7 @kkmuffme
8 months ago

Yes, so the PR should be to update the docs to remove "redirect" from sanitize_url docs?

#8 @kkmuffme
8 months ago

And maybe mark the wp_sanitize_redirect @internal since it doesn't really have a use case outside of wp_(safe_)redirect, does it?

#9 @kkmuffme
6 months ago

  • Keywords 2nd-opinion has-patch needs-refresh removed
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #60864.

#10 @desrosj
4 hours ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.