You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/06/11 04:54:14 UTC
(superset) branch claude/subdirectory-helpers-tdd updated: fix(subdirectory): close gap-review blockers in URL prefixing (B1-B6, P1, N3, GAP4)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch claude/subdirectory-helpers-tdd
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/claude/subdirectory-helpers-tdd by this push:
new 226bc60b4c9 fix(subdirectory): close gap-review blockers in URL prefixing (B1-B6, P1, N3, GAP4)
226bc60b4c9 is described below
commit 226bc60b4c9ea8cca04610095b195e254df63a29
Author: Joe Li <jo...@preset.io>
AuthorDate: Wed Jun 10 18:47:35 2026 -0700
fix(subdirectory): close gap-review blockers in URL prefixing (B1-B6, P1, N3, GAP4)
Fixes from a five-agent gap review of the subdirectory URL prefixing
work, each covered by new unit/integration tests:
- Legacy /superset/* shim now recognizes app-root-prefixed legacy URLs
(/myapp/superset/...) by stripping the app root, segment-boundary
aware, before the legacy-prefix check (B1)
- Dashboard query-source attribution matches the SPA /dashboard/ route
rather than the removed /superset/dashboard/ referrer shape (B2)
- SSO provider login hrefs wrapped in ensureAppRoot (B3)
- Extension remoteEntry URLs built via makeUrl so they carry the app
root (B4)
- Slice/Dashboard thumbnail_url built with url_for so SCRIPT_NAME is
honored under subdirectory deployments (B5)
- navigationUtils no longer double-encodes pre-encoded URLs passed to
navigation sinks (B6)
- AppRootMiddleware matches the app root on segment boundaries only and
normalizes a trailing slash; /myapparoo no longer mis-stripped (P1)
- get_explore_redirect_url drops url_for-reserved query keys
(_external, _scheme, _anchor, _method, endpoint) so request query
strings cannot steer URL building (N3)
- DatasetEditor fallback redirect uses url_for instead of a bare "/"
that escaped the application root (GAP4)
Co-Authored-By: Claude Fable 5 <no...@anthropic.com>
---
.../src/extensions/ExtensionsLoader.test.ts | 105 +++++++++++++++++++++
.../src/extensions/ExtensionsLoader.ts | 7 +-
superset-frontend/src/pages/Login/Login.test.tsx | 67 ++++++++++++-
superset-frontend/src/pages/Login/index.tsx | 8 +-
.../src/utils/navigationUtils.test.ts | 40 ++++++++
superset-frontend/src/utils/navigationUtils.ts | 42 +++++++--
superset/app.py | 15 ++-
superset/middleware/legacy_prefix_redirect.py | 15 ++-
superset/models/dashboard.py | 4 +-
superset/models/slice.py | 4 +-
superset/utils/core.py | 5 +-
superset/views/datasource/views.py | 6 +-
superset/views/utils.py | 18 +++-
.../views/test_explore_redirect.py | 20 ++++
tests/unit_tests/initialization_test.py | 59 ++++++++++++
.../middleware/test_legacy_prefix_redirect.py | 41 ++++++++
tests/unit_tests/models/dashboard_test.py | 46 +++++++++
tests/unit_tests/models/slice_test.py | 43 +++++++++
tests/unit_tests/utils/test_core.py | 4 +
19 files changed, 517 insertions(+), 32 deletions(-)
diff --git a/superset-frontend/src/extensions/ExtensionsLoader.test.ts b/superset-frontend/src/extensions/ExtensionsLoader.test.ts
index 1d2493671ad..817d2a8f8b4 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<string, []>(() => '');
+
+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> = {}): Extension {
return {
id: 'test-extension',
@@ -38,8 +51,38 @@ function createMockExtension(overrides: Partial<Extension> = {}): 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) {
+ setTimeout(() => {
+ (element.onerror as any)('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();
@@ -127,6 +170,68 @@ 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<void> {
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<void>((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/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<string, []>(() => '');
+
+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(<Login />, { 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(<Login />, { 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(<Login />, { 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 54e21b5ac5a..d704945d1fd 100644
--- a/superset-frontend/src/pages/Login/index.tsx
+++ b/superset-frontend/src/pages/Login/index.tsx
@@ -101,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;
diff --git a/superset-frontend/src/utils/navigationUtils.test.ts b/superset-frontend/src/utils/navigationUtils.test.ts
index 7858ca15359..d117d627ada 100644
--- a/superset-frontend/src/utils/navigationUtils.test.ts
+++ b/superset-frontend/src/utils/navigationUtils.test.ts
@@ -221,6 +221,18 @@ describe('redirect', () => {
});
});
+ // encodeURI-at-the-sink regression (gap review 2026-06-10): see the
+ // matching navigateWithState tests — `%27`/`%20` must survive un-doubled.
+ test('does not double-encode percent-encoded query strings', async () => {
+ await withApplicationRoot('/superset/', async () => {
+ const { redirect } = await import('src/utils/navigationUtils');
+ redirect('/dashboard/list/?filters=(label:%27a%20b%27)');
+ expect(window.location.href).toBe(
+ '/superset/dashboard/list/?filters=(label:%27a%20b%27)',
+ );
+ });
+ });
+
// AF-1 (2026-05-19, dual-lane adversarial review): backslash-laden URLs
// were accepted by `SAFE_NAVIGATION_URL_RE` and the sibling guards in
// `ensureAppRoot` / `isAllowedScheme`. Browsers normalise `/\` → `//` in
@@ -405,6 +417,34 @@ describe('navigateWithState (AF-1 + nit-1)', () => {
);
});
});
+
+ // encodeURI-at-the-sink regression (gap review 2026-06-10): the CodeQL
+ // `js/html-injection` sanitiser escapes `%` itself, so already-percent-
+ // encoded paths (the URL constructor's output always is) were double-
+ // encoded — `%20` → `%2520` — corrupting the pushed URL.
+ test('does not double-encode percent-encoded rison paths (pushState)', async () => {
+ await withApplicationRoot('', async () => {
+ const { navigateWithState } = await import('src/utils/navigationUtils');
+ navigateWithState('/dashboard/list/?filters=(label:%27a%20b%27)', {});
+ expect(pushSpy).toHaveBeenCalledWith(
+ {},
+ '',
+ '/dashboard/list/?filters=(label:%27a%20b%27)',
+ );
+ });
+ });
+
+ test('single-encodes raw spaces and unicode path segments (pushState)', async () => {
+ await withApplicationRoot('/superset/', async () => {
+ const { navigateWithState } = await import('src/utils/navigationUtils');
+ navigateWithState('/dashboard/百分比 slug/', {});
+ expect(pushSpy).toHaveBeenCalledWith(
+ {},
+ '',
+ '/superset/dashboard/%E7%99%BE%E5%88%86%E6%AF%94%20slug/',
+ );
+ });
+ });
});
describe('getShareableUrl', () => {
diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts
index 4c72d63b8dc..f58e16331b4 100644
--- a/superset-frontend/src/utils/navigationUtils.ts
+++ b/superset-frontend/src/utils/navigationUtils.ts
@@ -103,6 +103,18 @@ const FORBIDDEN_CONTROL_CHARS_RE = new RegExp(
'\\u2028\\u2029\\uFEFF]',
);
+// `encodeURI` (kept at the sinks below as CodeQL's recognised
+// `js/html-injection` sanitiser) escapes `%` itself, so an already-
+// percent-encoded path — and `parsed.pathname`/`.search`/`.hash` from the
+// URL constructor always are — would be double-encoded (`%20` → `%2520`)
+// and the navigation would land on the wrong URL. Mapping `%25XX`
+// (XX = hex) back to `%XX` after the encodeURI pass restores the
+// constructor's escape sequences, while anything encodeURI legitimately
+// escaped (no such character survives the URL constructor, but defence-
+// in-depth) stays encoded: `%` is the only character encodeURI escapes
+// that the WHATWG URL serialiser emits raw.
+const DOUBLE_ENCODED_ESCAPE_RE = /%25([0-9A-Fa-f]{2})/g;
+
function assertSafeNavigationUrl(url: string): string {
if (FORBIDDEN_CONTROL_CHARS_RE.test(url)) {
throw new Error(
@@ -164,9 +176,10 @@ function assertSafeNavigationUrl(url: string): string {
* (`encodeURI`, `encodeURIComponent`, `DOMPurify.sanitize`, …) and
* does NOT propagate startsWith/equality barriers from `target` or
* `parsed.*` through the URL-property derivation into the assigned
- * string. `encodeURI` is idempotent on already-URL-normalised paths
- * (`parsed.pathname` is produced by the URL constructor), so it is
- * a no-op at runtime for legitimate paths.
+ * string. Because `encodeURI` escapes `%` itself (it is NOT
+ * idempotent on percent-encoded input), each sink restores the
+ * `%XX` escape sequences afterwards via DOUBLE_ENCODED_ESCAPE_RE,
+ * making the pair a runtime no-op for legitimate paths.
*
* External-URL paths get their own block with `URL.protocol` literal-
* equality + userinfo guards on the URL-constructor's normalised `href`.
@@ -221,10 +234,15 @@ export function navigateTo(
// satisfy `js/url-redirection` and `js/xss-through-dom`, but
// `js/html-injection`'s default model only recognises specific
// through-function sanitisers (`encodeURI`, `encodeURIComponent`,
- // `DOMPurify.sanitize`, …). `encodeURI` is idempotent on already-
- // URL-normalised paths (`parsed.pathname` is produced by the URL
- // constructor), so this is a no-op at runtime for legitimate paths.
- const sinkValue = encodeURI(safePath);
+ // `DOMPurify.sanitize`, …). `encodeURI` escapes `%` itself, which
+ // would double-encode the constructor-normalised path, so the
+ // follow-up replace restores the `%XX` escape sequences (see
+ // DOUBLE_ENCODED_ESCAPE_RE) — net effect: a runtime no-op for
+ // legitimate paths with the sanitiser still visible at the sink.
+ const sinkValue = encodeURI(safePath).replace(
+ DOUBLE_ENCODED_ESCAPE_RE,
+ '%$1',
+ );
if (options?.newWindow) {
window.open(sinkValue, '_blank', NEW_TAB_FEATURES);
} else if (options?.assign) {
@@ -293,7 +311,10 @@ export function navigateTo(
!safeHome.includes('\\')
) {
// See router-relative branch above for `encodeURI` rationale.
- window.location.href = encodeURI(safeHome);
+ window.location.href = encodeURI(safeHome).replace(
+ DOUBLE_ENCODED_ESCAPE_RE,
+ '%$1',
+ );
}
}
}
@@ -344,7 +365,10 @@ export function navigateWithState(
!safePath.includes('\\')
) {
// See `navigateTo` for `encodeURI` rationale.
- const sinkValue = encodeURI(safePath);
+ const sinkValue = encodeURI(safePath).replace(
+ DOUBLE_ENCODED_ESCAPE_RE,
+ '%$1',
+ );
if (options?.replace) {
window.history.replaceState(state, '', sinkValue);
} else {
diff --git a/superset/app.py b/superset/app.py
index 71ea51e9684..cda77095864 100644
--- a/superset/app.py
+++ b/superset/app.py
@@ -218,15 +218,20 @@ class AppRootMiddleware:
app_root: str,
):
self.wsgi_app = wsgi_app
- self.app_root = app_root
+ # Normalize a trailing slash so "/myapp" and "/myapp/" configure the
+ # same prefix and the segment-boundary check below stays uniform.
+ self.app_root = app_root.rstrip("/")
def __call__(
self, environ: WSGIEnvironment, start_response: StartResponse
) -> Iterable[bytes]:
original_path_info = environ.get("PATH_INFO", "")
- if original_path_info.startswith(self.app_root):
- environ["PATH_INFO"] = original_path_info.removeprefix(self.app_root)
+ # Segment-boundary match: accept "/myapp" and "/myapp/..." but never
+ # a path that merely shares a string prefix (e.g. "/myapparoo/...").
+ if original_path_info == self.app_root or original_path_info.startswith(
+ self.app_root + "/"
+ ):
+ environ["PATH_INFO"] = original_path_info[len(self.app_root) :]
environ["SCRIPT_NAME"] = self.app_root
return self.wsgi_app(environ, start_response)
- else:
- return NotFound()(environ, start_response)
+ return NotFound()(environ, start_response)
diff --git a/superset/middleware/legacy_prefix_redirect.py b/superset/middleware/legacy_prefix_redirect.py
index a96890abcc2..398f403f845 100644
--- a/superset/middleware/legacy_prefix_redirect.py
+++ b/superset/middleware/legacy_prefix_redirect.py
@@ -190,13 +190,24 @@ class LegacyPrefixRedirectMiddleware:
path_info: str = environ.get("PATH_INFO", "")
method: str = environ.get("REQUEST_METHOD", "GET").upper()
+ # Under a subdirectory deployment, legacy bookmarks carry the app
+ # root too (`/myapp/superset/dashboard/1/`) — this shim sits outside
+ # AppRootMiddleware, so the raw PATH_INFO still has the app-root
+ # prefix. Strip it (segment-boundary aware) before the legacy-prefix
+ # check so both `/superset/...` and `{app_root}/superset/...` forms
+ # are recognised. The Location below is built from the captured
+ # app_root either way, so both forms 308 to the same canonical URL.
+ candidate = path_info
+ if self.app_root_prefix and candidate.startswith(self.app_root_prefix + "/"):
+ candidate = candidate[len(self.app_root_prefix) :]
+
# Cheap exit: not a /superset path at all.
if not (
- path_info == _LEGACY_PREFIX or path_info.startswith(_LEGACY_PREFIX + "/")
+ candidate == _LEGACY_PREFIX or candidate.startswith(_LEGACY_PREFIX + "/")
):
return self.wsgi_app(environ, start_response)
- canonical_after_strip = path_info[len(_LEGACY_PREFIX) :] or "/"
+ canonical_after_strip = candidate[len(_LEGACY_PREFIX) :] or "/"
match = _match(canonical_after_strip)
if match is None:
# Unenumerated /superset path — closed-set discipline: pass
diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py
index 794cc949704..eec6682cee3 100644
--- a/superset/models/dashboard.py
+++ b/superset/models/dashboard.py
@@ -243,7 +243,9 @@ class Dashboard(CoreDashboard, AuditMixinNullable, ImportExportMixin):
if the dashboard has changed
"""
if digest := self.digest:
- return f"/api/v1/dashboard/{self.id}/thumbnail/{digest}/"
+ # url_for respects SCRIPT_NAME, so the URL carries the application
+ # root prefix under subdirectory deployments.
+ return url_for("DashboardRestApi.thumbnail", pk=self.id, digest=digest)
return None
diff --git a/superset/models/slice.py b/superset/models/slice.py
index d8e8858ec6c..510f72a199c 100644
--- a/superset/models/slice.py
+++ b/superset/models/slice.py
@@ -245,7 +245,9 @@ class Slice( # pylint: disable=too-many-public-methods
if the dashboard has changed
"""
if digest := self.digest:
- return f"/api/v1/chart/{self.id}/thumbnail/{digest}/"
+ # url_for respects SCRIPT_NAME, so the URL carries the application
+ # root prefix under subdirectory deployments.
+ return url_for("ChartRestApi.thumbnail", pk=self.id, digest=digest)
return None
diff --git a/superset/utils/core.py b/superset/utils/core.py
index 045450b868e..110255081c0 100644
--- a/superset/utils/core.py
+++ b/superset/utils/core.py
@@ -2172,7 +2172,10 @@ def to_int(v: Any, value_if_invalid: int = 0) -> int:
def get_query_source_from_request() -> QuerySource | None:
if not request or not request.referrer:
return None
- if "/superset/dashboard/" in request.referrer:
+ # Match the SPA dashboard route (/dashboard/<id>/). The bare segment also
+ # covers legacy /superset/dashboard/ referrers and any application-root
+ # prefix (e.g. /myapp/dashboard/1/).
+ if "/dashboard/" in request.referrer:
return QuerySource.DASHBOARD
if "/explore/" in request.referrer:
return QuerySource.CHART
diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py
index 9a420bb10b1..00e80a46621 100644
--- a/superset/views/datasource/views.py
+++ b/superset/views/datasource/views.py
@@ -17,7 +17,7 @@
from collections import Counter
from typing import Any
-from flask import redirect, request
+from flask import redirect, request, url_for
from flask_appbuilder import expose, permission_name
from flask_appbuilder.api import rison as parse_rison
from flask_appbuilder.security.decorators import has_access, has_access_api
@@ -278,4 +278,6 @@ class DatasetEditor(BaseSupersetView):
dev = request.args.get("testing")
if dev is not None:
return super().render_app_template()
- return redirect("/")
+ # url_for keeps the redirect inside the application root under
+ # subdirectory deployments (a bare "/" would escape the prefix).
+ return redirect(url_for("Superset.welcome"))
diff --git a/superset/views/utils.py b/superset/views/utils.py
index 2feec157a7d..adfa89cf958 100644
--- a/superset/views/utils.py
+++ b/superset/views/utils.py
@@ -214,6 +214,14 @@ def loads_request_json(request_json_data: str) -> dict[Any, Any]:
return parsed if isinstance(parsed, dict) else {}
+#: Parameter names `url_for` interprets itself rather than appending to the
+#: query string. Request-supplied query keys matching these must never be
+#: forwarded as `url_for` kwargs.
+_RESERVED_URL_FOR_KWARGS = frozenset(
+ {"endpoint", "_external", "_scheme", "_anchor", "_method"}
+)
+
+
def get_explore_redirect_url() -> str | None: # noqa: C901
"""Construct the `/explore/?form_data_key=...` redirect URL, or None.
@@ -320,7 +328,15 @@ def get_explore_redirect_url() -> str | None: # noqa: C901
if form_data_key:
query_multi.pop("form_data", None)
query_multi["form_data_key"] = [form_data_key]
- query: dict[str, str] = {k: vals[0] for k, vals in query_multi.items() if vals}
+ # Drop keys that collide with `url_for`'s own parameters: a query string
+ # like `?_external=1&_scheme=ftp` would otherwise steer URL building
+ # (absolute URLs, scheme injection, fragment injection), and `?endpoint=x`
+ # would raise TypeError on the duplicated positional argument.
+ query: dict[str, str] = {
+ k: vals[0]
+ for k, vals in query_multi.items()
+ if vals and k not in _RESERVED_URL_FOR_KWARGS
+ }
target_url = url_for("ExploreView.root", **query)
# Loop guard: if the current request is already at the redirect target
diff --git a/tests/integration_tests/views/test_explore_redirect.py b/tests/integration_tests/views/test_explore_redirect.py
index d564d24d720..8992908f129 100644
--- a/tests/integration_tests/views/test_explore_redirect.py
+++ b/tests/integration_tests/views/test_explore_redirect.py
@@ -243,6 +243,26 @@ class TestExploreRedirect(SupersetTestCase):
rv = self.client.get(f"/explore/?form_data={quote(json.dumps(form_data))}")
assert rv.status_code == 200
+ @pytest.mark.usefixtures("load_energy_table_with_slice")
+ @mock.patch("superset.commands.explore.form_data.create.CreateFormDataCommand.run")
+ def test_explore_reserved_url_for_kwargs_stripped(self, mock_run: mock.Mock):
+ """Query keys colliding with ``url_for``'s own parameters must not
+ steer URL building: ``_external``/``_scheme`` would mint an absolute
+ URL with an attacker-chosen scheme, ``_anchor`` injects a fragment,
+ and ``endpoint`` raises TypeError on the duplicated positional
+ argument (500). All five reserved names are dropped from the
+ redirect target.
+ """
+ mock_run.return_value = "random_key"
+ self.login(ADMIN_USERNAME)
+ form_data = {"datasource": "1__table"}
+ rv = self.client.get(
+ f"/explore/?form_data={quote(json.dumps(form_data))}"
+ "&_external=1&_scheme=ftp&_anchor=evil&_method=POST&endpoint=x"
+ )
+ assert rv.status_code == 302
+ assert rv.headers["Location"] == "/explore/?form_data_key=random_key"
+
@pytest.mark.usefixtures("load_energy_table_with_slice")
@mock.patch("superset.commands.explore.form_data.create.CreateFormDataCommand.run")
def test_superset_explore_typed_entry_redirect_when_form_data_present(
diff --git a/tests/unit_tests/initialization_test.py b/tests/unit_tests/initialization_test.py
index c5897a63743..bd9306bc482 100644
--- a/tests/unit_tests/initialization_test.py
+++ b/tests/unit_tests/initialization_test.py
@@ -19,6 +19,8 @@ import os
from unittest.mock import MagicMock, patch
from sqlalchemy.exc import OperationalError
+from werkzeug.test import Client
+from werkzeug.wrappers import Response
from superset.app import AppRootMiddleware, create_app, SupersetApp
from superset.initialization import SupersetAppInitializer
@@ -275,3 +277,60 @@ class TestCreateAppRoot:
inner = _unwrap_to_app_root(app)
assert isinstance(inner, AppRootMiddleware)
assert inner.app_root == "/from-param"
+
+
+class TestAppRootMiddlewareBoundary:
+ """Direct PATH_INFO handling tests for AppRootMiddleware."""
+
+ @staticmethod
+ def _make(app_root: str):
+ captured: dict[str, str | None] = {}
+
+ def inner_app(environ, start_response):
+ captured["PATH_INFO"] = environ.get("PATH_INFO")
+ captured["SCRIPT_NAME"] = environ.get("SCRIPT_NAME")
+ start_response("200 OK", [("Content-Type", "text/plain")])
+ return [b"OK"]
+
+ return AppRootMiddleware(inner_app, app_root), captured
+
+ @staticmethod
+ def _call(middleware, path: str) -> str:
+ client = Client(middleware, response_wrapper=Response)
+ return str(client.get(path).status_code)
+
+ def test_strips_prefix_and_sets_script_name(self):
+ middleware, captured = self._make("/myapp")
+ status = self._call(middleware, "/myapp/dashboard/1/")
+ assert status.startswith("200")
+ assert captured["PATH_INFO"] == "/dashboard/1/"
+ assert captured["SCRIPT_NAME"] == "/myapp"
+
+ def test_exact_app_root_path_is_accepted(self):
+ middleware, captured = self._make("/myapp")
+ status = self._call(middleware, "/myapp")
+ assert status.startswith("200")
+ assert captured["PATH_INFO"] == ""
+ assert captured["SCRIPT_NAME"] == "/myapp"
+
+ def test_shared_string_prefix_is_404_not_stripped(self):
+ """Segment-boundary pin: "/myapparoo/..." merely shares a string
+ prefix with app_root "/myapp" and must 404, not be mangled into
+ PATH_INFO "aroo/..."."""
+ middleware, captured = self._make("/myapp")
+ status = self._call(middleware, "/myapparoo/dashboard/1/")
+ assert status.startswith("404")
+ assert "PATH_INFO" not in captured
+
+ def test_path_outside_app_root_is_404(self):
+ middleware, captured = self._make("/myapp")
+ status = self._call(middleware, "/other/welcome/")
+ assert status.startswith("404")
+ assert "PATH_INFO" not in captured
+
+ def test_trailing_slash_app_root_is_normalized(self):
+ middleware, captured = self._make("/myapp/")
+ status = self._call(middleware, "/myapp/welcome/")
+ assert status.startswith("200")
+ assert captured["PATH_INFO"] == "/welcome/"
+ assert captured["SCRIPT_NAME"] == "/myapp"
diff --git a/tests/unit_tests/middleware/test_legacy_prefix_redirect.py b/tests/unit_tests/middleware/test_legacy_prefix_redirect.py
index 3a3a546ca11..0103adf98e5 100644
--- a/tests/unit_tests/middleware/test_legacy_prefix_redirect.py
+++ b/tests/unit_tests/middleware/test_legacy_prefix_redirect.py
@@ -153,6 +153,47 @@ def test_legacy_get_redirects_308_under_subdir(legacy: str, canonical: str) -> N
assert resp.headers["Location"] == f"/myapp{canonical}"
+@pytest.mark.parametrize(("legacy", "canonical"), _GET_REDIRECT_ROWS)
+def test_app_root_prefixed_legacy_get_redirects_308_under_subdir(
+ legacy: str, canonical: str
+) -> None:
+ """Under subdir deployment, legacy bookmarks saved from a pre-upgrade
+ deployment under the same subdirectory carry the app root too:
+ `/myapp/superset/<canonical>`. The shim strips the captured app root
+ before the legacy-prefix check, so both the bare and the app-root-
+ prefixed legacy forms 308 to the same `/myapp<canonical>` target."""
+ client = _build_client(app_root="/myapp")
+ resp = client.get(f"/myapp{legacy}")
+ assert resp.status_code == 308
+ assert resp.headers["Location"] == f"/myapp{canonical}"
+
+
+def test_app_root_prefixed_legacy_preserves_query_string() -> None:
+ """Query strings survive the app-root-prefixed legacy redirect."""
+ client = _build_client(app_root="/myapp")
+ resp = client.get("/myapp/superset/explore/?form_data_key=abc")
+ assert resp.status_code == 308
+ assert resp.headers["Location"] == "/myapp/explore/?form_data_key=abc"
+
+
+def test_app_root_strip_respects_segment_boundary() -> None:
+ """A path that merely shares a string prefix with the app root (e.g.
+ `/myapparoo/...` under `app_root=/myapp`) is NOT treated as app-root-
+ prefixed — the strip only fires on `{app_root}/`."""
+ client = _build_client(app_root="/myapp")
+ resp = client.get("/myapparoo/superset/welcome/")
+ assert resp.status_code == 200
+ assert resp.data == _SENTINEL_BODY
+
+
+def test_app_root_prefixed_unenumerated_path_passes_through() -> None:
+ """Closed-set discipline holds for the app-root-prefixed form too."""
+ client = _build_client(app_root="/myapp")
+ resp = client.get("/myapp/superset/not-a-real-route/")
+ assert resp.status_code == 200
+ assert resp.data == _SENTINEL_BODY
+
+
@pytest.mark.parametrize(("legacy", "canonical"), _POST_BODY_PRESERVED_ROWS)
def test_legacy_post_body_preserved_308(legacy: str, canonical: str) -> None:
"""POST against a POST-capable canonical → 308 (308 is body-
diff --git a/tests/unit_tests/models/dashboard_test.py b/tests/unit_tests/models/dashboard_test.py
index 10ac3a2b63e..7f2a9cfb86a 100644
--- a/tests/unit_tests/models/dashboard_test.py
+++ b/tests/unit_tests/models/dashboard_test.py
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
+from unittest.mock import patch, PropertyMock
+
from flask import current_app
from superset.models.dashboard import Dashboard
@@ -66,3 +68,47 @@ def test_dashboard_link_renders_plain_slug(app_context: None) -> None:
assert "/superset/dashboard/sales/" in link
assert "Sales" in link
+
+
+def test_thumbnail_url_is_router_relative_at_root(app_context: None) -> None:
+ """thumbnail_url uses url_for, so at root it keeps the legacy shape."""
+ dash = Dashboard()
+ dash.id = 7
+
+ with patch.object(
+ Dashboard, "digest", new_callable=PropertyMock, return_value="abc123"
+ ):
+ with current_app.test_request_context("/"):
+ url = dash.thumbnail_url
+
+ assert url == "/api/v1/dashboard/7/thumbnail/abc123/"
+
+
+def test_thumbnail_url_carries_app_root_prefix(app_context: None) -> None:
+ """Under a subdirectory deployment the serialized thumbnail URL must carry
+ the application root, because the frontend treats thumbnail_url as an
+ already-prefixed raw fetch target (it is excluded from
+ normalizeBackendUrls and never passed through ensureAppRoot)."""
+ dash = Dashboard()
+ dash.id = 7
+
+ with patch.object(
+ Dashboard, "digest", new_callable=PropertyMock, return_value="abc123"
+ ):
+ with current_app.test_request_context(
+ "/", base_url="http://example.com/superset/"
+ ):
+ url = dash.thumbnail_url
+
+ assert url == "/superset/api/v1/dashboard/7/thumbnail/abc123/"
+
+
+def test_thumbnail_url_is_none_without_digest(app_context: None) -> None:
+ dash = Dashboard()
+ dash.id = 7
+
+ with patch.object(
+ Dashboard, "digest", new_callable=PropertyMock, return_value=None
+ ):
+ with current_app.test_request_context("/"):
+ assert dash.thumbnail_url is None
diff --git a/tests/unit_tests/models/slice_test.py b/tests/unit_tests/models/slice_test.py
index e8d6407f43f..0a223645460 100644
--- a/tests/unit_tests/models/slice_test.py
+++ b/tests/unit_tests/models/slice_test.py
@@ -19,6 +19,7 @@ import uuid
from unittest.mock import MagicMock, patch, PropertyMock
import pytest
+from flask import current_app
from parameterized import parameterized
from superset.models.slice import id_or_uuid_filter, Slice
@@ -146,3 +147,45 @@ class TestSlice:
# The injected tag and attribute-breakout quote are escaped.
assert "<img" not in html
assert '"onmouseover' not in html
+
+
+def test_thumbnail_url_is_router_relative_at_root(app_context: None) -> None:
+ """thumbnail_url uses url_for, so at root it keeps the legacy shape."""
+ slc = Slice()
+ slc.id = 42
+
+ with patch.object(
+ Slice, "digest", new_callable=PropertyMock, return_value="abc123"
+ ):
+ with current_app.test_request_context("/"):
+ url = slc.thumbnail_url
+
+ assert url == "/api/v1/chart/42/thumbnail/abc123/"
+
+
+def test_thumbnail_url_carries_app_root_prefix(app_context: None) -> None:
+ """Under a subdirectory deployment the serialized thumbnail URL must carry
+ the application root, because the frontend treats thumbnail_url as an
+ already-prefixed raw fetch target (it is excluded from
+ normalizeBackendUrls and never passed through ensureAppRoot)."""
+ slc = Slice()
+ slc.id = 42
+
+ with patch.object(
+ Slice, "digest", new_callable=PropertyMock, return_value="abc123"
+ ):
+ with current_app.test_request_context(
+ "/", base_url="http://example.com/superset/"
+ ):
+ url = slc.thumbnail_url
+
+ assert url == "/superset/api/v1/chart/42/thumbnail/abc123/"
+
+
+def test_thumbnail_url_is_none_without_digest(app_context: None) -> None:
+ slc = Slice()
+ slc.id = 42
+
+ with patch.object(Slice, "digest", new_callable=PropertyMock, return_value=None):
+ with current_app.test_request_context("/"):
+ assert slc.thumbnail_url is None
diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py
index 2b852796ebf..db3f90c88ba 100644
--- a/tests/unit_tests/utils/test_core.py
+++ b/tests/unit_tests/utils/test_core.py
@@ -637,8 +637,12 @@ def test_get_datasource_full_name():
(None, None),
("https://mysuperset.com/abc", None),
("https://mysuperset.com/superset/dashboard/", QuerySource.DASHBOARD),
+ ("https://mysuperset.com/dashboard/1/", QuerySource.DASHBOARD),
+ ("https://mysuperset.com/myapp/dashboard/1/", QuerySource.DASHBOARD),
("https://mysuperset.com/explore/", QuerySource.CHART),
+ ("https://mysuperset.com/myapp/explore/", QuerySource.CHART),
("https://mysuperset.com/sqllab/", QuerySource.SQL_LAB),
+ ("https://mysuperset.com/myapp/sqllab/", QuerySource.SQL_LAB),
],
)
def test_get_query_source_from_request(