Some content script API parameters are not validated (runtime, i18n)

VERIFIED FIXED

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

({leave-open})

Trunk
leave-open
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 verified)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

a year ago
Created attachment 8770759 [details]
content_script_invalid_params.zip

1. Load the attached extension.
2. Visit example.com (the content script calls several APIs with an invalid method signature).
3. Open the console of the tab and look at the printed messages.

Expected:
OK: Got expected error: Error: Incorrect argument types for storage.StorageArea.get.
OK: Got expected error: Error: Incorrect argument types for runtime.connect.
OK: Got expected error: Error: Incorrect argument types for runtime.sendMessage.
OK: Got expected error: Error: Incorrect argument types for i18n.

Actual:
OK: Got expected error: Error: Incorrect argument types for storage.StorageArea.get.
FAIL: Expected a parameter validation error for runtime.connect API
FAIL: Expected a parameter validation error for runtime.sendMessage API
FAIL: Expected a parameter validation error for i18n API

Note: The background page seems to not validate chrome.runtime.sendMessage's parameters. When I run the content script as a background script, the background console contains:
OK: Got expected error: Error: Incorrect argument types for storage.StorageArea.get.
OK: Got expected error: Error: Incorrect argument types for runtime.connect.
FAIL: Expected a parameter validation error for runtime.sendMessage API
OK: Got expected error: Error: Incorrect argument types for i18n.

Extra info:
There is a unit test for validating the params of runtime.connect in background pages, this test can be re-used for content scripts when you fix the bug.
toolkit/components/extensions/test/mochitest/test_ext_background_runtime_connect_params.html

Updated

a year ago
Priority: -- → P3
Whiteboard: triaged
(Assignee)

Comment 1

a year ago
The validation issues for connect and i18n will be resolved by bug 1287007.

Schema-based parameter validation is disabled for runtime.sendMessage due to the allowAmbiguousOptionalArguments flag, so that has to be addressed separately.
Depends on: 1287007
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8782268 - Flags: review?(wmccloskey)
Attachment #8782269 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

a year ago
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab35cb80b63

Comment 5

a year ago
mozreview-review
Comment on attachment 8782268 [details]
Bug 1286712 - Validate runtime.sendMessage parameters

https://reviewboard.mozilla.org/r/72464/#review70468

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js:9
(Diff revision 1)
> +
> +add_task(function* test_sendMessage_error() {
> +  function background() {
> +    let circ = {};
> +    circ.circ = circ;
> +    let testCases = [

Can you do some tests passing in undefined?
Attachment #8782268 - Flags: review?(wmccloskey) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8782269 [details]
Bug 1286712 - Remove unused ExtensionContext and GlobalManager globals

https://reviewboard.mozilla.org/r/72466/#review70474
Attachment #8782269 - Flags: review?(wmccloskey) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Keywords: checkin-needed, leave-open
(Assignee)

Updated

a year ago
Assignee: nobody → rob
Status: NEW → ASSIGNED
Second commit message is missing the bug number.
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Updated commit description as requested.

Also updated dependency - with the fixes for bug 1287010, this bug would be fixed as well.
Depends on: 1287010
No longer depends on: 1287007
Keywords: checkin-needed

Comment 13

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/99d6e016dcef
Validate runtime.sendMessage parameters r=billm
https://hg.mozilla.org/integration/autoland/rev/7ad45b51d189
Remove unused ExtensionContext and GlobalManager globals r=billm
Keywords: checkin-needed

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99d6e016dcef
https://hg.mozilla.org/mozilla-central/rev/7ad45b51d189
(Assignee)

Comment 15

a year ago
Verified fixed in Firefox 51.0a1 (2016-08-25) using the manual test from the report.

The test case still shows "FAIL: Expected a parameter validation error for runtime.sendMessage API", but that's because it only tests for synchronously thrown errors. The global console shows "Error: runtime.sendMessage received too many arguments", which is what I'd expect.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
I’ve also confirm that the expected results mentioned in Comment 15 are encountered in Firefox 51.0a1 (2016-09-12) under Windows 10 64-bit.
Status: RESOLVED → VERIFIED
status-firefox51: --- → verified
You need to log in before you can comment on or make changes to this bug.