Bug 26391 - Plug-ins don't always respect the cookie accept policy
: Plug-ins don't always respect the cookie accept policy
Status: VERIFIED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
: http://www.time.com/time/world/articl...
: InRadar, ReviewedForRadar
:
: 35905
  Show dependency treegraph
 
Reported: 2009-06-14 15:18 PST by
Modified: 2010-03-12 08:53 PST (History)


Attachments
WIP fix (4.14 KB, patch)
2010-03-08 23:50 PST, Brady Eidson
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Don't start a ResourceLoader with a null firstPartyForCookies (5.49 KB, patch)
2010-03-09 10:16 PST, Brady Eidson
beidson: review-
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Better ChangeLog (6.29 KB, patch)
2010-03-09 10:22 PST, Brady Eidson
darin: review+
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Quick review, perhaps? (3.34 KB, patch)
2010-03-09 13:21 PST, Brady Eidson
ap: review+
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-14 15:18:59 PST
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 From 2009-06-14 15:21:21 PST -------
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 #2 From 2009-06-14 15:56:33 PST -------
<rdar://problem/6971214>
------- Comment #3 From 2009-06-16 00:13:02 PST -------
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 #4 From 2009-06-17 01:37:31 PST -------
May be related to bug 9445.
------- Comment #5 From 2010-03-08 14:28:07 PST -------
*** Bug 9445 has been marked as a duplicate of this bug. ***
------- Comment #6 From 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 From 2010-03-08 23:50:37 PST -------
Created an attachment (id=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 From 2010-03-09 10:16:56 PST -------
Created an attachment (id=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 From 2010-03-09 10:17:43 PST -------
(From update of attachment 50321 [details])
Better changelog coming
------- Comment #10 From 2010-03-09 10:22:15 PST -------
Created an attachment (id=50322) [details]
Better ChangeLog
------- Comment #11 From 2010-03-09 10:31:37 PST -------
(From update of attachment 50322 [details])
This needs a "why" comment. The change log explains it well but the code needs something too.

r=me
------- Comment #12 From 2010-03-09 11:53:03 PST -------
http://trac.webkit.org/changeset/55738
------- Comment #13 From 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 From 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 From 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 From 2010-03-09 13:21:13 PST -------
Created an attachment (id=50341) [details]
Quick review, perhaps?
------- Comment #17 From 2010-03-09 13:23:59 PST -------
(From update of attachment 50341 [details])
r=me
------- Comment #18 From 2010-03-09 13:26:20 PST -------
http://trac.webkit.org/changeset/55742
------- Comment #19 From 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 From 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 From 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 From 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.