Andrew Swan [:aswan] Assignee
	Description • 8 years ago

If we are in a privileged scope and have an object from a different scope that needs to be stringified, calling JSON.stringify() in the privileged scope doesn't work since cross-compartment wrappers don't wrap functions, which breaks toJSON().

Provide a way to safely do the equivalent of JSON.stringify() from the original scope.

	Andrew Swan [:aswan] Assignee
	Comment 1 • 8 years ago

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

	Kris Maglione [:kmag]
	Comment 2 • 8 years ago

https://reviewboard.mozilla.org/r/54014/#review50746

::: js/xpconnect/src/XPCComponents.cpp:3296
(Diff revision 1)
>  }
>  
> +static bool
> +JSONCreator(const char16_t* aBuf, uint32_t aLen, void* aData)
> +{
> +    nsAString* result = static_cast<nsAString*>(aData);

This should probably be a static cast to `nsAutoString*`.

::: js/xpconnect/src/XPCComponents.cpp:3298
(Diff revision 1)
> +static bool
> +JSONCreator(const char16_t* aBuf, uint32_t aLen, void* aData)
> +{
> +    nsAString* result = static_cast<nsAString*>(aData);
> +    result->Append(static_cast<const char16_t*>(aBuf),
> +                   static_cast<uint32_t>(aLen));

No need for the casts.

::: js/xpconnect/src/XPCComponents.cpp:3309
(Diff revision 1)
> +                                            HandleValue aScope,
> +                                            JSContext* aCx,
> +                                            MutableHandleValue aResult)
> +{
> +    if (!aScope.isObject())
> +        return NS_ERROR_FAILURE;

`NS_ERROR_INVALID_ARG`

::: js/xpconnect/src/XPCComponents.cpp:3319
(Diff revision 1)
> +        JS_ReportError(aCx, "Permission denied to stringify object in scope");
> +        return NS_ERROR_FAILURE;
> +    }
> +
> +    // Jump into the target scope.
> +    JSAutoCompartment compartment(aCx, scope);

This, and the operations that need to happen in the target comparment, need to be in a separate block scope.

::: js/xpconnect/src/XPCComponents.cpp:3326
(Diff revision 1)
> +    // Wrap value appropriately.  Assuming aScope is where value was
> +    // created, the object should be unwrapped and we'll have access
> +    // to toJSON() etc.
> +    RootedValue val(aCx, aValue);
> +    MutableHandleValue value(&val);
> +    JS_WrapValue(aCx, value);

`JS_WrapValue(aCx, &val)`

::: js/xpconnect/src/XPCComponents.cpp:3327
(Diff revision 1)
> +    // created, the object should be unwrapped and we'll have access
> +    // to toJSON() etc.
> +    RootedValue val(aCx, aValue);
> +    MutableHandleValue value(&val);
> +    JS_WrapValue(aCx, value);
> +        

Nit: trailing space.

::: js/xpconnect/src/XPCComponents.cpp:3330
(Diff revision 1)
> +    MutableHandleValue value(&val);
> +    JS_WrapValue(aCx, value);
> +        
> +    // Do the actual stringification.
> +    nsAutoString json;
> +    if (!JS_Stringify(aCx, value, nullptr, JS::NullHandleValue, JSONCreator, &json)) {

s/value/&val/

::: js/xpconnect/src/XPCComponents.cpp:3334
(Diff revision 1)
> +    nsAutoString json;
> +    if (!JS_Stringify(aCx, value, nullptr, JS::NullHandleValue, JSONCreator, &json)) {
> +        return NS_ERROR_FAILURE;
> +    }
> +
> +    RootedString result(aCx, JS_NewUCStringCopyZ(aCx, json.get()));

This, and the parts that follow, need to happen in the caller's compartment.

Also, `JS_NewUCStringCopyN` would be better.

::: js/xpconnect/tests/unit/test_json.js:19
(Diff revision 1)
> +    do_check_eq(native, wrapper, `Stringify of ${what} with wrapper matches native stringify`);
> +  }
> +
> +  check_val("123", "simple number");
> +  check_val(`"abc"`, "simple string");
> +  check_val(`{ toJSON() { return "hello"; } }`, "Object with toJSON()");

We should probably also check that stringifying a value that the target global doesn't have access to will fail.

::: js/xpconnect/tests/unit/xpcshell.ini:76
(Diff revision 1)
>  [test_import.js]
>  [test_import_fail.js]
>  [test_interposition.js]
>  [test_isModuleLoaded.js]
>  [test_js_weak_references.js]
> +[test_json.js]

This file name should be a bit less generic. The full name of the method in place of json would do.

	Bobby Holley (:bholley)
	Comment 3 • 8 years ago

If the need for this is pretty isolated, why not just make a same-origin Sandbox and invoke JSON.stringify from there?

	Kris Maglione [:kmag]
	Comment 4 • 8 years ago

I did that initially for the extension storage API, but Bill was worried about the overhead of creating sandboxes for something like this, so we wound up backing it out.

	Bobby Holley (:bholley)
	Comment 5 • 8 years ago

Hm, what's the threshold here and how often will this be called? FWIW we create temporary sandboxes in DOM bindings for certain cases.

I'm not necessarily opposed to this I guess, I'd just like to understand if this is something that happens all the time or pretty infrequently.

	Andrew Swan [:aswan] Assignee
	Comment 6 • 8 years ago

The immediate case for this came up with the native messaging WebExtensions APIs.  Chrome docs for the equivalent feature are at https://developer.chrome.com/extensions/nativeMessaging but in short, an extension gets a pipe to a native process running on the same host and they pass messages that get serialized to JSON.
Typical uses like password managers probably don't have a huge volume of messages getting passed but I don't have a good handle on the tail of other things folks want to use this API for.

	Kris Maglione [:kmag]
	Comment 7 • 8 years ago

For the connectNative API, it needs to happen every time the extension sends a message. Depending on the extension, that could be many times per second.

I suppose we could cache the sandbox, but I'd rather avoid that if we can.

	Andrew Swan [:aswan] Assignee
	Comment 8 • 8 years ago

Though, I guess we could a single Sandbox for the lifetime of the native application, then the number of messages is irrelevant.  I would assume that the cost of creating and destroying sandboxes is small compared to launching native applications...

	Andrew Swan [:aswan] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8754518 [details]
MozReview Request: Bug 1274386 Implement Cu.stringifyJsonInScope()

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

	Andrew Swan [:aswan] Assignee
	Comment 10 • 8 years ago

Updated just to capture Kris's feedback, I'm now leaning toward scrapping this in favor of bholley's suggestion.

	Bobby Holley (:bholley)
	Comment 11 • 8 years ago

Up to you guys - I guess I'm ok with adding a random Cu API if you decide you want it, as long as the code that consumes it is in-tree, so we can remove it if we need to. I doubt anyone else will use it.

	Andrew Swan [:aswan] Assignee
	Comment 12 • 8 years ago

We're going to try other approaches for native messaging, this can be brought back from the graveyard if somebody ends up wanting it.