Skip to content

Commit 67de816

Browse files
committed
fix(charts): strip ECharts navigation sinks from .chart documents
A .chart document is author-controlled and rendered with setOption() straight into the app document, including on the anonymous /f/<token> share route. confineOptionToCanvas closed the innerHTML/document.write paths but left the navigation ones open: title.link, title.sublink, and a link on a treemap or sunburst data item each reach windowOpen, which assigns the URL to location.href, so a javascript: URL executed on the app origin on one click. Drop the link keys everywhere in the walk alongside toolbox. A chart has no reason to navigate its viewer, so they are stripped rather than scheme-checked, which would still leave an open redirect on an authenticated origin.
1 parent c53837f commit 67de816

2 files changed

Lines changed: 51 additions & 4 deletions

File tree

apps/sim/lib/charts/spec.test.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ describe('shapeTableRows', () => {
9595
})
9696

9797
const XSS_FORMATTER = '<img src=x onerror="alert(1)">'
98+
const XSS_LINK = 'javascript:alert(document.domain)'
9899

99100
describe('parseChartSpec option confinement', () => {
100101
it('forces the tooltip off the innerHTML path, keeping the formatter template', () => {
@@ -175,6 +176,23 @@ describe('parseChartSpec option confinement', () => {
175176
expect(media[0].option.toolbox).toBeUndefined()
176177
})
177178

179+
it('drops every navigation sink — title link/sublink and treemap/sunburst item links', () => {
180+
const option = parse({
181+
schema_version: 1,
182+
option: {
183+
title: { text: 'click me', link: XSS_LINK, sublink: XSS_LINK, target: 'self' },
184+
series: [
185+
{ type: 'treemap', data: [{ name: 'a', value: 1, link: XSS_LINK }] },
186+
{ type: 'sunburst', data: [{ name: 'b', value: 1, link: XSS_LINK }] },
187+
],
188+
baseOption: { title: { link: XSS_LINK } },
189+
media: [{ query: { minWidth: 100 }, option: { title: { link: XSS_LINK } } }],
190+
},
191+
})
192+
expect(JSON.stringify(option)).not.toContain('javascript:')
193+
expect(option.title).toEqual({ text: 'click me', target: 'self' })
194+
})
195+
178196
it('adds no tooltip to a document that declares none', () => {
179197
const option = parse({ schema_version: 1, option: { series: [{ type: 'bar', data: [1] }] } })
180198
expect('tooltip' in option).toBe(false)
@@ -191,7 +209,7 @@ describe('parseChartSpec option confinement', () => {
191209
})
192210

193211
it('leaves dataset rows alone — they hold data, not components', () => {
194-
const rows = [{ tooltip: 'ok', toolbox: 'ok' }]
212+
const rows = [{ tooltip: 'ok', toolbox: 'ok', link: 'ok' }]
195213
const option = parse({ schema_version: 1, option: { dataset: { source: rows } } })
196214
expect((option.dataset as Record<string, unknown>).source).toEqual(rows)
197215
})
@@ -268,6 +286,24 @@ describe('chart option confinement against echarts', () => {
268286
)
269287
expect(model.getComponent('toolbox')).toBeUndefined()
270288
})
289+
290+
it('leaves the title component no link to hand to windowOpen', () => {
291+
const model = renderModel(
292+
parse({
293+
schema_version: 1,
294+
option: {
295+
xAxis: {},
296+
yAxis: {},
297+
series: [{ type: 'bar', data: [1] }],
298+
title: { text: 'click me', link: XSS_LINK, sublink: XSS_LINK },
299+
},
300+
})
301+
)
302+
const title = model.getComponent('title')
303+
expect(title?.get('text')).toBe('click me')
304+
expect(title?.get('link')).toBeUndefined()
305+
expect(title?.get('sublink')).toBeUndefined()
306+
})
271307
})
272308

273309
describe('parseChartSpec table-shaping validation', () => {

apps/sim/lib/charts/spec.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ export interface ChartSpec {
5252
/** ECharts' tooltip render mode that draws into the chart canvas instead of the DOM. */
5353
const CANVAS_TOOLTIP_RENDER_MODE = 'richText'
5454

55+
/** Option keys stripped at every level: the `toolbox` DOM sink and the `link`/`sublink` navigation sinks. */
56+
const DROPPED_KEYS = ['toolbox', 'link', 'sublink'] as const
57+
5558
/**
5659
* Closes the paths by which an ECharts option reaches the DOM, so a `.chart`
5760
* document cannot inject markup into the page that renders it. A document is
@@ -63,7 +66,14 @@ const CANVAS_TOOLTIP_RENDER_MODE = 'richText'
6366
* string `formatter` is used as that content's template verbatim — only the
6467
* values substituted into it are escaped. A `toolbox` assigns `dataView.lang`
6568
* entries to `innerHTML` and fills a `saveAsImage` popup with `document.write`.
66-
* Forcing the render mode and dropping the toolbox leaves the document no DOM
69+
* Forcing the render mode and dropping the toolbox leaves it no DOM sink.
70+
*
71+
* ECharts also navigates: `title.link`, `title.sublink`, and a `link` on a
72+
* treemap or sunburst data item each reach `windowOpen`, which assigns the URL
73+
* to `location.href` — so a `javascript:` URL runs on this origin on a single
74+
* click. A chart has no reason to navigate its viewer, so the keys are dropped
75+
* everywhere rather than scheme-checked, which would still leave an open
76+
* redirect on an authenticated origin. Between them the document is left no
6777
* sink at all, which holds whatever any individual option value contains.
6878
*
6979
* The walk is deep because `tooltip` is not only a top-level component:
@@ -79,8 +89,9 @@ function confineOptionToCanvas(node: unknown): void {
7989
}
8090
if (node === null || typeof node !== 'object') return
8191
const record = node as Record<string, unknown>
82-
// biome-ignore lint/performance/noDelete: the key must be absent, not undefined-valued
83-
if ('toolbox' in record) delete record.toolbox
92+
for (const key of DROPPED_KEYS) {
93+
if (key in record) delete record[key]
94+
}
8495
for (const key of Object.keys(record)) {
8596
if (key === 'dataset') continue
8697
const value = record[key]

0 commit comments

Comments
 (0)