1390701 - The console should not consider system principal proxy objects to be safe

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Oriol Brufau [:Oriol]]

The console considers system principal objects to be safe even when not wrapped in Xrays. But this is bad if they are a proxy or contain a proxy in their prototype chain, because various traps may run.

Example:

    var global = Components.utils.Sandbox(null);
    var traps = global.eval(`var traps = {}; traps;`);
    var proxy = global.eval(`
      new Proxy({a:1}, new Proxy({}, {get: (_, trap) => {
        traps[trap] = (traps[trap] || 0) + 1;
      }}));
    `);
    requestAnimationFrame(() => console.log("Trap count: ", traps));
    Object.create(Cu.waiveXrays(proxy));

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: isSafeJSObject-proxy.patch

Comment 2

[Oriol Brufau [:Oriol]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=870db2ede0217427a06eef5c1b64bc75abcf0f17

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8900713 [details] [diff] [review]
isSafeJSObject-proxy.patch

Review of attachment 8900713 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding to Jim

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8900713 [details] [diff] [review]
isSafeJSObject-proxy.patch

Review of attachment 8900713 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/tests/unit/test_objectgrips-17.js
@@ +44,5 @@
>    const [,, threadClient] = await attachTestTabAndResume(gClient, title);
>    gThreadClient = threadClient;
>  
> +  // Test objects created in the debuggee.
> +  await testPrincipal(undefined, false);

If I remember the other patch correctly, the calls to test_principal check both proxy objects and objects with proxies on their prototype chain, so this exercises the recursive path in isSafeJSObject, correct?

Comment 5

[Oriol Brufau [:Oriol]]

(In reply to Jim Blandy :jimb from comment #4)
> If I remember the other patch correctly, the calls to test_principal check
> both proxy objects and objects with proxies on their prototype chain, so
> this exercises the recursive path in isSafeJSObject, correct?

Yes, exactly. The problem was that, when creating a non-proxy grip, things like `rawObj instanceof Ci.nsIDOMCSSMediaRule` are used to determine what kind of object `rawObj` is. But this ran proxy traps if there was a proxy in the prototype chain. Treating `rawObj` as unsafe prevents this, and the test ensures that no trap runs.

Comment 6

[Oriol Brufau [:Oriol]]

For some reason the try tests on OS X don't run, but this shouldn't be OS-dependent so I guess it's OK.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa18594d4f60
Let devtools consider system principal proxy objects to be unsafe. r=jimb

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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