Is eval() always evil?

[Shawn Dowler]

I recently put extended a solution from Mark Stanger (Crossfuze) on his blog called http://www.servicenowguru.com/system-definition/group-managers-manage-group-members/. The difficulty I came across was that I needed to allow a manager of any parent group to edit the users in any child group below the one they were the manager for. This necessitated that I be able to go all the way up the hierarchy looking at:

current.group.manager

current.group.parent.manager

current.group.parent.parent.manager

current.group.parent.parent.parent.manager

current.group.parent.parent.parent.parent.manager …etc.

There were many ways I could have tackled that, but the most efficient way to me wound up being a loop that uses eval() with a String in a loop to keep adding ".parent" to itself and sticking it in between "current.group" and ".manager" to keep checking. I hate using eval(), but I kept it to only being used to check if something is true or not.

My question is: is this bad? Is there a better way that I'm missing? When is eval() not evil? Here's a sample of part of the solution (Check here for my full post with all the code😞

// sys_user_grmember Write and Delete ACLs 
var answer = false; //Restrict access by default
if (gs.hasRole('user_admin')) {
   answer = true; //Allow access if user has 'user_admin'
} else {
   var parentsStr = "";
   var hasParent = false;
   do {
   if (eval("current.group" + parentsStr + ".manager") == gs.getUserID()) {
   answer = true; //Allow access if the user is manager of the group or one of group's parents
   }
   parentsStr = parentsStr + ".parent";
   hasParent = JSUtil.notNil(eval("current.group" + parentsStr));
   }
   while (hasParent == true && answer == false);
}

[Shawn Dowler]

As usual, I started with way too much code and the more I worked on it, the shorter it got. While I was writing it initially I realized that I needed to execute the code once no matter what. If you feel like you have to copy the contents of the while loop and paste it above it, then you really want a do-while loop. I agree that they are under utilized.

[randrews]

in configuration there is a method used that does the same thing... and derned if i can remember how.. it seems that would be the perfect method to use for this type of situation..

[chris_tucker]

I just stumbled across this, so resurrecting a rather old thread.   Eval is, generally, evil, even on the server side.   Three big reasons to avoid it:

1. It's hard to reason about.   For example, what would you expect the following to print out?

eval('var x = 10'); 

gs.print(x);

2. It can open up unexpected security holes:

var prop = g_request.getParameter('sysparm_propertyName');

eval("return myObject." + prop);

A carefully constructed sysparm_propertyName here could execute arbitrary code (e.g. sysparm_propertyName=prototype%3Bgs.log(%27ouch%27))

3. It defeats a lot of optimizations/performance enhancements the JavaScript runtime could otherwise apply.   This is more complex, but relates to how JavaScript is able to identify and control access to variables and scopes, and to know something about who will have access to what and when.   The impact here varies significantly by JS runtime.

Specifically to the problem posted in the original request, there's an even more elegant way to do what you want than the proposed do/while solution (which itself is a dramatic improvement on the eval), using a simple recursion:

function allowAccess(group) {

     if (group.nil())

           return false;
     if (group.manager === gs.getUserID())

           return true;
     else

           return allowAccess(group.parent);
}

var answer = gs.hasRole('user_admin') || allowAccess(current.group);

These kinds of recursive approaches are often useful when you want to navigate through a hierarchy where each "node" in that hierarchy has the same structure.   They're arguably a little less efficient than a loop, but unless you have a hierarchy that is thousands of layers deep you'll never be able to notice the difference.

[Shawn Dowler]

I knew intuitively that this was a recursive problem, but I couldn't come up with the right way to syntactically make that happen. This is a more elegant solution, for sure. Thanks!