Closed Bug 184059 Opened 17 years ago Closed 17 years ago

cookperm.txt entries should override default cookie setting

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.5alpha

People

(Reporter: dwitte, Assigned: dwitte)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
So basically you're asking for cookie whitelisting.

*** This bug has been marked as a duplicate of 75915 ***
Status: UNCONFIRMED → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
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.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
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.
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...!
Blocks: 187304
-> dwitte ;-)
Assignee: morse → dwitte
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 199883 has been marked as a duplicate of this bug. ***
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?
Target Milestone: --- → mozilla1.4beta
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?
QA Contact: tever → cookieqa
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).
Blocks: 100573
'... accept/cookperm.txt reject' doesn't work anymore (since 1.4a), see bug 199883.
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.
Blocks: 202608
Attached patch v1 patch to enable whitelisting (obsolete) — Splinter Review
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 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 ;)
Attachment #121145 - Flags: superreview?(jaggernaut)
Attachment #121145 - Flags: review?(darin)
> 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.
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).
dan: i don't think i'm going to be able to get to this anytime soon... sorry!
s'ok, 1.4b is long gone. :)

-> 1.5a
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Blocks: 141307
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 :/
Attachment #121145 - Flags: superreview?(jaggernaut)
Attachment #121145 - Flags: superreview?(bzbarsky)
Attachment #121145 - Flags: review?(mvl)
Attachment #121145 - Flags: review?(darin)
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?  ;)
Attachment #121145 - Flags: superreview?(bzbarsky)
Attachment #121145 - Flags: review?(mvl)
Attached patch v1 (unrotted)Splinter Review
same as before, but unrotted.
Attachment #121145 - Attachment is obsolete: true
Attachment #128655 - Flags: superreview?(bzbarsky)
Attachment #128655 - Flags: review?(mvl)
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.
Attachment #128655 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #128655 - Flags: review?(mvl) → review+
checked in on trunk
Status: NEW → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Depends on: 215460
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.
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?
there's already a bug filed for that - 217286. i'm thinking maybe i should fix
that now :/
*** Bug 145690 has been marked as a duplicate of this bug. ***
Thanks again, Dan.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.