Bug 26391 - Plug-ins don't always respect the cookie accept policy
Summary: Plug-ins don't always respect the cookie accept policy
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Brady Eidson
URL: http://www.time.com/time/world/articl...
Keywords: InRadar
: 9445 26880 (view as bug list)
Depends on:
Blocks: 35905
  Show dependency treegraph
 
Reported: 2009-06-14 15:18 PDT by Jeff Johnson
Modified: 2022-02-10 14:09 PST (History)
6 users (show)

See Also:


Attachments
WIP fix (4.14 KB, patch)
2010-03-08 23:50 PST, Brady Eidson
beidson: commit-queue-
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-
Details | Formatted Diff | Diff
Better ChangeLog (6.29 KB, patch)
2010-03-09 10:22 PST, Brady Eidson
darin: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Quick review, perhaps? (3.34 KB, patch)
2010-03-09 13:21 PST, Brady Eidson
ap: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 2 Mark Rowe (bdash) 2009-06-14 15:56:33 PDT
<rdar://problem/6971214>
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 4 Alexey Proskuryakov 2009-06-17 01:37:31 PDT
May be related to bug 9445.
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 10 Brady Eidson 2010-03-09 10:22:15 PST
Created attachment 50322 [details]
Better ChangeLog
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 12 Brady Eidson 2010-03-09 11:53:03 PST
http://trac.webkit.org/changeset/55738
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 18 Brady Eidson 2010-03-09 13:26:20 PST
http://trac.webkit.org/changeset/55742
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.
Comment 23 Brent Fulgham 2022-02-10 14:09:38 PST
*** Bug 26880 has been marked as a duplicate of this bug. ***