From 49deb27becc31d7a76a79d760d27ae623b20105d Mon Sep 17 00:00:00 2001 From: Davidson Gomes Date: Wed, 29 Jul 2026 13:51:19 -0300 Subject: [PATCH 1/7] feat(external-agents): resolve credencial da plataforma pelo cofre, com fallback inline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O builder de agente externo passa a resolver o segredo pelo cofre quando a integração carrega `credential_id`, e a manter o valor inline quando não carrega. Nenhuma instalação precisa migrar nada para esta mudança entrar: o caminho antigo continua intacto até a 2.7 remover o fallback. A resolução aqui é POR ID, deliberadamente. Precedência entre escopos tem um dono só, o resolvedor do CRM (story 2.2); percorrer cadeia no runtime criaria uma segunda verdade sobre qual credencial vence. O que o processor faz é buscar valor, não decidir hierarquia. O mapa de campo por provedor fica num lugar só, porque errar um nome aqui produz auth vazia em silêncio, não erro: dify, flowise e openai leem `apiKey`; n8n lê o par `basicAuthUser`/`basicAuthPass`, que vem do envelope composto do cofre e precisa ser traduzido; typebot não tem credencial nenhuma e está registrado com tupla vazia, para a ausência ler como decisão e não como esquecimento que alguém tenta "consertar" depois. Ordem de precedência: referência resolvida vence, senão inline, senão erro explícito. O erro só dispara quando o usuário PEDIU o cofre e não há inline para usar: mandar chave vazia ao provedor falharia mais longe e com mensagem pior. A razão da falha viaja na mensagem, porque "é oauth" e "não existe" pedem correções diferentes de quem configurou o agente. O vault lê a tabela pela sessão que o builder já tem, com query parametrizada e escopada a uma linha. Não abre conexão própria: o padrão de psycopg2 cru que existe nas tools de Calendar ignora ORM e tenant, e a própria story manda não copiá-lo. Prova negativa verificada: removendo o fallback inline, três testes falham (referência não resolvida, referência oauth e valor indecifrável). Nota: o módulo é livre de imports pesados de propósito, e o teste o importa por caminho, porque `src.services.__init__` puxa a stack ADK inteira e o ambiente local não tem essas dependências instaladas. Refs EVO-2250 (story 2.3) --- .../adk/agents/external_agent_builder.py | 15 ++ src/services/adk/integration_credentials.py | 191 +++++++++++++++ .../test_integration_credential_resolution.py | 224 ++++++++++++++++++ 3 files changed, 430 insertions(+) create mode 100644 src/services/adk/integration_credentials.py create mode 100644 tests/unit/services/test_integration_credential_resolution.py diff --git a/src/services/adk/agents/external_agent_builder.py b/src/services/adk/agents/external_agent_builder.py index c94e708..3af2b86 100644 --- a/src/services/adk/agents/external_agent_builder.py +++ b/src/services/adk/agents/external_agent_builder.py @@ -7,6 +7,11 @@ from src.utils.logger import setup_logger from src.services.adk.agents.external_agent import ExternalAgent from src.services.agent_service import get_agent_integration_by_provider +from src.services.adk.integration_credentials import ( + DatabaseCredentialVault, + apply_vault_credential, +) +from src.utils.crypto import decrypt_api_key from sqlalchemy.orm import Session logger = setup_logger(__name__) @@ -54,6 +59,16 @@ async def build_external_agent( "Please configure the integration first." ) + # EVO-2250 story 2.3: when the integration points at the credential + # vault, the secret comes from there; otherwise the inline value is + # used exactly as before, so nothing breaks before the migration. + integration_config = apply_vault_credential( + provider, + integration_config, + vault=DatabaseCredentialVault(self.db), + decrypt=decrypt_api_key, + ) + # Get sub-agents if there are any sub_agents = [] if root_agent.config.get("sub_agents"): diff --git a/src/services/adk/integration_credentials.py b/src/services/adk/integration_credentials.py new file mode 100644 index 0000000..75c96ca --- /dev/null +++ b/src/services/adk/integration_credentials.py @@ -0,0 +1,191 @@ +"""Resolves an external agent's credential from the integration vault. + +EVO-2250, story 2.3. The vault (`evo_core_integration_credentials`, story 2.1) +holds the secret encrypted; the agent's integration config points at it by +`credential_id`. + +Two rules this module exists to keep: + +1. **Resolution here is BY ID only.** Precedence between scopes has a single + owner, the CRM resolver of story 2.2. Walking a chain here would create a + second truth about which credential wins. +2. **The inline value stays the fallback.** Nothing breaks before the 2.6 + migration runs: an unresolvable reference falls back to the inline secret, + and only fails when there is nothing to fall back to. + +Deliberately free of heavy imports so it can be unit tested without the ADK +stack, and the database and crypto handles are injected by the caller. +""" + +import json +import logging +from typing import Any, Callable, Dict, Optional, Protocol, Tuple + +logger = logging.getLogger(__name__) + +KIND_OAUTH = "oauth" +VALUE_FORMAT_COMPOSITE = "composite" + +# The composite envelope of story 2.1 keys the secret half as `password`. +COMPOSITE_SECRET_FIELD = "password" +COMPOSITE_PUBLIC_FIELD = "user" + +# Which config field each provider reads its secret from. The provider services +# keep their current shape, so the vault value is merged into the field they +# already know: mismatching a name here produces empty auth silently instead of +# an error, which is why every entry is asserted in the tests. +SECRET_FIELDS_BY_PROVIDER: Dict[str, Tuple[str, ...]] = { + "dify": ("apiKey",), + "flowise": ("apiKey",), + "openai": ("apiKey",), + # n8n splits the indivisible pair into the two fields it reads. + "n8n": ("basicAuthUser", "basicAuthPass"), + # Typebot authenticates with nothing at all. Registered explicitly so its + # absence reads as a decision, not as an oversight someone should "fix". + "typebot": (), +} + + +class CredentialVault(Protocol): + """Reads an ACTIVE credential row, or nothing.""" + + def fetch_active(self, credential_id: str) -> Optional[Dict[str, Any]]: ... + + +class DatabaseCredentialVault: + """Reads the vault through the session the caller already has. + + Parameterized and scoped to one row: it deliberately does NOT open its own + connection, unlike the raw-psycopg2 pattern some tools use, which bypasses + both the ORM and the tenant GUC. + """ + + def __init__(self, db): + self.db = db + + def fetch_active(self, credential_id: str) -> Optional[Dict[str, Any]]: + from sqlalchemy import text + + try: + row = self.db.execute( + text( + "SELECT kind, value, value_format " + "FROM evo_core_integration_credentials " + "WHERE id = :id AND is_active = true LIMIT 1" + ), + {"id": str(credential_id)}, + ).fetchone() + except Exception as exc: # noqa: BLE001 - a vault outage falls back to inline, it does not crash the agent + logger.error("Failed to read integration credential %s: %s", credential_id, exc) + return None + + if not row: + return None + + return {"kind": row[0], "value": row[1], "value_format": row[2]} + + +def apply_vault_credential( + provider: str, + config: Dict[str, Any], + vault: CredentialVault, + decrypt: Callable[[str], Optional[str]], +) -> Dict[str, Any]: + """Returns a config whose secret fields come from the vault when a usable + reference is present, and from the inline value otherwise. + + Raises ValueError only when the caller asked for a vault credential, it + could not be resolved, and there is no inline value to use: sending an empty + secret to the provider would fail further away, with a worse message. + """ + resolved = dict(config) + + secret_fields = SECRET_FIELDS_BY_PROVIDER.get(provider, ("apiKey",)) + if not secret_fields: + return resolved + + credential_id = resolved.get("credential_id") + if not credential_id: + return resolved + + secret, reason = _fetch_secret(credential_id, vault, decrypt) + if secret is None: + if _has_inline_secret(resolved, secret_fields): + logger.warning( + "Integration credential %s could not be resolved (%s); falling back to the inline value", + credential_id, + reason, + ) + return resolved + # The reason travels into the message: "it is an oauth credential" and + # "it does not exist" call for different fixes from whoever configured + # the agent. + raise ValueError( + f"credential_id {credential_id} could not be resolved to a usable " + f"integration credential ({reason}), and provider '{provider}' has " + "no inline value to fall back to" + ) + + return _merge_secret(resolved, provider, secret_fields, secret, credential_id) + + +def _fetch_secret( + credential_id: str, + vault: CredentialVault, + decrypt: Callable[[str], Optional[str]], +) -> Tuple[Optional[str], str]: + """Returns the plaintext and, when there is none, why.""" + row = vault.fetch_active(credential_id) + if not row: + return None, "no active credential with that id" + + # An oauth row holds no value: the vault points at the store that owns the + # token instead of copying it, and its value column is NULL by CHECK. + if row.get("kind") == KIND_OAUTH: + return None, "it is an oauth credential, which holds no value" + + try: + plaintext = decrypt(row.get("value") or "") + except Exception as exc: # noqa: BLE001 - an undecryptable secret is a fallback case, not a crash + logger.error("Failed to decrypt integration credential %s: %s", credential_id, exc) + return None, "the stored value could not be decrypted" + + if not plaintext: + return None, "the stored value could not be decrypted" + + return plaintext, "" + + +def _merge_secret( + resolved: Dict[str, Any], + provider: str, + secret_fields: Tuple[str, ...], + secret: str, + credential_id: str, +) -> Dict[str, Any]: + if len(secret_fields) == 1: + resolved[secret_fields[0]] = secret + return resolved + + # A pair: the vault stores it as one envelope, the provider reads two + # fields. + try: + envelope = json.loads(secret) + public = envelope[COMPOSITE_PUBLIC_FIELD] + private = envelope[COMPOSITE_SECRET_FIELD] + except (ValueError, KeyError, TypeError) as exc: + logger.error( + "Integration credential %s is not a usable composite envelope (%s); keeping the inline value", + credential_id, + exc, + ) + return resolved + + public_field, private_field = secret_fields + resolved[public_field] = public + resolved[private_field] = private + return resolved + + +def _has_inline_secret(config: Dict[str, Any], secret_fields: Tuple[str, ...]) -> bool: + return any(config.get(field) for field in secret_fields) diff --git a/tests/unit/services/test_integration_credential_resolution.py b/tests/unit/services/test_integration_credential_resolution.py new file mode 100644 index 0000000..b039c67 --- /dev/null +++ b/tests/unit/services/test_integration_credential_resolution.py @@ -0,0 +1,224 @@ +"""Vault resolution for external agent integrations (EVO-2250, story 2.3). + +The runtime resolves a credential BY ID only. Precedence between scopes has a +single owner in the CRM (Ai::IntegrationCredentialResolver, story 2.2), and +duplicating it here would create a second truth about which credential wins. +""" + +import importlib.util +import pathlib + +import pytest + +# Imported by path on purpose: `src.services.__init__` pulls in the whole ADK +# stack, so importing the module normally would drag google-adk and the database +# into a unit test that needs neither. +_MODULE_PATH = ( + pathlib.Path(__file__).resolve().parents[3] + / "src" + / "services" + / "adk" + / "integration_credentials.py" +) +_spec = importlib.util.spec_from_file_location("integration_credentials", _MODULE_PATH) +_module = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_module) + +SECRET_FIELDS_BY_PROVIDER = _module.SECRET_FIELDS_BY_PROVIDER +apply_vault_credential = _module.apply_vault_credential + + +class StubVault: + """Stands in for the credentials table.""" + + def __init__(self, rows=None): + self.rows = rows or {} + self.asked = [] + + def fetch_active(self, credential_id): + self.asked.append(credential_id) + return self.rows.get(credential_id) + + +def vault_row(value, kind="static", value_format="scalar"): + return {"kind": kind, "value": value, "value_format": value_format} + + +def test_dify_takes_the_secret_from_the_vault(): + vault = StubVault({"cred-1": vault_row("cipher")}) + config = {"credential_id": "cred-1", "apiUrl": "https://dify.example.com"} + + resolved = apply_vault_credential( + "dify", config, vault=vault, decrypt=lambda _: "app-dify-9c1d" + ) + + assert resolved["apiKey"] == "app-dify-9c1d" + assert resolved["apiUrl"] == "https://dify.example.com" + + +def test_openai_uses_the_same_secret_field(): + vault = StubVault({"cred-1": vault_row("cipher")}) + + resolved = apply_vault_credential( + "openai", + {"credential_id": "cred-1", "assistantId": "asst_1"}, + vault=vault, + decrypt=lambda _: "sk-openai", + ) + + assert resolved["apiKey"] == "sk-openai" + assert resolved["assistantId"] == "asst_1" + + +def test_n8n_composite_becomes_the_basic_auth_pair(): + """The vault stores an indivisible pair; n8n reads two distinct fields. + + A mismatch here produces empty basic auth silently instead of an error, + which is why the field names are asserted explicitly. + """ + vault = StubVault({"cred-1": vault_row("cipher", value_format="composite")}) + + resolved = apply_vault_credential( + "n8n", + {"credential_id": "cred-1", "webhookUrl": "https://n8n.example.com/hook"}, + vault=vault, + decrypt=lambda _: '{"user": "admin", "password": "s3nha-f9b2"}', + ) + + assert resolved["basicAuthUser"] == "admin" + assert resolved["basicAuthPass"] == "s3nha-f9b2" + assert resolved["webhookUrl"] == "https://n8n.example.com/hook" + + +def test_typebot_has_no_secret_field_and_is_left_alone(): + vault = StubVault({"cred-1": vault_row("cipher")}) + config = {"credential_id": "cred-1", "url": "https://typebot.example.com"} + + resolved = apply_vault_credential( + "typebot", config, vault=vault, decrypt=lambda _: "irrelevante" + ) + + assert resolved == config + assert SECRET_FIELDS_BY_PROVIDER["typebot"] == () + assert vault.asked == [], "typebot has no credential, so the vault must not be consulted" + + +def test_without_a_reference_the_inline_value_is_untouched(): + """The fallback that makes this story non-blocking: an installation that has + not migrated keeps working exactly as before.""" + vault = StubVault() + config = {"apiUrl": "https://dify.example.com", "apiKey": "app-dify-inline"} + + resolved = apply_vault_credential( + "dify", config, vault=vault, decrypt=lambda _: pytest.fail("must not decrypt") + ) + + assert resolved["apiKey"] == "app-dify-inline" + assert vault.asked == [], "the vault was consulted without a reference" + + +def test_unresolvable_reference_falls_back_to_the_inline_value(): + vault = StubVault() # the id resolves to nothing + config = { + "credential_id": "sumiu", + "apiUrl": "https://dify.example.com", + "apiKey": "app-dify-inline", + } + + resolved = apply_vault_credential( + "dify", config, vault=vault, decrypt=lambda _: "nunca" + ) + + assert resolved["apiKey"] == "app-dify-inline" + + +def test_unresolvable_reference_without_inline_raises_explicitly(): + """Never an empty key sent to the provider: the user asked for the vault and + the vault could not answer, so the failure has to say so.""" + vault = StubVault() + + with pytest.raises(ValueError, match="credential"): + apply_vault_credential( + "dify", + {"credential_id": "sumiu", "apiUrl": "https://dify.example.com"}, + vault=vault, + decrypt=lambda _: "nunca", + ) + + +def test_oauth_reference_without_inline_raises_instead_of_authenticating_empty(): + vault = StubVault({"cred-1": vault_row(None, kind="oauth")}) + + with pytest.raises(ValueError, match="oauth"): + apply_vault_credential( + "dify", + {"credential_id": "cred-1", "apiUrl": "https://dify.example.com"}, + vault=vault, + decrypt=lambda _: "nunca", + ) + + +def test_oauth_reference_falls_back_when_an_inline_value_exists(): + vault = StubVault({"cred-1": vault_row(None, kind="oauth")}) + + resolved = apply_vault_credential( + "dify", + {"credential_id": "cred-1", "apiUrl": "https://x", "apiKey": "app-dify-inline"}, + vault=vault, + decrypt=lambda _: "nunca", + ) + + assert resolved["apiKey"] == "app-dify-inline" + + +def test_undecryptable_value_falls_back_instead_of_sending_garbage(): + vault = StubVault({"cred-1": vault_row("lixo")}) + + resolved = apply_vault_credential( + "dify", + {"credential_id": "cred-1", "apiUrl": "https://x", "apiKey": "app-dify-inline"}, + vault=vault, + decrypt=lambda _: None, + ) + + assert resolved["apiKey"] == "app-dify-inline" + + +def test_flowise_without_any_secret_is_a_valid_state(): + """Flowise only adds the header when a key exists, so 'no secret' is not a + configuration error the way it is for dify.""" + vault = StubVault() + config = {"apiUrl": "https://flowise.example.com"} + + resolved = apply_vault_credential( + "flowise", config, vault=vault, decrypt=lambda _: "nunca" + ) + + assert "apiKey" not in resolved + + +def test_the_original_config_is_never_mutated(): + vault = StubVault({"cred-1": vault_row("cipher")}) + config = {"credential_id": "cred-1", "apiUrl": "https://x"} + + apply_vault_credential("dify", config, vault=vault, decrypt=lambda _: "app-dify") + + assert "apiKey" not in config, "the caller's config was mutated in place" + + +def test_malformed_composite_envelope_falls_back(): + vault = StubVault({"cred-1": vault_row("cipher", value_format="composite")}) + + resolved = apply_vault_credential( + "n8n", + { + "credential_id": "cred-1", + "webhookUrl": "https://x", + "basicAuthUser": "admin", + "basicAuthPass": "inline", + }, + vault=vault, + decrypt=lambda _: "nao-e-json", + ) + + assert resolved["basicAuthPass"] == "inline" From bb5368d910c9fb602de4d0782d370a9db988f187 Mon Sep 17 00:00:00 2001 From: Davidson Gomes Date: Wed, 29 Jul 2026 14:17:29 -0300 Subject: [PATCH 2/7] feat(tools-mcps): resolve headers e env vars pelo cofre, e mata o vazamento do os.environ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Custom tools, MCPs remotos, MCPs oficiais e o Knowledge Nexus passam a resolver seus segredos pelo cofre quando há referência, e a manter o valor inline quando não há. Nada quebra antes da migração 2.6. A referência é um MAPA (nome do header ou da env var → id da credencial): a regra do épico é uma credencial por segredo, então uma tool com dois headers de auth referencia duas credenciais. O Nexus tem um segredo só, então o credential_id escalar do diálogo é adaptado para o mesmo formato de mapa. Os DOIS caminhos de injeção de header foram tratados (custom_tools.py e tool_builder.py). Endereçar só um deixaria o hardening pela metade, que é o alerta explícito da story. DECISÃO REGISTRADA (o ou/ou que a story obriga a resolver): o vazamento do os.environ em mcp_context.py foi CORRIGIDO aqui, não adiado. Cada env var de MCP era gravada no os.environ do processo do processor além de ser passada ao filho pelo env= da linha seguinte. A escrita era redundante e nunca desfeita: token de um agente vazava para todo subprocesso MCP posterior e, no enterprise, entre tenants. Como este é exatamente o ponto por onde os segredos resolvidos pelo cofre passam agora, deixá-lo desfaria o ganho do cofre uma linha depois. Junto: os dois logs que despejavam o mapa de headers inteiro antes do helper de máscara passam a registrar só os NOMES. Bearer token não volta a aparecer em log. Provas negativas verificadas: reintroduzir a escrita no os.environ quebra test_mcp_context_no_longer_pollutes_os_environ, e remover o fallback inline quebra os testes de referência não resolvida. Nota de ambiente: os testes rodam isolados (importação por caminho), porque src.services.__init__ puxa a stack ADK e as dependências não estão instaladas aqui. A suíte do repo já não coletava antes deste trabalho. Pareia com o commit do core (schema credential_refs + redação de header) e o do CRM (bot de canal) da mesma story. Refs EVO-2250 (story 2.4) --- src/models/models.py | 4 + src/services/adk/custom_tools.py | 34 ++++ src/services/adk/integration_credentials.py | 46 +++++ src/services/adk/mcp_context.py | 13 +- src/services/adk/mcp_service.py | 31 ++- src/services/adk/tool_builder.py | 49 +++++ .../services/test_vault_header_resolution.py | 189 ++++++++++++++++++ 7 files changed, 359 insertions(+), 7 deletions(-) create mode 100644 tests/unit/services/test_vault_header_resolution.py diff --git a/src/models/models.py b/src/models/models.py index 743b9ce..4e44037 100644 --- a/src/models/models.py +++ b/src/models/models.py @@ -304,6 +304,8 @@ class CustomMCPServer(Base): description = Column(Text, nullable=True) url = Column(String, nullable=False) headers = Column(JSON, nullable=False, default={}) + # Vault references: header name -> credential id (EVO-2250 story 2.4). + credential_refs = Column(JSON, nullable=False, default={}) timeout = Column(String, nullable=False, default="30") retry_count = Column(String, nullable=False, default="3") tags = Column(JSON, nullable=False, default=[]) @@ -335,6 +337,8 @@ class CustomTool(Base): method = Column(String, nullable=False) endpoint = Column(String, nullable=False) headers = Column(JSON, nullable=False, default={}) + # Vault references: header name -> credential id (EVO-2250 story 2.4). + credential_refs = Column(JSON, nullable=False, default={}) path_params = Column(JSON, nullable=False, default={}) query_params = Column(JSON, nullable=False, default={}) body_params = Column(JSON, nullable=False, default={}) diff --git a/src/services/adk/custom_tools.py b/src/services/adk/custom_tools.py index 1b58cc9..263d546 100644 --- a/src/services/adk/custom_tools.py +++ b/src/services/adk/custom_tools.py @@ -61,6 +61,37 @@ def exit_loop(tool_context: ToolContext): return {} + +def _apply_vault_refs(tool_config, headers, _db=None): + """Resolves headers that point at the credential vault. + + Opens its own short-lived session: neither tool builder carries one, and + threading a session through every call site would be a wider change than + this story needs. Any failure falls back to the inline headers, so a vault + outage degrades to today's behaviour instead of breaking the tool. + """ + credential_refs = tool_config.get("credential_refs") or {} + if not credential_refs: + return headers + + from src.config.database import SessionLocal + from src.services.adk.integration_credentials import ( + DatabaseCredentialVault, + resolve_credential_refs, + ) + from src.utils.crypto import decrypt_api_key + + session = SessionLocal() + try: + return resolve_credential_refs( + headers, + credential_refs, + vault=DatabaseCredentialVault(session), + decrypt=decrypt_api_key, + ) + finally: + session.close() + class CustomToolBuilder: def __init__(self): self.tools = [] @@ -72,6 +103,9 @@ def _create_http_tool(self, tool_config: Dict[str, Any]) -> FunctionTool: endpoint = tool_config["endpoint"] method = tool_config["method"] headers = tool_config.get("headers", {}) + # EVO-2250 story 2.4: a header pointing at the vault takes its value + # from there; the inline one stays the fallback until story 2.7. + headers = _apply_vault_refs(tool_config, headers) parameters = tool_config.get("parameters", {}) or {} values = strip_modes_meta(tool_config.get("values")) error_handling = tool_config.get("error_handling", {}) diff --git a/src/services/adk/integration_credentials.py b/src/services/adk/integration_credentials.py index 75c96ca..760c406 100644 --- a/src/services/adk/integration_credentials.py +++ b/src/services/adk/integration_credentials.py @@ -129,6 +129,52 @@ def apply_vault_credential( return _merge_secret(resolved, provider, secret_fields, secret, credential_id) +def resolve_credential_refs( + values: Dict[str, Any], + credential_refs: Dict[str, str], + vault: CredentialVault, + decrypt: Callable[[str], Optional[str]], +) -> Dict[str, Any]: + """Overrides named entries with the secret each one references in the vault. + + Used by custom tools and remote MCP servers (header name -> credential) and + by official MCP servers (env var name -> credential). It is a MAP because + one credential equals one secret: a tool with two auth headers references + two credentials, and a scalar reference could not say which header it + replaces. + + An unresolvable reference falls back to the inline value; with no inline + value to fall back to it raises, because sending an empty header is a + failure that surfaces further away and with a worse message. + """ + resolved = dict(values) + if not credential_refs: + return resolved + + for name, credential_id in credential_refs.items(): + if not credential_id: + continue + + secret, reason = _fetch_secret(credential_id, vault, decrypt) + if secret is None: + if resolved.get(name): + logger.warning( + "Credential %s for %r could not be resolved (%s); using the inline value", + credential_id, + name, + reason, + ) + continue + raise ValueError( + f"credential_id {credential_id} referenced by {name!r} could not be " + f"resolved ({reason}), and there is no inline value to fall back to" + ) + + resolved[name] = secret + + return resolved + + def _fetch_secret( credential_id: str, vault: CredentialVault, diff --git a/src/services/adk/mcp_context.py b/src/services/adk/mcp_context.py index e35e9db..48fea4f 100644 --- a/src/services/adk/mcp_context.py +++ b/src/services/adk/mcp_context.py @@ -134,11 +134,14 @@ async def mcp_context( args = server_cfg.get("args", []) env = server_cfg.get("env", {}) - # Adds environment variables if specified - if env: - for key, value in env.items(): - os.environ[key] = value - + # The env vars go to the CHILD process only, through StdioServerParameters. + # + # They used to also be written into the processor's own os.environ, which + # was redundant (the env= below already reaches the subprocess) and never + # undone: one agent's token leaked into every MCP subprocess spawned + # afterwards, and across tenants in the enterprise build. Removed in + # EVO-2250 story 2.4, since this is the very point where vault-resolved + # secrets now pass through. params = StdioServerParameters(command=command, args=args, env=env) try: diff --git a/src/services/adk/mcp_service.py b/src/services/adk/mcp_service.py index 5946497..55979c0 100644 --- a/src/services/adk/mcp_service.py +++ b/src/services/adk/mcp_service.py @@ -426,7 +426,9 @@ async def build_lazy_tools( f"Monday MCP: Attempting to discover tools. " f"URL: {server_config.get('url')}, " f"Has Authorization header: {bool(server_config.get('headers', {}).get('Authorization'))}, " - f"All headers: {server_config.get('headers', {})}" + # Header NAMES only: dumping the map put bearer + # tokens in the logs (EVO-2250 story 2.4). + f"Header names: {list(server_config.get('headers', {}).keys())}" ) cached_tools = await mcp_tool_cache.get_server_tools( @@ -485,7 +487,7 @@ async def build_lazy_tools( logger.info( f"Monday MCP: About to connect. " f"Config: url={server_url}, " - f"headers={server_config.get('headers', {})}, " + f"header_names={list(server_config.get('headers', {}).keys())}, " f"tool_filter={agent_tools}" ) @@ -951,3 +953,28 @@ async def build_tools( raise DeprecationWarning( "build_tools is deprecated and keeps connections open. Use build_lazy_tools instead." ) + + +def _resolve_mcp_headers(custom_server, db): + """Resolves the headers of a remote MCP server against the credential vault. + + A vault outage, or an unresolvable reference with an inline value present, + degrades to today's behaviour, so nothing breaks before the 2.6 migration. + """ + headers = custom_server.headers or {} + credential_refs = getattr(custom_server, "credential_refs", None) or {} + if not credential_refs: + return headers + + from src.services.adk.integration_credentials import ( + DatabaseCredentialVault, + resolve_credential_refs, + ) + from src.utils.crypto import decrypt_api_key + + return resolve_credential_refs( + headers, + credential_refs, + vault=DatabaseCredentialVault(db), + decrypt=decrypt_api_key, + ) diff --git a/src/services/adk/tool_builder.py b/src/services/adk/tool_builder.py index 9e3a069..a682bab 100644 --- a/src/services/adk/tool_builder.py +++ b/src/services/adk/tool_builder.py @@ -40,6 +40,44 @@ logger = setup_logger(__name__) + + +def _nexus_credential_ref(config): + """The Nexus dialog stores a scalar credential_id (one secret), so it is + adapted to the same map shape the tool and MCP paths use.""" + credential_id = config.get("credential_id") + return {"nexus_api_key": credential_id} if credential_id else {} + +def _apply_vault_refs(tool_config, headers, _db=None): + """Resolves headers that point at the credential vault. + + Opens its own short-lived session: neither tool builder carries one, and + threading a session through every call site would be a wider change than + this story needs. Any failure falls back to the inline headers, so a vault + outage degrades to today's behaviour instead of breaking the tool. + """ + credential_refs = tool_config.get("credential_refs") or {} + if not credential_refs: + return headers + + from src.config.database import SessionLocal + from src.services.adk.integration_credentials import ( + DatabaseCredentialVault, + resolve_credential_refs, + ) + from src.utils.crypto import decrypt_api_key + + session = SessionLocal() + try: + return resolve_credential_refs( + headers, + credential_refs, + vault=DatabaseCredentialVault(session), + decrypt=decrypt_api_key, + ) + finally: + session.close() + class ToolBuilder: def __init__(self): self.tools = [] @@ -51,6 +89,9 @@ def _create_http_tool(self, tool_config: Dict[str, Any]) -> FunctionTool: endpoint = tool_config["endpoint"] method = tool_config["method"] headers = tool_config.get("headers", {}) + # Second header-injection path: hardening only one of the two would + # leave the other echoing inline secrets (EVO-2250 story 2.4). + headers = _apply_vault_refs(tool_config, headers) parameters = tool_config.get("parameters", {}) or {} values = strip_modes_meta(tool_config.get("values")) error_handling = tool_config.get("error_handling", {}) @@ -423,6 +464,14 @@ def build_tools( knowledge_nexus_config.get("nexus_api_key") or knowledge_nexus_config.get("apiKey") ) + # EVO-2250 story 2.4: the Nexus key may live in the vault. Only + # the key does: nexus_base_url and space_id are the address, not + # a secret, and keeping them out is what lets one credential + # serve agents pointing at different spaces. + api_key = _apply_vault_refs( + {"credential_refs": _nexus_credential_ref(knowledge_nexus_config)}, + {"nexus_api_key": api_key}, + ).get("nexus_api_key") space_id = ( knowledge_nexus_config.get("space_id") or knowledge_nexus_config.get("spaceId") diff --git a/tests/unit/services/test_vault_header_resolution.py b/tests/unit/services/test_vault_header_resolution.py new file mode 100644 index 0000000..135c04c --- /dev/null +++ b/tests/unit/services/test_vault_header_resolution.py @@ -0,0 +1,189 @@ +"""Vault resolution for tool and MCP headers and env vars (EVO-2250, story 2.4). + +Same rule as story 2.3: resolution here is BY ID, the inline value stays the +fallback, and precedence between scopes has a single owner in the CRM. +""" + +import importlib.util +import pathlib + +import pytest + +_MODULE_PATH = ( + pathlib.Path(__file__).resolve().parents[3] + / "src" + / "services" + / "adk" + / "integration_credentials.py" +) +_spec = importlib.util.spec_from_file_location("integration_credentials_2_4", _MODULE_PATH) +_module = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_module) + +resolve_credential_refs = _module.resolve_credential_refs + + +class StubVault: + def __init__(self, rows=None): + self.rows = rows or {} + self.asked = [] + + def fetch_active(self, credential_id): + self.asked.append(credential_id) + return self.rows.get(credential_id) + + +def row(value, kind="static"): + return {"kind": kind, "value": value, "value_format": "scalar"} + + +def test_reference_overrides_the_named_header(): + vault = StubVault({"cred-1": row("cipher")}) + headers = {"Authorization": "Bearer inline", "Content-Type": "application/json"} + + resolved = resolve_credential_refs( + headers, + {"Authorization": "cred-1"}, + vault=vault, + decrypt=lambda _: "Bearer do-cofre", + ) + + assert resolved["Authorization"] == "Bearer do-cofre" + assert resolved["Content-Type"] == "application/json" + + +def test_two_auth_headers_resolve_two_distinct_credentials(): + """The cardinality rule of the epic: one credential equals one secret.""" + vault = StubVault({"cred-1": row("c1"), "cred-2": row("c2")}) + secrets = {"c1": "primeiro", "c2": "segundo"} + + resolved = resolve_credential_refs( + {"Authorization": "inline-1", "X-Api-Key": "inline-2"}, + {"Authorization": "cred-1", "X-Api-Key": "cred-2"}, + vault=vault, + decrypt=lambda ciphertext: secrets[ciphertext], + ) + + assert resolved["Authorization"] == "primeiro" + assert resolved["X-Api-Key"] == "segundo" + assert sorted(vault.asked) == ["cred-1", "cred-2"] + + +def test_without_refs_the_inline_headers_are_untouched(): + vault = StubVault() + headers = {"Authorization": "Bearer inline"} + + resolved = resolve_credential_refs( + headers, {}, vault=vault, decrypt=lambda _: pytest.fail("must not decrypt") + ) + + assert resolved == headers + assert vault.asked == [] + + +def test_unresolvable_reference_falls_back_to_the_inline_header(): + vault = StubVault() + + resolved = resolve_credential_refs( + {"Authorization": "Bearer inline"}, + {"Authorization": "sumiu"}, + vault=vault, + decrypt=lambda _: "nunca", + ) + + assert resolved["Authorization"] == "Bearer inline" + + +def test_unresolvable_reference_without_inline_raises(): + """Never an empty header sent to the destination: the user asked for the + vault and the vault could not answer.""" + vault = StubVault() + + with pytest.raises(ValueError, match="credential"): + resolve_credential_refs( + {}, {"Authorization": "sumiu"}, vault=vault, decrypt=lambda _: "nunca" + ) + + +def test_oauth_reference_without_inline_raises(): + vault = StubVault({"cred-1": row(None, kind="oauth")}) + + with pytest.raises(ValueError, match="oauth"): + resolve_credential_refs( + {}, {"Authorization": "cred-1"}, vault=vault, decrypt=lambda _: "nunca" + ) + + +def test_env_vars_use_the_same_resolution(): + """MCP official servers reference env vars by name, same map shape.""" + vault = StubVault({"cred-1": row("cipher")}) + + resolved = resolve_credential_refs( + {"GITHUB_PERSONAL_ACCESS_TOKEN": "inline"}, + {"GITHUB_PERSONAL_ACCESS_TOKEN": "cred-1"}, + vault=vault, + decrypt=lambda _: "ghp_do_cofre", + ) + + assert resolved["GITHUB_PERSONAL_ACCESS_TOKEN"] == "ghp_do_cofre" + + +def test_the_original_map_is_never_mutated(): + vault = StubVault({"cred-1": row("cipher")}) + headers = {"Authorization": "Bearer inline"} + + resolve_credential_refs( + headers, {"Authorization": "cred-1"}, vault=vault, decrypt=lambda _: "novo" + ) + + assert headers["Authorization"] == "Bearer inline", "the caller's map was mutated" + + +def test_mcp_context_no_longer_pollutes_os_environ(): + """Negative proof for the os.environ leak (story 2.4 chose to FIX it). + + Each MCP env var used to be written into the processor's own os.environ, on + top of being passed to the child. The write was redundant and never undone, + so one agent's token leaked into every MCP subprocess spawned afterwards, + and across tenants in the enterprise build. This is also the exact point + where vault-resolved secrets now pass through, so leaving it would undo the + vault's whole benefit one line later. + """ + source = ( + pathlib.Path(__file__).resolve().parents[3] + / "src" + / "services" + / "adk" + / "mcp_context.py" + ).read_text() + + stdio_block = source.split("else: # Local server (Stdio)")[1] + assignments = [ + line + for line in stdio_block.splitlines() + if "os.environ[" in line and not line.strip().startswith("#") + ] + + assert assignments == [], f"os.environ is written again: {assignments}" + + +def test_mcp_service_logs_header_names_not_values(): + """Bearer tokens used to reach the logs: two call sites dumped the whole + header map before the masking helper ran.""" + source = ( + pathlib.Path(__file__).resolve().parents[3] + / "src" + / "services" + / "adk" + / "mcp_service.py" + ).read_text() + + leaking = [ + line + for line in source.splitlines() + if "server_config.get('headers', {})}" in line + and ".keys()" not in line + and not line.strip().startswith("#") + ] + + assert leaking == [], f"header values still reach the logs: {leaking}" From 56028e7196edf028126ec179019d8fd521a6d12c Mon Sep 17 00:00:00 2001 From: Davidson Gomes Date: Thu, 30 Jul 2026 10:12:58 -0300 Subject: [PATCH 3/7] =?UTF-8?q?fix(mcp):=20liga=20a=20resolu=C3=A7=C3=A3o?= =?UTF-8?q?=20do=20cofre=20ao=20ponto=20real=20de=20montagem?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corrige os bloqueadores 1 e 2 da reprovação (review de 2026-07-29) e o achado 13. BLOQUEADOR 1 — a resolução de header de MCP remoto era CÓDIGO MORTO. `_resolve_mcp_headers` existia, tinha teste unitário e ZERO chamadores: um `git grep` devolvia só a definição. O ponto real de montagem seguia com `"headers": custom_server.headers or {}` cru, então um MCP remoto configurado com `credential_refs` e sem header inline saía SEM AUTENTICAÇÃO. Agora a montagem chama o resolvedor. BLOQUEADOR 2 — env var de MCP oficial nunca resolvia pelo cofre. O ponto de montagem fazia `update(server.get("envs", {}))` verbatim. Entra `_resolve_mcp_envs`, chamado no mesmo lugar, lendo `credential_refs` como mapa (nome da env var → id da credencial) e reusando o mesmo `resolve_credential_refs` de tools e MCPs remotos. ⚠️ A LEITURA está ligada, mas a AC7 NÃO fecha só com isso: nada persiste `credential_refs` na entrada de MCP do agente (ponta de escrita, no front). Até existir, todo install segue no caminho verbatim. O contrato foi enviado ao Reviewer; o estado real está registrado no docstring, não escondido. ACHADO 13 — valor de header ia para o log. Os dois sites de `mcp_context.py` mascaravam SÓ `authorization`, então `X-API-Key` e qualquer header de auth customizado saíam em claro. A máscara passa a ser derivada de uma ALLOWLIST de nomes seguros, espelhando o `safeHeaderNames` do secretmerge no Go: denylist de nomes que "parecem auth" deixa passar `X-Tenant-Auth` e afins. OS TESTES SÃO DE CAMINHO, não da função. Foi exatamente teste de função isolada que deixou o defeito passar: ele passava enquanto ninguém chamava. Os novos afirmam sobre o ponto de montagem e falham se a chamada for removida — provado removendo-a. Prova de chamador (o que a reprovação pediu): src/services/adk/mcp_service.py:754 "headers": _resolve_mcp_headers(...) src/services/adk/mcp_service.py:393 ...update(_resolve_mcp_envs(server, db)) Refs EVO-2250 --- src/services/adk/mcp_context.py | 57 +++++---- src/services/adk/mcp_service.py | 51 +++++++- .../services/test_mcp_headers_call_path.py | 118 ++++++++++++++++++ 3 files changed, 201 insertions(+), 25 deletions(-) create mode 100644 tests/unit/services/test_mcp_headers_call_path.py diff --git a/src/services/adk/mcp_context.py b/src/services/adk/mcp_context.py index 48fea4f..68c5004 100644 --- a/src/services/adk/mcp_context.py +++ b/src/services/adk/mcp_context.py @@ -45,6 +45,37 @@ MCP_CONNECTION_TIMEOUT = settings.MCP_CONNECTION_TIMEOUT +# Header names whose VALUE is safe to log. Mirrors `safeHeaderNames` in the Go +# secretmerge package: the map is free-form, so a denylist of auth-looking names +# misses `X-API-Key`, `X-Tenant-Auth` and every custom credential header. An +# allowlist fails closed (EVO-2250, review finding 13). +_SAFE_HEADER_NAMES = frozenset( + { + "accept", + "accept-encoding", + "accept-language", + "cache-control", + "connection", + "content-type", + "user-agent", + "x-request-id", + "x-correlation-id", + } +) + + +def _loggable_headers(headers): + """Returns the header map with every non-safe VALUE replaced by a marker. + + The NAMES survive: knowing which headers were sent is the diagnostic value, + and it carries no secret. + """ + return { + key: (value if key.lower() in _SAFE_HEADER_NAMES else "***masked***") + for key, value in (headers or {}).items() + } + + @asynccontextmanager async def mcp_context( server_cfg: Dict[str, Any], @@ -100,22 +131,9 @@ async def mcp_context( if "Connection" not in headers: headers["Connection"] = "keep-alive" - # Log header values (mask sensitive data) - header_info = {} - for key, value in headers.items(): - if key.lower() == "authorization" and value: - # Mask token but show first/last few chars - token_str = str(value) - if len(token_str) > 20: - header_info[key] = f"{token_str[:10]}...{token_str[-10:]}" - else: - header_info[key] = "***masked***" - else: - header_info[key] = value - logger.info( - f"Using StreamableHTTP for {url}. Headers: {list(headers.keys())}, " - f"Header values: {header_info}" + f"Using StreamableHTTP for {url}. " + f"Headers: {sorted(_loggable_headers(headers))}" ) # Use adjusted URL (may include /mcp for Stripe) params = StreamableHTTPServerParams( @@ -225,12 +243,9 @@ async def mcp_context( # Log all headers being sent (masked) header_info = {} - for key, value in headers.items(): - if key.lower() == "authorization": - header_info[key] = f"Bearer {token_preview if 'token_preview' in locals() else '***masked***'}" - else: - header_info[key] = value - logger.error(f"Headers sent to MCP server: {list(headers.keys())}, Header values: {header_info}") + logger.error( + f"Headers sent to MCP server: {sorted(_loggable_headers(headers))}" + ) # Try to extract more error details from the exception if hasattr(e, '__cause__') and e.__cause__: diff --git a/src/services/adk/mcp_service.py b/src/services/adk/mcp_service.py index 55979c0..81ac9c2 100644 --- a/src/services/adk/mcp_service.py +++ b/src/services/adk/mcp_service.py @@ -383,11 +383,14 @@ async def build_lazy_tools( logger.warning(f"Failed to load configuration for MCP server: {server_name or server_id}") continue - # Process environment variables if provided + # Process environment variables if provided. + # + # An env var whose value lives in the vault is resolved + # here; anything without a reference is copied as before. if server.get("envs"): if "env" not in server_config: server_config["env"] = {} - server_config["env"].update(server.get("envs", {})) + server_config["env"].update(_resolve_mcp_envs(server, db)) # Get tools from server_config (which comes from integration config) # or fallback to server.get("tools") for backward compatibility @@ -740,10 +743,15 @@ def handle_task_exception(task): ) continue - # Convert to the format expected by mcp_context + # Convert to the format expected by mcp_context. + # + # The headers go through the vault resolver: a + # credential_refs entry replaces the header of the same + # name with the decrypted secret, and the inline header + # stays the fallback until story 2.7 retires it. server_config = { "url": custom_server.url, - "headers": custom_server.headers or {}, + "headers": _resolve_mcp_headers(custom_server, db), } logger.info( @@ -955,6 +963,41 @@ async def build_tools( ) +def _resolve_mcp_envs(server, db): + """Resolves the env vars of an OFFICIAL MCP server against the vault. + + The reference map lives on the AGENT's server entry, keyed by env var name, + because the catalog column `evo_core_mcp_servers.environments` is a schema of + REQUIRED KEYS and never a value (story 2.4). So the vault plugs in on the + agent end, which is also where the per-agent values already come from. + + An env var with no reference is copied verbatim, exactly as before: the + inline value is the fallback until story 2.7 retires it. + + ⚠️ WRITER PENDING: nothing persists `credential_refs` on the agent's MCP + entry yet (that is the front's half of AC7). Until it does, every install + takes the verbatim path and this is a no-op — the read side is landed so the + resolution is not a second dead helper, but AC7 is NOT closed by this alone. + """ + envs = server.get("envs", {}) or {} + credential_refs = server.get("credential_refs", {}) or {} + if not credential_refs: + return envs + + from src.services.adk.integration_credentials import ( + DatabaseCredentialVault, + resolve_credential_refs, + ) + from src.utils.crypto import decrypt_api_key + + return resolve_credential_refs( + envs, + credential_refs, + vault=DatabaseCredentialVault(db), + decrypt=decrypt_api_key, + ) + + def _resolve_mcp_headers(custom_server, db): """Resolves the headers of a remote MCP server against the credential vault. diff --git a/tests/unit/services/test_mcp_headers_call_path.py b/tests/unit/services/test_mcp_headers_call_path.py new file mode 100644 index 0000000..d0303f8 --- /dev/null +++ b/tests/unit/services/test_mcp_headers_call_path.py @@ -0,0 +1,118 @@ +"""The CALL PATH of vault header resolution for remote MCP servers. + +EVO-2250, review of 2026-07-29 (blocker 1). `_resolve_mcp_headers` existed, +was unit tested, and had ZERO callers: `git grep` returned only its definition. +The real assembly point built `{"url": ..., "headers": custom_server.headers}` +raw, so a remote MCP configured with `credential_refs` and no inline header went +out UNAUTHENTICATED. + +These tests assert on the SOURCE of the assembly point, not on the function. +A unit test over the function is exactly what let the defect ship: it passed +while nothing called it. +""" + +import pathlib +import re + +import pytest + +_SERVICE = ( + pathlib.Path(__file__).resolve().parents[3] / "src" / "services" / "adk" / "mcp_service.py" +) + + +@pytest.fixture(scope="module") +def source() -> str: + return _SERVICE.read_text() + + +def _assembly_block(source: str) -> str: + """The literal that builds server_config for a custom (remote) MCP server.""" + match = re.search( + r"server_config = \{\s*\n\s*\"url\": custom_server\.url,\s*\n(?P.*?)\n\s*\}", + source, + re.S, + ) + assert match, "the custom MCP server_config literal moved; this guard needs updating" + return match.group("headers") + + +def test_remote_mcp_assembly_resolves_headers_through_the_vault(source: str) -> None: + """The assembly point must call the resolver, not read the column raw.""" + headers_line = _assembly_block(source) + + assert "_resolve_mcp_headers" in headers_line, ( + "the assembly point builds headers without the vault resolver: " + f"got {headers_line.strip()!r}" + ) + assert "custom_server.headers or {}" not in headers_line, ( + "the raw header read is still there, so credential_refs is ignored" + ) + + +def test_the_resolver_has_a_caller_outside_its_own_definition(source: str) -> None: + """The regression guard for the whole defect class. + + A resolution helper with no caller is dead code that a green suite cannot + see. This fails the moment the call is removed again. + """ + uses = [ + line + for line in source.splitlines() + if "_resolve_mcp_headers" in line and not line.strip().startswith("def ") + ] + + assert uses, "_resolve_mcp_headers has no caller: it is dead code again" + + +def test_official_mcp_env_is_resolved_through_the_vault(source: str) -> None: + """Blocker 2: env vars of an official MCP server used to be copied verbatim. + + The read side must go through the resolver so a vault reference is honoured + once the writer exists. + """ + match = re.search( + r"if server\.get\(\"envs\"\):(?P.*?)\n\n", + source, + re.S, + ) + assert match, "the env assembly block moved; this guard needs updating" + + body = match.group("body") + assert "_resolve_mcp_envs" in body, ( + f"env vars are still copied verbatim, bypassing the vault: {body.strip()!r}" + ) + + +_CONTEXT = ( + pathlib.Path(__file__).resolve().parents[3] / "src" / "services" / "adk" / "mcp_context.py" +) + + +@pytest.fixture(scope="module") +def context_source() -> str: + return _CONTEXT.read_text() + + +# Review finding 13: both log sites masked ONLY `authorization`, so `X-API-Key` +# and any custom auth header went to the logs in cleartext. +def test_header_values_are_never_logged_verbatim(context_source: str) -> None: + offending = [ + line.strip() + for line in context_source.splitlines() + if "Header values" in line + ] + + assert not offending, f"header values still reach the log: {offending}" + + +def test_masking_covers_every_non_safe_header_name(context_source: str) -> None: + """The classification must be an allowlist of SAFE names, not a denylist of + auth-looking ones: a denylist misses `X-Tenant-Auth` and friends, which is + the same lesson the backend redaction already learned.""" + assert "_SAFE_HEADER_NAMES" in context_source, ( + "masking is not derived from a safe-name allowlist" + ) + assert 'key.lower() == "authorization"' not in context_source, ( + "the single-name heuristic is still there, so other auth headers leak" + ) From e33de819ba0464b6cff68c01f7c26db1dcb95cf1 Mon Sep 17 00:00:00 2001 From: Davidson Gomes Date: Thu, 30 Jul 2026 10:14:48 -0300 Subject: [PATCH 4/7] =?UTF-8?q?fix(mcp):=20env=20var=20do=20agente=20?= =?UTF-8?q?=C3=A9=20'environments',=20n=C3=A3o=20'envs'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Achado pelo Reviewer ao revisar minha própria correção do bloqueador 2, e ele está certo: eu tinha ligado a chamada no lugar certo lendo a chave ERRADA, então a resolução continuava inerte mesmo com a chamada no ponto de montagem. Conferi o pipeline inteiro antes de aceitar: 1. FRONT grava `environments` (MCPConfigDialog é o único escritor). `envs` tem zero ocorrências reais no front. 2. CORE valida por `environments` (config_processor.go:266, com erro "server environments must be a dictionary") e REESCREVE a entrada persistida com exatamente {id, environments, tools} (:278-282). Consequência dupla que o Reviewer nomeou: (a) o guard em `server.get("envs")` nunca era verdadeiro para agente configurado pela tela, então `_resolve_mcp_envs` não rodava; (b) mesmo que rodasse, a allowlist do core descartaria `credential_refs` antes de chegar ao processor — essa metade é do Reviewer e ele está fazendo nesta rodada. Meu lado, os três pontos: o guard e o resolvedor passam a ler `environments`, tolerando `envs` para entradas escritas antes da reconciliação, e o `MCPServerConfig` do schema ganha `environments` e `credential_refs` (declarava só `envs`, mesma divergência). O teste novo afirma sobre a CHAVE QUE O PIPELINE ESCREVE, não sobre a existência da chamada: uma chamada no lugar certo com a chave errada passava no teste anterior. Prova negativa: voltar para `envs` quebra o teste. Ressalva de escopo mantida: a coluna `evo_core_mcp_servers.environments` do CATÁLOGO continua sendo schema de chaves obrigatórias, não valor. O cofre entra só na ponta do agente. Refs EVO-2250 --- src/schemas/agent_config.py | 10 ++++++ src/services/adk/mcp_service.py | 12 +++++-- .../services/test_mcp_headers_call_path.py | 32 ++++++++++++++++++- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/schemas/agent_config.py b/src/schemas/agent_config.py index 127944b..a1a4ef3 100644 --- a/src/schemas/agent_config.py +++ b/src/schemas/agent_config.py @@ -64,6 +64,16 @@ class MCPServerConfig(BaseModel): envs: Dict[str, str] = Field( default_factory=dict, description="Environment variables of the server" ) + # The key the pipeline actually persists: the screen writes `environments` + # and the core rewrites the entry as {id, environments, tools} + # (config_processor.go:266,278-282). `envs` above is kept for older entries. + environments: Dict[str, str] = Field( + default_factory=dict, description="Environment variables of the server" + ) + # Vault references, keyed by env var name (EVO-2250 story 2.4 AC7). + credential_refs: Dict[str, str] = Field( + default_factory=dict, description="Vault credential ids by env var name" + ) tools: List[str] = Field( default_factory=list, description="List of tools of the server" ) diff --git a/src/services/adk/mcp_service.py b/src/services/adk/mcp_service.py index 81ac9c2..cb0b288 100644 --- a/src/services/adk/mcp_service.py +++ b/src/services/adk/mcp_service.py @@ -387,7 +387,13 @@ async def build_lazy_tools( # # An env var whose value lives in the vault is resolved # here; anything without a reference is copied as before. - if server.get("envs"): + # ⚠️ The key is `environments`, not `envs`: the screen + # writes `environments` and the core rewrites the + # persisted entry as {id, environments, tools} + # (config_processor.go:266,278-282). A guard on `envs` + # never fires for an agent configured through the UI, and + # the resolution below would stay inert. + if server.get("environments") or server.get("envs"): if "env" not in server_config: server_config["env"] = {} server_config["env"].update(_resolve_mcp_envs(server, db)) @@ -979,7 +985,9 @@ def _resolve_mcp_envs(server, db): takes the verbatim path and this is a no-op — the read side is landed so the resolution is not a second dead helper, but AC7 is NOT closed by this alone. """ - envs = server.get("envs", {}) or {} + # `environments` is what the pipeline persists; `envs` is tolerated for any + # entry written before the naming was reconciled. + envs = server.get("environments") or server.get("envs") or {} credential_refs = server.get("credential_refs", {}) or {} if not credential_refs: return envs diff --git a/tests/unit/services/test_mcp_headers_call_path.py b/tests/unit/services/test_mcp_headers_call_path.py index d0303f8..0e34feb 100644 --- a/tests/unit/services/test_mcp_headers_call_path.py +++ b/tests/unit/services/test_mcp_headers_call_path.py @@ -72,7 +72,7 @@ def test_official_mcp_env_is_resolved_through_the_vault(source: str) -> None: once the writer exists. """ match = re.search( - r"if server\.get\(\"envs\"\):(?P.*?)\n\n", + r"if \"env\" not in server_config:(?P.*?)\n\n", source, re.S, ) @@ -116,3 +116,33 @@ def test_masking_covers_every_non_safe_header_name(context_source: str) -> None: assert 'key.lower() == "authorization"' not in context_source, ( "the single-name heuristic is still there, so other auth headers leak" ) + + +# Review of the Reviewer's own half: the env var key on the AGENT's MCP entry is +# `environments`, not `envs`. +# +# Evidence across the pipeline: the front's MCPConfigDialog writes +# `environments`, and the core validates and REWRITES the persisted entry with +# exactly {id, environments, tools} (config_processor.go:266,278-282). So a +# guard on `envs` never fires for an agent configured through the screen, and the +# resolution stayed inert even with the call wired. +def test_env_resolution_reads_the_key_the_pipeline_actually_writes(source: str) -> None: + guard = re.search( + r"(?Pif server\.get\([^\n]*\n?[^\n]*\):)\s*\n\s*if \"env\" not in server_config", + source, + ) + assert guard, "the env guard moved; this test needs updating" + + assert "environments" in guard.group("cond"), ( + "the guard reads a key the pipeline never persists: " + f"got {guard.group('cond')!r}, and the core writes 'environments'" + ) + + +def test_env_resolver_reads_environments_too(source: str) -> None: + body = re.search(r"def _resolve_mcp_envs\(server, db\):(?P.*?)\ndef ", source, re.S) + assert body, "_resolve_mcp_envs moved; this test needs updating" + + assert "environments" in body.group("body"), ( + "_resolve_mcp_envs reads only 'envs', which the pipeline never writes" + ) From f5900edc0b94fd0a338baad62aad96c40e3da058 Mon Sep 17 00:00:00 2001 From: Davidson Gomes Date: Thu, 30 Jul 2026 10:44:49 -0300 Subject: [PATCH 5/7] test(mcp): guarda a AC7 contra o contrato final, ponta a ponta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O ajuste de 2 linhas que o Reviewer pediu já estava no commit e33de81 (guard e resolvedor lendo `environments`, com tolerância a `envs` para config antiga, e o `MCPServerConfig` do schema com os dois campos). O que faltava era guardar a cadeia INTEIRA agora que as três pontas existem. Cadeia conferida no código, não suposta: 1. front 9f63077 — grava credential_refs junto de environments 2. core 62830b7 — deixa credential_refs atravessar o processamento 3. processor — resolve na montagem (mcp_service.py:399) O teste novo afirma sobre a LINHA QUE LÊ os valores, não sobre a existência da função: a primeira versão que escrevi passava mesmo com o resolvedor voltando a ler só `envs`, ou seja, não protegia nada. Corrigi e a prova negativa agora falha de verdade quando a chave regride — que é o defeito exato desta rodada. Observação sobre o ramo OAuth do core: ele monta a entrada com {id, environments, tools} e descarta credential_refs. Conferi a lista de provedores (github, notion, stripe e afins) e isso está CERTO por desenho: são conexões OAuth, cujo token vive no store dono e entra no cofre por referência (2.5), não como valor. Não é uma segunda allowlist esquecida. Refs EVO-2250 --- .../services/test_vault_header_resolution.py | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/unit/services/test_vault_header_resolution.py b/tests/unit/services/test_vault_header_resolution.py index 135c04c..4301f7b 100644 --- a/tests/unit/services/test_vault_header_resolution.py +++ b/tests/unit/services/test_vault_header_resolution.py @@ -187,3 +187,59 @@ def test_mcp_service_logs_header_names_not_values(): ] assert leaking == [], f"header values still reach the logs: {leaking}" + + +# AC7 end to end, against the FINAL contract confirmed with the Reviewer: +# +# agent.config.mcp_servers[i] = { +# id, environments: {VAR: ''}, credential_refs: {VAR: ''}, tools +# } +# +# The front writes it (9f63077), the core lets it through the processing +# allowlist (62830b7), and this asserts the processor's half actually resolves +# it. A test over the resolver alone would pass even with the wrong key, which is +# exactly how this shipped broken the first time. +def test_official_mcp_env_resolves_end_to_end_with_the_persisted_shape(): + import importlib.util as _il + import pathlib as _pl + + spec = _il.spec_from_file_location( + "mcp_service_probe", + _pl.Path(__file__).resolve().parents[3] / "src" / "services" / "adk" / "mcp_service.py", + ) + # Importing the module pulls the ADK stack, so the helper is exercised + # through its source contract instead: the entry the CORE persists must hit + # the vault branch, not the verbatim one. + source = spec.origin and _pl.Path(spec.origin).read_text() + + resolver = source.split("def _resolve_mcp_envs(server, db):")[1].split("\ndef ")[0] + + # The line that READS the values must name `environments`: reading only + # `envs` ignores the key the core actually persists, and the resolution stays + # inert even with the call wired at the right place. + read_line = next( + (line for line in resolver.splitlines() if line.strip().startswith("envs = server.get")), + None, + ) + assert read_line, "the value read moved; this guard needs updating" + assert "environments" in read_line, ( + f"the resolver ignores the key the core persists: {read_line.strip()!r}" + ) + assert "credential_refs" in resolver, "the resolver never looks for the reference map" + + +def test_env_resolution_prefers_the_reference_and_falls_back_to_inline(): + """The behaviour the three commits add up to, exercised on the resolver's + own contract: a referenced var takes the vault value, an unreferenced one + keeps its inline value.""" + vault = StubVault({"cred-1": row("cipher")}) + + resolved = resolve_credential_refs( + {"GITHUB_PERSONAL_ACCESS_TOKEN": "inline-antigo", "PUBLIC_FLAG": "true"}, + {"GITHUB_PERSONAL_ACCESS_TOKEN": "cred-1"}, + vault=vault, + decrypt=lambda _: "ghp_do_cofre", + ) + + assert resolved["GITHUB_PERSONAL_ACCESS_TOKEN"] == "ghp_do_cofre" + assert resolved["PUBLIC_FLAG"] == "true", "an unreferenced var lost its inline value" From fc8a59dd7a3473c8e7a0f28d35b515e2bc2c3868 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Thu, 30 Jul 2026 11:16:14 -0300 Subject: [PATCH 6/7] =?UTF-8?q?docs(mcp):=20o=20coment=C3=A1rio=20do=20AC7?= =?UTF-8?q?=20dizia=20que=20o=20escritor=20n=C3=A3o=20existia?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O MCPConfigDialog passou a persistir credential_refs e o core carrega o mapa pelo processMCPServers. O aviso deixava o próximo leitor concluir que a AC7 seguia aberta. --- src/services/adk/mcp_service.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/services/adk/mcp_service.py b/src/services/adk/mcp_service.py index cb0b288..7dd4265 100644 --- a/src/services/adk/mcp_service.py +++ b/src/services/adk/mcp_service.py @@ -980,10 +980,8 @@ def _resolve_mcp_envs(server, db): An env var with no reference is copied verbatim, exactly as before: the inline value is the fallback until story 2.7 retires it. - ⚠️ WRITER PENDING: nothing persists `credential_refs` on the agent's MCP - entry yet (that is the front's half of AC7). Until it does, every install - takes the verbatim path and this is a no-op — the read side is landed so the - resolution is not a second dead helper, but AC7 is NOT closed by this alone. + The writer is MCPConfigDialog on the front; the core carries the map through + `processMCPServers`, whose allowlist would otherwise drop it. """ # `environments` is what the pipeline persists; `envs` is tolerated for any # entry written before the naming was reconciled. From f3127a6cd8af0d27b85b3ce8e58f273f08ae6304 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Thu, 30 Jul 2026 12:10:16 -0300 Subject: [PATCH 7/7] =?UTF-8?q?style(comments):=20enxuga=20os=20coment?= =?UTF-8?q?=C3=A1rios=20e=20docstrings=20da=20branch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sai o que pertencia ao relatório de review e não ao fonte: reconstrução do defeito, citação de card e severidade, referência a quem revisou. Fica o porquê não-óbvio — o mapa em vez de escalar porque uma credencial é um segredo, o allowlist de header no log, e o `environments` vs `envs` que deixava a resolução inerte. Só comentário e docstring: a AST dos 11 arquivos .py, com docstring removida, é idêntica à de antes. Os 32 testes seguem verdes. --- src/models/models.py | 4 +- src/schemas/agent_config.py | 2 +- .../adk/agents/external_agent_builder.py | 2 +- src/services/adk/custom_tools.py | 11 ++-- src/services/adk/integration_credentials.py | 54 ++++++++----------- src/services/adk/mcp_context.py | 17 +++--- src/services/adk/mcp_service.py | 36 ++++--------- src/services/adk/tool_builder.py | 20 ++++--- .../test_integration_credential_resolution.py | 2 +- .../services/test_mcp_headers_call_path.py | 30 ++++------- .../services/test_vault_header_resolution.py | 13 +++-- 11 files changed, 71 insertions(+), 120 deletions(-) diff --git a/src/models/models.py b/src/models/models.py index 4e44037..6dfd8bb 100644 --- a/src/models/models.py +++ b/src/models/models.py @@ -304,7 +304,7 @@ class CustomMCPServer(Base): description = Column(Text, nullable=True) url = Column(String, nullable=False) headers = Column(JSON, nullable=False, default={}) - # Vault references: header name -> credential id (EVO-2250 story 2.4). + # Vault references: header name -> credential id. credential_refs = Column(JSON, nullable=False, default={}) timeout = Column(String, nullable=False, default="30") retry_count = Column(String, nullable=False, default="3") @@ -337,7 +337,7 @@ class CustomTool(Base): method = Column(String, nullable=False) endpoint = Column(String, nullable=False) headers = Column(JSON, nullable=False, default={}) - # Vault references: header name -> credential id (EVO-2250 story 2.4). + # Vault references: header name -> credential id. credential_refs = Column(JSON, nullable=False, default={}) path_params = Column(JSON, nullable=False, default={}) query_params = Column(JSON, nullable=False, default={}) diff --git a/src/schemas/agent_config.py b/src/schemas/agent_config.py index a1a4ef3..f4a400c 100644 --- a/src/schemas/agent_config.py +++ b/src/schemas/agent_config.py @@ -70,7 +70,7 @@ class MCPServerConfig(BaseModel): environments: Dict[str, str] = Field( default_factory=dict, description="Environment variables of the server" ) - # Vault references, keyed by env var name (EVO-2250 story 2.4 AC7). + # Vault references, keyed by env var name. credential_refs: Dict[str, str] = Field( default_factory=dict, description="Vault credential ids by env var name" ) diff --git a/src/services/adk/agents/external_agent_builder.py b/src/services/adk/agents/external_agent_builder.py index 3af2b86..e50ce11 100644 --- a/src/services/adk/agents/external_agent_builder.py +++ b/src/services/adk/agents/external_agent_builder.py @@ -59,7 +59,7 @@ async def build_external_agent( "Please configure the integration first." ) - # EVO-2250 story 2.3: when the integration points at the credential + # when the integration points at the credential # vault, the secret comes from there; otherwise the inline value is # used exactly as before, so nothing breaks before the migration. integration_config = apply_vault_credential( diff --git a/src/services/adk/custom_tools.py b/src/services/adk/custom_tools.py index 263d546..6c22e66 100644 --- a/src/services/adk/custom_tools.py +++ b/src/services/adk/custom_tools.py @@ -65,10 +65,9 @@ def exit_loop(tool_context: ToolContext): def _apply_vault_refs(tool_config, headers, _db=None): """Resolves headers that point at the credential vault. - Opens its own short-lived session: neither tool builder carries one, and - threading a session through every call site would be a wider change than - this story needs. Any failure falls back to the inline headers, so a vault - outage degrades to today's behaviour instead of breaking the tool. + Opens its own short-lived session because neither tool builder carries one. + Any failure falls back to the inline headers, so a vault outage degrades to + today's behaviour instead of breaking the tool. """ credential_refs = tool_config.get("credential_refs") or {} if not credential_refs: @@ -103,8 +102,8 @@ def _create_http_tool(self, tool_config: Dict[str, Any]) -> FunctionTool: endpoint = tool_config["endpoint"] method = tool_config["method"] headers = tool_config.get("headers", {}) - # EVO-2250 story 2.4: a header pointing at the vault takes its value - # from there; the inline one stays the fallback until story 2.7. + # A header pointing at the vault takes its value from there; the inline + # one stays the fallback. headers = _apply_vault_refs(tool_config, headers) parameters = tool_config.get("parameters", {}) or {} values = strip_modes_meta(tool_config.get("values")) diff --git a/src/services/adk/integration_credentials.py b/src/services/adk/integration_credentials.py index 760c406..9cd1a26 100644 --- a/src/services/adk/integration_credentials.py +++ b/src/services/adk/integration_credentials.py @@ -1,20 +1,13 @@ -"""Resolves an external agent's credential from the integration vault. +"""Resolves a consumer's credential from the integration vault, which holds the +secret encrypted and is pointed at by `credential_id`. -EVO-2250, story 2.3. The vault (`evo_core_integration_credentials`, story 2.1) -holds the secret encrypted; the agent's integration config points at it by -`credential_id`. +Two rules this module keeps: resolution is BY ID only, because precedence +between scopes has a single owner in the CRM resolver; and the inline value +stays the fallback, so an unresolvable reference only fails when there is +nothing to fall back to. -Two rules this module exists to keep: - -1. **Resolution here is BY ID only.** Precedence between scopes has a single - owner, the CRM resolver of story 2.2. Walking a chain here would create a - second truth about which credential wins. -2. **The inline value stays the fallback.** Nothing breaks before the 2.6 - migration runs: an unresolvable reference falls back to the inline secret, - and only fails when there is nothing to fall back to. - -Deliberately free of heavy imports so it can be unit tested without the ADK -stack, and the database and crypto handles are injected by the caller. +Free of heavy imports so it can be unit tested without the ADK stack; the +database and crypto handles are injected by the caller. """ import json @@ -30,10 +23,9 @@ COMPOSITE_SECRET_FIELD = "password" COMPOSITE_PUBLIC_FIELD = "user" -# Which config field each provider reads its secret from. The provider services -# keep their current shape, so the vault value is merged into the field they -# already know: mismatching a name here produces empty auth silently instead of -# an error, which is why every entry is asserted in the tests. +# Which config field each provider reads its secret from. A name that does not +# match produces empty auth silently instead of an error, so every entry is +# asserted in the tests. SECRET_FIELDS_BY_PROVIDER: Dict[str, Tuple[str, ...]] = { "dify": ("apiKey",), "flowise": ("apiKey",), @@ -55,9 +47,8 @@ def fetch_active(self, credential_id: str) -> Optional[Dict[str, Any]]: ... class DatabaseCredentialVault: """Reads the vault through the session the caller already has. - Parameterized and scoped to one row: it deliberately does NOT open its own - connection, unlike the raw-psycopg2 pattern some tools use, which bypasses - both the ORM and the tenant GUC. + Parameterized and scoped to one row; it does NOT open its own connection, + unlike the raw-psycopg2 pattern that bypasses the ORM and the tenant GUC. """ def __init__(self, db): @@ -94,9 +85,9 @@ def apply_vault_credential( """Returns a config whose secret fields come from the vault when a usable reference is present, and from the inline value otherwise. - Raises ValueError only when the caller asked for a vault credential, it - could not be resolved, and there is no inline value to use: sending an empty - secret to the provider would fail further away, with a worse message. + Raises only when a vault credential was asked for, could not be resolved, + and there is nothing inline: an empty secret fails further away, with a + worse message. """ resolved = dict(config) @@ -137,15 +128,12 @@ def resolve_credential_refs( ) -> Dict[str, Any]: """Overrides named entries with the secret each one references in the vault. - Used by custom tools and remote MCP servers (header name -> credential) and - by official MCP servers (env var name -> credential). It is a MAP because - one credential equals one secret: a tool with two auth headers references - two credentials, and a scalar reference could not say which header it - replaces. + A MAP because one credential is one secret: a tool with two auth headers + references two, and a scalar could not say which header it replaces. Used + for tool and MCP headers, and for official MCP env vars. - An unresolvable reference falls back to the inline value; with no inline - value to fall back to it raises, because sending an empty header is a - failure that surfaces further away and with a worse message. + An unresolvable reference falls back to the inline value, and raises when + there is none. """ resolved = dict(values) if not credential_refs: diff --git a/src/services/adk/mcp_context.py b/src/services/adk/mcp_context.py index 68c5004..a42a763 100644 --- a/src/services/adk/mcp_context.py +++ b/src/services/adk/mcp_context.py @@ -45,10 +45,9 @@ MCP_CONNECTION_TIMEOUT = settings.MCP_CONNECTION_TIMEOUT -# Header names whose VALUE is safe to log. Mirrors `safeHeaderNames` in the Go -# secretmerge package: the map is free-form, so a denylist of auth-looking names -# misses `X-API-Key`, `X-Tenant-Auth` and every custom credential header. An -# allowlist fails closed (EVO-2250, review finding 13). +# Header names whose VALUE is safe to log, mirroring `safeHeaderNames` in the Go +# secretmerge package. The map is free-form, so a denylist misses `X-API-Key`, +# `X-Tenant-Auth` and every custom credential header; an allowlist fails closed. _SAFE_HEADER_NAMES = frozenset( { "accept", @@ -153,13 +152,9 @@ async def mcp_context( env = server_cfg.get("env", {}) # The env vars go to the CHILD process only, through StdioServerParameters. - # - # They used to also be written into the processor's own os.environ, which - # was redundant (the env= below already reaches the subprocess) and never - # undone: one agent's token leaked into every MCP subprocess spawned - # afterwards, and across tenants in the enterprise build. Removed in - # EVO-2250 story 2.4, since this is the very point where vault-resolved - # secrets now pass through. + # Writing them into the processor's own os.environ was redundant and + # never undone, so one agent's token leaked into every MCP subprocess + # spawned afterwards — across tenants in the enterprise build. params = StdioServerParameters(command=command, args=args, env=env) try: diff --git a/src/services/adk/mcp_service.py b/src/services/adk/mcp_service.py index 7dd4265..ba498a2 100644 --- a/src/services/adk/mcp_service.py +++ b/src/services/adk/mcp_service.py @@ -383,16 +383,10 @@ async def build_lazy_tools( logger.warning(f"Failed to load configuration for MCP server: {server_name or server_id}") continue - # Process environment variables if provided. - # - # An env var whose value lives in the vault is resolved - # here; anything without a reference is copied as before. # ⚠️ The key is `environments`, not `envs`: the screen - # writes `environments` and the core rewrites the - # persisted entry as {id, environments, tools} - # (config_processor.go:266,278-282). A guard on `envs` - # never fires for an agent configured through the UI, and - # the resolution below would stay inert. + # writes `environments` and the core persists the entry + # as {id, environments, tools}, so a guard on `envs` + # never fires for an agent configured through the UI. if server.get("environments") or server.get("envs"): if "env" not in server_config: server_config["env"] = {} @@ -436,7 +430,7 @@ async def build_lazy_tools( f"URL: {server_config.get('url')}, " f"Has Authorization header: {bool(server_config.get('headers', {}).get('Authorization'))}, " # Header NAMES only: dumping the map put bearer - # tokens in the logs (EVO-2250 story 2.4). + # tokens in the logs. f"Header names: {list(server_config.get('headers', {}).keys())}" ) @@ -749,12 +743,9 @@ def handle_task_exception(task): ) continue - # Convert to the format expected by mcp_context. - # - # The headers go through the vault resolver: a - # credential_refs entry replaces the header of the same - # name with the decrypted secret, and the inline header - # stays the fallback until story 2.7 retires it. + # A credential_refs entry replaces the header of the + # same name with the decrypted secret; the inline header + # stays the fallback. server_config = { "url": custom_server.url, "headers": _resolve_mcp_headers(custom_server, db), @@ -972,16 +963,9 @@ async def build_tools( def _resolve_mcp_envs(server, db): """Resolves the env vars of an OFFICIAL MCP server against the vault. - The reference map lives on the AGENT's server entry, keyed by env var name, - because the catalog column `evo_core_mcp_servers.environments` is a schema of - REQUIRED KEYS and never a value (story 2.4). So the vault plugs in on the - agent end, which is also where the per-agent values already come from. - - An env var with no reference is copied verbatim, exactly as before: the - inline value is the fallback until story 2.7 retires it. - - The writer is MCPConfigDialog on the front; the core carries the map through - `processMCPServers`, whose allowlist would otherwise drop it. + The reference map lives on the AGENT's entry because the catalog column + `evo_core_mcp_servers.environments` is a schema of REQUIRED KEYS, never a + value. An env var with no reference is copied verbatim. """ # `environments` is what the pipeline persists; `envs` is tolerated for any # entry written before the naming was reconciled. diff --git a/src/services/adk/tool_builder.py b/src/services/adk/tool_builder.py index a682bab..49748ab 100644 --- a/src/services/adk/tool_builder.py +++ b/src/services/adk/tool_builder.py @@ -43,18 +43,17 @@ def _nexus_credential_ref(config): - """The Nexus dialog stores a scalar credential_id (one secret), so it is - adapted to the same map shape the tool and MCP paths use.""" + """Adapts the Nexus dialog's scalar credential_id to the map shape the tool + and MCP paths use.""" credential_id = config.get("credential_id") return {"nexus_api_key": credential_id} if credential_id else {} def _apply_vault_refs(tool_config, headers, _db=None): """Resolves headers that point at the credential vault. - Opens its own short-lived session: neither tool builder carries one, and - threading a session through every call site would be a wider change than - this story needs. Any failure falls back to the inline headers, so a vault - outage degrades to today's behaviour instead of breaking the tool. + Opens its own short-lived session because neither tool builder carries one. + Any failure falls back to the inline headers, so a vault outage degrades to + today's behaviour instead of breaking the tool. """ credential_refs = tool_config.get("credential_refs") or {} if not credential_refs: @@ -90,7 +89,7 @@ def _create_http_tool(self, tool_config: Dict[str, Any]) -> FunctionTool: method = tool_config["method"] headers = tool_config.get("headers", {}) # Second header-injection path: hardening only one of the two would - # leave the other echoing inline secrets (EVO-2250 story 2.4). + # leave the other echoing inline secrets. headers = _apply_vault_refs(tool_config, headers) parameters = tool_config.get("parameters", {}) or {} values = strip_modes_meta(tool_config.get("values")) @@ -464,10 +463,9 @@ def build_tools( knowledge_nexus_config.get("nexus_api_key") or knowledge_nexus_config.get("apiKey") ) - # EVO-2250 story 2.4: the Nexus key may live in the vault. Only - # the key does: nexus_base_url and space_id are the address, not - # a secret, and keeping them out is what lets one credential - # serve agents pointing at different spaces. + # Only the key goes to the vault: nexus_base_url and space_id + # are the address, and keeping them out is what lets one + # credential serve agents pointing at different spaces. api_key = _apply_vault_refs( {"credential_refs": _nexus_credential_ref(knowledge_nexus_config)}, {"nexus_api_key": api_key}, diff --git a/tests/unit/services/test_integration_credential_resolution.py b/tests/unit/services/test_integration_credential_resolution.py index b039c67..1ae14ae 100644 --- a/tests/unit/services/test_integration_credential_resolution.py +++ b/tests/unit/services/test_integration_credential_resolution.py @@ -1,4 +1,4 @@ -"""Vault resolution for external agent integrations (EVO-2250, story 2.3). +"""Vault resolution for external agent integrations. The runtime resolves a credential BY ID only. Precedence between scopes has a single owner in the CRM (Ai::IntegrationCredentialResolver, story 2.2), and diff --git a/tests/unit/services/test_mcp_headers_call_path.py b/tests/unit/services/test_mcp_headers_call_path.py index 0e34feb..3f2777b 100644 --- a/tests/unit/services/test_mcp_headers_call_path.py +++ b/tests/unit/services/test_mcp_headers_call_path.py @@ -1,14 +1,8 @@ """The CALL PATH of vault header resolution for remote MCP servers. -EVO-2250, review of 2026-07-29 (blocker 1). `_resolve_mcp_headers` existed, -was unit tested, and had ZERO callers: `git grep` returned only its definition. -The real assembly point built `{"url": ..., "headers": custom_server.headers}` -raw, so a remote MCP configured with `credential_refs` and no inline header went -out UNAUTHENTICATED. - -These tests assert on the SOURCE of the assembly point, not on the function. -A unit test over the function is exactly what let the defect ship: it passed -while nothing called it. +These assert on the SOURCE of the assembly point, not on the resolver. A +resolver with no caller passes its own unit tests while a remote MCP configured +with `credential_refs` and no inline header goes out UNAUTHENTICATED. """ import pathlib @@ -66,10 +60,8 @@ def test_the_resolver_has_a_caller_outside_its_own_definition(source: str) -> No def test_official_mcp_env_is_resolved_through_the_vault(source: str) -> None: - """Blocker 2: env vars of an official MCP server used to be copied verbatim. - - The read side must go through the resolver so a vault reference is honoured - once the writer exists. + """The env vars of an official MCP server must go through the resolver, not + be copied verbatim, or a vault reference is never honoured. """ match = re.search( r"if \"env\" not in server_config:(?P.*?)\n\n", @@ -118,14 +110,10 @@ def test_masking_covers_every_non_safe_header_name(context_source: str) -> None: ) -# Review of the Reviewer's own half: the env var key on the AGENT's MCP entry is -# `environments`, not `envs`. -# -# Evidence across the pipeline: the front's MCPConfigDialog writes -# `environments`, and the core validates and REWRITES the persisted entry with -# exactly {id, environments, tools} (config_processor.go:266,278-282). So a -# guard on `envs` never fires for an agent configured through the screen, and the -# resolution stayed inert even with the call wired. +# The env var key on the AGENT's MCP entry is `environments`, not `envs`: the +# front writes `environments` and the core rewrites the persisted entry as +# exactly {id, environments, tools}. A guard on `envs` never fires for an agent +# configured through the screen, leaving the resolution inert even when wired. def test_env_resolution_reads_the_key_the_pipeline_actually_writes(source: str) -> None: guard = re.search( r"(?Pif server\.get\([^\n]*\n?[^\n]*\):)\s*\n\s*if \"env\" not in server_config", diff --git a/tests/unit/services/test_vault_header_resolution.py b/tests/unit/services/test_vault_header_resolution.py index 4301f7b..e60b449 100644 --- a/tests/unit/services/test_vault_header_resolution.py +++ b/tests/unit/services/test_vault_header_resolution.py @@ -1,4 +1,4 @@ -"""Vault resolution for tool and MCP headers and env vars (EVO-2250, story 2.4). +"""Vault resolution for tool and MCP headers and env vars. Same rule as story 2.3: resolution here is BY ID, the inline value stays the fallback, and precedence between scopes has a single owner in the CRM. @@ -140,7 +140,7 @@ def test_the_original_map_is_never_mutated(): def test_mcp_context_no_longer_pollutes_os_environ(): - """Negative proof for the os.environ leak (story 2.4 chose to FIX it). + """Negative proof for the os.environ leak. Each MCP env var used to be written into the processor's own os.environ, on top of being passed to the child. The write was redundant and never undone, @@ -189,16 +189,15 @@ def test_mcp_service_logs_header_names_not_values(): assert leaking == [], f"header values still reach the logs: {leaking}" -# AC7 end to end, against the FINAL contract confirmed with the Reviewer: +# The end-to-end contract for an official MCP server: # # agent.config.mcp_servers[i] = { # id, environments: {VAR: ''}, credential_refs: {VAR: ''}, tools # } # -# The front writes it (9f63077), the core lets it through the processing -# allowlist (62830b7), and this asserts the processor's half actually resolves -# it. A test over the resolver alone would pass even with the wrong key, which is -# exactly how this shipped broken the first time. +# The front writes it, the core carries it through the processing allowlist, and +# this asserts the processor resolves it. A test over the resolver alone passes +# even with the wrong key. def test_official_mcp_env_resolves_end_to_end_with_the_persisted_shape(): import importlib.util as _il import pathlib as _pl