Closed Bug 1254221 Opened 8 years ago Closed 8 years ago

Support private browsing cookies in WebExtensions

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [cookies][private browsing])

Attachments

(1 file, 1 obsolete file)

I am writing a site-specific WebExtension that reads the session cookie from a site and use it to call the site's own public API to enrich the information shown in the browser. It does not work in Firefox in private browsing mode. It seems like the chrome.cookies API cannot read private browsing cookies. It works fine in Chrome incognito mode.
Whiteboard: [cookies][private browsing]
I am trying to see if I can implement this. I think I first need to do as described in https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#60 / bug 777620.

Then update https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-cookies.js

I think I can find "the current execution context's cookie store" per https://developer.chrome.com/extensions/cookies by looking at context.incognito from this line:

extensions.registerSchemaAPI("cookies", "cookies", (extension, context) => {

Then for my extension to work, I also need the incognito:split manifest property. I don't know yet how to implement that. Alternatively I need to find another way to find the correct storeId for a given message received by my extension background page.
Implementation of private cookie support in webextension API.

I noticed that the Firefox implementation of the cookies API typically return noting when it receives invalid input whereas Chrome throws an exception with a descriptive error message. (e.g. when you try to get cookies for a domain you do not have permisssion for) I have not changed that.

I added a new property on the tab interface named "cookieStoreId", which is not in Chrome. Is that OK? With this extra property I can port my extension to Firefox by adding just two simple lines of code, but without it, it would be somewhat more complex since Firefox does not support the incognito split mode.
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

https://reviewboard.mozilla.org/r/42815/#review39279

The cookie manager changes will need review by a necko peer.

The WebExtension portions look good to me.

::: toolkit/components/extensions/ext-cookies.js:16
(Diff revision 1)
> -// Cookies from private tabs currently can't be enumerated.
>  var DEFAULT_STORE = "firefox-default";
> +var PRIVATE_STORE = "firefox-private";
>  
> -function convert(cookie) {
> +global.setCookieStoreIdForTab = function(tab) {
> +  tab.cookieStoreId = tab.incognito ? PRIVATE_STORE : DEFAULT_STORE;

I don't think this is necessary, at least not for the moment.

::: toolkit/components/extensions/ext-cookies.js:153
(Diff revision 1)
> +    if(details.storeId == DEFAULT_STORE) {
> +      isPrivate = false;
> +    } else if (details.storeId == PRIVATE_STORE) {
> +      isPrivate = true;
> +    } else {
> +      return;

This should be an error.

::: toolkit/components/extensions/ext-cookies.js:163
(Diff revision 1)
>    let enumerator;
>    let uri;
>    if ("url" in details) {
>      try {
>        uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
> -      enumerator = Services.cookies.getCookiesFromHost(uri.host);
> +      Services.cookies.usePrivateMode(isPrivate, () => enumerator = Services.cookies.getCookiesFromHost(uri.host));

I'd rather wrap all of the relevant code in a single `usePrivateMode` call.

::: toolkit/components/extensions/ext-cookies.js:256
(Diff revision 1)
>  extensions.registerSchemaAPI("cookies", "cookies", (extension, context) => {
>    let self = {
>      cookies: {
>        get: function(details) {
>          // FIXME: We don't sort by length of path and creation time.
> -        for (let cookie of query(details, ["url", "name", "storeId"], extension)) {
> +        for (let cookie of query(details, ["url", "name", "storeId"], extension, convert, context.incognito)) {

There is no `context.incognito` property, and it probably doesn't make sense to add one, so please remove this argument (same for the other instances).

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:84
(Diff revision 1)
>      }).then(details => {
>        assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID}, details);
>        return browser.cookies.get({url: TEST_URL, name: "name1"});
>      }).then(cookie => {
>        browser.test.assertEq(null, cookie, "removed cookie not found");
>        return browser.cookies.getAllCookieStores();

We also need tests for this when there are private browsing tabs open.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:88
(Diff revision 1)
>        browser.test.assertEq(null, cookie, "removed cookie not found");
>        return browser.cookies.getAllCookieStores();
>      }).then(stores => {
>        browser.test.assertEq(1, stores.length, "expected number of stores returned");
>        browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
> -      browser.test.assertEq(0, stores[0].tabIds.length, "no tabs returned for store"); // Todo: Implement this.
> +      browser.test.assertEq(1, stores[0].tabIds.length, "no tabs returned for store");

Please fix the test description.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:167
(Diff revision 1)
>        return browser.cookies.set({url: TEST_URL});
>      }).then(cookie => {
>        browser.test.assertEq("", cookie.name, "default name set");
>        browser.test.assertEq("", cookie.value, "default value set");
>        browser.test.assertEq(true, cookie.session, "no expiry date created session cookie");
> +      return browser.cookies.set({url: TEST_URL, name: "store", value: "private", expirationDate: THE_FUTURE, storeId: PRIVATE_STORE_ID});

We shouldn't rely on this working when there are no private browsing windows open.
Attachment #8735519 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/42815/#review39279

> I don't think this is necessary, at least not for the moment.

Ok. I will see if I can figure out a way to implement incognito split mode instead. Per https://developer.chrome.com/extensions/manifest/incognito.

> This should be an error.

Ok, I think this needs some refactoring of the cookies API implementation to properly support error handling, and I was hoping that refactoring could be done after implementing support for private cookies.

> I'd rather wrap all of the relevant code in a single `usePrivateMode` call.

I may be wrong, but I think an XPCOM callback reduces any exceptions thrown within it to an XPCOM error code, so I wanted to avoid wrapping anything that was not needed. Anyway I will wait to change anthing until I have got a review for the changes to nsICookieManager.

> There is no `context.incognito` property, and it probably doesn't make sense to add one, so please remove this argument (same for the other instances).

I think it gets an incognito property from https://hg.mozilla.org/mozilla-central/file/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/toolkit/components/extensions/Extension.jsm#l239 . Am I wrong?

If that is not correct, I think I would need to add one anyway to support incognito split mode.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/42815/#review39279

> Ok. I will see if I can figure out a way to implement incognito split mode instead. Per https://developer.chrome.com/extensions/manifest/incognito.

That's going to be a quite big project. It's definitely something for a separate bug.

But, the more I think about it, the more I think this property makes sense. I think we just need to make sure we only add it if the extension has the "cookies" permission.

> I think it gets an incognito property from https://hg.mozilla.org/mozilla-central/file/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/toolkit/components/extensions/Extension.jsm#l239 . Am I wrong?
> 
> If that is not correct, I think I would need to add one anyway to support incognito split mode.

Sorry, you're right, the property does exist, I'm just not sure it's right for this purpose. I'll look into it some more.
Whiteboard: [cookies][private browsing] → [cookies][private browsing]triaged
Were you planning to submit the cookie service changes for review separately, or should I try to find someone to review them?

Sorry I didn't think to ask this earlier.
Flags: needinfo?(bugzilla)
I submitted it, but used bug 777620. But that review seems to have stalled.
Flags: needinfo?(bugzilla)
Thanks. I'll try to get someone on that.
Depends on: 777620
https://reviewboard.mozilla.org/r/42815/#review39279

> That's going to be a quite big project. It's definitely something for a separate bug.
> 
> But, the more I think about it, the more I think this property makes sense. I think we just need to make sure we only add it if the extension has the "cookies" permission.

I will add a check for the "cookies" permission.
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42815/diff/1-2/
https://reviewboard.mozilla.org/r/42815/#review39279

> Sorry, you're right, the property does exist, I'm just not sure it's right for this purpose. I'll look into it some more.

I didn't change this since you said you would look into it. Let me know if I should change it.
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

https://reviewboard.mozilla.org/r/42815/#review48549

Looks good so far. Just a few nits.

Thanks!

::: browser/components/extensions/ext-utils.js:499
(Diff revision 2)
>        height: tab.linkedBrowser.clientHeight,
>        audible: tab.soundPlaying,
>        mutedInfo,
>      };
> +    if (this.extension.hasPermission("cookies")) {
> +      setCookieStoreIdForTab(result);

I think this would probably be better as:

    result.cookieStoreId = getCookieStoreIdForTab(result);

::: toolkit/components/extensions/ext-cookies.js:163
(Diff revision 2)
>    let enumerator;
>    let uri;
>    if ("url" in details) {
>      try {
>        uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
> -      enumerator = Services.cookies.getCookiesFromHost(uri.host);
> +      Services.cookies.usePrivateMode(isPrivate, () => enumerator = Services.cookies.getCookiesFromHost(uri.host));

Please split these calls into multiple lines:

    Services.cookies.usePrivateMode(isPrivate, () => {
      enumerator = Services.cookies.getCookiesFromHost(uri.host);
    });

::: toolkit/components/extensions/ext-cookies.js:292
(Diff revision 2)
>          let secure = details.secure !== null ? details.secure : false;
>          let httpOnly = details.httpOnly !== null ? details.httpOnly : false;
>          let isSession = details.expirationDate === null;
>          let expiry = isSession ? Number.MAX_SAFE_INTEGER : details.expirationDate;
> -        // Ignore storeID.
> +        let isPrivate = context.incognito;
> +        if (details.storeId !== null) {

Please remove the outer if and move this condition to the last `else`.

::: toolkit/components/extensions/ext-cookies.js:309
(Diff revision 2)
>            return Promise.reject({message: `Permission denied to set cookie ${JSON.stringify(details)}`});
>          }
>  
>          // The permission check may have modified the domain, so use
>          // the new value instead.
> +        Services.cookies.usePrivateMode(isPrivate, () =>

Please add braces around the function body.

::: toolkit/components/extensions/ext-cookies.js:318
(Diff revision 2)
>          return self.cookies.get(details);
>        },
>  
>        remove: function(details) {
> -        for (let cookie of query(details, ["url", "name", "storeId"], extension)) {
> -          Services.cookies.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> +        for (let {cookie, isPrivate} of query(details, ["url", "name", "storeId"], extension, (cookie, isPrivate) => ({cookie, isPrivate}), context.incognito)) {
> +          Services.cookies.usePrivateMode(isPrivate, () => Services.cookies.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes));

Please split into multiple lines, and add braces around the function body.
Attachment #8735519 - Flags: review?(kmaglione+bmo)
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42815/diff/2-3/
Attachment #8735519 - Flags: review?(kmaglione+bmo)
Attachment #8750118 - Flags: review?(kmaglione+bmo)
Comment on attachment 8750118 [details]
MozReview Request: Bug 1254221 - Error handling for WebExtension cookies API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51299/diff/1-2/
Attachment #8735519 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

https://reviewboard.mozilla.org/r/42815/#review48957

Thanks!

::: toolkit/components/extensions/ext-cookies.js:301
(Diff revision 3)
> +        if (details.storeId !== null) {
> +          if(details.storeId == DEFAULT_STORE) {
> +            isPrivate = false;
> +          } else if (details.storeId == PRIVATE_STORE) {
> +            isPrivate = true;
> +          } else {

Please remove the outer `if` and move the conditional to this `else`
Comment on attachment 8750118 [details]
MozReview Request: Bug 1254221 - Error handling for WebExtension cookies API

https://reviewboard.mozilla.org/r/51299/#review48959

::: toolkit/components/extensions/ext-cookies.js:152
(Diff revision 2)
>    if(details.storeId == DEFAULT_STORE) {
>      isPrivate = false;
>    } else if (details.storeId == PRIVATE_STORE) {
>      isPrivate = true;
>    } else if ("storeId" in details) {
> -    return;
> +    throw {message: "Unknown storeId"};

Our ESLint rules don't allow throwing literals, including object literals. If you want to throw an error to be propagated to the extension, you need to pass in a context and throw a `context.cloneScope.Error` instance.

::: toolkit/components/extensions/ext-cookies.js:165
(Diff revision 2)
>      } catch (ex) {
>        // This often happens for about: URLs
> -      return;
>      }
> +    if (!uri || uri.scheme != "http" && uri.scheme != "https" ||
> +        !extension.whiteListedHosts.matchesIgnoringPath(uri)) {

I'm not sure what the purpose of this is. It doesn't seem to be related to this bug?

::: toolkit/components/extensions/ext-cookies.js:265
(Diff revision 2)
>  
>  extensions.registerSchemaAPI("cookies", "cookies", (extension, context) => {
>    let self = {
>      cookies: {
>        get: function(details) {
> +        return Promise.resolve().then(() => {

Why is this necessary?
Attachment #8750118 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/51299/#review48959

> Our ESLint rules don't allow throwing literals, including object literals. If you want to throw an error to be propagated to the extension, you need to pass in a context and throw a `context.cloneScope.Error` instance.

Ok, I will do that. Since query then takes both extension and context, I think I will move it into the closure of registerSchemaAPI

> I'm not sure what the purpose of this is. It doesn't seem to be related to this bug?

You asked me to throw an exception when the storeId is invalid. I think we should not thor exceptions for one arbitrary error case, and ignore other errors scilently, so I tried to add exceptions to all the error cases I could find in the cookies API. I made this in a separate changeset from the private browsing cookie changes. I originally asked you if we could make it in a separate bug.

> Why is this necessary?

I think this is required to turn the exceptions thrown by the query function into promise rejections.
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42815/diff/3-4/
Attachment #8750118 - Flags: review?(kmaglione+bmo)
Comment on attachment 8750118 [details]
MozReview Request: Bug 1254221 - Error handling for WebExtension cookies API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51299/diff/2-3/
Note: I updated both changesets, so you may want to look at the first one again even if you already gave r+ on the previous version.
https://reviewboard.mozilla.org/r/51299/#review48959

> Ok, I will do that. Since query then takes both extension and context, I think I will move it into the closure of registerSchemaAPI

Contexts always have a reference to the extension, so there shouldn't be a need to pass both.

> You asked me to throw an exception when the storeId is invalid. I think we should not thor exceptions for one arbitrary error case, and ignore other errors scilently, so I tried to add exceptions to all the error cases I could find in the cookies API. I made this in a separate changeset from the private browsing cookie changes. I originally asked you if we could make it in a separate bug.

OK. Can you please file a separate bug for that and move this change to it? It may have behavior implications, and if it causes breakage, it'll be much easier to deal with if it has its own bug.

> I think this is required to turn the exceptions thrown by the query function into promise rejections.

It shouldn't be necessary, as long as the exceptions we throw belong to the content scope. The binding layer should take care of turning those into rejected promises.
Blocks: 1272953
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42815/diff/4-5/
Attachment #8750118 - Attachment is obsolete: true
Attachment #8750118 - Flags: review?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #23)
> https://reviewboard.mozilla.org/r/51299/#review48959
> 
> > Ok, I will do that. Since query then takes both extension and context, I think I will move it into the closure of registerSchemaAPI
> 
> Contexts always have a reference to the extension, so there shouldn't be a
> need to pass both.
> 
> > You asked me to throw an exception when the storeId is invalid. I think we should not thor exceptions for one arbitrary error case, and ignore other errors scilently, so I tried to add exceptions to all the error cases I could find in the cookies API. I made this in a separate changeset from the private browsing cookie changes. I originally asked you if we could make it in a separate bug.
> 
> OK. Can you please file a separate bug for that and move this change to it?
> It may have behavior implications, and if it causes breakage, it'll be much
> easier to deal with if it has its own bug.

Ok, I created bug 1272953
> 
> > I think this is required to turn the exceptions thrown by the query function into promise rejections.
> 
> It shouldn't be necessary, as long as the exceptions we throw belong to the
> content scope. The binding layer should take care of turning those into
> rejected promises.

Oh, that was quite well hidden. I removed the Promise.resolve wrapping i bug 1272953, and updated so that all thrown errors and returned values in the cookies API are done the same way.
Attachment #8735519 - Flags: review?(kmaglione+bmo)
Attachment #8735519 - Flags: review?(kmaglione+bmo) → review+
How do we get things moving here?
Sorry, I probably should have mentioned this: the patch has r+, so you just need to add the checkin-needed keyword to the bug.
Keywords: checkin-needed
Keywords: checkin-needed
I tried to rebase the patch, but it conflicted with bug 1267910
Comment on attachment 8735519 [details]
Bug 1254221 - Support private browsing cookies in WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42815/diff/5-6/
Attachment #8735519 - Attachment description: MozReview Request: Bug 1254221 - Support private browsing cookies in WebExtensions → Bug 1254221 - Support private browsing cookies in WebExtensions
Attachment #8735519 - Flags: review+
I rebased it. Is it ok now?

I looked at if I could use the change to the cookie manager instead, but it doesn't use the private browsing mode from the origin attributes parameter, and it would be a larger effort to make it do so.

Some more work is needed to make the cookies api work with the new contexts / containers feature, but I guess that is another bug.
Flags: needinfo?(kmaglione+bmo)
How is this going?
How is this going?
Oh, sorry, I didn't realize this hadn't landed.
Flags: needinfo?(kmaglione+bmo)
Keywords: checkin-needed
has problems to apply:

applying 88abfd45c0d2
patching file browser/components/extensions/ext-utils.js
Hunk #1 FAILED at 500
1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/ext-utils.js.rej
patching file toolkit/components/extensions/ext-cookies.js
Hunk #3 FAILED at 231
1 out of 5 hunks FAILED -- saving rejects to file toolkit/components/extensions/ext-cookies.js.rej
patching file toolkit/components/extensions/test/mochitest/test_ext_cookies.html
Hunk #3 FAILED at 169
1 out of 3 hunks FAILED -- saving rejects to file toolkit/components/extensions/test/mochitest/test_ext_cookies.html.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
I have rebased the changesets. It did not produce any conflicts. Is it ok to land now?
Flags: needinfo?(bugzilla) → needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9b6a4552a5c2
Support private browsing cookies in WebExtensions r=kmag
For future reference, you can just set the checkin-needed keyword when you're ready for your patches to be checked in.
And a variety of others, the mochitest also fails on Android, and the chrome test test_ext_cookies_permissions.html  on Android, ASan, and at least sometimes on Linux64, https://treeherder.mozilla.org/logviewer.html#?job_id=4100951&repo=autoland
My fault. Bug 777620 needed to land before this one.
I have updated the patch to try to fix the lint errors. I tried to submit it (including Bug 777620) to try, but it wouldn't let me, it said: remote: Permission denied (publickey).
Flags: needinfo?(kmaglione+bmo)
If you're sure you're using the right public key, then you may need to file a bug (In Infrastructure & Operations :: MOC: Service Requests) to get your public key updated.

In the meantime, I triggered a try build from mozreview: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53a0102ef81c
Flags: needinfo?(kmaglione+bmo)
All the errors in the try build seems totally unrelated. What is the next step here?
This bug and bug 777620 need the checkin-needed keyword, and this one needs a whiteboard comment saying that bug 777620 needs to land first.
Keywords: checkin-needed
Whiteboard: [cookies][private browsing]triaged → [cookies][private browsing]triaged checkin-needed: bug 777620 needs to land first
Whiteboard: [cookies][private browsing]triaged checkin-needed: bug 777620 needs to land first → [cookies][private browsing]
Not sure why Pulsebot didn't comment in the bug when this landed.
https://hg.mozilla.org/integration/autoland/rev/f2181015902f1bd9e533bf6103bbbb9e65d2436e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2181015902f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1309610
Blocks: 1309637
Depends on: 1318948
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: