Adblock Plus and (a little) more
Five wrong reasons to use eval() in an extension · 2009-02-06 13:35 by Wladimir Palant
One of the most overused JavaScript features is the eval() function. I have seen it used in very many extensions but only a few actually had a good reason to use it. So I want to go through all the wrong reasons one would use eval().
1. Processing JSON data
The JSON format got popular for storing data lately. Its most convenient feature: it is so easy to parse! I mean, all you have to do is to write data = eval(json) and you are done.
What’s the catch? Right, json variable might contain something like {foo: "bar" + alert(Components.classes)} and then you will end up running JavaScript code, something that you didn’t intend. So this way of parsing JSON data is absolutely unsuitable for data coming from untrusted sources. And guess what: if you are a Firefox extension, data coming from any web server is untrusted. Even if it is “your” web server, it might be hacked or the data might have been manipulated on its way to the user (especially for unencrypted connections), and you really don’t want to put users at risk when something like that happens.
But there is more: even the data that your extension wrote itself (e.g. extension state saved on browser shutdown) cannot be always considered trusted. Often it will write out data that it received from the web in one way or other. If there is a bug in the way JSON is written and that data breaks out of JavaScript strings you will unintentionally run JavaScript code when you “parse” that JSON. This means that it is always better to use methods dedicated to processing JSON that will no longer run JavaScript when receiving invalid data.
2. Using object properties when property name is determined dynamically
What if your code needs to access obj.fooN where “N” is the value of the variable n? Meaning that the name of the property you have to access is not known in advance but has to be determined dynamically. Extensions will sometimes do things like eval("obj.foo" + n) to solve this problem. Here the extension would need to verify that the value of n cannot contain anything malicious — but how?
Fortunately, that question doesn’t need to be answered. There is a better way, one only has to remember that all objects in JavaScript are associative arrays. In other words, obj.foo and obj["foo"] are exactly the same thing, each property is at the same type an array member. So to solve the problem above you simply need to write obj["foo" + n] and that operation will always access a property, never do anything else.
But what about methods? Methods in JavaScript are properties as well, only difference is that their value is a function. You can use the method Function.call() to call that function with the correct value of the this pointer:
var method = obj["foo" + n];
method.call(obj, param1, param2);
Or in a more compressed way:
obj["foo" + n](param1, param2);
Same approach can even be applied to global variables and functions. Those are all properties of the “global object” which can usually be referenced by the name window. So window.foo or window["foo"] will give you the value of the global variable foo.
3. Telling functions what they should do when they are done
One pattern that I would occasionally see is calling a function like this:
foo("window.close()");
On other occasions the same function would be called with different JavaScript code as parameter. And when the function is done it would call eval() on its parameter to run the specified action.
Obviously, there are no security issues here, so what’s wrong with that approach? Actually, several things:
- This code will not be compiled until
eval()is called. That means that while the JavaScript interpreter will report syntax errors for the rest of the code immediately when the script is loaded, syntax errors in the function parameter will be reported late and might go unnoticed simply because you never tested the code path that will run that code. - Another consequence is that for errors in that code the JavaScript interpreter cannot report the correct source file and line number, it simply doesn’t know where that code came from. Debugging such errors is no fun.
- Finally, passing parameters between
foo()and the executed code is non-trivial and requires ugly workarounds.
Fortunately, all these problems go away if you use closures. Here is a rewritten an slightly extended version of the code above:
foo(function(error)
{
alert(error);
window.close();
});
And the function foo() would look like this:
function foo(callback)
{
...
callback("Full success");
}
4. Triggering inline event handlers in HTML or XUL
Let’s assume we have a button like this:
<button id="button" oncommand="doSomething();"/>
Then why not do eval(document.getElementById("button").getAttribute("oncommand")) to trigger that event handler? Typically, extensions will do this to trigger event handlers on elements that aren’t their own. However, generating a “command” event is much easier and will work regardless of how the event handler is defined:
document.getElementById("button").doCommand();
The method doCommand() is available for all XUL elements. As to other events, it is better to generate a real event object using document.createEvent() — because the event handler might expect one. For example:
var event = document.createEvent("MouseEvents");
event.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
document.getElementById("button").dispatchEvent();
But what if you defined your own custom attribute “onfooaction” that isn’t associated with any real event? Even in that situation using eval() isn’t the best choice because then the code will execute in the context of the function calling eval(). So if the event handler is using the global variable foo but your function calling that event handler has a local variable foo — the event handler will inadvertently access the local variable. And of course you cannot pass parameters to the event handler then. A better solution would be creating a function for the event handler:
var handler = new Function("param1", "param2", document.getElementById("button").getAttribute("onfooaction"));
handler("foo", "bar");
In this scenario the event handler will get “foo” as parameter param1 and “bar” as parameter param2 (this is how the usual inline event handlers get the event parameter).
5. Rewriting browser’s functions
Occasionally I see code that goes like this:
gBrowser.foo = eval(gBrowser.foo.toString().replace("foo", "bar"));
I recommend a public spanking for everybody who is rewriting browser’s functions like this. It is only slightly better than the extensions that simply replace browser’s functions by their own. The assumption in both cases is that the code of the function being rewritten never changes — but what if it does? In the best-case scenario the extension will simply stop working, not much damage done. But it could just as easily break the browser. Or, if the browser function changed to fix a security issue, the extension could reintroduce that issue.
In other words — don’t do this. In most cases the idea is not to change the way the browser function works but to insert additional code before/after it runs. Fortunately, there is a less dangerous way to achieve just this, you simply wrap the original function in your own, using closures again:
var origFoo = gBrowser.foo;
gBrowser.foo = function(param1, param2)
{
if (param1 == "top secret")
doSomethingBeforeFoo();
var result = origFoo.apply(this, arguments);
if (result == null)
doSomethingAfterFoo();
return result;
}
Note how Function.apply() is used to call the original function will all parameters you got. Even if that function receives only two parameters now this might change in a future browser version. Your extension might now know what to do with the new parameters but it should still pass them to the original function to avoid breaking it.
What about the valid uses of eval()?
I don’t think there are all that many valid uses for the eval() function. Some extensions allow users to enter JavaScript code that will be evaluated. Here eval() is justified though it might still be better create a function using Function() constructor and pass any variables that script might need explicitly as function parameters.
Another possible use of eval() is declaring constants conditionally:
if (typeof MY_CONSTANT == "undefined")
eval("const MY_CONSTANT = 'foo'");
That way if another script decides to declare the same constant you won’t get a syntax error. I still consider this approach a hack however: if you are afraid of clashing with unknown scripts running in same namespace you should make sure that your constants (as well as global variables) have unique names that other scripts won’t use. And as to your own scripts, making sure that the script containing constant declarations isn’t loaded more than once shouldn’t be too hard.
Finally, there are those obfuscated or “packed” scripts that make heavy use of eval() to generate their code at runtime. While I see the value of “compressing” scripts on the web, doing the same in extensions makes very little sense. Extensions are downloaded only once so saving two seconds of download time won’t help anybody. On the other hand, the “packed” script will cause delays every time it needs to be loaded which might be pretty often.
Comment [14]
Commenting is closed for this article.
web tasarımı · 2009-02-07 18:36 · #
thank you for infos..
Best regards.
Simon · 2009-02-07 18:53 · #
As to point #5: How would you propose to adjust code inside a function (i.e. when you can’t just pre- or append code but would have to rewrite the whole function otherwise)? And once one extension does what you propose, it’ll prevent all others from adjusting bits in the middle, forcing them to rewrite the whole code path and thus no longer allowing for extensions to peacefully coexist.
I’ve originally been proposing what you do, but been bitten by incompatibilites several times which could only be fixed by using eval. I’ll gladly learn a better way, but I’m afraid that yours doesn’t seem to be it…
As to point #4: How is |new Function(“some code”)()| safer than |eval(“some code”)|? Disallowing eval as you suggest in bug 477380 won’t help if you don’t disallow new Function as well.
Reply from Wladimir Palant:
Changes in the middle of a function you didn’t write are generally not a good idea. This is bound to end badly, no matter how you do it. If you cannot achieve what you need by other means (Object.watch() for example) then maybe you shouldn’t be doing it.
As to “new Function()” – it isn’t inherently safer than eval() but it is typically used for static code and doesn’t introduce issues. This is just about better coding practices that will make it somewhat harder to shoot yourself in the foot.
Mook · 2009-02-08 01:09 · #
Are there equivalents for unprivileged web pages? Gecko 1.9.1 will have the global JSON object, but is there an evalInSandbox equivalent?
Asking for UniversalXPConnect to use evalInSandbox feels like it.. kinda defeats the point…
Reply from Wladimir Palant:
I guess you are talking about XSS prevention – a sandbox for scripts isn’t sufficient then. That’s what http://bugzilla.mozilla.org/show_bug.cgi?id=341604 (and http://www.whatwg.org/specs/web-apps/current-work/#attr-iframe-sandbox) is about.
Minh Nguyễn · 2009-02-08 03:13 · #
I reluctantly added a call to eval() in my extension recently, because of an annoying corner case. The extension operates entirely within an onKeyPress event handler. Under some circumstances, the handler must call event.preventDefault() to keep the entered key from appearing in the textbox. But that also prevents the textbox’s oninput attribute from firing. As far as I can tell, there’s no way to fire that event without generating a new onKeyPress event (causing a feedback loop of sorts).
Minh Nguyễn · 2009-02-08 03:52 · #
Never mind. I solved the issue by generating an “input” event (of type “Events”) instead.
Reply from Wladimir Palant:
Glad to hear that. Yes, if you generated input to a text box without user interaction you should generate an “input” event as well.
Dorando · 2009-02-08 20:01 · #
Lets assume the original gBrowser.foo in point 5 is something like (similar code can easily be found throughout the Mozilla codebase, look for .isTrusted or permitUnload in tabbrowser.xml for examples):
gBrowser.foo = function(param1, param2)
{
if(!gBrowser.isItSaveToDoSomethingBeforeFoo())
return;
if (param1 == “do_nothing”)
return;
var color = “red”;
doSomething(color, param2.length);
if (param2 == “some_extensions_want_to_prevent_this”)
doSomethingAnnoying(“dark”+color);
}
a) How to ensure to only doSomethingAfterFoo if the function did doSomething?
b) How to ensure to only doSomethingBeforeFoo if the function will doSomething?
Duplicating some of the original code would work, but that could as well reintroduce a security issue (and has in the past) and somewhat defeats the purpose of not simply rewriting the function. Keeping references to something the function changes and compare it afterward might work in some situations but that seems kind of an overkill IMHO (and might slowdown the code quite a bit).
c) How to prevent gBrowser.foo to doSomethingAnnoying?
d) How to change gBrowser.foo to use green as color?
Replacing additional functions and checking the caller from there is an option but IMHO that only increases the likelihood of breaking something. Letting the function do its work and reverting it afterward is also possible but it doesn’t seem like that a good user experience.
The assumption in both cases is that the code of the function being rewritten never changes — but what if it does?
That’s what maxVersion is for.
Or, if the browser function changed to fix a security issue, the extension could reintroduce that issue.
As could replacing the function if isItSaveToDoSomethingBeforeFoo was introduced later and doSomethingBeforeFoo(); contains something similar to the patched in code.
On the other hand enables eval patching the extension author to patch bugs (including security related) before they are fixed in the original code. If correctly written, the eval patch would fail gracefully once the original code fixes the bug since that code portion would be gone.
Changes in the middle of a function you didn’t write are generally not a good idea.
Sure, as it is always the case by changing the built-in behavior of anything, but sometimes that is the only option an extension author has to achieve something as built-in interfaces/functions for this are missing.
Just to repeat what Simon already pointed out, replacing instead of patching the function would prevent this completely for extensions that need it, so please don’t advocate that unless you can provide an alternative.
This is bound to end badly, no matter how you do it.
So far I’ve seen more code break due other changes (including API) than I’ve seen break due to eval patching, despite the fact that the most minimal changes possible could break it.
Reply from Wladimir Palant:
a) You have to extend both gBrowser.foo and window.doSomething. When gBrowser.foo is entered you set a flag variable to false, when window.doSomething is entered you change it to true. doSomethingAfterFoo is only called if that flag variable is true.
b) I don’t think you want a clairvoyance module – you rather want to call doSomethingBeforeFoo when gBrowser.foo calls window.doSomething (but not if it is called from somewhere else). So again you extend both functions. When gBrowser.foo is entered you set a flag variable to true, when it is exited you set it to false. When window.doSomething is entered and the flag variable is true you run doSomethingBeforeFoo.
c) By extending gBrowser.foo in the same way as in b) and then extending doSomethingAnnoying to return if the flag variable is true. But think twice whether you really want to do it and whether you will break something by doing that.
d) Again extend gBrowser.foo in the same way as in b) and extend doSomething – if flag variable is set and arguments[0] is “red”, change arguments[0] to “green”. Here again, please think twice before doing that.
About maxVersion: extensions are typically compatible with 3.0.*, so this won’t catch security fixes that happen in minor releases.
“On the other hand enables eval patching the extension author to patch bugs (including security related) before they are fixed in the original code” – patching bugs this way is a really bad idea. File a bug in Bugzilla, provide a patch. If it is really an obvious bug and safe to fix that fix will be distributed with a minor release. And if you are doing something wrong you will get feedback – better than “patching” without asking anybody and breaking browser functionality.
Dorando · 2009-02-09 13:59 · #
doSomething (or the other functions) might be Components.classes[”“].createInstance().doSomething();
or a code block
or a function so often used that the additional processing time the replaced function needs for something that minimal is not excusable (especially if multiple extensions do that).
About maxVersion: extensions are typically compatible with 3.0.*, so this won’t catch security fixes that happen in minor releases.
Claiming to know the future ‘is bound to end badly’, so that would be the fault of the author actually. Also Mozilla security updates have changed something in a breaking way far too often anyway IMHO (the latest one I know of was bug 442333 ).
patching bugs this way is a really bad idea. File a bug in Bugzilla, provide a patch.
Might already be filed or even be fixed for the next major version, one could use the same code.
Some bugs might take months to actually be patched (even with patches) especially if only external code fails due to it.
If it is really an obvious bug and safe to fix that fix will be distributed with a minor release.
Which might be a month away assuming it actually will be included within the next minor release, backporting it properly wouldn’t harm.
[…] breaking browser functionality
If the intention of the extension was change what happens normally in the given situation then it likely wanted to break the default functionality.
Simon · 2009-02-09 14:42 · #
BTW: Another valid use of eval is dynamically injecting code into a different context through either
eval(“code to inject”, context);
or
domWindow.eval(“code to inject”);
which could only be substituted by writing the code to a file, using the subscript loader and then removing that file again because the subscript loader already disallows data: URIs (this work-around is quite a PITA, though, because it forces you to handle temporary files which would allow for different things to go subtly wrong).
Reply from Wladimir Palant:
“Different context” means “unprivileged context”, right? That’s a different thing, that code wouldn’t gain chrome privileges.
Simon · 2009-02-09 18:02 · #
“Different context” means “unprivileged context”, right?
Not necessarily. domWindow was meant to be what the window mediator returns and the context could just as well be an Object with chrome privileges (into which you load scripts with the subscript loader and then dynamically use those scripts without polluting the window object, e.g. inside an XBL binding).
To see the first effect, try eval’ing the following line in the Error Console:
Components.classes[”@mozilla.org/appshell/window-mediator;1”].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow(“navigator:browser”).eval(“location”);
phin · 2009-02-17 04:48 · #
thanks for this article #5 is extraordinarily helpful.
I had no idea how to do that, and tried a bunch of ways before resorting to plain reassignment (and “override“s in chrome.manifest) – it wasn’t pretty. You explained it so simply, and helped out alot.
phin · 2009-02-17 10:01 · #
you know, it worked great with gBrowser, but when I tried to do a closure of “getImageSrc” from treeView.js, it wouldn’t work.
Reply from Wladimir Palant:
Why not? PlacesTreeView.prototype.getImageSrc = …
phin · 2009-02-17 22:01 · #
For some reason, even that doesn’t seem to change it. I even tried reassigning it to null or alert(“hello”), still nothing. The only way that worked was to override the “load” method in tree.xml, let the PlacesTreeView object be created in that method, then reassign getImageSrc right there.
Maybe it’s being overwritten with GetImageSrc from nsTreeContentView.cpp or nsXULTreeBuilder.cpp. Thanks for replying! Basically, I’m trying to replace the icons for all the nodes with the return value of my own function.
Piro / SHIMODA Hiroshi · 2009-11-28 10:23 · #
This is very interesting discussion. I translated to Japanese, the article and some commends.
http://piro.sakura.ne.jp/latest/blosxom/mozilla/xul/2009-11-28_five-wrong-reasons-to-use-eval-in-an-extension.htm
Piro / SHIMODA Hiroshi · 2010-02-08 14:20 · #
I’ve published an objection to this topic.
http://piro.sakura.ne.jp/latest/blosxom/mozilla/xul/2010-02-08_eval-en.htm