refactor(custom): id-based routing, single source of truth, security fixes

Rework the multi custom-provider design per maintainer review:

1. Data model: use server-generated uuid4 short id as primary key;
   'name' is now a pure display label that can be freely renamed.

2. Routing: drop 'custom_active_provider'; activate a provider by
   setting bot_type to 'custom:<id>'. Single source of truth — no
   pointer drift between bot_type and a separate active selector.

3. Security: drag_sensitive() now recursively masks api_key/secret in
   nested structures (custom_providers list); previously only top-level
   string fields were masked.

4. Per-provider model: the provider's 'model' field now takes effect on
   the main chat path and agent path (was silently ignored before).

5. XSS fix: replace all inline onclick handlers in custom-provider UI
   with data-* attributes + event delegation. Provider names never
   appear in executable HTML contexts.

Legacy compatibility: bot_type='custom' (no colon) still reads the flat
custom_api_key/custom_api_base fields byte-for-byte identically.

Closes: consolidates #2876 into this PR as requested.
Ref: #2838
This commit is contained in:
kirs-hi
2026-06-11 17:25:24 +08:00
parent cffa590d3e
commit 1940d628a8
11 changed files with 504 additions and 353 deletions

View File

@@ -4,8 +4,9 @@ Unit tests for multiple custom (OpenAI-compatible) provider support (issue #2838
Covers models/custom_provider.py:
- Backward compatibility: legacy custom_api_key / custom_api_base fallback
- Multi-provider selection via custom_providers / custom_active_provider
- Robustness against malformed config (missing name, non-dict, non-list)
- Multi-provider selection via bot_type "custom:<id>" routing
- parse_custom_bot_type helper
- Robustness against malformed config (missing id, non-dict, non-list)
"""
import sys
import os
@@ -23,11 +24,43 @@ def set_conf(d):
config_module.config = Config(d)
class TestParseCustomBotType(unittest.TestCase):
"""parse_custom_bot_type() parsing logic."""
def setUp(self):
from models.custom_provider import parse_custom_bot_type
self.parse = parse_custom_bot_type
def test_legacy_custom(self):
is_custom, pid = self.parse("custom")
self.assertTrue(is_custom)
self.assertEqual(pid, "")
def test_custom_with_id(self):
is_custom, pid = self.parse("custom:3f2a9c1b")
self.assertTrue(is_custom)
self.assertEqual(pid, "3f2a9c1b")
def test_non_custom(self):
is_custom, pid = self.parse("openai")
self.assertFalse(is_custom)
self.assertEqual(pid, "")
def test_empty(self):
is_custom, pid = self.parse("")
self.assertFalse(is_custom)
self.assertEqual(pid, "")
def test_none(self):
is_custom, pid = self.parse(None)
self.assertFalse(is_custom)
self.assertEqual(pid, "")
class TestResolveCustomCredentials(unittest.TestCase):
"""resolve_custom_credentials() resolution order and fallbacks."""
def setUp(self):
# Import here so the module picks up our config-swapping helper.
from models.custom_provider import resolve_custom_credentials, get_custom_providers
self.resolve = resolve_custom_credentials
self.get_providers = get_custom_providers
@@ -49,31 +82,15 @@ class TestResolveCustomCredentials(unittest.TestCase):
set_conf({"bot_type": "custom"})
self.assertEqual(self.resolve(), ("", None, None))
# --- Multi-provider selection ---
# --- Multi-provider selection via bot_type ---
def test_multi_providers_no_active_uses_first(self):
def test_provider_selected_by_id(self):
set_conf({
"bot_type": "custom",
"bot_type": "custom:abc12345",
"custom_providers": [
{"name": "siliconflow", "api_key": "sf-key",
{"id": "sf001", "name": "siliconflow", "api_key": "sf-key",
"api_base": "https://api.siliconflow.cn/v1", "model": "deepseek-ai/DeepSeek-V3"},
{"name": "qiniu", "api_key": "qn-key",
"api_base": "https://api.qnaigc.com/v1", "model": "deepseek-v3"},
],
})
self.assertEqual(
self.resolve(),
("sf-key", "https://api.siliconflow.cn/v1", "deepseek-ai/DeepSeek-V3"),
)
def test_active_provider_selected(self):
set_conf({
"bot_type": "custom",
"custom_active_provider": "qiniu",
"custom_providers": [
{"name": "siliconflow", "api_key": "sf-key",
"api_base": "https://api.siliconflow.cn/v1", "model": "m1"},
{"name": "qiniu", "api_key": "qn-key",
{"id": "abc12345", "name": "qiniu", "api_key": "qn-key",
"api_base": "https://api.qnaigc.com/v1", "model": "deepseek-v3"},
],
})
@@ -82,25 +99,26 @@ class TestResolveCustomCredentials(unittest.TestCase):
("qn-key", "https://api.qnaigc.com/v1", "deepseek-v3"),
)
def test_active_name_missing_falls_back_to_first(self):
def test_id_not_found_falls_back_to_legacy(self):
set_conf({
"bot_type": "custom",
"custom_active_provider": "ghost",
"bot_type": "custom:ghost",
"custom_api_key": "legacy-key",
"custom_api_base": "https://legacy.example.com/v1",
"custom_providers": [
{"name": "siliconflow", "api_key": "sf-key",
{"id": "sf001", "name": "siliconflow", "api_key": "sf-key",
"api_base": "https://api.siliconflow.cn/v1"},
],
})
self.assertEqual(
self.resolve(),
("sf-key", "https://api.siliconflow.cn/v1", None),
("legacy-key", "https://legacy.example.com/v1", None),
)
def test_provider_without_model_returns_none_model(self):
set_conf({
"bot_type": "custom",
"bot_type": "custom:local01",
"custom_providers": [
{"name": "local", "api_key": "", "api_base": "http://localhost:11434/v1"},
{"id": "local01", "name": "local", "api_key": "", "api_base": "http://localhost:11434/v1"},
],
})
self.assertEqual(
@@ -112,12 +130,12 @@ class TestResolveCustomCredentials(unittest.TestCase):
def test_malformed_entries_filtered_and_fallback(self):
set_conf({
"bot_type": "custom",
"bot_type": "custom:nope",
"custom_api_key": "legacy-key",
"custom_api_base": "https://legacy.example.com/v1",
"custom_providers": [
{"api_key": "no-name-key"}, # invalid: no name
"not-a-dict", # invalid: wrong type
{"name": "no-id", "api_key": "no-id-key"}, # invalid: no id
"not-a-dict", # invalid: wrong type
],
})
# All entries invalid -> treated as empty -> legacy fallback
@@ -130,14 +148,14 @@ class TestResolveCustomCredentials(unittest.TestCase):
set_conf({
"bot_type": "custom",
"custom_providers": [
{"name": "ok", "api_key": "k", "api_base": "https://x/v1"},
{"api_key": "no-name"}, # dropped
123, # dropped
{"id": "ok1", "name": "ok", "api_key": "k", "api_base": "https://x/v1"},
{"name": "no-id", "api_key": "no-id"}, # dropped: no id
123, # dropped
],
})
providers = self.get_providers()
self.assertEqual(len(providers), 1)
self.assertEqual(providers[0]["name"], "ok")
self.assertEqual(providers[0]["id"], "ok1")
def test_custom_providers_not_a_list_falls_back(self):
set_conf({
@@ -152,6 +170,22 @@ class TestResolveCustomCredentials(unittest.TestCase):
)
class TestGenerateProviderId(unittest.TestCase):
"""generate_provider_id() produces valid short ids."""
def test_length_and_hex(self):
from models.custom_provider import generate_provider_id
pid = generate_provider_id()
self.assertEqual(len(pid), 8)
# Must be valid hex characters
int(pid, 16)
def test_uniqueness(self):
from models.custom_provider import generate_provider_id
ids = {generate_provider_id() for _ in range(100)}
self.assertEqual(len(ids), 100)
class TestConfigDefaults(unittest.TestCase):
"""The new config fields must exist with safe defaults."""
@@ -160,10 +194,46 @@ class TestConfigDefaults(unittest.TestCase):
self.assertIn("custom_providers", available_setting)
self.assertEqual(available_setting["custom_providers"], [])
def test_default_config_has_active_provider(self):
def test_default_config_no_custom_active_provider(self):
"""custom_active_provider was removed — replaced by bot_type routing."""
from config import available_setting
self.assertIn("custom_active_provider", available_setting)
self.assertEqual(available_setting["custom_active_provider"], "")
self.assertNotIn("custom_active_provider", available_setting)
class TestDragSensitiveNested(unittest.TestCase):
"""drag_sensitive() must mask api_key in nested structures."""
def test_nested_api_key_masked(self):
from config import drag_sensitive
import json
test_config = {
"open_ai_api_key": "sk-1234567890abcdef",
"custom_providers": [
{"id": "x1", "name": "test", "api_key": "sk-nested-secret-key-long", "api_base": "https://x/v1"}
],
}
result = drag_sensitive(test_config)
# Top-level key should be masked
self.assertNotIn("1234567890abcdef", str(result))
# Nested key should also be masked
self.assertNotIn("nested-secret-key-long", str(result))
# But the id/name/api_base should not be masked
self.assertIn("x1", str(result))
self.assertIn("test", str(result))
self.assertIn("https://x/v1", str(result))
def test_string_config_masked(self):
from config import drag_sensitive
import json
test_str = json.dumps({
"open_ai_api_key": "sk-1234567890abcdef",
"custom_providers": [
{"id": "x1", "api_key": "sk-nested-very-long-secret"}
],
})
result = drag_sensitive(test_str)
self.assertNotIn("1234567890abcdef", result)
self.assertNotIn("nested-very-long-secret", result)
if __name__ == "__main__":

View File

@@ -4,14 +4,11 @@ Unit tests for the multi custom-provider management API (issue #2838, web UI).
Covers channel/web/web_channel.py::ModelsHandler:
- _custom_provider_cards / _provider_overview expansion
- _handle_set_custom_provider (create / edit / rename / activate)
- _handle_set_custom_provider (create / edit / activate)
- _handle_delete_custom_provider
- _handle_set_active_custom_provider
These handlers are normally driven by the `web.py` framework, which isn't
available in the headless test environment, so we stub the `web` module before
import. The on-disk config read/write and the Bridge reset are patched to keep
the tests hermetic (no file I/O, no live bot routing).
Uses id-based routing (bot_type: "custom:<id>") — no custom_active_provider.
"""
import json
import os
@@ -72,7 +69,7 @@ class _HandlerHarness:
class TestSetCustomProvider(unittest.TestCase):
def setUp(self):
set_conf({"bot_type": "custom", "custom_providers": [], "custom_active_provider": ""})
set_conf({"bot_type": "custom", "custom_providers": []})
self.h = _HandlerHarness()
def test_create_first_provider_auto_activates(self):
@@ -80,11 +77,15 @@ class TestSetCustomProvider(unittest.TestCase):
api_base="https://api.siliconflow.cn/v1", api_key="sf-key")
self.assertEqual(res["status"], "success")
self.assertTrue(res["created"])
self.assertEqual(res["active"], "siliconflow")
self.assertIn("id", res)
# bot_type should be updated to "custom:<id>"
bot_type = config_module.conf().get("bot_type")
self.assertTrue(bot_type.startswith("custom:"))
self.assertEqual(bot_type, f"custom:{res['id']}")
providers = config_module.conf().get("custom_providers")
self.assertEqual(len(providers), 1)
self.assertEqual(providers[0]["id"], res["id"])
self.assertEqual(providers[0]["name"], "siliconflow")
self.assertEqual(config_module.conf().get("custom_active_provider"), "siliconflow")
self.assertEqual(self.h.bridge_resets, 1)
def test_create_requires_api_base(self):
@@ -97,161 +98,158 @@ class TestSetCustomProvider(unittest.TestCase):
self.assertEqual(res["status"], "error")
def test_second_provider_does_not_steal_active(self):
self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak")
res = self.h.call(action="set_custom_provider", name="b",
api_base="https://b/v1", api_key="bk")
self.assertTrue(res["created"])
res1 = self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak")
res2 = self.h.call(action="set_custom_provider", name="b",
api_base="https://b/v1", api_key="bk")
self.assertTrue(res2["created"])
# First provider stays active unless make_active is requested.
self.assertEqual(config_module.conf().get("custom_active_provider"), "a")
bot_type = config_module.conf().get("bot_type")
self.assertEqual(bot_type, f"custom:{res1['id']}")
def test_make_active_flag(self):
self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak")
self.h.call(action="set_custom_provider", name="b",
api_base="https://b/v1", api_key="bk", make_active=True)
self.assertEqual(config_module.conf().get("custom_active_provider"), "b")
def test_duplicate_name_rejected(self):
self.h.call(action="set_custom_provider", name="dup",
api_base="https://a/v1", api_key="ak")
res = self.h.call(action="set_custom_provider", name="dup",
api_base="https://b/v1", api_key="bk")
self.assertEqual(res["status"], "error")
self.assertIn("already exists", res["message"])
# The original entry must be untouched.
providers = config_module.conf().get("custom_providers")
self.assertEqual(len(providers), 1)
self.assertEqual(providers[0]["api_base"], "https://a/v1")
res2 = self.h.call(action="set_custom_provider", name="b",
api_base="https://b/v1", api_key="bk", make_active=True)
bot_type = config_module.conf().get("bot_type")
self.assertEqual(bot_type, f"custom:{res2['id']}")
def test_edit_keeps_key_when_omitted(self):
self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="secret")
# Edit only the base; omit api_key.
res = self.h.call(action="set_custom_provider", name="a",
original_name="a", api_base="https://a2/v1")
self.assertEqual(res["status"], "success")
self.assertFalse(res["created"])
api_base="https://a/v1", api_key="secret")
pid = res["id"]
# Edit only the base; omit api_key.
res2 = self.h.call(action="set_custom_provider", name="a",
id=pid, api_base="https://a2/v1")
self.assertEqual(res2["status"], "success")
self.assertFalse(res2["created"])
providers = config_module.conf().get("custom_providers")
self.assertEqual(providers[0]["api_base"], "https://a2/v1")
self.assertEqual(providers[0]["api_key"], "secret") # preserved
def test_rename_updates_active_pointer(self):
self.h.call(action="set_custom_provider", name="old",
api_base="https://a/v1", api_key="ak")
self.assertEqual(config_module.conf().get("custom_active_provider"), "old")
res = self.h.call(action="set_custom_provider", name="new",
original_name="old", api_base="https://a/v1")
self.assertEqual(res["status"], "success")
self.assertEqual(config_module.conf().get("custom_active_provider"), "new")
names = [p["name"] for p in config_module.conf().get("custom_providers")]
self.assertEqual(names, ["new"])
def test_edit_can_rename(self):
res = self.h.call(action="set_custom_provider", name="old",
api_base="https://a/v1", api_key="ak")
pid = res["id"]
res2 = self.h.call(action="set_custom_provider", name="new",
id=pid, api_base="https://a/v1")
self.assertEqual(res2["status"], "success")
providers = config_module.conf().get("custom_providers")
self.assertEqual(providers[0]["name"], "new")
# ID stays the same
self.assertEqual(providers[0]["id"], pid)
def test_edit_clears_model_when_empty(self):
self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak", model="m1")
res = self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak", model="m1")
pid = res["id"]
self.assertEqual(config_module.conf().get("custom_providers")[0]["model"], "m1")
self.h.call(action="set_custom_provider", name="a", original_name="a",
self.h.call(action="set_custom_provider", name="a", id=pid,
api_base="https://a/v1", model="")
self.assertNotIn("model", config_module.conf().get("custom_providers")[0])
class TestDeleteCustomProvider(unittest.TestCase):
def setUp(self):
set_conf({"bot_type": "custom", "custom_providers": [], "custom_active_provider": ""})
set_conf({"bot_type": "custom", "custom_providers": []})
self.h = _HandlerHarness()
self.h.call(action="set_custom_provider", name="a", api_base="https://a/v1", api_key="ak")
self.h.call(action="set_custom_provider", name="b", api_base="https://b/v1", api_key="bk")
self.res_a = self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak")
self.res_b = self.h.call(action="set_custom_provider", name="b",
api_base="https://b/v1", api_key="bk")
def test_delete_unknown(self):
res = self.h.call(action="delete_custom_provider", name="ghost")
res = self.h.call(action="delete_custom_provider", id="ghost")
self.assertEqual(res["status"], "error")
def test_delete_non_active(self):
res = self.h.call(action="delete_custom_provider", name="b")
res = self.h.call(action="delete_custom_provider", id=self.res_b["id"])
self.assertEqual(res["status"], "success")
names = [p["name"] for p in config_module.conf().get("custom_providers")]
self.assertEqual(names, ["a"])
self.assertEqual(config_module.conf().get("custom_active_provider"), "a")
ids = [p["id"] for p in config_module.conf().get("custom_providers")]
self.assertEqual(ids, [self.res_a["id"]])
# bot_type unchanged (still pointing to a)
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{self.res_a['id']}")
def test_delete_active_falls_back_to_first_remaining(self):
# 'a' is active (created first); deleting it should re-point to 'b'.
self.assertEqual(config_module.conf().get("custom_active_provider"), "a")
res = self.h.call(action="delete_custom_provider", name="a")
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{self.res_a['id']}")
res = self.h.call(action="delete_custom_provider", id=self.res_a["id"])
self.assertEqual(res["status"], "success")
self.assertEqual(config_module.conf().get("custom_active_provider"), "b")
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{self.res_b['id']}")
def test_delete_last_clears_active(self):
self.h.call(action="delete_custom_provider", name="a")
self.h.call(action="delete_custom_provider", name="b")
def test_delete_last_reverts_to_legacy(self):
self.h.call(action="delete_custom_provider", id=self.res_a["id"])
self.h.call(action="delete_custom_provider", id=self.res_b["id"])
self.assertEqual(config_module.conf().get("custom_providers"), [])
self.assertEqual(config_module.conf().get("custom_active_provider"), "")
# When all providers deleted, reverts to legacy "custom"
self.assertEqual(config_module.conf().get("bot_type"), "custom")
class TestSetActiveCustomProvider(unittest.TestCase):
def setUp(self):
set_conf({"bot_type": "custom", "custom_providers": [], "custom_active_provider": ""})
set_conf({"bot_type": "custom", "custom_providers": []})
self.h = _HandlerHarness()
self.h.call(action="set_custom_provider", name="a", api_base="https://a/v1", api_key="ak")
self.h.call(action="set_custom_provider", name="b", api_base="https://b/v1", api_key="bk")
self.res_a = self.h.call(action="set_custom_provider", name="a",
api_base="https://a/v1", api_key="ak")
self.res_b = self.h.call(action="set_custom_provider", name="b",
api_base="https://b/v1", api_key="bk")
def test_set_active_valid(self):
res = self.h.call(action="set_active_custom_provider", name="b")
res = self.h.call(action="set_active_custom_provider", id=self.res_b["id"])
self.assertEqual(res["status"], "success")
self.assertEqual(config_module.conf().get("custom_active_provider"), "b")
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{self.res_b['id']}")
def test_set_active_unknown(self):
res = self.h.call(action="set_active_custom_provider", name="ghost")
res = self.h.call(action="set_active_custom_provider", id="ghost")
self.assertEqual(res["status"], "error")
self.assertEqual(config_module.conf().get("custom_active_provider"), "a")
# bot_type unchanged
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{self.res_a['id']}")
class TestProviderOverviewExpansion(unittest.TestCase):
"""_provider_overview / _custom_provider_cards should expand the list."""
def test_no_custom_providers_keeps_single_card(self):
set_conf({"bot_type": "custom", "custom_providers": [], "custom_active_provider": ""})
set_conf({"bot_type": "custom", "custom_providers": []})
cards = ModelsHandler._custom_provider_cards(config_module.conf())
self.assertEqual(cards, [])
overview = ModelsHandler._provider_overview()
custom_cards = [c for c in overview if c.get("id") == "custom"]
# Legacy single custom card remains present.
self.assertEqual(len(custom_cards), 1)
self.assertTrue(custom_cards[0].get("is_custom"))
def test_multi_providers_expand_into_cards(self):
set_conf({
"bot_type": "custom",
"custom_active_provider": "b",
"bot_type": "custom:id_b",
"custom_providers": [
{"name": "a", "api_key": "ak", "api_base": "https://a/v1"},
{"name": "b", "api_key": "bk", "api_base": "https://b/v1", "model": "m"},
{"id": "id_a", "name": "a", "api_key": "ak", "api_base": "https://a/v1"},
{"id": "id_b", "name": "b", "api_key": "bk", "api_base": "https://b/v1", "model": "m"},
],
})
overview = ModelsHandler._provider_overview()
custom_cards = [c for c in overview if c.get("is_custom")]
self.assertEqual(len(custom_cards), 2)
by_name = {c["custom_name"]: c for c in custom_cards}
self.assertEqual(by_name["a"]["id"], "custom:a")
self.assertFalse(by_name["a"]["active"])
self.assertTrue(by_name["b"]["active"])
self.assertEqual(by_name["b"]["model"], "m")
by_id = {c["custom_id"]: c for c in custom_cards}
self.assertEqual(by_id["id_a"]["id"], "custom:id_a")
self.assertFalse(by_id["id_a"]["active"])
self.assertTrue(by_id["id_b"]["active"])
self.assertEqual(by_id["id_b"]["model"], "m")
# No single legacy "custom" card when expanded.
self.assertFalse(any(c.get("id") == "custom" for c in overview))
def test_active_defaults_to_first_when_unset(self):
def test_no_active_shows_none_active(self):
"""When bot_type is plain 'custom', no card is marked active."""
set_conf({
"bot_type": "custom",
"custom_active_provider": "",
"custom_providers": [
{"name": "a", "api_key": "ak", "api_base": "https://a/v1"},
{"name": "b", "api_key": "bk", "api_base": "https://b/v1"},
{"id": "id_a", "name": "a", "api_key": "ak", "api_base": "https://a/v1"},
{"id": "id_b", "name": "b", "api_key": "bk", "api_base": "https://b/v1"},
],
})
cards = ModelsHandler._custom_provider_cards(config_module.conf())
by_name = {c["custom_name"]: c for c in cards}
self.assertTrue(by_name["a"]["active"])
self.assertFalse(by_name["b"]["active"])
active_cards = [c for c in cards if c.get("active")]
self.assertEqual(len(active_cards), 0)
if __name__ == "__main__":