Skip to content
This repository was archived by the owner on Oct 23, 2023. It is now read-only.

fix: Do not override context when capturing non-error exceptions #444

Closed
kamilogorek wants to merge 1 commit into from
Closed

fix: Do not override context when capturing non-error exceptions #444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 10 additions & 10 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,8 @@ extend(Raven.prototype, {
cb = kwargs;
kwargs = {};
} else {
kwargs = kwargs || {};
// Do not use reference, as we'll modify this object later on
Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

why did you need to do this one too? because of line 353?

Copy link
Contributor Author

@kamilogorek kamilogorek Apr 5, 2018

Choose a reason for hiding this comment

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

Yes, because, we don't want to mutate someone else's objects.

var extraInfo = {
  extra: {
    something: 'fromDev'
  }
}

function foo () {
  try {
    // something, something
  } catch (e) {
    Raven.captureException(e, extraInfo);
  }
}

function bar () {
  try {
    // something else
  } catch (e) {
    Raven.captureMessage('something broke here', extraInfo);
  }
}

foo();
bar(); // whoops, this will already be modified `foo` call

I also wanted to be consistent across captureException/captureMessage.

kwargs = kwargs ? JSON.parse(stringify(kwargs)) : {};
}

var eventId = this.generateEventId();
Expand Down Expand Up @@ -367,25 +368,24 @@ extend(Raven.prototype, {
cb = kwargs;
kwargs = {};
} else {
kwargs = kwargs || {};
// Do not use reference, as we'll modify this object later on
Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

is this a normal way to accomplish a multiple arity function with an optional second arg? it seems a bit complicated but it's worked well so far

Copy link
Contributor Author

@kamilogorek kamilogorek Apr 5, 2018

Choose a reason for hiding this comment

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

Unfortunately yes :(

kwargs = kwargs ? JSON.parse(stringify(kwargs)) : {};
Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

is there anything not serializable that we should be worried about being passed here, like functions?

Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

did you consider only copying this at the point of mutation to avoid this code running on all execution paths?

not sure if that's actually a good idea or not.

Copy link
Contributor Author

@kamilogorek kamilogorek Apr 5, 2018

Choose a reason for hiding this comment

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

Yes, we may run into issues with req object from some frameworks... Although, I'm not sure why we even pass it down to errorHandler if it's already covered by adding req/res to context in requestHandler which is used in all our framework docs - https://github.com/getsentry/raven-node/blob/master/lib/client.js#L540-L568

Copy link
Contributor Author

@kamilogorek kamilogorek Apr 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I read through the codebase once again and it appears that we should be perfectly fine.

https://github.com/getsentry/raven-node/blob/master/lib/client.js#L549-L562

This middleware is passing just an err itself, so we only have to preserve req just in case someone wants to use it inside dataCallback.

b5e306c#diff-50cfa59973c04321b5da0c6da0fdf4feR367

}

if (!(err instanceof Error)) {
if (utils.isPlainObject(err)) {
// This will allow us to group events based on top-level keys
// which is much better than creating new group when any key/value change
var keys = Object.keys(err).sort();
var hash = md5(keys);
var message =
'Non-Error exception captured with keys: ' +
utils.serializeKeysForMessage(keys);
var serializedException = utils.serializeException(err);

kwargs.message = message;
kwargs.fingerprint = [hash];
kwargs.extra = {
__serialized__: serializedException
};
kwargs = extend(kwargs, {
Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

👍

message: message,
fingerprint: [md5(keys)],
extra: kwargs.extra || {}
});
kwargs.extra.__serialized__ = utils.serializeException(err);

err = new Error(message);
} else {
Expand Down
55 changes: 51 additions & 4 deletions test/raven.client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,22 @@ describe('raven.Client', function() {
done();
});
});

it('should copy object with extra data instead of using its reference directly', function(done) {
var old = client.send;
var info = {
extra: {
hello: 'there'
}
};
client.send = function mockSend(kwargs) {
client.send = old;
kwargs.extra.should.have.property('hello', 'there');
kwargs.extra.should.not.equal(info);
done();
};
client.captureMessage('exception', info);
});
});

describe('#captureException()', function() {
Expand Down Expand Up @@ -259,13 +275,9 @@ describe('raven.Client', function() {
var old = client.send;
client.send = function mockSend(kwargs) {
client.send = old;

kwargs.message.should.equal(
'Non-Error exception captured with keys: aKeyOne, bKeyTwo, cKeyThree, dKeyFour\u2026'
);

// Remove superfluous node version data to simplify the test itself
delete kwargs.extra.node;
kwargs.extra.should.have.property('__serialized__', {
aKeyOne: 'a',
bKeyTwo: 42,
Expand Down Expand Up @@ -385,6 +397,41 @@ describe('raven.Client', function() {
done();
});
});

it('should use and merge provided extra data instead of overriding it', function(done) {
var old = client.send;
client.send = function mockSend(kwargs) {
client.send = old;
kwargs.extra.should.have.property('hello', 'there');
kwargs.tags.should.deepEqual({'0': 'whoop'});
done();
};
client.captureException(
{some: 'exception'},
{
extra: {
hello: 'there'
},
tags: ['whoop']
}
);
});

it('should copy object with extra data instead of using its reference directly', function(done) {
var old = client.send;
var info = {
extra: {
hello: 'there'
}
};
client.send = function mockSend(kwargs) {
client.send = old;
kwargs.extra.should.have.property('hello', 'there');
kwargs.extra.should.not.equal(info);
Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

should this test be should.not.equal(info.extra)?

Copy link
Contributor

@MaxBittker MaxBittker Apr 4, 2018

Choose a reason for hiding this comment

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

maybe this test should assert on the shape of info.extra anyway to be doubly sure

Copy link
Contributor Author

@kamilogorek kamilogorek Apr 5, 2018

Choose a reason for hiding this comment

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

We want to check the reference of a whole object that was passed to capture calls as the second argument.
We have a shape-based tests like this one https://github.com/getsentry/raven-node/pull/444/files#diff-c953753f049964fe98760ee9244e055cR341

Copy link
Contributor

@MaxBittker MaxBittker Apr 5, 2018

Choose a reason for hiding this comment

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

I still think line 430 is a false positive and it should be should.not.equal(info.extra)

(this test in its current form passes without your change)

Copy link
Contributor Author

@kamilogorek kamilogorek Apr 9, 2018

Choose a reason for hiding this comment

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

You're totally right. Fixed.

done();
};
client.captureException({some: 'exception'}, info);
});
});

describe('#install()', function() {
Expand Down