Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM context object silently fails on Object.defineProperty #5679

Closed
kriskowal opened this issue Mar 12, 2016 · 10 comments
Closed

VM context object silently fails on Object.defineProperty #5679

kriskowal opened this issue Mar 12, 2016 · 10 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@kriskowal
Copy link
Contributor

  • Version: problem exists in 0.12.9 and later, does not exist in 0.10.36
  • Platform: Darwin XXXX 14.3.0 Darwin Kernel Version 14.3.0: Mon Mar 23 11:59:05 PDT 2015; root:xnu-2782.20.48~5/RELEASE_X86_64 x86_64
  • Subsystem: vm

The following script creates a VM context and runs a script that adds a property to that context, redefines that property as immutable, attempts to assign to that property, logs the error if there is one, then logs the value and property descriptor.

Expected behavior (exhibited in Node.js v0.10.36):

Cannot assign to read only property 'x' of #<Object>
20
{ value: 20,
  writable: false,
  enumerable: false,
  configurable: false }

Actual behavior (exhibited in Node.js v4.3.1, v5.8.0):

Cannot assign to read only property 'x' of #<Object>
30
{ value: 30,
  writable: true,
  enumerable: true,
  configurable: true }
var vm = require('vm');

var script = '(' + function () {
    'use strict';
    global.x = 10;
    Object.defineProperty(global, 'x', {
        writable: false,
        enumerable: false,
        configurable: false,
        value: 20
    });
    try {
        x = 30;
    } catch (e) {
        console.error(e);
    }
    console.log(x); // should be 20
    console.log(Object.getOwnPropertyDescriptor(global, 'x'));
} + ')()';

var global = {};
global.console = console;
global.global = global;

var context = vm.createContext(global);
vm.runInContext(script, context);

cc @erights

@claudiorodriguez claudiorodriguez added the vm Issues and PRs related to the vm subsystem. label Mar 12, 2016
@princejwesley
Copy link
Contributor

Duplicate of #5344 ?

@kriskowal
Copy link
Contributor Author

@princejwesley Very likely a duplicate. This is blocking Secure ECMAScript confinement. https://github.com/drses/ses5

@kriskowal
Copy link
Contributor Author

cc @domenic I am wondering whether you might be able to shine some light on the nature of VM context objects. What is special about the global object in a VM context that might make it hard to freeze properties?

@tniessen
Copy link
Member

tniessen commented Jun 3, 2017

This seems to work in node 7.x and 8.x, but not in 6.x and 4.x. Sadly, I cannot find the PR / commit which fixed the issue (maybe a v8 bug?). Do we want to backport the fix to 4.x / 6.x?

@bnoordhuis
Copy link
Member

There have been several commits that address this but I don't think they can be back-ported because some require extensive changes to V8.

I'll go ahead and close this out. People needing this feature should consider upgrading.

@AnnaMag
Copy link
Member

AnnaMag commented Jun 3, 2017

For the sake of completeness:
the attributes do not seem to be recovered correctly in node 7 nor 8 for me (the value is ok, but
all are true).

The behavior is fixed with this patch (with some extra V8 changes):
#13265

@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 3, 2017

Okay, I'll reopen pending #13265.

(Now that I think about it, I recall this being on track for node 9, not 8.)

@bnoordhuis bnoordhuis reopened this Jun 3, 2017
@kriskowal
Copy link
Contributor Author

Confirmed that the issue is partially fixed.

❯ uname -a
Linux DESKTOP-RA6J2R3 3.4.0+ #1 PREEMPT Thu Aug 1 17:06:05 CST 2013 x86_64 x86_64 x86_64 GNU/Linux
❯ node --version
v8.0.0
❯ node 5679.js
TypeError: Cannot assign to read only property 'x' of object '#<Object>'
    at evalmachine.<anonymous>:11:11
    at evalmachine.<anonymous>:17:3
    at ContextifyScript.Script.runInContext (vm.js:53:29)
    at Object.runInContext (vm.js:108:6)
    at Object.<anonymous> (/root/ses/5679.js:26:4)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
20
{ value: 20,
  writable: true,
  enumerable: true,
  configurable: true }

targos added a commit to targos/node that referenced this issue Sep 5, 2017
@fhinkel
Copy link
Member

fhinkel commented Oct 23, 2017

@fhinkel fhinkel closed this as completed Oct 23, 2017
@kriskowal
Copy link
Contributor Author

Confirmed that this is fully fixed in 9.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants