Page MenuHomePhabricator

Mobile redirect strips https
Closed, ResolvedPublic

Description

When I access https://en.wikipedia.org on my cellphone, the redirector sends me to http://en.m.wikipedia.org but not the https one.


Version: unspecified
Severity: normal

Details

Reference
bz35215

Related Objects

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:18 AM
bzimport added projects: HTTPS, acl*sre-team.
bzimport set Reference to bz35215.
bzimport added a subscriber: Unknown Object (MLST).

Good find. We should fix that as https://en.m.wikipedia.org works just fine.

Removing dependency to bug 27946 which is secure.wikimedia.org.

Can we make this happen sooner rather than later? Does someone need to be poked in RT?

Depending on redirect method, possible dupe/related to bug 31369.

This is actually being worked on as a part of the updates to redirector.c (https://mingle.corp.wikimedia.org/projects/mobile/cards/194). Hopefully this will be resolved in production next week or the week following.

I don't see how this can be fixed in redirector, as it can't redirect https URLs at all, but since it redirects, apparently it receives URLs with HTTPS already stripped.

Interest(In reply to comment #8)

I don't see how this can be fixed in redirector, as it can't redirect https
URLs at all, but since it redirects, apparently it receives URLs with HTTPS
already stripped.

Squid handles the invocation of the redirector code - the ACLs for mobile redirection live on fenari in /home/w/conf/squid/mobile_acls.conf (I'm not sure off the top of my head what git repo this might exist in), and redirector is defined as the url rewrite program in that file as well. If something is stripping https and sending 'http' to redirector, it's almost certainly in squid somewhere. Asher or Patrick may know more.

Figured it out: because our HTTPS servers act as proxies, the squids receive every request already via HTTP. MediaWiki detects the protocol with isset( $_SERVER['HTTP_X_FORWARDED_PROTO'] ) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https' however I'm not sure if Squid can pass enough information to the redirector for it to figure out the protocol.

The solution (admittedly, ugly one) is to make the redirector redirect to http or https depending on command-line argument and have two sets of redirection rules, one for http and another for https. In such case, the bottom of mobile_acls.conf would look something like that:

acl forwarded_https req_header X-Forwarded-Proto https

url_rewrite_access allow mobile wpcontent !nomobile !nomobileua !mobtrial !forwarded_https
url_rewrite_access allow wap wpcontent !nomobile !nomobileua !mobtrial !forwarded_https
url_rewrite_access allow mobile googlebot !forwarded_https
url_rewrite_program /usr/local/bin/redirector
url_rewrite_children 100
url_rewrite_concurrency 200

url_rewrite_access allow mobile wpcontent !nomobile !nomobileua !mobtrial forwarded_https
url_rewrite_access allow wap wpcontent !nomobile !nomobileua !mobtrial forwarded_https
url_rewrite_access allow mobile googlebot forwarded_https
url_rewrite_program /usr/local/bin/redirector --https
url_rewrite_children 100
url_rewrite_concurrency 200

  • Bug 42605 has been marked as a duplicate of this bug. ***

Max, run your changes by ops and see what they have to say. Even mailing to the ops list might be a good approach to get the changes made sooner rather than later.

Arthur brought the request up in the ops@ mailing list.

Quoting Asher Feldman's reply:
"The proposed solution in the bugzilla ticket is to run an additional 100
redirector processes on every squid just to handle https mobile redirects.
I don't like that. It will be easy to do cleanly once text has been
migrated to varnish, hopefully in 2013Q2."

mwalker wrote:

In theory, when someone accesses HTTPS they're doing it because they're concerned about their privacy/security. There may be concerns that their mobile carrier is injecting things, or that some national agency is monitoring their communications, or ??? It doesn't really matter though -- a user has requested we respect their privacy and then we go ahead and ignore it. Even beyond that; this is seriously unexpected behavior and it's not announced to the user that this is happening. This should be fixed immediately.

It also causes issues with other extensions being deployed to the mobile site. E.g. CentralNotice when it makes its RecordImpression call.

It's stupid to hang this bug on varnish maybe, or maybe not, having been deployed. Given that this is 2013Q2 apparently the varnish deploy has been pushed back.

! Let's fix this by redirecting everything to HTTPS. !

Not only is redirecting everything to HTTPS respectful to users wishes; it's also not unexpected -- HTTP to HTTPS promotion is commonly seen and does not cause CORS issues.

Comment in RT #2635 by Asher Feldman is:

"This has been a known issue since day 1 and will not be addressed while squid is still in use for article caching. It will be addressed when squid and the current redirector are both retired. This has no bearing on mobile login, or on any features being developed by the mobile team."

  • Bug 31334 has been marked as a duplicate of this bug. ***

I can no longer reproduce the problem in the original report in Firefox Beta and Opera Mini on Android.

On each app, I cleared the app data, clicked on the link https://en.wikipedia.org as appears on this bug, and was directed to https://en.m.wikipedia.org/wiki/Main_Page

Well done!

Was this fixed as part of another piece of work? Are there outstanding issues?
If it is completely fixed, this should be disseminated in a tech notes if it hasnt been done already.

(In reply to comment #18)

Was this fixed as part of another piece of work?

The move to varnish. (I guess) AIUI, squid is no longer in active use. (except maybe brewster?)

Are there outstanding
issues?

bug 31369 should be mostly still broken. (did ~3 mins of testing just now)

Also, just found a (possibly new) issue: bug 57132

I'm not sure if the mobile redirect already has tests but definitely deserves some :)

If it is completely fixed, this should be disseminated in a tech notes if it
hasnt been done already.

Looks like this is where the magic happens:
https://git.wikimedia.org/blob/operations%2Fpuppet.git/f3db6e91cdefa91f27e04f639f9de8146d1d14a5/templates%2Fvarnish%2Ftext-frontend.inc.vcl.erb#L23