Last Comment Bug 690529 - Scratchpad should evaluate expressions in the scope of the content window, not in a sandbox
: Scratchpad should evaluate expressions in the scope of the content window, no...
Status: RESOLVED WORKSFORME
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 585552 665834 756731 844620 (view as bug list)
Depends on: 783499 825039
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-29 13:51 PDT by Jim Blandy :jimb
Modified: 2013-08-02 11:34 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Page to highlight a couple confusing bits of behavior (793 bytes, text/html)
2011-10-28 07:04 PDT, Kevin Dangoor
no flags Details

Description Jim Blandy :jimb 2011-09-29 13:51:13 PDT
Expressions entered into the Web Console and the Scratchpad are evaluated in a sandbox global whose prototype is the content window: this.__proto__ === window.

This has some nice effects: variables declared in a Scratchpad are effectively local to that scratchpad, instead of polluting content's global; you can play around. If one does want to create a global variable visible to content, one can always simply create a property on window: 'window.newGlobal = "fruit"'.

However, I suspect this is actually not helpful. The simplest mental model for developers is for code entered in the Web Console / Scratchpad to be evaluated just like the contents of their <script> elements. Explaining why 'x' shows the content's x but 'x = 5' doesn't change it (but 'x.y = 5' *is* visible to content!) involves a degree of detail that only more engaged developers have any interest in.

It's certainly valuable to provide utility functions that aren't present in content; but that can be done by evaluating expressions in the scope of a 'with' expression applied to an object with the utility functions as its properties. 'With' isn't my favorite construct, but since we completely control the object's properties, the shortcomings of 'with' use in general don't apply; once you've decided to introduce some Web Console-only functions, the behavior of 'with' is pretty much what you need. (Don't forget that the following creates bindings for x and y on the global:

    with (o) { var x = 5; y = 6; }

So 'with' doesn't re-introduce the sandbox problems I mention above.)
Comment 1 Panos Astithas [:past] 2011-09-30 01:06:48 PDT
I'm not sure if using 'with' is possible here, due to the general move to use strict mode in Firefox code. Although the HUDService.jsm doesn't set itself in strict mode (yet), I've seen strict mode warnings pop up when its code paths are reached from another function that had the "use strict"; pragma. We could try this change and see if we can get away with it now, but I'm afraid this would only be a temporary solution.

Bug 665834 presents some similar problems with our current approach. bz suggested that either the JS engine should provide the sandbox with some magic or that we should give up and allow pollution of the page global with variables set in the console. What's your opinion on that?
Comment 2 David Dahl :ddahl 2011-09-30 09:07:04 PDT
Ccing mrbkap as he may have some insight here - perhaps a capability similar to using 'with' can be added to the sandbox?
Comment 3 Rob Campbell [:rc] (:robcee) 2011-10-03 13:35:16 PDT
I'm not sure these are "problems" per se.

The Sandbox is the preferred mechanism for evaluating user code in content interactively. We're giving the sandbox the content window's prototype so the Sandbox can inherit methods/functions defined there and interact with the page.

Variable locations are somewhat confusing if you look at them under a microscope like this, but I expect in practice, it won't come up. Why? Because if you're writing in a scratchpad and enter var x = window.someProperty ... ; That variable x is visible to the rest of the scratchpad. If the user then copies that scratchpad into a <script> in content, var x now lives in content and it should Just Work.

The only time it gets weird is when users do something like window.x = something and start polluting the content's window or doing DOM manipulations. Not really a bad thing. A reload makes them go away.

Can we WONTFIX this?
Comment 4 Kevin Dangoor 2011-10-03 19:54:49 PDT
I didn't try IE, but both Firebug and Chrome's developer tools evaluate in window scope as Jim was expecting. I agree with Jim's statement here:

"Explaining why 'x' shows the content's x but 'x = 5' doesn't change it (but 'x.y = 5' *is* visible to content!) involves a degree of detail that only more engaged developers have any interest in."

This is very subtle.
Comment 5 Panos Astithas [:past] 2011-10-03 23:53:34 PDT
I think the weird behavior described in bug 665834 is even more prone to confuse developers. IIUC we used a sandbox for security reasons. How does the risk that we had considered at that time compare against such developer confusion? What was the attack scenario that we were worried about?

If I'm mistaken and the sandbox was used to avoid confusing developers with content namespace pollution, then we can have a straightforward comparison between the two evils.
Comment 6 Kevin Dangoor 2011-10-05 06:15:38 PDT
bug 585552 appears to be the same bug and bug 585552 comment 3 mentions that it's a "sandbox issue". mrbkap is cc'ed here, which is good. I think the first thing to figure out is whether there is an actual security concern with just evaluating the code in page scope as the others seem to do.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-10-05 06:25:26 PDT
There is the aesthetic concern of using with(x) { evalscript } to run everything. Also, there's been a long-standing wish to purge with from js. I'm not really a fan of doing that.
Comment 8 David Dahl :ddahl 2011-10-05 07:12:45 PDT
(In reply to Panos Astithas [:past] from comment #5)
> If I'm mistaken and the sandbox was used to avoid confusing developers with
> content namespace pollution, then we can have a straightforward comparison
> between the two evils.

It is not a confusion/namespace issue at all. It is a security issue.

As far as evaluating the typed expression in the page scope, there is only one way to do it, and that is via a sandbox. 'eval' is a non actor here, as it is way too insecure.

If anything, we need to file a bug for more subtle control over the sandbox's scope, better mimicking the contentWindow.
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-10-06 17:23:33 PDT
As far as security concerns go, I have two:

- If we execute web console code directly in the browser's scope, how do we expose any additional functions without also exposing them to the content page itself? (Keep in mind that a sandbox protects us from caller.callee.callee type attacks.)

- If a web page wanted to, it could make inspecting its DOM pretty difficult from the console. With a sandbox, we could turn on Xray wrappers, which would fix that.

Aside from that, I don't see a reason we couldn't have an "execute code as though it were a <script> in the page" console mode. IMO most of this comes down to taste. I tend to feel that debug tools should operate by default on their own global to avoid affecting the debugged page as much as possible.
Comment 10 Jim Blandy :jimb 2011-10-08 15:02:07 PDT
I expect most people using debugging tools *want* to affect the debugged page, if that's what they type. Side effects should, you know, have side effects.

I'm not familiar with what sorts of security-sensitive utility functions we provide in the Web Console. I'm surprised that we provide any at all. I'll chat with folks on-line about this.
Comment 11 Jim Blandy :jimb 2011-10-08 15:04:44 PDT
For what it's worth:

function evalWithUtils(code, utils) {
    with(utils) {
        return (function () { "use strict"; return eval(code); })();
    }
}

js> evalWithUtils("(function q(){return q.caller.caller.caller;})()", {})
typein:16: TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them
js>
Comment 12 Jim Blandy :jimb 2011-10-08 15:16:47 PDT
But the 'with' and 'eval' stuff are implementation details, not the point of this bug. If we need new primitives to be able to say what we want to say, then we can have new primitives.

The point I'd like to persuade people of is that evaluating code in the Web Console should behave like evaluating code in the document, because the latter is the behavior people using the Web Console are trying to understand.

The ability to have temporary bindings seems not worth the confusion.

Preventing side effects altogether might be a valuable option. But what we do now is prevent some effects (say, variable declaration) while permitting others (say, property assignment and function calls), with the decision about what to prevent resting on what happens to be easy given the underlying facilities we're using to implement the thing. Come on, folks.

Security issues trump all, obviously. But I'd like to learn more about that; it's hard to believe we can't get secure and convenient behavior here.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-10-11 12:05:24 PDT
You write a compelling argument, Mr. Blandy.

I'd like to get some opinions on the relative insecurity of using $IMPLEMENTATION_DETAIL over a Sandbox from mrbkap and security people.

cc'ing David Chan for consultation.
Comment 14 David Chan [:dchan] 2011-10-18 14:24:18 PDT
If I am understanding the bug correctly, the desired solution for this bug would have to fulfill the following requirements
1. Provide the same safety as a sandbox
2. Not expose web console / scratchpad functions to content
3. Allow modification of the page content if desired

My primary concern would be item 2 due to potential privilege escalation attacks. I also think that executing in <script> should be a preference / option. 

It may be possible to construct a Proxy object in the sandbox which achieves these goals.
* proxy traps all gets / sets done in the web console / scratchpad
* missing variables / function declarations are defined in page context as needed
* catch calls to web console / scratchpad functions.
** if the page redefines these functions, we should probably call those instead similar to the current code for $() and $$()

The add-on SDK team uses a Proxy object to mediate content-scripts, though their goal was to prevent global pollution. See bug# 601295
Comment 15 Jim Blandy :jimb 2011-10-20 15:16:36 PDT
(In reply to David Chan [:dchan] from comment #14)
> My primary concern would be item 2 due to potential privilege escalation
> attacks.

In principle, we should be able to protect scratchpad-provided utility functions from content if they're in a different compartment. I know the right checks happen; I'm just not sure how to set it all up.

>I also think that executing in <script> should be a preference / option.

Preferences/options are just excuses for not engaging in a nice vigorous argument! :D

More seriously: a pref needs to be supported by an argument that people out there in the field want to be able to switch between both. That is, the pref itself must be desired by users; prefs cannot be used to settle UI disagreements amongst implementers.
Comment 16 Jim Blandy :jimb 2011-10-25 11:54:30 PDT
So, here's how I was imagining the security stuff would work:

- The utility functions would refuse to be called by content.
- The user's code would run with chrome privileges.
- The user's code would run with the content window as its global, so assignments would work.

In our current system, it's not possible to distinguish code's privileges from its global. So this is a dead end.
Comment 17 Kevin Dangoor 2011-10-28 06:39:32 PDT
*** Bug 585552 has been marked as a duplicate of this bug. ***
Comment 18 Kevin Dangoor 2011-10-28 06:53:58 PDT
*** Bug 665834 has been marked as a duplicate of this bug. ***
Comment 19 Kevin Dangoor 2011-10-28 07:04:05 PDT
Created attachment 570253 [details]
Page to highlight a couple confusing bits of behavior

Jim's comment 16 is doubtless correct, but this bug has been reported 3 times by quite technical people (Jim here, Julian Viereck and David Flanagan). I've attached a test page that shows a couple of ways in which users are quite likely to tripped up (including David Flanagan's object that is clearly an array but fails instanceof Array).

The Web Console's evaluation of code differs from every other JS console in this respect and I think it will only lead to confusion for our users.
Comment 20 Jim Blandy :jimb 2011-10-28 09:08:47 PDT
(In reply to Kevin Dangoor from comment #19)
> (including David Flanagan's object that
> is clearly an array but fails instanceof Array).

Oh, that's lovely.
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-25 08:54:18 PST
Triage - filter on PEGASUS
Comment 22 Panos Astithas [:past] 2012-01-30 02:04:19 PST
Er, it seems that David Flanagan's STR are working now. Tried on nightly and 9.0.1:

var a = [1,2,3];
a instanceof Array

returns true. Also, setting a = [1] modifies it, without requiring window.a to be used. When did this behavior change?
Comment 23 Bobby Holley (busy) 2012-02-14 09:49:15 PST
As noted in bug 726949, the current mechanism for making this work (prototyping the sandbox global to the window) is no longer possible with gecko. So this will probably have to change one way or another.
Comment 24 Kevin Dangoor 2012-03-13 07:11:48 PDT
(In reply to Panos Astithas [:past] from comment #22)
> Er, it seems that David Flanagan's STR are working now. Tried on nightly and
> 9.0.1:
> 
> var a = [1,2,3];
> a instanceof Array
> 
> returns true. Also, setting a = [1] modifies it, without requiring window.a
> to be used. When did this behavior change?

When you create an Array in the Web Console, instanceof Array will work because it's the same Array object.

When you do instanceof Array on an array that's set on window by content, it will fail. See attachment 570253 [details]
Comment 25 Panos Astithas [:past] 2012-03-13 08:38:13 PDT
(In reply to Kevin Dangoor from comment #24)
> (In reply to Panos Astithas [:past] from comment #22)
> > Er, it seems that David Flanagan's STR are working now. Tried on nightly and
> > 9.0.1:
> > 
> > var a = [1,2,3];
> > a instanceof Array
> > 
> > returns true. Also, setting a = [1] modifies it, without requiring window.a
> > to be used. When did this behavior change?
> 
> When you create an Array in the Web Console, instanceof Array will work
> because it's the same Array object.
> 
> When you do instanceof Array on an array that's set on window by content, it
> will fail. See attachment 570253 [details]

*Blush*
This always trips me up, as I've already confessed in bug 665834 comment 5.
Comment 26 Allison 2012-05-21 20:30:45 PDT
*** Bug 756731 has been marked as a duplicate of this bug. ***
Comment 27 Rob Campbell [:rc] (:robcee) 2013-02-05 14:27:02 PST
*** Bug 837060 has been marked as a duplicate of this bug. ***
Comment 28 Mihai Sucan [:msucan] 2013-02-07 11:21:23 PST
This is going to be fixed by bug 783499.
Comment 29 Mihai Sucan [:msucan] 2013-02-24 09:13:18 PST
*** Bug 844620 has been marked as a duplicate of this bug. ***
Comment 30 Mihai Sucan [:msucan] 2013-04-11 08:48:12 PDT
This bug is now fixed in the Web Console. Brandon is working on the fix for Scratchpad in bug 825039.
Comment 31 Masatoshi Kimura [:emk] 2013-05-30 00:18:05 PDT
|this===window| still returns false in the Web Console even with the latest Nightly.
Comment 32 Mihai Sucan [:msucan] 2013-06-18 10:40:35 PDT
(In reply to Masatoshi Kimura [:emk] from comment #31)
> |this===window| still returns false in the Web Console even with the latest
> Nightly.

Just tested and I confirm. I find this odd. Thanks for pointing it out.

Going to reopen bug 837060. This bug is about scratchpad evaluating expressions using the content window scope, not in a sandbox.
Comment 33 Brandon Benvie [:benvie] 2013-08-02 11:34:30 PDT
Aside from the behavior of `this !== window` which bug 837060 is about, this bug is fixed by bug 825039 landing.

Note You need to log in before you can comment on or make changes to this bug.