Five wrong reasons to use eval() in an extension

http://adblockplus.org/blog/five-wrong-reasons-to-use-eval-in-an-extension

 

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.