Bug 126518

Summary: REGRESSION(r161176): http/tests/misc/authentication-redirect-3/authentication-sent-to-redirect-same-origin-with-location-credentials.html is failing on GTK
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, danw, gustavo, mrobinson, rakuco
Priority: P2 Keywords: Gtk, LayoutTestFailure, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Temptative fix mrobinson: review+

Description Zan Dobersek 2014-01-06 05:37:02 PST
The http/tests/misc/authentication-redirect-3/authentication-sent-to-redirect-same-origin-with-location-credentials.html layout test is failing after r161176.
http://trac.webkit.org/changeset/161176
http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fmisc%2Fauthentication-redirect-3%2Fauthentication-sent-to-redirect-same-origin-with-location-credentials.html

Here's the diff:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/misc/authentication-redirect-3/authentication-sent-to-redirect-same-origin-with-location-credentials-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/misc/authentication-redirect-3/authentication-sent-to-redirect-same-origin-with-location-credentials-actual.txt
@@ -9,19 +9,19 @@
 --------
 Frame: '<!--framePath //<!--frame0-->-->'
 --------
-Resource loaded with HTTP authentication username 'redirectuser' and password 'redirectpassword'
+Resource loaded with HTTP authentication username 'testUser' and password 'testPassword'
 
 --------
 Frame: '<!--framePath //<!--frame1-->-->'
 --------
-Resource loaded with HTTP authentication username 'redirectuser' and password 'redirectpassword'
+Resource loaded with HTTP authentication username 'testUser' and password 'testPassword'
 
 --------
 Frame: '<!--framePath //<!--frame2-->-->'
 --------
-Resource loaded with HTTP authentication username 'redirectuser' and password 'redirectpassword'
+Resource loaded with HTTP authentication username 'testUser' and password 'testPassword'
 
 --------
 Frame: '<!--framePath //<!--frame3-->-->'
 --------
-Resource loaded with HTTP authentication username 'redirectuser' and password 'redirectpassword'
+Resource loaded with HTTP authentication username 'testUser' and password 'testPassword'
Comment 1 Zan Dobersek 2014-01-06 05:38:50 PST
And http/tests/security/redirect-BLOCKED-to-localURL.html as well.

Diff:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/redirect-BLOCKED-to-localURL-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/redirect-BLOCKED-to-localURL-actual.txt
@@ -1,2 +1,3 @@
+CONSOLE MESSAGE: Not allowed to load local resource: file-redirect-target.html
 
 This attempts to open a redirect link to a file URL, which should be blocked.
Comment 2 Martin Robinson 2014-01-06 08:44:00 PST
Carlos, are you able to fix this or should we roll out the change for now?
Comment 3 Carlos Garcia Campos 2014-01-07 00:36:34 PST
(In reply to comment #2)
> Carlos, are you able to fix this or should we roll out the change for now?

I'll take a look at it today.
Comment 4 Carlos Garcia Campos 2014-01-07 10:22:47 PST
hmm, both tests pass for me with wk1 and wk2. I indeed ran the tests before landing r161176. Is it flaky in the bots?
Comment 5 Carlos Garcia Campos 2014-01-07 10:32:18 PST
I think I know how to fix it in any case, but I'm unable to reproduce it, I'll fix it tomorrow, today I've been busy working on all other layout tests in the end (see bug #126570).
Comment 6 Carlos Garcia Campos 2014-01-08 01:28:15 PST
Created attachment 220610 [details]
Temptative fix

This patch restores the previous behaviour, clearing the credentials form the request before calling client()->willSendRequest(), but applying them right before creating the new SoupRequest. I don't know if it actually fixes the problem, because I haven't been able to reproduce it.
Comment 7 Martin Robinson 2014-01-08 07:49:58 PST
Comment on attachment 220610 [details]
Temptative fix

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

> Source/WebCore/ChangeLog:11
> +
> +        Clear the credentials before calling willSendRequest on the client
> +        to avoid sending the credentials to the API layer, but apply them
> +        again to the request right before creating the new SoupRequest.
> +

Hrm. Where are the credentials cleared? I only see you removing newRequest.removeCredentials(); in doRedirect.
Comment 8 Carlos Garcia Campos 2014-01-08 08:07:31 PST
(In reply to comment #7)
> (From update of attachment 220610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220610&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +
> > +        Clear the credentials before calling willSendRequest on the client
> > +        to avoid sending the credentials to the API layer, but apply them
> > +        again to the request right before creating the new SoupRequest.
> > +
> 
> Hrm. Where are the credentials cleared? I only see you removing newRequest.removeCredentials(); in doRedirect.

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp#L507
Comment 9 Carlos Garcia Campos 2014-01-09 00:01:10 PST
Committed r161549: <http://trac.webkit.org/changeset/161549>
Comment 10 Zan Dobersek 2014-01-13 02:34:52 PST
(In reply to comment #1)
> And http/tests/security/redirect-BLOCKED-to-localURL.html as well.
> 
> Diff:
> --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/redirect-BLOCKED-to-localURL-expected.txt
> +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/redirect-BLOCKED-to-localURL-actual.txt
> @@ -1,2 +1,3 @@
> +CONSOLE MESSAGE: Not allowed to load local resource: file-redirect-target.html
> 
>  This attempts to open a redirect link to a file URL, which should be blocked.

This failure still remains. The console message is not expected both in the GTK-specific and Mac-specific baselines, but it is in the global baseline.

Is it OK to remove the GTK-specific baseline? Is the console message a progression?

http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/redirect-BLOCKED-to-localURL-expected.txt
http://trac.webkit.org/browser/trunk/LayoutTests/platform/gtk/http/tests/security/redirect-BLOCKED-to-localURL-expected.txt
http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/http/tests/security/redirect-BLOCKED-to-localURL-expected.txt
Comment 11 Martin Robinson 2014-01-13 07:49:51 PST
(In reply to comment #10)

> Is it OK to remove the GTK-specific baseline? Is the console message a progression?


It seems like the test is passing as long as you don't see: "FAIL: This page shouldn't load via HTTP redirect to file:"
Comment 12 Carlos Garcia Campos 2014-01-15 05:40:34 PST
(In reply to comment #11)
> (In reply to comment #10)
> 
> > Is it OK to remove the GTK-specific baseline? Is the console message a progression?
> 
> 
> It seems like the test is passing as long as you don't see: "FAIL: This page shouldn't load via HTTP redirect to file:"

Right.
Comment 13 Zan Dobersek 2014-01-15 08:57:05 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > 
> > > Is it OK to remove the GTK-specific baseline? Is the console message a progression?
> > 
> > 
> > It seems like the test is passing as long as you don't see: "FAIL: This page shouldn't load via HTTP redirect to file:"
> 
> Right.

Thanks for the clarifications. Removed in r162068.
http://trac.webkit.org/changeset/162068