Closed Bug 1197417 Opened 4 years ago Closed 4 years ago

Implement cookies API for open extension API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: 4mr.minj, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

Status: UNCONFIRMED → NEW
Ever confirmed: true
I will give this a shot.
Assignee: nobody → evilpies
Attached patch WIP (obsolete) — Splinter Review
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 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.
Attachment #8664506 - Flags: feedback+
Attached patch v1 - Implement chrome.cookies (obsolete) — Splinter Review
Attachment #8664506 - Attachment is obsolete: true
Attachment #8664880 - Flags: review?(wmccloskey)
Attachment #8664880 - Flags: feedback?(josh)
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.
Attachment #8664880 - Flags: review?(wmccloskey)
Attachment #8664880 - Flags: feedback?(josh)
Attached patch v2 - Implement chrome.cookies (obsolete) — Splinter Review
Attachment #8664880 - Attachment is obsolete: true
Attachment #8665409 - Flags: review?(wmccloskey)
Attachment #8665411 - Flags: review?(wmccloskey)
I starting looking at this today, but all this cookie stuff is complicated. I'll try to finish it over the weekend.
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.
Attachment #8665409 - Flags: review?(wmccloskey) → review+
Comment on attachment 8665411 [details] [diff] [review]
v1 - Tests for  chrome.cookies

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

Thanks!
Attachment #8665411 - Flags: review?(wmccloskey) → review+
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.
Attachment #8665409 - Attachment is obsolete: true
Attachment #8667855 - Flags: review?(wmccloskey)
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.
Attachment #8667855 - Flags: review?(wmccloskey) → review+
Blocks: 1210996
Blocks: 1210997
https://hg.mozilla.org/mozilla-central/rev/e92426565d53
https://hg.mozilla.org/mozilla-central/rev/687c8a36b56e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1212089
Product: Toolkit → WebExtensions
Duplicate of this bug: 535336
You need to log in before you can comment on or make changes to this bug.