Skip to content

Commit

Permalink
assert: do not read Node.js modules
Browse files Browse the repository at this point in the history 
Prevent reading a Node.js module. That could theoretically lead to
false errors being thrown otherwise.

PR-URL: nodejs#18322
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
@BridgeAR @MayaLekova
BridgeAR authored and MayaLekova committed May 8, 2018
1 parent 10a390f commit acfbae6
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 50 deletions.
110 changes: 61 additions & 49 deletions lib/assert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand All @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
37 changes: 37 additions & 0 deletions test/parallel/test-assert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@
// 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 */

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) {
Expand Down Expand Up @@ -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'),
Expand Down

0 comments on commit acfbae6

Please sign in to comment.