Repository navigation
fix(query_context): raise 422 not 500 on malformed Jinja in Query datasource access check - #44011
Conversation
Code Review Agent Run #114d7eActionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: the new query_context fix is correct, secure, and well-tested — but the PR also carries a stale duplicate of already-merged #42714, which is the only reason it conflicts and hides a regression trap. Drop that commit and this is a clean approve.
🔴 High — First commit duplicates already-merged #42714 and its conflict drops _validate_rendered_sql
Commit e15e95af ("fix(alerts): wrap Jinja rendering errors…") is already on master — it landed as #42714 (7c03736623), same title, and master already has process_template inside the try plus the identical test_execute_query_wraps_template_rendering_error. Since #42714 merged, master also added self._validate_rendered_sql(rendered_sql) (via #42929) right after that line. git merge-tree origin/master <head> shows the only conflict is here in alert.py:
rendered_sql = sql_template.process_template(...)
<<<<<<< origin/master
self._validate_rendered_sql(rendered_sql)
=======
>>>>>>> <pr head>
limited_rendered_sql = ...apply_limit_to_sql(...)
Resolving it by taking "the PR side" silently deletes master's _validate_rendered_sql call (single-statement + read-only-DML enforcement) — a real regression. Fix: rebase onto master and drop commit e15e95af entirely (the alert.py hunk and the alert test are both redundant with master); keep only the query_context_processor.py change + its test, which merge cleanly.
🟢 Low — query_context test asserts the exception type but not the 422 status the PR is about
test_raise_for_access_wraps_template_error_for_query_datasource only does pytest.raises(SupersetTemplateException). The type implies 422 today (exceptions.py:201), but the PR's stated purpose is the status code, and nothing pins it — a future refactor making the exception a 500 would pass silently. Fix: with pytest.raises(SupersetTemplateException) as exc: … assert exc.value.status == 422.
🟢 Low — Neither template-wrap test asserts cause chaining
Both fixes correctly preserve the cause via raise … from ex, but the tests don't check it. Fix: add assert isinstance(exc.value.__cause__, TemplateError) so a later refactor can't drop the chaining unnoticed.
🟢 Low — Scoping the wrap to only the DatasourceType.QUERY branch is intentional — no action
For a reviewer's benefit: leaving the else (query_context=…) branch unwrapped matches the sibling check_query_access in superset/explore/utils.py, and omitting allow_query_authorship_bypass=True here is also correct (this path re-checks access on every chart/dashboard view). Both are deliberate; flagging only so they don't get "fixed."
Also checked (no action needed): the except TemplateError in query_context_processor.raise_for_access is narrowly scoped — SupersetSecurityException (403) is not a TemplateError subclass, so access denials still propagate as 403; and the wrapped str(ex) at 422 exposes no more than the pre-fix behavior already did (same message, previously surfaced as a 500).
github-actions
Bot
added
the
requires:rebase
ad7ac16 to
95102ac
Compare
Code Review Agent Run #4968a6Actionable 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: the blocking issue is resolved — thanks for dropping the stale alert commit. The PR is now a focused single-commit change to query_context_processor.py + its test, and the raise_for_access wrap is correct, security-scoped, and well-tested. One mechanical step before merge, plus two optional nits.
🟡 Medium — Still CONFLICTING, but it's now a trivial import-adjacency conflict (rebase to clear)
The branch is still on its old base, so git merge-tree origin/master <head> shows the raise_for_access body and the SupersetTemplateException import both auto-merge cleanly — the only conflict is one import line at the top of the file:
<<<<<<< origin/master
from pandas.api.types import infer_dtype # added on master
=======
from jinja2.exceptions import TemplateError # added by this PR
>>>>>>> <pr head>
Both sides just inserted a different import after from flask_babel import gettext as _. Resolution: keep both lines (isort order puts from jinja2.exceptions import TemplateError before from pandas.api.types import infer_dtype). A rebase / "Update branch" clears it — no code risk.
🟢 Low — Test asserts the exception type but not the 422 status the PR is about
test_raise_for_access_wraps_template_error_for_query_datasource only does pytest.raises(SupersetTemplateException). The type implies 422 today (exceptions.py), but nothing pins the status the PR exists to guarantee. Optional: with pytest.raises(SupersetTemplateException) as exc: … assert exc.value.status == 422.
🟢 Low — Test doesn't assert cause chaining
The fix correctly preserves the cause via raise … from ex, but the test doesn't check it. Optional: assert isinstance(exc.value.__cause__, TemplateError) so a later refactor can't silently drop the chaining.
Approving so this isn't gated on the two optional nits; just needs the rebase to become mergeable.
…asource access check QueryContextProcessor.raise_for_access() calls security_manager.raise_for_access(query=...) when the datasource is a SQL Lab Query. That method Jinja-renders the query's SQL to resolve the tables it touches; a malformed template raises a raw jinja2.TemplateError that was not caught, producing an opaque 500. Wrap the call in a try/except that converts TemplateError to SupersetTemplateException (status 422), matching the identical pattern already used in explore/utils.py::check_query_access(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
95102ac to
552f526
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| with pytest.raises(SupersetTemplateException): | ||
| processor.raise_for_access() |
There was a problem hiding this comment.
Suggestion: The test checks only the exception class, so it passes even if SupersetTemplateException.status is not 422 and cannot protect the API contract this change targets.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_context_processor.py
**Line:** 2245:2246
**Comment:**
*Api Mismatch: The test checks only the exception class, so it passes even if `SupersetTemplateException.status` is not 422 and cannot protect the API contract this change targets.
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 flagged issue is correct. The current test only verifies that a To resolve this, you should update the test to explicitly check the status code of the raised exception. You can modify the test as follows: with pytest.raises(SupersetTemplateException) as excinfo:
processor.raise_for_access()
assert excinfo.value.status == 422I have checked the PR comments, and there are no other comments to address. Would you like me to fetch all comments to validate and implement fixes for any other issues? tests/unit_tests/common/test_query_context_processor.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44011 +/- ##
=======================================
Coverage 81.06% 81.06%
=======================================
Files 2955 2955
Lines 178300 178304 +4
Branches 41307 41307
=======================================
+ Hits 144533 144539 +6
+ Misses 31056 31055 -1
+ Partials 2711 2710 -1
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:
|
Code Review Agent Run #d4860bActionable 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 |
3 participants
SUMMARY
QueryContextProcessor.raise_for_access()callssecurity_manager.raise_for_access(query=...)when the datasource is a SQL LabQuery(i.e.DatasourceType.QUERY— the "explore/chart a SQL Lab result without saving a dataset" flow). That method internally Jinja-renders the query's SQL to resolve the tables it touches. If the SQL contains a malformed Jinja template, a rawjinja2.TemplateErrorpropagates uncaught all the way to the Flask API layer, producing an opaque 500.This wraps the call in
try/except TemplateError→SupersetTemplateException(status 422), exactly matching the pattern already used insuperset/explore/utils.py::check_query_access()for the samesecurity_manager.raise_for_access(query=...)call. The globalSupersetExceptionerror handler insuperset/views/error_handling.pyconverts it to a proper JSON error response automatically.Sibling fix in the same bug family: #43866 (
fix(sql_lab): raise 400 not 500 on malformed Jinja during CSV export access check).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only error handling change.
TESTING INSTRUCTIONS
test_raise_for_access_wraps_template_error_for_query_datasourceintests/unit_tests/common/test_query_context_processor.py— mirrors the existingtest_raise_for_access_evaluates_access_before_validatetest structure.TemplateSyntaxErrorleaks), passes after the fix.test_raise_for_access_evaluates_access_before_validatecontinues to pass (no regression to theDatasourceType.TABLEpath).pytest tests/unit_tests/common/test_query_context_processor.py -k "test_raise_for_access" -vADDITIONAL INFORMATION