1389787 - Console runs proxy traps through transparent wrapper

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Oriol Brufau [:Oriol]]

The console does not run proxy traps when displaying a scripted proxy object.

However, if the proxy has a transparent wrapper, since the wrapper is not a scripted proxy, the console displays it as a normal object and runs internal methods of the wrapper, which are redirected to the scripted proxy.

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));
    Cu.waiveXrays(proxy);

The console shows "Object { a: 1 }" and runs various traps in the process.

I think the console should unwrap these transparent wrappers and display the underlying proxy instead. But no need to do so for opaque wrappers.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

Please review carefully because I don't know much about security wrappers and I don't want to create a security issue by removing wrappers which are not supposed to be removed.

I didn't add a test of a non-proxy which inherits from a proxy because this needs bug 1390701.

Comment 2

[Oriol Brufau [:Oriol]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e521f736f18668c06e742687de202d877ca1975

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

Also unwrapping proxies in _findSafeGetterValues.

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8897897 [details] [diff] [review]
unwrap-proxy.patch

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

(In reply to Oriol Brufau [:Oriol] from comment #1)
> Created attachment 8897659 [details] [diff] [review]
> unwrap-proxy.patch
> 
> Please review carefully because I don't know much about security wrappers
> and I don't want to create a security issue by removing wrappers which are
> not supposed to be removed.
> 
> I didn't add a test of a non-proxy which inherits from a proxy because this
> needs bug 1390701.

Going to forward this to Jim, I don't know enough about it to review

Comment 5

[Jim Blandy :jimb]

This is interesting. The patch doesn't look bad, although I don't like the bit that checks the class name against "Opaque"; I guarantee there will be other things that throw when you try to unwrap.

I think this hints at a more serious problem, though. The Debugger API doesn't doesn't provide the primitives for deciding what type of object a D.O refers to, that console really needs to show objects usefully. For example, this is the underlying cause of bug 1383158, where accessors throw unexpectedly because they're being applied to a proxy.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8897897 [details] [diff] [review]
unwrap-proxy.patch

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

This looks good overall; just one revision, and one question about the test case.

Debugger.Object.prototype.unwrap will refuse to unwrap wrappers that have security policies, so that should be okay.

::: devtools/server/actors/object.js
@@ +2383,5 @@
> + */
> +function unwrapProxy(obj) {
> +  if (obj.isProxy) {
> +    return obj;
> +  } else if (obj.class !== "Opaque") {

It's hard for me to believe that "Opaque" (I think this means a wrapper using OpaqueXrayTraits) is the only class of object that could cause `D.O.p.unwrap` to throw. I think you'll need a `try/catch` clause around the `unwrap` call.

::: devtools/server/tests/unit/test_objectgrips-18.js
@@ +53,5 @@
> +  gDebuggee.eval(`
> +    var global = Components.utils.Sandbox(null);
> +    var proxy = global.eval(\`
> +      var trapDidRun = false;
> +      new Proxy({}, new Proxy({}, {get: (_, trap) => {

Why do we have two layers of proxies here? From the description of the bug, I thought it would be sufficient to have a transparent wrapper around a Proxy; the bug is that the `get` handler method runs at all, right?

Comment 7

[Oriol Brufau [:Oriol]]

(In reply to Jim Blandy :jimb from comment #6)
> It's hard for me to believe that "Opaque" (I think this means a wrapper
> using OpaqueXrayTraits) is the only class of object that could cause
> `D.O.p.unwrap` to throw. I think you'll need a `try/catch` clause around the
> `unwrap` call.

In fact I was not trying to avoid exceptions. I only wanted to avoid unwrapping opaque wrappers because they already prevent calling proxy traps. I thought unwrap() was supposed to return null instead of throwing if it can't unwrap. But running the code in about:newtab seems to provide enough privileges to properly unwrap an opaque. So a try/catch won't prevent that, the condition or something else is needed.

> ::: devtools/server/tests/unit/test_objectgrips-18.js
> @@ +53,5 @@
> > +  gDebuggee.eval(`
> > +    var global = Components.utils.Sandbox(null);
> > +    var proxy = global.eval(\`
> > +      var trapDidRun = false;
> > +      new Proxy({}, new Proxy({}, {get: (_, trap) => {
> 
> Why do we have two layers of proxies here?

I could have only one proxy whose handler has all traps. But the list of traps is long and may change, so I use a second proxy as the handler and then I only need to care about the get trap.

Comment 8

[Oriol Brufau [:Oriol]]

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

Comment 9

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

Adding the try/catch.

Comment 10

[Jim Blandy :jimb]

(In reply to Oriol Brufau [:Oriol] from comment #7)
> In fact I was not trying to avoid exceptions. I only wanted to avoid
> unwrapping opaque wrappers because they already prevent calling proxy traps.
> I thought unwrap() was supposed to return null instead of throwing if it
> can't unwrap. But running the code in about:newtab seems to provide enough
> privileges to properly unwrap an opaque. So a try/catch won't prevent that,
> the condition or something else is needed.

I'm sorry, Oriol - I had forgotten how D.O.p.unwrap behaved. You are right, trying to unwrap something with a security wrapper returns null, rather than throwing an error, so there is probably no need for a try/catch there.

> I could have only one proxy whose handler has all traps. But the list of
> traps is long and may change, so I use a second proxy as the handler and
> then I only need to care about the get trap.

I see. This makes sense, but it is obscure, and needs a comment.

I will un-obsolete the original patch (without the try/catch) and approve that.

Comment 11

[Jim Blandy :jimb]

Comment on attachment 8897897 [details] [diff] [review]
unwrap-proxy.patch

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

::: devtools/server/tests/unit/test_objectgrips-18.js
@@ +53,5 @@
> +  gDebuggee.eval(`
> +    var global = Components.utils.Sandbox(null);
> +    var proxy = global.eval(\`
> +      var trapDidRun = false;
> +      new Proxy({}, new Proxy({}, {get: (_, trap) => {

This makes sense once it was explained to me, but it is obscure. Please add a comment explaining how the second proxy helps.

Comment 12

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

Just adding the requested comment to the approved patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20640980065f
Let the console unwrap proxy objects to avoid running traps. r=jimb

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7be912fc2a for https://treeherder.mozilla.org/logviewer.html#?job_id=123760736&repo=mozilla-inbound

Comment 15

[Phil Ringnalda (:philor)]

Also, timeouts in devtools/client/webconsole/test/browser_console.js like https://treeherder.mozilla.org/logviewer.html#?job_id=123765041&repo=mozilla-inbound

Comment 16

[Phil Ringnalda (:philor)]

Also chrome, https://treeherder.mozilla.org/logviewer.html#?job_id=123764695&repo=mozilla-inbound, and xpcshell, https://treeherder.mozilla.org/logviewer.html#?job_id=123764710&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=123764717&repo=mozilla-inbound

Comment 17

[Oriol Brufau [:Oriol]]

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

Comment 18

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

OK, so it seems unwrap() can throw indeed. And I didn't notice the eslint complaint.

Comment 19

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

Eslint was still complaining.

Comment 20

[Oriol Brufau [:Oriol]]

Instead of unwrapping manually, another possibility could be Cu.isProxy. And don't provide the proxy target and handler if the proxy is wrapped. What do you think?

Comment 21

[Jim Blandy :jimb]

(In reply to Oriol Brufau [:Oriol] from comment #18)
> Created attachment 8898303 [details] [diff] [review]
> unwrap-proxy.patch
> 
> OK, so it seems unwrap() can throw indeed.

Wow, I noticed the "invisible to debugger" error case, but assumed we should never run into that:

http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/js/src/vm/Debugger.cpp#11053

It's a surprise to me that our tests encounter objects in "invisible to debugger" compartments so frequently. I don't think they should. Filed as bug 1391449.

Comment 22

[Jim Blandy :jimb]

Comment on attachment 8898334 [details] [diff] [review]
unwrap-proxy.patch

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

::: devtools/server/actors/object.js
@@ +2373,5 @@
>  }
>  
> +/**
> + * Removes all the non-opaque security wrappers of a debuggee object
> + * and returns the debuggee object of underlying scripted proxy.

"of the underlying scripted proxy"

@@ +2395,5 @@
> +      return unwrapProxy(unwrapped);
> +    }
> +  }
> +  return undefined;
> +}

Something to consider, if you like: it's SpiderMonkey style to avoid 'else' after a 'return', and in general to try to keep the main flow of code unindented by returning early. That's not Devtools style, but I think it's helpful. For example, I'd find this version more legible:

    function unwrapProxy(obj) {
      if (obj.isProxy) {
        return obj;
      }

      if (obj.class === "Opaque") {
        return undefined;
      }

      let unwrapped;
      try {
        unwrapped = obj.unwrap();
      } catch (err) {
        return undefined;
      }

      if (!unwrapped || unwrapped === obj) {
        return undefined;
      }

      return unwrapProxy(unwrapped);
    }

::: devtools/server/tests/unit/test_objectgrips-18.js
@@ +53,5 @@
> +  // Create a proxy in a null principal, and debug it in a system principal context.
> +  // The proxy will have an opaque wrapper, or a transparent one when waiving Xrays.
> +  // To detect that no proxy trap runs, the proxy handler should define all possible
> +  // traps, but the list is long and may change. Therefore a second proxy is used as
> +  // the handler, so that a single `get` trap suffices.

Great comment. (Also: nice hack.)

Comment 23

[Jim Blandy :jimb]

(In reply to Oriol Brufau [:Oriol] from comment #20)
> Instead of unwrapping manually, another possibility could be Cu.isProxy. And
> don't provide the proxy target and handler if the proxy is wrapped. What do
> you think?

That seems okay to me, too.

Comment 24

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy.patch

Fixing typo and rearranging code to SpiderMonkey style.

Comment 25

[Oriol Brufau [:Oriol]]

(In reply to Jim Blandy :jimb from comment #23)
> That seems okay to me, too.

Then I will try that. I'm not much comfortable exposing the proxy target and handler if they are behind a wrapper.

Comment 26

[Oriol Brufau [:Oriol]]

I can't use Cu.isProxy alone because Cu is not defined on worker threads, and it seems proxies can still have security wrappers in worker threads (despite not having Xrays, presumably).

So I went back to the unwrap approach, and I started writing a much more complete test including the changes I will propose in bug 1390701.

However, I'm getting into cases in which unwrap() throws and Cu.isProxy works. So I think I will need to use both...

Comment 27

[Oriol Brufau [:Oriol]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=429e32f8dc5947f7566cd7a50a768880ccf8d984

Comment 28

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy-v2.patch

I ended up not using Cu.isProxy because when a security wrapper can't be removed it means that it denies access to the object, so proxy traps don't run. Therefore, not treating it as a proxy is fine.

Instead of adding a new test I rewrote test_objectgrips-17.js covering all cases: {system principal debuggee, null principal debugge} x {object from debugge, object from system principal with xrays, object from system principal without xrays, object from null principal with xrays, object from null principal without xrays}. But some tests are disabled until they are fixed by other bugs.

Comment 30

[Jim Blandy :jimb]

Comment on attachment 8899284 [details] [diff] [review]
unwrap-proxy-v2.patch

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

Okay, looks good to me. Just one typo.

::: devtools/server/actors/object.js
@@ +2397,5 @@
> +    return false;
> +  }
> +
> +  // Attempt to unwrap. If the debugger can't unwrap, then proxy traps won't be
> +  // allowed to run neither, so the object can be safely assumed to not be a proxy.

"allowed to run either"

Comment 31

[Oriol Brufau [:Oriol]]

Attachment: unwrap-proxy-v2.patch

Fixing the typo.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0574a1e59592
Let the console unwrap proxy objects to avoid running traps. r=jimb

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/0574a1e59592