sadpandajoe commented on code in PR #43666:
URL: https://github.com/apache/superset/pull/43666#discussion_r3989738293
##########
superset/dashboards/schemas.py:
##########
@@ -596,7 +601,7 @@ class GetFavStarIdsSchema(Schema):
class ImportV1DashboardSchema(Schema):
dashboard_title = fields.String(required=True)
description = fields.String(allow_none=True)
- css = fields.String(allow_none=True)
+ css = fields.String(allow_none=True, validate=validate_css)
Review Comment:
This rejects a same-version export containing grandfathered CSS even though
the update path deliberately permits that stored value to remain: export
serializes `Dashboard.css` verbatim, then this schema rejects the ZIP during
restore or migration. Could the export/import path safely preserve that round
trip, for example by explicitly cleaning or quarantining the CSS instead of
making the generated bundle unimportable?
##########
superset/commands/dashboard/update.py:
##########
@@ -131,6 +132,19 @@ def validate(self) -> None:
except ValidationError as ex:
exceptions.append(ex)
+ # A dashboard PUT resends the full object on every save, so only
+ # validate css when it's actually changing -- otherwise a dashboard
+ # whose existing css predates this check (or was imported without
+ # going through it) becomes uneditable for unrelated changes like a
+ # rename or a chart move.
+ if "css" in self._properties:
+ new_css = self._properties["css"]
+ if new_css != self._model.css:
+ try:
+ validate_css(new_css)
Review Comment:
This moves dangerous-CSS failures from the existing schema-level 400
response with a `css` error into a command-level 422 whose direct
`ValidationError` is keyed as `_schema`. Could we preserve the established
status and field-shaped error contract when validating only changed CSS?
##########
superset/commands/dashboard/update.py:
##########
@@ -130,6 +131,19 @@ def validate(self) -> None:
except ValidationError as ex:
exceptions.append(ex)
+ # A dashboard PUT resends the full object on every save, so only
+ # validate css when it's actually changing -- otherwise a dashboard
+ # whose existing css predates this check (or was imported without
+ # going through it) becomes uneditable for unrelated changes like a
+ # rename or a chart move.
+ if "css" in self._properties:
+ new_css = self._properties["css"]
Review Comment:
This PR removes the schema guard only for CSS, so the general lost-update
window becomes a validation bypass: request A can restore CSS that a direct PUT
would now reject. Could we keep the lock or write-time recheck scoped to
accepting an unchanged disallowed CSS value and cover that A/B sequence?
--
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]
