fix(sqllab): wrap process_template() in format_sql to prevent raw UndefinedError leak - #42917
fix(sqllab): wrap process_template() in format_sql to prevent raw UndefinedError leak#42917eschutho wants to merge 1 commit into
Conversation
…efinedError leak (SC-116908)
format_sql() (POST /api/v1/sqllab/format_sql/) calls process_template()
with no catch for jinja2 exceptions. When an undefined Jinja variable is
accessed via attribute/subscript (e.g. {{ tbl.name }}), process_template()
re-raises the raw jinja2.exceptions.UndefinedError instead of converting
it, so it escapes to the global exception handler as an opaque 500
instead of a typed 4xx.
Mirrors QueryEstimationCommand.run() (#42757): catch TemplateError,
raise SupersetErrorException(status=400). Same bug class as #42366,
#42401, #42714, #42757, #42802, #42851 — 7th call site of
process_template()'s bare-raise fallback in this series.
Fixes SUPERSET-PYTHON (raw UndefinedError leak, format_sql call site)
Code Review Agent Run #600332Actionable 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 |
dosubot
Bot
added
api
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42917 +/- ##
==========================================
+ Coverage 66.37% 66.38% +0.01%
==========================================
Files 2857 2857
Lines 161048 161177 +129
Branches 37046 37074 +28
==========================================
+ Hits 106892 107001 +109
- Misses 52141 52150 +9
- Partials 2015 2026 +11
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| assert rv.status_code == 400 | ||
| assert "tbl" in resp_data["errors"][0]["message"] | ||
| assert "undefined" in resp_data["errors"][0]["message"] |
There was a problem hiding this comment.
Suggestion: The regression test does not verify the required typed error contract. A different 400 response, such as a generic validation error containing the same message fragments, would pass these assertions even if error_type were no longer GENERIC_COMMAND_ERROR; assert the returned error type (and, if part of the contract, its level) explicitly. [api mismatch]
Severity Level: Minor 🧹
- ⚠️ Future error-type regressions can pass CI unnoticed.
- ⚠️ SQL Lab clients may receive an incorrect typed error contract.
- ⚠️ Current API handler explicitly promises GENERIC_COMMAND_ERROR responses.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/sql_lab/api_tests.py
**Line:** 358:360
**Comment:**
*Api Mismatch: The regression test does not verify the required typed error contract. A different 400 response, such as a generic validation error containing the same message fragments, would pass these assertions even if `error_type` were no longer `GENERIC_COMMAND_ERROR`; assert the returned error type (and, if part of the contract, its level) explicitly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The review suggestion is correct. The current test only verifies the HTTP status code and message content, which could pass for other types of 400 errors. To enforce the typed error contract, you should explicitly assert the To resolve this, update the test assertion in assert rv.status_code == 400
assert resp_data["errors"][0]["error_type"] == "GENERIC_COMMAND_ERROR"
assert resp_data["errors"][0]["level"] == "error"
assert "tbl" in resp_data["errors"][0]["message"]
assert "undefined" in resp_data["errors"][0]["message"]There are no other comments on this PR to address. tests/integration_tests/sql_lab/api_tests.py |
1 participant
SUMMARY
SqlLabRestApi.format_sql()(POST/api/v1/sqllab/format_sql/, SQL Lab's "Format SQL" button) callsprocess_template()with no catch for jinja2 exceptions. When an undefined Jinja variable is accessed via attribute/subscript (e.g.{{ tbl.name }}, not a bare{{ tbl }}),BaseTemplateProcessor.process_template()'s bare-raise fallback re-raises the rawjinja2.exceptions.UndefinedErrorinstead of converting it to a Superset exception. Since neither offormat_sql()'s existingexceptclauses (json.JSONDecodeError, and the outerValidationError) catches it, the raw exception escapes to Flask's global@app.errorhandler(Exception)catch-all as an opaque 500 (GENERIC_BACKEND_ERROR) instead of a typed, user-actionable 4xx.This is the same bug class fixed at 6 other
process_template()call sites in this series: #42366, #42401, #42714, #42757, #42802, #42851.superset/sqllab/api.pyhad not previously been touched by any of them.BEFORE/AFTER
Before: posting SQL with an undefined-attribute Jinja reference to
/api/v1/sqllab/format_sql/returns a500 GENERIC_BACKEND_ERRORwith a raw traceback-derived message.After: the same request returns a
400with a typedSupersetErrorException(GENERIC_COMMAND_ERROR) whose message names the undefined variable.FIX
Added
except TemplateError as ex: raise SupersetErrorException(SupersetError(message=str(ex), error_type=GENERIC_COMMAND_ERROR, level=ERROR), status=400) from exafter the existingexcept json.JSONDecodeErrorclause, mirroringQueryEstimationCommand.run()(#42757) exactly.SupersetErrorExceptionis handled by the global@app.errorhandler(SupersetErrorException), the same mechanismestimate_query_cost()(a few lines above, same file) already relies on for its ownSupersetErrorException— consistent with this file's existing pattern, not a new one. Additive-only; no existing behavior changes for well-formed templates.TESTING INSTRUCTIONS
POST /api/v1/sqllab/format_sql/withsql: "select * from {{ tbl.name }}",template_params: "{}"(or any non-empty dict that doesn't definetbl), and a validdatabase_id.500with a raw jinja2 traceback message.400with a structured error body namingtblas undefined.New regression test:
test_format_sql_request_with_undefined_jinja_attributeintests/integration_tests/sql_lab/api_tests.py, right after the existingtest_format_sql_request_with_jinja. Confirmed it fails on pre-fix code (git stashon the source fix, rawUndefinedErrortraceback,assert rv.status_code == 400fails with500) and passes post-fix.ADDITIONAL INFORMATION
Tradeoffs: none — this is an additive
exceptclause converting an unhandled 500 into a typed 400; no existing success-path behavior changes.Related: #42366, #42401, #42714, #42757, #42802, #42851 (same bug class, other
process_template()call sites).