From 3ad8d484d7001f15016b91884a5f9de40b693a4b Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Thu, 12 Mar 2026 21:21:43 +0300 Subject: [PATCH] fix: add parent_slice_id for multilayer charts to embed (#38243) (cherry picked from commit 95f61bd223ee7f72893197c77ec73a06d225443b) --- .../src/Multi/Multi.test.tsx | 137 ++++++++++++++++++ .../src/Multi/Multi.tsx | 2 + .../components/Chart/chartReducers.test.ts | 47 +++--- superset/security/manager.py | 58 +++++++- tests/unit_tests/security/manager_test.py | 114 +++++++++++++++ 5 files changed, 329 insertions(+), 29 deletions(-) diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx index f1e50c5b589..abce3761ad3 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx @@ -605,3 +605,140 @@ describe('DeckMulti Component Rendering', () => { }); }); }); + +test('includes parent_slice_id in child slice requests when parent has slice_id', async () => { + jest.clearAllMocks(); + const mockGet = jest.fn().mockResolvedValue({ + json: { + result: { + form_data: { + viz_type: 'deck_scatter', + datasource: '1__table', + }, + }, + data: { + features: [], + }, + }, + }); + (SupersetClient.get as jest.Mock) = mockGet; + const parentSliceId = 99; + const dashboardId = 5; + + const props = { + ...baseMockProps, + formData: { + ...baseMockProps.formData, + slice_id: parentSliceId, + dashboardId, + }, + }; + + renderWithProviders(); + + await waitFor(() => { + expect(mockGet).toHaveBeenCalled(); + }); + + // Check that the child slice requests include parent_slice_id + const { calls } = mockGet.mock; + calls.forEach(call => { + const { endpoint } = call[0]; + if (endpoint.includes('api/v1/explore/form_data')) { + const body = JSON.parse(call[0].body); + expect(body.form_data).toMatchObject({ + dashboardId, + parent_slice_id: parentSliceId, + }); + } + }); +}); + +test('includes parent_slice_id in embedded mode', async () => { + jest.clearAllMocks(); + const mockGet = jest.fn().mockResolvedValue({ + json: { + result: { + form_data: { + viz_type: 'deck_scatter', + datasource: '1__table', + }, + }, + data: { + features: [], + }, + }, + }); + (SupersetClient.get as jest.Mock) = mockGet; + const parentSliceId = 200; + const dashboardId = 10; + + const props = { + ...baseMockProps, + formData: { + ...baseMockProps.formData, + slice_id: parentSliceId, + dashboardId, + embedded: true, + }, + }; + + renderWithProviders(); + + await waitFor(() => { + expect(mockGet).toHaveBeenCalled(); + }); + + // Verify parent_slice_id is included in embedded mode + const { calls } = mockGet.mock; + calls.forEach(call => { + const { endpoint } = call[0]; + if (endpoint.includes('api/v1/explore/form_data')) { + const body = JSON.parse(call[0].body); + expect(body.form_data.parent_slice_id).toBe(parentSliceId); + } + }); +}); + +test('does not include parent_slice_id when parent has no slice_id', async () => { + jest.clearAllMocks(); + const mockGet = jest.fn().mockResolvedValue({ + json: { + result: { + form_data: { + viz_type: 'deck_scatter', + datasource: '1__table', + }, + }, + data: { + features: [], + }, + }, + }); + (SupersetClient.get as jest.Mock) = mockGet; + + const props = { + ...baseMockProps, + formData: { + ...baseMockProps.formData, + // No slice_id in parent + dashboardId: 5, + }, + }; + + renderWithProviders(); + + await waitFor(() => { + expect(mockGet).toHaveBeenCalled(); + }); + + // Verify parent_slice_id is not included when parent has no slice_id + const { calls } = mockGet.mock; + calls.forEach(call => { + const { endpoint } = call[0]; + if (endpoint.includes('api/v1/explore/form_data')) { + const body = JSON.parse(call[0].body); + expect(body.form_data.parent_slice_id).toBeUndefined(); + } + }); +}); diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx index c361eb755c8..3c52464a247 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx @@ -289,6 +289,8 @@ const DeckMulti = (props: DeckMultiProps) => { adhoc_filters: adhocFilters, // Preserve dashboard context for embedded mode permissions ...(formData.dashboardId && { dashboardId: formData.dashboardId }), + // Include parent multilayer chart ID for security checks + ...(formData.slice_id && { parent_slice_id: formData.slice_id }), }, } as any as JsonObject & { slice_id: number }; diff --git a/superset-frontend/src/components/Chart/chartReducers.test.ts b/superset-frontend/src/components/Chart/chartReducers.test.ts index ee5010358cb..d5b660dd810 100644 --- a/superset-frontend/src/components/Chart/chartReducers.test.ts +++ b/superset-frontend/src/components/Chart/chartReducers.test.ts @@ -35,36 +35,35 @@ describe('chart reducers', () => { }); test('should update endtime on fail', () => { - const controller = new AbortController(); - charts[chartKey] = { - ...charts[chartKey], - queryController: controller, - }; - const newState = chartReducer( - charts, - actions.chartUpdateStopped(chartKey, controller), - ); + const newState = chartReducer(charts, actions.chartUpdateStopped(chartKey)); expect(newState[chartKey].chartUpdateEndTime).toBeGreaterThan(0); expect(newState[chartKey].chartStatus).toEqual('stopped'); }); - test('should ignore stopped updates from stale controllers', () => { - const controller = new AbortController(); - const staleController = new AbortController(); - charts[chartKey] = { - ...charts[chartKey], - chartStatus: 'loading', - queryController: controller, - }; - - const newState = chartReducer( - charts, - actions.chartUpdateStopped(chartKey, staleController), + test('should handle chartUpdateStopped without queryController', () => { + const newState = chartReducer(charts, actions.chartUpdateStopped(chartKey)); + expect(newState[chartKey].chartStatus).toEqual('stopped'); + expect(newState[chartKey].chartAlert).toContain( + 'Updating chart was stopped', ); + expect(newState[chartKey].chartUpdateEndTime).toBeGreaterThan(0); + }); - expect(newState[chartKey].chartStatus).toEqual('loading'); - expect(newState[chartKey].chartUpdateEndTime).toEqual( - charts[chartKey].chartUpdateEndTime, + test('chartUpdateStopped sets state correctly', () => { + const chartsWithController = { + [chartKey]: { + ...testChart, + queryController: new AbortController(), + }, + }; + const newState = chartReducer( + chartsWithController, + actions.chartUpdateStopped(chartKey), + ); + // Verify the chart status and alert are set + expect(newState[chartKey].chartStatus).toEqual('stopped'); + expect(newState[chartKey].chartAlert).toContain( + 'Updating chart was stopped', ); }); diff --git a/superset/security/manager.py b/superset/security/manager.py index 6af83a4537a..05217235c57 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -688,6 +688,32 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods return False + def _validate_child_in_parent_multilayer( + self, child_slice_id: int, parent_slice: "Slice" + ) -> bool: + """ + Validate that a child slice ID is actually configured in the parent + multi-layer chart's deck_slices configuration. + + This prevents attackers from forging a parent_slice_id to gain + unauthorized access to arbitrary charts. + """ + try: + # Parse the parent chart's configuration + parent_form_data = json.loads(parent_slice.params or "{}") + + # Check if this is actually a multi-layer deck.gl chart + if parent_form_data.get("viz_type") != "deck_multi": + return False + + # Get the configured child slices + deck_slices = parent_form_data.get("deck_slices", []) + + # Validate the child is in the parent's configuration + return child_slice_id in deck_slices + except (json.JSONDecodeError, TypeError): + return False + def has_drill_by_access( self, form_data: dict[str, Any], @@ -2593,12 +2619,34 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods form_data.get("type") != "NATIVE_FILTER" and (slice_id := form_data.get("slice_id")) and ( - slc := self.session.query(Slice) - .filter(Slice.id == slice_id) - .one_or_none() + # Direct chart access (no parent) + ( + form_data.get("parent_slice_id") is None + and ( + slc := self.session.query(Slice) + .filter(Slice.id == slice_id) + .one_or_none() + ) + and slc in dashboard_.slices + and slc.datasource == datasource + ) + or + # Multi-layer chart child access (has parent) + ( + (parent_id := form_data.get("parent_slice_id")) + and ( + parent_slc := self.session.query(Slice) + .filter(Slice.id == parent_id) + .one_or_none() + ) + and parent_slc in dashboard_.slices + # Validate child is actually part of parent's config + and self._validate_child_in_parent_multilayer( + child_slice_id=slice_id, + parent_slice=parent_slc, + ) + ) ) - and slc in dashboard_.slices - and slc.datasource == datasource ) or self.has_drill_by_access(form_data, dashboard_, datasource) ) diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index a59b969f1e8..ca4b0b8df8b 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -1433,3 +1433,117 @@ def test_prefetch_rls_filters_works_for_guest_user( # Cache should be populated with (username, table_id) keys and empty lists assert mock_g._rls_filter_cache[("guest_user", 10)] == [] assert mock_g._rls_filter_cache[("guest_user", 20)] == [] + + +def test_validate_child_in_parent_multilayer_valid( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation succeeds for valid multi-layer child""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = json.dumps( + {"viz_type": "deck_multi", "deck_slices": [1, 2, 3]} + ) + + # Child 2 is in parent's deck_slices + assert sm._validate_child_in_parent_multilayer( + child_slice_id=2, parent_slice=parent_slice + ) + + +def test_validate_child_in_parent_multilayer_invalid_child( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation fails for child not in parent config""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = json.dumps( + {"viz_type": "deck_multi", "deck_slices": [1, 2, 3]} + ) + + # Child 5 is NOT in parent's deck_slices + assert not sm._validate_child_in_parent_multilayer( + child_slice_id=5, parent_slice=parent_slice + ) + + +def test_validate_child_in_parent_multilayer_wrong_viz_type( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation fails for non-multilayer charts""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = json.dumps( + { + "viz_type": "line", # Not deck_multi + "deck_slices": [1, 2, 3], + } + ) + + assert not sm._validate_child_in_parent_multilayer( + child_slice_id=2, parent_slice=parent_slice + ) + + +def test_validate_child_in_parent_multilayer_empty_deck_slices( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation fails when deck_slices is empty""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = json.dumps({"viz_type": "deck_multi", "deck_slices": []}) + + assert not sm._validate_child_in_parent_multilayer( + child_slice_id=1, parent_slice=parent_slice + ) + + +def test_validate_child_in_parent_multilayer_no_deck_slices( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation fails when deck_slices is missing""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = json.dumps( + { + "viz_type": "deck_multi" + # No deck_slices key + } + ) + + assert not sm._validate_child_in_parent_multilayer( + child_slice_id=1, parent_slice=parent_slice + ) + + +def test_validate_child_in_parent_multilayer_malformed_json( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation fails gracefully with malformed JSON""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = "not valid json {{" + + assert not sm._validate_child_in_parent_multilayer( + child_slice_id=1, parent_slice=parent_slice + ) + + +def test_validate_child_in_parent_multilayer_null_params( + app_context: None, mocker: MockerFixture +) -> None: + """Test validation fails gracefully with null params""" + sm = SupersetSecurityManager(appbuilder) + + parent_slice = mocker.MagicMock(spec=Slice) + parent_slice.params = None + + assert not sm._validate_child_in_parent_multilayer( + child_slice_id=1, parent_slice=parent_slice + )