NeilFraser / JS-Interpreter

A sandboxed JavaScript interpreter in JavaScript.
Apache License 2.0
1.98k stars 353 forks source link

Cannot define a property with a getter on the globalObject #192

Closed atifsyedali closed 3 years ago

atifsyedali commented 4 years ago

First, thank you for this awesome project.

I am trying to set a property with a getter on the global object so that I can lazily retrieve the values from the host. However, I keep hitting the following error regardless of how I try (also see tests below).

You can see the first and last test works, but is not really what I want, since I want to lazily fetch the values from the host using createAsyncFunction.

TypeError: Primitive data type has no properties

    at Interpreter../original-repo/interpreter.js.Interpreter.hasProperty (/redacted/node_modules/js-interpreter/lib/js-interpreter.js:4347:11)
    at Interpreter../original-repo/interpreter.js.Interpreter.stepIdentifier (/redacted/node_modules/js-interpreter/lib/js-interpreter.js:5633:18)
    at Interpreter../original-repo/interpreter.js.Interpreter.step (/redacted/node_modules/js-interpreter/lib/js-interpreter.js:2362:48)
    at Interpreter../original-repo/interpreter.js.Interpreter.run (/redacted/node_modules/js-interpreter/lib/js-interpreter.js:2392:32)
    at Object.<anonymous> (/redacted/js-executor/__tests__/JsExecutorTests.spec.ts:87:17)
    at /redacted/node_modules/jest-jasmine2/build/queueRunner.js:45:12
    at new Promise (<anonymous>)
    at mapper (/redacted/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
    at /redacted/node_modules/jest-jasmine2/build/queueRunner.js:75:41

import JSInterpreter from "js-interpreter";

describe("JsExecutorTests", () => {
  // Works
  it("should attach a string-valued property to globalObject", (done) => {
    const nameValue = "world";
    const initFunction = function (interpreter, globalObject) {
      interpreter.setProperty(globalObject, "name", nameValue);
    };

    const code = `"Hello " + name;`;

    const interpreter = new JSInterpreter(code, initFunction);
    interpreter.run();

    setTimeout(() => {
      expect(interpreter.value).toEqual("Hello " + nameValue);
      done();
    }, 1000);
  });

  // Doesn't work: `Primitive data type has no properties` in Interpreter.prototype.hasProperty
  it("should attach a property with getter to globalObject", (done) => {
    const nameValue = "world";
    const initFunction = function (interpreter, globalObject) {
      const wrapper = function getName() {
        return nameValue;
      };

      interpreter.setProperty(globalObject, "name", JSInterpreter.VALUE_IN_DESCRIPTOR, {
        get: interpreter.createNativeFunction(wrapper),
      });
    };

    const code = `"Hello " + name;`;

    const interpreter = new JSInterpreter(code, initFunction);
    interpreter.run();

    setTimeout(() => {
      expect(interpreter.value).toEqual("Hello " + nameValue);
      done();
    }, 1000);
  });

  // Doesn't work: `Primitive data type has no properties` in Interpreter.prototype.hasProperty
  it("should attach property with an async-getter to globalObject", (done) => {
    const nameValue = "world";
    const initFunction = function (interpreter, globalObject) {
      const wrapper = function getName(callback) {
        setTimeout(() => {
          callback(nameValue);
          interpreter.run();
        }, 0);
      };

      interpreter.setProperty(globalObject, "name", JSInterpreter.VALUE_IN_DESCRIPTOR, {
        get: interpreter.createAsyncFunction(wrapper),
      });
    };

    const code = `"Hello " + name;`;

    const interpreter = new JSInterpreter(code, initFunction);
    interpreter.run();

    setTimeout(() => {
      expect(interpreter.value).toEqual("Hello " + nameValue);
      done();
    }, 1000);
  });

  // Doesn't work: `Primitive data type has no properties` in Interpreter.prototype.hasProperty
  it("HACKED: should attach property with an async-getter to globalObject in user-defined code", (done) => {
    const nameValue = "world";
    const initFunction = function (interpreter, globalObject) {
      const wrapper = function getName(callback) {
        setTimeout(() => {
          callback(nameValue);
          interpreter.run();
        }, 0);
      };

      interpreter.setProperty(globalObject, "getName", interpreter.createAsyncFunction(wrapper));
      interpreter.properties;
    };

    let code = `"Hello " + name;`;

    // before run user code, we inject it with initialization stuff.
    code =
      `
    Object.defineProperty(this, "name", {
      get: function() { return getName(); },
    });
    ` + code;

    const interpreter = new JSInterpreter(code, initFunction);
    interpreter.run();

    setTimeout(() => {
      expect(interpreter.value).toEqual("Hello " + nameValue);
      done();
    }, 1000);
  });

  // works, but not really what is required here because the value retrieval is not lazy.
  it("HACKED: should attach value-property to globalObject in user-defined code", (done) => {
    const nameValue = "world";
    const initFunction = function (interpreter, globalObject) {
      const wrapper = function getName(callback) {
        setTimeout(() => {
          callback(nameValue);
          interpreter.run();
        }, 0);
      };

      interpreter.setProperty(globalObject, "getName", interpreter.createAsyncFunction(wrapper));
      interpreter.properties;
    };

    let code = `
    "Hello " + name;
    `;

    // before run user code, we inject it with initialization stuff.
    code =
      `
    window.name = getName();
    ` + code;

    const interpreter = new JSInterpreter(code, initFunction);
    interpreter.run();

    setTimeout(() => {
      expect(interpreter.value).toEqual("Hello " + nameValue);
      done();
    }, 1000);
  });
});
atifsyedali commented 4 years ago

The above tests pass if I change the following code for stepIdentifier method:

    while (!this.hasProperty(scope, node['name'])) {
      scope = scope.parentScope;
    }

to this

    while (scope !== this.globalScope && !this.hasProperty(scope, node['name'])) {
      scope = scope.parentScope;
    }

Security-wise, I have no idea what implications that will have as I am very familiar with the codebase right now.

cpcallen commented 4 years ago

Can you provide definitions for describe and it, please? Or, better still, a smaller reproduction not using them at all? It's pretty much impossible to know what you might be expecting this code to do.

atifsyedali commented 4 years ago

Smallest reproducible example...put the following code in the demo textbox on index.html and click Run, and notice console:

Object.defineProperty(window, "hello", {
  get: function() { return "world"; }
});

alert(hello);
NeilFraser commented 3 years ago

Thanks for the report. Fixed.