You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2026/08/23 16:43:43 UTC
[PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
Posted by "rebenitez1802 (via GitHub)" <gi...@apache.org>.
rebenitez1802 commented on PR #43433:
URL: https://github.com/apache/superset/pull/43433#issuecomment-5480502278
Approve — this supersedes my earlier request-changes. The follow-up (`0d0704b`) resolves both prior blockers: `SupersetParseError` is now caught alongside `TemplateError` (opaque 500 → clean 422, fail-closed, correctly scoped to the query branch), and the collected error is now a proper `ValidationError` (`TagAccessValidationError`) so it composites cleanly. Both new tests are genuine red-pre-fix/green-post-fix guards, and CI + lint are clean. Remaining items below are non-blocking nits.
🟢 **Low — Comment/test docstrings say the delete route calls `normalized_messages()`, but it doesn't**
The new comment (`delete.py`, "the delete route calls `normalized_messages()` on it") and both new test docstrings ("called by the single-object DELETE route") are inaccurate: the only route running `DeleteTaggedObjectCommand` is `delete_object` (`tags/api.py:472`), whose handler returns a bare `self.response_422()` (`api.py:474-475`) and never calls `normalized_messages()`. The routes that *do* call it (`api.py:542-543,598,681-682`) run the different `DeleteTagsCommand`. The `ValidationError` change is still the right call — just fix the rationale. Suggestion: reword to e.g. "Append a `ValidationError` so it composites cleanly if `normalized_messages()` is ever called (the bulk-delete route does so for `DeleteTagsCommand`)."
🟢 **Low — The `normalized_messages()` footgun is only partly closed in this command**
Only the query-access branch now appends a `ValidationError`. The other five branches in `DeleteTaggedObjectCommand` still append non-`ValidationError`s — `TaggedObjectDeleteFailedError` (missing-args/tag-not-found/bad-type at `delete.py:65,70,77`, the `SupersetSecurityException` branch, and the unsupported-type `else`) and `TaggedObjectNotFoundError` (`delete.py:90`). So a mixed failure (e.g. the access-check raises *and* `find_tagged_object` returns `None`) yields a `TagInvalidError` whose `normalized_messages()` would `AttributeError`. It can't crash today because `delete_object` uses a bare `response_422()` — hence purely latent — but the fix leaves the contract half-applied. Optional hardening: make every item appended into `TagInvalidError` in this command a `ValidationError` subclass, mirroring `DeleteTagsCommand`.
🟢 **Low — `{object_type}` renders as the enum repr in the message**
`ObjectType` is a plain `enum.Enum`, so `f"Access validation failed for {object_type} {object_id}: {ex}"` renders `"...for ObjectType.query 1: ..."`. Cosmetic only (and log-only on this route), and it matches the pre-existing sibling messages, so optional. Suggestion: use `{object_type.name}` for clean text.
🟢 **Low (optional) — `except TemplateError` also swallows Jinja `SecurityError`/`UndefinedError`**
Still downgrades a sandbox/internal templating failure from a 5xx to a WARNING-logged 422 during access validation (observability, not correctness/security — it stays fail-closed). Fine to leave; worth a note.
🟢 **Low (optional) — Tests stop at `.validate()`**
Both new tests exercise the command directly, so they don't assert the route renders a real 422, that the tag survives (fail-closed), or that a non-`TemplateError`/`SupersetParseError` still propagates. The public-`normalized_messages()` assertions are a solid improvement; a single route-level 422 test would close the last gap.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43433:
URL: https://github.com/apache/superset/pull/43433#issuecomment-5387132328
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43433:
URL: https://github.com/apache/superset/pull/43433#issuecomment-5446785681
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43433:
URL: https://github.com/apache/superset/pull/43433#issuecomment-5387150218
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43433?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `16.66667%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.36%. Comparing base ([`9f505eb`](https://app.codecov.io/gh/apache/superset/commit/9f505eb0cbbc39b78f512765d82fd63cf5ad70e6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7116b0f`](https://app.codecov.io/gh/apache/superset/commit/7116b0f0fadbc1349877641f022bce88f79abb1b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43433?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/tag/delete.py](https://app.codecov.io/gh/apache/superset/pull/43433?src=pr&el=tree&filepath=superset%2Fcommands%2Ftag%2Fdelete.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvdGFnL2RlbGV0ZS5weQ==) | 16.66% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43433?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (9f505eb) and HEAD (7116b0f). Click for more details.
>
> <details><summary>HEAD has 133 uploads less than BASE</summary>
>
>| Flag | BASE (9f505eb) | HEAD (7116b0f) |
>|------|------|------|
>|python|116|1|
>|presto|19|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43433 +/- ##
===========================================
- Coverage 78.85% 57.36% -21.50%
===========================================
Files 2876 2876
Lines 164581 164586 +5
Branches 38011 38011
===========================================
- Hits 129786 94420 -35366
- Misses 32348 69272 +36924
+ Partials 2447 894 -1553
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.01% <16.66%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.01% <16.66%> (-43.54%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43433?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43433:
URL: https://github.com/apache/superset/pull/43433#issuecomment-5445353806
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 0d0704bccce09a8c184c64edd2072cd1479a0443 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a90a96df53473000837d699 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43433--superset-docs-preview.netlify.app](https://deploy-preview-43433--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLDoAgDADRu3QtCQmK4g08RqX1k6CQ2oXGeHfZzeLNC0XyURRGgAZmwTNutZf9NspHSajMIlmM4mqIEysbjJGvq3LikvIzUR08BovB09K5tnfW2sH15EOA7wc) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43433--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): catch TemplateError when validating access for tagged SQL Lab queries on delete [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #43433:
URL: https://github.com/apache/superset/pull/43433
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org