Summary: | [Soup] "Only from websites I visit" cookie policy is broken | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hussam Al-Tayeb <ht990332> | ||||
Component: | WebKitGTK | Assignee: | Sergio Villar Senin <svillar> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, berto, bugs-noreply, cdumez, cgarcia, commit-queue, danw, gustavo, joepeck, mcatanzaro, mrobinson, svillar, zan | ||||
Priority: | P2 | ||||||
Version: | Other | ||||||
Hardware: | Unspecified | ||||||
OS: | Linux | ||||||
Attachments: |
|
Description
Hussam Al-Tayeb
2017-02-27 07:12:13 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? It looks like this was broken in 2.14.xx and epiphany 3.22.xx as well. 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. 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. Sergio, are you planning to submit a patch? (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. Actually I added it https://bugs.webkit.org/show_bug.cgi?id=64575 :O Created attachment 305004 [details]
Patch
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 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. (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. Committed r214246: <http://trac.webkit.org/changeset/214246> |