davideicardi / live-plugin-manager

Plugin manager and installer for Node.JS
MIT License
225 stars 43 forks source link

Literal Object prototype is not working properly inside the plugin's context #62

Open aacotroneo opened 2 years ago

aacotroneo commented 2 years ago

({}).constructor === Object is not working as expected, it should be true, but it's false if the code is run inside the plugin's code. As many libraries rely on that (bad practice) check, they don't fully work (such as aws sdk https://github.com/davideicardi/live-plugin-manager/issues/56)

This is a local hack, but easy enough to reproduce. It can be tested with any plugin, as we can simply replace 2 lines (in dist if you don't want to rebuild).

this line https://github.com/davideicardi/live-plugin-manager/blob/a05d1fd3b5c9271b734fec5143650a120d230f81/dist/src/PluginVm.js#L133

replace by the following 2 lines.

  console.log('This is outside plugin:', new Object().constructor === Object, ({}).constructor === Object)
  const iifeCode = `console.log('This is inside', new Object().constructor === Object, ({}).constructor === Object)`;

The output is

This is outside plugin: true true
This is inside true false

I spent some time trying to customize the context variables with no luck. Maybe someone with more expertise, I have no clue how the constructor of {} is defined in that child context we create.

Thanks!

aacotroneo commented 2 years ago

I've tried with a simple call to runInContext and it works as expected so it seems to be some of the config in this package. I'll try to identify the 'offending' line, maybe tomorrow

        vm.createContext(simpleContext);
        const code2 = `console.log('this is inside simpleCode', new Object().constructor === Object, ({}).constructor === Object)`;
        vm.runInContext(code2, simpleContext);

prints true true as expected

davideicardi commented 2 years ago

Thank you @aacotroneo for investigation. I also don't have any idea for now. It is probably some errors in the context configuration.

Can be something related to https://stackoverflow.com/questions/59009214/some-properties-of-the-global-instance-are-not-copied-by-spread-operator-or-by-o ?

This was the reason why here I have copied manually many definitions ... Maybe I should not do it?

davideicardi commented 2 years ago

@aacotroneo I have found some errors in the code that define the sandbox's globals, see #63 . Now console.log('this is inside simpleCode', new Object().constructor === Object, ({}).constructor === Object) is working as expected. I also added some more tests.

But this doesn't seems to actually fix the problem with aws-sdk.

I think that the fix is anycase valid, but maybe not related to the aws-sdk problem. What do you think?

aacotroneo commented 2 years ago

@davideicardi, nice, yes.. it seems that fixes this issue. I've been playing with aws-sdk, and I think the issue is a bit more complex. I'll see if I can reproduce with a test as you said in https://github.com/davideicardi/live-plugin-manager/issues/56 . I'll need to isolate and make it reproducible, but the last I found about aws-sdk was something like this. There're 2 files involved

// file A.js
const {aFunction} = require('./B')
const obj = {}
console.log(obj.constructor === Object); // true
aFunction(obj)
// file B.js
export function aFunction(obj) {
      console.log(obj.constructor === Object) // false!
}

So it now seems related to how the require shares context with the required file, and Object as a reference changing its value. That may make sense given the virtualization that this library uses. so there might not be a fix at all. I'll see if I can hack some test (maybe mocking the require function)

jamesgibson14 commented 2 years ago

I have run into a similar problem where String doesn't equal String, and Number, Object, etc. It seemed that even though I added global to the context, these literals were generated in the new context, so String !== String because they are different memory reference points. However, when I passed in a context of {String: String, Object: Object, Number: Number}, passing the same reference to the new context, then the equality checks would work. I hope this helps.

On Sun, Oct 24, 2021 at 6:53 PM Alejandro Cotroneo @.***> wrote:

@davideicardi https://github.com/davideicardi, nice, yes.. it seems that fixes this issue. I've been playing with aws-sdk, and I think the issue is a bit more complex. I'll see if I can reproduce with a test as you said in #56 https://github.com/davideicardi/live-plugin-manager/issues/56 . I'll need to isolate and make it reproducible, but the last I found about aws-sdk was something like this. There're 2 files involved

// file A.js const {aFunction} = require('./B') const obj = {} console.log(obj.constructor === Object); // true aFunction(obj)

// file B.js function aFunction(obj) { console.log(obj.constructor === Object) // false! }

So it now seems related to how the require shares context with the required file, and Object as a reference changing its value. That may make sense given the virtualization that this library uses. so there might not be a fix at all. I'll see if I can hack some test (maybe mocking the require function)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/davideicardi/live-plugin-manager/issues/62#issuecomment-950435588, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFY6KSJVBGPYBA3UFB776TUISTAPANCNFSM5GO3SYGA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

davideicardi commented 2 years ago

@jamesgibson14 Can you try with the branch of #63 ? (or I can publish a new version if you prefer ...) I suspect that at least this problem should be resolved with my fix.

jamesgibson14 commented 2 years ago

@davideicardi Hey sorry, I actually don't have it setup right now to test that same situation, I ended up adding the library to package.json :( it was mssql.

jamesgibson14 commented 2 years ago

Hi @davideicardi, I am back to using and testing your package, I ran into an error were "mailgun.js" was throwing an error "URLSearchParams is not defined" and it should be a global nodejs package. I update live-plugins to 0.17.1 and the error went away.

davideicardi commented 2 years ago

@jamesgibson14 Thank you for the update!