Skip to:

Opened 11 years ago

Closed 4 weeks ago

#4822 closed defect (bug) (invalid)

Login redirects to https when FORCE_ADMIN_SSL enabled

Reported by: will_c's profile will_c Owned by:
Milestone: Priority: normal
Severity: normal Version: 1.7
Component: Core Keywords: reporter-feedback


Using WP 3.5 and BP 1.7 trunk, I am experiencing issues where the user is redirected to an https version of the site when logging in (initially incorrectly) using the sidebar login widget. I believe I have tracked down these issues to problems with bp_core_login_redirect() and how bp_get_root_domain() handles retrieving the site url during login. In my mind this is a fairly significant bug because of the potential complications with cross domain requests.

To replicate this issue:

  2. Use the bp-default theme
  3. Attempt to login from the homepage (or any other page) using incorrect credentials. This will redirect you to https://[your url]/wp-login.php.
  4. Input the correct credentials and you will be redirected to https://[your site].com

4a. This issue also appears if you manually add a relative redirect_to value rather than an absolute one to the login url. This patch does not fix this issue.

As far as I can tell, this issue stems from more serious issues with how WP core deals with FORCE_ADMIN_SSL and is part of a much larger problem. The issue is that when the user is on the wp-login page, they are using https, and at that point, home_url(), site_url(), network_site_url(), etc. all return the https variant of the site url. It seems that this issue stems from the is_ssl() function that simply checks the $_SERVER global for 'HTTPS' and 'HTTPS_PORT', which at that point is true. This becomes a problem because bp_get_root_domain calls either $bp->root_domain or bp_core_get_root_domain() - which calls get_home_url(). Both of these return the https variant of the site url.

My proposed solution would add a check when a user isn't logged in to see if a redirect_to variable had been passed to wp-login.php. If it hasn't, it sets the redirect_to variable to wp_get_referer() so that the user is redirected back to where they came from using the same protocol that they had been using on the front end. I also added checks to prevent redirect loops. This should work with plugins that modify the $redirect_to variable themselves (as long as they use absolute urls).

With this new solution, I wasn't able to find a test case that would then proceed in bp_core_login_redirect() down to bp_get_root_domain(), so I would be fine getting rid of it, but kept it in for now.

Attachments (1)

ssl_login_patch.diff (637 bytes) - added by will_c 11 years ago.

Download all attachments as: .zip

Change History (7)

#1 @boonebgorges
11 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

I'm wary of putting hacks (relatively speaking) into place to fix an issue that should be solved upstream. If it's an issue with wp-login.php redirects, then you're quite correct that it should be fixed in WP itself. Are you aware of a WP ticket addressing the underlying problem?

FWIW, I'd hope that admins who have enabled an SSL and are using FORCE_ADMIN_SSL are also savvy enough to know that, in these kinds of cases, it's pretty trivial to set up a blanket https->http redirect for non-wp-admin pages, with an .htaccess rule. will_c, if you've written such a rule in the past, I encourage you to create a page on the WordPress or BuddyPress codex that explains its use and purpose. For the time being, that's really the most reliable solution.

In the longer run, if WP doesn't fix this, I don't have a problem doing some workarounds in BP. But I think we can do better than to simply force wp_get_referer() as the fix. It seems better to me to detect the presence of a redirect_to parameter, and to check whether it's http or https, and then to modify the URL appropriately based on SSL settings before reforwarding. That'll result in less unexpected behavior, especially for plugins that supply custom redirect_to after login.

There's not much we can do for the case of relative URL redirects, I'm afraid. Are they being caused by BP or by some other plugin?

#2 @r-a-y
11 years ago

Just taking a quick look at will_c's patch and he actually duplicates a bit of code in bp_core_login_redirect() and moves it into the logged-out conditional.

I agree that we should probably use the passed $redirect_to variable for checks instead of wp_get_referer().

This ticket gives us a reason to audit this function. (Also see #4199 for historical context.)

#3 @will_c
11 years ago

Thanks for the comments r-a-y and boonebgorges. I agree the less hacked the better, but I do think that keeping the $redirect_to may lead to more predictable results.

Apologies taking so long responding to this, but you're right boonebgorges that it's pretty trivial (in theory) to setup a blanket redirect for https. I'm creating a codex page now, but would appreciate if anyone has feedback for my redirect code. It's been working for us in production for a while now, but realize that there are probably edge cases we're not encountering. Here's the link:

Happy to help if you all decide to go forward and revisit this issue in the future.

#4 @DJPaul
11 years ago

  • Keywords reporter-feedback added; has-patch dev-feedback needs-testing needs-unit-tests removed

Did this ever get an upstream bug report? If so, which ticket? :)

#5 @will_c
11 years ago

I never created an upstream bug report, but could definitely do so if you think it would be useful. I guess the issue would be the framing. For WP purposes, the current behavior works as intended. It is tricky for plugins like BP that want to redirect users to the frontend on login, but I'm not sure how WP deals with edge cases like these. Do you have a sense that this is something they'd be interested in tackling?

I've never submitted to WP Trac before, but am willing to give it a shot.

#6 @espellcaste
4 weeks ago

  • Milestone Awaiting Contributions deleted
  • Resolution set to invalid
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.