1197417 - Implement cookies API for open extension API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Mindaugas <LA-MJ>]

https://developer.chrome.com/extensions/cookies

API is listed on https://wiki.mozilla.org/WebExtensions#List_of_APIs_we_will_likely_support_in_the_future

Comment 1

[Tom S. (please needinfo tschuster)]

I will give this a shot.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: WIP

Implements: get, getAll, remove Missing: set, getAllCookieStores, onChanged

I still need to fine tune the url/domain/path matching, because it's really not that obvious how that all fits together. I need to read up on how host matching for cookies is supposed to work. Currently we don't have a way to modify cookies from private tabs and so the CookieStorage concept is completely useless to us.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8664506 [details] [diff] [review]
WIP

Review of attachment 8664506 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look at this in detail, but the approach seems good. However, I think the code should go in toolkit/ rather than browser/.

::: browser/components/extensions/ext-tabs.js
@@ +341,5 @@
>          runSafe(context, callback, TabManager.convert(extension, tab));
>        },
>  
> +      getSelected: function(foo, callback) {
> +        runSafe(context, callback, TabManager.convert(extension, TabManager.activeTab));

I think this belongs in a separate bug.

::: toolkit/modules/addons/WebRequest.jsm
@@ +355,5 @@
>  };
>  
>  var onBeforeRequest = {
>    addListener(callback, filter = null, opt_extraInfoSpec = null) {
> +    let opts = parseExtra(opt_extraInfoSpec, ["requestBody", "blocking"]);

Don't think you meant for this to be here.

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: v1 - Implement chrome.cookies

Comment 5

[Tom S. (please needinfo tschuster)]

Comment on attachment 8664880 [details] [diff] [review]
v1 - Implement chrome.cookies

Review of attachment 8664880 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this version had a few bugs that I just noticed. I am going to first write some tests.

Comment 6

[Tom S. (please needinfo tschuster)]

Attachment: v2 - Implement chrome.cookies

Comment 7

[Tom S. (please needinfo tschuster)]

Attachment: v1 - Tests for chrome.cookies

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I starting looking at this today, but all this cookie stuff is complicated. I'll try to finish it over the weekend.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8665409 [details] [diff] [review]
v2 - Implement chrome.cookies

Review of attachment 8665409 [details] [diff] [review]:
-----------------------------------------------------------------

This is basically fine with some minor problems. Please fix them or file follow-ups.

::: toolkit/components/extensions/ext-cookies.js
@@ +1,2 @@
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +var {

Everything here is local to this file, so I'd prefer it all be |let| instead of |var|.

@@ +7,5 @@
> +
> +// Cookies from private tabs currently can't be enumerated.
> +var DEFAULT_STORE = "firefox-default";
> +
> +var console = (() => {

This is now unused.

@@ +15,5 @@
> +  };
> +  return new ConsoleAPI(consoleOptions);
> +})();
> +
> +var convert = function(cookie) {

function convert(cookie) {

@@ +34,5 @@
> +
> +  return result;
> +}
> +
> +var query = function* (allDetails, allowed) {

function* query(...)?

@@ +40,5 @@
> +  allowed.map(property => {
> +    if (property in allDetails) {
> +      details[property] = allDetails[property];
> +    }
> +  })

Missing ;

@@ +58,5 @@
> +  } else {
> +    enumerator = Services.cookies.enumerator;
> +  }
> +
> +  function matches(cookie) {

I'm worried that a lot of the code here is only an approximation of what our "real" cookie code does here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2633

What if we call nsICookieService.getCookieString and parse the result? That seems like it would be more likely to be correct. However, I'm okay checking in what you have here as long as we file a follow-up.

@@ +60,5 @@
> +  }
> +
> +  function matches(cookie) {
> +    // "Restricts the retrieved cookies to those that would match the given URL."
> +    if ("url" in details) {

We need to check permissions here. The docs say "If host permissions for this URL are not specified in the manifest file, the API call will fail."

@@ +61,5 @@
> +
> +  function matches(cookie) {
> +    // "Restricts the retrieved cookies to those that would match the given URL."
> +    if ("url" in details) {
> +      var uri = Services.io.newURI(details.url, null, null);

If the cookie is secure, then I'm guessing we don't want to match if details.url is not HTTPS.

@@ +64,5 @@
> +    if ("url" in details) {
> +      var uri = Services.io.newURI(details.url, null, null);
> +      // Todo: Is this right?
> +      if (cookie.isDomain) {
> +        if (!uri.host.endsWith(cookie.host.substring(1)))

What's the substring call for? Also, please always brace one-line blocks.

@@ +80,5 @@
> +    if ("name" in details && details.name != cookie.name)
> +      return false;
> +
> +    // "Restricts the retrieved cookies to those whose domains match or are subdomains of this one."
> +    if ("domain" in details && !cookie.host.endsWith(details.domain))

This will return true of details.domain is "xyz.com" and cookie.host is "abcxyz.com".

@@ +83,5 @@
> +    // "Restricts the retrieved cookies to those whose domains match or are subdomains of this one."
> +    if ("domain" in details && !cookie.host.endsWith(details.domain))
> +      return false;
> +
> +    if ("path" in details && !cookie.path.startsWith(details.path))

Why startsWith? The docs say "Restricts the retrieved cookies to those whose path exactly matches this string."

@@ +114,5 @@
> +        if (!details || !details.url || !details.name) {
> +          throw new Error("Mising required property");
> +        }
> +
> +        // Note: We don't sort by length of path and creation time.

Please use FIXME instead of Note. I've been trying to stick to that :-).

@@ +143,5 @@
> +        if (!details || !details.url) {
> +          throw new Error("Mising required property");
> +        }
> +
> +        let uri = Services.io.newURI(details.url, null, null);

We need to check the host permission for details.url at some point.

@@ +156,5 @@
> +        let path;
> +        if ("path" in details) {
> +          path = details.path;
> +        } else {
> +          // Chrome seems to trim the path after the lash slash

last

@@ +161,5 @@
> +          // /x/abc/ddd == /x/abc
> +          // /xxxx?abc == /
> +          // We always have at least one slash.
> +          let index = uri.path.slice(1).lastIndexOf("/");
> +          if (index == -1)

Braces here please.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8665411 [details] [diff] [review]
v1 - Tests for  chrome.cookies

Review of attachment 8665411 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: v3 - Implement chrome.cookies

I think I wouldn't mind a second look over this. I am going to open a new bug for using the nsCookieService code for matching and implementing the host permission check.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8667855 [details] [diff] [review]
v3 - Implement chrome.cookies

Review of attachment 8667855 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please also file a follow-up for checking host permissions for this API.

::: toolkit/components/extensions/ext-cookies.js
@@ +14,5 @@
> +    value: cookie.value,
> +    domain: cookie.host,
> +    hostOnly: !cookie.isDomain,
> +    path: cookie.path,
> +    secure: cookie.isHttpOnly,

Should this be isSecure?

@@ +20,5 @@
> +    session: cookie.isSession,
> +    storeId: DEFAULT_STORE
> +  }
> +
> +  if (!cookie.isSession)

Please use braces.

@@ +93,5 @@
> +        return false;
> +      }
> +    }
> +
> +    if ("name" in details && details.name != cookie.name)

Braces

@@ +105,5 @@
> +      }
> +    }
> +
> +    // "Restricts the retrieved cookies to those whose path exactly matches this string.""
> +    if ("path" in details && details.path != cookie.path)

More braces here and below.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e92426565d53
https://hg.mozilla.org/integration/mozilla-inbound/rev/687c8a36b56e

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e92426565d53
https://hg.mozilla.org/mozilla-central/rev/687c8a36b56e

Comment 15

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies