WordPress.org

Make WordPress Core

Opened 10 days ago

Last modified 3 days ago

#44353 new defect (bug)

Replace `site_url( 'wp-login.php' )` in `wp_send_user_request()`

Reported by: 7studio Owned by:
Milestone: 4.9.7 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: good-first-bug has-patch 2nd-opinion
Focuses: Cc:

Description

According to this old issue #31495, we should use site_url( 'wp-login.php', 'confirmaction' ) instead of site_url( 'wp-login.php' ) in the function wp_send_user_request() there https://github.com/WordPress/WordPress/blob/3ee58b55b14683af4c568bce1845c37408c88fbb/wp-includes/user.php#L3336

This change would be appreciable to help plugins which hide the login URL to not replace this link by the new secret login URL or/and replace easily the login URL part in the link to click on to confirm the account action.

Hope you will consider this ticket and it contributes to WP.

Attachments (2)

44353.2.diff (578 bytes) - added by usmankhalid 10 days ago.
Proposed solution
44353.diff (551 bytes) - added by desrosj 4 days ago.
Replace site_url( 'wp-login.php' ) with wp_login_url().

Download all attachments as: .zip

Change History (11)

#1 @desrosj
10 days ago

  • Keywords needs-patch added

@usmankhalid
10 days ago

Proposed solution

#2 @abdullahramzan
10 days ago

  • Keywords good-first-bug added; needs-patch removed

#3 @abdullahramzan
10 days ago

  • Keywords has-patch added

#4 @birgire
10 days ago

According to the documentation the scheme input argument of site_url() and get_site_url() is:

$scheme (string) (Optional) Scheme to give the site URL context. Accepts 'http', 'https', 'login', 'login_post', 'admin', or 'relative'.

Default value: null

with reference to set_url_scheme()

https://developer.wordpress.org/reference/functions/site_url/ https://developer.wordpress.org/reference/functions/get_site_url/ https://developer.wordpress.org/reference/functions/set_url_scheme/

It seems supporting confirmaction context would need other changes as well, at least for the documentation.

ps: we can see that wp_login_url() sets the login context, with site_url('wp-login.php', 'login'), so that might not be suitable here (as I previously suggested).

Last edited 9 days ago by birgire (previous) (diff)

#5 @rwebster85
7 days ago

Would it not be easier to add a filtered variable in the array instead of site_url()? That way we could hook into the filter and change it to a frontend page, for sites like mine that disable access to the default WP pages and have everything on the frontend.

Something like:

$site_url = apply_filters( 'some_filter', site_url( 'wp-login.php' ), 'confirmaction' )

$email_data = array(
        'email'       => $request->email,
        'description' => wp_user_request_action_description( $request->action_name ),
        'confirm_url' => add_query_arg( array(
                'action'      => 'confirmaction',
                'request_id'  => $request_id,
                'confirm_key' => wp_generate_user_request_key( $request_id ),
        ), $site_url ),
        'sitename'    => is_multisite() ? get_site_option( 'site_name' ) : get_option( 'blogname' ),
        'siteurl'     => network_home_url(),
);
Last edited 7 days ago by rwebster85 (previous) (diff)

#6 follow-up: @desrosj
7 days ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 4.9.7

The second parameter of site_url() is passed down to set_url_scheme(), as @birgire detailed above. This function ensures that http and https is used correctly in the URL, or that the protocol is stripped out if the scheme is relative. It doesn't seem that passing comfirmaction to site_url() for context is a good idea.

Because the URL links to wp-login.php, I think using wp_login_url() is the right thing to do here. That would allow a plugin or theme to use the login_url filter to filter the login URL, and ensure the correct scheme is applied (using the already supported login scheme). This would also handle moving the action, request_id, and comfirm_key parameters to the new login URL automatically instead of requiring plugins to handle that.

@rwebster85 Making the array of data that is passed filterable is also not a bad suggestion, but I think that deserves a separate ticket for discussing. Filters could be added to all privacy email data before constructing the emails. Can you open a ticket for that?

#7 in reply to: ↑ 6 @rwebster85
7 days ago

Replying to desrosj:

@rwebster85 Making the array of data that is passed filterable is also not a bad suggestion, but I think that deserves a separate ticket for discussing. Filters could be added to all privacy email data before constructing the emails. Can you open a ticket for that?

Good suggestion, I've added a new ticket.

@desrosj
4 days ago

Replace site_url( 'wp-login.php' ) with wp_login_url().

#8 @desrosj
4 days ago

  • Keywords 2nd-opinion added; needs-refresh removed

#9 @7studio
3 days ago

@desrosj I can understand your position about set_url_scheme() and the parameter $scheme but I think we should use this parameter like a context or a keyword (e.g.: login, login_post, admin, etc) which will be used in apply_filters( 'set_url_scheme', $url, $scheme, $orig_scheme ) no more. The login URL has not its place in a public email and plugins should do the difference between this URL and the login one :/ Plus, the ticket opened by @rwebster85 has been closed… It would be so easy to filter this URL via the filter hook set_url_scheme and not handle it like that:

<?php
function thistle_override_privacy_confirm_url( $email_text, $email_data ) {
    $confirm_url_query = parse_url( $email_data['confirm_url'], PHP_URL_QUERY );
    $confirm_url = home_url( 'donnees-personnelles/' ) . '?' . $confirm_url_query;

    $email_text = str_replace( '###CONFIRM_URL###', esc_url_raw( $confirm_url ), $email_text );

    return $email_text;
}
add_filter( 'user_request_action_email_content', 'thistle_override_privacy_confirm_url', 10, 2 );

Thank you very much to all for your proposals and your point of view, it's really interesting :)

Note: See TracTickets for help on using tickets.