diff --git a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts index b5ceb932c21..866448960a4 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts @@ -275,8 +275,23 @@ export default class SupersetClientClass { const host = inputHost ?? this.host; const cleanHost = host.slice(-1) === '/' ? host.slice(0, -1) : host; // no backslash + // Strip a leading appRoot segment so callers that accidentally pre-prefix + // their endpoint (e.g. by wrapping with ensureAppRoot before passing to the + // client) do not produce a doubled `/superset/superset/...` URL. The L2 + // static invariant still flags this pattern as a migration issue; this is + // the runtime safety net. + let cleanEndpoint = endpoint; + const root = this.appRoot; + if (root) { + if (cleanEndpoint === root) { + cleanEndpoint = ''; + } else if (cleanEndpoint.startsWith(`${root}/`)) { + cleanEndpoint = cleanEndpoint.slice(root.length); + } + } + return `${this.protocol}//${cleanHost}${this.appRoot}/${ - endpoint[0] === '/' ? endpoint.slice(1) : endpoint + cleanEndpoint[0] === '/' ? cleanEndpoint.slice(1) : cleanEndpoint }`; } } diff --git a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts index f3f425534b3..b9340d92a8a 100644 --- a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts @@ -42,12 +42,25 @@ describe('SupersetClient applies the application root exactly once', () => { ); }); - // Documents the double-prefix symptom: wrapping the endpoint in - // ensureAppRoot before passing it to SupersetClient duplicates the root. - // navigationUtils.invariants.test.ts catches this pattern statically. - test('does not double-apply the application root when caller pre-prefixes', () => { + // Runtime safety net: if a caller pre-prefixes the endpoint (e.g. by wrapping + // with ensureAppRoot before calling), getUrl strips the duplicate. The L2 + // static invariant still flags the pattern at the call site — this guards + // against the bug reaching production if the static check is bypassed. + test('dedupes a leading application-root segment from a pre-prefixed endpoint', () => { expect(buildClient().getUrl({ endpoint: '/superset/api/v1/chart' })).toBe( - 'https://config_host/superset/superset/api/v1/chart', + 'https://config_host/superset/api/v1/chart', + ); + }); + + test('dedupe is segment-boundary aware — `/supersetfoo` is not a prefix match', () => { + expect(buildClient().getUrl({ endpoint: '/supersetfoo/x' })).toBe( + 'https://config_host/superset/supersetfoo/x', + ); + }); + + test('dedupes the bare application root to an empty endpoint', () => { + expect(buildClient().getUrl({ endpoint: '/superset' })).toBe( + 'https://config_host/superset/', ); }); diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index 726aab0a0ef..aef1da991e7 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -48,7 +48,7 @@ import * as useNativeFiltersModule from './state'; fetchMock.get('glob:*/csstemplateasyncmodelview/api/read', {}); fetchMock.put('glob:*/api/v1/dashboard/*', {}); // Add mock for logging endpoint -fetchMock.post('glob:*/superset/log/?*', {}); +fetchMock.post('glob:*/log/?*', {}); jest.mock('src/dashboard/actions/dashboardState', () => ({ ...jest.requireActual('src/dashboard/actions/dashboardState'), diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx index 563cedf4198..5a7e8b2957a 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx @@ -29,7 +29,7 @@ import * as chartCustomizationActions from '../../actions/chartCustomizationActi fetchMock.get('glob:*/csstemplateasyncmodelview/api/read', {}); fetchMock.put('glob:*/api/v1/dashboard/*/colors*', {}); -fetchMock.post('glob:*/superset/log/?*', {}); +fetchMock.post('glob:*/log/?*', {}); jest.mock('@visx/responsive', () => ({ ParentSize: ({ diff --git a/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx b/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx index ea0b0815076..98e400e8a99 100644 --- a/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx @@ -26,6 +26,7 @@ import Table, { TableSize, } from '@superset-ui/core/components/Table'; import { DatasetObject } from 'src/features/datasets/AddDataset/types'; +import { openInNewTab } from 'src/utils/navigationUtils'; import { ITableColumn } from './types'; import MessageContent from './MessageContent'; @@ -227,11 +228,9 @@ const renderExistingDatasetAlert = (dataset?: DatasetObject) => ( { - window.open( - dataset?.explore_url, - '_blank', - 'noreferrer noopener popup=false', - ); + if (dataset?.explore_url) { + openInNewTab(dataset.explore_url); + } }} tabIndex={0} className="view-dataset-button" diff --git a/superset-frontend/src/middleware/logger.test.ts b/superset-frontend/src/middleware/logger.test.ts index 5b989e21713..b05469eaab6 100644 --- a/superset-frontend/src/middleware/logger.test.ts +++ b/superset-frontend/src/middleware/logger.test.ts @@ -88,13 +88,13 @@ describe('logger middleware', () => { expect(next.mock.calls.length).toBe(1); }); - test('should POST an event to /superset/log/ when called', () => { + test('should POST an event to /log/ when called', () => { (logger as Function)(mockStore)(next)(action); expect(next.mock.calls.length).toBe(0); jest.advanceTimersByTime(2000); expect(postStub.mock.calls.length).toBe(1); - expect(postStub.mock.calls[0][0].endpoint).toMatch('/superset/log/'); + expect(postStub.mock.calls[0][0].endpoint).toMatch('/log/'); }); test('should include ts, start_offset, event_name, impression_id, source, and source_id in every event', () => { @@ -165,7 +165,7 @@ describe('logger middleware', () => { expect(beaconMock.mock.calls.length).toBe(1); const endpoint = beaconMock.mock.calls[0][0]; - expect(endpoint).toMatch('/superset/log/'); + expect(endpoint).toMatch('/log/'); }); test('should pass a guest token to sendBeacon if present', () => { diff --git a/superset-frontend/src/middleware/loggerMiddleware.ts b/superset-frontend/src/middleware/loggerMiddleware.ts index 9a42e4ade38..b16c75503df 100644 --- a/superset-frontend/src/middleware/loggerMiddleware.ts +++ b/superset-frontend/src/middleware/loggerMiddleware.ts @@ -33,7 +33,12 @@ import { ensureAppRoot } from '../utils/navigationUtils'; import type { DashboardInfo, DashboardLayoutState } from '../dashboard/types'; import type { QueryEditor } from '../SqlLab/types'; -type LogEventSource = 'dashboard' | 'embedded_dashboard' | 'explore' | 'sqlLab' | 'slice'; +type LogEventSource = + | 'dashboard' + | 'embedded_dashboard' + | 'explore' + | 'sqlLab' + | 'slice'; interface LogEventData { source?: LogEventSource; @@ -88,7 +93,7 @@ interface LoggerStore { dispatch: Dispatch; } -const LOG_ENDPOINT = '/superset/log/?explode=events'; +const LOG_ENDPOINT = '/log/?explode=events'; const sendBeacon = (events: LogEventData[]): void => { if (events.length <= 0) { diff --git a/superset-frontend/src/pages/RedirectWarning/index.tsx b/superset-frontend/src/pages/RedirectWarning/index.tsx index bed0b96f564..e87b8a7304d 100644 --- a/superset-frontend/src/pages/RedirectWarning/index.tsx +++ b/superset-frontend/src/pages/RedirectWarning/index.tsx @@ -100,7 +100,7 @@ export default function RedirectWarning() { // Redirect immediately if the URL is already trusted useEffect(() => { if (targetUrl && isAllowedScheme(targetUrl) && isUrlTrusted(targetUrl)) { - window.location.href = targetUrl; + redirect(targetUrl); } }, [targetUrl]); @@ -109,7 +109,7 @@ export default function RedirectWarning() { if (trustChecked) { trustUrl(targetUrl); } - window.location.href = targetUrl; + redirect(targetUrl); }, [trustChecked, targetUrl]); const handleReturn = useCallback(() => { diff --git a/superset-frontend/src/pages/RedirectWarning/utils.ts b/superset-frontend/src/pages/RedirectWarning/utils.ts index 43aa834789a..d888045deec 100644 --- a/superset-frontend/src/pages/RedirectWarning/utils.ts +++ b/superset-frontend/src/pages/RedirectWarning/utils.ts @@ -36,8 +36,14 @@ function normalizeUrl(url: string): string { /** * Return true if the URL scheme is safe for navigation. * Blocks javascript:, data:, vbscript:, file:, etc. + * + * Protocol-relative URLs (`//host/...`) are rejected because they perform a + * cross-origin navigation despite parsing as "relative" — the standalone + * `new URL(...)` call throws on them, so without an explicit guard the catch + * branch would let them through. */ export function isAllowedScheme(url: string): boolean { + if (url.startsWith('//')) return false; try { const parsed = new URL(url); return ALLOWED_SCHEMES.includes(parsed.protocol); diff --git a/superset-frontend/src/utils/navigationUtils.test.ts b/superset-frontend/src/utils/navigationUtils.test.ts index aae326dded0..11f1e1b6f90 100644 --- a/superset-frontend/src/utils/navigationUtils.test.ts +++ b/superset-frontend/src/utils/navigationUtils.test.ts @@ -102,6 +102,16 @@ describe('openInNewTab', () => { expect(features).toContain('noreferrer'); }); }); + + test('refuses protocol-relative URLs to block open-redirect via `//evil.com`', async () => { + await withApplicationRoot('/superset/', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + expect(() => openInNewTab('//evil.example.com/phish')).toThrow( + /refused unsafe URL/, + ); + expect(openSpy).not.toHaveBeenCalled(); + }); + }); }); describe('redirect', () => { diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts index 76faeed118e..68e8f8e5de0 100644 --- a/superset-frontend/src/utils/navigationUtils.ts +++ b/superset-frontend/src/utils/navigationUtils.ts @@ -60,12 +60,12 @@ export function navigateWithState( const NEW_TAB_FEATURES = 'noopener noreferrer'; -// Allow-list of safe URL shapes for navigation: relative paths, protocol- -// relative URLs, and a small set of known-safe schemes. `ensureAppRoot` -// already neutralises `javascript:` / `data:` by prefixing them as relative -// paths, but checking here gives CodeQL a locally-visible sanitiser on the -// sinks below. -const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|\/\/|https?:|ftp:|mailto:|tel:)/i; +// Allow-list of safe URL shapes for navigation: router-relative paths and a +// small set of known-safe schemes. `ensureAppRoot` already neutralises +// `javascript:` / `data:` by prefixing them as relative paths; protocol- +// relative `//host` is intentionally excluded here because it is a cross- +// origin navigation primitive that previously enabled open redirects. +const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i; function assertSafeNavigationUrl(url: string): string { if (!SAFE_NAVIGATION_URL_RE.test(url)) { diff --git a/superset-frontend/src/utils/pathUtils.test.ts b/superset-frontend/src/utils/pathUtils.test.ts index fbcbf6311dc..522d4980c6a 100644 --- a/superset-frontend/src/utils/pathUtils.test.ts +++ b/superset-frontend/src/utils/pathUtils.test.ts @@ -294,6 +294,17 @@ test('stripAppRoot handles nested application roots', async () => { expect(stripAppRoot('/welcome/')).toBe('/welcome/'); }); +test('stripAppRoot greedily removes a doubled application root', async () => { + const { stripAppRoot } = await loadPathUtils('/superset/'); + + // Upstream double-prefix bug: ensure both copies are stripped so a downstream + // react-router basename re-prepend produces a single, correct prefix. + expect(stripAppRoot('/superset/superset/welcome/')).toBe('/welcome/'); + expect(stripAppRoot('/superset/superset/superset/welcome/')).toBe( + '/welcome/', + ); +}); + test('stripAppRoot passes absolute and protocol-relative URLs through', async () => { const { stripAppRoot } = await loadPathUtils('/superset/'); diff --git a/superset-frontend/src/utils/pathUtils.ts b/superset-frontend/src/utils/pathUtils.ts index 322dc18ed04..fc5fb23e95a 100644 --- a/superset-frontend/src/utils/pathUtils.ts +++ b/superset-frontend/src/utils/pathUtils.ts @@ -89,9 +89,12 @@ export function stripAppRoot(path: string): string { } const root = applicationRoot(); if (!root) return path; - if (path === root) return '/'; - if (path.startsWith(`${root}/`)) { - return path.slice(root.length); + let stripped = path; + // Greedy strip handles upstream double-prefix bugs (e.g. a backend payload + // that emitted `/superset/superset/x`). Without this, a single pass would + // leave one stale prefix, which react-router's `basename` would then re-add. + while (stripped === root || stripped.startsWith(`${root}/`)) { + stripped = stripped === root ? '/' : stripped.slice(root.length); } - return path; + return stripped; } diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 4f7268458e8..1d2f6b6ce89 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -299,10 +299,15 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods if app_root.endswith("/"): app_root = app_root.rstrip("/") + # FAB renders this href verbatim in menus and does not apply + # SCRIPT_NAME at render time, so the application_root has to be baked + # in at registration. The previous hardcoded `/superset/welcome/` + # collided with non-`/superset/` deployments AND with the new + # `Superset.route_base = ""`. `app_root` is normalized above. appbuilder.add_link( "Home", label=_("Home"), - href="/superset/welcome/", + href=f"{app_root}/welcome/", cond=lambda: bool(current_app.config["LOGO_TARGET_PATH"]), ) diff --git a/superset/models/core.py b/superset/models/core.py index 612ad6aaf37..305b1337917 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -1166,7 +1166,11 @@ class Database(CoreDatabase, AuditMixinNullable, ImportExportMixin): # pylint: @property def sql_url(self) -> str: - return f"/superset/sql/{self.id}/" + # SQL Lab moved to its own blueprint at /sqllab/; the legacy + # /superset/sql// route was removed when Superset.route_base + # collapsed to "". Deep-link by databaseId instead so this property + # resolves to a live route under any application_root. + return f"/sqllab/?dbid={self.id}" @hybrid_property def perm(self) -> str: diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 248bbdd9875..ccf43a40c07 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -22,7 +22,7 @@ from collections import defaultdict, deque from typing import Any, Callable import sqlalchemy as sqla -from flask import current_app as app +from flask import current_app as app, url_for from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders from flask_appbuilder.security.sqla.models import User @@ -221,7 +221,12 @@ class Dashboard(CoreDashboard, AuditMixinNullable, ImportExportMixin): @renders("dashboard_title") def dashboard_link(self) -> Markup: title = escape(self.dashboard_title or "") - return Markup(f'{title}') + # FAB list view renders this raw HTML; use url_for so Flask prepends + # SCRIPT_NAME (the application_root) and the row link works under + # subdirectory deployments. `Dashboard.url` itself stays router- + # relative so frontend callers can apply ensureAppRoot exactly once. + href = url_for("Superset.dashboard", dashboard_id_or_slug=self.slug or self.id) + return Markup(f'{title}') @property def digest(self) -> str | None: diff --git a/superset/models/slice.py b/superset/models/slice.py index 04c698ce95d..a4bfe77a9e5 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -21,6 +21,7 @@ from typing import Any, TYPE_CHECKING from urllib import parse import sqlalchemy as sqla +from flask import url_for from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders from markupsafe import escape, Markup @@ -309,7 +310,7 @@ class Slice( # pylint: disable=too-many-public-methods @property def explore_json_url(self) -> str: """Defines the url to access the slice""" - return self.get_explore_url("/superset/explore_json") + return self.get_explore_url("/explore_json") @property def edit_url(self) -> str: @@ -322,7 +323,11 @@ class Slice( # pylint: disable=too-many-public-methods @property def slice_link(self) -> Markup: name = escape(self.chart) - return Markup(f'{name}') + # FAB list view renders this raw HTML; use url_for so Flask prepends + # SCRIPT_NAME (the application_root). `Slice.url` itself stays router- + # relative so frontend callers can apply ensureAppRoot exactly once. + href = url_for("ExploreView.root", slice_id=self.id) + return Markup(f'{name}') @property def icons(self) -> str: diff --git a/superset/tasks/async_queries.py b/superset/tasks/async_queries.py index c50026c509e..f9aa9efc2fe 100644 --- a/superset/tasks/async_queries.py +++ b/superset/tasks/async_queries.py @@ -169,7 +169,7 @@ def load_explore_json_into_cache( # pylint: disable=too-many-locals set_and_log_cache( cache_instance, cache_key, cache_value, cache_timeout=cache_timeout ) - result_url = f"/superset/explore_json/data/{cache_key}" + result_url = f"/explore_json/data/{cache_key}" async_query_manager.update_job( job_metadata, async_query_manager.STATUS_DONE, diff --git a/tests/unit_tests/test_subdirectory_url_for.py b/tests/unit_tests/test_subdirectory_url_for.py index 8b390cf9038..b88d6895c2b 100644 --- a/tests/unit_tests/test_subdirectory_url_for.py +++ b/tests/unit_tests/test_subdirectory_url_for.py @@ -166,3 +166,67 @@ def test_dashboard_model_url_has_no_route_prefix() -> None: assert Dashboard.get_url(11) == "/dashboard/11/" assert Dashboard.get_url(11, "births") == "/dashboard/births/" + + +def test_slice_explore_json_url_has_no_route_prefix(app_context: None) -> None: + """`Slice.explore_json_url` previously baked `/superset/explore_json` into + the path. After `Superset.route_base = ""` the rule is `/explore_json/`, + so the literal pointed at a non-existent route. Pin the prefix-free shape + so downstream `ensureAppRoot` callers prepend the application root once. + """ + from superset.models.slice import Slice + + slc = Slice(id=42) + assert slc.explore_json_url.startswith("/explore_json/") + assert "/superset/explore_json" not in slc.explore_json_url + + +def test_database_sql_url_resolves_to_live_sqllab_route() -> None: + """`Database.sql_url` previously returned `/superset/sql//` — a route + that was removed when SQL Lab moved to its own blueprint at `/sqllab/`. + The property now deep-links to SQL Lab via the `dbid` query parameter so + it resolves under any application_root. + """ + from superset.models.core import Database + + db = Database(database_name="db", id=7) + assert db.sql_url == "/sqllab/?dbid=7" + + +def test_dashboard_link_emits_script_name_under_subdirectory( + app_context: None, +) -> None: + """`dashboard_link` renders raw HTML for the FAB list-view "title" column. + Flask does not auto-apply SCRIPT_NAME to string fragments, so a literal + `/dashboard//` href routed outside the application_root under + subdirectory deployment. Pin that the rendered href now carries the + SCRIPT_NAME prefix exactly once. + """ + from flask import current_app + + from superset.models.dashboard import Dashboard + + dash = Dashboard(id=5, slug=None, dashboard_title="t") + with current_app.test_request_context( + "/", environ_overrides={"SCRIPT_NAME": "/superset"} + ): + html = str(dash.dashboard_link()) + assert 'href="/superset/dashboard/5/"' in html + assert "/superset/superset/" not in html + + +def test_slice_link_emits_script_name_under_subdirectory( + app_context: None, +) -> None: + """Mirror of `dashboard_link` for `Slice.slice_link`.""" + from flask import current_app + + from superset.models.slice import Slice + + slc = Slice(id=9, slice_name="chart") + with current_app.test_request_context( + "/", environ_overrides={"SCRIPT_NAME": "/superset"} + ): + html = str(slc.slice_link) + assert 'href="/superset/explore/?slice_id=9"' in html + assert "/superset/superset/" not in html