fix(subdirectory): close adversarial-review gaps in helpers, backend models, and FAB links

Adversarial review surfaced six classes of subdirectory-deployment gaps not
covered by the existing TDD scaffold. Each is fixed where it lives, with
pinning tests added beside the change:

Helpers
- navigationUtils: drop `//` from the navigation safety regex so
  `openInNewTab('//evil.com')` can no longer open a cross-origin tab
- pathUtils.stripAppRoot: greedy strip so an upstream `/superset/superset/x`
  payload survives one strip + react-router basename re-prepend
- RedirectWarning.isAllowedScheme: explicit `//` guard; the `new URL(...)`
  catch branch was silently allowing protocol-relative URLs through
- SupersetClientClass.getUrl: implement the runtime appRoot dedupe the
  project memory was already documenting. Flips the contract test from
  pinning the doubled shape under a misleading name to asserting single-
  prefix emission with segment-boundary + bare-root coverage

Frontend literals and sinks
- loggerMiddleware: `/superset/log/` -> `/log/` (matches the live route
  after `Superset.route_base = ""`); updated three test fixtures
- DatasetPanel: raw `window.open(explore_url)` -> `openInNewTab` with null guard
- RedirectWarning: raw `window.location.href = targetUrl` -> `redirect()`
  so the helpers' validation applies

Backend literals and sinks
- Slice.explore_json_url: `/superset/explore_json` -> `/explore_json`
- Database.sql_url: `/superset/sql/<id>/` (route no longer exists) ->
  `/sqllab/?dbid=<id>` (the live SQL Lab deep-link)
- tasks/async_queries.result_url: same `/superset/` strip
- initialization Home menu: hardcoded `href="/superset/welcome/"` ->
  `f"{app_root}/welcome/"` so it works under any application_root

FAB list-view raw HTML
- dashboard_link / slice_link render raw `<a href=...>` strings, which do
  not receive SCRIPT_NAME at render time. Migrated both to `url_for`
  (`Superset.dashboard` / `ExploreView.root`) so subdir deployments emit
  single-prefix hrefs. The model properties themselves keep their
  router-relative shape for frontend callers using ensureAppRoot

Tests
- test_subdirectory_url_for.py grew from 7 to 11 cases pinning
  Slice.explore_json_url, Database.sql_url, dashboard_link, and slice_link
  under SCRIPT_NAME=/superset
- 82 helper Jest tests + 71 touched component tests green; pre-commit clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts

@@ -275,8 +275,23 @@ export default class SupersetClientClass {
     const host = inputHost ?? this.host;
     const cleanHost = host.slice(-1) === '/' ? host.slice(0, -1) : host; // no backslash
 
+    // Strip a leading appRoot segment so callers that accidentally pre-prefix
+    // their endpoint (e.g. by wrapping with ensureAppRoot before passing to the
+    // client) do not produce a doubled `/superset/superset/...` URL. The L2
+    // static invariant still flags this pattern as a migration issue; this is
+    // the runtime safety net.
+    let cleanEndpoint = endpoint;
+    const root = this.appRoot;
+    if (root) {
+      if (cleanEndpoint === root) {
+        cleanEndpoint = '';
+      } else if (cleanEndpoint.startsWith(`${root}/`)) {
+        cleanEndpoint = cleanEndpoint.slice(root.length);
+      }
+    }
+
     return `${this.protocol}//${cleanHost}${this.appRoot}/${
-      endpoint[0] === '/' ? endpoint.slice(1) : endpoint
+      cleanEndpoint[0] === '/' ? cleanEndpoint.slice(1) : cleanEndpoint
     }`;
   }
 }

superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts

@@ -42,12 +42,25 @@ describe('SupersetClient applies the application root exactly once', () => {
     );
   });
 
-  // Documents the double-prefix symptom: wrapping the endpoint in
-  // ensureAppRoot before passing it to SupersetClient duplicates the root.
-  // navigationUtils.invariants.test.ts catches this pattern statically.
-  test('does not double-apply the application root when caller pre-prefixes', () => {
+  // Runtime safety net: if a caller pre-prefixes the endpoint (e.g. by wrapping
+  // with ensureAppRoot before calling), getUrl strips the duplicate. The L2
+  // static invariant still flags the pattern at the call site — this guards
+  // against the bug reaching production if the static check is bypassed.
+  test('dedupes a leading application-root segment from a pre-prefixed endpoint', () => {
     expect(buildClient().getUrl({ endpoint: '/superset/api/v1/chart' })).toBe(
-      'https://config_host/superset/superset/api/v1/chart',
+      'https://config_host/superset/api/v1/chart',
+    );
+  });
+
+  test('dedupe is segment-boundary aware — `/supersetfoo` is not a prefix match', () => {
+    expect(buildClient().getUrl({ endpoint: '/supersetfoo/x' })).toBe(
+      'https://config_host/superset/supersetfoo/x',
+    );
+  });
+
+  test('dedupes the bare application root to an empty endpoint', () => {
+    expect(buildClient().getUrl({ endpoint: '/superset' })).toBe(
+      'https://config_host/superset/',
     );
   });
 

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'),

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx

@@ -29,7 +29,7 @@ import * as chartCustomizationActions from '../../actions/chartCustomizationActi
 
 fetchMock.get('glob:*/csstemplateasyncmodelview/api/read', {});
 fetchMock.put('glob:*/api/v1/dashboard/*/colors*', {});
-fetchMock.post('glob:*/superset/log/?*', {});
+fetchMock.post('glob:*/log/?*', {});
 
 jest.mock('@visx/responsive', () => ({
   ParentSize: ({

superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx

@@ -26,6 +26,7 @@ import Table, {
   TableSize,
 } from '@superset-ui/core/components/Table';
 import { DatasetObject } from 'src/features/datasets/AddDataset/types';
+import { openInNewTab } from 'src/utils/navigationUtils';
 import { ITableColumn } from './types';
 import MessageContent from './MessageContent';
 
@@ -227,11 +228,9 @@ const renderExistingDatasetAlert = (dataset?: DatasetObject) => (
         <span
           role="button"
           onClick={() => {
-            window.open(
-              dataset?.explore_url,
-              '_blank',
-              'noreferrer noopener popup=false',
-            );
+            if (dataset?.explore_url) {
+              openInNewTab(dataset.explore_url);
+            }
           }}
           tabIndex={0}
           className="view-dataset-button"

superset-frontend/src/middleware/logger.test.ts

@@ -88,13 +88,13 @@ describe('logger middleware', () => {
     expect(next.mock.calls.length).toBe(1);
   });
 
-  test('should POST an event to /superset/log/ when called', () => {
+  test('should POST an event to /log/ when called', () => {
     (logger as Function)(mockStore)(next)(action);
     expect(next.mock.calls.length).toBe(0);
 
     jest.advanceTimersByTime(2000);
     expect(postStub.mock.calls.length).toBe(1);
-    expect(postStub.mock.calls[0][0].endpoint).toMatch('/superset/log/');
+    expect(postStub.mock.calls[0][0].endpoint).toMatch('/log/');
   });
 
   test('should include ts, start_offset, event_name, impression_id, source, and source_id in every event', () => {
@@ -165,7 +165,7 @@ describe('logger middleware', () => {
 
     expect(beaconMock.mock.calls.length).toBe(1);
     const endpoint = beaconMock.mock.calls[0][0];
-    expect(endpoint).toMatch('/superset/log/');
+    expect(endpoint).toMatch('/log/');
   });
 
   test('should pass a guest token to sendBeacon if present', () => {

superset-frontend/src/middleware/loggerMiddleware.ts

@@ -33,7 +33,12 @@ import { ensureAppRoot } from '../utils/navigationUtils';
 import type { DashboardInfo, DashboardLayoutState } from '../dashboard/types';
 import type { QueryEditor } from '../SqlLab/types';
 
-type LogEventSource = 'dashboard' | 'embedded_dashboard' | 'explore' | 'sqlLab' | 'slice';
+type LogEventSource =
+  | 'dashboard'
+  | 'embedded_dashboard'
+  | 'explore'
+  | 'sqlLab'
+  | 'slice';
 
 interface LogEventData {
   source?: LogEventSource;
@@ -88,7 +93,7 @@ interface LoggerStore {
   dispatch: Dispatch;
 }
 
-const LOG_ENDPOINT = '/superset/log/?explode=events';
+const LOG_ENDPOINT = '/log/?explode=events';
 
 const sendBeacon = (events: LogEventData[]): void => {
   if (events.length <= 0) {

superset-frontend/src/pages/RedirectWarning/index.tsx

@@ -100,7 +100,7 @@ export default function RedirectWarning() {
   // Redirect immediately if the URL is already trusted
   useEffect(() => {
     if (targetUrl && isAllowedScheme(targetUrl) && isUrlTrusted(targetUrl)) {
-      window.location.href = targetUrl;
+      redirect(targetUrl);
     }
   }, [targetUrl]);
 
@@ -109,7 +109,7 @@ export default function RedirectWarning() {
     if (trustChecked) {
       trustUrl(targetUrl);
     }
-    window.location.href = targetUrl;
+    redirect(targetUrl);
   }, [trustChecked, targetUrl]);
 
   const handleReturn = useCallback(() => {

superset-frontend/src/pages/RedirectWarning/utils.ts

@@ -36,8 +36,14 @@ function normalizeUrl(url: string): string {
 /**
  * Return true if the URL scheme is safe for navigation.
  * Blocks javascript:, data:, vbscript:, file:, etc.
+ *
+ * Protocol-relative URLs (`//host/...`) are rejected because they perform a
+ * cross-origin navigation despite parsing as "relative" — the standalone
+ * `new URL(...)` call throws on them, so without an explicit guard the catch
+ * branch would let them through.
  */
 export function isAllowedScheme(url: string): boolean {
+  if (url.startsWith('//')) return false;
   try {
     const parsed = new URL(url);
     return ALLOWED_SCHEMES.includes(parsed.protocol);

superset-frontend/src/utils/navigationUtils.test.ts

@@ -102,6 +102,16 @@ describe('openInNewTab', () => {
       expect(features).toContain('noreferrer');
     });
   });
+
+  test('refuses protocol-relative URLs to block open-redirect via `//evil.com`', async () => {
+    await withApplicationRoot('/superset/', async () => {
+      const { openInNewTab } = await import('src/utils/navigationUtils');
+      expect(() => openInNewTab('//evil.example.com/phish')).toThrow(
+        /refused unsafe URL/,
+      );
+      expect(openSpy).not.toHaveBeenCalled();
+    });
+  });
 });
 
 describe('redirect', () => {

superset-frontend/src/utils/navigationUtils.ts

@@ -60,12 +60,12 @@ export function navigateWithState(
 
 const NEW_TAB_FEATURES = 'noopener noreferrer';
 
-// Allow-list of safe URL shapes for navigation: relative paths, protocol-
-// relative URLs, and a small set of known-safe schemes. `ensureAppRoot`
-// already neutralises `javascript:` / `data:` by prefixing them as relative
-// paths, but checking here gives CodeQL a locally-visible sanitiser on the
-// sinks below.
-const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|\/\/|https?:|ftp:|mailto:|tel:)/i;
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
 
 function assertSafeNavigationUrl(url: string): string {
   if (!SAFE_NAVIGATION_URL_RE.test(url)) {

superset-frontend/src/utils/pathUtils.test.ts

@@ -294,6 +294,17 @@ test('stripAppRoot handles nested application roots', async () => {
   expect(stripAppRoot('/welcome/')).toBe('/welcome/');
 });
 
+test('stripAppRoot greedily removes a doubled application root', async () => {
+  const { stripAppRoot } = await loadPathUtils('/superset/');
+
+  // Upstream double-prefix bug: ensure both copies are stripped so a downstream
+  // react-router basename re-prepend produces a single, correct prefix.
+  expect(stripAppRoot('/superset/superset/welcome/')).toBe('/welcome/');
+  expect(stripAppRoot('/superset/superset/superset/welcome/')).toBe(
+    '/welcome/',
+  );
+});
+
 test('stripAppRoot passes absolute and protocol-relative URLs through', async () => {
   const { stripAppRoot } = await loadPathUtils('/superset/');
 

superset-frontend/src/utils/pathUtils.ts

@@ -89,9 +89,12 @@ export function stripAppRoot(path: string): string {
   }
   const root = applicationRoot();
   if (!root) return path;
-  if (path === root) return '/';
-  if (path.startsWith(`${root}/`)) {
-    return path.slice(root.length);
+  let stripped = path;
+  // Greedy strip handles upstream double-prefix bugs (e.g. a backend payload
+  // that emitted `/superset/superset/x`). Without this, a single pass would
+  // leave one stale prefix, which react-router's `basename` would then re-add.
+  while (stripped === root || stripped.startsWith(`${root}/`)) {
+    stripped = stripped === root ? '/' : stripped.slice(root.length);
   }
-  return path;
+  return stripped;
 }

superset/initialization/__init__.py

@@ -299,10 +299,15 @@ class SupersetAppInitializer:  # pylint: disable=too-many-public-methods
         if app_root.endswith("/"):
             app_root = app_root.rstrip("/")
 
+        # FAB renders this href verbatim in menus and does not apply
+        # SCRIPT_NAME at render time, so the application_root has to be baked
+        # in at registration. The previous hardcoded `/superset/welcome/`
+        # collided with non-`/superset/` deployments AND with the new
+        # `Superset.route_base = ""`. `app_root` is normalized above.
         appbuilder.add_link(
             "Home",
             label=_("Home"),
-            href="/superset/welcome/",
+            href=f"{app_root}/welcome/",
             cond=lambda: bool(current_app.config["LOGO_TARGET_PATH"]),
         )
 

superset/models/core.py

@@ -1166,7 +1166,11 @@ class Database(CoreDatabase, AuditMixinNullable, ImportExportMixin):  # pylint:
 
     @property
     def sql_url(self) -> str:
-        return f"/superset/sql/{self.id}/"
+        # SQL Lab moved to its own blueprint at /sqllab/; the legacy
+        # /superset/sql/<id>/ route was removed when Superset.route_base
+        # collapsed to "". Deep-link by databaseId instead so this property
+        # resolves to a live route under any application_root.
+        return f"/sqllab/?dbid={self.id}"
 
     @hybrid_property
     def perm(self) -> str:

superset/models/dashboard.py

@@ -22,7 +22,7 @@ from collections import defaultdict, deque
 from typing import Any, Callable
 
 import sqlalchemy as sqla
-from flask import current_app as app
+from flask import current_app as app, url_for
 from flask_appbuilder import Model
 from flask_appbuilder.models.decorators import renders
 from flask_appbuilder.security.sqla.models import User
@@ -221,7 +221,12 @@ class Dashboard(CoreDashboard, AuditMixinNullable, ImportExportMixin):
     @renders("dashboard_title")
     def dashboard_link(self) -> Markup:
         title = escape(self.dashboard_title or "<empty>")
-        return Markup(f'<a href="{self.url}">{title}</a>')
+        # FAB list view renders this raw HTML; use url_for so Flask prepends
+        # SCRIPT_NAME (the application_root) and the row link works under
+        # subdirectory deployments. `Dashboard.url` itself stays router-
+        # relative so frontend callers can apply ensureAppRoot exactly once.
+        href = url_for("Superset.dashboard", dashboard_id_or_slug=self.slug or self.id)
+        return Markup(f'<a href="{href}">{title}</a>')
 
     @property
     def digest(self) -> str | None:

superset/models/slice.py

@@ -21,6 +21,7 @@ from typing import Any, TYPE_CHECKING
 from urllib import parse
 
 import sqlalchemy as sqla
+from flask import url_for
 from flask_appbuilder import Model
 from flask_appbuilder.models.decorators import renders
 from markupsafe import escape, Markup
@@ -309,7 +310,7 @@ class Slice(  # pylint: disable=too-many-public-methods
     @property
     def explore_json_url(self) -> str:
         """Defines the url to access the slice"""
-        return self.get_explore_url("/superset/explore_json")
+        return self.get_explore_url("/explore_json")
 
     @property
     def edit_url(self) -> str:
@@ -322,7 +323,11 @@ class Slice(  # pylint: disable=too-many-public-methods
     @property
     def slice_link(self) -> Markup:
         name = escape(self.chart)
-        return Markup(f'<a href="{self.url}">{name}</a>')
+        # FAB list view renders this raw HTML; use url_for so Flask prepends
+        # SCRIPT_NAME (the application_root). `Slice.url` itself stays router-
+        # relative so frontend callers can apply ensureAppRoot exactly once.
+        href = url_for("ExploreView.root", slice_id=self.id)
+        return Markup(f'<a href="{href}">{name}</a>')
 
     @property
     def icons(self) -> str:

superset/tasks/async_queries.py

@@ -169,7 +169,7 @@ def load_explore_json_into_cache(  # pylint: disable=too-many-locals
             set_and_log_cache(
                 cache_instance, cache_key, cache_value, cache_timeout=cache_timeout
             )
-            result_url = f"/superset/explore_json/data/{cache_key}"
+            result_url = f"/explore_json/data/{cache_key}"
             async_query_manager.update_job(
                 job_metadata,
                 async_query_manager.STATUS_DONE,

tests/unit_tests/test_subdirectory_url_for.py

@@ -166,3 +166,67 @@ def test_dashboard_model_url_has_no_route_prefix() -> None:
 
     assert Dashboard.get_url(11) == "/dashboard/11/"
     assert Dashboard.get_url(11, "births") == "/dashboard/births/"
+
+
+def test_slice_explore_json_url_has_no_route_prefix(app_context: None) -> None:
+    """`Slice.explore_json_url` previously baked `/superset/explore_json` into
+    the path. After `Superset.route_base = ""` the rule is `/explore_json/`,
+    so the literal pointed at a non-existent route. Pin the prefix-free shape
+    so downstream `ensureAppRoot` callers prepend the application root once.
+    """
+    from superset.models.slice import Slice
+
+    slc = Slice(id=42)
+    assert slc.explore_json_url.startswith("/explore_json/")
+    assert "/superset/explore_json" not in slc.explore_json_url
+
+
+def test_database_sql_url_resolves_to_live_sqllab_route() -> None:
+    """`Database.sql_url` previously returned `/superset/sql/<id>/` — a route
+    that was removed when SQL Lab moved to its own blueprint at `/sqllab/`.
+    The property now deep-links to SQL Lab via the `dbid` query parameter so
+    it resolves under any application_root.
+    """
+    from superset.models.core import Database
+
+    db = Database(database_name="db", id=7)
+    assert db.sql_url == "/sqllab/?dbid=7"
+
+
+def test_dashboard_link_emits_script_name_under_subdirectory(
+    app_context: None,
+) -> None:
+    """`dashboard_link` renders raw HTML for the FAB list-view "title" column.
+    Flask does not auto-apply SCRIPT_NAME to string fragments, so a literal
+    `/dashboard/<id>/` href routed outside the application_root under
+    subdirectory deployment. Pin that the rendered href now carries the
+    SCRIPT_NAME prefix exactly once.
+    """
+    from flask import current_app
+
+    from superset.models.dashboard import Dashboard
+
+    dash = Dashboard(id=5, slug=None, dashboard_title="t")
+    with current_app.test_request_context(
+        "/", environ_overrides={"SCRIPT_NAME": "/superset"}
+    ):
+        html = str(dash.dashboard_link())
+        assert 'href="/superset/dashboard/5/"' in html
+        assert "/superset/superset/" not in html
+
+
+def test_slice_link_emits_script_name_under_subdirectory(
+    app_context: None,
+) -> None:
+    """Mirror of `dashboard_link` for `Slice.slice_link`."""
+    from flask import current_app
+
+    from superset.models.slice import Slice
+
+    slc = Slice(id=9, slice_name="chart")
+    with current_app.test_request_context(
+        "/", environ_overrides={"SCRIPT_NAME": "/superset"}
+    ):
+        html = str(slc.slice_link)
+        assert 'href="/superset/explore/?slice_id=9"' in html
+        assert "/superset/superset/" not in html