feat(mcp): configurable transport host/Origin allowlist#397
Merged
Conversation
gbrlcustodio
added
refactor
hosted-mcp
gbrlcustodio
force-pushed
the
feat/379-auth-posture-bind-interlock
branch
from
July 13, 2026 16:32
7bf41c9 to
9c81845
Compare
gbrlcustodio
force-pushed
the
feat/303-transport-host-allowlist
branch
from
July 13, 2026 16:33
5ca6f7c to
42fbe88
Compare
gbrlcustodio
changed the base branch from
feat/379-auth-posture-bind-interlock
to
dev
July 13, 2026 19:39
FastMCP auto-enables a loopback-only DNS-rebinding allowlist when built with a loopback host and no explicit transport_security. build_pipefy_mcp_server always constructs FastMCP with settings.mcp.host (default 127.0.0.1), so behind a proxy that forwards the public Host the transport answers 421 Misdirected Request before any handler runs. Add allowed_hosts / allowed_origins to McpSettings (env PIPEFY_MCP_ALLOWED_* as JSON) and a build_transport_security resolver that widens the allowlist: loopback plus the resource_server_url host (the public origin the remote profile already declares) plus any explicit entries. The builder passes the result into FastMCP. When nothing is configured it returns None, leaving FastMCP's loopback-only default in force, so the local subprocess case is unchanged. The resolver is configuration resolution, not an MCP extension: it produces a static SDK config value, mirroring build_resource_server_auth (settings to the SDK's AuthSettings). It lives in the composition tier (core/), keeping the mcp SDK out of the settings boundary.
Add PIPEFY_MCP_ALLOWED_HOSTS / PIPEFY_MCP_ALLOWED_ORIGINS to the config reference (env table and TOML sample) and turn the AGENTS.md "separate follow-up work" note into a "Transport allowlist" section describing the derive-from-resource_server_url default and the explicit override.
gbrlcustodio
force-pushed
the
feat/303-transport-host-allowlist
branch
from
July 13, 2026 19:42
42fbe88 to
f87f3d7
Compare
gbrlcustodio
marked this pull request as ready for review
July 13, 2026 19:43
Replace the ad-hoc strip-and-truthiness filter in _stripped_nonempty with pipefy_infra.coerce.optional_str, the shared empty-to-None coercion, so the allowlist normalizer drops blank entries through the same helper the rest of the config boundary uses.
The ResourceServer value object (verbatim resource URL plus its parsed Host wire forms) modeled an OAuth RFC 9728 concept that auth/resource_server.py already owns, but lived in core/transport_security.py, which merely consumed it. Move the type and its from_url parser next to build_resource_server_auth so the resource-server vocabulary sits in one module; core/transport_security.py now imports it. The new core > auth import keeps the inward-only layering contract.
auth published ResourceServer but never used it: the builder read the raw resource_server_url string while only transport security exercised the type. Parse the RFC 9728 resource identity into a ResourceServer at the auth boundary and derive the token's stamped resource and the advertised AuthSettings metadata from that one carrier, so the concern that owns and publishes the type also exercises it.
The inbound-auth builder and the transport-allowlist builder each parsed resource_server_url into their own ResourceServer, so the one URL was parsed twice by two consumers that could in principle disagree on the host. Make McpRuntime.for_profile the single wiring point: it parses the URL into one ResourceServer and feeds that instance to both build_resource_server_auth and build_transport_security, holding the results (inbound_auth, transport_security) for the server to read. The builders stay standalone functions and now receive the parsed resource rather than a raw setting; the remote-without-resource fail-fast moves to the runtime's resource gate. server.py drops its direct build_transport_security call and reads runtime.transport_security.
adriannoes
previously approved these changes
Jul 13, 2026
adriannoes
left a comment
adriannoes
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Reviewed with checkout + tests (1622 passed, CI green on f87f3d7d). The allowlist design matches #303: derive from resource_server_url, widen via config, keep protection on, and leave FastMCP's loopback default when nothing is set. Ready to merge from my side.
What worked well
Pure build_transport_security in the composition tier (mcp SDK kept out of settings), dual host / host:* forms that match the SDK matcher, and both AC directions covered (default still 421s a public Host; configured allowlist clears the gate). The migration note correctly refuses the empty-disable escape.
Also noted
- CHANGELOG: worth an Unreleased bullet for
PIPEFY_MCP_ALLOWED_HOSTS/PIPEFY_MCP_ALLOWED_ORIGINSand the intentional protection-on-when-RS-URL flip (this window also covers the deferred note from #396). - Rollout: on a non-loopback bind, FastMCP previously left Host/Origin validation off; once
resource_server_urlis set this PR turns the gate on. Unauthenticated traffic still 401s before the Host check. Sweep authenticated callers that reach the server by a non-public Host (service DNS);PIPEFY_MCP_ALLOWED_HOSTScovers them if any exist.
In build_resource_server_auth the defaulted required_scopes sat before the required default_issuer_url, reading as if default_issuer_url were optional. Both are keyword-only and every caller passes by keyword, so reordering is behavior-neutral.
build_transport_security gated origins on `if explicit_origins:`, so an explicit allowed_origins=[] was falsy and silently replaced by the derived origins, despite the field advertising it as a strict override. Gate on `is not None` (and the early-return guard likewise) so [] is kept verbatim: it rejects any request that sends an Origin, while a request with no Origin still passes. Hosts stay additive (empty behaves like unset, since entries are extra on top of the derived set); the config.md wording is corrected to match.
for_profile parsed the resource and built the allowlist, then inlined both profile branches. Keep the single parse in for_profile and dispatch to _for_remote_profile and _for_local_profile, each carrying its own profile-specific rationale. Both receive the one parsed resource/allowlist, so the single-parse invariant holds.
build_resource_server_auth resolved the issuer itself (jwt_validation.resolve_issuer_url + a default_issuer_url fallback) and raised when it was unresolvable, mixing a cross-settings resolution and its fail-fast into a builder that should just wire. Move both to McpRuntime._for_remote_profile, where they sit beside the existing resource fail-fast, and pass a resolved issuer_url into the builder. The resolution rule stays owned by JwtValidationSettings.resolve_issuer_url (tested in pipefy_auth), so the builder's two resolution tests were redundant; the unresolvable-issuer fail-fast is now a runtime test.
The field holds the host[:port] values the resource presents in the Host header, which is the URL authority (RFC 3986 / RFC 9110). 'forms' was vague filler and read as a near-synonym of transport_security._host_forms (the allowlist match forms); 'host_authorities' names what it holds, matches the docstring's own 'Host authorities' wording, and clears the collision.
StartupIdentity, RequestScopedIdentity, and the AuthSource union resolve the outbound httpx.Auth a session acts as: StartupIdentity via pipefy_auth credential resolution, RequestScopedIdentity via auth's own require_request_bearer. They lived in core/runtime.py only because the runtime consumes them. Move them to auth/session_identity.py (the outbound counterpart to request_identity.py's inbound bearer) and publish them from auth's contract, so runtime.py is just the composition root: own the engine, wire resources per profile, open sessions. core imports the AuthSource contract inward from auth; the layering contract holds.
runtime.py reached into pipefy_mcp.auth.resource_server and pipefy_mcp.auth.session_identity submodules while the tests consumed the same symbols through the pipefy_mcp.auth facade. Import through the facade so the composition root integrates with the auth concern via its published contract, matching the tests and collapsing two import blocks into one.
The list counterpart to optional_str: strip each entry and drop blanks. Gives the strip-and-drop-empty pattern used across the CLI and MCP settings one shared home beside its scalar sibling.
An all-blank allowed_hosts/allowed_origins (e.g. an unfilled template value) now normalizes to None rather than []. An empty allowed_origins is the strict reject-all-Origin posture, so a blank value must not silently select it; an explicit [] is still preserved. Folds the two normalizers into one field validator.
Dedupe host forms with dict.fromkeys and drop the _host_forms helper; derive origins directly, since the deduped hosts make a duplicate impossible and the membership guard was unreachable.
The per-profile builders return the identity and inbound-auth pair; for_profile makes the single constructor call, so the transport allowlist is no longer threaded through each builder only to forward it unchanged.
The existing remote tests pin an explicit PIPEFY_JWT_ISSUER_URL, masking the override-absent path where the inbound issuer defaults to the login issuer this process authenticates against.
build_resource_server_auth stopped resolving the inbound issuer earlier in this stack, but the module docstring, package AGENTS.md, and the builder-test docstring still credited it with that job.
…g it A blank or whitespace-only entry in allowed_hosts/allowed_origins now fails at the settings boundary rather than being trimmed away. A silent drop hides the operator's typo, and for allowed_origins it could collapse the list to the strict reject-all-Origin posture. Real entries are still trimmed and an explicitly empty list is still accepted.
The transport allowlist normalizer now rejects blank entries rather than dropping them, so nonempty_strs has no remaining consumer. The CLI split sites that duplicate the pattern can reintroduce it when they actually adopt it.
_resource_server was a one-call helper whose docstring restated for_profile's own parsed-once rationale; fold it into for_profile and drop the now-unused ResourceServerSettings import.
adriannoes
previously approved these changes
Jul 14, 2026
Collaborator
There was a problem hiding this comment.
Summary
- Double-checked the commits after the previous APPROVE (
f87f3d7d→0c5d09b3). - Checkout + tests: 1634 passed, CI green. The #303 allowlist behavior still holds, and the empty-origins fix from the prior thread is in place (
allowed_origins=[]honored viais not None, docs and unit tests updated). Ready to merge from my side.
What worked well
Single ResourceServer parse in McpRuntime.for_profile feeding both the allowlist and inbound auth, blank allowlist entries failing closed at settings, IPv6 authorities bracketed for well-formed URLs, and the composition refactors keeping the mcp SDK out of the settings boundary.
Also noted
- CHANGELOG: still worth an Unreleased bullet for
PIPEFY_MCP_ALLOWED_HOSTS/PIPEFY_MCP_ALLOWED_ORIGINSand the intentional protection-on-when-RS-URL flip (covers the deferred note from #396 as well). - Rollout: on a non-loopback bind, FastMCP previously left Host/Origin validation off; once
resource_server_urlis set this PR turns the gate on. Sweep authenticated callers that reach the server by a non-public Host (service DNS);PIPEFY_MCP_ALLOWED_HOSTScovers them if any exist. - Unbracketed IPv6:
https://2001:db8::1/mcpcan still passResourceServerSettings(urlparse sees host2001) and then raise insideResourceServer.from_urlwhen reading.port. Bracketed forms work. Not a #303 AC blocker for DNS hostnames; worth a clear config error or settings-boundary reject if you touch that path again.
mocha06
previously approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #303.
What
Makes the HTTP transport's DNS-rebinding host/Origin allowlist configurable. The public host is derived from
resource_server_url, so a proxied deployment serves without a421while DNS-rebinding protection stays on, and the loopback-only default holds when nothing is configured.Why
FastMCP auto-enables a loopback-only allowlist when built with a loopback
hostand no explicittransport_security.build_pipefy_mcp_serveralways constructsFastMCP(host=settings.mcp.host, ...)with the default127.0.0.1, so behind a proxy that forwards the publicHostthe transport answers421 Misdirected Requestbefore any handler runs. The bind interface is irrelevant; the construction-timehostarg is what trips it.The change
McpSettings.allowed_hosts/allowed_origins(envPIPEFY_MCP_ALLOWED_HOSTS/PIPEFY_MCP_ALLOWED_ORIGINSas JSON), with a light normalization validator (strip, drop empties;localhostis a wanted entry, so no SSRF gate).ResourceServer(moved into theauthmodule) parsesresource_server_urlonce into the raw URL plus its wire-form host authorities. The runtime parses it a single time and feeds that one value to both consumers, the allowlist builder and the inbound resource-server auth, so they cannot disagree on the host.core/transport_security.py:build_transport_security(mcp, resource): pure resolver. Derives loopback plus the resource host (bare andhost:*) plus explicit entries; returnsNoneonly when there is no resource andallowed_originsis unset, so FastMCP's own default stands. Origins are derivedhttp/httpsper host unlessallowed_originsoverrides; an explicit emptyallowed_originsis honored verbatim as the strictest posture (reject any request that carries an Origin).build_pipefy_mcp_serverpasses the resolved value intoFastMCP(transport_security=...).Placement: this is configuration resolution, not an MCP extension, so it mirrors
build_resource_server_auth(settings to the SDK'sAuthSettings) and lives in the composition tier (core/, targetwiring/), keeping the mcp SDK out ofsettings.py. No enable/disable toggle: widen via the allowlist. NoX-Forwarded-Hosthandling (the nginx ingress preservesHost).Acceptance criteria
Hostthe server does not return421: exercised bytest_http_configured_allowlist_accepts_a_public_host.build_transport_securityreturnsNone, andtest_http_loopback_default_rejects_a_public_hostshows the default still421s a publicHost.Testing
packages/infra/tests packages/mcp/tests): 1630 passed, 9 skipped.cd packages/mcp && uv run lint-imports: contract kept.uv run ruff check packages/mcp && uv run ruff format --check packages/mcp: clean.Migration note
Once
resource_server_urlis set, DNS-rebinding protection is on. This deliberately does not reproduce the wrapper's current "emptyMCP_ALLOWED_HOSTSdisables the check" escape; widening is done by allowlisting, not by turning protection off.