You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/08/21 19:31:15 UTC
(superset) branch master updated: fix(frontend): tighten SQL Lab autorun scoping and HTML-rendering defaults (#43398)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new edda9d1bbbc fix(frontend): tighten SQL Lab autorun scoping and HTML-rendering defaults (#43398)
edda9d1bbbc is described below
commit edda9d1bbbc45364d0781576ab30921d8367dfba
Author: Evan Rusackas <ev...@preset.io>
AuthorDate: Fri Aug 21 12:31:08 2026 -0700
fix(frontend): tighten SQL Lab autorun scoping and HTML-rendering defaults (#43398)
Co-authored-by: Superset Dev <de...@superset.apache.org>
Co-authored-by: Claude Sonnet 5 <no...@anthropic.com>
---
.../superset-ui-core/src/utils/html.test.tsx | 37 ++++++++++++
.../packages/superset-ui-core/src/utils/html.tsx | 27 +++++++--
.../src/renderers/NumericCellRenderer.tsx | 12 +++-
.../test/NumericCellRenderer.test.tsx | 59 +++++++++++++++++++
superset-frontend/src/SqlLab/actions/sqlLab.ts | 12 +++-
.../src/SqlLab/components/ResultSet/index.tsx | 2 +-
.../src/SqlLab/components/SqlEditor/index.tsx | 2 +-
.../DatasourceEditor/DatasourceEditor.tsx | 1 -
.../src/components/FilterableTable/index.tsx | 2 +-
.../src/components/FilterableTable/utils.test.tsx | 29 ++++++++++
.../src/components/FilterableTable/utils.tsx | 6 +-
.../components/useGridResultTable.tsx | 6 +-
.../ColorBreakpointOption.test.tsx | 25 ++++++++
.../ColorBreakpointOption.tsx | 17 +++++-
.../useExploreAdditionalActionsMenu/index.tsx | 43 +++++++++++---
.../useExploreAdditionalActionsMenu.test.tsx | 40 +++++++++++++
.../src/pages/SqlLab/LocationContext.test.tsx | 66 ++++++++++++++++++++++
.../src/pages/SqlLab/LocationContext.tsx | 6 +-
18 files changed, 366 insertions(+), 26 deletions(-)
diff --git a/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx b/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx
index d2928d66d0b..36b63091a91 100644
--- a/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx
@@ -123,6 +123,25 @@ describe('isProbablyHTML', () => {
expect(isProbablyHTML('<canvas></canvas>')).toBe(true);
expect(isProbablyHTML('<iframe src="page.html"></iframe>')).toBe(true);
});
+
+ test('should return true for script-capable and foreign-content tags', () => {
+ expect(isProbablyHTML('<svg onload="alert(1)"></svg>')).toBe(true);
+ expect(isProbablyHTML('<math><mi>x</mi></math>')).toBe(true);
+ expect(
+ isProbablyHTML('<details open ontoggle="alert(1)">x</details>'),
+ ).toBe(true);
+ expect(isProbablyHTML('<summary>x</summary>')).toBe(true);
+ expect(isProbablyHTML('<object data="x"></object>')).toBe(true);
+ expect(isProbablyHTML('<embed src="x">')).toBe(true);
+ expect(isProbablyHTML('<marquee>x</marquee>')).toBe(true);
+ expect(isProbablyHTML('<template>x</template>')).toBe(true);
+ expect(isProbablyHTML('<dialog open>x</dialog>')).toBe(true);
+ });
+
+ test('should return true for elements that parse into document.head', () => {
+ expect(isProbablyHTML('<style>body { display: none; }</style>')).toBe(true);
+ expect(isProbablyHTML('<title>injected</title>')).toBe(true);
+ });
});
describe('sanitizeHtmlIfNeeded', () => {
@@ -137,6 +156,24 @@ describe('sanitizeHtmlIfNeeded', () => {
const sanitizedString = sanitizeHtmlIfNeeded(plainText);
expect(sanitizedString).toEqual(plainText);
});
+
+ test('should sanitize svg/details/style payloads instead of passing them through', () => {
+ const svgPayload = '<svg onload="alert(document.cookie)"></svg>';
+ const sanitizedSvg = sanitizeHtmlIfNeeded(svgPayload);
+ expect(sanitizedSvg).not.toContain('<svg');
+ expect(sanitizedSvg).not.toContain('onload');
+
+ // `details` (with its `open` attribute) is in js-xss's default
+ // whitelist, so the tag itself survives sanitization; the fix is that
+ // the payload is now routed through FilterXSS at all, which strips the
+ // non-whitelisted `ontoggle` handler instead of returning it verbatim.
+ const detailsPayload = '<details open ontoggle="alert(1)">x</details>';
+ const sanitizedDetails = sanitizeHtmlIfNeeded(detailsPayload);
+ expect(sanitizedDetails).toEqual('<details open>x</details>');
+
+ const stylePayload = '<style>body { display: none; }</style>';
+ expect(sanitizeHtmlIfNeeded(stylePayload)).not.toContain('<style');
+ });
});
describe('safeHtmlSpan', () => {
diff --git a/superset-frontend/packages/superset-ui-core/src/utils/html.tsx b/superset-frontend/packages/superset-ui-core/src/utils/html.tsx
index 8e5e8e48297..e3ecfc7fd92 100644
--- a/superset-frontend/packages/superset-ui-core/src/utils/html.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/utils/html.tsx
@@ -154,6 +154,20 @@ const KNOWN_HTML_TAGS = new Set([
'html',
'head',
'body',
+ // Script-capable elements and foreign-content roots (SVG/MathML). These
+ // must be classified as HTML so that downstream sanitization is applied;
+ // omitting them makes the heuristic fail open — payloads such as
+ // `<svg onload=...>` or `<details open ontoggle=...>` would be classified
+ // "not HTML" and returned verbatim by sanitizeHtmlIfNeeded.
+ 'svg',
+ 'math',
+ 'details',
+ 'summary',
+ 'object',
+ 'embed',
+ 'marquee',
+ 'template',
+ 'dialog',
]);
const HTML_TAG_PATTERN = new RegExp(
@@ -183,10 +197,15 @@ export function isProbablyHTML(text: string) {
const parser = new DOMParser();
const doc = parser.parseFromString(cleanedStr, 'text/html');
- // Check if parsing created actual HTML elements (not just text nodes)
- const elements = Array.from(doc.body.childNodes).filter(
- node => node.nodeType === 1,
- ) as Element[];
+ // Check if parsing created actual HTML elements (not just text nodes).
+ // Some elements (e.g. <style>, <title>, <meta>, <link>) parse into
+ // document.head rather than document.body, so both must be inspected —
+ // otherwise a bare <style> payload is classified "not HTML" and skips
+ // sanitization.
+ const elements = [
+ ...Array.from(doc.head.childNodes),
+ ...Array.from(doc.body.childNodes),
+ ].filter(node => node.nodeType === 1) as Element[];
// If no elements were created, it's not HTML
if (elements.length === 0) {
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx
index 349df06191c..58820a45b1f 100644
--- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx
@@ -27,11 +27,21 @@ const StyledTotalCell = styled.div`
`}
`;
+// `align` originates from the chart's stored column_config
+// (col.config.horizontalAlign), which can be set to an arbitrary string via
+// a direct chart-params API write. Emotion compiles interpolated strings as
+// CSS source, so the value must be clamped to a closed set of keywords
+// before it reaches the stylesheet — never interpolated raw.
+const ALLOWED_ALIGN_VALUES = new Set(['left', 'right', 'center']);
+
+const safeAlign = (align?: string) =>
+ align && ALLOWED_ALIGN_VALUES.has(align) ? align : 'left';
+
const CellContainer = styled.div<{ backgroundColor?: string; align?: string }>`
display: flex;
background-color: ${({ backgroundColor }) =>
backgroundColor || 'transparent'};
- justify-content: ${({ align }) => align || 'left'};
+ justify-content: ${({ align }) => safeAlign(align)};
`;
const ArrowContainer = styled.div<{ arrowColor?: string }>`
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/NumericCellRenderer.test.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/NumericCellRenderer.test.tsx
new file mode 100644
index 00000000000..6512442784b
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/NumericCellRenderer.test.tsx
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import '@testing-library/jest-dom';
+import { render } from '@superset-ui/core/spec';
+import { NumericCellRenderer } from '../src/renderers/NumericCellRenderer';
+
+const renderCell = (horizontalAlign?: string) => {
+ const params = {
+ value: 42,
+ valueFormatted: '42',
+ node: { rowPinned: undefined, rowIndex: 0 },
+ hasBasicColorFormatters: false,
+ basicColorFormatters: [],
+ col: {
+ isNumeric: true,
+ config: horizontalAlign ? { horizontalAlign } : {},
+ },
+ valueRange: undefined,
+ alignPositiveNegative: false,
+ colorPositiveNegative: false,
+ } as unknown as Parameters<typeof NumericCellRenderer>[0];
+ return render(<NumericCellRenderer {...params} />);
+};
+
+const collectInjectedCss = () =>
+ Array.from(document.querySelectorAll('style'))
+ .map(style => style.textContent ?? '')
+ .join('\n');
+
+test('applies an allowed horizontalAlign value from column config', () => {
+ const { container } = renderCell('center');
+ expect(container.firstChild).toHaveStyle({ justifyContent: 'center' });
+});
+
+test('does not compile a malicious horizontalAlign into the stylesheet', () => {
+ const payload =
+ 'right;} & { position:fixed; top:0; left:0; width:100vw; height:100vh; z-index:99999; background:#fff url(https://attacker.example/beacon) }';
+ const { container } = renderCell(payload);
+ const css = collectInjectedCss();
+ expect(css).not.toContain('position:fixed');
+ expect(css).not.toContain('attacker.example');
+ expect(container.firstChild).toHaveStyle({ justifyContent: 'left' });
+});
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.ts b/superset-frontend/src/SqlLab/actions/sqlLab.ts
index 290fa78609b..cd8346dbfbb 100644
--- a/superset-frontend/src/SqlLab/actions/sqlLab.ts
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.ts
@@ -1524,7 +1524,9 @@ export function popPermalink(key: string): SqlLabThunkAction<Promise<unknown>> {
dbId: json.dbId ? parseInt(json.dbId, 10) : undefined,
catalog: json.catalog ?? null,
schema: json.schema ?? undefined,
- autorun: json.autorun ? json.autorun : false,
+ // The recipient must review the prefilled query and press
+ // Run; a permalink payload never auto-runs.
+ autorun: false,
sql: json.sql ? json.sql : 'SELECT ...',
templateParams: json.templateParams,
}),
@@ -1548,7 +1550,9 @@ export function popStoredQuery(
dbId: json.dbId ? parseInt(json.dbId, 10) : undefined,
catalog: json.catalog ?? null,
schema: json.schema ?? undefined,
- autorun: json.autorun ? json.autorun : false,
+ // Same rule as popPermalink above — stored payloads never
+ // auto-run.
+ autorun: false,
sql: json.sql ? json.sql : 'SELECT ...',
templateParams: json.templateParams,
}),
@@ -1627,7 +1631,9 @@ export function popDatasourceQuery(
name: `${QUERY_TEXT} ${json.result.name}`,
dbId: json.result.database.id,
schema: json.result.schema,
- autorun: sql !== undefined,
+ // `sql` here can come straight from the URL, so its mere
+ // presence must never imply auto-execution.
+ autorun: false,
sql: sql || json.result.select_star,
}),
),
diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
index 18786158c12..3f9bfdfb341 100644
--- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
+++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
@@ -719,7 +719,7 @@ const ResultSet = ({
if (data && data.length > 0) {
const allowHTML = getItem(
LocalStorageKeys.SqllabIsRenderHtmlEnabled,
- true,
+ false,
);
const tableProps = {
diff --git a/superset-frontend/src/SqlLab/components/SqlEditor/index.tsx b/superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
index 2e75c38fb71..9efe18c8a5c 100644
--- a/superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
+++ b/superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
@@ -284,7 +284,7 @@ const SqlEditor: FC<Props> = ({
getItem(LocalStorageKeys.SqllabIsAutocompleteEnabled, true),
);
const [renderHTMLEnabled, setRenderHTMLEnabled] = useState(
- getItem(LocalStorageKeys.SqllabIsRenderHtmlEnabled, true),
+ getItem(LocalStorageKeys.SqllabIsRenderHtmlEnabled, false),
);
const [showCreateAsModal, setShowCreateAsModal] = useState(false);
const [createAs, setCreateAs] = useState('');
diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
index 738ae2214bb..847bc71f52e 100644
--- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
+++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
@@ -1993,7 +1993,6 @@ function DatasourceEditor({
col => col.column_name,
)}
height={300}
- allowHTML
/>
</>
)}
diff --git a/superset-frontend/src/components/FilterableTable/index.tsx b/superset-frontend/src/components/FilterableTable/index.tsx
index acb85a0c799..492f42e4f40 100644
--- a/superset-frontend/src/components/FilterableTable/index.tsx
+++ b/superset-frontend/src/components/FilterableTable/index.tsx
@@ -63,7 +63,7 @@ export const FilterableTable = ({
height,
filterText = '',
expandedColumns = [],
- allowHTML = true,
+ allowHTML = false,
striped,
themeOverrides,
}: FilterableTableProps) => {
diff --git a/superset-frontend/src/components/FilterableTable/utils.test.tsx b/superset-frontend/src/components/FilterableTable/utils.test.tsx
index cfc99995c03..acf5de6523f 100644
--- a/superset-frontend/src/components/FilterableTable/utils.test.tsx
+++ b/superset-frontend/src/components/FilterableTable/utils.test.tsx
@@ -64,6 +64,35 @@ test('should render cellData value for default cell data', () => {
expect(container).toHaveTextContent('regular_text');
});
+test('should render HTML cell data as inert text by default', () => {
+ const { container } = render(
+ <>
+ {renderResultCell({
+ cellData: '<img src="https://attacker.example/beacon.gif" />link',
+ columnKey: 'a',
+ })}
+ </>,
+ );
+ expect(container.querySelector('img')).not.toBeInTheDocument();
+ expect(container).toHaveTextContent(
+ '<img src="https://attacker.example/beacon.gif" />link',
+ );
+});
+
+test('should render sanitized HTML only when allowHTML is explicitly enabled', () => {
+ const { container } = render(
+ <>
+ {renderResultCell({
+ cellData: '<b>bold</b>',
+ columnKey: 'a',
+ allowHTML: true,
+ })}
+ </>,
+ );
+ expect(container.querySelector('b')).toBeInTheDocument();
+ expect(container).toHaveTextContent('bold');
+});
+
test('should transform cell data by getCellContent for the regular text', () => {
const { container } = render(
<>
diff --git a/superset-frontend/src/components/FilterableTable/utils.tsx b/superset-frontend/src/components/FilterableTable/utils.tsx
index e388c373d6e..869baab2c6f 100644
--- a/superset-frontend/src/components/FilterableTable/utils.tsx
+++ b/superset-frontend/src/components/FilterableTable/utils.tsx
@@ -32,11 +32,15 @@ type Params = CellParams & {
getCellContent?: (args: CellParams) => string;
};
+// Result cells carry untrusted warehouse data, so HTML rendering is opt-in:
+// even sanitized markup keeps active capabilities (img/video fetch beacons,
+// phishing anchors), which must not activate by default for data the viewer
+// did not author.
export const renderResultCell = ({
cellData,
getCellContent,
columnKey,
- allowHTML = true,
+ allowHTML = false,
}: Params) => {
const cellNode =
getCellContent?.({ cellData, columnKey }) ?? String(cellData);
diff --git a/superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx b/superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx
index 2765a05d5d6..d8be6c5a04f 100644
--- a/superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx
+++ b/superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx
@@ -19,7 +19,6 @@
import { useMemo, useCallback, useRef, useState } from 'react';
import {
getTimeFormatter,
- safeHtmlSpan,
TimeFormats,
getMetricLabel,
QueryFormMetric,
@@ -92,8 +91,11 @@ export function useGridColumns(
) {
return timeFormatter(value);
}
+ // Render string cells as plain text: this grid shows raw
+ // query results (untrusted warehouse data) to any viewer,
+ // so HTML must stay inert here even after sanitization.
if (typeof value === 'string') {
- return safeHtmlSpan(value);
+ return value;
}
return String(value);
},
diff --git a/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.test.tsx b/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.test.tsx
index a0cc3294acd..d9f54333337 100644
--- a/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.test.tsx
+++ b/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.test.tsx
@@ -112,6 +112,31 @@ describe('ColorBreakpointOption', () => {
expect(colorPreview).toBeInTheDocument();
});
+ test('should coerce non-numeric color channels instead of injecting CSS', async () => {
+ // Stored form_data is opaque JSON, so a saved chart can carry a string
+ // channel crafted to break out of the styled-component declaration.
+ const maliciousBreakpoint = {
+ id: 3,
+ color: {
+ r: '0,0,0,1); } body { display: none } .x { background: rgba(0' as unknown as number,
+ g: 0,
+ b: 0,
+ a: 1,
+ },
+ minValue: 0,
+ maxValue: 100,
+ };
+
+ renderComponent({ breakpoint: maliciousBreakpoint });
+
+ const colorPreview = await screen.findByTestId('color-preview');
+ expect(colorPreview).toBeInTheDocument();
+ // The malicious string is coerced to a safe numeric channel (0), so the
+ // rendered rule is a plain rgba() value with no injected CSS.
+ expect(colorPreview).toHaveStyle({ background: 'rgba(0, 0, 0, 1)' });
+ expect(document.body).toBeVisible();
+ });
+
test('should handle decimal values', async () => {
const decimalBreakpoint: ColorBreakpointType = {
id: 2,
diff --git a/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.tsx b/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.tsx
index fedee53bb83..c702321cf25 100644
--- a/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.tsx
+++ b/superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointOption.tsx
@@ -31,6 +31,19 @@ const BreakpointColorPreview = styled.div`
margin-right: ${({ theme }) => theme.sizeUnit}px;
`;
+// Chart params are stored server-side as opaque JSON, so breakpoint color
+// channels can arrive as arbitrary strings despite the compile-time numeric
+// type. The formatted color is interpolated into a styled-component template
+// (a stylesheet, not a per-property style assignment), so each channel must
+// be coerced to a plain number to keep attacker-controlled strings from
+// injecting CSS rules.
+const toRgbChannel = (channel: unknown): number => {
+ const value = Number(channel);
+ return Number.isFinite(value)
+ ? Math.min(255, Math.max(0, Math.round(value)))
+ : 0;
+};
+
const ColorBreakpointOption = ({
breakpoint,
colorBreakpoints,
@@ -41,7 +54,9 @@ const ColorBreakpointOption = ({
const { color, minValue, maxValue } = breakpoint;
const formattedColor = color
- ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ ? `rgba(${toRgbChannel(color.r)}, ${toRgbChannel(color.g)}, ${toRgbChannel(
+ color.b,
+ )}, 1)`
: '';
return (
diff --git a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx
index 6f8a58d3c60..fd9171be307 100644
--- a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx
+++ b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx
@@ -78,6 +78,37 @@ import { useExploreDataExport } from './useExploreDataExport';
export const SEARCH_THRESHOLD = 10;
+/**
+ * Escape a single CSV cell value.
+ *
+ * Mirrors the server-side chokepoint (superset/utils/csv.py escape_value):
+ * values starting with a spreadsheet formula prefix (=, +, -, @, |, %, or a
+ * leading tab/carriage return, optionally behind leading whitespace) are
+ * neutralized with a leading single quote so exported cells cannot execute
+ * as formulas when opened in Excel/LibreOffice/Google Sheets. Plain negative
+ * numbers are left untouched. RFC-4180 quoting is applied afterwards.
+ */
+export const escapeCsvValue = (v: unknown): string => {
+ if (v === null || v === undefined) return '';
+ let s = String(v);
+ if (s.length > 0) {
+ const stripped = s.replace(/^\s+/, '');
+ const startsLikeFormula =
+ s[0] === '\t' ||
+ s[0] === '\r' ||
+ (stripped.length > 0 && '-@+|=%'.includes(stripped[0]));
+ const isNegativeNumber = s.length > 1 && /^-[0-9.]+$/.test(s);
+ if (startsLikeFormula && !isNegativeNumber) {
+ // Escape pipe to be extra safe (DDE payloads), then prefix with a
+ // single quote to prevent formula evaluation. Existing backslashes
+ // must be escaped first so the resulting `\|`/`\\` sequences are
+ // unambiguous to a downstream unescaper.
+ s = `'${s.replace(/\\/g, '\\\\').replace(/\|/g, '\\|')}`;
+ }
+ }
+ return /[",\r\n]/.test(s) ? `"${s.replace(/"/g, '""')}"` : s;
+};
+
const MENU_KEYS = {
EDIT_PROPERTIES: 'edit_properties',
DASHBOARDS_ADDED_TO: 'dashboards_added_to',
@@ -488,15 +519,11 @@ export const useExploreAdditionalActionsMenu = (
filename: string,
) => {
if (!rows?.length || !columns?.length) return;
- const esc = (v: unknown): string => {
- if (v === null || v === undefined) return '';
- const s = String(v);
- const wrapped = /[",\n]/.test(s) ? `"${s.replace(/"/g, '""')}"` : s;
- return wrapped;
- };
- const header = columns.map(c => esc(c.label ?? c.key ?? '')).join(',');
+ const header = columns
+ .map(c => escapeCsvValue(c.label ?? c.key ?? ''))
+ .join(',');
const body = rows
- .map(r => columns.map(c => esc(r[c.key])).join(','))
+ .map(r => columns.map(c => escapeCsvValue(r[c.key])).join(','))
.join('\n');
const csv = `${header}\n${body}`;
const blob = new Blob([csv], { type: 'text/csv;charset=utf-8;' });
diff --git a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx
index f508c3406db..dbc9fefee06 100644
--- a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx
+++ b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/useExploreAdditionalActionsMenu.test.tsx
@@ -25,6 +25,7 @@ import downloadAsPdf from 'src/utils/downloadAsPdf';
import {
useExploreAdditionalActionsMenu,
getExportScreenshotMenuItems,
+ escapeCsvValue,
} from './index';
import * as exploreUtils from 'src/explore/exploreUtils';
import { Slice } from 'src/types/Chart';
@@ -180,6 +181,45 @@ test('hides Edit chart properties from a chart editor lacking chart write permis
expect(screen.queryByText('Edit chart properties')).not.toBeInTheDocument();
});
+test('escapeCsvValue neutralizes spreadsheet formula prefixes', () => {
+ // Mirrors superset/utils/csv.py escape_value so the client-built
+ // "Current View" CSV cannot ship live formulas (CSV injection).
+ expect(escapeCsvValue('=HYPERLINK("https://attacker.example")')).toBe(
+ `"'=HYPERLINK(""https://attacker.example"")"`,
+ );
+ expect(escapeCsvValue('@SUM(1+1)')).toBe(`'@SUM(1+1)`);
+ expect(escapeCsvValue('+cmd')).toBe(`'+cmd`);
+ expect(escapeCsvValue('%x')).toBe(`'%x`);
+ expect(escapeCsvValue('\t=1+1')).toBe(`'\t=1+1`);
+ expect(escapeCsvValue(' =1+1')).toBe(`' =1+1`);
+ expect(escapeCsvValue('=cmd|calc')).toBe(`'=cmd\\|calc`);
+});
+
+test('escapeCsvValue escapes pre-existing backslashes before escaping pipes', () => {
+ // A literal backslash sitting next to a pipe must not be left as-is: if it
+ // were, the escaped output (`\|`) would be indistinguishable from an
+ // escaped pipe, so a downstream unescaper couldn't recover the original
+ // value. Escaping backslashes first keeps the two cases unambiguous.
+ expect(escapeCsvValue('=cmd\\|calc')).toBe(`'=cmd\\\\\\|calc`);
+});
+
+test('escapeCsvValue RFC-4180-quotes a value containing a bare carriage return', () => {
+ // A raw \r inside a cell can be read as a record separator by some CSV
+ // consumers, so it must trigger outer quoting the same way \n does, even
+ // when it also triggered the formula-prefix guard above.
+ expect(escapeCsvValue('\r=1+1')).toBe(`"'\r=1+1"`);
+});
+
+test('escapeCsvValue keeps ordinary values intact', () => {
+ expect(escapeCsvValue('regular text')).toBe('regular text');
+ expect(escapeCsvValue('-12.5')).toBe('-12.5');
+ expect(escapeCsvValue(42)).toBe('42');
+ expect(escapeCsvValue(null)).toBe('');
+ expect(escapeCsvValue(undefined)).toBe('');
+ expect(escapeCsvValue('a,b')).toBe(`"a,b"`);
+ expect(escapeCsvValue('say "hi"')).toBe(`"say ""hi"""`);
+});
+
test('shows 413 error toast when exportCSV fails with 413', async () => {
mockExportChart.mockRejectedValue({ status: 413 });
diff --git a/superset-frontend/src/pages/SqlLab/LocationContext.test.tsx b/superset-frontend/src/pages/SqlLab/LocationContext.test.tsx
new file mode 100644
index 00000000000..260af64808f
--- /dev/null
+++ b/superset-frontend/src/pages/SqlLab/LocationContext.test.tsx
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { MemoryRouter } from 'react-router-dom';
+import { render, screen } from 'spec/helpers/testing-library';
+
+import { LocationProvider, useLocationState } from './LocationContext';
+
+const Probe = () => {
+ const { requestedQuery } = useLocationState();
+ return (
+ <>
+ <span data-test="autorun">{String(requestedQuery?.autorun)}</span>
+ <span data-test="sql">{String(requestedQuery?.sql)}</span>
+ </>
+ );
+};
+
+const setup = (initialEntry: string | { pathname: string; state: object }) =>
+ render(
+ <MemoryRouter initialEntries={[initialEntry]}>
+ <LocationProvider>
+ <Probe />
+ </LocationProvider>
+ </MemoryRouter>,
+ );
+
+test('never derives autorun from the URL querystring', () => {
+ // Regression test: a crafted cross-site GET link must not be able to
+ // auto-execute SQL in the victim's session (top-level navigation sends
+ // SameSite=Lax session cookies).
+ setup('/sqllab?dbid=1&sql=SELECT%20%2A%20FROM%20t&autorun=true');
+ expect(screen.getByTestId('autorun')).toHaveTextContent('false');
+ // The deep link still prefills the editor with the requested SQL.
+ expect(screen.getByTestId('sql')).toHaveTextContent('SELECT * FROM t');
+});
+
+test('querystring autorun stays false even when spread from raw params', () => {
+ // `...Object.fromEntries(queryParams)` must not reintroduce the raw
+ // `autorun` string value.
+ setup('/sqllab?sql=SELECT%201&autorun=true');
+ expect(screen.getByTestId('autorun')).toHaveTextContent('false');
+});
+
+test('honors autorun from in-app location.state navigations', () => {
+ setup({
+ pathname: '/sqllab',
+ state: { requestedQuery: { sql: 'SELECT 1', autorun: true } },
+ });
+ expect(screen.getByTestId('autorun')).toHaveTextContent('true');
+});
diff --git a/superset-frontend/src/pages/SqlLab/LocationContext.tsx b/superset-frontend/src/pages/SqlLab/LocationContext.tsx
index ae16704c91d..d64b4854fbd 100644
--- a/superset-frontend/src/pages/SqlLab/LocationContext.tsx
+++ b/superset-frontend/src/pages/SqlLab/LocationContext.tsx
@@ -41,12 +41,14 @@ export const LocationProvider: FC<{ children?: ReactNode }> = ({
const queryParams = new URLSearchParams(location.search);
const permalink = location.pathname.match(/\/p\/\w+/)?.[0].slice(3);
if (queryParams.size > 0 || permalink) {
- const autorun = queryParams.get('autorun') === 'true';
+ // Deep links (querystring or permalink) prefill the editor and wait
+ // for the user to press Run. Only in-app navigations that pass
+ // `location.state` (handled above) may request autorun.
const isDataset = queryParams.get('isDataset') === 'true';
const queryParamsState = {
requestedQuery: {
...Object.fromEntries(queryParams),
- autorun,
+ autorun: false,
permalink,
},
isDataset,