diff --git a/backend/apps/model_managment_app.py b/backend/apps/model_managment_app.py index 71855d72c..4a8914e77 100644 --- a/backend/apps/model_managment_app.py +++ b/backend/apps/model_managment_app.py @@ -123,14 +123,21 @@ def dump_full_catalog(): _LOG_UNSAFE_CHARS = re.compile(r"[\x00-\x1f\x7f]") +# Credential-bearing columns that must never reach an HTTP client. STT/TTS +# voice models authenticate with the (model_appid, access_token) pair instead +# of (or in addition to) api_key, so all three are stripped together. +_MODEL_CREDENTIAL_FIELDS = frozenset({"api_key", "model_appid", "access_token"}) + + def _sanitize_model_credentials(payload: Any) -> Any: - """Remove model API keys before returning model data to HTTP clients. + """Remove model credentials before returning model data to HTTP clients. Model records are also consumed by internal services, so credential removal belongs at the HTTP response boundary rather than in the database or model-management service layer. The presence of a configured key is intentionally not returned; callers that need to update a model can omit - ``api_key`` to keep the existing value. + ``api_key`` / ``model_appid`` / ``access_token`` to keep the existing + value (the update service drops empty-string values for these fields). """ if isinstance(payload, list): return [_sanitize_model_credentials(item) for item in payload] @@ -139,7 +146,7 @@ def _sanitize_model_credentials(payload: Any) -> Any: return { key: _sanitize_model_credentials(value) for key, value in payload.items() - if key != "api_key" + if key not in _MODEL_CREDENTIAL_FIELDS } return payload diff --git a/backend/services/config_sync_service.py b/backend/services/config_sync_service.py index ffb252185..5a5ff648e 100644 --- a/backend/services/config_sync_service.py +++ b/backend/services/config_sync_service.py @@ -188,6 +188,15 @@ def build_models_config(tenant_id: str) -> dict: def build_model_config(model_config: dict) -> dict: + """Build the /config/load_config model entry for one default-model slot. + + Credential values (api_key, and the STT/TTS model_appid / access_token + pair) are intentionally NOT included: load_config responses go to every + logged-in user, so leaking them would expose tenant-level provider + credentials. The frontend model page never displays or resubmits these + fields from this payload (model management submits credential updates + only through the /model/* endpoints), so nothing breaks by omitting them. + """ if not model_config: return { "id": None, @@ -204,7 +213,7 @@ def build_model_config(model_config: dict) -> dict: "name": get_model_name_from_config(model_config) if model_config else "", "displayName": model_config.get("display_name", ""), "apiConfig": { - "apiKey": model_config.get("api_key", ""), + "apiKey": "", "modelUrl": model_config.get("base_url", "") } } @@ -212,11 +221,13 @@ def build_model_config(model_config: dict) -> dict: if "embedding" in model_config.get("model_type", ""): config["dimension"] = model_config.get("max_tokens", 0) - # Add voice model specific fields (STT and TTS) + # Add voice model specific fields (STT and TTS). model_appid and + # access_token are credentials (the Volcano auth pair) and are omitted; + # the operator edits them from the model-management dialog instead. model_type = model_config.get("model_type", "") if model_type == "stt" or model_type == "tts": config["modelFactory"] = model_config.get("model_factory", "") - config["modelAppid"] = model_config.get("model_appid", "") - config["accessToken"] = model_config.get("access_token", "") + config["modelAppid"] = "" + config["accessToken"] = "" return config diff --git a/backend/services/model_management_service.py b/backend/services/model_management_service.py index 039145e8e..89da38914 100644 --- a/backend/services/model_management_service.py +++ b/backend/services/model_management_service.py @@ -924,6 +924,16 @@ async def update_single_model_for_tenant( else: model_data["ssl_verify"] = True + # Keep-existing semantics for voice-model credentials. List endpoints + # sanitize model_appid / access_token before returning records, so an + # edit dialog that the operator did not touch submits empty strings + # for these fields. Drop them instead of overwriting the stored + # values -- mirroring the api_key "empty means keep" contract that + # the frontend already relies on. + for credential_field in ("model_appid", "access_token"): + if credential_field in model_data and not model_data[credential_field]: + model_data.pop(credential_field) + # Carry model_type from the existing record so the legacy-alias # coercion can distinguish LLM/VLM updates from embedding updates # even when the caller payload omits model_type. We don't store the diff --git a/frontend/app/[locale]/models/components/modelConfig.tsx b/frontend/app/[locale]/models/components/modelConfig.tsx index 1e323d8c8..7842a893d 100644 --- a/frontend/app/[locale]/models/components/modelConfig.tsx +++ b/frontend/app/[locale]/models/components/modelConfig.tsx @@ -321,11 +321,14 @@ export const ModelConfigSection = forwardRef< for (const m of group.models) { try { // Partial update: only api_key + base_url are sent; the backend - // leaves every other field untouched. + // leaves every other field untouched. An empty key means "keep the + // existing credential" (list responses never carry stored keys), so + // it must be omitted rather than sent as an empty string -- sending + // "" would overwrite the stored key. await modelService.updateSingleModel({ currentDisplayName: m.displayName, url: patch.url, - apiKey: patch.apiKey, + ...(patch.apiKey.trim() ? { apiKey: patch.apiKey.trim() } : {}), source: m.source, }); } catch (e: any) { diff --git a/test/backend/app/test_model_managment_app.py b/test/backend/app/test_model_managment_app.py index 9dc707446..a179035b2 100644 --- a/test/backend/app/test_model_managment_app.py +++ b/test/backend/app/test_model_managment_app.py @@ -675,6 +675,63 @@ async def mock_list_models(*args, **kwargs): mock_list.assert_called_once_with(user_credentials[1]) +@pytest.mark.asyncio +async def test_get_model_list_strips_voice_model_credentials(client, auth_header, user_credentials, mocker): + """Voice-model credentials (model_appid / access_token) must never reach + an HTTP client, exactly like api_key: the pair is the full auth material + for Volcano Engine STT/TTS models.""" + mocker.patch('backend.apps.model_managment_app.get_current_user_id', return_value=user_credentials) + + async def mock_list_models(*args, **kwargs): + return [ + { + "model_id": "stt1", + "model_name": "volc/stt-bigmodel", + "display_name": "Volc STT", + "model_type": "stt", + "api_key": "stored-secret", + "model_appid": "stored-appid", + "access_token": "stored-token", + "connect_status": "operational", + } + ] + + mocker.patch('backend.apps.model_managment_app.list_models_for_tenant', side_effect=mock_list_models) + + response = client.get("/model/list", headers=auth_header) + + assert response.status_code == HTTPStatus.OK + record = response.json()["data"][0] + assert record["model_name"] == "volc/stt-bigmodel" + for credential_field in ("api_key", "model_appid", "access_token"): + assert credential_field not in record + + +def test_sanitize_model_credentials_strips_all_credential_fields(): + """Unit-level contract: api_key, model_appid and access_token are removed + from dicts at any nesting depth; all other fields pass through.""" + from backend.apps.model_managment_app import _sanitize_model_credentials + + payload = { + "model_id": 1, + "api_key": "sk-secret", + "model_appid": "appid-secret", + "access_token": "token-secret", + "nested": { + "api_key": "sk-nested", + "model_appid": "appid-nested", + "keep": "me", + }, + } + + sanitized = _sanitize_model_credentials(payload) + + assert sanitized == { + "model_id": 1, + "nested": {"keep": "me"}, + } + + # Tests for /model/llm_list endpoint @pytest.mark.asyncio async def test_get_llm_model_list_success(client, auth_header, user_credentials, mocker): diff --git a/test/backend/services/test_config_sync_service.py b/test/backend/services/test_config_sync_service.py index 5e5e4ee7e..5b10a32c4 100644 --- a/test/backend/services/test_config_sync_service.py +++ b/test/backend/services/test_config_sync_service.py @@ -1246,7 +1246,7 @@ def side_effect(config_key, tenant_id=None): # Verify successful config assert result["llm"]["displayName"] == "Test LLM" - assert result["llm"]["apiConfig"]["apiKey"] == "test-api-key" + assert result["llm"]["apiConfig"]["apiKey"] == "" # sanitized # Verify failed config was handled gracefully assert result["embedding"]["name"] == "" @@ -1300,7 +1300,7 @@ def side_effect(config_key, tenant_id=None): # Verify successful configs assert result["llm"]["displayName"] == "GPT-4" - assert result["llm"]["apiConfig"]["apiKey"] == "test-key" + assert result["llm"]["apiConfig"]["apiKey"] == "" # sanitized # Verify no warnings were logged (all successful) service_mocks['logger'].warning.assert_not_called() @@ -1642,7 +1642,7 @@ def test_build_model_config_non_embedding_model(self, service_mocks): # Assert assert result["name"] == "gpt-4" assert result["displayName"] == "GPT-4" - assert result["apiConfig"]["apiKey"] == "test-api-key" + assert result["apiConfig"]["apiKey"] == "" # sanitized assert result["apiConfig"]["modelUrl"] == "https://api.openai.com" # Should not have dimension field for non-embedding models assert "dimension" not in result @@ -1667,7 +1667,7 @@ def test_build_model_config_embedding_model(self, service_mocks): # Assert assert result["name"] == "text-embedding-ada-002" assert result["displayName"] == "Ada Embeddings" - assert result["apiConfig"]["apiKey"] == "test-api-key" + assert result["apiConfig"]["apiKey"] == "" # sanitized assert result["apiConfig"]["modelUrl"] == "https://api.openai.com" # Should have dimension field for embedding models assert result["dimension"] == 1536 @@ -1692,7 +1692,7 @@ def test_build_model_config_multi_embedding_model(self, service_mocks): # Assert assert result["name"] == "text-embedding-3-small" assert result["displayName"] == "Multi Ada Embeddings" - assert result["apiConfig"]["apiKey"] == "test-api-key" + assert result["apiConfig"]["apiKey"] == "" # sanitized assert result["apiConfig"]["modelUrl"] == "https://api.openai.com" # Should have dimension field for multi_embedding models assert result["dimension"] == 768 @@ -1739,7 +1739,7 @@ def test_build_model_config_embedding_without_max_tokens(self, service_mocks): # Assert assert result["name"] == "test-embedding" assert result["displayName"] == "Test Embedding" - assert result["apiConfig"]["apiKey"] == "test-key" + assert result["apiConfig"]["apiKey"] == "" # sanitized assert result["apiConfig"]["modelUrl"] == "https://test.com" # Should have dimension field with default value 0 assert result["dimension"] == 0 @@ -1763,7 +1763,7 @@ def test_build_model_config_model_type_partial_match(self, service_mocks): # Assert assert result["name"] == "test-model" assert result["displayName"] == "Test Model" - assert result["apiConfig"]["apiKey"] == "test-key" + assert result["apiConfig"]["apiKey"] == "" # sanitized # Should have dimension since model_type contains 'embedding' assert result["dimension"] == 512 diff --git a/test/backend/services/test_config_sync_service_voice.py b/test/backend/services/test_config_sync_service_voice.py index dba5b8763..4e2b034f5 100644 --- a/test/backend/services/test_config_sync_service_voice.py +++ b/test/backend/services/test_config_sync_service_voice.py @@ -197,8 +197,11 @@ def test_build_model_config_stt(self, service_mocks): result = build_model_config(model_config) assert result["modelFactory"] == "volc" - assert result["modelAppid"] == "stt_appid" - assert result["accessToken"] == "stt_token" + # Voice credentials are sanitized before reaching HTTP clients: + # model_appid / access_token must never be echoed back. + assert result["modelAppid"] == "" + assert result["accessToken"] == "" + assert result["apiConfig"]["apiKey"] == "" def test_build_model_config_stt_empty_fields(self, service_mocks): """Test build_model_config with STT model and empty voice fields.""" diff --git a/test/backend/services/test_model_management_service.py b/test/backend/services/test_model_management_service.py index 33ee12c35..ccaf68a72 100644 --- a/test/backend/services/test_model_management_service.py +++ b/test/backend/services/test_model_management_service.py @@ -1237,6 +1237,57 @@ async def test_update_single_model_for_tenant_success_single_model(): ) +async def test_update_single_model_for_tenant_empty_voice_credentials_keep_existing(): + """Empty-string model_appid / access_token must be dropped from the update + payload instead of overwriting the stored values. + + List endpoints sanitize these fields, so an edit dialog the operator did + not touch submits empty strings; the "empty means keep" contract mirrors + api_key handling.""" + svc = import_svc() + + existing_models = [ + {"model_id": 9, "model_type": "stt", "display_name": "volc-stt"}, + ] + model_data = { + "model_id": 9, + "display_name": "volc-stt", + "model_appid": "", + "access_token": "", + } + + with mock.patch.object(svc, "get_models_by_display_name", return_value=existing_models), \ + mock.patch.object(svc, "update_model_record") as mock_update: + await svc.update_single_model_for_tenant("u1", "t1", "volc-stt", model_data) + + call_payload = mock_update.call_args[0][1] + assert "model_appid" not in call_payload + assert "access_token" not in call_payload + + +async def test_update_single_model_for_tenant_new_voice_credentials_are_written(): + """Non-empty model_appid / access_token pass through to the update call.""" + svc = import_svc() + + existing_models = [ + {"model_id": 9, "model_type": "stt", "display_name": "volc-stt"}, + ] + model_data = { + "model_id": 9, + "display_name": "volc-stt", + "model_appid": "new-appid", + "access_token": "new-token", + } + + with mock.patch.object(svc, "get_models_by_display_name", return_value=existing_models), \ + mock.patch.object(svc, "update_model_record") as mock_update: + await svc.update_single_model_for_tenant("u1", "t1", "volc-stt", model_data) + + call_payload = mock_update.call_args[0][1] + assert call_payload["model_appid"] == "new-appid" + assert call_payload["access_token"] == "new-token" + + async def test_update_single_model_for_tenant_splits_repo_prefix_from_model_name(): """A repo-qualified model_name (as returned by the list endpoints) must be split on update, otherwise the model_name column accumulates a repo prefix