Implement the attached spec for p3p cookie management

Attaching interim patch. Missing is Harish's routines for determining the privacy policy of the site. Instead there is a temporary routine in nsCookies.cpp to always return a policy of P3P_ImplicitConsent (see routine P3P_SitePolicy). Note attachment consists of patches as well as two new files. The attachment forgot to mention what directory the new files go in. They are: mozilla/extensions/cookie/resources/content/p3p_xul mozilla/extensions/cookie/resources/locale/en-US/p3p.dtd Harish, apply the patch to your local tree and then integrate it with the changes that you are making. Thanks.

Ready for review even though the hook to harish's API is just a dummy for now. cc'ing alecf for sr, harish please r.

Comment on attachment 50638 [details] [diff] [review] simpler patche -- uses one pref instead of eight >@@ -387,18 +477,41 @@ > cookie_SetWarningPref(x); > prefs->RegisterCallback(cookie_warningPref, cookie_WarningPrefChanged, NULL); > >- // Initialize for cookie_lifetimePref >- if (NS_FAILED(prefs->GetIntPref(cookie_lifetimePref, &n))) { >- n = COOKIE_Normal; >+ // Initialize for cookie_lifetime >+ cookie_SetLifetimePref(COOKIE_Normal); >+ cookie_lifetimeDays = 0; >+ cookie_lifetimeCurrentSession = PR_FALSE; Shouldn't the life time of a cookie be restricted to session by default? >+ prefs->RegisterCallback(cookie_lifetimeEnabledPref, cookie_LifetimeEnabledPrefChanged, NULL); >+ prefs->RegisterCallback(cookie_lifetimeBehaviorPref, cookie_LifetimeBehaviorPrefChanged, NULL); >+ prefs->RegisterCallback(cookie_lifetimeDaysPref, cookie_LifetimeDaysPrefChanged, NULL); Shouldn't you be using nsnull instead of NULL? >+/* >+ * returns P3P_NoPolicy, P3P_NoConsent, P3P_ImplicitConsent, or P3P_ExplicitConsent >+ * based on site >+ */ >+int >+P3P_SitePolicy(char * curURL) { >+ // to be replaced with harishd's routine when available >+ return P3P_ImplicitConsent; >+} We need to discuss about this. >+/* >+ * returns P3P_Accept, P3P_Downgrade, or P3P_Reject based on user's preferences >+ */ >+int >+cookie_P3PUserPref(PRInt32 policy, PRBool foreign) { >+ if (cookie_P3P && PL_strlen(cookie_P3P) == 8) { >+ return (foreign ? cookie_P3P[policy+1] : cookie_P3P[policy]); >+ How do you make sure that cookie_P3P array is within bounds? >+ if ((cookie_GetBehaviorPref() == PERMISSION_P3P) && >+ (cookie_P3PDecision(address, firstAddress) == P3P_Reject)) { >+ return NULL; >+ } >+ NULL --> nsnull Also, the prefs variable needs null check. Note: I haven't reviewed the xul changes, since I know little about xul. However, I will try to review js/dom stuff in it.

> Shouldn't the life time of a cookie be restricted to session by default? It's actually not limited at all by default. That is, it is the value that is specified in the cookie. However if in the pref panel the user changes the default to use restricted lifetime, the default restriction that he will see is 90 days. That's actually reasonable if you consider that the original reason for having the lifetime restriction was that users were unhappy with sites that set cookies for 30 years. Now there is an inconsistency here which I just noticed. Although the default is 90 days in the pref panel, it is 0 days in the nsCookies module. That means that if the pref is not found due to some error, 0 days will be used. I'll change that to 90 days in the nsCookies module as well. > Shouldn't you be using nsnull instead of NULL? Agreed. I'll change that throughout the file. > We need to discuss about this. (hook to harish's module) Fine. But let's not hold up this checkin because of that. When you module is ready and the interface to it is defined, we can change the few lines in my module the link to yours. > How do you make sure that cookie_P3P array is within bounds? It has to be in bounds if your interface routine returns a correct value for policy -- namely 0, 2, 4, or 6. I'll add the following assertion to make sure that you gave me a good value: NS_ASSERTION(policy == P3P_NoPolicy || policy == P3P_NoConsent || policy == P3P_ImplicitConsent || policy == P3P_ExplicitConsent, "invalid value for p3p policy"); > Also, the prefs variable needs null check. Done.

Comment on attachment 50706 [details] [diff] [review] revised patch to address harish's comments r=harishd for all but xul / dtd files

Comment on attachment 50706 [details] [diff] [review] revised patch to address harish's comments This looks ok, just two comments: When you're declaring constants in JS like: + var cookies_disabled = "2"; You should use "const" instead: + const cookies_disabled = "2"; Secondly, does it make sense to have these "virtual" preferences like "pref.advanced.cookies.disable_button.more_info" or could we figure out the "disabled" state from one of these new prefs like network.cookie.p3p or something? I just don't like the idea of exposing information about the UI through the pref-locking mechanism.

cc'ing jag for review of xul/dtd files.

> When you're declaring constants in JS like: > + var cookies_disabled = "2"; > > You should use "const" instead: > + const cookies_disabled = "2"; Consider it done! > Secondly, does it make sense to have these "virtual" preferences like > "pref.advanced.cookies.disable_button.more_info" or could we figure out > the "disabled" state from one of these new prefs like network.cookie.p3p > or something? I just don't like the idea of exposing information > about the UI through the pref-locking mechanism. This comes as a complete surprise to me. I copied the button tag from the other two buttons in the file without carefully noticing what was in them. Now that you pointed it out to me, I have no idea what any of this is for. According to cvs-blame, it was added by eddyk to fix bug 70625. Alec, sounds like you understand what this is all about. So tell me what does it do, do we need it at all, is it correct as written, and if not how should it be? There are a total of three buttons involved here -- two original buttons and one that is being added by this patch. The actual enabling and disabling of these buttons are being taken care of by javascript code.

oh ok, no problem.. what it does is disable certain parts of the UI when certain prefs are set as "locked" by MCD - basically when mozilla is deployed in an enterprise environment, certain prefs can be locked down so they can't be changed.. in order to make this explicit to the user, we just disable certain controls. normally we just match the control the pref that it controls, but I think in the case of buttons that bring up subdialogs, eddy must have created these "virtual" prefs instead of trying to using an existing pref. I'm not a fan of that technique, but perhaps in the code you copied there wasn't an obvious pref to map it to.

Alec, does your comment above mean that you are sr='ing it?

Comment on attachment 50740 [details] [diff] [review] patch to address alec's var->const comment sr=alecf Yeah, just wanted to make sure you were aware of the situation. If you change the button pref, I won't object.

Comment on attachment 50740 [details] [diff] [review] patch to address alec's var->const comment >Index: pref-cookies.xul >=================================================================== >RCS file: /cvsroot/mozilla/extensions/cookie/resources/content/pref-cookies.xul,v >retrieving revision 1.12 >diff -u -r1.12 pref-cookies.xul >--- pref-cookies.xul 2001/09/07 14:22:47 1.12 >+++ pref-cookies.xul 2001/09/25 20:26:27 >+ <checkbox id="lifetimeEnabled" label="&limitLifetime.label;" accesskey="&limitLifetime.accesskey;" >+ pref="true" preftype="bool" prefstring="network.cookie.lifetime.enabled" >+ prefattribute="checked" oncommand="setDisables();"/> >+ <hbox> >+ <html/><html/> Instead of these <html/> which I suspect you're using for spacing, you should be able to put a class="indent" on the <hbox>. >+ <radiogroup id="lifetimeBehavior" orient="vertical" autostretch="never" >+ pref="true" preftype="int" prefstring="network.cookie.lifetime.behavior" >+ prefattribute="value"> >+ <radio group="lifetimeBehavior" value="0" label="&current.label;" >+ accesskey="&current.accesskey;" >+ oncommand="setDisables();"/> >+ <hbox> >+ <radio group="lifetimeBehavior" value="1" accesskey="&days.accesskey;" >+ oncommand="setDisables();"/> >+ <textbox id="lifetimeDays" pref="true" size="4" prefattribute="value" >+ preftype="int" prefstring="network.cookie.lifetime.days"/> >+ <html>&days.label;</html> >+ </hbox> >+ </radiogroup> >+ </hbox> >+ >+ </vbox> > > <separator/> > >*********************************************************************** >****** new file extensions/cookie/resources/content/p3p.xul *********** >*********************************************************************** >--- \blank.txt Mon Jan 22 08:28:48 2001 >+++ p3p.xul Mon Sep 24 18:06:15 2001 >@@ -0,0 +1,278 @@ >+<?xml version="1.0"?> >+<!-- >+ - Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ - >+ - The contents of this file are subject to the Mozilla Public License Version >+ - 1.1 (the "License"); you may not use this file except in compliance with >+ - the License. You may obtain a copy of the License at >+ - http://www.mozilla.org/MPL/ >+ - >+ - Software distributed under the License is distributed on an "AS IS" basis, >+ - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ - for the specific language governing rights and limitations under the >+ - License. >+ - >+ - The Original Code is Mozilla Communicator. ^^^^^^^^^^^^^^^^^^^^ That doesn't look right. >+<window id="privacySettings" >+ class="dialog" >+ title="&windowtitle.label;" >+ xmlns:html="http://www.w3.org/1999/xhtml" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ orient="vertical" >+ onload="init();"> >+ >+ <script type="application/x-javascript" src="chrome://global/content/strres.js" /> Do you need strres.js? >+ function settings(level) { >+ var settings = []; >+ >+ switch (level) { >+ case low: >+ settings[0] = "a"; >+ settings[1] = "d"; >+ settings[2] = "a"; >+ settings[3] = "d"; >+ settings[4] = "a"; >+ settings[5] = "a"; >+ settings[6] = "a"; >+ settings[7] = "a"; >+ break; >+ case medium: >+ settings[0] = "d"; >+ settings[1] = "r"; >+ settings[2] = "d"; >+ settings[3] = "r"; >+ settings[4] = "a"; >+ settings[5] = "a"; >+ settings[6] = "a"; >+ settings[7] = "a"; >+ break; >+ case high: >+ settings[0] = "r"; >+ settings[1] = "r"; >+ settings[2] = "r"; >+ settings[3] = "r"; >+ settings[4] = "r"; >+ settings[5] = "r"; >+ settings[6] = "a"; >+ settings[7] = "a"; >+ break; >+ case custom: >+ break; >+ } You could make this code simpler in two ways: 1) var settings; ... settings = ['a', 'd', 'a', 'd', 'a', 'a', 'a', 'a']; ... settings = ['d', 'r', 'd', 'r', 'a', 'a', 'a', 'a']; ... settings = ['r', 'r', 'r', 'r', 'r', 'r', 'a', 'a']; 2) var settings; ... settings = "adadaaaa"; ... settings = "drdraaaa"; ... settings = "rrrrrraa"; In either case you won't need to change the code that uses this below. >+ var aHide = (level != custom); I'd call this either "hide" or "disabled", or perhaps "isCustom" (and inverse the comparator). >+ var menulist; >+ >+ for (var j=0; j<p3pLength; j++) { >+ menulist = document.getElementById("menulist_" + j); >+ menulist.disabled = aHide; >+ if (level != custom) { So the above two lines could read: menulist.disabled = !isCustom; if (!isCustom) { or: if (isCustom) { menulist.disabled = false; // keep the value to whatever it was } else { menulist.disabled = true; menulist.value=settings[j]; } >+ menulist.value=settings[j]; >+ } >+ } ----- >+ <groupbox orient="vertical"> >+ <caption label="&privacyLevel.label;"/> >+ >+ <html>&p3pDetails;</html> >+ <html></html> I think you could use a <spacer/> instead of this empty <html>. >+ <html>&choose;</html> >+ >+ <radiogroup id="p3pLevel" orient="horizontal" autostretch="never"> autostretch="never" is deprecated. You probably need align="center". >+ <radio group="p3pLevel" value="0" label="&low.label;" >+ accesskey="&low.accesskey;" oncommand="settings(low);"/> >+ <radio group="p3pLevel" value="1" label="&medium.label;" >+ accesskey="&medium.accesskey;" oncommand="settings(medium);"/> >+ <radio group="p3pLevel" value="2" label="&high.label;" >+ accesskey="&high.accesskey;" oncommand="settings(high);"/> >+ <radio group="p3pLevel" value="3" label="&custom.label;" >+ accesskey="&custom.accesskey;" oncommand="settings(custom);"/> >+ </radiogroup> >+ >+ </groupbox> >+ >+ <groupbox id="customSettingBox" orient="vertical"> >+ <caption label="&customSettings.label;"/> >+ <grid> >+ <columns> >+ <column flex="1"/> >+ <column width="120"/> >+ <column width="120"/> >+ </columns> >+ <rows> >+ <row align="center"> >+ <html></html> If you need an empty filler, use <spacer/> I'll need to apply the patch and look at some of the things I commented / suggested here.

All of jag's comments but one look good. Attaching new patch that incorporates his comments. The one comment that I did not incorporate is: > >+ - The Original Code is Mozilla Communicator. > ^^^^^^^^^^^^^^^^^^^^ > That doesn't look right. That's exactly what is specified in http://www.mozilla.org/MPL/boilerplate-1.1/fields.html.

I forgot to mention that I tested the patch with jag's changes and it worked fine.

Well, the "Mozilla Communicator" is just a bad example, in my opinion, wrong but too subtly so for everyone to pick up on it. Just use "Mozilla.org code".

I don't understand. Are you saying that the boilerplate page is wrong. Currently there are over 1000 (lxr only shows the first 1000) cases of: "The Original Code is Mozilla Communicator client code, released ..." in the code base. Are all of these wrong?

Comment on attachment 51014 [details] [diff] [review] incorporates jag's comments sr=alecf once you resolve licensing wording

I've modified the license wording per jag's suggestion, although I still don't understand why. jag, please review. Thanks.

Comment on attachment 51105 [details] [diff] [review] Patch with license modified per jag's request r=jag

Patch checked in.

Cc:ing gerv for the licensing question.

These look lovely. Thanks to morse for doing the right thing. On the Mozilla Communicator question, that example on fields.html sucks and I will change it. "mozilla.org code" is the standard for Original Code if there's no better name. "Mozilla's P3P Support" or similar might have worked here, but it really doesn't make any differerence. Gerv

I'm using 2001092803 trunk build and nothing happens when I click view privacy preferences in the security&privacy cookie panel. Other features works fine though.

There is no button labeled "View Privacy Preferences" so I'm assuming that you are referring to the "View Privacy Levels" button. Did you have "Enable cookies based on privacy levels" selected? If not, that would explain why pressing the button didn't do anything. Are you getting a javascript error when you press the button?

Yes, "View Privacy Levels" is what I meant, sorry about that :( Yes I have selected "Enable cookies based on privacy levels" selected? And Yes, I get a javascript error, which is Error: viewP3P is not defined

That's very strange. I was getting that same when, prior to checkin, I copied all my changes to a fresh tree to run some pre-checkin tests. Error was that I forgot to copy extensions/cookie/resources/content/cookieOverlay.js to that new tree. When I copied that file over, everything worked fine. So I just checked to make sure that the correct version of that file got checked it. It indeed did. Therefore I am at a loss to explain why you are getting this error. Is anyone else able to reproduce this problem?

BTW I forgot to say I'm on Win98SE OK, problem fixed. I tried with a new profile and it worked. I wondered what was wrong with my current profile. At first I thought it was one of my custom prefs.js settings, but it wasn't that. I finally traced the problem down to the XML.mfl file, which I deleted and it fixed the problem. This file was rebuilt on mozilla start and it's size is now 598Ko (Was 1.54 Mo before deletion). Why is this file related to cookie management ?. On another note, I would like to know what "downgrade" does, I didn't find any info about it anywhere. Thanks in advance.

Downgrade is a term that has been coined by microsoft regarding p3p cookie management. It refers to downgrading the lifetime of a cookie to be only for the current session regardless of what was specified in the set-cookie header.

Hmmm, now that it's mentioned, I do find the term not to be self explanatory. I think we should add a paragraph detailing "Downgrade", or change the wording to "Accept for session" or some such. If others agree, please file a new bug on this.

That would be a good idea . I also noticed that my last comment (about problem resolved) has an error. XML.mfl should be XUL.mfl. That file is in my profile folder. (you probably already got that one though)

Looks like you've got a build with FASTLOAD turned on, and that's triggering a problem somewhere. By default it's turned off (not quite ready for prime time yet I think).

Yes, i enabled fastload by editing prefs.js for testing purposes. So far it's the first problem I encounter with this setting. Anyway, deleting XUL.mfl took care of the problem and I'm happy with it. Thanks to all.

Is there already a bug filed on "// to be replaced with harishd's routine when available" ?

Markus: Bug 104894

In the future, for such cases as // to be replaced with harishd's routine when available would it be a good idea to add an additional comment? // to be replaced with harishd's routine when available // http://bugzilla.mozilla.org/show_bug.cgi?id=104894

Verified.