feat(node) Sanitize URLs in Span descriptions and breadcrumbs (PII) by aldenquimby · Pull Request #7206 · getsentry/sentry-javascript

aldenquimby · 2023-02-16T16:05:15Z

My understanding of getsentry/develop#773 is that scrubbing will happen server side, but getsentry/sentry-python#1876 seems to do the scrubbing client side? #5347 could be related. Even if we do want client-side scrubbing, this PR could be a good first step.

First time contributing

Before submitting a pull request, please take a look at our Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

github-actions · 2023-02-16T20:05:52Z

Replay SDK metrics 🚀

		Plain	+Sentry			+Replay
	Revision	Value	Value	Diff	Ratio	Value	Diff	Ratio
LCP	This PR `b66559f`	86.09 ms	117.91 ms	+31.82 ms	+36.96 %	139.80 ms	+53.71 ms	+62.39 %
LCP	Previous `1cf8988`	70.81 ms	94.27 ms	+23.46 ms	+33.13 %	124.62 ms	+53.81 ms	+75.99 %
CLS	This PR `b66559f`	0.06 ms	0.06 ms	+0.00 ms	+0.06 %	0.06 ms	+0.00 ms	+0.16 %
CLS	Previous `1cf8988`	0.06 ms	0.06 ms	-0.00 ms	-0.39 %	0.06 ms	-0.00 ms	-0.46 %
CPU	This PR `b66559f`	17.83 %	19.17 %	+1.33 pp	+7.48 %	25.67 %	+7.84 pp	+43.95 %
CPU	Previous `1cf8988`	12.30 %	12.01 %	-0.29 pp	-2.37 %	17.10 %	+4.80 pp	+39.02 %
JS heap avg	This PR `b66559f`	1.94 MB	1.99 MB	+47.23 kB	+2.43 %	2.86 MB	+921.01 kB	+47.45 %
JS heap avg	Previous `1cf8988`	1.94 MB	1.99 MB	+48.93 kB	+2.52 %	2.87 MB	+929.88 kB	+47.85 %
JS heap max	This PR `b66559f`	2.3 MB	2.56 MB	+256.16 kB	+11.13 %	3.35 MB	+1.05 MB	+45.70 %
JS heap max	Previous `1cf8988`	2.3 MB	2.57 MB	+267.17 kB	+11.60 %	3.35 MB	+1.05 MB	+45.56 %
netTx	This PR `b66559f`	0 B	0 B	0 B	n/a	2.27 kB	+2.27 kB	n/a
netTx	Previous `1cf8988`	0 B	0 B	0 B	n/a	2.22 kB	+2.22 kB	n/a
netRx	This PR `b66559f`	0 B	0 B	0 B	n/a	41 B	+41 B	n/a
netRx	Previous `1cf8988`	0 B	0 B	0 B	n/a	41 B	+41 B	n/a
netCount	This PR `b66559f`	0	0	0	n/a	1	+1	n/a
netCount	Previous `1cf8988`	0	0	0	n/a	1	+1	n/a
netTime	This PR `b66559f`	0.00 ms	0.00 ms	0.00 ms	n/a	107.25 ms	+107.25 ms	n/a
netTime	Previous `1cf8988`	0.00 ms	0.00 ms	0.00 ms	n/a	91.07 ms	+91.07 ms	n/a

Previous results on branch: `develop`

Revision	LCP	CLS	CPU	JS heap avg	JS heap max	netTx	netRx	netCount	netTime
`1cf8988`	+53.81 ms	-0.00 ms	+4.80 pp	+929.88 kB	+1.05 MB	+2.22 kB	+41 B	+1	+91.07 ms
`68655e3`	+72.60 ms	+0.00 ms	+7.90 pp	+922.72 kB	+1.04 MB	+2.22 kB	+41 B	+1	+109.40 ms
`a8449de`	+58.27 ms	-0.00 ms	+7.12 pp	+927.42 kB	+1.05 MB	+2.2 kB	+41 B	+1	+98.31 ms
`79babe9`	+58.69 ms	-0.00 ms	+4.40 pp	+927.46 kB	+1.06 MB	+2.23 kB	+41 B	+1	+103.20 ms
`5359ba9`	+55.62 ms	-0.00 ms	+4.29 pp	+935.26 kB	+1.05 MB	+2.2 kB	+41 B	+1	+79.05 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Thu, 16 Feb 2023 20:05:52 GMT

antonpirker

Apart from the one question I asked in a comment it looks good.

cleptric · 2023-02-17T10:06:26Z

My understanding of getsentry/develop#773 is that scrubbing will happen server side, but getsentry/sentry-python#1876 seems to do the scrubbing client side? #5347 could be related. Even if we do want client-side scrubbing, this PR could be a good first step.

It's a bit of both. One of the goals of the RFC was to enable better server-side scrubbing by moving the data into a structured format. And it will also allow us to apply better client-side scrubbing in the future.

Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>

joshacheson · 2023-02-25T15:37:29Z

Just wanted to note that my org is really looking forward to this landing as well! Thanks @aldenquimby and Sentry folks for putting this work together.

aldenquimby · 2023-03-05T19:56:53Z

@antonpirker @cleptric is there anything else you'd like to see here to get this over the finish line? Appreciate the help, we're excited to remove sensitive info from our perf traces

cleptric · 2023-03-05T20:01:56Z

@aldenquimby Assigned the FE team, which takes care of the JS SDK.

Lms24

Hi @aldenquimby, thanks for opening this PR, really appreciate it! Your changes LGTM. I left a few comments regarding your questions and a small remark. Once they are addressed we'll merge this and include it in the next release.

aldenquimby · 2023-03-15T11:34:34Z

@Lms24 all changes made! You could argue we shouldn't deprecate rawUrl at all, and that user-controlled span/attachment filters should have full access to unsanitized URLs. If you'd prefer that I can do another small refactor to remove the deprecation and reuse a little more of the code. As it stands I tried to make rawUrl easy to delete in a future version

Lms24 · 2023-03-20T10:27:50Z

If you'd prefer that I can do another small refactor to remove the deprecation and reuse a little more of the code. As it stands I tried to make rawUrl easy to delete in a future version.

Hey @aldenquimby you're right about the deprecation being unnecessary. We discussed this internally and we think passing the raw url to the filter function is fine, also in the future. So yes, please if you have time, go ahead and make that change :)

Sorry for the back and forth and thanks for your patience! :)

As of getsentry/sentry-javascript#7206 the Node.js SDK will not append search parameters to the span's description or the URL. Instead, the search parameters will be in the `http.query` key.

Lms24 · 2023-03-30T10:16:39Z

Closing in favour of #7667 where I just added some comments and removed the deprecation.
Thanks again for opening this PR @aldenquimby. We'll make sure, you're mentioned as a contributor in the changelog :)

aldenquimby force-pushed the develop branch from 456cc46 to 48d42ceCompare February 16, 2023 16:05

aldenquimby commented Feb 16, 2023
View reviewed changes

Comment threadpackages/node/src/integrations/http.ts Outdated

aldenquimby mentioned this pull request Feb 16, 2023
Sanitize sensitive data from URLs sent to Sentry #6389
Closed

feat(pii) Sanitize URLs in Span descriptions and breadcrumbs
f8c5a6f

aldenquimby force-pushed the develop branch from 48d42ce to f8c5a6fCompare February 16, 2023 16:13

aldenquimby commented Feb 16, 2023
View reviewed changes

Comment threadpackages/node/test/integrations/http.test.ts Outdated

antonpirker reviewed Feb 17, 2023
View reviewed changes

Comment threadpackages/node/test/integrations/http.test.ts Outdated

cleptric reviewed Feb 17, 2023
View reviewed changes

Comment threadpackages/node/src/integrations/http.ts

cleptric reviewed Feb 17, 2023
View reviewed changes

Comment threadpackages/node/test/integrations/http.test.ts Outdated

cleptric reviewed Feb 17, 2023
View reviewed changes

Comment threadpackages/node/src/integrations/utils/http.ts Outdated

aldenquimbyand others added 3 commits February 17, 2023 07:20

Reviewer suggestion
e57ff25
Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>

Refer to official docs, not a PR
ad72855
Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>

Filter authority
a1126b4

aldenquimby commented Feb 17, 2023
View reviewed changes

Comment threadpackages/node/src/integrations/utils/http.ts

aldenquimby requested review from antonpirker and cleptric and removed request for antonpirker and cleptric February 17, 2023 19:41

cleptric requested review from a team, Lms24 and lforst and removed request for a teamMarch 5, 2023 20:01

Lms24 self-assigned this Mar 6, 2023

Lms24 reviewed Mar 6, 2023
View reviewed changes

Comment threadpackages/node/src/integrations/http.ts Outdated
Comment threadpackages/node/src/integrations/http.ts Outdated
Comment threadpackages/node/src/integrations/utils/http.ts

Lms24 changed the title ~~feat(pii) Sanitize URLs in Span descriptions and breadcrumbs~~feat(node) Sanitize URLs in Span descriptions and breadcrumbs (PII)Mar 8, 2023

aldenquimbyand others added 2 commits March 15, 2023 07:31

Updates from code review
98c3a58

Merge branch 'develop' into develop
2f70b17

This was referenced Mar 22, 2023
fix(perf-issues): Accept sanitized span descriptions getsentry/sentry#46187
Merged
fix(n1-api-call): Properly parse sanitized span descriptions getsentry/sentry#46195
Merged

Lms24 reviewed Mar 29, 2023
View reviewed changes

Comment threadpackages/node/src/integrations/utils/http.ts Outdated

Update packages/node/src/integrations/utils/http.ts
63a7dec

Lms24 mentioned this pull request Mar 30, 2023
feat(node): Sanitize URLs in Span descriptions and breadcrumbs #7667
Merged

Lms24 closed this Mar 30, 2023

Uh oh!

Uh oh!

Conversation

aldenquimby commented Feb 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First time contributing

Uh oh!

Uh oh!

Uh oh!

github-actionsBot commented Feb 16, 2023

Replay SDK metrics 🚀

Previous results on branch: develop

Uh oh!

antonpirker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cleptric commented Feb 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshacheson commented Feb 25, 2023

Uh oh!

aldenquimby commented Mar 5, 2023

Uh oh!

cleptric commented Mar 5, 2023

Uh oh!

Lms24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aldenquimby commented Mar 15, 2023

Uh oh!

Lms24 commented Mar 20, 2023

Uh oh!

Uh oh!

Lms24 commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aldenquimby commented Feb 16, 2023•
edited
Loading

Previous results on branch: `develop`

Lms24 left a comment •
edited
Loading