diff --git a/src/models/models.py b/src/models/models.py index 743b9ce..6dfd8bb 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. + 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. + 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/schemas/agent_config.py b/src/schemas/agent_config.py index 127944b..f4a400c 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. + 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/agents/external_agent_builder.py b/src/services/adk/agents/external_agent_builder.py index c94e708..e50ce11 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." ) + # 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/custom_tools.py b/src/services/adk/custom_tools.py index 1b58cc9..6c22e66 100644 --- a/src/services/adk/custom_tools.py +++ b/src/services/adk/custom_tools.py @@ -61,6 +61,36 @@ 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 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: + 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 +102,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", {}) + # 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")) error_handling = tool_config.get("error_handling", {}) diff --git a/src/services/adk/integration_credentials.py b/src/services/adk/integration_credentials.py new file mode 100644 index 0000000..9cd1a26 --- /dev/null +++ b/src/services/adk/integration_credentials.py @@ -0,0 +1,225 @@ +"""Resolves a consumer's credential from the integration vault, which holds the +secret encrypted and is pointed at 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. + +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 +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. 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",), + "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 does NOT open its own connection, + unlike the raw-psycopg2 pattern that bypasses 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 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) + + 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 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. + + 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, and raises when + there is none. + """ + 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, + 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/src/services/adk/mcp_context.py b/src/services/adk/mcp_context.py index e35e9db..a42a763 100644 --- a/src/services/adk/mcp_context.py +++ b/src/services/adk/mcp_context.py @@ -45,6 +45,36 @@ MCP_CONNECTION_TIMEOUT = settings.MCP_CONNECTION_TIMEOUT +# 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", + "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 +130,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( @@ -134,11 +151,10 @@ 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. + # 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: @@ -222,12 +238,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 5946497..ba498a2 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 - if server.get("envs"): + # ⚠️ The key is `environments`, not `envs`: the screen + # 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"] = {} - 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 @@ -426,7 +429,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. + f"Header names: {list(server_config.get('headers', {}).keys())}" ) cached_tools = await mcp_tool_cache.get_server_tools( @@ -485,7 +490,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}" ) @@ -738,10 +743,12 @@ def handle_task_exception(task): ) continue - # Convert to the format expected by mcp_context + # 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": custom_server.headers or {}, + "headers": _resolve_mcp_headers(custom_server, db), } logger.info( @@ -951,3 +958,56 @@ async def build_tools( raise DeprecationWarning( "build_tools is deprecated and keeps connections open. Use build_lazy_tools instead." ) + + +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 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. + envs = server.get("environments") or 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. + + 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..49748ab 100644 --- a/src/services/adk/tool_builder.py +++ b/src/services/adk/tool_builder.py @@ -40,6 +40,43 @@ logger = setup_logger(__name__) + + +def _nexus_credential_ref(config): + """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 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: + 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 +88,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. + 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 +463,13 @@ def build_tools( knowledge_nexus_config.get("nexus_api_key") or knowledge_nexus_config.get("apiKey") ) + # 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}, + ).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_integration_credential_resolution.py b/tests/unit/services/test_integration_credential_resolution.py new file mode 100644 index 0000000..1ae14ae --- /dev/null +++ b/tests/unit/services/test_integration_credential_resolution.py @@ -0,0 +1,224 @@ +"""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 +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" 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..3f2777b --- /dev/null +++ b/tests/unit/services/test_mcp_headers_call_path.py @@ -0,0 +1,136 @@ +"""The CALL PATH of vault header resolution for remote MCP servers. + +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 +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: + """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", + 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" + ) + + +# 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", + 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" + ) 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..e60b449 --- /dev/null +++ b/tests/unit/services/test_vault_header_resolution.py @@ -0,0 +1,244 @@ +"""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. +""" + +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. + + 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}" + + +# 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, 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 + + 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"