fix: adds the ability to disallow SQL functions per engine #28639

dpgaspar · 2024-05-22T13:32:52Z

SUMMARY

Adds a new configuration key named DISALLOWED_SQL_FUNCTIONS that defines disallowed function per engine on SQL statements. These functions will be disallowed on SQLLab and Charts.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

codecov · 2024-05-22T13:46:24Z

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.43%. Comparing base (76d897e) to head (396a8e0).
Report is 1094 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/db_engine_specs/base.py	75.00%	1 Missing ⚠️
superset/exceptions.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #28639       +/-   ##
===========================================
+ Coverage   60.48%   83.43%   +22.94%     
===========================================
  Files        1931      523     -1408     
  Lines       76236    37605    -38631     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31377    -14737     
+ Misses      28017     6228    -21789     
+ Partials     2105        0     -2105

Flag	Coverage Δ
hive	`48.99% <28.57%> (-0.18%)`	⬇️
javascript	`?`
postgres	`77.22% <71.42%> (?)`
presto	`53.56% <71.42%> (-0.24%)`	⬇️
python	`83.43% <92.85%> (+19.95%)`	⬆️
sqlite	`76.68% <71.42%> (?)`
unit	`58.95% <92.85%> (+1.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dpgaspar · 2024-05-22T15:36:15Z

superset/db_engine_specs/trino.py

            logger.debug("Query %d: Running query: %s", query_id, sql)

            try:
-                cls.execute(cursor, sql, query.database)
+                with app.app_context():
+                    cls.execute(cursor, sql, query.database)


threads don't have an app context: https://stackoverflow.com/questions/72541670/why-flask-app-context-is-lost-in-child-thread-when-application-factory-pattern-i

Oh, interesting.

betodealmeida · 2024-05-22T15:56:54Z

superset/db_engine_specs/trino.py

            logger.debug("Query %d: Running query: %s", query_id, sql)

            try:
-                cls.execute(cursor, sql, query.database)
+                with app.app_context():
+                    cls.execute(cursor, sql, query.database)


Oh, interesting.

betodealmeida · 2024-05-22T15:58:29Z

superset/sql_parse.py

+    :param function_list: The list of functions to search for
+    :param engine: The engine to use for parsing the SQL statement
+    """
+    return ParsedQuery(sql, engine=engine).check_functions_exist(function_list)


We (probably me) will have to convert this to use sqlglot and the SQLStatement class (#26786) but I'm happy to do it, seems simple enough.

Happy to do it myself, can I just not use sqlparse? implement something using the same pattern as extract_tables_from_statement?

john-bodley · 2024-05-22T17:22:41Z

superset/config.py

+# A set of disallowed SQL functions per engine. This is used to restrict the use of
+# unsafe SQL functions in SQL Lab and Charts. The keys of the dictionary are the engine
+# names, and the values are sets of disallowed functions.
+DISALLOWED_SQL_FUNCTIONS: dict[str, set[str]] = {


I'm unsure where the best place for this deny list, i.e., here in the configuration or within the extra JSON payload of the database.

Additionally should this be engine (dialect) specific or database specific? If it's the later then maybe the extra JSON payload field is preferable.

JSON payload at the database level is more dynamic and would avoid having to change the config to add remove disallowed functions. But on the other hand the user that actually registers the db could have intentions to "abuse" these functions.

)

fix: adds the ability to disallow SQL functions per engine

8bf5f48

pull-request-size bot added the size/M label May 22, 2024

dpgaspar added 3 commits May 22, 2024 15:03

fix test

0f97b45

fix lint

96d0468

fix trino thread

80bb186

pull-request-size bot added size/L and removed size/M labels May 22, 2024

dpgaspar marked this pull request as ready for review May 22, 2024 15:19

dpgaspar requested review from betodealmeida, craig-rueda and eschutho May 22, 2024 15:19

dosubot bot added data:databases Related to database configurations and connections sqllab Namespace | Anything related to the SQL Lab labels May 22, 2024

dpgaspar commented May 22, 2024

View reviewed changes

betodealmeida approved these changes May 22, 2024

View reviewed changes

john-bodley reviewed May 22, 2024

View reviewed changes

dpgaspar requested a review from john-bodley May 23, 2024 08:56

dpgaspar mentioned this pull request May 28, 2024

chore(🦾): bump python flask-appbuilder 4.4.1 -> 4.5.0 #28734

Closed

Merge branch 'master' into feat/disallow-sql-functions

396a8e0

dpgaspar merged commit 5dfbab5 into apache:master May 29, 2024
34 checks passed

dpgaspar deleted the feat/disallow-sql-functions branch May 29, 2024 09:51

EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024

fix: adds the ability to disallow SQL functions per engine (apache#28639

000c9a3

)

michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jun 26, 2024

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

ecb3b58

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

d12a5d0

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

ff2907f

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

c33b76a

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

69a0c38

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

840af70

michael-s-molina pushed a commit that referenced this pull request Jun 26, 2024

fix: adds the ability to disallow SQL functions per engine (#28639)

56f0103

Qadeem411 approved these changes Jul 7, 2024

View reviewed changes

mistercrunch added 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: adds the ability to disallow SQL functions per engine (apache#28639

4606b5d

)

mistercrunch added the 🚢 4.1.0 label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: adds the ability to disallow SQL functions per engine #28639

fix: adds the ability to disallow SQL functions per engine #28639

dpgaspar commented May 22, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

dpgaspar May 22, 2024

betodealmeida May 22, 2024

betodealmeida May 22, 2024

betodealmeida May 22, 2024

dpgaspar May 23, 2024

john-bodley May 22, 2024

dpgaspar May 23, 2024

fix: adds the ability to disallow SQL functions per engine #28639

fix: adds the ability to disallow SQL functions per engine #28639

Conversation

dpgaspar commented May 22, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

dpgaspar May 22, 2024

Choose a reason for hiding this comment

betodealmeida May 22, 2024

Choose a reason for hiding this comment

betodealmeida May 22, 2024

Choose a reason for hiding this comment

betodealmeida May 22, 2024

Choose a reason for hiding this comment

dpgaspar May 23, 2024

Choose a reason for hiding this comment

john-bodley May 22, 2024

Choose a reason for hiding this comment

dpgaspar May 23, 2024

Choose a reason for hiding this comment

dpgaspar commented May 22, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading