Disabling 3rd party cookies breaks sending cookies for channels with no docshell

RESOLVED FIXED

Status

()

Core
Networking: Cookies
--
major
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Moe Neve, Assigned: mcs)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---

Firefox Tracking Flags

(status1.9.1 wanted)

Details

(Whiteboard: [testcase in URL], URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008051206 Firefox/3.0

If 3rd party cookies are disabled, ALL experimental add-ons (themes + extensions) in the sand-box at addons.mozilla.org, produce an error and do not install.

Adding *.mozilla.org* to cookie allow exceptions did not fix.

AFAIK there is NO other way of installing these plugins ('save-as' tricks don't work), unless 3rd party cookies are enabled, or the xpi can be obtained from a alternate location.

Reproducible: Always

Steps to Reproduce:
1. Disable 3rd party cookies: either untick "Accept third-party cookies" or change 'network.cookie.cookieBehavior' to 1
2. Log-in to addons.mozilla.org to enable downloading of experimental sand-box add-ons.
3. Click 'Add To Firefox' on an experimental add-on.
Actual Results:  
"Firefox could not install the file at
https://addons.mozilla.org/... .xpi
because: Invalid file hash (possible download corruption)
-261"

Expected Results:  
correct retrival of file + installation

Reproducable on a clean/fresh profile.

Comment 1

9 years ago
Confirmed.  (3.0 RC2 on Linux)  Experimental add-ons only install with 3rd party cookies turned on.  Non-experimental add-ons are not affected.

This sounds more like a problem with AMO, rather than Firefox.

Also see bug 424339.  In fact, this exact problem is mentioned in bug 424339 comment #26.  However, that being said, I think this is not the same issue that report was started for, as the add-ons getting the error over there aren't necessarily sandboxed.

Might be dupeable to that, but for now I'll just confirm.
Status: UNCONFIRMED → NEW
Component: Extension/Theme Manager → Public Pages
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → addons.mozilla.org
QA Contact: extension.manager → web-ui
Hardware: PC → All

Updated

9 years ago
Depends on: 424339

Updated

9 years ago
Duplicate of this bug: 440776

Comment 3

9 years ago
> This sounds more like a problem with AMO, rather than Firefox.

I don't think so, FF3 is not sending the AMOv3 (session id) cookie when requesting the xpi. That cookie was set by a POST to https://addons.mozilla.org/en-US/firefox/users/login.  The xpi is requested from the same domain, there are no third-party issues here.  Also it works in FF2.

Comment 4

9 years ago
Stephend, can you please create a new profile, turn off third-party cookies and try installing an experimental add-on, then put a dump of the cookies into the bug for review?

Thanks!
Assignee: nobody → stephen.donner
Created attachment 326032 [details]
Screenshot of add-on cookies in SQLite Database Browser
Created attachment 326033 [details]
Logfile

This logfile was generated with Live HTTP headers, by:

[1] Logging in to production AMO
[2] Clicking the "Add to Firefox" button for the "10BetterPages" experimental add-on (https://addons.mozilla.org/en-US/firefox/addon/7489)
[3] Getting the installTrigger() logic
[4] Failing with a -261 error

Also, I have an AMOv3 session-only cookie.
CCing Mossop because apparently Fx3 is not sending any headers to AMO for the XPI request.
(In reply to comment #7)
> CCing Mossop because apparently Fx3 is not sending any headers to AMO for the
> XPI request.

And of course this doesn't make sense: What I wanted to say is, it doesn't send *cookie* headers. So without a login cookie, AMO does not deliver an experimental add-on.

Updated

9 years ago
Duplicate of this bug: 441402
This is I believe a fault in the cookie module, although I have a patch that will work around the problem for xpinstall.

The problem is that the loads for xpis have no associated docshell, case 2 at http://mxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookiePermission.cpp#424. This makes the attempt to get an originating URI fail.

This causes the permission check for passing cookies with the request to fail http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/cookie/src/nsCookieService.cpp&rev=1.94&mark=1807#1758

I'm a little confused why this sort of check is even being performed for the request case. It appears to mean that if a resource for a page is coming from a different host then no cookies will ever be sent for it. And the problem here is that if the original host cannot be determined then it is assumed to be different.

Seems to me that even if you should block cookies for inner resources that you shouldn't block cookies for resources that you can't find the originating url for.
Assignee: stephen.donner → nobody
Component: Public Pages → Networking: Cookies
Product: addons.mozilla.org → Core
QA Contact: web-ui → networking.cookies
Summary: disabling 3rd party cookies breaks installation of AMO experimental plugins (Invalid file hash -261) → Disabling 3rd party cookies breaks sending cookies for channels with no docshell
Version: unspecified → Trunk
This is a regression from Firefox 2. I'm requesting blocking 1.9.0.1 because this is effectively breaks cookies for background requests (think update requests, extension initiated requests etc) unless the caller is careful to work around it, which in the past there has been no need to do.
Flags: blocking1.9.1?
Flags: blocking1.9.0.1?
Keywords: regression
Is there a way for us to work around it at AMO?
(In reply to comment #12)
> Is there a way for us to work around it at AMO?

No
dwitte: want to take this?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-

Comment 15

9 years ago
One really simple (In reply to comment #12)
> Is there a way for us to work around it at AMO?
> 

One simple workaround is before even showing the download link - check the firefox version, then try and set a third party cookie (say for some non-existent mozilla site), then try to read it.  If you can't read it, AMO should actually tell people "You must enable third party cookies to download experimental add-ons", and again do not show the download link.

Simply put, it makes no sense to show a link that we know will not work.

The way it is now is just rough.  I'm a developer for the "Favicon Picker 3" extension, and lots of reviewers have had this problem.  Between the glacial approval speed for experimental addons, the installation errors, and the reviewers rating a 1 star because they couldn't even install (or read the solution right in the description) .... <sigh> this all really wears a guy down.

Comment 16

9 years ago
Possible server workaround - could we edit this line: http://mxr.mozilla.org/addons/source/trunk/site/app/controllers/downloads_controller.php#82  I think the cookie check here could be removed, or the "redirect" (line 85) cut out.  If you look at stephend's logfile, there is an "&m=1" on the end of the file he GETs that I think is from line 85.  Maybe somebody could try cutting out one or both lines.

If the "Add to Firefox" button already green at the time this check is made, then we already know the user is logged in, and perhaps this double check could be removed.  (If we wanted to prevent hot-linking from external sites, maybe we could put in a check for the referrer domain?  Or maybe this isn't a concern?)

The stated purpose of logging in is "as a reminder that you are about to undertake a risk step" - then maybe the user is OK.  I know the real problem is in firefox, not AMO, but the behavior now isn't smart.  That's my 2 cents.

Comment 17

9 years ago
We are needing to document workarounds as a temporary solution to this on AMO but this bug really should be nom'ed for 3.0.2 since it provides a pretty bad user experience. How do we nom it for 1.9.0.2?
Also affects extension developers who want to set global cookies - workaround at http://weblogs.mozillazine.org/doron/archives/2008/06/extensions_and_firefox_3_nsico.html
Assignee: nobody → dwitte
Whiteboard: [1.9.0.2+]

Updated

9 years ago
Flags: wanted1.9.1?

Comment 19

9 years ago
I can confirm this bug (on FF3Final).
Also see my comment on MozillaZine about it: http://forums.mozillazine.org/viewtopic.php?p=3593575#p3593575
I gave the bug a vote

Comment 20

9 years ago
The problem not only firefox 3.  but same in Thunderbird 2.0.14 (local install ok, but auto not).

I removed everything, re-installed main program and with only one add-ons update, but it still have error 261.

(In reply to comment #20)
> The problem not only firefox 3.  but same in Thunderbird 2.0.14 (local install
> ok, but auto not).

You are seeing something different
Flags: blocking1.9.0.2?
I just found this bug today.  I filed bug 439784 last month because a number of extensions are having a problem using XMLHttpRequest with third-party cookies disabled.

Is that a dupe of this bug?
Duplicate of this bug: 439784

Updated

9 years ago
Duplicate of this bug: 443150
I added the testcase provided by Memso in Bug 443150 comment 4 in the URL field.
Added "In Litmus" request.
Flags: in-litmus?
Keywords: testcase
Whiteboard: [1.9.0.2+] → [1.9.0.2+] [testcase in URL]
Created attachment 330188 [details]
Testcase source code by Memso (testcase in URL)

I think the other attachments can be marked as obsolete.

Are you sure this is a Core: Network - Cookies bug? I think the problem is Firefox 3 doesn't show you the alert UI and block third party cookies by default. I don't think this is a bug with cookie handling core component.
Attachment #326032 - Attachment is obsolete: true
Attachment #326033 - Attachment is obsolete: true

Comment 27

9 years ago
(In reply to comment #10)
> Seems to me that even if you should block cookies for inner resources that you
> shouldn't block cookies for resources that you can't find the originating url
> for.

so, the intent of doing things this way is simple: a literal interpretation of "block third party cookies" means "block sending and receiving cookies for any host i didn't type in or click on". you can read bug 421494 for the details of why. even chrome-based loads can't be trusted - favicon loads, for instance, come from chrome and have no docshell, and they will greedily load from any site a resource is fetched from. allowing third party cookies with those requests would be a major hole in third party blocking.

now obviously there should be some exceptions to that, such as xpi downloads (where it's not a traditional urlbar-based load, but the user did click on something and clearly wants cookies sent). as noted here and elsewhere, there are some extensions who require cookies to function and are broken by this change. i'm fine with any fix that keeps to the above philosophy. however there are background services that this change affects, such as safebrowsing, AUS etc, which arguably are cases where the services function just fine without cookies and so erring on the side of privacy, where the user clearly desires it, is sensible. i think this applies to most extensions and such as well: they don't need cookies, but there are a few that really do. (this is really just another twist on the problem of the user disabling cookies: the extensions already need to have a FAQ blurb on making sure your cookies are enabled. whitelisting the particular site in question is another workaround, since that'll override third party checks. but i think we need a programmatic solution.)

there are a few obvious ways we could fix this, and maybe some ingenious ones i haven't thought of:
1) allow third party cookies for the no docshell or chrome docshell cases, and find some way to disable third party cookies specifically for favicons. this would be least-effort for extensions etc, but it does mean we'll continually have to plug third party holes as they arise, so i dislike this approach.
2) find a way for consumers to notify loads to notify the cookieservice that they're a first party load, and want cookies. perhaps a loadflag? (could use the same approach for 1, i.e. a loadflag for favicons to say they're a third party load). consumers could always abuse this flag, but we're not out to prevent that, just to make it harder for obvious privacy holes to crop up in code.
3) consumers could tell the cookieservice directly, e.g. by supplying their own nsIChannel or nsIURI argument which we could look at. but doing this only helps some consumers (those that call nsICookieService directly) and so isn't very useful.

Comment 28

9 years ago
i can give ideas/reviews here but i don't have time to work on this directly at the moment - trying to graduate.
Assignee: dwitte → nobody
Keywords: helpwanted
Yeah, we really need an owner for this since Dan can't own it.

Damon, any ideas?
Flags: blocking1.9.0.2? → blocking1.9.0.2+
Whiteboard: [1.9.0.2+] [testcase in URL] → [testcase in URL]
(In reply to comment #27)
> (In reply to comment #10)
> > Seems to me that even if you should block cookies for inner resources that you
> > shouldn't block cookies for resources that you can't find the originating url
> > for.
> 
> so, the intent of doing things this way is simple: a literal interpretation of
> "block third party cookies" means "block sending and receiving cookies for any
> host i didn't type in or click on". you can read bug 421494 for the details of
> why. even chrome-based loads can't be trusted - favicon loads, for instance,
> come from chrome and have no docshell, and they will greedily load from any
> site a resource is fetched from. allowing third party cookies with those
> requests would be a major hole in third party blocking.

This makes a lot more sense to me now. It also means that we likely should be affecting extension authors and other consumers, provided they have some means to indicate that their request is user requested. The problem with that is that any change is liable to change APIs which makes it less viable for branch use.

I think there are already some ways that extension authors can use to get around this so perhaps the route here is just to fix the xpinstall case and then give better documentation on how to handle this case.

Updated

9 years ago
Duplicate of this bug: 426902
Here are a couple of code snippets that extension authors can use to work properly with third party cookie blocking. Unless anyone disagrees I think the best course is just to document and promote their use and fix this case for xpinstall.

For extensions wanting to make top level requests that have been initiated by the user, they can use this:

var ds = Cc["@mozilla.org/webshell;1"].
         createInstance(Ci.nsIDocShellTreeItem).
         Queryinterface(Ci.nsIInterfaceRequestor);
ds.itemType = Ci.nsIDocShellTreeItem.typeContent;
var request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
              createInstance(Ci.nsIXMLHttpRequest);
request.open("GET", url, true);
request.channel.loadGroup = ds.getInterface(Ci.nsILoadGroup);
request.channel.loadFlags |= Ci.nsIChannel.LOAD_DOCUMENT_URI;
request.send(null);


For extensions that want to make a request as a sub-request of a give page (and so have cookies blocked or not depending on the request uri and the prefs), then this will work:

var lg = window.QueryInterface(Ci.nsIInterfaceRequestor)
               .getInterface(Ci.nsIWebNavigation)
               .QueryInterface(nsIInterfaceRequestor)
               .getInterface(Ci.nsILoadGroup);
var request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
              createInstance(Ci.nsIXMLHttpRequest);
request.open("GET", url, true);
request.channel.loadGroup = lg;
request.send(null);

Comment 33

9 years ago
(In reply to comment #32)
> Here are a couple of code snippets that extension authors can use to work
> properly with third party cookie blocking. Unless anyone disagrees I think the
> best course is just to document and promote their use and fix this case for
> xpinstall.
> 

What about save as?

Would like to comment on the extension xmlhttpreqests. 3rd party issues arise when there is a primary 1st party root request that causes a cascade of ancillary subrequests, which if their host is different from the root are labeled 3rd party.

Extension xmlhttprequests don't fit this pattern.  There is no ancillary request cascade to be examined for 3rd partyness so it doesn't make sense for extension xmlhttprequests to be affected by the 3rd party setting.
(In reply to comment #33)
> (In reply to comment #32)
> > Here are a couple of code snippets that extension authors can use to work
> > properly with third party cookie blocking. Unless anyone disagrees I think the
> > best course is just to document and promote their use and fix this case for
> > xpinstall.
> > 
> 
> What about save as?

Can be fixed too.

> Would like to comment on the extension xmlhttpreqests. 3rd party issues arise
> when there is a primary 1st party root request that causes a cascade of
> ancillary subrequests, which if their host is different from the root are
> labeled 3rd party.
> 
> Extension xmlhttprequests don't fit this pattern.  There is no ancillary
> request cascade to be examined for 3rd partyness so it doesn't make sense for
> extension xmlhttprequests to be affected by the 3rd party setting.

I disagree. I think all extension initiated (indeed any requests that are for page display) fit one of 3 types:

Background request, not initiated by the user for pulling data for something (toolbar data etc).
A request for data, initiated by the user (not necessarily by typing an url, but by doing clicking something that is expected to retrieve data).
A request for data to fill in some content for the page, maybe to gather more information about resources or something.

The first type are the requests that dwitte means should not send cookies, the pref should "block sending and receiving cookies for any host i didn't type in or click on"
The second type can send cookies all the time, if they use the first code snippet I provided.
The third type should use the second code snippet I provided, that will correctly send or not send cookies depending on whether the new request is to a third party from the original page and based on the settings.

Comment 35

9 years ago
> 
> The first type are the requests that dwitte means should not send cookies, the
> pref should "block sending and receiving cookies for any host i didn't type in
> or click on"

You can argue for that but it is not a 3rd party issue. There is no 3rd party.  3rd party is about leaking user browsing patterns to a 3rd party, it involves a 1st party that the user navigates to and a 3rd party that gets informed about the 1st party visit.

If you install an extension whose purpose is to log in on startup, that is a 1st party request, there is no 3rd party being informed. 

So if you want to have a setting that forbids background extension xmlhttprequests sending and receiving cookies, that's fine, although I am not sure what is gained from that (please explain), but it shouldn't be conflated with the 3rd party setting.
(In reply to comment #32)
> var ds = Cc["@mozilla.org/webshell;1"].
>          createInstance(Ci.nsIDocShellTreeItem).
>          Queryinterface(Ci.nsIInterfaceRequestor);

This line generates the following error when I tried to add it to my extension:

Error: Components.classes['@mozilla.org/webshell;1'].createInstance(Components.interfaces.nsIDocShellTreeItem).Queryinterface is not a function
Source file: javascript:%20Components.classes["@mozilla.org/webshell;1"].createInstance(Components.interfaces.nsIDocShellTreeItem).Queryinterface(Components.interfaces.nsIInterfaceRequestor);
Line: 1

(In reply to comment #35)
> > 
> > The first type are the requests that dwitte means should not send cookies, the
> > pref should "block sending and receiving cookies for any host i didn't type in
> > or click on"
> 
> You can argue for that but it is not a 3rd party issue. There is no 3rd party. 
> 3rd party is about leaking user browsing patterns to a 3rd party, it involves a
> 1st party that the user navigates to and a 3rd party that gets informed about
> the 1st party visit.

Well, I could argue that the extension is the 3rd party. I could also say that I think the pref is poorly named for what it is intended to do. Maybe check the original bug.

(In reply to comment #36)
> (In reply to comment #32)
> > var ds = Cc["@mozilla.org/webshell;1"].
> >          createInstance(Ci.nsIDocShellTreeItem).
> >          Queryinterface(Ci.nsIInterfaceRequestor);

Typo, QueryInterface.

Comment 38

9 years ago
> Well, I could argue that the extension is the 3rd party. 

3rd party refers to a website (that the user doesn't even know about) that monitors the user's visits to 1st party websites (the ones the user actually knows and intends to visit)

an extension is a trusted agent making a 1st party request (login) on behalf of the user.  There is no 3rd party.

With a genuine 3rd party issue your are protecting the user from being identified to an actual 3rd party intent on profiling him.

With an extension xmlhttprequest cookie stripping which you argue is intended what is being accomplished?  Please explain what problem is being solved by this "feature." And if there is one it is not the same as 3rd party cookie privacy issue.

>I could also say that
>I think the pref is poorly named for what it is intended to do. Maybe check the
>original bug.

The pref is named just fine, because 3rd party cookies is a canonical label for a specific well understood phenomenon.  It just doesn't apply to extension xmlhttprequests, where there is no third party.
I want to add that in the testcase in URL the cookie is not a third party one. It seems this bug affects also non-third party cookie requests.
The work around works, but I still think this should actually be fixed.  The
changes made in bug 421494 were made to protect the user against third party
web sites, the fact that it also breaks extensions seems to be a side affect
based on comment #3 in the bug.  

Also since extensions are a part of the browser and there is a work around
listed above (or apparently by loading via nsISidebar) it's not really going to
"block" extensions nor should it.  If it can be made to work with extensions
without forcing the authors jump through hoops, it should.
I added the work around from comment #32 to my extension and while it works for me, for whatever reason it is causing the "onerror" event to continuously fire for XMLHttpRequests on a number people's systems (the OS doesn't seem to matter).

After implementing the changes I got a ton of emails complaining my extension was broken which forced me to back the changes out.
Correction to my previous comment, it turns out that the work around won't work if the NoScript addon is installed because NoScript believes it to be a XSS cross-scripting attack and blocks it.  It works if NoScript is not installed.

Comment 43

9 years ago
(In reply to comment #38)
> an extension is a trusted agent making a 1st party request (login) on behalf of
> the user.  There is no 3rd party.
> 
> With a genuine 3rd party issue your are protecting the user from being
> identified to an actual 3rd party intent on profiling him.
> 
> With an extension xmlhttprequest cookie stripping which you argue is intended
> what is being accomplished?  Please explain what problem is being solved by
> this "feature." And if there is one it is not the same as 3rd party cookie
> privacy issue.

I believe that prohibiting third party cookies shouldn't affect access to  cookies from extensions which are considered trusted code. If some extension wanted to issue a request it could modify trusted sites list via nsIPermissionManager. If extensions need to be restrict  from accessing cookies it should be considered as a separate issue and not mixed with third party cookies problem. Mixing these two concepts simply makes extensions to implement different workarounds to access third-party cookies when "accept third-party cookies" feature is off.

I had another problem associated with this bug.  The "View Page Source" function won't send/receive cookies and therefore the page source won't be displayed correctly on sites that require you to log in.

This is definitely not correct no matter what your definition of "third party" is.
Blocks: 421494
Connor said he'd love to do this.
Assignee: nobody → mconnor

Comment 46

9 years ago
See bug #444267 and #444272.

I've addressed this problem in Flock already. Given some feedback on this, I can submit patches for Firefox as well. But the desired behavior needs to be nailed down first.

It's quite frustrating to receive *zero* feedback on my proposed solutions, in almost a month.
Manish: are you proposing that those are blockers for this bug, or simply related?

Comment 48

9 years ago
Blockers looks like. At least bug #444272 blocks, since it looks like the problem here is that the XPInstall code needs to have cookies sent for the requests it makes.

Updated

9 years ago
Duplicate of this bug: 444272

Updated

9 years ago
Duplicate of this bug: 444267

Comment 51

9 years ago
(In reply to comment #27)
> 2) find a way for consumers to notify loads to notify the cookieservice that
> they're a first party load, and want cookies. perhaps a loadflag? (could use
> the same approach for 1, i.e. a loadflag for favicons to say they're a third
> party load). consumers could always abuse this flag, but we're not out to
> prevent that, just to make it harder for obvious privacy holes to crop up in
> code.

A load flag is exactly what I proposed in bug #444272, but I also think there should be some more explicit convenience in the XHR api.

Comment 52

9 years ago
ah, ok - if you think it's necessary please feel free to reopen bug 444272 for an XHR patch, and mark dependent on the loadflag work here.

Comment 53

9 years ago
So where should a load flag be defined? nsIChannel is marked as FROZEN, how serious is that, in terms of adding constants?
Manish, we can add load flags to nsIChannel, I already talked to boris about that last week.  The trick is the behaviour of the flag, and the default behaviour that we should have for cookies in chrome.

I think what is ideal is to define a load flag for allowing cookies in chrome loads, and default to not allowing cookies for chrome loads unless that flag is set.  That said, I'm concerned about the scope of changes here.  I think that we may need to add a load flag to docshell as well (like the URI fixup flag), so that loadURI consumers don't have to jump through hoops to get around this.  I didn't talk to Boris about that, but it seems like the sanest thing we can do.

(I could be convinced that we defer the "block by default" change to 3.1, and only block when third-party is blocked for 3.0.x, but keep the flags the same, so as extensions and vendors transition over to add the loadflag to work with third-party blocking enabled, they'll still be fine when we restrict by default in all cases.)

For the record, the main point of having the default be "no cookies" is that this means developers have to consciously choose to enable them, rather than jump through hoops to sandbox a connection.  Explicitly opting in also serves to highlight the implications during code review, since there should probably be a very good reason to allow cookies for app services.

Manish, if this makes sense, let's connect by IRC or email ASAP to figure out how to roll forward with this.

Comment 55

9 years ago
Sounds good overall, yeah.

Updated

9 years ago
Status: NEW → ASSIGNED

Comment 56

9 years ago
Is there any tentative ETA on the fix for XHR issue?
I'm moving this bug out per a previous conversation with mconnor. This won't make it today, which means it'll miss 1.9.0.2. We can revisit for 1.9.0.3.
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.2-
Flags: blocking1.9.0.2+
If this bug is moved to 1.9.0.3 and firefox 3.0.1 is offered as update to the public as of two days ago and there are still a lot of experimental add-ons in the queue, I'd suggest AMO to patch things as per comment #15. 

Or check if 3rd party cookies are disabled, and if so, display the "Download now" button instead of the "Install" button, because downloading the experimental add-ons seems to work (you get download button when changing the application in the URL from firefox to thunderbird).

This would solve the smaller bug 440776 which is duped to this one.
Manish, did you ever make progress on a patch here?
Assignee: mconnor → manish

Comment 60

9 years ago
I just got hit with what might be this - I was trying to download a video (the site says right-click) and if it was set to download the media type by default and I used a normal click, it would download.  If I got a "what do you wan't to do" box, I would get an HTML file with a login screen instead of the file.  What happens is if I have 3rd party cookies disabled (which I did at first), the second request (from Down Them All, but apparently from the built-in too) doesn't send the cookie so it doesn't think I've logged in:  (live http headers)

http://video.tickerforum.org/cgi-bin/feedme?m4v=2008-0911



GET /cgi-bin/feedme?m4v=2008-0911 HTTP/1.1

Host: video.tickerforum.org

User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.3) Gecko/2008092510 Ubuntu/8.04 (hardy) Firefox/3.0.3

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

Accept-Language: en-us,en;q=0.5

Accept-Encoding: gzip,deflate

Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7

Keep-Alive: 300

Connection: keep-alive

Referer: http://video.tickerforum.org/cgi-bin/feedme?login=tz

Cookie: AKCSUser=tz



HTTP/1.x 200 OK

Date: Tue, 30 Sep 2008 23:22:43 GMT

Server: Apache/2.2.8 (Unix)

Content-Length: 79718272

Keep-Alive: timeout=5, max=100

Connection: Keep-Alive

Content-Type: video/x-m4v

----------------------------------------------------------

http://video.tickerforum.org/cgi-bin/feedme?m4v=2008-0911



GET /cgi-bin/feedme?m4v=2008-0911 HTTP/1.1

Host: video.tickerforum.org

User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.3) Gecko/2008092510 Ubuntu/8.04 (hardy) Firefox/3.0.3

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

Accept-Language: en-us,en;q=0.5

Accept-Encoding: gzip,deflate

Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7

Connection: close

Referer: http://video.tickerforum.org/cgi-bin/feedme?login=tz

Pragma: no-cache

Cache-Control: no-cache



HTTP/1.x 200 OK

Date: Tue, 30 Sep 2008 23:22:49 GMT

Server: Apache/2.2.8 (Unix)

Connection: close

Transfer-Encoding: chunked

Content-Type: text/html

Comment 61

9 years ago
(In reply to comment #59)
> Manish, did you ever make progress on a patch here?

I have something half done. I can wrap it by next week, I deprioritized it for other work since it wasn't going to make FF 3.0.2 anyway, and then I went on vacation.
Wanted for branch, but lack of trunk patch or blocking status makes it hard to count on a fix in any particular release.
Flags: blocking1.9.0.4?
Duplicate of this bug: 458411
Can't realistically block on this, but I definitely want it.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
I should note that this is making day-to-day Bugzilla use a pain, since I can't save crasher attachments from security bugs without firing up Firefox 2...
Why does bug 424339 block this one?
> I think what is ideal is to define a load flag for allowing cookies in chrome
> loads, and default to not allowing cookies for chrome loads unless that flag is
> set.

So if we go this route that means we need to modify webnavigation and nsIWebBrowserPersist at the very least, right?  And that there will be no way to send cookies with images (which is sorta ok, I guess, since you don't want them with favicons)?
For what it's worth, here's my problem.  I'm happy to write this patch, but then getting reviews will be a pain because Christian is pretty busy.  If someone else writes the patch (and I'm happy to hand-hold as needed) I can review it.

Come to think of which, I'll try to dig up someone to work on this.
(In reply to comment #61)
> (In reply to comment #59)
> > Manish, did you ever make progress on a patch here?
> 
> I have something half done. I can wrap it by next week, I deprioritized it for
> other work since it wasn't going to make FF 3.0.2 anyway, and then I went on
> vacation.

Are you actually still working on this Manish? I might have spare cycles to do something here after the b2 freeze.

I've spun out bug 462739 for the xpinstall specific case since I think it's pretty easy to get that to behave correctly according to the third party cookie settings.
No longer depends on: 424339
Duplicate of this bug: 465976

Comment 71

9 years ago
Created attachment 349837 [details] [diff] [review]
added new ALLOW_THIRD_PARTY_COOKIE flag

first attempt. added new flag to nsIWebNavigation, nsIWebBrowserPersist, nsIHttpChannelInternal, as well as new logic to test for flag in nsCookiePermission
Attachment #349837 - Flags: review?(bzbarsky)

Comment 72

9 years ago
Comment on attachment 349837 [details] [diff] [review]
added new ALLOW_THIRD_PARTY_COOKIE flag

thanks for the patch, great to see some progress here! ;)

a few drive-by comments:

>   /**
>+   * This flag was added to fix bug 437174, . It allows a load to bypass an
>+   * unchecked "allow third-party cookies" box.
>+   */
>+  const unsigned long LOAD_FLAGS_ALLOW_THIRD_PARTY_COOKIE = 0x20000;

although we're adding this specifically to get around this third-party issue, this flag could be interpreted more generically as 'i want cookies for my load' - we may, for instance, eventually want to use it to bypass other restrictions in the cookieservice. perhaps we want to call it just LOAD_FLAGS_ALLOW_COOKIES, or somesuch... maybe bz has an opinion here?

>diff --git a/extensions/cookie/nsCookiePermission.cpp b/extensions/cookie/nsCookiePermission.cpp

all your changes here need updating, since bz just changed this code. ;)

>   // cases 2) and 3)
>-  if (!root || type != nsIDocShellTreeItem::typeContent)
>+  if ((!root || type != nsIDocShellTreeItem::typeContent) && 
>+      !(flags & nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_COOKIE))
>     return NS_ERROR_INVALID_ARG;

so the bug we're trying to fix here is allowing cookies when the load has no docshell, but if root is null, where are we going to get a URI from to pass back? we'll just fail a bit further down. (although bz changed all this code, this still applies, just to windows now instead of docshells.)

seems like we have two choices:
1) if root is null, and LOAD_FLAGS_ALLOW_THIRD_PARTY_COOKIE is true, just get the URI directly from the channel. not sure if this will give the right answer (bz?)

2) failing any way to get the URI, we'll have to change the GetOriginatingURI() API.
Depends on: 466681
Comment on attachment 349837 [details] [diff] [review]
added new ALLOW_THIRD_PARTY_COOKIE flag

>+++ b/docshell/base/nsIWebNavigation.idl
>   /**
>+   * This flag was added to fix bug 437174, . It allows a load to bypass an
>+   * unchecked "allow third-party cookies" box.
>+   */
>+  const unsigned long LOAD_FLAGS_ALLOW_THIRD_PARTY_COOKIE = 0x20000;

No need to list the bug number in the comment.  And I'd call this flag something like LOAD_FLAGS_FORCE_ALLOW_COOKIES perhaps, similar to what dwitte suggests.

There's a side issue here of the value for this flag; I filed bug 466681 about that mess.

>+++ b/embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl
>+   * Allow exceptional load from a third party cookie. See bug 437174.
>+   */
>+  const unsigned long PERSIST_FLAGS_ALLOW_THIRD_PARTY_COOKIE = 65536;

Again, no reason to put the bug number in the comment, and perhaps PERSIST_FLAGS_FORCE_ALLOW_COOKIES.

>+++ b/extensions/cookie/nsCookiePermission.cpp

As dwitte said, this part of the code changed heavily yesterday, so the patch needs some revamping.

>+  nsLoadFlags flags;
>+  aChannel->GetLoadFlags(&flags);

I think at this point you should just check for the appropriate load flag and if it's set return the channel URI and skip the rest of this method.  That seems like the right behavior to me...  That is, this should basically work like the "toplevel document load" case (which imo we should change to use this functionality, but that can be a followup bug).

The right flag to use here is the nsIHttpChannel flag (after QI to nsIHttpChannel)

>+++ b/netwerk/protocol/http/public/nsIHttpChannelInternal.idl
>+    /**
>+     * Allow load from third party cookie
>+     */
>+    const unsigned long FLAG_ALLOW_THIRD_PARTY_COOKIE = 0x1;

This is no good, since this is equal to LOAD_BACKGROUND.  This flag basically needs to live between the flags for nsIChannel and nsICachingChannel.  Something like (1 << 25) is probably the right flag, and ideally we'd add "all flags" bit masks to nsIChannel, nsICachingChannel, and nsIHttpChannel, and add static asserts to nsHttpChannel that the bitmasks don't overlap.

I think we might as well put this on nsIHttpChannel, since it's not changing the ABI.

And we want to call this flag FLAG_FORCE_ALLOW_COOKIES, like the others.  That said, it won't force cookies if cookies are turned off, so maybe the naming should be more nuanced.  dwitte, I'd love ideas.

This patch is missing the propagation of the flags from nsIWebNavigation and nsIWebBrowserPersist to the channel, so it doesn't actually work (even if cookies were checking for the right flag).  Ideally we'd check in tests for this; dwitte might be willing to help create some.
Attachment #349837 - Flags: review?(bzbarsky) → review-
OK, I just talked to biesi about the network end of this, and he'd really prefer a boolean attribute on nsIHttpChannelInternal (which cookies would check and consumers would set) to a channel loadflag.  Ian, if you'd be willing to do that...

Comment 75

9 years ago
thanks for the comments. i'll go over them and put in some more time over the next few days. boris, i'll email you separately so i don't clutter the page with questions.
BTW, the reason why I prefer an attribute is that this flag would only be used by the HTTP channel, and therefore I prefer not adding a new flag as there are only a limited number of flags available.

Comment 77

9 years ago
Created attachment 353293 [details] [diff] [review]
added new third party cookie flag

hopefully i'm closer to the mark this time.  thanks in advance for comments.
Attachment #349837 - Attachment is obsolete: true
Attachment #353293 - Flags: review?(bzbarsky)
Attachment #353293 - Flags: review?(bzbarsky) → review-
Comment on attachment 353293 [details] [diff] [review]
added new third party cookie flag

+++ b/docshell/base/nsDocShell.cpp
+      if (loadFlags & nsIWebNavigation::LOAD_FLAGS_FORCE_ALLOW_COOKIES) {
+        httpChannelInternal->SetForceAllowThirdPartyCookie(PR_TRUE);

But this function is what defines loadFlags, and that's an nsLoadFlags variable.  It can't possibly have nsIWebNavigation::LOAD_FLAGS_FORCE_ALLOW_COOKIES.

Your callers are going to pass this flag to LoadURI.  In the nsIURI version of LoadURI, you want to check for this load flag, and if it's set set a corresponding INTERNAL_LOAD_FLAGS_ALLOW_COOKIES flag (which you will need to add) to InternalLoad.  I'm sorry about the mess with flags here; we need to clean it up.

Then at the point where InternalLoad calls DoURILoad, you'll want to pass a boolean for the third-party cookie stuff to DoURILoad.  Just add another argument for it.

+++ b/docshell/base/nsIWebNavigation.idl
+   * Allow an exceptional load from third party cookie (for example, allows
+   * load even if "allow third-party cookies" box is unchecked.

How about:

  Force relevant cookies to be sent with this load even if normally they
  wouldn't be.

+++ b/embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl

Similar comment change here.

+++ b/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
+        nsCOMPtr<nsIHttpChannelInternal>httpChannelInternal(do_QueryInterface(inputChannel));

Toss in a pace after the '>'?  And maybe replace the constructor syntax with

  nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal =
    do_QueryInterface(inputChannel);

+        httpChannelInternal->SetForceAllowThirdPartyCookie(PR_TRUE);

You need to null-check httpChannelInternal.  It might be null.  We should have a test for persist from non-http channels.  :(

+++ b/extensions/cookie/nsCookiePermission.cpp
+  if ((ourWin == topWin) || 
+      (flags & nsIWebNavigation::LOAD_FLAGS_FORCE_ALLOW_COOKIES)) {

This isn't quite rightm because if LOAD_FLAGS_FORCE_ALLOW_COOKIES is set you'll then check for LOAD_DOCUMENT_URI, which is almost certainly going to be false.  You want to use the channel URI unconditionally if LOAD_FLAGS_FORCE_ALLOW_COOKIES.

+++ b/netwerk/protocol/http/public/nsIHttpChannelInternal.idl
+     * Force allow a load from a third-party cookie (eg if "Allow third-party
+     * cookies" is unchecked, allow load anyway)

Comment fix similar to the other IDLs here.  And put this new attribute at the end of the interface, maybe?  Also, you need to rev the iid here.

Comment 79

9 years ago
> hopefully i'm closer to the mark this time.  thanks in advance for comments.

Does the patch really allow third party overriding or is it just confusing naming?

None of the scenarios this bug impacts involve third party cookies, they are all first party loads that are mistakenly impacted by the third party setting.  Therefore no third party overriding is needed and the overriding of actual third party loads should not be allowed, meaning that no script/plugin/extension should be able to issue a top level browser request which results in third party secondary requests where cookies are allowed (with "forbid 3rd" party set).

The idea behind the fix, if I understood it correctly, is to flag a top level or root request as "first party" or "allow cookies" so it wouldn't be misidentified and treated as third party. That is not the same as overriding an actual third party restriction as SetForceAllowThirdPartyCookie and other names and comments in the patch imply.
That's a really good point.  Tim, are you willing to change the naming to something more like setIsFirstPartyLoad (for the HTTP channel IDL)?  For the other flags, I think it makes sense to keep the naming, since as dwitte pointed out we might use it for more than just third-party cookies at some point.

Updated

9 years ago
Blocks: 464404

Comment 81

9 years ago
bug 464404 is a prime candidate for this flag.
No longer blocks: 464404
Blocks: 464404
Actually, never mind comment 80.  I do think we want to leave general flexibility for using this flag for other cookie behavior overriding, so the naming in the last diff is fine; just the comments need changing.

And yes, bug 464404 is what I really want fixed here.
Duplicate of this bug: 477205

Comment 84

8 years ago
Could a separate bug be filed for AMO to work around this or at least warn users of FF3? My experimental extension is getting bad reviews and bugs filed because people can't download it without enabling third-party cookies.
(In reply to comment #84)
> Could a separate bug be filed for AMO to work around this or at least warn
> users of FF3? My experimental extension is getting bad reviews and bugs filed
> because people can't download it without enabling third-party cookies.

The case for extension installs is fixed in bug 462739 so should be working in Firefox 3.0.8
Blocks: 475238
Blocks: 469805

Comment 86

8 years ago
I have had two people attempt to download my add-on that is in a sandbox for Firefox 3.0.8 that report this issue.  Here is one of the reports:

Mozilla says:
Firefox could not install the file at
https://addons.mozilla.org/en-US/firefox/downloads/file/50594/patentlyuseful_for_the_uspto-0.5.3-fx.xpi
 

because: Invalid file hash (possible download corruption)
-261 


I will tell them to enable 3rd party cookies, but this should be fixed.

Comment 87

8 years ago
> The case for extension installs is fixed in bug 462739 so should be working in
> Firefox 3.0.8
It was renamed to Firefox 3.0.9 due to emergent ship of version 3.0.8. Bug 462739 is fixed1.9.0.9

After recent upgrade AMO doesn't require login to install experimental addons. So it seems AMO problem is soleved (tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8)

Will this bug be fixed in Firefx 3.5?
That's seeming rather unlikely, unless someone picks up the patch and addresses the review comments.

Comment 89

8 years ago
(In reply to comment #32)
> For extensions wanting to make top level requests that have been initiated by
> the user, they can use this:
> 
> var ds = Cc["@mozilla.org/webshell;1"].
>          createInstance(Ci.nsIDocShellTreeItem).
>          QueryInterface(Ci.nsIInterfaceRequestor);
> ds.itemType = Ci.nsIDocShellTreeItem.typeContent;
> var request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
>               createInstance(Ci.nsIXMLHttpRequest);
> request.open("GET", url, true);
> request.channel.loadGroup = ds.getInterface(Ci.nsILoadGroup);
> request.channel.loadFlags |= Ci.nsIChannel.LOAD_DOCUMENT_URI;
> request.send(null);
This does not work in Fx 3.5, likely because of the fix for bug 457153. Is there a functional workaround for 3.5?
The only work around I'm aware of is to add a cookie exception for any used web sites.  It's not desirable, but there's no alternative.
(Assignee)

Comment 91

8 years ago
Taking ownership.  Kathy Brade and I have revised the previous patch to account for bz's feedback in comment #78.  New patch coming soon.
Assignee: mozbugs → mcs
Keywords: helpwanted
(Assignee)

Comment 92

8 years ago
Created attachment 380431 [details] [diff] [review]
revised patch

This patch takes into account comment #78.  Please review.  The biggest change compared to the previous patch is inside nsCookiePermission::GetOriginatingURI() where we check the forceAllowThirdPartyCookie attribute on the HTTP channel very early (rather than checking the channel load flags, which did not seem correct given that we are not adding a new flag to the channel).

We also added code to set the new "force cookies" option when saving links (in nsContextMenu.js and contentAreaUtils.js).  Those changes aim to fix bug 464404 and bug 469805.
Attachment #353293 - Attachment is obsolete: true
Attachment #380431 - Flags: review?(bzbarsky)
Attachment #380431 - Flags: superreview+
Attachment #380431 - Flags: review?(bzbarsky)
Attachment #380431 - Flags: review+
Comment on attachment 380431 [details] [diff] [review]
revised patch

>+++ b/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>+        nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal = do_QueryInterface(inputChannel);

Linebreak after the '=' so as not to run over 80 columns, please.

>+    }
>     // Set the referrer, post data and headers 

And add a blank line after the '}' there.

r+sr=bzbarsky with those nits.  I'd love to see some tests here too, but I'm fine with that happening in a separate bug as needed.

Do you need this pushed to m-c once it reopens, or do you want to do it yourself?
(Assignee)

Comment 94

8 years ago
(In reply to comment #93)
> ...
> r+sr=bzbarsky with those nits.  I'd love to see some tests here too, but I'm
> fine with that happening in a separate bug as needed.
> 
> Do you need this pushed to m-c once it reopens, or do you want to do it
> yourself?

I can do it... I will fix the nits and push once the m-c tree re-opens.  Thanks for the review!
Flags: blocking1.9.2?
(Assignee)

Comment 95

8 years ago
Committed to the mozilla-central trunk:

changeset:   29288:c913f5afe23a
tag:         tip
user:        Mark Smith <mcs@pearlcrescent.com>
date:        Tue Jun 16 10:30:25 2009 -0400
files:       browser/base/content/nsContextMenu.js docshell/base/nsDocShell.cpp docshell/base/nsDocShell.h docshell/base/nsIDocShell.idl docshell/base/nsIWebNavigation.idl embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp extensions/cookie/nsCookiePermission.cpp netwerk/protocol/http/public/nsIHttpChannelInternal.idl netwerk/protocol/http/src/nsHttpChannel.cpp netwerk/protocol/http/src/nsHttpChannel.h toolkit/content/contentAreaUtils.js
description:
Bug 437174 - Disabling 3rd party cookies breaks sending cookies for
             channels with no docshell.  r+sr=bzbarsky.
  Added forceAllowThirdPartyCookie to nsIHttpChannelInternal.
  Added LOAD_FLAGS_FORCE_ALLOW_COOKIES to nsIWebNavigation.
  Added PERSIST_FLAGS_FORCE_ALLOW_COOKIES to nsIWebBrowserPersist.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 96

8 years ago
as i read through this bug, it isn't clear if the proposed fix will also fix the issue of:
save as...
save link as...
view source

all being affected by the 3rd party cookie blocking.  i have been running into those problems for a long time and only recently thought to see if 3rd party cookies were the issue.  also, is this fix going into 3.0.x?  or just 3.5?
>save as...
>save link as...

Yes.

>view source

No.  Needs a separate bug filed.

> also, is this fix going into 3.0.x?  or just 3.5?

Neither.  At the moment it's going into the version after 3.5.

Updated

8 years ago
Flags: blocking1.9.1.1?
Not going to block 1.9.1.1 on this, but we'll consider a well-baked, backported (as necessary) patch for a future 1.9.1.x release.
Flags: blocking1.9.1.1? → wanted1.9.1.x+
Flags: blocking1.9.2? → blocking1.9.2-
status1.9.1: --- → wanted
Flags: in-testsuite?

Comment 99

8 years ago
How do extensions use the new flags? Can they be used with XMLHttpRequest?
With XHR you can get its channel, QI to nsIHttpChannelInternal, and set forceAllowThirdPartyCookie.
Do extensions even need to set the forceAllowThirdPartyCookie flag?  I'm finding that in the Firefox 3.5 and above, that cookies will be sent via XHR in extensions even if 3rd party cookies are disabled regardless if the forceAllowThirdPartyCookie flag is false or true or even exists.

From my testing, the only version of Firefox where extensions using XHR won't send cookies if 3rd party cookies are disabled is Firefox 3.0.x
(Assignee)

Comment 102

8 years ago
(In reply to comment #101)
> Do extensions even need to set the forceAllowThirdPartyCookie flag?  I'm
> finding that in the Firefox 3.5 and above, that cookies will be sent via XHR in
> extensions even if 3rd party cookies are disabled regardless if the
> forceAllowThirdPartyCookie flag is false or true or even exists.

In my experience, the flag needs to be set or the extension needs to use a workaround such as setting channel.loadGroup to the load group of a browser window (at least when sending requests from inside a component).
(In reply to comment #102)
> In my experience, the flag needs to be set or the extension needs to use a
> workaround such as setting channel.loadGroup to the load group of a browser
> window (at least when sending requests from inside a component).

In Firefox 3.0 that's true.  From my testing, in Firefox 3.5 and above, the channel.loadGroup doesn't need to be set to anything, simple setting the "LOAD_DOCUMENT_URI" channel.loadFlags seems to be good enough.   This is inside a chrome window, so maybe it is different in a component.

I don't know if that's desired or not, but it's what is currently happening.  It's likely the result of fixing bug 457153.
> simple setting the "LOAD_DOCUMENT_URI" channel.loadFlags seems to be good
> enough.

Doing that in a Window is a pretty bad idea (e.g. will fire onload on the window when the load finishes).  There's a reason we added the new property...
(In reply to comment #104)
> Doing that in a Window is a pretty bad idea (e.g. will fire onload on the
> window when the load finishes).  There's a reason we added the new property...

I guess that would be bad.  Unfortunately it's the only thing that works in Firefox 3.5 (other than adding a cookie exception) since the new property doesn't exist there.

Updated

7 years ago
Duplicate of this bug: 475238

Updated

6 years ago
Blocks: 655730
You need to log in before you can comment on or make changes to this bug.