From 74bf73b499af391072ba56a3e5d0bbfa91b7a7d7 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 23 Jan 2018 16:45:28 +0100 Subject: [PATCH 1/2] assert: do not read Node.js modules Prevent reading a Node.js module. That could theoretically lead to false errors being thrown otherwise. --- 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 8c38cfcd7a58a8..da9d7d6ff41f8c 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 @@ -134,6 +134,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; @@ -156,54 +210,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'), From 16f1e48dccc643a3cd29c5010df0147747218d2b Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 31 Jan 2018 15:43:09 +0100 Subject: [PATCH 2/2] assert: fix generatedMessage The generatedMessage was wrong in case simple assert was used and a message was auto generated. This fixed the TODO. --- lib/assert.js | 15 ++++++++++----- test/parallel/test-assert.js | 8 ++++++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index da9d7d6ff41f8c..bf840e876073d4 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -192,7 +192,10 @@ function innerOk(args, fn) { var [value, message] = args; if (!value) { + let generatedMessage = false; + if (args.length === 0) { + generatedMessage = true; message = 'No value argument passed to `assert.ok()`'; } else if (message == null) { // Use the call as error message if possible. @@ -210,20 +213,22 @@ function innerOk(args, fn) { const call = err.stack[0]; Error.prepareStackTrace = tmpPrepare; - // TODO(BridgeAR): fix the "generatedMessage property" - // Since this is actually a generated message, it has to be - // determined differently from now on. - // Make sure it would be "null" in case that is used. message = getErrMessage(call) || message; + generatedMessage = true; + } else if (message instanceof Error) { + throw message; } - innerFail({ + + const err = new AssertionError({ actual: value, expected: true, message, operator: '==', stackStartFn: fn }); + err.generatedMessage = generatedMessage; + throw err; } } diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index e4784e0688173f..e422acbbfbcddd 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -919,6 +919,7 @@ common.expectsError( { code: 'ERR_ASSERTION', type: assert.AssertionError, + generatedMessage: true, message: `The expression evaluated to a falsy value:${EOL}${EOL} ` + `assert.ok(null)${EOL}` } @@ -928,6 +929,7 @@ common.expectsError( { code: 'ERR_ASSERTION', type: assert.AssertionError, + generatedMessage: true, message: `The expression evaluated to a falsy value:${EOL}${EOL} ` + `assert(typeof 123 === 'string')${EOL}` } @@ -1011,7 +1013,8 @@ common.expectsError( { code: 'ERR_ASSERTION', type: assert.AssertionError, - message: '0 == true' + message: '0 == true', + generatedMessage: true } ); @@ -1020,7 +1023,8 @@ common.expectsError( { code: 'ERR_ASSERTION', type: assert.AssertionError, - message: 'test' + message: 'test', + generatedMessage: false } );