TritonDataCenter / node-sshpk

Parse, convert, fingerprint and use SSH keys in pure node.js
MIT License
189 stars 49 forks source link

Use Buffer.(from|alloc) instead of deprecated Buffer API #46

Closed ChALkeR closed 6 years ago

ChALkeR commented 6 years ago

This also includes a dependnecy on a polyfill targeting older Node.js versions where Buffer.alloc() and Buffer.from() API is not implemented (Node.js < 4.5.0 and some 5.x versions).

This is mostly a line-to-line change, though in some cases, minor code simplification was done, e.g.

var version = new Buffer(1);
version.writeUInt8(1, 0);

was transformed into

var version = Buffer.from([1]);

Ref: https://github.com/nodejs/node/issues/19079 Ref: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor Ref: https://nodejs.org/api/buffer.html#buffer_class_buffer Ref: https://github.com/ChALkeR/safer-buffer/blob/master/Porting-Buffer.md Fixes: #45

Note that this does not completely solve the problem yet, for a complete solution, asn1 (a dependency) should also receive this fix (see below for that one).

But fixing the API usage in this package is still required, and a significant portion of tests pass without ever hitting the deprecated Buffer API in asn1 dependency.

ChALkeR commented 6 years ago

asn1 fix in https://github.com/joyent/node-asn1/pull/30.

BYK commented 6 years ago

@arekinath would be great if you can merge this and release a new version as this is blocking Yarn from being Node 10 compatible.

We can use the fork but that's less than ideal.

ChALkeR commented 6 years ago

/cc @cjihrig and @Trott perhaps?

stevestock commented 6 years ago

@arekinath

shahkashani commented 6 years ago

Bump <3 Would love to get this (and https://github.com/yarnpkg/yarn/issues/5477) fixed!

arekinath commented 6 years ago

Merged as 175758a, released in 1.14.2

ChALkeR commented 6 years ago

@arekinath Thanks!

BYK commented 6 years ago

@arekinath thanks a lot for the merge and release! It would be ideal if we can propagate this fix all the way to upstream. There are a few more fixes on joyent repos:

Do you think you can help with those too?

And since sshpk depends on asn1, we'd need to make another patch and release here once that PR is merged and released.