From 0af33fbe7004d8d13aaa912373bba2d20e62042b Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Sat, 9 May 2026 19:01:58 -0700 Subject: [PATCH] fix(ui): omit allowed_routes from key edit save when unchanged (#27553) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ui): omit allowed_routes from key edit save when unchanged When a team admin opens Edit Settings on a key with key_type=AI APIs and saves without changing anything, the UI re-sends the existing allowed_routes value, which the backend's _check_allowed_routes_caller_permission gate rejects for non-proxy-admins (LIT-2681). Strip allowed_routes from the patch in handleSubmit when it deep-equals the original keyData.allowed_routes. The backend treats absence as "leave alone," so no-op saves now succeed for non-admins. Admins explicitly editing the field still send the new value. * fix(ui): order-insensitive allowed_routes diff + cover null-original case Address Greptile review: - Switch the "is allowed_routes unchanged" check to a Set-based comparison so a server-side reorder of the array doesn't register as a user edit and re-trigger LIT-2681. - Add two regression tests: (1) keyData.allowed_routes is null and the form is untouched — patch should strip the field; (2) server returned routes in a different order than the user originally entered — patch should still recognize the value as unchanged. * chore(ui): strip ticket refs and tighten comments in key edit fix - Remove internal-tracker references from in-code comments - Tighten the WHY comment in handleSubmit to two lines - Drop redundant test-block comments — test names already describe the case * fix(ui): annotate Set generic in allowed_routes diff to fix tsc --- .../templates/key_edit_view.test.tsx | 162 ++++++++++++++---- .../components/templates/key_edit_view.tsx | 82 +++++---- 2 files changed, 180 insertions(+), 64 deletions(-) diff --git a/ui/litellm-dashboard/src/components/templates/key_edit_view.test.tsx b/ui/litellm-dashboard/src/components/templates/key_edit_view.test.tsx index 2e4d0d97e4..1886075a9d 100644 --- a/ui/litellm-dashboard/src/components/templates/key_edit_view.test.tsx +++ b/ui/litellm-dashboard/src/components/templates/key_edit_view.test.tsx @@ -158,8 +158,8 @@ describe("KeyEditView", () => { const { getByText } = renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -176,8 +176,8 @@ describe("KeyEditView", () => { const { getByText } = renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -194,8 +194,8 @@ describe("KeyEditView", () => { const { getByLabelText } = renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -219,7 +219,7 @@ describe("KeyEditView", () => { { }} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -241,8 +241,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -259,8 +259,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -277,8 +277,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -295,8 +295,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -314,7 +314,7 @@ describe("KeyEditView", () => { renderWithProviders( { }} + onCancel={() => {}} onSubmit={onSubmitMock} accessToken={"test-token"} userID={"test-user"} @@ -344,8 +344,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -367,8 +367,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={""} userID={""} userRole={""} @@ -385,8 +385,8 @@ describe("KeyEditView", () => { renderWithProviders( { }} - onSubmit={async () => { }} + onCancel={() => {}} + onSubmit={async () => {}} accessToken={"test-token"} userID={""} userRole={""} @@ -404,7 +404,7 @@ describe("KeyEditView", () => { renderWithProviders( { }} + onCancel={() => {}} onSubmit={onSubmitMock} accessToken={"test-token"} userID={"test-user"} @@ -434,10 +434,14 @@ describe("KeyEditView", () => { it("should handle empty allowed routes string on submit", async () => { const onSubmitMock = vi.fn().mockResolvedValue(undefined); + const keyDataWithRoutes = { + ...MOCK_KEY_DATA, + allowed_routes: ["llm_api_routes"], + }; renderWithProviders( { }} + keyData={keyDataWithRoutes} + onCancel={() => {}} onSubmit={onSubmitMock} accessToken={"test-token"} userID={"test-user"} @@ -463,6 +467,101 @@ describe("KeyEditView", () => { }); }); + it("should omit allowed_routes from submit when value is unchanged", async () => { + const onSubmitMock = vi.fn().mockResolvedValue(undefined); + const aiApisKeyData = { + ...MOCK_KEY_DATA, + allowed_routes: ["llm_api_routes"], + }; + renderWithProviders( + {}} + onSubmit={onSubmitMock} + accessToken={"test-token"} + userID={"test-user"} + userRole={"admin"} + premiumUser={false} + />, + ); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + const submitButton = screen.getByRole("button", { name: /save changes/i }); + await userEvent.click(submitButton); + + await waitFor(() => { + expect(onSubmitMock).toHaveBeenCalled(); + const callArgs = onSubmitMock.mock.calls[0][0]; + expect("allowed_routes" in callArgs).toBe(false); + }); + }); + + it("should omit allowed_routes from submit when keyData.allowed_routes is null and form is untouched", async () => { + const onSubmitMock = vi.fn().mockResolvedValue(undefined); + const keyDataNullRoutes = { + ...MOCK_KEY_DATA, + allowed_routes: null as unknown as string[], + }; + renderWithProviders( + {}} + onSubmit={onSubmitMock} + accessToken={"test-token"} + userID={"test-user"} + userRole={"admin"} + premiumUser={false} + />, + ); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + const submitButton = screen.getByRole("button", { name: /save changes/i }); + await userEvent.click(submitButton); + + await waitFor(() => { + expect(onSubmitMock).toHaveBeenCalled(); + const callArgs = onSubmitMock.mock.calls[0][0]; + expect("allowed_routes" in callArgs).toBe(false); + }); + }); + + it("should omit allowed_routes from submit when server returned routes in a different order", async () => { + const onSubmitMock = vi.fn().mockResolvedValue(undefined); + const keyDataReordered = { + ...MOCK_KEY_DATA, + allowed_routes: ["beta_routes", "alpha_routes"], + }; + renderWithProviders( + {}} + onSubmit={onSubmitMock} + accessToken={"test-token"} + userID={"test-user"} + userRole={"admin"} + premiumUser={false} + />, + ); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + const submitButton = screen.getByRole("button", { name: /save changes/i }); + await userEvent.click(submitButton); + + await waitFor(() => { + expect(onSubmitMock).toHaveBeenCalled(); + const callArgs = onSubmitMock.mock.calls[0][0]; + expect("allowed_routes" in callArgs).toBe(false); + }); + }); it("should pass access_group_ids to onSubmit when saving key with access groups", async () => { const onSubmitMock = vi.fn().mockResolvedValue(undefined); @@ -554,7 +653,7 @@ describe("KeyEditView", () => { renderWithProviders( { }} + onCancel={() => {}} onSubmit={onSubmitMock} accessToken={"test-token"} userID={"test-user"} @@ -576,10 +675,13 @@ describe("KeyEditView", () => { }); // Wait for the cancel button to actually be disabled (state update may take a moment) - await waitFor(() => { - const cancelButton = screen.getByRole("button", { name: /cancel/i }); - expect(cancelButton).toBeDisabled(); - }, { timeout: 3000 }); + await waitFor( + () => { + const cancelButton = screen.getByRole("button", { name: /cancel/i }); + expect(cancelButton).toBeDisabled(); + }, + { timeout: 3000 }, + ); // Clean up: resolve the promise to allow the form to complete if (resolveSubmit) { diff --git a/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx b/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx index 9b38d930a5..d5e410029a 100644 --- a/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx +++ b/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx @@ -78,7 +78,6 @@ const getKeyTypeFromRoutes = (allowedRoutes: string[] | null | undefined): strin return "default"; }; - export function KeyEditView({ keyData, onCancel, @@ -106,7 +105,7 @@ export function KeyEditView({ const [neverExpire, setNeverExpire] = useState(!keyData.expires); const [isKeySaving, setIsKeySaving] = useState(false); const [budgetLimits, setBudgetLimits] = useState( - Array.isArray(keyData.budget_limits) ? keyData.budget_limits : [] + Array.isArray(keyData.budget_limits) ? keyData.budget_limits : [], ); const { data: organizations, isLoading: isOrganizationsLoading } = useOrganizations(); const { data: projects } = useProjects(); @@ -116,9 +115,7 @@ export function KeyEditView({ const projectDisplay = (() => { if (!keyData.project_id) return null; const project = projects?.find((p) => p.project_id === keyData.project_id); - return project?.project_alias - ? `${project.project_alias} (${keyData.project_id})` - : keyData.project_id; + return project?.project_alias ? `${project.project_alias} (${keyData.project_id})` : keyData.project_id; })(); useEffect(() => { @@ -198,9 +195,10 @@ export function KeyEditView({ access_group_ids: keyData.access_group_ids || [], auto_rotate: keyData.auto_rotate || false, ...(keyData.rotation_interval && { rotation_interval: keyData.rotation_interval }), - allowed_routes: Array.isArray(keyData.allowed_routes) && keyData.allowed_routes.length > 0 - ? keyData.allowed_routes.join(", ") - : "", + allowed_routes: + Array.isArray(keyData.allowed_routes) && keyData.allowed_routes.length > 0 + ? keyData.allowed_routes.join(", ") + : "", }; useEffect(() => { @@ -226,9 +224,10 @@ export function KeyEditView({ access_group_ids: keyData.access_group_ids || [], auto_rotate: keyData.auto_rotate || false, ...(keyData.rotation_interval && { rotation_interval: keyData.rotation_interval }), - allowed_routes: Array.isArray(keyData.allowed_routes) && keyData.allowed_routes.length > 0 - ? keyData.allowed_routes.join(", ") - : "", + allowed_routes: + Array.isArray(keyData.allowed_routes) && keyData.allowed_routes.length > 0 + ? keyData.allowed_routes.join(", ") + : "", }); }, [keyData, form]); @@ -275,12 +274,25 @@ export function KeyEditView({ } // If it's already an array (shouldn't happen, but handle it), keep as is + // Backend rejects non-empty allowed_routes from non-admins, so re-sending + // an unchanged value 403s a team admin. Set compare tolerates reorder. + const originalRoutesSet = new Set(Array.isArray(keyData.allowed_routes) ? keyData.allowed_routes : []); + const submittedRoutesSet = new Set(Array.isArray(values.allowed_routes) ? values.allowed_routes : []); + const allowedRoutesUnchanged = + originalRoutesSet.size === submittedRoutesSet.size && + [...submittedRoutesSet].every((r) => originalRoutesSet.has(r)); + if (allowedRoutesUnchanged) { + delete values.allowed_routes; + } + if (neverExpire) { values.duration = null; } // Include multi-window budget limits (filter out incomplete entries) - const validWindows = budgetLimits.filter((w) => w.budget_duration && w.max_budget !== null && w.max_budget !== undefined); + const validWindows = budgetLimits.filter( + (w) => w.budget_duration && w.max_budget !== null && w.max_budget !== undefined, + ); values.budget_limits = validWindows.length > 0 ? validWindows : undefined; await onSubmit(values); @@ -305,9 +317,13 @@ export function KeyEditView({ {({ getFieldValue, setFieldValue }) => { const allowedRoutesValue = getFieldValue("allowed_routes") || ""; // Convert string to array for checking - const allowedRoutes = typeof allowedRoutesValue === "string" && allowedRoutesValue.trim() !== "" - ? allowedRoutesValue.split(",").map((r: string) => r.trim()).filter((r: string) => r.length > 0) - : []; + const allowedRoutes = + typeof allowedRoutesValue === "string" && allowedRoutesValue.trim() !== "" + ? allowedRoutesValue + .split(",") + .map((r: string) => r.trim()) + .filter((r: string) => r.length > 0) + : []; const isDisabled = allowedRoutes.includes("management_routes") || allowedRoutes.includes("info_routes"); const models = getFieldValue("models") || []; @@ -348,9 +364,13 @@ export function KeyEditView({ {({ getFieldValue, setFieldValue }) => { const allowedRoutesValue = getFieldValue("allowed_routes") || ""; // Convert string to array for getKeyTypeFromRoutes - const allowedRoutes = typeof allowedRoutesValue === "string" && allowedRoutesValue.trim() !== "" - ? allowedRoutesValue.split(",").map((r: string) => r.trim()).filter((r: string) => r.length > 0) - : []; + const allowedRoutes = + typeof allowedRoutesValue === "string" && allowedRoutesValue.trim() !== "" + ? allowedRoutesValue + .split(",") + .map((r: string) => r.trim()) + .filter((r: string) => r.length > 0) + : []; const keyTypeValue = getKeyTypeFromRoutes(allowedRoutes); return ( @@ -415,9 +435,7 @@ export function KeyEditView({ } name="allowed_routes" > - + @@ -442,10 +460,7 @@ export function KeyEditView({ } > - + @@ -579,7 +594,7 @@ export function KeyEditView({ !premiumUser ? "Premium feature - Upgrade to set allowed pass through routes by key" : Array.isArray(keyData.metadata?.allowed_passthrough_routes) && - keyData.metadata.allowed_passthrough_routes.length > 0 + keyData.metadata.allowed_passthrough_routes.length > 0 ? `Current: ${keyData.metadata.allowed_passthrough_routes.join(", ")}` : "Select or enter allowed pass through routes" } @@ -690,14 +705,13 @@ export function KeyEditView({ return team.team_alias?.toLowerCase().includes(input.toLowerCase()) ?? false; }} > - {(selectedOrganizationId - ? teams?.filter((t) => t.organization_id === selectedOrganizationId) - : teams - )?.map((team) => ( - - {`${team.team_alias} (${team.team_id})`} - - ))} + {(selectedOrganizationId ? teams?.filter((t) => t.organization_id === selectedOrganizationId) : teams)?.map( + (team) => ( + + {`${team.team_alias} (${team.team_id})`} + + ), + )} {enableProjectsUI && hasProject && (