fix(llm): honor legacy llm_override endpoints behind an opt-in flag - #834
fix(llm): honor legacy llm_override endpoints behind an opt-in flag#834paultranvan wants to merge 5 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds an opt-in legacy ChangesLegacy LLM override handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClientMetadata
participant VLLMClient
participant LLMEndpoint
ClientMetadata->>VLLMClient: Provide metadata.llm_override
VLLMClient->>VLLMClient: Gate and validate base_url
VLLMClient->>LLMEndpoint: Send chat request with resolved model and headers
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/services/inference/test_vllm_client.py (2)
387-397: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a malformed-port case here.
test_allowlist_entry_may_pin_a_portis the natural home forhttps://llm.internal:notaport/v1, which currently escapes_host_keyas aValueErrorrather than anInferenceError(see the comment onopenrag/services/inference/vllm_client.pyLine 135-145).🤖 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 `@tests/unit/services/inference/test_vllm_client.py` around lines 387 - 397, Add a malformed-port assertion to test_allowlist_entry_may_pin_a_port using a base URL such as https://llm.internal:notaport/v1, and verify _resolve_overrides raises InferenceError rather than leaking ValueError. Update the corresponding _host_key handling in the vLLM client so invalid ports are consistently converted to InferenceError.
399-410: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSharpen this test so it pins the no-op contract.
The override here is byte-identical to the configured endpoint, so the assertion passes whether the implementation returns
self._endpointor the client string. Using a differing path (e.g.http://default:8000/internal/admin) would surface the path-forwarding gap flagged onopenrag/services/inference/vllm_client.pyLine 245-249.🤖 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 `@tests/unit/services/inference/test_vllm_client.py` around lines 399 - 410, Update test_own_endpoint_host_needs_no_allowlist_and_keeps_server_key to use a same-host override with a different path, such as http://default:8000/internal/admin, while preserving the existing model and header assertions. Assert that _resolve_overrides forwards the override URL unchanged, pinning the no-op behavior and exposing any path loss.openrag/services/inference/vllm_client.py (1)
123-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAllowlist entries are only lowercased, not normalized.
An operator writing
https://api.openai.comorapi.openai.com/(both plausible given the env var holds URLs elsewhere in the file) silently never matches, and every override is rejected with a message that looks like the host isn't listed. Also worth documenting:llm.internal:443won't matchhttps://llm.internal/v1because the default port is implicit.Stripping a scheme prefix and trailing slashes at parse time, or logging a warning for entries containing
/or://, would make misconfiguration self-evident.🤖 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/services/inference/vllm_client.py` around lines 123 - 132, Update _allowed_override_hosts to normalize allowlist entries by removing an optional URL scheme and trailing slashes before lowercasing and storing them, so values such as https://api.openai.com and api.openai.com/ match host comparisons. Preserve explicit ports as entered and document or warn that entries like llm.internal:443 do not match URLs using an implicit default port.
🤖 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/services/inference/vllm_client.py`:
- Around line 245-249: Update the self-endpoint branch in the override
validation logic to return the configured self._endpoint rather than the
client-controlled candidate, while retaining the existing no-op detection based
on matching host and port. Preserve the existing None return and allow the
byte-identical endpoint test to continue passing.
- Around line 135-145: Normalize malformed candidate URL parsing failures into
the documented non-retryable 400 LLM_OVERRIDE_REJECTED response. Update the
candidate URL validation path around _host_key so it catches ValueError from
urlsplit or parts.port and routes the candidate through the existing rejection
handling, while avoiding conversion of self._endpoint configuration errors into
client-facing 400 responses.
---
Nitpick comments:
In `@openrag/services/inference/vllm_client.py`:
- Around line 123-132: Update _allowed_override_hosts to normalize allowlist
entries by removing an optional URL scheme and trailing slashes before
lowercasing and storing them, so values such as https://api.openai.com and
api.openai.com/ match host comparisons. Preserve explicit ports as entered and
document or warn that entries like llm.internal:443 do not match URLs using an
implicit default port.
In `@tests/unit/services/inference/test_vllm_client.py`:
- Around line 387-397: Add a malformed-port assertion to
test_allowlist_entry_may_pin_a_port using a base URL such as
https://llm.internal:notaport/v1, and verify _resolve_overrides raises
InferenceError rather than leaking ValueError. Update the corresponding
_host_key handling in the vLLM client so invalid ports are consistently
converted to InferenceError.
- Around line 399-410: Update
test_own_endpoint_host_needs_no_allowlist_and_keeps_server_key to use a
same-host override with a different path, such as
http://default:8000/internal/admin, while preserving the existing model and
header assertions. Assert that _resolve_overrides forwards the override URL
unchanged, pinning the no-op behavior and exposing any path loss.
🪄 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 Plus
Run ID: f27c2af0-1206-44a9-852d-0096fee243b3
📒 Files selected for processing (3)
infra/compose/.env.exampleopenrag/services/inference/vllm_client.pytests/unit/services/inference/test_vllm_client.py
| def _host_key(url: str) -> tuple[str, str, str]: | ||
| """Split *url* into ``(scheme, host, host:port)``, lowercased. | ||
|
|
||
| ``urlsplit().hostname`` is what makes the allowlist safe against userinfo | ||
| smuggling: for ``https://api.openai.com@evil.tld/v1`` it returns ``evil.tld``, | ||
| the host the request would actually reach, not the reassuring prefix. | ||
| """ | ||
| parts = urlsplit(url) | ||
| host = (parts.hostname or "").lower() | ||
| hostport = f"{host}:{parts.port}" if parts.port else host | ||
| return parts.scheme.lower(), host, hostport |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
urlsplit(...).port can raise ValueError, escaping the 4xx contract.
parts.port raises ValueError for a non-numeric or out-of-range port (https://api.openai.com:notaport/v1, https://api.openai.com:99999/v1), and urlsplit itself raises for malformed IPv6 literals (http://[::1/v1). Since base_url is client-supplied via metadata.llm_override, this propagates out of chat/generate as an unhandled ValueError (500, unstructured) instead of the documented non-retryable 400 LLM_OVERRIDE_REJECTED.
🛡️ Proposed fix: normalize parse failures into the rejection path
- parts = urlsplit(url)
- host = (parts.hostname or "").lower()
- hostport = f"{host}:{parts.port}" if parts.port else host
- return parts.scheme.lower(), host, hostport
+ try:
+ parts = urlsplit(url)
+ port = parts.port
+ except ValueError as exc:
+ raise InferenceError(
+ f"llm_override.base_url {url!r} is not a valid URL",
+ code="LLM_OVERRIDE_REJECTED",
+ status_code=400,
+ ) from exc
+ host = (parts.hostname or "").lower()
+ hostport = f"{host}:{port}" if port else host
+ return parts.scheme.lower(), host, hostportNote _host_key is also called on self._endpoint (Line 248); a server misconfiguration would then surface as a 400, so you may prefer to wrap only the candidate call site instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _host_key(url: str) -> tuple[str, str, str]: | |
| """Split *url* into ``(scheme, host, host:port)``, lowercased. | |
| ``urlsplit().hostname`` is what makes the allowlist safe against userinfo | |
| smuggling: for ``https://api.openai.com@evil.tld/v1`` it returns ``evil.tld``, | |
| the host the request would actually reach, not the reassuring prefix. | |
| """ | |
| parts = urlsplit(url) | |
| host = (parts.hostname or "").lower() | |
| hostport = f"{host}:{parts.port}" if parts.port else host | |
| return parts.scheme.lower(), host, hostport | |
| def _host_key(url: str) -> tuple[str, str, str]: | |
| """Split *url* into ``(scheme, host, host:port)``, lowercased. | |
| ``urlsplit().hostname`` is what makes the allowlist safe against userinfo | |
| smuggling: for ``https://api.openai.com@evil.tld/v1`` it returns ``evil.tld``, | |
| the host the request would actually reach, not the reassuring prefix. | |
| """ | |
| try: | |
| parts = urlsplit(url) | |
| port = parts.port | |
| except ValueError as exc: | |
| raise InferenceError( | |
| f"llm_override.base_url {url!r} is not a valid URL", | |
| code="LLM_OVERRIDE_REJECTED", | |
| status_code=400, | |
| ) from exc | |
| host = (parts.hostname or "").lower() | |
| hostport = f"{host}:{port}" if port else host | |
| return parts.scheme.lower(), host, hostport |
🤖 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/services/inference/vllm_client.py` around lines 135 - 145, Normalize
malformed candidate URL parsing failures into the documented non-retryable 400
LLM_OVERRIDE_REJECTED response. Update the candidate URL validation path around
_host_key so it catches ValueError from urlsplit or parts.port and routes the
candidate through the existing rejection handling, while avoiding conversion of
self._endpoint configuration errors into client-facing 400 responses.
| # Pointing the override at the endpoint the server would have used anyway | ||
| # is a no-op, so it needs no allowlist entry and keeps the server's key — | ||
| # no third party is involved and nothing is disclosed. | ||
| if (host, hostport) == _host_key(self._endpoint)[1:]: | ||
| return candidate, None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Self-endpoint "no-op" branch still forwards a client-controlled path with the server's key.
The host/port match makes this a no-op only at the host level — the path is still attacker-controlled. base_url = "http://default:8000/internal/admin" passes this branch, needs no allowlist entry, and the request goes out with the server's Authorization header to an arbitrary path on the internal LLM host. If it's genuinely a no-op, return the configured endpoint.
🔒️ Proposed fix
- if (host, hostport) == _host_key(self._endpoint)[1:]:
- return candidate, None
+ if (host, hostport) == _host_key(self._endpoint)[1:]:
+ return self._endpoint, Nonetests/unit/services/inference/test_vllm_client.py:399-410 still passes, since the override there is byte-identical to the configured endpoint.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Pointing the override at the endpoint the server would have used anyway | |
| # is a no-op, so it needs no allowlist entry and keeps the server's key — | |
| # no third party is involved and nothing is disclosed. | |
| if (host, hostport) == _host_key(self._endpoint)[1:]: | |
| return candidate, None | |
| # Pointing the override at the endpoint the server would have used anyway | |
| # is a no-op, so it needs no allowlist entry and keeps the server's key — | |
| # no third party is involved and nothing is disclosed. | |
| if (host, hostport) == _host_key(self._endpoint)[1:]: | |
| return self._endpoint, None |
🤖 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/services/inference/vllm_client.py` around lines 245 - 249, Update the
self-endpoint branch in the override validation logic to return the configured
self._endpoint rather than the client-controlled candidate, while retaining the
existing no-op detection based on matching host and port. Preserve the existing
None return and allow the byte-identical endpoint test to continue passing.
paultranvan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/services/inference/vllm_client.py (1)
237-245: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMalformed override URLs are neither handled nor tested.
urlsplitraisesValueErroron inputs likehttp://[::1/v1, and no test covers that path, so the gap between the documented 400LLM_OVERRIDE_REJECTEDcontract and the actual 500 goes unnoticed.
openrag/services/inference/vllm_client.py#L237-L245: wrap theurlsplit(candidate)call intry/except ValueErrorand re-raise asInferenceError(code="LLM_OVERRIDE_REJECTED", status_code=400).tests/unit/services/inference/test_vllm_client.py#L390-L400: add a case alongside thefile://test assertingbase_url="http://[::1/v1"also yields a non-retryable 400.🤖 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/services/inference/vllm_client.py` around lines 237 - 245, Malformed override URLs currently escape the documented rejection path. In openrag/services/inference/vllm_client.py lines 237-245, update the base_url parsing around urlsplit in the LLM override validation to catch ValueError and re-raise InferenceError with code LLM_OVERRIDE_REJECTED and status_code 400; in tests/unit/services/inference/test_vllm_client.py lines 390-400, add a case alongside the file:// test for base_url="http://[::1/v1" that asserts a non-retryable 400 rejection.
🧹 Nitpick comments (1)
tests/unit/services/inference/test_vllm_client.py (1)
259-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
TestVLLMClientOverrideshermetic w.r.t. the new env flag.
test_client_base_url_and_api_key_override_ignoredasserts the disabled behavior but never clearsLLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT, so it fails if a developer or CI image has the variable exported.TestLegacyEndpointOverridealready delenvs it.♻️ Proposed fix
- def _make_client(self): + def _make_client(self, monkeypatch): + monkeypatch.delenv("LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT", raising=False) return VLLMClient( endpoint="http://default:8000/v1", model_name="default-model", api_key="default-key", )Each test in the class then takes
monkeypatchand passes it through, or use anautousefixture on the class to avoid touching every signature.🤖 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 `@tests/unit/services/inference/test_vllm_client.py` around lines 259 - 264, Make TestVLLMClientOverrides hermetic by ensuring LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT is cleared before each test, preferably via an autouse fixture on the class or by applying monkeypatch in every test. Preserve the existing disabled-override assertions and avoid changing _make_client’s default client configuration.
🤖 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/services/inference/vllm_client.py`:
- Around line 247-250: Sanitize the legacy override endpoint before the debug
log in the llm_override handling flow, using the existing urlsplit result rather
than reparsing candidate. Keep parsing and parts.port access inside the existing
guarded error handling, and log a form that excludes userinfo and credentials
while preserving the safe endpoint details.
---
Outside diff comments:
In `@openrag/services/inference/vllm_client.py`:
- Around line 237-245: Malformed override URLs currently escape the documented
rejection path. In openrag/services/inference/vllm_client.py lines 237-245,
update the base_url parsing around urlsplit in the LLM override validation to
catch ValueError and re-raise InferenceError with code LLM_OVERRIDE_REJECTED and
status_code 400; in tests/unit/services/inference/test_vllm_client.py lines
390-400, add a case alongside the file:// test for base_url="http://[::1/v1"
that asserts a non-retryable 400 rejection.
---
Nitpick comments:
In `@tests/unit/services/inference/test_vllm_client.py`:
- Around line 259-264: Make TestVLLMClientOverrides hermetic by ensuring
LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT is cleared before each test, preferably via
an autouse fixture on the class or by applying monkeypatch in every test.
Preserve the existing disabled-override assertions and avoid changing
_make_client’s default client configuration.
🪄 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 Plus
Run ID: df051a1b-8c20-4f43-9e9d-ed9b9f4742fb
📒 Files selected for processing (3)
infra/compose/.env.exampleopenrag/services/inference/vllm_client.pytests/unit/services/inference/test_vllm_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- infra/compose/.env.example
6a4c6ba to
b489bb2
Compare
1 participant
Problem
Before the services refactor,
metadata.llm_overridehonoredbase_urlandapi_keyalongsidemodel(components/llm.py, pre-refactor):VLLMClient._resolve_overridesdeliberately dropped that — honoring aclient-supplied endpoint is SSRF, and falling back to the server's key would ship
it to the attacker's host.
The removal is right, but it fails silently and misleadingly.
modelis stillapplied while
base_urlis dropped, so a legacy client's request goes to theserver's endpoint carrying a third party's model name. What the operator sees
is an unrelated-looking 400 from the wrong provider:
Change
Opt-in
LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT. Unset — the default — changesnothing:
base_url/api_keyin an override stay ignored exactly as today, so anupgrade never turns an untouched deployment into an SSRF surface.
When on, the target URL is unrestricted. This is a deliberate trade, not an
oversight: deployments migrating off pre-refactor clients need arbitrary endpoints
and cannot enumerate them in advance. With the flag on, any caller who can reach
/v1/chat/completionscan make the server POST to any URL it can route to —cloud metadata, internal admin services.
.env.examplesays so in those words,and
test_no_host_restriction_when_enabledasserts it against three such URLs sothe exposure is documented in the test suite rather than discovered later.
Two properties survive the trade:
api_keyis used, or noAuthorizationat all — never a fallback toself._api_key.opaque transport error into a named 4xx, which also keeps
with_retry(429/502/503/504 only) from re-attempting.
Authorization moved to per-request headers
Required for the first guarantee, and a latent leak on its own.
Authorizationwas set on the shared
httpx.AsyncClient; httpx merges client-level headers intoevery request and offers no way to drop one, so the server's key would ride along
to an overridden endpoint whenever the override carried no key of its own. It is
now built per request in
_resolve_overrides, which consequently returns aheaders dict rather than
Optional.VLLMEmbedder/VLLMVisionare untouched —they take no overrides.
Tests
9 unit tests in
TestLegacyEndpointOverride, plus 4 assertion updates inTestVLLMClientOverridesfor the headers-dict return.Covered: arbitrary endpoint honored with the client key; no host restriction
(parametrized over metadata IP / localhost / userinfo-prefixed host); keyless
override yields no
Authorization, asserted both at the resolver and on the wire;non-http rejected and asserted non-retryable via
_is_retryable; disabled bydefault; caller metadata not mutated; end-to-end
chatlanding on the overriddenhost with the client's key; and the server key still sent when there is no
override (the regression the header move could have caused).
Full unit suite: no new failures (2204 → 2215 passing).
Summary by CodeRabbit