Partial fix for #864 - report property attributes from sandbox object by smikes · Pull Request #885 · nodejs/node

smikes · 2015-02-18T21:28:12Z

Here's a failing test for issue #864

cjihrig · 2015-02-19T16:01:42Z

Please do not merge this in while the test is still failing.

GlobalPropertyQueryCallback() was returning `None` as attributes for all properties, without regard to actual properties settings. This patch looks up attributes for sandbox-defined properties and returns them. Partial fix for nodejsgh-864 Add test for nodejsgh-864, non-default settings of object properties (e.g., `enumerable: false`) are not visible when object is used as a sandbox

smikes · 2015-02-19T22:37:22Z

Test is no longer failing; a change to node_contextify.cc is a partial fix for #864 and fixes the behavior tested in test-vm-preserves-property.js.

smikes · 2015-02-19T22:55:36Z

Although this PR was originally inspired by #864, it is complete in itself and reports sandbox property attributes correctly.

The full fix for #864 may involve changes to v8 APIs upstream (see @bnoordhuis comments in #884).

In the meantime, I believe this PR is now ready for review.

domenic · 2015-02-23T17:39:18Z

Shouldn't we also add if (in_proxy_global) { attr = proxy_global->GetPropertyAttributes(property); } as the fallback?

I tried that. It caused infinite recursion on proxy_global. I think @bnoordhuis has a patch into v8 to add a new api to support this.
That's why this change is "partial fix"

Got it. Also given that per #855 I want to at least attempt to rip out proxy global entirely that might end up being unnecessary.
Probably want to leave a TODO(smikes) in the source code though for later.

domenic · 2015-02-23T18:42:51Z

LGTM if you add a TODO. (I am not an official LGTM-er though.)

brendanashworth · 2015-03-17T02:37:56Z

Is there an update on this? see

smikes · 2015-03-27T21:45:13Z

I will try to finish this this weekend.

bnoordhuis · 2015-03-27T21:56:12Z

Apropos that V8 patch, it landed in v8/v8@726eb05 and should probably be back-ported. That makes it a semver-minor thing because it's a new public API. It's also going to break building against a shared libv8 that doesn't carry the patch. :-/

domenic · 2015-05-22T23:21:51Z

@smikes any chance of landing this on next, with the updates?

domenic · 2015-05-22T23:23:18Z

Why not HasRealNamedProperty ?

domenic · 2015-05-22T23:25:27Z

Actually I think I can take this over.

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis · 2015-06-05T15:29:43Z

Closing, fixed by a45fbfc.

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

smikes force-pushed the gh-864-failing-test branch from a5c74f5 to 2dfd002Compare February 19, 2015 22:33

smikes changed the title ~~add failing test~~Partial fix for #864 - report property attributes from sandbox objectFeb 19, 2015

smikes mentioned this pull request Feb 19, 2015
Change from 0.10 => 1.2; enumerable of this.escape in vm.runInNewContext was false, now true #864
Closed

domenic reviewed Feb 23, 2015
View reviewed changes

mscdex added the vm Issues and PRs related to the vm subsystem. label Mar 22, 2015

chrisdickinson added the land-on-master label Apr 17, 2015

domenic reviewed May 22, 2015
View reviewed changes

Comment threadsrc/node_contextify.cc Outdated
domenicMay 22, 2015
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not HasRealNamedProperty ?

domenic mentioned this pull request May 22, 2015
vm: fix a variety of bugs, using V8 4.3 APIs #1773
Closed

bnoordhuis closed this Jun 5, 2015

Uh oh!

Uh oh!

Conversation

smikes commented Feb 18, 2015

Uh oh!

cjihrig commented Feb 19, 2015

Uh oh!

smikes commented Feb 19, 2015

Uh oh!

smikes commented Feb 19, 2015

Uh oh!

domenicFeb 23, 2015

Choose a reason for hiding this comment

Uh oh!

smikesFeb 23, 2015

Choose a reason for hiding this comment

Uh oh!

domenicFeb 23, 2015

Choose a reason for hiding this comment

Uh oh!

domenic commented Feb 23, 2015

Uh oh!

brendanashworth commented Mar 17, 2015

Uh oh!

smikes commented Mar 27, 2015

Uh oh!

bnoordhuis commented Mar 27, 2015

Uh oh!

domenic commented May 22, 2015

Uh oh!

domenicMay 22, 2015

Choose a reason for hiding this comment

Uh oh!

domenic commented May 22, 2015

Uh oh!

bnoordhuis commented Jun 5, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants