diff --git a/UPDATING.md b/UPDATING.md index ab5994402bc..8432631c244 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,24 @@ assists people when migrating to a new version. ## Next +- [39925](https://github.com/apache/superset/pull/39925): URL prefixing for `SUPERSET_APP_ROOT` subdirectory deployments is now handled automatically by helpers in `src/utils/navigationUtils` (`openInNewTab`, `redirect`, `getShareableUrl`, ``). Direct imports of `ensureAppRoot` / `makeUrl` from `src/utils/pathUtils` are forbidden outside `navigationUtils.ts` (enforced by a static-invariant test); contributors writing new code should use the focused helpers instead. No runtime behaviour change for existing callers — all 19 prior call sites have been migrated and four pre-existing double-prefix and missing-prefix bugs are fixed as part of the migration. + +- [39925](https://github.com/apache/superset/pull/39925): `SupersetClient.getUrl()` now strips a single leading application-root segment from the supplied `endpoint` before building the request URL, so a caller that accidentally pre-prefixes its endpoint (for example by wrapping it with `ensureAppRoot` before passing it to the client) no longer produces a doubled `/superset/superset/...` URL under subdirectory deployment. The strip is **single-pass** — a genuine `/superset/superset/` route is preserved, not collapsed — and **silent** (no console warning); the static-invariant test remains the primary signal for pre-prefixing at the call site, and this runtime strip is a safety net beneath it. Code that intentionally targeted a literal `///...` endpoint through `getUrl` (a configuration that has no legitimate use under the prefixing model) would have its first redundant segment removed. + +- **Breaking — `Superset` view class route prefix removed.** The `Superset` view in `superset/views/core.py` now declares `route_base = ""`, overriding Flask-AppBuilder's auto-derived `/superset` prefix. Routes that previously lived at `/superset/welcome/`, `/superset/dashboard//`, `/superset/dashboard/p//`, `/superset/explore/`, etc. now respond at `/welcome/`, `/dashboard//`, `/dashboard/p//`, `/explore/`, etc. Under subdirectory deployment (`SUPERSET_APP_ROOT=/superset`) the URLs are unchanged from end-user perspective — `AppRootMiddleware` re-applies the prefix via `SCRIPT_NAME`. Under root deployments, any external integration or bookmark that hard-codes `/superset//` paths must be updated to drop the prefix. This fixes the doubled `/superset/superset/...` URLs that `url_for` emitted for these endpoints under subdirectory deployment and the related 404s on the routes themselves. + +- **Breaking — Three sibling view classes route prefix removed.** Following the same rationale as the `Superset` class above, `ExplorePermalinkView` (`superset/views/explore.py`), `TagModelView`, and `TaggedObjectsModelView` (`superset/views/tags.py`, `superset/views/all_entities.py`) now mount at the application root rather than a hard-coded `/superset/...`. The user-visible URLs `/superset/explore/p//`, `/superset/tags/`, and `/superset/all_entities/` are unchanged under subdirectory deployment; under root deployments these views now serve `/explore/p//`, `/tags/`, and `/all_entities/`, so any external integration or bookmark must drop the `/superset/` prefix. `Dashboard.url` and `Dashboard.get_url` likewise return `/dashboard//` instead of the prior `/superset/dashboard//` literal so downstream consumers (DashboardList row hrefs, MCP service `dashboard_url`) emit a single, deployment-correct prefix. + +- **Legacy `/superset/*` path support.** A new outermost WSGI middleware `LegacyPrefixRedirectMiddleware` (`superset/middleware/legacy_prefix_redirect.py`) 308-redirects every enumerated legacy `/superset/` path to its post-`route_base=""` canonical location (e.g. `/superset/welcome/` → `/welcome/` under root; → `/superset/welcome/` under `SUPERSET_APP_ROOT=/superset`, because the canonical resolves through `AppRootMiddleware`). Bookmarks, email links, and external integrations survive the route-base collapse for one release cycle. POST against a GET-only canonical returns 410 Gone instead of 308 (308 would 405 on retry). The shim is removed at EOL `5.0.0`, matching the `@deprecated(eol_version="5.0.0")` gate on `Superset.explore` and `Superset.explore_json`. + +- **PWA web app manifest served dynamically.** The PWA manifest is now served at `/pwa-manifest.json` (under `APPLICATION_ROOT`) by a new `PwaManifestView` (`superset/views/pwa_manifest.py`) instead of the static file at `/static/assets/pwa-manifest.json`. The legacy static source at `superset-frontend/src/pwa-manifest.json` has been removed (along with its `webpack.config.js` `CopyPlugin` rule). The new endpoint resolves `APPLICATION_ROOT` and `STATIC_ASSETS_PREFIX` at request time so PWA install works under subdirectory deployments and split static-prefix / app-root deployments (where `STATIC_ASSETS_PREFIX` points to a CDN host while the Superset backend stays under `APPLICATION_ROOT`). The `` href in `superset/templates/superset/spa.html` was updated correspondingly (using a new `application_root_rstrip` template global). Operators with a forked `spa.html` should switch any manifest `` to `{{ application_root_rstrip }}/pwa-manifest.json`. + +- **Hard re-bookmark break — `/superset/sql//`.** SQL Lab moved to its own blueprint at `/sqllab/`. The legacy `/superset/sql//` shape changed to a query-string form (`/sqllab/?dbid=`); no 1:1 path mapping exists, so `LegacyPrefixRedirectMiddleware` does **not** redirect this route — it passes through and surfaces a 404. Users with bookmarks to `/superset/sql//` must update them to `/sqllab/?dbid=`. + +- **`SqlaTable.sql_url` query-string format.** `SqlaTable.sql_url` now URL-encodes `table_name` and joins it as a query parameter rather than concatenating a second `?`. Previously, with `Database.sql_url` returning `/sqllab/?dbid=`, the concatenation produced `/sqllab/?dbid=?table_name=` — a malformed second `?` that broke the query parser. External code that parsed the legacy `?table_name=` shape now sees properly percent-encoded values (e.g. `/` → `%2F`, ` ` → `+` or `%20`); decode with `urllib.parse.parse_qsl`. + +- **New config flag `EMBEDDED_DISABLE_PERMALINK_ORIGIN_REWRITE` (default `False`).** Share/permalink URLs now substitute `window.location.origin` for the backend-supplied origin so a proxied or subdirectory-deployed Superset never hands the user an unreachable internal hostname. Operators whose reverse proxy correctly forwards `X-Forwarded-Host` *and* who want permalinks to carry the backend's literal origin can opt out by setting `EMBEDDED_DISABLE_PERMALINK_ORIGIN_REWRITE = True` in `superset_config.py`. Default `False` (rewrite is on); flipping the default would regress the dominant proxied/subdir deployment to an unreachable host. + ### SQL Lab denies large-object and information_schema access by default `DISALLOWED_SQL_FUNCTIONS` and `DISALLOWED_SQL_TABLES` now ship with additional default entries, so SQL Lab and chart-data queries that reference them are rejected where they were previously allowed: diff --git a/superset-frontend/cypress-base/cypress/utils/urls.ts b/superset-frontend/cypress-base/cypress/utils/urls.ts index 66110bcc64b..ce8eecaa992 100644 --- a/superset-frontend/cypress-base/cypress/utils/urls.ts +++ b/superset-frontend/cypress-base/cypress/utils/urls.ts @@ -19,9 +19,8 @@ export const DASHBOARD_LIST = '/dashboard/list/'; export const CHART_LIST = '/chart/list/'; -export const WORLD_HEALTH_DASHBOARD = '/superset/dashboard/world_health/'; -export const SAMPLE_DASHBOARD_1 = '/superset/dashboard/1-sample-dashboard/'; -export const SUPPORTED_CHARTS_DASHBOARD = - '/superset/dashboard/supported_charts_dash/'; -export const TABBED_DASHBOARD = '/superset/dashboard/tabbed_dash/'; +export const WORLD_HEALTH_DASHBOARD = '/dashboard/world_health/'; +export const SAMPLE_DASHBOARD_1 = '/dashboard/1-sample-dashboard/'; +export const SUPPORTED_CHARTS_DASHBOARD = '/dashboard/supported_charts_dash/'; +export const TABBED_DASHBOARD = '/dashboard/tabbed_dash/'; export const DATABASE_LIST = '/databaseview/list'; diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index 488c9c65c6e..72a867ecf65 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -8439,9 +8439,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -8459,9 +8456,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -8479,9 +8473,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -8499,9 +8490,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -8519,9 +8507,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -8539,9 +8524,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -8559,9 +8541,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -8579,9 +8558,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -10651,9 +10627,6 @@ "cpu": [ "arm64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0 AND MIT", "optional": true, "os": [ @@ -10670,9 +10643,6 @@ "cpu": [ "arm64" ], - "libc": [ - "musl" - ], "license": "Apache-2.0 AND MIT", "optional": true, "os": [ @@ -10689,9 +10659,6 @@ "cpu": [ "ppc64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0 AND MIT", "optional": true, "os": [ @@ -10708,9 +10675,6 @@ "cpu": [ "s390x" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0 AND MIT", "optional": true, "os": [ @@ -10727,9 +10691,6 @@ "cpu": [ "x64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0 AND MIT", "optional": true, "os": [ @@ -10746,9 +10707,6 @@ "cpu": [ "x64" ], - "libc": [ - "musl" - ], "license": "Apache-2.0 AND MIT", "optional": true, "os": [ @@ -26487,6 +26445,21 @@ } } }, + "node_modules/jsdom/node_modules/@noble/hashes": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-2.2.0.tgz", + "integrity": "sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==", + "dev": true, + "license": "MIT", + "optional": true, + "peer": true, + "engines": { + "node": ">= 20.19.0" + }, + "funding": { + "url": "https://paulmillr.com/funding/" + } + }, "node_modules/jsdom/node_modules/css-tree": { "version": "3.2.1", "resolved": "https://registry.npmjs.org/css-tree/-/css-tree-3.2.1.tgz", @@ -43106,6 +43079,21 @@ } } }, + "node_modules/whatwg-url/node_modules/@noble/hashes": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-2.2.0.tgz", + "integrity": "sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==", + "dev": true, + "license": "MIT", + "optional": true, + "peer": true, + "engines": { + "node": ">= 20.19.0" + }, + "funding": { + "url": "https://paulmillr.com/funding/" + } + }, "node_modules/whatwg-url/node_modules/webidl-conversions": { "version": "8.0.1", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-8.0.1.tgz", diff --git a/superset-frontend/packages/superset-ui-core/src/chart/clients/ChartClient.ts b/superset-frontend/packages/superset-ui-core/src/chart/clients/ChartClient.ts index 033c5dd9195..397a854a2a5 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/clients/ChartClient.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/clients/ChartClient.ts @@ -109,7 +109,7 @@ export default class ChartClient { (await buildQueryRegistry.get(visType)) ?? (() => formData); const requestConfig: RequestConfig = useLegacyApi ? { - endpoint: '/superset/explore_json/', + endpoint: '/explore_json/', postPayload: { form_data: buildQuery(formData), }, @@ -139,7 +139,7 @@ export default class ChartClient { ): Promise { return this.client .get({ - endpoint: `/superset/fetch_datasource_metadata?datasourceKey=${datasourceKey}`, + endpoint: `/fetch_datasource_metadata?datasourceKey=${datasourceKey}`, ...options, } as RequestConfig) .then(response => response.json as Datasource); diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx index 774bad741c4..baae1ae2ce4 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx @@ -263,9 +263,7 @@ export default function StatefulChart(props: StatefulChartProps) { if (!useLegacyApi && !queryContext.queries) { queryContext = { queries: [queryContext] }; } - const endpoint = useLegacyApi - ? '/superset/explore_json/' - : '/api/v1/chart/data'; + const endpoint = useLegacyApi ? '/explore_json/' : '/api/v1/chart/data'; const requestConfig: RequestConfig = { endpoint, 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 5bd2d3d64bf..dbc41065e48 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts @@ -82,7 +82,11 @@ export default class SupersetClientClass { unauthorizedHandler = undefined, }: ClientConfig = {}) { const url = new URL(`${protocol || 'https:'}//${host || 'localhost'}`); - this.appRoot = appRoot; + // Strip a trailing slash so the getUrl dedupe comparisons and the final + // `${this.appRoot}/${...}` build stay correct regardless of how the root + // was supplied. Mirrors normalizeBackendUrlString / AppRootMiddleware / + // LegacyPrefixRedirectMiddleware, which all rstrip the root. + this.appRoot = appRoot.replace(/\/$/, ''); this.host = url.host; this.protocol = url.protocol as Protocol; this.headers = { Accept: 'application/json', ...headers }; // defaulting accept to json @@ -296,8 +300,26 @@ export default class SupersetClientClass { const host = inputHost ?? this.host; const cleanHost = host.slice(-1) === '/' ? host.slice(0, -1) : host; // no backslash + // Strip a single 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. Single-pass strip mirrors + // `stripAppRoot` in `src/utils/pathUtils` and `normalizeBackendUrlString` + // exactly: a genuine `/superset/superset/` is a legitimate route, not + // a double-prefix bug. The L2 static invariant still flags pre-prefixing 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/src/connection/callApi/parseResponse.ts b/superset-frontend/packages/superset-ui-core/src/connection/callApi/parseResponse.ts index 0716f16a8ac..38c76b60ffd 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/callApi/parseResponse.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/callApi/parseResponse.ts @@ -55,24 +55,25 @@ export default async function parseResponse( if (parseMethod === 'json-bigint') { const rawData = await response.text(); const json = JSONbig.parse(rawData); + const decoded = cloneDeepWith(json, (value: any) => { + if ( + value?.isInteger?.() === true && + (value?.isGreaterThan?.(Number.MAX_SAFE_INTEGER) || + value?.isLessThan?.(Number.MIN_SAFE_INTEGER)) + ) { + // toFixed() avoids scientific notation, which BigInt() rejects. + return BigInt(value.toFixed()); + } + // // `json-bigint` could not handle floats well, see sidorares/json-bigint#62 + // // TODO: clean up after json-bigint>1.0.1 is released + if (value?.isNaN?.() === false) { + return value?.toNumber?.(); + } + return undefined; + }); const result: JsonResponse = { response, - json: cloneDeepWith(json, (value: any) => { - if ( - value?.isInteger?.() === true && - (value?.isGreaterThan?.(Number.MAX_SAFE_INTEGER) || - value?.isLessThan?.(Number.MIN_SAFE_INTEGER)) - ) { - // toFixed() avoids scientific notation, which BigInt() rejects. - return BigInt(value.toFixed()); - } - // // `json-bigint` could not handle floats well, see sidorares/json-bigint#62 - // // TODO: clean up after json-bigint>1.0.1 is released - if (value?.isNaN?.() === false) { - return value?.toNumber?.(); - } - return undefined; - }), + json: decoded, }; return result as ReturnType; } diff --git a/superset-frontend/packages/superset-ui-core/src/connection/index.ts b/superset-frontend/packages/superset-ui-core/src/connection/index.ts index 3ac9806a055..4fdf85e3f9b 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/index.ts @@ -21,6 +21,9 @@ export { default as callApi } from './callApi'; export { default as SupersetClient } from './SupersetClient'; export { default as SupersetClientClass } from './SupersetClientClass'; +export { normalizeBackendUrlString } from './normalizeBackendUrls'; +export type { NormalizeOptions } from './normalizeBackendUrls'; + export * from './types'; export * from './constants'; export { default as __hack_reexport_connection } from './types'; diff --git a/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts new file mode 100644 index 00000000000..31f2731e7fc --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Strips the configured application root from a single backend-supplied URL + * string so the frontend speaks router-relative paths. Apply it at the few call + * sites that surface a router-relative URL from an API response (e.g. a + * dataset's `explore_url`) before handing the value to a consumer that + * re-prefixes the root — `SupersetClient.getUrl`, `makeUrl`, or a react-router + * `` resolving against the Router `basename`. Without it those consumers + * would re-prefix an already-rooted path into `/superset/superset/...`. + * + * Absolute (`https:`, `ftp:`, `mailto:`, `tel:`) and protocol-relative (`//`) + * URLs pass through untouched, so an operator-configured external + * `default_endpoint` on a dataset is left alone. + */ + +export interface NormalizeOptions { + /** Application root to strip. Empty string disables normalisation. */ + applicationRoot: string; +} + +const SAFE_ABSOLUTE_URL_RE = /^(?:https?|ftp|mailto|tel):/i; + +function stripTrailingSlash(root: string): string { + return root.endsWith('/') ? root.slice(0, -1) : root; +} + +/** Normalise a single router-relative URL string. */ +export function normalizeBackendUrlString( + value: string, + options: NormalizeOptions, +): string { + const root = stripTrailingSlash(options.applicationRoot); + if (!root) return value; + if (SAFE_ABSOLUTE_URL_RE.test(value)) return value; + if (value.startsWith('//')) return value; + if (value === root) return '/'; + if (value.startsWith(`${root}/`)) { + return value.slice(root.length); + } + return value; +} diff --git a/superset-frontend/packages/superset-ui-core/src/query/api/legacy/getDatasourceMetadata.ts b/superset-frontend/packages/superset-ui-core/src/query/api/legacy/getDatasourceMetadata.ts index 344d5891cae..c4a5c0f26fb 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/api/legacy/getDatasourceMetadata.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/api/legacy/getDatasourceMetadata.ts @@ -32,7 +32,7 @@ export default function getDatasourceMetadata({ }: Params) { return client .get({ - endpoint: `/superset/fetch_datasource_metadata?datasourceKey=${datasourceKey}`, + endpoint: `/fetch_datasource_metadata?datasourceKey=${datasourceKey}`, ...requestConfig, }) .then(response => response.json as Datasource); diff --git a/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.test.ts b/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.test.ts index 5313a9c1c2e..170c04370ff 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.test.ts +++ b/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.test.ts @@ -283,6 +283,22 @@ test('lookalike OpenStreetMap hostnames do not receive OSM attribution', () => { } }); +test('relative raster tile templates do not receive OSM attribution', () => { + // A host-relative template cannot be parsed by `new URL`, so the OSM + // hostname check must fall through to "not OSM" rather than throw. + const relativeTileUrl = '/local-tiles/{z}/{x}/{y}.png'; + const style = resolveMapStyle( + `tile://${relativeTileUrl}`, + 'default-style.json', + ); + + expect(typeof style).toBe('object'); + if (typeof style !== 'string') { + expect(style.sources['osm-raster-tiles'].tiles).toEqual([relativeTileUrl]); + expect(style.sources['osm-raster-tiles']).not.toHaveProperty('attribution'); + } +}); + test('style JSON URLs pass through without raster wrapping', () => { const styleUrl = 'https://example.com/styles/custom-style.json'; diff --git a/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts b/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts index 56c4ca1714c..901cb00a9fd 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts +++ b/superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts @@ -88,6 +88,9 @@ type BootstrapData = { }; export function getBootstrapDataFromDocument(): unknown { + /* istanbul ignore if -- a missing document only occurs in SSR/worker + contexts, which Jest cannot simulate: jsdom pins `document` as a + non-configurable global */ if (typeof document === 'undefined') { return undefined; } diff --git a/superset-frontend/packages/superset-ui-core/test/chart/clients/ChartClient.test.ts b/superset-frontend/packages/superset-ui-core/test/chart/clients/ChartClient.test.ts index 5bc0b6a9f1a..8d11b98d175 100644 --- a/superset-frontend/packages/superset-ui-core/test/chart/clients/ChartClient.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/chart/clients/ChartClient.test.ts @@ -176,7 +176,9 @@ describe('ChartClient', () => { Promise.reject(new Error('Unexpected all to v1 API')), ); - fetchMock.post('glob:*/superset/explore_json/', { + // post `Superset.route_base = ""`, the legacy endpoint + // collapsed from `/superset/explore_json/` to `/explore_json/`. + fetchMock.post('glob:*/explore_json/', { field1: 'abc', field2: 'def', }); @@ -198,13 +200,10 @@ describe('ChartClient', () => { describe('.loadDatasource(datasourceKey, options)', () => { test('fetches datasource', () => { - fetchMock.get( - 'glob:*/superset/fetch_datasource_metadata?datasourceKey=1__table', - { - field1: 'abc', - field2: 'def', - }, - ); + fetchMock.get('glob:*/fetch_datasource_metadata?datasourceKey=1__table', { + field1: 'abc', + field2: 'def', + }); return expect(chartClient.loadDatasource('1__table')).resolves.toEqual({ field1: 'abc', @@ -264,13 +263,10 @@ describe('ChartClient', () => { color: 'living-coral', }); - fetchMock.get( - 'glob:*/superset/fetch_datasource_metadata?datasourceKey=1__table', - { - name: 'transactions', - schema: 'staging', - }, - ); + fetchMock.get('glob:*/fetch_datasource_metadata?datasourceKey=1__table', { + name: 'transactions', + schema: 'staging', + }); fetchMock.post('glob:*/api/v1/chart/data', { lorem: 'ipsum', 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 new file mode 100644 index 00000000000..ff8cf65863a --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts @@ -0,0 +1,113 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { SupersetClientClass } from '@superset-ui/core'; + +// SupersetClient is expected to apply the configured appRoot exactly once. +// Callers must pass router-relative endpoints; pre-prefixing causes the +// double-prefix bug documented below. + +describe('SupersetClient applies the application root exactly once', () => { + const buildClient = () => + new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + appRoot: '/superset', + }); + + test('endpoint without leading slash is concatenated correctly', () => { + expect(buildClient().getUrl({ endpoint: 'api/v1/chart' })).toBe( + 'https://config_host/superset/api/v1/chart', + ); + }); + + test('endpoint with leading slash is normalised to a single root segment', () => { + expect(buildClient().getUrl({ endpoint: '/api/v1/chart' })).toBe( + 'https://config_host/superset/api/v1/chart', + ); + }); + + // A trailing slash on the configured appRoot is stripped at construction + // (SupersetClientClass `appRoot.replace(/\/$/, '')`). Without it, a root of + // '/superset/' produced 'https://host/superset//foo', and the dedupe block's + // `startsWith('/superset//')` check silently failed to dedupe a pre-prefixed + // endpoint. This pins both behaviours against regression. + test('trailing-slash appRoot is normalised to a single root segment', () => { + const client = new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + appRoot: '/superset/', + }); + expect(client.getUrl({ endpoint: '/api/v1/chart' })).toBe( + 'https://config_host/superset/api/v1/chart', + ); + // and a pre-prefixed endpoint is still deduped, not doubled + expect(client.getUrl({ endpoint: '/superset/api/v1/chart' })).toBe( + 'https://config_host/superset/api/v1/chart', + ); + }); + + // 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/api/v1/chart', + ); + }); + + // Single-pass strip preserves a legitimate `/superset/superset/` + // route. Backend-supplied router-relative URLs are stripped of the root at + // the call sites that surface them (via `normalizeBackendUrlString`) before + // any re-prefixing helper sees them, so a doubled leading segment reaching + // `getUrl` is a real route, not a double-prefix bug. This pin guards against + // silent regression to a greedy strip. + test('strips exactly one application-root segment (single-pass)', () => { + expect( + buildClient().getUrl({ endpoint: '/superset/superset/api/v1/chart' }), + ).toBe('https://config_host/superset/superset/api/v1/chart'); + expect( + buildClient().getUrl({ + endpoint: '/superset/superset/superset/api/v1/chart', + }), + ).toBe('https://config_host/superset/superset/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/', + ); + }); + + test('empty application root produces no prefix segment', () => { + const client = new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + }); + expect(client.getUrl({ endpoint: '/api/v1/chart' })).toBe( + 'https://config_host/api/v1/chart', + ); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts new file mode 100644 index 00000000000..2100cbdedd7 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts @@ -0,0 +1,89 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { normalizeBackendUrlString } from '../../src/connection/normalizeBackendUrls'; + +const PREFIX = '/superset'; + +describe('normalizeBackendUrlString', () => { + test('strips application root from a router-relative path', () => { + expect( + normalizeBackendUrlString('/superset/explore/?slice_id=1', { + applicationRoot: PREFIX, + }), + ).toBe('/explore/?slice_id=1'); + }); + + test('strips a value that equals the application root exactly', () => { + expect( + normalizeBackendUrlString('/superset', { applicationRoot: PREFIX }), + ).toBe('/'); + }); + + test('tolerates a trailing slash on applicationRoot', () => { + expect( + normalizeBackendUrlString('/superset/foo', { + applicationRoot: '/superset/', + }), + ).toBe('/foo'); + }); + + // The negative cases below prove the helper is conservative: it doesn't + // mutate external URLs or path segments that merely share text with the root. + test('passes absolute URLs through unchanged', () => { + expect( + normalizeBackendUrlString('https://external.example.com/superset/foo', { + applicationRoot: PREFIX, + }), + ).toBe('https://external.example.com/superset/foo'); + }); + + test('passes protocol-relative URLs through unchanged', () => { + expect( + normalizeBackendUrlString('//cdn.example.com/superset/foo', { + applicationRoot: PREFIX, + }), + ).toBe('//cdn.example.com/superset/foo'); + }); + + test('does not strip a similar-but-different prefix segment', () => { + // /superset-public/... shares text with /superset but is a different path + // segment. Only /superset followed by / or end-of-string counts. + expect( + normalizeBackendUrlString('/superset-public/explore/?slice_id=1', { + applicationRoot: PREFIX, + }), + ).toBe('/superset-public/explore/?slice_id=1'); + }); + + test('is a no-op when application root is empty', () => { + expect( + normalizeBackendUrlString('/superset/explore/?slice_id=1', { + applicationRoot: '', + }), + ).toBe('/superset/explore/?slice_id=1'); + }); + + test('is idempotent: normalize(normalize(x)) === normalize(x)', () => { + const once = normalizeBackendUrlString('/superset/explore/?id=1', { + applicationRoot: PREFIX, + }); + const twice = normalizeBackendUrlString(once, { applicationRoot: PREFIX }); + expect(twice).toBe(once); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/test/query/api/legacy/getDatasourceMetadata.test.ts b/superset-frontend/packages/superset-ui-core/test/query/api/legacy/getDatasourceMetadata.test.ts index be94bfa9f68..5ae788d10ad 100644 --- a/superset-frontend/packages/superset-ui-core/test/query/api/legacy/getDatasourceMetadata.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/query/api/legacy/getDatasourceMetadata.test.ts @@ -35,8 +35,10 @@ describe('getFormData()', () => { field2: 'def', }; + // post-`route_base=""`, the legacy endpoint collapsed + // from `/superset/fetch_datasource_metadata` to `/fetch_datasource_metadata`. fetchMock.get( - 'glob:*/superset/fetch_datasource_metadata?datasourceKey=1__table', + 'glob:*/fetch_datasource_metadata?datasourceKey=1__table', mockData, ); diff --git a/superset-frontend/playwright/pages/DashboardPage.ts b/superset-frontend/playwright/pages/DashboardPage.ts index a61ff3415c1..9b81d493d94 100644 --- a/superset-frontend/playwright/pages/DashboardPage.ts +++ b/superset-frontend/playwright/pages/DashboardPage.ts @@ -44,7 +44,7 @@ export class DashboardPage { * @param slug - The dashboard slug (e.g., 'world_health') */ async gotoBySlug(slug: string): Promise { - await gotoWithRetry(this.page, `superset/dashboard/${slug}/`); + await gotoWithRetry(this.page, `dashboard/${slug}/`); } /** @@ -52,7 +52,7 @@ export class DashboardPage { * @param id - The dashboard ID */ async gotoById(id: number): Promise { - await gotoWithRetry(this.page, `superset/dashboard/${id}/`); + await gotoWithRetry(this.page, `dashboard/${id}/`); } /** diff --git a/superset-frontend/playwright/utils/urls.ts b/superset-frontend/playwright/utils/urls.ts index ed78c0b1c53..e35eddd2249 100644 --- a/superset-frontend/playwright/utils/urls.ts +++ b/superset-frontend/playwright/utils/urls.ts @@ -35,5 +35,5 @@ export const URL = { LOGIN: 'login/', SAVED_QUERIES_LIST: 'savedqueryview/list/', SQLLAB: 'sqllab', - WELCOME: 'superset/welcome/', + WELCOME: 'welcome/', } as const; diff --git a/superset-frontend/spec/helpers/sourceTreeScanner.ts b/superset-frontend/spec/helpers/sourceTreeScanner.ts new file mode 100644 index 00000000000..c5798592d0c --- /dev/null +++ b/superset-frontend/spec/helpers/sourceTreeScanner.ts @@ -0,0 +1,183 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { readdirSync, readFileSync, statSync } from 'fs'; +import { join, relative, resolve, sep } from 'path'; + +const DEFAULT_ROOTS = ['src', 'packages/superset-ui-core/src']; + +const ALWAYS_SKIP_SEGMENTS = new Set([ + 'node_modules', + 'dist', + 'build', + 'coverage', + '__mocks__', + 'cypress-base', + 'playwright', +]); + +const ALWAYS_SKIP_SUFFIXES = [ + '.test.ts', + '.test.tsx', + '.stories.ts', + '.stories.tsx', +]; + +const SOURCE_EXTENSIONS = ['.ts', '.tsx']; + +export interface ScanOptions { + /** Workspace-relative directories to scan. Defaults to the source tree. */ + roots?: string[]; + /** Extra path segments to skip on top of {@link ALWAYS_SKIP_SEGMENTS}. */ + ignoreSegments?: string[]; + /** Regex run against each line of each file. */ + pattern: RegExp; + /** Workspace-relative paths (forward slashes) exempt from this scan. */ + allowlist?: string[]; +} + +export interface ScanHit { + /** Workspace-relative path with forward slashes. */ + file: string; + /** 1-based line number. */ + line: number; + /** The text of the matching line, trimmed. */ + text: string; + /** The substring captured by `pattern`. */ + match: string; +} + +// __dirname resolves to /spec/helpers regardless of cwd. +const WORKSPACE_ROOT = resolve(__dirname, '..', '..'); + +function isSourceFile(name: string): boolean { + return ( + SOURCE_EXTENSIONS.some(ext => name.endsWith(ext)) && + !ALWAYS_SKIP_SUFFIXES.some(suffix => name.endsWith(suffix)) + ); +} + +function walk(directory: string, ignoreSegments: Set): string[] { + const found: string[] = []; + + let entries; + try { + entries = readdirSync(directory, { withFileTypes: true }); + } catch { + return found; + } + + for (const entry of entries) { + if (ignoreSegments.has(entry.name)) continue; + const absolute = join(directory, entry.name); + + if (entry.isDirectory()) { + found.push(...walk(absolute, ignoreSegments)); + } else if (entry.isFile() && isSourceFile(entry.name)) { + found.push(absolute); + } + } + + return found; +} + +function toForwardSlashes(path: string): string { + return sep === '/' ? path : path.split(sep).join('/'); +} + +/** + * Line-by-line regex scan over the source tree. Returns one {@link ScanHit} + * per matching line. Textual (not AST-based) — false positives on string + * literals should be fixed by tightening the regex. + */ +export function scanSource(options: ScanOptions): ScanHit[] { + const { + roots = DEFAULT_ROOTS, + ignoreSegments = [], + pattern, + allowlist = [], + } = options; + + const ignoreSet = new Set([...ALWAYS_SKIP_SEGMENTS, ...ignoreSegments]); + const allowSet = new Set(allowlist); + const hits: ScanHit[] = []; + + const seen = new Set(); + for (const root of roots) { + const absoluteRoot = resolve(WORKSPACE_ROOT, root); + let stat; + try { + stat = statSync(absoluteRoot); + } catch { + continue; + } + if (!stat.isDirectory()) continue; + + for (const absoluteFile of walk(absoluteRoot, ignoreSet)) { + if (seen.has(absoluteFile)) continue; + seen.add(absoluteFile); + + const relativePath = toForwardSlashes( + relative(WORKSPACE_ROOT, absoluteFile), + ); + if (allowSet.has(relativePath)) continue; + + const contents = readFileSync(absoluteFile, 'utf8'); + const lines = contents.split('\n'); + + // Reuse the regex per file. Without the `g` flag, `.exec` ignores + // lastIndex, so recompiling per-line was wasted allocation. + const lineRegex = pattern.flags.includes('g') + ? new RegExp(pattern.source, pattern.flags.replace('g', '')) + : pattern; + + for (let index = 0; index < lines.length; index += 1) { + const lineText = lines[index]; + const match = lineRegex.exec(lineText); + if (match) { + hits.push({ + file: relativePath, + line: index + 1, + text: lineText.trim(), + match: match[0], + }); + } + } + } + } + + return hits; +} + +/** Format hits as a multi-line failure message: ` file:line — text`. */ +export function formatHits(hits: ScanHit[], header: string): string { + if (hits.length === 0) return header; + const lines = hits + .slice(0, 50) + .map(hit => ` ${hit.file}:${hit.line} — ${hit.text}`); + const overflow = + hits.length > 50 ? `\n ... and ${hits.length - 50} more` : ''; + return `${header}\n${lines.join('\n')}${overflow}`; +} + +/** Throw with a formatted message if `hits` is non-empty. */ +export function expectNoHits(hits: ScanHit[], header: string): void { + if (hits.length > 0) { + throw new Error(formatHits(hits, header)); + } +} diff --git a/superset-frontend/spec/helpers/withApplicationRoot.ts b/superset-frontend/spec/helpers/withApplicationRoot.ts new file mode 100644 index 00000000000..a8453572356 --- /dev/null +++ b/superset-frontend/spec/helpers/withApplicationRoot.ts @@ -0,0 +1,53 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Run `callback` with `getBootstrapData().common.application_root` set to + * `applicationRoot`. Resets modules so any imports inside the callback see + * the configured value, then restores the prior DOM and module cache on exit. + * Pass `''` to simulate the default root-of-domain deployment. + */ +export async function withApplicationRoot( + applicationRoot: string, + callback: () => Promise | T, +): Promise { + const previousBody = document.body.innerHTML; + + try { + const bootstrapData = { common: { application_root: applicationRoot } }; + document.body.innerHTML = `
`; + jest.resetModules(); + await import('src/utils/getBootstrapData'); + return await callback(); + } finally { + document.body.innerHTML = previousBody; + jest.resetModules(); + } +} + +/** Run `body` once per scenario, each under a different application root. */ +export async function applicationRootScenarios( + scenarios: S[], + body: (scenario: S) => Promise | void, +): Promise { + for (const scenario of scenarios) { + // eslint-disable-next-line no-await-in-loop -- intentional: scenarios share document state. + await withApplicationRoot(scenario.root, () => body(scenario)); + } +} diff --git a/superset-frontend/src/SqlLab/components/QueryTable/index.tsx b/superset-frontend/src/SqlLab/components/QueryTable/index.tsx index 18a3d73235b..fda383430d4 100644 --- a/superset-frontend/src/SqlLab/components/QueryTable/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryTable/index.tsx @@ -44,7 +44,7 @@ import { import { fDuration, extendedDayjs } from '@superset-ui/core/utils/dates'; import { SqlLabRootState } from 'src/SqlLab/types'; import { UserWithPermissionsAndRoles as User } from 'src/types/bootstrapTypes'; -import { makeUrl } from 'src/utils/pathUtils'; +import { openInNewTab } from 'src/utils/navigationUtils'; import ResultSet from '../ResultSet'; import HighlightedSql from '../HighlightedSql'; import { StaticPosition, StyledTooltip, ModalResultSetWrapper } from './styles'; @@ -80,8 +80,7 @@ interface QueryTableProps { } const openQuery = (id: number) => { - const url = makeUrl(`/sqllab?queryId=${id}`); - window.open(url); + openInNewTab(`/sqllab?queryId=${id}`); }; const QueryTable = ({ diff --git a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx index 3885f195532..9e31eb5ab58 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx @@ -53,7 +53,28 @@ jest.mock('@superset-ui/core', () => ({ isFeatureEnabled: jest.fn().mockReturnValue(false), })); +// Mock openInNewTab so the Create-chart "new window" branch can be asserted +// without spawning a real window. The rest of navigationUtils stays real so +// existing CSV-download tests keep using the genuine `redirect`/`makeUrl`. +jest.mock('src/utils/navigationUtils', () => ({ + ...jest.requireActual('src/utils/navigationUtils'), + openInNewTab: jest.fn(), +})); +// eslint-disable-next-line import/order, import/first +import { openInNewTab } from 'src/utils/navigationUtils'; + +// Stub postFormData so the Create-chart click resolves quickly; this lets +// the test focus on the URL composition that happens after the resolve. +jest.mock('src/explore/exploreUtils/formData', () => ({ + ...jest.requireActual('src/explore/exploreUtils/formData'), + postFormData: jest.fn(), +})); +// eslint-disable-next-line import/order, import/first +import { postFormData } from 'src/explore/exploreUtils/formData'; + const mockIsFeatureEnabled = isFeatureEnabled as jest.Mock; +const mockOpenInNewTab = openInNewTab as jest.Mock; +const mockPostFormData = postFormData as jest.Mock; jest.mock('src/components/ErrorMessage', () => ({ ErrorMessageWithStackTrace: () =>
Error
, @@ -160,6 +181,9 @@ describe('ResultSet', () => { beforeEach(() => { applicationRootMock.mockReturnValue(''); mockStartExport.mockClear(); + mockOpenInNewTab.mockClear(); + mockPostFormData.mockReset(); + mockPostFormData.mockResolvedValue('test-form-data-key'); }); // Add cleanup after each test @@ -1009,4 +1033,103 @@ describe('ResultSet', () => { screen.getByRole('button', { name: 'Results Action' }), ).toBeInTheDocument(); }); + + test('Create chart in new window opens single-prefixed explore URL under subdirectory deployment', async () => { + // When the user metaKey-clicks "Create chart", the SQL-Lab result handoff + // composes an explore URL via mountExploreUrl(..., includeAppRoot=true). + // Under SUPERSET_APP_ROOT=/superset, the resulting URL must contain the + // prefix exactly once. A doubled prefix (/superset/superset/explore/…) + // produces a blank Explore page. + const appRoot = '/superset'; + applicationRootMock.mockReturnValue(appRoot); + + const queryWithId = { + ...queries[0], + results: { + ...queries[0].results, + query_id: 42, + }, + }; + + const { getByTestId } = setup( + { + ...mockedProps, + queryId: queryWithId.id, + database: { allows_subquery: true, allows_virtual_table_explore: true }, + }, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queryWithId.id]: queryWithId, + }, + }, + }), + ); + + const exploreButton = await waitFor(() => + getByTestId('explore-results-button'), + ); + fireEvent.click(exploreButton, { metaKey: true }); + + await waitFor(() => { + expect(mockOpenInNewTab).toHaveBeenCalledTimes(1); + }); + const url = mockOpenInNewTab.mock.calls[0][0] as string; + expect(url).toMatch(/^\/superset\/explore\/\?.*form_data_key=/); + expect(url).not.toMatch(/\/superset\/superset\//); + }); + + test('Create chart in same window pushes router-relative explore URL under subdirectory deployment', async () => { + // Same-tab click (no metaKey) goes through history.push under the SPA + // basename Router, so mountExploreUrl is called with includeAppRoot=false. + // The composed URL must NOT carry an app-root prefix — the router applies + // it once via . A premature prefix + // here would compound with the basename and yield /superset/superset/… + const appRoot = '/superset'; + applicationRootMock.mockReturnValue(appRoot); + + const queryWithId = { + ...queries[0], + results: { + ...queries[0].results, + query_id: 99, + }, + }; + + const store = mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queryWithId.id]: queryWithId, + }, + }, + }); + + const { getByTestId } = render( + , + { useRedux: true, store, useRouter: true }, + ); + + const exploreButton = await waitFor(() => + getByTestId('explore-results-button'), + ); + fireEvent.click(exploreButton); + + await waitFor(() => { + expect(mockPostFormData).toHaveBeenCalledTimes(1); + }); + expect(mockOpenInNewTab).not.toHaveBeenCalled(); + }); }); diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx index 732d1dec8f9..cb6ee6883ac 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { sanitizeUrl } from '@braintree/sanitize-url'; import { useCallback, useEffect, @@ -88,7 +87,7 @@ import { usePermissions } from 'src/hooks/usePermissions'; import { StreamingExportModal } from 'src/components/StreamingExportModal'; import { useStreamingExport } from 'src/components/StreamingExportModal/useStreamingExport'; import { useConfirmModal } from 'src/hooks/useConfirmModal'; -import { makeUrl } from 'src/utils/pathUtils'; +import { makeUrl, openInNewTab, redirect } from 'src/utils/navigationUtils'; import ExploreCtasResultsButton from '../ExploreCtasResultsButton'; import ExploreResultsButton from '../ExploreResultsButton'; import HighlightedSql from '../HighlightedSql'; @@ -312,7 +311,9 @@ const ResultSet = ({ includeAppRoot, ); if (openInNewWindow) { - window.open(url, '_blank', 'noreferrer'); + // `url` is from `mountExploreUrl(..., includeAppRoot=true)`; the + // helper re-applies `ensureAppRoot` idempotently. + openInNewTab(url); } else { history.push(url); } @@ -379,7 +380,13 @@ const ResultSet = ({ { rows: rowsCount.toLocaleString() }, ), onConfirm: () => { - window.location.href = sanitizeUrl(getExportCsvUrl(query.id)); + // `getExportCsvUrl` already runs the path through `makeUrl`; + // `redirect` re-applies `ensureAppRoot` idempotently and routes + // the sink through navigationUtils' barriers (scheme allowlist, + // userinfo rejection, backslash rejection), which is a + // strict superset of what `sanitizeUrl` from master PR #40546 + // provides. + redirect(getExportCsvUrl(query.id)); }, confirmText: t('OK'), cancelText: t('Close'), diff --git a/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx b/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx index e1de7b46634..b39ec14abcb 100644 --- a/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx +++ b/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { sanitizeUrl } from '@braintree/sanitize-url'; import { useCallback, useState, FormEvent } from 'react'; import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; import { Radio, RadioChangeEvent } from '@superset-ui/core/components/Radio'; @@ -58,6 +57,7 @@ import { postFormData } from 'src/explore/exploreUtils/formData'; import { URL_PARAMS } from 'src/constants'; import { isEmpty } from 'lodash-es'; import { clearDatasetCache } from 'src/utils/cachedSupersetGet'; +import { openInNewTab, redirect } from 'src/utils/navigationUtils'; interface QueryDatabase { id?: number; @@ -244,10 +244,16 @@ export const SaveDatasetModal = ({ useState(false); const createWindow = (url: string) => { + // `url` is from `mountExploreUrl(..., includeAppRoot=true)`; the + // navigationUtils helpers re-apply `ensureAppRoot` idempotently. if (openWindow) { - window.open(sanitizeUrl(url), '_blank', 'noreferrer'); + // `openInNewTab` / `redirect` route the sink through navigationUtils' + // barriers (scheme allowlist, userinfo rejection, backslash + // rejection) — strictly stronger than master PR #40546's `sanitizeUrl` + // wrap, which only rejects `javascript:` / `data:` / `vbscript:`. + openInNewTab(url); } else { - window.location.href = sanitizeUrl(url); + redirect(url); } }; const formDataWithDefaults = { diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx index f3cf210629b..3ca1a671754 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx @@ -84,7 +84,7 @@ import { } from 'src/database/actions'; import Mousetrap from 'mousetrap'; import { clearDatasetCache } from 'src/utils/cachedSupersetGet'; -import { makeUrl } from 'src/utils/pathUtils'; +import { makeUrl, openInNewTab } from 'src/utils/navigationUtils'; import { OwnerSelectLabel, OWNER_TEXT_LABEL_PROP, @@ -1181,7 +1181,9 @@ function DatasourceEditor({ }, [datasource]); const openOnSqlLab = useCallback(() => { - window.open(getSQLLabUrl(), '_blank', 'noopener,noreferrer'); + // `getSQLLabUrl()` already runs the path through `makeUrl`; `openInNewTab` + // re-applies `ensureAppRoot`, which is idempotent on already-prefixed paths. + openInNewTab(getSQLLabUrl()); }, [getSQLLabUrl]); const onQueryRun = useCallback(async () => { diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/DashboardLinksExternal.test.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/DashboardLinksExternal.test.tsx index 28cac0d17a1..853e8e7d5f5 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/DashboardLinksExternal.test.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/DashboardLinksExternal.test.tsx @@ -66,7 +66,7 @@ test('renders single dashboard link correctly', () => { const link = screen.getByText('Sales Dashboard'); expect(link).toBeInTheDocument(); - expect(link.closest('a')).toHaveAttribute('href', '/superset/dashboard/1/'); + expect(link.closest('a')).toHaveAttribute('href', '/dashboard/1/'); expect(link.closest('a')).toHaveAttribute('target', '_blank'); }); @@ -98,9 +98,9 @@ test('links have correct href attributes', () => { .getByText(', Very Long Dashboard Name That Should Be Truncated') .closest('a'); - expect(salesLink).toHaveAttribute('href', '/superset/dashboard/1/'); - expect(analyticsLink).toHaveAttribute('href', '/superset/dashboard/2/'); - expect(longNameLink).toHaveAttribute('href', '/superset/dashboard/3/'); + expect(salesLink).toHaveAttribute('href', '/dashboard/1/'); + expect(analyticsLink).toHaveAttribute('href', '/dashboard/2/'); + expect(longNameLink).toHaveAttribute('href', '/dashboard/3/'); }); test('applies correct styling classes', () => { @@ -124,5 +124,5 @@ test('handles dashboard with empty title', () => { const link = screen.getByRole('link'); expect(link).toHaveTextContent(''); - expect(link).toHaveAttribute('href', '/superset/dashboard/1/'); + expect(link).toHaveAttribute('href', '/dashboard/1/'); }); diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/index.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/index.tsx index d9b25673dae..0ac14ebbffc 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/index.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DashboardLinksExternal/index.tsx @@ -62,7 +62,7 @@ const DashboardLinksExternal = ({ {dashboards.map((dashboard, index) => ( {index === 0 diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditor.test.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditor.test.tsx index 97e1e5f5dfb..e4fd777d19a 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditor.test.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditor.test.tsx @@ -25,6 +25,7 @@ import { within, } from 'spec/helpers/testing-library'; import { DatasourceType, isFeatureEnabled } from '@superset-ui/core'; +import * as getBootstrapData from 'src/utils/getBootstrapData'; import { createProps, DATASOURCE_ENDPOINT, @@ -822,3 +823,57 @@ test('calculated column search is case-insensitive', async () => { expect(screen.getByDisplayValue('upper_name')).toBeInTheDocument(); }); }); + +test('Open in SQL lab href is single-prefixed under subdirectory deployment', () => { + // The Open-in-SQL-Lab link's href is produced by `getSQLLabUrl()`: + // return makeUrl(`/sqllab/?${queryParams.toString()}`); + // `makeUrl` is the idempotent app-root prefix helper from + // `src/utils/navigationUtils`. Rendering the link requires both the + // virtual datasourceType state AND a populated Redux `database.queryResult` + // slice (which is not part of the default test reducer tree). Calling + // `makeUrl` directly with a `/superset` mock exercises the exact path the + // component takes and pins the dedupe invariant for the underlying helper. + const applicationRootSpy = jest + .spyOn(getBootstrapData, 'applicationRoot') + .mockReturnValue('/superset'); + try { + const { makeUrl } = jest.requireActual('src/utils/navigationUtils'); + const queryParams = new URLSearchParams({ + dbid: '1', + sql: 'SELECT * FROM users', + name: 'Vehicle Sales', + schema: 'public', + autorun: 'true', + isDataset: 'true', + }); + const url = makeUrl(`/sqllab/?${queryParams.toString()}`); + expect(url).toMatch(/^\/superset\/sqllab\/\?/); + expect(url).not.toMatch(/\/superset\/superset\//); + } finally { + applicationRootSpy.mockRestore(); + } +}); + +test('DatasourceEditor source pins getSQLLabUrl/openOnSqlLab to the makeUrl + openInNewTab helpers', () => { + // Source-pin: lock the exact two-line shape the runtime behaviour depends + // on. `getSQLLabUrl` MUST wrap its `/sqllab/?...` path in `makeUrl` so the + // Layer-2 idempotent prefix runs at the click boundary; `openOnSqlLab` + // MUST delegate to `openInNewTab` so `ensureAppRoot` runs again (idempotent + // dedupe, see `navigationUtils.appRoot.test.tsx`). A refactor that drops + // either layer would let a doubled-prefix URL escape into a new tab. + // eslint-disable-next-line global-require + const { readFileSync } = require('fs'); + // eslint-disable-next-line global-require + const { join } = require('path'); + const src = readFileSync( + join(__dirname, '..', 'DatasourceEditor.tsx'), + 'utf8', + ); + expect(src).toMatch( + /return makeUrl\(`\/sqllab\/\?\$\{queryParams\.toString\(\)\}`\);/, + ); + expect(src).toMatch(/openInNewTab\(getSQLLabUrl\(\)\);/); + expect(src).toMatch( + /import \{ makeUrl, openInNewTab \} from 'src\/utils\/navigationUtils';/, + ); +}); diff --git a/superset-frontend/src/components/FacePile/index.tsx b/superset-frontend/src/components/FacePile/index.tsx index 220209afc2d..5fefd307d25 100644 --- a/superset-frontend/src/components/FacePile/index.tsx +++ b/superset-frontend/src/components/FacePile/index.tsx @@ -24,7 +24,7 @@ import { } from '@superset-ui/core'; import getOwnerName from 'src/utils/getOwnerName'; import { Avatar, AvatarGroup, Tooltip } from '@superset-ui/core/components'; -import { ensureAppRoot } from 'src/utils/pathUtils'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; import { getRandomColor } from './utils'; import type { FacePileProps } from './types'; diff --git a/superset-frontend/src/components/ListView/CrossLinks.test.tsx b/superset-frontend/src/components/ListView/CrossLinks.test.tsx index 403b94a084c..ae2b5f0ec8f 100644 --- a/superset-frontend/src/components/ListView/CrossLinks.test.tsx +++ b/superset-frontend/src/components/ListView/CrossLinks.test.tsx @@ -68,10 +68,9 @@ test('should render the link with just one item', () => { ], }); expect(screen.getByText('Test dashboard')).toBeInTheDocument(); - expect(screen.getByRole('link')).toHaveAttribute( - 'href', - `/superset/dashboard/1`, - ); + // default `linkPrefix` is now `/dashboard/` (post-route_base); + // legacy `/superset/dashboard/...` was the pre-collapse route. + expect(screen.getByRole('link')).toHaveAttribute('href', `/dashboard/1`); }); test('should render a custom prefix link', () => { diff --git a/superset-frontend/src/components/ListView/CrossLinks.tsx b/superset-frontend/src/components/ListView/CrossLinks.tsx index 6f4cd6d262b..d3f6f0a474c 100644 --- a/superset-frontend/src/components/ListView/CrossLinks.tsx +++ b/superset-frontend/src/components/ListView/CrossLinks.tsx @@ -65,7 +65,7 @@ const StyledCrossLinks = styled.div` function CrossLinks({ crossLinks, maxLinks = 20, - linkPrefix = '/superset/dashboard/', + linkPrefix = '/dashboard/', external = false, }: CrossLinksProps) { const [crossLinksRef, plusRef, elementsTruncated, hasHiddenElements] = diff --git a/superset-frontend/src/components/StreamingExportModal/useStreamingExport.ts b/superset-frontend/src/components/StreamingExportModal/useStreamingExport.ts index 4a8aa0fe0c3..617e93b9288 100644 --- a/superset-frontend/src/components/StreamingExportModal/useStreamingExport.ts +++ b/superset-frontend/src/components/StreamingExportModal/useStreamingExport.ts @@ -19,7 +19,7 @@ import { useState, useCallback, useRef, useEffect } from 'react'; import { SupersetClient } from '@superset-ui/core'; import { ExportStatus, StreamingProgress } from './StreamingExportModal'; -import { makeUrl } from 'src/utils/pathUtils'; +import { makeUrl } from 'src/utils/navigationUtils'; import { applicationRoot } from 'src/utils/getBootstrapData'; interface UseStreamingExportOptions { @@ -38,8 +38,8 @@ interface StreamingExportParams { * The API endpoint URL for the export request. * * URLs should be prefixed with the application root at the call site using - * `makeUrl()` from 'src/utils/pathUtils'. This ensures proper handling for - * subdirectory deployments (e.g., /superset/api/v1/...). + * `makeUrl()` from `src/utils/navigationUtils`. This ensures proper handling + * for subdirectory deployments (e.g., /superset/api/v1/...). * * A defensive guard (`ensureUrlPrefix`) will apply the prefix if missing, * but callers should not rely on this fallback behavior. diff --git a/superset-frontend/src/components/Tag/index.tsx b/superset-frontend/src/components/Tag/index.tsx index d09334e175b..0605977bc08 100644 --- a/superset-frontend/src/components/Tag/index.tsx +++ b/superset-frontend/src/components/Tag/index.tsx @@ -82,7 +82,7 @@ const SupersetTag = ({ {' '} {id ? ( diff --git a/superset-frontend/src/core/navigation/index.test.ts b/superset-frontend/src/core/navigation/index.test.ts index e7c2fbe183d..c717d3779ef 100644 --- a/superset-frontend/src/core/navigation/index.test.ts +++ b/superset-frontend/src/core/navigation/index.test.ts @@ -35,12 +35,12 @@ test('getPage falls back to "home" for the welcome page and unknown pathnames', const { navigation, notifyLocationChanged } = await importNavigation(); // The default pathname ('/') is not enumerated and falls back to home. expect(navigation.getPage()).toBe('home'); - notifyLocationChanged('/superset/welcome/'); + notifyLocationChanged('/welcome/'); expect(navigation.getPage()).toBe('home'); }); test('getPage derives the page from window.location.pathname', async () => { - window.location.pathname = '/superset/dashboard/42/'; + window.location.pathname = '/dashboard/42/'; const { navigation } = await importNavigation(); expect(navigation.getPage()).toBe('dashboard'); }); @@ -56,19 +56,19 @@ test('notifyLocationChanged fires listeners on page type change', async () => { const listener = jest.fn(); const disposable = navigation.onDidChangePage(listener); - notifyLocationChanged('/superset/dashboard/1/'); + notifyLocationChanged('/dashboard/1/'); expect(listener).toHaveBeenCalledWith('dashboard'); disposable.dispose(); }); test('notifyLocationChanged does not fire listeners when page type is unchanged', async () => { - window.location.pathname = '/superset/dashboard/1/'; + window.location.pathname = '/dashboard/1/'; const { navigation, notifyLocationChanged } = await importNavigation(); const listener = jest.fn(); navigation.onDidChangePage(listener); - notifyLocationChanged('/superset/dashboard/2/'); + notifyLocationChanged('/dashboard/2/'); expect(listener).not.toHaveBeenCalled(); }); @@ -78,7 +78,7 @@ test('onDidChangePage listener is removed after dispose', async () => { const disposable = navigation.onDidChangePage(listener); disposable.dispose(); - notifyLocationChanged('/superset/dashboard/1/'); + notifyLocationChanged('/dashboard/1/'); expect(listener).not.toHaveBeenCalled(); }); diff --git a/superset-frontend/src/core/navigation/index.ts b/superset-frontend/src/core/navigation/index.ts index d41e76e50bf..2e2a61d3fb4 100644 --- a/superset-frontend/src/core/navigation/index.ts +++ b/superset-frontend/src/core/navigation/index.ts @@ -36,8 +36,12 @@ type Page = navigationApi.Page; /** Maps route path patterns to their corresponding Page type. */ const PAGE_ROUTES: { path: string; page: Page }[] = [ - { path: RoutePaths.DASHBOARD, page: 'dashboard' }, + // List routes must precede their parameterised detail counterparts: with + // prefix-free paths, `matchPath(exact: false)` lets `/dashboard/:idOrSlug/` + // greedily capture `/dashboard/list/` (idOrSlug='list'), so the more specific + // list route has to win first — mirroring the `routes.tsx` Switch precedence. { path: RoutePaths.DASHBOARD_LIST, page: 'dashboard_list' }, + { path: RoutePaths.DASHBOARD, page: 'dashboard' }, { path: RoutePaths.QUERY_HISTORY, page: 'query_history' }, { path: RoutePaths.SAVED_QUERIES, page: 'saved_queries' }, { path: RoutePaths.SQLLAB, page: 'sqllab' }, diff --git a/superset-frontend/src/dashboard/actions/dashboardState.test.ts b/superset-frontend/src/dashboard/actions/dashboardState.test.ts index 34736df6513..89af8b335f5 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.test.ts +++ b/superset-frontend/src/dashboard/actions/dashboardState.test.ts @@ -54,7 +54,7 @@ import { } from 'spec/fixtures/mockSliceEntities'; import { emptyFilters } from 'spec/fixtures/mockDashboardFilters'; import mockDashboardData from 'spec/fixtures/mockDashboardData'; -import { navigateTo } from 'src/utils/navigationUtils'; +import { navigateTo, navigateWithState } from 'src/utils/navigationUtils'; jest.mock('@superset-ui/core', () => ({ ...jest.requireActual('@superset-ui/core'), @@ -72,6 +72,7 @@ jest.mock('src/utils/navigationUtils', () => ({ const mockIsFeatureEnabled = isFeatureEnabled as jest.Mock; const mockNavigateTo = navigateTo as jest.Mock; +const mockNavigateWithState = navigateWithState as jest.Mock; // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks describe('dashboardState actions', () => { @@ -253,7 +254,48 @@ describe('dashboardState actions', () => { await waitFor(() => expect(postStub.mock.calls.length).toBe(1)); expect(mockNavigateTo).toHaveBeenCalledWith( - `/superset/dashboard/${newDashboardId}/`, + `/dashboard/${newDashboardId}/`, + ); + }); + + // `navigateWithState` regression for the + // dashboard-properties-changed save path. Two assertions in one shape: + // (a) the emitted path is router-relative (`/dashboard//`), not + // the pre-migration `/superset/dashboard//` literal that under + // subdirectory deployment would double-prefix to + // `/superset/superset/dashboard//`; + // (b) the `event: 'dashboard_properties_changed'` history-state arg is + // preserved verbatim. A previous attempt to swap `navigateWithState` + // for a plain `navigateTo` would silently drop this state object and + // the dashboard would lose its post-save UX cue. + test('saves dashboard properties via navigateWithState with state preserved', async () => { + const updatedId = 777; + const { getState, dispatch } = setup({ + dashboardState: { hasUnsavedChanges: true }, + }); + + mockNavigateWithState.mockClear(); + putStub.mockRestore(); + putStub = jest.spyOn(SupersetClient, 'put').mockResolvedValue({ + json: { + result: { ...mockDashboardData, id: updatedId, slug: null }, + last_modified_time: 0, + }, + } as any); + + const thunk = saveDashboardRequest( + newDashboardData, + updatedId, + SAVE_TYPE_OVERWRITE, + ); + await thunk(dispatch, getState); + + await waitFor(() => expect(putStub.mock.calls.length).toBe(1)); + await waitFor(() => expect(mockNavigateWithState).toHaveBeenCalled()); + + expect(mockNavigateWithState).toHaveBeenCalledWith( + `/dashboard/${updatedId}/`, + { event: 'dashboard_properties_changed' }, ); }); }); @@ -513,13 +555,11 @@ describe('dashboardState actions', () => { }); getStub.mockRestore(); - getStub = jest - .spyOn(SupersetClient, 'get') - .mockRejectedValue( - new Response(JSON.stringify({ message: 'Not found' }), { - status: 404, - }), - ); + getStub = jest.spyOn(SupersetClient, 'get').mockRejectedValue( + new Response(JSON.stringify({ message: 'Not found' }), { + status: 404, + }), + ); await fetchFaveStar(id)(dispatch, getState); @@ -536,13 +576,11 @@ describe('dashboardState actions', () => { }); getStub.mockRestore(); - getStub = jest - .spyOn(SupersetClient, 'get') - .mockRejectedValue( - new Response(JSON.stringify({ message: 'Server error' }), { - status: 500, - }), - ); + getStub = jest.spyOn(SupersetClient, 'get').mockRejectedValue( + new Response(JSON.stringify({ message: 'Server error' }), { + status: 500, + }), + ); await fetchFaveStar(id)(dispatch, getState); diff --git a/superset-frontend/src/dashboard/actions/dashboardState.ts b/superset-frontend/src/dashboard/actions/dashboardState.ts index a5af637cce1..397e29a2d7f 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.ts +++ b/superset-frontend/src/dashboard/actions/dashboardState.ts @@ -584,9 +584,7 @@ export function saveDashboardRequest( }), ); dispatch(saveDashboardFinished()); - navigateTo( - `/superset/dashboard/${(response.json as JsonObject).result?.id}/`, - ); + navigateTo(`/dashboard/${(response.json as JsonObject).result?.id}/`); dispatch(addSuccessToast(t('This dashboard was saved successfully.'))); return response; }; @@ -639,7 +637,7 @@ export function saveDashboardRequest( } dispatch(saveDashboardFinished()); // redirect to the new slug or id - navigateWithState(`/superset/dashboard/${slug || id}/`, { + navigateWithState(`/dashboard/${slug || id}/`, { event: 'dashboard_properties_changed', }); diff --git a/superset-frontend/src/dashboard/actions/datasources.ts b/superset-frontend/src/dashboard/actions/datasources.ts index f48c1242845..adb5d057be2 100644 --- a/superset-frontend/src/dashboard/actions/datasources.ts +++ b/superset-frontend/src/dashboard/actions/datasources.ts @@ -62,7 +62,7 @@ export function fetchDatasourceMetadata(key: string) { } return SupersetClient.get({ - endpoint: `/superset/fetch_datasource_metadata?datasourceKey=${key}`, + endpoint: `/fetch_datasource_metadata?datasourceKey=${key}`, }).then(({ json }) => dispatch(setDatasource(json as Datasource, key))); }; } diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index 0a934a37e2f..a3be526a82d 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 5f70f3319e6..d941b9f2324 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', () => ({ useParentSize: () => ({ parentRef: { current: null }, width: 800 }), diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx new file mode 100644 index 00000000000..e58f036b3cc --- /dev/null +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx @@ -0,0 +1,172 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render, screen, userEvent } from 'spec/helpers/testing-library'; +import { VizType } from '@superset-ui/core'; +import mockState from 'spec/fixtures/mockState'; +import SliceHeaderControls, { SliceHeaderControlsProps } from '.'; + +// Subdirectory-specific regressions live here so the existing 676-line +// SliceHeaderControls.test.tsx doesn't need to mock getBootstrapData. + +// DO NOT switch this file to +// `spec/helpers/withApplicationRoot.ts`. The fixture does +// `jest.resetModules()` + dynamic `import('src/utils/getBootstrapData')` to +// install a fixture-configured applicationRoot. But `SliceHeaderControls` is +// imported statically at the top of this file; its transitive dependency +// chain (`SliceHeaderControls` → `navigationUtils` → `pathUtils` → +// `getBootstrapData::applicationRoot`) is bound to the pre-reset module +// instance. After `withApplicationRoot('/superset')` resets modules and +// re-imports getBootstrapData on the test side, the statically-imported +// component continues to reach the OLD module whose `application_root` was +// empty at first evaluation — so the rendered tree resolves +// `applicationRoot()` to `''`, NOT `/superset`. Gate (a) of the M7 go/no-go +// fails ("the rendered SliceHeaderControls tree must resolve +// applicationRoot() to the fixture-configured value"). The hand-rolled +// `jest.mock('src/utils/getBootstrapData', ...)` below remains until a +// later slice either (i) defers the SliceHeaderControls import into the +// withApplicationRoot callback or (ii) plumbs application_root through +// React context rather than a module-scoped cache. + +// Name must start with `mock` so Jest's hoisted jest.mock() factory may +// reference it. `default` returns a static shape (not mockApplicationRoot) +// because consumers like setupClient.ts call getBootstrapData() at import +// time — calling mockApplicationRoot inside `default` hits TDZ. +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => ({ + __esModule: true, + default: () => ({ + common: { application_root: '', static_assets_prefix: '' }, + }), + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: () => '', +})); + +const SLICE_ID = 371; + +const buildProps = (): SliceHeaderControlsProps => + ({ + addDangerToast: jest.fn(), + addSuccessToast: jest.fn(), + exploreChart: jest.fn(), + exportCSV: jest.fn(), + exportFullCSV: jest.fn(), + exportXLSX: jest.fn(), + exportFullXLSX: jest.fn(), + exportPivotExcel: jest.fn(), + forceRefresh: jest.fn(), + handleToggleFullSize: jest.fn(), + toggleExpandSlice: jest.fn(), + logEvent: jest.fn(), + logExploreChart: jest.fn(), + slice: { + slice_id: SLICE_ID, + slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20371%7D', + slice_name: 'Subdirectory regression chart', + slice_description: '', + form_data: { + slice_id: SLICE_ID, + datasource: '58__table', + viz_type: VizType.Sunburst, + }, + viz_type: VizType.Sunburst, + datasource: '58__table', + description: '', + description_markeddown: '', + owners: [], + modified: '', + changed_on: 0, + }, + isCached: [false], + isExpanded: false, + cachedDttm: [''], + updatedDttm: 0, + supersetCanExplore: true, + supersetCanCSV: true, + componentId: 'CHART-subdir', + dashboardId: 26, + isFullSize: false, + chartStatus: 'rendered', + showControls: true, + supersetCanShare: true, + formData: { + slice_id: SLICE_ID, + datasource: '58__table', + viz_type: VizType.Sunburst, + }, + exploreUrl: '/explore/?dashboard_page_id=abc&slice_id=371', + defaultOpen: true, + }) as unknown as SliceHeaderControlsProps; + +const renderControls = (): void => { + render(, { + useRedux: true, + useRouter: true, + initialState: { + ...mockState, + user: { + ...mockState.user, + roles: { Admin: [['can_samples', 'Datasource']] }, + }, + }, + }); +}; + +describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory deployment', () => { + let openSpy: jest.SpyInstance; + + beforeEach(() => { + mockApplicationRoot.mockReturnValue(''); + openSpy = jest.spyOn(window, 'open').mockImplementation(() => null); + }); + + afterEach(() => { + openSpy.mockRestore(); + }); + + test('opens the unprefixed exploreUrl when application root is empty', async () => { + mockApplicationRoot.mockReturnValue(''); + renderControls(); + + userEvent.click(screen.getByRole('button', { name: 'More Options' })); + const editChart = await screen.findByText('Edit chart'); + userEvent.click(editChart, { metaKey: true }); + + expect(openSpy).toHaveBeenCalledWith( + '/explore/?dashboard_page_id=abc&slice_id=371', + '_blank', + 'noopener noreferrer', + ); + }); + + test('opens the prefixed exploreUrl when deployed under a subdirectory', async () => { + mockApplicationRoot.mockReturnValue('/superset'); + renderControls(); + + userEvent.click(screen.getByRole('button', { name: 'More Options' })); + const editChart = await screen.findByText('Edit chart'); + userEvent.click(editChart, { metaKey: true }); + + expect(openSpy).toHaveBeenCalledWith( + '/superset/explore/?dashboard_page_id=abc&slice_id=371', + '_blank', + 'noopener noreferrer', + ); + }); +}); diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index a903787e215..250b149dfd2 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -57,6 +57,7 @@ import { useDrillDetailMenuItems } from 'src/components/Chart/useDrillDetailMenu import { LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE } from 'src/logger/LogUtils'; import { MenuKeys, RootState } from 'src/dashboard/types'; import DrillDetailModal from 'src/components/Chart/DrillDetail/DrillDetailModal'; +import { openInNewTab } from 'src/utils/navigationUtils'; import { usePermissions } from 'src/hooks/usePermissions'; import { useDatasetDrillInfo } from 'src/hooks/apiResources/datasets'; import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; @@ -263,7 +264,7 @@ const SliceHeaderControls = ( props.logExploreChart?.(props.slice.slice_id); if (domEvent.metaKey || domEvent.ctrlKey) { domEvent.preventDefault(); - window.open(props.exploreUrl, '_blank'); + openInNewTab(props.exploreUrl); } else { history.push(props.exploreUrl); } diff --git a/superset-frontend/src/dashboard/components/URLShortLinkButton/URLShortLinkButton.test.tsx b/superset-frontend/src/dashboard/components/URLShortLinkButton/URLShortLinkButton.test.tsx index fe4d92cea0d..326a3bd3aa5 100644 --- a/superset-frontend/src/dashboard/components/URLShortLinkButton/URLShortLinkButton.test.tsx +++ b/superset-frontend/src/dashboard/components/URLShortLinkButton/URLShortLinkButton.test.tsx @@ -26,6 +26,9 @@ const PERMALINK_PAYLOAD = { key: '123', url: 'http://fakeurl.com/123', }; +// rewritePermalinkOrigin substitutes window.location.origin (jsdom: http://localhost) +// for the permalink's origin while preserving the path. See urlUtils.ts. +const REWRITTEN_URL = `${window.location.origin}/123`; const FILTER_STATE_PAYLOAD = { value: '{}', }; @@ -58,9 +61,7 @@ test('renders overlay on click', async () => { test('obtains short url', async () => { render(, { useRedux: true }); userEvent.click(screen.getByRole('button')); - expect(await screen.findByRole('tooltip')).toHaveTextContent( - PERMALINK_PAYLOAD.url, - ); + expect(await screen.findByRole('tooltip')).toHaveTextContent(REWRITTEN_URL); }); test('creates email anchor', async () => { @@ -78,7 +79,7 @@ test('creates email anchor', async () => { }, ); - const href = `mailto:?Subject=${subject}%20&Body=${content}${PERMALINK_PAYLOAD.url}`; + const href = `mailto:?Subject=${subject}%20&Body=${content}${REWRITTEN_URL}`; userEvent.click(screen.getByRole('button')); expect(await screen.findByRole('link')).toHaveAttribute('href', href); }); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx new file mode 100644 index 00000000000..c26c807dd5e --- /dev/null +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx @@ -0,0 +1,146 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { FC } from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; + +// Tab.tsx is statically imported below; the mock pattern intercepts +// applicationRoot() rather than relying on withApplicationRoot (which is for +// dynamic-import unit tests). + +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + default: actual.default, + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: actual.staticAssetsPrefix, + }; +}); + +jest.mock('src/dashboard/util/getChartIdsFromComponent', () => + jest.fn(() => []), +); +jest.mock('src/dashboard/containers/DashboardComponent', () => + jest.fn(() =>
), +); +jest.mock('@superset-ui/core/components/EditableTitle', () => ({ + __esModule: true, + EditableTitle: jest.fn(() =>
), +})); +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + ...jest.requireActual('src/dashboard/components/dnd/DragDroppable'), + Droppable: jest.fn(props => ( +
{props.children ? props.children({}) : null}
+ )), +})); + +// eslint-disable-next-line import/first +import ActualTab from './Tab'; + +const Tab = ActualTab as unknown as FC>; + +const DASHBOARD_ID = 23; + +const buildProps = () => ({ + id: 'TAB-empty-', + parentId: 'TABS-empty-', + depth: 2, + index: 0, + renderType: 'RENDER_TAB_CONTENT', + availableColumnCount: 12, + columnWidth: 120, + isFocused: false, + component: { + children: [], + id: 'TAB-empty-', + meta: { text: 'Empty Tab' }, + parents: ['ROOT_ID', 'GRID_ID', 'TABS-empty-'], + type: 'TAB', + }, + parentComponent: { + children: ['TAB-empty-'], + id: 'TABS-empty-', + meta: {}, + parents: ['ROOT_ID', 'GRID_ID'], + type: 'TABS', + }, + editMode: true, + embeddedMode: false, + undoLength: 0, + redoLength: 0, + filters: {}, + directPathToChild: [], + directPathLastUpdated: 0, + dashboardId: DASHBOARD_ID, + focusedFilterScope: null, + isComponentVisible: true, + onDropOnTab: jest.fn(), + handleComponentDrop: jest.fn(), + updateComponents: jest.fn(), + setDirectPathToChild: jest.fn(), + onResizeStart: jest.fn(), + onResize: jest.fn(), + onResizeStop: jest.fn(), +}); + +const renderEmptyEditModeTab = () => + render(, { + useRedux: true, + useDnd: true, + initialState: { + dashboardInfo: { dash_edit_perm: true }, + }, + }); + +beforeEach(() => { + mockApplicationRoot.mockReturnValue(''); +}); + +test('Tab — empty edit-mode "create a new chart" link is unprefixed when application root is empty', () => { + mockApplicationRoot.mockReturnValue(''); + renderEmptyEditModeTab(); + + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', `/chart/add?dashboard_id=${DASHBOARD_ID}`); +}); + +test('Tab — empty edit-mode "create a new chart" link carries the application root under subdirectory deployment', () => { + mockApplicationRoot.mockReturnValue('/superset'); + renderEmptyEditModeTab(); + + // Single prefix — not /superset/superset/ — verifying ensureAppRoot's + // dedupe boundary holds against the path's leading slash. + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', `/superset/chart/add?dashboard_id=${DASHBOARD_ID}`); +}); + +test('Tab — empty edit-mode "create a new chart" link prefixes correctly for nested subdirectory roots', () => { + mockApplicationRoot.mockReturnValue('/a/b/c'); + renderEmptyEditModeTab(); + + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', `/a/b/c/chart/add?dashboard_id=${DASHBOARD_ID}`); +}); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx index 23e5a5deedd..2815007eee6 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx @@ -27,6 +27,7 @@ import { import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import { EditableTitle } from '@superset-ui/core/components'; import { setEditMode, onRefresh } from 'src/dashboard/actions/dashboardState'; +import * as getBootstrapData from 'src/utils/getBootstrapData'; import type { FC } from 'react'; import ActualTab from './Tab'; @@ -488,6 +489,36 @@ test('Render tab content with no children, editMode: true, canEdit: true', () => ).toHaveAttribute('href', '/chart/add?dashboard_id=23'); }); +test('empty-tab "create a new chart" link is single-prefixed under subdirectory deployment', () => { + // The empty-tab CTA composes the chart-add URL via ensureAppRoot. Under + // SUPERSET_APP_ROOT=/superset the rendered href must be exactly + // `/superset/chart/add?dashboard_id=23` — not `/chart/add?…` (no prefix) + // and not `/superset/superset/chart/add?…` (double prefix). The link uses + // target="_blank", so basename routing does NOT re-apply the prefix. + const applicationRootSpy = jest + .spyOn(getBootstrapData, 'applicationRoot') + .mockReturnValue('/superset'); + + try { + const props = createProps(); + props.editMode = true; + props.component.children = []; + render(, { + useRedux: true, + useDnd: true, + initialState: { + dashboardInfo: { dash_edit_perm: true }, + }, + }); + + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', '/superset/chart/add?dashboard_id=23'); + } finally { + applicationRootSpy.mockRestore(); + } +}); + test('Drag to empty state, editMode: true, canEdit: true', async () => { const props = createProps(); props.editMode = true; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx index f2486a80dc8..9bdbf9221c2 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx @@ -36,6 +36,7 @@ import getChartIdsFromComponent from 'src/dashboard/util/getChartIdsFromComponen import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import AnchorLink from 'src/dashboard/components/AnchorLink'; import { Typography } from '@superset-ui/core/components/Typography'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; import { useIsAutoRefreshing, useIsRefreshInFlight, @@ -333,7 +334,9 @@ const Tab = (props: TabProps): ReactElement => { {t('You can')}{' '} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.subdirectory.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.subdirectory.test.ts new file mode 100644 index 00000000000..212939941d8 --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.subdirectory.test.ts @@ -0,0 +1,207 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// `FilterBar/index.tsx::publishDataMask` is one of the five sanctioned +// `applicationRoot()` callers (memory `project_supersetclient_approot_dedupe`). +// It runs after a filter mutation to push the updated filter cache key into +// the URL via `history.replace`. Two appRoot-aware operations gate that +// replace: +// +// 1. The path-matching guard — only fire when the current pathname is a +// dashboard route under the configured appRoot. The bug class this +// catches is "filter writes pollute Explore's URL after navigation". +// +// 2. The prefix-strip — React Router applies `basename` internally, so +// `history.replace({ pathname })` must receive a path WITHOUT the +// appRoot. The bug class this catches is `/superset/superset/dashboard/...` +// in the URL bar after the first filter change. +// +// `publishDataMask` is module-private (declared as a `const debounce(...)`). +// Testing it through a rendered FilterBar requires the Redux store, the +// filter cache API, and the debounce timer — heavyweight relative to what +// the contract actually says. Instead this test does two things: +// +// A. Reads FilterBar/index.tsx as source and pins the two patterns that +// embody the contract. A future refactor that drops the guard or the +// strip fails here loudly with the exact line that drifted. +// B. Tests the *equivalent* pure logic (re-implementation of the same +// pattern) across every appRoot × pathname × Explore-vs-Dashboard +// input shape that matters in practice. If the actual code drifts +// from the documented invariant, the source-pin in (A) fires; if the +// documented invariant itself is wrong, (B) fires. + +import { readFileSync } from 'fs'; +import { join } from 'path'; + +const FILTERBAR_SRC = readFileSync(join(__dirname, 'index.tsx'), 'utf8'); + +// --------------------------------------------------------------------------- +// (A) Source-pin: the two patterns that implement the contract. +// --------------------------------------------------------------------------- + +test('FilterBar/index.tsx guards history.replace by the configured app root', () => { + // The guard short-circuits the URL mutation when the current path is not a + // dashboard route under the appRoot — e.g. when the user navigated to + // Explore (`/explore/?slice_id=...`), the FilterBar's debounced commit must + // not stomp Explore's query string with native_filters_key. + expect(FILTERBAR_SRC).toContain( + 'window.location.pathname.startsWith(`${applicationRoot()}/dashboard`)', + ); +}); + +test('FilterBar/index.tsx strips the app root before history.replace', () => { + // Both halves of the strip survive together — the appRoot != "/" check + // and the startsWith-before-substring guard. Each is load-bearing on its + // own (without the first, root deploy hits `.substring(1)` and clips off + // the leading slash; without the second, paths that diverge from the + // appRoot get incorrectly truncated). + expect(FILTERBAR_SRC).toContain( + "if (appRoot !== '/' && replacementPathname.startsWith(appRoot))", + ); + expect(FILTERBAR_SRC).toContain( + 'replacementPathname = replacementPathname.substring(appRoot.length);', + ); +}); + +test('FilterBar/index.tsx imports applicationRoot from getBootstrapData', () => { + // Centralised symbol — the static-scan invariant in + // navigationUtils.invariants.test.ts enumerates the sanctioned import + // sites. If FilterBar's import path drifts, that scan also fires; this + // one anchors the import locally so a `git blame` on FilterBar tells the + // story without needing to cross-reference the scan ledger. + expect(FILTERBAR_SRC).toMatch( + /import\s+\{\s*applicationRoot\s*\}\s+from\s+'src\/utils\/getBootstrapData'/, + ); +}); + +// --------------------------------------------------------------------------- +// (B) Characterisation: the documented invariant, exercised across the +// appRoot × pathname matrix. +// --------------------------------------------------------------------------- +// +// Re-implementation of the FilterBar guard + strip, kept here so the test +// can fail loudly if the *invariant itself* is wrong (rather than a typo in +// the implementation). The source-pin above catches the inverse case +// (implementation drifts away from the invariant). + +interface Scenario { + description: string; + appRoot: string; + pathname: string; + shouldReplace: boolean; + replacementPathname?: string; +} + +function applyFilterBarPathLogic( + appRoot: string, + pathname: string, +): { shouldReplace: boolean; replacementPathname?: string } { + if (!pathname.startsWith(`${appRoot}/dashboard`)) { + return { shouldReplace: false }; + } + let replacement = pathname; + if (appRoot !== '/' && replacement.startsWith(appRoot)) { + replacement = replacement.substring(appRoot.length); + } + return { shouldReplace: true, replacementPathname: replacement }; +} + +const SCENARIOS: ReadonlyArray = [ + // Root deploy — pathname matches `/dashboard` directly. + { + description: 'root deploy on a dashboard page', + appRoot: '', + pathname: '/dashboard/1/', + shouldReplace: true, + replacementPathname: '/dashboard/1/', + }, + { + description: 'root deploy on Explore — guard short-circuits', + appRoot: '', + pathname: '/explore/?slice_id=42', + shouldReplace: false, + }, + // Subdir deploy — appRoot is `/superset`, pathname carries the prefix. + { + description: 'subdir deploy on a dashboard page', + appRoot: '/superset', + pathname: '/superset/dashboard/2/', + shouldReplace: true, + // Stripped: React Router re-applies basename so the strip MUST happen. + replacementPathname: '/dashboard/2/', + }, + { + description: 'subdir deploy on a dashboard permalink', + appRoot: '/superset', + pathname: '/superset/dashboard/p/abc123/', + shouldReplace: true, + replacementPathname: '/dashboard/p/abc123/', + }, + { + description: 'subdir deploy on Explore — guard short-circuits', + appRoot: '/superset', + pathname: '/superset/explore/?slice_id=7', + shouldReplace: false, + }, + { + description: + 'subdir deploy on bare app root (no /dashboard) — short-circuits', + appRoot: '/superset', + pathname: '/superset/', + shouldReplace: false, + }, + // Operator deploy under a deeply nested basename. + { + description: 'deep-nested deploy on a dashboard page', + appRoot: '/tenant-a/superset', + pathname: '/tenant-a/superset/dashboard/9/', + shouldReplace: true, + replacementPathname: '/dashboard/9/', + }, + // Adversarial: appRoot `/dash` is a substring of `/dashboard`. The guard + // template is `${appRoot}/dashboard` so the prefix is `/dash/dashboard`, + // which (correctly) does NOT match a bare `/dashboard/1/` path. Pin the + // case so a maintainer doesn't "fix" the guard to also match prefix-free + // paths (which would re-introduce the Explore-stomp regression for + // operators whose root happens to share characters with `/dashboard`). + { + description: + 'appRoot is a substring prefix of /dashboard — guard does NOT match a bare /dashboard path', + appRoot: '/dash', + pathname: '/dashboard/5/', + shouldReplace: false, + }, +]; + +test.each(SCENARIOS)( + 'publishDataMask path logic: $description', + ({ appRoot, pathname, shouldReplace, replacementPathname }: Scenario) => { + const result = applyFilterBarPathLogic(appRoot, pathname); + expect(result.shouldReplace).toBe(shouldReplace); + if (shouldReplace) { + expect(result.replacementPathname).toBe(replacementPathname); + // The dedupe contract: no `/superset/superset/...` ever reaches React + // Router. Even if the source-pin drifts, this catches the user-visible + // symptom. + expect(result.replacementPathname).not.toMatch(/\/superset\/superset\//); + } else { + expect(result.replacementPathname).toBeUndefined(); + } + }, +); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx index ef9f3dc69a1..dcb8b0532bd 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx @@ -142,9 +142,11 @@ const publishDataMask = debounce( // pathname could be updated somewhere else through window.history // keep react router history in sync with window history - // replace params only when current page is /superset/dashboard + // replace params only when current page is a dashboard route under the + // configured applicationRoot (e.g. `/dashboard/...` for root deploy, + // `/superset/dashboard/...` for the legacy subdir deploy). // this prevents a race condition between updating filters and navigating to Explore - if (window.location.pathname.includes('/superset/dashboard')) { + if (window.location.pathname.startsWith(`${applicationRoot()}/dashboard`)) { // The history API is part of React router and understands that a basename may exist. // Internally it treats all paths as if they are relative to the root and appends // it when necessary. We strip any prefix so that history.replace adds it back and doesn't diff --git a/superset-frontend/src/dashboard/util/risonFilters.ts b/superset-frontend/src/dashboard/util/risonFilters.ts index d9b4ead12a0..9aa1678c21e 100644 --- a/superset-frontend/src/dashboard/util/risonFilters.ts +++ b/superset-frontend/src/dashboard/util/risonFilters.ts @@ -23,6 +23,7 @@ import { DataMaskStateWithId, } from '@superset-ui/core'; import rison from 'rison'; +import { navigateWithState } from 'src/utils/navigationUtils'; /** * Synthetic dataMask key for URL Rison filters that don't match any native @@ -238,7 +239,17 @@ export function prettifyRisonFilterUrl(): void { const prettifiedUrl = `${beforeRison}${separator}f=${risonValue}${afterRison}`; if (prettifiedUrl !== currentUrl) { - window.history.replaceState(window.history.state, '', prettifiedUrl); + // Route through navigateWithState so the navigationUtils guards + // (`assertSafeNavigationUrl` scheme/userinfo barriers + the + // CodeQL-recognised inline sanitisers) apply at the + // `window.history.replaceState` sink. The URL constructor inside + // `navigateWithState` is conservative about re-encoding: sub-delims + // like `(`, `)`, `:`, `!` (the meaningful Rison glyphs) survive, + // so the prettification's visual win is preserved for every + // character the prettifier actually targets. + navigateWithState(prettifiedUrl, window.history.state ?? {}, { + replace: true, + }); } } catch (error) { console.warn('Failed to prettify Rison URL:', error); @@ -351,12 +362,11 @@ export function updateUrlWithUnmatchedFilters( // With a real `BrowserRouter`, `history.replace` would do this too — but // under a `createMemoryHistory` (used in tests, or in some embedded // contexts) it does not, and we'd leak the stale URL into the next - // `getRisonFilterParam()` call. - window.history.replaceState( - window.history.state, - '', - currentUrl.toString(), - ); + // `getRisonFilterParam()` call. Routed through navigateWithState so the + // navigationUtils scheme/userinfo barriers gate the sink. + navigateWithState(currentUrl.toString(), window.history.state ?? {}, { + replace: true, + }); if (history) { history.replace({ pathname: currentUrl.pathname, diff --git a/superset-frontend/src/explore/components/EmbedCodeContent.subdirectory.test.tsx b/superset-frontend/src/explore/components/EmbedCodeContent.subdirectory.test.tsx new file mode 100644 index 00000000000..69674314d77 --- /dev/null +++ b/superset-frontend/src/explore/components/EmbedCodeContent.subdirectory.test.tsx @@ -0,0 +1,78 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import fetchMock from 'fetch-mock'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import EmbedCodeContent from 'src/explore/components/EmbedCodeContent'; + +// The chart-embed iframe `src` is produced by: +// 1. Backend `url_for(_external=True)` → absolute URL whose origin is the +// backend `Host` header (often the internal docker hostname under +// `superset-light:8088` when `ENABLE_PROXY_FIX` is off). +// 2. Frontend `rewritePermalinkOrigin` swaps the origin for +// `window.location.origin` so the iframe `src` is reachable from the +// browser that pasted the embed code. +// 3. The path segment (`/superset/explore/p//`) survives unchanged — +// the application_root must therefore be applied exactly once. +// +// This composition was previously verified only via manual QA +// (memory `project_supersetclient_approot_dedupe` records the discovery). +// This test pins the iframe-src shape so a future change to the permalink +// API, the origin-rewrite helper, or the EmbedCodeContent template would +// surface in CI rather than in a user-reported broken embed. + +const SUBDIR_PERMALINK_URL = + 'http://superset-light:8088/superset/explore/p/abc123/'; + +fetchMock.post('glob:*/api/v1/explore/permalink', { + url: SUBDIR_PERMALINK_URL, +}); + +const mockFormData = { + datasource: 'table__1', + viz_type: 'table', +}; + +test('iframe src under subdir deployment uses browser origin + single prefix', async () => { + render(, { useRedux: true }); + + // The textarea `value` contains the full iframe HTML once the permalink + // promise resolves. `data-test="embed-code-textarea"` is the stable hook. + const textarea = await screen.findByTestId('embed-code-textarea'); + + // Wait for the asynchronous permalink fetch to land in the textarea. + await waitFor(() => + expect((textarea as HTMLTextAreaElement).value).toContain(' ( {t('Add an annotation layer')}{' '} ` sink. The string fed in is a + // URL-normalised path (`/seg/seg`) so encodeURI is idempotent in + // practice — it does not alter the navigation target. + href={encodeURI(ensureAppRoot('/annotationlayer/list'))} target="_blank" rel="noopener noreferrer" > diff --git a/superset-frontend/src/explore/components/controls/ViewQuery.test.tsx b/superset-frontend/src/explore/components/controls/ViewQuery.test.tsx index 5fd67dafa07..e1b5c50c24b 100644 --- a/superset-frontend/src/explore/components/controls/ViewQuery.test.tsx +++ b/superset-frontend/src/explore/components/controls/ViewQuery.test.tsx @@ -185,6 +185,7 @@ test('opens SQL Lab in a new tab when View in SQL Lab button is clicked with met expect(window.open).toHaveBeenCalledWith( `/sqllab?datasourceKey=${datasource}&sql=${encodeURIComponent(sql)}`, '_blank', + 'noopener noreferrer', ); }); diff --git a/superset-frontend/src/explore/components/controls/ViewQuery.tsx b/superset-frontend/src/explore/components/controls/ViewQuery.tsx index 1d321d766e3..0befdd9cd82 100644 --- a/superset-frontend/src/explore/components/controls/ViewQuery.tsx +++ b/superset-frontend/src/explore/components/controls/ViewQuery.tsx @@ -40,7 +40,7 @@ import { import { CopyToClipboard } from 'src/components'; import { RootState } from 'src/dashboard/types'; import { findPermission } from 'src/utils/findPermission'; -import { makeUrl } from 'src/utils/pathUtils'; +import { openInNewTab } from 'src/utils/navigationUtils'; import CodeSyntaxHighlighter, { SupportedLanguage, preloadLanguages, @@ -140,11 +140,8 @@ const ViewQuery: FC = props => { }; if (domEvent.metaKey || domEvent.ctrlKey) { domEvent.preventDefault(); - window.open( - makeUrl( - `/sqllab?datasourceKey=${datasource}&sql=${encodeURIComponent(currentSQL)}`, - ), - '_blank', + openInNewTab( + `/sqllab?datasourceKey=${datasource}&sql=${encodeURIComponent(currentSQL)}`, ); } else { history.push({ pathname: '/sqllab', state: { requestedQuery } }); diff --git a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx index 2fdd4e29bee..d1f8ff69902 100644 --- a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx +++ b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx @@ -39,7 +39,7 @@ const TestDashboardsMenuItems = ({
{menuItems.map(item => (
- {typeof item.label === 'string' ? item!.label : 'Complex Label'} + {item!.label} {item!.disabled && disabled}
))} @@ -173,6 +173,35 @@ describe('DashboardsSubMenu', () => { expect(screen.getByTestId('menu-item-5')).toBeInTheDocument(); }); + test('renders SPA-relative dashboard links without the /superset/ prefix', () => { + // Regression: prior to the route_base="" alignment, this menu emitted + // `to="/superset/dashboard/"` which, combined with the React Router + // `basename={applicationRoot()}`, produced a doubled `/superset/superset/` + // path on subdirectory deployments and a backend 404. + const dashboards = [{ id: 9, dashboard_title: 'Sales Dashboard' }]; + render( + , + { useRouter: true }, + ); + + const link = screen.getByRole('link', { name: /Sales Dashboard/ }); + expect(link).toHaveAttribute('href', '/dashboard/9?focused_chart=102'); + }); + + test('omits the focused_chart query when chartId is undefined', () => { + const dashboards = [{ id: 9, dashboard_title: 'Sales Dashboard' }]; + render(, { + useRouter: true, + }); + + const link = screen.getByRole('link', { name: /Sales Dashboard/ }); + expect(link).toHaveAttribute('href', '/dashboard/9'); + }); + test('partial string search works correctly', () => { const dashboards = [ { id: 1, dashboard_title: 'Revenue Report' }, diff --git a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx index fcd2454615c..00b4a8a5f4f 100644 --- a/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx +++ b/superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx @@ -71,8 +71,8 @@ export const useDashboardsMenuItems = ({ label: ( { force: false, curUrl: 'http://superset.com', }); - compareURI(URI(url!), URI('/superset/explore_json/')); + compareURI(URI(url!), URI('/explore_json/')); }); test('generates proper json forced url', () => { const url = getExploreUrl({ @@ -75,10 +75,7 @@ describe('exploreUtils', () => { force: true, curUrl: 'superset.com', }); - compareURI( - URI(url!), - URI('/superset/explore_json/').search({ force: 'true' }), - ); + compareURI(URI(url!), URI('/explore_json/').search({ force: 'true' })); }); test('generates proper csv URL', () => { const url = getExploreUrl({ @@ -87,10 +84,7 @@ describe('exploreUtils', () => { force: false, curUrl: 'superset.com', }); - compareURI( - URI(url!), - URI('/superset/explore_json/').search({ csv: 'true' }), - ); + compareURI(URI(url!), URI('/explore_json/').search({ csv: 'true' })); }); test('generates proper standalone URL', () => { const url = getExploreUrl({ @@ -113,10 +107,7 @@ describe('exploreUtils', () => { force: false, curUrl: 'superset.com?foo=bar', }); - compareURI( - URI(url!), - URI('/superset/explore_json/').search({ foo: 'bar' }), - ); + compareURI(URI(url!), URI('/explore_json/').search({ foo: 'bar' })); }); test('generate proper save slice url', () => { const url = getExploreUrl({ @@ -125,10 +116,7 @@ describe('exploreUtils', () => { force: false, curUrl: 'superset.com?foo=bar', }); - compareURI( - URI(url!), - URI('/superset/explore_json/').search({ foo: 'bar' }), - ); + compareURI(URI(url!), URI('/explore_json/').search({ foo: 'bar' })); }); }); diff --git a/superset-frontend/src/explore/exploreUtils/exportChart.test.ts b/superset-frontend/src/explore/exploreUtils/exportChart.test.ts index 9ec504bdb4f..3311e92c761 100644 --- a/superset-frontend/src/explore/exploreUtils/exportChart.test.ts +++ b/superset-frontend/src/explore/exploreUtils/exportChart.test.ts @@ -201,9 +201,12 @@ test('exportChart legacy API (useLegacyApi=true) passes prefixed URL to onStartS expect(onStartStreamingExport).toHaveBeenCalledTimes(1); const callArgs = onStartStreamingExport.mock.calls[0][0]; - // The legacy blueprint path is /superset/explore_json/; with appRoot=/superset the - // full streaming URL is /superset/superset/explore_json/ (appRoot + blueprint prefix). - expect(callArgs.url).toBe('/superset/superset/explore_json/?csv=true'); + // post `Superset.route_base = ""`, the blueprint path is + // `/explore_json/`. With appRoot=/superset, the prefixed URL is + // `/superset/explore_json/?csv=true` — single-prefix, not the legacy + // doubled `/superset/superset/explore_json/...` that this test used to + // pin (which was the bug, now fixed at source). + expect(callArgs.url).toBe('/superset/explore_json/?csv=true'); expect(callArgs.exportType).toBe('csv'); }); @@ -228,7 +231,7 @@ test('exportChart legacy API builds relative URL for CSV export without app root expect(onStartStreamingExport).toHaveBeenCalledTimes(1); const callArgs = onStartStreamingExport.mock.calls[0][0]; - expect(callArgs.url).toBe('/superset/explore_json/?csv=true'); + expect(callArgs.url).toBe('/explore_json/?csv=true'); }); test('exportChart legacy API builds relative URL for xlsx export', async () => { @@ -253,7 +256,7 @@ test('exportChart legacy API builds relative URL for xlsx export', async () => { expect(onStartStreamingExport).toHaveBeenCalledTimes(1); const callArgs = onStartStreamingExport.mock.calls[0][0]; - expect(callArgs.url).toBe('/superset/explore_json/?xlsx=true'); + expect(callArgs.url).toBe('/explore_json/?xlsx=true'); }); test('exportChart legacy API calls postBlob with relative URL', async () => { @@ -278,7 +281,7 @@ test('exportChart legacy API calls postBlob with relative URL', async () => { expect(SupersetClient.postBlob).toHaveBeenCalledTimes(1); const [url] = SupersetClient.postBlob.mock.calls[0]; - expect(url).toBe('/superset/explore_json/?csv=true'); + expect(url).toBe('/explore_json/?csv=true'); expect(url).not.toMatch(/^https?:\/\//); expect(downloadBlob).toHaveBeenCalled(); }); @@ -305,7 +308,7 @@ test('exportChart legacy API includes force param when force=true', async () => expect(onStartStreamingExport).toHaveBeenCalledTimes(1); const callArgs = onStartStreamingExport.mock.calls[0][0]; - expect(callArgs.url).toBe('/superset/explore_json/?force=true&csv=true'); + expect(callArgs.url).toBe('/explore_json/?force=true&csv=true'); }); test('exportChart successfully exports chart as CSV', async () => { diff --git a/superset-frontend/src/explore/exploreUtils/getExploreUrl.test.ts b/superset-frontend/src/explore/exploreUtils/getExploreUrl.test.ts index 032133545e8..782d4412863 100644 --- a/superset-frontend/src/explore/exploreUtils/getExploreUrl.test.ts +++ b/superset-frontend/src/explore/exploreUtils/getExploreUrl.test.ts @@ -39,7 +39,7 @@ test('Get ExploreUrl with default params', () => { test('Get ExploreUrl with endpointType:full', () => { const params = createParams(); expect(getExploreUrl({ ...params, endpointType: 'full' })).toBe( - 'http://localhost/superset/explore_json/', + 'http://localhost/explore_json/', ); }); @@ -47,21 +47,21 @@ test('Get ExploreUrl with endpointType:full and method:GET', () => { const params = createParams(); expect( getExploreUrl({ ...params, endpointType: 'full', method: 'GET' }), - ).toBe('http://localhost/superset/explore_json/'); + ).toBe('http://localhost/explore_json/'); }); test('Get relative ExploreUrl with endpointType:csv', () => { const params = createParams(); expect( getExploreUrl({ ...params, endpointType: 'csv', relative: true }), - ).toBe('/superset/explore_json/?csv=true'); + ).toBe('/explore_json/?csv=true'); }); test('Get relative ExploreUrl with endpointType:xlsx', () => { const params = createParams(); expect( getExploreUrl({ ...params, endpointType: 'xlsx', relative: true }), - ).toBe('/superset/explore_json/?xlsx=true'); + ).toBe('/explore_json/?xlsx=true'); }); test('Get relative ExploreUrl with force:true', () => { @@ -73,7 +73,7 @@ test('Get relative ExploreUrl with force:true', () => { force: true, relative: true, }), - ).toBe('/superset/explore_json/?force=true&csv=true'); + ).toBe('/explore_json/?force=true&csv=true'); }); test('Get relative ExploreUrl with endpointType:base', () => { diff --git a/superset-frontend/src/explore/exploreUtils/getURIDirectory.test.ts b/superset-frontend/src/explore/exploreUtils/getURIDirectory.test.ts index 1d6ad40045a..7d3b004cd4d 100644 --- a/superset-frontend/src/explore/exploreUtils/getURIDirectory.test.ts +++ b/superset-frontend/src/explore/exploreUtils/getURIDirectory.test.ts @@ -19,8 +19,12 @@ import { getURIDirectory } from '.'; test('Cases in which the "explore_json" will be returned', () => { + // post `Superset.route_base = ""` collapse the legacy + // `/superset/explore_json/` endpoint is gone; `getURIDirectory` now + // returns the bare `/explore_json/` path (appRoot is applied at the + // outer `ensureAppRoot` layer when `includeAppRoot=true`). ['full', 'json', 'csv', 'query', 'results', 'samples'].forEach(name => { - expect(getURIDirectory(name)).toBe('/superset/explore_json/'); + expect(getURIDirectory(name)).toBe('/explore_json/'); }); }); diff --git a/superset-frontend/src/explore/exploreUtils/index.ts b/superset-frontend/src/explore/exploreUtils/index.ts index 10c70165314..3e1cd406fbe 100644 --- a/superset-frontend/src/explore/exploreUtils/index.ts +++ b/superset-frontend/src/explore/exploreUtils/index.ts @@ -33,7 +33,7 @@ import { import { availableDomains } from 'src/utils/hostNamesConfig'; import { safeStringify } from 'src/utils/safeStringify'; import { optionLabel } from 'src/utils/common'; -import { ensureAppRoot } from 'src/utils/pathUtils'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; import { downloadBlob, getFilenameFromResponse } from 'src/utils/export'; import { URL_PARAMS } from 'src/constants'; import { @@ -166,7 +166,7 @@ export function getURIDirectory( 'results', 'samples', ].includes(endpointType) - ? '/superset/explore_json/' + ? '/explore_json/' : '/explore/'; return includeAppRoot ? ensureAppRoot(uri) : uri; } diff --git a/superset-frontend/src/extensions/ExtensionsLoader.test.ts b/superset-frontend/src/extensions/ExtensionsLoader.test.ts index d2debd6d69c..a02e2515e33 100644 --- a/superset-frontend/src/extensions/ExtensionsLoader.test.ts +++ b/superset-frontend/src/extensions/ExtensionsLoader.test.ts @@ -23,6 +23,19 @@ import ExtensionsLoader from './ExtensionsLoader'; type Extension = core.Extension; +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + ...actual, + applicationRoot: () => mockApplicationRoot(), + }; +}); + function createMockExtension(overrides: Partial = {}): Extension { return { id: 'test-extension', @@ -37,8 +50,39 @@ function createMockExtension(overrides: Partial = {}): Extension { beforeEach(() => { (ExtensionsLoader as any).instance = undefined; + mockApplicationRoot.mockReturnValue(''); }); +/** + * Capture the src attribute of the remote-entry script element and trigger + * its onerror handler so loadModule short-circuits without webpack module + * federation globals. + */ +function captureRemoteEntryScript(): { + getSrc: () => string | null; + restore: () => void; +} { + let capturedSrc: string | null = null; + const appendChildSpy = jest + .spyOn(document.head, 'appendChild') + .mockImplementation((element: Node) => { + if (element instanceof HTMLScriptElement) { + capturedSrc = element.getAttribute('src'); + if (element.onerror) { + const errorHandler = element.onerror; + setTimeout(() => { + errorHandler('Script load halted by test'); + }, 0); + } + } + return element; + }); + return { + getSrc: () => capturedSrc, + restore: () => appendChildSpy.mockRestore(), + }; +} + test('creates a singleton instance', () => { const instance1 = ExtensionsLoader.getInstance(); const instance2 = ExtensionsLoader.getInstance(); @@ -126,6 +170,70 @@ test('logs success after initializeExtensions completes', async () => { infoSpy.mockRestore(); }); +// Subdirectory regression (gap review 2026-06-10): the backend emits a +// router-relative remoteEntry URL; assigning it raw to `script.src` resolved +// it against the domain root, 404ing every extension under a subdirectory +// deployment. +test('prefixes a router-relative remoteEntry with the application root', async () => { + mockApplicationRoot.mockReturnValue('/superset'); + const loader = ExtensionsLoader.getInstance(); + const errorSpy = jest.spyOn(logging, 'error').mockImplementation(); + const script = captureRemoteEntryScript(); + + await loader.initializeExtension( + createMockExtension({ + id: 'sub-ext', + remoteEntry: '/api/v1/extensions/pub/sub-ext/remoteEntry.js', + }), + ); + + expect(script.getSrc()).toBe( + '/superset/api/v1/extensions/pub/sub-ext/remoteEntry.js', + ); + + errorSpy.mockRestore(); + script.restore(); +}); + +test('leaves the remoteEntry unprefixed on root deployments', async () => { + const loader = ExtensionsLoader.getInstance(); + const errorSpy = jest.spyOn(logging, 'error').mockImplementation(); + const script = captureRemoteEntryScript(); + + await loader.initializeExtension( + createMockExtension({ + id: 'root-ext', + remoteEntry: '/api/v1/extensions/pub/root-ext/remoteEntry.js', + }), + ); + + expect(script.getSrc()).toBe( + '/api/v1/extensions/pub/root-ext/remoteEntry.js', + ); + + errorSpy.mockRestore(); + script.restore(); +}); + +test('passes an absolute remoteEntry URL through unchanged', async () => { + mockApplicationRoot.mockReturnValue('/superset'); + const loader = ExtensionsLoader.getInstance(); + const errorSpy = jest.spyOn(logging, 'error').mockImplementation(); + const script = captureRemoteEntryScript(); + + await loader.initializeExtension( + createMockExtension({ + id: 'cdn-ext', + remoteEntry: 'https://cdn.example.com/remoteEntry.js', + }), + ); + + expect(script.getSrc()).toBe('https://cdn.example.com/remoteEntry.js'); + + errorSpy.mockRestore(); + script.restore(); +}); + test('logs error when initializeExtensions fails', async () => { const loader = ExtensionsLoader.getInstance(); const errorSpy = jest.spyOn(logging, 'error').mockImplementation(); diff --git a/superset-frontend/src/extensions/ExtensionsLoader.ts b/superset-frontend/src/extensions/ExtensionsLoader.ts index 0b74ef9be86..f4abaf5fdbb 100644 --- a/superset-frontend/src/extensions/ExtensionsLoader.ts +++ b/superset-frontend/src/extensions/ExtensionsLoader.ts @@ -19,6 +19,7 @@ import { SupersetClient } from '@superset-ui/core'; import { logging } from '@apache-superset/core/utils'; import type { common as core } from '@apache-superset/core'; +import { makeUrl } from 'src/utils/navigationUtils'; type Extension = core.Extension; @@ -107,10 +108,12 @@ class ExtensionsLoader { private async loadModule(extension: Extension): Promise { const { remoteEntry, id } = extension; - // Load the remote entry script + // Load the remote entry script. The backend emits a router-relative + // remoteEntry URL; `makeUrl` applies the application root for + // subdirectory deployments (idempotent, and absolute URLs pass through). await new Promise((resolve, reject) => { const element = document.createElement('script'); - element.src = remoteEntry; + element.src = makeUrl(remoteEntry); element.type = 'text/javascript'; element.async = true; element.onload = () => resolve(); diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index df18f3a0e8e..67cf34e1b98 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -1427,7 +1427,7 @@ const AlertReportModal: FunctionComponent = ({ const openDashboardInNewTab = (dashboardId?: number | string | null) => { if (!dashboardId) return; - navigateTo(`/superset/dashboard/${dashboardId}`, { newWindow: true }); + navigateTo(`/dashboard/${dashboardId}/`, { newWindow: true }); }; const onChartChange = (chart: SelectValue) => { diff --git a/superset-frontend/src/features/allEntities/AllEntitiesTable.subdirectory.test.tsx b/superset-frontend/src/features/allEntities/AllEntitiesTable.subdirectory.test.tsx new file mode 100644 index 00000000000..8573e0b52fb --- /dev/null +++ b/superset-frontend/src/features/allEntities/AllEntitiesTable.subdirectory.test.tsx @@ -0,0 +1,152 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render } from 'spec/helpers/testing-library'; +import type { TaggedObjects } from 'src/types/TaggedObject'; +import AllEntitiesTable from './AllEntitiesTable'; + +// Regression for the tag list page (sc-108439): backend `.url` properties on +// Dashboard / Slice / SavedQuery are router-relative by contract +// (see `superset/models/dashboard.py` `dashboard_link` docstring: "`Dashboard.url` +// itself stays router-relative so frontend callers can apply ensureAppRoot +// exactly once"). The TagDAO emits those router-relative paths verbatim as +// `o.url` on tagged-object responses. AllEntitiesTable therefore must wrap +// `o.url` with `ensureAppRoot` exactly once; otherwise the row hrefs lack +// the `SUPERSET_APP_ROOT` prefix and clicks land on broken links. +// +// Mocking note (same as SliceHeaderControls.subdirectory.test.tsx): the +// component is imported statically, so the `withApplicationRoot` fixture's +// `jest.resetModules()` + dynamic re-import pattern can't retroactively +// change which `getBootstrapData` instance the already-bound module graph +// sees. We mock `src/utils/getBootstrapData` directly with a reconfigurable +// `mockApplicationRoot` factory and flip it per scenario. +// +// Name must start with `mock` so Jest's hoisted jest.mock() factory may +// reference it. `default` returns a STATIC shape (not mockApplicationRoot) +// because consumers like the reducer chain call getBootstrapData() at +// import time — calling mockApplicationRoot inside `default` hits TDZ. + +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => ({ + __esModule: true, + default: () => ({ + common: { application_root: '', static_assets_prefix: '' }, + }), + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: () => '', +})); + +const BACKEND_URLS = { + dashboard: '/dashboard/sales/', + chart: '/explore/?slice_id=42', + query: '/sqllab?savedQueryId=7', +}; + +const SAMPLE_OBJECTS: TaggedObjects = { + dashboard: [ + { + id: 1, + type: 'dashboard', + name: 'Sales', + url: BACKEND_URLS.dashboard, + changed_on: '2026-06-01T00:00:00Z', + created_by: 1, + creator: 'admin', + owners: [], + tags: [], + }, + ], + chart: [ + { + id: 42, + type: 'chart', + name: 'Top Customers', + url: BACKEND_URLS.chart, + changed_on: '2026-06-01T00:00:00Z', + created_by: 1, + creator: 'admin', + owners: [], + tags: [], + }, + ], + query: [ + { + id: 7, + type: 'query', + name: 'Daily Revenue', + url: BACKEND_URLS.query, + changed_on: '2026-06-01T00:00:00Z', + created_by: 1, + creator: 'admin', + owners: [], + tags: [], + }, + ], +}; + +const renderAndCollectHrefs = (): string[] => { + const { container } = render( + {}} + canEditTag + />, + { useRedux: true, useTheme: true }, + ); + return Array.from(container.querySelectorAll('a[href]')) + .map(a => a.getAttribute('href') ?? '') + .filter(href => href !== '' && href !== '#'); +}; + +beforeEach(() => { + mockApplicationRoot.mockReset(); +}); + +test('row hrefs carry the application root under /superset', () => { + mockApplicationRoot.mockReturnValue('/superset'); + const hrefs = renderAndCollectHrefs(); + + expect(hrefs).toEqual( + expect.arrayContaining([ + `/superset${BACKEND_URLS.dashboard}`, + `/superset${BACKEND_URLS.chart}`, + `/superset${BACKEND_URLS.query}`, + ]), + ); + hrefs.forEach(href => { + expect(href).not.toMatch(/^\/superset\/superset\//); + expect(href.startsWith('/superset')).toBe(true); + }); +}); + +test('row hrefs are unchanged under the default root-of-domain deployment', () => { + mockApplicationRoot.mockReturnValue(''); + const hrefs = renderAndCollectHrefs(); + + expect(hrefs).toEqual( + expect.arrayContaining([ + BACKEND_URLS.dashboard, + BACKEND_URLS.chart, + BACKEND_URLS.query, + ]), + ); + hrefs.forEach(href => { + expect(href).not.toMatch(/^\/superset/); + }); +}); diff --git a/superset-frontend/src/features/allEntities/AllEntitiesTable.tsx b/superset-frontend/src/features/allEntities/AllEntitiesTable.tsx index 78a9f146c2f..2e02e2379f5 100644 --- a/superset-frontend/src/features/allEntities/AllEntitiesTable.tsx +++ b/superset-frontend/src/features/allEntities/AllEntitiesTable.tsx @@ -27,6 +27,7 @@ import { EmptyState } from '@superset-ui/core/components'; import { FacePile, TagsList, type TagType } from 'src/components'; import { TaggedObject, TaggedObjects } from 'src/types/TaggedObject'; import { Typography } from '@superset-ui/core/components/Typography'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; const MAX_TAGS_TO_SHOW = 3; const PAGE_SIZE = 10; @@ -73,7 +74,9 @@ export default function AllEntitiesTable({ const renderTable = (type: objectType) => { const data = objects[type].map((o: TaggedObject) => ({ - [type]: {o.name}, + [type]: ( + {o.name} + ), modified: o.changed_on ? extendedDayjs.utc(o.changed_on).fromNow() : '', tags: o.tags, owners: o.owners, diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseModal.subdirectory.test.ts b/superset-frontend/src/features/databases/DatabaseModal/DatabaseModal.subdirectory.test.ts new file mode 100644 index 00000000000..886cc9f861b --- /dev/null +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseModal.subdirectory.test.ts @@ -0,0 +1,160 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Subdirectory regression coverage for the DatabaseModal "post-connection" +// call-to-action buttons (Create dataset + Query data in SQL Lab). +// +// Original bug class: under a subdir deployment (`applicationRoot = '/superset'`) +// the "Query data in SQL Lab" button navigated to a double-prefixed URL +// (`/superset/superset/sqllab?db=true`). The fix routes both CTA buttons +// through `redirectURL`, which delegates to `useHistory().push(...)`. React +// Router's `BrowserRouter basename={applicationRoot()}` re-applies the prefix +// internally, so the argument to `history.push` MUST be a router-relative +// path (no leading `${applicationRoot}` and no `ensureAppRoot` wrap). +// +// Reaching `renderCTABtns()` through a rendered modal requires walking the +// full "select engine → fill SQLAlchemy form → submit → wait for success" +// flow with every fetch mocked. The contract under test is much smaller +// than that surface, so this file uses the same source-pin + pure-logic +// characterisation pattern as +// `dashboard/components/nativeFilters/FilterBar/FilterBar.subdirectory.test.ts`. + +import { readFileSync } from 'fs'; +import { join } from 'path'; + +const MODAL_SRC = readFileSync(join(__dirname, 'index.tsx'), 'utf8'); + +// --------------------------------------------------------------------------- +// Source-pin: redirectURL receives router-relative paths. +// --------------------------------------------------------------------------- + +test('DatabaseModal redirectURL delegates to history.push', () => { + // The redirectURL helper is the single funnel for post-connection + // navigation. Both CTA buttons call it; the only safe argument shape is + // a router-relative path, because BrowserRouter's basename re-applies the + // app root. Pinning the helper body here means a future refactor that + // re-introduces `applicationRoot()` or `ensureAppRoot` at this layer + // fails loudly with the exact callsite. + expect(MODAL_SRC).toMatch( + /const redirectURL = \(url: string\) => \{\s*history\.push\(url\);\s*\};/, + ); +}); + +test('DatabaseModal "Query data in SQL Lab" pushes a router-relative /sqllab', () => { + // The exact string we want in source. If someone "fixes" subdir support + // by wrapping this in ensureAppRoot or hard-coding `/superset/sqllab`, + // this test fires before the bad change ships. + expect(MODAL_SRC).toContain("redirectURL('/sqllab?db=true')"); +}); + +test('DatabaseModal "Create dataset" pushes a router-relative /dataset/add/', () => { + // Symmetric invariant for the sibling CTA button — same risk class + // (basename double-prefix) applies. Pinning prevents drift where someone + // "fixes" one button and leaves the other inconsistent. + expect(MODAL_SRC).toContain("redirectURL('/dataset/add/')"); +}); + +test('DatabaseModal CTA buttons do NOT prefix the app root themselves', () => { + // The buttons must not call applicationRoot()/ensureAppRoot/makeUrl on + // these specific paths — basename handles the prefix, and any extra + // prefixing produces `/superset/superset/...`. Search the renderCTABtns + // block (lines ~1855-1879 at time of writing) for offending patterns. + const ctaMatch = MODAL_SRC.match( + /const renderCTABtns = \(\) =>[\s\S]*?<\/StyledBtns>\s*\);/, + ); + expect(ctaMatch).not.toBeNull(); + const ctaSrc = ctaMatch![0]; + expect(ctaSrc).not.toMatch(/applicationRoot\s*\(/); + expect(ctaSrc).not.toMatch(/ensureAppRoot\s*\(/); + expect(ctaSrc).not.toMatch(/makeUrl\s*\(/); + // And the specific double-prefixed string the original bug produced — + // pin it so a regression that hard-codes the app root never ships. + expect(ctaSrc).not.toContain('/superset/sqllab'); + expect(ctaSrc).not.toContain('/superset/dataset/add'); +}); + +// --------------------------------------------------------------------------- +// Characterisation: the documented invariant exercised across app-roots. +// --------------------------------------------------------------------------- +// +// Re-implements the behaviour the source-pin describes: a router-relative +// path pushed through history under BrowserRouter's basename produces a +// single-prefixed URL. If the documented invariant itself is wrong the +// source-pin is useless; this matrix catches that. + +interface Scenario { + description: string; + basename: string; + pushArg: string; + expected: string; +} + +// Mirror of how React Router composes basename + pushed path. React Router +// requires the pushed path to start with '/' and concatenates basename in +// front (stripping a trailing slash on basename if present). +function composeUnderBasename(basename: string, pushArg: string): string { + const normalisedBase = + basename === '/' || basename === '' ? '' : basename.replace(/\/+$/, ''); + return `${normalisedBase}${pushArg}`; +} + +const SCENARIOS: ReadonlyArray = [ + { + description: 'root deploy: bare basename + /sqllab?db=true', + basename: '', + pushArg: '/sqllab?db=true', + expected: '/sqllab?db=true', + }, + { + description: 'subdir deploy: /superset + /sqllab?db=true', + basename: '/superset', + pushArg: '/sqllab?db=true', + expected: '/superset/sqllab?db=true', + }, + { + description: 'subdir deploy: /superset + /dataset/add/', + basename: '/superset', + pushArg: '/dataset/add/', + expected: '/superset/dataset/add/', + }, + { + description: 'deep-nested deploy: /tenant-a/superset + /sqllab?db=true', + basename: '/tenant-a/superset', + pushArg: '/sqllab?db=true', + expected: '/tenant-a/superset/sqllab?db=true', + }, + { + description: 'subdir deploy: trailing slash on basename collapses cleanly', + basename: '/superset/', + pushArg: '/sqllab?db=true', + expected: '/superset/sqllab?db=true', + }, +]; + +test.each(SCENARIOS)( + 'redirectURL navigation: $description', + ({ basename, pushArg, expected }: Scenario) => { + const url = composeUnderBasename(basename, pushArg); + expect(url).toBe(expected); + // The dedupe contract: no `/superset/superset/...` ever reaches the + // browser. Even if the source-pin drifts, this catches the user-visible + // symptom for the subdir case. + expect(url).not.toMatch(/\/superset\/superset\//); + }, +); diff --git a/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.subdirectory.test.tsx b/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.subdirectory.test.tsx new file mode 100644 index 00000000000..0215158a61e --- /dev/null +++ b/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.subdirectory.test.tsx @@ -0,0 +1,114 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import userEvent from '@testing-library/user-event'; +import { render, screen } from 'spec/helpers/testing-library'; + +// DatasetPanel opens the dataset via openInNewTab -> ensureAppRoot, which reads +// applicationRoot() statically. Intercept it to simulate a subdirectory deploy. +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + default: actual.default, + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: actual.staticAssetsPrefix, + }; +}); + +// eslint-disable-next-line import/first +import DatasetPanel from 'src/features/datasets/AddDataset/DatasetPanel/DatasetPanel'; +// eslint-disable-next-line import/first +import { exampleColumns } from './fixtures'; +// eslint-disable-next-line import/first +import { DatasetObject } from 'src/features/datasets/AddDataset/types'; + +const APP_ROOT = '/superset'; + +let openSpy: jest.SpyInstance; + +beforeEach(() => { + mockApplicationRoot.mockReturnValue(APP_ROOT); + openSpy = jest.spyOn(window, 'open').mockImplementation(() => null); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +const datasetWith = (exploreUrl: string): DatasetObject[] => [ + { + db: { + id: 1, + database_name: 'test_database', + owners: [1], + backend: 'test_backend', + }, + schema: 'test_schema', + dataset_name: 'example_dataset', + table_name: 'example_table', + explore_url: exploreUrl, + }, +]; + +test('View Dataset opens a single-prefixed URL under a subdirectory deployment', async () => { + // The backend emits explore_url already carrying the application root. The + // strip must run before ensureAppRoot re-prefixes, otherwise the opened tab + // would point at a doubled /superset/superset/... path. + render( + , + { useRouter: true }, + ); + + await userEvent.click(screen.getByText('View Dataset')); + + expect(openSpy).toHaveBeenCalledTimes(1); + const openedUrl = openSpy.mock.calls[0][0]; + expect(openedUrl).toBe(`${APP_ROOT}/explore/?datasource=1__table`); + expect(openedUrl).not.toContain('/superset/superset'); +}); + +test('View Dataset passes an external explore_url through unprefixed', async () => { + render( + , + { useRouter: true }, + ); + + await userEvent.click(screen.getByText('View Dataset')); + + expect(openSpy).toHaveBeenCalledTimes(1); + expect(openSpy.mock.calls[0][0]).toBe( + 'https://external.example.com/custom-endpoint', + ); +}); diff --git a/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx b/superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx index e18cd00c6fa..6bf357a1f3b 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, stripAppRoot } from 'src/utils/navigationUtils'; import { ITableColumn } from './types'; import MessageContent from './MessageContent'; @@ -227,11 +228,12 @@ const renderExistingDatasetAlert = (dataset?: DatasetObject) => ( { - window.open( - dataset?.explore_url, - '_blank', - 'noreferrer noopener popup=false', - ); + if (dataset?.explore_url) { + // `explore_url` is router-relative from the backend (rooted under + // a subdirectory deployment); strip the root so openInNewTab's + // ensureAppRoot re-prefixes it once rather than doubling it. + openInNewTab(stripAppRoot(dataset.explore_url)); + } }} tabIndex={0} className="view-dataset-button" diff --git a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx index c23017aeed7..ef90f1815eb 100644 --- a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx @@ -29,8 +29,8 @@ import { DatasetObject, } from 'src/features/datasets/AddDataset/types'; import { Table } from 'src/hooks/apiResources'; -import { ensureAppRoot } from 'src/utils/pathUtils'; import { Typography } from '@superset-ui/core/components/Typography'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; interface LeftPanelProps { setDataset: Dispatch>; diff --git a/superset-frontend/src/features/home/Menu.subdirectory.test.tsx b/superset-frontend/src/features/home/Menu.subdirectory.test.tsx new file mode 100644 index 00000000000..4d1083fad2f --- /dev/null +++ b/superset-frontend/src/features/home/Menu.subdirectory.test.tsx @@ -0,0 +1,230 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import * as reactRedux from 'react-redux'; +import fetchMock from 'fetch-mock'; +import { BrowserRouter } from 'react-router-dom'; +import { QueryParamProvider } from 'use-query-params'; +import { ReactRouter5Adapter } from 'use-query-params/adapters/react-router-5'; +import { render, screen } from 'spec/helpers/testing-library'; +import * as CoreTheme from '@apache-superset/core/theme'; + +// Menu's brand link statically reads applicationRoot(); intercept it to +// simulate a subdirectory deployment. +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + default: actual.default, + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: actual.staticAssetsPrefix, + }; +}); + +jest.mock('@apache-superset/core/theme', () => ({ + ...jest.requireActual('@apache-superset/core/theme'), + useTheme: jest.fn(), +})); + +jest.mock('antd', () => ({ + ...jest.requireActual('antd'), + Grid: { + ...jest.requireActual('antd').Grid, + useBreakpoint: () => ({ md: true }), + }, +})); + +// IMPORTANT: unlike Menu.test.tsx this file does NOT mock GenericLink. It +// renders the real react-router-dom under a real +// `` — the same seam production builds via +// `` in src/views/App.tsx. BrowserRouter +// honours basename in createHref and re-prepends the root WITHOUT deduping, so +// this exercises the actual rendered href the user clicks. The seam mock in +// Menu.test.tsx (no basename) is blind to that re-prepend; this file covers it. + +// eslint-disable-next-line import/first +import { Menu } from './Menu'; + +const APP_ROOT = '/superset'; + +const user = { + createdOn: '2021-04-27T18:12:38.952304', + email: 'admin', + firstName: 'admin', + isActive: true, + lastName: 'admin', + permissions: {}, + roles: { + Admin: [ + ['can_sqllab', 'Superset'], + ['can_write', 'Dashboard'], + ['can_write', 'Chart'], + ], + }, + userId: 1, + username: 'admin', +}; + +const mockedProps = { + user, + data: { + menu: [], + brand: { + path: '/superset/welcome/', + icon: '/static/assets/images/superset-logo-horiz.png', + alt: 'Apache Superset', + width: '126', + tooltip: '', + text: '', + }, + environment_tag: { + text: 'Production', + color: '#000', + }, + navbar_right: { + show_watermark: false, + bug_report_url: '/report/', + documentation_url: '/docs/', + languages: { + en: { flag: 'us', name: 'English', url: '/lang/en' }, + }, + show_language_picker: true, + user_is_anonymous: true, + user_info_url: '/users/userinfo/', + user_logout_url: '/logout/', + user_login_url: '/login/', + locale: 'en', + version_string: '1.0.0', + version_sha: 'randomSHA', + build_number: 'randomBuildNumber', + }, + settings: [], + }, +}; + +const useSelectorMock = jest.spyOn(reactRedux, 'useSelector'); +const useThemeMock = CoreTheme.useTheme as jest.Mock; + +fetchMock.get( + 'glob:*api/v1/database/?q=(filters:!((col:allow_file_upload,opr:upload_is_enabled,value:!t)))', + {}, +); + +const renderUnderSubdirectory = () => + render( + // Mirror production's `` + // (views/App.tsx). BrowserRouter honours basename in createHref, so a + // router-relative `to` is re-prefixed exactly once — the seam the brand + // link's strip must feed correctly. + + +
+ + , + { useRedux: true, useTheme: true }, + ); + +beforeEach(() => { + mockApplicationRoot.mockReturnValue(APP_ROOT); + useSelectorMock.mockReturnValue({ roles: user.roles }); + // Keep the page URL under the basename so BrowserRouter is consistent. + window.history.replaceState({}, '', `${APP_ROOT}/welcome/`); +}); + +afterEach(() => { + window.history.replaceState({}, '', '/'); + useSelectorMock.mockClear(); + jest.restoreAllMocks(); +}); + +test('brand logo link is single-prefixed when brandLogoHref arrives already rooted', async () => { + // The backend emits brandLogoHref already carrying the app root. The Router + // basename re-prepends the root, so the strip must run first to avoid a + // doubled /superset/superset/... href in the rendered anchor. + useThemeMock.mockReturnValue({ + ...CoreTheme.supersetTheme, + brandLogoUrl: '/static/assets/images/custom-logo.png', + brandLogoHref: '/superset/welcome/', + }); + + renderUnderSubdirectory(); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); + expect(brandLink.getAttribute('href')).not.toContain('/superset/superset'); +}); + +test('brand logo link is single-prefixed when brandLogoHref is a bare path', async () => { + // A bare (un-rooted) brandLogoHref must end up prefixed exactly once after + // the Router basename re-prepend. + useThemeMock.mockReturnValue({ + ...CoreTheme.supersetTheme, + brandLogoUrl: '/static/assets/images/custom-logo.png', + brandLogoHref: '/welcome/', + }); + + renderUnderSubdirectory(); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); + expect(brandLink.getAttribute('href')).not.toContain('/superset/superset'); +}); + +test('brand logo link renders without crashing when brandLogoHref is unset (partial theme override)', async () => { + // A partial theme override can set brandLogoUrl (the image) while leaving + // brandLogoHref undefined. The internal branch must stay null-safe: stripping + // the ensureAppRoot'd value (which falls back to the app root for an undefined + // input) avoids a `undefined.startsWith` TypeError that would white-screen the + // nav chrome. The rendered href resolves to the app root, never doubled. + useThemeMock.mockReturnValue({ + ...CoreTheme.supersetTheme, + brandLogoUrl: '/static/assets/images/custom-logo.png', + brandLogoHref: undefined, + }); + + renderUnderSubdirectory(); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + expect(brandLink.getAttribute('href')).toMatch(/^\/superset\/?$/); + expect(brandLink.getAttribute('href')).not.toContain('/superset/superset'); +}); + +test('external brandLogoHref passes through without app-root prefixing', async () => { + useThemeMock.mockReturnValue({ + ...CoreTheme.supersetTheme, + brandLogoUrl: '/static/assets/images/custom-logo.png', + brandLogoHref: 'https://external.example.com', + }); + + renderUnderSubdirectory(); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + expect(brandLink).toHaveAttribute('href', 'https://external.example.com'); +}); diff --git a/superset-frontend/src/features/home/Menu.test.tsx b/superset-frontend/src/features/home/Menu.test.tsx index 99555d14296..08f1db0f5ae 100644 --- a/superset-frontend/src/features/home/Menu.test.tsx +++ b/superset-frontend/src/features/home/Menu.test.tsx @@ -25,6 +25,35 @@ import * as CoreTheme from '@apache-superset/core/theme'; import { Menu } from './Menu'; import * as getBootstrapData from 'src/utils/getBootstrapData'; +// Capture what `` receives so the SPA-route regression +// tests can assert on the value handed to react-router-dom (which applies its +// own basename in production via `` in +// src/views/App.tsx). The test harness's `` has no basename, +// so asserting on the rendered `` wouldn't catch the double-prefix. +let observedGenericLinkTo: unknown = null; +jest.mock('src/components/GenericLink', () => ({ + __esModule: true, + GenericLink: ({ + to, + children, + ...rest + }: { + to: unknown; + children: React.ReactNode; + [k: string]: unknown; + }) => { + observedGenericLinkTo = to; + return ( + )} + > + {children} + + ); + }, +})); + jest.mock('@apache-superset/core/theme', () => ({ ...jest.requireActual('@apache-superset/core/theme'), useTheme: jest.fn(), @@ -797,6 +826,161 @@ test('brand link falls back to brand.path when theme brandLogoUrl is absent', as expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); }); +// Regression: the real backend emits `brand.path` and `brand.icon` already +// carrying the app root (because they pass through `url_for`). The frontend +// must not double-prefix them — neither via +// `ensureAppRoot`/`ensureStaticPrefix` nor via React Router's `basename` +// re-prepend. +// +// In production the SPA-route branch goes through ` -> +// react-router-dom `, and the Router's `basename={applicationRoot()}` +// (src/views/App.tsx) re-prepends the app root to the rendered `href`. The +// test harness's `` has no basename, so asserting on the +// rendered `` wouldn't catch the bug. Instead we mock `GenericLink` +// at module load (top of this file) and assert the path *handed to it* +// already has the root stripped — the value the production Router will then +// safely re-prepend. + +describe('brand link single-prefix regressions (subdirectory deployment)', () => { + beforeEach(() => { + observedGenericLinkTo = null; + }); + + test('brand link hands a root-stripped path to GenericLink when brand.path arrives already rooted (SPA route)', async () => { + applicationRootMock.mockReturnValue('/superset'); + staticAssetsPrefixMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithRootedBrand = { + ...mockedProps, + isFrontendRoute: () => true, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/superset/welcome/', + icon: '/superset/static/assets/images/superset-logo-horiz.png', + }, + }, + }; + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + // Wait for the mocked GenericLink to render. + await screen.findByRole('link', { + name: new RegExp(propsWithRootedBrand.data.brand.alt, 'i'), + }); + expect(observedGenericLinkTo).toBe('/welcome/'); + }); + + test('brand link is single-prefix when brand.path arrives already rooted (non-SPA route)', async () => { + applicationRootMock.mockReturnValue('/superset'); + staticAssetsPrefixMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithRootedBrand = { + ...mockedProps, + isFrontendRoute: () => false, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/superset/welcome/', + icon: '/superset/static/assets/images/superset-logo-horiz.png', + }, + }, + }; + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: new RegExp(propsWithRootedBrand.data.brand.alt, 'i'), + }); + expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); + const brandImg = brandLink.querySelector('img'); + expect(brandImg).toHaveAttribute( + 'src', + '/superset/static/assets/images/superset-logo-horiz.png', + ); + }); + + test('brand link strips a nested application root before handing to GenericLink', async () => { + applicationRootMock.mockReturnValue('/preset/superset'); + staticAssetsPrefixMock.mockReturnValue('/preset/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithRootedBrand = { + ...mockedProps, + isFrontendRoute: () => true, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/preset/superset/welcome/', + icon: '/preset/superset/static/assets/images/superset-logo-horiz.png', + }, + }, + }; + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + await screen.findByRole('link', { + name: new RegExp(propsWithRootedBrand.data.brand.alt, 'i'), + }); + expect(observedGenericLinkTo).toBe('/welcome/'); + }); + + test('brand link from theme.brandLogoHref hands a root-stripped path to GenericLink when already rooted', async () => { + applicationRootMock.mockReturnValue('/superset'); + staticAssetsPrefixMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + useThemeMock.mockReturnValue({ + ...CoreTheme.supersetTheme, + brandLogoUrl: '/superset/static/assets/images/custom-logo.png', + brandLogoHref: '/superset/welcome/', + }); + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + // The internal brand logo link goes through StyledBrandLink -> GenericLink + // -> react-router . The production Router re-prepends the app root, so + // the value handed to GenericLink must already be stripped to avoid a + // doubled /superset/superset/... href. (Real-basename coverage of the + // resulting rendered href lives in Menu.subdirectory.test.tsx.) + expect(observedGenericLinkTo).toBe('/welcome/'); + expect(brandLink).toHaveAttribute('href', '/welcome/'); + const brandImg = brandLink.querySelector('img'); + expect(brandImg).toHaveAttribute( + 'src', + '/superset/static/assets/images/custom-logo.png', + ); + }); +}); + // --- Active tab highlighting (regression tests for issue #36403) --- // // The active top-level tab is highlighted by matching the current route to a diff --git a/superset-frontend/src/features/home/Menu.tsx b/superset-frontend/src/features/home/Menu.tsx index a7e8d2dcdf3..6819b05e0b0 100644 --- a/superset-frontend/src/features/home/Menu.tsx +++ b/superset-frontend/src/features/home/Menu.tsx @@ -20,7 +20,7 @@ import { useState, useEffect } from 'react'; import { styled, css, useTheme } from '@apache-superset/core/theme'; import { t } from '@apache-superset/core/translation'; import { ensureStaticPrefix } from 'src/utils/assetUrl'; -import { ensureAppRoot } from 'src/utils/pathUtils'; +import { ensureAppRoot, stripAppRoot } from 'src/utils/navigationUtils'; import { getUrlParam, isUrlExternal } from 'src/utils/urlUtils'; import { MainNav, MenuItem } from '@superset-ui/core/components/Menu'; import { Tooltip, Grid, Row, Col, Image } from '@superset-ui/core/components'; @@ -258,10 +258,18 @@ export function Menu({ // of the localized label. Fall back to the label when no name is provided. const key = name ?? label; if (url && isFrontendRoute) { + // `` re-prepends the app root to + // `to`, so handing it the already-rooted `url` from bootstrap_data + // would render a doubled `/superset/superset/...` anchor. Strip the + // root first; mirrors the brand-link treatment below. return { key, label: ( - + {label} ), @@ -288,7 +296,11 @@ export function Menu({ // collide with that parent. Fall back to the label when no name. key: child.name ?? `${child.label}`, label: child.isFrontendRoute ? ( - + {child.label} ) : ( @@ -327,7 +339,18 @@ export function Menu({ {brandImage} ) : ( - {brandImage} + // StyledBrandLink wraps GenericLink -> react-router , and + // `` re-prepends the app root + // to `to`. Strip the root so the rendered anchor is single-prefixed + // rather than a doubled `/superset/superset/...`. Strip `brandHref` + // (the ensureAppRoot'd value) rather than the raw + // `theme.brandLogoHref` so an unset href (partial theme override) + // stays null-safe — `ensureAppRoot(undefined)` yields the app root, + // which `stripAppRoot` then reduces to `/`. Mirrors the brand.path + // branch's single-prefix treatment. + + {brandImage} + )} ); @@ -335,8 +358,12 @@ export function Menu({ // --------------------------------------------------------------------------------- // TODO: deprecate this once Theme is fully rolled out // Kept as is for backwards compatibility with the old theme system / superset_config.py + // + // `` re-prepends the app root to the + // `to` prop, so handing it an already-rooted `brand.path` would render a + // doubled `/superset/superset/...` href. Strip the root first. link = ( - + { userEvent.hover(await screen.findByText(/Settings/i)); expect(screen.queryByText('Logout')).not.toBeInTheDocument(); }); + +test('Info link href is single-prefixed under subdirectory deployment', async () => { + // Backend emits a bare leading-slash path (`/user_info/` or `/users/userinfo/`). + // RightMenu wraps it with ensureAppRoot, which reads applicationRoot() + // dynamically. Under SUPERSET_APP_ROOT=/superset the rendered href must + // be exactly `/superset/users/userinfo/` — not `/users/userinfo/` (no + // prefix → 404) or `/superset/superset/users/userinfo/` (double prefix). + const applicationRootSpy = jest + .spyOn(getBootstrapData, 'applicationRoot') + .mockReturnValue('/superset'); + + try { + resetUseSelectorMock(); + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + userEvent.hover(await screen.findByText(/Settings/i)); + const infoLink = await screen.findByText('Info'); + expect(infoLink.closest('a')).toHaveAttribute( + 'href', + '/superset/users/userinfo/', + ); + } finally { + applicationRootSpy.mockRestore(); + } +}); + +test('Logout link href is single-prefixed under subdirectory deployment', async () => { + // The logout URL is built by Flask-AppBuilder's get_url_for_logout, which + // is SCRIPT_NAME-aware and returns `/superset/logout/` under app_root. + // The frontend then routes it through ensureAppRoot, whose idempotence + // contract (see pathUtils.parity.test.ts) must prevent doubling. + const applicationRootSpy = jest + .spyOn(getBootstrapData, 'applicationRoot') + .mockReturnValue('/superset'); + + try { + const props = createProps(); + // Mirror the SCRIPT_NAME-prefixed value the backend would emit under + // APPLICATION_ROOT=/superset. + props.navbarRight.user_logout_url = '/superset/logout/'; + resetUseSelectorMock(); + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + userEvent.hover(await screen.findByText(/Settings/i)); + const logoutLink = await screen.findByText('Logout'); + expect(logoutLink.closest('a')).toHaveAttribute( + 'href', + '/superset/logout/', + ); + } finally { + applicationRootSpy.mockRestore(); + } +}); diff --git a/superset-frontend/src/features/home/RightMenu.tsx b/superset-frontend/src/features/home/RightMenu.tsx index d05abae30c9..a9296a3498d 100644 --- a/superset-frontend/src/features/home/RightMenu.tsx +++ b/superset-frontend/src/features/home/RightMenu.tsx @@ -53,7 +53,7 @@ import { TelemetryPixel, } from '@superset-ui/core/components'; import type { ItemType, MenuItem } from '@superset-ui/core/components/Menu'; -import { ensureAppRoot } from 'src/utils/pathUtils'; +import { ensureAppRoot, stripAppRoot } from 'src/utils/navigationUtils'; import { isEmbedded } from 'src/dashboard/util/isEmbedded'; import { findPermission } from 'src/utils/findPermission'; import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; @@ -427,7 +427,7 @@ const RightMenu = ({ items.push({ key: menu.label, label: isFrontendRoute(menu.url) ? ( - {menu.label} + {menu.label} ) : ( {menu.label} @@ -443,7 +443,7 @@ const RightMenu = ({ items.push({ key: menu.label, label: isFrontendRoute(menu.url) ? ( - {menu.label} + {menu.label} ) : ( {menu.label} @@ -478,7 +478,9 @@ const RightMenu = ({ sectionItems.push({ key: child.label, label: isFrontendRoute(child.url) ? ( - {menuItemDisplay} + + {menuItemDisplay} + ) : ( { 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', () => { @@ -160,7 +160,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 a41d96a53e6..b16c75503df 100644 --- a/superset-frontend/src/middleware/loggerMiddleware.ts +++ b/superset-frontend/src/middleware/loggerMiddleware.ts @@ -29,11 +29,16 @@ import { LOG_ACTIONS_SPA_NAVIGATION, } from '../logger/LogUtils'; import DebouncedMessageQueue from '../utils/DebouncedMessageQueue'; -import { ensureAppRoot } from '../utils/pathUtils'; +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/AnnotationLayerList/index.tsx b/superset-frontend/src/pages/AnnotationLayerList/index.tsx index 02d94cea471..9dca6fe73bf 100644 --- a/superset-frontend/src/pages/AnnotationLayerList/index.tsx +++ b/superset-frontend/src/pages/AnnotationLayerList/index.tsx @@ -42,7 +42,7 @@ import AnnotationLayerModal from 'src/features/annotationLayers/AnnotationLayerM import { AnnotationLayerObject } from 'src/features/annotationLayers/types'; import { QueryObjectColumns } from 'src/views/CRUD/types'; import { Icons } from '@superset-ui/core/components/Icons'; -import { navigateTo } from 'src/utils/navigationUtils'; +import { ensureAppRoot, navigateTo } from 'src/utils/navigationUtils'; import { WIDER_DROPDOWN_WIDTH } from 'src/components/ListView/utils'; const PAGE_SIZE = 25; @@ -154,7 +154,9 @@ function AnnotationLayersList({ } return ( - + {name} ); diff --git a/superset-frontend/src/pages/AnnotationList/index.tsx b/superset-frontend/src/pages/AnnotationList/index.tsx index 68011f38772..948b1284a81 100644 --- a/superset-frontend/src/pages/AnnotationList/index.tsx +++ b/superset-frontend/src/pages/AnnotationList/index.tsx @@ -41,6 +41,7 @@ import { AnnotationObject } from 'src/features/annotations/types'; import AnnotationModal from 'src/features/annotations/AnnotationModal'; import { Icons } from '@superset-ui/core/components/Icons'; import { Typography } from '@superset-ui/core/components/Typography'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; const PAGE_SIZE = 25; @@ -280,7 +281,7 @@ function AnnotationList({ {hasHistory ? ( {t('Back to all')} ) : ( - + {t('Back to all')} )} diff --git a/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx b/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx index f177eaacf2c..7e95f888c23 100644 --- a/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx +++ b/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx @@ -564,7 +564,7 @@ test('renders dashboard crosslinks as navigable links', async () => { within(crosslinks).getByRole('link', { name: new RegExp(dashboard.dashboard_title), }), - ).toHaveAttribute('href', `/superset/dashboard/${dashboard.id}`); + ).toHaveAttribute('href', `/dashboard/${dashboard.id}`); }); }); @@ -604,7 +604,7 @@ test('shows tag column when TAGGING_SYSTEM is enabled', async () => { // Tag should be a link to all_entities page const tagLink = within(tag).getByRole('link'); - expect(tagLink).toHaveAttribute('href', '/superset/all_entities/?id=1'); + expect(tagLink).toHaveAttribute('href', '/all_entities/?id=1'); expect(tagLink).toHaveAttribute('target', '_blank'); }); diff --git a/superset-frontend/src/pages/ChartList/ChartList.testHelpers.tsx b/superset-frontend/src/pages/ChartList/ChartList.testHelpers.tsx index 1a27d5affeb..2ef4c28bf3d 100644 --- a/superset-frontend/src/pages/ChartList/ChartList.testHelpers.tsx +++ b/superset-frontend/src/pages/ChartList/ChartList.testHelpers.tsx @@ -33,7 +33,7 @@ export const mockHandleResourceExport = export const mockCharts = [ { id: 0, - url: '/superset/slice/0/', + url: '/explore/?slice_id=0', viz_type: 'table', slice_name: 'Test Chart 0', @@ -43,7 +43,7 @@ export const mockCharts = [ tags: [{ name: 'basic', type: 1, id: 1 }], datasource_name_text: 'public.test_dataset', - datasource_url: '/superset/explore/table/1/', + datasource_url: '/explore/?datasource_type=table&datasource_id=1', datasource_id: 1, changed_by_name: 'user', @@ -72,7 +72,7 @@ export const mockCharts = [ }, { id: 1, - url: '/superset/slice/1/', + url: '/explore/?slice_id=1', viz_type: 'bar', slice_name: 'Test Chart 1', @@ -93,7 +93,7 @@ export const mockCharts = [ ], datasource_name_text: 'sales_data', - datasource_url: '/superset/explore/table/2/', + datasource_url: '/explore/?datasource_type=table&datasource_id=2', datasource_id: 2, changed_by_name: 'admin', @@ -119,7 +119,7 @@ export const mockCharts = [ }, { id: 2, - url: '/superset/slice/2/', + url: '/explore/?slice_id=2', viz_type: 'line', slice_name: 'Test Chart 2', @@ -150,7 +150,7 @@ export const mockCharts = [ }, { id: 3, - url: '/superset/slice/3/', + url: '/explore/?slice_id=3', viz_type: 'area', slice_name: 'Test Chart 3', @@ -168,7 +168,7 @@ export const mockCharts = [ tags: [{ name: 'limit-test', type: 1, id: 10 }], datasource_name_text: 'public.limits_dataset', - datasource_url: '/superset/explore/table/4/', + datasource_url: '/explore/?datasource_type=table&datasource_id=4', datasource_id: 4, changed_by_name: 'limit_user', @@ -189,7 +189,7 @@ export const mockCharts = [ }, { id: 4, - url: '/superset/slice/4/', + url: '/explore/?slice_id=4', viz_type: 'bubble', slice_name: 'Test Chart 4', @@ -208,7 +208,7 @@ export const mockCharts = [ tags: [{ name: 'overflow', type: 1, id: 11 }], datasource_name_text: 'public.overflow_dataset', - datasource_url: '/superset/explore/table/5/', + datasource_url: '/explore/?datasource_type=table&datasource_id=5', datasource_id: 5, changed_by_name: 'overflow_user', diff --git a/superset-frontend/src/pages/DashboardList/DashboardList.testHelpers.tsx b/superset-frontend/src/pages/DashboardList/DashboardList.testHelpers.tsx index b5cc1e4ea20..876e634a1b5 100644 --- a/superset-frontend/src/pages/DashboardList/DashboardList.testHelpers.tsx +++ b/superset-frontend/src/pages/DashboardList/DashboardList.testHelpers.tsx @@ -39,7 +39,7 @@ export const mockHandleResourceExport = export const mockDashboards = [ { id: 1, - url: '/superset/dashboard/1/', + url: '/dashboard/1/', dashboard_title: 'Sales Dashboard', published: true, changed_by_name: 'admin', @@ -61,7 +61,7 @@ export const mockDashboards = [ }, { id: 2, - url: '/superset/dashboard/2/', + url: '/dashboard/2/', dashboard_title: 'Analytics Dashboard', published: false, changed_by_name: 'analyst', @@ -86,7 +86,7 @@ export const mockDashboards = [ }, { id: 3, - url: '/superset/dashboard/3/', + url: '/dashboard/3/', dashboard_title: 'Executive Overview', published: true, changed_by_name: 'admin', @@ -111,7 +111,7 @@ export const mockDashboards = [ }, { id: 4, - url: '/superset/dashboard/4/', + url: '/dashboard/4/', dashboard_title: 'Marketing Metrics', published: false, changed_by_name: 'marketing', @@ -133,7 +133,7 @@ export const mockDashboards = [ }, { id: 5, - url: '/superset/dashboard/5/', + url: '/dashboard/5/', dashboard_title: 'Ops Monitor', published: true, changed_by_name: 'ops', diff --git a/superset-frontend/src/pages/DatabaseList/index.tsx b/superset-frontend/src/pages/DatabaseList/index.tsx index b78eb0dd652..08efc789987 100644 --- a/superset-frontend/src/pages/DatabaseList/index.tsx +++ b/superset-frontend/src/pages/DatabaseList/index.tsx @@ -55,6 +55,7 @@ import { } from 'src/components'; import { Typography } from '@superset-ui/core/components/Typography'; import { getUrlParam } from 'src/utils/urlUtils'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; import { URL_PARAMS } from 'src/constants'; import { Icons } from '@superset-ui/core/components/Icons'; import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; @@ -1032,7 +1033,7 @@ function DatabaseList({ avatar={} title={ {result.title} @@ -1075,7 +1076,9 @@ function DatabaseList({ avatar={} title={ {result.slice_name} diff --git a/superset-frontend/src/pages/DatasetList/DatasetList.subdirectory.test.tsx b/superset-frontend/src/pages/DatasetList/DatasetList.subdirectory.test.tsx new file mode 100644 index 00000000000..2c8717f35d4 --- /dev/null +++ b/superset-frontend/src/pages/DatasetList/DatasetList.subdirectory.test.tsx @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { act, screen, waitFor } from '@testing-library/react'; +import fetchMock from 'fetch-mock'; +import { Provider } from 'react-redux'; +import { BrowserRouter } from 'react-router-dom'; +import { QueryParamProvider } from 'use-query-params'; +import { ReactRouter5Adapter } from 'use-query-params/adapters/react-router-5'; +import { render } from 'spec/helpers/testing-library'; + +// DatasetList statically imports applicationRoot(); intercept it so we can +// simulate a subdirectory deployment. Mirrors the Tab.subdirectory pattern. +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + default: actual.default, + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: actual.staticAssetsPrefix, + }; +}); + +// eslint-disable-next-line import/first +import DatasetList from 'src/pages/DatasetList'; +// eslint-disable-next-line import/first +import { + setupMocks, + mockDatasetListEndpoints, + mockAdminUser, + mockDatasets, + createMockStore, + createDefaultStoreState, +} from './DatasetList.testHelpers'; + +const APP_ROOT = '/superset'; + +const renderUnderSubdirectory = () => { + const store = createMockStore({ + ...createDefaultStoreState(mockAdminUser), + user: mockAdminUser, + }); + return render( + + {/* Mirror production's `` + (views/App.tsx). BrowserRouter honours basename in createHref, so a + router-relative `to` is re-prefixed once — exactly the seam the strip + must feed correctly. */} + + + + + + , + ); +}; + +beforeEach(() => { + setupMocks(); + mockApplicationRoot.mockReturnValue(APP_ROOT); + // Put the page URL under the basename so BrowserRouter is consistent. + window.history.replaceState({}, '', `${APP_ROOT}/tablemodelview/list/`); +}); + +afterEach(async () => { + jest.useRealTimers(); + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 0)); + }); + window.history.replaceState({}, '', '/'); + fetchMock.clearHistory().removeRoutes(); + jest.restoreAllMocks(); +}); + +test('explore link is single-prefixed under a subdirectory deployment', async () => { + // The backend emits explore_url already carrying the application root. The + // Router basename re-prefixes the root, so the strip must run first to avoid + // a doubled /superset/superset/... href. + const dataset = { + ...mockDatasets[0], + explore_url: `${APP_ROOT}/explore/?datasource=1__table`, + }; + mockDatasetListEndpoints({ result: [dataset], count: 1 }); + + renderUnderSubdirectory(); + + await waitFor(() => { + expect(screen.getByText(dataset.table_name)).toBeInTheDocument(); + }); + + const exploreLink = screen.getByRole('link', { name: dataset.table_name }); + expect(exploreLink).toHaveAttribute( + 'href', + `${APP_ROOT}/explore/?datasource=1__table`, + ); + expect(exploreLink.getAttribute('href')).not.toContain('/superset/superset'); +}); + +test('external default_endpoint passes through unprefixed', async () => { + const dataset = { + ...mockDatasets[0], + explore_url: 'https://external.example.com/custom-endpoint', + }; + mockDatasetListEndpoints({ result: [dataset], count: 1 }); + + renderUnderSubdirectory(); + + await waitFor(() => { + expect(screen.getByText(dataset.table_name)).toBeInTheDocument(); + }); + + const exploreLink = screen.getByRole('link', { name: dataset.table_name }); + expect(exploreLink.getAttribute('href')).toMatch( + /^https:\/\/external\.example\.com\/custom-endpoint/, + ); +}); diff --git a/superset-frontend/src/pages/DatasetList/index.tsx b/superset-frontend/src/pages/DatasetList/index.tsx index 4c655548b23..e82c5228e16 100644 --- a/superset-frontend/src/pages/DatasetList/index.tsx +++ b/superset-frontend/src/pages/DatasetList/index.tsx @@ -71,6 +71,7 @@ import { import type { SelectOption } from 'src/components/ListView/types'; import { Typography } from '@superset-ui/core/components/Typography'; import handleResourceExport from 'src/utils/export'; +import { ensureAppRoot, stripAppRoot } from 'src/utils/navigationUtils'; import SubMenu, { SubMenuProps, ButtonProps } from 'src/features/home/SubMenu'; import Owner from 'src/types/Owner'; import withToasts from 'src/components/MessageToasts/withToasts'; @@ -704,10 +705,17 @@ const DatasetList: FunctionComponent = ({ }, }, }: CellProps) => { + // `explore_url` arrives router-relative from the backend (already + // carrying the application root under a subdirectory deployment). + // react-router's / resolve `to` against the + // Router basename, which re-prefixes the root — so strip it here to + // avoid a doubled `/superset/superset/...`. External + // `default_endpoint` URLs pass through unchanged. + const exploreTo = stripAppRoot(exploreURL); let titleLink: JSX.Element; if (PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET) { titleLink = ( - + {datasetTitle} ); @@ -715,7 +723,7 @@ const DatasetList: FunctionComponent = ({ titleLink = ( // exploreUrl can be a link to Explore or an external link // in the first case use SPA routing, else use HTML anchor - {datasetTitle} + {datasetTitle} ); } try { @@ -1396,7 +1404,7 @@ const DatasetList: FunctionComponent = ({ avatar={} title={ {result.title} @@ -1439,7 +1447,9 @@ const DatasetList: FunctionComponent = ({ avatar={} title={ {result.slice_name} diff --git a/superset-frontend/src/pages/FileHandler/FileHandler.subdirectory.test.tsx b/superset-frontend/src/pages/FileHandler/FileHandler.subdirectory.test.tsx new file mode 100644 index 00000000000..f7d1942f850 --- /dev/null +++ b/superset-frontend/src/pages/FileHandler/FileHandler.subdirectory.test.tsx @@ -0,0 +1,267 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ComponentType } from 'react'; +import { Route, Router } from 'react-router-dom'; +import { createMemoryHistory, MemoryHistory } from 'history'; +import { + render, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import FileHandler from './index'; + +// Subdirectory regression for the five `history.push('/welcome/')` emitters +// in FileHandler. The sibling `index.test.tsx` mocks `useHistory` and so can +// only assert the value the emitter passes to `push`; this module wires up +// the real react-router pathway (real `` + a `createMemoryHistory` +// the test owns) and asserts that the resulting `history.location.pathname` +// resolves to `/welcome/` regardless of whether the user arrived under the +// root deployment (`/file-handler`) or a `/superset` subdirectory deployment +// (`/superset/file-handler`). +// +// Note on basename composition: Superset's dependency tree pairs +// `react-router-dom@5.3.4` with `history@5.3.0`, but the `basename` prop on +// `` is silently dropped in that combination (history v5 has +// no basename support; react-router-dom v5 was designed for history v4). The +// production subdirectory deployment composes the prefix at the URL-emitting +// layer via `applicationRoot()` in non-router callers (see +// `SliceHeaderControls.subdirectory.test.tsx`), and the in-app router relies +// on the unprefixed routes matching whatever path the user arrived at. As a +// result, asserting `history.createHref(...)` here would not exercise any +// real composition — so this module pins only what is meaningful in this +// stack: that the emitter pushes the unprefixed route and the post-push +// location reflects it. + +const mockAddDangerToast = jest.fn(); +const mockAddSuccessToast = jest.fn(); + +jest.setTimeout(60000); + +type ToastInjectedProps = { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; +}; + +jest.mock('src/components/MessageToasts/withToasts', () => ({ + __esModule: true, + default: (Component: ComponentType) => + function MockedWithToasts(props: Record) { + return ( + + ); + }, +})); + +interface UploadDataModalProps { + show: boolean; + onHide: () => void; + type: string; + allowedExtensions: string[]; + fileListOverride?: File[]; +} + +jest.mock('src/features/databases/UploadDataModel', () => ({ + __esModule: true, + default: ({ + show, + onHide, + type, + allowedExtensions, + fileListOverride, + }: UploadDataModalProps) => ( +
+
{show.toString()}
+
{type}
+
{allowedExtensions.join(',')}
+
{fileListOverride?.[0]?.name ?? ''}
+ +
+ ), +})); + +// NOTE: deliberately NO jest.mock('react-router-dom', ...) here — this module +// exists precisely to exercise the real useHistory() pathway, not a mock. + +type MockFileHandle = { + kind: 'file'; + name: string; + getFile: () => Promise; + isSameEntry: () => Promise; + queryPermission: () => Promise; + requestPermission: () => Promise; +}; + +const createMockFileHandle = ( + fileName: string, + opts: { throwOnGetFile?: boolean } = {}, +): MockFileHandle => ({ + kind: 'file', + name: fileName, + getFile: opts.throwOnGetFile + ? async () => { + throw new Error('File access denied'); + } + : async () => new File(['test'], fileName), + isSameEntry: async () => false, + queryPermission: async () => 'granted', + requestPermission: async () => 'granted', +}); + +type LaunchQueue = { + setConsumer: ( + consumer: (params: { files?: MockFileHandle[] }) => void, + ) => void; +}; + +const pendingTimerIds = new Set>(); +const MAX_CONSUMER_POLL_ATTEMPTS = 50; + +// Mirrors `setupLaunchQueue` in index.test.tsx: defer the consumer to a +// macrotask so it doesn't fire synchronously inside the component's useEffect +// (the MessageChannel mock in jsDomWithFetchAPI forces React to schedule via +// setTimeout, and inline consumer calls deadlock Jest). +const setupLaunchQueue = (fileHandle: MockFileHandle | null = null) => { + let savedConsumer: + | ((params: { files?: MockFileHandle[] }) => void | Promise) + | null = null; + (window as unknown as Window & { launchQueue: LaunchQueue }).launchQueue = { + setConsumer: (consumer: (params: { files?: MockFileHandle[] }) => void) => { + savedConsumer = consumer; + if (fileHandle) { + const id = setTimeout(() => { + pendingTimerIds.delete(id); + consumer({ files: [fileHandle] }); + }, 0); + pendingTimerIds.add(id); + } + }, + }; + return { + triggerConsumer: async (params: { files?: MockFileHandle[] }) => { + let attempts = 0; + while (!savedConsumer && attempts < MAX_CONSUMER_POLL_ATTEMPTS) { + // eslint-disable-next-line no-await-in-loop + await new Promise(resolve => { + setTimeout(resolve, 0); + }); + attempts += 1; + } + if (!savedConsumer) { + throw new Error( + `LaunchQueue consumer was never registered after ${MAX_CONSUMER_POLL_ATTEMPTS} polling attempts`, + ); + } + await savedConsumer(params); + }, + }; +}; + +type DeploymentLabel = 'root' | 'subdir'; + +const ENTRY_PATHS: Record = { + root: '/file-handler', + subdir: '/superset/file-handler', +}; + +const renderUnderEntry = (entryPath: string): MemoryHistory => { + const history = createMemoryHistory({ initialEntries: [entryPath] }); + render( + + + + + , + { useRedux: true }, + ); + return history; +}; + +const expectNavigatedToWelcome = async ( + history: MemoryHistory, +): Promise => { + await waitFor(() => { + expect(history.location.pathname).toBe('/welcome/'); + }); +}; + +beforeEach(() => { + jest.clearAllMocks(); + delete (window as unknown as Window & { launchQueue?: LaunchQueue }) + .launchQueue; +}); + +afterEach(() => { + pendingTimerIds.forEach(id => clearTimeout(id)); + pendingTimerIds.clear(); + delete (window as unknown as Window & { launchQueue?: LaunchQueue }) + .launchQueue; +}); + +// Run each redirect scenario under both deployment shapes. Both must end at +// `/welcome/`; the test fails if a future maintainer re-introduces the +// `/superset/` prefix into the emitter at any of the five call sites. +const DEPLOYMENTS: DeploymentLabel[] = ['root', 'subdir']; + +DEPLOYMENTS.forEach(label => { + const entryPath = ENTRY_PATHS[label]; + + test(`launchQueue unsupported → /welcome/ under ${label} (${entryPath})`, async () => { + const history = renderUnderEntry(entryPath); + await expectNavigatedToWelcome(history); + }); + + test(`no files provided → /welcome/ under ${label} (${entryPath})`, async () => { + const { triggerConsumer } = setupLaunchQueue(); + const history = renderUnderEntry(entryPath); + await triggerConsumer({ files: [] }); + await expectNavigatedToWelcome(history); + }); + + test(`unsupported file type → /welcome/ under ${label} (${entryPath})`, async () => { + const { triggerConsumer } = setupLaunchQueue(); + const history = renderUnderEntry(entryPath); + await triggerConsumer({ files: [createMockFileHandle('test.pdf')] }); + await expectNavigatedToWelcome(history); + }); + + test(`getFile() error → /welcome/ under ${label} (${entryPath})`, async () => { + const { triggerConsumer } = setupLaunchQueue(); + const history = renderUnderEntry(entryPath); + await triggerConsumer({ + files: [createMockFileHandle('test.csv', { throwOnGetFile: true })], + }); + await expectNavigatedToWelcome(history); + }); + + test(`modal close → /welcome/ under ${label} (${entryPath})`, async () => { + setupLaunchQueue(createMockFileHandle('test.csv')); + const history = renderUnderEntry(entryPath); + const modal = await screen.findByTestId('upload-modal'); + expect(modal).toBeInTheDocument(); + await userEvent.click(screen.getByRole('button', { name: 'Close' })); + await expectNavigatedToWelcome(history); + }); +}); diff --git a/superset-frontend/src/pages/FileHandler/index.test.tsx b/superset-frontend/src/pages/FileHandler/index.test.tsx index a4496625fa7..b51149daaa0 100644 --- a/superset-frontend/src/pages/FileHandler/index.test.tsx +++ b/superset-frontend/src/pages/FileHandler/index.test.tsx @@ -201,7 +201,7 @@ test('shows error when launchQueue is not supported', async () => { expect(mockAddDangerToast).toHaveBeenCalledWith( 'File handling is not supported in this browser. Please use a modern browser like Chrome or Edge.', ); - expect(mockHistoryPush).toHaveBeenCalledWith('/superset/welcome/'); + expect(mockHistoryPush).toHaveBeenCalledWith('/welcome/'); }); }); @@ -221,7 +221,7 @@ test('redirects when no files are provided', async () => { await triggerConsumer({ files: [] }); await waitFor(() => { - expect(mockHistoryPush).toHaveBeenCalledWith('/superset/welcome/'); + expect(mockHistoryPush).toHaveBeenCalledWith('/welcome/'); }); }); @@ -326,7 +326,7 @@ test('shows error for unsupported file type', async () => { expect(mockAddDangerToast).toHaveBeenCalledWith( 'Unsupported file type. Please use CSV, Excel, or Columnar files.', ); - expect(mockHistoryPush).toHaveBeenCalledWith('/superset/welcome/'); + expect(mockHistoryPush).toHaveBeenCalledWith('/welcome/'); }); }); @@ -378,7 +378,7 @@ test('handles errors during file processing', async () => { expect(mockAddDangerToast).toHaveBeenCalledWith( 'Failed to open file. Please try again.', ); - expect(mockHistoryPush).toHaveBeenCalledWith('/superset/welcome/'); + expect(mockHistoryPush).toHaveBeenCalledWith('/welcome/'); }); }); @@ -402,7 +402,7 @@ test('modal close redirects to welcome page', async () => { await userEvent.click(screen.getByRole('button', { name: 'Close' })); await waitFor(() => { - expect(mockHistoryPush).toHaveBeenCalledWith('/superset/welcome/'); + expect(mockHistoryPush).toHaveBeenCalledWith('/welcome/'); }); }); diff --git a/superset-frontend/src/pages/FileHandler/index.tsx b/superset-frontend/src/pages/FileHandler/index.tsx index 589db966f74..ea24ce9383d 100644 --- a/superset-frontend/src/pages/FileHandler/index.tsx +++ b/superset-frontend/src/pages/FileHandler/index.tsx @@ -59,13 +59,13 @@ const FileHandler = ({ addDangerToast, addSuccessToast }: FileHandlerProps) => { 'File handling is not supported in this browser. Please use a modern browser like Chrome or Edge.', ), ); - history.push('/superset/welcome/'); + history.push('/welcome/'); return; } launchQueue.setConsumer(async (launchParams: FileLaunchParams) => { if (!launchParams.files || launchParams.files.length === 0) { - history.push('/superset/welcome/'); + history.push('/welcome/'); return; } @@ -92,7 +92,7 @@ const FileHandler = ({ addDangerToast, addSuccessToast }: FileHandlerProps) => { 'Unsupported file type. Please use CSV, Excel, or Columnar files.', ), ); - history.push('/superset/welcome/'); + history.push('/welcome/'); return; } @@ -103,7 +103,7 @@ const FileHandler = ({ addDangerToast, addSuccessToast }: FileHandlerProps) => { } catch (error) { console.error('Error handling file launch:', error); addDangerToast(t('Failed to open file. Please try again.')); - history.push('/superset/welcome/'); + history.push('/welcome/'); } }); }; @@ -115,7 +115,7 @@ const FileHandler = ({ addDangerToast, addSuccessToast }: FileHandlerProps) => { setShowModal(false); setUploadFile(null); setUploadType(null); - history.push('/superset/welcome/'); + history.push('/welcome/'); }; if (!uploadFile || !uploadType) { diff --git a/superset-frontend/src/pages/Login/Login.test.tsx b/superset-frontend/src/pages/Login/Login.test.tsx index 03e9ad72671..3a20d9c70b8 100644 --- a/superset-frontend/src/pages/Login/Login.test.tsx +++ b/superset-frontend/src/pages/Login/Login.test.tsx @@ -31,15 +31,25 @@ const defaultBootstrapData = { }, }; -jest.mock('src/utils/getBootstrapData', () => ({ - __esModule: true, - default: jest.fn(() => defaultBootstrapData), -})); +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + ...actual, + default: jest.fn(() => defaultBootstrapData), + applicationRoot: () => mockApplicationRoot(), + }; +}); const mockGetBootstrapData = getBootstrapData as jest.Mock; beforeEach(() => { mockGetBootstrapData.mockReturnValue(defaultBootstrapData); + mockApplicationRoot.mockReturnValue(''); }); test('should render login form elements', () => { @@ -81,3 +91,52 @@ test('should render SAML provider buttons', () => { expect(screen.getByText('Sign in with Okta')).toBeInTheDocument(); expect(screen.getByText('Sign in with Onelogin')).toBeInTheDocument(); }); + +const samlBootstrapData = { + common: { + conf: { + AUTH_TYPE: 5, + AUTH_PROVIDERS: [{ name: 'okta', icon: 'okta' }], + AUTH_USER_REGISTRATION: false, + }, + }, +}; + +test('provider login links are root-relative on root deployments', () => { + mockGetBootstrapData.mockReturnValue(samlBootstrapData); + render(, { useRedux: true }); + expect( + screen.getByRole('link', { name: /sign in with okta/i }), + ).toHaveAttribute('href', '/login/okta'); +}); + +test('provider login links carry the application root on subdirectory deployments', () => { + mockApplicationRoot.mockReturnValue('/superset'); + mockGetBootstrapData.mockReturnValue(samlBootstrapData); + render(, { useRedux: true }); + expect( + screen.getByRole('link', { name: /sign in with okta/i }), + ).toHaveAttribute('href', '/superset/login/okta'); +}); + +test('provider login links preserve the next param under a subdirectory', () => { + mockApplicationRoot.mockReturnValue('/superset'); + mockGetBootstrapData.mockReturnValue(samlBootstrapData); + const next = '/superset/dashboard/1/'; + window.history.replaceState( + {}, + '', + `/superset/login/?next=${encodeURIComponent(next)}`, + ); + try { + render(, { useRedux: true }); + expect( + screen.getByRole('link', { name: /sign in with okta/i }), + ).toHaveAttribute( + 'href', + `/superset/login/okta?next=${encodeURIComponent(next)}`, + ); + } finally { + window.history.replaceState({}, '', '/'); + } +}); diff --git a/superset-frontend/src/pages/Login/index.tsx b/superset-frontend/src/pages/Login/index.tsx index e585bf08f54..d704945d1fd 100644 --- a/superset-frontend/src/pages/Login/index.tsx +++ b/superset-frontend/src/pages/Login/index.tsx @@ -34,6 +34,7 @@ import { capitalize } from 'lodash/fp'; import { addDangerToast } from 'src/components/MessageToasts/actions'; import { useDispatch } from 'react-redux'; import getBootstrapData from 'src/utils/getBootstrapData'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; type OAuthProvider = { name: string; @@ -100,10 +101,10 @@ export default function Login() { ); const buildProviderLoginUrl = (providerName: string) => { - const base = `/login/${providerName}`; - return nextUrl - ? `${base}${base.includes('?') ? '&' : '?'}next=${encodeURIComponent(nextUrl)}` - : base; + const base = `/login/${encodeURIComponent(providerName)}`; + return ensureAppRoot( + nextUrl ? `${base}?next=${encodeURIComponent(nextUrl)}` : base, + ); }; const authType: AuthType = bootstrapData.common.conf.AUTH_TYPE; @@ -256,7 +257,7 @@ export default function Login() { + + + + ); + } + return ( diff --git a/superset-frontend/src/pages/RedirectWarning/utils.test.ts b/superset-frontend/src/pages/RedirectWarning/utils.test.ts index 0cb38a2d80f..7c9907cda10 100644 --- a/superset-frontend/src/pages/RedirectWarning/utils.test.ts +++ b/superset-frontend/src/pages/RedirectWarning/utils.test.ts @@ -56,6 +56,65 @@ test('isAllowedScheme allows relative URLs (unparseable as absolute)', () => { expect(isAllowedScheme('/dashboard/1')).toBe(true); }); +test('isAllowedScheme blocks protocol-relative URLs', () => { + // `new URL('//evil.example.com')` throws standalone, so without the + // explicit guard the catch branch would let cross-origin protocol- + // relative URLs through as "relative". + expect(isAllowedScheme('//evil.example.com')).toBe(false); + expect(isAllowedScheme('//evil.example.com/phish?token=abc')).toBe(false); +}); + +test('isAllowedScheme does not block single-leading-slash absolute paths', () => { + // Guard against an over-broad fix that strips both `//` and `/foo`. + expect(isAllowedScheme('/dashboard/list/')).toBe(true); +}); + +// The up-front +// `startsWith('//')` check missed backslash variants. `new URL('/\\evil.com')` +// throws → the catch returns `true` (allow) → the interstitial UI shows +// `/\evil.com` inside an "External link warning" Card with a Continue +// button. Browsers normalise `/\` → `//` in the special-scheme authority, +// so the consented click became `https://evil.com`. The fix must reject +// **before** the `new URL` attempt so the throw cannot route through the +// allow branch. + +test('isAllowedScheme blocks /\\evil.com (backslash variant)', () => { + expect(isAllowedScheme('/\\evil.example.com')).toBe(false); +}); + +test('isAllowedScheme blocks \\/evil.com (backslash variant)', () => { + expect(isAllowedScheme('\\/evil.example.com')).toBe(false); +}); + +test('isAllowedScheme blocks \\\\evil.com (backslash variant)', () => { + expect(isAllowedScheme('\\\\evil.example.com')).toBe(false); +}); + +test('isAllowedScheme blocks /\\evil.com BEFORE the new URL attempt (not via the catch)', () => { + // Stub `URL` to throw — the result must still be `false`, proving the + // backslash rejection happens up-front and not via the catch's + // "relative URLs — allow" branch. (If the rejection were inside the + // try-block, a URL-constructor throw would route through `catch { return + // true }` and we'd see `true` here.) + const originalURL = global.URL; + global.URL = class { + constructor() { + throw new Error('stubbed URL constructor'); + } + } as unknown as typeof URL; + try { + expect(isAllowedScheme('/\\evil.example.com')).toBe(false); + } finally { + global.URL = originalURL; + } +}); + +test('isAllowedScheme still blocks the catch-branch protocol-relative case after backslash rejection', () => { + // Regression: hardening must not accidentally drop the existing `//host` + // protection. + expect(isAllowedScheme('//evil.example.com')).toBe(false); +}); + test('getTargetUrl reads the url query parameter', () => { const locationSpy = jest.spyOn(window, 'location', 'get').mockReturnValue({ search: '?url=https%3A%2F%2Fexample.com%2Fpage', diff --git a/superset-frontend/src/pages/RedirectWarning/utils.ts b/superset-frontend/src/pages/RedirectWarning/utils.ts index 43aa834789a..e9cd66299c1 100644 --- a/superset-frontend/src/pages/RedirectWarning/utils.ts +++ b/superset-frontend/src/pages/RedirectWarning/utils.ts @@ -36,8 +36,23 @@ 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. + * + * Backslash variants (`/\host`, `\/host`, `\\host`, and any URL containing a + * backslash) are rejected up-front, BEFORE the `new URL` attempt. Browsers + * normalise `/\` → `//` in special-scheme authorities, so a backslash + * anywhere in the input lets an attacker craft a cross-origin target that + * presents as router-relative to the eye. Without the explicit rejection, + * `new URL('/\\evil.com')` would throw and the catch branch would return + * `true`, allowing the interstitial UI to display the URL as if it were a + * safe relative path. */ export function isAllowedScheme(url: string): boolean { + if (/^[/\\][/\\]/.test(url) || url.includes('\\')) return false; try { const parsed = new URL(url); return ALLOWED_SCHEMES.includes(parsed.protocol); diff --git a/superset-frontend/src/pages/Register/index.tsx b/superset-frontend/src/pages/Register/index.tsx index cb962687de1..9c89ef41b6e 100644 --- a/superset-frontend/src/pages/Register/index.tsx +++ b/superset-frontend/src/pages/Register/index.tsx @@ -32,6 +32,7 @@ import { useState } from 'react'; import getBootstrapData from 'src/utils/getBootstrapData'; import ReactCAPTCHA from 'react-google-recaptcha'; import { useParams } from 'react-router-dom'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; interface RegisterForm { username: string; @@ -91,7 +92,11 @@ export default function Login() { 'Your account is activated. You can log in with your credentials.', )} extra={[ - , ]} diff --git a/superset-frontend/src/pages/SavedQueryList/index.tsx b/superset-frontend/src/pages/SavedQueryList/index.tsx index 92dde1d21bd..c390c5f3001 100644 --- a/superset-frontend/src/pages/SavedQueryList/index.tsx +++ b/superset-frontend/src/pages/SavedQueryList/index.tsx @@ -66,7 +66,7 @@ import type Owner from 'src/types/Owner'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import SavedQueryPreviewModal from 'src/features/queries/SavedQueryPreviewModal'; import { findPermission } from 'src/utils/findPermission'; -import { makeUrl } from 'src/utils/pathUtils'; +import { getShareableUrl, openInNewTab } from 'src/utils/navigationUtils'; const PAGE_SIZE = 25; const PASSWORDS_NEEDED_MESSAGE = t( @@ -244,11 +244,8 @@ function SavedQueryList({ // Action methods const openInSqlLab = (id: number, openInNewWindow: boolean) => { - copyTextToClipboard(() => - Promise.resolve( - `${window.location.origin}${makeUrl(`/sqllab?savedQueryId=${id}`)}`, - ), - ) + const path = `/sqllab?savedQueryId=${id}`; + copyTextToClipboard(() => Promise.resolve(getShareableUrl(path))) .then(() => { addSuccessToast(t('Link Copied!')); }) @@ -256,11 +253,11 @@ function SavedQueryList({ addDangerToast(t('Sorry, your browser does not support copying.')); }); if (openInNewWindow) { - window.open(makeUrl(`/sqllab?savedQueryId=${id}`)); + openInNewTab(path); } else { // React Router's basename already includes the application root; passing // a relative path ensures correct navigation under subdirectory deployments. - history.push(`/sqllab?savedQueryId=${id}`); + history.push(path); } }; diff --git a/superset-frontend/src/pages/Tags/index.tsx b/superset-frontend/src/pages/Tags/index.tsx index f18cf0d4f29..9527f4808ca 100644 --- a/superset-frontend/src/pages/Tags/index.tsx +++ b/superset-frontend/src/pages/Tags/index.tsx @@ -168,7 +168,7 @@ function TagList(props: TagListProps) { }, }: any) => ( - {tagName} + {tagName} ), Header: t('Name'), diff --git a/superset-frontend/src/preamble.ts b/superset-frontend/src/preamble.ts index cc304da84d3..c9f64c4160a 100644 --- a/superset-frontend/src/preamble.ts +++ b/superset-frontend/src/preamble.ts @@ -26,7 +26,7 @@ import setupFormatters from './setup/setupFormatters'; import setupDashboardComponents from './setup/setupDashboardComponents'; import { User } from './types/bootstrapTypes'; import getBootstrapData, { applicationRoot } from './utils/getBootstrapData'; -import { makeUrl } from './utils/pathUtils'; +import { makeUrl } from './utils/navigationUtils'; import './hooks/useLocale'; // Import dayjs plugin types for global TypeScript support @@ -73,7 +73,7 @@ export default function initPreamble(): Promise { }, LANGUAGE_PACK_REQUEST_TIMEOUT_MS); try { - const languagePackUrl = makeUrl(`/superset/language_pack/${lang}/`); + const languagePackUrl = makeUrl(`/language_pack/${lang}/`); const resp = await fetch(languagePackUrl, { signal: abortController.signal, }); diff --git a/superset-frontend/src/pwa-manifest.json b/superset-frontend/src/pwa-manifest.json deleted file mode 100644 index fcd8f2213eb..00000000000 --- a/superset-frontend/src/pwa-manifest.json +++ /dev/null @@ -1,65 +0,0 @@ -{ - "name": "Apache Superset", - "short_name": "Superset", - "description": "Modern data exploration and visualization platform", - "start_url": "/superset/welcome/", - "scope": "/", - "display": "standalone", - "background_color": "#ffffff", - "theme_color": "#20a7c9", - "icons": [ - { - "src": "/static/assets/images/pwa/icon-192.png", - "sizes": "192x192", - "type": "image/png", - "purpose": "any" - }, - { - "src": "/static/assets/images/pwa/icon-512.png", - "sizes": "512x512", - "type": "image/png", - "purpose": "any" - }, - { - "src": "/static/assets/images/pwa/icon-192.png", - "sizes": "192x192", - "type": "image/png", - "purpose": "maskable" - }, - { - "src": "/static/assets/images/pwa/icon-512.png", - "sizes": "512x512", - "type": "image/png", - "purpose": "maskable" - } - ], - "screenshots": [ - { - "src": "/static/assets/images/pwa/screenshot-wide.png", - "sizes": "1280x720", - "type": "image/png", - "form_factor": "wide", - "label": "Apache Superset Dashboard" - }, - { - "src": "/static/assets/images/pwa/screenshot-narrow.png", - "sizes": "540x720", - "type": "image/png", - "form_factor": "narrow", - "label": "Apache Superset Mobile View" - } - ], - "file_handlers": [ - { - "action": "/superset/file-handler", - "accept": { - "text/csv": [".csv"], - "application/vnd.ms-excel": [".xls"], - "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet": [ - ".xlsx" - ], - "application/vnd.apache.parquet": [".parquet"] - } - } - ] -} diff --git a/superset-frontend/src/utils/assetUrl.test.ts b/superset-frontend/src/utils/assetUrl.test.ts index f0de28b2c77..3d8307f6b3b 100644 --- a/superset-frontend/src/utils/assetUrl.test.ts +++ b/superset-frontend/src/utils/assetUrl.test.ts @@ -46,3 +46,42 @@ describe('assetUrl should ignore static asset prefix for absolute URLs', () => { expect(ensureStaticPrefix(absoluteResourcePath)).toBe(absoluteResourcePath); }); }); + +describe('ensureStaticPrefix should be idempotent', () => { + test('does not double-prefix a path that already starts with the static-assets prefix', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + const alreadyRooted = + '/superset/static/assets/images/superset-logo-horiz.png'; + expect(ensureStaticPrefix(alreadyRooted)).toBe(alreadyRooted); + }); + + test('still prefixes a relative path that does not yet carry the prefix', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + expect(ensureStaticPrefix('/static/assets/x.png')).toBe( + '/superset/static/assets/x.png', + ); + }); + + test('treats segment boundaries — `/supersetfoo` is not the same as `/superset`', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + expect(ensureStaticPrefix('/supersetfoo/img.png')).toBe( + '/superset/supersetfoo/img.png', + ); + }); + + test('handles nested application roots', () => { + staticAssetsPrefixMock.mockReturnValue('/preset/superset'); + const alreadyRooted = '/preset/superset/static/x.png'; + expect(ensureStaticPrefix(alreadyRooted)).toBe(alreadyRooted); + }); + + test('returns the prefix itself unchanged when passed in bare', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + expect(ensureStaticPrefix('/superset')).toBe('/superset'); + }); + + test('is a no-op when the static-assets prefix is empty (root deployment)', () => { + staticAssetsPrefixMock.mockReturnValue(''); + expect(ensureStaticPrefix('/static/x.png')).toBe('/static/x.png'); + }); +}); diff --git a/superset-frontend/src/utils/assetUrl.ts b/superset-frontend/src/utils/assetUrl.ts index a7fc2553b50..72a07eff5bd 100644 --- a/superset-frontend/src/utils/assetUrl.ts +++ b/superset-frontend/src/utils/assetUrl.ts @@ -30,10 +30,21 @@ export function assetUrl(path: string): string { /** * Returns the path prepended with the staticAssetsPrefix if the string is a relative path else it returns * the string as is. + * + * Idempotent — a path that already begins with the static-assets prefix at a + * segment boundary is returned unchanged, mirroring the dedupe pattern used by + * `ensureAppRoot` in pathUtils.ts and `SupersetClient.getUrl`. + * * @param url_or_path A url or relative path to a resource */ export function ensureStaticPrefix(url_or_path: string): string { - if (url_or_path.startsWith('/')) return assetUrl(url_or_path); - - return url_or_path; + if (!url_or_path.startsWith('/')) return url_or_path; + const prefix = staticAssetsPrefix(); + if ( + prefix && + (url_or_path === prefix || url_or_path.startsWith(`${prefix}/`)) + ) { + return url_or_path; + } + return assetUrl(url_or_path); } diff --git a/superset-frontend/src/utils/getBootstrapData.test.ts b/superset-frontend/src/utils/getBootstrapData.test.ts index a8076bd2f4e..db2e30d2766 100644 --- a/superset-frontend/src/utils/getBootstrapData.test.ts +++ b/superset-frontend/src/utils/getBootstrapData.test.ts @@ -111,33 +111,84 @@ describe('getBootstrapData and helpers', () => { }); test.each([ - ['//evil.example.com', 'protocol-relative URL'], + ['markup payload', '">/'], + ['protocol-relative URL', '//evil.example.com/app/'], + ['absolute URL', 'https://evil.example.com/app/'], // eslint-disable-next-line no-script-url -- intentional unsafe value under test - ['javascript:alert(1)', 'javascript scheme'], - ['https://evil.example.com', 'absolute URL'], - ['/foo">', 'path with HTML meta-characters'], + ['javascript scheme', 'javascript:alert(1)'], + ['embedded quote', "/my'app/"], + ['path traversal', '/app/..'], + ['double-dot segment', '/legit/../etc'], + ['backslash separator', '/app\\admin'], + ['leading double slash', '//app'], + ['double trailing slash', '/app//'], + ['unicode segment', '/café'], + ['tab control character', '/app\tadmin'], + ['newline control character', '/app\nadmin'], + ['null byte', '/app\x00admin'], ])( - 'should fall back to the default root when application_root is %s (%s)', - async unsafeRoot => { + 'should degrade a non-path application_root to root deployment (%s)', + async (_label, applicationRootValue) => { const customData = { common: { - application_root: unsafeRoot, + application_root: applicationRootValue, static_assets_prefix: '/custom-static/', }, }; - document.body.innerHTML = `
`; + document.body.innerHTML = `
`; + document + .getElementById('app') + ?.setAttribute('data-bootstrap', JSON.stringify(customData)); jest.resetModules(); const { default: getBootstrapData, applicationRoot } = await import('./getBootstrapData'); getBootstrapData(); - const expectedAppRoot = - DEFAULT_BOOTSTRAP_DATA.common.application_root.replace(/\/$/, ''); - expect(applicationRoot()).toEqual(expectedAppRoot); + expect(applicationRoot()).toEqual(''); }, ); + // Percent-encoded traversal sequences are NOT decoded or rejected here. + // application_root is operator-controlled server-rendered configuration + // (see SECURITY.md trust-boundary 2); the sanitizer only enforces the + // documented path shape on the literal value. Encoded forms like "%2e%2e" + // are valid path-segment characters and pass through unchanged — this is + // intentional and out of scope, documented here so the behavior is pinned. + test('should preserve a percent-encoded application_root (operator-controlled, out of scope)', async () => { + const customData = { + common: { + application_root: '/app/%2e%2e', + static_assets_prefix: '/custom-static/', + }, + }; + document.body.innerHTML = `
`; + + jest.resetModules(); + const { default: getBootstrapData, applicationRoot } = + await import('./getBootstrapData'); + getBootstrapData(); + + expect(applicationRoot()).toEqual('/app/%2e%2e'); + }); + + test('should preserve a multi-segment application_root', async () => { + const customData = { + common: { + application_root: '/team-a/superset/', + static_assets_prefix: '/custom-static/', + }, + }; + document.body.innerHTML = `
`; + + jest.resetModules(); + const { default: getBootstrapData, applicationRoot } = + await import('./getBootstrapData'); + getBootstrapData(); + + expect(applicationRoot()).toEqual('/team-a/superset'); + }); + test('should defaults without trailing slashes when #app element does not include application_root or static_assets_prefix', async () => { // Set up the fake #app element const customData = { diff --git a/superset-frontend/src/utils/getBootstrapData.ts b/superset-frontend/src/utils/getBootstrapData.ts index 8413bf860d2..5cf66a944f1 100644 --- a/superset-frontend/src/utils/getBootstrapData.ts +++ b/superset-frontend/src/utils/getBootstrapData.ts @@ -38,36 +38,36 @@ const normalizePathWithFallback = ( fallback: string, ): string => (path ?? fallback).replace(/\/$/, ''); -/** - * Matches a plain absolute path prefix (e.g. "" for root deployments or - * "/analytics" for a subdirectory). The character after the leading slash must - * not be another slash, so protocol-relative URLs ("//host") and scheme-bearing - * values ("javascript:...") do not qualify. - */ -const SAFE_APPLICATION_ROOT_RE = /^(\/[\w\-.][\w\-./]*)?$/; +// The application root is server-rendered operator configuration, but it +// flows into URL sinks across the app (script.src in ExtensionsLoader, +// history.pushState in navigationUtils), so enforce the documented +// "/segment(/segment)*" shape at the source: a value that doesn't conform +// degrades to a root deployment rather than reaching those sinks. +// Each segment must begin with a non-dot character so a literal "." or ".." +// segment (e.g. "/app/..") is rejected — otherwise browser path normalization +// would collapse `ensureAppRoot('/foo')` → `/app/../foo` → `/foo`, silently +// defeating subdirectory containment. Dots are still allowed inside a +// segment (e.g. "/foo.bar"). +// +// This guards the *literal* value only. Percent-encoded dot segments +// ("/app/%2e%2e") pass through unchanged: `%` is a permitted path-segment +// character and the regex does not decode. A browser would later normalize +// "/app/%2e%2e/foo" → "/foo", so such a value silently defeats subdirectory +// prefixing — but application_root is operator-controlled, server-rendered +// configuration (SECURITY.md trust-boundary 2), so an encoded-traversal value +// is an unsupported misconfiguration, not an attacker-reachable input. It is +// intentionally neither decoded nor rejected here; the behavior is pinned by a +// test in getBootstrapData.test.ts. +const APP_ROOT_PATH_RE = /^(?:\/[\w~%-]+(?:\.[\w~%-]+)*)*$/; -/** - * The application root (SUPERSET_APP_ROOT) is reflected into links and - * navigation, so constrain it to a plain absolute path before use. Anything - * that isn't a simple "/path" prefix falls back to the default root so a - * malformed value can't be reinterpreted as HTML or redirect off-origin. This - * also keeps the bootstrap-derived value from being treated as a tainted href - * source by static analysis. - */ -const sanitizeApplicationRoot = ( - path: string | undefined, - fallback: string, -): string => { - const normalizedFallback = normalizePathWithFallback(fallback, fallback); - const normalized = normalizePathWithFallback(path, fallback); - return SAFE_APPLICATION_ROOT_RE.test(normalized) - ? normalized - : normalizedFallback; -}; +const sanitizeAppRoot = (root: string): string => + APP_ROOT_PATH_RE.test(root) ? root : ''; -const APPLICATION_ROOT_NO_TRAILING_SLASH = sanitizeApplicationRoot( - getBootstrapData().common.application_root, - DEFAULT_BOOTSTRAP_DATA.common.application_root, +const APPLICATION_ROOT_NO_TRAILING_SLASH = sanitizeAppRoot( + normalizePathWithFallback( + getBootstrapData().common.application_root, + DEFAULT_BOOTSTRAP_DATA.common.application_root, + ), ); const STATIC_ASSETS_PREFIX_NO_TRAILING_SLASH = normalizePathWithFallback( diff --git a/superset-frontend/src/utils/navigationUtils.AppLink.test.tsx b/superset-frontend/src/utils/navigationUtils.AppLink.test.tsx new file mode 100644 index 00000000000..78709b75185 --- /dev/null +++ b/superset-frontend/src/utils/navigationUtils.AppLink.test.tsx @@ -0,0 +1,95 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render } from 'spec/helpers/testing-library'; +import { AppLink } from 'src/utils/navigationUtils'; + +// AppLink renders a real React element via React Testing Library, which is +// incompatible with the withApplicationRoot fixture's `jest.resetModules()` +// (it corrupts the testing-library module graph). Mock applicationRoot at +// file scope and vary per test instead. Variable must start with `mock` to +// satisfy Jest's hoisted-factory out-of-scope check. +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => ({ + __esModule: true, + default: () => ({ + common: { application_root: '', static_assets_prefix: '' }, + }), + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: () => '', +})); + +beforeEach(() => { + mockApplicationRoot.mockReturnValue(''); +}); + +test('renders an anchor with prefixed href under subdirectory deployment', () => { + mockApplicationRoot.mockReturnValue('/superset'); + const { container } = render(go); + const anchor = container.querySelector('a'); + expect(anchor).not.toBeNull(); + expect(anchor?.getAttribute('href')).toBe('/superset/foo'); +}); + +test('passes through other anchor props', () => { + const { container } = render( + + go + , + ); + const anchor = container.querySelector('a'); + expect(anchor?.getAttribute('target')).toBe('_blank'); + expect(anchor?.getAttribute('rel')).toBe('noreferrer'); +}); + +test('passes absolute URLs through without prefixing', () => { + mockApplicationRoot.mockReturnValue('/superset'); + const { container } = render( + x, + ); + expect(container.querySelector('a')?.getAttribute('href')).toBe( + 'https://external.example.com', + ); +}); + +// AppLink runs `assertSafeNavigationUrl(ensureAppRoot(href))` during render, so +// an unsafe href throws rather than emitting an anchor that points at the +// attacker-controlled target. Mirror the openInNewTab / getShareableUrl pins. +test('throws on a backslash-laden authority-spoof href', () => { + mockApplicationRoot.mockReturnValue('/superset'); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + try { + expect(() => render(x)).toThrow( + /refused unsafe URL/, + ); + } finally { + errorSpy.mockRestore(); + } +}); + +test('throws on a protocol-relative href', () => { + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + try { + expect(() => + render(x), + ).toThrow(/refused unsafe URL/); + } finally { + errorSpy.mockRestore(); + } +}); diff --git a/superset-frontend/src/utils/navigationUtils.appRoot.test.tsx b/superset-frontend/src/utils/navigationUtils.appRoot.test.tsx new file mode 100644 index 00000000000..e623f00b1d3 --- /dev/null +++ b/superset-frontend/src/utils/navigationUtils.appRoot.test.tsx @@ -0,0 +1,136 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { applicationRootScenarios } from 'spec/helpers/withApplicationRoot'; + +// Subdirectory regression for the nine `ensureAppRoot(...)` value-wrap +// callsites. Each row pins one callsite's input value +// (what the migrated component now passes to `ensureAppRoot`) and asserts the +// resulting URL under both the root deployment (`''`) and the `/superset` +// subdirectory deployment. +// +// Why this shape rather than per-callsite rendered-component tests: every +// migrated callsite is a value-only wrap (plain `
`, +// antd `