rusackas opened a new pull request, #43666:
URL: https://github.com/apache/superset/pull/43666
### SUMMARY
`validate_css` (added in #40640) blocks `expression(`, script-scheme URIs,
and `@import` on dashboard `css` at create/update time, but two gaps let a
dangerous value through or block edits it shouldn't:
- `ImportV1DashboardSchema.css` had no validator at all, so a YAML/ZIP
dashboard bundle import bypassed the check entirely while the create/update API
paths enforced it. Custom CSS is injected into the page unsanitised
(`injectCustomCss.ts` sets `style.innerHTML` directly on a real `<style>` tag),
so an imported `@import` genuinely fetches and executes at render regardless of
how it was saved. `external_url` in the same schema already gets its own
validator threaded through import in the same PR, so this looked like a
straightforward miss rather than an intentional gap.
- `DashboardPutSchema.css` re-validated on every PUT. A dashboard save
resends the full object, so renaming a dashboard, moving a chart, or editing a
filter all go through the same PUT and re-trigger the check even when `css`
isn't the field being changed. A dashboard whose existing `css` already
contained `@import` (from before this check existed, or via the import bypass
above) became uneditable for any change at all, not just a change to `css`.
This closes the import gap by adding `validate_css` to
`ImportV1DashboardSchema.css`, and moves PUT-path enforcement from the schema
to `UpdateDashboardCommand`, which now only validates `css` when the incoming
value actually differs from what's stored. A genuinely new `css` value, or an
existing dangerous value being changed to a different dangerous value, is still
rejected exactly as before, only an unrelated edit that resends an
already-stored value (safe or not) stops being blocked.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend validation change, no UI change.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/dashboards/schema_tests.py \
tests/unit_tests/commands/dashboard/update_test.py -q
```
New/changed coverage:
- `test_dashboard_import_css_rejects_dangerous_constructs` — import now
rejects the same constructs create/update already do.
- `test_dashboard_put_schema_does_not_validate_css` — replaces the old
PUT-schema-level test; documents that enforcement moved to the command.
- `tests/unit_tests/commands/dashboard/update_test.py` (new file) —
`UpdateDashboardCommand` accepts an unchanged existing `css` value even when it
contains `@import`, still rejects a genuinely new dangerous value, still
rejects a changed value that's dangerous in a different way, and skips the
check entirely when `css` isn't in the payload.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
