diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 4c104344e9f..3f896f78fab 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -47,6 +47,7 @@ jobs: permissions: actions: read contents: read + pull-requests: read security-events: write strategy: diff --git a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts index b5130faa3e2..bcbabc05e74 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts @@ -52,6 +52,13 @@ const SupersetClient: SupersetClientInterface = { request: request => getInstance().request(request), getCSRFToken: () => getInstance().getCSRFToken(), getUrl: (...args) => getInstance().getUrl(...args), + get guestTokenHeaderName() { + try { + return getInstance().guestTokenHeaderName; + } catch { + return 'X-GuestToken'; + } + }, }; export default SupersetClient; diff --git a/superset-frontend/packages/superset-ui-core/src/connection/types.ts b/superset-frontend/packages/superset-ui-core/src/connection/types.ts index 08aa2377a14..61f6a1b2601 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/types.ts @@ -163,6 +163,7 @@ export interface SupersetClientInterface extends Pick< configure: (config?: ClientConfig) => SupersetClientInterface; reset: () => void; getCSRFToken: () => CsrfPromise; + guestTokenHeaderName?: string; } export type SupersetClientResponse = Response | JsonResponse | TextResponse; diff --git a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts index 85aadb3910e..294f7ecfd7c 100644 --- a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts @@ -172,4 +172,15 @@ describe('SupersetClient', () => { const token = await SupersetClient.getCSRFToken(); expect(token).toBe('my_token'); }); + + test('guestTokenHeaderName returns the configured header name when instance exists', () => { + SupersetClient.configure({ guestTokenHeaderName: 'X-Custom-Guest' }); + expect(SupersetClient.guestTokenHeaderName).toBe('X-Custom-Guest'); + }); + + test('guestTokenHeaderName returns default X-GuestToken when instance is not configured', () => { + // Ensure instance is reset (afterEach calls SupersetClient.reset()) + // Access the property without calling configure() first + expect(SupersetClient.guestTokenHeaderName).toBe('X-GuestToken'); + }); }); diff --git a/superset-frontend/src/components/CrudThemeProvider.test.tsx b/superset-frontend/src/components/CrudThemeProvider.test.tsx index 382c978fd9e..816ded2b1a6 100644 --- a/superset-frontend/src/components/CrudThemeProvider.test.tsx +++ b/superset-frontend/src/components/CrudThemeProvider.test.tsx @@ -310,12 +310,7 @@ test('ignores non-array fontUrls in theme config without throwing', () => { }); test('skips the dashboard theme when an SDK theme config override is active', () => { - const themeConfig = { - token: { - colorPrimary: '#ff0000', - fontUrls: ['https://fonts.example.com/dashboard.css'], - }, - }; + const themeConfig = { token: { colorPrimary: '#ff0000' } }; render( { @@ -379,3 +372,52 @@ test('does not inject font style element when no fontUrls in config', () => { const fontStyle = document.querySelector('style[data-superset-fonts]'); expect(fontStyle).toBeNull(); }); + +test('prevents font injection and cleans up fonts when hasThemeConfigOverride becomes active', () => { + const fontUrl = 'https://fonts.example.com/custom.css'; + const themeConfig = { + token: { colorPrimary: '#ff0000', fontUrls: [fontUrl] }, + }; + + const { rerender } = render( + + +
Dashboard Content
+ + , + ); + + // Assert font is injected + let fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).not.toBeNull(); + expect(fontStyle?.textContent).toContain(`@import url("${fontUrl}")`); + + // Switch hasThemeConfigOverride dynamically to true + rerender( + + +
Dashboard Content
+ + , + ); + + // Assert font is cleaned up and removed + fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).toBeNull(); +}); diff --git a/superset-frontend/src/components/CrudThemeProvider.tsx b/superset-frontend/src/components/CrudThemeProvider.tsx index c89552147c4..c44e72d7fee 100644 --- a/superset-frontend/src/components/CrudThemeProvider.tsx +++ b/superset-frontend/src/components/CrudThemeProvider.tsx @@ -78,7 +78,9 @@ export default function CrudThemeProvider({ }, [theme?.json_data, hasThemeConfigOverride]); useEffect(() => { - if (!dashboardTheme || !fontUrls?.length) return undefined; + if (hasThemeConfigOverride || !dashboardTheme || !fontUrls?.length) { + return undefined; + } // JSON.stringify provides safe escaping to prevent CSS injection const css = fontUrls @@ -92,7 +94,7 @@ export default function CrudThemeProvider({ return () => { style.remove(); }; - }, [dashboardTheme, fontUrls]); + }, [dashboardTheme, fontUrls, hasThemeConfigOverride]); if (!dashboardTheme || hasThemeConfigOverride) { return <>{children}; diff --git a/superset-frontend/src/theme/ThemeController.ts b/superset-frontend/src/theme/ThemeController.ts index 08d613fb3fe..f32a49dd4aa 100644 --- a/superset-frontend/src/theme/ThemeController.ts +++ b/superset-frontend/src/theme/ThemeController.ts @@ -27,7 +27,7 @@ import { themeObject as supersetThemeObject, normalizeThemeConfig, } from '@apache-superset/core/theme'; -import { makeApi } from '@superset-ui/core'; +import { makeApi, SupersetClient } from '@superset-ui/core'; import type { BootstrapThemeData, BootstrapThemeDataConfig, @@ -188,6 +188,12 @@ export class ThemeController { ); this.onChangeCallbacks.clear(); + + // Clean up injected font styles + document + .querySelectorAll('style[data-superset-fonts]') + .forEach(el => el.remove()); + this.loadedFontUrls.clear(); } /** @@ -447,6 +453,7 @@ export class ThemeController { this.devThemeOverride = null; this.crudThemeId = null; this.dashboardCrudTheme = null; + this.themeConfigOverride = false; this.storage.removeItem(STORAGE_KEYS.DEV_THEME_OVERRIDE); this.storage.removeItem(STORAGE_KEYS.CRUD_THEME_ID); @@ -1018,44 +1025,105 @@ export class ThemeController { } } + /** + * Constructs the guest token authorization header using the configured + * header name from SupersetClient or bootstrap config, falling back to 'X-GuestToken'. + */ + private getGuestTokenHeader(): Record { + const headers: Record = {}; + try { + const guestToken = SupersetClient.getGuestToken(); + if (guestToken) { + let headerName = 'X-GuestToken'; + try { + if (SupersetClient.guestTokenHeaderName) { + headerName = SupersetClient.guestTokenHeaderName; + } + } catch { + const bootstrapData = getBootstrapData(); + headerName = + bootstrapData.config?.GUEST_TOKEN_HEADER_NAME || 'X-GuestToken'; + } + headers[headerName] = guestToken; + } + } catch (tokenError) { + // Ignore token retrieval error + } + return headers; + } + /** * Fetches a fresh system default theme from the API for runtime recovery. * Tries multiple fallback strategies to find a valid theme. * - * Note: Uses raw fetch() instead of SupersetClient because ThemeController - * initializes early in the app lifecycle, before SupersetClient is fully - * configured. This avoids boot-time circular dependencies. + * Note: First tries to use SupersetClient. If SupersetClient is not yet + * fully configured/initialized or if the request fails (e.g. in embedded + * guest-token environments where SupersetClient bootstrap is still in progress), + * it falls back to using raw fetch() with custom guest token headers. * * @returns The system default theme configuration or null if not found */ private async fetchSystemDefaultTheme(): Promise { try { - // Try to fetch theme marked as system default (is_system_default=true) - const defaultResponse = await fetch( - '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', - ); - if (defaultResponse.ok) { - const data = await defaultResponse.json(); - if (data.result?.length > 0) { - const themeConfig = JSON.parse(data.result[0].json_data); + // Try to use SupersetClient first if it has been configured + try { + const response = await SupersetClient.get({ + endpoint: + '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', + }); + if (response.json?.result?.length > 0) { + const themeConfig = JSON.parse(response.json.result[0].json_data); if (themeConfig && typeof themeConfig === 'object') { return themeConfig; } } + } catch (clientError) { + // If SupersetClient is not configured yet or request fails, fall back to native fetch + const headers = this.getGuestTokenHeader(); + + const defaultResponse = await fetch( + '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', + { headers }, + ); + if (defaultResponse.ok) { + const data = await defaultResponse.json(); + if (data.result?.length > 0) { + const themeConfig = JSON.parse(data.result[0].json_data); + if (themeConfig && typeof themeConfig === 'object') { + return themeConfig; + } + } + } } // Fallback: Try to fetch system theme named 'THEME_DEFAULT' - const fallbackResponse = await fetch( - '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', - ); - if (fallbackResponse.ok) { - const fallbackData = await fallbackResponse.json(); - if (fallbackData.result?.length > 0) { - const themeConfig = JSON.parse(fallbackData.result[0].json_data); + try { + const response = await SupersetClient.get({ + endpoint: + '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', + }); + if (response.json?.result?.length > 0) { + const themeConfig = JSON.parse(response.json.result[0].json_data); if (themeConfig && typeof themeConfig === 'object') { return themeConfig; } } + } catch (clientError) { + const headers = this.getGuestTokenHeader(); + + const fallbackResponse = await fetch( + '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', + { headers }, + ); + if (fallbackResponse.ok) { + const fallbackData = await fallbackResponse.json(); + if (fallbackData.result?.length > 0) { + const themeConfig = JSON.parse(fallbackData.result[0].json_data); + if (themeConfig && typeof themeConfig === 'object') { + return themeConfig; + } + } + } } } catch (error) { // Log for debugging but don't fail - fallback to cached theme will be used diff --git a/superset-frontend/src/theme/tests/ThemeController.test.ts b/superset-frontend/src/theme/tests/ThemeController.test.ts index f724758fd55..fe919ebd3ba 100644 --- a/superset-frontend/src/theme/tests/ThemeController.test.ts +++ b/superset-frontend/src/theme/tests/ThemeController.test.ts @@ -17,7 +17,7 @@ * under the License. */ import { theme as antdThemeImport } from 'antd'; -import {} from '@superset-ui/core'; +import { SupersetClient } from '@superset-ui/core'; import { type AnyThemeConfig, type SupersetThemeConfig, @@ -915,6 +915,7 @@ test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched t // Verify API was called to fetch system default theme expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining('/api/v1/theme/'), + expect.any(Object), ); // Verify the fetched theme was applied via applyThemeWithRecovery @@ -1854,3 +1855,263 @@ test('getResolvedThemeMode returns dark when default theme is dark but mode is D expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT); expect(controller.getCurrentModeResolved()).toBe('dark'); }); + +test('fallback fetch: uses custom guest token header from SupersetClient when client.get fails', async () => { + const originalFetch = global.fetch; + const mockGet = jest + .spyOn(SupersetClient, 'get') + .mockRejectedValue(new Error('Client not configured')); + const mockGetGuestToken = jest + .spyOn(SupersetClient, 'getGuestToken') + .mockReturnValue('custom-guest-token-123'); + + // Define getter for guestTokenHeaderName + Object.defineProperty(SupersetClient, 'guestTokenHeaderName', { + value: 'X-Custom-Guest-Header', + configurable: true, + }); + + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + result: [ + { + json_data: JSON.stringify({ + token: { colorPrimary: '#custom-header-theme' }, + }), + }, + ], + }), + }); + global.fetch = mockFetch; + + try { + const controller = createController(); + const result = await (controller as any).fetchSystemDefaultTheme(); + + expect(mockGet).toHaveBeenCalled(); + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('/api/v1/theme/'), + expect.objectContaining({ + headers: expect.objectContaining({ + 'X-Custom-Guest-Header': 'custom-guest-token-123', + }), + }), + ); + expect(result).toEqual({ token: { colorPrimary: '#custom-header-theme' } }); + } finally { + global.fetch = originalFetch; + mockGet.mockRestore(); + mockGetGuestToken.mockRestore(); + Object.defineProperty(SupersetClient, 'guestTokenHeaderName', { + value: undefined, + configurable: true, + }); + } +}); + +test('fallback fetch: uses bootstrap config for guest token header when SupersetClient is not configured', async () => { + const originalFetch = global.fetch; + const mockGet = jest + .spyOn(SupersetClient, 'get') + .mockRejectedValue(new Error('Client not configured')); + const mockGetGuestToken = jest + .spyOn(SupersetClient, 'getGuestToken') + .mockReturnValue('bootstrap-guest-token'); + + // Ensure SupersetClient.guestTokenHeaderName is undefined or throws + Object.defineProperty(SupersetClient, 'guestTokenHeaderName', { + get: () => { + throw new Error('Not configured'); + }, + configurable: true, + }); + + // Mock bootstrapData.config?.GUEST_TOKEN_HEADER_NAME + mockGetBootstrapData.mockReturnValue({ + ...createMockBootstrapData(), + config: { + GUEST_TOKEN_HEADER_NAME: 'X-Bootstrap-Custom-Header', + }, + } as any); + + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + result: [ + { + json_data: JSON.stringify({ + token: { colorPrimary: '#bootstrap-theme' }, + }), + }, + ], + }), + }); + global.fetch = mockFetch; + + try { + const controller = createController(); + const result = await (controller as any).fetchSystemDefaultTheme(); + + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('/api/v1/theme/'), + expect.objectContaining({ + headers: expect.objectContaining({ + 'X-Bootstrap-Custom-Header': 'bootstrap-guest-token', + }), + }), + ); + expect(result).toEqual({ token: { colorPrimary: '#bootstrap-theme' } }); + } finally { + global.fetch = originalFetch; + mockGet.mockRestore(); + mockGetGuestToken.mockRestore(); + mockGetBootstrapData.mockReturnValue(createMockBootstrapData()); + Object.defineProperty(SupersetClient, 'guestTokenHeaderName', { + value: undefined, + configurable: true, + }); + } +}); + +test('SDK override toggling and dynamic transitions', () => { + const controller = createController(); + + expect(controller.hasThemeConfigOverride()).toBe(false); + + // Set theme config override + const sdkThemeConfig: SupersetThemeConfig = { + theme_default: { token: { colorPrimary: '#sdk-default' } }, + theme_dark: { token: { colorPrimary: '#sdk-dark' } }, + }; + controller.setThemeConfig(sdkThemeConfig); + expect(controller.hasThemeConfigOverride()).toBe(true); + + // Clear local overrides (which should reset override flag) + controller.clearLocalOverrides(); + expect(controller.hasThemeConfigOverride()).toBe(false); +}); + +test('ThemeController cleans up injected fonts on destroy', () => { + const controller = createController(); + + // Inject some fonts + (controller as any).loadFonts(['https://fonts.example.com/font-test.css']); + + let fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).not.toBeNull(); + + controller.destroy(); + + fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).toBeNull(); +}); + +test('fallback fetch: uses bootstrap GUEST_TOKEN_HEADER_NAME when guestTokenHeaderName getter throws', async () => { + const originalFetch = global.fetch; + + // SupersetClient.get throws so we fall through to native fetch + const mockGet = jest + .spyOn(SupersetClient, 'get') + .mockRejectedValue(new Error('Client not configured')); + + // getGuestToken succeeds (we have a guest token) + const mockGetGuestToken = jest + .spyOn(SupersetClient, 'getGuestToken') + .mockReturnValue('my-guest-token'); + + // guestTokenHeaderName getter throws → should fall back to bootstrap config + Object.defineProperty(SupersetClient, 'guestTokenHeaderName', { + get: () => { + throw new Error('Not configured'); + }, + configurable: true, + }); + + // Return a bootstrap config with a custom header name + mockGetBootstrapData.mockReturnValue({ + ...createMockBootstrapData(), + config: { + GUEST_TOKEN_HEADER_NAME: 'X-Bootstrap-Header', + }, + } as any); + + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + result: [ + { + json_data: JSON.stringify({ + token: { colorPrimary: '#bootstrap-fallback' }, + }), + }, + ], + }), + }); + global.fetch = mockFetch; + + try { + const controller = createController(); + const result = await (controller as any).fetchSystemDefaultTheme(); + + // Verify the bootstrap header was used instead of SupersetClient.guestTokenHeaderName + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('/api/v1/theme/'), + expect.objectContaining({ + headers: expect.objectContaining({ + 'X-Bootstrap-Header': 'my-guest-token', + }), + }), + ); + expect(result).toEqual({ token: { colorPrimary: '#bootstrap-fallback' } }); + } finally { + global.fetch = originalFetch; + mockGet.mockRestore(); + mockGetGuestToken.mockRestore(); + mockGetBootstrapData.mockReturnValue(createMockBootstrapData()); + Object.defineProperty(SupersetClient, 'guestTokenHeaderName', { + value: undefined, + configurable: true, + }); + } +}); + +test('fetchSystemDefaultTheme: second named-theme fallback fetch succeeds when first API calls fail', async () => { + const originalFetch = global.fetch; + + // SupersetClient.get always throws (not configured) + const mockGet = jest + .spyOn(SupersetClient, 'get') + .mockRejectedValue(new Error('Client not configured')); + + const namedTheme = { token: { colorPrimary: '#named-theme' } }; + + // First fetch call (is_system_default) returns empty result; second (THEME_DEFAULT name) succeeds + const mockFetch = jest + .fn() + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ result: [] }), // first path: no results + }) + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ + result: [{ json_data: JSON.stringify(namedTheme) }], + }), + }); + global.fetch = mockFetch; + + try { + const controller = createController(); + const result = await (controller as any).fetchSystemDefaultTheme(); + + // Both fetches should have been called + expect(mockFetch).toHaveBeenCalledTimes(2); + + // The result should be from the second (named-theme) fallback fetch + expect(result).toEqual(namedTheme); + } finally { + global.fetch = originalFetch; + mockGet.mockRestore(); + } +}); diff --git a/superset/models/core.py b/superset/models/core.py index ecc232961a6..b0d662e3cf0 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -147,6 +147,24 @@ class Theme(AuditMixinNullable, ImportExportMixin, Model): export_fields = ["theme_name", "json_data"] +# Event listeners to clear the memoized bootstrap data cache when a theme is modified +@sqla.event.listens_for(Theme, "after_insert") +@sqla.event.listens_for(Theme, "after_update") +@sqla.event.listens_for(Theme, "after_delete") +def clear_bootstrap_cache( + _mapper: sqla.orm.Mapper, + _connection: sqla.engine.Connection, + _target: Theme, +) -> None: + from superset.extensions import cache_manager + from superset.views.base import cached_common_bootstrap_data + + try: + cache_manager.cache.delete_memoized(cached_common_bootstrap_data) + except Exception as ex: # pylint: disable=broad-except + logger.warning("Failed to clear theme bootstrap cache: %s", ex) + + class ConfigurationMethod(StrEnum): SQLALCHEMY_FORM = "sqlalchemy_form" DYNAMIC_FORM = "dynamic_form" diff --git a/tests/unit_tests/models/core_test.py b/tests/unit_tests/models/core_test.py index 8d15b23077b..0da5055624b 100644 --- a/tests/unit_tests/models/core_test.py +++ b/tests/unit_tests/models/core_test.py @@ -1556,3 +1556,44 @@ def test_database_execute_async_without_options(mocker: MockerFixture) -> None: mock_executor_class.assert_called_once_with(database) mock_executor.execute_async.assert_called_once_with("SELECT 1", None) assert result == mock_handle + + +def test_clear_bootstrap_cache_logs_warning_on_failure( + mocker: MockerFixture, +) -> None: + """ + Test that clear_bootstrap_cache logs a warning when cache invalidation fails. + + Exercises the ``except Exception`` branch in the event listener so that + Codecov registers it as covered. The function must not re-raise the + exception — callers (SQLAlchemy event dispatch) should be unaffected. + """ + from superset.models.core import clear_bootstrap_cache + + # Patch cache_manager so delete_memoized raises + mock_cache = mocker.MagicMock() + mock_cache.delete_memoized.side_effect = RuntimeError("Redis unavailable") + + mock_cache_manager = mocker.patch("superset.models.core.cache_manager") + mock_cache_manager.cache = mock_cache + + # Patch cached_common_bootstrap_data so the local import inside + # clear_bootstrap_cache resolves to our mock. + mocker.patch( + "superset.views.base.cached_common_bootstrap_data", + new=mocker.MagicMock(__name__="cached_common_bootstrap_data"), + ) + + mock_logger = mocker.patch("superset.models.core.logger") + + # Should not raise even though delete_memoized raises + clear_bootstrap_cache( + _mapper=mocker.MagicMock(), + _connection=mocker.MagicMock(), + _target=mocker.MagicMock(), + ) + + # Verify logger.warning was called with the correct message format + mock_logger.warning.assert_called_once() + call_args = mock_logger.warning.call_args + assert call_args[0][0] == "Failed to clear theme bootstrap cache: %s" diff --git a/tests/unit_tests/views/test_base_theme_helpers.py b/tests/unit_tests/views/test_base_theme_helpers.py index 5e84ec3b871..d019a6f1f72 100644 --- a/tests/unit_tests/views/test_base_theme_helpers.py +++ b/tests/unit_tests/views/test_base_theme_helpers.py @@ -886,3 +886,32 @@ class TestGetDefaultSpinnerSvg: assert result is not None assert "