fix: harden CORS credentials, create_views exception handling, and spend log cleanup loop - #25559
Conversation
…nup loop - proxy_server.py: disable allow_credentials when allow_origins=['*'] (wildcard + credentials is a browser security misconfiguration). Add LITELLM_CORS_ORIGINS env var to configure explicit allowed origins. - create_views.py: narrow broad 'except Exception' to only catch genuine 'view does not exist' errors; re-raise all other DB errors (auth, connection, etc.) that were previously silently swallowed. - spend_log_cleanup.py: validate execute_raw() return type is int before using it as a deletion count; break loop safely on unexpected types to prevent infinite deletion loops.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR hardens three independent issues: CORS credentials are now auto-disabled when wildcard origins are in use (with a Confidence Score: 5/5Safe to merge — all three fixes are correct, well-tested, and address real production issues. All remaining findings are P2 style suggestions (missing return type annotation). No P0/P1 issues remain after the previous thread concerns were resolved in this implementation. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Introduces _get_cors_config() to compute origins/credentials from env vars; correctly disables credentials when wildcard origins are used. Missing return type annotation on the new helper. |
| litellm/proxy/db/create_views.py | Replaces bare except Exception: pass with a module-level _VIEW_NOT_FOUND_MARKERS tuple; non-view-not-found errors are now re-raised correctly. Print statements replaced with verbose_logger.debug. |
| litellm/proxy/db/db_transaction_queue/spend_log_cleanup.py | Adds isinstance(deleted_result, int) guard in _delete_old_logs; non-int return from execute_raw now breaks the loop instead of silently continuing, preventing an infinite deletion loop. |
| tests/test_litellm/proxy/test_cors_config.py | Tests import _get_cors_config directly from proxy_server, exercising real production code. Covers wildcard/explicit origins, blank entries, credential overrides, and a module-level invariant check. |
| tests/test_litellm/proxy/db/test_create_views.py | New test file covering re-raise on connection/permission errors and creation on does not exist/undefined table markers. The undefined function test correctly verifies bare undefined is no longer treated as a view-not-found signal. |
| tests/test_litellm/proxy/test_spend_log_cleanup.py | New tests verify abort-on-non-int and multi-batch continuation; assertion formatting on existing test reformatted without weakening coverage. |
| docs/my-website/docs/proxy/config_settings.md | Adds documentation for new LITELLM_CORS_ALLOW_CREDENTIALS and LITELLM_CORS_ORIGINS env vars in alphabetical order. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[proxy_server startup] --> B["_get_cors_config()"]
B --> C{LITELLM_CORS_ORIGINS set?}
C -- No / empty --> D["origins = ['*']"]
C -- Yes --> E["origins = parsed list"]
D --> F{LITELLM_CORS_ALLOW_CREDENTIALS set?}
E --> F
F -- Yes --> G["allow_credentials = env value (true/false)"]
F -- No --> H{"'*' in origins?"}
H -- Yes --> I["allow_credentials = False ✅ secure default"]
H -- No --> J["allow_credentials = True"]
G --> K[CORSMiddleware configured]
I --> K
J --> K
subgraph create_views
L["query_raw(SELECT 1 FROM view)"]
L -- success --> M[View exists, skip]
L -- exception --> N{error_msg in _VIEW_NOT_FOUND_MARKERS?}
N -- Yes --> O["execute_raw(CREATE VIEW)"]
N -- No --> P[re-raise - real DB error]
end
subgraph spend_log_cleanup
Q["execute_raw(DELETE ...)"]
Q --> R{isinstance result int?}
R -- Yes --> S{result == 0?}
S -- No --> T[total_deleted += result]
T --> Q
S -- Yes --> U[break - done]
R -- No --> V[log error + break - prevents infinite loop]
end
Reviews (5): Last reviewed commit: "docs: add LITELLM_CORS_ORIGINS and LITEL..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _compute_cors_config(cors_origins_env): | ||
| """ | ||
| Mirror of the CORS config logic in proxy_server.py. | ||
| Kept here so tests remain isolated from module-level side-effects. | ||
| """ | ||
| if cors_origins_env is None or cors_origins_env.strip() == "": | ||
| origins = ["*"] | ||
| else: | ||
| origins = [o.strip() for o in cors_origins_env.split(",") if o.strip()] | ||
| allow_cors_credentials = "*" not in origins | ||
| return origins, allow_cors_credentials |
There was a problem hiding this comment.
_compute_cors_config is a local copy of the production logic rather than an import from proxy_server.py. If the origins / allow_cors_credentials logic in proxy_server.py diverges in the future, these tests will keep passing while the real code is broken. Consider importing and testing the actual values set at module load time, or at minimum add a structural test that proxy_server.allow_cors_credentials equals "*" not in proxy_server.origins so a drift will fail CI.
| _cors_origins_env = os.getenv("LITELLM_CORS_ORIGINS") | ||
| if _cors_origins_env is None or _cors_origins_env.strip() == "": | ||
| origins = ["*"] | ||
| else: | ||
| origins = [o.strip() for o in _cors_origins_env.split(",") if o.strip()] | ||
|
|
||
| allow_cors_credentials = "*" not in origins |
There was a problem hiding this comment.
allow_credentials
Prior to this PR, allow_credentials was hardcoded True regardless of origin. Now it defaults to False whenever LITELLM_CORS_ORIGINS is unset. Per the team's rule on backwards-incompatible changes, this should be gated behind a user-controlled flag so existing deployments are not silently affected.
In practice, browsers already reject credentialed responses with Access-Control-Allow-Origin: * per the CORS spec, so no browser client would have been working with the old config. However, non-browser HTTP clients or tests that inspect the raw Access-Control-Allow-Credentials header would see a change in behaviour without any migration path.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| error_msg = str(e).lower() | ||
| if "does not exist" not in error_msg and "undefined" not in error_msg: | ||
| raise |
There was a problem hiding this comment.
"undefined" substring match
Checking "undefined" not in error_msg is wide enough to match unrelated error messages — e.g. "column 'undefined_col' referenced in query" or "undefined function". Any such error would be misclassified as a missing-view signal and trigger a CREATE VIEW call rather than surfacing the real problem.
A tighter pattern would reduce false matches:
VIEW_NOT_FOUND_MSGS = ("does not exist", "no such table", "undefined table")
if not any(marker in error_msg for marker in VIEW_NOT_FOUND_MSGS):
raiseThe same pattern is repeated across all eight view blocks, so a single shared tuple keeps it DRY and easier to extend.
…w feedback) Add test_proxy_server_cors_invariant which directly imports and checks the module-level origins and allow_cors_credentials variables in proxy_server.py. This catches any future drift between the mirror helper and the real code.
…env vars reference
|
@yuneng-berri please review and merge based on your analysis of PR impact + approach |
b0a40fd
into
BerriAI:litellm_yj_apr14
|
Merging into a staging branch for full CI to run first. Thanks for the contribution! |
…fety-bugs fix: harden CORS credentials, create_views exception handling, and spend log cleanup loop
5 participants
Type
Bug Fix
Changes
1. CORS security fix (proxy_server.py)
allow_origins=["*"]+allow_credentials=Trueis a browser security misconfiguration — any origin can make credentialed cross-origin requests. Credentials are now automatically disabled when wildcard origins are used. SetLITELLM_CORS_ORIGINS=https://your-ui.comto re-enable credentials with specific origins.2. create_views.py — exception masking
Broad
except Exception: passsilently swallowed real DB errors (auth failures, connection errors) and blindly attemptedCREATE VIEW. Now only genuine "view does not exist"/"undefined" errors proceed to view creation; all others are re-raised.3. spend_log_cleanup.py — infinite loop guard
execute_raw()return type is now validated asintbefore use as a deletion count. An unexpected return type (e.g.None) previously madedeleted_count == 0evaluateFalse, causing an infinite deletion loop.