rusackas commented on code in PR #43666:
URL: https://github.com/apache/superset/pull/43666#discussion_r4118809560
##########
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:
Fair, this is sharper than the general lost-update point and I didn't engage
with it properly the first time. Traced through it again.
The race is real: A reads unchanged unsafe css (skips validation), B
concurrently writes safe css, A's write lands after and clobbers B's fix. But I
don't think it grants a new capability, it only affects whether an admin's
concurrent cleanup can get silently undone. The css has to already be sitting
in the DB as unsafe for there to be anything to restore, same exposure that
exists right now for any grandfathered row with no race involved at all.
A real fix means either a row lock or a compare-and-swap on the actual
UPDATE (`WHERE css = :expected`), and `DashboardDAO.update` is generic across
every field this command touches, not css-specific, so either approach reaches
well past this PR into how updates work here in general. Still think that's its
own piece of work rather than something to bolt on to closing the validation
gap. Want to see it tracked as a follow-up if you agree, rather than block this
on it, that concurrent-write integrity question keeps surfacing and probably
deserves its own look regardless of css.
--
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]
