Bug 129681

Summary: [GTK] Too many redirects visiting www.globalforestwatch.org
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, commit-queue, dpino, gns, gyuyoung.kim, mrobinson, rakuco, sergio, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.globalforestwatch.org
Attachments:
Description Flags
http://www.globalforestwatch.org/ loaded in MiniBrowser using Chrome's UA.
none
Loading page in Firefox using MiniBrowser's UA.
none
Patch
none
Patch none

Description Sergio Villar Senin 2014-03-04 04:58:41 PST
It actually redirects first to http://www.globalforestwatch.org/notsupportedbrowser and then the error happens. Maybe it's just an UA issue + some problem in the server, but we need to double check it.
Comment 1 Diego Pino 2014-03-12 07:49:10 PDT
I monitored the request with Wireshark and this is the output:

GET / HTTP/1.1
Host: www.globalforestwatch.org
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/537.30 (KHTML, like Gecko) Safari/537.30
Accept-Encoding: gzip, deflate
Accept-Language: en-us
Connection: Keep-Alive

HTTP/1.1 302 Found 
Cache-Control: no-cache
Content-Encoding: gzip
Content-Type: text/html; charset=utf-8
Date: Wed, 12 Mar 2014 14:32:27 GMT
Location: http://www.globalforestwatch.org/notsupportedbrowser
Server: WEBrick/1.3.1 (Ruby/2.1.0/2013-12-25)
Vary: Accept-Encoding
X-Frame-Options: ALLOWALL
X-Request-Id: e8a845bb-7bad-4e82-b35f-41132603e6a9
X-Runtime: 0.004316
Content-Length: 123
Connection: keep-alive

.....o S....A.. ......@.....X...kI.i....s.l.....;.r.R................c.....<....E+.b..C..d...KR....'.J..-...........
.3v...GET /notsupportedbrowser HTTP/1.1
Host: www.globalforestwatch.org
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/537.30 (KHTML, like Gecko) Safari/537.30
Accept-Encoding: gzip, deflate
Accept-Language: en-us
Connection: Keep-Alive

HTTP/1.1 302 Found 
Cache-Control: no-cache
Content-Encoding: gzip
Content-Type: text/html; charset=utf-8
Date: Wed, 12 Mar 2014 14:32:27 GMT
Location: http://www.globalforestwatch.org/notsupportedbrowser
Server: WEBrick/1.3.1 (Ruby/2.1.0/2013-12-25)
Vary: Accept-Encoding
X-Frame-Options: ALLOWALL
X-Request-Id: 24c51c74-bc63-4258-9639-5c991298c905
X-Runtime: 0.004940
Content-Length: 123
Connection: keep-alive

.....o S....A.. ......@.....X...kI.i....s.l.....;.r.R................c.....<....E+.b..C..d...KR....'.J..-...........
.3v...GET /notsupportedbrowser HTTP/1.1
Host: www.globalforestwatch.org
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/537.30 (KHTML, like Gecko) Safari/537.30
Accept-Encoding: gzip, deflate
Accept-Language: en-us
Connection: Keep-Alive

And this keeps going on and on until the "Too many redirects" error happen.

I think this a problem related to the site. If I request the root page using curl:

$ curl  http://www.globalforestwatch.org 

<html><body>You are being <a href="http://www.globalforestwatch.org/notsupportedbrowser">redirected</a>.</body></html>

If now I follow the redirection and request "http://www.globalforestwatch.org/notsupportedbrowser":

$ curl http://www.globalforestwatch.org/notsupportedbrowser

<html><body>You are being <a href="http://www.globalforestwatch.org/notsupportedbrowser">redirected</a>.</body></html>

It seems clear that "http://www.globalforestwatch.org/notsupportedbrowser" should not check whether the browser is supported or not, because otherwise it creates a redirection infinite loop.
Comment 2 Sergio Villar Senin 2014-03-12 08:46:47 PDT
> It seems clear that "http://www.globalforestwatch.org/notsupportedbrowser" should not check whether the browser is supported or not, because otherwise it creates a redirection infinite loop.

I mean, the site clearly has some kind of server side issue. What I'd like to know is if there is some mistake by our side. One interesting test would be to change the UA so that we claim to be one of the supported browsers and check what happens.
Comment 3 Diego Pino 2014-03-12 09:04:33 PDT
Created attachment 226512 [details]
http://www.globalforestwatch.org/ loaded in MiniBrowser using Chrome's UA.

I changed the UA of MiniBrowser to the UA of Chrome (which works fine).

"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.102 Safari/537.36"

The page was successfully loaded. Snapshot attached.
Comment 4 Sergio Villar Senin 2014-03-12 09:24:44 PDT
(In reply to comment #3)
> Created an attachment (id=226512) [details]
> http://www.globalforestwatch.org/ loaded in MiniBrowser using Chrome's UA.
> 
> I changed the UA of MiniBrowser to the UA of Chrome (which works fine).
> 
> "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.102 Safari/537.36"
> 
> The page was successfully loaded. Snapshot attached.

Great, what if you directly visit http://www.globalforestwatch.org/notsupportedbrowser ?
Comment 5 Diego Pino 2014-03-12 09:34:29 PDT
(In reply to comment #4)

> Great, what if you directly visit http://www.globalforestwatch.org/notsupportedbrowser ?

If I load "http://www.globalforestwatch.org/notsupportedbrowser" in MiniBrowser using Chrome's UA, the page is loaded successfully.

I also tested it in Safari as I thought that as MiniBrowser uses the same UA as Safari it should fail on Safari too. However in Safari it worked. Why is that? The difference between MiniBrowser UA and Safari UA is that Safari UA has a "Version" string. For instance:

"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.13+ (KHTML, like Gecko) Version/5.1.7 Safari/534.57.2"

I pressume that in the case of detecting the browser is a Safari, the code that checks the browser support may try to fetch the version string too, as not all versions of Safari might be supported by this site.

In fact, just by editing MiniBrowser UA and add "Version/6.0", the page gets loaded. If I try doing the same using "Version/3.0" the "too many redirects" error happens.
Comment 6 Sergio Villar Senin 2014-03-12 10:19:48 PDT
(In reply to comment #5)
> (In reply to comment #4)
> 
> > Great, what if you directly visit http://www.globalforestwatch.org/notsupportedbrowser ?
> 
> If I load "http://www.globalforestwatch.org/notsupportedbrowser" in MiniBrowser using Chrome's UA, the page is loaded successfully.
> 
> I also tested it in Safari as I thought that as MiniBrowser uses the same UA as Safari it should fail on Safari too. However in Safari it worked. Why is that? The difference between MiniBrowser UA and Safari UA is that Safari UA has a "Version" string. For instance:
> 
> "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.13+ (KHTML, like Gecko) Version/5.1.7 Safari/534.57.2"
> 
> I pressume that in the case of detecting the browser is a Safari, the code that checks the browser support may try to fetch the version string too, as not all versions of Safari might be supported by this site.
> 
> In fact, just by editing MiniBrowser UA and add "Version/6.0", the page gets loaded. If I try doing the same using "Version/3.0" the "too many redirects" error happens.

Awesome report. Martin, Gustavo what do you think about tunning a bit our UA? This time isn't about replacing Chrome by Safari or something but just adding the "Version" thing Diego mentioned.

It looks like it isn't only about this site, but I get many "unsupported browser" messages on sites that work fine with Safari versions older than my builds.
Comment 7 Martin Robinson 2014-03-12 10:42:35 PDT
(In reply to comment #6)

> Awesome report. Martin, Gustavo what do you think about tunning a bit our UA? This time isn't about replacing Chrome by Safari or something but just adding the "Version" thing Diego mentioned.

Sounds like we need to add the version string.

I think another interesting test is what happens to other browsers when you give them the bad UA. If they don't go into the redirect loop, this is actually two bugs.
Comment 8 Diego Pino 2014-03-13 01:44:13 PDT
Created attachment 226584 [details]
Loading page in Firefox using MiniBrowser's UA.

As Martin suggested, I tried to load the page in Firefox but using MiniBrowser's UA (I used the User Agent Switcher extension to do that). The result is an error stating that the page isn't redirecting properly. Snapshot attached.
Comment 9 Sergio Villar Senin 2014-03-13 02:05:46 PDT
(In reply to comment #8)
> Created an attachment (id=226584) [details]
> Loading page in Firefox using MiniBrowser's UA.
> 
> As Martin suggested, I tried to load the page in Firefox but using MiniBrowser's UA (I used the User Agent Switcher extension to do that). The result is an error stating that the page isn't redirecting properly. Snapshot attached.

Yeah then pretty sure they have some server side issue. Let's add the version string then. Perhaps we should use the one from the latest release of Safari.
Comment 10 Diego Pino 2014-03-13 03:14:23 PDT
Created attachment 226587 [details]
Patch
Comment 11 Martin Robinson 2014-03-13 07:34:43 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=226584) [details] [details]
> > Loading page in Firefox using MiniBrowser's UA.
> > 
> > As Martin suggested, I tried to load the page in Firefox but using MiniBrowser's UA (I used the User Agent Switcher extension to do that). The result is an error stating that the page isn't redirecting properly. Snapshot attached.
> 
> Yeah then pretty sure they have some server side issue. Let's add the version string then. Perhaps we should use the one from the latest release of Safari.

Sounds like we also need some infinite redirect protection.
Comment 12 Sergio Villar Senin 2014-03-13 08:08:16 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Created an attachment (id=226584) [details] [details] [details]
> > > Loading page in Firefox using MiniBrowser's UA.
> > > 
> > > As Martin suggested, I tried to load the page in Firefox but using MiniBrowser's UA (I used the User Agent Switcher extension to do that). The result is an error stating that the page isn't redirecting properly. Snapshot attached.
> > 
> > Yeah then pretty sure they have some server side issue. Let's add the version string then. Perhaps we should use the one from the latest release of Safari.
> 
> Sounds like we also need some infinite redirect protection.

What do you mean? We already show a "Too many redirects" error page
Comment 13 Martin Robinson 2014-03-13 08:16:36 PDT
(In reply to comment #12)

> What do you mean? We already show a "Too many redirects" error page

Oh, right! Sorry. I wasn't thinking properly.
Comment 14 Gustavo Noronha (kov) 2014-03-14 11:47:45 PDT
I agree. Another thing that we should look into is convincing google to give us some features it's not delivering like image search where you upload an image to search similars. It does if we say we're Chromium. boo
Comment 15 Diego Pino 2014-03-14 16:29:25 PDT
(In reply to comment #11)

> Sounds like we also need some infinite redirect protection.

What it might be interesting is to implement a mechanism to detect infinite redirection loops (at least the simplest case, which is a page redirecting to itself). This way we can avoid the all the unnecessary redirections until the maximum limit is reached.
Comment 16 Martin Robinson 2014-03-14 16:30:41 PDT
(In reply to comment #15)
> (In reply to comment #11)
> 
> > Sounds like we also need some infinite redirect protection.
> 
> What it might be interesting is to implement a mechanism to detect infinite redirection loops (at least the simplest case, which is a page redirecting to itself). This way we can avoid the all the unnecessary redirections until the maximum limit is reached.

I think that would be very difficult. A page might serve different content from the same URL.
Comment 17 Sergio Villar Senin 2014-03-25 06:55:32 PDT
What do we do with this then? Are you all OK with the change?
Comment 18 Martin Robinson 2014-03-25 09:24:36 PDT
Comment on attachment 226587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226587&action=review

Did you run unit tests. I think some of them check the expected user agent.

> Source/cmake/OptionsGTK.cmake:149
> +add_definitions(-DUSER_AGENT_SAFARI_MAJOR_VERSION=7)
> +add_definitions(-DUSER_AGENT_SAFARI_MINOR_VERSION=0)

Not sure how I feel about adding more command-line arguments to the compiler.
Comment 19 Diego Pino 2014-03-28 04:02:39 PDT
Created attachment 228036 [details]
Patch
Comment 20 Diego Pino 2014-03-28 04:07:08 PDT
(In reply to comment #18)
> (From update of attachment 226587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226587&action=review
> 
> Did you run unit tests. I think some of them check the expected user agent.
> 

Run master, Version/6.0 and Version/7.0. Here are the results:

Master:

* Tests that timed out (15): /WebKit2Gtk/TestWebKitWebView, /WebKit2Gtk/TestCookieManager, /WebKit2Gtk/TestWebExtensions, /WebKit2Gtk/TestWebKitWebViewGroup, /WebKit2Gtk/TestWebKitPolicyClient, /WebKit2Gtk/TestMultiprocess, /WebKit2Gtk/TestWebKitSettings, /WebKit2Gtk/TestWebKitAccessibility, /WebKit2Gtk/TestLoaderClient, /WebKit2Gtk/TestPrinting, /WebKit2Gtk/TestDOMNode, /WebKit2Gtk/TestFrame, /WebKit2Gtk/TestBackForwardList, /WebKit2Gtk/TestWebKitWebContext, /WebKitGtk/testwebinspector

* Tests skipped (23).

Version/6.0:

* Tests that timed out (13): /WebKit2Gtk/TestWebKitWebView, /WebKit2Gtk/TestCookieManager, /WebKit2Gtk/TestSSL, /WebKit2Gtk/TestWebExtensions, /WebKit2Gtk/TestWebKitWebViewGroup, /WebKit2Gtk/TestWebKitPolicyClient, /WebKit2Gtk/TestMultiprocess, /WebKit2Gtk/TestPrinting, /WebKit2Gtk/TestDOMNode, /WebKit2Gtk/TestFrame, /WebKit2Gtk/TestBackForwardList, /WebKit2Gtk/TestWebKitWebContext, /WebKitGtk/testwebinspector

* Tests skipped (23).

Version/7.0:

* Tests that timed out (16): /WebKit2Gtk/TestWebKitWebView, /WebKit2Gtk/TestCookieManager, /WebKit2Gtk/TestSSL, /WebKit2Gtk/TestWebExtensions, /WebKit2Gtk/TestWebKitWebViewGroup, /WebKit2Gtk/TestWebKitPolicyClient, /WebKit2Gtk/TestMultiprocess, /WebKit2Gtk/TestWebKitSettings, /WebKit2Gtk/TestWebKitAccessibility, /WebKit2Gtk/TestLoaderClient, /WebKit2Gtk/TestPrinting, /WebKit2Gtk/TestDOMNode, /WebKit2Gtk/TestFrame, /WebKit2Gtk/TestBackForwardList, /WebKit2Gtk/TestWebKitWebContext, /WebKitGtk/testwebinspector

* Tests skipped (23).

So, in the end I decided to set the version string to Version/6.0.

> > Source/cmake/OptionsGTK.cmake:149
> > +add_definitions(-DUSER_AGENT_SAFARI_MAJOR_VERSION=7)
> > +add_definitions(-DUSER_AGENT_SAFARI_MINOR_VERSION=0)
> 
> Not sure how I feel about adding more command-line arguments to the compiler.

OK, I appended Version/6.0 directly to the UserAgent string.
Comment 21 WebKit Commit Bot 2014-03-28 07:19:12 PDT
Comment on attachment 228036 [details]
Patch

Clearing flags on attachment: 228036

Committed r166405: <http://trac.webkit.org/changeset/166405>
Comment 22 WebKit Commit Bot 2014-03-28 07:19:17 PDT
All reviewed patches have been landed.  Closing bug.