bito-code-review[bot] commented on code in PR #40399:
URL: https://github.com/apache/superset/pull/40399#discussion_r3342922008
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +626,92 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestGenerateDashboardRequestLayoutTheme:
+ """generate_dashboard accepts optional position_json, theme overrides,
CSS."""
+
+ def test_layout_theme_css_fields_default_to_none(self) -> None:
+ req = GenerateDashboardRequest(chart_ids=[1])
+ assert req.position_json is None
+ assert req.json_metadata_overrides is None
+ assert req.css is None
+ assert req.slug is None
+
+ def test_position_json_accepted(self) -> None:
+ position = {
+ "ROOT_ID": {"children": ["GRID_ID"], "type": "ROOT"},
+ "GRID_ID": {"children": ["ROW-1"], "type": "GRID"},
+ }
+ req = GenerateDashboardRequest(chart_ids=[1], position_json=position)
+ assert req.position_json == position
+
+ def test_json_metadata_overrides_accepted(self) -> None:
+ overrides = {
+ "label_colors": {"Electronics": "#4C78A8"},
+ "cross_filters_enabled": False,
+ }
+ req = GenerateDashboardRequest(
+ chart_ids=[1], json_metadata_overrides=overrides
+ )
+ assert req.json_metadata_overrides == overrides
+
+ def test_css_accepted(self) -> None:
+ req = GenerateDashboardRequest(
+ chart_ids=[1], css=".header-controls{display:none}"
+ )
+ assert req.css == ".header-controls{display:none}"
+
+ def test_slug_accepted(self) -> None:
+ req = GenerateDashboardRequest(chart_ids=[1], slug="my-dashboard")
+ assert req.slug == "my-dashboard"
+
+ def test_css_max_length_enforced(self) -> None:
+ with pytest.raises(ValidationError, match="at most 50000"):
+ GenerateDashboardRequest(chart_ids=[1], css="x" * 50001)
+
+
+class TestUpdateDashboardRequest:
+ """Schema validation for update_dashboard's request."""
+
+ def test_identifier_required(self) -> None:
+ with pytest.raises(ValidationError, match="Field required"):
+ UpdateDashboardRequest() # type: ignore[call-arg]
+
+ def test_int_identifier_accepted(self) -> None:
+ req = UpdateDashboardRequest(identifier=42)
+ assert req.identifier == 42
+
+ def test_string_identifier_accepted(self) -> None:
+ req = UpdateDashboardRequest(identifier="my-slug")
+ assert req.identifier == "my-slug"
+
+ def test_all_optional_fields_default_to_none(self) -> None:
+ req = UpdateDashboardRequest(identifier=1)
+ assert req.dashboard_title is None
+ assert req.description is None
+ assert req.slug is None
+ assert req.published is None
+ assert req.position_json is None
+ assert req.json_metadata_overrides is None
+ assert req.css is None
+
+ def test_position_json_and_overrides_and_css(self) -> None:
+ req = UpdateDashboardRequest(
+ identifier=42,
+ position_json={"ROOT_ID": {"type": "ROOT"}},
+ json_metadata_overrides={"cross_filters_enabled": True},
+ css=".x{}",
+ )
+ assert req.position_json == {"ROOT_ID": {"type": "ROOT"}}
+ assert req.json_metadata_overrides == {"cross_filters_enabled": True}
+ assert req.css == ".x{}"
+
+ def test_title_alias_accepted(self) -> None:
+ """`title` is accepted as an alias for `dashboard_title`."""
+ req = UpdateDashboardRequest(identifier=1, title="New Title")
+ assert req.dashboard_title == "New Title"
+
+ def test_css_max_length_enforced(self) -> None:
+ with pytest.raises(ValidationError, match="at most 50000"):
+ UpdateDashboardRequest(identifier=1, css="x" * 50001)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing sanitization security tests</b></div>
<div id="fix">
`UpdateDashboardRequest` has the same `_detect_dashboard_title_sanitization`
model validator and `sanitize_dashboard_title` field validator as
`GenerateDashboardRequest` (schemas.py lines 762-809), but the test class has
no sanitization coverage. Without tests, regression in the XSS-rejection path
on the update path would go undetected. Add tests mirroring
`TestGenerateDashboardRequestTitleSanitization`.
</div>
</div>
<small><i>Code Review Run #58b3b3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -641,6 +685,169 @@ def sanitize_dashboard_title(cls, v: str | None) -> str |
None:
)
+class UpdateDashboardRequest(BaseModel):
+ """Request schema for updating an existing dashboard's layout/theme/style.
+
+ All fields are optional; only the fields explicitly passed are applied.
+ Use to retroactively set a custom layout, brand palette, or CSS on a
+ dashboard that was created via ``generate_dashboard`` (or earlier via
+ the REST API) without a full re-create.
+ """
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
+ dashboard_title: str | None = Field(
+ None,
+ max_length=500,
+ description="Optional new dashboard title.",
+ validation_alias=AliasChoices("dashboard_title", "title"),
+ )
+ description: str | None = Field(
+ None,
+ description="Optional new dashboard description.",
+ )
+ slug: str | None = Field(
+ None,
+ max_length=255,
+ description=(
+ "Optional new URL slug. Pass empty string to clear a slug."
+ ),
+ )
+ published: bool | None = Field(
+ None,
+ description="Optional published flag.",
+ )
+ position_json: Dict[str, Any] | None = Field(
+ None,
+ description=(
+ "Optional replacement layout (Superset's position_json dict). "
+ "When set, fully replaces the existing layout. Get the current "
+ "layout via ``get_dashboard_info`` first if you want to make "
+ "incremental changes."
+ ),
+ )
+ json_metadata_overrides: Dict[str, Any] | None = Field(
+ None,
+ description=(
+ "Optional overrides applied on top of the existing "
+ "json_metadata. Merged shallowly — pass only the keys you "
+ "want to change (e.g. label_colors, color_scheme, "
+ "cross_filters_enabled)."
+ ),
+ )
+ css: str | None = Field(
+ None,
+ max_length=50000,
+ description=(
+ "Optional new dashboard CSS. Pass empty string to clear "
+ "existing CSS."
+ ),
+ )
+ sanitization_warnings: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Internal: warnings emitted when user input was altered by "
+ "sanitization. Populated by the ``mode='before'`` validator "
+ "before dashboard_title is rewritten."
+ ),
+ )
+
+ @model_validator(mode="before")
+ @classmethod
+ def _detect_dashboard_title_sanitization(cls, data: Any) -> Any:
+ """Reject empty-after-sanitization titles and warn on partial strip.
+
+ Mirrors the same guard ``GenerateDashboardRequest`` applies so a
+ prompt-injected LLM cannot push XSS payloads through the update
+ path that the create path already rejects. Server-only
+ ``sanitization_warnings`` is reset here so a caller cannot inject
+ warning text.
+ """
+ if not isinstance(data, dict):
+ return data
+ data["sanitization_warnings"] = []
+ for key in ("dashboard_title", "title"):
+ if key in data:
+ raw = data[key]
+ break
+ else:
+ raw = None
+ if not isinstance(raw, str) or not raw.strip():
+ return data
+ sanitized, was_modified = sanitize_user_input_with_changes(
+ raw, "Dashboard title", max_length=500, allow_empty=True
+ )
+ 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."
+ )
+ if was_modified:
+ data["sanitization_warnings"].append(
+ "dashboard_title was modified during sanitization to "
+ "remove potentially unsafe content; the stored title "
+ "differs from the input."
+ )
+ return data
+
+ @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 == "":
+ return v
+ return sanitize_user_input(
+ v, "Dashboard title", max_length=500, allow_empty=True
+ )
+
+
+class UpdateDashboardResponse(BaseModel):
+ """Response schema for ``update_dashboard``.
+
+ Distinct from ``GenerateDashboardResponse`` because the semantics
+ differ: this response reports which fields actually changed on an
+ existing dashboard, rather than describing a newly created one.
+ """
+
+ dashboard: DashboardInfo | None = Field(
+ None, description="The updated dashboard info, if successful"
+ )
+ dashboard_url: str | None = Field(
+ None, description="URL to view the dashboard"
+ )
+ error: str | None = Field(None, description="Error message, if update
failed")
+ permission_denied: bool = Field(
+ default=False,
+ description=(
+ "True when the user lacks edit rights on the target "
+ "dashboard. When True, ``error`` carries the human-readable "
+ "explanation and the response is otherwise empty."
+ ),
+ )
+ changed_fields: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Names of fields that were actually applied. Empty when the "
+ "request was a no-op or failed before any field was applied."
+ ),
+ )
+ warnings: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Non-fatal advisory messages — for example, that the supplied "
+ "title was altered by sanitization."
+ ),
+ )
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for sanitization</b></div>
<div id="fix">
The `UpdateDashboardRequest` has no dedicated sanitization tests, unlike
`GenerateDashboardRequest` which has
`TestGenerateDashboardRequestTitleSanitization` (lines 571-629). Without
equivalent tests, regressions in the XSS guard could go undetected.
</div>
</div>
<small><i>Code Review Run #58b3b3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
