Bug 168912

Summary: [Soup] "Only from websites I visit" cookie policy is broken
Product: WebKit Reporter: Hussam Al-Tayeb <ht990332>
Component: WebKitGTKAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, berto, bugs-noreply, cdumez, cgarcia, commit-queue, danw, gns, joepeck, mcatanzaro, mrobinson, svillar, zan
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
Patch cgarcia: review+

Description Hussam Al-Tayeb 2017-02-27 07:12:13 PST
I noticed "Only from websites I visit" cookie policy is broken on WebKitGTK+ 2.15.90 and epiphany 2.23.90 (latest from git master branch). I don't have advertisements disabled and it is saving cookies from domains of hotlinked ads.
Comment 1 Michael Catanzaro 2017-02-27 10:34:20 PST
Lots of changes to this code recently. Is it a regression? Did it work properly in 2.14? Or has it always been broken like this?
Comment 2 Hussam Al-Tayeb 2017-03-01 12:50:33 PST
It looks like this was broken in 2.14.xx and epiphany 3.22.xx as well.
Comment 3 Sergio Villar Senin 2017-03-20 10:21:49 PDT
After some quick research it looks like we are not properly setting the first party for cookies in some cases, that's why cookies are being stored anyway. For example if I visit http://www.bmw.es I can see how some cookies are set by youtube because the first party for the requests is youtube.com.
Comment 4 Sergio Villar Senin 2017-03-20 11:46:00 PDT
OK so I finally found the culprit. It's in NetworkDataTaskSoup::continuteHTTPRedirection(). I am not sure what's the relationship with ResourceHandleSoup but they look pretty much the same. The problem is that we're setting the firstPartyForCookies to the URL of the redirected message meaning that any redirection will successfully bypass the "no third party cookies" policy.

That call is already present in ResourceHandleSoup and has been there for ages. I am almost sure that we can safely remove that call (the cocoa code does not do that BTW) as it does not correct. Setting the first party for cookies is handled by the FrameLoader and we should not overwrite that.
Comment 5 Michael Catanzaro 2017-03-20 16:29:48 PDT
Sergio, are you planning to submit a patch?
Comment 6 Carlos Garcia Campos 2017-03-21 01:34:46 PDT
(In reply to comment #4)
> OK so I finally found the culprit.

\o/ Thanks for investigating this.

> It's in
> NetworkDataTaskSoup::continuteHTTPRedirection().

No recent regression then . . .

> I am not sure what's the
> relationship with ResourceHandleSoup but they look pretty much the same.

NetworkDataTask is the new ResourceHandle. The latter is only used in the web process now for the few bits that hasn't been moved to the network process yet.

> The
> problem is that we're setting the firstPartyForCookies to the URL of the
> redirected message meaning that any redirection will successfully bypass the
> "no third party cookies" policy.

Actually, I think the problem is that we are assuming that all redirections are main resource loads. It's amazing that this problem has always been there!

> That call is already present in ResourceHandleSoup and has been there for
> ages.

Exactly, this is copied from ResourceHandle.

> I am almost sure that we can safely remove that call (the cocoa code
> does not do that BTW) as it does not correct. Setting the first party for
> cookies is handled by the FrameLoader and we should not overwrite that.

Right. In case of main resource, DocumentLoader::willSendRequest sets the first party for cookies after the redirection, and in case of subresources, the initial request first party for cookies is copied to the new one, so it's the right one too.
Comment 7 Sergio Villar Senin 2017-03-21 03:32:16 PDT
Actually I added it https://bugs.webkit.org/show_bug.cgi?id=64575 :O
Comment 8 Sergio Villar Senin 2017-03-21 08:22:10 PDT
Created attachment 305004 [details]
Patch
Comment 9 Michael Catanzaro 2017-03-21 18:03:54 PDT
Comment on attachment 305004 [details]
Patch

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

Looks excellent, thanks for the test. I'll let Carlos give final r+.

> Source/WebCore/ChangeLog:12
> +        The most notable effect was that subresources loaded via redirects where effectively

were

> LayoutTests/http/tests/security/cookies/third-party-cookie-blocking-redirect-expected.txt:2
> +This test PASS if you can see the text "FAILED: Cookie not set".

I'm not sure what you were thinking here, but this was a bad idea. :) Please change the text to "PASS: Cookie not set".
Comment 10 Carlos Garcia Campos 2017-03-22 00:40:10 PDT
Comment on attachment 305004 [details]
Patch

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

>> LayoutTests/http/tests/security/cookies/third-party-cookie-blocking-redirect-expected.txt:2
>> +This test PASS if you can see the text "FAILED: Cookie not set".
> 
> I'm not sure what you were thinking here, but this was a bad idea. :) Please change the text to "PASS: Cookie not set".

I think this is because it's reusing the cookies/resources/set-cookie-on-redirect.php that is used to test that cookies are indeed set on redirect, but here we want to test the opposite.
Comment 11 Sergio Villar Senin 2017-03-22 03:05:19 PDT
(In reply to Carlos Garcia Campos from comment #10)
> Comment on attachment 305004 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305004&action=review
> 
> >> LayoutTests/http/tests/security/cookies/third-party-cookie-blocking-redirect-expected.txt:2
> >> +This test PASS if you can see the text "FAILED: Cookie not set".
> > 
> > I'm not sure what you were thinking here, but this was a bad idea. :) Please change the text to "PASS: Cookie not set".
> 
> I think this is because it's reusing the
> cookies/resources/set-cookie-on-redirect.php that is used to test that
> cookies are indeed set on redirect, but here we want to test the opposite.

Right.
Comment 12 Sergio Villar Senin 2017-03-22 03:09:33 PDT
Committed r214246: <http://trac.webkit.org/changeset/214246>