Bug 26391 – Plug-ins don't always respect the cookie accept policy

• Component: New Bugs
• Status: VERIFIED FIXED

[Jeff Johnson]

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.

[Jeff Johnson]

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"

[Mark Rowe (bdash)]

<rdar://problem/6971214>

[Jeff Johnson]

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.

[Alexey Proskuryakov]

May be related to bug 9445.

[Brady Eidson]

*** Bug 9445 has been marked as a duplicate of this bug. ***

[Brady Eidson]

(In reply to comment #2)
> <rdar://problem/6971214>

Using <rdar://problem/7338359> to track this now

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

Comment on attachment 50321 [details]
Don't start a ResourceLoader with a null firstPartyForCookies

Better changelog coming

[Brady Eidson]

Created attachment 50322 [details]
Better ChangeLog

[Darin Adler]

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

[Brady Eidson]

http://trac.webkit.org/changeset/55738

[Eric Seidel (no email)]

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

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

Created attachment 50341 [details]
Quick review, perhaps?

[Alexey Proskuryakov]

Comment on attachment 50341 [details]
Quick review, perhaps?

r=me

[Brady Eidson]

http://trac.webkit.org/changeset/55742

[Brady Eidson]

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

[Brady Eidson]

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.

[Jeff Johnson]

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.

[Brady Eidson]

And for full closure on the failing test issue, we made changes to *really* keep the tests consistent and they seem to have stuck.

[Brent Fulgham]

*** Bug 26880 has been marked as a duplicate of this bug. ***