Closed Bug 1487331 Opened Last year Closed Last year

STATE_BLOCKED_TRACKING_COOKIES should tell why cookies are blocked.

Categories

(Core :: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file)

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;
Attached patch cookie_4.patchSplinter Review
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.
Assignee: nobody → amarchesini
Status: NEW → ASSIGNED
Attachment #9005127 - Flags: review?(ehsan)
Blocks: 1487390
Blocks: 1487396
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/
Attachment #9005127 - Flags: review?(ehsan) → review+
This is missing a state - STATE_THIRD_PARTY_COOKIES_BLOCKED.
I take that back - that is STATE_COOKIES_BLOCKED_FOREIGN.
That's right.  It is following the verbiage around the network.cookie.cookieBehavior (BEHAVIOR_REJECT_FOREIGN).
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f1881a5854
STATE_BLOCKED_TRACKING_COOKIES must tell why cookies are blocked, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/b9f1881a5854
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.