diff --git a/.agents/skills/llmobs-integrations/references/implementation-guide.md b/.agents/skills/llmobs-integrations/references/implementation-guide.md index 82fdb2fda5a..0c5c6474e59 100644 --- a/.agents/skills/llmobs-integrations/references/implementation-guide.md +++ b/.agents/skills/llmobs-integrations/references/implementation-guide.md @@ -141,6 +141,8 @@ Oversized inline media makes `_writer._truncate_span_event` blank the span's ent Do not dump raw `kwargs` into LLMObs metadata. Prefer shared helpers such as `get_metadata_from_kwargs()` and provider-specific helpers in `ddtrace/llmobs/_integrations/utils.py` when they apply. They filter provider sentinel defaults such as OpenAI `Omit`/`NotGiven`, serialize SDK objects through `load_data_value()`, and keep metadata limited to supported fields. `_annotate_llmobs_span_data()` coerces metadata keys to strings, but integrations should still avoid sending noisy or high-cardinality raw objects. +Never copy tool or model configurations verbatim either. Hosted MCP tools carry credentials in `headers`, `authorization` and the server URL (userinfo, path, query), and container network policies carry `domain_secrets` values: record tools through `safe_tool_config()` in `ddtrace/llmobs/_integrations/utils.py`. Model settings can carry `extra_headers`, `extra_query`, `extra_body` and `extra_args`: filter them with `ALLOWED_MODEL_SETTINGS_KEYS` and `is_flat_scalar_value()` from `agent_manifest.py`. + ### Token Field Mapping | Library | Input | Output | Cache Fields | diff --git a/ddtrace/internal/utils/http.py b/ddtrace/internal/utils/http.py index 1a66608de47..7afe0ab1028 100644 --- a/ddtrace/internal/utils/http.py +++ b/ddtrace/internal/utils/http.py @@ -181,6 +181,29 @@ def strip_query_string(url: str) -> str: return h + fs + f +_DEFAULT_PORTS = {"http": 80, "https": 443, "ws": 80, "wss": 443} + + +def url_origin(url: str) -> Optional[str]: + """Reduce url to scheme://host[:port], dropping userinfo, path, query and fragment, which can hold credentials. + + Scheme and host are lowercased and a default port dropped. Returns None without scheme or host. + """ + try: + parts = parse.urlsplit(url) + host = parts.hostname + port = parts.port + except (TypeError, ValueError): + return None + if not parts.scheme or not host: + return None + scheme = parts.scheme.lower() + if ":" in host: + host = f"[{host}]" + netloc = host if port is None or _DEFAULT_PORTS.get(scheme) == port else f"{host}:{port}" + return f"{scheme}://{netloc}" + + def _redact_query_string_match(match: re.Match[bytes]) -> bytes: return (match.group(1) or b"") + b"" diff --git a/ddtrace/llmobs/_integrations/openai_agents.py b/ddtrace/llmobs/_integrations/openai_agents.py index 5442cead821..706b852220e 100644 --- a/ddtrace/llmobs/_integrations/openai_agents.py +++ b/ddtrace/llmobs/_integrations/openai_agents.py @@ -14,10 +14,14 @@ from ddtrace.llmobs._constants import DISPATCH_ON_TOOL_CALL_OUTPUT_USED from ddtrace.llmobs._constants import OAI_HANDOFF_TOOL_ARG from ddtrace.llmobs._constants import ROOT_PARENT_ID +from ddtrace.llmobs._integrations.agent_manifest import ALLOWED_MODEL_SETTINGS_KEYS +from ddtrace.llmobs._integrations.agent_manifest import is_flat_scalar_value from ddtrace.llmobs._integrations.base import BaseLLMIntegration from ddtrace.llmobs._integrations.utils import LLMObsTraceInfo from ddtrace.llmobs._integrations.utils import OaiSpanAdapter from ddtrace.llmobs._integrations.utils import OaiTraceAdapter +from ddtrace.llmobs._integrations.utils import safe_mcp_tool_config +from ddtrace.llmobs._integrations.utils import safe_tool_config from ddtrace.llmobs._utils import _annotate_llmobs_span_data from ddtrace.llmobs._utils import _get_nearest_llmobs_ancestor from ddtrace.llmobs._utils import get_llmobs_parent_id @@ -356,7 +360,16 @@ def _extract_model_settings_from_agent(self, agent): if not isinstance(model_settings, dict): model_settings = getattr(model_settings, "__dict__", None) - return load_data_value(model_settings) + settings = load_data_value(model_settings) + if not isinstance(settings, dict): + return None + # extra_headers, extra_query, extra_body and extra_args can carry credentials, so only + # inference params are recorded. + return { + key: value + for key, value in settings.items() + if key in ALLOWED_MODEL_SETTINGS_KEYS and is_flat_scalar_value(value) + } def _extract_tools_from_agent(self, agent): if not hasattr(agent, "tools") or not agent.tools: @@ -391,15 +404,15 @@ def _extract_tools_from_agent(self, agent): tool_dict["on_safety_check"] = tool.on_safety_check elif tool_name == "code_interpreter": if hasattr(tool, "tool_config"): - tool_dict["tool_config"] = tool.tool_config + tool_dict["tool_config"] = safe_tool_config(tool.tool_config) elif tool_name == "hosted_mcp": if hasattr(tool, "tool_config"): - tool_dict["tool_config"] = tool.tool_config + tool_dict["tool_config"] = safe_mcp_tool_config(tool.tool_config) if hasattr(tool, "on_approval_request"): tool_dict["on_approval_request"] = tool.on_approval_request elif tool_name == "image_generation": if hasattr(tool, "tool_config"): - tool_dict["tool_config"] = tool.tool_config + tool_dict["tool_config"] = safe_tool_config(tool.tool_config) elif tool_name == "local_shell": if hasattr(tool, "executor"): tool_dict["executor"] = tool.executor diff --git a/ddtrace/llmobs/_integrations/utils.py b/ddtrace/llmobs/_integrations/utils.py index 5897416891d..beb1d1d0afa 100644 --- a/ddtrace/llmobs/_integrations/utils.py +++ b/ddtrace/llmobs/_integrations/utils.py @@ -14,6 +14,7 @@ from ddtrace.internal.utils.formats import _get_attr from ddtrace.internal.utils.formats import format_trace_id from ddtrace.internal.utils.formats import safe_load_json +from ddtrace.internal.utils.http import url_origin from ddtrace.llmobs._constants import AUDIO_FALLBACK_MARKER from ddtrace.llmobs._constants import DISPATCH_ON_LLM_TOOL_CHOICE from ddtrace.llmobs._constants import DISPATCH_ON_TOOL_CALL_OUTPUT_USED @@ -1500,6 +1501,64 @@ def _openai_decision_answer_to_dict(answer: Any) -> dict[str, Any]: return {} +# OpenAI hosted MCP tool fields that carry no credential; headers, authorization and the URL path, +# query and fragment can, so only these and the URL origin are recorded. +_SAFE_OPENAI_MCP_TOOL_FIELDS = ( + "type", + "server_label", + "server_description", + "connector_id", + "require_approval", + "allowed_tools", +) + + +def safe_mcp_tool_config(tool: Any, *, allowlist: Iterable[str] = _SAFE_OPENAI_MCP_TOOL_FIELDS) -> dict[str, Any]: + """Hosted MCP tool configuration keeping only the allowlist fields, with server_url reduced to its origin.""" + config = load_data_value(tool) + if not isinstance(config, dict): + return {} + safe = {key: config[key] for key in allowlist if config.get(key) is not None} + server_url = config.get("server_url") + origin = url_origin(server_url) if isinstance(server_url, str) else None + if origin: + safe["server_url"] = origin + return safe + + +def _without_domain_secret_values(value: Any) -> Any: + """Copy of value where each container network policy domain secret keeps only its domain and name.""" + if isinstance(value, dict): + return { + key: [ + {field: secret[field] for field in ("domain", "name") if field in secret} + for secret in item + if isinstance(secret, dict) + ] + if key == "domain_secrets" and isinstance(item, list) + else _without_domain_secret_values(item) + for key, item in value.items() + } + if isinstance(value, list): + return [_without_domain_secret_values(item) for item in value] + return value + + +def safe_tool_config(tool: Any, *, allowlist: Iterable[str] = _SAFE_OPENAI_MCP_TOOL_FIELDS) -> Any: + """Tool configuration as recorded: MCP tools keep the allowlist fields, domain secret values are dropped.""" + config = load_data_value(tool) + if isinstance(config, dict) and config.get("type") == "mcp": + return safe_mcp_tool_config(config, allowlist=allowlist) + return _without_domain_secret_values(config) + + +def _safe_response_tools(tools: Any, *, allowlist: Iterable[str] = _SAFE_OPENAI_MCP_TOOL_FIELDS) -> Any: + """Response tools as recorded in metadata, each reduced by safe_tool_config.""" + if not isinstance(tools, list): + return tools + return [safe_tool_config(tool, allowlist=allowlist) for tool in tools] + + def _openai_get_tool_definitions(tools: list[Any]) -> list[ToolDefinition]: tool_definitions = [] for tool in tools: @@ -1806,7 +1865,8 @@ def llmobs_metadata(self) -> Optional[dict[str, Any]]: if hasattr(self.response, field): value = getattr(self.response, field) if value is not None: - metadata[field] = load_data_value(value) + value = load_data_value(value) + metadata[field] = _safe_response_tools(value) if field == "tools" else value if hasattr(self.response, "text") and self.response.text: metadata["text"] = load_data_value(self.response.text) diff --git a/releasenotes/notes/fix-llmobs-openai-agents-mcp-credentials-c6c2e23460dd3806.yaml b/releasenotes/notes/fix-llmobs-openai-agents-mcp-credentials-c6c2e23460dd3806.yaml new file mode 100644 index 00000000000..4dc7210608e --- /dev/null +++ b/releasenotes/notes/fix-llmobs-openai-agents-mcp-credentials-c6c2e23460dd3806.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + LLM Observability: Fixes an issue where OpenAI Agents spans recorded internal information. + Only non-secret settings and the scheme, host and port of MCP server URLs are now recorded. diff --git a/tests/contrib/openai_agents/test_openai_agents_llmobs.py b/tests/contrib/openai_agents/test_openai_agents_llmobs.py index 78f950cea5f..1536cd9b58c 100644 --- a/tests/contrib/openai_agents/test_openai_agents_llmobs.py +++ b/tests/contrib/openai_agents/test_openai_agents_llmobs.py @@ -1,11 +1,27 @@ import asyncio +import json from typing import Optional from unittest import mock import openai import pytest + +try: + from openai.types.responses.tool import Mcp +except ImportError: # openai versions without hosted MCP tools + Mcp = None +try: + from openai.types.responses.function_shell_tool import FunctionShellTool + from openai.types.responses.tool import CodeInterpreter +except ImportError: # openai versions without container network policies + CodeInterpreter = FunctionShellTool = None + from ddtrace.internal.utils.version import parse_version +from ddtrace.llmobs._integrations.openai_agents import OpenAIAgentsIntegration +from ddtrace.llmobs._integrations.utils import OaiSpanAdapter +from ddtrace.llmobs._integrations.utils import safe_mcp_tool_config +from ddtrace.llmobs._integrations.utils import safe_tool_config from ddtrace.llmobs._utils import _get_llmobs_data_metastruct from ddtrace.llmobs._utils import get_llmobs_span_name from tests.llmobs._utils import _assert_span_link @@ -808,3 +824,160 @@ async def mock_func(*args, **kwargs): args=(simple_agent,), kwargs={"input": "What is the capital of France?"}, ) + + +# Fake values shaped like real credentials. Prefixes are split so secret scanners do not flag them. +_MCP_PASSWORD = "Tr0ub4dor-3xample!" +_MCP_PATH_TOKEN = "NGQ3ZTk1YjItOGM0MS00ZTFhLWI2ZDMtOWYyYTdjMWU1ZDQ4" +_MCP_QUERY_KEY = "sk" + "-proj-" + "Zt8yQm3vLk2pW9xRn5bC7dF1gHj4sA6eUo0iTqYwErT" +_MCP_HEADER_TOKEN = "xo" + "xb-" + "1234567890123-4567890123456-Hq7Lm2Xw9Rt4Yb8Nc3Vd6Js1" +_MCP_OAUTH_TOKEN = "ya" + "29." + "a0AfB_byC7kq2ZxW9mLpT4vNbY8cHdF3sJ6aQe1uXo0iR" +_MCP_CREDENTIALS = (_MCP_PASSWORD, _MCP_PATH_TOKEN, _MCP_QUERY_KEY, _MCP_HEADER_TOKEN, _MCP_OAUTH_TOKEN, "/api/mcp/s/") +_LEAKY_MCP_TOOL_CONFIG = { + "type": "mcp", + "server_label": "zapier", + "server_description": "Zapier actions", + "server_url": f"https://svc:{_MCP_PASSWORD}@MCP.Zapier.com:443/api/mcp/s/{_MCP_PATH_TOKEN}/mcp?api_key={_MCP_QUERY_KEY}#x", + "headers": {"Authorization": f"Bearer {_MCP_HEADER_TOKEN}", "Cookie": "sessionid=8f2b"}, + "authorization": _MCP_OAUTH_TOKEN, + "require_approval": "never", + "allowed_tools": ["send_email"], +} +_SAFE_MCP_TOOL_CONFIG = { + "type": "mcp", + "server_label": "zapier", + "server_description": "Zapier actions", + "server_url": "https://mcp.zapier.com", + "require_approval": "never", + "allowed_tools": ["send_email"], +} + + +def _assert_no_mcp_credentials(recorded): + serialized = json.dumps(recorded, default=str) + for credential in _MCP_CREDENTIALS + ("sessionid",): + assert credential not in serialized + + +def test_agent_manifest_keeps_only_the_safe_view_of_hosted_mcp_tools(agents): + """A HostedMCPTool config is the application's own, so nothing redacts its credentials upstream.""" + agent = agents.Agent( + name="mcp agent", + tools=[agents.HostedMCPTool(tool_config=dict(_LEAKY_MCP_TOOL_CONFIG))], + ) + + (tool,) = OpenAIAgentsIntegration._extract_tools_from_agent(None, agent) + + assert tool["name"] == "hosted_mcp" + assert tool["tool_config"] == _SAFE_MCP_TOOL_CONFIG + _assert_no_mcp_credentials(tool) + + +@pytest.mark.skipif(Mcp is None, reason="openai has no hosted MCP tool type") +def test_response_metadata_keeps_only_the_safe_view_of_hosted_mcp_tools(): + """OpenAI echoes response.tools with header values redacted but server_url verbatim.""" + echoed = dict(_LEAKY_MCP_TOOL_CONFIG, headers={"Authorization": ""}, authorization=None) + function_tool = {"type": "function", "name": "lookup", "parameters": {"type": "object"}, "strict": True} + response = mock.Mock(spec=["tools"], tools=[Mcp.model_validate(echoed), function_tool]) + span = mock.Mock(span_data=mock.Mock(type="response", response=response)) + + metadata = OaiSpanAdapter(span).llmobs_metadata + + assert metadata["tools"] == [_SAFE_MCP_TOOL_CONFIG, function_tool] + _assert_no_mcp_credentials(metadata) + + +@pytest.mark.parametrize( + "server_url,expected", + [ + (None, None), + ("not a url", None), + ("https://mcp.example.com:notaport/mcp", None), + ], +) +def test_safe_mcp_tool_config_omits_unusable_urls(server_url, expected): + safe = safe_mcp_tool_config({"type": "mcp", "server_label": "x", "server_url": server_url}) + + assert safe.get("server_url") == expected + assert safe["server_label"] == "x" + + +_DOMAIN_SECRET = "fake-domain-secret-" + "Zt8yQm3vLk2pW9xRn5bC" +_NETWORK_POLICY = { + "type": "allowlist", + "allowed_domains": ["api.example.com"], + "domain_secrets": [{"domain": "api.example.com", "name": "API_KEY", "value": _DOMAIN_SECRET}], +} +_SAFE_NETWORK_POLICY = { + "type": "allowlist", + "allowed_domains": ["api.example.com"], + "domain_secrets": [{"domain": "api.example.com", "name": "API_KEY"}], +} + + +def test_agent_manifest_drops_code_interpreter_domain_secret_values(agents): + tool_config = {"type": "code_interpreter", "container": {"type": "auto", "network_policy": _NETWORK_POLICY}} + agent = agents.Agent(name="code agent", tools=[agents.CodeInterpreterTool(tool_config=tool_config)]) + + (tool,) = OpenAIAgentsIntegration._extract_tools_from_agent(None, agent) + + assert tool["tool_config"] == { + "type": "code_interpreter", + "container": {"type": "auto", "network_policy": _SAFE_NETWORK_POLICY}, + } + assert _DOMAIN_SECRET not in json.dumps(tool, default=str) + + +def test_agent_manifest_model_settings_keep_only_inference_params(agents): + """extra_headers, extra_query, extra_body and extra_args can carry proxy or provider credentials.""" + settings = { + "temperature": 0.2, + "max_tokens": 512, + "extra_headers": {"Authorization": f"Bearer {_MCP_HEADER_TOKEN}", "api-key": _MCP_QUERY_KEY}, + "extra_query": {"api-version": "2024-10-21", "sig": _MCP_PATH_TOKEN}, + "extra_body": {"metadata": {"secret": _MCP_OAUTH_TOKEN}}, + } + if "extra_args" in agents.ModelSettings.__dataclass_fields__: + settings["extra_args"] = {"api_key": _MCP_PASSWORD} + agent = agents.Agent(name="settings agent", model_settings=agents.ModelSettings(**settings)) + + recorded = OpenAIAgentsIntegration._extract_model_settings_from_agent(None, agent) + + assert recorded["temperature"] == 0.2 + assert recorded["max_tokens"] == 512 + assert not {"extra_headers", "extra_query", "extra_body", "extra_args"} & set(recorded) + _assert_no_mcp_credentials(recorded) + + +@pytest.mark.skipif(CodeInterpreter is None, reason="openai has no container network policies") +def test_response_metadata_drops_domain_secret_values(): + """Applied to every response tool, in case OpenAI echoes container policies like it echoes server_url.""" + code_interpreter = CodeInterpreter.model_validate( + {"type": "code_interpreter", "container": {"type": "auto", "network_policy": _NETWORK_POLICY}} + ) + shell = FunctionShellTool.model_validate( + {"type": "shell", "environment": {"type": "container_auto", "network_policy": _NETWORK_POLICY}} + ) + response = mock.Mock(spec=["tools"], tools=[code_interpreter, shell]) + span = mock.Mock(span_data=mock.Mock(type="response", response=response)) + + metadata = OaiSpanAdapter(span).llmobs_metadata + + code_interpreter_tool, shell_tool = metadata["tools"] + assert code_interpreter_tool["container"]["network_policy"] == _SAFE_NETWORK_POLICY + assert shell_tool["environment"]["network_policy"] == _SAFE_NETWORK_POLICY + assert _DOMAIN_SECRET not in json.dumps(metadata, default=str) + + +def test_safe_tool_config_takes_a_custom_mcp_allowlist(): + """Other integrations can record a different MCP field set without losing the URL origin rule.""" + config = dict(_LEAKY_MCP_TOOL_CONFIG) + + safe = safe_tool_config(config, allowlist=("type", "server_label")) + + assert safe == {"type": "mcp", "server_label": "zapier", "server_url": "https://mcp.zapier.com"} + assert safe_mcp_tool_config(config, allowlist=("server_label",)) == { + "server_label": "zapier", + "server_url": "https://mcp.zapier.com", + } + _assert_no_mcp_credentials(safe) diff --git a/tests/tracer/test_utils.py b/tests/tracer/test_utils.py index a550dfd08f7..6f7535dbcbe 100644 --- a/tests/tracer/test_utils.py +++ b/tests/tracer/test_utils.py @@ -17,6 +17,7 @@ from ddtrace.internal.utils.formats import flatten_key_value from ddtrace.internal.utils.formats import is_sequence from ddtrace.internal.utils.formats import parse_tags_str +from ddtrace.internal.utils.http import url_origin from ddtrace.internal.utils.http import w3c_get_dd_list_member from ddtrace.internal.utils.importlib import func_name from ddtrace.trace import Context @@ -673,3 +674,25 @@ def test_hourglass_turn(): def test_hourglass_sorting(): """Test that we can sort hourglasses.""" sorted(time.HourGlass(1) for _ in range(100)) + + +@pytest.mark.parametrize( + "url,expected", + [ + ("https://mcp.example.com/mcp", "https://mcp.example.com"), + ("HTTPS://MCP.Example.COM:443/Mcp/", "https://mcp.example.com"), + ("http://mcp.example.com:80/sse", "http://mcp.example.com"), + ("http://mcp.example.com:8080/sse", "http://mcp.example.com:8080"), + ("https://user:secret@mcp.example.com/mcp?token=abc#frag", "https://mcp.example.com"), + ("https://mcp.example.com/api/mcp/s/s3cr3t-t0ken/mcp", "https://mcp.example.com"), + ("https://mcp.example.com/mcp;params?q=1", "https://mcp.example.com"), + ("http://[::1]:9000/mcp", "http://[::1]:9000"), + ("https://mcp.example.com", "https://mcp.example.com"), + ("https://mcp.example.com:notaport/mcp", None), + ("/relative/path", None), + ("not a url", None), + ("", None), + ], +) +def test_url_origin(url, expected): + assert url_origin(url) == expected