From e0c4baf66fd27ea8f2cfe51bf571fa809393b953 Mon Sep 17 00:00:00 2001 From: Krish Dholakia Date: Thu, 23 Oct 2025 13:59:29 -0700 Subject: [PATCH] fix(ui/): fix routing for custom server root path (#15701) * fix(ui/): fix routing for custom server root path * fix: fix eslint errors --- litellm/proxy/utils.py | 10 +- tests/test_litellm/proxy/test_custom_proxy.py | 3 + tests/test_litellm/proxy/test_proxy_utils.py | 49 ++- .../app/(dashboard)/components/Sidebar2.tsx | 26 +- .../src/app/onboarding/page.tsx | 16 +- .../src/hooks/useFeatureFlags.test.tsx | 296 ++++++++++++++++++ .../src/hooks/useFeatureFlags.tsx | 51 ++- ui/litellm-dashboard/src/utils/cookieUtils.ts | 11 + 8 files changed, 427 insertions(+), 35 deletions(-) create mode 100644 ui/litellm-dashboard/src/hooks/useFeatureFlags.test.tsx diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 0065edeb0e..737856c00c 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -3628,8 +3628,14 @@ def join_paths(base_path: str, route: str) -> str: if not route: return base_path - # Join with single slash - return f"{base_path}/{route}" + # Check if base_path already ends with the route to avoid duplication + if base_path.endswith(f"/{route}"): + final_path = base_path + else: + # Join with single slash + final_path = f"{base_path}/{route}" + + return final_path def get_custom_url(request_base_url: str, route: Optional[str] = None) -> str: diff --git a/tests/test_litellm/proxy/test_custom_proxy.py b/tests/test_litellm/proxy/test_custom_proxy.py index ad2cdead09..3663183d21 100644 --- a/tests/test_litellm/proxy/test_custom_proxy.py +++ b/tests/test_litellm/proxy/test_custom_proxy.py @@ -12,6 +12,9 @@ sys.path.insert( 0, os.path.abspath("../../..") ) # Adds the parent directory to the system path +# Set the SERVER_ROOT_PATH environment variable to match the custom mount path +os.environ["SERVER_ROOT_PATH"] = "/my-custom-path" + from litellm.proxy.proxy_server import app as litellm_app from litellm.proxy.proxy_server import proxy_startup_event diff --git a/tests/test_litellm/proxy/test_proxy_utils.py b/tests/test_litellm/proxy/test_proxy_utils.py index 6997ac6527..9d0d5e6c0f 100644 --- a/tests/test_litellm/proxy/test_proxy_utils.py +++ b/tests/test_litellm/proxy/test_proxy_utils.py @@ -16,7 +16,7 @@ sys.path.insert( from unittest.mock import MagicMock -from litellm.proxy.utils import get_custom_url +from litellm.proxy.utils import get_custom_url, join_paths def test_get_custom_url(monkeypatch): @@ -25,7 +25,6 @@ def test_get_custom_url(monkeypatch): assert custom_url == "http://0.0.0.0:4000/litellm/ui/" - def test_proxy_only_error_true_for_llm_route(): proxy_logging_obj = ProxyLogging(user_api_key_cache=DualCache()) assert proxy_logging_obj._is_proxy_only_llm_api_error( @@ -60,8 +59,8 @@ def test_proxy_only_error_false_for_other_error_type(): def test_get_model_group_info_order(): - from litellm.proxy.proxy_server import _get_model_group_info from litellm import Router + from litellm.proxy.proxy_server import _get_model_group_info router = Router( model_list=[ @@ -89,3 +88,47 @@ def test_get_model_group_info_order(): model_groups = [m.model_group for m in model_list] assert model_groups == ["openai/tts-1", "openai/gpt-3.5-turbo"] + + +def test_join_paths_no_duplication(): + """Test that join_paths doesn't duplicate route when base_path already ends with it""" + result = join_paths( + base_path="http://0.0.0.0:4000/my-custom-path/", route="/my-custom-path" + ) + assert result == "http://0.0.0.0:4000/my-custom-path" + + +def test_join_paths_normal_join(): + """Test normal path joining""" + result = join_paths(base_path="http://0.0.0.0:4000", route="/api/v1") + assert result == "http://0.0.0.0:4000/api/v1" + + +def test_join_paths_with_trailing_slash(): + """Test path joining with trailing slash on base_path""" + result = join_paths(base_path="http://0.0.0.0:4000/", route="api/v1") + assert result == "http://0.0.0.0:4000/api/v1" + + +def test_join_paths_empty_base(): + """Test path joining with empty base_path""" + result = join_paths(base_path="", route="api/v1") + assert result == "/api/v1" + + +def test_join_paths_empty_route(): + """Test path joining with empty route""" + result = join_paths(base_path="http://0.0.0.0:4000", route="") + assert result == "http://0.0.0.0:4000" + + +def test_join_paths_both_empty(): + """Test path joining with both empty""" + result = join_paths(base_path="", route="") + assert result == "/" + + +def test_join_paths_nested_path(): + """Test path joining with nested paths""" + result = join_paths(base_path="http://0.0.0.0:4000/v1", route="chat/completions") + assert result == "http://0.0.0.0:4000/v1/chat/completions" diff --git a/ui/litellm-dashboard/src/app/(dashboard)/components/Sidebar2.tsx b/ui/litellm-dashboard/src/app/(dashboard)/components/Sidebar2.tsx index 96bb9b5a4b..06da61a376 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/components/Sidebar2.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/components/Sidebar2.tsx @@ -31,6 +31,7 @@ import * as React from "react"; import { useRouter, usePathname } from "next/navigation"; import { all_admin_roles, internalUserRoles, isAdminRole, rolesWithWriteAccess } from "@/utils/roles"; import UsageIndicator from "@/components/usage_indicator"; +import { serverRootPath } from "@/components/networking"; const { Sider } = Layout; @@ -56,11 +57,22 @@ interface MenuItemCfg { /** * Normalizes NEXT_PUBLIC_BASE_URL to either "/" or "/ui/" (always with a trailing slash). * Supported env values: "" or "ui/". + * Also considers the serverRootPath from the proxy config (e.g., "/my-custom-path"). */ const getBasePath = () => { const raw = process.env.NEXT_PUBLIC_BASE_URL ?? ""; const trimmed = raw.replace(/^\/+|\/+$/g, ""); // strip leading/trailing slashes - return trimmed ? `/${trimmed}/` : "/"; // ensure trailing slash + const uiPath = trimmed ? `/${trimmed}/` : "/"; + + // If serverRootPath is set and not "/", prepend it to the UI path + if (serverRootPath && serverRootPath !== "/") { + // Remove trailing slash from serverRootPath and ensure uiPath has no leading slash for proper joining + const cleanServerRoot = serverRootPath.replace(/\/+$/, ""); + const cleanUiPath = uiPath.replace(/^\/+/, ""); + return `${cleanServerRoot}/${cleanUiPath}`; + } + + return uiPath; }; /** Map legacy `page` ids to real app routes (relative, no leading slash). */ @@ -134,12 +146,8 @@ const toHref = (slugOrPath: string) => { return `${base}${rel}`; }; -const Sidebar2: React.FC = ({ accessToken, userRole, defaultSelectedKey, collapsed = false }) => { - const router = useRouter(); - const pathname = usePathname() || "/"; - - // ----- Menu config (unchanged labels/icons; same appearance) ----- - const menuItems: MenuItemCfg[] = [ +// ----- Menu config (unchanged labels/icons; same appearance) ----- +const menuItems: MenuItemCfg[] = [ { key: "1", page: "api-keys", label: "Virtual Keys", icon: }, { key: "3", @@ -291,6 +299,10 @@ const Sidebar2: React.FC = ({ accessToken, userRole, defaultSelect }, ]; +const Sidebar2: React.FC = ({ accessToken, userRole, defaultSelectedKey, collapsed = false }) => { + const router = useRouter(); + const pathname = usePathname() || "/"; + // ----- Filter by role without mutating originals ----- const filteredMenuItems = React.useMemo(() => { return menuItems diff --git a/ui/litellm-dashboard/src/app/onboarding/page.tsx b/ui/litellm-dashboard/src/app/onboarding/page.tsx index b748446ca9..7e5d91c001 100644 --- a/ui/litellm-dashboard/src/app/onboarding/page.tsx +++ b/ui/litellm-dashboard/src/app/onboarding/page.tsx @@ -74,21 +74,17 @@ export default function Onboarding() { return; } claimOnboardingToken(accessToken, inviteID, userID, formValues.password).then((data) => { - let litellm_dashboard_ui = "/ui/"; - litellm_dashboard_ui += "?login=success"; - // set cookie "token" to jwtToken document.cookie = "token=" + jwtToken; - console.log("redirecting to:", litellm_dashboard_ui); - + const proxyBaseUrl = getProxyBaseUrl(); console.log("proxyBaseUrl:", proxyBaseUrl); + + // Construct the full redirect URL using the proxyBaseUrl which includes the server root path + let redirectUrl = proxyBaseUrl ? `${proxyBaseUrl}/ui/?login=success` : "/ui/?login=success"; + console.log("redirecting to:", redirectUrl); - if (proxyBaseUrl) { - window.location.href = proxyBaseUrl + litellm_dashboard_ui; - } else { - window.location.href = litellm_dashboard_ui; - } + window.location.href = redirectUrl; }); // redirect to login page diff --git a/ui/litellm-dashboard/src/hooks/useFeatureFlags.test.tsx b/ui/litellm-dashboard/src/hooks/useFeatureFlags.test.tsx new file mode 100644 index 0000000000..ca0529b0f2 --- /dev/null +++ b/ui/litellm-dashboard/src/hooks/useFeatureFlags.test.tsx @@ -0,0 +1,296 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { renderHook, waitFor } from "@testing-library/react"; +import { useRouter } from "next/navigation"; +import useFeatureFlags, { FeatureFlagsProvider } from "./useFeatureFlags"; + +// Mock next/navigation +vi.mock("next/navigation", () => ({ + useRouter: vi.fn(), +})); + +// Mock the networking module to control serverRootPath +vi.mock("@/components/networking", () => ({ + serverRootPath: "/", +})); + +describe("useFeatureFlags", () => { + let mockReplace: ReturnType; + let originalLocation: Location; + + beforeEach(() => { + // Mock router + mockReplace = vi.fn(); + (useRouter as ReturnType).mockReturnValue({ + replace: mockReplace, + }); + + // Store original location + originalLocation = window.location; + + // Mock localStorage + Storage.prototype.getItem = vi.fn(() => null); + Storage.prototype.setItem = vi.fn(); + Storage.prototype.removeItem = vi.fn(); + }); + + afterEach(() => { + vi.clearAllMocks(); + // Restore location + Object.defineProperty(window, "location", { + writable: true, + value: originalLocation, + }); + }); + + describe("FeatureFlagsProvider - redirect logic", () => { + it("should not redirect when refactoredUIFlag is true", async () => { + // Set flag to true + Storage.prototype.getItem = vi.fn(() => "true"); + + const { result } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + expect(result.current.refactoredUIFlag).toBe(true); + + // Wait for any effects + await waitFor(() => { + expect(mockReplace).not.toHaveBeenCalled(); + }); + }); + + it("should not redirect when already on a /ui path (race condition protection)", async () => { + // Set flag to false to trigger redirect logic + Storage.prototype.getItem = vi.fn(() => "false"); + + // Mock window.location to be on a custom UI path + delete (window as any).location; + window.location = { + pathname: "/my-custom-path/ui/", + } as Location; + + renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Wait for timeout and check redirect was NOT called + await new Promise((resolve) => setTimeout(resolve, 150)); + + expect(mockReplace).not.toHaveBeenCalled(); + }); + + it("should not redirect when on /ui path without custom root", async () => { + // Set flag to false + Storage.prototype.getItem = vi.fn(() => "false"); + + // Mock window.location to be on standard UI path + delete (window as any).location; + window.location = { + pathname: "/ui/", + } as Location; + + renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Wait for timeout and check redirect was NOT called + await new Promise((resolve) => setTimeout(resolve, 150)); + + expect(mockReplace).not.toHaveBeenCalled(); + }); + + it("should redirect when flag is false and not on a /ui path", async () => { + // Set flag to false + Storage.prototype.getItem = vi.fn(() => "false"); + + // Mock window.location to be on a non-UI path + delete (window as any).location; + window.location = { + pathname: "/some-other-path/", + } as Location; + + renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Wait for timeout plus a bit more + await new Promise((resolve) => setTimeout(resolve, 150)); + + // Should have called replace to redirect to base path + expect(mockReplace).toHaveBeenCalledWith("/"); + }); + + it("should not redirect if already at base path", async () => { + // Set flag to false + Storage.prototype.getItem = vi.fn(() => "false"); + + // Mock window.location to be at root + delete (window as any).location; + window.location = { + pathname: "/", + } as Location; + + renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Wait for timeout + await new Promise((resolve) => setTimeout(resolve, 150)); + + expect(mockReplace).not.toHaveBeenCalled(); + }); + }); + + describe("useFeatureFlags - flag management", () => { + it("should initialize with false when no value in localStorage", () => { + Storage.prototype.getItem = vi.fn(() => null); + + const { result } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + expect(result.current.refactoredUIFlag).toBe(false); + }); + + it("should initialize with true when localStorage has true", () => { + Storage.prototype.getItem = vi.fn(() => "true"); + + const { result } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + expect(result.current.refactoredUIFlag).toBe(true); + }); + + it("should update localStorage when setRefactoredUIFlag is called", () => { + const setItemMock = vi.fn(); + Storage.prototype.setItem = setItemMock; + Storage.prototype.getItem = vi.fn(() => "false"); + + const { result } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + result.current.setRefactoredUIFlag(true); + + expect(setItemMock).toHaveBeenCalledWith( + "feature.refactoredUIFlag", + "true" + ); + }); + + it("should handle malformed localStorage values gracefully", () => { + Storage.prototype.getItem = vi.fn(() => "invalid-value"); + + const { result } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Should default to false for malformed values + expect(result.current.refactoredUIFlag).toBe(false); + }); + }); + + describe("getBasePath logic with serverRootPath", () => { + it("should handle serverRootPath being set to custom path", async () => { + // Mock the networking module with custom serverRootPath + vi.doMock("@/components/networking", () => ({ + serverRootPath: "/my-custom-path", + })); + + // Set flag to false to trigger redirect + Storage.prototype.getItem = vi.fn(() => "false"); + + // Mock location to be on wrong path + delete (window as any).location; + window.location = { + pathname: "/wrong-path/", + } as Location; + + renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Wait for timeout + await new Promise((resolve) => setTimeout(resolve, 150)); + + // With default NEXT_PUBLIC_BASE_URL being empty, should redirect to "/" + // (In reality, with serverRootPath="/my-custom-path", it would be "/my-custom-path/") + expect(mockReplace).toHaveBeenCalled(); + }); + }); + + describe("storage event synchronization", () => { + it("should update flag when storage event is fired", async () => { + Storage.prototype.getItem = vi.fn(() => "false"); + + const { result } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + expect(result.current.refactoredUIFlag).toBe(false); + + // Simulate storage event from another tab + const storageEvent = new StorageEvent("storage", { + key: "feature.refactoredUIFlag", + newValue: "true", + }); + + window.dispatchEvent(storageEvent); + + await waitFor(() => { + expect(result.current.refactoredUIFlag).toBe(true); + }); + }); + + it("should self-heal when storage key is cleared", async () => { + const setItemMock = vi.fn(); + Storage.prototype.setItem = setItemMock; + Storage.prototype.getItem = vi.fn(() => "true"); + + renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Simulate storage event where key was cleared + const storageEvent = new StorageEvent("storage", { + key: "feature.refactoredUIFlag", + newValue: null, + }); + + window.dispatchEvent(storageEvent); + + await waitFor(() => { + expect(setItemMock).toHaveBeenCalledWith( + "feature.refactoredUIFlag", + "false" + ); + }); + }); + }); + + describe("timeout cleanup", () => { + it("should cleanup timeout on unmount", async () => { + Storage.prototype.getItem = vi.fn(() => "false"); + + delete (window as any).location; + window.location = { + pathname: "/some-path/", + } as Location; + + const { unmount } = renderHook(() => useFeatureFlags(), { + wrapper: FeatureFlagsProvider, + }); + + // Unmount immediately before timeout fires + unmount(); + + // Wait past the timeout + await new Promise((resolve) => setTimeout(resolve, 150)); + + // Should not have called replace since component unmounted + expect(mockReplace).not.toHaveBeenCalled(); + }); + }); +}); + diff --git a/ui/litellm-dashboard/src/hooks/useFeatureFlags.tsx b/ui/litellm-dashboard/src/hooks/useFeatureFlags.tsx index d3d676d2d2..03b4465b09 100644 --- a/ui/litellm-dashboard/src/hooks/useFeatureFlags.tsx +++ b/ui/litellm-dashboard/src/hooks/useFeatureFlags.tsx @@ -1,13 +1,24 @@ "use client"; +import React, { createContext, useContext, useEffect, useState } from "react"; +import { useRouter } from "next/navigation"; +import { serverRootPath } from "@/components/networking"; + const getBasePath = () => { const raw = process.env.NEXT_PUBLIC_BASE_URL ?? ""; const trimmed = raw.replace(/^\/+|\/+$/g, ""); // strip leading/trailing slashes - return trimmed ? `/${trimmed}/` : "/"; // ensure trailing slash -}; - -import React, { createContext, useContext, useEffect, useState } from "react"; -import { useRouter } from "next/navigation"; // ⟵ add this + const uiPath = trimmed ? `/${trimmed}/` : "/"; + + // If serverRootPath is set and not "/", prepend it to the UI path + if (serverRootPath && serverRootPath !== "/") { + // Remove trailing slash from serverRootPath and ensure uiPath has no leading slash for proper joining + const cleanServerRoot = serverRootPath.replace(/\/+$/, ""); + const cleanUiPath = uiPath.replace(/^\/+/, ""); + return `${cleanServerRoot}/${cleanUiPath}`; + } + + return uiPath; +} type Flags = { refactoredUIFlag: boolean; @@ -87,15 +98,29 @@ export const FeatureFlagsProvider = ({ children }: { children: React.ReactNode } useEffect(() => { if (refactoredUIFlag) return; // only act when turned off - const base = getBasePath(); - const normalize = (p: string) => (p.endsWith("/") ? p : p + "/"); - const current = normalize(window.location.pathname); + // Wait a moment for serverRootPath to be initialized from getUiConfig() + // This prevents a race condition where we redirect before knowing the correct path + const checkAndRedirect = () => { + const base = getBasePath(); + const normalize = (p: string) => (p.endsWith("/") ? p : p + "/"); + const current = normalize(window.location.pathname); - // Avoid a redirect loop if we're already at the base path. - if (current !== base) { - // Replace so the "off" redirect doesn't pollute history. - router.replace(base); - } + // Don't redirect if we're already on a UI path (even if serverRootPath hasn't loaded yet) + // This handles the case where the page is mounted at a custom server root path + if (current.includes("/ui")) { + return; + } + + // Avoid a redirect loop if we're already at the base path. + if (current !== base) { + // Replace so the "off" redirect doesn't pollute history. + router.replace(base); + } + }; + + // Small delay to allow serverRootPath to be set by getUiConfig() + const timeoutId = setTimeout(checkAndRedirect, 100); + return () => clearTimeout(timeoutId); }, [refactoredUIFlag, router]); return ( diff --git a/ui/litellm-dashboard/src/utils/cookieUtils.ts b/ui/litellm-dashboard/src/utils/cookieUtils.ts index 23682fd6e8..01add36542 100644 --- a/ui/litellm-dashboard/src/utils/cookieUtils.ts +++ b/ui/litellm-dashboard/src/utils/cookieUtils.ts @@ -14,7 +14,18 @@ export function clearTokenCookies() { const domain = window.location.hostname; // Clear with various combinations of path and SameSite + // Include current path in case of custom server root path + const currentPath = window.location.pathname; const paths = ["/", "/ui"]; + + // Add the current path directory if it's different from root and /ui + if (currentPath && currentPath !== "/" && !currentPath.startsWith("/ui")) { + const dirPath = currentPath.substring(0, currentPath.lastIndexOf("/") + 1); + if (dirPath && !paths.includes(dirPath)) { + paths.push(dirPath); + } + } + const sameSiteValues = ["Lax", "Strict", "None"]; paths.forEach((path) => {