Skip to content

Commit 9d4ab90

Browse files
addaleaxBridgeAR
addaleax
authored and
BridgeAR
committed
buffer: do deprecation warning outside node_modules
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. PR-URL: #19524 Refs: #19079 (comment) 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]>
1 parent e048b15 commit 9d4ab90

6 files changed

Lines changed: 163 additions & 14 deletions

File tree

‎doc/api/buffer.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,10 @@ It can be constructed in a variety of ways.
304304
<!-- YAML
305305
deprecated: v6.0.0
306306
changes:
307+
- version: REPLACEME
308+
pr-url: https://github.com/nodejs/node/pull/19524
309+
description: Calling this constructor emits a deprecation warning when
310+
run from code outside the `node_modules` directory.
307311
- version: v7.2.1
308312
pr-url: https://github.com/nodejs/node/pull/9529
309313
description: Calling this constructor no longer emits a deprecation warning.
@@ -328,6 +332,10 @@ const buf = new Buffer([0x62, 0x75, 0x66, 0x66, 0x65, 0x72]);
328332
added: v3.0.0
329333
deprecated: v6.0.0
330334
changes:
335+
- version: REPLACEME
336+
pr-url: https://github.com/nodejs/node/pull/19524
337+
description: Calling this constructor emits a deprecation warning when
338+
run from code outside the `node_modules` directory.
331339
- version: v7.2.1
332340
pr-url: https://github.com/nodejs/node/pull/9529
333341
description: Calling this constructor no longer emits a deprecation warning.
@@ -380,6 +388,10 @@ console.log(buf);
380388
<!-- YAML
381389
deprecated: v6.0.0
382390
changes:
391+
- version: REPLACEME
392+
pr-url: https://github.com/nodejs/node/pull/19524
393+
description: Calling this constructor emits a deprecation warning when
394+
run from code outside the `node_modules` directory.
383395
- version: v7.2.1
384396
pr-url: https://github.com/nodejs/node/pull/9529
385397
description: Calling this constructor no longer emits a deprecation warning.
@@ -410,6 +422,10 @@ console.log(buf2.toString());
410422
<!-- YAML
411423
deprecated: v6.0.0
412424
changes:
425+
- version: REPLACEME
426+
pr-url: https://github.com/nodejs/node/pull/19524
427+
description: Calling this constructor emits a deprecation warning when
428+
run from code outside the `node_modules` directory.
413429
- version: v8.0.0
414430
pr-url: https://github.com/nodejs/node/pull/12141
415431
description: new Buffer(size) will return zero-filled memory by default.
@@ -447,6 +463,10 @@ console.log(buf);
447463
<!-- YAML
448464
deprecated: v6.0.0
449465
changes:
466+
- version: REPLACEME
467+
pr-url: https://github.com/nodejs/node/pull/19524
468+
description: Calling this constructor emits a deprecation warning when
469+
run from code outside the `node_modules` directory.
450470
- version: v7.2.1
451471
pr-url: https://github.com/nodejs/node/pull/9529
452472
description: Calling this constructor no longer emits a deprecation warning.

‎doc/api/deprecations.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ be used.
7272
<a id="DEP0005"></a>
7373
### DEP0005: Buffer() constructor
7474

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

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

96+
As of REPLACEME, a deprecation warning is printed at runtime when
97+
`--pending-deprecation` is used or when the calling code is
98+
outside `node_modules` in order to better target developers, rather than users.
99+
96100
<a id="DEP0006"></a>
97101
### DEP0006: child\_process options.customFds
98102

‎lib/buffer.js‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ try {
4848
}
4949
const {
5050
customInspectSymbol,
51+
isInsideNodeModules,
5152
normalizeEncoding,
5253
kIsEncodingSymbol
5354
} = require('internal/util');
@@ -139,25 +140,29 @@ function alignPool() {
139140
}
140141
}
141142

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

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

157-
const doFlaggedDeprecation =
158-
pendingDeprecation ?
159-
showFlaggedDeprecation :
160-
function() {};
163+
process.emitWarning(bufferWarning, 'DeprecationWarning', 'DEP0005');
164+
bufferWarningAlreadyEmitted = true;
165+
}
161166

162167
/**
163168
* The Buffer() constructor is deprecated in documentation and should not be
@@ -170,7 +175,7 @@ const doFlaggedDeprecation =
170175
* Deprecation Code: DEP0005
171176
*/
172177
function Buffer(arg, encodingOrOffset, length) {
173-
doFlaggedDeprecation();
178+
showFlaggedDeprecation();
174179
// Common case.
175180
if (typeof arg === 'number') {
176181
if (typeof encodingOrOffset === 'string') {

‎lib/internal/util.js‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,61 @@ function spliceOne(list, index) {
366366
list.pop();
367367
}
368368

369+
const kNodeModulesRE = /^(.*)[\\/]node_modules[\\/]/;
370+
371+
let getStructuredStack;
372+
let mainPrefix;
373+
374+
function isInsideNodeModules() {
375+
// Match the prefix of the main file, if it is inside a `node_modules` folder,
376+
// up to and including the innermost `/node_modules/` bit, so that
377+
// we can later ignore files which are at the same node_modules level.
378+
// For example, having the main script at `/c/node_modules/d/d.js`
379+
// would match all code coming from `/c/node_modules/`.
380+
if (mainPrefix === undefined) {
381+
const match = process.mainModule &&
382+
process.mainModule.filename &&
383+
process.mainModule.filename.match(kNodeModulesRE);
384+
mainPrefix = match ? match[0] : '';
385+
}
386+
387+
if (getStructuredStack === undefined) {
388+
// Lazy-load to avoid a circular dependency.
389+
const { runInNewContext } = require('vm');
390+
// Use `runInNewContext()` to get something tamper-proof and
391+
// side-effect-free. Since this is currently only used for a deprecated API,
392+
// the perf implications should be okay.
393+
getStructuredStack = runInNewContext('(' + function() {
394+
Error.prepareStackTrace = function(err, trace) {
395+
err.stack = trace;
396+
};
397+
Error.stackTraceLimit = Infinity;
398+
399+
return function structuredStack() {
400+
// eslint-disable-next-line no-restricted-syntax
401+
return new Error().stack;
402+
};
403+
} + ')()', {}, { filename: 'structured-stack' });
404+
}
405+
406+
const stack = getStructuredStack();
407+
408+
// Iterate over all stack frames and look for the first one not coming
409+
// from inside Node.js itself:
410+
for (const frame of stack) {
411+
const filename = frame.getFileName();
412+
// If a filename does not start with / or contain \,
413+
// it's likely from Node.js core.
414+
if (!/^\/|\\/.test(filename))
415+
continue;
416+
const match = filename.match(kNodeModulesRE);
417+
return !!match && match[0] !== mainPrefix;
418+
}
419+
420+
return false; // This should be unreachable.
421+
}
422+
423+
369424
module.exports = {
370425
assertCrypto,
371426
cachedResult,
@@ -379,6 +434,7 @@ module.exports = {
379434
getSystemErrorName,
380435
getIdentificationOf,
381436
isError,
437+
isInsideNodeModules,
382438
join,
383439
normalizeEncoding,
384440
objectToString,

‎test/parallel/test-buffer-constructor-node-modules-paths.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
'use strict';
2+
3+
const child_process = require('child_process');
4+
const assert = require('assert');
5+
const common = require('../common');
6+
7+
if (process.env.NODE_PENDING_DEPRECATION)
8+
common.skip('test does not work when NODE_PENDING_DEPRECATION is set');
9+
10+
function test(main, callSite, expected) {
11+
const { stderr } = child_process.spawnSync(process.execPath, ['-p', `
12+
process.mainModule = { filename: ${JSON.stringify(main)} };
13+
14+
vm.runInNewContext('new Buffer(10)', { Buffer }, {
15+
filename: ${JSON.stringify(callSite)}
16+
});`], { encoding: 'utf8' });
17+
if (expected)
18+
assert(stderr.includes('[DEP0005] DeprecationWarning'), stderr);
19+
else
20+
assert.strictEqual(stderr.trim(), '');
21+
}
22+
23+
test('/a/node_modules/b.js', '/a/node_modules/x.js', true);
24+
test('/a/node_modules/b.js', '/a/node_modules/foo/node_modules/x.js', false);
25+
test('/a/node_modules/foo/node_modules/b.js', '/a/node_modules/x.js', false);
26+
test('/node_modules/foo/b.js', '/node_modules/foo/node_modules/x.js', false);
27+
test('/a.js', '/b.js', true);
28+
test('/a.js', '/node_modules/b.js', false);
29+
test('c:\\a\\node_modules\\b.js', 'c:\\a\\node_modules\\x.js', true);
30+
test('c:\\a\\node_modules\\b.js',
31+
'c:\\a\\node_modules\\foo\\node_modules\\x.js', false);
32+
test('c:\\node_modules\\foo\\b.js',
33+
'c:\\node_modules\\foo\\node_modules\\x.js', false);
34+
test('c:\\a.js', 'c:\\b.js', true);
35+
test('c:\\a.js', 'c:\\node_modules\\b.js', false);

‎test/parallel/test-buffer-constructor-outside-node-modules.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Flags: --no-warnings
2+
'use strict';
3+
4+
const vm = require('vm');
5+
const assert = require('assert');
6+
const common = require('../common');
7+
8+
if (new Error().stack.includes('node_modules'))
9+
common.skip('test does not work when inside `node_modules` directory');
10+
if (process.env.NODE_PENDING_DEPRECATION)
11+
common.skip('test does not work when NODE_PENDING_DEPRECATION is set');
12+
13+
const bufferWarning = 'Buffer() is deprecated due to security and usability ' +
14+
'issues. Please use the Buffer.alloc(), ' +
15+
'Buffer.allocUnsafe(), or Buffer.from() methods instead.';
16+
17+
process.addListener('warning', common.mustCall((warning) => {
18+
assert(warning.stack.includes('this_should_emit_a_warning'), warning.stack);
19+
}));
20+
21+
vm.runInNewContext('new Buffer(10)', { Buffer }, {
22+
filename: '/a/node_modules/b'
23+
});
24+
25+
common.expectWarning('DeprecationWarning', bufferWarning, 'DEP0005');
26+
27+
vm.runInNewContext('new Buffer(10)', { Buffer }, {
28+
filename: '/this_should_emit_a_warning'
29+
});

0 commit comments

Comments
 (0)