Closed Bug 223782 Opened 21 years ago Closed 21 years ago

[cookies] remove support for dom.disable_cookie_[get,set] prefs

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

(Whiteboard: checklinux)

Attachments

(2 files, 2 obsolete files)

final patch
11.51 KB, patch
Details | Diff | Splinter Review
supplementary patch
5.11 KB, patch
Details | Diff | Splinter Review
it's nonsensical to have a pref for one specific mode of getting/setting a cookie, and not for the others... i don't think we should have these prefs anymore. in addition, they cause a constant pain to both users and bug triagers because the UI for these is hidden away separately from the main cookie prefs. so, if there are no objections, i'd like to go ahead and nuke them...
Attached patch nuke'm (obsolete) — — Splinter Review
this removes the backend + UI for the prefs. (it also removes some unnecessary nsIPrompt fu in nsHTMLDocument, since the cookieservice doesn't do anything with that inparam anymore. we'll rev the interface to remove it soon, but i might as well remove some bloat now, while i'm in there). i'd like to get reviews from caillon and jag/alecf here, if that's agreeable...
Attachment #134219 - Flags: superreview?(alecf)
Attachment #134219 - Flags: review?(caillon)
I dunno.. don't look at this from a cookie perspective, look at this from a script perspective. This is just one of many pieces of script granularity in that pref pane. I'm going to theorize that setting a cookie from an HTTP header requires control of the server, whereas using a script allows pages like those on geocities to change cookies. (I could at least be partly wrong in that <meta> tags might allow you to set cookies, though I believe sites like geocities don't allow you to set <meta> tags, you can only edit the body of the document) is there CVS blame for this pref, and justification in whatever bug this was? I would be interested to know if anyone uses this setting for a particular purpose. Anyway, I'm not totally against this, I just want to see better justification than "it's nonsensical..."
>(I could at least be partly wrong in that <meta> tags might >allow you to set cookies, though I believe sites like geocities don't allow you >to set <meta> tags, you can only edit the body of the document) hmm.. couldn't a page on geocities document.write the <meta> tag? anyways, i think we parse <meta> tags that appear in the <body> section to account for authors who forget to put them in the <head> section. if we disable document.cookies, then it seems like we should also disable setting cookies via <meta> tags. otherwise, it does seem pretty bogus.
>I could at least be partly wrong in that <meta> tags might allow you to set >cookies yeah, like darin said, you can set cookies from metatags :) >is there CVS blame for this pref it looks like the original bug here was bug 75731, where mpt drew up a spec for the prefpanel which included the cookie prefs: http://bugzilla.mozilla.org/attachment.cgi?id=49497&action=view i grepped around the bug and didn't find any arguments as to why that particular pref should exist. >I just want to see better justification than "it's nonsensical..." fair enough :) in addition to darin's argument (which doesn't alone justify removal), i'd say the best reason for removing it is that it's inconsistent with the other prefs there. the other prefs deal with visual things that shouldn't affect script function - the script doesn't care if moving/resizing a window failed; it just might mess up the layout a bit. cookies are different. blocking cookies from a script or site very often breaks it, and it's up to the user to figure out what caused that breakage (or come here and file a bug). for that reason, i'd argue that blocking cookies for all scripts is not a useful thing to do. we have another mechanism for blocking particular sites from manipulating cookies - the permissionmanager. i think it's more logical to control cookies on a site-by-site basis than on a mechanism basis, and i don't think we need both.
Comment on attachment 134219 [details] [diff] [review] nuke'm sounds reasonable to me then! thanks for going into it a bit more. The code removal looks good... and you're sure we don't need that prompter.. sr=alecf
Attachment #134219 - Flags: superreview?(alecf) → superreview+
hmm... i know the cookies backend can usually get a prompter from the channel, but what if the channel does not have one? is there value in providing one explicitly? ... because at least in this case, we seem to know that we can get the right one. :-/
yeah mvl and i were just talking about that... currently of course we do nothing with the nsIPrompt that's passed in, so as it stands the removal is fine, but maybe we do want to use a passed-in prompter if the channel's one is fubar. or should the nsHTMLDocument code just set the channel's prompter itself? so i'll forego the removal of the prompter code and just save it for another bug, then. ;)
Comment on attachment 134219 [details] [diff] [review] nuke'm I think I own the actual cvsblame for this, by way of a patch from doron. Anyway, benc and I have had discussions about this and have come to the conclusion that this pref needs to go. Aside from the afore mentioned reasons, benc notes that it confuses users who read about the scripts prefs one of the many "tips" sites which reference this panel. They see the prefs here and think these prefs are the main cookie get/set prefs. This patch is generally fine, however we need to (properly) address the issue that this got checked in for: we cannot throw exceptions if the cookie service does not exist. The existing route did a lame job of it by making you set the pref. nsHTMLDocument::GetCookie and SetCookie should never fail unless we are propogating an OOM error. Also, I'd like to see jst's input on this patch since he was heavily involved with this going in to the tree.
Attachment #134219 - Flags: review?(caillon) → review-
Attached patch round 2 (obsolete) — — Splinter Review
thanks for the tight review caillon! this puts the prompt fu back in per darin, and fixes the error propagation for the no-cookieservice case.
Attachment #134219 - Attachment is obsolete: true
Comment on attachment 134298 [details] [diff] [review] round 2 Index: content/html/document/src/nsHTMLDocument.cpp >+ // not having a cookie service isn't an error >+ nsCOMPtr<nsICookieService> service = do_GetService(kCookieServiceCID); > if (service) { ... >+ rv = service->GetCookieString(codebaseURI, mChannel, getter_Copies(cookie)); > if (NS_SUCCEEDED(rv) && cookie) > CopyASCIItoUCS2(nsDependentCString(cookie), aCookie); > } > return rv; > } nit: and GetCookieString only fails if there is an out-of-memory condition? that isn't required by the interface is it? doesn't that make this somewhat fragile? sr=darin (but maybe still get jst's approval per caillon)
Attachment #134298 - Flags: superreview+
Comment on attachment 134298 [details] [diff] [review] round 2 yeah, it would be safer to not propagate rv from GetCookieString() and SetCookieString(). caillon said he's ok with the patch if i make that change. jst, with that in mind, care to do the honors? ;)
Attachment #134298 - Flags: review?(jst)
Comment on attachment 134298 [details] [diff] [review] round 2 Yeah, r=jst with that change.
Attachment #134298 - Flags: review?(jst) → review+
Attached patch final patch — — Splinter Review
this is what i checked in.
Attachment #134298 - Attachment is obsolete: true
i missed a bunch of all.js files sprawled around the tree, in my original patch. this covers them.
mkaply, any chance you could check in http://bugzilla.mozilla.org/attachment.cgi?id=134794&action=view (it touches mail/ and browser/) ? thanks!
*** Bug 143810 has been marked as a duplicate of this bug. ***
urgh, i didn't notice that older bug... thanks jesse!
Fix checked in to composer, browser, mail, calendar
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Does this mean it's no longer possible to allow server cookies for a domain (e.g. mozillazine.com) and concurrently disable JavaScript access to cookies on that domain? I've had dom.disable_cookie_[get,set] set to false for all sites to prevent the possibility of "cookie-grabber" code from gaining access to my cookies. This is a valid concern on all sites with dynamic user-created content (e.g. forums, gaming sites). Websites rarely use scripting for essential cookies, and I'd like to be able to universally disable that mechanism. If it's possible to achieve this with a CAPS policy, please post that information here for the sake of people who have been relying on dom.disable_cookie_[get,set] up to this point.
capability.policy.default.HTMLDocument.cookie.get and capability.policy.default.HTMLDocument.cookie.set can be set to noAccess
Thanks very much, Boris. Good to know there's also a possibility of scripted cookie whitelisting through CAPS policies.
V/fixed. Mozilla 1.6f, Mac OS X.
Status: RESOLVED → VERIFIED
QA Contact: cookieqa → benc
Whiteboard: checkwin checklinux
V/fixed: mozilla 1.7.2/xp
Whiteboard: checkwin checklinux → checklinux
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: