Fix deprecated Buffer constructor usage and add safeguards

[ChALkeR]

This avoids using deprecated Buffer constructor API on newer Node.js versions.

To achieve that, Buffer.from presence is checked, with validating that it's not the same method as Uint8Array.from.

Also, additional checks were added for older Node.js versions to ensure that a number is never accidentally passed to the Buffer constructor. Throwing is in line with browser atob/btoa behavior, and in line with what Buffer.from does on numbers in newer Node.js versions.

No actual security issues present in that code, the safeguard has been added preemptively to avoid accidential calls to atob/btoa(number) in the future.

Refs:
https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor

[kzc]

This PR is out of scope for uglify.

[ChALkeR]

@kzc Is minify.js an external lib that was checked in here? If so, could you point me to its upstream, please? I don't see that information anywhere, unfortunately. 😞

[kzc]

minify.js is part of uglify.

As long as the old Buffer constructor works there's no need to change it.

[ChALkeR]

@kzc If minify.js is part of UglifyJS2, then this is the correct place for this PR, and isn't out of scope.

1. I would argue that the safeguards are needed in place around the old Buffer API to make sure that unexpected things won't happen if any of those two functions (to_ascii/to_base64) would be called from some other code in minify.js/UglifyJS2 later.
2. Old Buffer API is expected to print runtime deprecation warnings soon, I filed this PR in advance so that this package won't get affected.

[ChALkeR]

@kzc Alternatively, if this looks too cumbersome, I would recommend to just use Buffer.from and drop the detection, fallback, and safeguards in fallback from this PR. This would be a -2 +2 change, then ;-).

It would imply dropping support for Node.js ≤ 4.5.0 then, but those shouldn't be used nowdays. Even the whole 4.x branch (v4.8.7 is the latest patch release there atm) would be out of maintenance LTS in about two months from now.

[alexlamsl]

@ChALkeR thanks for the contribution and the information about Node.js deprecation plans.

I think the best course of action would be to leave the PR open for now and wait for the deprecation to happen, since it sounds to me nothing would actually break.

FWIW, those two functions are only used for source-map related functionalities, so the majority use case of mangle & compress would not trigger any future warning messages.

[ChALkeR]

Tracking: nodejs/node#19079

[fabiosantoscode]

Node 10 is out now. This PR should be applied.

[ChALkeR]

Alternative approach would be to drop support for Node.js < 4.5.0, then this patch would become a trivial two-liner change with no line additions.

Note that the whole Node.js 4.x branch (including 4.9.x) would become unsupported iby upstream in several days.

Upd: ah, I already mentioned that.

[kzc]

No actual security issues present in that code, the safeguard has been added preemptively to avoid accidential calls to atob/btoa(number) in the future.

Accidental calls by a future uglify maintainer?

Does uglify-js error out or emit warnings to stderr when run on node 10 without this PR?

[ChALkeR]

@kzc

Accidental calls by a future uglify maintainer?

Accidential calls to to_ascii/to_base64 with invalid input in the future, by a future or current uglify maintainer. Because to_base64(number) currently does a very bad thing.

Does uglify-js error out or emit warnings to stderr when run on node 10 without this PR?

Testcase:

var UglifyJS = require(".");
var result = UglifyJS.minify({
  "file1.js": "var a = function() {};"
}, {
  sourceMap: { url: "inline" }
});
console.log(result);

This is what happens:

var a=function(){};
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZpbGUxLmpzIl0sIm5hbWVzIjpbImEiXSwibWFwcGluZ3MiOiJBQUFBLElBQUlBLEVBQUkifQ==
(node:15872) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

It's not an error, but it's a warning. That warning is currently not emitted when UglifyJS2 is placed inside a node_modules dir.

Running npm t for UglifyJS2 also prints that warning to stderr.

[goto-bus-stop]

although the function is called to_ascii, converting the buffer to ascii instead of utf8 breaks parsing of source maps with valid unicode characters in them. i think this branch should just do .toString() just like the one below.

e; only relevant if maintainers want to take this at all, of course :)

[ChALkeR]

Browser-side atob() and btoa() methods (used above) do not support non-ascii.

Refs:

• https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/atob
• https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/btoa

So parsing of source maps with valid unicode characters is already broken when this method is executed in a browser environment, and changing this single .toString() won't help much. I will update it, though, to keep the behavior closer to the original one.

[ChALkeR]

Done.