Function to generate safe & trusted URLs

[johnjamesjacoby]

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.

[johnjamesjacoby]

Introduce wp_generate_url()

[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.

[johnjamesjacoby]

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.

[helen]

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

[rmccue]

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.)

[johnjamesjacoby]

Related: #20771

[ocean90]

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