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.
This commit is contained in:
Martin Gausby 2015-01-18 23:16:35 -08:00 committed by Trent Mick
parent a96a1d50b3
commit c0ca774238
7 changed files with 146 additions and 2 deletions

View file

@ -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,8 +820,22 @@ Logger.prototype._emit = function (rec, noemit) {
// Stringify the object. Attempt to warn/recover on error.
var str;
if (noemit || this.haveNonRawStreams) {
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)
return str;

View file

@ -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.*",

View file

@ -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__');

View file

@ -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__');

View file

@ -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');
}

View file

@ -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');
}

View file

@ -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 <https://github.com/trentm/node-bunyan/pull/182>
*/
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();
});
});