Skip to content

Commit

Permalink
buffer: do deprecation warning outside node_modules
Browse files Browse the repository at this point in the history
In addition to `--pending-deprecation`, emit a deprecation warning
for usage of the `Buffer()` constructor for call sites that are outside
of `node_modules`.

The goal of this is to better target developers, rather than
burdening users with an omnipresent and quickly ignored warning.

This implements the result of a TSC meeting discussion
from March 22, 2018.

Refs: #19079 (comment)
PR-URL: #19524
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
  • Loading branch information
addaleax committed Apr 4, 2018
1 parent 3567ea0 commit 332271d
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 14 deletions.
20 changes: 20 additions & 0 deletions doc/api/buffer.md
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,10 @@ It can be constructed in a variety of ways.
<!-- YAML
deprecated: v6.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/19524
description: Calling this constructor emits a deprecation warning when
run from code outside the `node_modules` directory.
- version: v7.2.1
pr-url: https://github.com/nodejs/node/pull/9529
description: Calling this constructor no longer emits a deprecation warning.
Expand All @@ -328,6 +332,10 @@ const buf = new Buffer([0x62, 0x75, 0x66, 0x66, 0x65, 0x72]);
added: v3.0.0
deprecated: v6.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/19524
description: Calling this constructor emits a deprecation warning when
run from code outside the `node_modules` directory.
- version: v7.2.1
pr-url: https://github.com/nodejs/node/pull/9529
description: Calling this constructor no longer emits a deprecation warning.
Expand Down Expand Up @@ -380,6 +388,10 @@ console.log(buf);
<!-- YAML
deprecated: v6.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/19524
description: Calling this constructor emits a deprecation warning when
run from code outside the `node_modules` directory.
- version: v7.2.1
pr-url: https://github.com/nodejs/node/pull/9529
description: Calling this constructor no longer emits a deprecation warning.
Expand Down Expand Up @@ -410,6 +422,10 @@ console.log(buf2.toString());
<!-- YAML
deprecated: v6.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/19524
description: Calling this constructor emits a deprecation warning when
run from code outside the `node_modules` directory.
- version: v8.0.0
pr-url: https://github.com/nodejs/node/pull/12141
description: new Buffer(size) will return zero-filled memory by default.
Expand Down Expand Up @@ -447,6 +463,10 @@ console.log(buf);
<!-- YAML
deprecated: v6.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/19524
description: Calling this constructor emits a deprecation warning when
run from code outside the `node_modules` directory.
- version: v7.2.1
pr-url: https://github.com/nodejs/node/pull/9529
description: Calling this constructor no longer emits a deprecation warning.
Expand Down
6 changes: 5 additions & 1 deletion doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ be used.
<a id="DEP0005"></a>
### DEP0005: Buffer() constructor

Type: Documentation-only (supports [`--pending-deprecation`][])
Type: Runtime (supports [`--pending-deprecation`][])

The `Buffer()` function and `new Buffer()` constructor are deprecated due to
API usability issues that can potentially lead to accidental security issues.
Expand All @@ -93,6 +93,10 @@ is strongly recommended:
* [`Buffer.from(string[, encoding])`][from_string_encoding] - Create a `Buffer`
that copies `string`.

As of REPLACEME, a deprecation warning is printed at runtime when
`--pending-deprecation` is used or when the calling code is
outside `node_modules` in order to better target developers, rather than users.

<a id="DEP0006"></a>
### DEP0006: child\_process options.customFds

Expand Down
31 changes: 18 additions & 13 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ try {
}
const {
customInspectSymbol,
isInsideNodeModules,
normalizeEncoding,
kIsEncodingSymbol
} = require('internal/util');
Expand Down Expand Up @@ -139,25 +140,29 @@ function alignPool() {
}
}

var bufferWarn = true;
let bufferWarningAlreadyEmitted = false;
let nodeModulesCheckCounter = 0;
const bufferWarning = 'Buffer() is deprecated due to security and usability ' +
'issues. Please use the Buffer.alloc(), ' +
'Buffer.allocUnsafe(), or Buffer.from() methods instead.';

function showFlaggedDeprecation() {
if (bufferWarn) {
// This is a *pending* deprecation warning. It is not emitted by
// default unless the --pending-deprecation command-line flag is
// used or the NODE_PENDING_DEPRECATION=1 env var is set.
process.emitWarning(bufferWarning, 'DeprecationWarning', 'DEP0005');
bufferWarn = false;
if (bufferWarningAlreadyEmitted ||
++nodeModulesCheckCounter > 10000 ||
(!pendingDeprecation &&
isInsideNodeModules())) {
// We don't emit a warning, because we either:
// - Already did so, or
// - Already checked too many times whether a call is coming
// from node_modules and want to stop slowing down things, or
// - We aren't running with `--pending-deprecation` enabled,
// and the code is inside `node_modules`.
return;
}
}

const doFlaggedDeprecation =
pendingDeprecation ?
showFlaggedDeprecation :
function() {};
process.emitWarning(bufferWarning, 'DeprecationWarning', 'DEP0005');
bufferWarningAlreadyEmitted = true;
}

/**
* The Buffer() constructor is deprecated in documentation and should not be
Expand All @@ -170,7 +175,7 @@ const doFlaggedDeprecation =
* Deprecation Code: DEP0005
*/
function Buffer(arg, encodingOrOffset, length) {
doFlaggedDeprecation();
showFlaggedDeprecation();
// Common case.
if (typeof arg === 'number') {
if (typeof encodingOrOffset === 'string') {
Expand Down
56 changes: 56 additions & 0 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,61 @@ function spliceOne(list, index) {
list.pop();
}

const kNodeModulesRE = /^(.*)[\\/]node_modules[\\/]/;

let getStructuredStack;
let mainPrefix;

function isInsideNodeModules() {
// Match the prefix of the main file, if it is inside a `node_modules` folder,
// up to and including the innermost `/node_modules/` bit, so that
// we can later ignore files which are at the same node_modules level.
// For example, having the main script at `/c/node_modules/d/d.js`
// would match all code coming from `/c/node_modules/`.
if (mainPrefix === undefined) {
const match = process.mainModule &&
process.mainModule.filename &&
process.mainModule.filename.match(kNodeModulesRE);
mainPrefix = match ? match[0] : '';
}

if (getStructuredStack === undefined) {
// Lazy-load to avoid a circular dependency.
const { runInNewContext } = require('vm');
// Use `runInNewContext()` to get something tamper-proof and
// side-effect-free. Since this is currently only used for a deprecated API,
// the perf implications should be okay.
getStructuredStack = runInNewContext('(' + function() {
Error.prepareStackTrace = function(err, trace) {
err.stack = trace;
};
Error.stackTraceLimit = Infinity;

return function structuredStack() {
// eslint-disable-next-line no-restricted-syntax
return new Error().stack;
};
} + ')()', {}, { filename: 'structured-stack' });
}

const stack = getStructuredStack();

// Iterate over all stack frames and look for the first one not coming
// from inside Node.js itself:
for (const frame of stack) {
const filename = frame.getFileName();
// If a filename does not start with / or contain \,
// it's likely from Node.js core.
if (!/^\/|\\/.test(filename))
continue;
const match = filename.match(kNodeModulesRE);
return !!match && match[0] !== mainPrefix;
}

return false; // This should be unreachable.
}


module.exports = {
assertCrypto,
cachedResult,
Expand All @@ -379,6 +434,7 @@ module.exports = {
getSystemErrorName,
getIdentificationOf,
isError,
isInsideNodeModules,
join,
normalizeEncoding,
objectToString,
Expand Down
35 changes: 35 additions & 0 deletions test/parallel/test-buffer-constructor-node-modules-paths.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';

const child_process = require('child_process');
const assert = require('assert');
const common = require('../common');

if (process.env.NODE_PENDING_DEPRECATION)
common.skip('test does not work when NODE_PENDING_DEPRECATION is set');

function test(main, callSite, expected) {
const { stderr } = child_process.spawnSync(process.execPath, ['-p', `
process.mainModule = { filename: ${JSON.stringify(main)} };
vm.runInNewContext('new Buffer(10)', { Buffer }, {
filename: ${JSON.stringify(callSite)}
});`], { encoding: 'utf8' });
if (expected)
assert(stderr.includes('[DEP0005] DeprecationWarning'), stderr);
else
assert.strictEqual(stderr.trim(), '');
}

test('/a/node_modules/b.js', '/a/node_modules/x.js', true);
test('/a/node_modules/b.js', '/a/node_modules/foo/node_modules/x.js', false);
test('/a/node_modules/foo/node_modules/b.js', '/a/node_modules/x.js', false);
test('/node_modules/foo/b.js', '/node_modules/foo/node_modules/x.js', false);
test('/a.js', '/b.js', true);
test('/a.js', '/node_modules/b.js', false);
test('c:\\a\\node_modules\\b.js', 'c:\\a\\node_modules\\x.js', true);
test('c:\\a\\node_modules\\b.js',
'c:\\a\\node_modules\\foo\\node_modules\\x.js', false);
test('c:\\node_modules\\foo\\b.js',
'c:\\node_modules\\foo\\node_modules\\x.js', false);
test('c:\\a.js', 'c:\\b.js', true);
test('c:\\a.js', 'c:\\node_modules\\b.js', false);
29 changes: 29 additions & 0 deletions test/parallel/test-buffer-constructor-outside-node-modules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Flags: --no-warnings
'use strict';

const vm = require('vm');
const assert = require('assert');
const common = require('../common');

if (new Error().stack.includes('node_modules'))
common.skip('test does not work when inside `node_modules` directory');
if (process.env.NODE_PENDING_DEPRECATION)
common.skip('test does not work when NODE_PENDING_DEPRECATION is set');

const bufferWarning = 'Buffer() is deprecated due to security and usability ' +
'issues. Please use the Buffer.alloc(), ' +
'Buffer.allocUnsafe(), or Buffer.from() methods instead.';

process.addListener('warning', common.mustCall((warning) => {
assert(warning.stack.includes('this_should_emit_a_warning'), warning.stack);
}));

vm.runInNewContext('new Buffer(10)', { Buffer }, {
filename: '/a/node_modules/b'
});

common.expectWarning('DeprecationWarning', bufferWarning, 'DEP0005');

vm.runInNewContext('new Buffer(10)', { Buffer }, {
filename: '/this_should_emit_a_warning'
});

0 comments on commit 332271d

Please sign in to comment.