578534 - navigator.cookieEnabled does not take exceptions into account.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alex]

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]

Confirming Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100711 Gentoo Firefox/4.0b2pre

Comment 2

[Sindre Dammann]

Attachment: Proposed patch + test

I don't know who to request review from.

Comment 3

[FlagMan]

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 5

[Daniel.S]

@Sindre: +1

Comment 6

[Sindre Dammann]

Attachment: 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]

Attachment: Patch v3 + Test v3

Comment 8

[Sindre Dammann]

Attachment: Patch v4 + Test v3

Uses do_QueryInterface instead of QueryInterface.

Comment 9

[Jonadab the Unsightly One (Nathan Eady)]

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]

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]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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]]

http://hg.mozilla.org/mozilla-central/rev/ad8831aa105f

Comment 16

[Daniel.S]

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]]

Docs updated:

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

And added to Firefox 8 for developers.

Comment 18

[ella sofia]

Hello,
I am an game tool developer ,Can you help how to add this in my tool .It show the Data ,My gaming tool is https://wordfeudwoordennl.com/