184059 - cookperm.txt entries should override default cookie setting

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126

I think that site entries in cookperm.txt should override the default cookie
behaviour (accept all/reject all/accept from originating site). This would allow
users to have a blanket policy for all cookies, but specify exceptions in
cookperm.txt.

Presently, this works for 'default accept/cookperm.txt reject' but not for
'default reject/cookperm.txt accept'. Presumably the default reject policy works
at too high a level, without checking for exceptions in cookperm.txt.

This bug blocks my recent filing (bug 183830, which is a dup of bug 33467).

Reproducible: Always

Steps to Reproduce:
1. Set default cookie permissions to 'accept, but ask before storing'.
2. Surf to a site that needs cookies to work; tell Moz to accept the cookies and
remember the decision. (-> entry added to cookperm.txt)
3. Set default cookie permissions to 'always reject'
4. Browse to same site.
Actual Results:  
Site was blocked access to cookie.

Expected Results:  
Site had an entry in cookperm.txt to allow cookie; Moz should have allowed the
cookie.

Comment 1

[Sander]

So basically you're asking for cookie whitelisting.

*** This bug has been marked as a duplicate of 75915 ***

Comment 2

[[e-mail]]

Yes.

The reason I didn't feel that bug 75915 was appropriate is because this bug 
here blocks the whitelisting functionality. For whitelisting to be implemented, 
an additional UI change will have to be made to the cookie manager. Since no 
apparent progress has been made on bug 75915 this in more than a year (please 
note, this is not a criticism - merely an observation), I felt it might be 
helpful to atomize the functionality a little further. Note also that there's 
been lots of UI debate going on in 75915, which isn't related to the more 
fundamental problem of simply allowing whitelisting from a permissions 
perspective.

After getting my head around nsCookies.cpp, I'm fairly sure I can patch this 
one. I'll go ahead and see what I can do for this bug, but the UI is something 
I don't really want to mess with - so I'll leave that to someone else in bug 
75915.

Comment 3

[Jay Davis]

How's it coming, Dan... it seems like websites have more cookies all the time
and Mozilla won't leave us alone!

I agree your approach is better for now than the whitelist bug.

Comment 4

[[e-mail]]

I'm still working on this one... the original idea of making a small patch for 
nsCookies has turned into more of a rewrite. I'm sifting thru standards 
specifications at the moment to try and figure out exactly how we should be 
behaving, and there are a few other fundamental things left to figure out; 
apart from that, it's coming along nicely.

Now, here's hoping I can find someone to r/sr...!

Comment 5

[Suresh]

-> dwitte ;-)

Comment 6

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

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

Comment 7

[[e-mail]]

okay, so now most of the rewrite has landed and we're heading to beta, it's
probably time to fix this up.

discussed with jag very briefly around a week ago. he agreed that our current
logic isn't really logical. what we currently have for cookies & images is:

  if (default pref == block)
    reject cookie/image;

  else if (default pref == block foreign sites && cookie/image is foreign)
    reject cookie/image;

  else if (permission entry exists for requesting site)
    return permission entry;

so default prefs are given precedence over site permission entries. this is
pretty bad, because it allows blacklisting but not whitelisting, as explained
above. the way we should be doing this, is shifting the site permission test to
be first, so if there's any entry (either reject or deny) we ignore default
prefs. the proposed change would affect cookies & images.

ideally, we should also add some minimal UI to support this. We could change the
options under Tools/Cookie Manager and Tools/Image Manager menus to be
context-sensitive (similar to popups). Currently we have

    Block Cookies from this Site
    Unblock Cookies from this Site

which could be changed to context-sensitive options; if your default pref is to
reject cookies, then it could be

    Allow Cookies from this Site
    Use default permissions for this Site

and if your default pref is to accept, then

    Block Cookies from this Site
    Use default permissions for this Site

or, since this context-dependence may annoy power users, we could have both the
allow and block options always present. this would allow you to easily add an
allow or block permission independent of your default pref.

so; what do people think about the logic change - can we decide this for 1.4b?

Comment 8

[benc]

That sounds good. I found this confusing enough that it slowed my running of
Tom's old cookie functional tests.

This might be better discussed in another bug, but it intersects with what you
area describing here. Can you state how you think the confirmation dialog should
work with these settings?

My impression is "blocked" means -don't bug user at all-. Perhaps we need
language that conveys three treatments of cookies:

accept, display, block?

Comment 9

[[e-mail]]

perhaps i should qualify a little. prompting the user will be untouched by this
proposal - that will happen after these checks. in other words, we check for a
site permission entry and then check the default prefs. if the cookie is
accepted after these checks, then we check the 'ask me' pref. if the user has
enabled it, then we throw the prompt dialog.

this is currently how we do things, so the prompting behavior won't change.
(note to those familiar with the code - currently the permission list lookup &
user prompting are combined into the same function call from cookie code. this
will obviously have to be split into two separate functions as a result of this
change, but that's just a minor API change and won't affect prompting
functionality).

Comment 10

[OstGote!]

'... accept/cookperm.txt reject' doesn't work anymore (since 1.4a), see bug 199883.

Comment 11

[[e-mail]]

it does work, just not the same way as before. until we fix it, you can get
around this by manually altering your permissions, if it's important to you. see
bug 199903 comment 1.

Comment 12

[[e-mail]]

Attachment: v1 patch to enable whitelisting

this patch just refactors code (and makes some small cleanups) to enable
whitelisting.

it touches two places:
a) image permission code, which is really trivial (just shifts one block of
code)
b) cookie permission code, which is slightly more involved. we now perform a
permissionlist lookup in cookie_CheckPrefs(), whereas before we did it in
nsICookiePermission::TestPermission(). so now, the latter function just handles
prompting the user.

(it may seem like nsICookiePermission isn't serving much purpose now, but it's
important to have that portion abstracted because it makes it easy for
embeddors to override/change anything they want there - e.g. it'd be easy for
embeddors to implement blocking by any cookie field they want).

there's some further cleanup that can follow this patch, but i'll just keep it
simple for now... more cleanups can wait for 1.5.

Comment 13

[[e-mail]]

Comment on attachment 121145 [details] [diff] [review]
v1 patch to enable whitelisting

darin, jag: please see comment 5 for an explanation of what this patch does.

there's no UI change yet, and (imo) the backend support can land without
affecting the applicability/operation of our current UI. so we can alter the UI
at our leisure, if we see fit.

what do you guys think about having this for 1.4b? it's fairly low-risk and has
been tested, and (as I believe jag mentioned a while ago) it really is
something we should support - our current behavior just doesn't make much
sense.

over to you ;)

Comment 14

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

> b) cookie permission code, which is slightly more involved. we now perform a
> permissionlist lookup in cookie_CheckPrefs(), whereas before we did it in
> nsICookiePermission::TestPermission(). so now, the latter function just handles
> prompting the user.

Why are you moving the check? Is that to prevent parsing in case the cookie will
not be accepted anyway?
I think the api is a bit bloated like this. Why not leave the check where it is,
and move the order of tests in nsCookiePermission? Move it when you are cleaning up.

Comment 15

[[e-mail]]

well, it really makes sense to have the two checks separate...

a) if we didn't bail at CheckPrefs when we realize there's no permission entry
for that site, and the user has cookies disabled, we'd call SetCookieInternal n
times for n cookies in the header. this is an important perf case because the
user has cookies _disabled_, we just shouldn't waste cycles.

b) you just can't put the list lookup in nsCookiePermission::TestPermission
right now. the order of checks _must_ be as follows:
   i) list lookup
   ii) default pref check
   iii) prompt user
if you switch i) and ii), you'd have to pass in the default pref decision to
nsCookiePermission, so that you don't prompt the user if they have cookies
disabled. that's no better than this patch.

c) we should be doing a permlist lookup in GetCookie too, which the current
patch deals with nicely. (we do _not_ want to prompt the user in that case).

now, if nsCookiePermission handled the default prefs, then shifting it into
::TestPermission would be a valid option. or if we just put a wrapper method
nsCookiePermission::ListLookup, that would also work, we could do that (and it
would give embeddors more flexibility to change the lookup).

Comment 16

[Darin Fisher]

dan: i don't think i'm going to be able to get to this anytime soon... sorry!

Comment 17

[[e-mail]]

s'ok, 1.4b is long gone. :)

-> 1.5a

Comment 18

[[e-mail]]

Comment on attachment 121145 [details] [diff] [review]
v1 patch to enable whitelisting

this one's been sitting a while now, so I'll move around the r/sr requests -
mvl, bz, feel free to kick this back if you don't want to review this. (these
are backend changes only, no UI yet).

see comment 7 and 9 for explanations, and also note that this patch has rotted
a tad :/

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121145 [details] [diff] [review]
v1 patch to enable whitelisting

> and also note that this patch has rotted
a tad :/

How about fixing that _then_ asking for review?  ;)

Comment 20

[[e-mail]]

Attachment: v1 (unrotted)

same as before, but unrotted.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128655 [details] [diff] [review]
v1 (unrotted)

>Index: extensions/cookie/nsCookiePermission.cpp

>+  // check if the user wants to be prompted. we only do this if a prior

"check whether"

>Index: extensions/cookie/nsCookieService.cpp

>+    mPermissionService->TestPermission(aHostURI,
>+                                       NS_STATIC_CAST(nsICookie*, NS_STATIC_CAST(nsCookie*, cookie)),
>+                                       nsnull,
>+                                       countFromHost, foundCookie,
>+                                       mCookiesAskPermission,
>+				       aListPermission,
>+                                       &permission);

What's with the indent?

>+  // 4) go thru enumerated permissions to see which one we have:

"through"

sr=bzbarsky with those nits.

Comment 22

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

Comment on attachment 128655 [details] [diff] [review]
v1 (unrotted)

r=mvl

But try to look at the UI before 1.5b is out. It might be confusing at places.
Maybe the cookie pref panel need some blurp about the exceptions list.

Comment 23

[Darin Fisher]

checked in on trunk

Comment 24

[Darin Fisher]

please see bug 215460.  seems like this patch is not worth the regression.  we
should either fix the regressions or back this out for 1.5 beta.

Comment 25

[Jeremy M. Dolan]

This doesn't seem to work under the following circumstances. I have:

%0      cookie
bugzilla.mozilla.org    0T

In my cookperm file, and cookie prefs set to "enabled, for originating site, for
current session", but sites with cookies explicitly enabled still expire at the
end of the session. Should a seperate bug be filed for this usage?

Comment 26

[[e-mail]]

there's already a bug filed for that - 217286. i'm thinking maybe i should fix
that now :/

Comment 27

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

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

Comment 28

[Frankie]

Thanks again, Dan.