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 ***

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.

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...!

-> dwitte ;-)

*** 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?

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?

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).

'... 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.

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 ;)

> 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

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 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? ;)

same as before, but unrotted.

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 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.

checked in on trunk

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.