rebenitez1802 commented on code in PR #43795:
URL: https://github.com/apache/superset/pull/43795#discussion_r3924497231
##########
superset/security/manager.py:
##########
@@ -1629,11 +1629,25 @@ def query_context_modified(query_context:
"QueryContext") -> bool:
)
return True
- stored_query_context = (
- json.loads(cast(str, stored_chart.query_context))
- if stored_chart.query_context
- else None
- )
+ try:
+ stored_query_context = (
+ json.loads(cast(str, stored_chart.query_context))
+ if stored_chart.query_context
+ else None
+ )
+ except (json.JSONDecodeError, TypeError):
+ # A stored query_context that fails to parse cannot be compared against
+ # the guest payload, so it is treated as modified/tampered (returning
+ # True triggers the SupersetSecurityException 403 in raise_for_access),
+ # consistent with the other rejection branches below. Malformed
+ # query_context can be persisted by the query-context-only update path
+ # (see ChartUpdateCommand._validate_query_context_datasource).
+ logger.warning(
+ "Guest chart payload rejected for slice %s: stored query_context "
+ "is not valid JSON",
+ stored_chart.id,
+ )
+ return True
Review Comment:
🟡 **Medium — a valid-but-non-object stored `query_context` still 500s this
guest path**
`json.loads` succeeds for *any* valid JSON, so a stored `query_context` of
`"[1,2,3]"`, `"123"`, or `"true"` parses to a non-`dict`, slips past the new
`except`, and then `stored_query_context.get("queries")` a few lines below
raises `AttributeError` → an uncaught 500 on the exact guest load this PR
hardens. `validate=utils.validate_json` on `ChartPutSchema.query_context` only
checks parseability (not object shape), so an editor can persist `"[1,2,3]"`
through the normal PUT — this case is actually more reachable than the
unparseable one. Coerce a non-object to tampered right after the parse,
matching the malformed-JSON branch:
```suggestion
return True
if stored_query_context is not None and not
isinstance(stored_query_context, dict):
# A stored query_context that parses but is not a JSON object (e.g. a
# list or scalar) cannot be compared against the guest payload and
would
# otherwise raise AttributeError on stored_query_context.get(...)
below;
# treat it as modified/tampered, matching the malformed-JSON branch.
logger.warning(
"Guest chart payload rejected for slice %s: stored query_context
"
"is not a JSON object",
stored_chart.id,
)
return True
```
--
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]
