225298 - clean up p3p hooks in cookies

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

the p3p hooks in nsCookieService are a bit messy... random switch/case
statements strewn about, and duplication of calls into the p3pservice. i also
don't think the cookieservice should be caring about the p3p pref string (the
8-letter string that defines the user's desired policy->status mapping) directly
- this would be better factored into the p3pservice.

patch coming up to clean up the hooks and move code out of the cookieservice and
into the p3pservice.

Comment 1

[[e-mail]]

Attachment: patch v1

Comment 2

[[e-mail]]

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

pike, would you be ok with reviewing this?

Comment 3

[[e-mail]]

oh, btw... there is a slight functional change in this patch. previously, we'd
get the site's p3p policy and store it with the cookie, regardless of whether
the user had his cookie prefs set to use p3p. so, this would mean the cookie
site policies for the current session of browsing would be visible in
cookiemanager, even if p3p wasn't being used.

however, storing these policies incurs a perf penalty, and if p3p isn't being
used i don't think there's any point in doing it. further, p3p policies aren't
stored in the cookie file, thus any advantage gained by unconditionally fetching
the policy would be lost in between sessions.

Comment 4

[[e-mail]]

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

pike said he knows not this code... so, i'll pick a random reviewer... mvl,
you're it ;)

Comment 5

[[e-mail]]

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

might as well ask for sr concurrently...

darin, i know p3p isn't really your cup of tea, but i don't think there's
anyone else that knows this code... mind taking a look? :/

Comment 6

[[e-mail]]

fwiw, with this patch, if your cookie pref is set to something other than p3p,
we won't init the p3p service or load libp3p.so at all... so it's quite lazy :)

Comment 7

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

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

>Index: netwerk/cookie/public/nsICookieConsent.idl

>+  nsCookiePolicy getPolicy(in nsIURI         uri,
>+                           in nsIHttpChannel httpChannel);
>+                 
>+  nsCookieStatus getConsent(in nsCookiePolicy policy,
>+                            in boolean        isForeign);

You could combine those into one method. does the cookie service are about the
policy?


>Index: netwerk/cookie/src/nsCookieService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/cookie/src/nsCookieService.cpp,v
>retrieving revision 1.15
>diff -u -6 -p -d -r1.15 nsCookieService.cpp
>--- netwerk/cookie/src/nsCookieService.cpp	11 Nov 2003 05:00:25 -0000	1.15
>+++ netwerk/cookie/src/nsCookieService.cpp	11 Nov 2003 08:45:12 -0000

>@@ -459,44 +446,26 @@ nsCookieService::Observe(nsISupports    
>+    nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
>+    PrefChanged(prefBranch);

you should check if the QI succeeded. (here, or in PrefChanged())

>Index: extensions/p3p/src/nsP3PService.cpp
>===================================================================
>@@ -84,58 +124,100 @@ nsP3PService::ProcessResponseHeader(nsIH

>-    result = ProcessResponseHeader(aHttpChannel);
>-    NS_ENSURE_SUCCESS(result,result);
>+    rv = ProcessResponseHeader(aHttpChannel);
>+    if (NS_FAILED(rv)) return rv;

just use NS_ENSURE_SUCCESS. That seems to be the module style. (And i like it
better. It asserts when something goes wrong)

>Index: extensions/p3p/src/nsP3PService.h

>+   * each of the eight characters can be one of the following:
>+   *   'a': accept the cookie
>+   *   'd': accept the cookie but downgrade it to a session cookie
>+   *   'r': reject the cookie
>+   */

You can also have f: flagged

Comment 8

[[e-mail]]

Attachment: patch v1.1

addresses comments. i merged the getPolicy and getConsent methods into one...
it was semantically nice to have them separate, but that doesn't really matter
:)

i also changed the handling of unknown nsCookieStatuses... getConsent will no
longer return nsICookie::STATUS_UNKNOWN, since that is ambiguous. if it finds
something it can't digest, it returns STATUS_REJECTED.

Comment 9

[[e-mail]]

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

carrying over r+

Comment 10

[Darin Fisher]

>+  nsCAutoString uriSpec;
>+  nsresult rv = aURI->GetSpec(uriSpec);
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (aHttpChannel) {
> 
> #ifdef NS_DEBUG
>     nsCOMPtr<nsIURI> uri;
>     aHttpChannel->GetURI(getter_AddRefs(uri));  
>     
>     if (uri) {
>-      nsCAutoString spec;
>-      uri->GetSpec(spec);
>+      nsCAutoString uriSpecFromChannel;
>+      uri->GetSpec(uriSpecFromChannel);
> 
>-      if (aURI) {
>-        NS_ASSERTION(spec.Equals(aURI), "URIs don't match");
>-      }
>+      NS_ASSERTION(uriSpec.Equals(uriSpecFromChannel), "URIs don't match");
>     }
> #endif

why not use nsIURI::equals here?  string comparisons like this are fragile
since some URI types allow case insensitive comparisons over certain parts
of the URI.  moreover, one of the URIs might have more or less escaped 
characters.  (i'm not saying that nsIURI::equals gets all of this right
either; i'm just saying that it theoretically could.)

Comment 11

[Darin Fisher]

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

sr=darin

Comment 12

[[e-mail]]

nice point... fixed, and checked in. thanks for the fast reviews!

Comment 13

[Daniel Veditz [:dveditz]]

This fix broke some proprietary legacy code I'm working on by changing the P3P
default value for error conditions from UNKNOWN to REJECTED (comment 8). In
addition, having no HTTP channel used to be handled (still is, in GetConsent)
but this change makes that a REJECTED cookie.

I guess I could change my extension to make up a channel, but I'm sure this
affects others. Since the default cookie behavior is P3P (why, oh why?) a
default REJECTED status is overly harsh. If P3P is available the default policy
in the worst case is "FLAGGED".