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

Frozen object tries to create property on receiver #346

Merged
merged 1 commit into from May 14, 2021

Conversation

XmiliaH
Copy link
Collaborator

@XmiliaH XmiliaH commented Apr 20, 2021

This should fix #330. We now try to create the property on the receiver if an frozen object is discovered in the property chain.

@scaredcat
Copy link

any updates on when this will be included in a release?

@sumbricht
Copy link

It seems that this was once included in the version tagged "v3.9.2" (mind the "v" in the tag name) but is no longer in any of the tagged versions 3.9.3 - 3.9.11 (without the "v"). These don't even contain the file lib/contextify.js.
Was this removed again on purpose? As I'm also getting the same errors as described in #330 when using context: 'sandbox', I'd be very happy for this solution to be reconsidered.

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Oct 4, 2022

This is still included.

vm2/lib/bridge.js

Lines 733 to 742 in 62062ad

set(target, key, value, receiver) {
// Note: target@this(unsafe) key@prim value@this(unsafe) receiver@this(unsafe) throws@this(unsafe)
return thisReflectDefineProperty(receiver, key, {
__proto__: null,
value: value,
writable: true,
enumerable: true,
configurable: true
});
}

@sumbricht
Copy link

This is still included.

vm2/lib/bridge.js

Lines 733 to 742 in 62062ad

set(target, key, value, receiver) {
// Note: target@this(unsafe) key@prim value@this(unsafe) receiver@this(unsafe) throws@this(unsafe)
return thisReflectDefineProperty(receiver, key, {
__proto__: null,
value: value,
writable: true,
enumerable: true,
configurable: true
});
}

Thank you very much for your quick reply, @XmiliaH. I have now been able to run through and pinpoint what's happening.

What I'm doing:

  • Require 'exceljs' from within a vm2 NodeVM (with require.context: 'sandbox')
  • Try to load a CSV file from disk
  • -> The constructor of CsvParserStream (from node module '@fast-csv') tries to initialize properties:
class CsvParserStream extends stream_1.Transform {
    constructor(parserOptions) {
        super({ objectMode: parserOptions.objectMode });
        this.lines = '';
        [..]
    }
    [..]
}
  • The line this.lines = '' triggers ReadOnlyHandler.set(...) (as you referenced in your reply)
  • thisReflectDefineProperty in ReadOnlyHandler.set points to ReadOnlyHandler.defineProperty and NOT to Reflect.defineProperty as I would have expected
  • ReadOnlyHandler.defineProperty always returns false, therefore causing the error "TypeError: 'set' on proxy: trap returned falsish for property 'lines'"

If you look at the last two bullet points, does this make sense to you or is there something wrong happening?

If it helps, I'm happy to create a new issue and a minimal reproducible example, just let me know. If you already have a suspicion, even better :-).
Thanks a lot for your help!

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Oct 5, 2022

The call from ReadOnlyHandler.set will call Reflect.defineProperty but handled by the proxy handler ReadOnlyHandler.defineProperty as receiver === target. This is expected.

I think you are running into the known limitation:

It is not possible to define a class that extends a proxied class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'x'
3 participants