CookieManager should remove cookies only if they match the userContextId.

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Preferences
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 2 bugs, {addon-compat, dev-doc-needed})

unspecified
Firefox 47
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8714917 [details] [diff] [review]
cookie.patch
Attachment #8714917 - Flags: review?(ttaubert)
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.
Attachment #8714917 - Flags: review?(ttaubert)
(Assignee)

Comment 2

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

Smaug?
Attachment #8714917 - Flags: review?(bugs)
Comment on attachment 8714917 [details] [diff] [review]
cookie.patch

ehsan has, IIRC, played with cookies.
Attachment #8714917 - Flags: review?(bugs) → review?(ehsan)

Comment 4

2 years ago
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?
Attachment #8714917 - Flags: review?(ehsan) → review-
(Assignee)

Comment 5

2 years ago
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 :)
Flags: needinfo?(ehsan)

Comment 6

2 years ago
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.
Blocks: 1165267
No longer blocks: 1191418
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
Flags: needinfo?(ehsan)
Keywords: regression

Comment 7

2 years ago
It is actually only a regression if you enable containers.
Blocks: 1191418
No longer blocks: 1165267
status-firefox44: affected → ---
status-firefox45: affected → ---
status-firefox46: affected → ---
status-firefox47: affected → ---
tracking-firefox45: ? → ---
tracking-firefox46: ? → ---
tracking-firefox47: ? → ---
Keywords: regression

Updated

2 years ago
Keywords: addon-compat

Comment 8

2 years ago
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?
Attachment #8714917 - Flags: review- → review+
Please don't make this code userContextId specific. Use full OriginAttributes structures instead.
(Assignee)

Comment 10

2 years ago
jorgev, this patch changes nsICookieManager.remove() and it breaks several addons.
Can we inform the addon developers about it?
(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?)
(Assignee)

Comment 12

2 years ago
Created attachment 8715451 [details] [diff] [review]
cookie.patch
Attachment #8714917 - Attachment is obsolete: true
Attachment #8715451 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Flags: needinfo?(jorge)
(Assignee)

Comment 13

2 years ago
(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.
(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.
Flags: needinfo?(jorge)
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.
Attachment #8715451 - Flags: review?(jonas) → review-
(Assignee)

Comment 16

2 years ago
Created attachment 8716933 [details] [diff] [review]
cookie.patch
Attachment #8715451 - Attachment is obsolete: true
Attachment #8716933 - Flags: review?(jonas)
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;|
(Assignee)

Comment 18

2 years ago
> Juse do |return aA == aB;|

We don't have operator== for WebIDL dictionaries.
(Assignee)

Updated

2 years ago
Blocks: 1249606
backed out for build problems like https://treeherder.mozilla.org/logviewer.html#?job_id=22017228&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
> 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
(Assignee)

Comment 22

2 years ago
We have an operator for OriginAttributes but not for OriginAttributeDictionary.
Flags: needinfo?(amarchesini)
Can you then do OriginAttributes(aA) == OriginAttributes(aB)?
(Assignee)

Comment 24

2 years ago
It seems that we don't want to have a public CTOR for OriginAttributes class.
(Assignee)

Comment 26

2 years ago
Created attachment 8723132 [details] [diff] [review]
cookie2.patch
Attachment #8723132 - Flags: review?(bugs)
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.
Attachment #8723132 - Flags: review?(bugs) → review+

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab3a305b3e41
https://hg.mozilla.org/mozilla-central/rev/ff0797af7e1e
https://hg.mozilla.org/mozilla-central/rev/21513aca36b4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Assignee)

Comment 31

2 years ago
Why this backout?
Flags: needinfo?(cbook)
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
Flags: needinfo?(cbook) → needinfo?(anygregor)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1247517
I guess thats a bug in pulsebot. The merge contains the backout and the re-landing.
Flags: needinfo?(anygregor)

Updated

2 years ago
Depends on: 1255177

Updated

2 years ago
Depends on: 1256153
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

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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

Updated

2 years ago
Depends on: 1257484

Comment 37

2 years ago
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]
Keywords: dev-doc-needed

Comment 38

2 years ago
Web Developer is very popular, need doc.
Depends on: 1257847
(Assignee)

Updated

2 years ago
Blocks: 1259169

Updated

2 years ago
Blocks: 1260120

Updated

2 years ago
Blocks: 1251368
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!
Flags: needinfo?(amarchesini)
(Assignee)

Comment 40

2 years ago
(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?
Flags: needinfo?(amarchesini) → needinfo?(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?
Flags: needinfo?(tanvi)
Tanvi if we uplift the patches in bug 1259169 to beta (46) does that affect your issue here?
Flags: needinfo?(tanvi)
I don't think anything needs to be done here.  This patch landed in 47 and should stay that way.
Flags: needinfo?(tanvi)
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?
(Assignee)

Comment 45

2 years ago
> 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.
You need to log in before you can comment on or make changes to this bug.