fix(explore): catch TemplateError when validating access for query-backed form_data - #43470
fix(explore): catch TemplateError when validating access for query-backed form_data#43470eschutho wants to merge 1 commit into
Conversation
…cked form_data check_query_access() calls raise_for_access(query=query), which Jinja-renders the query's SQL to resolve table references. A malformed template surfaces as a raw jinja2.exceptions.TemplateError instead of a Superset exception, leaking as an opaque 500 from the explore form_data endpoints (used by the chart Explore/Drill-by cache) whenever datasource_type=query. Wrap the call and re-raise as the existing SupersetTemplateException (422), matching the same conversion already used in datasets/api.py, and map it to a proper response in ExploreFormDataRestApi's four handlers.
dosubot
Bot
added
change:backend
Code Review Agent Run #544c6cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — minimal, correct fix that matches the established datasets/api.py idiom and the wider "catch TemplateError from raise_for_access → re-raise a classified Superset exception" prior art. The except SupersetTemplateException clause is reachable in all four handlers (sibling of CommandException, not shadowed by the ValidationError/TemporaryCache* clauses above it), the commands catch only SQLAlchemyError so the exception genuinely propagates, and this is pure 500→422 re-classification with no authorization decision skipped.
One non-blocking ask before/after merge:
🟡 Test coverage — the api.py handler change is untested. test_query_malformed_jinja_template exercises the real check_query_access (good, covers utils.py), but none of the four except SupersetTemplateException handler clauses in superset/explore/form_data/api.py is covered by any test. An integration harness already exists at tests/integration_tests/explore/form_data/api_tests.py but wasn't extended, so deleting any of the four clauses would silently regress the endpoint back to a 500 with no failing test. Suggest adding one endpoint-level test asserting a 422 + message round-trip (or extending that api_tests harness) so the handler mapping can't silently regress.
2 participants
SUMMARY
ExploreFormDataRestApi(the cache backing chart Explore / Drill-by state) lets a client reference a SQL Lab query as the chart's datasource (datasource_type=query). Access to that datasource is validated viasuperset.explore.utils.check_query_access, which callssecurity_manager.raise_for_access(query=query). When the query hasn't been executed yet (or was edited after last execution), that call falls back to Jinja-rendering the query's raw SQL (process_jinja_sql) to resolve the tables it references — the exact same rendering step already known to raisejinja2.exceptions.TemplateErrorfor malformed templates (see #42366, #42401, and the more recent #43423/#43433 in the tag-access-check family).Unlike those command-layer call sites, this one had no
except TemplateErrorat all, so the raw jinja2 exception propagated straight throughCreateFormDataCommand/GetFormDataCommand/UpdateFormDataCommand/DeleteFormDataCommand(none of which catch anything butSQLAlchemyError) up toExploreFormDataRestApi, landing as an opaque, unclassified 500 from all fourPOST/PUT/GET/DELETE /api/v1/explore/form_datahandlers.PROBLEM
A user-controlled input (a saved SQL Lab query with malformed Jinja templating, referenced as a chart's datasource) can trigger a raw
jinja2.exceptions.TemplateErrorthat surfaces as an unclassified 500, instead of a proper 4xxSupersetException.FIX
superset/explore/utils.py::check_query_accessnow catchesTemplateErroraround theraise_for_access(query=query)call and re-raises it asSupersetTemplateException(existing, 422 — no new exception class needed), preserving the original message.superset/explore/form_data/api.pymapsSupersetTemplateExceptionto a properex.statusresponse in all fourExploreFormDataRestApihandlers, mirroring the existingexcept SupersetTemplateException as ex: return self.response(ex.status, message=str(ex))idiom already used insuperset/datasets/api.py.No behavior change for any other exception type or datasource type.
TESTING INSTRUCTIONS
tests/unit_tests/explore/utils_test.py::test_query_malformed_jinja_template: mocksraise_for_accessto raisejinja2.exceptions.TemplateSyntaxErrorand assertscheck_datasource_accessnow raisesSupersetTemplateExceptioninstead. Verified this test fails on pre-fix code (rawTemplateSyntaxErrorpropagates) and passes post-fix.ruff check/ruff format --checkclean on all changed files.tests/unit_tests/explore/andtests/unit_tests/commands/explore/suites pass (24 tests).ADDITIONAL INFORMATION