sha174n opened a new pull request, #45020: URL: https://github.com/apache/superset/pull/45020
### SUMMARY `check_sqlalchemy_uri()` blocks a fixed set of SQLAlchemy dialects (`sqlite`, `shillelagh`, `duckdb`) because they can resolve local filesystem paths at query time. Today the only lever an operator has to relax that is `PREVENT_UNSAFE_DB_CONNECTIONS=False`, which disables the whole check. That is all-or-nothing: an operator who deliberately runs duckdb/MotherDuck has to drop the guard for every other dialect too. This PR adds per-dialect granularity. A new config setting lets an operator opt a specific dialect back in while the rest of the blocklist keeps applying. ### Mechanism - New config `ALLOWED_UNSAFE_DB_DIALECTS: set[str] = set()` (next to `PREVENT_UNSAFE_DB_CONNECTIONS`). It holds base dialect names, the part of the drivername before any `+driver`. - In `check_sqlalchemy_uri()`, when a blocklist entry matches, the base dialect is compared against the allowlist; if the operator listed it, that one dialect is permitted and the loop moves on. Everything else (the `superset`/`ENABLE_SUPERSET_META_DB` handling, the raised error) is unchanged. - Base-dialect matching means `duckdb` covers `duckdb+duckdb_engine://` and the MotherDuck cloud variant, consistent with how the blocklist regexes already treat `+driver` suffixes. ### Backward compatibility Defaults to an empty set, so behaviour is identical to today unless an operator explicitly opts a dialect in. No effect when `PREVENT_UNSAFE_DB_CONNECTIONS` is `False` (the check is skipped entirely in that case). This complements the existing all-or-nothing flag rather than replacing it. ### TESTING INSTRUCTIONS `tests/integration_tests/security/analytics_db_safety_tests.py` adds coverage through the real `check_sqlalchemy_uri` (no mocking of the check): - a dialect added to `ALLOWED_UNSAFE_DB_DIALECTS` is accepted, including the MotherDuck URI and the `+duckdb_engine` driver; - allowing one dialect does not re-enable the others (per-dialect, not global); - the default empty allowlist preserves existing behaviour. ``` pytest tests/integration_tests/security/analytics_db_safety_tests.py ``` ### 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]
