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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Rob Wu [:robwu]]

Attachment: 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

Comment 1

[Rob Wu [:robwu]]

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.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1286712 - Validate runtime.sendMessage parameters

The main motive for this patch is to remove the use of the GlobalManager
global (which was used to see if an extension ID is valid, which was
specifically added in order to create thebrowser_ext_lastError.js test).
To preserve test coverage I implemented a full validation of the
runtime.sendMessage method.

Now the error for a non-existent extension is identical in both the
content script and background pages. Note that this also fixes a
minor privacy leak: Previously extensions could see whether another
extension is installed by sending a message to the specified extension
and using the different responses to see whether another extension is
installed.

Review commit: https://reviewboard.mozilla.org/r/72464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72464/

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1286712 - Remove unused ExtensionContext and GlobalManager globals

The presence of these globals interfere with the attempt to get ext-*.js
scripts to load in a content process because these globals are only
available in the main process.

Review commit: https://reviewboard.mozilla.org/r/72466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72466/

Comment 4

[Rob Wu [:robwu]]

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

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8782269 [details]
Bug 1286712 - Remove unused ExtensionContext and GlobalManager globals

https://reviewboard.mozilla.org/r/72466/#review70474

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 8782268 [details]
Bug 1286712 - Validate runtime.sendMessage parameters

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

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8782269 [details]
Bug 1286712 - Remove unused ExtensionContext and GlobalManager globals

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Second commit message is missing the bug number.

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 8782268 [details]
Bug 1286712 - Validate runtime.sendMessage parameters

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72464/diff/2-3/

Comment 11

[Rob Wu [:robwu]]

Comment on attachment 8782269 [details]
Bug 1286712 - Remove unused ExtensionContext and GlobalManager globals

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72466/diff/2-3/

Comment 12

[Rob Wu [:robwu]]

Updated commit description as requested.

Also updated dependency - with the fixes for bug 1287010, this bug would be fixed as well.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99d6e016dcef
https://hg.mozilla.org/mozilla-central/rev/7ad45b51d189

Comment 15

[Rob Wu [:robwu]]

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.

Comment 16

[Vasilica Tamas, QA [:vtamas]]

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.