Closed Bug 690529 Opened 13 years ago Closed 11 years ago

Scratchpad should evaluate expressions in the scope of the content window, not in a sandbox

Categories

(DevTools Graveyard :: Scratchpad, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: jimb, Unassigned)

References

Details

Attachments

(1 file)

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.)
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?
Ccing mrbkap as he may have some insight here - perhaps a capability similar to using 'with' can be added to the sandbox?
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?
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.
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.
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.
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.
(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.
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.
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.
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>
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.
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.
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
(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.
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
Triage - filter on PEGASUS
Priority: -- → P2
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?
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.
(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]
(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.
This is going to be fixed by bug 783499.
Component: Developer Tools → Developer Tools: Console
Depends on: 783499
OS: Linux → All
Hardware: x86_64 → All
This bug is now fixed in the Web Console. Brandon is working on the fix for Scratchpad in bug 825039.
Component: Developer Tools: Console → Developer Tools: Scratchpad
Depends on: 825039
Summary: Web Console and 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, not in a sandbox
Version: unspecified → Trunk
|this===window| still returns false in the Web Console even with the latest Nightly.
(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.
Aside from the behavior of `this !== window` which bug 837060 is about, this bug is fixed by bug 825039 landing.
Status: REOPENED → RESOLVED
Closed: 13 years ago11 years ago
Resolution: --- → WORKSFORME
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: