rusackas commented on code in PR #43397:
URL: https://github.com/apache/superset/pull/43397#discussion_r3832202700
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts:
##########
@@ -386,7 +417,9 @@ export const tooltipSchema = z.object({
z.array(z.union([z.number(), z.string()])),
])
.optional(),
- formatter: z.string().optional(), // Only string formatters
+ // Only string formatters, and only markup-free ones: a string tooltip
+ // formatter is rendered via innerHTML (default renderMode 'html').
+ formatter: htmlFreeFormatterSchema.optional(),
Review Comment:
Good catch, fixed. Formatter strings now go through the same allowlist
sanitizer used for other tooltip HTML instead of being rejected outright, so
`<br/>`/`<b>` still work and dangerous markup gets stripped.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/utils.ts:
##########
@@ -85,19 +86,29 @@ export const renderNormalizedTooltip = (
};
});
+ // SECURITY: the tooltip is rendered via innerHTML (ECharts default
+ // renderMode 'html'). `seriesName` is built from raw group-by values in
+ // the query results and `metric` from creator-controlled metric labels,
+ // so both must be HTML-escaped — matching the sanitizeHtml/tooltipHtml
+ // treatment every other echarts tooltip path applies. Values are escaped
+ // too for consistency (formatter output is plain text).
const tooltipRows = metricValues
.map(
({ metric, value }) => `
<div style="display:flex;">
- <div>${colorDot}${metric}:</div>
- <div style="font-weight:bold;margin-left:auto;">${value}</div>
+ <div>${colorDot}${sanitizeHtml(metric)}:</div>
Review Comment:
Good catch, fixed. Wrapped color in the same escaping used for the rest of
this tooltip so a quote in it can't break out of the style attribute.
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts:
##########
@@ -17,22 +17,51 @@
* under the License.
*/
-import { WfsLayerConf } from '../../src/types';
+import { WfsLayerConf, XyzLayerConf } from '../../src/types';
import {
createLayer,
createWfsLayer,
createWmsLayer,
createXyzLayer,
+ escapeAttribution,
} from '../../src/util/layerUtil';
describe('layerUtil', () => {
+ describe('escapeAttribution', () => {
+ test('escapes HTML markup in attribution strings', () => {
+ expect(escapeAttribution('(c) OSM <img src=x onerror=alert(1)>')).toBe(
+ '(c) OSM <img src=x onerror=alert(1)>',
+ );
+ expect(escapeAttribution('a & "b" \'c\'')).toBe(
+ 'a & "b" 'c'',
+ );
+ expect(escapeAttribution(undefined)).toBeUndefined();
+ });
+ });
+
describe('createWmsLayer', () => {
test('exists', () => {
// function is trivial
expect(createWmsLayer).toBeDefined();
});
});
Review Comment:
Good catch, fixed. Added an attribution-escaping test for createWmsLayer
matching the createXyzLayer one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
