Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicating message objects to avoid modifying input #1938

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/winston/create-logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ module.exports = function (opts = {}) {
// Optimize the hot-path which is the single object.
if (args.length === 1) {
const [msg] = args;
const info = msg && msg.message && msg || { message: msg };
// NOTE: If `msg` is an Object, clone it so changes made, such as the addition of the level field,
// do not affect the original object.
const info = msg && msg.message && { ...msg } || { message: msg };
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

structuredClone can solve this issue assuming support for it aligns with Winston's minimum supported platforms.

info.level = info[LEVEL] = level;
self._addDefaultMeta(info);
self.write(info);
Expand Down
3 changes: 3 additions & 0 deletions lib/winston/logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ class Logger extends Transform {
// Slightly less hotpath, but worth optimizing for.
if (arguments.length === 2) {
if (msg && typeof msg === 'object') {
// NOTE: If `msg` is an Object, clone it so changes made, such as the addition of the level field,
// do not affect the original object.
msg = { ...msg };
msg[LEVEL] = msg.level = level;
this._addDefaultMeta(msg);
this.write(msg);
Expand Down
51 changes: 36 additions & 15 deletions test/logger.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('Logger', function () {

Object.keys(winston.config.syslog.levels).forEach(level => {
assume(logger[level]).is.a('function');
})
});
});

it('new Logger({ levels }) custom methods are not bound to instance', function (done) {
Expand All @@ -74,16 +74,16 @@ describe('Logger', function () {
transports: []
});

let logs = [];
let extendedLogger = Object.create(logger, {
const logs = [];
const extendedLogger = Object.create(logger, {
write: {
value: function(...args) {
value: function (...args) {
logs.push(args);
if (logs.length === 4) {
assume(logs.length).is.eql(4);
assume(logs[0]).is.eql([{ test: 1, level: 'info' }]);
assume(logs[1]).is.eql([{ test: 2, level: 'warn' }]);
assume(logs[2]).is.eql([{ message: 'test3', level: 'info' }])
assume(logs[2]).is.eql([{ message: 'test3', level: 'info' }]);
assume(logs[3]).is.eql([{ with: 'meta',
test: 4,
level: 'warn',
Expand Down Expand Up @@ -160,7 +160,7 @@ describe('Logger', function () {

it('.configure({ transports, format })', function () {
var logger = winston.createLogger(),
format = logger.format;
format = logger.format;

assume(logger.transports.length).equals(0);

Expand All @@ -177,7 +177,7 @@ describe('Logger', function () {
it('.remove() [transport not added]', function () {
var transports = [
new winston.transports.Console(),
new winston.transports.File({ filename: path.join(__dirname, 'fixtures', 'logs', 'filelog.log' )})
new winston.transports.File({ filename: path.join(__dirname, 'fixtures', 'logs', 'filelog.log') })
];

var logger = winston.createLogger({ transports: transports })
Expand All @@ -193,7 +193,7 @@ describe('Logger', function () {
it('.remove() [TransportStream]', function () {
var transports = [
new winston.transports.Console(),
new winston.transports.File({ filename: path.join(__dirname, 'fixtures', 'logs', 'filelog.log' )})
new winston.transports.File({ filename: path.join(__dirname, 'fixtures', 'logs', 'filelog.log') })
];

var logger = winston.createLogger({ transports: transports });
Expand All @@ -211,7 +211,7 @@ describe('Logger', function () {
assume(logger.transports.length).equals(0);
});

it ('.clear() [transports]', function () {
it('.clear() [transports]', function () {
var logger = winston.createLogger({
transports: [new winston.transports.Console()]
});
Expand Down Expand Up @@ -343,9 +343,9 @@ describe('Logger (levels)', function () {
it('custom levels', function (done) {
var logger = winston.createLogger({
levels: {
bad: 0,
bad: 0,
test: 1,
ok: 2
ok: 2
}
});

Expand Down Expand Up @@ -628,7 +628,7 @@ describe('Logger (stream semantics)', function () {

it(`rethrows errors from user-defined formats`, function () {
stdMocks.use();
const logger = winston.createLogger( {
const logger = winston.createLogger({
transports: [new winston.transports.Console()],
format: winston.format.printf((info) => {
// Set a trap.
Expand Down Expand Up @@ -672,7 +672,7 @@ describe('Logger (winston@2 logging API)', function () {
done();
});

logger.log('info', 'Some super awesome log message')
logger.log('info', 'Some super awesome log message');
});

it(`.log(level, undefined) creates info with { message: undefined }`, function (done) {
Expand Down Expand Up @@ -810,6 +810,27 @@ describe('Logger (logging exotic data types)', function () {
logger.info('Hello', { label: 'world' });
logger.info('Hello %d', 100, { label: 'world' });
});

it('does not modify objects passed as messages', () => {
const logger = winston.createLogger();
const msg = {
name: 'BadRequest',
message: 'This is a test',
code: 400,
className: 'bad-request',
data: {
foo: 'bar'
},
errors: {}
};
const originalMsg = { ...msg };
logger.info(msg);
assume(msg).deep.equals(originalMsg);

// NOTE: We test both info() and log() because they are two separate code paths.
logger.log('info', msg);
assume(msg).deep.equals(originalMsg);
});
});

describe('.info', function () {
Expand Down Expand Up @@ -871,7 +892,7 @@ describe('Logger (profile, startTimer)', function (done) {
logger.profile('testing1', {
something: 'ok',
level: 'info'
})
});
}, 100);
});

Expand All @@ -894,7 +915,7 @@ describe('Logger (profile, startTimer)', function (done) {
logger.profile('testing2', {
something: 'ok',
level: 'info'
})
});
}, 100);
});

Expand Down