fix(ui): omit unchanged allowed_routes on key update - #27148
fix(ui): omit unchanged allowed_routes on key update#27148krrish-berri-2 wants to merge 2 commits into
Conversation
Non-admin team admins could not save key edit settings because the UI always submitted the current allowed_routes value in the update payload, triggering the backend permission check added in PR #25445. This follows the established precedent from commit 2c41f3c (policies field fix): strip allowed_routes from the update payload when the form value equals the previously persisted value so non-admin editors don't trip the backend 'setting allowed_routes' permission check on a no-op save. Genuine route changes (including clears) still pass through. Adds normalizeStringList and areStringListsEqual helpers for robust comparison of the route list values. Closes #27005 Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com>
|
|
Greptile SummaryThis PR fixes a 403 error for non-admin team admins saving key settings by stripping Confidence Score: 4/5Safe to merge; the fix is well-scoped and backed by tests, with only a minor order-sensitivity edge case. Only P2 findings — the positional comparison in No files require special attention.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/templates/key_info_view.tsx | Adds normalizeStringList/areStringListsEqual helpers and strips allowed_routes from the update payload when unchanged; order-sensitive comparison may occasionally send unnecessary updates if route order differs. |
| ui/litellm-dashboard/src/components/templates/key_info_view.test.tsx | Adds 3 focused tests covering the main normalization cases (unchanged routes dropped, empty-to-empty dropped, clear kept); tests only exercise the array path of normalizeStringList, leaving the comma-separated-string branch untested. |
Reviews (1): Last reviewed commit: "fix(ui): omit unchanged allowed_routes o..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const areStringListsEqual = (left: unknown, right: unknown): boolean => { | ||
| const normalizedLeft = normalizeStringList(left); | ||
| const normalizedRight = normalizeStringList(right); | ||
| return ( | ||
| normalizedLeft.length === normalizedRight.length && | ||
| normalizedLeft.every((entry, index) => entry === normalizedRight[index]) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
areStringListsEqual compares lists positionally (index-by-index). If the backend returns ["b", "a"] and the form submits ["a", "b"] — same set, different order — the two lists are deemed unequal and the update is sent, which would still trip the backend permission check for a no-op save. Sorting before comparison makes the equality check order-independent, matching the semantic intent of route lists.
| const areStringListsEqual = (left: unknown, right: unknown): boolean => { | |
| const normalizedLeft = normalizeStringList(left); | |
| const normalizedRight = normalizeStringList(right); | |
| return ( | |
| normalizedLeft.length === normalizedRight.length && | |
| normalizedLeft.every((entry, index) => entry === normalizedRight[index]) | |
| ); | |
| }; | |
| const areStringListsEqual = (left: unknown, right: unknown): boolean => { | |
| const normalizedLeft = normalizeStringList(left).sort(); | |
| const normalizedRight = normalizeStringList(right).sort(); | |
| return ( | |
| normalizedLeft.length === normalizedRight.length && | |
| normalizedLeft.every((entry, index) => entry === normalizedRight[index]) | |
| ); | |
| }; |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…-edit-allowed-routes-27005-b201
|
@krrish-berri-2 note that this seemed to only partially fix #27005 - still seeing this bug in v1.90.0, check my new comment in the linked issue |
6 participants
Relevant issues
Fixes #27005
Pre-Submission checklist
tests/directory — 3 new tests added inkey_info_view.test.tsx@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
🐛 Bug Fix
Changes
Non-admin team admins cannot save key edit settings because the UI always submits the current
allowed_routesvalue in the update payload, triggering the backend permission check (_check_allowed_routes_caller_permission) added in PR #25445.This fix follows the established precedent from commit
2c41f3c29(thepoliciesfield fix): stripallowed_routesfrom the update payload when the form value equals the previously persisted value, so non-admin editors don't trip the backend "setting allowed_routes" permission check on a no-op save. Genuine route changes (including clears) still pass through.What changed
key_info_view.tsx:normalizeStringListandareStringListsEqualhelper functions for robust comparison of route list values (handles arrays, comma-separated strings, whitespace normalization)handleKeyUpdatethat stripsallowed_routesfrom the update payload when the submitted value matches the existing key's valuekey_info_view.test.tsx:allowed_routes payload normalizationtest suite with 3 tests:allowed_routeswhen submitted value matches existing key (the bug fix)allowed_routeswhen the key previously had no route overrideallowed_routeswhen the user genuinely clears an existing route overrideRoot cause
PR #16034 (UI Key Type select) made the edit form always submit
allowed_routes. PR #25445 added_check_allowed_routes_caller_permissionto block non-admins from settingallowed_routes. The collision means any non-admin saving a key (even without changes) gets a 403.Screenshots / Proof of Fix
Backend verification
Confirmed the backend bug: a team admin sending unchanged
allowed_routesin the update payload gets 403:Test results (all 28 tests pass, including 3 new allowed_routes tests)
Test results showing all 28 tests passing
Video walkthrough of tests running
test_results_allowed_routes_fix.mp4
To show artifacts inline, enable in settings.
Slack Thread