WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 months ago

#32071 new enhancement

Function to generate safe & trusted URLs

Reported by: johnjamesjacoby Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

Recent developments around plugins and themes misusing add_query_arg() and remove_query_arg() got me to thinking about how WordPress has a responsibility to make generating safe and reliable URLs as easy as possible.

I'm proposing we introduce a new helper function that all URLs in core would be switched over to use, to lead by example and completely eliminate any margin for errors when it comes to the order of operations of manipulating URLs and outputting them to the browser.

Patch imminent.

Attachments (1)

32071.01.patch (2.7 KB) - added by johnjamesjacoby 3 years ago.
Introduce wp_generate_url()

Download all attachments as: .zip

Change History (8)

@johnjamesjacoby
3 years ago

Introduce wp_generate_url()

#1 follow-ups: @johnbillion
3 years ago

I'm not a fan of escaping inside functions. Developers should become familiar with late escaping. I think a function like this introduces yet more inconsistency and gives the wrong impression that you can trust the output of some functions.

#2 in reply to: ↑ 1 @johnjamesjacoby
3 years ago

Replying to johnbillion:

I'm not a fan of escaping inside functions. Developers should become familiar with late escaping. I think a function like this introduces yet more inconsistency and gives the wrong impression that you can trust the output of some functions.

I don't disagree about late escaping, but I disagree strongly with it being less consistent.

I can think of several functions without looking that escape and echo their unescaped equivalent: the_permalink(), comments_link(), and a bunch of feed functions. Regardless of these examples, it's semantics whether it's esc_url() or wp_generate_url() or some other function that's doing it.

The mechanism through which late-escaping occurs matters much less to me than being confident that specific functions perform specific tasks given specific parameters with sane defaults and a reasonable ability to override.

I want a way to reduce the number of permutations of how URL's are constructed down to a single function, to avoid differences like the following examples:

bp_core_redirect( add_query_arg( array( 'action' => 'bpnoaccess' ), wp_login_url( $redirect ) ) );
echo esc_url( add_query_arg( array( 'page' => 'bp-components', 'action' => 'all'      ), bp_get_admin_url( $page ) ) );
esc_url( bp_get_admin_url( add_query_arg( array( 'page' => 'bp-components' ), 'admin.php' ) ) )
echo esc_url( wp_nonce_url( add_query_arg( array( 'action' => 'do_delete', 'gid' => implode( ',', $gids ) ), $base_url ), 'bp-groups-delete' ) )

And probably one of my favorites:

esc_url( network_admin_url( add_query_arg( '_wp_http_referer', urlencode( wp_unslash( $_SERVER['REQUEST_URI'] ) ), wp_nonce_url( 'users.php', 'deleteuser' ) . '&action=deleteuser&id=' . $user->ID ) ) )

If nothing else, I'll update the patch without the escaping bit at the end. My point still stands that developers are forced to nest too much complex logic into a series of somewhat unpredictable functions just to generate the only thing that connects one page on the web to another.

#3 @helen
3 years ago

Related: #31603 (which I think shows we need a proper canonical current admin URL).

#4 in reply to: ↑ 1 @rmccue
3 years ago

Replying to johnbillion:

I'm not a fan of escaping inside functions. Developers should become familiar with late escaping.

I agree for escaping that isn't idempotent (i.e. can't be applied infinitely), like esc_html, esc_attr, etc. esc_url_raw on the other hand can be applied infinitely, so it doesn't have to follow late escaping. I'd prefer that all functions that say they return URLs actually return URLs, and we still escape them regardless.

(On that note, I think esc_url doing HTML escaping is a bit dumb, esc_html( esc_url_raw( or esc_attr( esc_url_raw would have been a better choice.)

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


3 years ago

#7 @ocean90
3 years ago

Some related tickets which should be tackled before this IMO: #22951, #13051, #16859, #23605

Note: See TracTickets for help on using tickets.