fix(charts): sanitize tooltip HTML across nvd3, rose and partition plugins#40502
Open
sha174n wants to merge 9 commits into
Open
fix(charts): sanitize tooltip HTML across nvd3, rose and partition plugins#40502sha174n wants to merge 9 commits into
sha174n wants to merge 9 commits into
Conversation
Route the annotation tooltip HTML through DOMPurify before it is passed to d3-tip's .html() sink, matching the sanitization already applied in sibling tooltip helpers in this file.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40502 +/- ##
==========================================
+ Coverage 63.99% 64.02% +0.02%
==========================================
Files 2637 2637
Lines 141678 141677 -1
Branches 32570 32570
==========================================
+ Hits 90670 90705 +35
+ Misses 49452 49416 -36
Partials 1556 1556
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ugins Expand the SEC-125 cluster's annotation-tooltip fix to cover every confirmed tooltip HTML sink in the cluster, plus the default nvd3 tooltip path that shares the same root cause. - legacy-preset-chart-nvd3/src/utils.ts: wrap generateBubbleTooltipContent, generateMultiLineTooltipContent, and wrapTooltip output with DOMPurify; flip getFormattedKey shouldDompurify to true for consistency. - legacy-plugin-chart-rose/src/Rose.ts: sanitize series keys in tooltipData and legendData via sanitizeHtml. - legacy-plugin-chart-partition/src/Partition.ts: sanitize tooltip HTML with sanitizeHtml(t) before tip.html(t). - legacy-preset-chart-nvd3/test/utils.test.ts: regression tests for the three nvd3 utils helpers (script and img onerror payloads). - scripts/oxlint.sh: drive-by; last line was '[ -n "$output" ] && echo "$output"' which under 'set -e' aborts the script when oxlint succeeds with empty output. Replaced with an explicit 'if [ -n "$output" ]; then ... fi' so a clean lint run no longer fails the pre-commit hook. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
github-actions
Bot
added
the
risk:ci-script
sha174n
changed the title
sha174n
requested review from
betodealmeida,
craig-rueda,
dpgaspar,
eschutho,
geido,
hainenber,
kgabryje,
mistercrunch,
nytai,
rusackas,
sadpandajoe and
villebro
as code owners
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Contributor
|
The PR comments file contains only the header row and no actual comment data. I cannot analyze or validate the review suggestion without the specific comment content. |
Contributor
Code Review Agent Run #7ed07cActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Restore the raw `v.name` as the legend `key` in Rose. The legend's color callback is keyed on `key`, while arcs are filled by `colorFn(d.name)` on the raw name, so sanitizing one side and not the other made the two identities diverge and swatch/arc colors stop matching. nvd3-fork's legend renders `key` via `.text()`, so the raw value is already escaped at the DOM sink, no markup can reach the page. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Code Review Agent Run #bfa2b4Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Closes the tooltip-HTML sinks across the legacy NVD3 preset and the Rose and Partition plugins. Routes user-controlled column / series / annotation values through DOMPurify (in the nvd3 preset, which already depends on it) and through
sanitizeHtmlfrom@superset-ui/core(in Rose and Partition, where adding a new dep is unwanted) before they reachd3-tip's ornv.models.tooltip's.html()sink.Changes
superset-frontend/plugins/legacy-preset-chart-nvd3/src/utils.tstipFactory(): wrap the annotation-tooltip HTML indompurify.sanitize(...)before returning, consistent with the siblinggenerateCompareTooltipContent/generateTimePivotTooltiphelpers.generateBubbleTooltipContent(): wrap the returned HTML indompurify.sanitize(...). Closes the Bubble-chart tooltip variant.generateMultiLineTooltipContent(): wrap the returned HTML indompurify.sanitize(...), and passshouldDompurify=trueintogetFormattedKeyso the per-series key is also sanitised in line withgenerateCompareTooltipContent.wrapTooltip(): sanitise the wrapped content. This is the single choke point through which every chart constructed inNVD3Vis.tsflows (it is invoked at the bottom of the chart factory), so it closes the defaultnvd3-forktooltip path used by Line, Bar, Area, Pie, BoxPlot, etc. when no customcontentGeneratoris set. Custom generators set earlier already return sanitised output, making this a belt-and-braces wrap.superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.tstooltipData(): passseries[].key(a user-controlled column value) throughsanitizeHtmlbefore it reachesnv.models.tooltip().data(...), which renders the key via.html().legendData(): sanitisekeyat the data boundary too. The legend currently renderskeyvia.text(), but applying the same boundary makes the safety property local to the data layer rather than relying on the vendored legend module's rendering choice.superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.tspositionAndPopulate(): wrap the built tooltip HTML withsanitizeHtml(t)beforetip.html(t). The xss-filters allowlist preservestable,tr,td,div, andstyleattributes so visual rendering is unchanged for benign input; dangerous markup (script, on*, iframe, etc.) is stripped.scripts/oxlint.sh(drive-by)[ -n "$output" ] && echo "$output"with an explicitif, because underset -ethe bare conditional aborts the script whenoxlintsucceeds with empty output (i.e. clean lint). With oxlint 1.66+ installed the pre-commit hook was failing on no-output runs; this restores it.Tests
superset-frontend/plugins/legacy-preset-chart-nvd3/test/utils.test.tsRegression tests assert that DOMPurify strips a
<script>/<img onerror>payload smuggled through:generateBubbleTooltipContentviapoint.entityandpoint.groupgenerateMultiLineTooltipContentviaseries.keytipFactoryvia a description column valueRose and Partition rely on inner closures over
d3/nvd3-forkDOM state and are not unit-testable in isolation; their fixes are minimal wrappers around well-tested sanitisers. Manual verification covers each chart type end-to-end.Testing instructions
cd superset-frontend && npx jest plugins/legacy-preset-chart-nvd3/test/utils.test.ts— 13 tests pass (9 existing + 4 new XSS regressions).<img src=x onerror=alert(1)>; the tooltip on hover must render the text without firing the payload.Additional information