Page MenuHomePhabricator

Varnish: Mobile site redirect interferes with OAuth authorization process
Open, NormalPublic

Description

Author: veradekok

Description:
Hi,

I have to always authorize the Flickr2Commons tool before using it. I also use this tool on my smartphone. When I've set the user agent of the browser I use (Habit Browser) to be Android, I get redirected to a blanc page after clicking on "allow". This is not the case when I set the browsers user agent to PC.


Version: wmf-deployment
Severity: normal
OS: other
Platform: Smartphone

Details

Reference
bz72186

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 3:46 AM
bzimport set Reference to bz72186.
bzimport added a subscriber: Unknown Object (MLST).
bzimport created this task.Oct 17 2014, 3:23 PM

veradekok wrote:

Correction: the blank page says "Error retrieving token: mwoauthdatastore-request-token-not-found"

What is the exact user agent string on the phone? Do you literally refer to the string "PC"? Exact strings welcome in order to reproduce...

veradekok wrote:

The user agent resulting in the error, eg. "Android":

Mozilla/5.0 (Linux; U; Android 4.0.1; ja-jp; Galaxy Nexus Build/ITL41D) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30

The "PC" user agent not resulting in error:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_3; ja-jp) AppleWebKit/533.16 (KHTML, like Gecko) Version/5.0 Safari/533.16

Verified, when editing from a user-agent that would be redirected to the mobile site, after authorizing, page gets redirected to mobile site and errors.

E006


Steps to reproduce:
*From a cell phone, where you have not opted out of mobile interface and it sends a mobile UA
*Go to http://tools.wmflabs.org/oauth-hello-world/index.php?action=authorize
*Follow instructions, eventually get an error.

(In reply to Bawolff (Brian Wolff) from comment #4)

Verified, when editing from a user-agent that would be redirected to the
mobile site, after authorizing, page gets redirected to mobile site and
errors.

I had suspected this, thanks for confirming.

After some further investigation, it looks to me like it's actually a bug in WMF's varnish layer trying to redirect mobile clients to the mobile site: it doesn't hit on the first request that uses /w/index.php?title=Special:OAuth/authorize&oauth_token=abc123&oauth_consumer_key=abc123, but the POST back to /wiki/Special:OAuth/authorize is caught and since browsers treat a 302 redirect as 303 rather than 307 this breaks everything. The relevant code appears to be in the operations/puppet repo, templates/varnish/text-frontend.inc.vcl.erb, sub mobile_redirect.

Change 167453 had a related patch set uploaded by MaxSem:
Perform mobile redirect only for GET requests

https://gerrit.wikimedia.org/r/167453

(In reply to Brad Jorsch from comment #5)

(In reply to Bawolff (Brian Wolff) from comment #4)

Verified, when editing from a user-agent that would be redirected to the
mobile site, after authorizing, page gets redirected to mobile site and
errors.

I had suspected this, thanks for confirming.
After some further investigation, it looks to me like it's actually a bug in
WMF's varnish layer trying to redirect mobile clients to the mobile site: it
doesn't hit on the first request that uses
/w/index.php?title=Special:OAuth/
authorize&oauth_token=abc123&oauth_consumer_key=abc123, but the POST back to
/wiki/Special:OAuth/authorize is caught and since browsers treat a 302
redirect as 303 rather than 307 this breaks everything. The relevant code
appears to be in the operations/puppet repo,
templates/varnish/text-frontend.inc.vcl.erb, sub mobile_redirect.

Yep, this is actually a known issue.

Mobile can't really redirect calls to /w/index.php, so OAuth app authors need to redirect their users to "/wiki/Special:OAuth/authorize?oauth_token=..." instead of "/w/index.php?title=...".

So Max'es patch will probably work, although then the login experience on mobile isn't great. And we'll have to make sure the centralauth handshake continues to work. Or OAuth app authors can use /wiki/Special:OAuth urls, and the experience is better, but we can't control their code. Or we make a special varnish rule to allow mobile redirecting for this specific url pattern ("/w/index.php?title=Special:OAuth/authorize")... But I haven't fully thought through what else that would impact.

I thought we had trouble with OAuth getting confused by internal rewriting somewhere that changed /wiki/Special:OAuth to /w/index.php?title=Special:OAuth and broke the signature validation. Did that get fixed?

(In reply to Brad Jorsch from comment #8)

I thought we had trouble with OAuth getting confused by internal rewriting
somewhere that changed /wiki/Special:OAuth to
/w/index.php?title=Special:OAuth and broke the signature validation. Did
that get fixed?

Yeah, that's one of the confusing parts. For any calls that are signed, that is the case, so title=Special:OAuth is the best format for the url. The /authorize call is the only one not signed, since it's just redirecting the user, so the clean url can be used.

Change 167453 merged by Faidon Liambotis:
Perform mobile redirect only for GET and HEAD requests

https://gerrit.wikimedia.org/r/167453

Is this still an issue after the (long since) merge of that gerrit changeset?

ArielGlenn set Security to None.

Is this still an issue after the (long since) merge of that gerrit changeset?

That change didn't affect this particular issue. This issue occurs because the mobile redirect only happens for /wiki/ urls, but to work around some badly designed OAuth libraries (like the one in phabricator...), server-side OAuth calls have to use /w/index.php?title=Special:OAuth, so they redirect the user to the same url structure for the authorization, and mobile doesn't redirect them.

So, Chris, what needs to happen here?

I think line 61 should become something like,

&& ( req.url ~ "^/(wiki|(gan|ike|iu|kk|ku|shi|sr|tg|uz|zh)(-[a-z]+)?)[/\?]"
    || req.url ~ "^/w/index.php\?title=Special:OAuth/authorize" ) ) {

I think line 61 should become something like,

&& ( req.url ~ "^/(wiki|(gan|ike|iu|kk|ku|shi|sr|tg|uz|zh)(-[a-z]+)?)[/\?]"
    || req.url ~ "^/w/index.php\?title=Special:OAuth/authorize" ) ) {

Uh, so you want to redirect some ugly URLs to mobile? I thought the opposite is what is needed. Also, special page names (or at least the Special: namespace) can be localised, rendring this regex unmanageable.

Uh, so you want to redirect some ugly URLs to mobile? I thought the opposite is what is needed. Also, special page names (or at least the Special: namespace) can be localised, rendring this regex unmanageable.

Right, that's why this is really best to push onto the app owner to implement. Unfortunately, things like Phabricator's OAuth library are horrible broken, but upstream won't accept patches to let us work around this issue... so we may need to do something ugly on our end too.

fgiunchedi added a subscriber: fgiunchedi.

Adding Traffic for visibility

ema moved this task from Triage to Caching on the Traffic board.Dec 5 2016, 11:46 AM