774753 - "document instanceof Object" -> false in a sandbox (Web Console)

Status: RESOLVED INACTIVE

(Core :: JavaScript Engine, defect)

Description

[Paul Rouget [:paul]]

If I'm not mistaken, it should be "true"

Comment 1

[Jeff Walden [:Waldo]]

The problem is sandboxes run in their own global object, and therefore have their own Object binding, I think.  So document instanceof Object gets the sandbox's Object, and document's not an instanceof that -- but it is an instanceof window.Object.

(Web console's mechanism is broken with no obvious fix; film at 11.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

If we really wanted to, we could forward the standard classes to the window, but then "{} instanceof Object" would test false.

Basically, there's no way to get sane behavior as long as the global for the console is not exactly the Window.  Which means the web console needs to not use a sandbox at all, since the whole point of a sandbox is to have a different global object.

Comment 3

[Bobby Holley (:bholley)]

We could do some nasty standard class remapping using the machinery I implemented for COWs in bug 760109. But I'm not sure I'm willing to do that. If you want everything to work transparently just like you were in the window, then you should execute your code in the window's scope.

Comment 4

[Mihai Sucan [:msucan]]

How can we avoid using a sandbox in the web console? How should we safely evaluate user's input?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Can you define "safely"?  What are your constraints other than not running it with the system principal?

If all that's needed is an API to run script in the exact same scripting environment as some Window (as in, just like an inline <script> in that web page would run), we can simply expose such a thing on windowutils.

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Can you define "safely"?  What are your constraints other than not running
> it with the system principal?
> 
> If all that's needed is an API to run script in the exact same scripting
> environment as some Window (as in, just like an inline <script> in that web
> page would run), we can simply expose such a thing on windowutils.

This would be really useful, but we also need to include our own globals for helpers. We have pprint(), $(), help() and other functions we add to the sandbox global "on top" of the window global. We need to do this without actually modifying the global scope of the web page.

If windowUtls would expose a way to execute JS in the given window scope, with an additional optional object that we can provide so the object keys become available in the global scope of JS execution, then that would allow us to replace the sandbox.

Something like:

windowUtils.executeInWindow(window, "pprint('foobar', location)", { pprint: function() { ... } });

Comment 7

[Boris Zbarsky [:bzbarsky]]

So we want:

1)  The global object is actually the window.
2)  Some bareword references refer to this other stuff we're injecting.

If the page defines a $() of its own (e.g. includes jQuery), do you want your $() to override that, or be overriden by it?  I believe right now you override it, but trying to understand whether that's design or happenstance....

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> So we want:
> 
> 1)  The global object is actually the window.
> 2)  Some bareword references refer to this other stuff we're injecting.
> 
> If the page defines a $() of its own (e.g. includes jQuery), do you want
> your $() to override that, or be overriden by it?  I believe right now you
> override it, but trying to understand whether that's design or
> happenstance....

We special-case $() and $$(). If the page already defines these two global functions we do not override them.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  What about if a page already has a global function called "help"?

In any case, if we're willing to have our stuff override the page's functions, I think we can do that: we just need the scope chain for our script evaluation to have our custom object with our stuff, followed by the window.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> OK.  What about if a page already has a global function called "help"?

In that case our help() takes precedence. If the user wants to execute the page global help() function he needs to do window.help(). We can't please everyone. We balanced it with $() and $$() only because we considered these are important.

> In any case, if we're willing to have our stuff override the page's
> functions, I think we can do that: we just need the scope chain for our
> script evaluation to have our custom object with our stuff, followed by the
> window.

Sure. The API should always override page's functions - we decide in the Web Console if we override stuff or not.

Probably stating the obvious: content scripts that execute as a result of input evaluation must not have access to the methods we add. These must be available only to the user's input scope.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, of course.

Bobby, how does this implementation plan sound?

1)  Add an API on windowutils, only callable from chrome, that takes a string and an
    object.
2)  The API creates a transparent proxy parented to the window wrapping the given object.
3)  Evaluate the string, using the proxy as a scope object.

What, if anything, do we need to do to ensure security stuff?

Note that the only reason for #2 is that we don't want to set the parent of the given object.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Yes, of course.
> 
> Bobby, how does this implementation plan sound?
> 
> 1)  Add an API on windowutils, only callable from chrome, that takes a
> string and an
>     object.
> 2)  The API creates a transparent proxy parented to the window wrapping the
> given object.
> 3)  Evaluate the string, using the proxy as a scope object.
> 
> What, if anything, do we need to do to ensure security stuff?
> 
> Note that the only reason for #2 is that we don't want to set the parent of
> the given object.

Yes, this should work. Luke tells me that enclosing scope lookup will bounce to the parent if the property isn't on the proxy. He couldn't remember whether the vm would check hasProperty or hasOwnProperty, which is important IMO with respect to which window.Object we end up with. If it's hasProperty, we'll probably need to change it.

Comment 13

[Mihai Sucan [:msucan]]

Also see bug 690529.

How would this new API behave if I eval |var foobar = 'hello world'|? Would foobar end up in the window object, accessible for the content scripts?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Luke tells me that enclosing scope lookup will bounce to the parent

Right, exactly.

> which is important IMO with respect to which window.Object we end up with. 

It's not important.  The proxy would be wrapping a non-global object, so it would be jjust fine.  Again, my main concern is around security: chrome is passing a chrome object to WindowUtils; we want to expose it to script running in the content compartment, so presumably we need a proxy to do the security and cross-compartment stuff here.  This could even be the proxy from #2 above, as long as we can control its parent.

> Would foobar end up in the window object, accessible for the content scripts?

Yes.  You can't have var end up on object 1 while Object comes off object 2 in a sane way...

Comment 15

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> > Luke tells me that enclosing scope lookup will bounce to the parent
> 
> Right, exactly.
> 
> > which is important IMO with respect to which window.Object we end up with. 
> 
> It's not important.  The proxy would be wrapping a non-global object, so it
> would be jjust fine.

I don't follow. If the proxy is wrapping an object in another compartment, and if scope looking returns non-own properties, then doing an unqualified lookup of |Object| is going to find |Object| on the proxy before ever finding it on the global.

> Again, my main concern is around security: chrome is
> passing a chrome object to WindowUtils; we want to expose it to script
> running in the content compartment, so presumably we need a proxy to do the
> security and cross-compartment stuff here.  This could even be the proxy
> from #2 above, as long as we can control its parent.

My assumption was that this would be a COW (which we get by default), with specifically enumerated __exposedProps__. Actually, come to think of it, once my patches in bug 760109 land, the "which window.Object" thing won't be an issue anymore, because COWs will remap prototype lookups for standard JS classes to the standard classes in the content compartment.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> then doing an unqualified lookup of |Object| is going to find |Object| on the proxy
> before ever finding it on the global.

Bareword lookup walks up the scope chain.  In this case the scope chain is two elements:
[ proxy, Window].

For each object on the scope chain, a property lookup is done.  This checks for a property on that object.

So this would do a property lookup on the proxy, which would forward to the underlying object, which would check for own properties on that object and up its proto chain.  But there is no global on its proto chain (it's just a random object, so its proto is Object.prototype), so it wouldn't find Object.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #16)

> So this would do a property lookup on the proxy, which would forward to the
> underlying object, which would check for own properties on that object and
> up its proto chain.  But there is no global on its proto chain (it's just a
> random object, so its proto is Object.prototype), so it wouldn't find Object.

Ah, right! I was confused. Thanks for the explanation. :-)

Comment 18

[Rob Campbell [:rc] (:robcee)]

so, I think rather than trying to wedge of all the DOM objects into the sandbox' global, we're thinking of having the Debug protocol have an "offline" or "disabled" mode that still lets us use its eval mechanism but without incurring the penalties associated with full-on debugging. AIUI, doing this would not be that hard and would do away with our need for sandbox shenanigans.

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #14)
> > Would foobar end up in the window object, accessible for the content scripts?
> 
> Yes.  You can't have var end up on object 1 while Object comes off object 2
> in a sane way...

Cool. This would fix bug 690529. This is what devs seem to want and expect. Rob.... are we sure we want this?

Comment 20

[Jim Blandy :jimb]

(In reply to Boris Zbarsky (:bz) from comment #16)
> > then doing an unqualified lookup of |Object| is going to find |Object| on the proxy
> > before ever finding it on the global.
> 
> Bareword lookup walks up the scope chain.  In this case the scope chain is
> two elements:
> [ proxy, Window].
> 
> For each object on the scope chain, a property lookup is done.  This checks
> for a property on that object.
> 
> So this would do a property lookup on the proxy, which would forward to the
> underlying object, which would check for own properties on that object and
> up its proto chain.  But there is no global on its proto chain (it's just a
> random object, so its proto is Object.prototype), so it wouldn't find Object.

Well, it would find things like toString, which is also wrong.

We should use a custom proxy, which only presents its target's *own* properties as bindings.

Comment 21

[Jim Blandy :jimb]

Whatever underlying mechanism we decide to offer here to get evaluation done right, we can present it via the debugging protocol without requiring an attached debugger. Perhaps a "quickEvaluate" message understood by BrowserTabActors (those presented in response to a "listTabs" request sent to the root actor). No attaching needed.

Comment 22

[Jim Blandy :jimb]

There's no way to ensure that content won't be able to reach the utility functions. The developer can always simply evaluate:

window.f = myPreciousUtility;

The [proxy, Window] chain makes it impossible for content to get at utilities without the developer's help, but we should assume that developers will do dangerous things like the above.

So it seems to me:

1) The utilities must be designed to cope with dangerous input. They need to assume that hostile content is going to call them with carefully-crafted arguments.

2) The web console needs to be written under the assumption that the utility function objects themselves will get messed with: custom toString properties placed on them, for example. So, when we associate a web console with a given window, the web console should eval up a fresh set of function objects that just call our utility functions, expose those fresh function objects via the proxy, and never touch them again. This ensures that the only content-to-console references are from the fresh function objects' scope chains, which are not accessible other than by simply calling the functions. So content can't reach any object the console implementation will touch.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Jim Blandy :jimb from comment #20)
> Well, it would find things like toString, which is also wrong.
> 
> We should use a custom proxy, which only presents its target's *own*
> properties as bindings.

IMO this should be be the default behavior when examining an enclosing scope (i.e. the VM should be checking hasOwnProperty rather than hasProperty before deciding whether to bounce to the parent). Is there are reason to do it the other way?

Comment 24

[Rob Campbell [:rc] (:robcee)]

ok, having had a chance to reread through the preceding comments and Jimb's followups I think bz' and bholley's recommendations, having a proxy around window and preventing scope creeping by restricting with hasOwnProperty seems sane.

(In reply to Mihai Sucan [:msucan] from comment #19)
> (In reply to Boris Zbarsky (:bz) from comment #14)
> > > Would foobar end up in the window object, accessible for the content scripts?
> > 
> > Yes.  You can't have var end up on object 1 while Object comes off object 2
> > in a sane way...
> 
> Cool. This would fix bug 690529. This is what devs seem to want and expect.
> Rob.... are we sure we want this?

That's pretty-much what we have now. You can still manipulate properties in the window object through the sandbox (and indeed, make modifications to the DOM).

Jim, I think the implementation of a Debugger protocol quickEval method can be done as a wrapper around whatever the solution is here. Does that make sense?

Comment 25

[Jim Blandy :jimb]

(In reply to Rob Campbell [:rc] (:robcee) from comment #24)
> Jim, I think the implementation of a Debugger protocol quickEval method can
> be done as a wrapper around whatever the solution is here. Does that make
> sense?

Sure --- the protocol mechanism is independent of how we get the VM to behave the way we want. That's what I meant in comment 21.

Comment 26

[Thaddee Tyl [:espadrine]]

Attachment: WIP Using the debugger API

This patch leverages the API introduced by bug 785174 to fix the following unexpected side-effects of the current implementation:

> document instanceof Object  // false

> var foo = 5
> window.foo  // undefined

I'd love to get a quick review, specifically on the command line API.
The way this patch works uses Debugger.Object.prototype.makeDebuggeeValue()
and Debugger.Object.prototype.evalInGlobalWithBindings() (the latter requiring the former).

However, this comes with restrictions; objects that come out of it are no longer inspectable. Not sure why.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 659578 [details] [diff] [review]
WIP Using the debugger API

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

(In reply to Thaddee Tyl [:espadrine] from comment #26)
> I'd love to get a quick review, specifically on the command line API.
> The way this patch works uses Debugger.Object.prototype.makeDebuggeeValue()
> and Debugger.Object.prototype.evalInGlobalWithBindings() (the latter
> requiring the former).

Quick, eh? I can do that!

> However, this comes with restrictions; objects that come out of it are no
> longer inspectable. Not sure why.

Did you make sure that inspection is not performed against the Debugger.Object returned by evalInGlobalWithBindings? You probably need to change that part to use Debugger.Object.getOwnPropertyNames etc.

::: browser/devtools/webconsole/HUDService-content.js
@@ +724,4 @@
>    get console() this.window.console,
>  
>    /**
> +   * The Debuggee() object where code is evaluated.

The parens are misleading (there is no Debuggee constructor).

@@ +730,3 @@
>  
> +  _debugger: null,
> +  consoleAPI: {},

Not important, but how come you didn't follow the existing practice of storing the console object as a debuggee property?

@@ +983,4 @@
>    {
> +    this._evalLocation = this.window.location;
> +    let debuggees = this._debugger.getDebuggees();
> +    if (debuggees.length > 0) {

No need for this check, and also a for...of loop would be shorter.

@@ +988,5 @@
> +        this._debugger.removeDebuggee(debuggees[i]);
> +      }
> +    }
> +    this._debugger.addDebuggee(this.window);
> +    this.debuggee = this._debugger.getDebuggees()[0];

addDebuggee already returns the Debugger.Object you need to store in this.debuggee.

@@ +1008,5 @@
>    /**
> +   * Convert an object to a command that can be added to consoleAPI.
> +   *
> +   * @param mixed
> +   *        The object to convert to a command.

This comment needs fixing.

@@ +1037,4 @@
>        aString = "help()";
>      }
>  
> +    return this.debuggee.evalInGlobalWithBindings(aString, this.consoleAPI).return;

You need to check that the completion value contains a "return" property and handle the other cases as well.

Furthermore, if you are returning a Debugger.Object you need to make sure callers know how to deal with it.

Comment 28

[Mihai Sucan [:msucan]]

Comment on attachment 659578 [details] [diff] [review]
WIP Using the debugger API

Thaddee: this looks good. Thank you for the patch! Not much to add to Panos's comments, but I do have some concerns:

The debugger will return a Debuggee.Object, which means all callers need to be updated to work with it. This is why objects are no longer inspectable. Making these changes will quickly turn this patch into something rather big. I would like us to avoid having to do a huge rebase for bug 768096.

Panos: my second concern is related to the debugger: won't this turn off JS optimizations in the web page? The main reason I avoided doing attachThread() in bug 768096 was to avoid performance issues - slowing down the web page. Has that decision been changed?

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #28)
> The debugger will return a Debuggee.Object, which means all callers need to
> be updated to work with it. This is why objects are no longer inspectable.
> Making these changes will quickly turn this patch into something rather big.
> I would like us to avoid having to do a huge rebase for bug 768096.

Indeed, I think this patch should probably be rebased on top of the Big Console Rewrite.

> Panos: my second concern is related to the debugger: won't this turn off JS
> optimizations in the web page? The main reason I avoided doing
> attachThread() in bug 768096 was to avoid performance issues - slowing down
> the web page. Has that decision been changed?

AFAIK it will have a perf impact, once the Debugger API is used. But note that this patch only uses the Debugger API when the user decides to evaluate code or inspect an object. In such cases my hunch is that a small drop in performance may not be noticeable, compared to something that would add overhead to every console.log call.

Also, if the consensus among our JS mavens is that evalInGlobalWithBindings is the API we need to use for the console, then that's certainly something that we should factor into our decision. Performance is an important goal, but intuitive behavior is pretty important, too.

This patch, as it currently stands, presents a good opportunity to measure the kind of perf impact the Debugger API will have, without the remote protocol overhead getting in the way. If you find it regresses substantially your recent perf improvements, then that's another important thing to consider.

Comment 30

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #29)
> (In reply to Mihai Sucan [:msucan] from comment #28)
> > The debugger will return a Debuggee.Object, which means all callers need to
> > be updated to work with it. This is why objects are no longer inspectable.
> > Making these changes will quickly turn this patch into something rather big.
> > I would like us to avoid having to do a huge rebase for bug 768096.
> 
> Indeed, I think this patch should probably be rebased on top of the Big
> Console Rewrite.

Thank you. That is what I would like.


> > Panos: my second concern is related to the debugger: won't this turn off JS
> > optimizations in the web page? The main reason I avoided doing
> > attachThread() in bug 768096 was to avoid performance issues - slowing down
> > the web page. Has that decision been changed?
> 
> AFAIK it will have a perf impact, once the Debugger API is used. But note
> that this patch only uses the Debugger API when the user decides to evaluate
> code or inspect an object. In such cases my hunch is that a small drop in
> performance may not be noticeable, compared to something that would add
> overhead to every console.log call.
> 
> Also, if the consensus among our JS mavens is that evalInGlobalWithBindings
> is the API we need to use for the console, then that's certainly something
> that we should factor into our decision. Performance is an important goal,
> but intuitive behavior is pretty important, too.
> 
> This patch, as it currently stands, presents a good opportunity to measure
> the kind of perf impact the Debugger API will have, without the remote
> protocol overhead getting in the way. If you find it regresses substantially
> your recent perf improvements, then that's another important thing to
> consider.

This is something that can break expected behavior for users: code runs at certain speeds once, later it runs slower (after poking with the web console). This can make a dev believe he has caused certain regressions. As long as we cause noticeable performance impact we should notify the user - or otherwise cause this slowdown from the start (when he opens the web console).

Thaddee: please let me know when this patch is ready for performance testing. Currently it needs some updates - to make it properly work with the rest of the console code. Thanks!

Comment 31

[Boris Zbarsky [:bzbarsky]]

I'm pretty opposed to causing slowdown when the web console is just open.  That's what we get with Firebug, and it's caused us huge perception problems amongst web developers....

Comment 32

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #31)
> I'm pretty opposed to causing slowdown when the web console is just open. 
> That's what we get with Firebug, and it's caused us huge perception problems
> amongst web developers....

I'm also pretty unhappy with the idea of causing slowdowns when I decide to inspect an object... for no "obvious" reasons (as a web dev).

Comment 33

[Jim Blandy :jimb]

Although eventually we will want to evaluate code in the context of the script debugger's currently selected frame, for evaluating expressions in global objects, this need not have any ongoing Debugger-caused performance impact, if we do evaluation like this:

- add the global as a debuggee.
- call evalInGlobalWithBindings
- remove the global as a debuggee.

When the global isn't a debuggee, Debugger should have no effect on the code generated for it. Entering and leaving debug mode will cause previously generated machine code to get flushed, but so do lots of other operations.

Comment 34

[Mihai Sucan [:msucan]]

|document instanceof Object| returns |true| with the patch for bug 783499. This issue is going to be fixed within the Web Console once that patch lands.

Comment 35

[Mihai Sucan [:msucan]]

Bug 783499 landed and |document instanceof Object| returns true in the Web Console. This bug is fixed now. However, the problem still stands with Cu.Sandbox().

Feel free to keep this open for Cu.Sandbox() or close it if this bug is only tracking the Web Console feature.