🔒 [security fix] Replace innerHTML with textContent to mitigate XSS risk#3651
Conversation
There was a problem hiding this comment.
Pull request overview
This PR mitigates a potential XSS vector in the Deep Dependencies graph node sizing logic by avoiding HTML parsing when measuring text, and adds a regression/security-focused unit test.
Changes:
- Replaced
innerHTMLwithtextContentincalcRects()to ensure measurement strings are treated as plain text. - Added a new unit test intended to verify HTML-like input is handled as text.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.ts | Uses textContent instead of innerHTML when measuring text widths (XSS mitigation). |
| packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.test.js | Adds a security regression test for HTML-tag-like input handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.ts
Outdated
Show resolved
Hide resolved
...ages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This change mitigates a potential Cross-Site Scripting (XSS) vulnerability by replacing `innerHTML` with `textContent` when setting the content of a measurement span. Since the purpose of the code is to measure the width of plain text strings (service names, operation names, etc.), `textContent` is a safer and more appropriate choice as it does not parse input as HTML. - Updated `packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.ts` - Added security test case to `packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.test.js` to ensure HTML tags are treated as plain text. Signed-off-by: Jules <jules@example.com> Co-authored-by: jkowall <1859948+jkowall@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Jonah Kowall <jkowall@kowall.net>
3e1abd1 to
d01a71d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3651 +/- ##
==========================================
+ Coverage 89.15% 89.26% +0.11%
==========================================
Files 305 306 +1
Lines 9743 9894 +151
Branches 2597 2630 +33
==========================================
+ Hits 8686 8832 +146
- Misses 1053 1059 +6
+ Partials 4 3 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jaegertracing#3651) Replaced `innerHTML` with `textContent` in `calc-positioning.ts` to prevent potential Cross-Site Scripting (XSS) when measuring service and operation names. Note: This is a duplicate of jaegertracing#3651 which is already merged to main. changelog:security Co-authored-by: jkowall <1859948+jkowall@users.noreply.github.com>
🎯 What: The vulnerability fixed is a potential Cross-Site Scripting (XSS) issue in the
calcRectsfunction ofpackages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/calc-positioning.ts.innerHTMLwhen setting the content of a measurement span could allow an attacker to inject and execute malicious scripts if the input strings (like service or operation names) are not properly sanitized or originate from an untrusted source.🛡️ Solution: The fix replaces
innerHTMLwithtextContent. Since the code is only intended to measure the visual dimensions of plain text strings,textContentprovides a secure alternative that prevents the browser from parsing the input as HTML, thus eliminating the XSS vector. A security-focused unit test has also been added to verify this behavior.PR created automatically by Jules for task 10016137243870737156 started by @jkowall