From 42f269ccf2af85366b363fd770ab3d9fe4507d00 Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Tue, 21 Jul 2026 16:11:31 -0700 Subject: [PATCH] fix(ui): stop dashboard key-edit form 403ing on non-budget saves (#34112) The key-edit form sent budget_limits on every save (the stored windows, or [] when a key has none). The backend treats any budget_limits in a /key/update request as an admin-only budget change, so a non-admin key owner editing a non-budget field (models, MCP servers, alias) always hit 403 with "Only proxy admins, team admins, or org admins can call /key/update". Only include budget_limits when the user actually changed the budget windows, mirroring how the same handler already drops an unchanged allowed_routes. The comparison is on (duration, cap) ignoring the server-owned reset_at and window order; [] is still sent when the user deletes the last window so clearing keeps working. No backend or API behavior changes. --- .../templates/key_edit_view.test.tsx | 68 ++++++++++++++++++- .../components/templates/key_edit_view.tsx | 25 +++++-- 2 files changed, 85 insertions(+), 8 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 f8b8e5b6de3..8d7c8e76011 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 @@ -662,11 +662,14 @@ describe("KeyEditView", () => { }); }); - it("should resend existing budget windows on submit when they are left untouched", async () => { + it("should omit budget_limits when existing windows are left untouched (issue #33246)", async () => { + // The backend treats any budget_limits in the payload as an admin-only + // budget change, so re-sending untouched windows 403s a non-admin owner. + // Leaving the field off keeps the stored windows and passes the gate. const onSubmitMock = vi.fn().mockResolvedValue(undefined); const keyDataWithWindow = { ...MOCK_KEY_DATA, - budget_limits: [{ budget_duration: "30d", max_budget: 100 }], + budget_limits: [{ budget_duration: "30d", max_budget: 100, reset_at: "2026-08-01T00:00:00" }], }; renderWithProviders( { await waitFor(() => { expect(onSubmitMock).toHaveBeenCalled(); const callArgs = onSubmitMock.mock.calls[0][0]; - expect(callArgs.budget_limits).toEqual([{ budget_duration: "30d", max_budget: 100 }]); + expect(callArgs.budget_limits).toBeUndefined(); + }); + }); + + it("should omit budget_limits on a key that has no windows (issue #33246 repro)", async () => { + // Core repro: a non-admin owner edits a non-budget field on a key with no + // budget windows. The form previously always sent budget_limits: [], which + // the backend read as a budget change and rejected. + const onSubmitMock = vi.fn().mockResolvedValue(undefined); + renderWithProviders( + {}} + onSubmit={onSubmitMock} + accessToken={"test-token"} + userID={"test-user"} + userRole={"admin"} + premiumUser={false} + />, + ); + + const submitButton = await screen.findByRole("button", { name: /save changes/i }); + await userEvent.click(submitButton); + + await waitFor(() => { + expect(onSubmitMock).toHaveBeenCalled(); + const callArgs = onSubmitMock.mock.calls[0][0]; + expect(callArgs.budget_limits).toBeUndefined(); + }); + }); + + it("should send budget_limits when a window's cap is changed", async () => { + const onSubmitMock = vi.fn().mockResolvedValue(undefined); + const keyDataWithWindow = { + ...MOCK_KEY_DATA, + budget_limits: [{ budget_duration: "30d", max_budget: 100 }], + }; + renderWithProviders( + {}} + onSubmit={onSubmitMock} + accessToken={"test-token"} + userID={"test-user"} + userRole={"admin"} + premiumUser={false} + />, + ); + + const maxBudgetInput = await screen.findByPlaceholderText("Max spend ($)"); + await userEvent.clear(maxBudgetInput); + await userEvent.type(maxBudgetInput, "200"); + + 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(callArgs.budget_limits).toEqual([{ budget_duration: "30d", max_budget: 200 }]); }); }); 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 dce7b9bd949..79fa9c63890 100644 --- a/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx +++ b/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx @@ -307,14 +307,29 @@ export function KeyEditView({ } // Reconcile multi-window budget limits from the editor state, dropping - // incomplete entries (no max_budget). Sending [] tells the backend to clear - // all stored windows, so only send it when the user removed every window; - // when entries remain but are still incomplete, omit the field so the saved - // windows are left untouched (JSON.stringify drops the undefined key). + // incomplete entries (no max_budget). The backend treats any budget_limits + // in a /key/update request as an admin-only budget change, so re-sending + // the stored windows on an unrelated edit 403s a non-admin key owner + // (issue #33246). Only send the field when the user actually changed the + // windows, mirroring how allowed_routes is dropped above when unchanged: + // compare on (duration, cap), ignoring server-owned reset_at and order. + // Sending [] clears every window, so send it only when the user removed + // the last one; otherwise leave the field off (JSON.stringify drops the + // undefined key) so an unchanged or incomplete editor state never touches + // storage. + const windowSignature = (windows: Array<{ budget_duration: string; max_budget: number | null }> | undefined) => + (windows ?? []) + .filter((w) => w.budget_duration && w.max_budget !== null && w.max_budget !== undefined) + .map((w) => `${w.budget_duration}:${w.max_budget}`) + .sort() + .join("|"); const validWindows = budgetLimits.filter( (w) => w.budget_duration && w.max_budget !== null && w.max_budget !== undefined, ); - if (validWindows.length > 0) { + const budgetLimitsUnchanged = windowSignature(keyData.budget_limits) === windowSignature(validWindows); + if (budgetLimitsUnchanged) { + // no-op: leave budget_limits off the payload + } else if (validWindows.length > 0) { values.budget_limits = validWindows; } else if (budgetLimits.length === 0) { values.budget_limits = [];