fix(security): ignore client-supplied LLM endpoint/credentials in llm_override - #467
Conversation
…_override metadata.llm_override allowed any authenticated user to set base_url and api_key. The server then issued requests to that URL (SSRF to internal services / cloud metadata) and forwarded the server's own API key in the Authorization header to the attacker-controlled host (credential theft). Only the 'model' field may now be overridden; the endpoint and credentials always come from server configuration.
📝 WalkthroughWalkthrough
ChangesSSRF/Key-Leak Fix: Restrict llm_override to model only
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openrag/models/openai.py`:
- Line 35: The description string in the openray/models/openai.py file correctly
documents that llm_override only supports model-only overrides, but the external
API documentation in docs/content/docs/documentation/API.mdx still references
the old contract allowing base_url and api_key overrides. Update the API
documentation to remove all mentions of base_url and api_key override
capabilities for the llm_override parameter and align it with the current
model-only restriction to maintain consistency between the code contract and
public-facing documentation.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b14c43f4-c413-4e80-9e62-6ee02b4ee363
📒 Files selected for processing (3)
openrag/components/llm.pyopenrag/components/test_llm.pyopenrag/models/openai.py
| "llm_override": None, | ||
| }, | ||
| description="Extra custom parameters. Supports 'llm_override' object with optional 'base_url', 'api_key', and 'model' to override the downstream LLM endpoint.", | ||
| description="Extra custom parameters. Supports 'llm_override' object with an optional 'model' to override the downstream model name. The LLM endpoint and credentials are fixed by server configuration and cannot be overridden by the client.", |
There was a problem hiding this comment.
Update external API docs to match the model-only override contract.
Line 35 correctly narrows llm_override to model-only, but the public docs snippet in docs/content/docs/documentation/API.mdx still advertises base_url/api_key overrides. Please align that doc to prevent client-side contract drift after this breaking change.
🤖 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 `@openrag/models/openai.py` at line 35, The description string in the
openray/models/openai.py file correctly documents that llm_override only
supports model-only overrides, but the external API documentation in
docs/content/docs/documentation/API.mdx still references the old contract
allowing base_url and api_key overrides. Update the API documentation to remove
all mentions of base_url and api_key override capabilities for the llm_override
parameter and align it with the current model-only restriction to maintain
consistency between the code contract and public-facing documentation.
Issue (Critical)
metadata.llm_overridelet any authenticated user overridebase_urlandapi_keyof the downstream LLM call (components/llm.py):base_urlathttp://169.254.169.254/...,http://localhost:8265(Ray), internal services, etc. — the server makes the request from inside the trust boundary.base_urland omitapi_key→ the server's real LLM API key is sent in theAuthorizationheader to the attacker's host.No allowlist, scheme/host check, or role gate existed.
Fix
Only
modelmay be overridden by the client;base_urlandapi_keyalways come from server configuration. Updated the field description and tests (added a regression test asserting a clientbase_url/api_keyis ignored).Breaking change
Clients can no longer redirect the LLM endpoint per-request. If a legitimate per-request endpoint feature is needed, it should be re-added behind admin auth + a server-side host allowlist.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests