Bug 168912 – [Soup] "Only from websites I visit" cookie policy is broken

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Hussam Al-Tayeb]

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.

[Michael Catanzaro]

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?

[Hussam Al-Tayeb]

It looks like this was broken in 2.14.xx and epiphany 3.22.xx as well.

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

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.

[Michael Catanzaro]

Sergio, are you planning to submit a patch?

[Carlos Garcia Campos]

(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.

[Sergio Villar Senin]

Actually I added it https://bugs.webkit.org/show_bug.cgi?id=64575 :O

[Sergio Villar Senin]

Created attachment 305004 [details]
Patch

[Michael Catanzaro]

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".

[Carlos Garcia Campos]

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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

Committed r214246: <http://trac.webkit.org/changeset/214246>