Closed
Bug 1363860
Opened 8 years ago
Closed 8 years ago
Allow WebExtensions to control cookie behaviour
Categories
(WebExtensions :: General, enhancement, P2)
WebExtensions
General
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: triaged)
Attachments
(1 file, 2 obsolete files)
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•8 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•8 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)
Comment 3•8 years ago
|
||
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•8 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•8 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•8 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)
Comment 7•8 years ago
|
||
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•8 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 | ||
Comment 9•8 years ago
|
||
We're planning to use permissions for this now, after 1313939 is implemented.
Depends on: 1313939
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
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: → 1406675
Comment 12•8 years 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) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years 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.
Comment 16•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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•8 years ago
|
Attachment #8932975 -
Attachment is obsolete: true
Attachment #8932975 -
Flags: review?(jhofmann)
Assignee | ||
Updated•8 years ago
|
Attachment #8932976 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years 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•8 years 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•8 years 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•8 years 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)
Comment 26•8 years ago
|
||
(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.
Comment 27•8 years ago
|
||
I also think a followup regarding the cookies api respecting the prefs can be a followup bug.
Comment 28•8 years 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•8 years 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•8 years 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)
Comment 31•8 years ago
|
||
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•8 years 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)
Comment 33•8 years ago
|
||
(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)
Comment 34•8 years ago
|
||
> 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•8 years 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•8 years 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•8 years 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) |
Comment 42•8 years ago
|
||
> 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)
Assignee | ||
Comment 43•8 years 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•8 years ago
|
||
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21de927fd6ad
Allow WebExtensions to control cookie behaviour, r=mixedpuppy
Comment 45•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 46•8 years 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•8 years ago
|
Flags: needinfo?(bob.silverberg) → qe-verify-
Comment 47•8 years ago
|
||
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)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•