From 3e910fb8f7bc42cfd9f114260322265d0ec00278 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 23 Jan 2018 16:45:28 +0100 Subject: [PATCH] assert: do not read Node.js modules Prevent reading a Node.js module. That could theoretically lead to false errors being thrown otherwise. PR-URL: https://github.com/nodejs/node/pull/18322 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung --- lib/assert.js | 110 +++++++++++++++++++---------------- lib/internal/errors.js | 3 +- test/parallel/test-assert.js | 37 ++++++++++++ 3 files changed, 100 insertions(+), 50 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 75d2473c486261..a6447278f0c59e 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -25,13 +25,13 @@ const { isDeepEqual, isDeepStrictEqual } = require('internal/util/comparisons'); -const { AssertionError, TypeError } = require('internal/errors'); +const { AssertionError, TypeError, errorCache } = require('internal/errors'); const { openSync, closeSync, readSync } = require('fs'); const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn'); const { inspect } = require('util'); const { EOL } = require('os'); +const nativeModule = require('native_module'); -const codeCache = new Map(); // Escape control characters but not \n and \t to keep the line breaks and // indentation intact. // eslint-disable-next-line no-control-regex @@ -146,6 +146,60 @@ function getBuffer(fd, assertLine) { return buffers; } +function getErrMessage(call) { + const filename = call.getFileName(); + const line = call.getLineNumber() - 1; + const column = call.getColumnNumber() - 1; + const identifier = `${filename}${line}${column}`; + + if (errorCache.has(identifier)) { + return errorCache.get(identifier); + } + + // Skip Node.js modules! + if (filename.endsWith('.js') && nativeModule.exists(filename.slice(0, -3))) { + errorCache.set(identifier, undefined); + return; + } + + var fd; + try { + fd = openSync(filename, 'r', 0o666); + const buffers = getBuffer(fd, line); + const code = Buffer.concat(buffers).toString('utf8'); + const nodes = parseExpressionAt(code, column); + // Node type should be "CallExpression" and some times + // "SequenceExpression". + const node = nodes.type === 'CallExpression' ? nodes : nodes.expressions[0]; + const name = node.callee.name; + // Calling `ok` with .apply or .call is uncommon but we use a simple + // safeguard nevertheless. + if (name !== 'apply' && name !== 'call') { + // Only use `assert` and `assert.ok` to reference the "real API" and + // not user defined function names. + const ok = name === 'ok' ? '.ok' : ''; + const args = node.arguments; + var message = code + .slice(args[0].start, args[args.length - 1].end) + .replace(escapeSequencesRegExp, escapeFn); + message = 'The expression evaluated to a falsy value:' + + `${EOL}${EOL} assert${ok}(${message})${EOL}`; + } + // Make sure to always set the cache! No matter if the message is + // undefined or not + errorCache.set(identifier, message); + + return message; + + } catch (e) { + // Invalidate cache to prevent trying to read this part again. + errorCache.set(identifier, undefined); + } finally { + if (fd !== undefined) + closeSync(fd); + } +} + function innerOk(args, fn) { var [value, message] = args; @@ -168,54 +222,12 @@ function innerOk(args, fn) { const call = err.stack[0]; Error.prepareStackTrace = tmpPrepare; - const filename = call.getFileName(); - const line = call.getLineNumber() - 1; - const column = call.getColumnNumber() - 1; - const identifier = `${filename}${line}${column}`; + // TODO(BridgeAR): fix the "generatedMessage property" + // Since this is actually a generated message, it has to be + // determined differently from now on. - if (codeCache.has(identifier)) { - message = codeCache.get(identifier); - } else { - var fd; - try { - fd = openSync(filename, 'r', 0o666); - const buffers = getBuffer(fd, line); - const code = Buffer.concat(buffers).toString('utf8'); - const nodes = parseExpressionAt(code, column); - // Node type should be "CallExpression" and some times - // "SequenceExpression". - const node = nodes.type === 'CallExpression' ? - nodes : - nodes.expressions[0]; - // TODO: fix the "generatedMessage property" - // Since this is actually a generated message, it has to be - // determined differently from now on. - - const name = node.callee.name; - // Calling `ok` with .apply or .call is uncommon but we use a simple - // safeguard nevertheless. - if (name !== 'apply' && name !== 'call') { - // Only use `assert` and `assert.ok` to reference the "real API" and - // not user defined function names. - const ok = name === 'ok' ? '.ok' : ''; - const args = node.arguments; - message = code - .slice(args[0].start, args[args.length - 1].end) - .replace(escapeSequencesRegExp, escapeFn); - message = 'The expression evaluated to a falsy value:' + - `${EOL}${EOL} assert${ok}(${message})${EOL}`; - } - // Make sure to always set the cache! No matter if the message is - // undefined or not - codeCache.set(identifier, message); - } catch (e) { - // Invalidate cache to prevent trying to read this part again. - codeCache.set(identifier, undefined); - } finally { - if (fd !== undefined) - closeSync(fd); - } - } + // Make sure it would be "null" in case that is used. + message = getErrMessage(call) || message; } innerFail({ actual: value, diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 3530d63710cf77..a41df135407341 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -398,7 +398,8 @@ module.exports = exports = { URIError: makeNodeError(URIError), AssertionError, SystemError, - E // This is exported only to facilitate testing. + E, // This is exported only to facilitate testing. + errorCache: new Map() // This is in here only to facilitate testing. }; // To declare an error message, use the E(sym, val) function above. The sym diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index b61df45b46f66b..e4784e0688173f 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -19,6 +19,8 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. +// Flags: --expose-internals + 'use strict'; /* eslint-disable prefer-common-expectserror */ @@ -26,6 +28,9 @@ const common = require('../common'); const assert = require('assert'); const { EOL } = require('os'); +const EventEmitter = require('events'); +const { errorCache } = require('internal/errors'); +const { writeFileSync, unlinkSync } = require('fs'); const a = assert; function makeBlock(f) { @@ -1019,6 +1024,38 @@ common.expectsError( } ); +// Do not try to check Node.js modules. +{ + const e = new EventEmitter(); + + e.on('hello', assert); + + let threw = false; + try { + e.emit('hello', false); + } catch (err) { + const frames = err.stack.split('\n'); + const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/); + // Reset the cache to check again + errorCache.delete(`${filename}${line - 1}${column - 1}`); + const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` + + 'ok(failed(badly));'; + try { + writeFileSync(filename, data); + assert.throws( + () => e.emit('hello', false), + { + message: 'false == true' + } + ); + threw = true; + } finally { + unlinkSync(filename); + } + } + assert(threw); +} + common.expectsError( // eslint-disable-next-line no-restricted-syntax () => assert.throws(() => {}, 'Error message', 'message'),