Last Comment Bug 578534 - navigator.cookieEnabled does not take exceptions into account.
: navigator.cookieEnabled does not take exceptions into account.
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla8
Assigned To: Sindre Dammann
:
Mentors:
http://www.game.co.uk
: 230350 495695 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-13 17:03 PDT by Alex
Modified: 2014-04-26 02:22 PDT (History)
18 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch + test (5.72 KB, patch)
2010-08-15 14:54 PDT, Sindre Dammann
no flags Details | Diff | Splinter Review
Patch v2 + Test v2 (5.11 KB, patch)
2010-08-20 13:25 PDT, Sindre Dammann
no flags Details | Diff | Splinter Review
Patch v3 + Test v3 (5.72 KB, patch)
2010-08-28 16:50 PDT, Sindre Dammann
no flags Details | Diff | Splinter Review
Patch v4 + Test v3 (5.64 KB, patch)
2010-09-11 18:26 PDT, Sindre Dammann
bzbarsky: review+
Details | Diff | Splinter Review

Description Alex 2010-07-13 17:03:12 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100407 Ubuntu/9.04 Shiretoko/3.5.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100407 Ubuntu/9.04 Shiretoko/3.5.9

If a website tries to check whether it can set cookies by looking at navigator.cookieEnabled when cookies are disabled globally, it will return false even if there is an exception allowing the site. This can result in being redirected to a error page claiming cookies are required, when the site is actually able to set cookies perfectly fine.

Reproducible: Always

Steps to Reproduce:
1. Uncheck "Accept cookies from sites" in preferences.
2. Add an exception to allow an affected site.
3. Go to the site.
Actual Results:  
navigator.cookieEnabled will return false for the site even though it can set cookies.

Expected Results:  
navigator.cookieEnabled should return true for this site, because it is able to set cookies.
Comment 1 Aaron Slunt 2010-07-13 19:19:45 PDT
Confirming Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100711 Gentoo Firefox/4.0b2pre
Comment 2 Sindre Dammann 2010-08-15 14:54:38 PDT
Created attachment 466171 [details] [diff] [review]
Proposed patch + test

I don't know who to request review from.
Comment 3 FlagMan 2010-08-18 13:49:57 PDT
Oh great a patch!
<sarcasm>
Maybe I would have confirmed and marked this bug a dupe of bug 230350 if I would have been allowed to do so.
If you follow that link you will probably learn something about 'review process' and 'patience' there...
</sarcasm>
Seriously: thank you Sindre, this bug is annoying as hell!
Comment 4 Aaron Slunt 2010-08-18 13:57:07 PDT
*** Bug 230350 has been marked as a duplicate of this bug. ***
Comment 5 Daniel.S 2010-08-19 09:46:44 PDT
@Sindre: +1
Comment 6 Sindre Dammann 2010-08-20 13:25:03 PDT
Created attachment 467869 [details] [diff] [review]
Patch v2 + Test v2

This version uses nsCOMPtr every time, and resorts to the old behavior if any part of checking for an exception fails (The review in Bug 230350 makes it sound like this is desired). 

Also changed a bunch of other stuff so it hopefully conforms more with the Mozilla Coding style.
Comment 7 Sindre Dammann 2010-08-28 16:50:33 PDT
Created attachment 470222 [details] [diff] [review]
Patch v3 + Test v3
Comment 8 Sindre Dammann 2010-09-11 18:26:22 PDT
Created attachment 474447 [details] [diff] [review]
Patch v4 + Test v3

Uses do_QueryInterface instead of QueryInterface.
Comment 9 Jonadab the Unsightly One (Nathan Eady) 2010-09-20 12:19:49 PDT
Library catalogs based on Polaris Library Systems software are one example of a type of site known to be vulnerable to this problem.  (You can't even search the catalog if the cookie test fails -- it just tells you to enable cookies.  A per-site exception is useless, because the test still fails.)

For the same reason, if cookies are allowed globally but a site is blocked, the test will tell the site it can set cookies.  (This version of the issue is less likely to cause problems, but I thought I'd mention it anyway.)  I also created a testcase, here:
http://cgi.galion.lib.oh.us/staff/tests/cookie-enablement.html
All this page does is test navigator.cookiesEnabled and report the result (via document.write).
Comment 10 Daniel.S 2011-03-25 10:55:03 PDT
Dan, can you (or anyone else) please have a look?
I know there are other important bugs, but this patch had its review requested half a year ago. Thank you. Sorry for bugspam.
Comment 11 Brendan Eich [:brendan] 2011-07-26 14:53:03 PDT
Comment on attachment 474447 [details] [diff] [review]
Patch v4 + Test v3

Sid, can you review this patch or solicit a fresh one if this has rotted too much? Or pass r? along to a better person? Thanks,

/be
Comment 12 Boris Zbarsky [:bz] (TPAC) 2011-08-03 11:31:29 PDT
Comment on attachment 474447 [details] [diff] [review]
Patch v4 + Test v3

This looks great.  A few nits:

1)  Declare variables when you need them, not up front.
2)  Names starting with 'm' are for member variables.
3)  We should probably use the principal URI, just like we do in
    nsHTMLDocument::SetCookie.
4)  The channel is unused by the callee, and the only other caller passes null,
    so probably better to pass null.

I'll make those changes and get this checked in.  r=me
Comment 13 Boris Zbarsky [:bz] (TPAC) 2011-08-03 11:34:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad8831aa105f

Sindre, thank you for the patch!  I'm sorry that it took so long to get someone to look at it.  :(
Comment 14 Marco Bonardo [::mak] 2011-08-04 03:06:44 PDT
http://hg.mozilla.org/mozilla-central/rev/ad8831aa105f
Comment 15 Daniel.S 2011-08-04 12:53:26 PDT
*** Bug 495695 has been marked as a duplicate of this bug. ***
Comment 16 Daniel.S 2011-08-28 03:21:07 PDT
The documentation of document.cookieEnabled needs to be updated.

Notes/suggestions:

1. IE and Chrome also allow exceptions to the global cookie setting. However, IE has this issue as well, Chrome does not. Opera and Safari apparently do not allow exceptions.

2. Since cookieEnabled is not reliable, add a note that setting document.cookie and immediatly trying to read document.cookie is a more reliable method to check whether cookies can be set.

P.S. Thank you, Boris!
Comment 17 Eric Shepherd [:sheppy] 2011-08-29 11:54:54 PDT
Docs updated:

https://developer.mozilla.org/en/DOM/window.navigator.cookieEnabled

And added to Firefox 8 for developers.

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