Skip to content

Commit fea8a7e

Browse files
committed
fix: prefix env vars and reject origins without hosts
Two defects found by testing the merge risk of this branch against a live server rather than only through mocks. --allowed-origins without --allowed-hosts enabled DNS-rebinding protection with an empty host allowlist, which rejects *every* request with 421 Misdirected Request. Measured on a running server: no allowlist -> 200, origins-only -> 421, hosts set -> 200. Origins-only is therefore never a usable configuration, and the README presented both flags side by side, so reaching it took only using one of them. The combination now fails at startup instead of serving an endpoint that answers nothing. The environment variables were unprefixed. A bare MCP_TRANSPORT belongs to no particular server, so a value left over from an unrelated one turned a stdio launch -- how every desktop MCP client starts this server -- into an HTTP listener that never answers the client's handshake; confirmed by launching with MCP_TRANSPORT=http and watching uvicorn come up on a stdio invocation. They are now COMMIT_CHECK_MCP_*, and a test pins that the unprefixed names are ignored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
1 parent 17d6932 commit fea8a7e

3 files changed

Lines changed: 73 additions & 22 deletions

File tree

‎README.md‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,10 @@ commit-check-mcp --transport http --host 0.0.0.0 --port 8080
248248
```
249249

250250
The same settings are available as environment variables for container
251-
images: `MCP_TRANSPORT=http`, `MCP_HOST`, and `MCP_PORT`.
251+
images: `COMMIT_CHECK_MCP_TRANSPORT=http`, `COMMIT_CHECK_MCP_HOST`, and
252+
`COMMIT_CHECK_MCP_PORT`. They are deliberately prefixed — an unprefixed
253+
`MCP_TRANSPORT` belongs to no particular server, and a stray value would
254+
turn a stdio launch into an HTTP listener that never answers its client.
252255

253256
> [!IMPORTANT]
254257
> The server has no built-in authentication or TLS. The `0.0.0.0` bind is
@@ -263,7 +266,10 @@ images: `MCP_TRANSPORT=http`, `MCP_HOST`, and `MCP_PORT`.
263266
> --allowed-origins https://app.example.com
264267
> ```
265268
>
266-
> (also available as `MCP_ALLOWED_HOSTS` / `MCP_ALLOWED_ORIGINS`)
269+
> (also available as `COMMIT_CHECK_MCP_ALLOWED_HOSTS` /
270+
> `COMMIT_CHECK_MCP_ALLOWED_ORIGINS`). `--allowed-origins` requires
271+
> `--allowed-hosts`: an empty host allowlist rejects every request with
272+
> 421, so the server refuses to start on that combination.
267273
268274
Each request is self-contained — clients can `POST` a `tools/call` directly
269275
to `/mcp` without an `initialize` handshake or `Mcp-Session-Id` header:

‎src/commit_check_mcp/server.py‎

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -620,30 +620,35 @@ def main(argv: list[str] | None = None) -> None:
620620
"""
621621
import argparse
622622

623+
# Environment variables carry the COMMIT_CHECK_MCP_ prefix on purpose. A
624+
# bare MCP_TRANSPORT belongs to no particular server, so an unrelated
625+
# value left in the environment would turn a stdio launch — how every
626+
# desktop MCP client starts this server — into an HTTP listener that
627+
# never answers the client's handshake.
623628
parser = argparse.ArgumentParser(prog="commit-check-mcp")
624629
parser.add_argument(
625630
"--transport",
626631
choices=["stdio", "http"],
627-
default=os.environ.get("MCP_TRANSPORT", "stdio"),
632+
default=os.environ.get("COMMIT_CHECK_MCP_TRANSPORT", "stdio"),
628633
help="stdio for local clients (default); http for a stateless remote server",
629634
)
630635
parser.add_argument(
631636
"--host",
632-
default=os.environ.get("MCP_HOST", "127.0.0.1"),
637+
default=os.environ.get("COMMIT_CHECK_MCP_HOST", "127.0.0.1"),
633638
help="bind address for --transport http (default 127.0.0.1; use 0.0.0.0 in containers)",
634639
)
635640
# A string default is converted through type=int only when --port is
636-
# absent, so an invalid inherited MCP_PORT still fails loudly on its own
637-
# but cannot veto an explicit, valid --port.
641+
# absent, so an invalid inherited port still fails loudly on its own but
642+
# cannot veto an explicit, valid --port.
638643
parser.add_argument(
639644
"--port",
640645
type=int,
641-
default=os.environ.get("MCP_PORT", "8000"),
646+
default=os.environ.get("COMMIT_CHECK_MCP_PORT", "8000"),
642647
help="port for --transport http (default 8000)",
643648
)
644649
parser.add_argument(
645650
"--allowed-hosts",
646-
default=os.environ.get("MCP_ALLOWED_HOSTS", ""),
651+
default=os.environ.get("COMMIT_CHECK_MCP_ALLOWED_HOSTS", ""),
647652
help=(
648653
"comma-separated Host header allowlist for --transport http"
649654
" (e.g. mcp.example.com,mcp.example.com:443); enables strict"
@@ -652,14 +657,17 @@ def main(argv: list[str] | None = None) -> None:
652657
)
653658
parser.add_argument(
654659
"--allowed-origins",
655-
default=os.environ.get("MCP_ALLOWED_ORIGINS", ""),
656-
help="comma-separated Origin header allowlist for --transport http",
660+
default=os.environ.get("COMMIT_CHECK_MCP_ALLOWED_ORIGINS", ""),
661+
help=(
662+
"comma-separated Origin header allowlist for --transport http;"
663+
" requires --allowed-hosts"
664+
),
657665
)
658666
args = parser.parse_args(argv)
659667

660668
# argparse does not check `choices` against env-supplied defaults, and a
661-
# typo in MCP_TRANSPORT must not silently fall back to stdio inside a
662-
# container that expects an HTTP listener.
669+
# typo in COMMIT_CHECK_MCP_TRANSPORT must not silently fall back to stdio
670+
# inside a container that expects an HTTP listener.
663671
if args.transport not in ("stdio", "http"):
664672
parser.error(
665673
f"argument --transport: invalid choice: {args.transport!r}"
@@ -677,7 +685,16 @@ def main(argv: list[str] | None = None) -> None:
677685
allowed_origins = [
678686
o.strip() for o in args.allowed_origins.split(",") if o.strip()
679687
]
680-
if allowed_hosts or allowed_origins:
688+
# Turning on DNS-rebinding protection with an empty host allowlist
689+
# rejects *every* request with 421, so origins-only is never a usable
690+
# configuration — fail at startup instead of serving a server that
691+
# answers nothing.
692+
if allowed_origins and not allowed_hosts:
693+
parser.error(
694+
"--allowed-origins requires --allowed-hosts: an empty host"
695+
" allowlist rejects every request with 421 Misdirected Request"
696+
)
697+
if allowed_hosts:
681698
from mcp.server.transport_security import TransportSecuritySettings
682699

683700
http_kwargs["transport_security"] = TransportSecuritySettings(

‎tests/test_server.py‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -909,11 +909,11 @@ def fake_run(**kwargs: object) -> None:
909909
}
910910

911911
def test_transport_from_environment(self, monkeypatch: pytest.MonkeyPatch) -> None:
912-
"""MCP_TRANSPORT/MCP_HOST/MCP_PORT configure containers without argv."""
912+
"""COMMIT_CHECK_MCP_TRANSPORT/_HOST/_PORT configure containers without argv."""
913913
captured: dict[str, object] = {}
914-
monkeypatch.setenv("MCP_TRANSPORT", "http")
915-
monkeypatch.setenv("MCP_HOST", "0.0.0.0")
916-
monkeypatch.setenv("MCP_PORT", "8080")
914+
monkeypatch.setenv("COMMIT_CHECK_MCP_TRANSPORT", "http")
915+
monkeypatch.setenv("COMMIT_CHECK_MCP_HOST", "0.0.0.0")
916+
monkeypatch.setenv("COMMIT_CHECK_MCP_PORT", "8080")
917917
monkeypatch.setattr(server.mcp, "run", lambda **kw: captured.update(kw))
918918
server.main([])
919919
assert captured["transport"] == "streamable-http"
@@ -929,9 +929,9 @@ def test_rejects_unknown_transport_from_environment(
929929
self, monkeypatch: pytest.MonkeyPatch
930930
) -> None:
931931
"""argparse skips `choices` for env-supplied defaults; a typo in
932-
MCP_TRANSPORT must fail loudly, not silently serve stdio in a
932+
COMMIT_CHECK_MCP_TRANSPORT must fail loudly, not silently serve stdio in a
933933
container that expects an HTTP listener."""
934-
monkeypatch.setenv("MCP_TRANSPORT", "htpp")
934+
monkeypatch.setenv("COMMIT_CHECK_MCP_TRANSPORT", "htpp")
935935
monkeypatch.setattr(
936936
server.mcp, "run", lambda **kw: pytest.fail("server must not start")
937937
)
@@ -941,16 +941,16 @@ def test_rejects_unknown_transport_from_environment(
941941
def test_rejects_non_integer_port_from_environment(
942942
self, monkeypatch: pytest.MonkeyPatch
943943
) -> None:
944-
monkeypatch.setenv("MCP_PORT", "eight thousand")
944+
monkeypatch.setenv("COMMIT_CHECK_MCP_PORT", "eight thousand")
945945
with pytest.raises(SystemExit):
946946
server.main([])
947947

948948
def test_cli_port_overrides_invalid_environment_port(
949949
self, monkeypatch: pytest.MonkeyPatch
950950
) -> None:
951-
"""An invalid inherited MCP_PORT must not veto an explicit --port."""
951+
"""An invalid inherited COMMIT_CHECK_MCP_PORT must not veto an explicit --port."""
952952
captured: dict[str, object] = {}
953-
monkeypatch.setenv("MCP_PORT", "eight thousand")
953+
monkeypatch.setenv("COMMIT_CHECK_MCP_PORT", "eight thousand")
954954
monkeypatch.setattr(server.mcp, "run", lambda **kw: captured.update(kw))
955955
server.main(["--transport", "http", "--port", "9000"])
956956
assert captured["port"] == 9000
@@ -985,3 +985,31 @@ def test_no_transport_security_by_default(
985985
monkeypatch.setattr(server.mcp, "run", lambda **kw: captured.update(kw))
986986
server.main(["--transport", "http"])
987987
assert "transport_security" not in captured
988+
989+
def test_origins_without_hosts_is_rejected(
990+
self, monkeypatch: pytest.MonkeyPatch
991+
) -> None:
992+
"""Origins-only would enable rebinding protection with an empty host
993+
allowlist, which answers every request with 421 — verified against a
994+
live server. Refuse to start rather than serve a dead endpoint."""
995+
monkeypatch.setattr(
996+
server.mcp, "run", lambda **kw: pytest.fail("server must not start")
997+
)
998+
with pytest.raises(SystemExit):
999+
server.main(
1000+
["--transport", "http", "--allowed-origins", "https://app.example.com"]
1001+
)
1002+
1003+
def test_unprefixed_env_vars_are_ignored(
1004+
self, monkeypatch: pytest.MonkeyPatch
1005+
) -> None:
1006+
"""A bare MCP_TRANSPORT belongs to no particular server. Honouring it
1007+
would turn a stdio launch into an HTTP listener that never answers the
1008+
client's handshake."""
1009+
captured: dict[str, object] = {}
1010+
monkeypatch.setenv("MCP_TRANSPORT", "http")
1011+
monkeypatch.setenv("MCP_PORT", "9999")
1012+
monkeypatch.delenv("COMMIT_CHECK_MCP_TRANSPORT", raising=False)
1013+
monkeypatch.setattr(server.mcp, "run", lambda **kw: captured.update(kw))
1014+
server.main([])
1015+
assert captured == {"transport": "stdio"}

0 commit comments

Comments
 (0)