mirror of
https://github.com/infiniflow/ragflow.git
synced 2026-08-22 08:13:21 +08:00
fix(api): report why provider model discovery failed instead of swallowing it (#18027)
### Summary Providers whose static catalogue is empty discover their models by calling the base URL the user typed. `verify_api_key` wrapped that call in a bare `except Exception: pass` and then returned a flat `No models found for provider 'X'`, so an unreachable host, a closed port, a wrong scheme and a bad TLS setup all produced the same sentence, with the actual error discarded and not even logged.
This commit is contained in:
Ali Farhan
@@ -17,6 +17,7 @@ import os
|
||||
import json
|
||||
import logging
|
||||
import asyncio
|
||||
from urllib.parse import urlparse, urlunparse
|
||||
|
||||
from common.constants import LLMType, ActiveStatusEnum, ModelVerifyStatusEnum
|
||||
from common.settings import FACTORY_LLM_INFOS
|
||||
@@ -52,6 +53,53 @@ def _normalize_provider_base_url(provider_name: str, base_url: str | None):
|
||||
return base_url
|
||||
|
||||
|
||||
def _redact_url(url: str | None) -> str:
|
||||
"""Drop credentials from a user-supplied URL before it reaches a log or a response.
|
||||
|
||||
Provider base URLs are typed by the user and routinely carry secrets, either as
|
||||
userinfo (`https://user:token@host/v1`) or as a query token
|
||||
(`https://host/v1?api-key=...`). Keep the scheme, host, port and path, which is
|
||||
what makes a discovery failure diagnosable, and drop the rest.
|
||||
"""
|
||||
if not url:
|
||||
return ""
|
||||
try:
|
||||
parsed = urlparse(url)
|
||||
# A base URL with no authority is malformed for our purposes, and `.port` only
|
||||
# validates on access, so an unparsable port raises here rather than at parse
|
||||
# time. Either way the input is unsafe to echo back, so give up on it entirely.
|
||||
if not parsed.netloc:
|
||||
return "<unparsable url>"
|
||||
netloc = parsed.hostname or ""
|
||||
if parsed.port:
|
||||
netloc = f"{netloc}:{parsed.port}"
|
||||
except ValueError:
|
||||
return "<unparsable url>"
|
||||
if parsed.username or parsed.password:
|
||||
netloc = f"***@{netloc}"
|
||||
return urlunparse((parsed.scheme, netloc, parsed.path, "", "", ""))
|
||||
|
||||
|
||||
def _scrub_url_secrets(text: str, url: str | None) -> str:
|
||||
"""Remove every secret-bearing fragment of *url* from provider-produced text.
|
||||
|
||||
A client echoes back the URL it was handed, whole or in part, so replacing only
|
||||
the exact string it was given is not enough to keep userinfo and query tokens out
|
||||
of a message built from an exception.
|
||||
"""
|
||||
if not text or not url:
|
||||
return text
|
||||
text = text.replace(url, _redact_url(url))
|
||||
try:
|
||||
parsed = urlparse(url)
|
||||
except ValueError:
|
||||
return text
|
||||
fragments = [parsed.password, parsed.username, parsed.query, parsed.fragment]
|
||||
for fragment in sorted((f for f in fragments if f), key=len, reverse=True):
|
||||
text = text.replace(fragment, "***")
|
||||
return text
|
||||
|
||||
|
||||
def _normalize_provider_api_key(provider_name: str, api_key: str | dict | None):
|
||||
if provider_name == "VLLM" and not api_key:
|
||||
return "x"
|
||||
@@ -670,6 +718,7 @@ async def verify_api_key(provider_id_or_name: str, api_key: str | dict, base_url
|
||||
factory_llms = factory_info[0]["llm"]
|
||||
if not factory_llms:
|
||||
model_base_url = base_url or factory_info[0].get("url", "")
|
||||
discovery_error = ""
|
||||
try:
|
||||
if provider_name in ModelMeta:
|
||||
remote_models = await ModelMeta[provider_name](api_key, model_base_url).get_model_list()
|
||||
@@ -682,10 +731,19 @@ async def verify_api_key(provider_id_or_name: str, api_key: str | dict, base_url
|
||||
for m in remote_models
|
||||
for mt in m.get("model_types", [])
|
||||
]
|
||||
except Exception:
|
||||
pass
|
||||
except Exception as e:
|
||||
# Discovery reaches a user-supplied base URL, so it fails for mundane reasons:
|
||||
# the host is unreachable from inside the container, TLS is wrong, the port is
|
||||
# closed. Reporting only "no models found" sends people hunting for a bad key.
|
||||
safe_url = _redact_url(model_base_url)
|
||||
reason = _scrub_url_secrets(str(e), model_base_url)
|
||||
# logging.exception would write the raw `e` and its traceback, and clients
|
||||
# echo the URL they were handed, so the credentials would land in the log
|
||||
# even though the caller-visible message is clean. Log the scrubbed reason.
|
||||
logging.error("Model discovery failed for provider %s at %s: %s: %s", provider_name, safe_url, type(e).__name__, reason)
|
||||
discovery_error = f" Discovery against {safe_url} failed: {reason}"
|
||||
if not factory_llms:
|
||||
return False, f"No models found for provider '{provider_id_or_name}'", {}
|
||||
return False, f"No models found for provider '{provider_id_or_name}'.{discovery_error}", {}
|
||||
|
||||
model_verify_result = {}
|
||||
# test if api key works
|
||||
|
||||
@@ -0,0 +1,219 @@
|
||||
#
|
||||
# Copyright 2026 The InfiniFlow Authors. All Rights Reserved.
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License");
|
||||
# you may not use this file except in compliance with the License.
|
||||
# You may obtain a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS,
|
||||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
# See the License for the specific language governing permissions and
|
||||
# limitations under the License.
|
||||
#
|
||||
"""Regression tests for dynamic model discovery inside `verify_api_key`
|
||||
(api/apps/services/provider_api_service.py).
|
||||
|
||||
Providers with an empty static catalogue discover their models by calling the
|
||||
base URL the user typed. That call fails for mundane reasons: the host is not
|
||||
reachable from inside the container, the port is closed, TLS is wrong. The
|
||||
failure used to be swallowed by a bare `except Exception: pass` and reported as
|
||||
a flat "No models found for provider 'X'", which reads as a credential or
|
||||
catalogue problem and sends people hunting for a bad API key.
|
||||
"""
|
||||
|
||||
import importlib.util
|
||||
import logging
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from types import ModuleType, SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
pytestmark = pytest.mark.p2
|
||||
|
||||
PROVIDER = "LM-Studio"
|
||||
BASE_URL = "http://127.0.0.1:1234/v1"
|
||||
|
||||
|
||||
def _stub(monkeypatch, name, **attrs):
|
||||
mod = ModuleType(name)
|
||||
for key, value in attrs.items():
|
||||
setattr(mod, key, value)
|
||||
monkeypatch.setitem(sys.modules, name, mod)
|
||||
if "." in name:
|
||||
parent_name, _, child_name = name.rpartition(".")
|
||||
parent_mod = sys.modules.get(parent_name)
|
||||
if parent_mod is not None:
|
||||
monkeypatch.setattr(parent_mod, child_name, mod, raising=False)
|
||||
return mod
|
||||
|
||||
|
||||
def _load_service(monkeypatch, discovery):
|
||||
"""Load provider_api_service with `ModelMeta[PROVIDER]` bound to `discovery`.
|
||||
|
||||
`discovery` is a zero-argument coroutine function standing in for
|
||||
`get_model_list`, so a test can make discovery raise or come back empty.
|
||||
"""
|
||||
|
||||
class _Meta:
|
||||
def __init__(self, api_key, base_url):
|
||||
self.api_key = api_key
|
||||
self.base_url = base_url
|
||||
|
||||
async def get_model_list(self):
|
||||
return await discovery()
|
||||
|
||||
_stub(monkeypatch, "common.settings", FACTORY_LLM_INFOS=[{"name": PROVIDER, "llm": [], "url": ""}])
|
||||
_stub(monkeypatch, "api.db.db_models", DB=SimpleNamespace())
|
||||
_stub(
|
||||
monkeypatch,
|
||||
"api.db.joint_services.tenant_model_service",
|
||||
resolve_model_config=lambda *_a, **_k: {},
|
||||
delete_models_by_instance_ids=lambda *_a, **_k: None,
|
||||
delete_instances_by_provider_ids=lambda *_a, **_k: None,
|
||||
)
|
||||
_stub(monkeypatch, "api.db.services.tenant_model_provider_service", TenantModelProviderService=SimpleNamespace(get_by_id=lambda _id: (False, None)))
|
||||
_stub(monkeypatch, "api.db.services.tenant_model_instance_service", TenantModelInstanceService=SimpleNamespace())
|
||||
_stub(monkeypatch, "api.db.services.tenant_model_service", TenantModelService=SimpleNamespace())
|
||||
_stub(monkeypatch, "api.utils.model_utils", get_model_type_human=lambda *_a, **_k: "", calculate_model_type=lambda *_a, **_k: 0)
|
||||
_stub(
|
||||
monkeypatch,
|
||||
"rag.llm",
|
||||
ChatModel={},
|
||||
CvModel={},
|
||||
EmbeddingModel={},
|
||||
ModelMeta={PROVIDER: _Meta},
|
||||
OcrModel={},
|
||||
RerankModel={},
|
||||
Seq2txtModel={},
|
||||
TTSModel={},
|
||||
)
|
||||
|
||||
repo_root = Path(__file__).resolve().parents[5]
|
||||
module_path = repo_root / "api" / "apps" / "services" / "provider_api_service.py"
|
||||
spec = importlib.util.spec_from_file_location("test_provider_api_service_verify_discovery_mod", module_path)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
monkeypatch.setitem(sys.modules, "test_provider_api_service_verify_discovery_mod", module)
|
||||
spec.loader.exec_module(module)
|
||||
return module
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unreachable_base_url_is_reported_instead_of_swallowed(monkeypatch):
|
||||
"""The connection error and the URL that was probed both reach the caller."""
|
||||
|
||||
async def _refused():
|
||||
raise ConnectionRefusedError("Cannot connect to host 127.0.0.1:1234")
|
||||
|
||||
module = _load_service(monkeypatch, _refused)
|
||||
ok, message, _ = await module.verify_api_key(PROVIDER, "sk-test", base_url=BASE_URL)
|
||||
|
||||
assert ok is False
|
||||
assert "Cannot connect to host 127.0.0.1:1234" in message
|
||||
assert BASE_URL in message
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_discovery_that_simply_finds_nothing_stays_terse(monkeypatch):
|
||||
"""No exception means no reason to append; the caller keeps the short message."""
|
||||
|
||||
async def _empty():
|
||||
return []
|
||||
|
||||
module = _load_service(monkeypatch, _empty)
|
||||
ok, message, _ = await module.verify_api_key(PROVIDER, "sk-test", base_url=BASE_URL)
|
||||
|
||||
assert ok is False
|
||||
assert f"No models found for provider '{PROVIDER}'" in message
|
||||
assert "failed" not in message
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_credentials_in_the_base_url_never_reach_the_caller(monkeypatch, caplog):
|
||||
"""Users paste endpoints carrying secrets, as userinfo or as a query token.
|
||||
Neither the response nor the application log may repeat them, and the client
|
||||
echoes the URL it was handed back inside the exception text."""
|
||||
secret_url = "https://svc:s3cr3t-token@models.internal:8443/v1?api-key=AKIA-SECRET#frag"
|
||||
|
||||
async def _refused():
|
||||
raise ConnectionRefusedError(f"Cannot connect to {secret_url}")
|
||||
|
||||
module = _load_service(monkeypatch, _refused)
|
||||
with caplog.at_level(logging.DEBUG):
|
||||
_, message, _ = await module.verify_api_key(PROVIDER, "sk-test", base_url=secret_url)
|
||||
|
||||
logged = caplog.text
|
||||
for secret in ("s3cr3t-token", "AKIA-SECRET", "svc:"):
|
||||
assert secret not in message, message
|
||||
assert secret not in logged, logged
|
||||
assert "https://***@models.internal:8443/v1" in message
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_authority_less_base_url_is_not_echoed_back(monkeypatch, caplog):
|
||||
"""`urlparse` happily accepts a URL with no authority, e.g. one the user typed
|
||||
without a scheme. There is no host to keep, so nothing may be echoed."""
|
||||
secret_url = "models.internal/v1?api-key=AKIA-SECRET"
|
||||
|
||||
async def _refused():
|
||||
raise ConnectionRefusedError(f"Cannot connect to {secret_url}")
|
||||
|
||||
module = _load_service(monkeypatch, _refused)
|
||||
with caplog.at_level(logging.DEBUG):
|
||||
_, message, _ = await module.verify_api_key(PROVIDER, "sk-test", base_url=secret_url)
|
||||
|
||||
assert "AKIA-SECRET" not in message, message
|
||||
assert "AKIA-SECRET" not in caplog.text, caplog.text
|
||||
assert "<unparsable url>" in message
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_invalid_port_does_not_propagate_out_of_the_error_handler(monkeypatch):
|
||||
"""`urlparse` defers port validation to attribute access, so a bad port raises
|
||||
from inside the `except` block rather than at parse time."""
|
||||
secret_url = "https://svc:s3cr3t-token@models.internal:notaport/v1"
|
||||
|
||||
async def _refused():
|
||||
raise ConnectionRefusedError("Cannot connect")
|
||||
|
||||
module = _load_service(monkeypatch, _refused)
|
||||
ok, message, _ = await module.verify_api_key(PROVIDER, "sk-test", base_url=secret_url)
|
||||
|
||||
assert ok is False
|
||||
assert "s3cr3t-token" not in message, message
|
||||
assert "<unparsable url>" in message
|
||||
|
||||
|
||||
def test_redact_url_keeps_what_makes_a_failure_diagnosable(monkeypatch):
|
||||
async def _unused():
|
||||
return []
|
||||
|
||||
redact = _load_service(monkeypatch, _unused)._redact_url
|
||||
|
||||
assert redact("http://127.0.0.1:1234/v1") == "http://127.0.0.1:1234/v1"
|
||||
assert redact("https://user:pw@host/v1") == "https://***@host/v1"
|
||||
assert redact("https://host/v1?api-key=SECRET") == "https://host/v1"
|
||||
assert redact("https://host/v1#tok=SECRET") == "https://host/v1"
|
||||
assert redact("host/v1?api-key=SECRET") == "<unparsable url>"
|
||||
assert redact("https://host:notaport/v1") == "<unparsable url>"
|
||||
assert redact("") == ""
|
||||
assert redact(None) == ""
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_discovery_failure_does_not_abort_the_request(monkeypatch):
|
||||
"""A raising probe still yields a normal (False, message, {}) result rather
|
||||
than propagating out of verify_api_key."""
|
||||
|
||||
async def _boom():
|
||||
raise RuntimeError("TLS handshake failed")
|
||||
|
||||
module = _load_service(monkeypatch, _boom)
|
||||
ok, message, verify_result = await module.verify_api_key(PROVIDER, "sk-test", base_url=BASE_URL)
|
||||
|
||||
assert ok is False
|
||||
assert verify_result == {}
|
||||
assert "TLS handshake failed" in message
|
||||
Reference in New Issue
Block a user