node_preamble.dart icon indicating copy to clipboard operation
node_preamble.dart copied to clipboard

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

Open TheBosZ opened this issue 6 years ago • 13 comments

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.

TheBosZ avatar Jan 22 '19 22:01 TheBosZ

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!

mbullington avatar Jan 23 '19 01:01 mbullington

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 avatar Jan 23 '19 17:01 TheBosZ

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!

TheBosZ avatar Jan 23 '19 17:01 TheBosZ

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

mischnic avatar Mar 14 '19 23:03 mischnic

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.

TheBosZ avatar Mar 14 '19 23:03 TheBosZ

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 😄

mischnic avatar Mar 14 '19 23:03 mischnic

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

TheBosZ avatar Mar 14 '19 23:03 TheBosZ

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 avatar Mar 21 '19 23:03 mbullington

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

mbullington avatar Mar 21 '19 23:03 mbullington

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  {...}

mischnic avatar Mar 22 '19 00:03 mischnic

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:

  • crypto (which I'll have to read up on, but I believe has different functionality due to its nature of being for secure crypto)
  • Web worker support (where I believe self is the only way to access global scope and has limitations)
  • Firefox support (which may just implement it completely different from WebKit variants)

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.

mbullington avatar Mar 22 '19 00:03 mbullington

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?)

onedayitwillmake avatar Nov 11 '20 22:11 onedayitwillmake

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;
		}
	}
});

Zekfad avatar Apr 21 '23 13:04 Zekfad