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.
This commit is contained in:
ryan-crabbe-berri 2026-07-21 16:11:31 -07:00 committed by GitHub
parent 2b2ae4ca49
commit 42f269ccf2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 85 additions and 8 deletions

View file

@ -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(
<KeyEditView
@ -686,7 +689,66 @@ describe("KeyEditView", () => {
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(
<KeyEditView
keyData={MOCK_KEY_DATA} // no budget_limits
onCancel={() => {}}
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(
<KeyEditView
keyData={keyDataWithWindow}
onCancel={() => {}}
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 }]);
});
});

View file

@ -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 = [];