1394559 - Better handling of denied objects in the console

Status: RESOLVED FIXED

(DevTools :: Console, enhancement)

Description

[Oriol Brufau [:Oriol]]

There are objects which have security wrappers that deny all access. These objects have an "Object" class (or "Function" if callable), so the console attempts to do all kinds of things to display that object.

However, all these operations throw errors, which are displayed in the browser console, and this seems wrong. And displaying the object as "Object {}" can be confusing, the user might think it's really an empty object while in fact it's just an object which denies access.

Example of such an object:

1. Load http://example.com
2. Open web console
3. Enter this code:
    var iframe = document.createElement('iframe');
    iframe.sandbox = 'allow-scripts';
    iframe.src = 'data:text/html,<script>console.log({a:123})<\/script>';
    document.body.appendChild(iframe);
4. Right-click the logged object, choose "Store as global variable"
5. "temp0" appears in the console, press enter

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: denied-object.patch

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

Comment 2

[Oriol Brufau [:Oriol]]

Comment on attachment 8901974 [details] [diff] [review]
denied-object.patch

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

::: devtools/server/actors/object.js
@@ +90,5 @@
> +      // wrapper. Change the class so that this will be visible in the UI.
> +      if (DevToolsUtils.isCPOW(this.obj)) {
> +        g.class = "CPOW: " + g.class;
> +      } else {
> +        g.class = "Denied";

Better naming suggestions?

@@ +100,5 @@
>      } else {
> +      g.class = this.obj.class;
> +      g.extensible = this.obj.isExtensible();
> +      g.frozen = this.obj.isFrozen();
> +      g.sealed = this.obj.isSealed();

Removing the try statement passes all tests in the try push, but not sure if it would be safer to keep it.

@@ +116,5 @@
> +        let length = DevToolsUtils.getProperty(this.obj, "length");
> +        g.ownPropertyLength = length;
> +      } else if (!["Function", "Proxy"].includes(g.class)) {
> +        // Bug 1163520: Assert on internal functions
> +        g.ownPropertyLength = this.obj.getOwnPropertyNames().length;

Ditto.

::: devtools/shared/webconsole/test/test_jsterm_queryselector.html
@@ +92,5 @@
>    let response = yield evaluateJS("$$(':foo')");
>    checkObject(response, {
>      input: "$$(':foo')",
>      exceptionMessage: "SyntaxError: ':foo' is not a valid selector",
> +    exception: {}

Not sure why this case is treated as a Denied object and thus loses it's preview. The cases above in this test file still have a preview.

Comment 3

[Jim Blandy :jimb]

Filed bug 1394992 for the obsolete bug 1163520 workaround. That conflicts with the patch here; this one should probably land first.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8901974 [details] [diff] [review]
denied-object.patch

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

I'm rejecting this solely because the jsterm_queryselector.html regression looks serious to me. I'd want to understand what's going on there before landing this. The rest of it looks good.

::: devtools/server/actors/object.js
@@ +83,5 @@
>        "type": "object",
>        "actor": this.actorID
>      };
>  
> +    let accesDenied = !unwrap(this.obj);

Should be spelled "accessDenied".

@@ +90,5 @@
> +      // wrapper. Change the class so that this will be visible in the UI.
> +      if (DevToolsUtils.isCPOW(this.obj)) {
> +        g.class = "CPOW: " + g.class;
> +      } else {
> +        g.class = "Denied";

How about "Inaccessible"?

@@ +100,5 @@
>      } else {
> +      g.class = this.obj.class;
> +      g.extensible = this.obj.isExtensible();
> +      g.frozen = this.obj.isFrozen();
> +      g.sealed = this.obj.isSealed();

We should do whatever makes it easiest to learn about cases that we haven't covered (and I don't doubt there are some). I think that means leaving the `try` out, because a throw here will cause mochitests to fail and alert us to the problem. If we keep the `try` with the empty `catch`, that's a silent failure, which is not helpful.

@@ +105,3 @@
>      }
>  
> +    if (g.class != "DeadObject" && !accesDenied) {

I would definitely expect the `isFrozen` and `isSealed` calls above to throw on dead objects. Could we handle dead objects up above, after the "access denied" case?

Overall, I guess I would expect this function to have the structure:

check for opaque wrapper
    construct limited grip and return

check for dead object
    construct limited grip and return

check for proxy
    construct limited grip and return

common tail for ordinary objects

@@ +111,5 @@
>  
>        // FF40+: Allow to know how many properties an object has
>        // to lazily display them when there is a bunch.
> +      if (TYPED_ARRAY_CLASSES.indexOf(g.class) != -1) {
> +        // Bug 1348761: getOwnPropertyNames is unecessary slow on TypedArrays

While you're here, could we fix this typo? should be "unnecessarily slow"

::: devtools/shared/webconsole/test/test_jsterm_queryselector.html
@@ +92,5 @@
>    let response = yield evaluateJS("$$(':foo')");
>    checkObject(response, {
>      input: "$$(':foo')",
>      exceptionMessage: "SyntaxError: ':foo' is not a valid selector",
> +    exception: {}

I think this regression needs to be worked out before we can land this patch.

Comment 5

[Oriol Brufau [:Oriol]]

(In reply to Jim Blandy :jimb from comment #4)
> I would definitely expect the `isFrozen` and `isSealed` calls above to throw
> on dead objects. Could we handle dead objects up above, after the "access
> denied" case?

It seems they don't throw. Enter this in a privileged web console:

    var win = window.open();
    win.close();
    setTimeout(() => console.log({
      obj: win,
      extensible: Object.isExtensible(win),
      frozen: Object.isFrozen(win),
      sealed: Object.isSealed(win)
    }), 1e3);

Result: Object { obj: DeadObject, extensible: true, frozen: false, sealed: false }

Comment 6

[Oriol Brufau [:Oriol]]

Attachment: denied-object.patch

Renamed grip class to "Inaccessible".

Early returns for CPOW, Inaccessible and DeadObject objects. Not for Proxy, because it can have a preview with the target and handler.

Fixed "unnecessarily slow" typo.

I noticed that _findSafeGetterValues could run proxy traps if the proxy was inaccessible. So now safe getters are not searched in inaccessible objects. Then I removed the isProxy function because in all calls I already had the unwrapped object at hand.

Added more tests in test_objectgrips-17.js to ensure proxy traps don't run.

The querySelector problem was just that the thrown error belonged to a system principal, and thus wasn't accessible from non-system principals debuggees. This is being fixed in bug 1395276.

Comment 7

[Oriol Brufau [:Oriol]]

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

Comment 8

[Oriol Brufau [:Oriol]]

Attachment: denied-object.patch

Updating the patch with the changes from bug 1394992.

Comment 9

[Oriol Brufau [:Oriol]]

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

Comment 10

[Oriol Brufau [:Oriol]]

Attachment: denied-object.patch

I was relying on bug 1274657 to prevent object inspector from running proxy traps. However, a proxy could be behind an inaccessible object, and then expanding it will run traps. So I changed onPrototypeAndProperties to prevent this, and added new tests.

Comment 11

[Oriol Brufau [:Oriol]]

I meant bug 1388831 instead of bug 1274657.

Comment 12

[Oriol Brufau [:Oriol]]

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

Comment 13

[Jim Blandy :jimb]

This series of bugs has really brought home to me that I don't have a grip on just how many different kinds of objects one may encounter in Firefox. There's unimportant variety: different constructors, different WebIDL objects. But even limiting the conversation to objects with different fundamental behaviors: native objects, proxies, xrays, dead objects, opaque wrappers, and so on, I don't have a lot of confidence that we've identified all the cases we need to cover.

This isn't a criticism of the patch at all; investigating, patching, and writing tests is a fine process to improve things. But I would be much more comfortable if we were working from an authoritative list of behaviors we need to cope with, than treating the code base like an unexplored country, and handling cases as we discover them.

Comment 14

[Oriol Brufau [:Oriol]]

Of course, but it seems there is no such authoritative list of behaviors. BTW, I changed the review request of bug 1395276 because you were taking so long to respond, but if you are available now maybe you have more context to review it than nchevobbe.

Comment 15

[Jim Blandy :jimb]

(In reply to Oriol Brufau [:Oriol] from comment #14)
> BTW, I changed the review request of bug 1395276 because you were taking so
> long to respond, but if you are available now maybe you have more context to
> review it than nchevobbe.

That's completely reasonable. I can finish this review tomorrow.

Comment 16

[Jim Blandy :jimb]

Comment on attachment 8907846 [details] [diff] [review]
denied-object.patch

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

Thanks very much.

::: devtools/server/actors/object.js
@@ +103,5 @@
> +    // Otherwise, increment grip depth and attempt to create a preview.
> +    this.hooks.incrementGripDepth();
> +
> +    // The `isProxy` getter is called on `unwrapped` instead of `this.obj` in order
> +    // to detect proxies behing transparent wrappers, and thus avoid running traps.

"behind"

Comment 17

[Oriol Brufau [:Oriol]]

Attachment: denied-object.patch

Fixing the typo. But this can't land until bug 1395276 is fixed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d899ef058ab
Better handling of inaccessible objects in the console. r=jimb

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/3d899ef058ab

Comment 20

[Marcus Niemietz]

I do not want to criticize this patch but I think it is not a smart way to just print out "Inaccessible { }". From my point of view, it could be a good idea to display the old output *and* an additional "Inaccessible" flag. 

Example: As described in ticket 1408376, we have the following behavior in FF 58.0a1-2017-10-15 Nightly:
- cd(window.frames[0])
- top.location.href
SecurityError: Permission denied to access property "href" on cross-origin object
- window.parent
Inaccessible {  }

You might think that window.parent is completely inaccessible including sub-properties like "length"; this is of course wrong. Furthermore, we have two different messages. Message two does not give you any valuable information why the browser disallows access.

Comment 21

[Oriol Brufau [:Oriol]]

Yes, when I did this patch I assumed that objects from a global not subsumed by the debuggee would be completely inaccessible. And I thought that other objects would mostly be safe, so with this check most try statements could be removed. However, it's not that simple. Cross-origin Window and Location objects may expose some properties, and some same-origin objects are problematic (bug 1406961).

Since it seems there is no reliable way to know whether an object can be accessed or not without accessing it, it might be better to just do it, but protecting everything with try statements.

That said, scripted proxy objects should still never be accessed. And objects from invisible-to-debugger compartments might be scripted proxies, so they should be blacklisted too.

So maybe the logic could be

 1. Attempt to unwrap.
 2. If that throws, it's from an invisible-to-debugger compartment.
    1. If it's a CPOW, mark it as "CPOW: class" and do not access it. End.
    2. Otherwise, mark it as "InvisibleToDebugger: class" and do not access it. End.
 3. If an object is returned,
    1. Check if it's a proxy. If so, mark it as "Proxy" and expose its target and handler. End.
 4. If null is returned, mark it as potentially/partially inaccessible (any name suggestion better than "Inaccessible"?)
 5. Create a preview, but expect that internal operations may throw.

Jim, does this sound good? I could do this while fixing bug 1403536.

Comment 22

[Oriol Brufau [:Oriol]]

(In reply to Oriol Brufau [:Oriol] from comment #21)
> any name suggestion better than "Inaccessible"?

Just after posting the comment I thought that "Restricted" could be better.

Comment 23

[Marcus Niemietz]

This somehow sounds like a "hard" statement. How about:

[Partial restricted Object] Error Message XY with details

Comment 24

[Oriol Brufau [:Oriol]]

(In reply to Marcus Niemietz from comment #23)
> This somehow sounds like a "hard" statement. How about:
> 
> [Partial restricted Object] Error Message XY with details

A class this long would be bad when you want to display a bunch of these objects in an array.

So showing long messages should be handled in reps, because this may need different approaches depending on the mode (tiny, short, long).

Reps could check whether the grip class is "Restricted" to know when to show some kind of message. It could explain that the targeted document is not allowed to fully access the object, and suggest using "Select an iframe as the current targeted document" to fix it.

Comment 25

[Marcus Niemietz]

This sounds goood. I am fine with it.

Comment 26

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 57.0a1 (2017-08-28) on Windows 10, 64 Bit!

This bug's fix is verified latest Beta!

Build ID   : 20171218174357
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment 27

[Oriol Brufau [:Oriol]]

Clearing flag because I did it anyways in bug 1403536.

There is https://github.com/devtools-html/debugger.html/issues/6120 about improving how reps deals with restricted objects.