|Summary:||Plug-ins don't always respect the cookie accept policy|
|Product:||WebKit||Reporter:||Jeff Johnson <opendarwin>|
|Component:||New Bugs||Assignee:||Brady Eidson <beidson>|
|Severity:||Normal||CC:||andersca, ap, beidson, eric, lutzp, rachael|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:|
Description Jeff Johnson 2009-06-14 15:18:59 PDT
Summary: On my system, I have the HTTPCookieStorage cookieAcceptPolicy set to NSHTTPCookieAcceptPolicyOnlyFromMainDocumentDomain (corresponding to the Safari preference "Only from sites I visit"). However, when I load the page http://www.time.com/time/world/article/0,8599,1904577,00.html with WebKit, I get a cookie from ".clearspring.com", which is obviously not within the domain of "time.com". Steps to reproduce: 1) Launch Safari 4 on Mac OS X 10.5.7. 2) Open Safari preferences 3) Select Accept cookies: Only from sites I visit 4) Click "Show Cookies" 5) Click "Remove All" and "Done" 6) Load the page http://www.time.com/time/world/article/0,8599,1904577,00.html 7) Open preferences again 8) Click "Show Cookies" again Expected results: I only see cookies from "time.com" Actual results: In addition to cookies from "time.com", I see one cookies from ".clearspring.com" Regression: This bug also occurred with the immediately preceding version of Safari on Mac OS X 10.5.7. Can't remember the exact version #, but it was 3.2.x. The bug also occurs if you use "/Developer/Examples/WebKit/MiniBrowser" to load the page rather than Safari. Notes: Running in the debugger, I set breakpoints at -[NSHTTPCookieStorage setCookie:], -[NSHTTPCookieStorage setCookieAcceptPolicy:], and -[NSHTTPCookieStorage setCookies:forURL:mainDocumentURL:], as well as at setCookies() in "WebKit/WebCore/platform/mac/CookieJar.mm" with git commit f8f4e69a4ceb5909eb64ea91fe1de7108710c552 (corresponding to svn r43960). The methods setCookie: and setCookieAcceptPolicy: never get called. The method setCookies:forURL:mainDocumentURL: does get called frequently, also from setCookies() in WebKit, but when printing the cookies I never see ".clearspring.com". Thus, it's a mystery to me where that cookie is getting set. I guess it must be set somehow without NSHTTPCookieStorage, which may explain why it's not respecting the cookieAcceptPolicy.
Comment 1 Jeff Johnson 2009-06-14 15:21:21 PDT
Typo in the notes: it should say "The method setCookies:forURL:mainDocumentURL: does get called frequently, ALWAYS from setCookies() in WebKit" rather than "The method setCookies:forURL:mainDocumentURL: does get called frequently, ALSO from setCookies() in WebKit"
Comment 3 Jeff Johnson 2009-06-16 00:13:02 PDT
In my testing, it seems that the cookie is set when the page loads the application/x-shockwave-flash URL http://widgets.clearspring.com/o/475db72337b302d4/-/-/-/ The URL request is created in -[WebNetscapePluginView loadStream]. The request by default has HTTPShouldHandleCookies YES. Then in ResourceRequest::doUpdatePlatformRequest() it calls [nsRequest setMainDocumentURL:firstPartyForCookies()], but firstPartyForCookies() returns nil.
Comment 5 Brady Eidson 2010-03-08 14:28:07 PST
*** Bug 9445 has been marked as a duplicate of this bug. ***
Comment 6 Brady Eidson 2010-03-08 14:30:45 PST
(In reply to comment #2) > <rdar://problem/6971214> Using <rdar://problem/7338359> to track this now
Comment 7 Brady Eidson 2010-03-08 23:50:37 PST
Created attachment 50280 [details] WIP fix Work in progress. Almost done, actually. I'm still thinking through the ResourceLoader level being the right place where the firstPartyForCookies url is set. Currently FrameLoader handles this in ::addExtraFieldsToRequest() before handing the request off to SubresourceLoader and MainResourceLoader, which both then pipe down to ResourceLoader. It's kind of sad that the FrameLoader::addExtraFieldsToRequest() doesn't happen at the ResourceLoader level itself but I don't know that simply moving it is a great idea. I'll decide something tomorrow and hopefully put it up for review in the morning.
Comment 8 Brady Eidson 2010-03-09 10:16:56 PST
Created attachment 50321 [details] Don't start a ResourceLoader with a null firstPartyForCookies After sleeping on it, I think this is the right way to go for now. It's a shame that plugin code doesn't go through FrameLoader, and therefore doesn't automatically get the extra fields added. Since plug-in code is currently so strewn about and very platform specific, I think reworking it needs to be a task for that domain's expert. I don't think there's any harm in this targeted fix for the hole in our cookie accept policy until that can happen.
Comment 9 Brady Eidson 2010-03-09 10:17:43 PST
Comment on attachment 50321 [details] Don't start a ResourceLoader with a null firstPartyForCookies Better changelog coming
Comment 11 Darin Adler 2010-03-09 10:31:37 PST
Comment on attachment 50322 [details] Better ChangeLog This needs a "why" comment. The change log explains it well but the code needs something too. r=me
Comment 13 Eric Seidel (no email) 2010-03-09 12:57:55 PST
http/tests/cookies/third-party-cookie-relaxing.html -> failed failed on the commit-bot when doing the "does trunk actually build and test" check before trying to commit. I believe that failure is due to this checkin moments ago and that the bots will soon roll red. :(
Comment 14 Brady Eidson 2010-03-09 13:03:32 PST
(In reply to comment #13) > http/tests/cookies/third-party-cookie-relaxing.html -> failed > failed on the commit-bot when doing the "does trunk actually build and test" > check before trying to commit. > > I believe that failure is due to this checkin moments ago and that the bots > will soon roll red. :( Looking at the layouttest failures, it's apparently that the various cookie tests don't reset themselves to a consistent state. Amazing we didn't have problems like this with cookie tests in the past. I will try to fix the two tests I've added recently that have gone red because of this.
Comment 15 Brady Eidson 2010-03-09 13:15:41 PST
(In reply to comment #14) > (In reply to comment #13) > > http/tests/cookies/third-party-cookie-relaxing.html -> failed > > failed on the commit-bot when doing the "does trunk actually build and test" > > check before trying to commit. > > > > I believe that failure is due to this checkin moments ago and that the bots > > will soon roll red. :( > > Looking at the layouttest failures, it's apparently that the various cookie > tests don't reset themselves to a consistent state. Amazing we didn't have > problems like this with cookie tests in the past. I will try to fix the two > tests I've added recently that have gone red because of this. There were some cookie utility functions that were used by *SOME* cookie tests to clear the cookie state. It doesn't seem to be used consistently. I'm using it before and after each of these two new tests to keep things in a consistent state.
Comment 16 Brady Eidson 2010-03-09 13:21:13 PST
Created attachment 50341 [details] Quick review, perhaps?
Comment 17 Alexey Proskuryakov 2010-03-09 13:23:59 PST
Comment on attachment 50341 [details] Quick review, perhaps? r=me
Comment 19 Brady Eidson 2010-03-09 14:14:43 PST
(In reply to comment #18) > http://trac.webkit.org/changeset/55742 That fix apparently didn't work. I still don't know why not. Clearly these tests are leaking cookies, causing the failure. I just don't know why the bots are seeing this and I can't reproduce here.
Comment 20 Brady Eidson 2010-03-09 16:09:18 PST
SnowLeopard release bot and Leopard debug bots are clear. I'm still lost as to why Tiger and Leopard release are still failing these tests.
Comment 21 Jeff Johnson 2010-03-12 08:28:21 PST
Verified as fixed with git commit 83853ec7af2cf0f4937a743fe99371b6592017b1 svn r55820. The original time.com URL is no longer demonstrating the original bug with stock 10.6.2 WebKit, but I found another web site that does reproduce the original bug with 10.6.2 WebKit. Fixed in the trunk.
Comment 22 Brady Eidson 2010-03-12 08:53:02 PST
And for full closure on the failing test issue, we made changes to *really* keep the tests consistent and they seem to have stuck.