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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
