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: