1387957 - browser.cookies.remove removes only one cookie, not all

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

The implementation of cookies.remove returns as soon as any cookie has been found and removed:
https://searchfox.org/mozilla-central/rev/765cc1b925c5d32d05111c364257a0b79bf2952e/toolkit/components/extensions/ext-cookies.js#348-357
        remove: function(details) {
          for (let {cookie, storeId} of query(details, ...)) {
            Services.cookies.remove(...);

            // Todo: could there be multiple per subdomain?
            return Promise.resolve({
              url: details.url,
              name: details.name,
              storeId,
            });

The answer to the TODO in the source is: yes, e.g. the query specified by "details" can match multiple domain cookies and a host-only cookie.

The expected behavior is to delete all matching cookies, and if any cookie has been deleted, return the (parsed version) of the details object. That is, a normalized URL, and -if the storeId is not set- the used storeId.

See Chromium's implementation for an example:
- Handling the request to chrome.cookies.remove: https://chromium.googlesource.com/chromium/src/+/54ed2d3d71238fd9b0605f03c7ac6f5b8c6cb793/chrome/browser/extensions/api/cookies/cookies_api.cc#471
- Calling the callback: https://chromium.googlesource.com/chromium/src/+/54ed2d3d71238fd9b0605f03c7ac6f5b8c6cb793/chrome/browser/extensions/api/cookies/cookies_api.cc#510


Do note that this chrome.cookies.remove API is useless for removing specific cookies, because of the inability to specify more parameters. To delete a specific cookie, a developer has to use browser.cookies.set with an expirationDate in the past (e.g. the value zero).

Comment 1

[Shane Caraveo (:mixedpuppy)]

Can there really be more than one cookie with the same name on the same domain/path?

chrome docs says "removes *a* cookie by name".

https://developer.chrome.com/extensions/cookies#method-remove

The details object for the callback only supports one cookie, though there is no mention whether the callback can be called multiple times.

I'm not at all familiar with chrome code, but this looks like it only deletes one cookie.

https://chromium.googlesource.com/chromium/src/+/54ed2d3d71238fd9b0605f03c7ac6f5b8c6cb793/chrome/browser/extensions/api/cookies/cookies_api.cc#503

What am I missing?

Comment 2

[Rob Wu [:robwu]]

Attachment: cookies-remove.zip

Test case:

1. Load the extension, via about:debugging (Firefox) or chrome://extensions (Chrome).
2. Open the console for the background page: "Debug" button at about:debugging (Firefox) or "Inspect views: background page" (Chrome)
3. Either type test() or click on the extension button.

The extension does the following:
- cookies.set host-only cookie [+assert cookie is set]
- cookies.set domain cookie [+assert cookie is set]
- cookies.getAll print cookies [+assert cookie count = 2]
- cookies.remove
- cookies.getAll print cookies again [+assert cookie count = 0]


Expected:
- No assertion errors, the test ends with (Chrome 62):
Number of cookies after removal is 0 []

Actual:
- The last assertion fails, the test ends with (Firefox 55):
Number of cookies after removal is 1 Array [ Object ]
Expected number of cookies to be 0

The assertion failure indicates that in Firefox, .remove removes only one cookie, whereas Chrome removes all matching cookies.



Interestingly, and worth a bug report on its own: both browsers fail the second assertion. This is because in both implementations, the cookie returned by the second cookies.set call is NOT the cookie that has just been set, but actually the first cookie.
I think that Chrome's (and also Firefox's) implementation of the cookies.set callback makes exactly the same mistake as Firefox's chrome.remove method, namely that the assumption that only one cookie can match for a given URL. This assumption is false, as shown in this test case.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I'd be interested in why you think remove should remove more than one cookie given comment 1.  My conclusion is that the api is documented and implemented to remove only one cookie.  Your test case behaves as I would expect based on documentation.  Is this a bug or a feature request?

Comment 4

[Rob Wu [:robwu]]

In my test case (comment 2) I showed that Chrome's implementation of remove does NOT remove one cookie.

If the intention is to remove only one cookie, then the parameters of cookies.remove should be extended to support enough parameters to uniquely identify a specific cookie (in the examples I have given, whether a cookie is a domain cookie or a host-only cookie - if the (new) "domain" property is present, a domain cookie, otherwise a host-only cookie. This is a behavioral change in the API and may cause compatibility issues for extensions that rely on the old behavior).
But since it is already possible to delete specific cookies through the cookies.set API (with expirationDate:<some past date>), I believe that we should just make sure that cookies.remove behaves the same across implementations.

Comment 5

[Shane Caraveo (:mixedpuppy)]

So does Chrome call the callback multiple times?  If not, how does this work considering the documented result is a single cookie that was removed?  It seems like simply removing multiple cookies here exchanges one problem for another, whereas the API should probably be changed.

Comment 6

[Rob Wu [:robwu]]

Chrome calls the callback once.

The result does not indicate anything about the actual cookie, it is simply the parsed input.

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> seems like simply removing multiple cookies here exchanges one problem for
> another, whereas the API should probably be changed.

See my previous comment for my opinion on that.

Comment 7

[Shane Caraveo (:mixedpuppy)]

I'd be fine with adding options to the call to make it more explicit what is being removed.  I'm not fine with removing multiple cookies without also changing the callback.  Chrome has either a bug in documentation or a bug in implementation because they do not match.

Comment 8

[Shane Caraveo (:mixedpuppy)]

1. the API is basically poorly designed, doesn't work well
2. Chrome implementation doesn't match doc
3. Firefox implementation does match doc

Lets define the API we want for cookie.remove (while keeping it compatible to current docs), then put that up for design-decision-needed.

We should also doc this as a compatibility issue.

Rob, would you like to define the API enhancements?

Comment 9

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> 1. the API is basically poorly designed, doesn't work well
> 2. Chrome implementation doesn't match doc
> 3. Firefox implementation does match doc
> 
> Lets define the API we want for cookie.remove (while keeping it compatible
> to current docs), then put that up for design-decision-needed.
> 
> We should also doc this as a compatibility issue.
> 
> Rob, would you like to define the API enhancements?

Ok. My reply is a bit long, so I'll give an overview here:
1. Identification of issue with current API.
2. Proposed API.
3. cookies.remove return value.
4. Analysis of API usage by extensions in the wild.
5. Points of discussion (for design-triage meeting).


### 1. Identification of issue with current API.
The fundamental issue is that the chrome.cookies.{get,remove} API assumes that every URL will have only one cookie with a given name. This is not true.
For instance, if the URL is www.example.com/sub/dir/, then a cookie with one name can ambiguously refer to:
- domain=www.example.com; path=/          <-- host-only cookie
- domain=.www.example.com; path=/
- domain=.example.com; path=/
and also, for each path:
- domain=www.example.com; path=/sub/      <-- host-only cookie
- domain=.www.example.com; path=/sub/
- domain=.example.com; path=/sub/
- domain=www.example.com; path=/sub/dir/  <-- host-only cookie
- domain=.www.example.com; path=/sub/dir/
- domain=.example.com; path=/sub/dir/

This ambiguity could be resolved by trying to match cookies with the longest path and/or domain. However, our current implementation does not even sort the results of the matched cookies. And more importantly, the API gives little control over which cookie is matched.

The fact that cookies.get API is lacking is not a critical issue, because there is a cookies.getAll method to locate all cookies. (But if we fix cookies.remove, it can make sense to fix cookies.get in a similar way).



### 2. Proposed API.
To fix cookies.remove, we need a way to match a specific cookie.
I propose to add new properties:
- domain: string | null
- hostOnly: boolean | null
- path: string | null

The behavior extends the current cookies.remove API:
- domain: if set, require the domain to be an exact match.
- hostOnly : if true, select host-only cookies. If false, select domain cookies only. If null, select either.
- path: if set, require the path to be an exact match.
- Either "url" or "domain" is REQUIRED.

Examples:
- domain: "www.example.com"
  matches: "www.example.com"
  matches: ".www.example.com"
  no match: ".example.com" (in contrast with the cookies.getAll API, subdomains of the given domain are also matched)
  no match: "sub.www.example.com"

- hostOnly: true (with domain="example.com")
  matches: "example.com"
  no match: ".example.com"

- path: "/sub/dir/"
  matches: "/sub/dir/"
  no match: "/sub/" (in contrast with the cookies.getAll API, where "parent" paths of the given path are also matched)
  no match: "/sub/dir/deeper/"


Unless all properties are specified, it is possible for a query to match more than one cookie.
Firefox currently removes the first cookie,
Chrome removes all matching cookies.



### 3. cookies.remove return value.
Currently, the return value of cookies.get is derived from the input, {url,name}.
None of the dozens of extensions that I investigated (section 4) actually care about this return value. The majority are just calling chrome.cookies.remove and not passing a callback (and if they do, they don't use the returned value).
The few extensions that do use the returned result only use it as a signal for whether the cookie was removed or not (truthy -> success, falsey -> fail).

To delete cookies, extensions typically use cookies.get/getAll first. In these cases, the contents of the return value serves no purpose.

The only way that the result is useful is when multiple cookies could match the removal query, but this can be avoided by using cookies.get/getAll and setting "domain", "hostOnly" and "path".

Hence, I see no point in adding more properties to the return value of cookies.remove.

When "url" is not specified in the parameters to details.remove, then we can either:
- omit the "url" property from the returned result object.
- or reconstruct the "url" property from the "domain" and "path" parameters.



### 4. Analysis of API usage by extensions in the wild.
I looked through the sources of WebExtension extensions on AMO (via DXR) and Chrome extensions in the Chrome Web Store*. Every single extension seemed to assume that cookies can uniquely be identified by {url,name}.
The most common pattern that I have found is calling cookies.getAll followed by cookies.remove on part of the result.
I expected to also find usages that use cookies.remove to remove all cookies for a given domain, but extensions that delete cookies either use the browsingData API, or first call cookies.getAll and then cookies.remove.

Thus, the desire to maintain Firefox' current behavior of deleting only one cookie (instead of all) seems compatible with the uses of these extensions.


* including but not limited to:
https://chrome.google.com/webstore/search/cookie%20remove?hl=EN&_category=extensions
https://chrome.google.com/webstore/detail/privacy-manager/giccehglhacakcfemddmfhdkahamfcmd version 3.2 (last updated this month)
https://chrome.google.com/webstore/detail/editthiscookie/fngmhnnpilhplaeedifhccceomclgfbg version 1.4.1 (last updated this month)
https://chrome.google.com/webstore/detail/vanilla-cookie-manager/gieohaicffldbmiilohhggbidhephnjj (last updated 2014)
https://chrome.google.com/webstore/detail/cookie-monster/cdadghomkpbpgchnhhkaijgihmgmhddp 1.1 (last updated 2014)
https://chrome.google.com/webstore/detail/cookie-crumbs/hkdjopjjoogbicnmbcenniplfnmcnhof version 0.1 (last updated jan 2017)
https://chrome.google.com/webstore/detail/vanilla-cookie-manager/gieohaicffldbmiilohhggbidhephnjj 1.4.0 (last updated 2014)
https://chrome.google.com/webstore/detail/selective-cookie-remover/nagfacmodkbckhjiimdccehaabobmnnh 1.11 (last updated 2015)



### 5. Points of discussion (for design-triage meeting).
- Do we want this proposed improvement? (section 2)
- Do we want cookies.remove to only remove the first cookie, or all matching cookies? (end of section 2 + section 4)
- Do we want cookies.remove's return value to have an empty "url" if it is not set, or should it re-constructed from the "domain" and "path" properties? (section 3)

Comment 10

[Caitlin Neiman [:caitmuenster]]

Hi Rob, this is on the agenda for the WebExtensions APIs triage on April 16, 2018. :) Anyone else who is interested in this bug is welcome to attend. 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1A7lwSkunTIdPE8FGqYV93oO-2l8xo3GFqTbY1l1QC1w/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 11

[Caitlin Neiman [:caitmuenster]]

Sorry -- that date should have been April 17, 2018! :)

Comment 12

[Shane Caraveo (:mixedpuppy)]

Rob, can you post a bug on the chrome implementation, lets see what they have to say about it before deciding further what should be done with the api.

Comment 13

[Rob Wu [:robwu]]

I have opened https://bugs.chromium.org/p/chromium/issues/detail?id=834699

As an extension developer who tried to build a cookie manager that is compatible with Firefox and Chrome (https://github.com/Rob--W/cookie-manager), my "user" experience with the API is as follows:

- cookies.get and cookies.remove  are not reliable, because they cannot uniquely match a cookie. Anyone who wishes to cover all edge cases should not use these APIs in their current form.

- cookies.getAll offers all information that an add-on needs.

- cookies.set offers enough freedom to uniquely update a cookie. Removal can be implemented by setting an expiry date in the past.


I expect that removing cookies by cookies.remove could be more efficient than using cookies.set with a past expiry date. If the performance benefit (from Firefox's perspective) is negligible, then we can keep the API in its current form and document the limitations.

Comment 14

[Mike Conca [:mconca]]

Some action will be take taken on this, either an update to the API, and update to the documentation, or both.