From 4e775f9c5e2afa86db786468661c2c78282b3a14 Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 16:09:48 +0200 Subject: [PATCH 1/5] fix(llm): restore the legacy llm_override endpoint behind an env flag --- infra/compose/.env.example | 12 ++ openrag/core/config/endpoints.py | 24 +++ openrag/services/inference/vllm_client.py | 74 +++++-- .../services/inference/test_vllm_client.py | 187 +++++++++++++++++- 4 files changed, 281 insertions(+), 16 deletions(-) diff --git a/infra/compose/.env.example b/infra/compose/.env.example index 28461de32..9b8c10fd3 100644 --- a/infra/compose/.env.example +++ b/infra/compose/.env.example @@ -14,6 +14,18 @@ BASE_URL= API_KEY= MODEL= LLM_SEMAPHORE=10 +# Legacy escape hatch for pre-refactor clients. Clients may always override the +# model name via metadata.llm_override; set this to also honor base_url/api_key. +# +# DANGER: there is no restriction on the target URL. With this on, any caller who +# can reach /v1/chat/completions can make the server POST to any URL it can route +# to — cloud metadata endpoints, internal admin services. That is server-side +# request forgery. Enable only where every API caller is already trusted with it. +# The server's own API key is never forwarded to an overridden endpoint. +# +# Prefer registering a named endpoint under /model-endpoints and binding it to +# the partition (chat_llm), which needs no such trade-off. +# LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT=true # ── VLM (vision model, used for image understanding) ──────────────────────── # Can reuse the LLM values above if that model accepts images. diff --git a/openrag/core/config/endpoints.py b/openrag/core/config/endpoints.py index 52da0a7cc..a30f76c23 100644 --- a/openrag/core/config/endpoints.py +++ b/openrag/core/config/endpoints.py @@ -2,10 +2,34 @@ from __future__ import annotations +import os + from pydantic import Field from .base import ConfigMixin +LLM_OVERRIDE_ENDPOINT_ENV = "LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT" + + +def custom_endpoint_override_enabled() -> bool: + """Is a client-supplied ``llm_override.base_url`` honored at all? + + Off by default, and deliberately not host-restricted when on: enabling it + lets any authenticated caller make the server issue requests to an arbitrary + URL, which is a server-side request forgery primitive (cloud metadata + endpoints, internal admin services). Turn it on only where every API caller + is already trusted with that. + + Lives here rather than beside its main consumer in ``services.inference`` + because the API layer needs it too — the chat router skips its server-side + ``max_tokens`` default for overridden endpoints — and ``api -> services`` is + a forbidden import direction. + + Read on demand rather than at import so a test — or a reloaded worker — sees + the current environment. + """ + return os.environ.get(LLM_OVERRIDE_ENDPOINT_ENV, "").strip().lower() in ("1", "true", "yes", "on") + class LLMParamsConfig(ConfigMixin): """Shared parameters for LLM/VLM endpoints.""" diff --git a/openrag/services/inference/vllm_client.py b/openrag/services/inference/vllm_client.py index ef5befaf3..4e6e6222f 100644 --- a/openrag/services/inference/vllm_client.py +++ b/openrag/services/inference/vllm_client.py @@ -16,8 +16,10 @@ import base64 import re from collections.abc import AsyncIterator, Mapping +from urllib.parse import urlsplit import httpx +from core.config.endpoints import custom_endpoint_override_enabled from core.embeddings import Embedder, embedder_registry from core.llm import LLM, llm_registry from core.utils.exceptions import ( @@ -148,10 +150,14 @@ def __init__( self._api_key = api_key self._enable_thinking = enable_thinking self._defaults: dict = kwargs - headers: dict[str, str] = {"Content-Type": "application/json"} - if api_key: - headers["Authorization"] = f"Bearer {api_key}" - self._client = httpx.AsyncClient(timeout=timeout, headers=headers) + self._allow_custom_endpoint = custom_endpoint_override_enabled() + # Authorization is sent per request, not set on the client: httpx merges + # client-level headers into every request and offers no way to drop one, + # so a client-level Authorization would ride along to an overridden + # endpoint even when the override carries no key of its own — handing the + # server's credential to a third-party host. + self._auth_headers: dict[str, str] = {"Authorization": f"Bearer {api_key}"} if api_key else {} + self._client = httpx.AsyncClient(timeout=timeout, headers={"Content-Type": "application/json"}) # Same construction breadcrumb as VLLMEmbedder: the component factories # cache instances per endpoint name, so this fires once per configured # endpoint and shows which base URL/model a preset name resolved to. @@ -162,28 +168,68 @@ def __init__( enable_thinking=self._enable_thinking, ).debug(f"{type(self).__name__} ready") - def _resolve_overrides(self, kwargs: dict) -> tuple[str, str, dict[str, str] | None]: + def _resolve_overrides(self, kwargs: dict) -> tuple[str, str, dict[str, str]]: """Read ``metadata.llm_override`` from *kwargs* without mutating caller data. Pure read: ``kwargs`` is untouched so retries see the original override on every attempt. The caller strips ``metadata`` from the outbound payload via ``_payload_kwargs`` — every ``metadata`` key is OpenRAG-internal and never belongs on the wire. + + Returns the headers to send, never ``None``: the server's Authorization + lives here rather than on the shared client precisely so an overridden + endpoint can be given different credentials — or none. """ base_url = self._endpoint model = self._model - override_headers: dict[str, str] | None = None + headers = self._auth_headers - # Only `model` may be overridden by the client. `base_url` / `api_key` - # are deliberately NOT read from the request: honoring a client-supplied - # endpoint enables SSRF (the server would issue requests to an arbitrary - # host, e.g. cloud metadata) and would leak the server's API key to that - # host. The endpoint and credentials always come from server config. + # `model` is always client-overridable. `base_url` / `api_key` are honored + # only when the operator sets LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT — see + # `_resolve_endpoint_override` for what that opens up. llm_override = (kwargs.get("metadata") or {}).get("llm_override") or {} if llm_override.get("model"): model = llm_override["model"] - return base_url, model, override_headers + if llm_override.get("base_url") and self._allow_custom_endpoint: + base_url, headers = self._resolve_endpoint_override(llm_override) + + return base_url, model, headers + + def _resolve_endpoint_override(self, llm_override: Mapping) -> tuple[str, dict[str, str]]: + """Honor a legacy ``llm_override`` endpoint (opt-in, unrestricted). + + Restores the pre-refactor contract — ``base_url`` + ``api_key`` + ``model`` + in ``metadata.llm_override`` — for deployments whose clients still send it. + + The target URL is deliberately unconstrained, so with the flag on this is a + full SSRF primitive: any caller who can reach ``/v1/chat/completions`` can + make the server issue a POST to any URL it can route to. Two properties + keep the blast radius to that: + + * The server's own API key is never sent to an overridden endpoint. The + override's ``api_key`` is used, or no ``Authorization`` at all — never a + fallback to ``self._api_key``. + * Only ``http``/``https``. httpx would reject the rest anyway; failing here + turns an opaque transport error into a named one. + + Rejections are 4xx so ``with_retry`` (429/502/503/504 only) does not + re-attempt a request that can never succeed. + """ + candidate = str(llm_override["base_url"]).strip().rstrip("/") + scheme = urlsplit(candidate).scheme.lower() + + if scheme not in ("http", "https"): + raise InferenceError( + f"llm_override.base_url scheme {scheme!r} is not allowed (http/https only)", + code="LLM_OVERRIDE_REJECTED", + status_code=400, + ) + + api_key = llm_override.get("api_key") + headers = {"Authorization": f"Bearer {api_key}"} if api_key else {} + logger.bind(endpoint=candidate, configured=self._endpoint).debug("Honoring legacy llm_override endpoint") + return candidate, headers def _chat_payload_kwargs(self, kwargs: dict) -> dict: payload_kwargs = {**self._defaults, **kwargs} @@ -510,6 +556,10 @@ async def caption_image(self, image_bytes: bytes, prompt: str | None = None) -> resp = await self._client.post( f"{self._endpoint}/chat/completions", json=payload, + # Explicit because VLLMClient moved Authorization off the shared + # httpx client; captioning always uses the configured endpoint, + # so it always carries the server's key. + headers=self._auth_headers, ) resp.raise_for_status() except httpx.ConnectError as exc: diff --git a/tests/unit/services/inference/test_vllm_client.py b/tests/unit/services/inference/test_vllm_client.py index a20ba34e2..4a8690e54 100644 --- a/tests/unit/services/inference/test_vllm_client.py +++ b/tests/unit/services/inference/test_vllm_client.py @@ -15,6 +15,7 @@ InferenceTimeoutError, ) from services.inference._circuit_breaker import _breakers +from services.inference._retry import _is_retryable from services.inference.vllm_client import ( _SUSPECT_UNICODE_ESCAPE, VLLMClient, @@ -268,7 +269,7 @@ def test_no_override_uses_defaults(self): base_url, model, headers = client._resolve_overrides(kwargs) assert base_url == "http://default:8000/v1" assert model == "default-model" - assert headers is None + assert headers == {"Authorization": "Bearer default-key"} def test_llm_override_model_applied_endpoint_and_key_ignored(self): client = self._make_client() @@ -284,7 +285,7 @@ def test_llm_override_model_applied_endpoint_and_key_ignored(self): # Only `model` is honored; endpoint and credentials stay server-side. assert model == "custom-model" assert base_url == "http://default:8000/v1" - assert headers is None + assert headers == {"Authorization": "Bearer default-key"} # kwargs must not be mutated — retries depend on llm_override surviving. assert kwargs["metadata"] is original_metadata assert "llm_override" in kwargs["metadata"] @@ -299,7 +300,7 @@ def test_llm_override_partial(self): base_url, model, headers = client._resolve_overrides(kwargs) assert base_url == "http://default:8000/v1" assert model == "override-model" - assert headers is None + assert headers == {"Authorization": "Bearer default-key"} assert kwargs["metadata"] is original_metadata assert kwargs["metadata"] == { "llm_override": {"model": "override-model"}, @@ -322,7 +323,168 @@ def test_client_base_url_and_api_key_override_ignored(self): base_url, model, headers = client._resolve_overrides(kwargs) assert model == "custom-model" assert base_url == "http://default:8000/v1" - assert headers is None + assert headers == {"Authorization": "Bearer default-key"} + + +class TestLegacyEndpointOverride: + """LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT re-enables the pre-refactor llm_override. + + Before the services refactor, ``llm_override`` honored ``base_url`` and + ``api_key`` as well as ``model``. That was removed as an SSRF and + key-exfiltration vector. This opt-in restores it verbatim — no restriction on + the target URL — so deployments with legacy clients can migrate without + editing the client. What it must NOT do is leak the server's own credential. + """ + + def _make_client(self, monkeypatch, enabled: bool): + if enabled: + monkeypatch.setenv("LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT", "true") + else: + monkeypatch.delenv("LLM_OVERRIDE_ALLOW_CUSTOM_ENDPOINT", raising=False) + return VLLMClient( + endpoint="http://default:8000/v1", + model_name="default-model", + api_key="default-key", + ) + + def _kwargs(self, **override): + return {"metadata": {"llm_override": override}} + + def test_arbitrary_endpoint_is_honored_with_client_key(self, monkeypatch): + client = self._make_client(monkeypatch, enabled=True) + base_url, model, headers = client._resolve_overrides( + self._kwargs(base_url="https://api.openai.com/v1/", api_key="sk-client", model="gpt-5.1") + ) + + assert base_url == "https://api.openai.com/v1" + assert model == "gpt-5.1" + assert headers == {"Authorization": "Bearer sk-client"} + + @pytest.mark.parametrize( + "url", + [ + "http://169.254.169.254/latest/meta-data", + "http://localhost:9200", + "https://api.openai.com@evil.tld/v1", + ], + ) + def test_no_host_restriction_when_enabled(self, monkeypatch, url): + """Explicit: with the flag on there is no allowlist. Cloud metadata, an + internal service and a userinfo-prefixed host are all reachable — that is + the SSRF the flag trades away, asserted so it cannot regress silently. + """ + client = self._make_client(monkeypatch, enabled=True) + base_url, _, _ = client._resolve_overrides(self._kwargs(base_url=url, api_key="k", model="m")) + + assert base_url == url.rstrip("/") + + def test_server_api_key_never_reaches_an_overridden_endpoint(self, monkeypatch): + """The one hard guarantee left: an override with no api_key sends no + Authorization at all rather than falling back to the server's credential. + """ + client = self._make_client(monkeypatch, enabled=True) + _, _, headers = client._resolve_overrides(self._kwargs(base_url="https://evil.tld/v1", model="m")) + + assert headers == {} + + def test_non_http_scheme_is_rejected_without_retry(self, monkeypatch): + """httpx would reject file:// anyway; failing here turns an opaque + transport error into a named 4xx that `with_retry` will not re-attempt. + """ + client = self._make_client(monkeypatch, enabled=True) + + with pytest.raises(InferenceError) as exc_info: + client._resolve_overrides(self._kwargs(base_url="file:///etc/passwd", api_key="k")) + + assert exc_info.value.status_code == 400 + assert not _is_retryable(exc_info.value) + + def test_disabled_by_default_still_ignores_base_url(self, monkeypatch): + """Unset env keeps the hardened post-refactor behaviour verbatim, so an + upgrade never turns an untouched deployment into an SSRF surface. + """ + client = self._make_client(monkeypatch, enabled=False) + base_url, model, headers = client._resolve_overrides( + self._kwargs(base_url="https://api.openai.com/v1", api_key="sk-client", model="gpt-5.1") + ) + + assert base_url == "http://default:8000/v1" + assert model == "gpt-5.1" + assert headers == {"Authorization": "Bearer default-key"} + + def test_override_does_not_mutate_caller_metadata(self, monkeypatch): + """Retries re-read the same kwargs, so the override must survive intact.""" + client = self._make_client(monkeypatch, enabled=True) + kwargs = self._kwargs(base_url="https://api.openai.com/v1", api_key="sk-client", model="gpt-5.1") + original = kwargs["metadata"]["llm_override"].copy() + + client._resolve_overrides(kwargs) + + assert kwargs["metadata"]["llm_override"] == original + + @pytest.mark.asyncio + async def test_override_reaches_the_wire_on_chat(self, monkeypatch): + """End-to-end through `chat`: the request must actually be issued to the + overridden host, carrying the client's key rather than the server's. + """ + client = self._make_client(monkeypatch, enabled=True) + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["url"] = str(request.url) + seen["auth"] = request.headers.get("Authorization") + return _chat_response("ok") + + client._client = httpx.AsyncClient(transport=_make_transport(handler)) + await client.chat( + [{"role": "user", "content": "hi"}], + metadata={ + "llm_override": { + "base_url": "https://api.openai.com/v1", + "api_key": "sk-client", + "model": "gpt-5.1", + } + }, + ) + + assert seen["url"] == "https://api.openai.com/v1/chat/completions" + assert seen["auth"] == "Bearer sk-client" + + @pytest.mark.asyncio + async def test_keyless_override_sends_no_authorization_on_the_wire(self, monkeypatch): + """The header-level counterpart of the guarantee above: httpx merges + client-level headers into every request, so this only holds because + Authorization is set per request rather than on the shared client. + """ + client = self._make_client(monkeypatch, enabled=True) + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["auth"] = request.headers.get("Authorization") + return _chat_response("ok") + + client._client = httpx.AsyncClient(transport=_make_transport(handler)) + await client.chat( + [{"role": "user", "content": "hi"}], + metadata={"llm_override": {"base_url": "https://no-auth.internal/v1", "model": "m"}}, + ) + + assert seen["auth"] is None + + @pytest.mark.asyncio + async def test_server_key_is_sent_when_there_is_no_override(self, monkeypatch): + """Moving Authorization off the client must not silently drop it.""" + client = self._make_client(monkeypatch, enabled=True) + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["auth"] = request.headers.get("Authorization") + return _chat_response("ok") + + client._client = httpx.AsyncClient(transport=_make_transport(handler)) + await client.chat([{"role": "user", "content": "hi"}]) + + assert seen["auth"] == "Bearer default-key" # --------------------------------------------------------------------------- @@ -533,6 +695,23 @@ def handler(request: httpx.Request) -> httpx.Response: await self._make_vision(handler).caption_image(b"\x89PNG\r\n\x1a\n") + @pytest.mark.asyncio + async def test_caption_image_sends_the_configured_api_key(self): + """VLLMClient sets Authorization per request, not on the shared httpx + client, so an overridden llm endpoint never receives the server's key. + Captioning inherits that client but always calls the configured endpoint, + so it must pass the header explicitly or authenticate as nobody. + """ + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["auth"] = request.headers.get("Authorization") + return _chat_response("ok") + + await self._make_vision(handler).caption_image(b"img") + + assert seen["auth"] == "Bearer test-key" + @pytest.mark.asyncio async def test_caption_images_batch(self): call_count = 0 From af55f01f56c6603788f3213ddd6b49aa37748fbf Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 16:10:25 +0200 Subject: [PATCH 2/5] fix(llm): skip server sampling defaults on overridden endpoints --- openrag/services/inference/vllm_client.py | 46 ++++++++++++++++++++--- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/openrag/services/inference/vllm_client.py b/openrag/services/inference/vllm_client.py index 4e6e6222f..c7cfbf109 100644 --- a/openrag/services/inference/vllm_client.py +++ b/openrag/services/inference/vllm_client.py @@ -231,9 +231,30 @@ def _resolve_endpoint_override(self, llm_override: Mapping) -> tuple[str, dict[s logger.bind(endpoint=candidate, configured=self._endpoint).debug("Honoring legacy llm_override endpoint") return candidate, headers - def _chat_payload_kwargs(self, kwargs: dict) -> dict: - payload_kwargs = {**self._defaults, **kwargs} - enable_thinking = payload_kwargs.pop("enable_thinking", self._enable_thinking) + def _has_endpoint_override(self, kwargs: dict) -> bool: + """Is this request being routed to a client-supplied endpoint? + + Read before ``kwargs.pop("metadata")``. Decides whether the server's + sampling defaults apply — see ``_chat_payload_kwargs``. + """ + if not self._allow_custom_endpoint: + return False + llm_override = (kwargs.get("metadata") or {}).get("llm_override") or {} + return bool(llm_override.get("base_url")) + + def _chat_payload_kwargs(self, kwargs: dict, *, use_defaults: bool = True) -> dict: + """Merge server sampling defaults under the request's own params. + + ``use_defaults=False`` for a client-supplied endpoint: the defaults + (``temperature``, ``logprobs``, ``enable_thinking``) describe the server's + *configured* model, and a different provider may reject them outright — + e.g. Gemini 400s on an unsolicited ``logprobs``. The pre-refactor client + had the same rule, and dropping it is what still made a legacy + ``llm_override`` fail after the endpoint itself was restored. + """ + payload_kwargs = {**self._defaults, **kwargs} if use_defaults else dict(kwargs) + fallback_thinking = self._enable_thinking if use_defaults else None + enable_thinking = payload_kwargs.pop("enable_thinking", fallback_thinking) if enable_thinking is not None and enable_thinking is True: chat_template_kwargs = dict(payload_kwargs.get("chat_template_kwargs") or {}) chat_template_kwargs.setdefault("enable_thinking", enable_thinking) @@ -244,8 +265,9 @@ def _chat_payload_kwargs(self, kwargs: dict) -> dict: @with_retry(max_attempts=3) async def generate(self, prompt: str, **kwargs) -> dict: base_url, model, headers = self._resolve_overrides(kwargs) + overridden = self._has_endpoint_override(kwargs) kwargs.pop("metadata", None) - payload = {**self._defaults, **kwargs, "model": model, "prompt": prompt} + payload = {**({} if overridden else self._defaults), **kwargs, "model": model, "prompt": prompt} try: resp = await self._client.post(f"{base_url}/completions", json=payload, headers=headers) resp.raise_for_status() @@ -264,8 +286,14 @@ async def generate(self, prompt: str, **kwargs) -> dict: @with_retry(max_attempts=3) async def chat(self, messages: list[dict[str, str]], **kwargs) -> dict: base_url, model, headers = self._resolve_overrides(kwargs) + overridden = self._has_endpoint_override(kwargs) kwargs.pop("metadata", None) - payload = {**self._chat_payload_kwargs(kwargs), "model": model, "messages": messages, "stream": False} + payload = { + **self._chat_payload_kwargs(kwargs, use_defaults=not overridden), + "model": model, + "messages": messages, + "stream": False, + } try: resp = await self._client.post(f"{base_url}/chat/completions", json=payload, headers=headers) resp.raise_for_status() @@ -282,8 +310,14 @@ async def chat(self, messages: list[dict[str, str]], **kwargs) -> dict: async def stream_chat(self, messages: list[dict[str, str]], **kwargs) -> AsyncIterator[str]: base_url, model, headers = self._resolve_overrides(kwargs) + overridden = self._has_endpoint_override(kwargs) kwargs.pop("metadata", None) - payload = {**self._chat_payload_kwargs(kwargs), "model": model, "messages": messages, "stream": True} + payload = { + **self._chat_payload_kwargs(kwargs, use_defaults=not overridden), + "model": model, + "messages": messages, + "stream": True, + } try: async with self._client.stream( "POST", f"{base_url}/chat/completions", json=payload, headers=headers From b6951b30ef09e185151cc978f68c5a8d297e92f9 Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 16:10:55 +0200 Subject: [PATCH 3/5] fix(llm): warn when an llm_override endpoint is dropped --- openrag/services/inference/vllm_client.py | 25 ++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/openrag/services/inference/vllm_client.py b/openrag/services/inference/vllm_client.py index c7cfbf109..d6d38f38f 100644 --- a/openrag/services/inference/vllm_client.py +++ b/openrag/services/inference/vllm_client.py @@ -19,7 +19,7 @@ from urllib.parse import urlsplit import httpx -from core.config.endpoints import custom_endpoint_override_enabled +from core.config.endpoints import LLM_OVERRIDE_ENDPOINT_ENV, custom_endpoint_override_enabled from core.embeddings import Embedder, embedder_registry from core.llm import LLM, llm_registry from core.utils.exceptions import ( @@ -191,8 +191,27 @@ def _resolve_overrides(self, kwargs: dict) -> tuple[str, str, dict[str, str]]: if llm_override.get("model"): model = llm_override["model"] - if llm_override.get("base_url") and self._allow_custom_endpoint: - base_url, headers = self._resolve_endpoint_override(llm_override) + if llm_override.get("base_url"): + if self._allow_custom_endpoint: + base_url, headers = self._resolve_endpoint_override(llm_override) + else: + # The failure this warning exists for: `model` is applied while + # `base_url` is dropped, so the request goes to the *server's* + # endpoint carrying a *third party's* model name. The provider + # then answers "invalid model name", which reads as a client bug + # and says nothing about the override having been ignored. + logger.bind( + requested_endpoint=llm_override.get("base_url"), + used_endpoint=base_url, + model=model, + ).warning( + f"Ignoring llm_override.base_url — {LLM_OVERRIDE_ENDPOINT_ENV} is not enabled. " + f"The request goes to the configured endpoint with model={model!r}." + ) + elif llm_override: + logger.bind(keys=sorted(llm_override), model=model).warning( + "llm_override carries no base_url; only the model name is overridden" + ) return base_url, model, headers From b489bb28ecf2557e9d37aa80e7254cd47dc3563d Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Wed, 29 Jul 2026 16:11:25 +0200 Subject: [PATCH 4/5] fix(chat): skip the server max_tokens default on overridden endpoints --- openrag/api/routers/user/chat.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/openrag/api/routers/user/chat.py b/openrag/api/routers/user/chat.py index 9e315d110..cfd5b66a2 100644 --- a/openrag/api/routers/user/chat.py +++ b/openrag/api/routers/user/chat.py @@ -30,6 +30,7 @@ from api.routers.user.source_links import build_document_source_link from api.schemas.user.chat import OpenAIChatCompletionRequest, OpenAICompletionRequest from core.config import load_config +from core.config.endpoints import custom_endpoint_override_enabled from core.models.preset import resolve_partition_chat_llm from core.utils.exceptions import OpenRAGError from core.utils.logging import get_logger @@ -401,9 +402,22 @@ def _apply_default_max_tokens( consistent with the endpoint that serves the request. An explicit client-supplied value is always honoured. + + Skipped entirely when the request routes to a client-supplied endpoint: the + budget resolved here belongs to the *server's* endpoint and says nothing + about the client's provider, which may not even accept the parameter — + OpenAI's newer models reject ``max_tokens`` outright in favour of + ``max_completion_tokens``. Leaving it unset drops it from the payload + (``model_dump(exclude_none=True)``) and lets the provider apply its own + default. The preflight below is unaffected: ``validate_tokens_limit`` falls + back to the configured default when ``max_tokens`` is ``None``. """ - if request.max_tokens is None: - request.max_tokens = _effective_max_output_tokens(config, partitions) + if request.max_tokens is not None: + return + llm_override = (getattr(request, "metadata", None) or {}).get("llm_override") or {} + if llm_override.get("base_url") and custom_endpoint_override_enabled(): + return + request.max_tokens = _effective_max_output_tokens(config, partitions) def check_tokens_limit( From e7b83f35f31dbe451ee3ab85274fe035b693b062 Mon Sep 17 00:00:00 2001 From: Paul Tran-Van Date: Thu, 30 Jul 2026 22:10:09 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix(llm):=20harden=20llm=5Foverride=20endpo?= =?UTF-8?q?int=20=E2=80=94=20https-only,=20no=20path=20control?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- infra/compose/.env.example | 11 ++-- openrag/core/config/endpoints.py | 7 ++- openrag/services/inference/vllm_client.py | 55 ++++++++++++++---- .../services/inference/test_vllm_client.py | 56 ++++++++++++++----- 4 files changed, 97 insertions(+), 32 deletions(-) diff --git a/infra/compose/.env.example b/infra/compose/.env.example index 9b8c10fd3..32edc4232 100644 --- a/infra/compose/.env.example +++ b/infra/compose/.env.example @@ -17,10 +17,13 @@ LLM_SEMAPHORE=10 # Legacy escape hatch for pre-refactor clients. Clients may always override the # model name via metadata.llm_override; set this to also honor base_url/api_key. # -# DANGER: there is no restriction on the target URL. With this on, any caller who -# can reach /v1/chat/completions can make the server POST to any URL it can route -# to — cloud metadata endpoints, internal admin services. That is server-side -# request forgery. Enable only where every API caller is already trusted with it. +# DANGER: the target HOST is unrestricted. With this on, any caller who can reach +# /v1/chat/completions can make the server POST to any host it can route to — a +# server-side request forgery primitive. It is constrained to https and to the +# fixed /chat/completions path (no client-controlled query, fragment, or path +# traversal), which keeps it off plaintext internal infra and out of arbitrary +# internal endpoints — but a host that itself serves /chat/completions is still +# reachable. Enable only where every API caller is already trusted with that. # The server's own API key is never forwarded to an overridden endpoint. # # Prefer registering a named endpoint under /model-endpoints and binding it to diff --git a/openrag/core/config/endpoints.py b/openrag/core/config/endpoints.py index a30f76c23..e3f599b0c 100644 --- a/openrag/core/config/endpoints.py +++ b/openrag/core/config/endpoints.py @@ -16,9 +16,10 @@ def custom_endpoint_override_enabled() -> bool: Off by default, and deliberately not host-restricted when on: enabling it lets any authenticated caller make the server issue requests to an arbitrary - URL, which is a server-side request forgery primitive (cloud metadata - endpoints, internal admin services). Turn it on only where every API caller - is already trusted with that. + host, which is a server-side request forgery primitive. The request itself is + constrained (https only, fixed ``/chat/completions`` path — see + ``VLLMClient._resolve_endpoint_override``), but the host is not. Turn it on + only where every API caller is already trusted with that. Lives here rather than beside its main consumer in ``services.inference`` because the API layer needs it too — the chat router skips its server-side diff --git a/openrag/services/inference/vllm_client.py b/openrag/services/inference/vllm_client.py index d6d38f38f..05dbcd2e6 100644 --- a/openrag/services/inference/vllm_client.py +++ b/openrag/services/inference/vllm_client.py @@ -216,35 +216,66 @@ def _resolve_overrides(self, kwargs: dict) -> tuple[str, str, dict[str, str]]: return base_url, model, headers def _resolve_endpoint_override(self, llm_override: Mapping) -> tuple[str, dict[str, str]]: - """Honor a legacy ``llm_override`` endpoint (opt-in, unrestricted). + """Honor a legacy ``llm_override`` endpoint (opt-in, host-unrestricted). Restores the pre-refactor contract — ``base_url`` + ``api_key`` + ``model`` in ``metadata.llm_override`` — for deployments whose clients still send it. - The target URL is deliberately unconstrained, so with the flag on this is a - full SSRF primitive: any caller who can reach ``/v1/chat/completions`` can - make the server issue a POST to any URL it can route to. Two properties - keep the blast radius to that: - + With the flag on, *which host* is reached is deliberately unconstrained: + any caller who can reach ``/v1/chat/completions`` can make the server POST + to any host it can route to. What is constrained is *what the request looks + like*, and that is what keeps this from being a read primitive against + internal HTTP APIs: + + * **https only.** Refusing ``http`` keeps the override off plaintext + internal infra — Milvus, Postgres-over-HTTP, admin panels — which is + where the SSRF payoff lives, those services trusting anything on their + network. + * **No client-controlled path.** The endpoint is always hit as + ``{base_url}/chat/completions``. A ``#`` or ``?`` in ``base_url`` would + truncate that appended suffix once concatenated + (``…/collections/list#/chat/completions`` reaches ``…/collections/list``), + and a ``..`` segment would traverse out of it — either turns the override + into a path-picker aimed at arbitrary internal endpoints. Rejected here, + the suffix is always appended literally, so only a host that genuinely + serves ``…/chat/completions`` can answer. A real LLM base URL + (``https://api.openai.com/v1``) trips none of these. * The server's own API key is never sent to an overridden endpoint. The override's ``api_key`` is used, or no ``Authorization`` at all — never a fallback to ``self._api_key``. - * Only ``http``/``https``. httpx would reject the rest anyway; failing here - turns an opaque transport error into a named one. + + Note the ``#``/``?`` test is on the raw string, not ``urlsplit`` parts: a + trailing ``#`` parses as an *empty* fragment on ``base_url`` alone and only + becomes meaningful after ``/chat/completions`` is concatenated, so the + parsed view would miss exactly the truncation being defended against. Rejections are 4xx so ``with_retry`` (429/502/503/504 only) does not re-attempt a request that can never succeed. """ - candidate = str(llm_override["base_url"]).strip().rstrip("/") - scheme = urlsplit(candidate).scheme.lower() + candidate = str(llm_override["base_url"]).strip() - if scheme not in ("http", "https"): + if "#" in candidate or "?" in candidate: + raise InferenceError( + "llm_override.base_url must not carry a query string or fragment", + code="LLM_OVERRIDE_REJECTED", + status_code=400, + ) + parts = urlsplit(candidate) + scheme = parts.scheme.lower() + if scheme != "https": + raise InferenceError( + f"llm_override.base_url scheme {scheme!r} is not allowed (https only)", + code="LLM_OVERRIDE_REJECTED", + status_code=400, + ) + if ".." in parts.path.split("/"): raise InferenceError( - f"llm_override.base_url scheme {scheme!r} is not allowed (http/https only)", + "llm_override.base_url must not contain a '..' path segment", code="LLM_OVERRIDE_REJECTED", status_code=400, ) + candidate = candidate.rstrip("/") api_key = llm_override.get("api_key") headers = {"Authorization": f"Bearer {api_key}"} if api_key else {} logger.bind(endpoint=candidate, configured=self._endpoint).debug("Honoring legacy llm_override endpoint") diff --git a/tests/unit/services/inference/test_vllm_client.py b/tests/unit/services/inference/test_vllm_client.py index 4a8690e54..ef7a0ed09 100644 --- a/tests/unit/services/inference/test_vllm_client.py +++ b/tests/unit/services/inference/test_vllm_client.py @@ -331,9 +331,11 @@ class TestLegacyEndpointOverride: Before the services refactor, ``llm_override`` honored ``base_url`` and ``api_key`` as well as ``model``. That was removed as an SSRF and - key-exfiltration vector. This opt-in restores it verbatim — no restriction on - the target URL — so deployments with legacy clients can migrate without - editing the client. What it must NOT do is leak the server's own credential. + key-exfiltration vector. This opt-in restores it for legacy clients, but with + the request shape constrained so it can't be aimed at internal HTTP APIs: the + *host* is unrestricted, yet the scheme must be https and the path is fixed + (no client-controlled query / fragment / ``..``). It must also never leak the + server's own credential. """ def _make_client(self, monkeypatch, enabled: bool): @@ -363,15 +365,16 @@ def test_arbitrary_endpoint_is_honored_with_client_key(self, monkeypatch): @pytest.mark.parametrize( "url", [ - "http://169.254.169.254/latest/meta-data", - "http://localhost:9200", + "https://169.254.169.254/latest/meta-data", + "https://internal-service.corp/v1", "https://api.openai.com@evil.tld/v1", ], ) - def test_no_host_restriction_when_enabled(self, monkeypatch, url): - """Explicit: with the flag on there is no allowlist. Cloud metadata, an - internal service and a userinfo-prefixed host are all reachable — that is - the SSRF the flag trades away, asserted so it cannot regress silently. + def test_no_host_allowlist_when_enabled(self, monkeypatch, url): + """Explicit: with the flag on there is no host allowlist. Cloud metadata, + an internal service and a userinfo-prefixed host are all reachable over + https — that is the residual SSRF the flag trades away (host, not path), + asserted so it cannot regress into a false sense of safety. """ client = self._make_client(monkeypatch, enabled=True) base_url, _, _ = client._resolve_overrides(self._kwargs(base_url=url, api_key="k", model="m")) @@ -387,14 +390,41 @@ def test_server_api_key_never_reaches_an_overridden_endpoint(self, monkeypatch): assert headers == {} - def test_non_http_scheme_is_rejected_without_retry(self, monkeypatch): - """httpx would reject file:// anyway; failing here turns an opaque - transport error into a named 4xx that `with_retry` will not re-attempt. + @pytest.mark.parametrize("base_url", ["http://milvus:19530/v2", "file:///etc/passwd"]) + def test_non_https_scheme_is_rejected_without_retry(self, monkeypatch, base_url): + """https only. Plaintext http is where the internal-infra SSRF payoff lives + (Milvus, Postgres-over-HTTP, admin panels are overwhelmingly http); file:// + httpx would reject anyway. Failing here turns both into a named 4xx that + `with_retry` will not re-attempt. """ client = self._make_client(monkeypatch, enabled=True) with pytest.raises(InferenceError) as exc_info: - client._resolve_overrides(self._kwargs(base_url="file:///etc/passwd", api_key="k")) + client._resolve_overrides(self._kwargs(base_url=base_url, api_key="k")) + + assert exc_info.value.status_code == 400 + assert not _is_retryable(exc_info.value) + + @pytest.mark.parametrize( + "base_url", + [ + "https://milvus:19530/v2/vectordb/collections/list#", + "https://milvus:19530/v2/vectordb/collections/list?x=1", + "https://milvus:19530/v2/vectordb/../collections", + ], + ) + def test_client_controlled_path_is_rejected_without_retry(self, monkeypatch, base_url): + """The concrete read primitive from the security writeup: the request is + always issued as ``{base_url}/chat/completions``, so a fragment or query + truncates that appended suffix and a ``..`` traverses out of it — either + one lets the caller aim at an arbitrary internal path (here Milvus' REST + API, reading cross-partition vectors). All rejected, so the suffix is + always appended literally and only a real /chat/completions host answers. + """ + client = self._make_client(monkeypatch, enabled=True) + + with pytest.raises(InferenceError) as exc_info: + client._resolve_overrides(self._kwargs(base_url=base_url, api_key="k")) assert exc_info.value.status_code == 400 assert not _is_retryable(exc_info.value)