User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124) Gecko/20100407 Ubuntu/9.04 Shiretoko/3.5.9 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:126.96.36.199) 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.
Confirming Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100711 Gentoo Firefox/4.0b2pre
Created attachment 466171 [details] [diff] [review] Proposed patch + test I don't know who to request review from.
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!
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.
Created attachment 470222 [details] [diff] [review] Patch v3 + Test v3
Created attachment 474447 [details] [diff] [review] Patch v4 + Test v3 Uses do_QueryInterface instead of QueryInterface.
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).
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 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 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
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. :(
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!
Docs updated: https://developer.mozilla.org/en/DOM/window.navigator.cookieEnabled And added to Firefox 8 for developers.