crypto.createHash fix for node.js < 11

[jas-]

This PR will update the sha1() function @ lib/protocol/Auth.js to update the crypto.createHash function to use the .write() & .end() functions vs. the depreciated .update() & .digest() methods when working with node.js < v0.10.*

I tried finding the exact version which removes the .update() & .digest() functions and through my tests I could only determine it to be between v0.9.* & v0.10.*.

[sidorares]

👍 lgtm

[dougwilson]

Since the version of node.js does not change at runtime, the if statement can be moved out of the function body such that the function's implementation is decided when the module is required:

var sha1;
if (Number(process.version.match(/^v\d+\.(\d+)/)[1]) >= 10){
  sha1 = function sha1(msg) {
    var hash = Crypto.createHash('sha1');
    hash.setEncoding('binary');
    hash.write(msg);
    hash.end();
    return hash.read();
  };
} else {
  sha1 = function sha1(msg) {
    var hash = Crypto.createHash('sha1');
    hash.update(msg);
    // hash.digest() does not output buffers yet
    return hash.digest('binary');
  };
}

[jas-]

Is there a consensus with what @dougwilson has suggested? I will update the pr in the morning.

[sidorares]

@jas- yeah, I like @dougwilson suggestion

[jas-]

@sidorares @dougwilson pr has been updated to reflect recommendations in comment #35435407.

Thanks, I wouldn't have thought of that.

[dougwilson]

unnecessary semicolon

[dougwilson]

Looks good to me 👍
I added a comment about an unnecessary semicolon, but it doesn't actually need to be fixed; we can merge this even if @jas- doesn't get a chance to remove it.

[jas-]

@dougwilson Fixed the semicolon

[dougwilson]

@jas- ooo, so fast :)

[jas-]

@dougwilson I have time to address things like this right now when it relates to dependencies for another project. :)

[sidorares]

thanks guys :)

[jas-]

Thanks from me as well @sidorares & @dougwilson

[jas-]

@sidorares or @dougwilson Sorry to bug you, but is this get handed over to the npm registry so I can update my package.json to use that or should I just use the github repo for now?

[dougwilson]

@jas- I don't think you are bugging us :) If you need the fix ASAP and you don't check in your node_modules in version control, then by all means you can point npm to the git repo at commit 688dca6 (git://github.com/felixge/node-mysql.git#688dca6).

@sidorares may be able to weigh in on the timeline of a new release on npm.

[sidorares]

@jas- @dougwilson I updated changelog and tagged v2.1.0 release but having some strange npm errors

npm http PUT https://registry.npmjs.org/mysql/-/mysql-2.1.0.tgz/-rev/96-b5d0963e7050414ceb069b4261eb56a6
npm http 500 https://registry.npmjs.org/mysql/-/mysql-2.1.0.tgz/-rev/96-b5d0963e7050414ceb069b4261eb56a6
npm ERR! registry error parsing json

@dougwilson - could you try to publish yourself? You seem to have npm access rights

[dougwilson]

@sidorares I just tried and am getting the same error. Searching for npm ERR! registry error parsing json seems to indicate that it is a transient error from NPM, so we may need to wait to publish later.

[dougwilson]

Oh, I just got it to publish. @jas- 2.1.0 is on npm now for you :)

[jas-]

You guys are quick, thanks!

[dougwilson]

@jas- I see your comment is deleted now :) I think npm was behind on updating their indexes when you had tried to install 2.1.0 is all.

[jas-]

@dougwilson Yeah, I was quick to comment. Sorry about that!

[dougwilson]

Hi @jas-, it has turned out this commit broke sha1 in the early versions on node.js 0.10. What was the specific version on node.js you were using where the old sha1 method was not functioning? The commit message leads me to believe that before this it was broken on node.js < 0.11, but perhaps you meant > 0.11? The weird part is it looks like .update() and .digest() have not even been removed from 0.11 yet, which you suggested they had been removed somewhere.

[jas-]

@dougwilson Strange because I have tested it with v0.10.25 & v0.10.26 as well as v0.11.5 and didn't encounter the problem.

Perhaps further version tests are in order? I have encountered the same problem when enabling the tls module requestCert option which was I have issued a pr for.

What versions (specifically) does the pr break? And your right, the .update() and .digest() methods are still available for that version.

[dougwilson]

It breaks under node.js 0.10.1, 0.10.2 and 0.10.5 (these are all confirmed). Since the old sha1 function works under all node.js 0.10, how about I change it so the new sha1 you implemented is just for 0.11+?

[dougwilson]

@jas- the commit nodejs/node-v0.x-archive@4bf1d10 is where it changed in node.js; before that, your call to hash.setEncoding does not work, after it the call does work (i.e. you cannot call hash.setEncoding() before writing to the stream before that commit).

Even after that commit, it seems something it not working correctly with the sha1 function.

TL;DR node.js >= 0.10.0 and < 0.10.5 the sha1 function will blow up. node.js 0.10.5 to something the call does not blow up, but seems to not return the correct results.

You can refer to #746 as well.

[dougwilson]

@jas- as a further update, due to bug fixed in commit nodejs/node-v0.x-archive@d515857 before node.js 0.10.6, the call to hash.write assumed the string was encoded as utf8 and not binary, which is why people with 0.10.5 are not seeing it blow up, but are getting authentication failed.