From c0ca7742385c5a2e54b9dff18352842fddb2b5b1 Mon Sep 17 00:00:00 2001 From: Martin Gausby Date: Sun, 18 Jan 2015 23:16:35 -0800 Subject: [PATCH] Defend against throwing defined props in stringify If an object has a defined property, that is enumerable, and this property throws an error, it will make JSON stringify throw an error, and potentially bring down the program. The solution so far is to try-catch with the usual json stringifyer, that guards against circular references. If this throws an error we will attempt to guard against defined properties; and return [Throws] if a property throws an error when accesed. The following examples illustrate the problem: ```js var obj = {}; obj.__defineGetter__('foo', function() { throw new Error('ouch!'); }); JSON.stringify(obj.foo); // error thrown ``` And using `Object.defineProperty`: ```js var obj = {}; Object.defineProperty(obj, 'foo', { get: function() { throw new Error('ouch!'); } enumerable: true // enumerable is false by default }); JSON.stringify(obj.foo); // error thrown ``` The cases we have seen in production is third party modules that has enumerable getters that try to access properties on undefined objects. Fixes #182. --- lib/bunyan.js | 22 ++++++++++- package.json | 3 +- test/safe-json-stringify-1.js | 11 ++++++ test/safe-json-stringify-2.js | 11 ++++++ test/safe-json-stringify-3.js | 16 ++++++++ test/safe-json-stringify-4.js | 17 ++++++++ test/safe-json-stringify.test.js | 68 ++++++++++++++++++++++++++++++++ 7 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 test/safe-json-stringify-1.js create mode 100644 test/safe-json-stringify-2.js create mode 100644 test/safe-json-stringify-3.js create mode 100644 test/safe-json-stringify-4.js create mode 100644 test/safe-json-stringify.test.js diff --git a/lib/bunyan.js b/lib/bunyan.js index 725d9b9..f305684 100644 --- a/lib/bunyan.js +++ b/lib/bunyan.js @@ -38,6 +38,12 @@ try { } var EventEmitter = require('events').EventEmitter; +try { + var safeJsonStringify = require('safe-json-stringify'); +} catch (e) { + safeJsonStringify = null; +} + // The 'mv' module is required for rotating-file stream support. try { /* Use `+ ''` to hide this import from browserify. */ @@ -814,7 +820,21 @@ Logger.prototype._emit = function (rec, noemit) { // Stringify the object. Attempt to warn/recover on error. var str; if (noemit || this.haveNonRawStreams) { - str = JSON.stringify(rec, safeCycles()) + '\n'; + try { + str = JSON.stringify(rec, safeCycles()) + '\n'; + } + catch(err) { + if (safeJsonStringify) { + str = safeJsonStringify(rec) + '\n'; + } + else { + str = '(Could not JSON stringify data. See stderr for details.)'; + _warn(format('bunyan: ERROR: Could not JSON stringify a property. ' + + 'Please consider installing safe-json-stringify for more ' + + 'information. Stack trace: %s' + , err.stack)); + } + } } if (noemit) diff --git a/package.json b/package.json index 93bc35d..e120163 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,8 @@ "// comment2": "'mv' required for RotatingFileStream", "optionalDependencies": { "dtrace-provider": "~0.3 >0.3.0", - "mv": "~2" + "mv": "~2", + "safe-json-stringify": "1.0.1" }, "devDependencies": { "nodeunit": "0.9.*", diff --git a/test/safe-json-stringify-1.js b/test/safe-json-stringify-1.js new file mode 100644 index 0000000..04c8edf --- /dev/null +++ b/test/safe-json-stringify-1.js @@ -0,0 +1,11 @@ +var bunyan = require('../lib/bunyan'); + +var log = bunyan.createLogger({ + name: 'safe-json-stringify-1' +}); + +var obj = {}; +obj.__defineGetter__('boom', + function() { throw new Error('__defineGetter__ ouch!'); }); +log.info({obj: obj}, 'using __defineGetter__'); + diff --git a/test/safe-json-stringify-2.js b/test/safe-json-stringify-2.js new file mode 100644 index 0000000..79c1ca7 --- /dev/null +++ b/test/safe-json-stringify-2.js @@ -0,0 +1,11 @@ +process.env.BUNYAN_TEST_NO_SAFE_JSON_STRINGIFY = '1'; +var bunyan = require('../lib/bunyan'); + +var log = bunyan.createLogger({ + name: 'safe-json-stringify-2' +}); + +var obj = {}; +obj.__defineGetter__('boom', + function() { throw new Error('__defineGetter__ ouch!'); }); +log.info({obj: obj}, 'using __defineGetter__'); diff --git a/test/safe-json-stringify-3.js b/test/safe-json-stringify-3.js new file mode 100644 index 0000000..87bf120 --- /dev/null +++ b/test/safe-json-stringify-3.js @@ -0,0 +1,16 @@ +var bunyan = require('../lib/bunyan'); + +var log = bunyan.createLogger({ + name: 'safe-json-stringify-3' +}); + +// And using `Object.defineProperty`. +var obj = {}; +Object.defineProperty(obj, 'boom', { + get: function() { throw new Error('defineProperty ouch!'); }, + enumerable: true // enumerable is false by default +}); +// Twice to test the 'warnKey' usage. +for (var i = 0; i < 2; i++) { + log.info({obj: obj}, 'using defineProperty'); +} diff --git a/test/safe-json-stringify-4.js b/test/safe-json-stringify-4.js new file mode 100644 index 0000000..85f94d4 --- /dev/null +++ b/test/safe-json-stringify-4.js @@ -0,0 +1,17 @@ +process.env.BUNYAN_TEST_NO_SAFE_JSON_STRINGIFY = '1'; +var bunyan = require('../lib/bunyan'); + +var log = bunyan.createLogger({ + name: 'safe-json-stringify-4' +}); + +// And using `Object.defineProperty`. +var obj = {}; +Object.defineProperty(obj, 'boom', { + get: function() { throw new Error('defineProperty ouch!'); }, + enumerable: true // enumerable is false by default +}); +// Twice to test the 'warnKey' usage. +for (var i = 0; i < 2; i++) { + log.info({obj: obj}, 'using defineProperty'); +} diff --git a/test/safe-json-stringify.test.js b/test/safe-json-stringify.test.js new file mode 100644 index 0000000..918f20d --- /dev/null +++ b/test/safe-json-stringify.test.js @@ -0,0 +1,68 @@ +/* + * Copyright (c) 2015 Trent Mick. All rights reserved. + * + * If available, use `safe-json-stringfy` as a fallback stringifier. + * This covers the case where an enumerable property throws an error + * in its getter. + * + * See + */ + +var p = console.warn; +var exec = require('child_process').exec; + +// node-tap API +if (require.cache[__dirname + '/tap4nodeunit.js']) + delete require.cache[__dirname + '/tap4nodeunit.js']; +var tap4nodeunit = require('./tap4nodeunit.js'); +var after = tap4nodeunit.after; +var before = tap4nodeunit.before; +var test = tap4nodeunit.test; + + +test('__defineGetter__ boom', function (t) { + var cmd = process.execPath + ' ' + __dirname + '/safe-json-stringify-1.js'; + exec(cmd, function (err, stdout, stderr) { + t.ifError(err, err); + var rec = JSON.parse(stdout.trim()); + t.equal(rec.obj.boom, '[Throws]'); + t.end(); + }); +}); + +test('__defineGetter__ boom, without safe-json-stringify', function (t) { + var cmd = process.execPath + ' ' + __dirname + '/safe-json-stringify-2.js'; + exec(cmd, function (err, stdout, stderr) { + t.ifError(err, err); + t.ok(stdout.indexOf('Exception in JSON.stringify') !== -1); + t.ok(stderr.indexOf( + 'You can install the "safe-json-stringify" module') !== -1); + t.end(); + }); +}); + +test('defineProperty boom', function (t) { + var cmd = process.execPath + ' ' + __dirname + '/safe-json-stringify-3.js'; + exec(cmd, function (err, stdout, stderr) { + t.ifError(err, err); + var recs = stdout.trim().split(/\n/g); + t.equal(recs.length, 2); + var rec = JSON.parse(recs[0]); + t.equal(rec.obj.boom, '[Throws]'); + t.end(); + }); +}); + +test('defineProperty boom, without safe-json-stringify', function (t) { + var cmd = process.execPath + ' ' + __dirname + '/safe-json-stringify-4.js'; + exec(cmd, function (err, stdout, stderr) { + t.ifError(err, err); + t.ok(stdout.indexOf('Exception in JSON.stringify') !== -1); + t.equal(stdout.match(/Exception in JSON.stringify/g).length, 2); + t.ok(stderr.indexOf( + 'You can install the "safe-json-stringify" module') !== -1); + t.equal(stderr.match( + /You can install the "safe-json-stringify" module/g).length, 1); + t.end(); + }); +});