From 7eddd5e8859e427256342e82272f229a00b61b3a Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 29 Sep 2017 17:06:28 -0700 Subject: [PATCH 1/2] errors: make properties mutable Userland code can break if it depends on a mutable `code` property for errors. Allow users to change the `code` property but do not propagate changes to the error `name`. Additionally, make `message` and `name` consistent with `Error` object (non-enumerable). Test that `console.log()` and `.toString()` calls on internal `Error` objects with mutated properties have analogous results with the standard ECMAScript `Error` objects. Fixes: https://github.com/nodejs/node/issues/15658 --- lib/internal/errors.js | 16 +++++----- test/parallel/test-internal-errors.js | 45 +++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 48ef2dcc3dfa..e3c18b48c76f 100755 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -22,15 +22,13 @@ function makeNodeError(Base) { return class NodeError extends Base { constructor(key, ...args) { super(message(key, args)); - this[kCode] = key; - } - - get name() { - return `${super.name} [${this[kCode]}]`; - } - - get code() { - return this[kCode]; + this[kCode] = this.code = key; + Object.defineProperty(this, 'name', { + configurable: true, + enumerable: false, + value: `${super.name} [${this[kCode]}]`, + writable: true + }); } }; } diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 410c5aa97431..c16036c21d84 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -1,9 +1,9 @@ // Flags: --expose-internals 'use strict'; - const common = require('../common'); -const errors = require('internal/errors'); + const assert = require('assert'); +const errors = require('internal/errors'); function invalidKey(key) { return new RegExp(`^An invalid error message key was used: ${key}\\.$`); @@ -301,3 +301,44 @@ assert.strictEqual( 'The value "bar" is invalid for argument "foo"' ); } + +// Test that `code` property is mutable and that changing it does not change the +// name. +{ + const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + assert.strictEqual(myError.code, 'ERR_TLS_HANDSHAKE_TIMEOUT'); + const initialName = myError.name; + myError.code = 'FHQWHGADS'; + assert.strictEqual(myError.code, 'FHQWHGADS'); + assert.strictEqual(myError.name, initialName); + assert.ok(myError.name.includes('ERR_TLS_HANDSHAKE_TIMEOUT')); + assert.ok(!myError.name.includes('FHQWHGADS')); +} + +// Test that `name` and `message` are mutable and that changing them alters +// `toString()` but not `console.log()` results, which is the behavior of +// `Error` objects in the browser. +{ + function test(prop) { + let initialConsoleLog = ''; + common.hijackStdout((data) => { initialConsoleLog += data; }); + const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + const initialToString = myError.toString(); + console.log(myError); + assert.notStrictEqual(initialConsoleLog, ''); + + common.restoreStdout(); + + let subsequentConsoleLog = ''; + common.hijackStdout((data) => { subsequentConsoleLog += data; }); + myError[prop] = 'Fhqwhgads'; + assert.notStrictEqual(myError.toString(), initialToString); + console.log(myError); + assert.strictEqual(subsequentConsoleLog, initialConsoleLog); + + common.restoreStdout(); + } + + test('name'); + test('message'); +} From 9efeea3e0f05995c1b4459f0ef05d24a4e4e638a Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Sun, 1 Oct 2017 16:14:45 -0700 Subject: [PATCH 2/2] error: make `code` and `name` properties settable For internal errors, make `code` and `name` settable while keeping them non-own properties by default. Fixes: https://github.com/nodejs/node/issues/15658 --- lib/internal/errors.js | 36 ++++++++++++--- test/parallel/test-internal-errors.js | 64 ++++++++++++++++----------- 2 files changed, 69 insertions(+), 31 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index e3c18b48c76f..6b9c31a5286d 100755 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -11,9 +11,8 @@ const kCode = Symbol('code'); const messages = new Map(); -const { - kMaxLength -} = process.binding('buffer'); +const { kMaxLength } = process.binding('buffer'); +const { defineProperty } = Object; // Lazily loaded var util = null; @@ -22,11 +21,36 @@ function makeNodeError(Base) { return class NodeError extends Base { constructor(key, ...args) { super(message(key, args)); - this[kCode] = this.code = key; - Object.defineProperty(this, 'name', { + defineProperty(this, kCode, { configurable: true, enumerable: false, - value: `${super.name} [${this[kCode]}]`, + value: key, + writable: true + }); + } + + get name() { + return `${super.name} [${this[kCode]}]`; + } + + set name(value) { + defineProperty(this, 'name', { + configurable: true, + enumerable: true, + value, + writable: true + }); + } + + get code() { + return this[kCode]; + } + + set code(value) { + defineProperty(this, 'code', { + configurable: true, + enumerable: true, + value, writable: true }); } diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index c16036c21d84..84bdf765e572 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -307,38 +307,52 @@ assert.strictEqual( { const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); assert.strictEqual(myError.code, 'ERR_TLS_HANDSHAKE_TIMEOUT'); + assert.strictEqual(myError.hasOwnProperty('code'), false); + assert.strictEqual(myError.hasOwnProperty('name'), false); + assert.deepStrictEqual(Object.keys(myError), []); const initialName = myError.name; myError.code = 'FHQWHGADS'; assert.strictEqual(myError.code, 'FHQWHGADS'); assert.strictEqual(myError.name, initialName); + assert.deepStrictEqual(Object.keys(myError), ['code']); assert.ok(myError.name.includes('ERR_TLS_HANDSHAKE_TIMEOUT')); assert.ok(!myError.name.includes('FHQWHGADS')); } -// Test that `name` and `message` are mutable and that changing them alters -// `toString()` but not `console.log()` results, which is the behavior of -// `Error` objects in the browser. +// Test that `name` is mutable and that changing it alters `toString()` but not +// `console.log()` results, which is the behavior of `Error` objects in the +// browser. Note that `name` becomes enumerable after being assigned. { - function test(prop) { - let initialConsoleLog = ''; - common.hijackStdout((data) => { initialConsoleLog += data; }); - const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); - const initialToString = myError.toString(); - console.log(myError); - assert.notStrictEqual(initialConsoleLog, ''); - - common.restoreStdout(); - - let subsequentConsoleLog = ''; - common.hijackStdout((data) => { subsequentConsoleLog += data; }); - myError[prop] = 'Fhqwhgads'; - assert.notStrictEqual(myError.toString(), initialToString); - console.log(myError); - assert.strictEqual(subsequentConsoleLog, initialConsoleLog); - - common.restoreStdout(); - } - - test('name'); - test('message'); + const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + assert.deepStrictEqual(Object.keys(myError), []); + const initialToString = myError.toString(); + + myError.name = 'Fhqwhgads'; + assert.deepStrictEqual(Object.keys(myError), ['name']); + assert.notStrictEqual(myError.toString(), initialToString); +} + +// Test that `message` is mutable and that changing it alters `toString()` but +// not `console.log()` results, which is the behavior of `Error` objects in the +// browser. Note that `message` remains non-enumerable after being assigned. +{ + let initialConsoleLog = ''; + common.hijackStdout((data) => { initialConsoleLog += data; }); + const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + assert.deepStrictEqual(Object.keys(myError), []); + const initialToString = myError.toString(); + console.log(myError); + assert.notStrictEqual(initialConsoleLog, ''); + + common.restoreStdout(); + + let subsequentConsoleLog = ''; + common.hijackStdout((data) => { subsequentConsoleLog += data; }); + myError.message = 'Fhqwhgads'; + assert.deepStrictEqual(Object.keys(myError), []); + assert.notStrictEqual(myError.toString(), initialToString); + console.log(myError); + assert.strictEqual(subsequentConsoleLog, initialConsoleLog); + + common.restoreStdout(); }