Skip to content

Frozen object tries to create property on receiver#346

Merged
XmiliaH merged 1 commit intopatriksimek:masterfrom
XmiliaH:fix-330
May 14, 2021
Merged

Frozen object tries to create property on receiver#346
XmiliaH merged 1 commit intopatriksimek:masterfrom
XmiliaH:fix-330

Conversation

@XmiliaH
Copy link
Copy Markdown
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
Copy Markdown

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

@sumbricht
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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