rebenitez1802 commented on PR #40955: URL: https://github.com/apache/superset/pull/40955#issuecomment-4960183599
Approve with one question: well-scoped, well-tested defensive check with a sound rationale — mergeable as-is, pending confirmation that query-context datasource ids are always integers. **What it does:** On a query-context-only chart update (the relaxed-auth path used by report/alert workers, which skips editorship), `_validate_query_context_datasource` rejects a payload whose `query_context.datasource` points at an id/type other than the chart's own `datasource_id`/`datasource_type`. This keeps the stored query context from drifting away from — or being repointed at — a datasource the caller may not otherwise be entitled to, while leaving datasource-less payloads untouched (they fall back to the chart's own datasource at execution). Scoping is correct: it only runs in the `else` branch of `validate()`, so trusted editors on the normal path are unaffected. 🟡 Medium — UUID datasource references would be wrongly rejected `update.py` compares with `int(datasource["id"]) == self._model.datasource_id`, but `query_context_factory._convert_to_model` → `DatasourceDAO.get_datasource` explicitly accept a UUID string as the datasource id (`database_id_or_uuid`, `daos/datasource.py:68-73`). A query context that references the chart's own datasource by UUID makes `int()` raise `ValueError` → `ids_match = False` → a valid payload gets a 400. If query contexts in practice only ever carry integer datasource ids this is theoretical; if not, it's a regression. Fix (if applicable): when `datasource["id"]` isn't an int, resolve/compare against the datasource's UUID, and add a UUID test case. 🟢 Low — Non-string `query_context` silently skips the check `json.loads(raw_query_context)` catches `TypeError`, so an already-parsed `dict` (rather than a JSON string) is silently skipped. Fine given the schema serializes it as a string, but the `except (TypeError, ValueError): return` conflates "unparseable string" with "wrong Python type" — worth a one-line comment noting the dict case is intentionally out of scope here. 🟢 Low — Test gap for the hardening claim The parametrized tests cover matching, id/type mismatch, string-id, missing-type, and no-datasource/null/unparseable well. Missing: a UUID-string `datasource.id` (the Medium above) and a non-`table` datasource type — both would harden the "no false positives on valid payloads" guarantee. Worth adding to the PR body: on the relaxed-auth path this also has a mild authorization benefit (a principal with chart access but no access to datasource X can no longer stash `datasource: X` for a worker to later execute). If that's intended, stating it lets reviewers weigh it against the `SECURITY.md` role/capability matrix rather than as cosmetic drift-prevention. -- 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]
