Make WordPress Core

Opened 11 years ago

Closed 5 years ago

#9207 closed defect (bug) (wontfix)

redirect_to wp-admin Should Force SSL If FORCE_SSL_ADMIN is enabled

Reported by: g30rg3x Owned by: hakre
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Security Keywords: has-patch reporter-feedback
Focuses: Cc:
PR Number:


Around Lines 406 to 426 on wp-login.php:

	$secure_cookie = '';

	// If the user wants ssl but the session is not ssl, force a secure cookie.
	if ( !empty($_POST['log']) && !force_ssl_admin() ) {
		$user_name = sanitize_user($_POST['log']);
		if ( $user = get_userdatabylogin($user_name) ) {
			if ( get_user_option('use_ssl', $user->ID) ) {
				$secure_cookie = true;

	if ( isset( $_REQUEST['redirect_to'] ) ) {
		$redirect_to = $_REQUEST['redirect_to'];
		// Redirect to https if user wants ssl
		if ( $secure_cookie && false !== strpos($redirect_to, 'wp-admin') )
			$redirect_to = preg_replace('|^http://|', 'https://', $redirect_to);
	} else {
		$redirect_to = admin_url();

As we can see on the present code, if a redirection is set while login and this redirection goes to the plain version of the dashboard then client will go to the non-SSL version of the dashboard which therefore will move the client to the secure version (generating and extra request).
I know this is kinda a tongue twister sentence so i think is better to put a request example of the problem...

Client: POST http://foo.bar/wp-login.php?redirect_to=http%3A%2F%2Ffoo.bar%2Fwp-admin%2Findex.php
Server: HTTP 302 ... Location: http://foo.bar/wp-admin/index.php
Client: GET http://foo.bar/wp-admin/index.php
Server: HTTP 302 ... Location: https://foo.bar/wp-admin/index.php

I know that wordpress is actually working as suppose to work (cause we told to move to non-SSL version of the dashboard) but and a extra http request is issued.
IMHO if we (admins) have enabled FORCE_SSL_ADMIN, then all redirections to wp-admin should go SSL/HTTPs even if we fill redirect_to with the plain version of the dashboard.
There is part of the code that detect this and replace it but it has issues or well it isn't prepared to do this.
At the moment we can filter login_redirect to fix this but (again) IMHO this should move to the core...

Attachments (1)

9207.patch (2.8 KB) - added by hakre 10 years ago.

Download all attachments as: .zip

Change History (14)

#1 @Denis-de-Bernardy
11 years ago

  • Component changed from General to Optimization
  • Keywords needs-patch added; 2nd-opinion removed

#2 @janeforshort
10 years ago

  • Milestone changed from 2.8 to Future Release

Punting due to feature freeze. Reconsider with next release.

#3 @Denis-de-Bernardy
10 years ago

  • Priority changed from normal to lowest
  • Severity changed from normal to trivial

#4 @Denis-de-Bernardy
10 years ago

  • Milestone changed from Future Release to 2.9
  • Type changed from defect (bug) to enhancement

#5 @hakre
10 years ago

That code need to stripslash he value because since it was written, $_REQUEST has been changed and is slashed now.

#6 @hakre
10 years ago

  • Owner changed from anonymous to hakre
  • Status changed from new to accepted

I put this ticket on my list at for a secure wordpress setup

#7 @ryan
10 years ago

  • Milestone changed from 2.9 to Future Release

10 years ago

#8 @hakre
10 years ago

  • Component changed from Optimization to Security
  • Keywords has-patch reporter-feedback added; needs-patch removed
  • Milestone changed from Future Release to 3.0
  • Priority changed from lowest to normal
  • Severity changed from trivial to normal
  • Type changed from enhancement to defect (bug)
  • Version set to 2.9

Attached you find a patch against current head that solves the problem.

The routines were infact already in there, only the FORCE_SSL_ADMIN define was not reflected properly. While doing the review I used the time as well to make the code simpler (and therefore less errorprone).

I already tested the patch and it would be great if the original reporter still is available to test if this is fit for the reported case.

The case infact is a bug because it should have been redirected properly already as far I was able to read the code (see the user wants ssl scenario). So this is not optimization nor an enhancement. I relate this to security because SSL is a security layer.

#9 @hakre
10 years ago

Related: #11643

#10 @hakre
10 years ago

(In [12666]) Typo fix in comment. see #11643 -> patch needs an update, typo in comment as well.

#11 @nacin
9 years ago

  • Milestone changed from 3.0 to Future Release

#12 @keytuna
7 years ago

  • Cc keytuna added

Attempted to reproduce issue with version 3.5.1 of WordPress, XAMPP 1.8.1, and Curl 7.29.0.

Used default settings, attempted with this command:

curl -v -X POST http://localhost/wordpress/wp-login.php?redirect_to=http%3A%2F%2Flocalhost%2wordpress%2Fwp-admin%2Findex.php

Expected to see a 302 redirect, found a 200 OK response.

Is anyone else still able to reproduce this issue?

#13 @nacin
5 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

In trunk, here's what happens:

This is proper behavior. That said, it is still possible to go to https://wordpress/wp-login.php?redirect_to=http://wordpress/wp-admin/, which will indeed end up with the double-redirect being described.

The reason is because $secure_cookie is set to '' under a forced SSL situation, and only set to true when we're not forcing SSL but the user's preference is an SSL experience. (This user preference is largely a dead concept as of #10267.) The authentication functions then set $secure_cookie to true when '' and under is_ssl(). Trusting if ( $secure_cookie ) rather than if ( force_ssl_admin() ) thus triggers a double-redirect for forced SSL.

This is actually by design, though. This happened in [8701] simply to ensure we are following the user's preference. Since this no longer (never?) can happen normally (as going to http wp-admin redirects to https wp-admin before redirecting to wp-login), there's nothing to fix here. Closing out.

Note: See TracTickets for help on using tickets.