Is eval() always evil?

Shawn Dowler
Tera Guru

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);
}
1 ACCEPTED SOLUTION

andrew_venables
ServiceNow Employee
ServiceNow Employee

eval can be very evil in traditional web development but since the author of scripts in ServiceNow has access to write background code anyway i thinks its risk is limited.



There are a few situations where eval is necessary - but this isn't one of them. Try the below script instead (untested):



var answer = false;


if (gs.hasRole('user_admin')) {


        answer = true;


} else {


        var grp = current.group;


        do {


                  if (grp.manager == gs.getUserID()) {


                            answer = true;


                  }


                  grp = grp.parent;


        } while (!grp.nil());


}


View solution in original post

8 REPLIES 8

marcguy
ServiceNow Employee
ServiceNow Employee

I believe eval can be evil when it's allowed to run client side (because something could be injected or tampered with), but server side is not too much of a problem (because the code can't be tampered with as easily by end user), that's what I understand anyway.


andrew_venables
ServiceNow Employee
ServiceNow Employee

eval can be very evil in traditional web development but since the author of scripts in ServiceNow has access to write background code anyway i thinks its risk is limited.



There are a few situations where eval is necessary - but this isn't one of them. Try the below script instead (untested):



var answer = false;


if (gs.hasRole('user_admin')) {


        answer = true;


} else {


        var grp = current.group;


        do {


                  if (grp.manager == gs.getUserID()) {


                            answer = true;


                  }


                  grp = grp.parent;


        } while (!grp.nil());


}


I didn't even think of it like that. Wow, talk about overcomplicating things. This will come in handy elsewhere, I'm sure. Here is what the tested version looks like:



var answer = false; //Restrict access by default


if (gs.hasRole('user_admin')) {


      answer = true; //Allow access if user has 'user_admin'


} else {


      var grp = current.group;


      do {


              if (grp.manager == gs.getUserID()) {


                      answer = true; //Allow access if the user is manager of the group or one of group's parents


              }


              grp = grp.parent;


      }


      while (!grp.nil() && answer == false);


}


justin_drysdale
Mega Guru

Bonus points for the do-while loop. It is not used enough IMO.