eschutho opened a new pull request, #42757:
URL: https://github.com/apache/superset/pull/42757
### SUMMARY
`QueryEstimationCommand.run()` (`superset/commands/sql_lab/estimate.py`, the
command behind SQL Lab's "estimate query cost" feature) calls
`template_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-`raise` fallback for `jinja2.exceptions.UndefinedError` when an
undefined template variable is accessed via attribute/subscript (e.g. `{{
foo.bar }}`) rather than called as a function. That raw exception propagates
past `estimate.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 around `command.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 t
he 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.py` had no equivalent guard for this call site.
Same bug class as apache/superset#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 in `except TemplateError` (jinja2's
own base exception, not Superset's), converting the leak into
`SupersetErrorException(status=400)`. This is additive-only:
`SupersetSyntaxErrorException`/`SupersetTemplateException` are Superset's own
exception hierarchy, not `TemplateError` subclasses, so their existing
(already-correct) propagation paths are untouched.
### TESTING INSTRUCTIONS
- New regression test `test_run_wraps_raw_jinja_undefined_error` in
`tests/unit_tests/commands/sql_lab/test_estimate.py`: mocks
`get_template_processor().process_template` to raise a raw
`jinja2.exceptions.UndefinedError`, calls `QueryEstimationCommand.run()`,
asserts a `SupersetErrorException` with `status == 400` is raised instead.
- Confirmed the test fails on pre-fix code (raw `UndefinedError` propagates
uncaught) and passes post-fix.
- Manual repro: in SQL Lab, estimate the cost of a query with a template
like `SELECT {{ foo.bar }}` where `foo` is 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 --check` clean on both changed files (pinned
0.9.7 via `uvx`).
### 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
**Tradeoffs**: additive-only — no existing failure-mode semantics change.
The one previously-uncaught path (raw `UndefinedError` for undefined
attribute/subscript access in an estimate-time template) now returns a typed
400 instead of an opaque 500; no other paths are touched.
--
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]
