Open
Bug 1272953
Opened 8 years ago
Updated 9 months ago
Error handling for WebExtension cookies API
Categories
(WebExtensions :: General, defect, P5)
WebExtensions
General
Tracking
(Not tracked)
NEW
People
(Reporter: bugzilla, Unassigned)
References
Details
(Whiteboard: [cookies][private browsing]triaged)
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Details |
+++ 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.
Reporter | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years 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.
Reporter | ||
Comment 4•8 years 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•7 years 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)
Updated•7 years ago
|
webextensions: --- → ?
Updated•7 years ago
|
webextensions: ? → ---
Updated•7 years ago
|
Priority: -- → P5
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 7•6 years ago
|
||
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
Comment 8•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: bugzilla → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•