217286 - Session cookie option overrides cookie whitelist

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Joe Harrington]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030824 Mozilla Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030824 Mozilla Firebird/0.6.1+

This is related to bug 184059, which is fixed, and could possibly be considered
part of bug 75915.

The implementation of bug 184059 allows you to set up a cookie whitelist. 
However, it seems that if you have a whitelist set up, and have your default
cookie action to enable cookies for the current session only, then sites in your
whitelist are only able to set cookies for the current session only.

I propose that your whitelist/blacklist should override the "default" actions.
In particular, I would find it useful to allow sites in the whitelist to set
permanent cookies, and all other sites to only set cookies for the current session.

In other words, cookperm.txt should override the default cookie settings. 
Currently, the default cookie settings override cookperm.txt

Reproducible: Always

Steps to Reproduce:
1. Ensure "Enable cookies for current session only" is checked
2. Put a site in your cookie whitelist (cookperm.txt)
3. Go to that site to get a permanent cookie, or update an existing permanent cookie
4. View the stored cookie

Actual Results:  
Viewing the stored cookie will show that it will expire at the end of the session.

Expected Results:  
If the site was in your whitelist, the cookie should be allowed to be stored
permanently.

Comment 1

[Darin Fisher]

dwitte, mvl: your thoughts on this?

Comment 2

[[e-mail]]

well, the reporter's analysis isn't quite correct; cookperm.txt _does_ override
most default settings, just not the lifetime-related ones.

the problem is, i can imagine cases where both interpretations would be
desirable; either allowing whitelisted sites to set cookies for session-only, or
allowing whitelisted sites to set permanent and others to set session-only.

this could be fixed in a flexible way if the cookie permissions UI allowed flags
to be set for sites (e.g. 'always block, always allow, session-only, 5 day
expiry'), and then had a 'default' behavior for sites not on the
white/blacklist. that's a different RFE, so maybe we should dupe this to it...

Comment 3

[Jeremy M. Dolan]

> i can imagine cases where both interpretations would be desirable

At some point, you have to transition to something rediculously complex like a
privacy web-proxy.

> flags to be set for sites (e.g. 'always block, ... session-only, 5 day expiry')

And THAT would be that point. That's a monsterous RFE of use to only the
incredibly paranoid.

The proposal here is a simple, logical extention of the work already done. I'd
request that this be implemented for now, and we can worry about using regexps
to flag certain cookies to 4 minute expirations later.

Using what's already implemented (everything blocked by default, except for
known sites) creates a miserable browsing experience, where many sites do not
work. Until status bar UI is added, the work done so far is pretty much useless.
Adding a session/permanent possibility instead of just blocked/permanent would
create a reasonably useable solution that power users can take advantage of now.

(Note that black-listing should override a default of session as well, so there
would be three posibilites: blacklisted would block, no entry would default to
session, while whitelisted would allow permanent entries from that host.)

Comment 4

[Darin Fisher]

>However, it seems that if you have a whitelist set up, and have your default
>cookie action to enable cookies for the current session only, then sites in
>your whitelist are only able to set cookies for the current session only.

hmm... to some users this might be expected behavior.  for example, suppose a
user forgets about some whitelisting that he did.  some time later on he goes
into his prefs, and says: "i don't want cookies to last past this browser
session."  so, he checks that option.  now, won't he be surprised to find
cookies persisting across browser sessions?  he will probably think that moz has
a bug.

my point is that it can reasonably make sense either way.  of course this
problem can be solved by either another pref or more simply with some text in
the cookie pref UI to explain that white/black-listing does or does not take
precidence over the pref values.

Comment 5

[Jeremy M. Dolan]

> some time later on he goes into his prefs, and says: "i don't want cookies to
> last past this browser session."

This is no different than him going I don't want any cookies. Whitelisted
cookies are still accepted in that case, as they should be for the session-only
default.


> some text in the cookie pref UI to explain

From the exceptions panel:

"You can specify which web sites are always or never allowed to use cookies."

Just add to that something like:

"Exceptions always overrule the default cookie action."

Comment 6

[[e-mail]]

i think it boils down to this: either behavior can be considered perfectly
logical, provided we explicitly say it in the pref window (or in help or something).

so, we should pick which one is likely to be more useful, and do that... :/

Comment 7

[[e-mail]]

and, by the way:

>And THAT would be that point. That's a monsterous RFE of use to only the
>incredibly paranoid.

don't be silly. it's a perfectly flexible extension of what we have now, and
already exists in concepts like security zones. (which would be nicely
compatible with the kind of flexible proposal i made). security zones are just a
watered-down version of being able to specify things per individual site. of
course, zones are probably more convenient in practice... if we had them, then
i'd agree that fine-grained per-site control is, well, too fine-grained.

Comment 8

[[e-mail]]

Attachment: patch v1

so, this patch is actually more about progress toward a non-#ifdefed cookie
core (GRE love) than about fixing session behavior... ;)

this basically shifts out all of the expiry-related stuff from the
cookieservice, and into where it belongs, the fork-happy nsCookiePermission.
this is kinda a prerequisite for making the whitelist override default expiry
prefs, since we need to apply the defaults only when we know the cookie hasn't
been whitelisted.

it also adds an extra permissionlist value for cookies, ACCESS_SESSION - if a
site had this permission, nsCookiePermission will allow that site to set
cookies only for the session. it has no UI support yet, but that will come; it
was so trivial to implement, i just couldn't resist. ;)

diffed against the patch in bug 220624, which basically means it's against
darin's patch v2.1 in bug 210561, since i still can't wait for it to land. :)

Comment 9

[[e-mail]]

Comment on attachment 132406 [details] [diff] [review]
patch v1

can i get some r/sr love over here?

Comment 10

[[e-mail]]

ack, this patch has a bug... new patch coming soon

Comment 11

[[e-mail]]

*** Bug 221060 has been marked as a duplicate of this bug. ***

Comment 12

[[e-mail]]

Attachment: patch v1.1

this fixes the following problem (pasted from irc):

* dwitte was doing evil things with publicly-defined values :/
<mvl> how did you fix?
<mvl> remove the publicly defined values? :)
<dwitte> pretty much :)
<dwitte> i added ACCESS_SESSION, but that was wrong.
<dwitte> you can't add random values which are intended for internal use only,
to a public API
<dwitte> i can only add it to nsCookiePermission.cpp, and then i have to munge
that value into a public one for the API.
<dwitte> since nsCookieService doesn't (and shouldn't need to) understand
ACCESS_SESSION.
<dwitte> so, it needs to be munged into ACCESS_ALLOW
<dwitte> and then nsCookiePermission handles the downgrade itself
<mvl> ah
<mvl> cookiepermission should only return "yes" or "no"
<dwitte> right

in addition, note that the adding of setters to nsICookie2 for the
expiry/isSession attrs, will not be threadsafe. (some random consumer could
change one of those fields when the cookieservice is reading it... it's racy).
but that's okay, since none of cookieservice is threadsafe. so i'm just
pointing it out for completeness.

ready for review now. ;)

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 132553 [details] [diff] [review]
patch v1.1

>+++ extensions/cookie/nsCookie.cpp	2003-09-30 04:17:53.000000000 -0700
>@@ -127,12 +127,14 @@ NS_IMETHODIMP nsCookie::GetPath(nsACStri
> NS_IMETHODIMP nsCookie::GetExpiry(PRInt64 *aExpiry)        { *aExpiry = Expiry();       return NS_OK; }
> NS_IMETHODIMP nsCookie::GetIsSession(PRBool *aIsSession)   { *aIsSession = IsSession(); return NS_OK; }
...
>+NS_IMETHODIMP nsCookie::SetExpiry(PRInt64 aExpiry)         { mExpiry = aExpiry;         return NS_OK; }
>+NS_IMETHODIMP nsCookie::SetIsSession(PRBool aIsSession)    { mIsSession = aIsSession;   return NS_OK; }

Nit: move the setters next to the getters they go with

>+++ extensions/cookie/nsCookieService.cpp	2003-10-02 16:13:12.370494536 -0700

>@@ -1410,13 +1336,12 @@ nsCookieService::SetCookieInternal(nsIUR

>   if (!cookie) {
>-    COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, cookieHeader, "unable to allocate memory for new cookie");
>     return newCookie;
>   }
why remove logging the failure?

>+++ extensions/cookie/nsCookieService.h	2003-09-30 03:52:04.000000000 -0700
>@@ -139,17 +139,14 @@ class nsCookieService : public nsICookie
>-    PRPackedBool                  mCookiesLifetimeEnabled,            // Cookie lifetime limit enabled
>-                                  mCookiesLifetimeCurrentSession,     // Limit cookie lifetime to current session
>-                                  mCookiesStrictDomains; // Optional pref to apply stricter domain checks
>+    PRPackedBool                  mCookiesStrictDomains; // Optional pref to apply stricter domain checks
>     PRUint8                       mCookiesPermissions;   // BEHAVIOR_{ACCEPT, REJECTFOREIGN, REJECT, P3P}
>-    PRInt32                       mCookiesLifetimeSec;                // Lifetime limit specified in seconds

Nit: keep the comments aligned

Comment 14

[[e-mail]]

Attachment: patch v1.2

updated per mvl's comments. thx!

>why remove logging the failure?
logging OOM just seems silly to me... it's a transient condition
(nonrepeatable) and rare, so i killed it.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 132829 [details] [diff] [review]
patch v1.2

>+    // check whether the user wants to be prompted
>+    if (mCookiesAskPermission) {
>     // default to rejecting, in case the prompting process fails
>     *aResult = PR_FALSE;

r=mvl, provided this (and in some other places) indenting is just because you
used diff -w

Comment 16

[[e-mail]]

Comment on attachment 132829 [details] [diff] [review]
patch v1.2

>r=mvl, provided this (and in some other places) indenting is just because you used diff -w

yes, of course it is :)

Comment 17

[Darin Fisher]

Comment on attachment 132829 [details] [diff] [review]
patch v1.2

>+++ extensions/cookie_217286/nsCookiePermission.cpp	2003-10-06 14:51:12.000000000 -0700

>+// additional values for nsCookieAccess, which are for internal use only
>+// (and thus do not belong in the public nsICookiePermission API). these
>+// private values could be defined as a separate set of constants, but
>+// have been chosen to extend nsCookieAccess for convenience.
>+const nsCookieAccess ACCESS_SESSION = 3;

should these values maybe start at some higher offset?	maybe they
could start at 0xFFFF or something like that.  point being that this
will help avoid collision if we go to define other public access types.


>+++ extensions/cookie_217286/nsICookie2.idl	2003-10-02 16:35:31.000000000 -0700
...
>-    readonly attribute boolean isSession;
>+    attribute boolean isSession;
...
>-    readonly attribute PRInt64 expiry;
>+    attribute PRInt64 expiry;

so now a handle to a nsICookie2 becomes a mutable entity. that is a big
change in the interface i think.  is immutability something we want to
preserve in the interface?


sr=darin

Comment 18

[[e-mail]]

>so now a handle to a nsICookie2 becomes a mutable entity. that is a big
>change in the interface i think.  is immutability something we want to
>preserve in the interface?

i think i agree with you here... i don't like the idea of doing that anymore.
the alternative is a little messier (passing out an nsInt64 expiry parameter
from CanSetCookie, so we still honor the seamonkey limit-lifetime-to-x-days
pref), but i'll code something up and update this patch...

in the longer term, i think we should decide exactly what properties of the
cookie CanSetCookie is allowed to mutate. my preferred answer is just one:
downgrade-to-session. i'd like to get rid of seamonkey's lifetime-days pref and
UI... any thoughts on that?

Comment 19

[[e-mail]]

Attachment: supplemental patch

... and back to immutability again. this pushes the isSession/expiry mutation
into cookieservice, and adds inout params to canSetCookie to effect the
changes. not as syntactically sweet, but better. (regarding the ACCESS_SESSION
value... the range allowed by nsIPermissionManager is only 0 - 15, so i chose 8
instead of 0xFFFF or something).

this is diffed against patch v1.2, and so is quite small... care to give it a
quick look? thanks!

Comment 20

[[e-mail]]

actually, ACCESS_SESSION should probably be a public value... the UI will need
to know about it eventually, so it makes sense to define it in the idl. we can
just add a comment to canAccess that ACCESS_SESSION is not an allowed return
value. i won't post a new patch just for that, but can i get the r/sr to cover it?

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 133114 [details] [diff] [review]
supplemental patch

>diff -u6 -p extensions/cookie_217286_3/nsCookiePermission.cpp extensions/cookie/nsCookiePermission.cpp
>@@ -273,63 +273,59 @@ nsCookiePermission::CanAccess(nsIURI    

>   mPermMgr->TestPermission(aURI, kPermissionType, (PRUint32 *) &perm);
>   switch (perm) {

>+  case ACCESS_SESSION:

>+  case ACCESS_DENY:

mPermMgr->TestPermission returns whatever you put in there, but for values 0,1
and 2 assumes the predefined values for unkown, allow and deny. The UI assumes
that too. Those values you use here have the same value, but don;t show where
they come from. So you should use nsIPermissionManager::ACCESS_DENY. for
session, you can use what you want, there is no predefined value for that.

>@@ -392,14 +388,13 @@ nsCookiePermission::CanSetCookie(nsIURI 
>-                     *aResult ? (PRUint32) nsIPermissionManager::ALLOW_ACTION
>-                              : (PRUint32) nsIPermissionManager::DENY_ACTION);
>+                      *aResult ? (PRUint32) ACCESS_ALLOW : (PRUint32) ACCESS_DENY);

Same here. (So, just don't change this :) )

Otherwise, looks good. r=mvl

Comment 22

[Darin Fisher]

Comment on attachment 133114 [details] [diff] [review]
supplemental patch

can you add extra comments to the IDL explaining why the nsICookiePermission
instance would want to modify these attribute.

sr=darin

Comment 23

[[e-mail]]

checked in with comments addressed. (i realized the error of my ways and left
ACCESS_SESSION private, not public. the UI will just have to deal...)

Comment 24

[[e-mail]]

hmm, -> me ;)

Comment 25

[[e-mail]]

.

Comment 26

[jon rekai]

This fix caused bug 228396

Comment 27

[Jeremy M. Dolan]

functionality verified with Fox 0.8

works beautiful guys. thanks for the work here. I can finally stop having to
manually clean out my cookies every so often.