diff --git a/packages/pickled-core/src/pickled_core/mcp/transport.py b/packages/pickled-core/src/pickled_core/mcp/transport.py index e7d94f4..afc64c9 100644 --- a/packages/pickled-core/src/pickled_core/mcp/transport.py +++ b/packages/pickled-core/src/pickled_core/mcp/transport.py @@ -3,22 +3,27 @@ from __future__ import annotations import ipaddress +import socket from typing import Any, Literal def _is_unspecified_address(host: str) -> bool: """Return True iff ``host`` is an IPv4/IPv6 wildcard ("unspecified") address. - The previous implementation only rejected the literal string ``0.0.0.0``, - which let callers bind to ``::`` / ``[::]`` (the IPv6 wildcard) and bypass - the "no public binding without --allow-public" guard. That is a real - accidental-exposure footgun on any host with a routable IPv6 address: the - OS happily listens on every IPv6 interface, including public ones, and - typically accepts IPv4 traffic too via dual-stack mapping. + The original guard rejected only the literal string ``0.0.0.0``. PR #27 + extended it to cover the IPv6 wildcard (``::`` / ``[::]`` and friends). + Both are still bypassed by ``inet_aton``-style legacy IPv4 short forms: + ``socket.bind(("0", port))``, ``("0.0", port)``, ``("0.0.0", port)``, + ``("0x0", port)`` and ``("00000000", port)`` all silently resolve to + ``0.0.0.0`` on POSIX systems (and uvicorn / asyncio happily forward them + to ``socket.bind``). On a host with a public network interface, any of + these would expose the MCP server without the user passing + ``--allow-public``. We accept the bracketed form ``[::]`` (uvicorn / FastMCP take it as well), - fully-expanded forms such as ``0:0:0:0:0:0:0:0``, and the IPv4-mapped - wildcard ``::ffff:0.0.0.0``. Whitespace is stripped so the heuristic is + fully-expanded forms such as ``0:0:0:0:0:0:0:0``, the IPv4-mapped + wildcard ``::ffff:0.0.0.0``, and any ``inet_aton``-compatible string that + canonicalises to ``0.0.0.0``. Whitespace is stripped so the heuristic is not defeated by trivial copy-paste artifacts. Hostnames are intentionally left untouched — we deliberately do not resolve DNS in this guard. """ @@ -28,12 +33,20 @@ def _is_unspecified_address(host: str) -> bool: try: addr = ipaddress.ip_address(stripped) except ValueError: + addr = None + if addr is not None: + if addr.is_unspecified: + return True + if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None: + return addr.ipv4_mapped.is_unspecified return False - if addr.is_unspecified: - return True - if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None: - return addr.ipv4_mapped.is_unspecified - return False + if not stripped or any(ch.isspace() for ch in stripped): + return False + try: + packed = socket.inet_aton(stripped) + except OSError: + return False + return packed == b"\x00\x00\x00\x00" def resolve_transport( diff --git a/packages/pickled-core/tests/test_mcp_transport.py b/packages/pickled-core/tests/test_mcp_transport.py index 0420fb1..3b328d0 100644 --- a/packages/pickled-core/tests/test_mcp_transport.py +++ b/packages/pickled-core/tests/test_mcp_transport.py @@ -66,3 +66,33 @@ def test_allow_loopback_and_specific_hosts_without_flag(host: str) -> None: """Loopback addresses and specific hostnames must not be misidentified as wildcards.""" kw = resolve_transport("http", host, None, False) assert kw["host"] == host + + +@pytest.mark.parametrize( + "host", + ["0", "0.0", "0.0.0", "0x0", "00000000", "0.00.0.0", " 0 ", "0x00000000"], +) +def test_refuse_inet_aton_short_forms_without_flag(host: str) -> None: + """``inet_aton``-style short forms of 0.0.0.0 must also require --allow-public. + + POSIX ``socket.bind(("0", port))``, ``("0.0", port)`` etc. silently + canonicalise to ``0.0.0.0`` — the very bypass we are guarding against. + """ + with pytest.raises(RuntimeError, match="wildcard"): + resolve_transport("http", host, None, False) + + +@pytest.mark.parametrize( + "host", + ["1", "127", "127.1", "10.0.0.1", "0.0.0.1", "0.1.0.0"], +) +def test_allow_inet_aton_specific_addresses_without_flag(host: str) -> None: + """``inet_aton``-style short forms that resolve to a *specific* address are fine.""" + kw = resolve_transport("http", host, None, False) + assert kw["host"] == host + + +def test_hostnames_with_zero_letters_are_not_wildcards() -> None: + """A hostname like 'zero.example.com' must not be misclassified as a wildcard.""" + kw = resolve_transport("http", "zero.example.com", None, False) + assert kw["host"] == "zero.example.com"