1487331 - STATE_BLOCKED_TRACKING_COOKIES should tell why cookies are blocked.

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

Instead of that state, I want to introduce these:

   /**
+   * Cookie Jar blocking
+   *
+   * These flags describe the reason of cookie jar rejection.
+   *
+   * STATE_COOKIES_BLOCKED_BY_PERMISSION
+   *   Rejected for custom site permission.
+   *
+   * STATE_COOKIES_BLOCKED_TRACKER
+   *   Rejected because the resource is a tracker and cookie policy doesn't
+   *   allow its loading.
+   *
+   * STATE_COOKIES_BLOCKED_ALL
+   *   Rejected because cookie policy blocks any cookie.
+   *
+   * STATE_COOKIES_BLOCKED_FOREIGN
+   *   Rejected because cookie policy blocks 3rd party cookies.
+   */
+  const unsigned long STATE_COOKIES_BLOCKED_BY_PERMISSION = 0x10000000;
+  const unsigned long STATE_COOKIES_BLOCKED_TRACKER       = 0x20000000;
+  const unsigned long STATE_COOKIES_BLOCKED_ALL           = 0x40000000;
+  const unsigned long STATE_COOKIES_BLOCKED_FOREIGN       = 0x80000000;

Comment 1

[Andrea Marchesini [:baku]]

Attachment: cookie_4.patch

I don't think I can avoid the change of CheckPrefs because only in that method the reject/accept decision is taken. I don't want to duplicate code, and I think this is the correct way to propagate the rejectionReason.

Tell me if you see a different approach.

Comment 2

[(no longer active)]

Comment on attachment 9005127 [details] [diff] [review]
cookie_4.patch

Review of attachment 9005127 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great!

::: netwerk/cookie/nsCookieService.cpp
@@ +4201,4 @@
>  {
>    nsresult rv;
>  
> +  uint32_t rejectedReason;

Please initialize this to 0.  Also please add a comment saying you're doing this instead of null checks further down.

@@ +4247,5 @@
>          if (aIsForeign) {
>            COOKIE_LOGFAILURE(aCookieHeader ? SET_COOKIE : GET_COOKIE, aHostURI,
>                              aCookieHeader, "third party cookies are blocked "
>                              "for this site");
> +        *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_BY_PERMISSION;

Nit: indentation

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +335,5 @@
>  {
>    MOZ_ASSERT(aWindow);
>    MOZ_ASSERT(aURI);
>  
> +  uint32_t rejectedReason;

Ditto.

@@ +485,5 @@
>  {
>    MOZ_ASSERT(aURI);
>    MOZ_ASSERT(aChannel);
>  
> +  uint32_t rejectedReason;

Ditto.

::: uriloader/base/nsIWebProgressListener.idl
@@ +303,5 @@
> +   *   Rejected because the resource is a tracker and cookie policy doesn't
> +   *   allow its loading.
> +   *
> +   * STATE_COOKIES_BLOCKED_ALL
> +   *   Rejected because cookie policy blocks any cookie.

s/any cookie/all cookies/

Comment 3

[Tanvi Vyas[:tanvi]]

This is missing a state - STATE_THIRD_PARTY_COOKIES_BLOCKED.

Comment 4

[Tanvi Vyas[:tanvi]]

I take that back - that is STATE_COOKIES_BLOCKED_FOREIGN.

Comment 5

[(no longer active)]

That's right.  It is following the verbiage around the network.cookie.cookieBehavior (BEHAVIOR_REJECT_FOREIGN).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f1881a5854
STATE_BLOCKED_TRACKING_COOKIES must tell why cookies are blocked, r=ehsan

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b9f1881a5854