bito-code-review[bot] commented on code in PR #44560:
URL: https://github.com/apache/superset/pull/44560#discussion_r4117187284
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2344,6 +2348,246 @@ class ManageNativeFiltersResponse(BaseModel):
)
+# ---------------------------------------------------------------------------
+# manage_dashboard_markdown schemas
+# ---------------------------------------------------------------------------
+
+
+class BaseNewDashboardComponentSpec(BaseModel):
+ """Common placement fields shared by all new markdown/header/divider
specs."""
+
+ model_config = ConfigDict(extra="forbid")
+
+ target_tab: str | None = Field(
+ None,
+ description=(
+ "Tab to add the component to, matched by display name or "
+ "component ID (see get_dashboard_layout for available tabs). "
+ "Omit to use the first tab, or the grid if there are no tabs; "
+ "specify a target when the component should land in a "
+ "specific one rather than the first tab."
+ ),
+ )
+
+
+class MarkdownComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new markdown/text tile.
+
+ Placed in its own new row (a MARKDOWN component sits alongside charts,
+ not as a full-width band), so it composes with existing rows/charts on
+ the target grid or tab.
+ """
+
+ component_type: Literal["markdown"] = Field(
+ ..., description="Discriminator - must be 'markdown'"
+ )
+ code: str = Field(
+ ..., min_length=1, description="Markdown (and safe inline HTML) source"
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to apply an HTML allowlist sanitizer to
`MarkdownComponentSpec.code` is not necessary in this context. The dashboard
rendering process already utilizes `SafeMarkdown`, which applies
`rehype-sanitize` after raw-HTML parsing, and the application configuration
allows operators to control HTML sanitization globally. Since the system is
designed to store the original Markdown source and sanitize it at the point of
rendering, adding an additional sanitizer at the storage layer would be
redundant and inconsistent with the existing architecture.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2344,6 +2348,246 @@ class ManageNativeFiltersResponse(BaseModel):
)
+# ---------------------------------------------------------------------------
+# manage_dashboard_markdown schemas
+# ---------------------------------------------------------------------------
+
+
+class BaseNewDashboardComponentSpec(BaseModel):
+ """Common placement fields shared by all new markdown/header/divider
specs."""
+
+ model_config = ConfigDict(extra="forbid")
+
+ target_tab: str | None = Field(
+ None,
+ description=(
+ "Tab to add the component to, matched by display name or "
+ "component ID (see get_dashboard_layout for available tabs). "
+ "Omit to use the first tab, or the grid if there are no tabs; "
+ "specify a target when the component should land in a "
+ "specific one rather than the first tab."
+ ),
+ )
+
+
+class MarkdownComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new markdown/text tile.
+
+ Placed in its own new row (a MARKDOWN component sits alongside charts,
+ not as a full-width band), so it composes with existing rows/charts on
+ the target grid or tab.
+ """
+
+ component_type: Literal["markdown"] = Field(
+ ..., description="Discriminator - must be 'markdown'"
+ )
+ code: str = Field(
+ ..., min_length=1, description="Markdown (and safe inline HTML) source"
+ )
+ width: int = Field(
+ GRID_DEFAULT_CHART_WIDTH,
+ ge=1,
+ le=GRID_COLUMN_COUNT,
+ description=(
+ f"Tile width in grid columns (1-{GRID_COLUMN_COUNT}, "
+ f"default {GRID_DEFAULT_CHART_WIDTH})"
+ ),
+ )
+ height: int = Field(
+ 50,
+ ge=1,
+ description="Tile height in grid units (one unit is 8 pixels; default
50)",
+ )
+
+
+class HeaderComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new section header band.
+
+ Placed directly on the target grid/tab (not inside a row) so it spans
+ the full dashboard width, matching how the dashboard builder places
+ dragged header components.
+ """
+
+ component_type: Literal["header"] = Field(
+ ..., description="Discriminator - must be 'header'"
+ )
+ text: str = Field(..., min_length=1, description="Header display text")
+ header_size: Literal["SMALL_HEADER", "MEDIUM_HEADER", "LARGE_HEADER"] =
Field(
+ "MEDIUM_HEADER", description="Header text size"
+ )
+ background: Literal["BACKGROUND_TRANSPARENT", "BACKGROUND_WHITE"] = Field(
+ "BACKGROUND_TRANSPARENT", description="Header band background"
+ )
+
+ @field_validator("text")
+ @classmethod
+ def sanitize_text(cls, v: str) -> str:
+ """Sanitize header text to prevent XSS; it renders as plain title
text."""
+ sanitized: str | None = sanitize_user_input(
+ v, "text", max_length=500, allow_empty=True
+ )
+ if not sanitized:
+ raise ValueError("text has no content left after sanitization.")
+ return sanitized
+
+
+class DividerComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new horizontal divider.
+
+ Placed directly on the target grid/tab (not inside a row), same as
+ ``HeaderComponentSpec``. Carries no content — only placement.
+ """
+
+ component_type: Literal["divider"] = Field(
+ ..., description="Discriminator - must be 'divider'"
+ )
+
+
+NewDashboardComponentSpec = Annotated[
+ MarkdownComponentSpec | HeaderComponentSpec | DividerComponentSpec,
+ Field(discriminator="component_type"),
+]
+
+
+class DashboardComponentUpdateSpec(BaseModel):
+ """Partial update for an existing markdown/header/divider component.
+
+ ``id`` and at least one non-null update field are required. Provided
+ fields are merged into the existing component. Fields that only apply to
+ one component type (e.g. ``code`` for markdown, ``text``/``header_size``
+ for header) are rejected when used against the wrong component type.
+ A component's type cannot be changed; remove and re-add instead.
+ """
+
+ model_config = ConfigDict(extra="forbid")
+
+ id: str = Field(..., min_length=1, description="ID of the component to
update")
+ code: str | None = Field(
+ None, min_length=1, description="New markdown source (markdown only)"
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to sanitize `DashboardComponentUpdateSpec.code` is not
required because the existing architecture already ensures safety. The UI
writes the source code directly to the component's metadata, and the dashboard
rendering process uses `SafeMarkdown`, which applies `rehype-sanitize` after
parsing or leaves the HTML escaped based on the `ESCAPE_MARKDOWN_HTML`
configuration. Since the REST API and UI share the same storage and rendering
contract, the current implementation is consistent with existing security
practices.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
