Error handling for WebExtension cookies API

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Untriaged
a year ago
2 months ago

People

(Reporter: Jesper Kristensen, Assigned: Jesper Kristensen)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cookies][private browsing]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
+++ This bug was initially created as a clone of Bug #1254221 +++

When the WebExtensions cookies API receives invalid data, it should give an error instead of returning null or an empty array.
(Assignee)

Comment 1

a year ago
Created attachment 8752561 [details]
Bug 1272953 - Error handling for WebExtension cookies API

Review commit: https://reviewboard.mozilla.org/r/52636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52636/
Attachment #8752561 - Flags: review?(kmaglione+bmo)
Comment on attachment 8752561 [details]
Bug 1272953 - Error handling for WebExtension cookies API

https://reviewboard.mozilla.org/r/52636/#review50110

I like this overall, but there are some issues that definitely need to be addressed, and I'm unsure about a few parts.

::: toolkit/components/extensions/Extension.jsm:571
(Diff revision 1)
>            callback = defaultCallback;
>          }
>  
>          let promise;
>          try {
> -          promise = findPathInObject(schemaApi, path)[name](...args);
> +          promise = Promise.resolve(findPathInObject(schemaApi, path)[name](...args));

We treat promises belonging to the target compartment specially, so we don't want to wrap return values that are already promises.

::: toolkit/components/extensions/ext-cookies.js:168
(Diff revision 1)
> -      return;
>      }
> +    if (!uri || uri.scheme != "http" && uri.scheme != "https" ||
> +        !context.extension.whiteListedHosts.matchesIgnoringPath(uri)) {
> +      if (props.includes("domain")) {
> +        return; // Scilently ignore for getAll

*Silently

::: toolkit/components/extensions/ext-cookies.js:170
(Diff revision 1)
> +    if (!uri || uri.scheme != "http" && uri.scheme != "https" ||
> +        !context.extension.whiteListedHosts.matchesIgnoringPath(uri)) {
> +      if (props.includes("domain")) {
> +        return; // Scilently ignore for getAll
> +      }
> +      throw new context.cloneScope.Error(`No host permissions for cookies at url: "${details.url}".`);

Unfortunately, the permissions matching for cookies is much more complicated than this. Is there a particular reason we need to add this additional check?

::: toolkit/components/extensions/ext-cookies.js:308
(Diff revision 1)
>          if(details.storeId == DEFAULT_STORE) {
>            isPrivate = false;
>          } else if (details.storeId == PRIVATE_STORE) {
>            isPrivate = true;
>          } else if (details.storeId !== null) {
> -          return Promise.reject({message: "Unknown storeId"});
> +          throw new context.cloneScope.Error("Unknown storeId");

I'd prefer to return a rejected promise in the cases where it's practical.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:82
(Diff revision 1)
> +      }, error => {
> +        browser.test.assertEq(error.message, "Unknown storeId");
> +      });
> +    }).then(cookies => {
> +      return browser.cookies.get({url: "http://no-permission", name: "name1"}).then(cookie => {
> +        browser.test.fail("using url without permission should reject");

Please capitalize messages.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:86
(Diff revision 1)
> +      return browser.cookies.get({url: "http://no-permission", name: "name1"}).then(cookie => {
> +        browser.test.fail("using url without permission should reject");
> +      }, error => {
> +        browser.test.assertEq(error.message, 'No host permissions for cookies at url: "http://no-permission".');
> +      });
>      }).then(cookies => {

This resolution function shouldn't have an argument.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:205
(Diff revision 1)
> +        return browser.cookies.set({url: TEST_URL, name: "store", value: "invalid", expirationDate: THE_FUTURE, storeId: "invalid"}).then(cookie => {
> +          browser.test.fail("using invalid storeId should reject");
> +        }, error => {
> +          browser.test.assertEq(error.message, "Unknown storeId");
> +        });
> +      }).then(cookie => {

This function also shouldn't have an argument.
Attachment #8752561 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

a year ago
https://reviewboard.mozilla.org/r/52636/#review50110

> We treat promises belonging to the target compartment specially, so we don't want to wrap return values that are already promises.

Ok. Just a side thought: If we don't want to implictly convert things to a promise here, wouldn't it be better to not use the throw exception as the promise exception two lines above, and instead reject the promise with "An unexpected error occurred", to make sure we don't leak an accidentally thrown exception?

> Unfortunately, the permissions matching for cookies is much more complicated than this. Is there a particular reason we need to add this additional check?

The same check is used other places in the same file, so why should it not be just at good here?

My intention is tho make the API throw/reject on invalid input instead of returning null or an empty string. I tried to model this based on some tests of what Chrome does. Chrome seems to throw when the URL or the storeId is not allowed. It seems wrong to me to reject when receiving an invalid storeId but just resolving to null when receiving an invalid url.

> I'd prefer to return a rejected promise in the cases where it's practical.

Ok. But I think it is very confusing to have code that sometimes returns promises, and sometimes throws errors. I think explicitly returning a promise is better, but then I think we should do it in all cases. So I want to figure out how to propagate the errors from the query function into explicitly rejected promises.
(Assignee)

Comment 4

a year ago
Comment on attachment 8752561 [details]
Bug 1272953 - Error handling for WebExtension cookies API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52636/diff/1-2/
Attachment #8752561 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 6

3 months ago
mozreview-review
Comment on attachment 8752561 [details]
Bug 1272953 - Error handling for WebExtension cookies API

https://reviewboard.mozilla.org/r/52636/#review118896

Sorry, this patch got buried in my queue, and now it's bit rotted. I think this has been covered by the user context ID changes, though.
Attachment #8752561 - Flags: review?(kmaglione+bmo)
webextensions: --- → ?

Updated

2 months ago
webextensions: ? → ---
You need to log in before you can comment on or make changes to this bug.