msyavuz commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3873802913
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -246,6 +244,7 @@ export default function ExplorePage() {
? makeApi<void, { result: Chart }>({
method: 'GET',
endpoint: `api/v1/chart/${chartId}`,
+ signal: controller.signal,
Review Comment:
This `signal` isn't paired with an AbortError guard in the `.catch` below —
on unmount the abort fires while `fetchGeneration` is unchanged, so `isStale()`
is false and `hydrateExplore` still dispatches for a torn-down page (the outer
catch around line 211 does filter AbortError).
##########
superset/explore/api.py:
##########
@@ -135,7 +135,11 @@ def get(self) -> Response:
return self.response(404, message=str(ex))
except WrongEndpointError as ex:
return self.response(302, redirect=ex.redirect)
- except TemporaryCacheAccessDeniedError as ex:
- return self.response(403, message=str(ex))
+ except TemporaryCacheAccessDeniedError:
+ return self.response(
+ 403,
+ message="You do not have permission to access this datasource",
+ extra={"is_access_denial": True},
Review Comment:
This body has no `error_type`/`level` (unlike the `**ex.to_dict()` branch
above), so `ErrorMessageWithStackTrace` has nothing to dispatch on and renders
the generic fallback rather than `DatasourceSecurityAccessErrorMessage` — the
"Request access"/owners UI that `is_access_denial` exists to enable never shows
on this path.
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -258,7 +257,6 @@ export default function ExplorePage() {
}
const slice = {
...data,
Review Comment:
The dataset name still reaches the unauthorized user here: `GET
/api/v1/chart/<id>` returns
`datasource_name_text`/`datasource_url`/`datasource_uuid`
(`ChartGetResponseSchema`), and `ChartFilter` grants that GET to any chart
viewer regardless of dataset access — so `...data` spreads it into
`state.explore.slice` even with `datasource: err.extra?.datasource_name`
removed.
##########
superset/explore/api.py:
##########
@@ -135,7 +135,11 @@ def get(self) -> Response:
return self.response(404, message=str(ex))
except WrongEndpointError as ex:
return self.response(302, redirect=ex.redirect)
- except TemporaryCacheAccessDeniedError as ex:
- return self.response(403, message=str(ex))
+ except TemporaryCacheAccessDeniedError:
Review Comment:
`TemporaryCacheAccessDeniedError` never carried the dataset name (its
message is "You don't have permission to modify the value."), and
`check_access` in `commands/explore/form_data/utils.py` raises it for
`ChartAccessDeniedError` too — so this turns a chart-permission denial into a
datasource denial, and the hardcoded string also drops the `lazy_gettext` that
`str(ex)` provided.
##########
superset/security/manager.py:
##########
@@ -226,7 +228,7 @@ def _render_permission_instructions_link(
for token, value in (
("datasource_id", datasource_id),
- ("datasource_name", datasource_name),
+ ("datasource_name", ""), # removed to prevent information disclosure
Review Comment:
Substituting `""` rather than dropping the token means a deployment with
`PERMISSION_INSTRUCTIONS_LINK = "https://acme/request/{datasource_name}"`
silently renders `https://acme/request/`; leaving the placeholder literal or
logging a deprecation would fail visibly.
--
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]
