Skip to content

feat(mcp): replace bind-host guard with auth-posture interlock#396

Merged
gbrlcustodio merged 5 commits into
devfrom
feat/379-auth-posture-bind-interlock
Jul 13, 2026
Merged

feat(mcp): replace bind-host guard with auth-posture interlock#396
gbrlcustodio merged 5 commits into
devfrom
feat/379-auth-posture-bind-interlock

Conversation

@gbrlcustodio

@gbrlcustodio gbrlcustodio commented Jul 12, 2026

Copy link
Copy Markdown
Member

Closes #379.

What

Replaces the bind-interface guard _assert_safe_http_bind with an auth-posture interlock, realizing the safety interlock scoped in #300 and retiring the legacy guard it was meant to supersede.

Why the old guard was wrong

The old guard refused any non-loopback HTTP bind. It checked the wrong axis. The property worth protecting is "an unauthenticated profile must not be served to untrusted callers," but bind interface is a poor proxy:

  • In a container everything binds 0.0.0.0 and is still private (network namespace, no published port), so the guard false-positived on the entire hosted profile.
  • Behind a proxy the server must bind non-loopback, and the remote profile is safe there because it has inbound auth (resource-server validation, per-request bearer). Every firing against remote was a false positive.

It also lived in run_server, not the app factory, so it protected nothing for callers that build the ASGI app directly via streamable_http_app(). That bypass was the intended off-loopback path today, which trains callers to route around the control and has already led a downstream repo to vendor a guard-less copy.

The interlock

Keyed on auth posture, not bind host, and expressed as a cross-field validator at the settings boundary (McpSettings._enforce_bind_safety), so no serving path can route around it: build_pipefy_mcp_server and any direct-ASGI-app caller both take a resolved Settings.

  • No-inbound-auth posture (profile == "local"): refuses a non-loopback HTTP bind unless the escape hatch PIPEFY_MCP_ALLOW_INSECURE_HTTP_BIND is set. This is the real footgun (the full tool surface, no inbound bearer, reachable off the local machine).
  • Resource-server posture (profile == "remote"): bind host is irrelevant and is not checked. The operator binds 0.0.0.0 legitimately. remote always carries inbound auth or fails fast in McpRuntime.for_profile, so keying on profile == "local" is a faithful proxy for posture at the settings layer.

Loopback detection moves to a new pipefy_infra.security.is_loopback_host, a generic security primitive that uses ipaddress(...).is_loopback to cover all of 127.0.0.0/8 and ::1, not the three string literals the old _LOOPBACK_HOSTS frozenset hardcoded.

Acceptance criteria

  • _assert_safe_http_bind (the bind-interface check) is removed.
  • The remote / resource-server profile serves on a non-loopback host with no bind-host guard and no bypass.
  • The no-auth profile refuses non-loopback HTTP unless the escape hatch is set, exercised by a test.
  • No supported serving path requires calling internal factory methods to route around a guard.

Testing

  • uv run python -m pytest packages/infra/tests/test_security.py packages/mcp/tests/test_settings.py packages/mcp/tests/test_server.py packages/mcp/tests/test_main.py: 145 passed.
  • Full non-integration packages/infra/tests packages/mcp/tests: 1607 passed, 5 skipped.
  • cd packages/mcp && uv run lint-imports: contract kept.
  • uv run ruff check packages/infra packages/mcp: clean.

Not in scope

  • The configurable transport host / Origin allowlist for DNS-rebinding protection (Configurable transport host allowlist #303) stays open and is unaffected.
  • Follow-up worth doing: the "auth posture" axis is re-derived from profile == "remote"/"local" at several sites (settings, server, runtime); a single named posture property on McpSettings would consolidate it. Deferred to keep this change focused.

The transport-flag summary said local runs "over loopback HTTP", baking loopback into local-over-HTTP as if inherent. The bind-safety interlock's escape hatch can lift it, and that section already owns the nuance, so point there instead.
The bind-safety interlock rationale was stated in three places: the
run_server docstring, an inline comment in the HTTP serving branch, and
the McpSettings._enforce_bind_safety docstring/AGENTS.md.

Keep it once, at the boundary that owns the invariant. run_server no
longer narrates a settings-boundary invariant; it carries only a short
regression guard at the .run site (do not re-check host/port here). The
coverage claim (every serving path inherits the guarantee) now lives on
_enforce_bind_safety, and AGENTS.md points there instead of restating it.
@gbrlcustodio
gbrlcustodio requested a review from adriannoes July 13, 2026 16:15
@gbrlcustodio
gbrlcustodio marked this pull request as ready for review July 13, 2026 16:15
@gbrlcustodio
gbrlcustodio requested a review from mocha06 July 13, 2026 16:23
@gbrlcustodio
gbrlcustodio force-pushed the feat/379-auth-posture-bind-interlock branch from 9c81845 to 7bf41c9 Compare July 13, 2026 16:27
@gbrlcustodio gbrlcustodio added refactor hosted-mcp Hosted, multi-user, on-behalf-of MCP server profile labels Jul 13, 2026
@gbrlcustodio gbrlcustodio added this to the Hosted server foundation milestone Jul 13, 2026
@gbrlcustodio
gbrlcustodio force-pushed the feat/379-auth-posture-bind-interlock branch from 7bf41c9 to 9c81845 Compare July 13, 2026 16:32

@adriannoes adriannoes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Looks good to merge.

  • Local checkout+tests on 9c818451 (ruff, targeted 145, full infra+mcp 1607, lint-imports kept) and CI lint/test are green.
  • The auth-posture interlock at McpSettings._enforce_bind_safety matches #379: local+http refuses non-loopback without the hatch, remote can bind off-loopback, and the old run-path _assert_safe_http_bind is gone.

What worked well

  • Moving the control to the settings boundary closes the ASGI/streamable_http_app bypass of the old run_server-only guard.
  • Keying on auth posture (local vs remote) fixes the false positives on container 0.0.0.0 and authenticated hosted binds.
  • is_loopback_host via ipaddress.is_loopback, with clear escape-hatch docs and tests.

Also noted

  • Merge-order heads-up with the open #373 stack (PR #376 / feat/373-request-log-middleware): server.py conflicts where this PR removes _assert_safe_http_bind and that branch inserts _serve_streamable_http, and both rewrite the tail of run_server. The new test test_run_server_remote_serves_off_loopback_without_a_bind_guard asserts fake_app.run("streamable-http"), which that serve path replaces with anyio.run(_serve_streamable_http, ...), so whoever lands second needs to retarget that assertion. The settings-boundary interlock still covers that custom uvicorn path, which is exactly the coverage argument here.

@gbrlcustodio
gbrlcustodio merged commit 6d25fe1 into dev Jul 13, 2026
4 checks passed
@gbrlcustodio
gbrlcustodio deleted the feat/379-auth-posture-bind-interlock branch July 13, 2026 19:40
adriannoes added a commit that referenced this pull request Jul 14, 2026
Keep stack-2 HTTP serve via _serve_streamable_http (uvicorn + request
log middleware); drop _assert_safe_http_bind in favor of
McpSettings._enforce_bind_safety from #396.
adriannoes added a commit that referenced this pull request Jul 14, 2026
Take #396 bind-safety settings/docs and keep stack-2 HTTP serve via
_serve_streamable_http; drop the duplicated serve helper from the merge.
gbrlcustodio added a commit that referenced this pull request Jul 14, 2026
Add Unreleased entries for the configurable DNS-rebinding Host/Origin
allowlist (PIPEFY_MCP_ALLOWED_HOSTS / PIPEFY_MCP_ALLOWED_ORIGINS and the
protection-on-when-resource-server-URL-set flip, #303) and the
auth-posture bind-safety interlock that #396 left undocumented (#379).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hosted-mcp Hosted, multi-user, on-behalf-of MCP server profile refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants