Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#31495 closed defect (bug) (fixed)

Always use 'login' as $scheme parameter for "login-ish" URLs, and other inconsistencies

Reported by: greglone's profile GregLone Owned by: johnbillion's profile johnbillion
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Hi.

I found a lot of references of site_url() used for login/register/whatever that do not use the second parameter $scheme (it should be 'login'). But this is a first step, we should use wp_login_url(), wp_registration_url(), and friends.

So far I found the following:

In wp-signup.php:
wp_redirect( site_url('wp-login.php?action=register') ); should use wp_registration_url().

$login_url = site_url( 'wp-login.php?redirect_to=' . urlencode( network_site_url( 'wp-signup.php' ) ) ); should use wp_login_url( network_site_url( 'wp-signup.php' ) ).

Even worse, the function confirm_another_blog_signup() uses "http://" . $domain.$path . "wp-login.php".

In wp-activate.php:
wp_redirect( site_url( '/wp-login.php?action=register' ) ); should use wp_registration_url().
Also note the "/" at the beginning. Not important, but not consistent with other URLs.

And few lines later:
$url . 'wp-login.php' should be replaced by get_site_url( (int) $result['blog_id'], 'wp-login.php', 'login' ) imho.

I also wonder if network_site_url('wp-login.php', 'login') shouldn't be replaced by site_url('wp-login.php', 'login'), since this is the only file where it is used that way anyway.

In wp-includes/canonical.php:
wp_redirect( site_url('wp-login.php?action=register') ); should use wp_registration_url().

In wp-admin/install.php:
There's a '</p><p class="step"><a href="../wp-login.php" class="button button-large">' . __( 'Log In' ) . '</a></p></body></html>'.
And another <a href="../wp-login.php" class="button button-large"><?php _e( 'Log In' ); ?></a>.

In wp-includes/ms-functions.php:
There is Log in here: BLOG_URLwp-login.php in an email template.
And another <a href="../wp-login.php" class="button button-large"><?php _e( 'Log In' ); ?></a>.

In wp-admin/network.php:
<a href="<?php echo esc_url( site_url( 'wp-login.php' ) ); ?>"><?php _e( 'Log In' ); ?></a>

In wp-includes/schema.php:
There is Log in here: BLOG_URLwp-login.php in an email template.

Any thought?

Attachments (3)

31495.patch (7.0 KB) - added by GregLone 9 years ago.
Replace some site_url( 'wp-login.php' ) with wp_login_url() and friends
31495.2.patch (5.3 KB) - added by johnbillion 9 years ago.
31495.3.patch (3.8 KB) - added by johnbillion 8 years ago.

Download all attachments as: .zip

Change History (16)

#1 follow-up: @DrewAPicture
9 years ago

  • Version 4.1.1 deleted

Would you like to make a patch? Standouts I would skip are:

  • wp-activate.php: The use of network_site_url(). I think that needs to be left alone because it's specifically for the multisite/network URL use-case. If it isn't multisite, it'll fall back to site_url() anyway.
  • wp-includes/ms-functions.php: email template string
  • wp-includes/schema.php: email template

For the others, it might be worth a try fixing. Some of those have been in core since we switched from using direct file links more than four years ago.

#2 @DrewAPicture
9 years ago

Related:

#19097
use wp_register() in wp-login.php
#21089
Add admin url to install notification email
#27632
`wp_registration_url()` doesn't have a redirect argument


@GregLone
9 years ago

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

#3 @GregLone
9 years ago

In 31495.patch:
wp-activate.php:

  • Use wp_registration_url() for redirection instead of site_url( '/wp-login.php?action=register' ).
  • Use switch_to_blog() + wp_login_url() + restore_current_blog() instead of $url . 'wp-login.php'.

wp-signup.php:

  • Use wp_registration_url() for redirection instead of site_url( 'wp-login.php?action=register' ).
  • In validate_another_blog_signup(), use the blog ID returned by wpmu_create_blog() and pass it to confirm_another_blog_signup(). In confirm_another_blog_signup(), use the new parameter $blog_id to get the login URL. Also removed two unused parameters $user_email and $meta.
  • Use wp_login_url( network_site_url( 'wp-signup.php' ) ) instead of site_url( 'wp-login.php?redirect_to=' . urlencode( network_site_url( 'wp-signup.php' ) ) ).

wp-admin/install.php:

  • Use esc_url( wp_login_url() ) instead of href="../wp-login.php".

wp-admin/network.php:

  • Use esc_url( wp_login_url() ) instead of esc_url( site_url( 'wp-login.php' ) ).

wp-includes/canonical.php:

  • Use wp_registration_url() for redirection instead of site_url( 'wp-login.php?action=register' ).
  • Use wp_login_url() for redirection instead of site_url( 'wp-login.php', 'login' ).

#4 in reply to: ↑ 1 @GregLone
9 years ago

Replying to DrewAPicture:

  • wp-activate.php: The use of network_site_url(). I think that needs to be left alone because it's specifically for the multisite/network URL use-case. If it isn't multisite, it'll fall back to site_url() anyway.

As far as I can tell, network_site_url() is used only for "lost password" and "reset password". Is there any reason we should keep using network_site_url('wp-login.php', 'login') here for the login address? As it is not used anywhere else in core, it should be replaced by esc_url( wp_login_url() ) imho. At least, it should be discussed.

  • wp-includes/ms-functions.php: email template string
  • wp-admin/includes/schema.php: email template

I agree for schema.php, we should leave it as it is now.
May I suggest something simple for ms-functions.php? A bit hacky, but back-compat'.

switch_to_blog( $blog_id );
$login_url = wp_login_url();
restore_current_blog();

$welcome_email = str_replace( 'BLOG_URLwp-login.php', $login_url, $welcome_email );

Some of those have been in core since we switched from using direct file links more than four years ago.

Yes, sorry for the "Version 4.1.1" x)

Last edited 9 years ago by GregLone (previous) (diff)

#5 @GregLone
9 years ago

  • Keywords has-patch needs-testing added

@johnbillion
9 years ago

#6 @johnbillion
9 years ago

  • Keywords needs-testing removed
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to johnbillion
  • Status changed from new to assigned

31495.2.patch refreshes the patch for the non-contentious instances of wp-login.php. Note that since [28609], the login_post scheme is the same as the login and admin schemes, so we can also replace a couple more instances where the login_post scheme is used.

#7 @GregLone
9 years ago

What about:

  • the hard-coded wp-login.php in confirm_another_blog_signup(),
  • network_site_url( 'wp-login.php', 'login' ) in wp-activate.php?

#8 @johnbillion
9 years ago

In 34213:

Implement wp_login_url() and wp_registration_url() in places where wp-login.php is currently hard-coded.

See #31495
Props GregLone

#9 @johnbillion
9 years ago

In 34358:

Implement some more uses of wp_login_url() in places where wp-login.php is hard-coded.

See #31495

#10 @johnbillion
8 years ago

  • Keywords dev-feedback added

31495.3.patch tidies up the remaining changes originally patched by @GregLone.

Just needs a second set of eyes and the nod.

@johnbillion
8 years ago

#11 follow-up: @johnbillion
8 years ago

@jeremyfelt 31495.3.patch introduces two situations where home_url() is nestled between switch_to_blog() and restore_current_blog(). Can you envisage something breaking in this case?

#12 in reply to: ↑ 11 @jeremyfelt
8 years ago

Replying to johnbillion:

@jeremyfelt 31495.3.patch introduces two situations where home_url() is nestled between switch_to_blog() and restore_current_blog(). Can you envisage something breaking in this case?

I think the only breakage would be if someone was blindly filtering home_url without being aware of the context in multisite. It doesn't seem like much to worry about here as this is all happening at the network level on new sites.

We may want to run things through esc_url() though, as they are filtered.

#13 @johnbillion
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 34790:

Use wp_login_url() for login links when signing up for a new blog or activating a new blog on Multisite.

Fixes #31495
Props GregLone for the intial patch

Note: See TracTickets for help on using tickets.