Closed
Bug 1487093
Opened 6 years ago
Closed 6 years ago
Propagate STATE_BLOCKED_TRACKING_COOKIES correctly when something is blocked
Categories
(Core :: Security, enhancement)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(5 files, 1 obsolete file)
6.51 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
5.67 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
15.87 KB,
patch
|
ehsan.akhgari
:
review+
valentin
:
review+
|
Details | Diff | Splinter Review |
9.30 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
58.66 KB,
patch
|
ehsan.akhgari
:
feedback+
|
Details | Diff | Splinter Review |
Currently we don't call onSecurityChange() correctly when something is blocked because of one of the cookie behavior policies.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9004881 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9004882 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9004883 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9004940 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9004940 -
Attachment is obsolete: true
Attachment #9004940 -
Flags: review?(ehsan)
Attachment #9004941 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•6 years ago
|
||
I probably need to rebase this patch on top of yours, but in the meantime, here what I have.
Attachment #9004994 -
Flags: review?(ehsan)
Comment 7•6 years ago
|
||
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!
Attachment #9004881 -
Flags: review?(ehsan) → review+
Updated•6 years ago
|
Attachment #9004882 -
Flags: review?(ehsan) → review+
Comment 8•6 years ago
|
||
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.
Attachment #9004883 -
Flags: review?(ehsan) → review+
Comment 9•6 years ago
|
||
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...
Attachment #9004941 -
Flags: review?(ehsan) → review+
Comment 10•6 years ago
|
||
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?
Attachment #9004994 -
Flags: review?(ehsan) → feedback+
Assignee | ||
Updated•6 years ago
|
Attachment #9004883 -
Flags: review?(valentin.gosu)
Comment 11•6 years ago
|
||
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 :)
Attachment #9004883 -
Flags: review?(valentin.gosu) → review+
Comment 12•6 years ago
|
||
Pushed by amarchesini@mozilla.com: 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•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•