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

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

Open
TheBosZ opened this issue Jan 22, 2019 · 13 comments
Open

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

TheBosZ opened this issue Jan 22, 2019 · 13 comments

Comments

@TheBosZ
Copy link

TheBosZ commented Jan 22, 2019

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: uuidjs/uuid#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
Copy link
Owner

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
Copy link
Author

TheBosZ commented Jan 23, 2019

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
Copy link
Author

TheBosZ commented Jan 23, 2019

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

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
Copy link
Author

TheBosZ commented Mar 14, 2019

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

mischnic commented Mar 14, 2019

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
Copy link
Author

TheBosZ commented Mar 14, 2019

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

@mbullington
Copy link
Owner

mbullington commented Mar 21, 2019

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 mbullington changed the title Crypto stuff doesn't work in browser Random.secure() does not work in browser after packaged Mar 21, 2019
@mbullington
Copy link
Owner

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

mischnic commented Mar 22, 2019

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
Copy link
Owner

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.

@onedayitwillmake
Copy link

onedayitwillmake commented Nov 11, 2020

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

Zekfad commented Apr 21, 2023

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

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

No branches or pull requests

5 participants