laverdet / isolated-vm

Secure & isolated JS environments for nodejs
ISC License
2.18k stars 154 forks source link

Cannot access to reference within another reference #488

Closed RugolettoRomuald closed 3 months ago

RugolettoRomuald commented 3 months ago

Hi ! First at all, thank-you for this library !

My real use case

For the purposes of my project, I've wrapped it into a VM class in which I authorize certain functions by using a bridge pattern. Everything works fine until I run into this problem:

  robotBridge.registerAsyncFunction('move', async (distance: number) => {
    robots[playerIdx].transform.moveForward(distance)
    const fnOnMoveEnd = await vm.get('onMoveEnd') // Get fn within fn doesn't work šŸ˜¢
    fnOnMoveEnd.fold(() => undefined, async fn => await fn.invoke())
  })

In the preceding code, I'd like the move function to call the onMoveEnd function only if my user has declared it in his robot's code. However, my get function timeout šŸ˜•

Simple tests to highlight the problem

So I wrote some tests that use IsolateVM outside my wrapper to highlight my problem, see below :

const IsolateHelper = (context: Context) => {
  const setAsyncFunction = async (
    fnName: string,
    fn: (...args: any[]) => Promise<any>
  ): Promise<void> => {
    await Promise.all([
      context.global.set(`__${fnName}`, new Reference(fn)),
      context.eval(`
        async function ${fnName}(...args) {
          return __${fnName}.applySyncPromise(undefined, [...args])
        }
      `)
    ])
  }
  return {
    setAsyncFunction
  }
}

it(`access reference fn then apply within other reference`, async () => { // Fail: cause timeout
  // Arrange
  const isolate = new Isolate()
  const context = await isolate.createContext()
  await context.global.set('increment', (value: number) => value += 1)
  await IsolateHelper(context).setAsyncFunction('test', async (value: number) => {
    console.log('beforeGet')
    const fn = await context.global.get('increment') // Timeout here
    console.log('beforeApply') // Not called
    return await fn.apply(undefined, [value]) as number
  })

  // Act
  const fn = await context.global.get('test', { reference: true })
  const result = await fn.apply(undefined, [21], { result: { promise: true } }) as number

  // Assert
  chai.assert.equal(result, 22)
})

it(`apply reference fn within other reference`, async () => { // Success
  // Arrange
  const isolate = new Isolate()
  const context = await isolate.createContext()
  await context.global.set('increment', (value: number) => value += 1)
  const fnIncrement = await context.global.get('increment')
  await IsolateHelper(context).setAsyncFunction('test', async (value: number) => (
    await fnIncrement.apply(undefined, [value]) as number
  ))

  // Act
  const fn = await context.global.get('test', { reference: true })
  const result = await fn.apply(undefined, [21], { result: { promise: true } }) as number

  // Assert
  chai.assert.equal(result, 22)
})

As you can see, if I get the reference to the increment function from the nodejs context, I can apply it. Otherwise, I'm stuck at the first step and can't even get the reference to the increment function.

Work around until a better solution

I can work around the problem by executing an intermediate function like below, but it seems strange to have to use this kind of strategy.

it(`workaround to move then calling onMoveEnd`, async () => {
  // Arrange
  let playerBotDistance = 0
  let nbOnMoveEndCall = 0
  const isolate = new Isolate()
  const context = await isolate.createContext()
  await context.global.set('onMoveEnd', () => nbOnMoveEndCall += 1)
  await context.global.set('__move', new Reference(async (distance: number) => {
    // Some async stuff
    playerBotDistance += distance
    return Promise.resolve()
  }))
  context.eval(`
    async function move(...args) {
      await __move.applySyncPromise(undefined, [...args])
      try {
        onMoveEnd()
      } catch (e) {
      }
    }
  `)

  // Act
  const fn = await context.global.get('move', { reference: true })
  await fn.apply(undefined, [21]) as number

  // Assert
  chai.assert.equal(playerBotDistance, 21)
  chai.assert.equal(nbOnMoveEndCall, 1)
})
RugolettoRomuald commented 3 months ago

here's the solution I finally came up with:

it(`user fire on move end`, async () => {
  // Arrange
  let playerBotDistance = 0
  let nbFireCall = 0
  const isolate = new Isolate()
  const context = await isolate.createContext()
  await context.global.set('global', context.global.derefInto()) // allows you to define function in 'context.eval'
  await context.global.set('fire', () => nbFireCall += 1)
  await context.global.set('__move', new Reference(async (distance: number) => {
    playerBotDistance += distance // updates common game elements
    await promiseUtils.waitDelay(1000) // waits 1000ms to mock async stuffs
    return Promise.resolve()
  }))
  await context.eval(`
    (async () => {
      global.move = (() => {
        const internalFn = __move
        delete __move
        return async function (distance, callback) {
          await internalFn.applySyncPromise(undefined, [distance])
          callback?.()
        }
      })()
    })()
  `)
  await context.eval(`delete global`) // removes access to global before running user code

  // Act
  const untrustedUserCode = await context.eval(`
    (() => {
      function onMoveEnd() {
        fire()
      }
      move(21, onMoveEnd)
    })
  `, { reference: true })
  await untrustedUserCode.apply(undefined, [])

  // Assert
  chai.assert.equal(playerBotDistance, 21)
  chai.assert.equal(nbFireCall, 1)
})