This is an automated email from the ASF dual-hosted git repository.
aminghadersohi 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 fb79c3c16d8 fix(mcp): report length and empty-title reasons for MCP
title fields (#45575)
fb79c3c16d8 is described below
commit fb79c3c16d890b65a167ce3758e5cd30e3a3e264
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Sat Oct 24 15:27:57 2026 -0400
fix(mcp): report length and empty-title reasons for MCP title fields
(#45575)
---
UPDATING.md | 5 +
docs/admin_docs/configuration/mcp-server.mdx | 60 +
.../src/Bullet/transformProps.ts | 15 +-
.../plugin-chart-echarts/src/Rose/EchartsRose.tsx | 6 +-
.../src/Rose/transformProps.ts | 10 +-
.../test/Bullet/transformProps.test.ts | 66 +-
.../test/Rose/EchartsRose.test.tsx | 39 +
.../test/Rose/transformProps.test.ts | 50 +-
superset/commands/dataset/exceptions.py | 4 +-
superset/mcp_service/chart/schemas.py | 161 +--
superset/mcp_service/chart/tool/generate_chart.py | 12 +-
superset/mcp_service/chart/tool/update_chart.py | 2 +-
superset/mcp_service/chart/validation/pipeline.py | 9 +-
superset/mcp_service/dashboard/schemas.py | 107 +-
superset/mcp_service/dataset/schemas.py | 67 +-
superset/mcp_service/sql_lab/schemas.py | 25 +-
superset/mcp_service/theme/schemas.py | 18 +-
superset/mcp_service/utils/sanitization.py | 76 +-
superset/mcp_service/utils/schema_utils.py | 11 +-
.../mcp_service/chart/tool/test_generate_chart.py | 73 +-
.../mcp_service/chart/tool/test_update_chart.py | 59 +-
.../validation/test_column_name_normalization.py | 32 +
.../dashboard/tool/test_dashboard_generation.py | 34 +
.../dashboard/tool/test_update_dashboard.py | 34 +
.../test_sanitization_warning_inputs.py | 1279 +++++++++++++++++++-
.../mcp_service/utils/test_sanitization.py | 192 +++
26 files changed, 2245 insertions(+), 201 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index c6bce7af5e7..782b938a02d 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -24,6 +24,11 @@ assists people when migrating to a new version.
## Next
+- MCP `update_dashboard` rejects blank or whitespace-only `dashboard_title`
+ values with "Text must not be empty"; omit the field to keep the stored
title.
+ MCP chart names and theme names are capped at 250 characters, matching their
+ model columns (the published chart-name limit was 255).
+
- **MCP authentication hooks and disabled API keys:** Custom
`MCP_USER_RESOLVER`
functions that read `access_token.client_id` must use
`get_verifier_client_id(access_token)` from
diff --git a/docs/admin_docs/configuration/mcp-server.mdx
b/docs/admin_docs/configuration/mcp-server.mdx
index 57f0e248615..a88b984bcac 100644
--- a/docs/admin_docs/configuration/mcp-server.mdx
+++ b/docs/admin_docs/configuration/mcp-server.mdx
@@ -2437,3 +2437,63 @@ Context names are returned verbatim per the Tool Result
Value Contract.
"Did you mean" column candidates also preserve dataset spelling, including
quotes and ampersands, with a 200-character bound per candidate. Use
`get_dataset_info` for the complete schema.
+
+### Title validation errors
+
+Rejected MCP titles report a field-specific reason without echoing the input.
+Titles exceeding 500 characters for dashboards or 250 characters for charts
+report "String exceeds the allowed maximum length". `duplicate_dashboard`
+requires a non-blank title and reports "Text must not be empty" for blank or
+whitespace-only input. A title containing only removed HTML/script content
+(such as `<script>alert(1)</script>`) reports "Contains disallowed content"
+(`unsafe_text`), not an empty-text error. Optional titles on creation retain
+their existing empty-value behavior, including whitespace-only values longer
+than the title limit. On creation, blank chart names fall back to generated
+names; whitespace-only dashboard titles fall back to chart-based titles, while
+an explicit empty string is preserved. `update_dashboard` rejects explicit
blank
+or whitespace-only dashboard renames with "Text must not be empty"; omitting
+`dashboard_title` leaves the stored title unchanged.
+
+Blank names in `create_virtual_dataset`, `create_theme`,
`create_dataset_metric`,
+`update_dataset_metric`, and `update_dataset`, and whitespace-only labels in
+`save_sql_query`, report "Text must not be empty" at the corresponding name
field.
+Blank-name checks precede length checks, including whitespace-only inputs
longer
+than the field limit.
+Explicit null metric or dataset names remain invalid; omitting them from
updates
+leaves the stored name unchanged.
+
+### Chart and dashboard titles
+
+MCP chart names and dashboard titles preserve literal comparisons such as
+`Users aged <30`, `Revenue <$1M`, and `Love <3 charts`. HTML tags are stripped;
+script-only names, dangerous URL schemes, and entity-encoded markup that would
+leave tag-shaped text or rebuild entities that would change on resubmission
+are rejected. A title altered by sanitization
+includes a warning. Python callers supplying UTF-8 bytes or bytearrays receive
+the same title normalization as string inputs; invalid UTF-8 reports an error
on
+the title field. Names are sanitized once before
+persistence, including by
+`generate_chart`'s schema revalidation, so a stored name does not change when
+it is sent back.
+
+### Object-name validation
+
+MCP object names must fit their persisted model columns: chart and dataset
names
+and theme names allow 250 characters, dashboard titles allow 500, saved-query
+labels and SQL Lab tab titles allow 256, and dashboard slugs allow 255.
+For fields that trim whitespace, length validation applies after trimming.
+Physical-dataset `schema` and `catalog` values trim whitespace and treat blanks
+as omitted namespaces, including UTF-8 bytes supplied by Python callers.
+For fields using the shared user-text sanitizer (including chart names,
dashboard
+titles, column names, labels, and certification text), the limit applies to the
+trimmed sanitized text, not removed markup, entity encoding, or surrounding
+whitespace exposed by tag removal. Raw user-text sanitizer
+input has a separate 10,000-character resource bound (or the field limit, if
larger).
+Nonblank theme names, saved-metric names, and dataset renames retain their
+whitespace and count it toward the limit.
+
+Blank saved-query labels and explicit blank dashboard renames return
+`Text must not be empty`, matching virtual-dataset names and duplicated
dashboard
+titles. Omit an update field to retain the existing value.
+Chart creation and updates reject names reduced entirely to disallowed content;
+partial sanitization returns a warning alongside the normalized name.
diff --git
a/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
b/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
index de7eb3b2c36..4c2ace5806c 100644
---
a/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
+++
b/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
@@ -27,6 +27,7 @@ import {
import { t } from '@apache-superset/core/translation';
import type { EChartsCoreOption } from 'echarts/core';
import { Refs } from '../types';
+import { sanitizeHtml as escapeHtml } from '../utils/series';
import { EchartsBulletChartProps, BulletChartTransformedProps } from './types';
import { tokenizeToNumericArray, tokenizeToStringArray } from './utils';
import { estimateWrappedLegendRowCount } from '../utils/legendLayout';
@@ -225,11 +226,15 @@ export default function transformProps(
tooltip: {
formatter: (params?: { dataIndex?: number }) => {
const i = params?.dataIndex ?? 0;
- const prefix = grouped ? `${categories[i]} \u2014 ` : '';
+ const prefix = grouped
+ ? `${escapeHtml(categories[i])} \u2014 `
+ : '';
const band = containingRangeLabel(measures[i]);
- const bandLine = band ? `<br/>${t('Range')}: ${band}` : '';
+ const bandLine = band
+ ? `<br/>${escapeHtml(t('Range'))}: ${escapeHtml(band)}`
+ : '';
return sanitizeHtml(
- `${prefix}${metricLabel}:
<b>${formatter(measures[i])}</b>${bandLine}`,
+ `${prefix}${escapeHtml(metricLabel)}:
<b>${escapeHtml(formatter(measures[i]))}</b>${bandLine}`,
);
},
},
@@ -258,7 +263,7 @@ export default function transformProps(
tooltip: {
formatter: () =>
sanitizeHtml(
- `${rangeLabels[index] || ''} \u2264
<b>${formatter(value)}</b>`,
+ `${escapeHtml(rangeLabels[index] || '')} \u2264
<b>${escapeHtml(formatter(value))}</b>`,
),
},
})),
@@ -275,7 +280,7 @@ export default function transformProps(
tooltip: {
formatter: () =>
sanitizeHtml(
- `${markerLabelAt(index, markerLabels, markers)}:
<b>${formatter(value)}</b>`,
+ `${escapeHtml(markerLabelAt(index, markerLabels, markers))}:
<b>${escapeHtml(formatter(value))}</b>`,
),
},
})),
diff --git
a/superset-frontend/plugins/plugin-chart-echarts/src/Rose/EchartsRose.tsx
b/superset-frontend/plugins/plugin-chart-echarts/src/Rose/EchartsRose.tsx
index d7c3dd8f4f4..93778a16ca0 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Rose/EchartsRose.tsx
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Rose/EchartsRose.tsx
@@ -20,13 +20,13 @@ import { useMemo, useState } from 'react';
import {
CategoricalColorNamespace,
getNumberFormatter,
- sanitizeHtml,
} from '@superset-ui/core';
import { t } from '@apache-superset/core/translation';
import { useTheme } from '@apache-superset/core/theme';
import type { EChartsCoreOption } from 'echarts/core';
import Echart from '../components/Echart';
import { allEventHandlers } from '../utils/eventHandlers';
+import { sanitizeHtml as escapeHtml } from '../utils/series';
import { RoseChartTransformedProps } from './types';
/**
@@ -78,9 +78,7 @@ export default function EchartsRose(props:
RoseChartTransformedProps) {
tooltip: {
confine: true,
formatter: (params: { name: string; value: number; percent: number })
=>
- `<div><b>${period.label}</b></div>${sanitizeHtml(params.name)}:
${format(
- params.value,
- )} (${params.percent}%)`,
+
`<div><b>${escapeHtml(period.label)}</b></div>${escapeHtml(params.name)}:
${escapeHtml(format(params.value))} (${params.percent}%)`,
},
series: [
{
diff --git
a/superset-frontend/plugins/plugin-chart-echarts/src/Rose/transformProps.ts
b/superset-frontend/plugins/plugin-chart-echarts/src/Rose/transformProps.ts
index f85ebc8547f..3045acdbc38 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Rose/transformProps.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Rose/transformProps.ts
@@ -23,10 +23,10 @@ import {
getNumberFormatter,
getTimeFormatter,
QueryFormMetric,
- sanitizeHtml,
} from '@superset-ui/core';
import type { EChartsCoreOption } from 'echarts/core';
import { Refs } from '../types';
+import { sanitizeHtml as escapeHtml } from '../utils/series';
import transformData, { RoseEntry } from './transformData';
import {
EchartsRoseChartProps,
@@ -133,12 +133,12 @@ export default function transformProps(
)};margin-right:5px"></span>`;
const emphasis =
entry.seriesName === highlightSeries ? 'font-weight:bold' : '';
- return `<div
style="${emphasis}">${marker}${sanitizeHtml(entry.seriesName)}: ${format(
- entry.value,
+ return `<div
style="${emphasis}">${marker}${escapeHtml(entry.seriesName)}: ${escapeHtml(
+ format(entry.value),
)}</div>`;
})
.join('');
- return `<div><b>${period.label}</b></div>${rows}`;
+ return `<div><b>${escapeHtml(period.label)}</b></div>${rows}`;
};
const echartOptions: EChartsCoreOption = {
@@ -180,7 +180,7 @@ export default function transformProps(
e => e.seriesName === params.seriesName,
);
if (!period || !entry) return '';
- return
`<div><b>${period.label}</b></div>${sanitizeHtml(params.seriesName)}:
${format(entry.value)}`;
+ return
`<div><b>${escapeHtml(period.label)}</b></div>${escapeHtml(params.seriesName)}:
${escapeHtml(format(entry.value))}`;
},
}),
},
diff --git
a/superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
b/superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
index c8d68b2065f..af1c15203de 100644
---
a/superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
+++
b/superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
@@ -16,11 +16,25 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ChartProps, SqlaFormData, VizType } from '@superset-ui/core';
+import {
+ ChartProps,
+ getNumberFormatterRegistry,
+ NumberFormatter,
+ SqlaFormData,
+ VizType,
+} from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
import { supersetTheme } from '@apache-superset/core/theme';
import transformProps from '../../src/Bullet/transformProps';
import { EchartsBulletChartProps } from '../../src/Bullet/types';
+jest.mock('@apache-superset/core/translation', () => {
+ const translation = jest.requireActual<
+ typeof import('@apache-superset/core/translation')
+ >('@apache-superset/core/translation');
+ return { ...translation, t: jest.fn(translation.t) };
+});
+
const formData: SqlaFormData = {
datasource: '5__table',
viz_type: VizType.Bullet,
@@ -35,7 +49,7 @@ const formData: SqlaFormData = {
const chartProps = (
overrides: Partial<SqlaFormData> = {},
- data: Array<Record<string, number>> = [{ sum__num: 120 }],
+ data: Array<Record<string, string | number>> = [{ sum__num: 120 }],
) =>
new ChartProps({
width: 800,
@@ -424,3 +438,51 @@ test('keeps exact-case dimensions and uses per-index
fallbacks for short labels'
.map(series => series.markLine?.data[0].label.formatter()),
).toEqual(['stretch', '260.0']);
});
+
+test('escapes literal comparison text in every Bullet tooltip', () => {
+ const label = 'Users <30 & <img src=x>';
+ const metric = '<b>Total <3</b>';
+ const range = '<b>Range</b>';
+ jest.mocked(t).mockReturnValueOnce(range);
+ const value = '<b>120</b>';
+ const yAxisFormat = 'bullet-tooltip-markup';
+ getNumberFormatterRegistry().registerValue(
+ yAxisFormat,
+ new NumberFormatter({ id: yAxisFormat, formatFunc: () => value }),
+ );
+ const props = chartProps(
+ {
+ metric,
+ yAxisFormat,
+ rangeLabels: label,
+ ranges: '200',
+ markerLabels: label,
+ markers: '150',
+ groupby: ['group'],
+ },
+ [{ [metric]: 120, group: label }],
+ );
+ const { echartOptions } = transformProps(props);
+ const series = echartOptions.series as Array<{
+ tooltip?: { formatter: () => string };
+ data: Array<{ tooltip?: { formatter: () => string } }>;
+ }>;
+ const tips = [
+ series[0].tooltip!.formatter(),
+ ...series.slice(1).flatMap(s => s.data.map(d => d.tooltip!.formatter())),
+ ];
+ getNumberFormatterRegistry().remove(yAxisFormat);
+ tips.forEach(html => {
+ const element = document.createElement('div');
+ element.innerHTML = html;
+ expect(element.textContent).toContain(label);
+ expect(element.textContent).toContain(value);
+ expect(element.querySelectorAll('b')).toHaveLength(1);
+ expect(element.querySelector('img')).toBeNull();
+ });
+ const element = document.createElement('div');
+ element.innerHTML = tips[0];
+ expect(element.textContent).toContain(metric);
+ expect(element.textContent).toContain(`${range}: ${label}`);
+ expect(element.querySelector('b')).not.toBeNull();
+});
diff --git
a/superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx
b/superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx
index 81e1a1e0f4c..28147b6eebf 100644
---
a/superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx
+++
b/superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx
@@ -20,6 +20,8 @@ import { act, render } from '@testing-library/react';
import {
CategoricalColorNamespace,
getLabelsColorMap,
+ getNumberFormatterRegistry,
+ NumberFormatter,
} from '@superset-ui/core';
import { supersetTheme, ThemeProvider } from '@apache-superset/core/theme';
import EchartsRose from '../../src/Rose/EchartsRose';
@@ -184,3 +186,40 @@ test('clicking a different period while drilled returns to
the rose view', () =>
});
expect(lastEchartCall().echartOptions).toBe(ROSE_ECHART_OPTIONS);
});
+
+test('drilled Rose tooltips preserve literal series and period text', () => {
+ const name = 'Love <3 charts & <img src=x>';
+ const label = 'Users <30 & <b>period</b>';
+ const value = '<b>30</b>';
+ const numberFormat = 'drilled-rose-tooltip-markup';
+ getNumberFormatterRegistry().registerValue(
+ numberFormat,
+ new NumberFormatter({ id: numberFormat, formatFunc: () => value }),
+ );
+ renderRose({
+ ...baseProps,
+ seriesNames: [name],
+ numberFormat,
+ periods: [
+ {
+ time: 1,
+ label,
+ entries: [{ seriesName: name, value: 30, increment: 1 }],
+ },
+ ],
+ });
+ act(() => {
+ lastEchartCall().eventHandlers.click({ dataIndex: 0 });
+ });
+ const html = lastEchartCall().echartOptions.tooltip.formatter({
+ name,
+ value: 30,
+ percent: 100,
+ });
+ getNumberFormatterRegistry().remove(numberFormat);
+ const element = document.createElement('div');
+ element.innerHTML = html;
+ expect(element.textContent).toBe(`${label}${name}: ${value} (100%)`);
+ expect(element.querySelector('img')).toBeNull();
+ expect(element.querySelectorAll('b')).toHaveLength(1);
+});
diff --git
a/superset-frontend/plugins/plugin-chart-echarts/test/Rose/transformProps.test.ts
b/superset-frontend/plugins/plugin-chart-echarts/test/Rose/transformProps.test.ts
index b64933e23ce..611a94246e1 100644
---
a/superset-frontend/plugins/plugin-chart-echarts/test/Rose/transformProps.test.ts
+++
b/superset-frontend/plugins/plugin-chart-echarts/test/Rose/transformProps.test.ts
@@ -16,7 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ChartProps, SqlaFormData, VizType } from '@superset-ui/core';
+import {
+ ChartProps,
+ getNumberFormatterRegistry,
+ NumberFormatter,
+ SqlaFormData,
+ VizType,
+} from '@superset-ui/core';
import { supersetTheme } from '@apache-superset/core/theme';
import transformProps from '../../src/Rose/transformProps';
import { EchartsRoseChartProps } from '../../src/Rose/types';
@@ -133,7 +139,10 @@ test('sanitizes series names in tooltips', () => {
dataIndex: 0,
seriesName: '<img src=x onerror=alert(1)>',
});
- expect(html).not.toContain('onerror');
+ const element = document.createElement('div');
+ element.innerHTML = html;
+ expect(element.querySelector('[onerror]')).toBeNull();
+ expect(element.textContent).toContain('<img src=x onerror=alert(1)>');
});
test('handles empty results without crashing', () => {
@@ -193,3 +202,40 @@ test('renders when __timestamp is serialized as a
non-epoch string', () => {
expect(seriesNames).toEqual(['East', 'West']);
expect(series[0].data.map((d: any) => d.value)).toEqual([30, 10]);
});
+
+test.each([true, false])(
+ 'escapes literal series text in Rose tooltips (rich=%s)',
+ richTooltip => {
+ const name = 'Love <3 charts & <img src=x>';
+ const period = '<b>period</b>';
+ const value = '<b>30</b>';
+ const numberFormat = 'rose-tooltip-markup';
+ getNumberFormatterRegistry().registerValue(
+ numberFormat,
+ new NumberFormatter({ id: numberFormat, formatFunc: () => value }),
+ );
+ const props = new ChartProps({
+ width: 800,
+ height: 600,
+ formData: {
+ ...formData,
+ richTooltip,
+ dateTimeFormat: period,
+ numberFormat,
+ },
+ theme: supersetTheme,
+ queriesData: [{ data: [{ __timestamp: T1, [`sum__num, ${name}`]: 30 }]
}],
+ hooks: {},
+ }) as unknown as EchartsRoseChartProps;
+ const { echartOptions } = transformProps(props);
+ const tooltip = echartOptions.tooltip as {
+ formatter: (params: { dataIndex: number; seriesName: string }) => string;
+ };
+ const element = document.createElement('div');
+ element.innerHTML = tooltip.formatter({ dataIndex: 0, seriesName: name });
+ getNumberFormatterRegistry().remove(numberFormat);
+ expect(element.textContent).toBe(`${period}${name}: ${value}`);
+ expect(element.querySelector('img')).toBeNull();
+ expect(element.querySelectorAll('b')).toHaveLength(1);
+ },
+);
diff --git a/superset/commands/dataset/exceptions.py
b/superset/commands/dataset/exceptions.py
index b68a82d36d1..827ccb3b26a 100644
--- a/superset/commands/dataset/exceptions.py
+++ b/superset/commands/dataset/exceptions.py
@@ -54,7 +54,9 @@ def get_template_render_error_msg(ex: Exception) -> str:
def get_dataset_exist_error_msg(table: Table) -> str:
- return _("Dataset %(table)s already exists", table=table)
+ """Render a readable display name, not the URL-encoded Table identity."""
+ name = ".".join(part for part in (table.catalog, table.schema,
table.table) if part)
+ return str(_("Dataset %(table)s already exists", table=name))
class MultiCatalogDisabledValidationError(ValidationError):
diff --git a/superset/mcp_service/chart/schemas.py
b/superset/mcp_service/chart/schemas.py
index f0b6b47a94b..d217d8e13af 100644
--- a/superset/mcp_service/chart/schemas.py
+++ b/superset/mcp_service/chart/schemas.py
@@ -455,6 +455,8 @@ class GetChartInfoRequest(BaseModel):
extract_permalink_key_from_url,
)
+ if isinstance(value, (bytes, bytearray)):
+ value = value.decode()
if isinstance(value, str) and "/" in value:
if key := extract_permalink_key_from_url(value):
return key
@@ -872,10 +874,14 @@ class BaseChartConfig(UnknownFieldCheckMixin,
OmittedMeansUnchanged):
max_length=255,
)
- @field_validator("temporal_column")
+ @field_validator("temporal_column", mode="before")
@classmethod
- def sanitize_temporal_column(cls, v: str | None) -> str | None:
+ def sanitize_temporal_column(cls, v: object) -> object:
"""Sanitize temporal column names to prevent SQL injection."""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
return sanitize_user_input(
v,
"Temporal column",
@@ -994,20 +1000,26 @@ class ColumnRef(UnknownFieldCheckMixin,
OmittedMeansUnchanged):
self.aggregate = None
return self
- @field_validator("name")
+ @field_validator("name", mode="before")
@classmethod
- def sanitize_name(cls, v: str | None) -> str | None:
+ def sanitize_name(cls, v: object) -> object:
"""Sanitize column name to prevent XSS and SQL injection."""
- if v is None:
- return None
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
return sanitize_user_input(
v, "Column name", max_length=255, check_sql_keywords=True
)
- @field_validator("label")
+ @field_validator("label", mode="before")
@classmethod
- def sanitize_label(cls, v: str | None) -> str | None:
+ def sanitize_label(cls, v: object) -> object:
"""Sanitize display label to prevent XSS attacks."""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
return sanitize_user_input(v, "Label", max_length=500,
allow_empty=True)
@field_validator("sql_expression")
@@ -1123,15 +1135,17 @@ class FilterConfig(UnknownFieldCheckMixin,
OmittedMeansUnchanged):
raise ValueError(f"Filter operator {self.op!r} requires 'value'.")
return self
- @field_validator("column")
+ @field_validator("column", mode="before")
@classmethod
- def sanitize_column(cls, v: str) -> str:
+ def sanitize_column(cls, v: object) -> object:
"""Sanitize filter column name to prevent injection attacks."""
- # sanitize_user_input raises ValueError when allow_empty=False
(default)
- # so the return value is guaranteed to be a non-None str
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
return sanitize_user_input(
v, "Filter column", max_length=255, check_sql_keywords=True
- ) # type: ignore[return-value]
+ )
@field_validator("value")
@classmethod
@@ -1204,16 +1218,20 @@ class GanttSortByConfig(UnknownFieldCheckMixin):
description="Sort this column ascending when true, descending when
false",
)
- @field_validator("column")
+ @field_validator("column", mode="before")
@classmethod
- def sanitize_column(cls, value: str) -> str:
+ def sanitize_column(cls, value: object) -> object:
"""Sanitize the order column like every other dimension name."""
+ if isinstance(value, (bytes, bytearray)):
+ value = value.decode()
+ if not isinstance(value, str):
+ return value
return sanitize_user_input(
value,
"Gantt order column",
max_length=255,
check_sql_keywords=True,
- ) # type: ignore[return-value]
+ )
# Actual chart types
@@ -2754,10 +2772,14 @@ class SunburstChartConfig(BaseChartConfig):
# typos and stale type-specific payload rather than silently dropping
it.
return data
- @field_validator("time_range")
+ @field_validator("time_range", mode="before")
@classmethod
- def sanitize_time_range(cls, value: str | None) -> str | None:
+ def sanitize_time_range(cls, value: object) -> object:
"""Sanitize a human-readable Superset time-range expression."""
+ if isinstance(value, (bytes, bytearray)):
+ value = value.decode()
+ if not isinstance(value, str):
+ return value
return sanitize_user_input(
value,
"Time range",
@@ -3146,12 +3168,14 @@ class InteractivePivotChartConfig(BaseChartConfig):
data["time_compare"] = periods[0] if periods else None
return data
- @field_validator("comparison_period")
+ @field_validator("comparison_period", mode="before")
@classmethod
- def sanitize_comparison_period(cls, value: str | None) -> str | None:
+ def sanitize_comparison_period(cls, value: object) -> object:
"""Sanitize the relative time-shift label."""
- if value is None:
- return None
+ if isinstance(value, (bytes, bytearray)):
+ value = value.decode()
+ if not isinstance(value, str):
+ return value
return sanitize_user_input(
value,
"Comparison period",
@@ -4631,9 +4655,14 @@ class BulletChartConfig(BaseChartConfig):
result.append(sanitized or "")
return result
- @field_validator("time_range")
+ @field_validator("time_range", mode="before")
@classmethod
- def sanitize_time_range(cls, value: str | None) -> str | None:
+ def sanitize_time_range(cls, value: object) -> object:
+ """Sanitize the time range before enforcing its text length limit."""
+ if isinstance(value, (bytes, bytearray)):
+ value = value.decode()
+ if not isinstance(value, str):
+ return value
return sanitize_user_input(
value, "Time range", max_length=1000, allow_empty=True
)
@@ -6320,25 +6349,15 @@ class ChartTargetMixin(BaseModel):
return self
-class GenerateChartRequest(
- ChartRequestNormalizerMixin, ChartTargetMixin, QueryCacheControl
-):
- model_config = ConfigDict(populate_by_name=True)
+class ChartNameMixin(BaseModel):
+ """Share chart-name normalization and server-only sanitization notices."""
- config: Annotated[ChartConfig, chart_config_reference_schema()] = Field(
- ..., description=CHART_CONFIG_DESCRIPTION
- )
chart_name: str | None = Field(
None,
description="Auto-generates if omitted",
- max_length=255,
+ max_length=250,
validation_alias=AliasChoices("chart_name", "name", "title",
"slice_name"),
)
- save_chart: bool = Field(default=False, description="Save permanently in
Superset")
- generate_preview: bool = True
- preview_formats: List[Literal["url", "ascii", "vega_lite", "table"]] =
Field(
- default_factory=lambda: ["url"],
- )
sanitization_warnings: SkipJsonSchema[List[str]] = Field(
default_factory=list,
description=(
@@ -6354,7 +6373,7 @@ class GenerateChartRequest(
def _detect_chart_name_sanitization(cls, data: Any) -> Any:
"""Record a warning when chart_name sanitization alters user input.
- Runs before the ``chart_name`` field validator strips HTML, so we
+ Sanitizes once before field constraints are applied, so we
can compare the caller's raw input against the sanitizer output
and tell the tool whether to notify the caller. Empty-after-
sanitization is rejected with a ValueError so the caller gets a
@@ -6373,12 +6392,19 @@ class GenerateChartRequest(
break
else:
raw = None
- if not isinstance(raw, str) or not raw.strip():
- return data
with field_validation_error("chart_name"):
+ if isinstance(raw, (bytes, bytearray)):
+ raw = raw.decode()
+ if not isinstance(raw, str):
+ return data
+ if not raw.strip():
+ data[key] = None
+ return data
sanitized, was_modified = sanitize_user_input_with_changes(
- raw, "Chart name", max_length=255, allow_empty=True
+ raw, "Chart name", max_length=250, allow_empty=True
)
+ if isinstance(sanitized, str):
+ sanitized = sanitized.strip()
if was_modified and not sanitized:
raise ValueError(
"chart_name contained only disallowed content "
@@ -6386,6 +6412,7 @@ class GenerateChartRequest(
"sanitization. Provide a chart_name with plain text, or "
"omit it to auto-generate one."
)
+ data[key] = sanitized
if was_modified:
data["sanitization_warnings"].append(
"chart_name was modified during sanitization to remove "
@@ -6396,16 +6423,24 @@ class GenerateChartRequest(
@field_validator("chart_name")
@classmethod
- def sanitize_chart_name(cls, v: str | None) -> str | None:
- """Sanitize chart name to prevent XSS attacks."""
- try:
- return sanitize_user_input(
- v, "Chart name", max_length=255, allow_empty=True
- )
- except ValueError as ex:
- raise PydanticCustomError(
- "unsafe_text", "{error}", {"error": str(ex)}
- ) from ex
+ def normalize_blank_chart_name(cls, v: str | None) -> str | None:
+ """Keep the optional-name fallback; nonblank names are sanitized
above."""
+ return v if v and v.strip() else None
+
+
+class GenerateChartRequest(
+ ChartRequestNormalizerMixin, ChartTargetMixin, QueryCacheControl,
ChartNameMixin
+):
+ model_config = ConfigDict(populate_by_name=True)
+
+ config: Annotated[ChartConfig, chart_config_reference_schema()] = Field(
+ ..., description=CHART_CONFIG_DESCRIPTION
+ )
+ save_chart: bool = Field(default=False, description="Save permanently in
Superset")
+ generate_preview: bool = True
+ preview_formats: List[Literal["url", "ascii", "vega_lite", "table"]] =
Field(
+ default_factory=lambda: ["url"],
+ )
@model_validator(mode="after")
def validate_cache_timeout(self) -> "GenerateChartRequest":
@@ -6449,7 +6484,10 @@ class GenerateExploreLinkRequest(
class UpdateChartRequest(
- ChartRequestNormalizerMixin, OmittedMeansUnchanged, QueryCacheControl
+ ChartRequestNormalizerMixin,
+ OmittedMeansUnchanged,
+ QueryCacheControl,
+ ChartNameMixin,
):
model_config = ConfigDict(populate_by_name=True, loc_by_alias=False)
@@ -6475,12 +6513,6 @@ class UpdateChartRequest(
"columns to an existing table chart."
),
)
- chart_name: str | None = Field(
- None,
- description="Auto-generates if omitted",
- max_length=255,
- validation_alias=AliasChoices("chart_name", "name", "title",
"slice_name"),
- )
dataset_id: int | None = Field(
None,
description=(
@@ -6540,19 +6572,6 @@ class UpdateChartRequest(
raise ValueError("'add_columns' must contain at least one column")
return self
- @field_validator("chart_name")
- @classmethod
- def sanitize_chart_name(cls, v: str | None) -> str | None:
- """Sanitize chart name to prevent XSS attacks."""
- try:
- return sanitize_user_input(
- v, "Chart name", max_length=255, allow_empty=True
- )
- except ValueError as ex:
- raise PydanticCustomError(
- "unsafe_text", "{error}", {"error": str(ex)}
- ) from ex
-
class UpdateChartPreviewRequest(ChartRequestNormalizerMixin,
FormDataCacheControl):
model_config = ConfigDict(populate_by_name=True)
diff --git a/superset/mcp_service/chart/tool/generate_chart.py
b/superset/mcp_service/chart/tool/generate_chart.py
index b81a20cc86e..f3639aaf5e1 100644
--- a/superset/mcp_service/chart/tool/generate_chart.py
+++ b/superset/mcp_service/chart/tool/generate_chart.py
@@ -252,8 +252,12 @@ async def generate_chart( # noqa: C901
is_valid=valid, request=request, error=semantic_error
)
else:
- # Preserve omitted controls through schema revalidation.
- request_data: dict[str, Any] =
request.model_dump(exclude_unset=True)
+ # Preserve omitted controls through schema revalidation. The
+ # name was sanitized when this request was validated; a second
+ # sanitizer pass is not idempotent, so it is carried over.
+ request_data: dict[str, Any] = request.model_dump(
+ exclude_unset=True, exclude={"chart_name"}
+ )
request_data["config"]["chart_type"] =
request.config.chart_type
validation_result =
ValidationPipeline.validate_request_with_warnings(
request_data
@@ -261,7 +265,9 @@ async def generate_chart( # noqa: C901
if validation_result.is_valid and validation_result.request is not
None:
# Use the validated request going forward
- request = validation_result.request
+ request = validation_result.request.model_copy(
+ update={"chart_name": request.chart_name}
+ )
# Capture runtime warnings (informational, not blocking)
if validation_result.warnings:
diff --git a/superset/mcp_service/chart/tool/update_chart.py
b/superset/mcp_service/chart/tool/update_chart.py
index b6420223bcf..8f1ac5d9bcc 100644
--- a/superset/mcp_service/chart/tool/update_chart.py
+++ b/superset/mcp_service/chart/tool/update_chart.py
@@ -1407,7 +1407,7 @@ async def update_chart( # noqa: C901
"is_unsaved_state": not saved,
},
"error": None,
- "warnings": warnings,
+ "warnings": request.sanitization_warnings + warnings,
"form_data": _wrapped_form_data_for_response(new_form_data),
"previews": previews,
"preview_errors": preview_errors,
diff --git a/superset/mcp_service/chart/validation/pipeline.py
b/superset/mcp_service/chart/validation/pipeline.py
index 4026fb4f525..4de370ad358 100644
--- a/superset/mcp_service/chart/validation/pipeline.py
+++ b/superset/mcp_service/chart/validation/pipeline.py
@@ -323,11 +323,14 @@ class ValidationPipeline:
dataset_context=dataset_context,
)
- # Create a new request with the normalized config
- request_dict = request.model_dump()
+ # Create a new request with the normalized config. The name is
+ # already sanitized; carry it over instead of sanitizing it again.
+ request_dict = request.model_dump(exclude={"chart_name"})
request_dict["config"] =
normalized_config.model_dump(exclude_unset=True)
- return GenerateChartRequest.model_validate(request_dict)
+ return
GenerateChartRequest.model_validate(request_dict).model_copy(
+ update={"chart_name": request.chart_name}
+ )
except (GanttSemanticNormalizationError,
AmbiguousDatasetReferenceError):
# These name a real, actionable schema conflict rather than a
diff --git a/superset/mcp_service/dashboard/schemas.py
b/superset/mcp_service/dashboard/schemas.py
index e7732a5bb87..ec5d70ff848 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -879,12 +879,19 @@ class GenerateDashboardRequest(BaseModel):
break
else:
raw = None
- if not isinstance(raw, str) or not raw.strip():
- return data
with field_validation_error("dashboard_title"):
+ if isinstance(raw, (bytes, bytearray)):
+ raw = raw.decode()
+ if not isinstance(raw, str):
+ return data
+ if not raw.strip():
+ data[key] = "" if raw == "" else None
+ return data
sanitized, was_modified = sanitize_user_input_with_changes(
raw, "Dashboard title", max_length=500, allow_empty=True
)
+ if isinstance(sanitized, str):
+ sanitized = sanitized.strip()
if was_modified and not sanitized:
raise ValueError(
"dashboard_title contained only disallowed content "
@@ -892,6 +899,7 @@ class GenerateDashboardRequest(BaseModel):
"sanitization. Provide a dashboard_title with plain text, "
"or omit it to auto-generate one from chart names."
)
+ data[key] = sanitized
if was_modified:
data["sanitization_warnings"].append(
"dashboard_title was modified during sanitization to "
@@ -902,8 +910,8 @@ class GenerateDashboardRequest(BaseModel):
@field_validator("dashboard_title")
@classmethod
- def sanitize_dashboard_title(cls, v: str | None) -> str | None:
- """Sanitize dashboard title to prevent XSS.
+ def normalize_blank_dashboard_title(cls, v: str | None) -> str | None:
+ """Normalize blank titles; nonblank titles are sanitized above.
Preserves an explicit empty string (caller-provided ``""``) rather
than collapsing it to ``None``, since the tool treats ``None`` as
@@ -912,9 +920,7 @@ class GenerateDashboardRequest(BaseModel):
"""
if v is None or v == "":
return v
- return sanitize_user_input(
- v, "Dashboard title", max_length=500, allow_empty=True
- )
+ return v if v.strip() else None
class UpdateDashboardRequest(OmittedMeansUnchanged):
@@ -926,7 +932,7 @@ class UpdateDashboardRequest(OmittedMeansUnchanged):
the REST API) without a full re-create.
"""
- model_config = ConfigDict(populate_by_name=True)
+ model_config = ConfigDict(populate_by_name=True, loc_by_alias=False)
identifier: int | str = Field(
...,
@@ -1044,18 +1050,27 @@ class UpdateDashboardRequest(OmittedMeansUnchanged):
break
else:
raw = None
- if not isinstance(raw, str) or not raw.strip():
- return data
with field_validation_error("dashboard_title"):
+ if isinstance(raw, (bytes, bytearray)):
+ raw = raw.decode()
+ if not isinstance(raw, str):
+ return data
+ if not raw.strip():
+ raise PydanticCustomError(
+ "empty_text", "dashboard_title cannot be empty"
+ )
sanitized, was_modified = sanitize_user_input_with_changes(
raw, "Dashboard title", max_length=500, allow_empty=True
)
+ if isinstance(sanitized, str):
+ sanitized = sanitized.strip()
if was_modified and not sanitized:
raise ValueError(
"dashboard_title contained only disallowed content "
"(HTML/script/URL schemes) and was removed entirely by "
"sanitization. Provide a dashboard_title with plain text."
)
+ data[key] = sanitized
if was_modified:
data["sanitization_warnings"].append(
"dashboard_title was modified during sanitization to "
@@ -1066,17 +1081,17 @@ class UpdateDashboardRequest(OmittedMeansUnchanged):
@field_validator("dashboard_title")
@classmethod
- def sanitize_dashboard_title(cls, v: str | None) -> str | None:
- """Sanitize dashboard title to prevent XSS."""
- if v is None or v == "":
+ def normalize_blank_dashboard_title(cls, v: str | None) -> str | None:
+ """Reject blank updates; nonblank titles are sanitized above."""
+ if v is None:
return v
- return sanitize_user_input(
- v, "Dashboard title", max_length=500, allow_empty=True
- )
+ if not v.strip():
+ raise PydanticCustomError("empty_text", "dashboard_title cannot be
empty")
+ return v
- @field_validator("slug")
+ @field_validator("slug", mode="before")
@classmethod
- def normalize_slug(cls, v: str | None) -> str | None:
+ def normalize_slug(cls, v: object) -> object:
"""Normalize the slug to match the REST DashboardPutSchema contract.
Mirrors ``BaseDashboardSchema.post_load``: strip, replace spaces with
@@ -1087,7 +1102,9 @@ class UpdateDashboardRequest(OmittedMeansUnchanged):
REST schema behavior. Raises ``ValueError`` when a non-whitespace input
normalizes to empty (e.g. ``"!!!"``), preventing accidental slug
clearing.
"""
- if not v:
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str) or not v:
return v
stripped = v.strip()
if not stripped:
@@ -1381,11 +1398,15 @@ class ManageDashboardCertificationRequest(BaseModel):
raise ValueError("identifier must be an integer ID, UUID, or slug
string")
return value
- @field_validator("certified_by")
+ @field_validator("certified_by", mode="before")
@classmethod
- def sanitize_certified_by(cls, v: str | None) -> str | None:
+ def sanitize_certified_by(cls, v: object) -> object:
"""Sanitize certified_by to prevent XSS; it renders as a UI badge."""
- if v is None or v == "":
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
+ if v == "":
return v
sanitized: str | None = sanitize_user_input(
v, "certified_by", max_length=500, allow_empty=True
@@ -1401,12 +1422,16 @@ class ManageDashboardCertificationRequest(BaseModel):
)
return sanitized
- @field_validator("certification_details")
+ @field_validator("certification_details", mode="before")
@classmethod
- def sanitize_certification_details(cls, v: str | None) -> str | None:
+ def sanitize_certification_details(cls, v: object) -> object:
"""Sanitize certification_details to prevent XSS; it renders in the
same CertifiedBadge tooltip as certified_by."""
- if v is None or v == "":
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
+ if v == "":
return v
sanitized: str | None = sanitize_user_input(
v, "certification_details", max_length=5000, allow_empty=True
@@ -1462,7 +1487,7 @@ class GenerateDashboardResponse(BaseModel):
class DuplicateDashboardRequest(BaseModel):
"""Request schema for duplicating an existing dashboard."""
- model_config = ConfigDict(populate_by_name=True)
+ model_config = ConfigDict(populate_by_name=True, loc_by_alias=False)
dashboard_id: Annotated[
int | str,
@@ -1502,8 +1527,8 @@ class DuplicateDashboardRequest(BaseModel):
def _detect_dashboard_title_sanitization(cls, data: Any) -> Any:
"""Reject empty-after-sanitization titles and warn on partial strip.
- Runs before the ``dashboard_title`` field validator rewrites the
- value. If the caller supplied a title that sanitization would strip
+ Sanitizes once before field constraints are applied.
+ If the caller supplied a title that sanitization would strip
entirely (XSS-only content), we raise so the caller gets a clear
error instead of a blank-titled dashboard. When the sanitizer only
trims part of the title, we record a warning the tool can return
@@ -1522,18 +1547,27 @@ class DuplicateDashboardRequest(BaseModel):
break
else:
raw = None
- if not isinstance(raw, str) or not raw.strip():
- return data
with field_validation_error("dashboard_title"):
+ if isinstance(raw, (bytes, bytearray)):
+ raw = raw.decode()
+ if not isinstance(raw, str):
+ return data
+ if not raw.strip():
+ raise PydanticCustomError(
+ "empty_text", "dashboard_title cannot be empty"
+ )
sanitized, was_modified = sanitize_user_input_with_changes(
raw, "Dashboard title", max_length=500, allow_empty=True
)
+ if isinstance(sanitized, str):
+ sanitized = sanitized.strip()
if was_modified and not sanitized:
raise ValueError(
"dashboard_title contained only disallowed content "
"(HTML/script/URL schemes) and was removed entirely by "
"sanitization. Provide a dashboard_title with plain text."
)
+ data[key] = sanitized
if was_modified:
data["sanitization_warnings"].append(
"dashboard_title was modified during sanitization to "
@@ -1544,14 +1578,11 @@ class DuplicateDashboardRequest(BaseModel):
@field_validator("dashboard_title")
@classmethod
- def sanitize_dashboard_title(cls, v: str) -> str:
- """Sanitize dashboard title to prevent XSS."""
- sanitized = sanitize_user_input(
- v, "Dashboard title", max_length=500, allow_empty=True
- )
- if not sanitized:
- raise ValueError("dashboard_title cannot be empty")
- return sanitized
+ def reject_blank_dashboard_title(cls, v: str) -> str:
+ """Reject blank duplicates; nonblank titles are sanitized above."""
+ if not v.strip():
+ raise PydanticCustomError("empty_text", "dashboard_title cannot be
empty")
+ return v
class DuplicateDashboardResponse(BaseModel):
diff --git a/superset/mcp_service/dataset/schemas.py
b/superset/mcp_service/dataset/schemas.py
index d94c9873b8e..2fa5b0d4e64 100644
--- a/superset/mcp_service/dataset/schemas.py
+++ b/superset/mcp_service/dataset/schemas.py
@@ -35,6 +35,7 @@ from pydantic import (
model_validator,
StrictBool,
)
+from pydantic_core import PydanticCustomError
from superset.daos.base import ColumnOperator, ColumnOperatorEnum
from superset.mcp_service.chart.schemas import (
@@ -64,7 +65,10 @@ from superset.mcp_service.system.schemas import (
TagInfo,
)
from superset.mcp_service.utils.response_utils import humanize_timestamp
-from superset.mcp_service.utils.schema_utils import OmittedMeansUnchanged
+from superset.mcp_service.utils.schema_utils import (
+ field_validation_error,
+ OmittedMeansUnchanged,
+)
from superset.mcp_service.utils.serialization import (
JsonSafeRows,
OptionalRowCount,
@@ -556,6 +560,8 @@ class CreateDatasetRequest(BaseModel):
rejects them, rather than silently treating a malformed value (e.g. an
int or dict) as an omitted namespace.
"""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
if isinstance(v, str):
return v.strip() or None
return v
@@ -564,6 +570,8 @@ class CreateDatasetRequest(BaseModel):
@classmethod
def _strip_table_name(cls, v: object) -> object:
"""Strip leading/trailing whitespace from table_name."""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
if isinstance(v, str):
return v.strip()
return v
@@ -621,12 +629,19 @@ class CreateVirtualDatasetRequest(BaseModel):
raise ValueError("sql must not be empty")
return v.strip()
- @field_validator("dataset_name")
+ @field_validator("dataset_name", mode="before")
@classmethod
- def dataset_name_must_not_be_empty(cls, v: str) -> str:
- if not v.strip():
- raise ValueError("dataset_name must not be empty")
- return v.strip()
+ def dataset_name_must_not_be_empty(cls, v: object) -> object:
+ """Trim names and reject blanks before checking their length."""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if isinstance(v, str):
+ if not v.strip():
+ raise PydanticCustomError(
+ "empty_text", "dataset_name must not be empty"
+ )
+ return v.strip()
+ return v
class CreateVirtualDatasetResponse(BaseModel):
@@ -729,6 +744,19 @@ class DatasetMetricProperties(OmittedMeansUnchanged):
None, description="JSON-encoded string with extra metric metadata."
)
+ @field_validator("metric_name", mode="before")
+ @classmethod
+ def reject_blank_metric_name(cls, value: object) -> object:
+ """Reject blank names before the length cap without trimming valid
names."""
+ if isinstance(value, (bytes, bytearray)):
+ try:
+ value = value.decode()
+ except UnicodeDecodeError:
+ return value # Let Pydantic report its string_unicode error.
+ if isinstance(value, str) and not value.strip():
+ raise PydanticCustomError("empty_text", "metric_name cannot be
empty")
+ return value
+
@field_validator("d3format")
@classmethod
def d3format_must_be_valid(cls, value: str | None) -> str | None:
@@ -763,8 +791,11 @@ class DatasetMetricProperties(OmittedMeansUnchanged):
"At least one metric property must be provided to update. "
f"Updatable properties: {sorted(UPDATABLE_METRIC_FIELDS)}."
)
- if "metric_name" in provided and not (self.metric_name or "").strip():
- raise ValueError("metric_name cannot be empty or null")
+ if "metric_name" in provided and self.metric_name is None:
+ with field_validation_error("metric_name"):
+ raise PydanticCustomError(
+ "empty_text", "metric_name cannot be empty or null"
+ )
if "expression" in provided and not (self.expression or "").strip():
raise ValueError("expression cannot be empty or null")
if "extra" in provided and self.extra is not None:
@@ -1033,6 +1064,19 @@ class UpdateDatasetRequest(OmittedMeansUnchanged):
"pick up schema changes in the underlying table or query.",
)
+ @field_validator("table_name", mode="before")
+ @classmethod
+ def reject_blank_table_name(cls, value: object) -> object:
+ """Reject blank names before the length cap without trimming valid
names."""
+ if isinstance(value, (bytes, bytearray)):
+ try:
+ value = value.decode()
+ except UnicodeDecodeError:
+ return value # Let Pydantic report its string_unicode error.
+ if isinstance(value, str) and not value.strip():
+ raise PydanticCustomError("empty_text", "table_name cannot be
empty")
+ return value
+
@field_validator("dataset_id", mode="before")
@classmethod
def reject_bool_identifier(cls, value: object) -> object:
@@ -1074,8 +1118,11 @@ class UpdateDatasetRequest(OmittedMeansUnchanged):
"sync_columns must be true. Updatable properties: "
f"{sorted(UPDATABLE_DATASET_FIELDS)}."
)
- if "table_name" in provided and not (self.table_name or "").strip():
- raise ValueError("table_name cannot be empty or null")
+ if "table_name" in provided and self.table_name is None:
+ with field_validation_error("table_name"):
+ raise PydanticCustomError(
+ "empty_text", "table_name cannot be empty or null"
+ )
if "sql" in provided and not (self.sql or "").strip():
raise ValueError("sql cannot be empty or null")
return self
diff --git a/superset/mcp_service/sql_lab/schemas.py
b/superset/mcp_service/sql_lab/schemas.py
index f173505bc6f..8124f59110e 100644
--- a/superset/mcp_service/sql_lab/schemas.py
+++ b/superset/mcp_service/sql_lab/schemas.py
@@ -27,6 +27,7 @@ from pydantic import (
field_validator,
model_serializer,
)
+from pydantic_core import PydanticCustomError
from superset.mcp_service.utils.serialization import (
JsonSafeRows,
@@ -264,11 +265,16 @@ class SaveSqlQueryRequest(BaseModel):
raise ValueError("SQL query cannot be empty")
return v.strip()
- @field_validator("label")
+ @field_validator("label", mode="before")
@classmethod
- def label_not_empty(cls, v: str) -> str:
- if not v or not v.strip():
- raise ValueError("Label cannot be empty")
+ def label_not_empty(cls, v: object) -> object:
+ """Trim labels and reject blanks before checking their length."""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
+ if not isinstance(v, str):
+ return v
+ if not v.strip():
+ raise PydanticCustomError("empty_text", "Label cannot be empty")
return v.strip()
@@ -324,13 +330,16 @@ class OpenSqlLabRequest(BaseModel):
max_length=256,
)
- @field_validator("title")
+ @field_validator("title", mode="before")
@classmethod
- def title_strip_or_none(cls, v: str | None) -> str | None:
+ def title_strip_or_none(cls, v: object) -> object:
+ """Normalize tab titles before enforcing the persisted length cap."""
+ if isinstance(v, (bytes, bytearray)):
+ v = v.decode()
# Whitespace-only would render as a blank tab label; fall back to
# SQL Lab's default "Untitled Query N" naming instead.
- if v is None:
- return None
+ if not isinstance(v, str):
+ return v
stripped = v.strip()
return stripped or None
diff --git a/superset/mcp_service/theme/schemas.py
b/superset/mcp_service/theme/schemas.py
index 2e0a35aeff7..19f683b9a19 100644
--- a/superset/mcp_service/theme/schemas.py
+++ b/superset/mcp_service/theme/schemas.py
@@ -31,6 +31,7 @@ from pydantic import (
field_validator,
model_serializer,
)
+from pydantic_core import PydanticCustomError
from superset.daos.base import ColumnOperator, ColumnOperatorEnum
from superset.mcp_service.common.pagination_schemas import (
@@ -139,7 +140,7 @@ class CreateThemeRequest(BaseModel):
theme_name: Annotated[
str,
- Field(description="Human-readable name for the theme"),
+ Field(description="Human-readable name for the theme", max_length=250),
]
json_data: Annotated[
dict[str, Any] | str,
@@ -149,12 +150,17 @@ class CreateThemeRequest(BaseModel):
),
]
- @field_validator("theme_name")
+ @field_validator("theme_name", mode="before")
@classmethod
- def reject_blank_theme_name(cls, value: str) -> str:
- """Mirror the REST ThemePostSchema check: no empty/whitespace names."""
- if not value or not value.strip():
- raise ValueError("Theme name cannot be empty.")
+ def reject_blank_theme_name(cls, value: object) -> object:
+ """Reject blank names before the length cap without trimming valid
names."""
+ if isinstance(value, (bytes, bytearray)):
+ try:
+ value = value.decode()
+ except UnicodeDecodeError:
+ return value # Let Pydantic report its string_unicode error.
+ if isinstance(value, str) and not value.strip():
+ raise PydanticCustomError("empty_text", "Theme name cannot be
empty.")
return value
diff --git a/superset/mcp_service/utils/sanitization.py
b/superset/mcp_service/utils/sanitization.py
index 421e6e88c39..b4ac808f95a 100644
--- a/superset/mcp_service/utils/sanitization.py
+++ b/superset/mcp_service/utils/sanitization.py
@@ -43,9 +43,9 @@ def _strip_html_tags(value: str) -> str:
Decodes all layers of HTML entity encoding BEFORE passing to nh3,
so entity-encoded tags (e.g., ``<script>``) are decoded into
real tags that nh3 can detect and strip. After nh3 removes all tags,
- we only restore ``&`` back to ``&`` (not a full html.unescape)
- to preserve ampersands in display text without risking XSS from
- re-introducing angle brackets or other HTML-significant characters.
+ we restore ampersands and less-than signs that cannot start HTML tags.
+ Tag-opening angle brackets remain escaped; a full html.unescape here
+ could reconstruct markup from text joined by tag removal.
Args:
value: The input string that may contain HTML
@@ -70,11 +70,42 @@ def _strip_html_tags(value: str) -> str:
# url_schemes=set() blocks all URL schemes in any remaining attributes
cleaned = nh3.clean(decoded, tags=set(), url_schemes=set())
- # Only restore & β & to preserve ampersands in display text (e.g. "A &
B").
- # Do NOT use html.unescape() here: nh3 may pass through HTML entities from
- # the input (e.g. <script>), and a full unescape would re-introduce
- # raw angle brackets, creating an XSS vector.
- return cleaned.replace("&", "&")
+ # Restore brackets only before comparison punctuation, numbers or
whitespace.
+ # Non-ASCII letters can case-fold into tag names in downstream renderers;
+ # leave those and reconstructed tag openers escaped.
+ # Normalize removable characters before deciding whether a bracket is safe;
+ # otherwise a later Unicode strip could turn <\u200bscript into markup.
+ cleaned = _remove_dangerous_unicode(cleaned)
+ # Greater-than signs and NBSP cannot open tags. Restore them so plain text
+ # neither changes on display nor outgrows the model-column length limits.
+ return (
+ re.sub(
+ # Control whitespace stays escaped: report titles strip it, which
+ # could join "<" to a Slack control sequence such as <!channel>.
+
r"<(?=[\d$β¬Β£Β₯%+=.,:;)\]&-]|[^\S\x00-\x1f\x7f-\x9f]|(?:\s| )*$)",
+ "<",
+ cleaned,
+ )
+ .replace(">", ">")
+ .replace(" ", "\u00a0")
+ .replace("&", "&")
+ )
+
+
+# A tag opener left escaped by _strip_html_tags: markup reconstructed by
+# decoding or by joining text around a stripped tag.
+_ESCAPED_TAG_OPENER_RE = re.compile(r"<[A-Za-z/!?]")
+
+
+def _check_stable_html(value: str, field_name: str) -> None:
+ """Reject stripped text that would change when submitted again."""
+ # Tag or Unicode removal can join entity fragments (e.g. &am<b></b>p;).
+ # Check the complete stripping pass, including recursive entity decoding,
+ # rather than just looking for literal escaped tag openers.
+ if _ESCAPED_TAG_OPENER_RE.search(value) or _strip_html_tags(value) !=
value:
+ raise PydanticCustomError(
+ "unsafe_text", f"{field_name} contains potentially malicious HTML"
+ )
def _scheme_pattern(scheme: str) -> str:
@@ -242,7 +273,7 @@ def sanitize_user_input(
Args:
value: The input string to sanitize
field_name: Name of the field (for error messages)
- max_length: Maximum allowed length
+ max_length: Maximum allowed sanitized length; raw input is bounded
separately
check_sql_keywords: Whether to check for SQL injection keywords
allow_empty: Whether to allow empty/None values
@@ -254,6 +285,7 @@ def sanitize_user_input(
Security checks performed:
- Strips all HTML tags using nh3 (Rust-based sanitizer)
+ - Rejects tag-shaped text left after stripping (e.g. encoded tags)
- Blocks JavaScript/VBScript/data URL schemes
- Blocks event handlers (onclick=, onerror=, etc.)
- Removes dangerous Unicode characters (zero-width, control chars)
@@ -271,12 +303,14 @@ def sanitize_user_input(
return None
raise PydanticCustomError("empty_text", f"{field_name} cannot be
empty")
- # Length check first to prevent ReDoS attacks
- if len(value) > max_length:
+ # Bound parser/regex work independently of the persisted text budget.
+ # Markup and entities can be longer than the text they sanitize to.
+ raw_max_length = max(10000, max_length)
+ if len(value) > raw_max_length:
raise PydanticCustomError(
"string_too_long",
f"{field_name} too long ({len(value)} characters). "
- f"Maximum allowed length is {max_length} characters.",
+ f"Maximum allowed length is {raw_max_length} characters.",
)
# Remove dangerous Unicode characters BEFORE any check so zero-widths
@@ -304,6 +338,22 @@ def sanitize_user_input(
# has been intentionally stripped away.
value = _strip_html_tags(value)
value = _remove_dangerous_unicode(value)
+ # Tag removal and entity decoding can expose surrounding whitespace.
+ # Apply the text cap to the trimmed value that callers persist.
+ value = value.strip()
+
+ # Reject rather than store tag-shaped text: a later sanitizer pass would
+ # decode and strip it, so the stored value would not be stable.
+ _check_stable_html(value, field_name)
+
+ # Conservative bracket escaping can expand text even after tags are
removed.
+ # Enforce the storage cap for callers without a Pydantic Field length
limit.
+ if len(value) > max_length:
+ raise PydanticCustomError(
+ "string_too_long",
+ f"{field_name} too long ({len(value)} characters). "
+ f"Maximum allowed length is {max_length} characters.",
+ )
# Check for dangerous patterns (URL schemes, event handlers)
_check_dangerous_patterns(value, field_name)
@@ -361,6 +411,8 @@ def sanitize_filter_value(
value = _strip_html_tags(value)
value = _remove_dangerous_unicode(value)
+ _check_stable_html(value, "Filter value")
+
# Check for dangerous patterns
_check_dangerous_patterns(value, "Filter value")
diff --git a/superset/mcp_service/utils/schema_utils.py
b/superset/mcp_service/utils/schema_utils.py
index 404733fb923..e027078c30a 100644
--- a/superset/mcp_service/utils/schema_utils.py
+++ b/superset/mcp_service/utils/schema_utils.py
@@ -46,8 +46,17 @@ def field_validation_error(field_name: str) ->
Iterator[None]:
"""
try:
yield
+ except UnicodeDecodeError as ex:
+ raise ValidationError.from_exception_data(
+ "Field validation",
+ [{"type": "string_unicode", "loc": (field_name,), "input":
ex.object}],
+ ) from ex
except ValueError as ex:
- error = PydanticCustomError("unsafe_text", "{error}", {"error":
str(ex)})
+ error = (
+ ex
+ if isinstance(ex, PydanticCustomError)
+ else PydanticCustomError("unsafe_text", "{error}", {"error":
str(ex)})
+ )
raise ValidationError.from_exception_data(
"Field validation", [{"type": error, "loc": (field_name,)}]
) from ex
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
b/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
index 8c882b2c269..e89e4d9a1d3 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
@@ -705,6 +705,8 @@ async def _generate_saved_chart(
refetch: Any,
compile_result: CompileResult | None = None,
config: Any = None,
+ chart_name: str | None = None,
+ validate: Any = None,
) -> tuple[Any, _DetachableSlice, Mock]:
"""Run generate_chart(save_chart=True) with a chart that detaches on
commit.
@@ -713,6 +715,7 @@ async def _generate_saved_chart(
"""
request = GenerateChartRequest(
dataset_id="1",
+ chart_name=chart_name,
config=config
or TableChartConfig(chart_type="table",
columns=[ColumnRef(name="region")]),
save_chart=True,
@@ -734,7 +737,16 @@ async def _generate_saved_chart(
# The instance is detached right after the commit, before any of the reads
# that build the response.
session.refresh.side_effect = lambda _chart: chart.detach()
- create_command = Mock(return_value=Mock(run=Mock(return_value=chart)))
+
+ command_instance = Mock(run=Mock(return_value=chart))
+
+ def create(payload: dict[str, Any]) -> Mock:
+ """Emulate storing the name passed to the create command."""
+ if chart_name is not None:
+ chart._values["slice_name"] = payload["slice_name"]
+ return command_instance
+
+ create_command = Mock(return_value=command_instance, side_effect=create)
with (
patch(
@@ -745,6 +757,7 @@ async def _generate_saved_chart(
"superset.mcp_service.chart.validation.ValidationPipeline."
"validate_request_with_warnings",
return_value=validation_result,
+ side_effect=validate,
),
patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=dataset),
patch(
@@ -1162,3 +1175,61 @@ async def
test_generate_saved_chart_retains_id_after_response_size_failure() ->
assert result.chart.id == 42
assert result.form_data == {}
assert result.warnings
+
+
[email protected]
[email protected](
+ "title",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+async def test_generate_chart_literal_less_than_storage_round_trip(title: str)
-> None:
+ """The create command and readback retain literal non-tag less-than
signs."""
+ result, _, create = await _generate_saved_chart(
+ refetch=Mock(side_effect=SQLAlchemyError("use captured stored
scalars")),
+ chart_name=title,
+ )
+ assert create.call_args.args[0]["slice_name"] == title
+ assert result.success is True
+ assert result.chart.slice_name == title
+
+
[email protected]
[email protected](
+ "chart_name", ["Weekly <img src=x>KPIs", "Q3 <b>revenue</b> & costs"]
+)
+async def test_generate_chart_revalidation_keeps_single_sanitized_name(
+ chart_name: str,
+) -> None:
+ """Schema revalidation persists the name sanitized once at tool entry."""
+ from superset.mcp_service.chart.validation.schema_validator import (
+ SchemaValidator,
+ )
+ from superset.mcp_service.utils.sanitization import sanitize_user_input
+
+ expected = sanitize_user_input(chart_name, "Chart name", max_length=250)
+ with patch(
+ "superset.mcp_service.utils.sanitization.sanitize_user_input",
+ wraps=sanitize_user_input,
+ ) as sanitize:
+
+ def validate(request_data: dict[str, Any]) -> ValidationResult:
+ is_valid, request, error =
SchemaValidator.validate_request(request_data)
+ return ValidationResult(
+ is_valid=is_valid, request=request, error=error, warnings={}
+ )
+
+ result, _, create = await _generate_saved_chart(
+ refetch=Mock(side_effect=SQLAlchemyError("use captured stored
scalars")),
+ chart_name=chart_name,
+ validate=validate,
+ )
+ name_calls = [c for c in sanitize.call_args_list if c.args[1] == "Chart
name"]
+ assert len(name_calls) == 1
+ assert result.success is True
+ assert create.call_args.args[0]["slice_name"] == expected
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
index 6ac309128d5..52203bd0c26 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
@@ -1077,6 +1077,7 @@ class TestUpdateChartNameOnly:
@patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
@patch("superset.db.session")
@pytest.mark.asyncio
+ @pytest.mark.parametrize("sanitized", [False, True])
async def test_name_only_update_success(
self,
mock_db_session,
@@ -1084,6 +1085,7 @@ class TestUpdateChartNameOnly:
mock_update_cmd_cls,
mock_check_access,
mcp_server,
+ sanitized: bool,
):
"""Successful name-only update (identifier + chart_name, no config)."""
mock_chart = Mock()
@@ -1110,13 +1112,18 @@ class TestUpdateChartNameOnly:
request = {
"identifier": 1,
- "chart_name": "Renamed Chart",
+ "chart_name": "Renamed Chart <img src=x>" if sanitized else
"Renamed Chart",
"generate_preview": False,
}
async with Client(mcp) as client:
result = await client.call_tool("update_chart", {"request":
request})
+ if sanitized:
+ assert any(
+ "sanitization" in warning
+ for warning in result.structured_content["warnings"]
+ )
assert result.structured_content["success"] is True
assert result.structured_content["chart"]["slice_name"] ==
"Renamed Chart"
assert result.structured_content["chart"]["is_unsaved_state"] is
False
@@ -3451,3 +3458,53 @@ def
test_big_number_rebind_does_not_inherit_saved_temporal_column() -> None:
assert params["viz_type"] == "big_number_total"
assert "granularity_sqla" not in params
+
+
[email protected]
[email protected](
+ "title",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+async def test_update_chart_literal_less_than_storage_round_trip(title: str)
-> None:
+ """Rename persistence and the tool response use the same literal text."""
+ chart = Mock(id=1, datasource_id=10, slice_name="Old", viz_type="table",
uuid="abc")
+
+ def update(_: int, payload: dict[str, Any]) -> Mock:
+ """Emulate the update command's persisted attribute mutation."""
+ chart.slice_name = payload["slice_name"]
+ return Mock(run=Mock(return_value=chart))
+
+ with (
+ patch("superset.db.session"),
+ patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart),
+ patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ return_value=DatasetValidationResult(
+ is_valid=True, dataset_id=10, dataset_name="ds", warnings=[]
+ ),
+ ),
+ patch(
+ "superset.commands.chart.update.UpdateChartCommand",
side_effect=update
+ ) as command,
+ ):
+ async with Client(mcp) as client:
+ result = await client.call_tool(
+ "update_chart",
+ {
+ "request": {
+ "identifier": 1,
+ "chart_name": title,
+ "generate_preview": False,
+ }
+ },
+ )
+ assert command.call_args.args[1]["slice_name"] == title
+ assert chart.slice_name == title
+ assert result.structured_content["success"] is True
+ assert result.structured_content["chart"]["slice_name"] == title
diff --git
a/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
b/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
index 237a5924cc4..b87ec5f4e4a 100755
---
a/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
+++
b/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
@@ -319,6 +319,38 @@ class TestNormalizeColumnNames:
by_alias=True, exclude_unset=True
) == {"columnWidth": 120}
+ @pytest.mark.parametrize(
+ "chart_name", ["Weekly <img src=x>KPIs", "Q3 <b>revenue</b> &
costs"]
+ )
+ def test_pipeline_keeps_single_sanitized_chart_name(
+ self, mock_dataset_context: DatasetContext, chart_name: str
+ ) -> None:
+ """Normalization must not sanitize an already-sanitized name again."""
+ from superset.mcp_service.utils.sanitization import sanitize_user_input
+
+ request = GenerateChartRequest.model_validate(
+ {
+ "dataset_id": 18,
+ "chart_name": chart_name,
+ "config": {"chart_type": "table", "columns": [{"name":
"sales"}]},
+ }
+ )
+
+ with patch(
+ "superset.mcp_service.utils.sanitization.sanitize_user_input",
+ wraps=sanitize_user_input,
+ ) as sanitize:
+ normalized = ValidationPipeline._normalize_column_names(
+ request,
+ mock_dataset_context,
+ typed_config=request.config,
+ )
+
+ assert not [c for c in sanitize.call_args_list if c.args[1] == "Chart
name"]
+ assert isinstance(normalized.config, TableChartConfig)
+ assert normalized.config.columns[0].name == "Sales"
+ assert normalized.chart_name == request.chart_name
+
@patch.object(DatasetValidator, "_get_dataset_context")
def test_table_normalization_canonicalizes_raw_column_config_key(
self, mock_get_context, mock_dataset_context: DatasetContext
diff --git
a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
index 0a393ab62e3..80dbe216e68 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
@@ -2543,3 +2543,37 @@ class TestGenerateDashboardCreatorGroups:
)
assert mock_dashboard.viewers == [viewer]
+
+
[email protected]
[email protected](
+ "title",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+async def test_generate_dashboard_literal_less_than_storage_round_trip(
+ title: str, mcp_server: object
+) -> None:
+ """Dashboard creation and readback preserve non-tag less-than text."""
+ dashboard = _mock_dashboard(id=10, title="Old")
+ with (
+ patch("superset.models.dashboard.Dashboard") as model,
+ patch("superset.daos.dashboard.DashboardDAO.find_by_id") as find,
+ patch("superset.db.session") as session,
+ ):
+ _setup_generate_dashboard_mocks(
+ session, find, model, [_mock_chart()], dashboard
+ )
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "generate_dashboard",
+ {"request": {"chart_ids": [1], "dashboard_title": title}},
+ )
+ assert dashboard.dashboard_title == title
+ assert result.structured_content["error"] is None
+ assert result.structured_content["dashboard"]["dashboard_title"] == title
diff --git
a/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
b/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
index e22d2f5d144..241acfb8188 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
@@ -749,3 +749,37 @@ class TestUpdateDashboard:
match="characters that are removed during normalization",
):
UpdateDashboardRequest(identifier=1, slug="!!!")
+
+
[email protected]
[email protected](
+ "title",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+async def test_update_dashboard_literal_less_than_storage_round_trip(
+ title: str, mcp_server: object
+) -> None:
+ """Dashboard rename persistence agrees with the returned display title."""
+ dashboard = _mock_dashboard(title="Old")
+ with (
+ patch("superset.extensions.db.session"),
+ patch(
+ "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug",
+ return_value=dashboard,
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "update_dashboard",
+ {"request": {"identifier": 42, "dashboard_title": title}},
+ )
+ assert dashboard.dashboard_title == title
+ payload = json.loads(result.content[0].text)
+ assert payload.get("error") is None
+ assert payload["dashboard"]["dashboard_title"] == title
diff --git a/tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py
b/tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py
index 21d47ac9f7f..4b9d8b96dd0 100644
--- a/tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py
+++ b/tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py
@@ -44,7 +44,16 @@ from superset.mcp_service.dashboard.schemas import (
ManageDashboardCertificationRequest,
UpdateDashboardRequest,
)
+from superset.mcp_service.dataset.schemas import (
+ CreateDatasetMetricRequest,
+ CreateDatasetRequest,
+ CreateVirtualDatasetRequest,
+ UpdateDatasetMetricRequest,
+ UpdateDatasetRequest,
+)
from superset.mcp_service.middleware import ToolResultCompatibilityMiddleware
+from superset.mcp_service.sql_lab.schemas import OpenSqlLabRequest,
SaveSqlQueryRequest
+from superset.mcp_service.theme.schemas import CreateThemeRequest
from superset.utils import json
@@ -180,6 +189,77 @@ def test_title_sanitization_error_names_field(
assert exc.value.errors()[0]["loc"] == (field,)
+def make_title_server() -> FastMCP:
+ """Register the real title request schemas for wire-level validation."""
+ server = FastMCP("title-validation")
+ server.add_middleware(ToolResultCompatibilityMiddleware())
+
+ @server.tool
+ def generate_dashboard(request: GenerateDashboardRequest) -> str:
+ """Validate dashboard creation."""
+ return "ok"
+
+ @server.tool
+ def update_dashboard(request: UpdateDashboardRequest) -> str:
+ """Validate dashboard updates."""
+ return "ok"
+
+ @server.tool
+ def duplicate_dashboard(request: DuplicateDashboardRequest) -> str:
+ """Validate dashboard duplication."""
+ return "ok"
+
+ @server.tool
+ def generate_chart(request: GenerateChartRequest) -> str:
+ """Validate chart creation."""
+ return "ok"
+
+ @server.tool
+ def update_chart(request: UpdateChartRequest) -> str:
+ """Validate chart updates."""
+ return "ok"
+
+ return server
+
+
+def make_name_server() -> FastMCP:
+ """Register required object-name request schemas for wire validation."""
+ server = FastMCP("name-validation")
+ server.add_middleware(ToolResultCompatibilityMiddleware())
+
+ @server.tool
+ def create_virtual_dataset(request: CreateVirtualDatasetRequest) -> str:
+ """Validate virtual dataset creation."""
+ return "ok"
+
+ @server.tool
+ def create_theme(request: CreateThemeRequest) -> str:
+ """Validate theme creation."""
+ return "ok"
+
+ @server.tool
+ def create_dataset_metric(request: CreateDatasetMetricRequest) -> str:
+ """Validate metric creation."""
+ return "ok"
+
+ @server.tool
+ def update_dataset_metric(request: UpdateDatasetMetricRequest) -> str:
+ """Validate metric updates."""
+ return "ok"
+
+ @server.tool
+ def update_dataset(request: UpdateDatasetRequest) -> str:
+ """Validate dataset updates."""
+ return "ok"
+
+ @server.tool
+ def save_sql_query(request: SaveSqlQueryRequest) -> str:
+ """Validate saved query creation."""
+ return "ok"
+
+ return server
+
+
@pytest.mark.asyncio
@pytest.mark.parametrize(
("tool", "payload", "field"),
@@ -208,6 +288,7 @@ def test_title_sanitization_error_names_field(
"java\tscript:private-input",
"vb\nscript:private-input",
"dat\ra:text/html,private-input",
+ "Weekly &am<b></b>p;lt;b&gt;KPIs",
],
)
async def
test_title_sanitization_wire_error_names_field_with_actionable_reason(
@@ -218,33 +299,7 @@ async def
test_title_sanitization_wire_error_names_field_with_actionable_reason(
rejected: str,
) -> None:
"""Real MCP argument validation names fields without exposing rejected
input."""
- server = FastMCP("title-validation")
- server.add_middleware(ToolResultCompatibilityMiddleware())
-
- @server.tool
- def generate_dashboard(request: GenerateDashboardRequest) -> str:
- """Validate dashboard creation."""
- return "ok"
-
- @server.tool
- def update_dashboard(request: UpdateDashboardRequest) -> str:
- """Validate dashboard updates."""
- return "ok"
-
- @server.tool
- def duplicate_dashboard(request: DuplicateDashboardRequest) -> str:
- """Validate dashboard duplication."""
- return "ok"
-
- @server.tool
- def generate_chart(request: GenerateChartRequest) -> str:
- """Validate chart creation."""
- return "ok"
-
- @server.tool
- def update_chart(request: UpdateChartRequest) -> str:
- """Validate chart updates."""
- return "ok"
+ server = make_title_server()
async with Client(server) as client:
result = await client.call_tool_mcp(
@@ -297,6 +352,7 @@ async def
test_title_sanitization_wire_error_names_field_with_actionable_reason(
[
"Exec (source data: source-a, source-b) private-input",
"javascript:private-input",
+ "Weekly &am<b></b>p;lt;b&gt;KPIs",
],
)
async def
test_sibling_sanitization_wire_error_names_field_with_actionable_reason(
@@ -524,3 +580,1172 @@ async def
test_sibling_alias_sanitization_wire_error_names_canonical_field(
)
assert f"request.{field}: Contains disallowed content" in message
assert "private-input" not in message
+
+
[email protected]
[email protected](
+ ("tool", "payload", "field", "limit"),
+ [
+ ("generate_dashboard", {"chart_ids": [1]}, "dashboard_title", 500),
+ ("update_dashboard", {"identifier": 1}, "dashboard_title", 500),
+ ("duplicate_dashboard", {"dashboard_id": 1}, "dashboard_title", 500),
+ (
+ "generate_chart",
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ 250,
+ ),
+ ("update_chart", {"identifier": 1}, "chart_name", 250),
+ ],
+)
[email protected]("alias", [None, "title", "name"])
[email protected]("length", [None, 600])
+async def test_title_length_wire_reason(
+ tool: str,
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ alias: str | None,
+ length: int | None,
+) -> None:
+ """Length errors retain their reason through warning detection and MCP."""
+ rejected = "x" * (length or limit + 1)
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ tool, {"request": {**payload, alias or field: rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text=f"Error: Validation error in {tool}: request.{field}: "
+ "String exceeds the allowed maximum length",
+ )
+ ]
+
+
[email protected]
[email protected]("field", ["dashboard_title", "title", "name"])
[email protected]("rejected", ["", " \t\n"])
+async def test_duplicate_dashboard_empty_title_wire_reason(
+ field: str,
+ rejected: str,
+) -> None:
+ """Blank required titles have a static, actionable reason on the wire."""
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ "duplicate_dashboard", {"request": {"dashboard_id": 1, field:
rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text="Error: Validation error in duplicate_dashboard: "
+ "request.dashboard_title: Text must not be empty",
+ )
+ ]
+
+
+_REQUIRED_NAME_CASES: list[tuple[str, dict[str, Any], str, list[str | None]]]
= [
+ (
+ "create_virtual_dataset",
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ ["", " \t\n"],
+ ),
+ ("create_theme", {"json_data": {}}, "theme_name", ["", " \t\n"]),
+ (
+ "create_dataset_metric",
+ {"dataset_id": 1, "expression": "count(*)"},
+ "metric_name",
+ ["", " \t\n"],
+ ),
+ (
+ "update_dataset_metric",
+ {"dataset_id": 1, "metric": 1},
+ "metric_name",
+ ["", " \t\n", None],
+ ),
+ ("update_dataset", {"dataset_id": 1}, "table_name", ["", " \t\n", None]),
+ (
+ "save_sql_query",
+ {"database_id": 1, "sql": "select 1"},
+ "label",
+ ["", " \t\n"],
+ ),
+]
+
+
+_BLANK_ERROR_LIMIT_CASES: list[tuple[type[BaseModel], dict[str, Any], str,
int]] = [
+ (UpdateDashboardRequest, {"identifier": 1}, "dashboard_title", 500),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}, "dashboard_title", 500),
+ (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ 250,
+ ),
+ (CreateThemeRequest, {"json_data": {}}, "theme_name", 250),
+ (
+ CreateDatasetMetricRequest,
+ {"dataset_id": 1, "expression": "count(*)"},
+ "metric_name",
+ 255,
+ ),
+ (UpdateDatasetMetricRequest, {"dataset_id": 1, "metric": 1},
"metric_name", 255),
+ (UpdateDatasetRequest, {"dataset_id": 1}, "table_name", 250),
+ (
+ SaveSqlQueryRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "label",
+ 256,
+ ),
+]
+
+
[email protected]("model,payload,field,limit", _BLANK_ERROR_LIMIT_CASES)
[email protected]("input_type", [str, bytes, bytearray])
[email protected]("whitespace", [" ", "\u2003"])
+def test_documented_blank_errors_precede_length(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ input_type: type[str] | type[bytes] | type[bytearray],
+ whitespace: str,
+) -> None:
+ """Every documented blank-name error takes precedence over its field
cap."""
+ blank = whitespace * (limit + 1)
+ value = blank if input_type is str else input_type(blank.encode())
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ errors = exc.value.errors()
+ assert len(errors) == 1
+ assert errors[0]["type"] == "empty_text"
+ assert errors[0]["loc"] == (field,)
+
+
[email protected]("model,payload,field,limit", _BLANK_ERROR_LIMIT_CASES)
[email protected]("input_type", [str, bytes, bytearray])
[email protected]("extra", [0, 1])
+def test_documented_name_nonblank_length_boundary(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ input_type: type[str] | type[bytes] | type[bytearray],
+ extra: int,
+) -> None:
+ """Blank-first validation must not bypass the cap for nonblank names."""
+ text = "x" * (limit + extra)
+ value = text if input_type is str else input_type(text.encode())
+ if extra:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ assert exc.value.errors()[0]["loc"] == (field,)
+ else:
+ assert getattr(model.model_validate({**payload, field: value}), field)
== text
+
+
+_LITERAL_NAME_LIMIT_CASES = [
+ case
+ for case in _BLANK_ERROR_LIMIT_CASES
+ if case[2] in ("theme_name", "metric_name", "table_name")
+]
+
+
[email protected]("model,payload,field,limit",
_LITERAL_NAME_LIMIT_CASES)
[email protected]("input_type", [str, bytes, bytearray])
[email protected]("extra", [0, 1])
+def test_literal_names_keep_nonblank_whitespace(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ input_type: type[str] | type[bytes] | type[bytearray],
+ extra: int,
+) -> None:
+ """Blank-first checks preserve literal names and count their whitespace."""
+ text = " " + "x" * (limit - 2 + extra) + " "
+ value = text if input_type is str else input_type(text.encode())
+ if extra:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ else:
+ assert getattr(model.model_validate({**payload, field: value}), field)
== text
+
+
[email protected]("model,payload,field,limit",
_LITERAL_NAME_LIMIT_CASES)
[email protected](
+ "value,error_type",
+ [
+ (b"\xff", "string_unicode"),
+ (bytearray(b"\xff"), "string_unicode"),
+ (1, "string_type"),
+ ],
+)
+def test_literal_names_reject_malformed_inputs(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ value: bytes | bytearray | int,
+ error_type: str,
+) -> None:
+ """Blank-first checks leave malformed-input errors to Pydantic."""
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == error_type
+ assert exc.value.errors()[0]["loc"] == (field,)
+
+
[email protected]
[email protected](
+ "tool,payload,field",
+ [(tool, payload, field) for tool, payload, field, _ in
_REQUIRED_NAME_CASES],
+)
[email protected]("extra", [1, 50])
+async def test_required_name_over_cap_blank_wire_reason(
+ tool: str,
+ payload: dict[str, Any],
+ field: str,
+ extra: int,
+) -> None:
+ """Over-cap blank names retain the documented safe reason over MCP."""
+ limit = {"metric_name": 255, "label": 256}.get(field, 250)
+ async with Client(make_name_server()) as client:
+ result = await client.call_tool_mcp(
+ tool, {"request": {**payload, field: " " * (limit + extra)}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text=f"Error: Validation error in {tool}: request.{field}: "
+ "Text must not be empty",
+ )
+ ]
+
+
[email protected]
[email protected](
+ ("tool", "payload", "field", "rejected"),
+ [
+ (tool, payload, field, rejected)
+ for tool, payload, field, blanks in _REQUIRED_NAME_CASES
+ for rejected in blanks
+ ],
+)
+async def test_required_name_empty_wire_reason(
+ tool: str, payload: dict[str, Any], field: str, rejected: str | None
+) -> None:
+ """Blank object names report a safe reason at the declared field."""
+ async with Client(make_name_server()) as client:
+ result = await client.call_tool_mcp(
+ tool, {"request": {**payload, field: rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text=f"Error: Validation error in {tool}: request.{field}: "
+ "Text must not be empty",
+ )
+ ]
+
+
[email protected](
+ "model,payload",
+ [
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ ),
+ (UpdateChartRequest, {"identifier": 1}),
+ ],
+)
[email protected]("length", [250, 251, 252, 253, 254, 255])
+def test_chart_name_model_column_boundary(
+ model: type[BaseModel], payload: dict[str, Any], length: int
+) -> None:
+ """Chart names must fit the persisted Slice name column."""
+ from superset.models.slice import Slice
+
+ limit = Slice.__table__.c.slice_name.type.length
+ assert limit == 250
+ if length <= limit:
+ assert (
+ model.model_validate({**payload, "chart_name": "x" *
length}).chart_name
+ == "x" * length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, "chart_name": "x" * length})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ assert exc.value.errors()[0]["loc"] == ("chart_name",)
+
+
[email protected](
+ "model,payload",
+ [
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ ),
+ (UpdateChartRequest, {"identifier": 1}),
+ ],
+)
[email protected]("alias", ["chart_name", "name", "title",
"slice_name"])
[email protected]("length", [250, 251])
+def test_padded_chart_name_boundary(
+ model: type[BaseModel], payload: dict[str, Any], alias: str, length: int
+) -> None:
+ """Validate the trimmed chart name against the model column boundary."""
+ value = " " + "x" * length + " "
+ if length == 250:
+ assert (
+ model.model_validate({**payload, alias: value}).chart_name == "x"
* length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, alias: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected]("length", [250, 251, 310])
+def test_theme_name_model_column_boundary(length: int) -> None:
+ """Theme creation must respect its persisted name column."""
+ from superset.models.core import Theme
+
+ assert Theme.__table__.c.theme_name.type.length == 250
+ if length == 250:
+ assert (
+ CreateThemeRequest(theme_name="x" * length,
json_data={}).theme_name
+ == "x" * length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ CreateThemeRequest(theme_name="x" * length, json_data={})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected]
[email protected]("alias", ["dashboard_title", "title", "name"])
[email protected]("value", ["", " \t\n"])
+async def test_update_dashboard_blank_title_wire_reason(alias: str, value:
str) -> None:
+ """Reject blank renames rather than reporting a title that was not
saved."""
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ "update_dashboard", {"request": {"identifier": 1, alias: value}}
+ )
+ assert result.isError is True
+ assert "request.dashboard_title: Text must not be empty" in
result.content[0].text
+
+
[email protected]("alias", ["chart_name", "name", "title",
"slice_name"])
[email protected]("script_only", [True, False])
+def test_update_chart_sanitization_matches_generate(
+ alias: str, script_only: bool
+) -> None:
+ """Update rejects stripped-only names and warns on partially stripped
names."""
+ if script_only:
+ with pytest.raises(ValidationError) as exc:
+ UpdateChartRequest.model_validate(
+ {"identifier": 1, alias: "<script>alert(1)</script>"}
+ )
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+ assert exc.value.errors()[0]["loc"] == ("chart_name",)
+ return
+ request = UpdateChartRequest.model_validate(
+ {
+ "identifier": 1,
+ alias: "Weekly KPIs <img src=x onerror=alert(1)>",
+ "sanitization_warnings": ["untrusted"],
+ }
+ )
+ assert request.chart_name == "Weekly KPIs"
+ assert len(request.sanitization_warnings) == 1
+ assert "untrusted" not in request.sanitization_warnings
+ assert "sanitization_warnings" not in json.dumps(
+ UpdateChartRequest.model_json_schema()
+ )
+
+
+def test_duplicate_dataset_error_is_readable() -> None:
+ """Duplicate errors use a plain string and preserve display-name
characters."""
+ from superset.commands.dataset.exceptions import
DatasetExistsValidationError
+ from superset.sql.parse import Table
+
+ name = "Sales βQ3β & Growth"
+ error = DatasetExistsValidationError(
+ Table(name, schema="my schema", catalog="my catalog")
+ )
+ assert error.normalized_messages() == {
+ "table": [f"Dataset my catalog.my schema.{name} already exists"]
+ }
+ assert type(error.messages[0]) is str
+
+
[email protected](
+ "kind,limit", [("dataset", 250), ("saved_query", 256), ("tab", 256)]
+)
[email protected]("extra", [0, 1])
+def test_padded_sibling_name_boundary(kind: str, limit: int, extra: int) ->
None:
+ """Persisted names that trim must check the length after trimming."""
+ from superset.mcp_service.sql_lab.schemas import OpenSqlLabRequest
+
+ cases: dict[str, tuple[type[BaseModel], dict[str, Any], str]] = {
+ "dataset": (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ ),
+ "saved_query": (
+ SaveSqlQueryRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "label",
+ ),
+ "tab": (OpenSqlLabRequest, {"database_id": 1}, "title"),
+ }
+ model, payload, field = cases[kind]
+ value = "x" * (limit + extra)
+ if not extra:
+ assert (
+ getattr(model.model_validate({**payload, field: f" {value} "}),
field)
+ == value
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: f" {value} "})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected](
+ "model,payload",
+ [
+ (GenerateDashboardRequest, {"chart_ids": [1]}),
+ (UpdateDashboardRequest, {"identifier": 1}),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}),
+ ],
+)
[email protected]("alias", ["dashboard_title", "title", "name"])
[email protected]("length", [500, 501])
+def test_padded_dashboard_title_boundary(
+ model: type[BaseModel], payload: dict[str, Any], alias: str, length: int
+) -> None:
+ """Dashboard aliases check the trimmed title against the column cap."""
+ if length == 500:
+ assert (
+ model.model_validate(
+ {**payload, alias: " " + "x" * length + " "}
+ ).dashboard_title
+ == "x" * length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, alias: " " + "x" * length + "
"})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
+def test_padded_dashboard_slug_boundary() -> None:
+ """Update slug normalization precedes its persisted column limit."""
+ request = UpdateDashboardRequest(identifier=1, slug=" " + "x" * 255 + "
")
+ assert request.slug == "x" * 255
+ with pytest.raises(ValidationError):
+ UpdateDashboardRequest(identifier=1, slug=" " + "x" * 256 + " ")
+
+
+_TITLE_SANITIZATION_CASES = [
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ "save_chart": False,
+ },
+ "chart_name",
+ ),
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ ),
+ (UpdateChartRequest, {"identifier": 1}, "chart_name"),
+ (GenerateDashboardRequest, {"chart_ids": [1]}, "dashboard_title"),
+ (UpdateDashboardRequest, {"identifier": 1}, "dashboard_title"),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}, "dashboard_title"),
+]
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "Users aged <30 & βVIPβ π",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+def test_literal_less_than_title_round_trip(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Non-tag less-than signs survive validation and JSON readback
unchanged."""
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == value
+ assert request.sanitization_warnings == []
+ assert json.loads(request.model_dump_json())[field] == value
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected](
+ "value", ["Weekly <img src=x>KPIs", "Q3 <b>revenue</b> & costs"]
+)
+def test_title_sanitized_once(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Persist the single sanitizer result, never a second-pass empty
fallback."""
+ from unittest.mock import patch
+
+ from superset.mcp_service.utils.sanitization import sanitize_user_input
+
+ expected = sanitize_user_input(value, "Title", max_length=500,
allow_empty=True)
+ assert expected
+ with (
+ patch(
+ "superset.mcp_service.utils.sanitization.sanitize_user_input",
+ wraps=sanitize_user_input,
+ ) as sanitize,
+ patch("superset.mcp_service.chart.schemas.sanitize_user_input",
sanitize),
+ patch("superset.mcp_service.dashboard.schemas.sanitize_user_input",
sanitize),
+ ):
+ request = model.model_validate({**payload, field: value})
+ calls = [
+ c
+ for c in sanitize.call_args_list
+ if c.args[1] in ("Chart name", "Dashboard title")
+ ]
+ assert len(calls) == 1
+ assert getattr(request, field) == expected
+ assert len(request.sanitization_warnings) == 1
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected](
+ "value", ["<<</b>x", "&lt;</script>click= <b>π"]
+)
+def test_reconstructed_tag_title_rejected(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Encoded tag text is rejected instead of stored or auto-named."""
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["loc"] == (field,)
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
+def test_empty_sanitized_title_never_uses_fallback(
+ model: type[BaseModel], payload: dict[str, Any], field: str
+) -> None:
+ """Encoded script-only names retain the existing disallowed-content
reason."""
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field:
"&lt;script>alert(1)</script>"})
+ assert exc.value.errors()[0]["loc"] == (field,)
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+
+
+_SIBLING_SANITIZATION_CASES = [
+ (ManageDashboardCertificationRequest, {"identifier": 1}, "certified_by"),
+ (
+ ManageDashboardCertificationRequest,
+ {"identifier": 1},
+ "certification_details",
+ ),
+ (BaseChartConfig, {}, "temporal_column"),
+ (ColumnRef, {}, "name"),
+ (ColumnRef, {"name": "amount"}, "label"),
+ (FilterConfig, {"op": "=", "value": "safe"}, "column"),
+ (FilterConfig, {"column": "region", "op": "="}, "value"),
+ (GanttSortByConfig, {}, "column"),
+ (
+ SunburstChartConfig,
+ {
+ "hierarchy": [{"name": "region"}],
+ "metric": {"name": "amount", "aggregate": "SUM"},
+ },
+ "time_range",
+ ),
+ (
+ InteractivePivotChartConfig,
+ {
+ "rows": [{"name": "region"}],
+ "metrics": [{"name": "amount", "aggregate": "SUM"}],
+ },
+ "comparison_period",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "time_range",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "range_labels",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "marker_labels",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "marker_line_labels",
+ ),
+]
+
+
[email protected]("model,payload,field", _SIBLING_SANITIZATION_CASES)
[email protected](
+ "value", ["Users aged <30", "Users aged >30", "Users\u00a0>30"]
+)
+def test_sibling_fields_preserve_literal_comparison(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Every sibling accepts a non-tag comparison and preserves its literal
text."""
+ if model is InteractivePivotChartConfig:
+ payload = {**payload, "comparison_type": "values"}
+ expected = [value] if field.endswith("_labels") else value
+ request = model.model_validate({**payload, field: expected})
+ assert getattr(request, field) == expected
+
+
[email protected]("model,payload,field", _SIBLING_SANITIZATION_CASES)
[email protected](
+ "value", ["<<</b>x", "&lt;</script>click= <b>π"]
+)
+def test_sibling_fields_reject_reconstructed_tag(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Reconstructed tag openers fail with a canonical field and typed
reason."""
+ if model is InteractivePivotChartConfig:
+ payload = {**payload, "comparison_type": "values"}
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate(
+ {**payload, field: [value] if field.endswith("_labels") else value}
+ )
+ assert [(error["type"], error["loc"]) for error in exc.value.errors()] == [
+ ("unsafe_text", (field,))
+ ]
+
+
[email protected](
+ "model,payload,field",
+ [
+ (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ ),
+ (UpdateDatasetRequest, {"dataset_id": 1}, "table_name"),
+ (
+ CreateDatasetMetricRequest,
+ {"dataset_id": 1, "expression": "SUM(x)"},
+ "metric_name",
+ ),
+ (UpdateDatasetMetricRequest, {"dataset_id": 1, "metric": 1},
"metric_name"),
+ (CreateThemeRequest, {"json_data": {}}, "theme_name"),
+ (SaveSqlQueryRequest, {"database_id": 1, "sql": "select 1"}, "label"),
+ (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1", "dataset_name": "ds"},
+ "description",
+ ),
+ (UpdateDatasetRequest, {"dataset_id": 1}, "description"),
+ (
+ SaveSqlQueryRequest,
+ {"database_id": 1, "sql": "select 1", "label": "query"},
+ "description",
+ ),
+ (UpdateDashboardRequest, {"identifier": 1}, "description"),
+ ],
+)
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "<<</b>x",
+ "&lt;</script>click= <b>π",
+ ],
+)
+def test_unsanitized_sibling_names_remain_literal(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Non-sanitizer paths retain their existing literal identifier/text
contract."""
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == value
+
+
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "<<</b>x",
+ "&lt;</script>click= <b>π",
+ ],
+)
+def test_sql_lab_title_remains_literal(value: str) -> None:
+ """SQL Lab tab titles trim whitespace without HTML sanitization."""
+ from superset.mcp_service.sql_lab.schemas import OpenSqlLabRequest
+
+ assert OpenSqlLabRequest(database_id=1, title=value).title == value
+
+
[email protected]
[email protected]("field", ["dashboard_title", "title", "name"])
[email protected](
+ "rejected", ["<script>alert(1)</script>", "Weekly
&am<b></b>p;lt;b&gt;KPIs"]
+)
+async def test_duplicate_dashboard_removed_or_unstable_title_wire_reason(
+ field: str, rejected: str
+) -> None:
+ """Removed or unstable markup is unsafe content, not an empty input."""
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ "duplicate_dashboard", {"request": {"dashboard_id": 1, field:
rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text="Error: Validation error in duplicate_dashboard: "
+ "request.dashboard_title: Contains disallowed content "
+ "(HTML, script or data: URL, event handler, or SQL keyword); "
+ "a 'data:' label must not be followed by a comma",
+ )
+ ]
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("character", [">", "\u00a0"])
[email protected]("at_limit", [True, False])
+def test_title_length_counts_plain_text(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ character: str,
+ at_limit: bool,
+) -> None:
+ """Entity serialization must not inflate persisted titles or reject valid
ones."""
+ limit = 250 if field == "chart_name" else 500
+ value = "x" * (limit - 2 + (not at_limit)) + character + "y"
+ if not at_limit:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ return
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == value
+ assert len(getattr(request, field)) == limit
+ assert request.sanitization_warnings == []
+ assert getattr(model.model_validate(request.model_dump()), field) == value
+
+
+def test_builtin_name_validation_documented_for_administrators() -> None:
+ """Built-in validation guidance belongs with server administration."""
+ from pathlib import Path
+
+ root = Path(__file__).resolve().parents[3]
+ admin = (root / "docs/admin_docs/configuration/mcp-server.mdx").read_text()
+ extensions = (root / "docs/developer_docs/extensions/mcp.md").read_text()
+ for heading in ("Chart and dashboard titles", "Object-name validation"):
+ assert heading in admin
+ assert heading not in extensions
+ assert (
+ "Nonblank theme names, saved-metric names, and dataset renames retain
their "
+ "whitespace and count it toward the limit."
+ ) in " ".join(admin.split())
+ assert "Blank-name checks precede length checks" in admin
+ assert "length validation applies after trimming" in admin
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
+def test_escaped_title_still_fits_model_column(
+ model: type[BaseModel], payload: dict[str, Any], field: str
+) -> None:
+ """Conservative bracket escaping cannot bypass a persisted title's length
cap."""
+ limit = 250 if field == "chart_name" else 500
+ value = "x" * (limit - 2) + "<ΕΏ"
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("whitespace", ["\t", "\n", "\r", "\v", "\f", "\x85"])
[email protected](
+ "control",
+ ["!channel", "@U123", "#C123|general", "https://example.invalid|Open
dashboard"],
+)
+def test_title_control_whitespace_cannot_form_slack_markup(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ whitespace: str,
+ control: str,
+) -> None:
+ """Report title normalization must not join brackets into Slack
controls."""
+ from superset.utils.file import sanitize_title
+
+ value = f"Q3 <{whitespace}{control}>"
+ request = model.model_validate({**payload, field: value})
+ stored = getattr(request, field)
+ assert "<" not in sanitize_title(stored)
+ assert getattr(model.model_validate(request.model_dump()), field) == stored
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("alias_index", range(4))
[email protected]("length_delta", [-5, 0, 1])
[email protected]("padding", ["", " ", " ", "\u00a0"])
+def test_markup_title_length_uses_sanitized_text(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ alias_index: int,
+ length_delta: int,
+ padding: str,
+) -> None:
+ """Markup is not part of the persisted title's character budget."""
+ aliases = (
+ ("chart_name", "name", "title", "slice_name")
+ if field == "chart_name"
+ else ("dashboard_title", "title", "name", "dashboard_title")
+ )
+ limit = 250 if field == "chart_name" else 500
+ text = "x" * (limit + length_delta)
+ value = "<b>" + padding + text + padding + "</b>"
+ if length_delta > 0:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, aliases[alias_index]: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ else:
+ request = model.model_validate({**payload, aliases[alias_index]:
value})
+ assert getattr(request, field) == text
+ assert len(request.sanitization_warnings) == 1
+
+
[email protected](
+ "model,payload,field",
+ [case for case in _SIBLING_SANITIZATION_CASES if case[2] != "value"],
+)
[email protected]("extra", [0, 1])
[email protected]("padding", ["", " ", " ", "\u00a0"])
+def test_sibling_sanitized_length_budget(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ extra: int,
+ padding: str,
+) -> None:
+ """All callers of the shared text sanitizer enforce the final text cap."""
+ limits = {
+ "certified_by": 500,
+ "certification_details": 5000,
+ "temporal_column": 255,
+ "name": 255,
+ "label": 500,
+ "column": 255,
+ "time_range": 1000,
+ "comparison_period": 100,
+ "range_labels": 200,
+ "marker_labels": 200,
+ "marker_line_labels": 200,
+ }
+ if model is InteractivePivotChartConfig:
+ payload = {**payload, "comparison_type": "values"}
+ text = "x" * (limits[field] + extra)
+ value = "<b>" + padding + text + padding + "</b>"
+ if extra:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate(
+ {**payload, field: [value] if field.endswith("_labels") else
value}
+ )
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ else:
+ request = model.model_validate(
+ {**payload, field: [value] if field.endswith("_labels") else value}
+ )
+ assert getattr(request, field) == (
+ [text] if field.endswith("_labels") else text
+ )
+
+
[email protected](
+ "model,payload,field",
+ _TITLE_SANITIZATION_CASES
+ + [
+ case
+ for case in _SIBLING_SANITIZATION_CASES
+ if case[2] != "value" and not case[2].endswith("_labels")
+ ],
+)
[email protected]("value", [b"<b>Safe</b>", bytearray(b"<b>Safe</b>")])
+def test_sibling_byte_strings_are_sanitized_before_coercion(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ value: bytes | bytearray,
+) -> None:
+ """Pydantic's byte-string decoding must not bypass before sanitizers."""
+ if model is InteractivePivotChartConfig:
+ payload = {**payload, "comparison_type": "values"}
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == "Safe"
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("alias_index", range(4))
[email protected]("input_type", [str, bytes, bytearray])
[email protected]("length", [0, 251, 501, 10001])
+def test_blank_titles_normalize_before_length(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ alias_index: int,
+ input_type: type[str] | type[bytes] | type[bytearray],
+ length: int,
+) -> None:
+ """Blank fallback or required-title errors precede the persisted length
cap."""
+ aliases = (
+ ("chart_name", "name", "title", "slice_name")
+ if field == "chart_name"
+ else ("dashboard_title", "title", "name", "dashboard_title")
+ )
+ blank = " " * length
+ value = blank if input_type is str else input_type(blank.encode())
+ if model in (UpdateDashboardRequest, DuplicateDashboardRequest):
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, aliases[alias_index]: value})
+ assert exc.value.errors()[0]["type"] == "empty_text"
+ assert exc.value.errors()[0]["loc"] == (field,)
+ else:
+ request = model.model_validate({**payload, aliases[alias_index]:
value})
+ expected = "" if model is GenerateDashboardRequest and length == 0
else None
+ assert getattr(request, field) == expected
+ assert request.sanitization_warnings == []
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("alias_index", range(4))
[email protected]("as_bytearray", [False, True])
[email protected]("extra", [0, 1])
+def test_byte_title_sanitized_length_boundary(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ alias_index: int,
+ as_bytearray: bool,
+ extra: int,
+) -> None:
+ """Decoded title aliases sanitize before their final text length is
checked."""
+ aliases = (
+ ("chart_name", "name", "title", "slice_name")
+ if field == "chart_name"
+ else ("dashboard_title", "title", "name", "dashboard_title")
+ )
+ limit = 250 if field == "chart_name" else 500
+ text = "x" * (limit + extra)
+ encoded = f"<b> {text} </b>".encode()
+ value = bytearray(encoded) if as_bytearray else encoded
+ if extra:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, aliases[alias_index]: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ else:
+ request = model.model_validate({**payload, aliases[alias_index]:
value})
+ assert getattr(request, field) == text
+ assert len(request.sanitization_warnings) == 1
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("as_bytearray", [False, True])
+def test_byte_title_removed_content_is_rejected(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ as_bytearray: bool,
+) -> None:
+ """Byte decoding cannot bypass the stripped-only title guard."""
+ encoded = b"<script>alert(1)</script>"
+ value = bytearray(encoded) if as_bytearray else encoded
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+ assert exc.value.errors()[0]["loc"] == (field,)
+
+
[email protected](
+ "model,payload,field,limit",
+ [
+ (CreateDatasetRequest, {"database_id": 1}, "table_name", 250),
+ (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ 250,
+ ),
+ (SaveSqlQueryRequest, {"database_id": 1, "sql": "select 1"}, "label",
256),
+ (OpenSqlLabRequest, {"database_id": 1}, "title", 256),
+ ],
+)
[email protected]("as_bytearray", [False, True])
+def test_byte_object_name_trims_before_length(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ as_bytearray: bool,
+) -> None:
+ """Literal-name models also normalize decoded whitespace before their
cap."""
+ text = "x" * limit
+ encoded = f" {text} ".encode()
+ value = bytearray(encoded) if as_bytearray else encoded
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == text
+
+
[email protected]("as_bytearray", [False, True])
[email protected](
+ "value,expected",
+ [(" " + "x" * 255 + " ", "x" * 255), (" My slug!? ", "My-slug"), (" " *
256, "")],
+)
+def test_byte_dashboard_slug_normalizes_before_length(
+ as_bytearray: bool, value: str, expected: str
+) -> None:
+ """Slug coercion also precedes its existing REST-compatible
normalization."""
+ encoded = value.encode()
+ byte_value = bytearray(encoded) if as_bytearray else encoded
+ request = UpdateDashboardRequest.model_validate(
+ {"identifier": 1, "slug": byte_value}
+ )
+ assert request.slug == expected
+
+
[email protected]("field", ["schema_", "schema", "catalog"])
[email protected]("as_bytearray", [False, True])
[email protected](
+ "value,expected",
+ [
+ (" public ", "public"),
+ (" ", None),
+ (" " * 251, None),
+ (" " + "x" * 250 + " ", "x" * 250),
+ ],
+)
+def test_byte_dataset_namespace_normalizes_before_length(
+ field: str, as_bytearray: bool, value: str, expected: str | None
+) -> None:
+ """Namespace bytes follow the same trim/blank contract as string inputs."""
+ encoded = value.encode()
+ request = CreateDatasetRequest.model_validate(
+ {
+ "database_id": 1,
+ "table_name": "t",
+ field: bytearray(encoded) if as_bytearray else encoded,
+ }
+ )
+ assert getattr(request, "schema_" if field == "schema" else field) ==
expected
+
+ from superset.mcp_service.dataset.tool.create_dataset import (
+ _build_dataset_properties,
+ )
+
+ properties, error = _build_dataset_properties(request)
+ assert error is None
+ expected_properties: dict[str, object] = {"database": 1, "table_name": "t"}
+ if expected is not None:
+ expected_properties["schema" if field == "schema_" else field] =
expected
+ assert properties == expected_properties
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("alias_index", range(4))
[email protected]("value", [b"\xff", bytearray(b"\xff")])
+def test_invalid_utf8_title_error_is_field_located(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ alias_index: int,
+ value: bytes | bytearray,
+) -> None:
+ """Model-level byte decoding retains Pydantic's field-level Unicode
error."""
+ aliases = (
+ ("chart_name", "name", "title", "slice_name")
+ if field == "chart_name"
+ else ("dashboard_title", "title", "name", "dashboard_title")
+ )
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, aliases[alias_index]: value})
+ assert exc.value.errors()[0]["type"] == "string_unicode"
+ assert exc.value.errors()[0]["loc"] == (field,)
+
+
[email protected](
+ "value", [b"/explore/p/safe-key/", bytearray(b"/explore/p/safe-key/")]
+)
+def test_byte_permalink_url_extracts_key(value: bytes | bytearray) -> None:
+ """Byte permalink URLs cannot skip the same normalization as text URLs."""
+ from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+ request = GetChartInfoRequest.model_validate({"permalink_key": value})
+ assert request.permalink_key == "safe-key"
+
+
[email protected]("field", ["schema_", "schema", "catalog"])
[email protected](
+ "value",
+ ["x" * 251, b"x" * 251, bytearray(b"x" * 251), 1, True, {}, b"\xff"],
+)
+def test_dataset_namespace_rejects_malformed_values(field: str, value: object)
-> None:
+ """Namespace trimming does not omit malformed inputs or bypass the cap."""
+ with pytest.raises(ValidationError) as exc:
+ CreateDatasetRequest.model_validate(
+ {"database_id": 1, "table_name": "t", field: value}
+ )
+ assert exc.value.errors()[0]["loc"] == (field,)
+
+
[email protected]("as_bytearray", [False, True])
+def test_byte_permalink_rejects_other_url(as_bytearray: bool) -> None:
+ """Byte URLs outside the Explore permalink path cannot become cache
keys."""
+ from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+ encoded = b"https://superset.example.com/superset/dashboard/p/key/";
+ with pytest.raises(ValidationError) as exc:
+ GetChartInfoRequest.model_validate(
+ {"permalink_key": bytearray(encoded) if as_bytearray else encoded}
+ )
+ assert exc.value.errors()[0]["loc"] == ("permalink_key",)
diff --git a/tests/unit_tests/mcp_service/utils/test_sanitization.py
b/tests/unit_tests/mcp_service/utils/test_sanitization.py
index 65e51d7bd53..1ac35afbf19 100644
--- a/tests/unit_tests/mcp_service/utils/test_sanitization.py
+++ b/tests/unit_tests/mcp_service/utils/test_sanitization.py
@@ -30,6 +30,7 @@ from superset.mcp_service.utils.sanitization import (
_strip_html_tags,
sanitize_filter_value,
sanitize_user_input,
+ sanitize_user_input_with_changes,
)
# --- _strip_html_tags tests ---
@@ -903,3 +904,194 @@ def
test_data_label_followed_by_control_whitespace_is_preserved(
"""Whitespace after a comma-free data: label may be a tab or line break."""
value = f"Substitute data:{separator}Jira support tickets"
assert sanitize(value) == value.replace("\r\n", "\n")
+
+
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "5 < 10",
+ "A < B",
+ "A <",
+ "A <> B",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+def test_strip_html_tags_preserves_non_tag_less_than(value: str) -> None:
+ """Preserve display text without decoding tag-opening brackets."""
+ assert _strip_html_tags(value) == value
+
+
[email protected](
+ "value",
+ [
+ "<</b>script>alert(1)",
+ "<​script>alert(1)",
+ "<</b>img src=x onerror=alert(1)",
+ ],
+)
+def test_strip_html_tags_does_not_reconstruct_tag_openers(value: str) -> None:
+ """Removed tags or invisible characters must not reveal raw markup."""
+ result = _strip_html_tags(value)
+ assert "<script" not in result
+ assert "<img" not in result
+
+
[email protected](
+ "value",
+ [
+ "<<b></b>script>alert(1)",
+ "<​script>alert(1)",
+ "&lt;</script>click= <b>π",
+ "<<</b>x",
+ ],
+)
+def test_sanitize_user_input_rejects_reconstructed_tag_text(value: str) ->
None:
+ """Tag-shaped text left after stripping is rejected, never stored."""
+ with pytest.raises(ValueError, match="malicious HTML"):
+ sanitize_user_input(value, "Title")
+
+
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "A <> B",
+ "a << b",
+ "Weekly <img src=x>KPIs",
+ "Q3 <b>revenue</b> & costs",
+ "<3 charts",
+ ],
+)
+def test_sanitize_user_input_is_stable_on_resubmission(value: str) -> None:
+ """Sanitizing a stored value again leaves it unchanged."""
+ once = sanitize_user_input(value, "Title")
+ assert once is not None
+ assert sanitize_user_input(once, "Title") == once
+
+
[email protected](
+ "sanitize",
+ [
+ partial(sanitize_user_input, field_name="Title"),
+ partial(sanitize_user_input_with_changes, field_name="Title"),
+ sanitize_filter_value,
+ ],
+)
[email protected](
+ "value",
+ [
+ "Weekly &am<b></b>p;lt;b&gt;KPIs",
+ "Weekly &l<b></b>t;b>KPIs",
+ "Weekly &#x<b></b>3c;b>KPIs",
+ "Weekly &co<b></b>py; KPIs",
+ "Weekly &am​p;lt;b&gt;KPIs",
+ ],
+)
+def test_reject_entities_reconstructed_by_removal(
+ sanitize: Callable[[str], object], value: str
+) -> None:
+ """Do not store text whose rebuilt entities change on resubmission."""
+ with pytest.raises(ValueError, match="malicious HTML"):
+ sanitize(value)
+
+
[email protected](
+ "value", ["Q3 <ΕΏcript src=//example.invalid/x.js", 'x <βͺbd title="y']
+)
+def test_strip_html_tags_keeps_casefolding_tag_openers_escaped(value: str) ->
None:
+ """Case conversion must not turn a restored bracket into an HTML tag."""
+ result = _strip_html_tags(value)
+ assert "<" not in result
+ assert "<" in result
+
+
[email protected]("value", [">5", "Users\u00a0>30"])
+def test_filter_value_preserves_greater_than_and_nbsp(value: str) -> None:
+ """Text comparisons must not gain entity semicolons rejected as shell
syntax."""
+ assert sanitize_filter_value(value) == value
+
+
[email protected]("whitespace", ["\t", "\n", "\r", "\v", "\f", "\x85"])
[email protected](
+ "sanitize",
+ [_strip_html_tags, partial(sanitize_user_input, field_name="title")],
+)
+def test_control_whitespace_keeps_less_than_escaped(
+ sanitize: Callable[[str], str], whitespace: str
+) -> None:
+ """All sanitization entry points keep control-separated brackets inert."""
+ from superset.utils.file import sanitize_title
+
+ result = sanitize(f"Q3 <{whitespace}!channel>")
+ assert "<" not in sanitize_title(result)
+
+
[email protected]("value", ["A <\t", "A <\n\u00a0", "A <\u00a0 10"])
+def test_strip_html_tags_preserves_safe_comparison_whitespace(value: str) ->
None:
+ """Trailing whitespace and visible comparison spacing cannot join
controls."""
+ assert _strip_html_tags(value) == value
+
+
[email protected]("whitespace", ["\t", "\n", "\r"])
+def test_filter_rejects_control_separated_slack_markup(whitespace: str) ->
None:
+ """Escaped control-separated brackets retain the filter shell-character
gate."""
+ with pytest.raises(ValueError, match="shell"):
+ sanitize_filter_value(f"Q3 <{whitespace}!channel>")
+
+
[email protected]("whitespace", ["\t", "\n", "\r", "	", " ",
" "])
+def test_control_whitespace_warning_wrapper_keeps_slack_markup_inert(
+ whitespace: str,
+) -> None:
+ """The warning wrapper must persist the escaped result, not the raw
title."""
+ from superset.utils.file import sanitize_title
+
+ result, changed = sanitize_user_input_with_changes(
+ f"Q3 <{whitespace}!channel>", field_name="title"
+ )
+ assert result is not None
+ assert sanitize_title(result) == "Q3 <!channel>"
+ assert changed is True
+
+
[email protected](
+ "sanitize", [sanitize_user_input, sanitize_user_input_with_changes]
+)
[email protected]("limit", [100, 200, 250, 255, 500, 1000, 5000])
[email protected](
+ "wrapper",
+ [
+ ("<b>", "</b>"),
+ ("<b> ", " </b>"),
+ ("<b> ", " </b>"),
+ ("<b>\u00a0", "\u00a0</b>"),
+ ("", "​"),
+ ("", "&"),
+ ],
+)
+def test_sanitized_length_budget(
+ sanitize: Callable[..., object], limit: int, wrapper: tuple[str, str]
+) -> None:
+ """Tag removal, Unicode removal and entity decoding precede the text
cap."""
+ prefix, suffix = wrapper
+ text = "x" * (limit - (1 if suffix == "&" else 0))
+ expected = text + ("&" if suffix == "&" else "")
+ result = sanitize(prefix + text + suffix, "Text", max_length=limit)
+ assert result == (
+ (expected, True) if sanitize is sanitize_user_input_with_changes else
expected
+ )
+
+
+def test_raw_sanitization_input_is_bounded_before_html_parsing() -> None:
+ """Keep a separate resource bound even if all markup would be stripped."""
+ from unittest.mock import patch
+
+ with patch("superset.mcp_service.utils.sanitization._strip_html_tags") as
strip:
+ with pytest.raises(ValueError, match="10000"):
+ sanitize_user_input("<b></b>" * 1429, "Text", max_length=250)
+ strip.assert_not_called()
