betodealmeida opened a new pull request, #24032: URL: https://github.com/apache/superset/pull/24032
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY <!--- Describe the change below, including rationale and design decisions --> The function `has_table_query` is used to detect if a given query is selecting from one or more tables, in order to prevent malicious ad-hoc expressions that bypass RLS. Because `sqlparse` is non-validating the function needs to keep track of a lot of state, and often fails on edge cases. This PR rewrites the function to use `sqloxide`. The advantage is that the function is now more robust, supporting custom dialects and handling more edge cases. There are a couple disadvantages, though: - `sqloxide` was an optional dependency, with this PR it becomes mandatory. - The function `has_table_query` now requires a full SQL query, so for validating expressions they need to be wrapped in `f"SELECT {expression}"`. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> N/A ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> The unit tests pass mostly unmodified, the only exception is that `table` is usually a reserved keyword and had to be quoted in order to `sqloxide` to parse it (`sqlparse` is much more lenient). I added a new test to cover another edge case that was found could bypass RLS. ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] 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]
