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/03/02 05:14:35 UTC
(superset) branch fix-app-root-logos created (now acd81b988d)
This is an automated email from the ASF dual-hosted git repository.
jli pushed a change to branch fix-app-root-logos
in repository https://gitbox.apache.org/repos/asf/superset.git
at acd81b988d test(frontend): consolidate repeated absolute URL literals into constants
This branch includes the following new commits:
new 396bcc2363 test(frontend): add failing tests for ensureAppRoot absolute URL passthrough
new cdee487674 fix(frontend): preserve absolute and protocol-relative URLs in ensureAppRoot
new 3b655f9c28 fix(frontend): update makeUrl docstring and pin URI scheme edge case
new acd81b988d test(frontend): consolidate repeated absolute URL literals into constants
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/04: test(frontend): add failing tests for ensureAppRoot absolute URL passthrough
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch fix-app-root-logos
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 396bcc23639e661976e1beada009f01c597155b4
Author: Joe Li <jo...@preset.io>
AuthorDate: Sun Mar 1 10:57:54 2026 -0800
test(frontend): add failing tests for ensureAppRoot absolute URL passthrough
TDD RED phase for fix of brandLogoHref being double-prefixed with app root
when LOGO_TARGET_PATH is an absolute URL in subdirectory deployments.
- pathUtils.test.ts: 4 failing tests covering https://, http://, ftp://,
mailto:, and protocol-relative (//...) passthrough for both ensureAppRoot
and makeUrl; adds loadPathUtils() helper to reduce bootstrap boilerplate
- Menu.test.tsx: 2 failing tests for absolute and protocol-relative
brandLogoHref; 3 passing guardrails for brand.path subdir prefixing,
fallback branch behavior, and useTheme mock infrastructure
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset-frontend/src/features/home/Menu.test.tsx | 115 +++++++++++++++++++++-
superset-frontend/src/utils/pathUtils.test.ts | 63 ++++++++++++
2 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/superset-frontend/src/features/home/Menu.test.tsx b/superset-frontend/src/features/home/Menu.test.tsx
index 4b7e744188..2c07963c7b 100644
--- a/superset-frontend/src/features/home/Menu.test.tsx
+++ b/superset-frontend/src/features/home/Menu.test.tsx
@@ -21,9 +21,15 @@ import fetchMock from 'fetch-mock';
import { render, screen, userEvent } from 'spec/helpers/testing-library';
import setupCodeOverrides from 'src/setup/setupCodeOverrides';
import { getExtensionsRegistry } from '@superset-ui/core';
+import * as CoreUI from '@apache-superset/core/ui';
import { Menu } from './Menu';
import * as getBootstrapData from 'src/utils/getBootstrapData';
+jest.mock('@apache-superset/core/ui', () => ({
+ ...jest.requireActual('@apache-superset/core/ui'),
+ useTheme: jest.fn(),
+}));
+
const dropdownItems = [
{
label: 'Data',
@@ -243,6 +249,8 @@ const staticAssetsPrefixMock = jest.spyOn(
getBootstrapData,
'staticAssetsPrefix',
);
+const applicationRootMock = jest.spyOn(getBootstrapData, 'applicationRoot');
+const useThemeMock = CoreUI.useTheme as jest.Mock;
fetchMock.get(
'glob:*api/v1/database/?q=(filters:!((col:allow_file_upload,opr:upload_is_enabled,value:!t)))',
@@ -252,8 +260,11 @@ fetchMock.get(
beforeEach(() => {
// setup a DOM element as a render target
useSelectorMock.mockClear();
- // By default use empty static assets prefix
+ // By default use empty static assets prefix and default app root
staticAssetsPrefixMock.mockReturnValue('');
+ applicationRootMock.mockReturnValue('');
+ // By default useTheme returns the real default theme (brandLogoUrl is falsy)
+ useThemeMock.mockReturnValue(CoreUI.supersetTheme);
});
test('should render', async () => {
@@ -675,3 +686,105 @@ test('should not render the brand text if not available', async () => {
const brandText = screen.queryByText(text);
expect(brandText).not.toBeInTheDocument();
});
+
+test('brand logo href should not be prefixed with app root when brandLogoHref is an absolute URL', async () => {
+ applicationRootMock.mockReturnValue('/superset');
+ useThemeMock.mockReturnValue({
+ ...CoreUI.supersetTheme,
+ brandLogoUrl: '/static/assets/images/custom-logo.png',
+ brandLogoHref: 'https://external.example.com',
+ });
+ useSelectorMock.mockReturnValue({ roles: user.roles });
+
+ render(<Menu {...mockedProps} />, {
+ useRedux: true,
+ useQueryParams: true,
+ useRouter: true,
+ useTheme: true,
+ });
+
+ const brandLink = await screen.findByRole('link', {
+ name: /apache superset/i,
+ });
+ expect(brandLink).toHaveAttribute('href', 'https://external.example.com');
+});
+
+test('brand logo href should not be prefixed with app root when brandLogoHref is protocol-relative', async () => {
+ applicationRootMock.mockReturnValue('/superset');
+ useThemeMock.mockReturnValue({
+ ...CoreUI.supersetTheme,
+ brandLogoUrl: '/static/assets/images/custom-logo.png',
+ brandLogoHref: '//external.example.com',
+ });
+ useSelectorMock.mockReturnValue({ roles: user.roles });
+
+ render(<Menu {...mockedProps} />, {
+ useRedux: true,
+ useQueryParams: true,
+ useRouter: true,
+ useTheme: true,
+ });
+
+ const brandLink = await screen.findByRole('link', {
+ name: /apache superset/i,
+ });
+ expect(brandLink).toHaveAttribute('href', '//external.example.com');
+});
+
+test('brand path should be prefixed with app root in subdirectory deployment', async () => {
+ applicationRootMock.mockReturnValue('/superset');
+ useSelectorMock.mockReturnValue({ roles: user.roles });
+
+ const propsWithSimplePath = {
+ ...mockedProps,
+ data: {
+ ...mockedProps.data,
+ brand: {
+ ...mockedProps.data.brand,
+ path: '/welcome/',
+ },
+ },
+ };
+
+ render(<Menu {...propsWithSimplePath} />, {
+ useRedux: true,
+ useQueryParams: true,
+ useRouter: true,
+ useTheme: true,
+ });
+
+ const brandLink = await screen.findByRole('link', {
+ name: new RegExp(propsWithSimplePath.data.brand.alt, 'i'),
+ });
+ expect(brandLink).toHaveAttribute('href', '/superset/welcome/');
+});
+
+test('brand link falls back to brand.path when theme brandLogoUrl is absent', async () => {
+ // useThemeMock default returns supersetTheme with brandLogoUrl undefined (falsy)
+ applicationRootMock.mockReturnValue('/superset');
+ useSelectorMock.mockReturnValue({ roles: user.roles });
+
+ const propsWithFallbackPath = {
+ ...mockedProps,
+ data: {
+ ...mockedProps.data,
+ brand: {
+ ...mockedProps.data.brand,
+ path: '/welcome/',
+ },
+ },
+ };
+
+ render(<Menu {...propsWithFallbackPath} />, {
+ useRedux: true,
+ useQueryParams: true,
+ useRouter: true,
+ useTheme: true,
+ });
+
+ const brandLink = await screen.findByRole('link', {
+ name: new RegExp(propsWithFallbackPath.data.brand.alt, 'i'),
+ });
+ // ensureAppRoot must have been applied: /welcome/ → /superset/welcome/
+ expect(brandLink).toHaveAttribute('href', '/superset/welcome/');
+});
diff --git a/superset-frontend/src/utils/pathUtils.test.ts b/superset-frontend/src/utils/pathUtils.test.ts
index bf24053e67..5c8aed9df0 100644
--- a/superset-frontend/src/utils/pathUtils.test.ts
+++ b/superset-frontend/src/utils/pathUtils.test.ts
@@ -158,3 +158,66 @@ test('makeUrl should handle URLs with anchors', async () => {
'/superset/dashboard/123#anchor',
);
});
+
+// Sets up bootstrap data and returns a fresh pathUtils module instance.
+// Passing appRoot='' (default) simulates no subdirectory deployment.
+async function loadPathUtils(appRoot = '') {
+ const bootstrapData = { common: { application_root: appRoot } };
+ document.body.innerHTML = `<div id="app" data-bootstrap='${JSON.stringify(bootstrapData)}'></div>`;
+ jest.resetModules();
+ await import('./getBootstrapData');
+ return import('./pathUtils');
+}
+
+test('ensureAppRoot should preserve absolute and protocol-relative URLs unchanged with default root', async () => {
+ const { ensureAppRoot } = await loadPathUtils();
+
+ expect(ensureAppRoot('https://external.example.com')).toBe(
+ 'https://external.example.com',
+ );
+ expect(ensureAppRoot('http://external.example.com')).toBe(
+ 'http://external.example.com',
+ );
+ expect(ensureAppRoot('//external.example.com')).toBe(
+ '//external.example.com',
+ );
+});
+
+test('ensureAppRoot should preserve absolute URLs unchanged with custom subdirectory', async () => {
+ const { ensureAppRoot } = await loadPathUtils('/superset/');
+
+ expect(ensureAppRoot('https://external.example.com')).toBe(
+ 'https://external.example.com',
+ );
+ expect(ensureAppRoot('http://external.example.com')).toBe(
+ 'http://external.example.com',
+ );
+ // Non-http absolute schemes: implementation must not rely on http/https check alone
+ expect(ensureAppRoot('ftp://files.example.com/data')).toBe(
+ 'ftp://files.example.com/data',
+ );
+ expect(ensureAppRoot('mailto:user@example.com')).toBe(
+ 'mailto:user@example.com',
+ );
+});
+
+test('ensureAppRoot should preserve protocol-relative URLs unchanged', async () => {
+ const { ensureAppRoot } = await loadPathUtils('/superset/');
+
+ expect(ensureAppRoot('//external.example.com')).toBe(
+ '//external.example.com',
+ );
+});
+
+test('makeUrl should preserve absolute and protocol-relative URLs unchanged', async () => {
+ const { makeUrl } = await loadPathUtils('/superset/');
+
+ expect(makeUrl('https://external.example.com')).toBe(
+ 'https://external.example.com',
+ );
+ expect(makeUrl('//external.example.com')).toBe('//external.example.com');
+ // Non-http absolute scheme parity with ensureAppRoot
+ expect(makeUrl('ftp://files.example.com/data')).toBe(
+ 'ftp://files.example.com/data',
+ );
+});
(superset) 03/04: fix(frontend): update makeUrl docstring and pin URI scheme edge case
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch fix-app-root-logos
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3b655f9c281c49b9f6f4983883daf22aa7cb46ee
Author: Joe Li <jo...@preset.io>
AuthorDate: Sun Mar 1 21:06:00 2026 -0800
fix(frontend): update makeUrl docstring and pin URI scheme edge case
- makeUrl JSDoc now documents the absolute/protocol-relative passthrough
behavior introduced alongside the ensureAppRoot fix
- Adds an explicit test for custom URI schemes (e.g. foo:bar): the RFC 3986
scheme regex treats them as absolute and returns them unchanged, which is
safer than prepending the app root to produce a broken path
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset-frontend/src/utils/pathUtils.test.ts | 9 +++++++++
superset-frontend/src/utils/pathUtils.ts | 14 ++++++++------
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/superset-frontend/src/utils/pathUtils.test.ts b/superset-frontend/src/utils/pathUtils.test.ts
index 5c8aed9df0..b061d943d5 100644
--- a/superset-frontend/src/utils/pathUtils.test.ts
+++ b/superset-frontend/src/utils/pathUtils.test.ts
@@ -221,3 +221,12 @@ test('makeUrl should preserve absolute and protocol-relative URLs unchanged', as
'ftp://files.example.com/data',
);
});
+
+test('ensureAppRoot should treat any URI scheme as absolute (passthrough)', async () => {
+ // The RFC 3986 scheme regex matches any `word:` prefix, including custom schemes.
+ // Passthrough is the safe behavior — prepending the app root would produce a
+ // broken path like /superset/foo:bar, which is never what a caller would want.
+ const { ensureAppRoot } = await loadPathUtils('/superset/');
+
+ expect(ensureAppRoot('foo:bar')).toBe('foo:bar');
+});
diff --git a/superset-frontend/src/utils/pathUtils.ts b/superset-frontend/src/utils/pathUtils.ts
index a330d81d38..185ddc113a 100644
--- a/superset-frontend/src/utils/pathUtils.ts
+++ b/superset-frontend/src/utils/pathUtils.ts
@@ -36,16 +36,18 @@ export function ensureAppRoot(path: string): string {
}
/**
- * Creates a URL with the proper application root prefix for subdirectory deployments.
- * Use this when constructing URLs for navigation, API calls, or file downloads.
+ * Creates a URL suitable for navigation, API calls, or file downloads. Relative
+ * paths are prefixed with the application root for subdirectory deployments.
+ * Absolute URLs (e.g. https://...) and protocol-relative URLs (e.g. //example.com)
+ * are returned unchanged.
*
- * @param path - The path to convert to a full URL (e.g., '/sqllab', '/api/v1/chart/123')
- * @returns The path prefixed with the application root (e.g., '/superset/sqllab')
+ * @param path - The path or URL to resolve (e.g., '/sqllab', 'https://example.com')
+ * @returns The resolved URL (e.g., '/superset/sqllab' or 'https://example.com')
*
* @example
* // In a subdirectory deployment at /superset
- * makeUrl('/sqllab?new=true') // returns '/superset/sqllab?new=true'
- * makeUrl('/api/v1/chart/export/123/') // returns '/superset/api/v1/chart/export/123/'
+ * makeUrl('/sqllab?new=true') // returns '/superset/sqllab?new=true'
+ * makeUrl('https://external.example.com') // returns 'https://external.example.com'
*/
export function makeUrl(path: string): string {
return ensureAppRoot(path);
(superset) 02/04: fix(frontend): preserve absolute and protocol-relative URLs in ensureAppRoot
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch fix-app-root-logos
in repository https://gitbox.apache.org/repos/asf/superset.git
commit cdee48767494a44c6809801199e38f296449dcbd
Author: Joe Li <jo...@preset.io>
AuthorDate: Sun Mar 1 19:15:38 2026 -0800
fix(frontend): preserve absolute and protocol-relative URLs in ensureAppRoot
ensureAppRoot() previously prepended the application root to all inputs,
breaking navigation when LOGO_TARGET_PATH is an absolute URL — the result
was a double-prefixed URL like
https://<workspace>/https://manage.app.preset.io.
URLs with a URI scheme (https://, http://, ftp://, mailto:, etc.) and
protocol-relative URLs (//example.com) are now returned unchanged. Relative
paths continue to receive the application root prefix as before.
Fixes absolute brandLogoHref navigation broken by apache/superset#36058.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset-frontend/src/utils/pathUtils.ts | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/superset-frontend/src/utils/pathUtils.ts b/superset-frontend/src/utils/pathUtils.ts
index 0e6cac429f..a330d81d38 100644
--- a/superset-frontend/src/utils/pathUtils.ts
+++ b/superset-frontend/src/utils/pathUtils.ts
@@ -21,9 +21,17 @@ import { applicationRoot } from 'src/utils/getBootstrapData';
/**
* Takes a string path to a resource and prefixes it with the application root that is
* defined in the application configuration. The application path is sanitized.
- * @param path A string path to a resource
+ *
+ * Absolute URLs (e.g. https://..., ftp://..., mailto:...) and protocol-relative
+ * URLs (e.g. //example.com) are returned unchanged — only relative paths receive
+ * the application root prefix.
+ *
+ * @param path A string path or URL to a resource
*/
export function ensureAppRoot(path: string): string {
+ if (/^[a-zA-Z][a-zA-Z0-9+\-.]*:/.test(path) || path.startsWith('//')) {
+ return path;
+ }
return `${applicationRoot()}${path.startsWith('/') ? path : `/${path}`}`;
}
(superset) 04/04: test(frontend): consolidate repeated absolute URL literals into constants
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch fix-app-root-logos
in repository https://gitbox.apache.org/repos/asf/superset.git
commit acd81b988d96ae5a1839e768ee6e4cc14c58b2c5
Author: Joe Li <jo...@preset.io>
AuthorDate: Sun Mar 1 21:09:12 2026 -0800
test(frontend): consolidate repeated absolute URL literals into constants
Extract five URL constants (HTTPS_URL, HTTP_URL, PROTOCOL_RELATIVE_URL,
FTP_URL, MAILTO_URL) shared across the absolute-URL passthrough tests.
No behavior change.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset-frontend/src/utils/pathUtils.test.ts | 49 ++++++++++-----------------
1 file changed, 18 insertions(+), 31 deletions(-)
diff --git a/superset-frontend/src/utils/pathUtils.test.ts b/superset-frontend/src/utils/pathUtils.test.ts
index b061d943d5..79fded3761 100644
--- a/superset-frontend/src/utils/pathUtils.test.ts
+++ b/superset-frontend/src/utils/pathUtils.test.ts
@@ -159,6 +159,13 @@ test('makeUrl should handle URLs with anchors', async () => {
);
});
+// Representative URLs used across the absolute-URL passthrough tests below.
+const HTTPS_URL = 'https://external.example.com';
+const HTTP_URL = 'http://external.example.com';
+const PROTOCOL_RELATIVE_URL = '//external.example.com';
+const FTP_URL = 'ftp://files.example.com/data';
+const MAILTO_URL = 'mailto:user@example.com';
+
// Sets up bootstrap data and returns a fresh pathUtils module instance.
// Passing appRoot='' (default) simulates no subdirectory deployment.
async function loadPathUtils(appRoot = '') {
@@ -172,54 +179,34 @@ async function loadPathUtils(appRoot = '') {
test('ensureAppRoot should preserve absolute and protocol-relative URLs unchanged with default root', async () => {
const { ensureAppRoot } = await loadPathUtils();
- expect(ensureAppRoot('https://external.example.com')).toBe(
- 'https://external.example.com',
- );
- expect(ensureAppRoot('http://external.example.com')).toBe(
- 'http://external.example.com',
- );
- expect(ensureAppRoot('//external.example.com')).toBe(
- '//external.example.com',
- );
+ expect(ensureAppRoot(HTTPS_URL)).toBe(HTTPS_URL);
+ expect(ensureAppRoot(HTTP_URL)).toBe(HTTP_URL);
+ expect(ensureAppRoot(PROTOCOL_RELATIVE_URL)).toBe(PROTOCOL_RELATIVE_URL);
});
test('ensureAppRoot should preserve absolute URLs unchanged with custom subdirectory', async () => {
const { ensureAppRoot } = await loadPathUtils('/superset/');
- expect(ensureAppRoot('https://external.example.com')).toBe(
- 'https://external.example.com',
- );
- expect(ensureAppRoot('http://external.example.com')).toBe(
- 'http://external.example.com',
- );
+ expect(ensureAppRoot(HTTPS_URL)).toBe(HTTPS_URL);
+ expect(ensureAppRoot(HTTP_URL)).toBe(HTTP_URL);
// Non-http absolute schemes: implementation must not rely on http/https check alone
- expect(ensureAppRoot('ftp://files.example.com/data')).toBe(
- 'ftp://files.example.com/data',
- );
- expect(ensureAppRoot('mailto:user@example.com')).toBe(
- 'mailto:user@example.com',
- );
+ expect(ensureAppRoot(FTP_URL)).toBe(FTP_URL);
+ expect(ensureAppRoot(MAILTO_URL)).toBe(MAILTO_URL);
});
test('ensureAppRoot should preserve protocol-relative URLs unchanged', async () => {
const { ensureAppRoot } = await loadPathUtils('/superset/');
- expect(ensureAppRoot('//external.example.com')).toBe(
- '//external.example.com',
- );
+ expect(ensureAppRoot(PROTOCOL_RELATIVE_URL)).toBe(PROTOCOL_RELATIVE_URL);
});
test('makeUrl should preserve absolute and protocol-relative URLs unchanged', async () => {
const { makeUrl } = await loadPathUtils('/superset/');
- expect(makeUrl('https://external.example.com')).toBe(
- 'https://external.example.com',
- );
- expect(makeUrl('//external.example.com')).toBe('//external.example.com');
+ expect(makeUrl(HTTPS_URL)).toBe(HTTPS_URL);
+ expect(makeUrl(PROTOCOL_RELATIVE_URL)).toBe(PROTOCOL_RELATIVE_URL);
// Non-http absolute scheme parity with ensureAppRoot
- expect(makeUrl('ftp://files.example.com/data')).toBe(
- 'ftp://files.example.com/data',
- );
+ expect(makeUrl(FTP_URL)).toBe(FTP_URL);
});
test('ensureAppRoot should treat any URI scheme as absolute (passthrough)', async () => {