mbullington / node_preamble.dart

:memo: Better node.js preamble for dart2js, use it in your build system.
Other
19 stars 12 forks source link

Random.secure() does not work in browser after packaged #14

Open TheBosZ opened 5 years ago

TheBosZ commented 5 years ago

Because of how the self variable is created, any access to self.crypto throws an Uncaught TypeError: Illegal invocation exception.

This will happen when the Random.secure() constructor is called. It compiles to Javascript that looks like this:

var $crypto = self.crypto;
      if ($crypto != null)
        if ($crypto.getRandomValues != null)
          return;
      throw H.wrapException(P.UnsupportedError$("No source of cryptographically secure random numbers available."));

But accessing crypto will throw.

You can try this yourself in Chrome:

  1. Open a dev console
  2. var a = Object.create(window);
  3. a.crypto
  4. Exception is thrown

According to this: https://github.com/kelektiv/node-uuid/issues/256, it's because it's losing the Crypto context.

My hacky fix is to just make the first line be var self = global; and call it good.

mbullington commented 5 years ago

Thank you for reporting this!

I tried it out and can reproduce, I know I messed around with this before but remember Object.create(window) being an acceptable solution. Your quick fix would work in this case, but then require, module, and process would break their scope.

I'll investigate more and see if this is a regression with newer versions of Chrome.

Any ideas for possible solutions would be greatly appreciated!

TheBosZ commented 5 years ago

This same thing happens in Firefox. It throws a more instructive exception though: TypeError: 'get crypto' called on an object that does not implement interface Window.

TheBosZ commented 5 years ago

Would it work to change it to use Object.assign()?

If I do that, it doesn't throw errors:

var a = Object.assign({}, window);
a.crypto.getRandomValues(new Uint8Array(16));
> Uint8Array(16)Ā [154, 140, 83, 149, 168, 44, 48, 218, 5, 237, 230, 179, 224, 10, 238, 173]

As an update: this doesn't work. I get problems with constructors not working if I do this. So close!

mischnic commented 5 years ago

This global object patching makes many properties on that object unuseable:

Reproduction:

(function(global){
  // make sure to keep this as 'var'
  // we don't want block scoping
  var self = Object.create(global);
  // ...

  // wrapper above

  console.log(self.location) // Throws an error described below 
})(self);

Chrome 72: In main (window) context: works Running in a Web Worker: Uncaught TypeError: Illegal invocation Firefox 65: window: TypeError: 'get location' called on an object that does not implement interface Window Worker: same error as in window (... interface WorkerGlobalScope) Safari 12: window: works Worker: The WorkerGlobalScope.location getter can only be used on instances of WorkerGlobalScope

TheBosZ commented 5 years ago

If you want, you can edit the source code to just be var self = global;

That's what I'm doing for some other stuff.

mischnic commented 5 years ago

That's what I'm doing for some other stuff.

Yes, but for me, it's a dependency and fiddling around in node_modules isn't particularly persistent šŸ˜„

TheBosZ commented 5 years ago

Yeah, it's annoying. You could check out a copy of dart-sass, modify it, and then point your package.json to it.

mbullington commented 5 years ago

Hello, sorry for the late response.

I know that web workers handle the 'self' keyword differently, could you please file a separate issue for that? Also a separate issue for Firefox support please? I'm not sure if this is something that is an easy fix.

Also based on the issues you've reported on other repositories, I would check to see if Dart Sass supports working in the browser. Other factors than just node_preamble may be cause for issues, and are out of the scope of this project.

Still searching for a solution, I may just introduce a fix specifically for getRandomValues. As explained above var self = global; might work in the short term, but overall will break your application if you have more than one library using the preamble.

mbullington commented 5 years ago

It seems at least Chrome is doing something really weird with the Window object when it's a prototype, try this...

var a = Object.create(window)
a.crypto = 1
a.crypto
// Illegal invocation
mischnic commented 5 years ago

I know that web workers handle the 'self' keyword differently, could you please file a separate issue for that? Also a separate issue for Firefox support please? I'm not sure if this is something that is an easy fix.

I can do that, but these are actually all caused by the same problem: duplicating the global object doesn't play well the "native" properties

Also based on the issues you've reported on other repositories, I would check to see if Dart Sass supports working in the browser. Other factors than just node_preamble may be cause for issues, and are out of the scope of this project.

I've got it working, though I have a feeling the dart-sass developers don't really care. (Only needed to replace a self.require with require so that bundlers can statically analyze it).

As explained above var self = global; might work in the short term

Yes, it's a one-off workaround.

It seems at least Chrome is doing something really weird with the Window object when it's a prototype, try this...

But not for all properties:

Chrome: 
> var a = Object.create(window); a.crypto
Uncaught TypeError: Illegal invocation
    at <anonymous>:2:3
> var a = Object.create(window); a.location
LocationĀ {...}

Firefox:

> var a = Object.create(window); a.crypto
TypeError: 'get crypto' called on an object that does not implement interface Window.
> var a = Object.create(window); a.location
TypeError: 'get location' called on an object that does not implement interface Window.

Safari: 
> var a = Object.create(window); a.crypto
Crypto {...}
> var a = Object.create(window); a.location
LocationĀ  {...}
mbullington commented 5 years ago

I know that web workers handle the 'self' keyword differently, could you please file a separate issue for that? Also a separate issue for Firefox support please? I'm not sure if this is something that is an easy fix.

I can do that, but these are actually all caused by the same problem: duplicating the global object doesn't play well the "native" properties

Great, thank you!

I'm aware that duplicating the object isn't perfect, but I feel there are some slight differences to the problem that I discuss below.

Also based on the issues you've reported on other repositories, I would check to see if Dart Sass supports working in the browser. Other factors than just node_preamble may be cause for issues, and are out of the scope of this project.

I've got it working, though I have a feeling the dart-sass developers don't really care. (Needed to replace a self.require to require so that bundlers can statically analyze it).

As explained above var self = global; might work in the short term

Yes, it's a one-off workaround.

It seems at least Chrome is doing something really weird with the Window object when it's a prototype, try this...

But not for all properties:

Chrome: 
> var a = Object.create(window); a.crypto
Uncaught TypeError: Illegal invocation
    at <anonymous>:2:3
> var a = Object.create(window); a.location
LocationĀ {...}

Firefox:

> var a = Object.create(window); a.crypto
TypeError: 'get crypto' called on an object that does not implement interface Window.
> var a = Object.create(window); a.location
TypeError: 'get location' called on an object that does not implement interface Window.

Safari: 
> var a = Object.create(window); a.crypto
Crypto {...}
> var a = Object.create(window); a.location
LocationĀ  {...}

Correct, I was speaking directly to the crypto issue described in this ticket.

The way I'm looking to tackle it, making the global object a prototype with Object.create isn't perfect but does work, with caveats:

Any PR could tackle one or more of these problems, but they're also separated to where I can investigate each individually. In addition, it's super important to make sure any change would still fully work with Node.js, as that's the primary goal of this library.

onedayitwillmake commented 3 years ago

Is there any traction on this issue?

It seems to be effecting more dependencies of this library when using the output in Chrome (perhaps it is behaving more strictly than in the past with code acting on the window object?)

Zekfad commented 1 year ago

What about using Proxy to return falling objects from native globalThis? E.g. something like this:

_globalThis = (typeof globalThis !== 'undefined' && globalThis) ||
    (typeof global !== 'undefined' && global) ||
    (typeof window !== 'undefined' && window) ||
    null;
self = new Proxy(Object.create(_globalThis), {
    get(target, prop, receiver) {
        try {
            return target[prop];
        } catch (e) {
            if (-1 !== e.message.indexOf('Illegal invocation'))
                return _globalThis[prop];
            throw e;
        }
    }
});