eschutho opened a new pull request, #43433:
URL: https://github.com/apache/superset/pull/43433
### SUMMARY
When deleting a tag from a saved SQL Lab query,
`DeleteTaggedObjectCommand._validate_object_access` calls
`security_manager.raise_for_access(query=...)` to authorize the operation. For
a user relying on per-table/dataset permissions (no blanket database access),
that authorization path parses the query's Jinja-templated SQL via
`process_jinja_sql()`.
`process_jinja_sql()` explicitly documents that it `:raises
jinja2.exceptions.TemplateError: If the Jinjafied SQL could not be rendered` —
for example a `TemplateSyntaxError` on malformed Jinja such as `select {{
unclosed`. The command only wrapped `_validate_object_access` in a narrow
`except SupersetSecurityException:`, so a raw `TemplateError` escaped uncaught
and surfaced to the client as an opaque **500** instead of a proper validation
error.
**Fix:** wrap only the `query` branch of `_validate_object_access` in its
own `try/except TemplateError`. When caught, it is logged (a `TemplateError`
here is genuinely unexpected, unlike the routine `SupersetSecurityException`
denial) and converted into a `TaggedObjectDeleteFailedError` that preserves the
real template error text, so validation fails cleanly with a 422 instead of a
500.
The catch is deliberately scoped to the `query` branch only — the
`dashboard`/`chart`/`dataset` branches never touch Jinja/`process_jinja_sql`,
so wrapping all four would risk silently misreporting an unrelated future error
as a query-access-validation failure. The existing outer `except
SupersetSecurityException:` is unchanged. `SupersetParseError` (also raised by
`process_jinja_sql`, for unresolvable partition macros) is intentionally left
alone — it is already a proper `SupersetErrorException` with a correct 422
status, not a raw-exception leak.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change.
### TESTING INSTRUCTIONS
Automated: `pytest tests/unit_tests/tags/commands/delete_test.py`
The new hermetic regression test mocks
`SupersetSecurityManager.raise_for_access` to raise a `TemplateSyntaxError` and
asserts that `DeleteTaggedObjectCommand(...).validate()` raises
`TagInvalidError` (422) rather than letting the raw `TemplateSyntaxError`
escape, and that the original template error text is preserved in the collected
exceptions. It fails on the pre-fix code (raw `TemplateSyntaxError` escapes)
and passes after the fix. The full `tests/unit_tests/tags/commands/` suite
passes.
Manual: as a user with only per-table/dataset access (no blanket database
access), tag a saved SQL Lab query whose SQL contains malformed Jinja (e.g.
`select {{ unclosed`), then attempt to delete that tag from the query. Before
this change the request returns a 500; after, it returns a clean validation
error.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
- [ ] 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
This applies the same raw-`TemplateError`-leak fix, in the same command
layer, as #43423 (the originating pattern PR, which fixes it on the create
path). This PR covers the delete path.
--
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]
