codeant-ai-for-open-source[bot] commented on code in PR #41017:
URL: https://github.com/apache/superset/pull/41017#discussion_r3409033647
##########
superset/security/manager.py:
##########
@@ -342,10 +342,29 @@ def _init_properties(self) -> None:
ViewMenuModelView.include_route_methods = {RouteMethod.LIST}
+# Keys on an adhoc column/metric that a guest may legitimately change through a
+# supported native filter, and which therefore must not count as payload
+# tampering. The time grain of a temporal x-axis is baked into its `BASE_AXIS`
+# column by `normalizeTimeColumn` on the frontend (it copies
+# `extras.time_grain_sqla` onto the column), so a Time Grain filter alters the
+# column payload without changing which data is queried.
+GUEST_OVERRIDABLE_VALUE_KEYS = frozenset({"timeGrain"})
+
+
def freeze_value(value: Any) -> str:
"""
Used to compare column and metric sets.
+
+ Guest-overridable keys (e.g. the time grain baked into a temporal x-axis
+ column) are dropped so that legitimate native-filter changes don't read as
+ payload tampering.
"""
+ if isinstance(value, dict):
+ value = {
+ key: val
+ for key, val in value.items()
+ if key not in GUEST_OVERRIDABLE_VALUE_KEYS
+ }
Review Comment:
**Suggestion:** `freeze_value` removes guest-overridable keys only when they
appear at the top level of a dict, but query payloads can contain adhoc columns
nested inside list/tuple structures (for example in `orderby`). In those cases
`timeGrain` is not stripped, so legitimate Time Grain native-filter changes can
still be flagged as tampering. Normalize recursively (or at least handle dicts
nested inside sequences) before serializing. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Embedded guest /v1/query charts fail after time-grain change.
- ⚠️ Guest dashboard interactions break when sorting by temporal axis.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the guest tamper guard in `superset/security/manager.py:52-106` where
`query_context_modified()` iterates over `["metrics", "columns", "groupby",
"orderby"]`
and, for each `key`, builds `queries_values = {freeze_value(value) for query
in
query_context.queries for value in getattr(query, key, []) or []}`.
2. Observe that for `key == "orderby"`, each `value` is an `OrderBy` tuple
as defined in
`superset/superset_typing.py:7-9` (`OrderBy: tuple[Metric | Column, bool]`),
where
`Column` can be an `AdhocColumn` dict containing a `timeGrain` field.
3. Confirm from `superset/common/query_object.py:25-35` that
`QueryObject.orderby` is
stored as `list[OrderBy]`, so `query.orderby` is a list of tuples whose
first element may
be an adhoc column dict (e.g. tests in
`tests/unit_tests/semantic_layers/mapper_test.py:580-7` show
`orderby=[({"label":
"custom_order", "sqlExpression": "RAND()"}, True)]`).
4. When an embedded guest hits the `/v1/query/` API
(`superset/views/api.py:60-18`),
`query_context.raise_for_access()` eventually calls
`SupersetSecurityManager.raise_for_access()`
(`superset/security/manager.py:3168-20`),
which invokes `query_context_modified(query_context)` for guest users; for a
chart ordered
by a temporal base axis, a Time Grain native filter mutates the axis adhoc
column's
`timeGrain` property, so the stored `orderby` tuples contain `{"timeGrain":
"OLD", ...}`
while the guest request's `orderby` tuples contain `{"timeGrain": "NEW",
...}`. Since
`freeze_value` at `superset/security/manager.py:35-49` only strips
`timeGrain` when the
top-level `value` is a `dict`, and `orderby` entries are tuples, the nested
`timeGrain` is
preserved in the JSON; the resulting `queries_values` set for the guest
request is not a
subset of `stored_values`, causing `query_context_modified()` to return
`True` and
`raise_for_access()` to raise `SupersetSecurityException("Guest user cannot
modify chart
payload")` even though only the time grain changed via a supported native
filter.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=acb6e8a26040441fbd49133cb46c8e14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=acb6e8a26040441fbd49133cb46c8e14&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:** 362:367
**Comment:**
*Logic Error: `freeze_value` removes guest-overridable keys only when
they appear at the top level of a dict, but query payloads can contain adhoc
columns nested inside list/tuple structures (for example in `orderby`). In
those cases `timeGrain` is not stripped, so legitimate Time Grain native-filter
changes can still be flagged as tampering. Normalize recursively (or at least
handle dicts nested inside sequences) before serializing.
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%2F41017&comment_hash=f5532dda7770422303d986eb6edecf170f54b738288bf7adc230c47ba45cbf9f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41017&comment_hash=f5532dda7770422303d986eb6edecf170f54b738288bf7adc230c47ba45cbf9f&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: [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]
