diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 092bc8ad..34f59722 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -396,8 +396,10 @@ jobs: for i in $(seq 1 60); do # Any HTTP response (including 405 on GET to a POST-only endpoint) # means the server is up and accepting connections. + # ``||`` runs on the assignment so curl's "000" stdout and the + # fallback don't concatenate when the connection is refused. HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" --max-time 1 \ - http://127.0.0.1:3001/mcp 2>/dev/null || echo "000") + http://127.0.0.1:3001/mcp 2>/dev/null) || HTTP_CODE="000" if [ "$HTTP_CODE" != "000" ]; then echo "Seller agent ready (HTTP ${HTTP_CODE}, pid ${AGENT_PID})" break @@ -550,8 +552,10 @@ jobs: # renames or removes a specific network. The endpoint is # always present on the harness-side mock. for i in $(seq 1 120); do + # ``||`` runs on the assignment so curl's "000" stdout and the + # fallback don't concatenate when the connection is refused. HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" --max-time 1 \ - http://127.0.0.1:4503/_debug/traffic 2>/dev/null || echo "000") + http://127.0.0.1:4503/_debug/traffic 2>/dev/null) || HTTP_CODE="000" if [ "$HTTP_CODE" = "200" ]; then echo "Upstream mock ready (HTTP 200, pid $MOCK_PID, $i polls)" break @@ -585,8 +589,10 @@ jobs: SELLER_PID=$! echo "SELLER_PID=$SELLER_PID" >> "$GITHUB_ENV" for i in $(seq 1 60); do + # ``||`` runs on the assignment so curl's "000" stdout and the + # fallback don't concatenate when the connection is refused. HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" --max-time 1 \ - http://127.0.0.1:3001/mcp 2>/dev/null || echo "000") + http://127.0.0.1:3001/mcp 2>/dev/null) || HTTP_CODE="000" if [ "$HTTP_CODE" != "000" ]; then echo "Seller ready (HTTP ${HTTP_CODE}, pid ${SELLER_PID})" break diff --git a/examples/seller_agent.py b/examples/seller_agent.py index 61f599b3..979687b3 100644 --- a/examples/seller_agent.py +++ b/examples/seller_agent.py @@ -43,6 +43,38 @@ PORT = int(os.environ.get("ADCP_PORT") or os.environ.get("PORT") or 3001) AGENT_URL = f"http://localhost:{PORT}/mcp" +# Spec-valid values for ``Product.channels`` (the canonical +# ``MediaChannelSchema`` enum from schemas/cache/enums/channels.json). +# Storyboard fixtures occasionally seed legacy channel names ("video") +# that aren't in the enum; ``seed_product`` filters incoming fixture +# channels against this set so the demo seller doesn't echo invalid +# values back through ``get_products`` and trip strict response +# validation. +_VALID_CHANNELS: frozenset[str] = frozenset( + { + "display", + "olv", + "social", + "search", + "ctv", + "linear_tv", + "radio", + "streaming_audio", + "podcast", + "dooh", + "ooh", + "print", + "cinema", + "email", + "gaming", + "retail_media", + "influencer", + "affiliate", + "product_placement", + "sponsored_intelligence", + } +) + accounts: dict[str, dict[str, Any]] = {} media_buys: dict[str, dict[str, Any]] = {} creatives: dict[str, dict[str, Any]] = {} @@ -769,6 +801,17 @@ async def seed_product( data = dict(fixture or {}) pid = product_id or data.get("product_id") or f"seeded-{uuid.uuid4().hex[:8]}" data["product_id"] = pid + # Filter ``channels`` to spec-valid values from the canonical + # ``MediaChannelSchema`` enum. Upstream storyboard fixtures + # occasionally ship legacy names like ``"video"`` that aren't + # in the enum; surfacing them through get_products would fail + # strict response validation. + if "channels" in data: + valid = [c for c in data.get("channels") or [] if c in _VALID_CHANNELS] + if valid: + data["channels"] = valid + else: + data.pop("channels", None) # Ensure schema-required fields are present so downstream validation # passes even when the runner sends a minimal fixture with only # product_id. Defaults are spec-valid (non-empty arrays where diff --git a/examples/v3_reference_seller/src/app.py b/examples/v3_reference_seller/src/app.py index c01a9a36..6bf8847b 100644 --- a/examples/v3_reference_seller/src/app.py +++ b/examples/v3_reference_seller/src/app.py @@ -90,6 +90,14 @@ async def _bootstrap_schema(engine) -> None: """ async with engine.begin() as conn: await conn.run_sync(Base.metadata.create_all) + # asyncpg binds connection-internal Future objects to the loop + # they were opened on. Bootstrapping via ``asyncio.run`` runs on + # a transient loop that closes when ``asyncio.run`` returns; if + # those connections stay in the pool, uvicorn's own loop trips + # ``RuntimeError: got Future attached to a different loop`` on + # the first request. Dispose so uvicorn opens a fresh pool on + # its own loop. + await engine.dispose() def main() -> None: @@ -170,6 +178,27 @@ def main() -> None: validation=ValidationHookConfig(requests="strict", responses="strict"), mock_ad_server=mock_ad_server, enable_debug_endpoints=True, + # The reference platform doesn't emit completion webhooks — + # turn off the F12 auto-emit gate so server boot doesn't trip + # ``validate_webhook_sender_for_platform``. Adopters whose + # platforms need webhook delivery wire a + # :class:`WebhookSender` (or + # :class:`InMemoryWebhookDeliverySupervisor`) and remove this + # kwarg — see the webhook_supervisor module for the wiring + # pattern. + auto_emit_completion_webhooks=False, + # FastMCP's TransportSecurityMiddleware enforces DNS-rebinding + # protection: its default ``allowed_hosts`` accepts only + # loopback (``127.0.0.1:*``, ``localhost:*``, ``[::1]:*``), so + # subdomain hosts like ``acme.localhost:3001`` are rejected + # with ``421 Misdirected Request``. ``SubdomainTenantMiddleware`` + # above already validates the Host header against the seeded + # tenant table — that's the load-bearing host check for this + # seller. Disabling the MCP-layer check avoids duplicating + # the same validation against a static, hard-to-extend list. + # Adopters that don't run a tenant-aware ASGI middleware leave + # this kwarg unset to keep the FastMCP defaults active. + enable_dns_rebinding_protection=False, ) diff --git a/examples/v3_reference_seller/src/tenant_router.py b/examples/v3_reference_seller/src/tenant_router.py index 56ca8f90..84e9eafc 100644 --- a/examples/v3_reference_seller/src/tenant_router.py +++ b/examples/v3_reference_seller/src/tenant_router.py @@ -49,6 +49,13 @@ def __init__( self._cache_lock = asyncio.Lock() async def resolve(self, host: str) -> Tenant | None: + # The middleware passes the raw Host header. RFC 7230 makes it + # case-insensitive and lets the client include ``:port``; the + # Protocol docstring is explicit that implementations strip the + # port suffix as needed. Normalize before the cache lookup AND + # the DB query so ``acme.localhost:3001`` resolves the same + # row as the seeded ``acme.localhost``. + host = host.strip().lower().split(":", 1)[0] # Bounded FIFO cache — when full, the oldest insertion is # evicted regardless of access frequency. Fine for stable # tenant sets under ``cache_size``; adopters with churn or diff --git a/examples/v3_reference_seller/tests/test_smoke.py b/examples/v3_reference_seller/tests/test_smoke.py index b7adae3b..573e4584 100644 --- a/examples/v3_reference_seller/tests/test_smoke.py +++ b/examples/v3_reference_seller/tests/test_smoke.py @@ -102,6 +102,41 @@ def scalar_one_or_none(self): assert result is None +@pytest.mark.asyncio +async def test_tenant_router_strips_port_and_lowercases_host() -> None: + """The middleware passes the raw Host header. RFC 7230 makes it + case-insensitive and lets the client include ``:port``; the + Protocol docstring is explicit that implementations strip the + port suffix as needed. ``ACME.localhost:3001`` and + ``acme.localhost`` MUST hit the same DB row.""" + from src.tenant_router import SqlSubdomainTenantRouter + + captured: list[str] = [] + + class _CapturingSession: + async def __aenter__(self): + return self + + async def __aexit__(self, *args): + return None + + async def execute(self, stmt): + captured.append(str(stmt.compile(compile_kwargs={"literal_binds": True}))) + + class _Result: + def scalar_one_or_none(self): + return None + + return _Result() + + router = SqlSubdomainTenantRouter(sessionmaker=lambda: _CapturingSession()) # type: ignore[arg-type] + await router.resolve("ACME.localhost:3001") + assert captured, "expected a SQL execute" + assert ( + "'acme.localhost'" in captured[-1] + ), f"router did not normalize host before query: {captured[-1]!r}" + + @pytest.mark.asyncio async def test_buyer_registry_returns_none_without_tenant() -> None: """Without a tenant context (ContextVar unset), the registry diff --git a/src/adcp/server/mcp_tools.py b/src/adcp/server/mcp_tools.py index 1a5d0df2..51e84e67 100644 --- a/src/adcp/server/mcp_tools.py +++ b/src/adcp/server/mcp_tools.py @@ -1553,6 +1553,15 @@ def _generate_pydantic_output_schemas() -> dict[str, dict[str, Any]]: tool_name, ) continue + # MCP requires ``outputSchema`` root-level ``type: "object"`` — + # the schema describes ``CallToolResult.structuredContent`` which + # is always a JSON object. Discriminated-union responses + # (CreateMediaBuyResponse, AcquireRightsResponse, etc.) come + # back from Pydantic as ``{"anyOf": [...]}`` with no ``type``, + # which Zod-validated MCP clients reject. Every variant in the + # union is itself an object, so adding ``"type": "object"`` + # at the root is semantically equivalent and MCP-spec-conformant. + schema.setdefault("type", "object") schemas[tool_name] = schema return schemas diff --git a/src/adcp/server/serve.py b/src/adcp/server/serve.py index 3c0d22f4..458347cb 100644 --- a/src/adcp/server/serve.py +++ b/src/adcp/server/serve.py @@ -444,6 +444,9 @@ def serve( base_url: str | None = None, specialisms: list[str] | None = None, description: str | None = None, + allowed_hosts: Sequence[str] | None = None, + allowed_origins: Sequence[str] | None = None, + enable_dns_rebinding_protection: bool | None = None, ) -> None: """Start an MCP or A2A server from an ADCP handler or server builder. @@ -691,6 +694,9 @@ async def force_account_status(self, account_id, status): base_url=base_url, specialisms=specialisms, description=description, + allowed_hosts=allowed_hosts, + allowed_origins=allowed_origins, + enable_dns_rebinding_protection=enable_dns_rebinding_protection, ) elif transport == "both": _serve_mcp_and_a2a( @@ -713,6 +719,9 @@ async def force_account_status(self, account_id, status): base_url=base_url, specialisms=specialisms, description=description, + allowed_hosts=allowed_hosts, + allowed_origins=allowed_origins, + enable_dns_rebinding_protection=enable_dns_rebinding_protection, ) else: valid = ", ".join(sorted(("a2a", "both", "streamable-http", "sse", "stdio"))) @@ -996,6 +1005,9 @@ def _serve_mcp( base_url: str | None = None, specialisms: list[str] | None = None, description: str | None = None, + allowed_hosts: Sequence[str] | None = None, + allowed_origins: Sequence[str] | None = None, + enable_dns_rebinding_protection: bool | None = None, ) -> None: """Start an MCP server.""" mcp = create_mcp_server( @@ -1010,6 +1022,9 @@ def _serve_mcp( advertise_all=advertise_all, streaming_responses=streaming_responses, validation=validation, + allowed_hosts=allowed_hosts, + allowed_origins=allowed_origins, + enable_dns_rebinding_protection=enable_dns_rebinding_protection, ) if test_controller is not None: @@ -1199,6 +1214,9 @@ def _build_mcp_and_a2a_app( base_url: str | None = None, specialisms: list[str] | None = None, description: str | None = None, + allowed_hosts: Sequence[str] | None = None, + allowed_origins: Sequence[str] | None = None, + enable_dns_rebinding_protection: bool | None = None, ) -> Any: """Build the unified MCP+A2A ASGI app without starting a server. @@ -1233,6 +1251,9 @@ def _build_mcp_and_a2a_app( advertise_all=advertise_all, streaming_responses=streaming_responses, validation=validation, + allowed_hosts=allowed_hosts, + allowed_origins=allowed_origins, + enable_dns_rebinding_protection=enable_dns_rebinding_protection, ) if test_controller is not None: from adcp.server.test_controller import register_test_controller @@ -1336,6 +1357,9 @@ def _serve_mcp_and_a2a( base_url: str | None = None, specialisms: list[str] | None = None, description: str | None = None, + allowed_hosts: Sequence[str] | None = None, + allowed_origins: Sequence[str] | None = None, + enable_dns_rebinding_protection: bool | None = None, ) -> None: """Serve MCP and A2A on a single port via path dispatch. @@ -1377,6 +1401,9 @@ def _serve_mcp_and_a2a( base_url=base_url, specialisms=specialisms, description=description, + allowed_hosts=allowed_hosts, + allowed_origins=allowed_origins, + enable_dns_rebinding_protection=enable_dns_rebinding_protection, ) app = _apply_asgi_middleware(app, asgi_middleware) @@ -1411,6 +1438,9 @@ def create_mcp_server( advertise_all: bool = False, streaming_responses: bool = False, validation: ValidationHookConfig | None = DEFAULT_VALIDATION, + allowed_hosts: Sequence[str] | None = None, + allowed_origins: Sequence[str] | None = None, + enable_dns_rebinding_protection: bool | None = None, ) -> Any: """Create a FastMCP server from an ADCP handler without starting it. @@ -1526,6 +1556,32 @@ def create_mcp_server( # AdCP tools, which return one complete envelope per request. mcp.settings.stateless_http = True mcp.settings.json_response = True + # FastMCP's TransportSecurityMiddleware enforces DNS-rebinding + # protection: the default ``allowed_hosts`` accepts only loopback + # patterns (``127.0.0.1:*``, ``localhost:*``, ``[::1]:*``). Adopters + # serving multi-tenant subdomain hosts (``acme.example.com``, + # ``acme.localhost``) extend the list or the transport returns + # ``421 Misdirected Request`` and MCP discovery fails. Adopters + # whose outer ASGI middleware already validates hosts against a + # tenant table (e.g. :class:`SubdomainTenantMiddleware`) can set + # ``enable_dns_rebinding_protection=False`` so the MCP-layer check + # doesn't duplicate the upstream validation. + if ( + enable_dns_rebinding_protection is not None + or allowed_hosts is not None + or allowed_origins is not None + ): + from mcp.server.transport_security import TransportSecuritySettings + + if mcp.settings.transport_security is None: + mcp.settings.transport_security = TransportSecuritySettings() + ts = mcp.settings.transport_security + if enable_dns_rebinding_protection is not None: + ts.enable_dns_rebinding_protection = enable_dns_rebinding_protection + if allowed_hosts: + ts.allowed_hosts = [*ts.allowed_hosts, *allowed_hosts] + if allowed_origins: + ts.allowed_origins = [*ts.allowed_origins, *allowed_origins] _register_handler_tools( mcp, handler, diff --git a/tests/test_serve_transport_security.py b/tests/test_serve_transport_security.py new file mode 100644 index 00000000..2d77f4a7 --- /dev/null +++ b/tests/test_serve_transport_security.py @@ -0,0 +1,84 @@ +"""``create_mcp_server`` plumbs DNS-rebinding-protection knobs through +to FastMCP's :class:`TransportSecuritySettings`. + +FastMCP's default ``allowed_hosts`` accepts only loopback patterns +(``127.0.0.1:*``, ``localhost:*``, ``[::1]:*``). Adopters serving +multi-tenant subdomain hosts (``acme.example.com``, +``acme.localhost``) need to either extend the list or disable the +MCP-layer check entirely (when a tenant-aware ASGI middleware +already validates the Host header). Without these kwargs the +transport returns ``421 Misdirected Request`` and MCP discovery +fails — see PR #443 / storyboard CI ``v3_reference_seller`` +job for the original symptom. + +Pin the plumbing here so a future refactor doesn't silently drop +it. +""" + +from __future__ import annotations + +from typing import Any + +from adcp.server.base import ADCPHandler +from adcp.server.serve import create_mcp_server + + +class _StubHandler(ADCPHandler[Any]): + """Empty handler — only the FastMCP settings are under test.""" + + +def test_default_transport_security_keeps_loopback_allowlist() -> None: + """No kwargs → FastMCP defaults intact (loopback-only host list).""" + mcp = create_mcp_server(_StubHandler(), name="t") + ts = mcp.settings.transport_security + assert ts is not None + assert ts.enable_dns_rebinding_protection is True + # FastMCP's loopback defaults — match exactly so a regression in + # this list (e.g. an upstream rename) breaks here, not at runtime. + assert "localhost:*" in ts.allowed_hosts + assert "127.0.0.1:*" in ts.allowed_hosts + + +def test_allowed_hosts_extends_default_list() -> None: + """``allowed_hosts=[...]`` extends rather than replaces the + FastMCP default — loopback probes still work alongside the + adopter's tenant hosts. + """ + mcp = create_mcp_server( + _StubHandler(), + name="t", + allowed_hosts=["acme.localhost:*", "beta.localhost:*"], + ) + ts = mcp.settings.transport_security + assert ts is not None + assert "localhost:*" in ts.allowed_hosts # default preserved + assert "acme.localhost:*" in ts.allowed_hosts + assert "beta.localhost:*" in ts.allowed_hosts + + +def test_allowed_origins_extends_default_list() -> None: + """Symmetric to ``allowed_hosts`` — origins extend the default.""" + mcp = create_mcp_server( + _StubHandler(), + name="t", + allowed_origins=["http://acme.localhost:*"], + ) + ts = mcp.settings.transport_security + assert ts is not None + assert "http://localhost:*" in ts.allowed_origins # default preserved + assert "http://acme.localhost:*" in ts.allowed_origins + + +def test_enable_dns_rebinding_protection_false_disables_check() -> None: + """Adopters with their own tenant-aware host validation pass + ``enable_dns_rebinding_protection=False`` so the MCP-layer check + doesn't duplicate the upstream validation. + """ + mcp = create_mcp_server( + _StubHandler(), + name="t", + enable_dns_rebinding_protection=False, + ) + ts = mcp.settings.transport_security + assert ts is not None + assert ts.enable_dns_rebinding_protection is False