SVG sanitization for XSS

[maxweng-sentry]

https://linear.app/getsentry/issue/CCMRG-1609/sec-stored-xss-in-svg-graph

Adds some cleaning in the svg to prevent XSS attackes

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

[bruno-garcia]

do we have some tests we can run to make sure this works?

[maxweng-sentry]

Added

[sentry]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.85%. Comparing base (519a0a9) to head (5f5e9fd).
⚠️ Report is 8 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #574   +/-   ##
=======================================
  Coverage   93.85%   93.85%           
=======================================
  Files        1284     1284           
  Lines       46428    46430    +2     
  Branches     1524     1524           
=======================================
+ Hits        43574    43577    +3     
+ Misses       2545     2544    -1     
  Partials      309      309           

Flag	Coverage Δ
apiunit	96.55% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codecov-eu]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[codecov-notifications]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

[thomasrockhu-codecov]

I'd probably have Michelle check this and make sure this is how we want to handle (not sure if title is the only attack vector

[michelletran-sentry]

Sanitizing the title will mitigate the reported attack. So this LGTM!

[ryan953]

The reporter had some other suggestions to mitigate issues:

NOTE: I was able to inject the payload by using a file name, but there may be other ways to inject the payload, so I'd recommend sanitizing all content that is included in the SVG.
NOTE: Last time I gave the same recommendation, but only the open redirect that was used to bypass the CSP policy was fixed, now that there is no CSP policy, the issue can be exploited directly.

Can we keep going and escape everything in all the string-templates in this file?
Idk how many other files are doing unsanitized output like this, but it should be a solved problem to use a templating library instead of "".format() for our outputs and avoid this class of problems altogether. We can setup a project around that if needed.

Also, the note about CSP policy, sounds like it was removed? We should be including that in all responses. I can get the security team involved there to help figure out what the specific directive values should be, I'd like to see that followup ticket too.

edit: ah found some csp stuf completed here: #531 👍

[maxweng-sentry]

I checked all the other strings in the svg and they were all numeric strings generated by internal functions so I believe they should be safe.

[ryan953]

I checked all the other strings in the svg and they were all numeric strings generated by internal functions so I believe they should be safe.

kk, cool! lets do it then