It is non-standard and it makes "private variables" via closures advisory only. See the URL. IIRC Venkman and other Mozilla apps use eval(s, o). These XUL apps should use evalInSandbox, if they aren't already. /be

Brian, can you take this onto your list to investigate? mrbkap is your Obi-Wan. Thanks, /be

Sure!

bclary has an extension that depends on the 2nd argument. At the moment, I don't think it's possible to use evalInSandbox as a drop-in because you can't control the variables object with it. It seems that this is a pretty common use-case, though, so we should add that feature, somehow.

actually, atm the extension depends on window.eval working even with the warning about being called directly. eval(...) nor eval(..., window) work. I do need a way to compile a script at least into the current window.

Oh, never mind me. I got my eval forms messed up. Carry on with the removal!

Help me, Obi Wan. You're my only reviewer.

Comment on attachment 327463 [details] [diff] [review] as clumsy as a blaster A thermal detonator would have been nicer (who needs eval anyway?) but this'll do nicely for now.

Should I be reporting a strict warning here, perhaps? Would that make it easier for extensions depending on this, errr, extension to track it down?

Trivial, but I thought you might like to review the error message itself.

Comment on attachment 327674 [details] [diff] [review] with a strict warning instead I think the message is ok. Saying too much would probably just beg questions ;-). >- * Script.prototype.compile/exec and Object.prototype.eval all take an >- * optional trailing argument that overrides the scope object. >+ * Script.prototype.compile/exec and Object.prototype.eval no longer take >+ * an optional trailing argument. Comment is still out of date (was before, but may as well fix). Strike the Script.prototype.* noise and just say "eval", since eval is no longer in Object.prototype. > */ > if (argc >= 2) { >- if (!js_ValueToObject(cx, argv[1], &scopeobj)) >- return JS_FALSE; >- argv[1] = OBJECT_TO_JSVAL(scopeobj); >+ if (!JS_ReportErrorFlagsAndNumber(cx, >+ JSREPORT_WARNING | JSREPORT_STRICT, >+ js_GetErrorMessage, NULL, >+ JSMSG_EVAL_ARITY)) { >+ return JS_FALSE; >+ } > } Could use if (&&) instead of if-if, but the diff is nicer this way. No big, your call. r=me with comment fix and optional nesting change. /be

Comment on attachment 327674 [details] [diff] [review] with a strict warning instead Cleaned up the comment and merged the conditional (preferring code over patch), landed as: changeset: 15612:e4fea8e9d036

Can we do this before 1.9.1?

Could we move them into bogus section and perform a repository ticket reverse?

I'm not sure I understand your question in relation to this bug, Andrea

My question was: could we please ignore this bug and leave this feature that for some reason "we discovered only today" and instead of use them for amazing stuff we are scared about "some undefined closure virus"? What I mean, seriously, is that this is not a bug, but a Gecko feature, and this is not a security problem, no more than eval itself, because if this is a security problem, our site is not secure at all even before somebody could use this feature. So, why did you choose to "fix" them killing hundreds of possibilities for code introspection, sandbox possibilities, and who knows what else? I do like your way to work, but this time, in my opinion, it was too quick :) Best Regards

Andrea: Do you have a specific compelling use-case for this "feature"? Are you sure you can't implement it via other techniques?

I wrote these two example rught now without testing (and thinking) too much, just as example: {{{ // sandbox: redefined environemnt var myEnvironMent = { eval:function(script){ return eval(script); }, alert:function(String){ (document.body || document.documentElement) .appendChild(document.createTextNode(String)); } }; // evaluate a script from a string or a textarea eval("alert('Hello World')", myEnvironMent); /* you cannot do this with a with statement with(myEnvironMent) eval("alert('Hello World')"); */ // privileged protected methods in prototype Protected = function(){ function _showMessage(message){ alert(this.name.concat(" say ", message)); } return function(name){ this.name = name; }; }(); Protected.prototype._ = function(method){ return eval("_" + method, this).apply(this, Array.prototype.slice.call(arguments, 1)); }; var p = new Protected("Andrea"); p._("showMessage", "Hello World"); }}}

You probably know that, but please note that last example could be executed inside the closure as well, but could not be executed outside, and that is only a method. So, it means that you cannot redefine a prototype of your own script runtime, outside one closure, using that feature. Regards

It's true we are taking away functionality not available otherwise. One question of whether the loss is worth the gain in standard-following and integrity is hard to answer in ways that convince different parties. Those valuing integrity hate this extension. Those using it for reflection and extensibility love it. Dessert topping and floor wax. Another question: is there a way of fixing this bug without removing the optional second argument? Could we avoid exposing SpiderMonkey's intrinsic scope chain link, the parent slot (__parent__, called [[Scope]] in ES3), which is an implementation detail? That is, the hole for private variables lies in the fact that eval(s, o) uses o, o.__parent__, etc. up the scope chain. If o is a function scoped by an activation containing private variables, o.__parent__ is the activation and the privacy has no integrity. We do not want to switch to an extrinsic link, but could we censor the intrinsic one in just this case? If we wrap o in a With object, then it behaves as would an object in an implementation with extrinsic scope chain links. Crowder, the with wrapping is already there for the o.eval(s) code, in the #if JS_HAS_EVAL_THIS_SCOPE code. This should be reused. Sorry I didn't raise this possibility sooner. /be

Brendan, first of all thank you to consider my point of view. Secondly, your suggestion removes the feature as well, because doing them, I will not be able to create something like that: ---------------- const safeEnvironment = function(A, B, D, F, N, O, R, S){ const Array = A, Boolean = B, Date = D, Function= F, Number = N, Object = O, RegExp = R, String = S, eval = function(String){ return self.eval("(" + String + ")"); }; return new function(){}; }( Array, Boolean, Date, Function, Number, Object, RegExp, String ); ---------------- as a secure environment for code execution and/or evaluation, i.e. ---------------- eval('Array=function(){}; alert(Array);', safeEnvironment); // native Array, no redefinition possibilities thanks to const keyword ---------------- Please reconsider this bogus and ask to Webkit and Opera guys to implement this feature, instead of removing them from every browser :D As a clever developer said in John blog, it is like somebody asks to remove Reflection from .NET or other languages because it is able to read private stuff. Does it make any sense? So, why should we block this language possibility? Kind Regards

Would it be alright to handle restoring the reflection/utility of eval() in another bug? I'd like to just get this fix landed, it is simple and straightforward.

Brian, I would like to move this ticket into bogus, or mark them as WONT FIX. There is no reason at all to consider this one a bug, while this other one, for example, could be more interesting, isn't it? ---------------- eval.call(this, "var PARTIALLY_HIDDEN = 123;"); alert(PARTIALLY_HIDDEN) // undefined !!! unexpected alert(this.PARTIALLY_HIDDEN) // 123, as expected alert("PARTIALLY_HIDDEN" in this) // true, as expected // the same with (function(){ eval.call(this, "var PARTIALLY_HIDDEN = 123;"); })(); // the interesting stuff come with const, that works as expected eval.call(this, "const GLOBAL = 123;"); alert(GLOBAL) // 123 ---------------- Please, do not fix / close this ticket related Gecko feature. Thank you.

As noted in comment #11, I have already landed a patch on the main-trunk. I have not landed that same patch on the 1.9 branch, but I think it is sufficient, doesn't break web-content, and is an important step in addressing legitimate concerns about this feature. I still feel the remaining development (ie., restoring the utility of the second argument in a secure way) should continue in another bug. I think the examples you cite in comment #23 are an issue with duplicates another bug already in bugzilla; though I cannot find it right now.

(In reply to comment #24) > As noted in comment #11, I have already landed a patch on the main-trunk. Is there any reason to not re-resolve this bug as FIXED then?

I marked it fixed, but Brendan re-opened. I asked for forgiveness in comment #22 and he hasn't replied yet. I'd like to do my penance in another bug.

Regressing and then restoring "later" risks never. If this is a bad enough change for some people, we should avoid regressing. /be

We're not regressing, we're making a change in order to achieve compliance with the spec. The restoration of whatever reflection utility is desired is an enhancement. It's not entirely clear to me what's required for that, anyway, since Andrea seems dissatisfied with the "with" wrapping fix you suggested. I would like to land this fix for 1.9.0.2 in a timely fashion, and address Andrea's concerns in another bug. Yes, that bug would be an enhancement bug, and yes, there would be a risk of "never" fixing it, but in the worst possible case, that would mean that we behave exactly as IE, Safari, and Opera do today.

In the intro to chapter 15, paragraphs 5 and 6, ES3 says (IIRC ES1 said): "Unless otherwise specified in the description of a particular function, if a function or constructor described in this section is given more arguments than the function is specified to allow, the behaviour of the function or constructor is undefined. In particular, an implementation is permitted (but not required) to throw a TypeError exception in this case. NOTE Implementations that add additional capabilities to the set of built-in functions are encouraged to do so by adding new functions rather than adding new parameters to existing functions." So there's not a spec compliance issue, we just didn't do as encouraged 10 ago (see rev 3.3 of jsobj.c). Changing our ~10 year old behavior in a breaking way is a regression, no point in denying it. Undoing the regression while avoiding the problem Peter Michaux pointed out, all in a separate bug, is ok -- just don't LATER it :-/. Please cite it here and mark this one FIXED. Thanks, /be

(disclosure: I came to know about this eval feature only after reading Peter Michaux) One of the possibilities I see with this eval feature is, you can write smaller bookmarklet or Greasemonkey script to fix some website problem. And the fact is most intranet sites are still not supporting Firefox. I would like to give power to the end user than a web developer. so want I vote for making this WONTFIX And for "caja" they can solve it by a rewrite rule for eval.

It's not just Caja -- some people view private variables as advisory while others want mandatory cross-browser enforcement. I think the followup bug, or this one if the patch backs out (no need, just saying that would make the REOPEN => ASSIGNED transition sensible), should be patched to hide the intrinsic scope chain link implementation detail. [[Scope]] in a function is JSSLOT_PARENT in SpiderMonkey, and universal for all objects. But that can be hidden with some special casing in obj_eval for function object as 2nd arg. One way is to skip all Call objects, using the global reached at the end of the scope chain. /be

Isn't one of the main utilities of this function that of being able to reflect into functions? Can someone clarify for me the use-cases we're trying to preserve? The use-cases we're trying to defend against? They seem to be rather at-odds with one another in my current understanding; but maybe I'm missing something. I'm increasingly tempted to just back-out my changeset for this on the trunk, and WONTFIX this bug (with another bug perhaps to be opened to refine this feature as needed?).

After a discussion with mrbkap, I've decided to follow my original plan to proceed with the current removal (already done on trunk) patches, and follow-up to restore whatever functionality Andrea, Brendan, and Biju are looking for in bug 446026.

(In reply to comment #32) > Isn't one of the main utilities of this function that of being able to reflect > into functions? No. But that's not what Peter Michaux showed: he showed peeking into closures from functions. > Can someone clarify for me the use-cases we're trying to > preserve? See comment 17 and comment 20, again. /be

I'm disappointed to see this capability removed before the replacement capability is defined, discussed, and implemented (even if in the same release). It seems to have great utility for allowing eval in a controlled, sandboxed environment.

Bill, there's nothing controlled or sandboxed about eval(s,o) -- too many ways to attack o from within s, get out of the sandbox. The evalInSandbox method of Components.utils is the better way to go. The most to be done for eval(s, o) is avoid leaking the scope chain abstraction's intrinsic link implementation. /be

I trust crowder to restore eval(s, o) to some semblance of its former self, or to add evalInSandbox and show that it covers the cited use-cases. If this does not happen in the 3.1 development cycle (a short one!), then I will be the first to re-open this bug and ask for a back-out. /be

Should we get a test for this before landing on the stable branch?

It seems like this shouldn't actually land on branch unless bug 446026 is a branch bug as well.

I don't think that bug 446026 should block this bug landing anywhere, and that is one of the reasons I advocated moving that work there. Here's a test: function env() { var foo = "bar"; return function () { foo } }; eval("foo", env()) That should throw a reference error, if this bug is fixed.

As I said to Brian on IRC earlier, let's get an automated test for this before approval.

There is a clear need for certain types of applications to be able to access closure variables of functions. In particular, serialization comes to mind. If I want to serialize an object where one of its properties is a function which accesses enclosed variables, how should I go about that without access to __parent__ or using this method. Serialization is not something I would call a "bug", especially since it has been available as long as I can remember in SpiderMonkey. Removing it is a regression and will break code I have written. In fact, if anything needs to be fixed, its that we need to use eval to do it rather than simply giving back access to __parent__, which was another hastily decided "bug fix". If we want to explore ways to strictly enforce private variables, I would consider that a feature request, not a bug fix, since SpiderMonkey hasn't had strictly enforced private variables before.

There are other ways to implement serializable structures in the language and libraries that were specified in ECMA-262. This feature was not specified in the language or libraries documented there and it presents other issues that must be resolved (documented elsewhere). I am interested in restoring the utility of a feature along these lines that better suits the recommendations of the language spec, but this is neither to place to do that work, nor to advocate for it. Bug 446026 is the place for the work, but bugzilla is not a good place for advocacy in general. A number of different blog entries have been written on the matter, and newsgroups are also available for discussions like this. Please direct concerns like this to those places, instead.

How about this: -------------------------------------------- var classT= (function(){ var pv=(Math.random()+'').replace(/\./,''), _=function(){}; eval('pv=(function(){var _'+pv+'=[12,34];return function(){return _'+pv+';}})();'); pv.toString=function(){return '';}; _.get=function(){ return pv(); }; return _; })(); alert(typeof pv); alert(classT.get()); eval('alert(""+pv);',classT); -------------------------------------------- If we can't get the variables pv() returns, we may not need to fix the bug.

(In reply to comment #44) > How about this: Sorry, there's still some way to get the variables. My test is not enough.

Any update here on getting a test? Bob?

/cvsroot/mozilla/js/tests/js1_8/regress/regress-442333-01.js,v <-- regress-442333-01.js initial revision: 1.1 mozilla-central: changeset: 16480:df0c529a1069

Comment on attachment 329522 [details] [diff] [review] applied to 1.9 branch Approved for 1.9.0.2. Please land in CVS. a=ss

verified fixed 1.9.0/trunk linux/mac/win

What was the reasoning behind taking this behaviour change on the stable branch where it has broken a number (albeit small) of extensions and presumably websites. Was there any announcement of the change? I do not see it in the 3.0.2 release notes.

I wouldn't presume that it broke web sites, but it's possible that it did -- have you any examples? (Running script in another context isn't nearly as widely done on the web, this technique was not very well known, and it's not available in any other browser.)

(In reply to comment #39) > It seems like this shouldn't actually land on branch unless bug 446026 is a > branch bug as well. Dao was right, and I did not pay attention to bugmail -- sorry. This patch should not have gone into cvs. /be

(In reply to comment #52) > (In reply to comment #39) > > It seems like this shouldn't actually land on branch unless bug 446026 is a > > branch bug as well. > > Dao was right, and I did not pay attention to bugmail -- sorry. This patch > should not have gone into cvs. Should we back it out of CVS for a coming update? I presume the 3.0.3 boat has sailed already.

Not clear to me that more churn helps, especially if we're not sure what the 3.1 behaviour will finally look like. There are available workarounds in the form of eIS for extension script, and I don't think it'll matter by the time 3.0.4 has shipped, unless people stay on 3.0.1. That would be very unfortunate, since there are some important security fixes in 3.0.2. (3.0.3 is indeed not an available vehicle.)

Alright, this is totally my fault. I think this should be backed out (in CVS and m-c) until I have a better understanding of what extension authors need here. The "bug" being fixed isn't a huge one in any case.

What's the current status here?

The "restore utility of eval()" patch has landed on tracemonkey, so this test is no longer valid.

The "verified" on this bug makes no sense on 1.9.1 right now (won't make sense when tracemonkey changes land there). But the test will still valid for 1.9.0.x, doesn't it?

caio, the test will be excluded on 1.9.1. i suppose we could retarget this bug to just 1.9.0. i'll leave that call to crowder.