bcoe / sandcastle

A simple and powerful sandbox for running untrusted JavaScript.
MIT License
222 stars 48 forks source link

Supporting shared state between script execution needs some work #49

Open bcoe opened 9 years ago

bcoe commented 9 years ago

Running multiple functions with shared state does not work as one would expect, given the documentation that was in the README.

As all functions belong to the same script you can pass objects to the same API instance and receive them later.

State API:

exports.api = {
  _state: {},

  getState: function () {
    return _state;
  },

  setState: function (state) {
    _state = state;
  }
};

A Script Using the API:

var script = sandcastle.createScript("\
  exports.main = {\
    foo: function() {\
      setState('foo', true);\
      exit('Hello Foo!');\
    },\
    bar: function() {\
      setState('bar', true);\
      exit('Hello Bar!');\
    }\,
    hello: function() {\
      setState('hello', true);\
      exit('Hey ' + name + ' Hello World!');\
    }\,
    getStates: function() {\
      return {\
        foo: getState('foo'),\
        bar: getState('bar'),\
        hello: getState('hello')\
      };\
    }\
  };\
");

script.run('main.getStates'); // { foo: undefined, bar: undefined, hello: undefined }
script.run('main.foo');
script.run('main.bar');
script.run('main.hello', {name: 'Ben'});
script.run('main.getStates'); // { foo: true, bar: true, hello: true }

Problem 1

A script has a this.exited flag which is set when a script exists, therefore if we run:

script.run('main.getStates'); // { foo: undefined, bar: undefined, hello: undefined }
script.run('main.foo');
script.run('main.bar');
script.run('main.hello', {name: 'Ben'});
script.run('main.getStates'); // { foo: true, bar: true, hello: true }

Only one method will complete execution.

Problem 2

A new contextObject is created as each script is executed:

Sandbox.prototype.executeScript = function(connection, data) {
  var contextObject = {
      runTask: function (taskName, options) {
        options = options || {};

        try {
          connection.write(JSON.stringify({
            task: taskName,
            options: options
          }) + '\u0000'); // task seperator
        } catch(e) {
          this._sendError(connection, e, false);
        }
      },
      exit: function(output) {
        try {
          connection.write(JSON.stringify(output) + '\u0000\u0000'); // exit/start separator
        } catch(e) {
          _this._sendError(connection, e, true);
        }
      }
    };
}

This makes sharing data between methods in scripts difficult.

Thoughts

I really like the work that @joernroeder has done on this feature, and I think we're 99% of the way there. It would be great if we could: