Skip to content

feat: allow drill-to-details to a specific chart (optionaly) configured at the dataset level #34785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mistercrunch wants to merge 13 commits into
base: master
Choose a base branch
from
+1,222 −71
Open

feat: allow drill-to-details to a specific chart (optionaly) configured at the dataset level #34785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a891ccd
fix package-lock
mistercrunch Aug 21, 2025
56152c2
fix: Ensure drill-through chart field works consistently across CRUD …
mistercrunch Aug 30, 2025
06e6411
refactor: Add dashboard filter context to drill-through StatefulChart
mistercrunch Aug 30, 2025
d8ee744
refactor: Improve ChartSelect type safety and URL building utilities
mistercrunch Aug 30, 2025
e05061b
refactor: Simplify SelectAsyncControl by leveraging SupersetClient se…
mistercrunch Aug 30, 2025
abb6069
fix two tests
mistercrunch Sep 2, 2025
cf4fd3e
feat: enhance drill-through modal Edit chart button visibility and na…
mistercrunch Sep 3, 2025
843b81c
feat: improve drill-through modal functionality and navigation
mistercrunch Sep 4, 2025
41e7f51
fix: Drill-to-detail context menu filters now display as dashboard-in…
mistercrunch Sep 4, 2025
d4d02c5
refactor: Add generateExploreUrl utility to centralize URL generation
mistercrunch Sep 4, 2025
dd3b896
fix last test
mistercrunch Sep 4, 2025
02790b7
refactor a dup logic
mistercrunch Sep 4, 2025
d2d2cee
refactor a dup logic
mistercrunch Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions superset-frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 2 additions & 7 deletions superset-frontend/src/SqlLab/components/ResultSet/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ import {
SaveDatasetModal,
} from 'src/SqlLab/components/SaveDatasetModal';
import { EXPLORE_CHART_DEFAULT, SqlLabRootState } from 'src/SqlLab/types';
import { mountExploreUrl } from 'src/explore/exploreUtils';
import { postFormData } from 'src/explore/exploreUtils/formData';
import { generateExploreUrl } from 'src/explore/exploreUtils/formData';
import ProgressBar from '@superset-ui/core/components/ProgressBar';
import { addDangerToast } from 'src/components/MessageToasts/actions';
import { prepareCopyToClipboardTabularData } from 'src/utils/common';
Expand All @@ -78,7 +77,6 @@ import {
reFetchQueryResults,
reRunQuery,
} from 'src/SqlLab/actions/sqlLab';
import { URL_PARAMS } from 'src/constants';
import useLogAction from 'src/logger/useLogAction';
import {
LOG_ACTIONS_SQLLAB_COPY_RESULT_TO_CLIPBOARD,
Expand Down Expand Up @@ -277,16 +275,13 @@ const ResultSet = ({
const openInNewWindow = clickEvent.metaKey;
logAction(LOG_ACTIONS_SQLLAB_CREATE_CHART, {});
if (results?.query_id) {
const key = await postFormData(results.query_id, 'query', {
const url = await generateExploreUrl(results.query_id, 'query', {
...EXPLORE_CHART_DEFAULT,
datasource: `${results.query_id}__query`,
...{
all_columns: results.columns.map(column => column.column_name),
},
});
const url = mountExploreUrl(null, {
[URL_PARAMS.formDataKey.name]: key,
});
if (openInNewWindow) {
window.open(url, '_blank', 'noreferrer');
} else {
Expand Down
13 changes: 7 additions & 6 deletions superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {
} from '@superset-ui/core/components';
import { RootState } from 'src/dashboard/types';
import { DashboardPageIdContext } from 'src/dashboard/containers/DashboardPage';
import { postFormData } from 'src/explore/exploreUtils/formData';
import { generateExploreUrl } from 'src/explore/exploreUtils/formData';
import { simpleFilterToAdhoc } from 'src/utils/simpleFilterToAdhoc';
import { useDatasetMetadataBar } from 'src/features/datasets/metadataBar/useDatasetMetadataBar';
import { useToasts } from 'src/components/MessageToasts/withToasts';
Expand Down Expand Up @@ -96,11 +96,12 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => {
useEffect(() => {
// short circuit if the user is embedded as explore is not available
if (isEmbedded()) return;
postFormData(Number(datasource_id), datasource_type, formData, 0)
.then(key => {
setUrl(
`/explore/?form_data_key=${key}&dashboard_page_id=${dashboardPageId}`,
);
generateExploreUrl(Number(datasource_id), datasource_type, formData, {
chartId: 0,
dashboardPageId,
})
.then(url => {
setUrl(url);
})
.catch(() => {
addDangerToast(t('Failed to generate chart edit URL'));
Expand Down
59 changes: 40 additions & 19 deletions superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ jest.mock('react-router-dom', () => ({
}),
}));

jest.mock('src/explore/exploreUtils', () => ({
...jest.requireActual('src/explore/exploreUtils'),
getExploreUrl: jest.fn(
({ formData }) =>
`/explore/?dashboard_page_id=&slice_id=${formData.slice_id}`,
),
}));

const { id: chartId, form_data: formData } = chartQueries[sliceId];
const { slice_name: chartName } = formData;
const store = getMockStoreWithNativeFilters();
Expand All @@ -43,7 +51,10 @@ const drillToDetailModalState = {
},
};

const renderModal = async (overrideState: Record<string, any> = {}) => {
const renderModal = async (
overrideState: Record<string, any> = {},
dataset?: any,
) => {
const DrillDetailModalWrapper = () => {
const [showModal, setShowModal] = useState(false);
return (
Expand All @@ -57,6 +68,7 @@ const renderModal = async (overrideState: Record<string, any> = {}) => {
initialFilters={[]}
showModal={showModal}
onHideModal={() => setShowModal(false)}
dataset={dataset}
/>
</>
);
Expand All @@ -80,11 +92,21 @@ test('should render the title', async () => {
expect(screen.getByText(`Drill to detail: ${chartName}`)).toBeInTheDocument();
});

test('should render the button', async () => {
test('should not render Explore button when no drill-through chart is configured', async () => {
await renderModal();
expect(
screen.getByRole('button', { name: 'Edit chart' }),
).toBeInTheDocument();
screen.queryByRole('button', { name: 'Explore' }),
).not.toBeInTheDocument();
expect(screen.getAllByRole('button', { name: 'Close' })).toHaveLength(2);
});

test('should render Explore button when drill-through chart is configured', async () => {
const datasetWithDrillThrough = {
drill_through_chart_id: 123,
id: 456, // Required for URL generation
};
await renderModal({}, datasetWithDrillThrough);
expect(screen.getByRole('button', { name: 'Explore' })).toBeInTheDocument();
expect(screen.getAllByRole('button', { name: 'Close' })).toHaveLength(2);
});

Expand All @@ -95,20 +117,19 @@ test('should close the modal', async () => {
expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
});

test('should forward to Explore', async () => {
await renderModal();
userEvent.click(screen.getByRole('button', { name: 'Edit chart' }));
expect(mockHistoryPush).toHaveBeenCalledWith(
`/explore/?dashboard_page_id=&slice_id=${sliceId}`,
);
});

test('should render "Edit chart" as disabled without can_explore permission', async () => {
await renderModal({
user: {
...drillToDetailModalState.user,
roles: { Admin: [['invalid_permission', 'Superset']] },
test('should render "Explore" as disabled without can_explore permission', async () => {
const datasetWithDrillThrough = {
drill_through_chart_id: 123,
id: 456, // Required for URL generation
};
await renderModal(
{
user: {
...drillToDetailModalState.user,
roles: { Admin: [['invalid_permission', 'Superset']] },
},
},
});
expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled();
datasetWithDrillThrough,
);
expect(screen.getByRole('button', { name: 'Explore' })).toBeDisabled();
});
101 changes: 84 additions & 17 deletions superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
* under the License.
*/

import { useCallback, useContext, useMemo } from 'react';
import { useHistory } from 'react-router-dom';
import { useContext, useMemo, useState } from 'react';
import {
BinaryQueryObjectFilterClause,
css,
Expand All @@ -33,37 +32,46 @@
import { Slice } from 'src/types/Chart';
import { RootState } from 'src/dashboard/types';
import { findPermission } from 'src/utils/findPermission';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { getFormDataWithDashboardContext } from 'src/explore/controlUtils/getFormDataWithDashboardContext';
import { useDashboardFormData } from 'src/dashboard/hooks/useDashboardFormData';
import { generateExploreUrl } from 'src/explore/exploreUtils/formData';
import { Dataset } from '../types';
import DrillDetailPane from './DrillDetailPane';

interface ModalFooterProps {
canExplore: boolean;
closeModal?: () => void;
exploreChart: () => void;
showEditButton: boolean;
onExploreClick?: (event: React.MouseEvent) => void;
isGeneratingUrl: boolean;
}

const ModalFooter = ({
canExplore,
closeModal,
exploreChart,
showEditButton,
onExploreClick,
isGeneratingUrl,
}: ModalFooterProps) => {
const theme = useTheme();

return (
<>
{!isEmbedded() && (
{!isEmbedded() && showEditButton && (
<Button
buttonStyle="secondary"
buttonSize="small"
onClick={exploreChart}
disabled={!canExplore}
onClick={canExplore ? onExploreClick : undefined}
disabled={!canExplore || isGeneratingUrl}
loading={isGeneratingUrl}
tooltip={
!canExplore
? t('You do not have sufficient permissions to edit the chart')
? t('You do not have sufficient permissions to explore the chart')
: undefined
}
>
{t('Edit chart')}
{t('Explore')}
</Button>
)}
<Button
Expand Down Expand Up @@ -99,8 +107,10 @@
dataset,
}: DrillDetailModalProps) {
const theme = useTheme();
const history = useHistory();
const dashboardPageId = useContext(DashboardPageIdContext);
const { addDangerToast } = useToasts();
const [isGeneratingUrl, setIsGeneratingUrl] = useState(false);

const { slice_name: chartName } = useSelector(
(state: { sliceEntities: { slices: Record<number, Slice> } }) =>
state.sliceEntities?.slices?.[chartId] || {},
Expand All @@ -109,14 +119,65 @@
findPermission('can_explore', 'Superset', state.user?.roles),
);

const exploreUrl = useMemo(
() => `/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${chartId}`,
[chartId, dashboardPageId],
const showEditButton = Boolean(dataset?.drill_through_chart_id);
const dashboardContextFormData = useDashboardFormData(
dataset?.drill_through_chart_id,
);

const exploreChart = useCallback(() => {
history.push(exploreUrl);
}, [exploreUrl, history]);
const drillThroughFormData = useMemo(() => {
if (!dataset?.drill_through_chart_id || !dataset?.id) {
return null;
}

const drillThroughBaseFormData = {
slice_id: dataset.drill_through_chart_id,
datasource: `${dataset.id}__table`,
viz_type: 'table',
};

return getFormDataWithDashboardContext(
drillThroughBaseFormData,
dashboardContextFormData,
undefined,
initialFilters,
);
}, [
dataset?.drill_through_chart_id,
dataset?.id,
dashboardContextFormData,
initialFilters,
]);
const handleExploreClick = async (event: React.MouseEvent) => {
event.preventDefault();

if (
!dataset?.drill_through_chart_id ||
!drillThroughFormData ||
!dataset?.id
) {
return;
}

setIsGeneratingUrl(true);

try {
const url = await generateExploreUrl(
dataset.id,
'table',
drillThroughFormData,
{
chartId: dataset.drill_through_chart_id,
dashboardPageId,
},
);

window.location.href = url;

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
Loading
is reinterpreted as HTML without escaping meta-characters.

Copilot Autofix

AI 6 months ago

The root cause is the processing of the data-bootstrap DOM attribute, which is parsed as JSON and used to set values like application_root. These values then propagate through helpers, are used to construct URLs, and eventually drive navigation. To fix this, we must ensure that any data loaded from the DOM (specifically, anything parsed from data-bootstrap) is sanitized:

  • For path segments like application_root, ensure they only contain valid path characters and do not contain dangerous meta-characters (e.g., <, >, ", ', ;, and fragments like javascript:, etc).
  • The best and lowest-impact fix is to validate and sanitize application_root (and other similar fields) immediately after parsing data-bootstrap within getBootstrapData(), before any parts of the code base use them for constructing URLs.
  • If application_root is missing or invalid, fall back to the default value.
  • Implement a helper called sanitizeAppRoot, which (i) rejects dangerous characters, (ii) ensures it starts with /, (iii) strips fragments, and (iv) returns a safe string.
  • Use sanitizeAppRoot inside getBootstrapData to guarantee that all uses downstream (e.g., in ensureAppRoot and all calling code) are safe.

This approach avoids altering callers, only changes the population of bootstrap data, requires a single helper implementation, and does not affect functionality.

Suggested changeset 1
superset-frontend/src/utils/getBootstrapData.ts
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/superset-frontend/src/utils/getBootstrapData.ts b/superset-frontend/src/utils/getBootstrapData.ts
--- a/superset-frontend/src/utils/getBootstrapData.ts
+++ b/superset-frontend/src/utils/getBootstrapData.ts
@@ -21,13 +21,49 @@
 
 let cachedBootstrapData: BootstrapData | null = null;
 
+// Helper to sanitize application root paths
+function sanitizeAppRoot(path: string): string {
+  if (typeof path !== 'string') return DEFAULT_BOOTSTRAP_DATA.common.application_root;
+  // Remove dangerous characters, allow only typical path characters
+  // Remove anything after a '?' or '#' (no query/fragments)
+  let safePath = path.split(/[?#]/)[0];
+  // Disallow dangerous meta characters
+  if (/[^a-zA-Z0-9\/._-]/.test(safePath)) {
+    return DEFAULT_BOOTSTRAP_DATA.common.application_root;
+  }
+  // Ensure starts with /
+  if (!safePath.startsWith('/')) {
+    safePath = '/' + safePath;
+  }
+  // Remove trailing slashes except root
+  if (safePath.length > 1) {
+    safePath = safePath.replace(/\/+$/, '');
+  }
+  return safePath;
+}
+
 export default function getBootstrapData(): BootstrapData {
   if (cachedBootstrapData === null) {
     const appContainer = document.getElementById('app');
     const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
-    cachedBootstrapData = dataBootstrap
-      ? JSON.parse(dataBootstrap)
-      : DEFAULT_BOOTSTRAP_DATA;
+    if (dataBootstrap) {
+      try {
+        const parsed = JSON.parse(dataBootstrap);
+        // Sanitize the application_root immediately after parsing bootstrap data
+        if (
+          parsed &&
+          parsed.common &&
+          typeof parsed.common.application_root === 'string'
+        ) {
+          parsed.common.application_root = sanitizeAppRoot(parsed.common.application_root);
+        }
+        cachedBootstrapData = parsed;
+      } catch (e) {
+        cachedBootstrapData = DEFAULT_BOOTSTRAP_DATA;
+      }
+    } else {
+      cachedBootstrapData = DEFAULT_BOOTSTRAP_DATA;
+    }
   }
   // Add a fallback to ensure the returned value is always of type BootstrapData
   return cachedBootstrapData ?? DEFAULT_BOOTSTRAP_DATA;
EOF
@@ -21,13 +21,49 @@

let cachedBootstrapData: BootstrapData | null = null;

// Helper to sanitize application root paths
function sanitizeAppRoot(path: string): string {
if (typeof path !== 'string') return DEFAULT_BOOTSTRAP_DATA.common.application_root;
// Remove dangerous characters, allow only typical path characters
// Remove anything after a '?' or '#' (no query/fragments)
let safePath = path.split(/[?#]/)[0];
// Disallow dangerous meta characters
if (/[^a-zA-Z0-9\/._-]/.test(safePath)) {
return DEFAULT_BOOTSTRAP_DATA.common.application_root;
}
// Ensure starts with /
if (!safePath.startsWith('/')) {
safePath = '/' + safePath;
}
// Remove trailing slashes except root
if (safePath.length > 1) {
safePath = safePath.replace(/\/+$/, '');
}
return safePath;
}

export default function getBootstrapData(): BootstrapData {
if (cachedBootstrapData === null) {
const appContainer = document.getElementById('app');
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
cachedBootstrapData = dataBootstrap
? JSON.parse(dataBootstrap)
: DEFAULT_BOOTSTRAP_DATA;
if (dataBootstrap) {
try {
const parsed = JSON.parse(dataBootstrap);
// Sanitize the application_root immediately after parsing bootstrap data
if (
parsed &&
parsed.common &&
typeof parsed.common.application_root === 'string'
) {
parsed.common.application_root = sanitizeAppRoot(parsed.common.application_root);
}
cachedBootstrapData = parsed;
} catch (e) {
cachedBootstrapData = DEFAULT_BOOTSTRAP_DATA;
}
} else {
cachedBootstrapData = DEFAULT_BOOTSTRAP_DATA;
}
}
// Add a fallback to ensure the returned value is always of type BootstrapData
return cachedBootstrapData ?? DEFAULT_BOOTSTRAP_DATA;
Copilot is powered by AI and may make mistakes. Always verify output.
} catch (error) {
console.error('Failed to generate chart explore URL:', error);
addDangerToast(t('Failed to generate chart explore URL'));
setIsGeneratingUrl(false);
}
};

return (
<Modal
Expand All @@ -131,7 +192,12 @@
name={t('Drill to detail: %s', chartName)}
title={t('Drill to detail: %s', chartName)}
footer={
<ModalFooter exploreChart={exploreChart} canExplore={canExplore} />
<ModalFooter
canExplore={canExplore}
showEditButton={showEditButton}
onExploreClick={handleExploreClick}
isGeneratingUrl={isGeneratingUrl}
/>
}
responsive
resizable
Expand All @@ -151,6 +217,7 @@
formData={formData}
initialFilters={initialFilters}
dataset={dataset}
drillThroughFormData={drillThroughFormData}
/>
</Modal>
);
Expand Down
Loading
Loading