refactor(proxy): ban raw datetime.fromisoformat via parse_utc_datetime helper - #34128
Open
ryan-crabbe-berri wants to merge 3 commits into
Open
refactor(proxy): ban raw datetime.fromisoformat via parse_utc_datetime helper#34128ryan-crabbe-berri wants to merge 3 commits into
ryan-crabbe-berri wants to merge 3 commits into
Conversation
…e helper Extract parse_utc_datetime into litellm_core_utils/datetime_utils.py as the single ISO-8601 parse entrypoint for the proxy: it accepts str or datetime, handles the Z suffix on Python 3.10, and assumes naive values are UTC so results always compare safely against datetime.now(timezone.utc). Convert all 27 raw fromisoformat sites under litellm/proxy/ to the helper, collapsing the hand-rolled tzinfo guards they each carried. This also fixes a real bug in get_mcp_oauth_user_credential_status: a tz-naive expires_at raised TypeError against the aware now(), the bare except swallowed it, and the credential never read as expired. Two semgrep rules enforce the pattern in CI: raw fromisoformat is banned under litellm/proxy/, and a repo-wide taint rule flags comparing or subtracting an unnormalized fromisoformat result.
Contributor
Greptile SummaryThis PR centralizes UTC-aware datetime parsing across the proxy. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/datetime_utils.py | Adds the shared parser, UTC normalization, and predictable errors for unsupported input types |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Normalizes credential expiry values and prevents malformed timestamp types from failing response validation |
| .semgrep/rules/python/reliability/naive-datetime.yml | Adds checks that ban raw proxy datetime parsing and flag comparisons using unnormalized parsed timestamps |
| tests/test_litellm/litellm_core_utils/test_datetime_utils.py | Covers naive, UTC, offset, datetime, invalid-string, and unsupported-type inputs |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Adds focused coverage for naive expired credentials and malformed non-string expiry values |
Reviews (2): Last reviewed commit: "fix(datetime_utils): raise TypeError for..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Merged
5 tasks
…itellm_ban_unnormalized_datetime_compare
Greptile flagged that narrowing get_mcp_oauth_user_credential_status from a bare except to except ValueError left a truthy non-string expires_at (possible in raw DB JSON) raising AttributeError through the endpoint as a 500. The same gap applied to every refactored site that catches (ValueError, TypeError): datetime.fromisoformat raised TypeError for non-strings, which those handlers were written against, but the helper's passthrough branch leaked AttributeError instead. parse_utc_datetime now validates its input and raises TypeError for anything that is not str or datetime, restoring the stdlib error contract at every call site. The MCP status endpoint catches it and also omits non-string expires_at/connected_at values from the response instead of failing response validation.
Contributor
Author
|
@greptileai re review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issues
Follow-up to #33840, which fixed one aware-vs-naive datetime crash in
_get_temp_budget_increase. This PR eliminates the classLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
All runs against a live proxy on
localhost:4000at commit 509ef61 hitting the real Groq API (groq/openai/gpt-oss-120bregistered via/model/new)The documented tz-aware format from #33840 keeps working through the new helper.
/key/updatewith"temp_budget_expiry": "2026-08-01T00:00:00Z"stores aware metadata and the key serves:The legacy naive format also still serves:
Key expiry enforcement (the three collapsed guard sites in
user_api_key_auth.py) still rejects expired keys, now with unambiguous aware timestamps in the error:The MCP credential-status bug fix is proven by a red-to-green regression test (reverting the one-line fix makes
test_get_mcp_oauth_user_credential_status_naive_past_expiry_is_expiredfail); a live repro needs a connected OAuth MCP server, which the unit test simulates at the exact seam that swallowed the TypeErrorType
🐛 Bug Fix
🧹 Refactoring
Changes
datetime.fromisoformaton strings from API input, DB metadata, or serialized state returns aware datetimes when the string carriesZ/+00:00and naive ones otherwise. Comparing the two raisesTypeError, and the proxy had 27 call sites each hand-rolling (or forgetting) the same tzinfo guard. #33840 fixed one such crash; grepping for the pattern found another live one plus 15 sites that only stayed safe by carrying duplicated guard codeThis PR adds
parse_utc_datetimeinlitellm/litellm_core_utils/datetime_utils.pyas the single parse entrypoint: acceptsstr | datetime, handles theZsuffix on Python 3.10 (stdlib only accepts it from 3.11), and assumes naive values are UTC, so results always compare safely againstdatetime.now(timezone.utc). All 27fromisoformatsites underlitellm/proxy/now go through it, and the per-site guards are deletedIt also fixes a real bug found by the sweep:
get_mcp_oauth_user_credential_statuscompared an unnormalized parse against aware now inside a bareexcept Exception, so a tz-naiveexpires_atraisedTypeError, the except swallowed it, and the credential never reported as expired. The except is narrowed toValueErrorand the parse normalizedTwo semgrep rules in
.semgrep/rules/python/reliability/naive-datetime.ymlmake the pattern unrepresentable: rawdatetime.fromisoformatis banned underlitellm/proxy/(ERROR), and a repo-wide taint rule flags comparing or subtracting an unnormalizedfromisoformatresult. Both run in the existing Semgrep CI job and are at zero findings on this branchBehavior notes: the model-cost-map and anthropic-beta-headers reload timers in
proxy_server.pymoved from naiveutcnow()to aware UTC, solast_run/next_runin their status endpoints now carry a+00:00suffix (same instants). Inusage_endpoints.py, astart_datewith a time but no offset now filters as UTC instead of naive. Partition-drop cutoffs inspend_logs_partition_manager.pycompare aware-vs-aware; production callers already passed aware UTCTests:
tests/test_litellm/litellm_core_utils/test_datetime_utils.pycovers the helper contract (naive-as-UTC,Zsuffix, offset preservation, datetime passthrough, invalid input). The MCP regression test proves the swallowed-TypeError bug cannot return. Existing partition-manager tests updated to the aware convention. 480 tests across the touched suites passFollow-up from review:
parse_utc_datetimenow raisesTypeErrorfor input that is neitherstrnordatetime, mirroringdatetime.fromisoformat's own contract. Without this, a truthy non-stringexpires_atin stored credential JSON leakedAttributeErrorpast theexcept (ValueError, TypeError)handlers the refactored sites already carry, turning the MCP credential-status endpoint into a 500. That endpoint also omits non-stringexpires_at/connected_atfrom its response instead of failing response validation, with a test proving a malformed stored expiry degrades gracefullyFinal Attestation