Open Bug 1272953 Opened 8 years ago Updated 9 months ago

Error handling for WebExtension cookies API

Categories

(WebExtensions :: General, defect, P5)

defect

Tracking

(Not tracked)

People

(Reporter: bugzilla, Unassigned)

References

Details

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

Attachments

(1 file)

+++ 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.
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)
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.
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 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: --- → ?
webextensions: ? → ---
Priority: -- → P5
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: bugzilla → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: