EnxDev commented on PR #43111:
URL: https://github.com/apache/superset/pull/43111#issuecomment-5340888749
## EnxDev's Review Agent โ apache/superset#43111 ยท HEAD 5bec901
**comment** โ right direction and the vectors it does cover are handled
carefully, but the control is bypassed by omitting `slice_id`, and two
legitimate guest flows now 403.
On the earlier bot comments: the multi-query "predicate transfer" flag is a
non-issue (all queries in a context share one datasource and predicates only
restrict rows โ the docstring at `manager_test.py:3876` states this correctly),
and the missing end-to-end coverage is addressed by
`test_raise_for_access_guest_user_sql_filter_injection_blocked`. CI is green.
### ๐ด Functional
- **`superset/security/manager.py:1403-1410`** ยท _High_ โ the fix is
bypassed by dropping `slice_id`. A guest POSTs `/api/v1/chart/data` with
`form_data: {"dashboardId": N}` (no `slice_id`, no `chart_id`, no `type:
NATIVE_FILTER`) and arbitrary `extras.where`: `slice_` stays `None`
(`query_context_factory.py:64-68`), so `query_context_modified` takes the
chartless branch and `_native_filter_request_modified` returns `False` at its
marker check (`manager.py:797-800`) โ the new check never runs. Datasource
access is still granted, via `has_drill_access` (`manager.py:2076-2081`:
`slice_id is None and chart_id is None and datasource in
dashboard.datasources`) plus `can_access_dashboard`. So the exact attack in the
PR summary still executes against any dataset on the embedded dashboard,
including blind exfiltration through subqueries in the WHERE clause. Related:
the new comment "these paths are still protected by datasource-access checks in
`raise_for_access`" is not accurate โ
those checks gate *which dataset* is queried, not *what SQL* runs against it
(`_native_filter_request_modified`'s docstring makes the same claim). Constrain
`extras.where`/`having` and adhoc `filter[].col` on the chartless path against
the charts of the dashboard in `form_data["dashboardId"]`, or reject extras
that derive from no chart on it. **regression test:** guest `raise_for_access`
with `form_data={"dashboardId": 1}`, no `slice_id`, `extras={"where": "1=1"}` โ
must raise.
- **`superset/security/manager.py:1224-1229`** ยท _High_ โ cross-filtering
from a custom-SQL dimension now 403s the other charts. `getCrossFilterDataMask`
emits `{col, op, val}` carrying the **source** chart's raw `QueryFormColumn`
(`plugin-chart-echarts/src/utils/eventHandlers.ts:67-81`), which is an adhoc
dict whenever that dimension is Custom SQL, and `buildQueryObject` appends it
to every in-scope chart's `filters` (`buildQueryObject.ts:91`). `allowed` is
built only from the **target** chart, so unless the target happens to store the
same expression the guest gets "Guest user cannot modify chart payload". Before
this PR an adhoc col that didn't resolve degraded gracefully
(`rejected_adhoc_filters_columns`, `models/helpers.py:4361-4368`); now it's a
hard 403. Allow adhoc `col.sqlExpression` values found on any chart of the
requesting dashboard. **regression test:** stored chart B without the
expression + `filter[0].col.sqlExpression` = chart A's dimension, same
`dashboardId`
โ not modified.
- **`superset/security/manager.py:1216-1222`** ยท _Medium_ โ the async
results fetch compares normalized SQL against the raw stored SQL.
`QueryObject._sanitize_filters` rewrites `extras["where"/"having"]` in place
(`common/query_object.py:342-380`): Jinja is rendered when
`ENABLE_TEMPLATE_PROCESSING` is on, and `sanitize_clause` re-renders any clause
containing a comment (`sql/parse.py:2425-2437`). `get_payload_result` then
merges `query.to_dict()` into `cache_values["queries"]` and caches it
(`common/query_context_processor.py:516-528`), and the guest's `GET
/api/v1/chart/data/<cache_key>` rebuilds from that cached value and re-runs
`raise_for_access` (`charts/data/api.py:411-417`). With `GLOBAL_ASYNC_QUERIES`
on, a chart whose saved Custom SQL filter contains `--` or Jinja passes the
POST and then 403s on the results fetch โ the chart renders as an error for the
guest. Normalize both sides before comparing (run the stored expression through
`sanitize_clause` too), or skip th
e SQL comparison when the context was rehydrated from the query-context cache.
**regression test:** saved filter `a > 0 -- note`, re-validate with the
sanitized `(a > 0 /* note */)` โ not modified.
### ๐ก Should-fix
- **`tests/unit_tests/security/manager_test.py`** โ the `\n` strip added in
`_split_extras_clauses:1142` (in response to review feedback) has no test. Add
the case it exists for: stored `sqlExpression = "a > 0 -- x"`, request
`extras.where = "(a > 0 -- x\n)"` โ allowed. Without it the next refactor
silently drops comment handling.
### ๐ต Nits
- `superset/security/manager.py:1206-1214` โ the docstring's `) AND (`
caveat only fails closed for charts **with** a stored `query_context`; the
full-composed fallback is collected solely from stored query contexts
(`_add_allowed_sql_from_query_context:1155-1159`), so a chart with
`query_context = NULL` and such an expression 403s. Worth saying so, since NULL
query contexts are common.
### ๐ Praise
- `superset/security/manager.py:1120-1143` โ reversing `sanitizeClause`'s
parenthesis wrapping and its `--` newline, and allowing the `1 = 0`
required-filter sentinel, is exactly the frontend detail this check needed to
not break real dashboards.
<!-- enxdev-review-agent:5bec901 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 5bec901._
--
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]
