Best way for loading code logic (AI) from external file (json)

[Ezelia]

Hi all,

 

I'm working on a HTML RPG project and since now, I was writing my AI code directly inside NPCs classes.

 

but to make team work and game maintenance easier, I'm rolling a set of tools to create quests, NPCs ...etc those tools generate json file with configuration data.

 

now I want to make it possible to add custom behaviour to NPCs via custom script ... I have a solution using eval.

//npcJSONData contains NPC data from external server side file.//npc is the current NPC we are creatingvar npc = new NPC(/* initialisation stuff */)if (npcJSONData.customScript !== undefined){(function() {   //isolate eval from main context    var bhv = undefined;    eval ("var bhv = function (me) {" + npcJSONData.customScript+ "return true;};");    if (bhv !== undefined) npc.setCustomBehaviour(bhv);})()}

I don't like using eval() but I didn't find any alternative and efficient way ...

 

is this code safe ? is there any preferable way to do it ?

 

 

[Chris]

Why don't you load your scripts with a module system like requireJS? I use it for all my code in every application I write.

[dhaber]

(function() {   //isolate eval from main context    var bhv = undefined;    eval ("var bhv = function (me) {" + npcJSONData.customScript+ "return true;};");    if (bhv !== undefined) npc.setCustomBehaviour(bhv);})() ;

I'm not 100% certain about any of this, but I believe that eval() runs the code in the current context, which in this case would be the context of the self-executing anonymous function.   Nothing prevents the eval'ed code from reaching out and modifying the window, or anything of that sort.  I don't think the outer anonymous function offers much protection.   Anything from a parent context will be accessible, unless you take steps to block it.

 

Since the eval() occurs in the function's context, your second "var bhv" is redundant.   Kind of like this:

var x;var x = 2;

The second "var" has no effect, since x (or bhv) is already defined within that context.

 

I'm a little confused about what you have in the eval.   You are creating a function and setting bhv to the anonymous function.   You then check if bhv is not undefined, but how could it not be since you just set it to an anonymous function in the prior line?  I guess the only time that could happen is if there was a syntax error in your eval'ed code?  Even then, wouldn't that probably break execution and keep it from getting to the conditional?   If that is to catch errors, you may be better off using try/catch around the eval so that errors in the customScript can be captured safely.

 

Is your customScript really legal JSON data?   If so, then why not use JSON.parse()?   This works so long as you don't need to support old browsers like IE7.  Here is the support table:

 

     http://caniuse.com/json

 

From your code, I'm guessing it isn't really legal JSON?   Even with this direction, I still don't think eval() is ideal.   Instead, code can be written in files that are loaded through normal JS means.   This could mean writing your own loader, such as dynamically creating script tags, or using one of the many options out there, like requireJS as was suggested.   Any of these ways would have many advantages over using eval().  Eval is slow, dangerous from a security perspective, and difficult to debug, so it is best avoided.

[Ezelia]

thank you for your replies

to make things clear here is an example (very simple) JSON Data file :

{  "npcId" : "Little cat",  "npcImg" : "path/to/some/image.png",  "npcArea" : {"x":10, "y":10, "w":3, "h":3},  "customScript" : "var life = me.getLife(); if (life > 10) me.move(); else me.stay() "}

as you can see, I want to keep the whole NPC configuration including the custom script inside one single json file.
I can't use dynamic loading à la requireJS since my script is part of the NPC configuration.

the vulnerability I identify here is : if someone gain access to the server and change my NPCs datafiles contents, but then I'll have more serious problem than eval ... and event dynamic loading will not prevent such attack ...

is there any other senario when a hacker can exploit my eval() ?

 

 

@dhaber the redundunt bhv var is just to avoid a TypeScript warning since he don't take in consideration the evaled code
the check for undefined variable is because I do some other stuff in my code witch may result in an undefined variable what I posted here is a simplified version. and I dont use try catch cause when it fails it just crash the anonymous function and that's not really a problem.


 

[Chris]

Why does it have to be a JSON file? Make it a JS file and you don't have to eval() anything...

[Ezelia]

the usage of json is a requirement for interoperability but also to be able to compress data (once in production) and uncompress it on the fly before parsing.

even if I forget about compression and other constraints, is there a real benefit from using dynamic loading instead of eval ? (requireJS or other dynamic loader will just create a <script> block dynamically).

[dhaber]

Seeing your JSON, things are making more sense now.  Some of my assumptions above were wrong.

 

From a style perspective, what you did there doesn't seem right to me.    JSON is for data, and js files are for code.    You've put code into JSON, and while it could work, it seems out of place.  There are lots of downsides.   You can't format the code in there.   Large amounts of code will be near impossible to maintain.   Editors will see that as a string and not as code and won't be as helpful with syntax highlighting, completion, etc.

 

On top of that there are the flaws of eval.   It is slow and can make debugging tricky.

 

the vulnerability I identify here is : if someone gain access to the server and change my NPCs datafiles contents, but then I'll have more serious problem than eval ... and event dynamic loading will not prevent such attack ...

 

 

The security implications exist, but I think you are guessing correctly that security probably won't be an issue.   It only becomes an issue if someone is able to figure out a way to alter the what makes it into the eval on a remote client.   It is one more layer of complexity, and so one more risk, but if people can never alter what is eval'ed, then things should be fine.

 

even if I forget about compression and other constraints, is there a real benefit from using dynamic loading instead of eval ? 

 

 

When it comes down to it, if you aren't bothered by the performance, maintainability, potential security implications, or bad aesthetics, then what you are doing could work.   While it could hurt performance a little more, you may want to consider doing a try/catch around the eval so that you can catch and log errors in a clean way.

 

Personally, I would find a way of restructuring that so that JSON contains only data, and no code.   For example, you could have an AI class that the JSON contains the behavior for as settings rather than as code.    Keys could define various behavior modes, thresholds and limits, etc.  This would change the style a bit, but it would also make the AI centrally maintained, reusable, and easier to update in bulk.

 

Your initial question was whether there is a preferable way over eval, and I think most people would agree that there is.  In the end though, whatever works well enough for your usage may be alright.   The insides of finished games often aren't perfect and beautiful, and the player is always oblivious so long as it works.    The biggest downside I see is maintainability, but if you are working alone or on a small team, it may be something you can choose to live with.

[Chris]

the usage of json is a requirement for interoperability but also to be able to compress data (once in production) and uncompress it on the fly before parsing.

even if I forget about compression and other constraints, is there a real benefit from using dynamic loading instead of eval ? (requireJS or other dynamic loader will just create a <script> block dynamically).

 

Look, I will translate your JSON object into a JS file with requireJS:

define({  "npcId" : "Little cat",  "npcImg" : "path/to/some/image.png",  "npcArea" : {  	"x":10,  	"y":10,  	"w":3,  	"h":3  },  "customScript" : function(){  	var life = me.getLife();  	if (life > 10){  		me.move();  	}  	else {  		me.stay();  	}  	  }});

Isn't that much more readable? And you don't have to dabble with the eval() and what not all.

 

So how do you get that stuff to use it? Its easy. Lets assume you saved it in the file "someExtension.js":

require(['someExtension'], function(ext){	ext.customScript();});

About the compression: You just don't have to care. Your webserver is automatically compressing the output of the requested JS files with GZIP, so everything will be as small as possible

[Ezelia]

thank you Chriss for the proposition, this seems good for me except for compression constraint : it's not only for server, the engine is also supposed to work standalone, so on mobiles it make sens to have things compressed).

btw : the file format I give is not supposed to be edited on an external editor. there is a tool that generate everything and have a code editor with formatting and syntax highlight ...etc
this editor saves file in an intermediate format . then I "compile" everything to an optimised json format (without meta-data ...etc) ... so readability is not an issue.

 

[Chris]

Well, you could still concatenate all resources into a JS object definition instead of a JSON string. After your JSON data or the JS data is loaded and/or eval'd into the mobile browsers memory, compression is just beneath your possibilities. Either the browser handles it good, or not. But I dare to say that the JS will leave a smaller memory footprint.

[soybean]

This is an interesting topic. I might try toying around with Require.js too. Thanks all for sharing.

 

A question: does the system implement some sort of plugin API? because I wonder how to make sure AI submitted by other users are safe and not calling unwanted methods. 

[Ezelia]

@soybean : my first aproach was to implement an intermediate interpreter that will translate the produced script to JS, but this may take lot of time. my current aproach is to use Javascript, later I'll add some regex cheking to make sure only allowed functions was used.

 

@dhaber btw, I forgot to say that I already implemented some common AI behaviour, the script thing is only for cases that need special/custom behaviour

[dreta]

he vulnerability I identify here is : if someone gain access to the server and change my NPCs datafiles contents, but then I'll have more serious problem than eval ... and event dynamic loading will not prevent such attack ...

 

 

If somebody gains access to your server, they can do what ever they want to your users, whether you use eval() or not.

 

eval() gets bad reputation because it's used incorrectly. If you want to download a piece of JavaScript code from your server and then run it, then it's alright to use eval(), that's what it's for. The performance implications of eval() versus attaching a <script> may exist, but that's going to be VM dependent, and it certainly won't come into play unless your scripts are huge, on top of that, you're only calling eval() once per script. I don't see the performance ever being an issue.

 

Alternatively, since you want to create a function out of a piece of JavaScript code, you could just use the Function constructor. Functions created with the constructor run in the window context, so if you had no or few global variables (by running all your code in an immediately executed function) then the scripts wouldn't have access to much of anything. Then you could just send the functions and objects available to the script as an argument.

[Ezelia]

I think I finaly found a good alternative, I didn't implemented it yet but here is the idea.

instead of compiling my NPC configuration to one json file including data and code, I'll compile only NPC data to json files, and replace the script code with a uniq id.

beside that, I'll export a related js file with a hashtable of scripts.
the js file can be lazy-loaded ...

that way I'll keep data and code separated.

now I need to decide wether if I export all my scripts inside one singe js file or I export a js file for each JSON


what do you think about this aproach ?