Page MenuHomePhabricator

$wgSecureLogin does not redirect to http if wpStickHTTPS is unchecked
Closed, ResolvedPublic

Description

When using $wgSecureLogin, if a user leaves wpStickHTTPS unchecked, they are stil redirected to an https page after login.

I think it's because getFullURL returns a protocol relative url by default now, so preg_replace( '/^https:/', 'http:', $redirectUrl ) has no effect.

(NB: fixing this seems to prevent a user from logging in without wpStickHTTPS checked, because their session cookies are set with the secure attribute, but they are immediately redirected to an insecure page, so their session cookie no longer exists in the request.)


Version: 1.20.x
Severity: major

Details

Reference
bz40541

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 1:12 AM
bzimport set Reference to bz40541.

Can we use insecure cookies when wpStickHTTPS is not checked?

I'll work on this if nobody else is, since I'm the one who filed the bug this blocks.

Thanks Tyler. The redirect should be an easy fix. The cookies might be easier after gerrit 24026, or there might be a better way to do it.

Well, unfortunately it's not that easy a fix. I'm working on it now, but the big problem is that say, for example, a hook injects HTML into the login complete page. The redirect does not occur automatically, and instead the user is given a sort of "login success" page and a link returning to where they were. The returnto link is added using OutputPage::addReturnTo, which in turn uses the Linker, and in that workflow there is no way to change the protocol. Right now I'm working on adding an option to the Linker so that you can force the link to have a certain protocol.

I just added a patch for the cookie handling, that I think will allow insecure links/redirects to work. Gerrit 25525.

Roan pointed out that gerrit 25530 results in an infinite redirect if $wgServer includes an http:// protocol. This is because when $wgServer includes the protocol, it's assumed by the code that this means that there is no ssl available, so an http:// url is returned from wfExpandUrl(), even when PROTO_HTTPS is given.

I added an ugly hack to wfExpandUrl in gerrit 25721 to ignore this assumption if the site owner has enabled $wgSecureLogin. However, I think it would be best to back up and actually define a couple of things:

  1. What does it mean if $wgServer starts with http://? Is it that there is no ssl available / intended, and MediaWiki should never generate an https link? Or should we assume that it's a default configuration, and we can override it if other settings like $wgSecureLogin are set?
  1. If $wgServer starting with http:// means that the site should not use tls, but a site also includes $wgSecureLogin set to true, what *should* this mean to mediawiki? Should it be treated as an error? Or should one of the settings be overridden?

My preference for #2 is that we assume that the most secure preference is desired. I wouldn't even mind changing the protocol of $wgServer to be relative if $wgSecureLogin is true in Setup.php. But again, I'm not sure all of the places where assumptions are made about #1 in the code, and in our user base.

(In reply to comment #7)

I added an ugly hack to wfExpandUrl in Gerrit change #25721 to ignore this assumption
if the site owner has enabled $wgSecureLogin. However, I think it would be best
to back up and actually define a couple of things:

  1. What does it mean if $wgServer starts with http://? Is it that there is no

ssl available / intended, and MediaWiki should never generate an https link? Or
should we assume that it's a default configuration, and we can override it if
other settings like $wgSecureLogin are set?

Honestly, if MediaWiki is explicitly requesting an HTTPS link, I think it is erroneous to produce something different, especially with something as ambiguous as what $wgServer's scheme is. I think PROTO_HTTPS should give https regardless of $wgServer.

In reality, I would love to just finish https://gerrit.wikimedia.org/r/22167 and stop using wfExpandUrl in new code altogether.

(In reply to comment #8)

Honestly, if MediaWiki is explicitly requesting an HTTPS link, I think it is
erroneous to produce something different, especially with something as
ambiguous as what $wgServer's scheme is. I think PROTO_HTTPS should give https
regardless of $wgServer.

I totally agree, in theory. However, even when I do that, if I load a page over ssl on a site where $wgServer starts with http://, all of the links to load.php and api.php use http instead of https. So the assumption that $wgServer can be used to generate urls with the correct protocol seems to be used all over the place.

I tested out just setting $wgServer to be protocol-relative if $wgSecureLogin is true, and it's working well. I don't like the idea of secretly flagging or changing a user's config, but in this case it seems like it may make sense to fix up conflicting config options?

Yeah, I mean if a user is turning on wgSecureLogin (or a similar option), they should already be aware (hopefully) that this implies HTTPS. At the very least, the bug of a infinite redirect will be replaced with that of getting an error when you try to browse HTTPS when none exists.

Closing this as resolved since the actual bug has indeed been fixed. Opened a separate report (bug 40679) for the redirect issue.

I don't understand on what release this is supposed to be fixed and at what conditions ([[mw:Manual:$wgSecureLogin]] says nothing).
wiki.openstreetmap.org is at 1.20.3 (beb501d) and I'm always stuck on HTTPS with $wgSecureLogin = true; $wgServer = '//wiki.openstreetmap.org';

Indee this seems to be hitting us again, see Chris and Tim on https://gerrit.wikimedia.org/r/#/c/79960/
4eopening this because the fix never worked for me on released versions of MW and its status has to be checked.

That change was merged on the 21st, but the "included in" info on the change only says wmf12. We're on 13/14 now.

Chris/Chad: is this live on the testwikis+mediawiki.org. We have another report of the issue from S in bug 53379.

(In reply to comment #14)

That change was merged on the 21st, but the "included in" info on the change
only says wmf12. We're on 13/14 now.

Chris/Chad: is this live on the testwikis+mediawiki.org. We have another
report
of the issue from S in bug 53379.

I can't comment as to what is deployed, but I tested testwiki+mediawiki earlier this week and the specific issue mentioned at the top of this bug report doesn't occur.

This is different from S's bug. The functionality is now present in both core and centralauth with the patches that were merged into master, and deployed to wmf14 by Chad on Saturday.

After bug 52283, we no longer have the wpStickHTTPS checkbox on the login form, instead we use the user's originating page + preference + geoip to make the determination.

So what's the alternative to $wgSecureLoginDefaultHTTPS now? What happens (now and in 1.21) if $wgSecureLogin is true but $wgServer is http:// ?

(In reply to comment #17)

So what's the alternative to $wgSecureLoginDefaultHTTPS now?

Nothing, because it doesn't make sense anymore. $wgSecureLoginDefaultHTTPS was made to decide what the default value of the mStickHTTPS checkbox was. However, that checkbox doesn't exist anymore, so there's no purpose in having a default value for it.

Whether you're sent to HTTPS, rather than being decided by a checkbox, is decided by a combination of what protocol you were previously browsing and the user preference prefershttps.

What happens (now
and in 1.21) if $wgSecureLogin is true but $wgServer is http:// ?

I forget what release it was in, but having $wgSecureLogin as true and $wgServer with an HTTP protocol is considered a configuration error, so a patch was added that automatically shuts off $wgSecureLogin in Setup.php if that condition matches.

(In reply to comment #18)

(In reply to comment #17)

So what's the alternative to $wgSecureLoginDefaultHTTPS now?

Nothing, because it doesn't make sense anymore. $wgSecureLoginDefaultHTTPS
was
made to decide what the default value of the mStickHTTPS checkbox was.
However,
that checkbox doesn't exist anymore, so there's no purpose in having a
default
value for it.

Whether you're sent to HTTPS, rather than being decided by a checkbox, is
decided by a combination of what protocol you were previously browsing and
the
user preference prefershttps.

What are the combinations?

What happens (now
and in 1.21) if $wgSecureLogin is true but $wgServer is http:// ?

I forget what release it was in, but having $wgSecureLogin as true and
$wgServer with an HTTP protocol is considered a configuration error, so a
patch
was added that automatically shuts off $wgSecureLogin in Setup.php if that
condition matches.

Yes, that's what the configuration suggests. I still don't understand how one is supposed to send logged in users to http after the secure login.

(In reply to comment #19)

What are the combinations?

  1. If you have prefershttps turned on, you always go to HTTPS.
  2. If you don't have prefershttps turned on, you are sent to whatever protocol you were originally on. In other words, if you came from HTTP, you go back to HTTP. If you came from HTTPS, you go back to HTTPS.

(In reply to comment #20)

(In reply to comment #19)

What are the combinations?

  1. If you have prefershttps turned on, you always go to HTTPS.
  2. If you don't have prefershttps turned on, you are sent to whatever

protocol
you were originally on. In other words, if you came from HTTP, you go back to
HTTP. If you came from HTTPS, you go back to HTTPS.

The full set of options I was using while implementing this ended up as:

preferhttps geoip-https browsing final protocol
true no http http
true no https https
true yes http https
true yes https https
false no http http
false no https https (w/ insec cookies)
false yes http http
false yes https https (w/ insec cookies?)

OSM upgraded to 1.21.2 (404e36a) and fixed https://trac.openstreetmap.org/ticket/3919 ; leaving the checkbox to stay on HTTPS successfully keeps me on HTTP.