Skip to content

Trim URLs in breadcrumbs; add maxUrlLength config opt #906

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

Merged
merged 1 commit into from
Mar 27, 2017
Merged
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
6 changes: 6 additions & 0 deletions docs/config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@ Those configuration options are documented below:
By default, Raven does not truncate messages. If you need to truncate
characters for whatever reason, you may set this to limit the length.

.. describe:: maxUrlLength

By default, Raven will truncate URLs as they appear in breadcrumbs and other meta
interfaces to 250 characters in order to minimize bytes over the wire. This does *not*
affect URLs in stack traces.

.. describe:: autoBreadcrumbs

Enables/disables automatic collection of breadcrumbs. Possible values are:
Expand Down
40 changes: 40 additions & 0 deletions src/raven.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ function Raven() {
crossOrigin: 'anonymous',
collectWindowErrors: true,
maxMessageLength: 0,

// By default, truncates URL values to 250 chars
maxUrlLength: 250,
stackTraceLimit: 50,
autoBreadcrumbs: true,
sampleRate: 1
Expand Down Expand Up @@ -1320,9 +1323,46 @@ Raven.prototype = {
exception.value = truncate(exception.value, max);
}

var request = data.request;
if (request) {
if (request.url) {
request.url = truncate(request.url, this._globalOptions.maxUrlLength);
}
if (request.Referer) {
request.Referer = truncate(request.Referer, this._globalOptions.maxUrlLength);
}
}

if (data.breadcrumbs && data.breadcrumbs.values)
this._trimBreadcrumbs(data.breadcrumbs);

return data;
},

/**
* Truncate breadcrumb values (right now just URLs)
*/
_trimBreadcrumbs: function (breadcrumbs) {
// known breadcrumb properties with urls
// TODO: also consider arbitrary prop values that start with (https?)?://
var urlprops = {to: 1, from: 1, url: 1},
crumb,
data;

for (var i = 0; i < breadcrumbs.values.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

possible improvement would be truncating differently based on how many total breadcrumbs there are (maxurl length = (12500 / breadcrumbs.values.length)) but definitely not a big deal

Copy link
Contributor

Choose a reason for hiding this comment

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

this would make it harder to search for exact matches in breadcrumb urls though so shrug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah we're not gonna get that fancy. I'd sooner drop the maxBreadcrumbs default to 50.

crumb = breadcrumbs.values[i];
if (!crumb.hasOwnProperty('data'))
continue;

data = crumb.data;
for (var prop in urlprops) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@benvinegar late to the party here but this is slightly unsafe - if Object.prototype has a property added to it and data has an own property of that same name, we might truncate something we don't mean to. Simplest fix is to add && prop.hasOwnProperty(prop) but I'm not sure why we don't just make urlprops an array and iterate through. Call it one way or the other and I'll make a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems low risk, but yeah, I agree.

I'm not sure why we don't just make urlprops an array and iterate through

I was feeling fancy. Let's just go with array.

if (data.hasOwnProperty(prop)) {
data[prop] = truncate(data[prop], this._globalOptions.maxUrlLength);
}
}
}
},

_getHttpData: function() {
if (!this._hasNavigator && !this._hasDocument) return;
var httpData = {};
Expand Down
46 changes: 46 additions & 0 deletions test/raven.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ describe('globals', function() {

describe('trimPacket', function() {
it('should work as advertised', function() {
// message/excepion value (maxMessageLength)
Raven._globalOptions.maxMessageLength = 3;
assert.deepEqual(
Raven._trimPacket({message: 'lol'}),
Expand All @@ -105,6 +106,51 @@ describe('globals', function() {
Raven._trimPacket({message: 'lolol', exception: {values: [{value: 'lolol'}]}}),
{message: 'lol\u2026', exception: {values: [{value: 'lol\u2026'}]}}
);

// request URLs (maxUrlLength)
Raven._globalOptions.maxUrlLength = 20;
assert.deepEqual(
Raven._trimPacket({request: {url: 'http://example.com/foo/bar/baz.js', Referer: 'http://example.com/f\u2026'}}),
{request: {url: 'http://example.com/f\u2026', Referer: 'http://example.com/f\u2026'}}
)
});
});

describe('_trimBreadcrumbs', function() {
it('should work as expected', function() {
Raven._globalOptions.maxUrlLength = 20;

var breadcrumbs = {
values: [{
data: {
donttouch: 'this-is-not-a-url-field-and-should-not-be-touched',
to: 'http://example.com/foo/bar/baz.js',
from: 'http://example.com/foo/bar/baz.js'
}
}, {
data: {
donttouch: 'this-is-not-a-url-field-and-should-not-be-touched',
url: 'http://example.com/foo/bar/baz.js'
}
}]
};

// NOTE: _trimBreadcrumbs mutates data in-place
Raven._trimBreadcrumbs(breadcrumbs);
assert.deepEqual(breadcrumbs, {
values: [{
data: {
donttouch: 'this-is-not-a-url-field-and-should-not-be-touched',
to: 'http://example.com/f\u2026', // 20 chars
from: 'http://example.com/f\u2026'
}
}, {
data: {
donttouch: 'this-is-not-a-url-field-and-should-not-be-touched',
url: 'http://example.com/f\u2026'
}
}]
});
});
});

Expand Down
3 changes: 3 additions & 0 deletions typescript/raven.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ interface RavenOptions {
/** By default, Raven does not truncate messages. If you need to truncate characters for whatever reason, you may set this to limit the length. */
maxMessageLength?: number;

/** By default, Raven will truncate URLs as they appear in breadcrumbs and other meta interfaces to 250 characters in order to minimize bytes over the wire. This does *not* affect URLs in stack traces. */
maxUrlLength?: number;

/** Override the default HTTP data transport handler. */
transport?: (options: RavenTransportOptions) => void;
}
Expand Down