fix(tags): catch TemplateError when validating access for tagged SQL Lab queries - #43423
fix(tags): catch TemplateError when validating access for tagged SQL Lab queries#43423eschutho wants to merge 2 commits into
Conversation
…Lab queries When tagging a saved SQL Lab query, CreateCustomTagCommand._validate_object_access calls security_manager.raise_for_access(query=...). For a user relying on per-table/dataset permissions (no blanket database access), that path parses the query's Jinja-templated SQL via process_jinja_sql(), which can raise a raw jinja2 TemplateError (e.g. TemplateSyntaxError on malformed Jinja). The narrow `except SupersetSecurityException:` let it escape as an unhandled 500. Widen the except to also catch TemplateError so it surfaces as a validation error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #eb956fActionable 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43423 +/- ##
=======================================
Coverage 78.85% 78.86%
=======================================
Files 2876 2876
Lines 164581 164582 +1
Branches 38011 38011
=======================================
+ Hits 129786 129791 +5
+ Misses 32348 32344 -4
Partials 2447 2447
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}
| ) | ||
| ) | ||
| except SupersetSecurityException: | ||
| except (SupersetSecurityException, TemplateError): |
There was a problem hiding this comment.
Suggestion: The exception handling still omits SupersetParseError, which process_jinja_sql() raises when a saved query contains an unresolvable partition macro. That exception escapes _validate_object_access() instead of being appended to exceptions, so tagging such a query bypasses the command's TagInvalidError validation response. Catch SupersetParseError here as well and map it into the collected validation errors. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Tagging affected SQL Lab queries returns an unhandled server error.
- ⚠️ Tag API misses its expected validation response.
- ⚠️ Queries using dynamic partition macros cannot be tagged reliably.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/tag/create.py
**Line:** 102:102
**Comment:**
*Incomplete Implementation: The exception handling still omits `SupersetParseError`, which `process_jinja_sql()` raises when a saved query contains an unresolvable partition macro. That exception escapes `_validate_object_access()` instead of being appended to `exceptions`, so tagging such a query bypasses the command's `TagInvalidError` validation response. Catch `SupersetParseError` here as well and map it into the collected validation errors.
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 implementation in the PR catches Here is the corrected implementation: from superset.exceptions import SupersetParseError
# ...
except (SupersetSecurityException, TemplateError, SupersetParseError):
exceptions.append(
TagCreateFailedError(
f"Could not validate access for {object_type} {object_id}"
)
)I have checked the available PR comments, and there are no other pending review comments to address. Would you like me to proceed with any other changes? superset/commands/tag/create.py |
…ency The regression test let the real security_manager.raise_for_access(query=...) run, which opens a live DB connection to introspect table-level perms before reaching the Jinja parse. That only passed locally because a DB was up; the unit_tests CI sandbox has no Postgres, so it failed there. Mock raise_for_access directly to raise TemplateError instead. This still proves what matters: except (SupersetSecurityException, TemplateError) in create.py catches it and surfaces as TagInvalidError rather than escaping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed the diff. Core fix is correct and the new test genuinely exercises the malformed-Jinja code path via raise_for_access -> process_jinja_sql. Two findings worth addressing before merge, plus one NIT.
| ) | ||
| ) | ||
| except SupersetSecurityException: | ||
| except (SupersetSecurityException, TemplateError): |
There was a problem hiding this comment.
No logging when swallowing TemplateError here. SupersetSecurityException being swallowed silently is intentional (routine, expected auth denial), but TemplateError represents a genuinely unexpected condition — previously it propagated as an unhandled 500 with a full traceback visible in server logs/Sentry. Now it's converted to a generic TagCreateFailedError with zero logger.warning/logger.exception call anywhere in this file. A malformed-Jinja saved query (or any other bug that raises TemplateError here) becomes invisible server-side. Suggest binding the exception (except (SupersetSecurityException, TemplateError) as ex:) and logging str(ex) before appending to exceptions.
| ) | ||
| ) | ||
| except SupersetSecurityException: | ||
| except (SupersetSecurityException, TemplateError): |
There was a problem hiding this comment.
The PR description says this follows the pattern already established in superset/commands/sql_lab/results.py, but that file actually uses two separate except arms with materially different handling:
except SupersetSecurityException as ex:
raise SupersetErrorException(SupersetError(message=__("Cannot access the query"), ...), status=403) from ex
except TemplateError as ex:
raise SupersetErrorException(SupersetError(message=str(ex), ...), status=400) from exIt preserves the real Jinja error text (str(ex)) and distinguishes security vs. validation failures by status code. This PR instead merges both exception types into one except (...) tuple with a single hardcoded generic message, discarding str(ex) and the security/validation distinction the referenced pattern was built to keep. Worth at least surfacing str(ex) so the actual template error isn't lost.
| ) | ||
| ) | ||
| except SupersetSecurityException: | ||
| except (SupersetSecurityException, TemplateError): |
There was a problem hiding this comment.
NIT: this except wraps all four object_type branches (dashboard/chart/query/dataset), but only the query branch can currently raise TemplateError (confirmed by reading raise_for_access — the dashboard/chart/dataset branches never touch Jinja/process_jinja_sql). The comment above implies the fix is scoped to the query case; consider narrowing the except to just the query branch so a future unrelated TemplateError raised from another branch isn't silently misreported as an access-validation failure for that object type.
Code Review Agent Run #31b26cActionable 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 |
2 participants
SUMMARY
When tagging a saved SQL Lab query,
CreateCustomTagCommand._validate_object_access()callssecurity_manager.raise_for_access(query=target_object), wrapped only inexcept SupersetSecurityException:.For a user who lacks blanket database-level access and relies on per-table/dataset permissions,
raise_for_access()parses the query's Jinja-templated SQL viasuperset/sql/parse.py::process_jinja_sql()(which callsprocessor.env.parse(sql)) to determine referenced tables. As its docstring states, that can raise a rawjinja2.exceptions.TemplateError(e.g.TemplateSyntaxErroron malformed Jinja such as an unclosed{% if %}block). Because that exception is not aSupersetSecurityException, it escaped the narrowexceptclause and propagated as an unhandled 500 instead of a proper Superset validation error.Fix: widen the
exceptclause in_validate_object_accesstoexcept (SupersetSecurityException, TemplateError):, keeping the file's existing append-to-exceptions-list convention, and reword the message slightly since a template-render failure isn't strictly an access denial.PROBLEM
Tagging a saved SQL Lab query whose SQL contains malformed Jinja returned an unhandled 500 (raw
jinja2.TemplateSyntaxError) rather than aTagInvalidErrorvalidation response, for users authorized via per-table/dataset permissions.FIX
superset/commands/tag/create.py: catchTemplateErroralongsideSupersetSecurityExceptioninCreateCustomTagCommand._validate_object_access, mapping the template-parse failure into the collected validation exceptions.TESTING INSTRUCTIONS
test_validate_object_access_query_malformed_jinjaintests/unit_tests/tags/commands/create_test.py, which exercises the query path with malformed Jinja SQL (SELECT * FROM {% if %} broken) and forces the per-table authorization path (can_access_database→False), asserting it surfaces asTagInvalidError.jinja2.exceptions.TemplateSyntaxErrorescaping throughprocess_jinja_sql; post-fix it passes.pytest tests/unit_tests/tags/commands/create_test.py→ 3 passed.ruff checkandruff format --checkpass on the changed files.ADDITIONAL INFORMATION
This is the same recurring bug class as PRs #42366, #42401, #42757, #42917, #43145, and #43226 — a raw jinja2
TemplateErrorescaping araise_for_access()/SQL-parsing call site instead of being mapped to a Superset exception.superset/commands/sql_lab/results.pyalready establishes the local pattern of catchingTemplateErroralongsideSupersetSecurityException.Note: the sibling site
superset/commands/tag/delete.pyhas the identical duplicated_validate_object_accessmethod with the same bug. It is intentionally left untouched here and will be addressed in a follow-up PR.🤖 Generated with Claude Code