login_url Filter is now applied to Login Form Action Attribute

[salcode]

Previously, the login_url filter was not applied to the action attribute on the form element of the login form.

This allowed setting up a custom login page and using the login_url filter to point to it.

For example,

add_filter( 'login_url', 'new_login_url', 10, 2 );
function new_login_url( $login_url, $redirect ) {
    return '/log-in/';
}

taken from ​https://trepmal.com/2011/04/07/redirect-users-to-a-custom-log-in-page/.

With the changes introduced in [34213], the action attribute now uses wp_login_url() instead of site_url( 'wp-login.php', 'login_post' ). This applies the login_url attribute filter to the action attribute, which should not be.

site_url() differentiates between login and login_post based on the scheme parameter, however wp_login_url() does not. wp_login_url() uses a scheme parameter of login, which makes it inappropriate for use as the action attribute of the form element (since we want a scheme of login_post in this case).

See #34925 for a related discussion on another impact of the form action attribute using the login rather than login_post scheme

[salcode]

I'd like to see these changes rolled back as suggested in patch https://core.trac.wordpress.org/attachment/ticket/34925/34925.patch

[DrewAPicture]

@johnbillion Care to weigh in?

I think this report further highlights the downside of wp-login.php serving both as the "login URL" and post handler for the login form.

And in lieu of handing form submission outside of wp-login.php, such as how comment submissions were moved to wp_handle_comment_submission() in [34799], we're kind of stuck in a back-compat rabbit hole.

Seems like two best options would be:

1. Revert back to using site_url() as before [34213]
2. As in [35897], use set_url_scheme() with login_post and add a third parameter to wp_login_url() that effectively bypasses the login_url filter on return.

#2 feels kind of gross, but it makes sense if we want to continue to promote standardization of wp_login_url()/wp_registration_url() as in #31495.

[khag7]

I have always understood wp_login_url() to mean "The url which one can visit to use a login form" but not necessarily the url to which the form data is posted.

Previous to 4.4.0 the login form generated by wp_login_form() had the action hard coded to wp-login.php. This made it easy (and not uncommon) for users to have wp_login_url() filtered to point to a custom page. It was also easy for these users to have a shortcode put a login form on that page which draws from wp_login_form(). They didn't need to worry about making their page handle the posted data since the form was hard-coded to post the form submission to wp-login.php.

The patch proposed in #34925 fixes the problem outlined there, but doesn't fix the bug described here. The desired action is that the function wp_login_url should not be used to get the url to which the form should be submitted. We need another way of determining the url to post the data to.

I've written a function, wp_login_handler_url() which solves the problem here and the problem from the other ticket. Now we can use wp_login_url() and wp_login_handler_url() separately. By default, they are going to both be pointing at wp-login.php, but users can now filter login_url while still allowing the login form post data to be sent to wp-login.php.

If we commit this patch, we could (and should) search through core files for use of site_url('wp-login.php') and instead use either wp_login_handler_url or login_url depending on the circumstance of use.

Edit:
The spirit of the original change was good. We shouldn't have hard-coded references to wp-login.php, we should have a function and one place that can be filtered. And attempting to use the existing function, wp_login_url() to get that url was a good idea, but its obvious that two separate but similar functions are needed here.

[khag7]

creates new function wp_login_handler_url which returns the url to which login and logout post data should be sent

[salcode]

@khag7 I really like your thought about adding a new function.

However, for a bug fix release like 4.4.1, my vote is revert back to site_url() and then revisit your patch in 4.5.

[KrissieV]

Forgive me if this is not the correct place for this, I'm new to trac and testing patches. I was able to test patch 34925.2 that was in the linked ticket above and confirm it does not solve this issue as @khag7 stated. I reverted back and have been trying to apply the new patch, 35103 but am getting a "Hunk #5 FAILED at 495" error. It's likely user error, as I am really new to this, and I've done a lot of searching to find out where I'm going wrong, but can't find a solution. I'd love to help get this issue resolved however I can contribute as I have a site in development that uses the login_url filter.

[DrewAPicture]

Replying to KrissieV:

Forgive me if this is not the correct place for this, I'm new to trac and testing patches. I was able to test patch 34925.2 that was in the linked ticket above and confirm it does not solve this issue as @khag7 stated. I reverted back and have been trying to apply the new patch, 35103 but am getting a "Hunk #5 FAILED at 495" error. It's likely user error, as I am really new to this, and I've done a lot of searching to find out where I'm going wrong, but can't find a solution. I'd love to help get this issue resolved however I can contribute as I have a site in development that uses the login_url filter.

The patch isn't being applied because it needs to be refreshed against current trunk due to [35897].

[DrewAPicture]

Replying to salcode:

@khag7 I really like your thought about adding a new function.

However, for a bug fix release like 4.4.1, my vote is revert back to site_url() and then revisit your patch in 4.5.

Precisely. My suggestion was in terms of a short-term fix for 4.4.1. We can always look at a more robust solution for a major release, but would prefer not to introduce new functionality in a point release. Reverting to using site_url() for this case in the interim 4.4.x branch seems like a reasonable short-term solution.

[KrissieV]

I was able to get @khag7's patch working, per @DrewAPicture's instructions, but I understand the reasons behind not implementing it in 4.4.x. I've created a patch that simply reverts the action on the form back to using site_url() as requested.

[KrissieV]

Reverts the action on the login form to site_url()

[khag7]

Good point, all, I wasn't really considering the fact that this was for a point release. Let's do what makes sense for 4.4.1 and then at some point in the future we can consider the merits of using the patch I suggested or something similar.

[salcode]

Thanks for your patch @KrissieV but this only addresses login forms generated with the function wp_login_form().

This issue also arises with the action attribute on the form tag on the login page. (e.g. ​http://example.com/wp-login.php).

If you refer to https://core.trac.wordpress.org/attachment/ticket/34925/34925.patch you can see the second change made in wp-login.php that addresses this.

This other patch also addressed rolling back the use of

wp_registration_url()

which has changed since that time to

set_url_scheme( wp_registration_url(), 'login_post' )

In the patch, it was suggested rolling this back to

site_url( 'wp-login.php?action=register', 'login_post' )

I support this change too since wp_registration_url() calls

site_url( 'wp-login.php?action=register', 'login' )

which applies the scheme login instead of the appropriate login_post scheme in this context.

While I am not aware of any bugs due to the wp_registration_url() call, I think we should roll it back anyway since the wrong scheme is being applied.

[salcode]

Refreshed version of 34925.patch

[salcode]

I'm using Git for my development and I've pushed a copy of the branch to ​https://github.com/salcode/wordpress-core/commits/login-url-filter, if that is helpful to anyone else.

Thanks.

[swissspidy]

Looks good to me. So I think the plan is as follows:

• Commit 35103.3.patch​ to 4.4.1
• Move #34925 to the 4.5 branch, as [35897] is made obsolete by this patch for 4.4.1
• Integrate something like 35103.patch​ as part of #34925

[johnbillion]

In 36042:

Login: Revert [34213] and [35897]. It has become apparent that there is a need for a separate function (and corresponding filter) which allows for the login form action URL to differ from the URL used to access the login form, so that plugins or implementations which change the login URL do not need to worry about handling the form submission at the same URL.

For now, we'll revert to the pre-4.4 behaviour of hard-coding the login form action URL as wp-login.php and look at implementing a separate function and corresponding filter in 4.5.

Props KrissieV, salcode, JPry
Fixes #34925
See #35103

[johnbillion]

In 36043:

Login: Revert [34213] and [35897]. It has become apparent that there is a need for a separate function (and corresponding filter) which allows for the login form action URL to differ from the URL used to access the login form, so that plugins or implementations which change the login URL do not need to worry about handling the form submission at the same URL.

For now, we'll revert to the pre-4.4 behaviour of hard-coding the login form action URL as wp-login.php and look at implementing a separate function and corresponding filter in 4.5.

Merges [36042] to the 4.4 branch.

Props KrissieV, salcode, JPry
Fixes #34925
Fixes #35103

[johnbillion]

@khag7 I've opened a new ticket, #35177, using your comment as its description. I'll move your patch over there.

[khag7]

Replying to johnbillion:

@khag7 I've opened a new ticket, #35177, using your comment as its description. I'll move your patch over there.

Thanks for your good work @johnbillion