fix(router): validate LiteLLM master key and enforce client auth in responses_api (#407) - #467
Merged
sheepdestroyer merged 1 commit intoAug 13, 2026
Conversation
Contributor
Reviewer's GuideThis PR hardens LiteLLM integration and router security by validating the LiteLLM master key, enforcing client Bearer auth on the /v1/responses endpoint, improving LiteLLM/OpenRouter model registration, and tightening tracing and proxy behavior, with tests updated accordingly. Sequence diagram for secured responses_api flowsequenceDiagram
actor Client
participant Router as responses_api
participant LiteLLM as LiteLLM_admin
Client->>Router: HTTP POST /v1/responses
Router->>Router: read Authorization header
Router-->>Client: HTTP 401 (Missing or invalid Authorization) [if header invalid]
Router->>Router: _validate_litellm_master_key()
Router-->>Client: HTTP 500 (invalid master key) [on validation failure]
Router->>LiteLLM: forward request with Bearer LITELLM_MASTER_KEY
LiteLLM-->>Router: response
Router-->>Client: proxied response
Sequence diagram for OpenRouter model DB registration at lifespansequenceDiagram
participant App as lifespan
participant Registrar as _register_openrouter_models_in_db
participant DB as _purge_stale_deployments
participant LiteLLM as LiteLLM_admin
App->>Registrar: _register_openrouter_models_in_db(litellm_master_key)
Registrar->>Registrar: load config.yaml for model_list
Registrar->>DB: _purge_stale_deployments(DATABASE_URL, "openrouter-%")
DB-->>Registrar: purge complete
loop each openrouter model
Registrar->>LiteLLM: client.post(/model/new, headers=Authorization Bearer master_key)
LiteLLM-->>Registrar: HTTP 200/201 or error
end
Registrar-->>App: registration summary logged
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Warning Review limit reached
Next review available in:116 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe router prioritizes explicit Llama endpoints, validates LiteLLM master keys, enforces Responses API client authentication, registers OpenRouter models during startup, propagates Langfuse identifiers, and updates related tests and test environment defaults. ChangesRouter hardening and startup synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesAPI
participant LiteLLM
Client->>ResponsesAPI: send Bearer-authenticated request
ResponsesAPI->>ResponsesAPI: validate client header and master key
ResponsesAPI->>LiteLLM: proxy with validated credential
LiteLLM-->>ResponsesAPI: return upstream response
ResponsesAPI-->>Client: return response
sequenceDiagram
participant ApplicationLifespan
participant LiteLLMConfig
participant DeploymentDatabase
participant LiteLLM
ApplicationLifespan->>LiteLLMConfig: load OpenRouter definitions
LiteLLMConfig-->>ApplicationLifespan: return configured or fallback models
ApplicationLifespan->>DeploymentDatabase: purge stale deployments
ApplicationLifespan->>LiteLLM: register models
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
_validate_litellm_master_keyhelper logs the raw master key value on error, which risks leaking secrets into logs; consider redacting or omitting the actual key string in the log message. - Client auth enforcement was added to
responses_apibut not tochat_completionsor other endpoints that use the router; if the intent is consistent client authentication, consider centralizing this check or applying it uniformly. - Using
HTTPExceptiondirectly in_validate_litellm_master_keycouples this low-level helper to the HTTP layer and makes it harder to reuse; consider raising a custom exception and translating toHTTPExceptionat the endpoint boundary instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- The `_validate_litellm_master_key` helper logs the raw master key value on error, which risks leaking secrets into logs; consider redacting or omitting the actual key string in the log message.
- Client auth enforcement was added to `responses_api` but not to `chat_completions` or other endpoints that use the router; if the intent is consistent client authentication, consider centralizing this check or applying it uniformly.
- Using `HTTPException` directly in `_validate_litellm_master_key` couples this low-level helper to the HTTP layer and makes it harder to reuse; consider raising a custom exception and translating to `HTTPException` at the endpoint boundary instead.
## Individual Comments### Comment 1
<locationpath="router/main.py"line_range="806-808" />
<code_context>
+ Raises:
+ HTTPException(500): If master key is missing, empty, or placeholder string.+ """
+ key = (os.getenv("LITELLM_MASTER_KEY") or "").strip()
+ if not key or key in _INVALID_MASTER_KEYS or "PLACEHOLDER" in key.upper():
+ logger.error(f"Invalid or missing LITELLM_MASTER_KEY: '{key}'")+ raise HTTPException(+ status_code=500,
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid logging the raw master key value to prevent secret exposure in logs.
The current error log prints the full `LITELLM_MASTER_KEY`, which can expose the secret in log storage and monitoring systems. Instead, log only that the key is invalid/missing, and if needed include non-sensitive metadata (e.g., length or a partially redacted prefix) rather than the full value.
</issue_to_address>
### Comment 2
<locationpath="router/main.py"line_range="2307-2311" />
<code_context>
(such as gpt-4o-mini, local-qwen-3.6-hass) and tool/streaming executions.
"""
+# Enforce client authentication+ auth_header = request.headers.get("Authorization") or request.headers.get("authorization")
+ if not auth_header or not auth_header.startswith("Bearer "):
+ raise HTTPException(status_code=401, detail="Missing or invalid Authorization header")+ client_token = auth_header[7:].strip()
+ if not client_token:
+ raise HTTPException(status_code=401, detail="Missing or invalid Authorization header")+
</code_context>
<issue_to_address>
**issue (bug_risk):** Authorization scheme checking is case-sensitive and may reject otherwise valid headers.
`auth_header.startswith("Bearer ")` will fail for `bearer` or other case variants, even though RFC 6750 specifies a case-insensitive scheme. Consider parsing and normalizing the scheme (e.g., split on space and check `scheme.lower() == "bearer"`) to avoid returning 401 for otherwise valid headers.
</issue_to_address>
### Comment 3
<locationpath="router/tests/test_register_openrouter_models_in_db.py"line_range="18-21" />
<code_context>
+ yield+++@pytest.mark.asyncio
+async def test_register_openrouter_models_no_master_key(mock_env, caplog):
+ await _register_openrouter_models_in_db(None)
+ assert "No LiteLLM master key provided" in caplog.text
++
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for behavior when `DATABASE_URL` is missing to validate the non-purge branch
Current tests exercise the purge path (with `DATABASE_URL` set) and registration from static fallback and config, but not the branch where `DATABASE_URL` is absent. Please add a test that runs `_register_openrouter_models_in_db` with no `DATABASE_URL` and asserts that `_purge_stale_deployments` is not called and the warning is logged, so this control flow is covered.
Suggested implementation:
```python@pytest.fixturedefmock_env():
with patch.dict(os.environ, {
"DATABASE_URL": "postgresql://test:test@localhost:5432/test",
"ROUTER_API_KEY": "test_api_key"
}, clear=False):
yield@pytest.mark.asyncioasyncdeftest_register_openrouter_models_no_database_url(caplog, monkeypatch):
# Ensure DATABASE_URL is not present to exercise the non-purge branch
monkeypatch.delenv("DATABASE_URL", raising=False)
# Patch _purge_stale_deployments to verify it is not calledwith patch("router.main._purge_stale_deployments") as mock_purge:
await _register_openrouter_models_in_db(None)
mock_purge.assert_not_called()
# Assert that the expected warning is logged when DATABASE_URL is missingassert"DATABASE_URL not set; skipping stale deployment purge"in caplog.text
```1. Ensure the warning string in the assertion matches the actual log message emitted in `_register_openrouter_models_in_db` when `DATABASE_URL` is absent; update the string in the test if the wording differs.
2. If `_register_openrouter_models_in_db` logs via a logger with a specific name or level, you may want to set `caplog.set_level(...)` appropriately at the start of the test to guarantee capture of the warning.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+2307
to
+2311
| auth_header = request.headers.get("Authorization") or request.headers.get("authorization") | ||
| if not auth_header or not auth_header.startswith("Bearer "): | ||
| raise HTTPException(status_code=401, detail="Missing or invalid Authorization header") | ||
| client_token = auth_header[7:].strip() | ||
| if not client_token: |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Authorization scheme checking is case-sensitive and may reject otherwise valid headers.
auth_header.startswith("Bearer ") will fail for bearer or other case variants, even though RFC 6750 specifies a case-insensitive scheme. Consider parsing and normalizing the scheme (e.g., split on space and check scheme.lower() == "bearer") to avoid returning 401 for otherwise valid headers.
Comment on lines
+18
to
+21
| @pytest.mark.asyncio | ||
| async def test_register_openrouter_models_no_master_key(mock_env, caplog): | ||
| await _register_openrouter_models_in_db(None) | ||
| assert "No LiteLLM master key provided" in caplog.text |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for behavior when DATABASE_URL is missing to validate the non-purge branch
Current tests exercise the purge path (with DATABASE_URL set) and registration from static fallback and config, but not the branch where DATABASE_URL is absent. Please add a test that runs _register_openrouter_models_in_db with no DATABASE_URL and asserts that _purge_stale_deployments is not called and the warning is logged, so this control flow is covered.
Suggested implementation:
@pytest.fixturedefmock_env():
withpatch.dict(os.environ, {
"DATABASE_URL": "postgresql://test:test@localhost:5432/test",
"ROUTER_API_KEY": "test_api_key"
}, clear=False):
yield@pytest.mark.asyncioasyncdeftest_register_openrouter_models_no_database_url(caplog, monkeypatch):
# Ensure DATABASE_URL is not present to exercise the non-purge branchmonkeypatch.delenv("DATABASE_URL", raising=False)
# Patch _purge_stale_deployments to verify it is not calledwithpatch("router.main._purge_stale_deployments") asmock_purge:
await_register_openrouter_models_in_db(None)
mock_purge.assert_not_called()
# Assert that the expected warning is logged when DATABASE_URL is missingassert"DATABASE_URL not set; skipping stale deployment purge"incaplog.text- Ensure the warning string in the assertion matches the actual log message emitted in
_register_openrouter_models_in_dbwhenDATABASE_URLis absent; update the string in the test if the wording differs. - If
_register_openrouter_models_in_dblogs via a logger with a specific name or level, you may want to setcaplog.set_level(...)appropriately at the start of the test to guarantee capture of the warning.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/main.py`:
- Around line 806-812: Update the validation error logging around the
LITELLM_MASTER_KEY check to use a generic message without interpolating or
exposing key. Preserve the existing invalid-key condition and HTTPException
behavior while removing the secret value from logger.error.
- Around line 2306-2312: Update the client authentication logic around
auth_header and client_token to compare the extracted Bearer token with the
configured client credential, rejecting mismatches with HTTP 401 before any
LiteLLM/upstream processing. Add coverage verifying an invalid token returns 401
and does not trigger the upstream request.
- Around line 998-1005: Align the registration condition in the model-loading
flow with the purge query’s OpenRouter selection rule: either register only
names matching the openrouter- prefix or update purge to also match targets
beginning with openrouter/. Ensure non-prefixed aliases such as gpt-4o-mini-tts
are handled consistently across restarts, and add a regression test covering
that alias.
In `@router/tests/test_register_openrouter_models_in_db.py`:
- Around line 64-70: The test fixture’s OpenRouter alias gpt-4o-mini-tts is not
covered by the openrouter- name prefix, so stale rows can persist. Update
_register_openrouter_models_in_db to identify managed OpenRouter rows by their
persisted provider target or complete managed model set, purge removed aliases
including gpt-4o-mini-tts, and add a test verifying this alias is removed when
absent from configuration.
In `@router/tests/test_sync_adaptive_router_roster.py`:
- Line 71: Update the purge verification in the sync_adaptive_router_roster test
to assert exactly one awaited call with the expected agent-% argument, replacing
the broad mock_purge.assert_called() check while preserving the existing
contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b20f0e3a-ff23-4e18-a590-f9b51a742346
📒 Files selected for processing (7)
router/main.pyrouter/tests/test_lifespan.pyrouter/tests/test_register_openrouter_models_in_db.pyrouter/tests/test_responses_api.pyrouter/tests/test_routing_behavior.pyrouter/tests/test_sync_adaptive_router_roster.pytests/conftest.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+64
to
+70
| { | ||
| "model_name": "gpt-4o-mini-tts", | ||
| "litellm_params": {"model": "openrouter/openai/tts-1"} | ||
| }, | ||
| { | ||
| "model_name": "local-qwen-3.6", | ||
| "litellm_params": {"model": "openai/local-qwen-3.6"} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Purge OpenRouter aliases that do not use the openrouter- prefix.
Line 66 configures an OpenRouter deployment with the name gpt-4o-mini-tts. _register_openrouter_models_in_db only purges names matching openrouter-%. If this alias is later removed from configuration, it remains in the LiteLLM database and can stay selectable.
Identify stale OpenRouter rows from their persisted provider target or from the complete managed model set. Add a removal test for this alias.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/tests/test_register_openrouter_models_in_db.py` around lines 64 - 70,
The test fixture’s OpenRouter alias gpt-4o-mini-tts is not covered by the
openrouter- name prefix, so stale rows can persist. Update
_register_openrouter_models_in_db to identify managed OpenRouter rows by their
persisted provider target or complete managed model set, purge removed aliases
including gpt-4o-mini-tts, and add a test verifying this alias is removed when
absent from configuration.
Uh oh!
There was an error while loading. Please reload this page.
…g in responses_api, and purge openrouter aliases
sheepdestroyerforce-pushed
the
fix/responses-api-auth-and-masterkey-validation
branch
from
August 13, 2026 10:39
e800aed to
45ffe6cCompareUh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes#407
Summary by Sourcery
Enforce secure LiteLLM integration and client authentication for routing and responses APIs while registering OpenRouter models into the LiteLLM DB at startup.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Security
Bug Fixes