Issue 4465 - Move rsa module from adblockpluschrome

Issue 4465 - Move rsa module from adblockpluschrome

[kzar, 2016-09-23 12:29:29]

Patch Set 1

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js
File lib/jsbn.js (right):

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js#newcode48
lib/jsbn.js:48: exports.BigInteger = BigInteger;
I had trouble updating to the latest jsbn as you suggested, so decided to simply
export BigInteger.

[Wladimir Palant, 2016-09-23 12:48:46]

What about unit tests? Please convert
adblockpluschrome/qunit/tests/signatures.js - this piece of code is too
complicated to go without test coverage.

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js
File lib/jsbn.js (right):

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js#newcode48
lib/jsbn.js:48: exports.BigInteger = BigInteger;
On 2016/09/23 12:29:29, kzar wrote:
> I had trouble updating to the latest jsbn as you suggested, so decided to
simply
> export BigInteger.

I'd rather not modify third-party code, this would quickly turn into a mess.
What was the problem with the new version, did the API change?

[kzar, 2016-09-23 13:47:03]

Patch Set 2 : Add unit tests

I was in the process of converting the signature tests from adblockplustests for
#4223, but I guess you're right that it's better to move those over right away.

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js
File lib/jsbn.js (right):

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js#newcode48
lib/jsbn.js:48: exports.BigInteger = BigInteger;
On 2016/09/23 12:48:46, Wladimir Palant wrote:
> On 2016/09/23 12:29:29, kzar wrote:
> > I had trouble updating to the latest jsbn as you suggested, so decided to
> simply
> > export BigInteger.
> 
> I'd rather not modify third-party code, this would quickly turn into a mess.
> What was the problem with the new version, did the API change?

Some changes I had to make to the latest version:

 - He assumed `module` existed if `exports` did so I had to change that.
 - Js Hydra seemed not to like the IIFE he used for some reason, I had to remove
that.
 - I had to add the licence header back in.
 - IIRC I had to change other things too but I forget now.

Anyway after all that I still had failing tests. It was then when looking at
diffs from the old version I noticed that we had already made some other changes
too. See the `/** MODIFIED **/` comments. That is the point I gave up.

[kzar, 2016-09-23 13:48:45]

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js#newcode29
lib/rsa.js:29: global.atob = data => new Buffer(data,
"base64").toString("binary");
If I simply did `var atob = ...` here the tests broke in adblockpluschrome.
Don't ask me why, since `atob` should never be undefined there...

[kzar, 2016-09-23 13:48:45]

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js#newcode29
lib/rsa.js:29: global.atob = data => new Buffer(data,
"base64").toString("binary");
If I simply did `var atob = ...` here the tests broke in adblockpluschrome.
Don't ask me why, since `atob` should never be undefined there...

[Wladimir Palant, 2016-09-23 20:02:29]

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js
File lib/jsbn.js (right):

https://codereview.adblockplus.org/29354773/diff/29354774/lib/jsbn.js#newcode48
lib/jsbn.js:48: exports.BigInteger = BigInteger;
On 2016/09/23 13:47:03, kzar wrote:
> Anyway after all that I still had failing tests. It was then when looking at
> diffs from the old version I noticed that we had already made some other
changes
> too. See the `/** MODIFIED **/` comments. That is the point I gave up.

You are right, I realized now that I made a few changes in order to simplify
importing binary data. Fair enough, we can look into using the new version
later.

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js#newcode29
lib/rsa.js:29: global.atob = data => new Buffer(data,
"base64").toString("binary");
On 2016/09/23 13:48:45, kzar wrote:
> If I simply did `var atob = ...` here the tests broke in adblockpluschrome.
> Don't ask me why, since `atob` should never be undefined there...

atob() is defined on the window object, and Node doesn't have one (see
https://github.com/nodejs/node/issues/3462). Patching the module isn't the right
approach, you should rather add this method globally when running unit tests.

I've had the same problem with Easy Passwords, there I run nodeunit like this
now:

  let atob = str => new Buffer(str, "base64").toString("binary");
  let btoa = str => new Buffer(str, "binary").toString("base64");
  let navigator = {};

  let nodeunit = require("sandboxed-module").require("nodeunit", {
    globals: {atob, btoa, navigator}
  });
  nodeunit.reporters.default.run(files);

It seems that we need to do the same here in test_wrapper.js, requires
introducing sandboxed-module as development dependency. This dependency is well
worth it, it will also simplify rerouting requires significantly.

https://codereview.adblockplus.org/29354773/diff/29354814/test/signatures.js
File test/signatures.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/test/signatures.js#...
test/signatures.js:136: };
I assume that this file was taken over without any changes?

[kzar, 2016-09-25 12:46:48]

Patch Set 3 : Define atob in test_wrapper.js

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js#newcode29
lib/rsa.js:29: global.atob = data => new Buffer(data,
"base64").toString("binary");
On 2016/09/23 20:02:29, Wladimir Palant wrote:
> On 2016/09/23 13:48:45, kzar wrote:
> > If I simply did `var atob = ...` here the tests broke in adblockpluschrome.
> > Don't ask me why, since `atob` should never be undefined there...
> 
> atob() is defined on the window object, and Node doesn't have one (see
> https://github.com/nodejs/node/issues/3462). Patching the module isn't the
right
> approach, you should rather add this method globally when running unit tests.
> 
> I've had the same problem with Easy Passwords, there I run nodeunit like this
> now:
> 
>   let atob = str => new Buffer(str, "base64").toString("binary");
>   let btoa = str => new Buffer(str, "binary").toString("base64");
>   let navigator = {};
> 
>   let nodeunit = require("sandboxed-module").require("nodeunit", {
>     globals: {atob, btoa, navigator}
>   });
>   nodeunit.reporters.default.run(files);
> 
> It seems that we need to do the same here in test_wrapper.js, requires
> introducing sandboxed-module as development dependency. This dependency is
well
> worth it, it will also simplify rerouting requires significantly.

Good point. I already did a bunch of similar work[1] to get other tests running
for #4223, so if it's all the same will simply assign `GLOBAL.atob`.

(This makes me realise there are already far too many changes sitting in that
branch, I will upload a review today for what I have so far.)

[1] -
https://github.com/kzar/adblockpluscore/blob/4223-migrate-tests/test_wrapper.js

https://codereview.adblockplus.org/29354773/diff/29354814/test/signatures.js
File test/signatures.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/test/signatures.js#...
test/signatures.js:136: };
On 2016/09/23 20:02:29, Wladimir Palant wrote:
> I assume that this file was taken over without any changes?

I made a few changes after copying this over from adblockplustests:

 - I had to require the rsa module.
 - I removed the IIFE.
 - I made changes required for the qunit to node unit switch. For example
`test("512 bit signing key", 6, function()` -> `exports.test512bitSigningKey =
function(test)`.
 - I added "use strict".

[Wladimir Palant, 2016-09-25 13:02:28]

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29354773/diff/29354814/lib/rsa.js#newcode29
lib/rsa.js:29: global.atob = data => new Buffer(data,
"base64").toString("binary");
On 2016/09/25 12:46:47, kzar wrote:
> Good point. I already did a bunch of similar work[1] to get other tests
running
> for #4223, so if it's all the same will simply assign `GLOBAL.atob`.

sandboxed-module seems to be a cleaner solution to me but I won't insist.

https://codereview.adblockplus.org/29354773/diff/29354852/lib/jsbn.js
File lib/jsbn.js (right):

https://codereview.adblockplus.org/29354773/diff/29354852/lib/jsbn.js#newcode98
lib/jsbn.js:98: if(j_lm && (typeof navigator != "undefined" && navigator.appName
== "Microsoft Internet Explorer")) {
Just define a navigator global in the tests, don't mess with the code here.

https://codereview.adblockplus.org/29354773/diff/29354852/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29354773/diff/29354852/test_wrapper.js#new...
test_wrapper.js:3: GLOBAL.atob = data => new Buffer(data,
"base64").toString("binary");
Define btoa as well, for consistency?

[kzar, 2016-09-25 13:19:03]

Patch Set 4 : Add btoa and navigator stub

https://codereview.adblockplus.org/29354773/diff/29354852/lib/jsbn.js
File lib/jsbn.js (right):

https://codereview.adblockplus.org/29354773/diff/29354852/lib/jsbn.js#newcode98
lib/jsbn.js:98: if(j_lm && (typeof navigator != "undefined" && navigator.appName
== "Microsoft Internet Explorer")) {
On 2016/09/25 13:02:28, Wladimir Palant wrote:
> Just define a navigator global in the tests, don't mess with the code here.

Yea, I wasn't thinking too hard when I changed this apparently. Done.

https://codereview.adblockplus.org/29354773/diff/29354852/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29354773/diff/29354852/test_wrapper.js#new...
test_wrapper.js:3: GLOBAL.atob = data => new Buffer(data,
"base64").toString("binary");
On 2016/09/25 13:02:28, Wladimir Palant wrote:
> Define btoa as well, for consistency?

Done.

[Wladimir Palant, 2016-09-25 13:23:09]

LGTM