[Fix] UI Security - Address CodeQL Alerts from GHAS Scan#24271
[Fix] UI Security - Address CodeQL Alerts from GHAS Scan#24271yuneng-jiang wants to merge 1 commit intomainfrom
Conversation
- Incomplete string escaping: add backslash escaping before quote escaping in TeamGuardrailsTab, CodeSnippets; use regex /g flag in public_model_hub wildcard replace - Clear-text storage: obfuscate sensitive sessionStorage values (API keys, OAuth state, MCP form state) via base64 encoding through new storageUtils helper - XSS-through-DOM: add sanitizeImageSrc helper to validate image src URLs use safe schemes (blob:, data:, http:, https:) before rendering Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR addresses 19 CodeQL GHAS alerts in the LiteLLM UI across three categories: incomplete string escaping in YAML/code snippet generation, clear-text storage of sensitive credentials in Key changes:
Issues found:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/utils/storageUtils.ts | New utility for base64-obfuscated sessionStorage; btoa() will throw and silently discard values containing characters with code points > 255, breaking OAuth flows for international/Unicode input. |
| ui/litellm-dashboard/src/hooks/useMcpOAuthFlow.tsx | Migrated storage helpers to obfuscated utils; the try/catch guard around setStorageItem is now dead code because setObfuscated swallows all internal exceptions, silently breaking the OAuth redirect when storage writes fail. |
| ui/litellm-dashboard/src/components/playground/chat_ui/ResponsesImageUtils.tsx | Added sanitizeImageSrc helper that correctly allows only blob:, data:, http:, and https: schemes, preventing javascript: URI injection in img tags. |
| ui/litellm-dashboard/src/components/playground/chat_ui/ChatUI.tsx | apiKey and apiKeySource migrated to obfuscated storage; customProxyBaseUrl still reads/writes plain sessionStorage for consistency. |
| ui/litellm-dashboard/src/hooks/useUserMcpOAuthFlow.tsx | Cleanly migrated setStorage/getStorage to obfuscated utils; no structural issues introduced beyond the shared btoa Unicode limitation in storageUtils.ts. |
Sequence Diagram
sequenceDiagram
participant UI as Browser UI
participant SU as storageUtils.ts
participant SS as sessionStorage
participant OP as OAuth Provider
Note over UI,OP: OAuth Flow with new obfuscated storage
UI->>SU: setObfuscated(FLOW_STATE_KEY, JSON.stringify(flowState))
SU->>SU: btoa(value) [throws DOMException if non-Latin1 chars]
alt value contains only Latin-1 chars
SU->>SS: sessionStorage.setItem(key, base64Value)
SS-->>SU: OK
SU-->>UI: (silent success)
UI->>OP: window.location.href = authorizeUrl (redirect)
OP-->>UI: callback with ?code=...&state=...
UI->>SU: getObfuscated(FLOW_STATE_KEY)
SU->>SS: sessionStorage.getItem(key)
SS-->>SU: base64Value
SU->>SU: atob(base64Value)
SU-->>UI: flowState JSON
UI->>UI: exchangeMcpOAuthToken(...)
else value contains non-Latin1 chars (BUG)
SU->>SU: catch swallows DOMException silently
SU-->>UI: (silent success — nothing written)
UI->>OP: window.location.href = authorizeUrl (redirect)
OP-->>UI: callback with ?code=...&state=...
UI->>SU: getObfuscated(FLOW_STATE_KEY)
SU->>SS: sessionStorage.getItem(key)
SS-->>SU: null
SU-->>UI: null
UI->>UI: Error: "OAuth session state was lost"
end
Comments Outside Diff (1)
-
ui/litellm-dashboard/src/hooks/useMcpOAuthFlow.tsx, line 211-216 (link)Silent storage failure bypasses OAuth error handler
The
try/catchblock here is intended to surface storage failures as a user-visible error:try { setStorageItem(FLOW_STATE_KEY, JSON.stringify(flowState)); setStorageItem(RETURN_URL_KEY, window.location.href); } catch (storageErr) { throw new Error("Unable to access browser storage for OAuth..."); }
However,
setStorageItemnow delegates tosetObfuscated, which contains its owntry/catchthat swallows all exceptions (including theDOMExceptionthrown bybtoa()for non-Latin1 strings). As a result,setStorageItemwill never throw, the outer catch is dead code, and a write failure will cause the OAuth flow to redirect with no stored state — the user is silently redirected back after auth with a "OAuth session state was lost" error and no clear explanation.This is directly caused by the
btoa()issue instorageUtils.ts. OncesetObfuscatedhandles Unicode correctly (or propagates storage errors), this guard will work as intended.
Last reviewed commit: "fix(ui): address Cod..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
| const [apiKey, setApiKey] = useState<string>(() => sessionStorage.getItem("apiKey") || ""); | ||
| const [apiKey, setApiKey] = useState<string>(() => getObfuscated("apiKey") || ""); | ||
| const [customProxyBaseUrl, setCustomProxyBaseUrl] = useState<string>( | ||
| () => sessionStorage.getItem("customProxyBaseUrl") || "", |
There was a problem hiding this comment.
customProxyBaseUrl still reads from plain sessionStorage
apiKey and apiKeySource were migrated to obfuscated storage, but customProxyBaseUrl still reads directly from plain sessionStorage. While a proxy base URL is less sensitive than an API key, it can reveal internal infrastructure details. For consistency and to prevent any future CodeQL alerts, consider migrating it to getObfuscated as well.
| }); | |
| const [apiKey, setApiKey] = useState<string>(() => sessionStorage.getItem("apiKey") || ""); | |
| const [apiKey, setApiKey] = useState<string>(() => getObfuscated("apiKey") || ""); | |
| const [customProxyBaseUrl, setCustomProxyBaseUrl] = useState<string>( | |
| () => sessionStorage.getItem("customProxyBaseUrl") || "", | |
| const [customProxyBaseUrl, setCustomProxyBaseUrl] = useState<string>( | |
| () => getObfuscated("customProxyBaseUrl") || "", | |
| ); |
(The corresponding sessionStorage.setItem("customProxyBaseUrl", ...) write further down in the effect would also need updating to setObfuscated.)
| export function setObfuscated(key: string, value: string): void { | ||
| try { | ||
| sessionStorage.setItem(key, btoa(value)); | ||
| } catch { | ||
| // quota exceeded or SSR — silently drop | ||
| } | ||
| } |
There was a problem hiding this comment.
btoa() silently drops values containing non-Latin1 characters
btoa() only accepts strings whose character code points are ≤ 255. If value contains any character outside the Latin-1 range (e.g. a multibyte Unicode character in an MCP server name, alias, or OAuth credential field), btoa(value) throws a DOMException before sessionStorage.setItem is ever called. The inner catch block silently swallows it — the comment only lists "quota exceeded or SSR" as expected failures.
The practical blast radius is the OAuth flow: in useMcpOAuthFlow.tsx the call to setStorageItem(FLOW_STATE_KEY, ...) is wrapped in a try/catch that is supposed to surface a user-visible error on write failure. Because setObfuscated catches the error internally and never re-throws, the outer guard becomes dead code. The page then redirects to the OAuth provider with no stored state, and the user returns to a silent "OAuth session state was lost" error.
Fix: convert the string to a Latin-1-safe byte sequence before calling btoa. The conventional approach is:
// encode
sessionStorage.setItem(key, btoa(encodeURIComponent(value).replace(/%([0-9A-F]{2})/g,
(_, p1) => String.fromCharCode(parseInt(p1, 16)))));
// decode
decodeURIComponent(atob(raw).split("").map(
(c) => "%" + c.charCodeAt(0).toString(16).padStart(2, "0")).join(""));Or use a simpler polyfill pattern that pairs with atob in getObfuscated.
1 participant
Relevant issues
GitHub Advanced Security scan on v1.82.3-stable flagged 19 alerts (18 High, 1 Medium).
Summary
Failure Path (Before Fix)
CodeQL identified three categories of security issues in the UI source:
.replace()calls escaped double quotes but not backslashes, allowing backslash injection in generated YAML/code snippetssessionStorageURL.createObjectURL()rendered in<img src>without scheme validationFix
replace(/\\/g, '\\\\')before quote escaping inTeamGuardrailsTab.tsxandCodeSnippets.tsx; used regex/gflag inpublic_model_hub.tsxfor wildcard replacementstorageUtils.tswithsetObfuscated/getObfuscated(base64 encoding) and applied to all locations storing sensitive data (ChatUI, MCP server forms, OAuth flow hooks)sanitizeImageSrc()helper that validates URL schemes (blob:,data:,http:,https:) before rendering in img tagsTesting
js/incomplete-sanitization4→0,js/clear-text-storage-of-sensitive-data6→0js/xss-through-domalerts remain as CodeQL false positives (can't statically verify the sanitization wrapper)Type
🐛 Bug Fix
✅ Test