msyavuz commented on PR #43111:
URL: https://github.com/apache/superset/pull/43111#issuecomment-5344974835
Two issues on the latest commits (`a0403ee`):
**1. `_add_dashboard_column_expressions` trusts an unauthorized
`dashboardId`.** `form_data["dashboardId"]` is request-controlled and the
lookup does no authorization check — no `has_guest_access(dashboard)`, and no
check that `stored_chart` is actually on that dashboard. Dashboard ids are
sequential ints, so a guest can name any dashboard and pull every adhoc-column
`sqlExpression` from every chart on it into `allowed`. The re-check then runs
the full `_query_has_novel_sql`, which covers `extras.where`/`having` and not
just the `filters[].col` vector the docstring describes, so those foreign
expressions become injectable as complete WHERE/HAVING predicates (correlated
subqueries included) against the datasource the guest can already reach.
Suggest resolving the dashboard through the guest-access check and confirming
`stored_chart.id in {s.id for s in dashboard.slices}`, then scoping the
expansion to the `filters[].col` vector rather than re-running the whole check.
**2. Scalar column params are silently dropped, in both
`_collect_allowed_sql` and the new `_add_dashboard_column_expressions`.**
`_STORED_COLUMN_PARAMS` mixes list-valued controls (`columns`, `groupby`) with
scalar ones (`x_axis`, `entity`, `series`, `granularity_sqla`); for the scalars
`params[key]` is an adhoc-column dict, so `for col in params.get(key) or []`
iterates the dict's *keys*, `isinstance(col, dict)` is always False, and
nothing is added — no error:
```python
params = {"x_axis": {"sqlExpression": "DATE_TRUNC('month', ts)", "label":
"m"},
"groupby": [{"sqlExpression": "UPPER(country)", "label": "c"}]}
_collect_allowed_sql(...) # -> {'1 = 0', 'UPPER(country)'} x_axis missing
```
So a cross-filter from a sibling chart whose dimension is an adhoc
**x-axis** — the case these commits were added to fix — still 403s.
`_stored_param_values` already handles this correctly (`items = value if
isinstance(value, (list, tuple)) else [value]`); reusing that pattern in both
places is the fix. The new test only exercises a list-valued `columns`, which
is why it passes.
--
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]
