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

1.0.7 and later broken in the browser #8

Closed
mcelrath opened this issue Mar 23, 2017 · 14 comments
Closed

1.0.7 and later broken in the browser #8

mcelrath opened this issue Mar 23, 2017 · 14 comments

Comments

@mcelrath
Copy link

mcelrath commented Mar 23, 2017

Commit #7 breaks the use of brorand by bcoin in Cordova.

You never define self anywhere, and it throws TypeError: Cannot read property 'getRandomValues' of undefined. I don't know how this is supposed to work...

@indutny
Copy link
Owner

indutny commented Mar 23, 2017

I think I've fixed it later on. Is it still a problem on 1.1.0?

@mcelrath
Copy link
Author

Yes it's still broken in 1.1.0.

https://github.com/indutny/brorand/blob/master/index.js#L31

You don't have var self anywhere...I don't understand how this code could ever work...

@indutny
Copy link
Owner

indutny commented Mar 23, 2017

Well, this code relies on typeof self === 'object'... I'm not sure how it can even get to the load of getRandomValues.

I'd say that you are most likely using old version of it somehow, perhaps it is either a cache or stale build result. I'm using this updated module for: http://indutny.github.io/self-signed/ and it appears to work in the browser.

@mcelrath
Copy link
Author

It's not an old version...we've fallen back to 1.0.6 for the time being. How does crypto get on self in the first place? How does self get set?

@indutny
Copy link
Owner

indutny commented Mar 23, 2017

typeof self === 'object' does not throw in JS, even if self is not present. It will continue to if's body only if self is actually present and is an object. Same thing with self.crypto, it won't attempt to load self.crypto.getRandomBytes unless self.crypto is present.

Would you care to share some code with me so I could reproduce it?

Thanks, and sorry for the problem!

@mcelrath
Copy link
Author

The code is the bcoin project...we wrap it into a Cordova app, and it's throwing because self.crypto is undefined... Here's a full stack trace when using brorand 1.1.0:

Uncaught TypeError: Cannot read property 'getRandomValues' of undefined
    at Rand._rand (file:///android_asset/www/bundle.js:44277:19)
    at Rand.generate (file:///android_asset/www/bundle.js:44258:16)
    at Object.rand (file:///android_asset/www/bundle.js:44249:13)
    at EC.genKeyPair (file:///android_asset/www/bundle.js:48644:43)
    at Object.generatePrivateKey (file:///android_asset/www/bundle.js:73451:24)
    at new PoolOptions (file:///android_asset/www/bundle.js:116977:26)
    at new Pool (file:///android_asset/www/bundle.js:111402:19)
    at new Blockchain (file:///android_asset/www/bundle.js:59640:16)
    at file:///android_asset/www/bundle.js:59389:23
    at Array.forEach (native)

While the source lines won't be useful, I'm sure you can recognize the piece of bcoin and elliptic:
https://github.com/indutny/elliptic/blob/master/lib/elliptic/ec/index.js#L53

@mcelrath
Copy link
Author

mcelrath commented Mar 24, 2017

bcoin is specifying elliptic 6.3.2, which is not the latest, if that's an issue:
https://github.com/bcoin-org/bcoin/blob/master/package.json#L26

Again I still don't understand who/what/where self or self.crypto is getting set at all...

@indutny
Copy link
Owner

indutny commented Mar 24, 2017

@mcelrath could you please provide a couple of lines around bundle.js:44277?

@mcelrath
Copy link
Author

bundle.js:44277 is https://github.com/indutny/brorand/blob/master/index.js#L35
bundle.js:44258 is https://github.com/indutny/brorand/blob/master/index.js#L16

Here's the entire thing, cut and pasted from the inspector. It's just brorand 1.1.0.

/* 1130 */
/***/ function(module, exports, __webpack_require__) {

	var r;
	
	module.exports = function rand(len) {
	  if (!r)
	    r = new Rand(null);
	
	  return r.generate(len);
	};
	
	function Rand(rand) {
	  this.rand = rand;
	}
	module.exports.Rand = Rand;
	
	Rand.prototype.generate = function generate(len) {
	  return this._rand(len);
	};
	
	// Emulate crypto API using randy
	Rand.prototype._rand = function _rand(n) {
	  if (this.rand.getBytes)
	    return this.rand.getBytes(n);
	
	  var res = new Uint8Array(n);
	  for (var i = 0; i < res.length; i++)
	    res[i] = this.rand.getByte();
	  return res;
	};
	
	if (typeof self === 'object') {
	  if (self.crypto && self.crypto.getRandomValues) {
	    // Modern browsers
	    Rand.prototype._rand = function _rand(n) {
	      var arr = new Uint8Array(n);
	      self.crypto.getRandomValues(arr);
	      return arr;
	    };
	  } else if (self.msCrypto && self.msCrypto.getRandomValues) {
	    // IE
	    Rand.prototype._rand = function _rand(n) {
	      var arr = new Uint8Array(n);
	      self.msCrypto.getRandomValues(arr);
	      return arr;
	    };
	
	  // Safari's WebWorkers do not have `crypto`
	  } else if (typeof window === 'object') {
	    // Old junk
	    Rand.prototype._rand = function() {
	      throw new Error('Not implemented yet');
	    };
	  }
	} else {
	  // Node.js or Web worker with no crypto support
	  try {
	    var crypto = __webpack_require__(1131);
	    if (typeof crypto.randomBytes !== 'function')
	      throw new Error('Not supported');
	
	    Rand.prototype._rand = function _rand(n) {
	      return crypto.randomBytes(n);
	    };
	  } catch (e) {
	  }
	}


/***/ },

@indutny
Copy link
Owner

indutny commented Mar 28, 2017

It looks like someone has set self.crypto on init, but then removed it... Do you think it may be a Cordova bug?

@mcelrath
Copy link
Author

You're depending on some implicit behavior with respect to the self variable, which is defined nowhere in this code and AFAIK is not a defined behavior of javascript.

  1. What do you expect self to be and who sets it?
  2. If you want to capture the value of self from some external code setting it, you need to wrap your initialization in a closure. You shouldn't depend on external, global variables...

All that said, I found some of our code is spuriously setting window.self, which it shouldn't be doing anyway, so I'll fix that. But could you answer my questions about self?I do think you should go back to using window directly...

@indutny
Copy link
Owner

indutny commented Mar 28, 2017

I believe self is pretty standard: https://developer.mozilla.org/ru/docs/Web/API/Window/self . There was some trick about it, I don't remember which one. Perhaps git blame could help?

@mcelrath
Copy link
Author

I've never seen that before...that is tricky.

Ok I'm closing this as it looks like my bug. Sorry for the noise.

@indutny
Copy link
Owner

indutny commented Mar 28, 2017

No problem at all! Thanks for using it, and sorry for causing trouble by the recent update!

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

No branches or pull requests

2 participants