Allow WebExtensions to control cookie behaviour

RESOLVED FIXED in Firefox 59

Status

P2
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla59
dev-doc-complete
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: triaged)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
This will use browserSetting [1] to manage which extension has control over the pref. This does not really belong in privacy so will go into the browserSettings API.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/BrowserSetting
(Assignee)

Comment 1

2 years ago
It was proposed in [1] that this can be done by changing the `network.cookie.cookieBehavior` pref. Also, I retract my statement above about this not belonging in privacy. I think it makes sense there, as privacy already has a setting for thirdPartyCookiesAllowed (which we have not implemented yet) and whether to allow cookies to be recorded or not can be considered a privacy concern.

So this will likely be implemented as browser.privacy.cookieBehavior, with the values being the 4 available values of the network.cookie.cookieBehavior pref [2], namely:

BEHAVIOR_ACCEPT         = 0; // allow all cookies
BEHAVIOR_REJECT_FOREIGN = 1; // reject all third-party cookies
BEHAVIOR_REJECT         = 2; // reject all cookies
BEHAVIOR_LIMIT_FOREIGN  = 3; // reject third-party cookies unless the
                             // eTLD already has at least one cookie

[1] https://docs.google.com/document/d/1HZiI-dqFATzLpmTtN1Bg6tD8y4N0Ne3D74LEWWgttOk/edit#
[2] http://searchfox.org/mozilla-central/source/netwerk/cookie/nsICookieService.idl#76
Summary: Implement API to change the network.cookie.cookieBehavior pref → Allow WebExtensions to disable cookies
(Assignee)

Comment 2

2 years ago
Andrea, could you please take a look at the WebExtensions API proposal in comment 1 above and let me know whether it seems sound? Will flipping the above pref give us the results we expect? Is there anything else we need to consider for an API that will allow extensions to change cookie behaviour?
Flags: needinfo?(amarchesini)
The proposal sounds reasonable and yes, using network.cookie.cookieBehavior does the wanted behavior.

Note that this is not per domain, but it is a global preference. Basically you are changing the browser behavior (and this seems to be what you want). But if you want to have a similar behavior but per domain, you must change the permission for that domain. See nsICookiePermission.idl:

  /**
   * nsCookieAccess values
   */
  const nsCookieAccess ACCESS_DEFAULT = 0;
  const nsCookieAccess ACCESS_ALLOW   = 1;
  const nsCookieAccess ACCESS_DENY    = 2;

  /**
   * additional values for nsCookieAccess which may not match
   * nsIPermissionManager. Keep 3-7 available to allow nsIPermissionManager to
   * add values without colliding. ACCESS_SESSION is not directly returned by
   * any methods on this interface.
   */
  const nsCookieAccess ACCESS_SESSION = 8;
  const nsCookieAccess ACCESS_ALLOW_FIRST_PARTY_ONLY = 9;
  const nsCookieAccess ACCESS_LIMIT_THIRD_PARTY = 10;
Flags: needinfo?(amarchesini)
(Assignee)

Comment 4

2 years ago
Thanks Andrea. Yes, this is to control global cookie behaviour. Another bug may be raised in the future to control cookies per domain, so the above information will be useful if and when we get to that.
(Assignee)

Comment 5

2 years ago
Chrome already has a privacy setting called `websites.thirdPartyCookiesAllowed` which we have not yet implemented. That controls cookies, but only third-party cookies, so while we should add a setting for privacy.websites.cookieBehavior (as described above), I think we should also implement websites.thirdPartyCookiesAllowed to increase our Chrome compatibility. This bug will cover adding both of those.

This will be the first time we have encountered a situation in which two different settings want to alter the same pref, and the PreferencesManager isn't really able to handle that correctly at the moment. I think a simple solution to that, however, is to just map the websites.thirdPartyCookiesAllowed API to the same setting internally as the websites.cookieBehavior API.
(Assignee)

Comment 6

2 years ago
Looking at this in more detail, I think implementing Chrome's websites.thirdPartyCookiesAllowed may be more complicated than it is worth. For example, what happens if extension A has set cookieBehavior to "reject" and extension B, which was installed later, then tries to set thirdPartyCookiesAllowed to "false"? Do we simply do nothing, because "reject" includes thirdPartyCookiesAllowed == false, or do we change the behavior to "reject_foreign" because that is what extension B is asking us to do. In this case I think the answer is the former, but it means we have to add a bunch of logic that checks the current value of the pref before doing anything, which is something we have not done previously with ExtensionPreferencesManager. What I describe above is only one such case - there are a number of combinations that we'd have to address.

We can do this, but I wonder if it makes more sense to only implement websites.cookieBehavior, which provides the same functionality as websites.thirdPartyCookiesAllowed (and then some), and treat websites.thirdPartyCookiesAllowed as deprecated. I'm not really sure which is a better approach. Andrew and/or Andy, what do you think?
Flags: needinfo?(aswan)
Flags: needinfo?(amckay)
I think the bigger problem here is not conflicts between extensions setting those two preferences but the fact that websites.cookieBehavior is settable from about:preferences which raises all the same problems as bug 1361364...

For the more specific question though, my initial reaction is that trying to handle cross-browser compatibility at this layer is a mistake.  We should expose an API that matches the underlying preference (assuming that the existing preference is something that makes sense to the necko team and not something they wish to change) and let extensions (or javascript libraries used by extension authors) sort out cross-browser issues.
Flags: needinfo?(aswan)

Comment 8

2 years ago
We already have this problem with home page URLs. Let's file a new bug for dealing with this in the API and as a separate project work on improving the visibility to users within Firefox showing what an extension has changed.
Flags: needinfo?(amckay)
(Assignee)

Updated

2 years ago
Depends on: 1368537
(Assignee)

Comment 9

2 years ago
We're planning to use permissions for this now, after 1313939 is implemented.
Depends on: 1313939
(Assignee)

Updated

2 years ago
No longer depends on: 1368537
Depends on: 1379560
No longer depends on: 1313939
Keywords: dev-doc-needed
It would be great if the preference "network.cookie.lifetimePolicy" could also be exposed in some way. I have an extension that toggles the browser between rejecting all cookies (except for permanently whitelisted sites) and accepting cookies from any site as session cookies. Right now I can't do this as a WebExtension.
(Assignee)

Updated

a year ago
Blocks: 1390160
I'm adding Bug 1406675 to the "See Also" section of this bugzilla issue.

The reason is that in Bug 1406675 we have determined that by setting "network.cookie.cookieBehavior" about:config preference to 2, which is COOKIE_BEHAVIOR_REJECT, has currently the side effect of preventing the extension pages to use IndexedDB and localStorage.

It looks that we are not going to change the "network.cookie.cookieBehavior" about:config preference to achieve this, nevertheless it still looks reasonable to test explicitly that the strategy that we are going to use to implement this doesn't have the same side-effect on the extension pages (and that if it has this side-effect, we have to fix it as well).
See Also: → bug 1406675

Comment 12

a year ago
The uMatrix WebExtension can allow or block cookies: https://addons.mozilla.org/en-US/firefox/addon/umatrix/
So this bug can be marked as fixed, no?
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
Comment on attachment 8932975 [details]
Bug 1363860 - Part 1: Add cookies to the set of permissions that can receive a global default,

Johann, this doesn't seem to work. I'm guessing we need more code that just this small change, but I'm not sure what else needs to change. Any advice you have would be appreciated.
Hmm, without having tried this patch, as I mentioned somewhere already, with cookies and popups we have a different situation than with other permissions, e.g. Notifications or Geolocation.

For cookies, there already is a global default pref which is sort of accepted everywhere, that means we have special-cased it in all places (except in the permission manager) to act as a fallback for the cookie permissions. That pref is network.cookie.cookieBehavior. As a long-term project one could consider migrating this pref to permissions.default.cookies, but at the moment I'm not sure whether it's correct to use permissions.default.cookies in its place. All UI we have currently accepts network.cookie.cookieBehavior as the default setting for cookie permissions (such as in about:preferences). It would be very valuable to have an indication in our UI that the pref was changed by WebExtensions and I think two default prefs aren't helpful in that regard.

I understand that network.cookie.cookieBehavior is giving you problems outlined in comment 11, but I'm not sure whether that would be any different with permissions.default.cookies (maybe it would, I don't know yet).

Sorry for that. Can you let me know if there are any other concerns around using network.cookie.cookieBehavior instead?
(Assignee)

Comment 17

a year ago
Thanks Johann. Based on the above, we can go back to implementing browser.privacy.cookieBehavior as described in comment 1. We will also need to hold off landing this until bug 1406675 is fixed, as per comment 11.
(Assignee)

Comment 18

a year ago
Updating this to also deal with reading and writing the network.cookie.lifetimePolicy pref, which would allow extensions to control the lifetime of cookies. The three allowable values for network.cookie.lifetimePolicy [1] are:

ACCEPT_NORMALLY   = 0; // accept normally
// Value = 1 is considered the same as 0 (See Bug 606655).
ACCEPT_SESSION    = 2; // downgrade to session
ACCEPT_FOR_N_DAYS = 3; // limit lifetime to N days

This would be exposed as browser.privacy.websites.cookieLifetimePolicy, which will be in addition to browser.privacy.websites.cookieBehavior.

[1] https://searchfox.org/mozilla-central/rev/477ac066b565ae0eb3519875581a62dfb1430e98/netwerk/cookie/nsICookieService.idl#85-99
Summary: Allow WebExtensions to disable cookies → Allow WebExtensions to control cookie behaviour
(Assignee)

Comment 19

a year ago
(In reply to Bob Silverberg [:bsilverberg] from comment #18)
> Updating this to also deal with reading and writing the
> network.cookie.lifetimePolicy pref, which would allow extensions to control
> the lifetime of cookies. The three allowable values for
> network.cookie.lifetimePolicy [1] are:
> 
> ACCEPT_NORMALLY   = 0; // accept normally
> // Value = 1 is considered the same as 0 (See Bug 606655).
> ACCEPT_SESSION    = 2; // downgrade to session
> ACCEPT_FOR_N_DAYS = 3; // limit lifetime to N days

It doesn't look like ACCEPT_FOR_N_DAYS = 3 is actually supported, and it's not exposed via the about:preferences UI, so I don't think we should support it for browser.privacy.websites.cookieLifetimePolicy either. As that really just leaves 2 options, either 
ACCEPT_NORMALLY or ACCEPT_SESSION, I wonder if we should make this a boolean setting instead which simply controls whether all cookies are downgraded to session cookies?

What do folks think?
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8932975 - Attachment is obsolete: true
Attachment #8932975 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8932976 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Duplicate of this bug: 1422434
(Assignee)

Comment 22

a year ago
Just adding a note based on my testing of how the prefs actually work:

When you set browser.privacy.websites.nonPersistentCookies to `true`, which is meant to force all cookies to be session cookies, it works for cookies set by means other than the browser.cookies API (e.g., document.cookie), but if you use browser.cookies.set and pass in an expirationDate then it overrides the pref and creates a non-session cookie. That may be fine, as it might be expected that the pref is just a default and one can override it in an extension using browser.cookies.set, but I think we need to both confirm and document that.
(Assignee)

Comment 23

a year ago
The same thing seems to apply for browser.privacy.websites.cookieBehavior. Setting cookies via browser.cookies.set overrides the pref and allows you to, for example, create a cookie even when browser.privacy.cookieBehavior is set to `2` which is BEHAVIOR_REJECT.
(Assignee)

Comment 24

a year ago
Silverberg [:bsilverberg] from comment #23)
> The same thing seems to apply for browser.privacy.websites.cookieBehavior.
> Setting cookies via browser.cookies.set overrides the pref and allows you
> to, for example, create a cookie even when browser.privacy.cookieBehavior is
> set to `2` which is BEHAVIOR_REJECT.

Note that browser.privacy.cookieBehavior above should read network.cookie.cookieBehavior - that's the name of the pref.
(Assignee)

Comment 25

a year ago
Andrea, as documented in comment 22 and comment 23, it seems that our WebExtensions API code for cookies, i.e., browser.cookies.set() [1], does not respect either network.cookie.cookieBehavior or network.cookie.lifetimePolicy. I don't think we realized this, and I don't think that's what we want. Looking at the code for the API, it simply calls Services.cookies.add(), so I assume that also does not respect those prefs. As someone who really understands the way Firefox handles cookies, I was wondering if you could comment on the following:

1. Do you think the WebExtensions API to add cookies should respect the above mentioned prefs?
2. Do you think Services.cookies.add() should respect the above mentioned prefs?
3. How would you suggest addressing the fact that the WebExtensions API does not currently respect the above mentioned prefs?

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies/set
Flags: needinfo?(amarchesini)
(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> (In reply to Bob Silverberg [:bsilverberg] from comment #18)

> It doesn't look like ACCEPT_FOR_N_DAYS = 3 is actually supported, and it's
> not exposed via the about:preferences UI, so I don't think we should support
> it for browser.privacy.websites.cookieLifetimePolicy either. As that really
> just leaves 2 options, either 

It is, looks like you also have to set network.cookie.lifetime.days and network.cookie.lifetime.enabled.  It's no longer in preferences, but that may be more a reason to have it in webextensions.  We should verify longevity of supporting that before doing it, so a followup bug would be best.
I also think a followup regarding the cookies api respecting the prefs can be a followup bug.

Comment 28

a year ago
mozreview-review
Comment on attachment 8934225 [details]
Bug 1363860 - Allow WebExtensions to control cookie behaviour,

https://reviewboard.mozilla.org/r/205148/#review210764

::: toolkit/components/extensions/ext-privacy.js:18
(Diff revision 1)
> +const cookieBehaviorValues = {
> +  "allow_all": 0,
> +  "reject_third_party": 1,
> +  "reject_all": 2,
> +  "reject_third_party_if_not_visited": 3,
> +};

Use the idl values:

"allow_all": nsICookieService.BEHAVIOR_ACCEPT

::: toolkit/components/extensions/ext-privacy.js:270
(Diff revision 1)
>          websites: {
> +          cookieBehavior: getPrivacyAPI(extension,
> +            "websites.cookieBehavior",
> +            () => {
> +              let prefValue = Preferences.get("network.cookie.cookieBehavior");
> +              for (let prop in cookieBehaviorValues) {

cookieBehaviorValues.keys()[prefValue]
Attachment #8934225 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 29

a year ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> (In reply to Bob Silverberg [:bsilverberg] from comment #19)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #18)
> 
> > It doesn't look like ACCEPT_FOR_N_DAYS = 3 is actually supported, and it's
> > not exposed via the about:preferences UI, so I don't think we should support
> > it for browser.privacy.websites.cookieLifetimePolicy either. As that really
> > just leaves 2 options, either 
> 
> It is, looks like you also have to set network.cookie.lifetime.days and
> network.cookie.lifetime.enabled.  It's no longer in preferences, but that
> may be more a reason to have it in webextensions.  We should verify
> longevity of supporting that before doing it, so a followup bug would be
> best.

My concern about addressing this in a follow-up bug is that allowing for a third option will change this setting from a boolean to an enum, so I think we should make a decision about this up front. I don't want to implement a boolean and then change it to an enum in a follow-up. 

Andrea, could you also address the question of the longevity of ACCEPT_FOR_N_DAYS / network.cookie.lifetime.days / network.cookie.lifetime.enabled? Are these settings something we anticipate supporting in the future, or are they slated for deprecation?
(Assignee)

Comment 30

a year ago
I was discussing this with Markus with respect to bug 1390160 and he has some concerns with the fact that two settings will be controlling two different aspects of cookies, which to users appear to be related. I wonder if we should, as part of this bug, only implement the API for browser.privacy.websites.cookieBehavior and put off dealing with a setting which interacts with network.cookie.lifetimePolicy to a future bug. As far as I know, nobody is currently asking for the latter.

Mike, what do you think?
Flags: needinfo?(mconca)
Markus and I had a chance to catch up in Austin. We came to the conclusion that exposing each extension that controls a setting in about:preferences doesn't scale very well (not news, I know).  The right way to handle this is via a completely redesigned add-ons manager that Emanuela is prototyping. There, users will be able to see exactly which permissions and settings are controlled by which extensions in an easy layout that also provides fine-grained control over those permissions and settings.

What to do now: I'd like us to continue to make privacy-related API available to developers, including both of these related to cookies. Assuming there are no technical issues (based on comments above), Markus and I are good with a short-term band-aid solution for bug 1390160 that essentially would display a more generic "Extension XYZ controls your cookies" message, regardless of whether is controls cookie behavior or cookie lifetime. In the unlikely case where two different extensions control each setting independently, the UI should pick one to display. If that one is uninstalled/disabled, display the other. Note that this case is extremely unlikely both logically (users generally only have a single extension to control cookies) and empirically (only 44 extensions out of ~8,000 on AMO even request the privacy permission today).
Flags: needinfo?(mconca)
(Assignee)

Comment 32

a year ago
I just commented on bug 1390160, but I'll echo the comment here. I have a different idea about how to deal with this specific issue. I think that instead of having browser.privacy.websites.cookieBehavior and browser.privacy.websites.nonPersistentCookies as separate APIs/settings, we should combine them into one setting, which accepts an object that can define behavior for both things. This would ensure that only one extension can be in control of "cookie settings" at a time.

What do you think, Mike?
Flags: needinfo?(mconca)
(In reply to Bob Silverberg [:bsilverberg] from comment #32)
> I just commented on bug 1390160, but I'll echo the comment here. I have a
> different idea about how to deal with this specific issue. I think that
> instead of having browser.privacy.websites.cookieBehavior and
> browser.privacy.websites.nonPersistentCookies as separate APIs/settings, we
> should combine them into one setting, which accepts an object that can
> define behavior for both things. This would ensure that only one extension
> can be in control of "cookie settings" at a time.
> 
> What do you think, Mike?

Very elegant solution, I like it!
Flags: needinfo?(mconca)
> 1. Do you think the WebExtensions API to add cookies should respect the
> above mentioned prefs?

Currently, if a cookie is set from devtools, we don't follow the prefs.
Those prefs are currently used only for cookies coming from the network.
If a cookie is created manually, those prefs are not used.

I think it's fine if we introduce the support for these prefs in WebExtensions, but would be great if we do it at the same time in devtools. I also think that we should not touch 'nsICookieManager::add' but, instead, we should introduce a new method that does the pref check.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 35

a year ago
(In reply to Andrea Marchesini [:baku] from comment #34)
> > 1. Do you think the WebExtensions API to add cookies should respect the
> > above mentioned prefs?
> 
> Currently, if a cookie is set from devtools, we don't follow the prefs.
> Those prefs are currently used only for cookies coming from the network.
> If a cookie is created manually, those prefs are not used.
> 
> I think it's fine if we introduce the support for these prefs in
> WebExtensions, but would be great if we do it at the same time in devtools.
> I also think that we should not touch 'nsICookieManager::add' but, instead,
> we should introduce a new method that does the pref check.

Thanks Andrea.

Are you saying that you recommend we introduce support for these prefs in WebExtensions, or just that you would be okay with it if we (the WebExtensions team) decides that we want to add that support? We have not decided yet whether we should add that support or not, so I'm interested to know whether you think we _should_ or not.

Also, I had another question, in comment 29, about the longevity of ACCEPT_FOR_N_DAYS / network.cookie.lifetime.days / network.cookie.lifetime.enabled? Are these settings something we anticipate supporting in the future, or are they slated for deprecation?
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

a year ago
Comment on attachment 8934225 [details]
Bug 1363860 - Allow WebExtensions to control cookie behaviour,

The design of the API changed, so re-requesting review.
Attachment #8934225 - Flags: review+ → review?(mixedpuppy)
Comment hidden (mozreview-request)

Comment 39

a year ago
mozreview-review
Comment on attachment 8934225 [details]
Bug 1363860 - Allow WebExtensions to control cookie behaviour,

https://reviewboard.mozilla.org/r/205148/#review217646

::: toolkit/components/extensions/ext-privacy.js:20
(Diff revision 3)
> +
> +const cookieBehaviorValues = new Map([
> +  ["allow_all", cookieSvc.BEHAVIOR_ACCEPT],
> +  ["reject_third_party", cookieSvc.BEHAVIOR_REJECT_FOREIGN],
> +  ["reject_all", cookieSvc.BEHAVIOR_REJECT],
> +  ["reject_third_party_if_not_visited", cookieSvc.BEHAVIOR_LIMIT_FOREIGN],

This is wordy, can we shorten to something like allow_visited?
Attachment #8934225 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
> decided yet whether we should add that support or not, so I'm interested to
> know whether you think we _should_ or not.

To me, yes, we should support these prefs in WebExtensions, because the user is not aware of when cookies are created, modified and so on by addons.

> Also, I had another question, in comment 29, about the longevity of
> ACCEPT_FOR_N_DAYS / network.cookie.lifetime.days /
> network.cookie.lifetime.enabled? Are these settings something we anticipate
> supporting in the future, or are they slated for deprecation?

It seems that these 2 prefs are not used anymore. We should file a bug and remove them.
Flags: needinfo?(amarchesini)
Depends on: 1430095
(Assignee)

Comment 43

a year ago
Regarding comment 42, I have started a discussion with the WebExtensions community to see if overall sentiment is similar to Andrea's opinion. If we do decide to support the prefs in WebExtensions I will open a bug for that, and will also look into coordinating similar changes for devtools.

Comment 44

a year ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21de927fd6ad
Allow WebExtensions to control cookie behaviour, r=mixedpuppy
Depends on: 1430232
No longer depends on: 1430232
See Also: → bug 1430232

Comment 45

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21de927fd6ad
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 46

a year ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Considering that uMatrix works and was created for previous versions of the browser tests would not apply to the current behavior.
Flags: needinfo?(bob.silverberg)
(Assignee)

Updated

a year ago
Flags: needinfo?(bob.silverberg) → qe-verify-
I've added this to the privacy.websites page:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/websites

The compat data update is in review: https://github.com/mdn/browser-compat-data/pull/954

Please let me know if this covers it.
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 48

a year ago
Looks good, thanks Will.
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed → dev-doc-complete

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.