1487093 - Propagate STATE_BLOCKED_TRACKING_COOKIES correctly when something is blocked

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

Currently we don't call onSecurityChange() correctly when something is blocked because of one of the cookie behavior policies.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 0 - cookie service

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - AntiTrackingCommon::NotifyRejection

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsIChannel child propagation

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - split the state in 4

I probably need to rebase this patch on top of yours, but in the meantime, here what I have.

Comment 7

[(no longer active)]

Comment on attachment 9004881 [details] [diff] [review]
part 0 - cookie service

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

::: toolkit/components/antitracking/AntiTrackingCommon.h
@@ +91,5 @@
> +  // This method can be called on the parent process or on the content process.
> +  // The notification is propagated to the child channel if aChannel is a parent
> +  // channel proxy.
> +  static void
> +  NotifyRejection(nsIHttpChannel* aChannel);

Sorry to be a pain, but I find it a limiting choice to QI to nsIHttpChannel at the call site rather than in the body of this method.  I would much prefer if you did this the other way, that is, made this argument an nsIChannel* and QI inside NotifyRejection.  That way, if one day we decide to extend this code to support other channel types, we wouldn't need to go and modify every call site to this function...

Not a big deal though, I would also be fine if you chose to ignore this, it's more of a nit than anything!

Comment 8

[(no longer active)]

Comment on attachment 9004883 [details] [diff] [review]
part 2 - nsIChannel child propagation

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

Looks great to me, but can you please have a Necko peer also look over these changes?  r=me with that.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +814,5 @@
> +  // Can be called in EITHER the parent or child process.
> +  nsCOMPtr<nsIParentChannel> parentChannel;
> +  NS_QueryNotificationCallbacks(aChannel, parentChannel);
> +  if (parentChannel) {
> +    // This channel is a parent-process proxy for a child process request.

MOZ_ASSERT(XRE_IsParentProcess()); before this line.

@@ +824,1 @@
>    nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil = services::GetThirdPartyUtil();

MOZ_ASSERT(XRE_IsContentProcess()); before this line.

Comment 9

[(no longer active)]

Comment on attachment 9004941 [details] [diff] [review]
part 3 - tests

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

::: toolkit/components/antitracking/test/browser/head.js
@@ +26,5 @@
> +      allowList: false,
> +      callback: callbackTracking,
> +      extraPrefs,
> +      expectedBlockingNotifications,
> +    });

Much better!  This list of arguments was getting out of hand.  :-)

@@ -114,2 @@
>      add_task(async function() {
> -      info("Starting " + (blockingByCookieBehavior ? "blocking" : "non-blocking") + " cookieBehavior and " +

Hmm, I think you may need to do some rebasing here...  IIRC I was printing the numeric value of cookieBehavior in parenthesis here...

Comment 10

[(no longer active)]

Comment on attachment 9004994 [details] [diff] [review]
part 4 - split the state in 4

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

This is good, but:

  a) I would like to see if you can eliminate the checks in nsCookieService::CheckPrefs() as we discussed earlier today.  Can you give that a shot please?
  b) I'd like to look at a final version on top of the latest code that I've landed if possible.  Do you mind moving this part to a follow-up?  You can land the rest of your patches in this bug without this part...

::: netwerk/cookie/nsCookieService.h
@@ +269,5 @@
>    static bool DomainMatches(nsCookie* aCookie, const nsACString& aHost);
>    static bool IsSameSiteEnabled();
>    static bool PathMatches(nsCookie* aCookie, const nsACString& aPath);
>    static bool CanSetCookie(nsIURI *aHostURI, const nsCookieKey& aKey, nsCookieAttributes &aCookieAttributes, bool aRequireHostMatch, CookieStatus aStatus, nsDependentCString &aCookieHeader, int64_t aServerTime, bool aFromHttp, nsIChannel* aChannel, bool aLeaveSercureAlone, bool &aSetCookie, mozIThirdPartyUtil* aThirdPartyUtil);
> +  static CookieStatus CheckPrefs(nsICookiePermission *aPermissionServices, uint8_t aCookieBehavior, bool aThirdPartySession, bool aThirdPartyNonsecureSession, nsIURI *aHostURI, bool aIsForeign, bool aIsTrackingResource, bool aIsFirstPartyStorageAccessGranted, const char *aCookieHeader, const int aNumOfCookies, const OriginAttributes& aOriginAttrs, uint32_t* aRejectedState);

Uber-nit: rejected reason everywhere please.

::: uriloader/base/nsIWebProgressListener.idl
@@ +293,5 @@
>  
>    /**
> +   * Cookie Jar blocking
> +   */
> +  const unsigned long STATE_COOKIES_BLOCKED_BY_PERMISSION = 0x10000000;

Can you please document what each one of these means?

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9004883 [details] [diff] [review]
part 2 - nsIChannel child propagation

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

Looks good.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1815,5 @@
> +{
> +  LOG(("HttpChannelParent::NotifyTrackingCookieBlocked [this=%p]\n", this));
> +  if (!mIPCClosed) {
> +    MOZ_ASSERT(mBgParent);
> +    Unused << mBgParent->OnNotifyTrackingCookieBlocked();

nit: LOG the result, or NS_WARNING. Just in case we have to debug it in the future :)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3472f48b9fe
Call AntiTrackingCommon::NotifyRejection when cookies are rejected because of cookie policies, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/08c242ce352f
Implement AntiTrackingCommon::NotifyRejection, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/61a5ffad37d2
Propagate cookie blocking notification to child nsIChannel, r=ehsan, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/83df421e939a
Implement test for AntiTracking notification, r=ehsan

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f3472f48b9fe
https://hg.mozilla.org/mozilla-central/rev/08c242ce352f
https://hg.mozilla.org/mozilla-central/rev/61a5ffad37d2
https://hg.mozilla.org/mozilla-central/rev/83df421e939a