Proposal: add cookies.removeAll() method #690
Conversation
Initial draft
| #### Special cases: | ||
| ```cookies.removeAll({})``` | ||
|
|
||
| >An empty details object will result in all cookies being removed. |
There was a problem hiding this comment.
I'd like some input on this. My thought was to keep the behavior the same between cookies.getAll and this implementation but it might make sense to block it or direct developers to use the already existing browserData API
There was a problem hiding this comment.
Open to thoughts, but I'd prefer to make this an error. As you mentioned, there is already the browsingData API for this, and even just during development it would be frustrating to accidentally clear all cookies. This would also allow us to match the CookieDetails object from the remove() API where name and url are currently required.
There was a problem hiding this comment.
I would also like you make {} be an error. In Safari, we only return cookies you have host permissions for in getAll({}), so that is what would be removed if we supported it with removeAll({}). However, it does seem to be too much of a foot gun.
I think requiring one key is still useful and safe.
There was a problem hiding this comment.
Thank you for the feedback @xeenon and @oliverdunk , having {} be an error works for me.
I'm inclined to agree with @xeenon, that having at least one key is useful and safe. Would requiring the caller to provide at least a url or a topLevelSite sound good?
A url allows for a similar requirement as the existing cookies.remove() and it's inclusion allows for preserving the functionality. While a topLevelSite enables the new functionality of removing all cookies associated with a single topLevelSite introduced by the API. Keeping both of these parameters optional but requiring at least one of them, matches existing APIs such as webNavigation.getFrame() that already have the pattern of having all optional parameters but returning an error when no parameters or an invalid combination of parameters are provided.
There was a problem hiding this comment.
I'm comfortable with that. Thanks!
| #### Special cases: | ||
| ```cookies.removeAll({})``` | ||
|
|
||
| >An empty details object will result in all cookies being removed. |
There was a problem hiding this comment.
Open to thoughts, but I'd prefer to make this an error. As you mentioned, there is already the browsingData API for this, and even just during development it would be frustrating to accidentally clear all cookies. This would also allow us to match the CookieDetails object from the remove() API where name and url are currently required.
| ### Behavior | ||
|
|
||
| The API will remove all cookies that match the `details` object parameter with the exception of the cases outlined in the implementation details. | ||
| If the extension does not have [host permissions](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#host_permissions) for this URL, the API call will fail. |
There was a problem hiding this comment.
If we allow calling this API without a url (see below) we should add a note here about the behavior if you only have some host permissions. Presumably, we would just remove the cookies for the sites you have access to.
aselya
changed the title
|
|
||
| ### Objective | ||
|
|
||
| This new API addresses two workflows for developers: |
There was a problem hiding this comment.
It seems that the examples you provided is how extensions would have to achieve the functionality these days. Can you rephrase to emphasize that? To someone not deeply aware of the details, it can be read as part of the proposal for something net, not of existing behavior.
There was a problem hiding this comment.
Updated the language and I would appreciate feedback on if my rephrasing addresses the issue you brought up.
| >[`url`](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/remove#url) optional: | ||
| > | ||
| >A `string` representing the URL associated with the cookie. | ||
|
|
There was a problem hiding this comment.
"Return value" is not documented in this proposal. What are you going to return?
The remove method returns the deleted cookie, and that could be reasonable to do the same in cookies.removeAll. A concrete use case is to allow for extensions to undo the removal. Currently extensions have to call cookies.get or cookies.getAll and keep track of the cookie. This tracking requires extensions to accurately predict which cookie would be removed for a given cookies.removeAll call, but it is not easy to determine which cookies would be removed.
Another option is to return nothing (undefined) and encourage extension developers to call getAll with the given parameters, under the assumption that the cookies.getAll call would return exactly the same set of cookies as matched by cookies.removeAll. In theory this is prone to TOCTOU issues, but I think that this is not that big of a deal in practice.
Yet another option is to add an option to the cookies.removeCookies call to flag whether deleted cookies should be returned.
There was a problem hiding this comment.
I think returning a list of cookies that were removed makes sense here. In addition to addressing the use case you mentioned it also matches the behavior of cookies.getAll().
aselya
changed the title
|
Wanted to add some context to why I'm proposing this. When working on updating the implementation of the cookies API to include the Despite the behavior being a bug, I found the ability to delete multiple cookies with the same call to be quite useful and thought it made sense to add it to the cookies api. |
Updating parameters to explicitly reference cookies.remove Co-authored-by: Rob Wu <rob@robwu.nl>
Add more related issues Update the motivation language to reflect the related issues.
Add link to bug in the motivation
Update the objective to be more explicit about the pain point for developers.
Update language in objective.
Update language to make `{}` and error
|
|
||
| ``` | ||
| cookies.removeAll({ | ||
| topLevelSite: “https://example.com” |
There was a problem hiding this comment.
Why is topLevelSite a separate key? I would expect it to be a member of partitionKey.
There was a problem hiding this comment.
Having topLevelSite be a separate (optional) parameter addresses the second motivation "Removal of cookies associated with a topLevelSite".
This could be replaced by having the developer pass in a partitionKey that contains the toplevelSite and no value for hasCrossSiteAncestor, such aspartitionKey:{topLevelSite: "https/example.com"}. But I wanted to provide an easier option for developers who may not be as familiar with the intricacies of partitioned cookies.
Update case: `partitionKey : {}` to match `cookies.getAll`
Update requirements of details object.
Proposal introducing a new API cookies.removeAll()