fix(sqllab): wrap process_template() in QueryEstimationCommand to prevent raw UndefinedError leak - #42757
Merged
Merged
Conversation
…vent raw UndefinedError leak
BaseTemplateProcessor.process_template() has a bare `raise` fallback for
jinja2 UndefinedError on undefined attribute/subscript access (e.g.
{{ foo.bar }}) that doesn't match its "is it a function call" heuristic.
QueryEstimationCommand.run() called process_template() with no try/except
around it, so this raw jinja2.exceptions.UndefinedError propagated past the
API layer and hit Flask's catch-all handler, producing an opaque 500 instead
of a typed 4xx error for a bad Jinja template typed into SQL Lab's cost
estimator.
Wrap the call in a try/except TemplateError and convert it into a
SupersetErrorException(status=400), matching the broad-catch pattern
ExecuteSqlCommand already uses for the same class of leak in the sibling
execute.py command.
dosubot
Bot
added
global:jinja
Contributor
Code Review Agent Run #be2372Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42757 +/- ##
==========================================
- Coverage 65.59% 65.57% -0.02%
==========================================
Files 2819 2819
Lines 160372 160376 +4
Branches 36617 36617
==========================================
- Hits 105188 105162 -26
- Misses 53120 53147 +27
- Partials 2064 2067 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sadpandajoe
added
the
review:checkpoint
Antonio-RiveroMartnez
approved these changes
Aug 4, 2026
sadpandajoe
removed
the
review:checkpoint
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
QueryEstimationCommand.run()(superset/commands/sql_lab/estimate.py, the command behind SQL Lab's "estimate query cost" feature) callstemplate_processor.process_template()with no try/except around it.process_template()(superset/jinja_context.py) normally converts Jinja errors into typed Superset exceptions (SupersetSyntaxErrorException,SupersetTemplateException,UndefinedTemplateFunctionException) — but it has one bare-raisefallback forjinja2.exceptions.UndefinedErrorwhen an undefined template variable is accessed via attribute/subscript (e.g.{{ foo.bar }}) rather than called as a function. That raw exception propagates pastestimate.py's unguarded call site, past the API layer (superset/sqllab/api.py::estimate_query_cost,/api/v1/sqllab/estimate/), which also has no try/except aroundcommand.run(), and lands on Flask's global catch-all handler — an opaque 500 instead of a typed 4xx, for what's just a malformed Jinja template typed into the SQL Lab editor.The sibling command in the same package,
ExecuteSqlCommand.run()(superset/commands/sql_lab/execute.py), already guards against this class of leak by wrapping its whole body and converting any non-Superset exception into a typed one.estimate.pyhad no equivalent guard for this call site.Same bug class as #42366, #42401, #42426, #42442, #42714 — a prior daily-cleanup pipeline that's been converting these raw-exception leaks into properly typed/statused Superset exceptions across the codebase.
FIX
Wraps only the
process_template()call inexcept TemplateError(jinja2's own base exception, not Superset's), converting the leak intoSupersetErrorException(status=400). This is additive-only:SupersetSyntaxErrorException/SupersetTemplateExceptionare Superset's own exception hierarchy, notTemplateErrorsubclasses, so their existing (already-correct) propagation paths are untouched.TESTING INSTRUCTIONS
test_run_wraps_raw_jinja_undefined_errorintests/unit_tests/commands/sql_lab/test_estimate.py: mocksget_template_processor().process_templateto raise a rawjinja2.exceptions.UndefinedError, callsQueryEstimationCommand.run(), asserts aSupersetErrorExceptionwithstatus == 400is raised instead.UndefinedErrorpropagates uncaught) and passes post-fix.SELECT {{ foo.bar }}wherefoois not defined in the Jinja context — pre-fix this 500s with a generic error; post-fix it returns a 400 with the Jinja error message.ruff check/ruff format --checkclean on both changed files (pinned 0.9.7 viauvx).ADDITIONAL INFORMATION
Tradeoffs: additive-only — no existing failure-mode semantics change. The one previously-uncaught path (raw
UndefinedErrorfor undefined attribute/subscript access in an estimate-time template) now returns a typed 400 instead of an opaque 500; no other paths are touched.Correction for accuracy: the
except TemplateErrorclause is slightly broader than the illustrative example above suggests.SparkTemplateProcessor/TrinoTemplateProcessor(jinja_context.py) overrideprocess_template()entirely with no internal try/except of their own, so for those two engines anyTemplateError(not just the attribute/subscriptUndefinedErrorcase) was previously leaking raw and is now also caught and converted to a typed 400. This is strictly the same failure mode at the same call site (raw 500-leak → typed 400), just triggered by moreTemplateErrorsubtypes/engines than the one example covers — not a new or different behavior change.