EnxDev commented on PR #43458: URL: https://github.com/apache/superset/pull/43458#issuecomment-5412673061
## EnxDev's Review Agent — apache/superset#43458 · HEAD 68a1a92 comment — legit fix for the disclosure, fully verified end-to-end; one untested (but safe) fallback branch ### Summary Restricted users could learn a dataset's name (and id) via the 403 body returned by `GET /api/v1/explore/`. The fix strips `datasource`/`datasource_name` from `SupersetSecurityException.extra` (`superset/security/manager.py`) and replaces them with a boolean `is_access_denial` flag; the frontend renders a generic message off that flag instead of interpolating a name. I traced the response path (`superset/explore/api.py`'s `except SupersetSecurityException: ... ex.to_dict()`) to confirm `extra` is exactly what serializes into the JSON body — this is the right place to fix it. No open review threads; CI checks were still queued at review time. I additionally verified the frontend side closes a second leak vector: `superset-frontend/src/pages/Chart/index.tsx` no longer seeds the fallback `dataset.name`/`dataset.id` from `err.extra`. That matters beyond cleanup — `DatasourceControl/index.tsx:483-486` renders `datasource.name` directly as the control's title when set, so leaving the old assignment in place would have re-leaked the name into the UI even after the backend stopped sending it. Good catch bundling that in. ### 🟡 Should-fix - **`superset/commands/explore/get.py:79-88`** — the new `try/except TemporaryCacheAccessDeniedError` fallback (swallow-and-continue when `datasource_id`/`slice_id` is present) has no test. Codecov flags exactly 2 missing lines here. I traced it manually and it's safe — the swallowed path still runs through `security_manager.raise_for_access()` later, so it can't bypass authz — but it's new control flow in an access-denial code path with zero coverage. Add a unit test (e.g. alongside `tests/unit_tests/commands/explore/test_get_explore_rls.py`) that mocks `GetFormDataCommand.run()` to raise `TemporaryCacheAccessDeniedError` with `datasource_id` set, and asserts the command falls through to a normal `raise_for_access` 403 instead of propagating the cache error. ### 🔵 Nits - `superset-frontend/.../DatasourceSecurityAccessErrorMessage.test.tsx:48` — test title "shows a friendly title without revealing the dataset name" doesn't actually exercise that guarantee: `baseProps.extra` never contained `datasource_name` (the type no longer has the field), so there's nothing being verified not to render. The real regression guard for the backend leak already lives in `test_permission_instructions_link.py`; consider renaming this test or dropping the "without revealing" framing. ### 🙌 Praise - `tests/unit_tests/security/test_permission_instructions_link.py:159-172` — `test_datasource_error_object_includes_sorted_owner_names` and `test_datasource_access_error_msg_is_generic` explicitly assert `datasource_name`/`datasource` are absent from `extra` and that the id/name strings don't appear in the message. That's exactly the regression guard this fix needs. <!-- enxdev-review-agent:68a1a92 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 68a1a92._ -- 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]
