You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org> on 2026/01/22 22:06:47 UTC
[PR] fix(embedded): allow guest users to sort by visible columns [superset]
YuriyKrasilnikov opened a new pull request, #37371:
URL: https://github.com/apache/superset/pull/37371
## Summary
Fixes #37061: Guest users in embedded dashboards can now sort table columns.
**Before:** Clicking any column header → `"Guest user cannot modify chart payload"`
**After:** Sorting by visible columns works; hidden columns and SQL injection still blocked.
---
## Problem Analysis
### Root Cause
`query_context_modified()` treated orderby identically to metrics/columns:
```python
for key, equivalent in [
("metrics", ["metrics"]),
("columns", ["columns", "groupby"]),
("orderby", ["orderby"]), # ← Same logic as metrics
]:
if not requested.issubset(stored):
return True # BLOCKED
```
**Issue:** User clicks column → frontend sends new orderby → `orderby ≠ stored_orderby` → blocked.
### Why This Is Wrong
| Field | Expected Behavior |
|-------|-------------------|
| metrics | Strict subset of stored (security) |
| columns | Strict subset of stored (security) |
| **orderby** | Can be ANY visible column (UX requirement) |
Sorting is **navigation**, not **data access expansion**.
---
## Security Research
Before implementing, we analyzed attack vectors:
### Attack Vector Analysis
| Vector | Risk | Example | Our Mitigation |
|--------|------|---------|----------------|
| **SQL Injection** | HIGH | `ORDER BY random()`, `sleep(5)` | Block `expressionType: "SQL"` |
| **Data Exfiltration** | HIGH | `ORDER BY credit_card_number` reveals ranking | Whitelist visible columns only |
| **RLS Bypass** | MEDIUM | Sort by filtered column reveals existence | Whitelist prevents column probing |
| **DoS** | LOW | Expensive sort on unindexed column | Existing query timeouts apply |
### Industry Standard
| Platform | Approach |
|----------|----------|
| Looker | Whitelist via LookML |
| Metabase | Visible fields only |
| Tableau | Pre-configured sorts |
| **Our solution** | Whitelist by visible columns |
### Mathematical Model
```
V = visible columns (columns ∪ groupby ∪ metrics)
S = requested sort columns
Rule: S ⊆ V (sort only by what user can see)
```
---
## Architecture Decision
### Why Not Just Remove orderby Check?
```python
# Option A: Remove orderby from loop (REJECTED)
for key in ["metrics", "columns", "groupby"]: # no orderby
```
**Rejected because:** Opens SQL injection and data exfiltration vectors.
### Why Not Frontend-Only Sorting?
**Rejected because:** Client-side sort on 1000 rows ≠ database sort on 100000 rows. Wrong results.
### Why Comparator Pattern?
During implementation, we discovered a **bug in existing code**:
```python
# EXISTING BUG (lines 260-264):
for key, equivalent in [...]:
for key in equivalent: # ← key OVERWRITTEN!
stored_values.update(...)
# After inner loop: key = last value of equivalent, not original key
```
This bug affects iterations 2-4 of the outer loop.
**Solution:** Refactor to explicit variables + custom comparator for orderby:
```python
for mapping in _FIELD_MAPPINGS:
field_name = mapping["field"] # explicit, not reused
equiv_fields = mapping["equivalent"]
comparator = mapping.get("comparator", default_compare)
for equiv_field in equiv_fields: # different variable name
...
```
### Why TypedDict?
```python
class FieldMapping(TypedDict):
field: str
equivalent: list[str]
comparator: NotRequired[Callable[...]]
```
1. **Type safety:** MyPy catches errors
2. **Extensibility:** Add new comparators without changing loop
3. **Documentation:** Structure is self-documenting
---
## Implementation Details
### New Functions
| Function | Purpose | Returns |
|----------|---------|---------|
| `_get_visible_columns(chart)` | Extract columns ∪ groupby ∪ metrics | `set[str]` |
| `_extract_orderby_column_name(item)` | Normalize orderby format; None = unsafe SQL | `str \| None` |
| `_orderby_whitelist_compare(ctx, chart, visible)` | Whitelist validation for orderby | `bool` (True = block) |
| `_default_field_compare(...)` | Default subset comparison (extracted for complexity) | `bool` |
### Why `_extract_orderby_column_name` Returns None for SQL?
```python
if orderby_item.get("expressionType") == "SQL":
return None # Triggers block
```
**Reason:** SQL expressions can contain:
- `random()` — non-deterministic, DoS potential
- `pg_sleep(5)` — timing attack
- `(SELECT password FROM users LIMIT 1)` — data exfiltration
### Why Check Both form_data AND queries?
```python
# Check form_data.orderby
for orderby_tuple in form_data.get("orderby") or []:
...
# Check query_context.queries[].orderby
for query in query_context.queries:
for orderby_tuple in getattr(query, "orderby", None) or []:
...
```
**Reason:** Attacker could send valid `form_data.orderby` but inject malicious `query.orderby`. Both must be validated.
### Why `equiv_field` Instead of `key`?
**Reason:** Python loop variables leak into outer scope. Reusing `key` corrupted subsequent iterations.
---
## Changes Summary
| File | Change |
|------|--------|
| `superset/security/manager.py` | +FieldMapping TypedDict, +4 functions, refactor loop |
| `tests/unit_tests/security/manager_test.py` | +3 tests for whitelist behavior |
### New Tests
| Test | Scenario | Expected |
|------|----------|----------|
| `test_..._visible_column_allowed` | Sort by column in chart | ✅ Allowed |
| `test_..._hidden_column_blocked` | Sort by `credit_card_number` | ❌ Blocked |
| `test_..._direction_change_allowed` | Change ASC↔DESC | ✅ Allowed |
Existing `test_query_context_modified_orderby` (SQL injection) continues to pass.
---
## Backward Compatibility
| Aspect | Impact |
|--------|--------|
| API | No change |
| Behavior for non-guest users | No change |
| Behavior for guest users | **Sorting now works** |
| Existing tests | All pass (28/28) |
---
## How To Test
1. Create embedded dashboard with Table chart
2. Get guest token
3. Click column header to sort
4. **Before:** Error "Guest user cannot modify chart payload"
5. **After:** Table sorts correctly
---
## Checklist
- [x] Security research completed (4 attack vectors analyzed)
- [x] Architecture decision documented
- [x] Bug fix: variable shadowing in loop
- [x] Unit tests for whitelist behavior
- [x] Existing tests pass
- [x] MyPy types added (TypedDict, type hints)
- [x] ruff/pylint pass
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4754495137
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 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(embedded): allow guest users to sort by visible columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3814191909
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 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(embedded): allow guest users to sort by visible columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3791398047
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 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(embedded): allow guest users to sort by visible columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4763947830
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 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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r2738631824
##########
superset/security/manager.py:
##########
@@ -212,6 +213,182 @@ def freeze_value(value: Any) -> str:
return json.dumps(value, sort_keys=True)
+# -----------------------------------------------------------------------------
+# Field comparison configuration for query_context_modified()
+# -----------------------------------------------------------------------------
+
+
+class FieldMapping(TypedDict):
+ """Configuration for comparing a field between requested and stored values."""
+
+ field: str
+ equivalent: list[str]
+ comparator: NotRequired[
+ Callable[
+ ["QueryContext", "Slice", set[str]], # query_context, stored_chart, visible
+ bool, # True if modified (should block)
+ ]
+ ]
+
+
+def _extract_orderby_column_name(orderby_item: Any) -> str | None:
+ """
+ Extract column/metric name from an orderby tuple element.
+
+ Returns None for SQL expressions which should be blocked for security.
+ """
+ if isinstance(orderby_item, str):
+ return orderby_item
+
+ if isinstance(orderby_item, dict):
+ # Block adhoc SQL expressions - potential injection vector
+ if orderby_item.get("expressionType") == "SQL":
+ return None
+ if label := orderby_item.get("label"):
+ return label
+ if col := orderby_item.get("column"):
+ if isinstance(col, dict):
+ return col.get("column_name")
+ return None
+
+ return None
+
+
+def _get_visible_columns(stored_chart: "Slice") -> set[str]:
+ """
+ Extract column/metric names visible in the chart.
+
+ Guest users can only sort by these columns (whitelist approach).
+ """
+ params = stored_chart.params_dict
+ visible: set[str] = set()
+
+ for col in params.get("columns") or []:
+ if isinstance(col, str):
+ visible.add(col)
+ elif isinstance(col, dict) and (label := col.get("label")):
+ visible.add(label)
+
+ for col in params.get("groupby") or []:
+ if isinstance(col, str):
+ visible.add(col)
+ elif isinstance(col, dict) and (label := col.get("label")):
+ visible.add(label)
+
+ for metric in params.get("metrics") or []:
+ if isinstance(metric, str):
+ visible.add(metric)
+ elif isinstance(metric, dict) and (label := metric.get("label")):
+ visible.add(label)
+
+ return visible
+
+
+def _validate_orderby_list(
+ orderby: Any,
+ visible_columns: set[str],
+) -> bool:
+ """
+ Validate a single orderby list against visible columns.
+
+ Returns True if invalid (should block), False if valid.
+ """
+ if orderby is not None and not isinstance(orderby, list):
+ return True
+ for orderby_tuple in orderby or []:
+ if not isinstance(orderby_tuple, (list, tuple)) or len(orderby_tuple) < 1:
+ return True
+ col_name = _extract_orderby_column_name(orderby_tuple[0])
Review Comment:
**Suggestion:** `_validate_orderby_list` relies on `_extract_orderby_column_name` but treats dict-based orderby items whose `"column"` key is a plain string as invalid (because `_extract_orderby_column_name` returns None for that shape). Extract the column name for dicts with a string `"column"` key before delegating to `_extract_orderby_column_name`, and accept outer `orderby` that are tuples as well. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Guest sorting blocked for dict-shaped orderby items.
- ⚠️ Blocks valid column sorts with `{"column":"col"}` shape.
- ⚠️ Affects embedded dashboard sort UX.
```
</details>
```suggestion
# Accept both lists and tuples as the outer container
if orderby is not None and not isinstance(orderby, (list, tuple)):
return True
for orderby_tuple in orderby or []:
if not isinstance(orderby_tuple, (list, tuple)) or len(orderby_tuple) < 1:
return True
first = orderby_tuple[0]
# Handle common shapes:
# - string column names
# - dicts with 'label' or 'expressionType'
# - dicts with 'column' which may be either a dict or a plain string
col_name = None
if isinstance(first, dict):
# If column is a plain string (e.g., {"column": "col_name"}), accept it.
col = first.get("column")
if isinstance(col, str):
col_name = col
else:
# Fall back to the shared extractor which handles dict shapes,
# including blocking SQL expressionType.
col_name = _extract_orderby_column_name(first)
else:
col_name = _extract_orderby_column_name(first)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect superset/security/manager.py: the helper `_validate_orderby_list` is added in
the PR hunk starting at line 287 and its outer-type guard lives at line 296 (`if orderby
is not None and not isinstance(orderby, list): return True`).
2. Consider an orderby element shaped as a dict with a plain `column` string, for example:
`orderby = [["", {"column": "user_email"}]]` or a tuple-equivalent attached to
`query.orderby` or `form_data["orderby"]`. Such shapes can be produced by code that
encodes column references using `{"column": "col_name"}`.
3. `_validate_orderby_list` currently delegates extraction to
`_extract_orderby_column_name(first)` (line 301). `_extract_orderby_column_name` returns
`None` when the `column` key is present but is a plain string (it only extracts when
`column` is a dict and returns its `column_name`), causing `col_name` to be None.
4. Because `col_name` is None, `_validate_orderby_list` returns True (line 302-303),
causing `_orderby_whitelist_compare` to treat the orderby as invalid and block the
request. The blocked request surfaces as the guest-facing "Guest user cannot modify chart
payload" when `query_context_modified()` is used.
Note: The code already intentionally blocks dicts with `expressionType: "SQL"` (security).
This suggestion only affects dicts where `column` is a plain string — a common, non-SQL
shape that should be accepted and whitelisted if the column is visible.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 296:301
**Comment:**
*Logic Error: `_validate_orderby_list` relies on `_extract_orderby_column_name` but treats dict-based orderby items whose `"column"` key is a plain string as invalid (because `_extract_orderby_column_name` returns None for that shape). Extract the column name for dicts with a string `"column"` key before delegating to `_extract_orderby_column_name`, and accept outer `orderby` that are tuples as well.
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.
```
</details>
##########
superset/security/manager.py:
##########
@@ -212,6 +213,182 @@ def freeze_value(value: Any) -> str:
return json.dumps(value, sort_keys=True)
+# -----------------------------------------------------------------------------
+# Field comparison configuration for query_context_modified()
+# -----------------------------------------------------------------------------
+
+
+class FieldMapping(TypedDict):
+ """Configuration for comparing a field between requested and stored values."""
+
+ field: str
+ equivalent: list[str]
+ comparator: NotRequired[
+ Callable[
+ ["QueryContext", "Slice", set[str]], # query_context, stored_chart, visible
+ bool, # True if modified (should block)
+ ]
+ ]
+
+
+def _extract_orderby_column_name(orderby_item: Any) -> str | None:
+ """
+ Extract column/metric name from an orderby tuple element.
+
+ Returns None for SQL expressions which should be blocked for security.
+ """
+ if isinstance(orderby_item, str):
+ return orderby_item
+
+ if isinstance(orderby_item, dict):
+ # Block adhoc SQL expressions - potential injection vector
+ if orderby_item.get("expressionType") == "SQL":
+ return None
+ if label := orderby_item.get("label"):
+ return label
+ if col := orderby_item.get("column"):
+ if isinstance(col, dict):
+ return col.get("column_name")
+ return None
+
+ return None
+
+
+def _get_visible_columns(stored_chart: "Slice") -> set[str]:
+ """
+ Extract column/metric names visible in the chart.
+
+ Guest users can only sort by these columns (whitelist approach).
+ """
+ params = stored_chart.params_dict
+ visible: set[str] = set()
+
+ for col in params.get("columns") or []:
+ if isinstance(col, str):
+ visible.add(col)
+ elif isinstance(col, dict) and (label := col.get("label")):
+ visible.add(label)
+
+ for col in params.get("groupby") or []:
+ if isinstance(col, str):
+ visible.add(col)
+ elif isinstance(col, dict) and (label := col.get("label")):
+ visible.add(label)
+
+ for metric in params.get("metrics") or []:
+ if isinstance(metric, str):
+ visible.add(metric)
+ elif isinstance(metric, dict) and (label := metric.get("label")):
+ visible.add(label)
+
+ return visible
+
+
+def _validate_orderby_list(
+ orderby: Any,
+ visible_columns: set[str],
+) -> bool:
+ """
+ Validate a single orderby list against visible columns.
+
+ Returns True if invalid (should block), False if valid.
+ """
+ if orderby is not None and not isinstance(orderby, list):
+ return True
+ for orderby_tuple in orderby or []:
+ if not isinstance(orderby_tuple, (list, tuple)) or len(orderby_tuple) < 1:
+ return True
+ col_name = _extract_orderby_column_name(orderby_tuple[0])
+ if col_name is None or col_name not in visible_columns:
+ return True
+ return False
+
+
+def _orderby_whitelist_compare(
+ query_context: "QueryContext",
+ stored_chart: "Slice",
+ visible_columns: set[str],
+) -> bool:
+ """
+ Compare orderby using whitelist approach.
+
+ Allows sorting by any visible column, blocks hidden columns and SQL expressions.
+ Returns True if modified (request should be blocked).
+
+ Defensive barriers:
+ - If orderby is not a list, block (fail-closed)
+ - If orderby element is not a tuple/list, block (fail-closed)
+ """
+ form_data = query_context.form_data or {}
+
+ # Check form_data orderby
+ if _validate_orderby_list(form_data.get("orderby"), visible_columns):
+ return True
+
+ # Check queries orderby
+ for query in query_context.queries:
+ if _validate_orderby_list(getattr(query, "orderby", None), visible_columns):
Review Comment:
**Suggestion:** The comparator iterates `query_context.queries` and directly validates `query.orderby`; if a `query.orderby` is a tuple it will be considered invalid and block the request. Normalize tuple orderby values from each `query` to a list before calling `_validate_orderby_list`. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Embedded guest dashboard sorting blocked (server-side queries).
- ⚠️ Stored QueryContext deserialization may be rejected.
- ⚠️ Affects guest UX when query.orderby shape varies.
```
</details>
```suggestion
orderby = getattr(query, "orderby", None)
if isinstance(orderby, tuple):
orderby = list(orderby)
if _validate_orderby_list(orderby, visible_columns):
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open superset/security/manager.py and inspect `_orderby_whitelist_compare` (hunk
beginning around line 312). The loop over `query_context.queries` starts at line 329 and
performs the validation call at line 330: `if _validate_orderby_list(getattr(query,
"orderby", None), visible_columns):`.
2. Produce a QueryContext where one of the `query` objects has its `.orderby` attribute
set to a Python tuple (this can occur if server-side code constructs Query objects with
tuple-valued orderby, or if a stored `query_context` decoded from JSON is converted into
tuple structures before being attached to `query_context.queries`).
3. When `_validate_orderby_list` (defined at lines 287-304) receives a tuple for
`orderby`, the first guard (`if orderby is not None and not isinstance(orderby, list):
return True`) treats it as invalid and returns True.
4. The `_orderby_whitelist_compare` loop then returns True (line 330), causing
`query_context_modified()` to report modification and
`SupersetSecurityManager.raise_for_access()` to raise a `SupersetSecurityException`,
blocking the request (guest sorting blocked).
Note: This reproducer traces the exact call site that validates `query.orderby` (line
330). If all code paths that build `query.orderby` use lists, the issue won't surface. If
some server-side code uses tuples, this is a real problem.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 330:330
**Comment:**
*Possible Bug: The comparator iterates `query_context.queries` and directly validates `query.orderby`; if a `query.orderby` is a tuple it will be considered invalid and block the request. Normalize tuple orderby values from each `query` to a list before calling `_validate_orderby_list`.
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.
```
</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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r3510852523
##########
superset/connectors/sqla/models.py:
##########
@@ -1687,9 +1690,13 @@ def adhoc_metric_to_sqla(
)
else:
sqla_column = column(column_name)
- sqla_metric = self.sqla_aggregations[metric["aggregate"]](sqla_column)
+ sqla_metric = self.sqla_aggregations[aggregate](sqla_column)
elif expression_type == utils.AdhocMetricExpressionType.SQL:
- expression = metric.get("sqlExpression")
+ expression: str | None = metric.get("sqlExpression")
+ if not isinstance(expression, str) or not expression:
Review Comment:
**Suggestion:** The SQL-expression validation accepts whitespace-only strings, so obviously invalid expressions pass this guard and fail later during SQL generation/execution. Trim the string (or check `expression.strip()`) so blank SQL is rejected early with `QueryObjectValidationError`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart data API accepts clearly invalid blank SQL metrics.
- ⚠️ Users see backend SQL errors instead of validation feedback.
- ⚠️ Embedded dashboards may fail on crafted whitespace metrics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a chart query request to the chart data endpoint (e.g., POST /api/v1/chart/data)
that builds a query using `SqlaTable.adhoc_metric_to_sqla` in
`superset/connectors/sqla/models.py:1678-1700`.
2. In the request JSON, include an adhoc metric with `"expressionType": "SQL"` and set
`"sqlExpression"` to a whitespace-only string, for example `"sqlExpression": " "` while
still marking the metric as active.
3. During processing, `adhoc_metric_to_sqla` reads `expression` at
`superset/connectors/sqla/models.py:1695` and evaluates the guard `if not
isinstance(expression, str) or not expression:` at line 1696; the whitespace-only string
passes because it is a non-empty `str`.
4. The function proceeds to build a SQLAlchemy expression from the blank SQL later in the
function, leading to a malformed or empty SQL fragment and a downstream SQLAlchemy/DB
error during query execution rather than an early, clear `QueryObjectValidationError` at
validation time.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a9e8734ce9547d381a6ed539277f201&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3a9e8734ce9547d381a6ed539277f201&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 1695:1696
**Comment:**
*Logic Error: The SQL-expression validation accepts whitespace-only strings, so obviously invalid expressions pass this guard and fail later during SQL generation/execution. Trim the string (or check `expression.strip()`) so blank SQL is rejected early with `QueryObjectValidationError`.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=cde1434bf512448cd6282599f58d35a2767c5ad3ef71acb88daf3a3e89a89979&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=cde1434bf512448cd6282599f58d35a2767c5ad3ef71acb88daf3a3e89a89979&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1678,6 +1678,9 @@ def adhoc_metric_to_sqla(
label = utils.get_metric_name(metric, self.verbose_map)
if expression_type == utils.AdhocMetricExpressionType.SIMPLE:
+ aggregate: str | None = metric.get("aggregate")
+ if aggregate not in self.sqla_aggregations:
Review Comment:
**Suggestion:** The new aggregate validation can still raise an uncaught runtime error when `aggregate` is an unhashable JSON type (for example, a list/object). `aggregate not in self.sqla_aggregations` attempts a dict-key hash and will throw `TypeError`, resulting in a 500 instead of a controlled `QueryObjectValidationError`. Validate `aggregate` is a string before the membership check. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart data API can 500 on malformed aggregate payload.
- ⚠️ Embedded/guest dashboards can crash on crafted adhoc metric.
- ⚠️ Error handling bypasses intended QueryObjectValidationError validation path.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a chart query request to the chart data endpoint (e.g., POST /api/v1/chart/data)
that ultimately builds a query using `SqlaTable.adhoc_metric_to_sqla` in
`superset/connectors/sqla/models.py:1678-1700`.
2. In the request JSON, include an adhoc metric with `"expressionType": "SIMPLE"` and set
`"aggregate"` to an unhashable JSON type, for example `"aggregate": ["SUM"]` or
`"aggregate": {"op": "SUM"}` within the metric object.
3. When the request is processed, `adhoc_metric_to_sqla` reads `aggregate` from the metric
at `superset/connectors/sqla/models.py:1681` and then executes `if aggregate not in
self.sqla_aggregations:` at line 1682.
4. Because `aggregate` is a list/dict, Python attempts to hash it for dict key membership,
raising `TypeError: unhashable type` which is not caught, causing a 500 error instead of a
controlled `QueryObjectValidationError` response.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cfa2bb0afd12424b832763852c8e0939&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cfa2bb0afd12424b832763852c8e0939&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 1681:1682
**Comment:**
*Type Error: The new aggregate validation can still raise an uncaught runtime error when `aggregate` is an unhashable JSON type (for example, a list/object). `aggregate not in self.sqla_aggregations` attempts a dict-key hash and will throw `TypeError`, resulting in a 500 instead of a controlled `QueryObjectValidationError`. Validate `aggregate` is a string before the membership check.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=838e04ccc1faa4ff1a9447409e2cef63b86416ba1e01c3e4abb462ab73cb0ade&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=838e04ccc1faa4ff1a9447409e2cef63b86416ba1e01c3e4abb462ab73cb0ade&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1519,6 +1601,442 @@ def test_query_context_modified_time_grain_in_orderby(
assert not query_context_modified(query_context)
+def test_query_context_modified_orderby_direction_change_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that changing sort direction (ASC/DESC) is allowed for visible columns.
+ """
+ query_context = mocker.MagicMock()
+ query_context.slice_.id = 42
+ query_context.slice_.query_context = None
+ query_context.slice_.params_dict = {
+ "columns": ["name"],
+ "groupby": [],
+ "metrics": ["count"],
+ "orderby": [["name", True]], # Original: DESC
+ }
+ query_context.form_data = {
+ "slice_id": 42,
+ "columns": ["name"],
+ "metrics": ["count"],
+ "orderby": [["name", False]], # Changed to ASC - should be allowed
Review Comment:
**Suggestion:** The inline direction comments are reversed: in this codebase the second `orderby` boolean means ascending (`True`) vs descending (`False`), so these comments currently document the opposite behavior and can mislead future maintenance of sorting/security tests. Update the comments to match the actual boolean semantics. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Comments in security manager tests misrepresent orderby direction semantics.
⚠️ Future maintainers might misread True/False effects on sorting.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/tests/unit_tests/security/manager_test.py` and locate
`test_query_context_modified_orderby_direction_change_allowed` around lines 1610–1625;
observe the comments on the two `orderby` entries at lines 1617 and 1623 stating `"True" =
Original: DESC` and `"False" = Changed to ASC`.
2. Open `superset/superset/security/manager.py` and inspect `_is_valid_orderby_entry` at
lines 754–763; its docstring says order-by entries have ``[term, ascending]`` shape,
indicating the boolean flag represents ascending when `True`.
3. Open `superset/tests/integration_tests/db_engine_specs/datastore_tests.py` and inspect
the test around lines 252–271; it builds a query object with `"orderby": [["gender_cc",
True]]` and asserts the generated SQL contains `ORDER BY gender_cc ASC`, confirming that
`True` means ascending in actual query building.
4. Compare these semantics with the unit-test comments in `manager_test.py`: they describe
`True` as DESC and `False` as ASC, which contradicts the verified behavior from
`datastore_tests.py` and the `_is_valid_orderby_entry` docstring, showing the comments are
inverted documentation rather than reflecting real code behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87a91385e9f54f0e931de72c96a56437&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=87a91385e9f54f0e931de72c96a56437&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1617:1623
**Comment:**
*Comment Mismatch: The inline direction comments are reversed: in this codebase the second `orderby` boolean means ascending (`True`) vs descending (`False`), so these comments currently document the opposite behavior and can mislead future maintenance of sorting/security tests. Update the comments to match the actual boolean semantics.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=a6d210ea317d19fe6ec69d6d98e67b0fb5209ae6d1e7a13e60a3309c8c7c3443&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=a6d210ea317d19fe6ec69d6d98e67b0fb5209ae6d1e7a13e60a3309c8c7c3443&reaction=dislike'>👎</a>
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org>.
YuriyKrasilnikov commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3814023409
## Response to codeant-ai bot suggestions
Both suggestions are **incorrect** per Superset's type system (see [previous analysis](https://github.com/apache/superset/pull/37371#issuecomment-3791086817)).
### 1. `{"column": "string"}` format
Does not exist. `AdhocMetric.column` is `AdhocMetricColumn` (dict) or `None`, never a plain string. Test at line 1360-1375 explicitly documents this as INVALID.
### 2. `tuple` as outer orderby container
Also incorrect. Type is `list[OrderBy]` ([superset_typing.py:127](https://github.com/apache/superset/blob/master/superset/superset_typing.py#L127)), schema is `fields.List` ([schemas.py:1345](https://github.com/apache/superset/blob/master/superset/charts/schemas.py#L1345)). The outer container is always `list`, not `tuple`. `tuple` is the **inner** element (`OrderBy = tuple[Metric | Column, bool]`).
No changes needed.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r3510807709
##########
superset/security/manager.py:
##########
@@ -520,48 +530,240 @@ def _native_filter_request_modified(query_context: "QueryContext") -> bool:
)
+def _get_form_data_item_label(item: Any, is_metric: bool) -> str | None:
+ """
+ Return the result-key label Superset uses for a column or metric definition.
+ """
+ label: Any
+ try:
+ label = get_metric_name(item) if is_metric else get_column_name(item)
+ except (AttributeError, KeyError, TypeError, ValueError):
+ return None
+ return label if isinstance(label, str) and label else None
+
+
+def _is_hidden_table_column(column_config: Any, name: str) -> bool:
+ """
+ Whether Table column_config marks a result column as hidden.
+ """
+ config = column_config.get(name) if isinstance(column_config, dict) else None
+ return isinstance(config, dict) and config.get("visible") is False
+
+
+def _stored_sort_target_identifiers(item: Any, is_metric: bool) -> set[str]:
+ """
+ Identifiers that can refer to a stored column/metric in orderby.
+
+ The exact frozen value preserves dict-shaped adhoc references. The label
+ matches result keys sent by Table server pagination and labels accepted by
+ SQL query building for adhoc columns/metrics.
+ """
+ identifiers = {freeze_value(item)}
+ if label := _get_form_data_item_label(item, is_metric=is_metric):
+ identifiers.add(freeze_value(label))
+ return identifiers
+
+
+def _requested_sort_target_identifiers(item: Any) -> set[str]:
+ """
+ Identifiers a requested orderby term may use.
+
+ String terms are result keys. Dict-shaped terms carry expression bodies, so
+ they authorize only by exact stored identity and never by a reused label.
+ """
+ if isinstance(item, (str, dict)):
+ return {freeze_value(item)}
+ return set()
+
+
+def _add_visible_sort_targets(
+ allowed: set[str],
+ values: Any,
+ column_config: Any,
+ *,
+ is_metric: bool,
+) -> None:
+ """
+ Add visible column/metric orderby identifiers from a stored control value.
+ """
+ if not isinstance(values, (list, tuple)):
+ return
+ for value in values:
+ label = _get_form_data_item_label(value, is_metric=is_metric)
+ if label is not None and _is_hidden_table_column(column_config, label):
+ continue
+ allowed.update(_stored_sort_target_identifiers(value, is_metric=is_metric))
+
+
def _collect_sortable_identifiers(
stored_chart: "Slice",
stored_query_context: Optional[dict[str, Any]],
) -> set[str]:
"""
- Frozen column names and metric labels/definitions a guest may legitimately
- sort by: every column or metric the stored chart already references.
-
- Order-by only changes the ordering of the result, not which data is read, so
- any column or metric already part of the chart is a safe sort target. A term
- that is not present in the stored chart (for example a free-form ``random()``
- expression) cannot be validated and must be rejected. Order-by entries are
- ``(column_or_metric, ascending)`` pairs, so only their first element is
- collected.
+ Identifiers a guest may use for a new sort target.
+
+ These are the visible columns/metrics the stored chart exposes. Exact
+ owner-defined orderby replay is handled separately because saved orderby may
+ intentionally reference a non-visible helper term, while guest-initiated
+ sorting should be limited to visible result columns.
"""
allowed: set[str] = set()
+ params = stored_chart.params_dict
+ column_config = params.get("column_config")
+
+ for key in ("columns", "groupby", "all_columns"):
+ _add_visible_sort_targets(
+ allowed,
+ params.get(key),
+ column_config,
+ is_metric=False,
+ )
+ _add_visible_sort_targets(
+ allowed,
+ params.get("metrics"),
+ column_config,
+ is_metric=True,
+ )
+ # Legacy charts store a single metric under the singular ``metric`` key.
+ if params.get("metric") is not None:
+ _add_visible_sort_targets(
+ allowed,
+ [params["metric"]],
+ column_config,
+ is_metric=True,
+ )
+
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ for key in ("columns", "groupby", "all_columns"):
+ _add_visible_sort_targets(
+ allowed,
+ query.get(key),
+ column_config,
+ is_metric=False,
+ )
+ _add_visible_sort_targets(
+ allowed,
+ query.get("metrics"),
+ column_config,
+ is_metric=True,
+ )
+
+ return allowed
+
+
+def _collect_stored_orderby_entries(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
+ """
+ Frozen saved orderby entries a guest may replay exactly.
+ """
+ allowed = {
+ freeze_value(entry) for entry in stored_chart.params_dict.get("orderby") or []
+ }
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ allowed.update(freeze_value(entry) for entry in query.get("orderby") or [])
+ return allowed
+
+
+def _metric_control_values(value: Any) -> list[Any]:
+ """
+ Return non-empty values from a metric-valued control.
+ """
+ if value is None or value == "":
+ return []
+ if isinstance(value, (list, tuple)):
+ return [item for item in value if item is not None and item != ""]
+ return [value]
- def add(values: Any) -> None:
- for value in values or []:
- allowed.add(freeze_value(value))
- def add_orderby(entries: Any) -> None:
- for entry in entries or []:
- if isinstance(entry, (list, tuple)) and entry:
- allowed.add(freeze_value(entry[0]))
+def _add_frozen_metric_control_values(allowed: set[str], value: Any) -> None:
+ """
+ Add exact metric-control values to an authorization set.
+ """
+ allowed.update(freeze_value(metric) for metric in _metric_control_values(value))
+
+def _collect_stored_series_limit_metric_identifiers(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
+ """
+ Exact metric selectors a guest may use for series limiting.
+ """
+ allowed: set[str] = set()
params = stored_chart.params_dict
- for key in ("columns", "groupby", "metrics", "all_columns"):
- add(params.get(key))
- # Legacy charts store a single metric under the singular ``metric`` key.
- add([params["metric"]] if params.get("metric") is not None else None)
- add_orderby(params.get("orderby"))
+
+ _add_frozen_metric_control_values(allowed, params.get("metrics"))
+ _add_frozen_metric_control_values(allowed, params.get("metric"))
+ for key in ("series_limit_metric", "timeseries_limit_metric"):
+ _add_frozen_metric_control_values(allowed, params.get(key))
if stored_query_context:
for query in stored_query_context.get("queries") or []:
- for key in ("columns", "groupby", "metrics", "all_columns"):
- add(query.get(key))
- add_orderby(query.get("orderby"))
+ _add_frozen_metric_control_values(allowed, query.get("metrics"))
+ for key in ("series_limit_metric", "timeseries_limit_metric"):
+ _add_frozen_metric_control_values(allowed, query.get(key))
return allowed
+def _series_limit_metric_value_modified(value: Any, allowed: set[str]) -> bool:
+ """
+ Whether a requested series-limit metric is absent from stored metric controls.
+ """
+ for metric in _metric_control_values(value):
+ if not isinstance(metric, (str, dict)) or not metric:
+ return True
+ if freeze_value(metric) not in allowed:
+ return True
+ return False
+
+
+def _series_limit_metric_modified(
+ query_context: "QueryContext",
+ form_data: dict[str, Any],
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+ """
+ Whether series-limit metric selectors introduce a metric not stored on chart.
+
+ Series limiting uses the selector to rank top-N groups, so guest requests may
+ only reuse stored metric controls. Dict-shaped selectors must match exactly;
+ labels are not an authorization key for expression objects.
+ """
+ allowed = _collect_stored_series_limit_metric_identifiers(
+ stored_chart,
+ stored_query_context,
+ )
Review Comment:
**Suggestion:** Annotate this local variable with its concrete type instead of relying on inference in newly added logic. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable is unannotated even though its type is clear from context (`set[str]`). The custom rule flags modified Python code that omits type hints on variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d5e8fd35efb644aaab7ed7a9f50880ec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d5e8fd35efb644aaab7ed7a9f50880ec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 738:741
**Comment:**
*Custom Rule: Annotate this local variable with its concrete type instead of relying on inference in newly added logic.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=29fa054d85bfad0dacc1685b09a2c663dd5c87866a7f1a6c9d44933032036edb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=29fa054d85bfad0dacc1685b09a2c663dd5c87866a7f1a6c9d44933032036edb&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -520,48 +530,240 @@ def _native_filter_request_modified(query_context: "QueryContext") -> bool:
)
+def _get_form_data_item_label(item: Any, is_metric: bool) -> str | None:
+ """
+ Return the result-key label Superset uses for a column or metric definition.
+ """
+ label: Any
+ try:
+ label = get_metric_name(item) if is_metric else get_column_name(item)
+ except (AttributeError, KeyError, TypeError, ValueError):
+ return None
+ return label if isinstance(label, str) and label else None
+
+
+def _is_hidden_table_column(column_config: Any, name: str) -> bool:
+ """
+ Whether Table column_config marks a result column as hidden.
+ """
+ config = column_config.get(name) if isinstance(column_config, dict) else None
+ return isinstance(config, dict) and config.get("visible") is False
+
+
+def _stored_sort_target_identifiers(item: Any, is_metric: bool) -> set[str]:
+ """
+ Identifiers that can refer to a stored column/metric in orderby.
+
+ The exact frozen value preserves dict-shaped adhoc references. The label
+ matches result keys sent by Table server pagination and labels accepted by
+ SQL query building for adhoc columns/metrics.
+ """
+ identifiers = {freeze_value(item)}
+ if label := _get_form_data_item_label(item, is_metric=is_metric):
+ identifiers.add(freeze_value(label))
+ return identifiers
+
+
+def _requested_sort_target_identifiers(item: Any) -> set[str]:
+ """
+ Identifiers a requested orderby term may use.
+
+ String terms are result keys. Dict-shaped terms carry expression bodies, so
+ they authorize only by exact stored identity and never by a reused label.
+ """
+ if isinstance(item, (str, dict)):
+ return {freeze_value(item)}
+ return set()
+
+
+def _add_visible_sort_targets(
+ allowed: set[str],
+ values: Any,
+ column_config: Any,
+ *,
+ is_metric: bool,
+) -> None:
+ """
+ Add visible column/metric orderby identifiers from a stored control value.
+ """
+ if not isinstance(values, (list, tuple)):
+ return
+ for value in values:
+ label = _get_form_data_item_label(value, is_metric=is_metric)
+ if label is not None and _is_hidden_table_column(column_config, label):
+ continue
+ allowed.update(_stored_sort_target_identifiers(value, is_metric=is_metric))
+
+
def _collect_sortable_identifiers(
stored_chart: "Slice",
stored_query_context: Optional[dict[str, Any]],
) -> set[str]:
"""
- Frozen column names and metric labels/definitions a guest may legitimately
- sort by: every column or metric the stored chart already references.
-
- Order-by only changes the ordering of the result, not which data is read, so
- any column or metric already part of the chart is a safe sort target. A term
- that is not present in the stored chart (for example a free-form ``random()``
- expression) cannot be validated and must be rejected. Order-by entries are
- ``(column_or_metric, ascending)`` pairs, so only their first element is
- collected.
+ Identifiers a guest may use for a new sort target.
+
+ These are the visible columns/metrics the stored chart exposes. Exact
+ owner-defined orderby replay is handled separately because saved orderby may
+ intentionally reference a non-visible helper term, while guest-initiated
+ sorting should be limited to visible result columns.
"""
allowed: set[str] = set()
+ params = stored_chart.params_dict
+ column_config = params.get("column_config")
+
+ for key in ("columns", "groupby", "all_columns"):
+ _add_visible_sort_targets(
+ allowed,
+ params.get(key),
+ column_config,
+ is_metric=False,
+ )
+ _add_visible_sort_targets(
+ allowed,
+ params.get("metrics"),
+ column_config,
+ is_metric=True,
+ )
+ # Legacy charts store a single metric under the singular ``metric`` key.
+ if params.get("metric") is not None:
+ _add_visible_sort_targets(
+ allowed,
+ [params["metric"]],
+ column_config,
+ is_metric=True,
+ )
+
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ for key in ("columns", "groupby", "all_columns"):
+ _add_visible_sort_targets(
+ allowed,
+ query.get(key),
+ column_config,
+ is_metric=False,
+ )
+ _add_visible_sort_targets(
+ allowed,
+ query.get("metrics"),
+ column_config,
+ is_metric=True,
+ )
+
+ return allowed
+
+
+def _collect_stored_orderby_entries(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
+ """
+ Frozen saved orderby entries a guest may replay exactly.
+ """
+ allowed = {
+ freeze_value(entry) for entry in stored_chart.params_dict.get("orderby") or []
+ }
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced local set so the modified code fully complies with the type-hint requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code, and the local set `allowed` is inferable but not explicitly typed. That matches the custom rule requiring type hints on relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=77097125f90d45da8590479716db9087&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=77097125f90d45da8590479716db9087&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 662:664
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local set so the modified code fully complies with the type-hint requirement.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=97e8b582d8e085cbeff8558aa9dac96e077a48c8b6fbb37774fe0ea0995556c0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=97e8b582d8e085cbeff8558aa9dac96e077a48c8b6fbb37774fe0ea0995556c0&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -575,28 +777,35 @@ def _orderby_modified(
metrics is legitimate and must not read as tampering; introducing a new
expression is not, and is rejected.
"""
- allowed = _collect_sortable_identifiers(stored_chart, stored_query_context)
+ visible_targets = _collect_sortable_identifiers(stored_chart, stored_query_context)
+ stored_orderby_entries = _collect_stored_orderby_entries(
+ stored_chart, stored_query_context
+ )
form_data = query_context.form_data or {}
# Both ``form_data`` and each ``QueryObject`` can carry an order-by, and in
# the common frontend path they carry the same one. Either source could
# smuggle an unauthorized term, so validate the union of both rather than
# trusting one over the other; the duplication is harmless.
- requested = list(form_data.get("orderby") or [])
+ form_orderby = form_data.get("orderby")
+ if form_orderby is not None and not isinstance(form_orderby, list):
+ return True
+ requested = list(form_orderby or [])
Review Comment:
**Suggestion:** Add a type annotation to this newly added local list variable to satisfy the enforced type-hint rule for modified code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced local list in modified Python code and it lacks an explicit type hint, so it violates the type-hint requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5fd72981f91c42b8aa5b722bc7599cad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5fd72981f91c42b8aa5b722bc7599cad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 792:792
**Comment:**
*Custom Rule: Add a type annotation to this newly added local list variable to satisfy the enforced type-hint rule for modified code.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=de5b69cf7302450f34531bf8d924f5a1146ce696bcba014c83082273d49735a3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=de5b69cf7302450f34531bf8d924f5a1146ce696bcba014c83082273d49735a3&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1412,6 +1444,31 @@ def test_query_context_modified_time_grain_native_filter(
assert not query_context_modified(query_context)
+def test_query_context_modified_orderby_visible_column_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that guest user can sort by a visible column (whitelist approach).
+ """
+ query_context = mocker.MagicMock()
Review Comment:
**Suggestion:** Add an explicit type annotation for this local mock context variable to satisfy the type-hint requirement for relevant variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a real violation of the type-hint rule: the new test helper and several new test cases introduce local variables like `query_context` without any annotation, even though they are relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1d32ff307fe8499bb3696dc4973dd3fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1d32ff307fe8499bb3696dc4973dd3fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1453:1453
**Comment:**
*Custom Rule: Add an explicit type annotation for this local mock context variable to satisfy the type-hint requirement for relevant variables.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=4102f21c91a6746f2d5ed6deed6fc424b89574b5fd0816f403f1108445615f56&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=4102f21c91a6746f2d5ed6deed6fc424b89574b5fd0816f403f1108445615f56&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -2057,6 +2057,102 @@ def test_orderby_adhoc_column(database: Database) -> None:
assert "ORDER BY" in sql.upper()
+def test_orderby_adhoc_column_label_takes_precedence_over_saved_metric(
+ database: Database,
+) -> None:
+ """
+ Test that orderby by an adhoc column label resolves to the selected column.
+ """
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+
+ table = SqlaTable(
Review Comment:
**Suggestion:** Add an explicit type annotation to `table` so the new test keeps local variables typed consistently with the type-hint rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test introduces a local variable initialized with `SqlaTable(...)` and leaves it unannotated. This is a newly added Python variable that can be typed explicitly, so it matches the type-hint rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d808cf08c37f4dc2b3ed3cb5c5e01eeb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d808cf08c37f4dc2b3ed3cb5c5e01eeb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/helpers_test.py
**Line:** 2068:2068
**Comment:**
*Custom Rule: Add an explicit type annotation to `table` so the new test keeps local variables typed consistently with the type-hint rule.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=8b092cf48a4a8069cd2abd1f0cf282b0af6237ce000f66f1ce9d01ae8d0b9fb6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=8b092cf48a4a8069cd2abd1f0cf282b0af6237ce000f66f1ce9d01ae8d0b9fb6&reaction=dislike'>👎</a>
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3786990878
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org>.
YuriyKrasilnikov commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4764512234
@rusackas thanks again for rebasing this and for the security-review guidance.
I pushed one more pass in b68d882830 and the PR is green now, including `pre-commit (current)`, `unit-tests (current)`, and `unit-tests-required`.
I re-traced the guest sorting path from the Table frontend down to backend validation and tightened the security boundary a bit more:
- the guest orderby whitelist now respects Table `column_config[<columnKey>].visible === false`, so a manually modified guest payload cannot sort by a selected-but-hidden Table column;
- label extraction now uses Superset's backend `get_column_name()` / `get_metric_name()` helpers, which keeps adhoc SIMPLE metrics without custom labels aligned with actual result keys like `SUM(sales)`;
- new guest `orderby` entries now have to be strict `[term, bool]` pairs, and newly supplied SQL expression objects / malformed SIMPLE metric dicts fail closed;
- saved/frozen owner-defined `orderby` entries are still allowed as before.
Local targeted validation: `.venv/bin/pytest tests/unit_tests/security/manager_test.py -q` passes with `89 passed`.
Since this does loosen guest-user query validation and CI is green now, CC @dpgaspar @mistercrunch for the careful security pass you asked for.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4699374609
Thanks for the thorough root-cause writeup @YuriyKrasilnikov. It's gone conflicting (again, sorry), so we'll need a rebase to get it merged, but we're getting better at staying on top of all these PRs these days. Since this loosens guest-user query validation it wants a careful security pass, so CC @dpgaspar @mistercrunch, and myself once it's green, and/or reach out on Slack.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4858207704
Thanks for tackling this! Guest column sorting for embedded dashboards was since merged in #41218, which looks to supersede this (and the branch now conflicts with master), so I think it can be closed. Appreciate the work — the sortable-column handling here was nicely defensive.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org>.
YuriyKrasilnikov commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3791086817
## Response to codeant-ai bot review
### The bot's suggestion is incorrect
The bot suggested supporting `{"column": "string"}` format. After **thorough investigation of Superset's type system**, this format **does not exist**:
```python
# superset/superset_typing.py:51-57
class AdhocMetric(TypedDict, total=False):
column: AdhocMetricColumn | None # ← DICT or None, NEVER string!
# superset-frontend/packages/superset-ui-core/src/query/types/Metric.ts
export interface AdhocMetricSimple {
column: { column_name?: string, ... }; // ← Always DICT
}
```
### Data Flow Analysis
```
Frontend (formData)
↓
saveModalActions.ts:172 → JSON.stringify(formData)
↓
/api/v1/chart/{id} (PUT/POST)
↓
charts/schemas.py → fields.Raw() ← NO STRUCTURE VALIDATION
↓
DATABASE: slices.params (MediumText)
↓
[Query Request]
↓
query_context_factory.py → ChartDAO.find_by_id()
↓
stored_chart.params_dict → json_to_dict(self.params)
↓
security/manager.py:_orderby_whitelist_compare() ← OUR BARRIERS
```
**Key point**: `fields.Raw()` in schema accepts ANY input without validation. Barriers must be in security code.
### However, we added defensive barriers anyway
Even though the bot's specific scenario is invalid, we applied **fail-closed defensive coding**. This protects against malformed API requests, 500 errors, and potential bypasses.
### What we added
**1. Validate orderby is a list** (`_orderby_whitelist_compare`):
```python
if form_orderby is not None and not isinstance(form_orderby, list):
return True # block invalid format
```
**Attack prevented:**
```python
form_data = {"orderby": "malicious"} # string, not list
# Without barrier: iterates chars "m","a","l"... all skip → PASSES!
# With barrier: blocked immediately
```
**2. Validate each element is tuple/list**
**3. Validate nested column is dict** (already existed)
### Similar patterns in Superset codebase
Our barriers follow existing Superset patterns:
| Location | Pattern | Code |
|----------|---------|------|
| `security/manager.py:812` | isinstance before access | `if isinstance(datasource, BaseDatasource):` |
| `charts/client_processing.py:112` | Type check before use | `if not isinstance(df, pd.DataFrame):` |
| `models/slice.py:374` | Safe conversion | `if id_or_uuid.isdigit(): return int()` |
| `common/query_object.py:279` | Validation method | `def validate(): ... raise QueryObjectValidationError` |
| `views/base.py:337` | try/except with fallback | `except json.JSONDecodeError: return fallback` |
### Why defensive barriers even if bot was wrong
| Reason | Explanation |
|--------|-------------|
| **Security-critical code** | Must be fail-closed (block unknown, don't pass) |
| **Schema accepts Raw()** | No structure validation at API level |
| **Defense in depth** | Don't rely on "frontend won't send this" |
| **Prevents 500 errors** | Graceful block instead of crash |
### Tests added (20 new tests)
- `test_extract_orderby_column_name_*` - unit tests for extraction
- `test_get_visible_columns_*` - unit tests for whitelist
- `test_query_context_modified_orderby_*` - integration tests for barriers
- Including: `test_query_context_modified_orderby_string_instead_of_list_blocked`
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4861911119
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 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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r3509764220
##########
superset/connectors/sqla/models.py:
##########
@@ -1678,6 +1678,11 @@ def adhoc_metric_to_sqla(
label = utils.get_metric_name(metric, self.verbose_map)
if expression_type == utils.AdhocMetricExpressionType.SIMPLE:
+ aggregate = metric.get("aggregate")
Review Comment:
**Suggestion:** Add an explicit type annotation for `aggregate` to satisfy the requirement for type-hinted newly introduced variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The added local variable `aggregate` is introduced without any type annotation, and it is used in new validation and indexing logic. This matches the rule requiring type hints for newly added relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9cdc7e9fd1c14fd7987c133c7b35b471&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9cdc7e9fd1c14fd7987c133c7b35b471&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 1681:1681
**Comment:**
*Custom Rule: Add an explicit type annotation for `aggregate` to satisfy the requirement for type-hinted newly introduced variables.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=17192fe9093bb92eed890127acc674bd98ed4aa565cea1ac70dcbddc147c1697&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=17192fe9093bb92eed890127acc674bd98ed4aa565cea1ac70dcbddc147c1697&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -2585,16 +2585,25 @@ def adhoc_metric_to_sqla(
label = utils.get_metric_name(metric)
if expression_type == utils.AdhocMetricExpressionType.SIMPLE:
+ aggregate = metric.get("aggregate")
Review Comment:
**Suggestion:** Add an explicit type annotation for the new local variable so the aggregate value’s expected type is clear and statically checked. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new local variable in modified Python code, and its type can be annotated (for example as an optional string). Because it is introduced without a type hint, it matches the rule requiring type hints for relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=938fe40e390749408c1e986e90e7bbec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=938fe40e390749408c1e986e90e7bbec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 2588:2588
**Comment:**
*Custom Rule: Add an explicit type annotation for the new local variable so the aggregate value’s expected type is clear and statically checked.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=6424e42d90ae793115cfbc84c13903451b29661882210a4e8fb655e43649f9e4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=6424e42d90ae793115cfbc84c13903451b29661882210a4e8fb655e43649f9e4&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1687,9 +1692,13 @@ def adhoc_metric_to_sqla(
)
else:
sqla_column = column(column_name)
- sqla_metric = self.sqla_aggregations[metric["aggregate"]](sqla_column)
+ sqla_metric = self.sqla_aggregations[aggregate](sqla_column)
elif expression_type == utils.AdhocMetricExpressionType.SQL:
expression = metric.get("sqlExpression")
+ if not isinstance(expression, str) or not expression:
Review Comment:
**Suggestion:** Add an explicit type annotation for `expression` so the new variable conforms to the type-hinting requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The added local variable `expression` is introduced without a type annotation. Since this variable is part of new validation logic and can be annotated, the omission aligns with the Python type-hint requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=42b176cbd8b94c129f346812b084e755&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=42b176cbd8b94c129f346812b084e755&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 1698:1698
**Comment:**
*Custom Rule: Add an explicit type annotation for `expression` so the new variable conforms to the type-hinting requirement.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=6e7460d9213c2109be70afcb381864e90c9075825549fba15e71ccf6e5d7062f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=6e7460d9213c2109be70afcb381864e90c9075825549fba15e71ccf6e5d7062f&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -2057,6 +2057,63 @@ def test_orderby_adhoc_column(database: Database) -> None:
assert "ORDER BY" in sql.upper()
+@pytest.mark.parametrize("aggregate", [None, "MEDIAN"])
+def test_adhoc_metric_to_sqla_invalid_simple_aggregate_raises_validation_error(
+ database: Database,
+ aggregate: str | None,
+) -> None:
+ """
+ Test that malformed SIMPLE adhoc metrics fail with a validation error.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.exceptions import QueryObjectValidationError
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[
+ TableColumn(column_name="a"),
+ ],
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this local variable to satisfy the type-hint requirement for newly introduced relevant variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test code introduces a local variable `table` without an explicit type annotation. Since this is newly added Python code and `SqlaTable` is annotatable, it matches the rule requiring type hints on relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b6690eeeef1a49bda9a84dfac47accca&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b6690eeeef1a49bda9a84dfac47accca&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/helpers_test.py
**Line:** 2071:2078
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable to satisfy the type-hint requirement for newly introduced relevant variables.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=59db7de880b33bb3f74a9d23ae9c4fff2e1e9cea54c111f66124b401f337aa0d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=59db7de880b33bb3f74a9d23ae9c4fff2e1e9cea54c111f66124b401f337aa0d&reaction=dislike'>👎</a>
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r2718818874
##########
superset/security/manager.py:
##########
@@ -212,6 +213,162 @@ def freeze_value(value: Any) -> str:
return json.dumps(value, sort_keys=True)
+# -----------------------------------------------------------------------------
+# Field comparison configuration for query_context_modified()
+# -----------------------------------------------------------------------------
+
+
+class FieldMapping(TypedDict):
+ """Configuration for comparing a field between requested and stored values."""
+
+ field: str
+ equivalent: list[str]
+ comparator: NotRequired[
+ Callable[
+ ["QueryContext", "Slice", set[str]], # query_context, stored_chart, visible
+ bool, # True if modified (should block)
+ ]
+ ]
+
+
+def _extract_orderby_column_name(orderby_item: Any) -> str | None:
+ """
+ Extract column/metric name from an orderby tuple element.
+
+ Returns None for SQL expressions which should be blocked for security.
+ """
+ if isinstance(orderby_item, str):
+ return orderby_item
+
+ if isinstance(orderby_item, dict):
+ # Block adhoc SQL expressions - potential injection vector
+ if orderby_item.get("expressionType") == "SQL":
+ return None
+ if label := orderby_item.get("label"):
+ return label
+ if col := orderby_item.get("column"):
+ return col.get("column_name")
Review Comment:
**Suggestion:** _extract_orderby_column_name may raise AttributeError when the orderby dict contains a "column" value that is a string (not a dict). The current code calls .get on `col` unguarded, which will fail if `col` is a str; handle string and dict cases explicitly and return None when the structure is unexpected. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Embedded dashboard sorting triggers 500 error.
- ❌ Guest user chart rendering fails during sorting.
- ⚠️ Backend raises unexpected AttributeError in validation.
```
</details>
```suggestion
col = orderby_item.get("column")
# `column` in orderby can be a dict (with column_name) or a plain string.
if isinstance(col, dict):
return col.get("column_name")
if isinstance(col, str):
return col
# Unexpected structure -> treat as blocked (None)
return None
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect helper at superset/security/manager.py:234 (_extract_orderby_column_name).
2. Construct a QueryContext whose orderby uses a dict with column as string:
- e.g., form_data = {"orderby": [[{"column": "country"}, "desc"]]}
- This mirrors frontend orderby payloads embedded dashboards may send.
3. Call query_context_modified(query_context) at superset/security/manager.py:372.
4. query_context_modified uses _FIELD_MAPPINGS and invokes the orderby comparator
_orderby_whitelist_compare (defined at superset/security/manager.py:285), which
calls _extract_orderby_column_name(orderby_tuple[0])
(superset/security/manager.py:234).
5. The current implementation does `if col := orderby_item.get("column"): return
col.get("column_name")`.
If `col` is a plain string (e.g., "country"), calling `col.get(...)` raises
AttributeError,
producing an unhandled 500 during the guest-request validation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 249:250
**Comment:**
*Possible Bug: _extract_orderby_column_name may raise AttributeError when the orderby dict contains a "column" value that is a string (not a dict). The current code calls .get on `col` unguarded, which will fail if `col` is a str; handle string and dict cases explicitly and return None when the structure is unexpected.
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.
```
</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(embedded): allow guest users to sort by visible columns [superset]
Posted by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org>.
YuriyKrasilnikov commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3789425390
## Full Analysis: `{"column": "country"}` format does not exist
We traced all data flows to verify if the proposed bug scenario can occur.
---
### 1. Type Definitions (superset/superset_typing.py)
```python
# Line 116
OrderBy = tuple[Metric | Column, bool] # Note: bool, not string "desc"
# Line 114-115
Column = AdhocColumn | str
Metric = AdhocMetric | str
# Lines 51-57
class AdhocMetric(TypedDict, total=False):
aggregate: str
column: AdhocMetricColumn | None # ← DICT or None, never string
expressionType: Literal["SIMPLE", "SQL"]
hasCustomLabel: bool | None
label: str | None
sqlExpression: str | None
# Lines 37-48
class AdhocMetricColumn(TypedDict, total=False):
column_name: str | None # ← Nested dict with column_name
description: str | None
expression: str | None
...
```
**Type conclusion:** `AdhocMetric.column` is `AdhocMetricColumn` (dict) or `None`, never a plain string.
---
### 2. API Entry Points
| Entry Point | File:Lines | Format |
|-------------|------------|--------|
| POST /api/v1/chart/data | charts/data/api.py:193-285 | JSON → QueryContextFactory |
| GET /api/v1/chart/{id}/data | charts/data/api.py:72-191 | Saved chart → QueryContextFactory |
| Schema validation | charts/schemas.py:1345-1363 | `fields.List(fields.Tuple((fields.Raw(), fields.Boolean())))` |
| QueryObject init | query_object.py:127,151 | `orderby: list[OrderBy]` |
| QueryContextFactory | query_context_factory.py:74-86 | Passes via **kwargs |
---
### 3. Frontend Components Analysis
| Component | File:Line | Actual Format |
|-----------|-----------|---------------|
| Table Chart sortBy | plugin-chart-table/src/buildQuery.ts:240 | `[[sortByItem.key, bool]]` where key is **string** |
| Table SortByItem type | plugin-chart-table/src/types.ts:119-123 | `{id: string, key: string, desc?: boolean}` |
| Filter Select | filters/components/Select/buildQuery.ts:63-66 | `sortColumns.map(column => [column, bool])` where column is **string** |
| MatrixifyControl | explore/.../fetchTopNValues.ts:57 | `[[metric, bool]]` where metric is **string** |
| DndMetricSelect | explore/.../DndMetricSelect.tsx:358 | `column: itemValue` where itemValue is `ColumnMeta` (**dict** with column_name) |
**Frontend conclusion:** orderby is always `[[string, bool]]` or `[[AdhocMetric_with_column_as_dict, bool]]`. Never `{"column": "string"}`.
---
### 4. Data Flow Trace
```
HTTP Request (JSON body)
↓
ChartDataRestApi.data() [api.py:193]
↓
json_body = request.json [api.py:244-248]
↓
_create_query_context_from_form(json_body) [api.py:253]
↓
ChartDataQueryContextSchema().load() [api.py:546]
↓
QueryContextFactory.create() [query_context_factory.py:74]
↓
QueryObject(orderby=...) [query_object.py:151]
↓
query_context_modified() [manager.py:372]
↓
_orderby_whitelist_compare() [manager.py:285]
↓
_extract_orderby_column_name(orderby_tuple[0]) [manager.py:301]
```
At no point is `{"column": "string"}` created or transformed.
---
### 5. Problems with Proposed Test Case
The example `form_data = {"orderby": [[{"column": "country"}, "desc"]]}` has two issues:
1. **`"desc"` should be `bool`** — OrderBy type requires `tuple[..., bool]`, not `tuple[..., str]`
2. **`{"column": "country"}` is invalid** — No TypedDict has this structure:
- `AdhocMetric.column` = dict (AdhocMetricColumn) or None
- `AdhocColumn` has no `column` field
- Plain columns are strings `"country"`, not `{"column": "country"}`
---
### Conclusion
The structure `{"column": "string_value"}` does not exist in the codebase:
- Violates TypedDict contracts
- Never generated by frontend
- Never transformed from valid input
- No code path creates this format
Current implementation is correct per type definitions.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r3509779738
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1247,6 +1247,36 @@ def _table_sort_query_context(
return query_context
+def _series_limit_metric_query_context(
+ mocker: MockerFixture,
+ requested_metric: Any,
+ *,
+ stored_metrics: Optional[list[Any]] = None,
+ form_metric_key: str = "series_limit_metric",
+) -> Any:
+ """
+ Build a minimal chart query context with a series-limit metric selector.
+ """
+ metrics: list[Any] = stored_metrics if stored_metrics is not None else ["count"]
+ query_context = mocker.MagicMock()
+ query_context.queries = [
+ QueryObject(metrics=metrics, series_limit_metric=requested_metric),
+ ]
Review Comment:
**Suggestion:** The helper always injects `series_limit_metric` into `QueryObject` regardless of `form_metric_key`, so the deprecated-key test does not actually isolate `timeseries_limit_metric` handling and can pass even if form-data support for that key regresses. Build the query object with the matching metric key (or omit query-level metric in that test) so the test verifies the intended contract. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Deprecated series-limit key coverage weaker than expected.
- ⚠️ Future regressions in timeseries_limit_metric may go undetected.
- ⚠️ Guest metric selector authorization tests give false sense of safety.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/security/manager_test.py` and locate
`_series_limit_metric_query_context` at lines 51–78 (in the read snapshot: helper starts
around line 51, `query_context.queries` assignment at lines 63–65). Note that it always
builds `QueryObject(metrics=metrics, series_limit_metric=requested_metric)` regardless of
the `form_metric_key` argument.
2. Open `superset/security/manager.py` and inspect `_series_limit_metric_modified` at
lines 720–31. Observe that it loops over both keys `("series_limit_metric",
"timeseries_limit_metric")` for values in `form_data` and for each `query` via
`getattr(query, key, None)`.
3. Inspect the test
`test_query_context_modified_timeseries_limit_metric_stored_metric_allowed` in
`tests/unit_tests/security/manager_test.py` at lines 747–760. The test passes
`form_metric_key="timeseries_limit_metric"` to the helper, so `form_data` uses the
deprecated key, but `query_context.queries` still carries
`series_limit_metric=requested_metric` due to the helper’s hardcoded field.
4. Consider a regression where `_series_limit_metric_modified` stops checking
`form_data.get("timeseries_limit_metric")` but still checks `series_limit_metric` on
queries (e.g., by narrowing the first loop to just `"series_limit_metric"`). Running the
tests in this scenario would still make
`test_query_context_modified_timeseries_limit_metric_stored_metric_allowed` pass, because
the query-level `series_limit_metric` remains valid, demonstrating that the helper’s
hardcoding can hide regressions in deprecated `timeseries_limit_metric` form-data
handling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cef4e2a6b4f549799307b3d9fd3553b1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cef4e2a6b4f549799307b3d9fd3553b1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1262:1264
**Comment:**
*Logic Error: The helper always injects `series_limit_metric` into `QueryObject` regardless of `form_metric_key`, so the deprecated-key test does not actually isolate `timeseries_limit_metric` handling and can pass even if form-data support for that key regresses. Build the query object with the matching metric key (or omit query-level metric in that test) so the test verifies the intended contract.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=52242666de820c9623dfebda8393e577132a0ea5bbe375cbd7134f5fcb7933ec&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=52242666de820c9623dfebda8393e577132a0ea5bbe375cbd7134f5fcb7933ec&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -2166,3 +2682,204 @@ def test_reset_password_self_service_pk_string_clears_flag(
# Coerced to int when clearing, regardless of the inbound id type.
mock_clear.assert_called_once_with(5)
+
+
+# -----------------------------------------------------------------------------
+# Tests for orderby with invalid formats - integration tests
+# -----------------------------------------------------------------------------
Review Comment:
**Suggestion:** This header is factually incorrect: these are unit tests using mocks in a unit-test module, not integration tests, which misleads future maintainers about test scope and guarantees; update the comment to reflect unit-test coverage. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Comment misleads about invalid orderby test scope.
- ⚠️ Maintainers may overestimate integration coverage here.
- ⚠️ Suggestion addresses documentation, not runtime behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/security/manager_test.py` and navigate to the section header
around lines 2687–2689, where the comment reads `# Tests for orderby with invalid formats
- integration tests`.
2. Confirm from the file path (`tests/unit_tests/security/manager_test.py`) and
surrounding code (all tests in this module use `mocker.MagicMock` and do not hit real
services or databases) that these are unit tests, not integration tests.
3. Observe that the comment explicitly labels the group as “integration tests,” which
misrepresents the scope and may mislead maintainers about the guarantees these tests
provide.
4. Because this is purely a documentation mismatch and does not affect runtime behavior or
test execution, the issue does not manifest as a functional bug but can cause confusion
about coverage; updating the comment would align documentation with actual test type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bd50a9ba5dd74ad0a4d577531c3732e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bd50a9ba5dd74ad0a4d577531c3732e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 2687:2689
**Comment:**
*Comment Mismatch: This header is factually incorrect: these are unit tests using mocks in a unit-test module, not integration tests, which misleads future maintainers about test scope and guarantees; update the comment to reflect unit-test coverage.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=8749de420eabd8076ad616d3f886d9fc2706aecc3cb3916b806a4743a34dbf3a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=8749de420eabd8076ad616d3f886d9fc2706aecc3cb3916b806a4743a34dbf3a&reaction=dislike'>👎</a>
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3813503389
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/37371?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 `12.35955%` with `78 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 43.60%. Comparing base ([`87bbd54`](https://app.codecov.io/gh/apache/superset/commit/87bbd54d0aca3b98671e0516e0b2d7a9e66bc3c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`32990fb`](https://app.codecov.io/gh/apache/superset/commit/32990fbb7365f5eb768aa4badf7da3f9f94459d1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 92 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/37371?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/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/37371?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 12.35% | [78 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/37371?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #37371 +/- ##
===========================================
+ Coverage 0 43.60% +43.60%
===========================================
Files 0 642 +642
Lines 0 49074 +49074
Branches 0 5520 +5520
===========================================
+ Hits 0 21401 +21401
- Misses 0 27376 +27376
- Partials 0 297 +297
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/37371/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/37371/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.86% <12.35%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/37371/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.88% <12.35%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/37371/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.60% <12.35%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/37371/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/37371?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(embedded): allow guest users to sort by visible columns [superset]
Posted by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org>.
YuriyKrasilnikov commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3813949880
## Fixed in 81e9859859
### ruff C901: Function too complex
**Problem:** `_orderby_whitelist_compare` had cyclomatic complexity 12 (max 10).
**Fix:** Extracted helper function `_validate_orderby_list` to handle orderby validation logic, reducing complexity of the main function.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "YuriyKrasilnikov (via GitHub)" <gi...@apache.org>.
YuriyKrasilnikov commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r2720532951
##########
superset/security/manager.py:
##########
@@ -212,6 +213,162 @@ def freeze_value(value: Any) -> str:
return json.dumps(value, sort_keys=True)
+# -----------------------------------------------------------------------------
+# Field comparison configuration for query_context_modified()
+# -----------------------------------------------------------------------------
+
+
+class FieldMapping(TypedDict):
+ """Configuration for comparing a field between requested and stored values."""
+
+ field: str
+ equivalent: list[str]
+ comparator: NotRequired[
+ Callable[
+ ["QueryContext", "Slice", set[str]], # query_context, stored_chart, visible
+ bool, # True if modified (should block)
+ ]
+ ]
+
+
+def _extract_orderby_column_name(orderby_item: Any) -> str | None:
+ """
+ Extract column/metric name from an orderby tuple element.
+
+ Returns None for SQL expressions which should be blocked for security.
+ """
+ if isinstance(orderby_item, str):
+ return orderby_item
+
+ if isinstance(orderby_item, dict):
+ # Block adhoc SQL expressions - potential injection vector
+ if orderby_item.get("expressionType") == "SQL":
+ return None
+ if label := orderby_item.get("label"):
+ return label
+ if col := orderby_item.get("column"):
+ return col.get("column_name")
Review Comment:
## Full Analysis: `{"column": "country"}` format does not exist
We traced all data flows to verify if the proposed bug scenario can occur.
---
### 1. Type Definitions (superset/superset_typing.py)
```python
# Line 116
OrderBy = tuple[Metric | Column, bool] # Note: bool, not string "desc"
# Line 114-115
Column = AdhocColumn | str
Metric = AdhocMetric | str
# Lines 51-57
class AdhocMetric(TypedDict, total=False):
aggregate: str
column: AdhocMetricColumn | None # ← DICT or None, never string
expressionType: Literal["SIMPLE", "SQL"]
hasCustomLabel: bool | None
label: str | None
sqlExpression: str | None
# Lines 37-48
class AdhocMetricColumn(TypedDict, total=False):
column_name: str | None # ← Nested dict with column_name
description: str | None
expression: str | None
...
```
**Type conclusion:** `AdhocMetric.column` is `AdhocMetricColumn` (dict) or `None`, never a plain string.
---
### 2. API Entry Points
| Entry Point | File:Lines | Format |
|-------------|------------|--------|
| POST /api/v1/chart/data | charts/data/api.py:193-285 | JSON → QueryContextFactory |
| GET /api/v1/chart/{id}/data | charts/data/api.py:72-191 | Saved chart → QueryContextFactory |
| Schema validation | charts/schemas.py:1345-1363 | `fields.List(fields.Tuple((fields.Raw(), fields.Boolean())))` |
| QueryObject init | query_object.py:127,151 | `orderby: list[OrderBy]` |
| QueryContextFactory | query_context_factory.py:74-86 | Passes via **kwargs |
---
### 3. Frontend Components Analysis
| Component | File:Line | Actual Format |
|-----------|-----------|---------------|
| Table Chart sortBy | plugin-chart-table/src/buildQuery.ts:240 | `[[sortByItem.key, bool]]` where key is **string** |
| Table SortByItem type | plugin-chart-table/src/types.ts:119-123 | `{id: string, key: string, desc?: boolean}` |
| Filter Select | filters/components/Select/buildQuery.ts:63-66 | `sortColumns.map(column => [column, bool])` where column is **string** |
| MatrixifyControl | explore/.../fetchTopNValues.ts:57 | `[[metric, bool]]` where metric is **string** |
| DndMetricSelect | explore/.../DndMetricSelect.tsx:358 | `column: itemValue` where itemValue is `ColumnMeta` (**dict** with column_name) |
**Frontend conclusion:** orderby is always `[[string, bool]]` or `[[AdhocMetric_with_column_as_dict, bool]]`. Never `{"column": "string"}`.
---
### 4. Data Flow Trace
```
HTTP Request (JSON body)
↓
ChartDataRestApi.data() [api.py:193]
↓
json_body = request.json [api.py:244-248]
↓
_create_query_context_from_form(json_body) [api.py:253]
↓
ChartDataQueryContextSchema().load() [api.py:546]
↓
QueryContextFactory.create() [query_context_factory.py:74]
↓
QueryObject(orderby=...) [query_object.py:151]
↓
query_context_modified() [manager.py:372]
↓
_orderby_whitelist_compare() [manager.py:285]
↓
_extract_orderby_column_name(orderby_tuple[0]) [manager.py:301]
```
At no point is `{"column": "string"}` created or transformed.
---
### 5. Problems with Proposed Test Case
The example `form_data = {"orderby": [[{"column": "country"}, "desc"]]}` has two issues:
1. **`"desc"` should be `bool`** — OrderBy type requires `tuple[..., bool]`, not `tuple[..., str]`
2. **`{"column": "country"}` is invalid** — No TypedDict has this structure:
- `AdhocMetric.column` = dict (AdhocMetricColumn) or None
- `AdhocColumn` has no `column` field
- Plain columns are strings `"country"`, not `{"column": "country"}`
---
### Conclusion
The structure `{"column": "string_value"}` does not exist in the codebase:
- Violates TypedDict contracts
- Never generated by frontend
- Never transformed from valid input
- No code path creates this format
Current implementation is correct per type definitions.
--
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(embedded): allow guest users to sort by visible columns [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-3791092119
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 32990fbb7365f5eb768aa4badf7da3f9f94459d1 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6973a064a557f30008a58a78 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-37371--superset-docs-preview.netlify.app](https://deploy-preview-37371--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37371--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(embedded): allow guest users to sort by visible columns [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37371:
URL: https://github.com/apache/superset/pull/37371#issuecomment-4753464956
Sorry about the confusion on the Issue... I'll try to rebase and resolve conflicts here, so we can get this through and close the issue for real!
--
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