From 332271dbbb6c0823f3382b891cafeed273940f91 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 22 Mar 2018 00:30:21 +0100
Subject: [PATCH] buffer: do deprecation warning outside `node_modules`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/nodejs/node/issues/19079#issuecomment-375121443
PR-URL: https://github.com/nodejs/node/pull/19524
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
---
 doc/api/buffer.md                             | 20 +++++++
 doc/api/deprecations.md                       |  6 +-
 lib/buffer.js                                 | 31 +++++-----
 lib/internal/util.js                          | 56 +++++++++++++++++++
 ...t-buffer-constructor-node-modules-paths.js | 35 ++++++++++++
 ...buffer-constructor-outside-node-modules.js | 29 ++++++++++
 6 files changed, 163 insertions(+), 14 deletions(-)
 create mode 100644 test/parallel/test-buffer-constructor-node-modules-paths.js
 create mode 100644 test/parallel/test-buffer-constructor-outside-node-modules.js

diff --git a/doc/api/buffer.md b/doc/api/buffer.md
index eb3b93e0f0c3a8..402e3684713ea6 100644
--- a/doc/api/buffer.md
+++ b/doc/api/buffer.md
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index ba9580a7f5cca5..38751b30be5a1c 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -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.
@@ -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
 
diff --git a/lib/buffer.js b/lib/buffer.js
index 73e3bc44209ee4..0737b8eccc5584 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -48,6 +48,7 @@ try {
 }
 const {
   customInspectSymbol,
+  isInsideNodeModules,
   normalizeEncoding,
   kIsEncodingSymbol
 } = require('internal/util');
@@ -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
@@ -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') {
diff --git a/lib/internal/util.js b/lib/internal/util.js
index 247ac2dd5a7287..b92e4c5b588479 100644
--- a/lib/internal/util.js
+++ b/lib/internal/util.js
@@ -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,
@@ -379,6 +434,7 @@ module.exports = {
   getSystemErrorName,
   getIdentificationOf,
   isError,
+  isInsideNodeModules,
   join,
   normalizeEncoding,
   objectToString,
diff --git a/test/parallel/test-buffer-constructor-node-modules-paths.js b/test/parallel/test-buffer-constructor-node-modules-paths.js
new file mode 100644
index 00000000000000..717f7835114f0d
--- /dev/null
+++ b/test/parallel/test-buffer-constructor-node-modules-paths.js
@@ -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);
diff --git a/test/parallel/test-buffer-constructor-outside-node-modules.js b/test/parallel/test-buffer-constructor-outside-node-modules.js
new file mode 100644
index 00000000000000..1a1e146f2dd9d7
--- /dev/null
+++ b/test/parallel/test-buffer-constructor-outside-node-modules.js
@@ -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'
+});