msyavuz commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3864088270
##########
superset/commands/explore/get.py:
##########
@@ -76,9 +77,17 @@ def run(self) -> Optional[dict[str, Any]]: # noqa: C901
initial_form_data["url_params"] = dict(url_params)
permalink_chart_state = state.get("chartState")
elif self._form_data_key:
- parameters = FormDataCommandParameters(key=self._form_data_key)
- value = GetFormDataCommand(parameters).run()
- initial_form_data = json.loads(value) if value else {}
+ try:
+ parameters = FormDataCommandParameters(key=self._form_data_key)
+ value = GetFormDataCommand(parameters).run()
+ initial_form_data = json.loads(value) if value else {}
+ except TemporaryCacheAccessDeniedError:
Review Comment:
This swallows a genuine 403: `check_access` denies via
`can_access_datasource`, but `raise_for_access` uses a different check set
(`can_access_schema` / `datasource_access` perm / `is_editor`) and never calls
it, so the denial isn't re-raised —
`test_get_dataset_access_denied_with_form_data_key` asserts 403 on exactly this
path and should now get 200.
##########
superset/commands/explore/get.py:
##########
@@ -76,9 +77,17 @@ def run(self) -> Optional[dict[str, Any]]: # noqa: C901
initial_form_data["url_params"] = dict(url_params)
permalink_chart_state = state.get("chartState")
elif self._form_data_key:
- parameters = FormDataCommandParameters(key=self._form_data_key)
- value = GetFormDataCommand(parameters).run()
- initial_form_data = json.loads(value) if value else {}
+ try:
+ parameters = FormDataCommandParameters(key=self._form_data_key)
+ value = GetFormDataCommand(parameters).run()
+ initial_form_data = json.loads(value) if value else {}
+ except TemporaryCacheAccessDeniedError:
+ # The cached form data references a datasource the user cannot
+ # access. Fall through so the datasource-based access check
+ # (raise_for_access) runs and returns a proper error — but
+ # only when we have enough context to resolve the datasource.
+ if not (self._datasource_id or self._slice_id):
Review Comment:
This guard doesn't guarantee `raise_for_access` actually runs: with only
`slice_id`, `get_form_data` sets `slc = None` on a failed `can_access_chart`,
so `get_datasource_info` raises, `datasource` stays `None` and the `if
datasource:` block is skipped entirely (same for a `datasource_id` whose
dataset is gone, since `DatasourceNotFound` is suppressed). The denied request
then returns 200 with `[Missing Dataset]` and "Form data not found in cache,
reverting to chart metadata."
##########
superset/security/manager.py:
##########
@@ -2416,7 +2413,7 @@ def get_datasource_access_link(
return _render_permission_instructions_link(
datasource_id=str(datasource.data["id"]),
- datasource_name=str(datasource.data["name"]),
+ # datasource_name intentionally omitted to prevent name disclosure
Review Comment:
`{datasource_name}` is still documented in `config.py:2128` and in this
method's + `_render_permission_instructions_link`'s docstrings, so deployments
templating it now silently get an empty value in their access-request URL —
worth updating those and adding an `UPDATING.md` note (the `datasource_name`
param also becomes dead code).
##########
superset/security/manager.py:
##########
@@ -2416,7 +2413,7 @@ def get_datasource_access_link(
return _render_permission_instructions_link(
datasource_id=str(datasource.data["id"]),
Review Comment:
The id is still disclosed here — with
`PERMISSION_INSTRUCTIONS_LINK="...?dataset={datasource_id}"` an unauthorized
user gets it in the 403 body anyway. Either the id is safe to disclose (and
dropping `extra["datasource"]` costs frontend functionality for nothing) or it
isn't and this link needs the same treatment.
--
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]
