patriksimek / vm2

Advanced vm/sandbox for Node.js
MIT License
3.86k stars 293 forks source link

freeze doesn't work on Array methods (shift, pop, etc) #511

Open poef opened 1 year ago

poef commented 1 year ago

running node v18.15.0 if i have this code:

import {VM} from 'vm2'
const vm = new VM({
    allowAsync: false,
    wasm: false
})
let result = [1,2,3]
vm.freeze(result, 'data')

I expect that this will fail:

vm.run(`
data.pop()
`)

However, the array is changed and if I run it multiple times, I get 3, 2 then 1 as a result.

XmiliaH commented 1 year ago

Freeze does only stop the sandbox from manipulating the object. The pop method is from the host and is allowed to manipulate the array.

poef commented 1 year ago

Hi, I understand your explanation about why the vm.freeze() call doesn't work on Array.pop() here. However, given the documentation of freeze(), I would expect the data to be immutable. Since pop() alters the array, it isn't immutable. So the freeze() function doesn't turn arrays immutable, which I believe it should.

XmiliaH commented 1 year ago

If you want to make the array immutable just use Object.freeze.

poef commented 1 year ago

I would politely suggest to rename 'freeze' to something else, or at least update the documentation so that it tells people that it doesn't actually freeze the data entirely. As it is now, the behaviour is rather surprising. And yes, I did use Object.freeze to fix it.

BTW, thank you for your work on VM2, I really like how simple it is to use this library.