Page MenuHomePhabricator

Prototypes don't work on Safari
Closed, ResolvedPublic

Description

Api queries to restbase mobile-sections from iOS/Safari doesn't work when the page is a redirect, since the browser sends an Accept-Encoding header after the 302 but that is not allowed by the cors Access-Control-Allow-Headers which only reports Accept as a valid header to set.

As a result the browser fails to make the request.

See http://jsbin.com/cuqula/edit?js,output for broken example.

In Safari < 10 the redirect is being preflighted with an OPTIONS request with Access-Control-Request-Headers: accept-encoding, origin, accept-language but restbase responds with Access-Control-Allow-Headers: accept, content-type, which causes the request to fail (see wikimedia/restbase/lib/security_response_header_filter.js#L62)

Fixes

  • Temporarily use a CORS proxy for Safari browsers
  • Add the Safari sent headers to Access-Control-Allow-Headers in restbase

Event Timeline

If we care about safari/ios for the user research, this should be high prio.

Jhernandez updated the task description. (Show Details)Oct 17 2016, 8:26 AM
atgo moved this task from Backlog to Doing on the New-Readers board.Oct 17 2016, 9:13 AM
atgo triaged this task as Medium priority.Oct 17 2016, 3:19 PM
atgo moved this task from Doing to Epics in progress on the New-Readers board.

I'm not sure that we do particularly care about safari/iOS, we should get OS stats from these countries... but the research did show mostly Android.

Jhernandez updated the task description. (Show Details)Oct 17 2016, 5:19 PM

Seems to be Safari < 10 only

joakino> Joaquin mobrovac: has there been any issues with the cors headers from safari on restbase before? when i make a request from safari or ios the redirect breaks because safari sends 'accept-encoding' automatically after the 302 http://jsbin.com/cuqula/edit?js,output
19:03:48 ^ that page doesn't work with safari
19:04:11 see console for Request header field Accept-Encoding is not allowed by Access-Control-Allow-Headers.
19:07:52 <+mobrovac> joakino: i don't see anything in jsbin's console, and i don't have safari
19:07:55 <Pchelolo> joakino: works on safari 10 for me.

@atgo If we need to we have a way to work around this, just fyi.

(using a cors proxy for safari like http://crossorigin.me/ or a custom one.

Jhernandez lowered the priority of this task from Medium to Lowest.Oct 18 2016, 9:44 AM

I've wrapped the api requests for safari in a cors proxy I deployed myself to be sure there was nothing malicious going on.

In chat the services folk mentioned fixing the headers on the rest api, so let's keep this open for now, but with lower priority since a workaround is in place.

Jhernandez updated the task description. (Show Details)
Jhernandez added subscribers: Pchelolo, GWicke.

Sounds good, thanks @Jhernandez. Also iOS is low priority for this round most likely.

Some irc log from yesterday:

<joakino> mobrovac: has there been any issues with the cors headers from safari on restbase before? when i make a request from safari or ios the redirect breaks because safari sends 'accept-encoding' automatically after the 302 http://jsbin.com/cuqula/edit?js,output
<joakino> ^ that page doesn't work with safari
<joakino> see console for Request header field Accept-Encoding is not allowed by Access-Control-Allow-Headers.
<+mobrovac> joakino: i don't see anything in jsbin's console, and i don't have safari
<Pchelolo> joakino: works on safari 10 for me.
<joakino> hrm may be safari 9 is the problem
<joakino> yep, i have safari 9.1
<joakino> grr
<Pchelolo> joakino: gr, no way to install older safari on a mac and browserstack gives safari 9 only in a paid plan...
<joakino> damn
<joakino> that may be why some iphones were having issues and others didn't
<+gwicke> it's odd that it complains about that header when the request isn't trying to set or read it explicitly
<joakino> i'm not sure why it complains only when following the redirect
<joakino> apparently with safari the redirect is being preflighted with OPTIONS and Access-Control-Request-Headers: accept-encoding, origin, accept-language but restbase responds with Access-Control-Allow-Headers: accept, content-type, which causes the request to fail
<Pchelolo> joakino: that's set up in https://github.com/wikimedia/restbase/blob/master/lib/security_response_header_filter.js#L62
<Pchelolo> I think we can safely add the headers requested by safari to the list
<joakino> i'm not very experienced with csp, but that would be great
<gwicke> at first sight I don't see much harm in whitelisting accept-encoding too
<gwicke> lets just create a PR and CC Darian?
GWicke raised the priority of this task from Lowest to High.Oct 18 2016, 2:30 PM
GWicke added a project: Services (next).

Upped priority, as this should be easy and safe to do.

Pchelolo moved this task from next to doing on the Services board.Oct 18 2016, 10:22 PM
Pchelolo edited projects, added Services (doing); removed Services (next).

The fix has been deployed in beta cluster. @Jhernandez could you please retest with some page on https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page ? I don't have safari 9 to test, sorry. If all's good we will deploy to production.

I've tested it in OSX 10.11 Safari 9.1.1 and iOS simulator 8.3 Safari by using this test (http://output.jsbin.com/cuqula/8) querying a url on beta cluster.

It works sometimes and sometimes it fails. When it works it all looks fine, when it doesn't it doesn't give any information as of why.

I also uploaded an example here http://safari-restbase-cors.surge.sh just in case jsbin was doing something funny, but same results.

Okey, we've figured out what the problems was, and our initial solution was incorrect, so I've create a PR to revert it: https://github.com/wikimedia/restbase/pull/691

The problem is also reproducible in latest Chrome, and actually we can't fix it. The issue is that according to CORS specifications, redirects are not allowed for requests that require an OPTIONS preflight requests (see http://stackoverflow.com/a/38810391/1924141). So, in the example at the pastebin a content-type: application/json header is set for a request, making it require a preflight.

The question is why do you need to set that header? What's the practical example where you require to set some header on the GET request other then Accept header?

I'm inclined to close this ticket as invalid.

@Pchelolo The content-type is unnecessary and we can remove it. Apparently I forgot about it there from some tests I was doing.

If you remove it, it doesn't work either, but because other not allowed headers being sent.

I've updated the pastebin and raw html to not send that header:

This are the details of the failure:

Normal window

Cache-control header is being sent but is not allowed:

Incognito window

DNT header is being sent and is not allowed:


Maybe that is why it was working when shift+refreshing, because safari doesn't send the cache-control header and that actually matches what was done in the PR, but if you load/refresh normally then the cache-control header is sent and it breaks since that PR didn't allow it.
(I can't actually see the requests when it works because devtools collapses the internal redirects into just one row on the network pane).

Also in incognito mode the DNT header is set so if that isn't allowed then requests on those modes will fail.


I hope all this helps 🍪

@Jhernandez, it's fairly odd that UA-controlled headers like Cache-Control or DNT would run into CORS header restrictions. Is this the underlying Safari bug?

@Jhernandez I've live-hacked beta, now it has to allow cache-control and dnt. Could you redo re test? Apparently there's just no way of installing Safari 9 on Sierra

PR merged, to be deployed early next week. Moving to done, will close when deployed and confirmed.

Pchelolo closed this task as Resolved.Oct 24 2016, 5:50 PM

RESTBase deployed, resolving.

Thanks all. I've removed the cors proxy from the prototypes code and tested it with en.wiki and seems to behave fine in Safari 9.