Skip to content

Trim URLs in breadcrumbs; add maxUrlLength config opt - #906

Merged
benvinegar merged 1 commit into
masterfrom
truncate-urls
Mar 27, 2017
Merged

benvinegar merged 1 commit into
masterfrom
truncate-urls

Conversation

@benvinegar

Copy link
Copy Markdown
Contributor

Fixes #874 (sort of)

cc @LewisJEllis @MaxBittker

@benvinegar
benvinegar requested a review from MaxBittker March 25, 2017 20:57

@MaxBittker MaxBittker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice 👍

Comment thread src/raven.js
crumb,
data;

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@benvinegar
benvinegar merged commit 49db400 into master Mar 27, 2017
@benvinegar
benvinegar deleted the truncate-urls branch March 27, 2017 18:27
Comment thread src/raven.js
continue;

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

Copy link
Copy Markdown
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
Copy Markdown
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.

pull Bot pushed a commit to TheDegenerateDev5150/sentry-javascript that referenced this pull request Jun 19, 2026
…entry#906)

axios 1.14.1 contains a supply chain attack via the plain-crypto-js
dependency. Pin to 1.13.5 to prevent accidental upgrades.

See: https://x.com/feross/status/2038807290422370479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants