diff --git a/docs/config.md b/docs/config.md index 6c7c8848..e5b20528 100644 --- a/docs/config.md +++ b/docs/config.md @@ -125,7 +125,8 @@ These variables load into `pipefy_mcp.McpSettings` (`settings.mcp`). TOML keys u |----------|---------|--------| | `PIPEFY_MCP_PROFILE` | `local` | Launch profile: `local` registers every tool; `remote` exposes only remote-safe tools and validates an inbound bearer per request. | | `PIPEFY_MCP_TRANSPORT` | derived from profile | `stdio` or `http`. Unset: `local` → `stdio`, `remote` → `http`. | -| `PIPEFY_MCP_HOST` | `127.0.0.1` | HTTP bind host (loopback only until hosted OBO lands). | +| `PIPEFY_MCP_HOST` | `127.0.0.1` | HTTP bind host. The unauthenticated `local` profile refuses a non-loopback bind unless `PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND` is set; the authenticated `remote` profile binds any host. | | `PIPEFY_MCP_PORT` | `8000` | HTTP bind port. | +| `PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND` | `false` | Escape hatch: lets the unauthenticated `local` profile serve HTTP on a non-loopback host, exposing the full tool surface with no inbound bearer. The `remote` profile never needs it. | | `PIPEFY_MCP_UNIFIED_ENVELOPE` | `true` | When true, migrated tools return `{success, data, ...}`. | | `PIPEFY_MCP_LOG_LEVEL` | `INFO` | Governs **both** hosted structured JSON lines on **stdout** and the FastMCP root logger (RichHandler text on **stderr**). Accepts `DEBUG`, `INFO`, `WARNING`, `ERROR`, or `CRITICAL` (case-insensitive). The server passes the resolved level to `FastMCP(log_level=...)` at construction, so this is the single operator knob for MCP logging. Some MCP tooling documents a separate `FASTMCP_LOG_LEVEL` env var; when using `pipefy-mcp-server`, prefer `PIPEFY_MCP_LOG_LEVEL` — it configures the root logger explicitly and keeps structured events on the same level. | diff --git a/packages/infra/src/pipefy_infra/security.py b/packages/infra/src/pipefy_infra/security.py index 8f556056..e713ff41 100644 --- a/packages/infra/src/pipefy_infra/security.py +++ b/packages/infra/src/pipefy_infra/security.py @@ -12,6 +12,9 @@ literal-IP check (callers must follow up with the DNS gate). * :func:`assert_hostname_is_not_internal`: building block of the above; exposed for callers that already have a parsed hostname. +* :func:`is_loopback_host`: predicate reporting whether a bind host keeps + a server reachable only from the local machine (``localhost`` or a + literal IP in ``127.0.0.0/8`` or ``::1``). * :func:`assert_hostname_resolves_to_public_ips`: asynchronous DNS gate. Resolves the hostname and rejects when any resolved IP is in a blocked range. Counter to DNS-rebinding attacks that point a public name at an @@ -101,6 +104,33 @@ def assert_hostname_is_not_internal(hostname: str, *, context: str) -> None: ) +def is_loopback_host(host: str) -> bool: + """Report whether ``host`` binds a server to the local machine only. + + Loopback means ``localhost`` or a literal IP in ``127.0.0.0/8`` or ``::1``. + ``0.0.0.0`` (and ``::``) is unspecified, not loopback, so it is reported as + non-loopback. A hostname that is not a literal IP (anything other than + ``localhost``) is reported as non-loopback: it is not resolved here, and + treating an unresolved name as reachable is the safe default for a + bind-safety gate. + + Bracketed IPv6 literals (``[::1]``) are tolerated for callers passing a raw + URL host slot rather than ``urlparse(...).hostname``. + """ + candidate = (host or "").strip().lower() + if candidate == "localhost": + return True + candidate = ( + candidate[1:-1] + if candidate.startswith("[") and candidate.endswith("]") + else candidate + ) + try: + return ipaddress.ip_address(candidate).is_loopback + except ValueError: + return False + + def validate_https_url( url: str, field_label: str, diff --git a/packages/infra/tests/test_security.py b/packages/infra/tests/test_security.py index 2cc6a368..60022b43 100644 --- a/packages/infra/tests/test_security.py +++ b/packages/infra/tests/test_security.py @@ -291,3 +291,21 @@ async def test_validate_and_assert_public_url_allow_insecure_skips_literal_ip(): allow_insecure=True, ) assert hostname == "127.0.0.1" + + +@pytest.mark.unit +@pytest.mark.parametrize( + "host", + ["127.0.0.1", "127.0.0.2", "localhost", "LOCALHOST", "::1", "[::1]", " ::1 "], +) +def test_is_loopback_host_accepts_loopback(host: str) -> None: + assert security.is_loopback_host(host) is True + + +@pytest.mark.unit +@pytest.mark.parametrize( + "host", + ["0.0.0.0", "::", "203.0.113.5", "example.com", "", " "], +) +def test_is_loopback_host_rejects_non_loopback(host: str) -> None: + assert security.is_loopback_host(host) is False diff --git a/packages/mcp/AGENTS.md b/packages/mcp/AGENTS.md index 5ad44b2e..d8a35555 100644 --- a/packages/mcp/AGENTS.md +++ b/packages/mcp/AGENTS.md @@ -33,7 +33,8 @@ marker is described below. resource-server URL is configured, validates an inbound bearer per request. - **`--transport {stdio|http}`** (env `PIPEFY_MCP_TRANSPORT`). Left unset it follows the profile: `local` speaks stdio, `remote` serves over Streamable HTTP. Set it - explicitly to run `local` over loopback HTTP. `remote` over stdio is rejected: a + explicitly to run `local` over HTTP (loopback by default; see "Bind-safety + interlock"). `remote` over stdio is rejected: a per-request bearer has no stdio equivalent. The pair is resolved (and validated) once, at startup, by `resolve_mcp_settings`. @@ -45,8 +46,9 @@ bearer per request and opens a per-request session carrying a snapshot of that validated bearer, so concurrent callers each act as themselves rather than as a single identity resolved at startup. All sessions share one process-scoped engine (the GraphQL endpoints and their schema cache). `local` runs as the one credential -resolved at startup. The transport still binds loopback-only until the -DNS-rebinding host/Origin allowlist lands. +resolved at startup. The unauthenticated `local` profile binds loopback-only over +HTTP unless the `PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND` escape hatch is set; the +authenticated `remote` profile binds any host (see "Bind-safety interlock" below). **Resource-server profile.** Config is split by domain. *Token validation* is an auth concern and lives in `pipefy_auth.JwtValidationSettings` (`settings.jwt`, @@ -71,14 +73,23 @@ module) resolves the inbound issuer and pairs the verifier with `AuthSettings`. The runtime (`McpRuntime.for_profile`) calls it for the `remote` profile and holds the pair as `inbound_auth`, which `server.py` wires into the app. -**Loopback bind.** `_assert_safe_http_bind` restricts the HTTP transport to a -loopback bind, unconditionally for now. Per-request on-behalf-of identity (each -call runs as the validated caller, not a single startup identity) means inbound -identity is not the constraint; the constraint is DNS-rebinding protection, the -configurable host / Origin allowlist for a proxied deployment. Off-loopback binding -stays off until that lands (see `experiments/hosted-obo/RFC-OUTLINE.md`). The -attachment tools' local `file_path` inputs also still assume a loopback peer that -shares the client's disk (remote-safe file inputs are separate follow-up work). +**Bind-safety interlock.** The property protected is auth posture, not bind +interface: an unauthenticated profile must not be reachable by untrusted callers. +`McpSettings._enforce_bind_safety` (a `model_validator` at the settings boundary) +refuses a non-loopback HTTP bind under the unauthenticated `local` profile unless +`PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND` is set. Living at the settings boundary means +no serving path routes around it (the coverage argument, why every serving path +inherits the guarantee, lives on the `_enforce_bind_safety` docstring). The `remote` profile +validates a per-request bearer, so its bind host is irrelevant and is not checked +(a container binds `0.0.0.0` and is still private). Loopback detection is +`pipefy_infra.security.is_loopback_host`, which covers all of `127.0.0.0/8` and +`::1`. This replaced an earlier bind-interface guard (`_assert_safe_http_bind`) that +false-positived on the entire hosted profile and lived in the run path where the +ASGI-app path bypassed it. DNS-rebinding protection (the configurable host / Origin +allowlist for a proxied deployment) is separate follow-up work; see +`experiments/hosted-obo/RFC-OUTLINE.md`. The attachment tools' local `file_path` +inputs also still assume a loopback peer that shares the client's disk (remote-safe +file inputs are separate follow-up work). ## Tool registration diff --git a/packages/mcp/src/pipefy_mcp/server.py b/packages/mcp/src/pipefy_mcp/server.py index 9c23d579..8f07fb50 100644 --- a/packages/mcp/src/pipefy_mcp/server.py +++ b/packages/mcp/src/pipefy_mcp/server.py @@ -24,12 +24,6 @@ You are connected to a Pipefy MCP server for managing Kanban-style workflow processes. """).strip() -# Hosts that keep the server reachable only from the local machine. The HTTP -# transport refuses to bind anywhere else (a routable interface or 0.0.0.0) -# until the DNS-rebinding host/Origin allowlist lands; see -# _assert_safe_http_bind. -_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "::1"}) - def _make_lifespan( runtime: McpRuntime, @@ -143,29 +137,6 @@ def build_pipefy_mcp_server( return app -def _assert_safe_http_bind(*, host: str) -> None: - """Refuse to bind the HTTP transport to a non-loopback host. - - The HTTP transport is restricted to loopback for now. The resource-server - profile carries per-request on-behalf-of identity (each call runs as the - validated caller, not a single startup identity), so inbound identity is not - the constraint; the constraint is DNS-rebinding protection, the configurable - host / Origin allowlist. (The filesystem tools, e.g. the attachment uploads, - also only make sense on loopback, where the server shares the client's disk; - remote-safe file inputs are separate follow-up work.) - - Off-loopback binding stays off until that allowlist lands; see - experiments/hosted-obo/RFC-OUTLINE.md. - """ - if host in _LOOPBACK_HOSTS: - return - raise RuntimeError( - f"Refusing to serve over HTTP on a non-loopback host ({host}). The HTTP " - f"transport is restricted to loopback (127.0.0.1/localhost/::1) until " - f"the hosted on-behalf-of profile lands." - ) - - def run_server(settings: Settings) -> None: """Run the Pipefy MCP server. The single serve entry point for both transports. @@ -175,8 +146,7 @@ def run_server(settings: Settings) -> None: pair) and passes the result, so this module reads no process globals. ``settings.mcp.transport == "stdio"`` speaks MCP over stdio; ``"http"`` serves - over Streamable HTTP on ``host``/``port``, restricted to a loopback bind until - the hosted on-behalf-of profile lands (see :func:`_assert_safe_http_bind`). + over Streamable HTTP on ``host``/``port``. ``settings.mcp.profile == "remote"`` selects the default-deny remote-safe tool surface and validates an inbound bearer per request; it requires a configured @@ -191,8 +161,7 @@ def run_server(settings: Settings) -> None: Both transports build the same app through :func:`build_pipefy_mcp_server`, which builds the app-scoped runtime via :meth:`McpRuntime.for_profile` (owning the inbound resource-server pair for ``remote`` and failing fast when that - profile has no resource server). They differ only in the transport ``run`` and - HTTP's bind concerns. + profile has no resource server). They differ only in the transport ``run``. """ mcp = settings.mcp configure_observability_logging(log_level=mcp.log_level) @@ -214,6 +183,7 @@ def run_server(settings: Settings) -> None: mcp.profile, "active" if mcp.profile == "remote" else "inactive", ) - _assert_safe_http_bind(host=mcp.host) + # Bind safety is enforced at the settings boundary (McpSettings._enforce_bind_safety); + # host/port arrive already vetted, so there is nothing to re-check here. build_pipefy_mcp_server(settings).run("streamable-http") diff --git a/packages/mcp/src/pipefy_mcp/settings.py b/packages/mcp/src/pipefy_mcp/settings.py index a6756bdd..8d06af48 100644 --- a/packages/mcp/src/pipefy_mcp/settings.py +++ b/packages/mcp/src/pipefy_mcp/settings.py @@ -97,8 +97,9 @@ def settings_customise_sources( description=( "Bind host for the Streamable HTTP transport (env: PIPEFY_MCP_HOST). " "Only consulted when serving over HTTP (--transport http); the stdio " - "transport ignores it. Must stay loopback (the default): the HTTP " - "transport refuses a non-loopback bind while it is unauthenticated." + "transport ignores it. Under the unauthenticated 'local' profile a " + "non-loopback bind is refused unless PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND " + "is set; the authenticated 'remote' profile binds any host." ), ) @@ -120,6 +121,18 @@ def settings_customise_sources( ), ) + allow_insecure_http_bind: bool = Field( + default=False, + description=( + "When true (env: PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND), the " + "unauthenticated 'local' profile may serve HTTP on a non-loopback " + "host. The escape hatch for exposing the full tool surface with no " + "inbound bearer to callers that are not on the local machine. The " + "authenticated 'remote' profile never needs it: it validates a " + "per-request bearer, so its bind host is unrestricted." + ), + ) + @field_validator("log_level", mode="before") @classmethod def _normalize_log_level(cls, value: object) -> object: @@ -145,6 +158,43 @@ def _resolve_transport(self) -> Self: ) return self + @model_validator(mode="after") + def _enforce_bind_safety(self) -> Self: + """Refuse to serve the unauthenticated tool surface off loopback. + + The property protected is auth posture, not bind interface: an + unauthenticated profile must not be reachable by untrusted callers. The + 'local' profile registers every tool and wires no inbound bearer, so + over HTTP on a non-loopback host it exposes the full surface to whoever + can reach the port. The 'remote' profile validates a per-request bearer, + so its bind host is irrelevant and is not checked here (a container + binds 0.0.0.0 legitimately). Runs after :meth:`_resolve_transport`, so + ``transport`` is concrete. + + Enforcing at the settings boundary means every serving path inherits the + guarantee: :func:`build_pipefy_mcp_server` and a caller building the ASGI + app directly both take a resolved ``Settings``, so none re-checks the bind + or routes around it. + + ``allow_insecure_http_bind`` is the explicit escape hatch for operators + who accept an unauthenticated public bind. + """ + if ( + self.transport == "http" + and self.profile == "local" + and not self.allow_insecure_http_bind + and not security.is_loopback_host(self.host) + ): + raise ValueError( + "the 'local' profile serves every tool with no inbound bearer, " + f"so it refuses a non-loopback HTTP bind ({self.host!r}). Bind a " + "loopback host, switch to '--profile remote' with a resource " + "server to validate per-request callers, or set " + "PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND to accept an " + "unauthenticated public bind." + ) + return self + class ResourceServerSettings(BaseSettings): """This MCP server's identity as an OAuth protected resource (HTTP profile). diff --git a/packages/mcp/tests/test_main.py b/packages/mcp/tests/test_main.py index 99fa9aa9..c2a8a380 100644 --- a/packages/mcp/tests/test_main.py +++ b/packages/mcp/tests/test_main.py @@ -165,7 +165,7 @@ def test_rejects_an_empty_host(mocker, bad_host): Without the guard, an empty value reaches ``resolve_mcp_settings`` as an explicit init-kwarg, silently displacing the ``127.0.0.1`` default and later - failing the loopback check with a misleading non-loopback error. + tripping the bind-safety interlock with a misleading non-loopback error. """ server_mock = mocker.patch("pipefy_mcp.main.run_server") argv = [bad_host] if bad_host.startswith("--host=") else ["--host", bad_host] diff --git a/packages/mcp/tests/test_server.py b/packages/mcp/tests/test_server.py index 95196076..bd0673cd 100644 --- a/packages/mcp/tests/test_server.py +++ b/packages/mcp/tests/test_server.py @@ -15,7 +15,6 @@ from pipefy_mcp.core.tool_middleware import ToolCallContext, short_circuit_error from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.server import ( - _assert_safe_http_bind, _make_lifespan, _register_pipefy_tools, build_pipefy_mcp_server, @@ -272,20 +271,6 @@ async def foreign_mcp_tool() -> str: # --- HTTP (Streamable) transport profile ------------------------------------ -@pytest.mark.unit -@pytest.mark.parametrize("host", ["127.0.0.1", "localhost", "::1"]) -def test_loopback_http_bind_allows_loopback_hosts(host): - _assert_safe_http_bind(host=host) - - -@pytest.mark.unit -@pytest.mark.parametrize("host", ["0.0.0.0", "203.0.113.5"]) -def test_loopback_http_bind_refuses_non_loopback_hosts(host): - """A non-loopback bind is refused until the hosted on-behalf-of profile lands.""" - with pytest.raises(RuntimeError, match="non-loopback host"): - _assert_safe_http_bind(host=host) - - @pytest.mark.unit def test_run_server_stdio_logs_the_argv_resolved_profile(monkeypatch): """The startup log reflects the argv-resolved profile, not the ambient env. @@ -382,9 +367,7 @@ def test_run_server_remote_without_resource_server_fails_fast(monkeypatch): The remote profile acts on behalf of the caller, so it needs a per-request bearer to validate. Without a configured resource server there is no per-request identity, and silently falling back to the single startup - credential would defeat the profile, so the runtime refuses to build. The - loopback bind guard runs first, so the host here is loopback to reach the - resource-server check. + credential would defeat the profile, so the runtime refuses to build. """ monkeypatch.delenv("PIPEFY_MCP_RS_RESOURCE_SERVER_URL", raising=False) with pytest.raises(RuntimeError, match="requires a resource server"): @@ -435,17 +418,26 @@ def test_run_server_http_respects_an_explicit_zero_port(remote_rs_env): @pytest.mark.unit -def test_run_server_http_refuses_non_loopback_before_building(remote_rs_env): - """The loopback guard fires before the app is built or served.""" - with patch("pipefy_mcp.server.build_pipefy_mcp_server") as mock_build: - with pytest.raises(RuntimeError, match="Refusing to serve"): - run_server( - resolve_mcp_settings( - profile="remote", transport="http", host="0.0.0.0", port=9123 - ) +def test_run_server_remote_serves_off_loopback_without_a_bind_guard(remote_rs_env): + """The resource-server profile binds a non-loopback host with no guard or bypass. + + Auth posture, not bind interface, is the axis: ``remote`` validates a + per-request bearer, so ``0.0.0.0`` is a legitimate hosted bind and nothing + refuses it (a container binds ``0.0.0.0`` and is still private). + """ + fake_app = MagicMock() + with patch( + "pipefy_mcp.server.build_pipefy_mcp_server", return_value=fake_app + ) as mock_build: + run_server( + resolve_mcp_settings( + profile="remote", transport="http", host="0.0.0.0", port=9123 ) + ) - mock_build.assert_not_called() + (built_settings,), _ = mock_build.call_args + assert built_settings.mcp.host == "0.0.0.0" + fake_app.run.assert_called_once_with("streamable-http") # --- Resource-server role (OAuth 2.0 inbound bearer validation) -------------- diff --git a/packages/mcp/tests/test_settings.py b/packages/mcp/tests/test_settings.py index 126ac4c0..ec3d410d 100644 --- a/packages/mcp/tests/test_settings.py +++ b/packages/mcp/tests/test_settings.py @@ -214,3 +214,60 @@ def test_mcp_remote_over_stdio_is_rejected(): """'remote' has no stdio equivalent (no per-request bearer), so it is refused.""" with pytest.raises(ValidationError, match="requires the 'http' transport"): McpSettings(profile="remote", transport="stdio") + + +# --- bind-safety interlock (auth posture, not bind interface) ---------------- + + +@pytest.mark.unit +@pytest.mark.parametrize("host", ["0.0.0.0", "203.0.113.5"]) +def test_mcp_local_http_refuses_non_loopback_bind(host): + """The unauthenticated 'local' profile refuses a non-loopback HTTP bind.""" + with pytest.raises(ValidationError, match="non-loopback HTTP bind"): + McpSettings(profile="local", transport="http", host=host) + + +@pytest.mark.unit +def test_mcp_local_http_non_loopback_allowed_with_escape_hatch(): + """The escape hatch opts in to an unauthenticated non-loopback bind.""" + settings = McpSettings( + profile="local", + transport="http", + host="0.0.0.0", + allow_insecure_http_bind=True, + ) + assert settings.host == "0.0.0.0" + + +@pytest.mark.unit +def test_mcp_local_http_escape_hatch_from_env(monkeypatch): + """The escape hatch is settable via PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND.""" + monkeypatch.setenv("PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND", "true") + settings = McpSettings(profile="local", transport="http", host="0.0.0.0") + assert settings.allow_insecure_http_bind is True + + +@pytest.mark.unit +@pytest.mark.parametrize("host", ["127.0.0.1", "localhost", "::1", "127.0.0.2"]) +def test_mcp_local_http_allows_loopback_bind(host): + """Loopback covers all of 127.0.0.0/8 and ::1, not just literal 127.0.0.1.""" + assert McpSettings(profile="local", transport="http", host=host).host == host + + +@pytest.mark.unit +def test_mcp_local_stdio_ignores_a_non_loopback_host(): + """stdio never binds, so a non-loopback host is not checked.""" + settings = McpSettings(profile="local", transport="stdio", host="0.0.0.0") + assert settings.host == "0.0.0.0" + + +@pytest.mark.unit +def test_mcp_remote_binds_any_host_without_the_escape_hatch(): + """The authenticated 'remote' profile binds a non-loopback host unrestricted. + + Its per-request bearer is the control, so bind interface is irrelevant. The + resource-server requirement is a separate runtime check, not this validator. + """ + settings = McpSettings(profile="remote", transport="http", host="0.0.0.0") + assert settings.host == "0.0.0.0" + assert settings.allow_insecure_http_bind is False