1320181 - Storage sanitizer should return an empty object belonging to the correct scope.

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[21Naown]

chrome.storage.local.set({mapObject: mapObjectOther});

... show this message in console:

Security wrapper denied access to property (void 0) on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported. (source inconnue)

Comment 1

[Kris Maglione [:kmag]]

The message is only a warning. storage.local only supports storing JSON-compatible objects, which means that maps ignored, and when used as the root object of a storage value, replaced with an empty object.

Comment 2

[21Naown]

Hum, my bad for JSON-compatible objects, I did not check if maps were compatible. I assume you are focusing on the "unknown source" now, that is why you let this ticket opened?

Comment 3

[Kris Maglione [:kmag]]

The security wrapper warning still needs to be fixed, yes. It's harmless, but it's also confusing.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1320181 - Storage sanitizer should return an empty object belonging to the correct scope.

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8815009 [details]
Bug 1320181 -  Storage sanitizer should return an empty object belonging to the correct scope.

https://reviewboard.mozilla.org/r/96026/#review96140

::: toolkit/components/extensions/ExtensionStorage.jsm:66
(Diff revision 1)
> +        default:
> +          Cu.reportError(`${context.contentWindow.location.href} is trying to serialize a ${className} instance into a WebExtensions storage`);

I don't think this is necessary.

::: toolkit/components/extensions/ExtensionStorage.jsm:75
(Diff revision 1)
> -    return {};
> +    // NOTE: using cloneInto here to prevent security wrappers errors to be logged.
> +    return Cu.cloneInto({}, context.contentWindow);

`return new context.cloneScope.Object()`

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8815009 [details]
Bug 1320181 -  Storage sanitizer should return an empty object belonging to the correct scope.

Hi Kris,
I've just attached a small patch that change the sanitizer helper to:

- return an empty object which belongs to the correct scope.
- log a detailed warning message when the saved value of an unsupported type (e.g. if it's a function or an instance of a non-supported constructor like a Map or a Set etc), but without preventing the other properties to be saved correctly.

Let me know if the change it is in the right direction or if you had other ideas of how to handle this scenarios.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8815009 [details]
Bug 1320181 -  Storage sanitizer should return an empty object belonging to the correct scope.

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

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=6954aa4fda1573cdfe01781ecbd6110d86cc888c&selectedJob=31972625

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8815009 [details]
Bug 1320181 -  Storage sanitizer should return an empty object belonging to the correct scope.

https://reviewboard.mozilla.org/r/96026/#review96500

::: toolkit/components/extensions/ExtensionStorage.jsm:24
(Diff revision 2)
>                                    "resource://gre/modules/AsyncShutdown.jsm");
>  
> -function jsonReplacer(key, value) {
> +/**
> + * Helper function used to sanitize the objects that have to be saved in the ExtensionStorage.
> + *
> + * @param {ExtensionBaseContextChild | ContentScriptContextChild} context

`@param {BaseContext}`

::: toolkit/components/extensions/ExtensionStorage.jsm:25
(Diff revision 2)
> + *   the current extension context.
> + * @param {string} key
> + *   the key of the current JSON property.
> + * @param {any} value
> + *   the value of the current JSON property.

Please capitalize descriptions.

::: toolkit/components/extensions/ExtensionStorage.jsm:88
(Diff revision 2)
>     *        The extension context in which to sanitize the value
>     * @returns {value}
>     *        The sanitized value.
>     */
>    sanitize(value, context) {
> -    let json = context.jsonStringify(value, jsonReplacer);
> +    let json = context.jsonStringify(value, (...args) => jsonReplacer(context, ...args));

`jsonReplacer.bind(context)`

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8815009 [details]
Bug 1320181 -  Storage sanitizer should return an empty object belonging to the correct scope.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f46301403352
Storage sanitizer should return an empty object belonging to the correct scope. r=kmag

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f46301403352