1245184 - CookieManager should remove cookies only if they match the userContextId.

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: cookie.patch

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8714917 [details] [diff] [review]
cookie.patch

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

I don't know the cookies code well, and I can't review code in netwerk/. Please check the VCS logs to find someone for the cookies code and ask a netwerk peer too.

::: netwerk/cookie/nsICookieManager.idl
@@ +16,1 @@
>  interface nsICookieManager : nsISupports

This shouldn't be necessary anymore.

@@ +38,5 @@
>     *              strings. If the target cookie is a domain cookie, a leading
>     *              dot must be present.
>     * @param aName The name specified in the cookie
>     * @param aPath The path for which the cookie was set
>     * @param aBlocked Indicates if cookies from this host should be permanently blocked

This should list aUserContextId with a short description.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8714917 [details] [diff] [review]
cookie.patch

Smaug?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714917 [details] [diff] [review]
cookie.patch

ehsan has, IIRC, played with cookies.

Comment 4

[(no longer active)]

Comment on attachment 8714917 [details] [diff] [review]
cookie.patch

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

nsICookieManager.remove() is by no way special.  It is one of the many ways we delete a cookie.  See all of the other call sites of RemoveCookieFromList().  This should be done for all of them.

Also is there any reason why you would want to support deleting other users' cookies?  That seems pretty questionable.  It's definitely the wrong thing to do at least under the generic remove() name.  If there is a use case for that, I think we should have a separate nsICookieManager API like removeFromOtherUser() or some such.  But I'm uncomfortable adding an API like this, it's too big of a footgun.

If the only use case here is doing the right thing in the face of multiple users, I would like to instead internally use the current user context ID, always.

There's a couple of code-related issues below but I suspect looking at them may be unnecessary since I'm suggesting a completely different approach.  :-)

::: netwerk/cookie/nsCookieService.cpp
@@ +2370,4 @@
>                          bool             aBlocked)
>  {
>    NeckoOriginAttributes attrs;
> +  attrs.mUserContextId = aUserContextId;

You should add this to the other Remove() override too.

::: netwerk/cookie/nsICookieManager.idl
@@ +45,5 @@
> +  void remove(in AUTF8String   aHost,
> +              in ACString      aName,
> +              in AUTF8String   aPath,
> +              in unsigned long aUserContextId,
> +              in boolean       aBlocked);

Have you checked how many add-ons use this?

Comment 5

[Andrea Marchesini [:baku]]

The reason why I changed the remove() is because, simply, it doesn't work with containers.
What we do in remove is:

  NeckoOriginAttributes attrs;
  return Remove(aHost, attrs, aName, aPath, aBlocked);

Use-case: the user opens the cookie manager from about:profiles. He/she selects 1 cookie from 1 particular container and it tries to remove it: he/she cannot, because that cookie has a particular userContextId, but we are using a generic NeckoOriginAttributes, that doesn't have such userContextId flag set. Result: we delete the cookie with UserContextId == 0, instead the selected cookie.

> If the only use case here is doing the right thing in the face of multiple
> users, I would like to instead internally use the current user context ID,
> always.

I don't know what you mean with multiple users...
but, the current way we delete cookies in RemoveCookieFromList works. We just don't have a correct remove method from the UI :)

Comment 6

[(no longer active)]

This is a regression from bug 1165267, one that we have shipped. :((

If you go to the cookie viewer and delete the cookie, you'll end up deleting the cookie for the wrong user, or not do anything at all.

Comment 7

[(no longer active)]

It is actually only a regression if you enable containers.

Comment 8

[(no longer active)]

Comment on attachment 8714917 [details] [diff] [review]
cookie.patch

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

OK, so now that I understand what this patch is doing, please ignore my comments before.  :-)  Just remove the uuid change.

::: netwerk/cookie/nsCookieService.cpp
@@ +2370,4 @@
>                          bool             aBlocked)
>  {
>    NeckoOriginAttributes attrs;
> +  attrs.mUserContextId = aUserContextId;

You should add this to the other Remove() override too.

::: netwerk/cookie/nsICookieManager.idl
@@ +45,5 @@
> +  void remove(in AUTF8String   aHost,
> +              in ACString      aName,
> +              in AUTF8String   aPath,
> +              in unsigned long aUserContextId,
> +              in boolean       aBlocked);

Have you checked how many add-ons use this?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please don't make this code userContextId specific. Use full OriginAttributes structures instead.

Comment 10

[Andrea Marchesini [:baku]]

jorgev, this patch changes nsICookieManager.remove() and it breaks several addons.
Can we inform the addon developers about it?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(Without looking at the patch, could we make such interface changes that we don't end up breaking addons? And add a warning of using deprecated method?)

Comment 12

[Andrea Marchesini [:baku]]

Attachment: cookie.patch

Comment 13

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #11)
> (Without looking at the patch, could we make such interface changes that we
> don't end up breaking addons? And add a warning of using deprecated method?)

I can add a new method, but, talking with ehsan, that seems too fragile.

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> jorgev, this patch changes nsICookieManager.remove() and it breaks several
> addons.
> Can we inform the addon developers about it?

We can include it in the compatibility communications, but we won't be able to include it in the automatic validation because "remove" is too generic of a test.

I just need that the target milestone is set when this lands so I can include it in the right blog post.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8715451 [details] [diff] [review]
cookie.patch

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

Generally speaking, if your patch contains the string "userContextId" or "appId", it is probably not handling OriginAttributes correctly.

::: browser/components/preferences/cookies.js
@@ +71,5 @@
>    _cookieEquals: function (aCookieA, aCookieB, aStrippedHost) {
>      return aCookieA.rawHost == aStrippedHost &&
>             aCookieA.name == aCookieB.name &&
> +           aCookieA.path == aCookieB.path &&
> +           aCookieA.originAttributes.userContextId == aCookieB.originAttributes.userContextId;

You should compare all attributes in the originAttributes.

We might need to add a function on ChromeUtils for comparing two OriginAttributes with each other.

@@ +285,5 @@
>            var cookie = parent.cookies[i];
>            if (item.rawHost == cookie.rawHost &&
> +              item.name == cookie.name &&
> +              item.path == cookie.path &&
> +              item.originAttributes.userContextId == cookie.originAttributes.userContextId) {

Same here.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: cookie.patch

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8716933 [details] [diff] [review]
cookie.patch

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

r=me with that fixed, or with the '!' explained.

::: browser/components/preferences/cookies.js
@@ +71,5 @@
>    _cookieEquals: function (aCookieA, aCookieB, aStrippedHost) {
>      return aCookieA.rawHost == aStrippedHost &&
>             aCookieA.name == aCookieB.name &&
> +           aCookieA.path == aCookieB.path &&
> +           !ChromeUtils.compareOriginAttributes(aCookieA.originAttributes,

Why the '!'. Below it looks like compareOriginAttributes returns true if the two attributes are the same, which is also what I would expect from an API like that.

@@ +286,5 @@
>            var cookie = parent.cookies[i];
>            if (item.rawHost == cookie.rawHost &&
> +              item.name == cookie.name &&
> +              item.path == cookie.path &&
> +              !ChromeUtils.compareOriginAttributes(item.originAttributes,

Same here.

::: dom/base/ChromeUtils.cpp
@@ +88,5 @@
>    aAttrs = attrs;
>  }
>  
> +/* static */ bool
> +ChromeUtils::CompareOriginAttributes(dom::GlobalObject& aGlobal,

Maybe name this IsOriginAttributesEqual to make it clear that this doesn't return -1, 0, 1 depending on which is "bigger". Which is what "compare"/"cmp" functions sometimes do.

@@ +92,5 @@
> +ChromeUtils::CompareOriginAttributes(dom::GlobalObject& aGlobal,
> +                                     const dom::OriginAttributesDictionary& aA,
> +                                     const dom::OriginAttributesDictionary& aB)
> +{
> +  return aA.mAddonId == aB.mAddonId &&

Juse do |return aA == aB;|

Comment 18

[Andrea Marchesini [:baku]]

> Juse do |return aA == aB;|

We don't have operator== for WebIDL dictionaries.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27f3968ae8b2

Comment 20

[Carsten Book [:Tomcat]]

backed out for build problems like https://treeherder.mozilla.org/logviewer.html#?job_id=22017228&repo=mozilla-inbound

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> We don't have operator== for WebIDL dictionaries.

We use operator!=/== everywhere else where we compare OAs. See for example [1]. I'm not sure if it's generated by the WebIDL compiler, if it's in one of the C++ subclasses that we use for OAs, or if it's the default operators provided by C++. But it works :)

[1] http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp#201

Comment 22

[Andrea Marchesini [:baku]]

We have an operator for OriginAttributes but not for OriginAttributeDictionary.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can you then do OriginAttributes(aA) == OriginAttributes(aB)?

Comment 24

[Andrea Marchesini [:baku]]

It seems that we don't want to have a public CTOR for OriginAttributes class.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Use GenericOriginAttributes?

Comment 26

[Andrea Marchesini [:baku]]

Attachment: cookie2.patch

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8723132 [details] [diff] [review]
cookie2.patch

>+NS_IMETHODIMP_(nsresult)
Why not just NS_IMETHODIMP

>+nsCookieService::RemoveNative(const nsACString &aHost,
>+                              const nsACString &aName,
>+                              const nsACString &aPath,
>+                              NeckoOriginAttributes* aOriginAttributes,
>+                              bool aBlocked)
>+{
>+  if (NS_WARN_IF(!aOriginAttributes)) {
>+    return NS_ERROR_FAILURE;
>+  }
Ah, I thought you'd let one to pass null here and if so, call Remove with 
a NeckoOriginAttributes created in this method.
But if this works, fine.

>+
>+      mozilla::NeckoOriginAttributes attrs;
>+
so I thought this wasn't possible to use here.

>+
>+// Stubs to make this test happy
>+
>+mozilla::dom::OriginAttributesDictionary::OriginAttributesDictionary() {}
ok, if this all links, fine.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab3a305b3e41
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0797af7e1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/21513aca36b4

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab3a305b3e41
https://hg.mozilla.org/mozilla-central/rev/ff0797af7e1e
https://hg.mozilla.org/mozilla-central/rev/21513aca36b4

Comment 30

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/f88f85b0de5f

Comment 31

[Andrea Marchesini [:baku]]

Why this backout?

Comment 32

[Carsten Book [:Tomcat]]

gwagner, i guess this is a fallout from your merge to b2g-i (In reply to Pulsebot from comment #30)
> Backout:
> https://hg.mozilla.org/integration/b2g-inbound/rev/f88f85b0de5f

can you take a look, no idea why pulsebot thinks this was a backout

Comment 34

[Gregor Wagner [:gwagner]]

I guess thats a bug in pulsebot. The merge contains the backout and the re-landing.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

At least one add-on (CookieCuller, [1]) was using this API and is now broken. I'm assuming that's intentional and that the add-on needs to be updated, but heads-up in case it wasn't.

[1] https://addons.mozilla.org/en-US/firefox/addon/cookieculler/

Comment 36

[Eduardo Sánchez [:esanchma (UTC+1)]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #35)
> At least one add-on (CookieCuller, [1]) was using this API and is now
> broken. I'm assuming that's intentional and that the add-on needs to be
> updated, but heads-up in case it wasn't.
> 
> [1] https://addons.mozilla.org/en-US/firefox/addon/cookieculler/

Reddit Enhancement Suite, which is used for addon advocacy, also had to fix this [1]. Potentially a lot of addons will need to be updated for this change, so maybe we could add an entry in the release notes?

[1] https://github.com/honestbleeps/Reddit-Enhancement-Suite/issues/2779

Comment 37

[Loic]

2 more add-ons affected by this change:
* Remove Cookie(s) for Site: https://addons.mozilla.org/en-US/firefox/addon/remove-cookies-for-site/
* Web Developer: https://addons.mozilla.org/en-US/firefox/addon/web-developer/

They throw errors:
NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsICookieManager.remove]
and
NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsICookieManager2.remove]

Comment 38

[Loic]

Web Developer is very popular, need doc.

Comment 39

[Tanvi Vyas[:tanvi]]

Is it time to nominate these for aurora uplift?

Baku, can you also attach the final versions of the patch to this bug.  I think there was some modifications between the patches here and what was pushed to nightly.  Thanks!

Comment 40

[Andrea Marchesini [:baku]]

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #39)
> Is it time to nominate these for aurora uplift?

These patches are currently in m-i/m-c only, right? If we uplift them we must land bug 1259169 to aurora as well otherwise we break add-ons there too. I prefer to keep it as it is and once bug 1259169 is fully reviewed and landed, we can uplift them together. What do you think?

Comment 41

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini (:baku) from comment #40)
> (In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #39)
> > Is it time to nominate these for aurora uplift?
> 
> These patches are currently in m-i/m-c only, right? If we uplift them we
> must land bug 1259169 to aurora as well otherwise we break add-ons there
> too. I prefer to keep it as it is and once bug 1259169 is fully reviewed and
> landed, we can uplift them together. What do you think?

Sorry baku, I'm getting the bugs mixed up.  

I've commented in bug 1259169.  That's the one we need to land and uplift to aurora 47.  

This bug is already landed in 47 right?  This is what caused bug 1259169?

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Tanvi if we uplift the patches in bug 1259169 to beta (46) does that affect your issue here?

Comment 43

[Tanvi Vyas[:tanvi]]

I don't think anything needs to be done here.  This patch landed in 47 and should stay that way.

Comment 44

[[e-mail]]

Comment on attachment 8716933 [details] [diff] [review]
cookie.patch

>-              cookieMgr.remove(cookie.host, cookie.name, cookie.path, false);
>+              cookieMgr.remove(cookie.host, cookie.name, cookie.path,
>+                               cookie.originAttributes, false);
Removing an enumerated cookie seems to be pretty popular. Is it too late to ask for (or write a patch to provide) a dedicated method to do this?

Comment 45

[Andrea Marchesini [:baku]]

> Removing an enumerated cookie seems to be pretty popular. Is it too late to
> ask for (or write a patch to provide) a dedicated method to do this?

We already did it.