From 252befd8fca42c9e32021752d52d58c34a2d99fa Mon Sep 17 00:00:00 2001 From: Joachim Rosskopf Date: Tue, 1 Sep 2026 03:49:03 +0200 Subject: [PATCH 1/2] fix(mcp): validate the Host header, not only Origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Origin validation (#45, shipped in v2026.08.31) does not stop DNS rebinding, and the comment on #50 is right about why. Once evil.example makes rebind.evil.example resolve to 127.0.0.1, the browser treats the call as same-origin: the request carries Host: rebind.evil.example and either a matching Origin or none at all. Measured against that build, both shapes were answered 200 — the first passing ClassifyOrigin's same-origin rule, which proves nothing when the attacker owns both sides of the comparison, the second passing the no-Origin rule. The Host header is the half an attacker cannot launder, so it is now classified on its own, before Origin. Loopback names and IP literals pass unconditionally (an IP address has no name to rebind, which is what keeps --mcp-host 0.0.0.0 reachable at a LAN address with no config), as does the address bound and anything named by the new --allowed-hosts. Passing --allowed-hosts is what turns refusal on. Without it an unrecognised Host is still served, with one warning per distinct host — so a deployment reached through a DNS name or a reverse proxy does not break on upgrade. That leaves the rebinding path open by default, which is a deliberate trade-off and the reason #50 does not close here. --- README.md | 4 +- docs/cli-usage.md | 19 ++++ include/erpl_adt/mcp/http_security.hpp | 62 ++++++++++-- src/cli/command_executor.cpp | 13 ++- src/main.cpp | 11 ++- src/mcp/http_security.cpp | 86 ++++++++++++++++- src/mcp/mcp_http_server.cpp | 42 +++++++- test/mcp/test_http_security.cpp | 128 +++++++++++++++++++++++++ test/mcp/test_mcp_http_server.cpp | 116 ++++++++++++++++++++++ 9 files changed, 458 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 9878f84..0308be7 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,9 @@ Both HTTP servers (`mcp --http` and `catalog webui`) refuse cross-origin browser with 403 — a page you visit cannot post writes to your SAP system through them. Requests without an `Origin` header (curl, native MCP clients), same-origin requests and loopback origins are always allowed; add others with `--cors-origin`, and require a token with -`--auth-token` / `--auth-token-env`. See [docs/cli-usage.md](docs/cli-usage.md#http-transport-and-access-control). +`--auth-token` / `--auth-token-env`. Origin checking cannot see DNS rebinding, where the +attacker controls the `Host` too — pass `--allowed-hosts` to refuse any `Host` other than +loopback, an IP literal and the address bound. See [docs/cli-usage.md](docs/cli-usage.md#http-transport-and-access-control). The web UI ([`flutter/erpl_catalog_kit`](flutter/erpl_catalog_kit), compiled and embedded straight into the `erpl-adt` binary — see [Building from source](#building-from-source)) is **read-only against the cache except for curation**: Search, Browse, Entity Detail, Lineage, and Driver Tree all query the same fast `catalog_*` MCP tools the CLI and AI agents use; the Curate screen is the only one that writes, via `catalog_annotate`. There's no build/sync button — `catalog webui` doesn't hold a live SAP connection, so building, exporting, and syncing stay CLI-only operations. The Sync Status screen shows past sync runs and cache health, and Feed Export surfaces the exact `erpl-adt catalog build --format ...` command to run for each format, rather than re-implementing either client-side. diff --git a/docs/cli-usage.md b/docs/cli-usage.md index db585ec..ee16528 100644 --- a/docs/cli-usage.md +++ b/docs/cli-usage.md @@ -245,6 +245,7 @@ matters: | Flag | Effect | |------|--------| | `--cors-origin ` | Comma-separated extra browser origins allowed to call `/mcp`. `*` allows every origin. | +| `--allowed-hosts ` | Comma-separated `Host` header values this server answers to. Passing it refuses any other `Host` with 403. `*` allows every host. | | `--auth-token ` | Require `Authorization: Bearer `; requests without it get 401 and run nothing. | | `--auth-token-env ` | Read that token from an environment variable instead of the command line. | @@ -256,6 +257,24 @@ the case where a page the developer merely visited could otherwise post writes t SAP system, and binding to `127.0.0.1` does not prevent it because the browser is already inside the loopback boundary. +Origin validation alone does not stop **DNS rebinding**, which is why `--allowed-hosts` +exists. Once a page at `evil.example` makes `rebind.evil.example` resolve to `127.0.0.1`, +the browser believes it is talking to its own origin: the request arrives with `Host: +rebind.evil.example` and either a matching `Origin` or none at all, and both of those +satisfy the rules above — the attacker controls each side of the comparison. The `Host` +header is the half they cannot launder. + +`--allowed-hosts` names the hosts this server answers to; loopback names, IP literals (an +IP address has no DNS name to rebind, so `--mcp-host 0.0.0.0` reached at a LAN address +keeps working) and the address bound are always allowed. **Passing the flag is what turns +refusal on.** Without it, an unrecognised `Host` is still served, with one warning per +distinct host on stderr — so a deployment reached through a DNS name or a reverse proxy +does not break on upgrade, and closing the hole is one flag: + +```bash +erpl-adt mcp --http --mcp-host 0.0.0.0 --allowed-hosts mcp.internal.example +``` + Authentication is off unless a token is configured. Binding beyond loopback without one warns on stderr; `/healthz` never requires the token so liveness probes keep working. diff --git a/include/erpl_adt/mcp/http_security.hpp b/include/erpl_adt/mcp/http_security.hpp index 8762ef5..fc34762 100644 --- a/include/erpl_adt/mcp/http_security.hpp +++ b/include/erpl_adt/mcp/http_security.hpp @@ -19,16 +19,30 @@ namespace erpl_adt { // Binding to 127.0.0.1 does not help there — the browser is inside the // loopback boundary. // +// Host validation stops the same browser being aimed here by DNS +// rebinding, which Origin validation cannot see: once evil.example makes +// rebind.evil.example resolve to 127.0.0.1, the browser treats the call as +// same-origin and sends either a matching Origin or none at all. Both of +// those satisfy the Origin rules, because the attacker controls each side +// of the comparison. The Host header is the half they cannot launder. +// // The bearer token stops everything else that can reach the port. // -// Both are permissive by default so that no existing deployment breaks: a -// request without an Origin header is not a browser and is allowed, and the -// token is only enforced once one is configured. +// All three are permissive by default so that no existing deployment breaks: +// a request without an Origin header is not a browser and is allowed, an +// unrecognised Host is served with a warning until --allowed-hosts opts in to +// refusing it, and the token is only enforced once one is configured. // --------------------------------------------------------------------------- struct HttpSecurityOptions { // Extra origins allowed beyond same-origin and loopback. The single // entry "*" restores the historical allow-everything behaviour. std::vector allowed_origins; + // Hosts allowed beyond loopback and IP literals — the bind host and + // anything named by --allowed-hosts. "*" allows every host. + std::vector allowed_hosts; + // Whether an unrecognised Host is refused (true) or served with a + // warning (false, the default). Set by passing --allowed-hosts. + bool enforce_hosts = false; // When non-empty, /mcp requires "Authorization: Bearer ". std::string auth_token; }; @@ -55,6 +69,27 @@ enum class OriginVerdict { const std::string& host, const HttpSecurityOptions& options); +// Why a request's Host was accepted or refused. +enum class HostVerdict { + NoHost, // no Host header — browsers always send one, so not a browser + Loopback, // localhost / 127.0.0.1 / [::1] + IpLiteral, // an IP address — there is no name to rebind + Allowlisted, // the bind host, or named by --allowed-hosts + Wildcard, // --allowed-hosts '*' + Unrecognised, // a DNS name we were not told about — the rebinding shape +}; + +[[nodiscard]] constexpr bool IsAllowed(HostVerdict verdict) { + return verdict != HostVerdict::Unrecognised; +} + +// Classify a Host header against the options. `host` is the raw header value +// ("name" or "name:port"); an empty one means the header was absent. Whether +// an Unrecognised host is actually refused is the caller's decision, via +// HttpSecurityOptions::enforce_hosts. +[[nodiscard]] HostVerdict ClassifyHost(const std::string& host, + const HttpSecurityOptions& options); + // True when `header` carries the configured bearer token. Comparison is // constant-time so a token cannot be recovered one byte at a time by timing // the response. An empty configured token means "no auth required" and every @@ -62,18 +97,25 @@ enum class OriginVerdict { [[nodiscard]] bool BearerTokenMatches(const std::string& authorization_header, const std::string& expected_token); -// Split a comma-separated --cors-origin value into individual origins, -// trimming whitespace and dropping empties. -[[nodiscard]] std::vector ParseOriginList(const std::string& value); +// Split a comma-separated flag value into pieces, trimming whitespace and +// dropping empties. +[[nodiscard]] std::vector ParseCommaList(const std::string& value); + +// Historical name, kept because --cors-origin is the older flag. +[[nodiscard]] inline std::vector ParseOriginList( + const std::string& value) { + return ParseCommaList(value); +} // Build the options from CLI flag values, shared by `mcp --http` and -// `catalog webui`. Warns on `err` about the two configurations worth -// noticing — a wildcard origin, and binding somewhere other than loopback -// without a token — but does not refuse either, so nothing that runs today -// stops running. Returns nullopt only on an unusable configuration (an +// `catalog webui`. Warns on `err` about the configurations worth noticing — +// a wildcard origin or host, and binding somewhere other than loopback +// without a token — but does not refuse any of them, so nothing that runs +// today stops running. Returns nullopt only on an unusable configuration (an // --auth-token-env naming a variable that is not set), having reported it. [[nodiscard]] std::optional ResolveHttpSecurity( const std::string& cors_origin_flag, + const std::string& allowed_hosts_flag, const std::string& auth_token_flag, const std::string& auth_token_env_flag, const std::string& bind_host, diff --git a/src/cli/command_executor.cpp b/src/cli/command_executor.cpp index d1d81fd..c6321e6 100644 --- a/src/cli/command_executor.cpp +++ b/src/cli/command_executor.cpp @@ -2900,6 +2900,7 @@ int HandleCatalogWebui(const CommandArgs& args) { RegisterCatalogStoreTools(registry, store); auto security = ResolveHttpSecurity(GetFlag(args, "cors-origin"), + GetFlag(args, "allowed-hosts"), GetFlag(args, "auth-token"), GetFlag(args, "auth-token-env"), host, std::cerr); @@ -7952,13 +7953,19 @@ void RegisterAllCommands(CommandRouter& router) { "having been run first, this serves an instructional message instead of the app.\n\n" "Access control: requests without an Origin header (curl, native clients), " "same-origin requests and loopback origins are allowed; any other browser " - "origin is refused with 403 unless named with --cors-origin. Binding beyond " - "127.0.0.1 without --auth-token exposes the catalog API — including the " - "curation writes of catalog_annotate — to everyone who can reach the port."; + "origin is refused with 403 unless named with --cors-origin. Origin alone " + "does not stop DNS rebinding, though: a page that points its own name at " + "127.0.0.1 arrives with a Host it controls and an Origin to match. Pass " + "--allowed-hosts to refuse any Host other than loopback, an IP literal and " + "the address bound; without it such a request is served with a warning. " + "Binding beyond 127.0.0.1 without --auth-token exposes the catalog API — " + "including the curation writes of catalog_annotate — to everyone who can " + "reach the port."; help.flags = { {"port", "", "Port to listen on (default: 8383)", false}, {"host", "", "Host/address to bind (default: 127.0.0.1)", false}, {"cors-origin", "", "Comma-separated extra origins allowed to call the API", false}, + {"allowed-hosts", "", "Host headers this server answers to; passing it refuses any other with 403", false}, {"auth-token", "", "Require 'Authorization: Bearer ' on the API", false}, {"auth-token-env", "", "Read that token from an environment variable", false}, }; diff --git a/src/main.cpp b/src/main.cpp index bd4c94d..3a62375 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -123,6 +123,12 @@ void PrintMcpHelp(std::ostream& out) { out << " Same-origin, loopback and non-browser (no Origin\n"; out << " header) requests are always allowed; anything else is\n"; out << " refused with 403. '*' allows every origin.\n"; + out << " --allowed-hosts Comma-separated Host header values this server answers\n"; + out << " to. Loopback, IP literals and the bound address are\n"; + out << " always allowed. Passing this refuses any other Host\n"; + out << " with 403 (DNS-rebinding defence); without it such a\n"; + out << " request is served with a warning. '*' allows every\n"; + out << " Host.\n"; out << " --auth-token Require 'Authorization: Bearer ' on /mcp\n"; out << " --auth-token-env Read that token from an environment variable\n"; } @@ -625,8 +631,9 @@ int HandleMcpServer(int argc, const char* const* argv) { } auto mcp_host = get("mcp-host", "127.0.0.1"); - auto security = ResolveHttpSecurity(get("cors-origin"), get("auth-token"), - get("auth-token-env"), mcp_host, std::cerr); + auto security = ResolveHttpSecurity(get("cors-origin"), get("allowed-hosts"), + get("auth-token"), get("auth-token-env"), + mcp_host, std::cerr); if (!security.has_value()) { return kExitInternal; } diff --git a/src/mcp/http_security.cpp b/src/mcp/http_security.cpp index 6278406..c03f698 100644 --- a/src/mcp/http_security.cpp +++ b/src/mcp/http_security.cpp @@ -59,6 +59,25 @@ bool IsLoopbackHost(const std::string& host) { host == "::1"; } +// Is this host an IP address rather than a name? Only names can be pointed at +// 127.0.0.1 by an attacker's DNS server, so an IP literal cannot carry a +// rebinding attack — which is what lets `--host 0.0.0.0` stay reachable at a +// LAN address with no configuration. +// +// Deliberately loose: a bracketed value is IPv6, and anything made only of +// digits and dots is IPv4. Both tests are cheap and neither can be satisfied +// by a registrable domain name, which is the only property that matters here. +bool IsIpLiteral(const std::string& host) { + if (host.size() >= 2 && host.front() == '[' && host.back() == ']') { + return true; + } + if (host.empty()) { + return false; + } + return host.find_first_not_of("0123456789.") == std::string::npos && + host.find('.') != std::string::npos; +} + } // namespace OriginVerdict ClassifyOrigin(const std::string& origin, const std::string& host, @@ -98,6 +117,43 @@ OriginVerdict ClassifyOrigin(const std::string& origin, const std::string& host, return OriginVerdict::Denied; } +HostVerdict ClassifyHost(const std::string& host, + const HttpSecurityOptions& options) { + const auto authority = Authority(host); + if (authority.empty()) { + // Browsers always send Host, so an absent one is not the shape this + // check exists for — it is an HTTP/1.0 or hand-rolled client. + return HostVerdict::NoHost; + } + + for (const auto& allowed : options.allowed_hosts) { + if (Trim(allowed) == "*") { + return HostVerdict::Wildcard; + } + } + + const auto name = HostOf(authority); + if (IsLoopbackHost(name)) { + return HostVerdict::Loopback; + } + if (IsIpLiteral(name)) { + return HostVerdict::IpLiteral; + } + + for (const auto& allowed : options.allowed_hosts) { + const auto entry = Authority(allowed); + // An entry naming a port is held to it; a bare name matches any port, + // because "the host I serve on" is what the operator meant to say. + const bool matches = (HostOf(entry) == entry) ? (entry == name) + : (entry == authority); + if (matches) { + return HostVerdict::Allowlisted; + } + } + + return HostVerdict::Unrecognised; +} + bool BearerTokenMatches(const std::string& authorization_header, const std::string& expected_token) { if (expected_token.empty()) { @@ -128,11 +184,22 @@ bool BearerTokenMatches(const std::string& authorization_header, } std::optional ResolveHttpSecurity( - const std::string& cors_origin_flag, const std::string& auth_token_flag, - const std::string& auth_token_env_flag, const std::string& bind_host, - std::ostream& err) { + const std::string& cors_origin_flag, const std::string& allowed_hosts_flag, + const std::string& auth_token_flag, const std::string& auth_token_env_flag, + const std::string& bind_host, std::ostream& err) { HttpSecurityOptions options; - options.allowed_origins = ParseOriginList(cors_origin_flag); + options.allowed_origins = ParseCommaList(cors_origin_flag); + options.allowed_hosts = ParseCommaList(allowed_hosts_flag); + // Passing the flag at all is what turns refusal on. Without it an + // unrecognised Host is served with a warning, so no deployment reached + // through a DNS name today stops working. + options.enforce_hosts = !options.allowed_hosts.empty(); + // The address the operator bound to is allowed without having to name it + // twice. Loopback and IP literals are already allowed by ClassifyHost. + const auto bind_name = HostOf(Authority(bind_host)); + if (!bind_name.empty() && !IsLoopbackHost(bind_name) && !IsIpLiteral(bind_name)) { + options.allowed_hosts.push_back(bind_name); + } options.auth_token = auth_token_flag; if (options.auth_token.empty() && !auth_token_env_flag.empty()) { @@ -153,6 +220,15 @@ std::optional ResolveHttpSecurity( } } + for (const auto& host : options.allowed_hosts) { + if (host == "*") { + err << "Warning: --allowed-hosts '*' accepts any Host header, which " + "leaves DNS rebinding open. Name the hosts you serve on " + "instead.\n"; + break; + } + } + if (options.auth_token.empty() && !IsLoopbackHost(HostOf(Authority(bind_host)))) { err << "Warning: binding " << bind_host << " exposes this server beyond this machine with no authentication. " @@ -162,7 +238,7 @@ std::optional ResolveHttpSecurity( return options; } -std::vector ParseOriginList(const std::string& value) { +std::vector ParseCommaList(const std::string& value) { std::vector out; size_t pos = 0; while (pos <= value.size()) { diff --git a/src/mcp/mcp_http_server.cpp b/src/mcp/mcp_http_server.cpp index ba5ee8a..616cb28 100644 --- a/src/mcp/mcp_http_server.cpp +++ b/src/mcp/mcp_http_server.cpp @@ -3,7 +3,9 @@ #include #include +#include #include +#include #include #include @@ -61,9 +63,29 @@ struct McpHttpServer::Impl { std::mutex mcp_mutex; HttpSecurityOptions security; + // Hosts already warned about, so a scripted attack cannot flood stderr + // with one line per request. Guarded by warned_hosts_mutex because the + // warning is emitted from the pre-routing hook, which runs on httplib's + // thread pool — before /mcp's own lock is taken. + std::set warned_hosts; + std::mutex warned_hosts_mutex; Impl(ToolRegistry registry, HttpSecurityOptions security_options) : mcp(std::move(registry)), security(std::move(security_options)) {} + + // Emit the DNS-rebinding warning once per distinct Host. + void WarnAboutHostOnce(const std::string& host) { + { + std::lock_guard lock(warned_hosts_mutex); + if (!warned_hosts.insert(host).second) { + return; + } + } + std::cerr << "Warning: served a request for Host '" << host + << "', which is not loopback, an IP literal, or a configured " + "host. A page using DNS rebinding looks exactly like this. " + "Pass --allowed-hosts to refuse it.\n"; + } }; McpHttpServer::McpHttpServer(ToolRegistry registry, bool serve_webui) @@ -85,9 +107,25 @@ McpHttpServer::McpHttpServer(ToolRegistry registry, bool serve_webui, // can't accidentally omit it. impl_->http.set_pre_routing_handler([this](const httplib::Request& req, httplib::Response& res) { + // Host first, and on its own: DNS rebinding hands the attacker both + // the Host and the Origin, so the same-origin rule below cannot see + // it. Checking Host first means that once enforcement is on, a + // rebound name is refused whatever Origin it claims. + const auto host = req.get_header_value("Host"); + const auto host_verdict = ClassifyHost(host, impl_->security); + if (!IsAllowed(host_verdict)) { + if (impl_->security.enforce_hosts) { + res.status = 403; + res.set_content( + R"({"error":"host not allowed; pass --allowed-hosts to allow it"})", + "application/json"); + return httplib::Server::HandlerResponse::Handled; + } + impl_->WarnAboutHostOnce(host); + } + const auto origin = req.get_header_value("Origin"); - const auto verdict = - ClassifyOrigin(origin, req.get_header_value("Host"), impl_->security); + const auto verdict = ClassifyOrigin(origin, host, impl_->security); if (!IsAllowed(verdict)) { res.status = 403; diff --git a/test/mcp/test_http_security.cpp b/test/mcp/test_http_security.cpp index 8fe32ad..13eaf92 100644 --- a/test/mcp/test_http_security.cpp +++ b/test/mcp/test_http_security.cpp @@ -2,6 +2,9 @@ #include +#include +#include + using namespace erpl_adt; // =========================================================================== @@ -142,3 +145,128 @@ TEST_CASE("ParseOriginList: empty pieces are dropped", "[mcp][security]") { REQUIRE(parsed.size() == 1); CHECK(parsed[0] == "https://a.example"); } + +// =========================================================================== +// Host classification +// +// Origin validation alone does not stop DNS rebinding. Once a page at +// evil.example makes rebind.evil.example resolve to 127.0.0.1, the browser +// considers the request same-origin and sends Host: rebind.evil.example with +// either a matching Origin or none at all — and both of those are allowed by +// the Origin rules above. The Host header is the half the attacker cannot +// launder, so it is checked on its own. +// +// An IP literal is a deliberate pass: there is no name to rebind, which is +// what keeps `catalog webui --host 0.0.0.0` reachable at a LAN address with +// no configuration. +// =========================================================================== + +TEST_CASE("ClassifyHost: loopback names are recognised", "[mcp][security]") { + HttpSecurityOptions options; + CHECK(ClassifyHost("localhost:8383", options) == HostVerdict::Loopback); + CHECK(ClassifyHost("127.0.0.1:8383", options) == HostVerdict::Loopback); + CHECK(ClassifyHost("[::1]:8383", options) == HostVerdict::Loopback); + CHECK(ClassifyHost("LocalHost", options) == HostVerdict::Loopback); +} + +TEST_CASE("ClassifyHost: an IP literal cannot be rebound", "[mcp][security]") { + HttpSecurityOptions options; + CHECK(ClassifyHost("192.168.1.5:8383", options) == HostVerdict::IpLiteral); + CHECK(ClassifyHost("10.0.0.7", options) == HostVerdict::IpLiteral); + CHECK(ClassifyHost("[fe80::1]:8383", options) == HostVerdict::IpLiteral); +} + +TEST_CASE("ClassifyHost: an unknown name is the rebinding shape", + "[mcp][security]") { + HttpSecurityOptions options; + CHECK(ClassifyHost("rebind.evil.example:8383", options) == + HostVerdict::Unrecognised); + // A name that merely contains a loopback label is still a name. + CHECK(ClassifyHost("localhost.evil.example", options) == + HostVerdict::Unrecognised); + CHECK(ClassifyHost("127.0.0.1.evil.example", options) == + HostVerdict::Unrecognised); +} + +TEST_CASE("ClassifyHost: a configured host is allowed", "[mcp][security]") { + HttpSecurityOptions options; + options.allowed_hosts = {"mcp.internal.example"}; + CHECK(ClassifyHost("mcp.internal.example:8383", options) == + HostVerdict::Allowlisted); + CHECK(ClassifyHost("mcp.internal.example", options) == HostVerdict::Allowlisted); + CHECK(ClassifyHost("other.internal.example", options) == HostVerdict::Unrecognised); +} + +TEST_CASE("ClassifyHost: a configured host may name a port", "[mcp][security]") { + // "host:port" and a bare host both mean the same thing to a user; only + // the port-qualified form is picky, and then only about that port. + HttpSecurityOptions options; + options.allowed_hosts = {"mcp.internal.example:8383"}; + CHECK(ClassifyHost("mcp.internal.example:8383", options) == + HostVerdict::Allowlisted); + CHECK(ClassifyHost("mcp.internal.example:9999", options) == + HostVerdict::Unrecognised); +} + +TEST_CASE("ClassifyHost: '*' allows any host", "[mcp][security]") { + HttpSecurityOptions options; + options.allowed_hosts = {"*"}; + CHECK(ClassifyHost("rebind.evil.example", options) == HostVerdict::Wildcard); +} + +TEST_CASE("ClassifyHost: a missing Host header is not a browser", + "[mcp][security]") { + // Browsers always send Host, so an absent one cannot be the rebinding + // shape — it is an HTTP/1.0 client or a hand-rolled request. + HttpSecurityOptions options; + CHECK(ClassifyHost("", options) == HostVerdict::NoHost); + CHECK(ClassifyHost(" ", options) == HostVerdict::NoHost); +} + +TEST_CASE("ClassifyHost: only Unrecognised is ever refused", "[mcp][security]") { + CHECK(IsAllowed(HostVerdict::NoHost)); + CHECK(IsAllowed(HostVerdict::Loopback)); + CHECK(IsAllowed(HostVerdict::IpLiteral)); + CHECK(IsAllowed(HostVerdict::Allowlisted)); + CHECK(IsAllowed(HostVerdict::Wildcard)); + CHECK(!IsAllowed(HostVerdict::Unrecognised)); +} + +// =========================================================================== +// ResolveHttpSecurity — the flag-to-options mapping +// =========================================================================== + +TEST_CASE("ResolveHttpSecurity: enforcement is off until --allowed-hosts", + "[mcp][security]") { + // The default has to stay warn-only: every deployment reached by a DNS + // name today keeps working, and enforcement is one flag away. + std::ostringstream err; + auto options = ResolveHttpSecurity("", "", "", "", "127.0.0.1", err); + REQUIRE(options.has_value()); + CHECK(!options->enforce_hosts); + + std::ostringstream err2; + auto enforcing = + ResolveHttpSecurity("", "mcp.internal.example", "", "", "0.0.0.0", err2); + REQUIRE(enforcing.has_value()); + CHECK(enforcing->enforce_hosts); + CHECK(ClassifyHost("mcp.internal.example", *enforcing) == + HostVerdict::Allowlisted); +} + +TEST_CASE("ResolveHttpSecurity: the bind host is allowed without naming it twice", + "[mcp][security]") { + std::ostringstream err; + auto options = ResolveHttpSecurity("", "other.example", "", "", + "mcp.internal.example", err); + REQUIRE(options.has_value()); + CHECK(ClassifyHost("mcp.internal.example:8383", *options) == + HostVerdict::Allowlisted); +} + +TEST_CASE("ResolveHttpSecurity: --allowed-hosts '*' warns", "[mcp][security]") { + std::ostringstream err; + auto options = ResolveHttpSecurity("", "*", "", "", "0.0.0.0", err); + REQUIRE(options.has_value()); + CHECK(err.str().find("--allowed-hosts") != std::string::npos); +} diff --git a/test/mcp/test_mcp_http_server.cpp b/test/mcp/test_mcp_http_server.cpp index b20fdaa..1e5f928 100644 --- a/test/mcp/test_mcp_http_server.cpp +++ b/test/mcp/test_mcp_http_server.cpp @@ -466,3 +466,119 @@ TEST_CASE("McpHttpServer: the web UI catch-all does not swallow GET /mcp", server.Stop(); server_thread.join(); } + +// =========================================================================== +// Host validation (DNS rebinding) +// =========================================================================== + +namespace { + +// The message body used by the Host tests below — a tool call, so a request +// that is wrongly let through is visible as an executed tool. +std::string EchoCall() { + return nlohmann::json{{"jsonrpc", "2.0"}, + {"id", 1}, + {"method", "tools/call"}, + {"params", {{"name", "echo"}, + {"arguments", {{"text", "hi"}}}}}} + .dump(); +} + +} // anonymous namespace + +TEST_CASE("McpHttpServer: an unknown Host is served by default", + "[mcp][http][security]") { + // Warn-only until the operator opts in: a deployment reached through a + // DNS name or a reverse proxy keeps working with no new flag. + McpHttpServer server(MakeEchoRegistry()); + auto port = static_cast(TestPort() + 30); + + std::thread server_thread([&] { (void)server.Run("127.0.0.1", port); }); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + + httplib::Client client("127.0.0.1", port); + const httplib::Headers rebound{{"Host", "rebind.evil.example"}}; + auto response = client.Post("/mcp", rebound, EchoCall(), "application/json"); + REQUIRE(response != nullptr); + CHECK(response->status == 200); + + server.Stop(); + server_thread.join(); +} + +TEST_CASE("McpHttpServer: with --allowed-hosts, a rebound Host is refused", + "[mcp][http][security]") { + // The DNS-rebinding shape: evil.example points rebind.evil.example at + // 127.0.0.1, so the browser calls this server believing it is same-origin + // and sends an Origin the Origin check cannot fault. Host is the half the + // attacker cannot launder. + HttpSecurityOptions security; + security.allowed_hosts = {"mcp.internal.example"}; + security.enforce_hosts = true; + + McpHttpServer server(MakeEchoRegistry(), false, security); + auto port = static_cast(TestPort() + 31); + + std::thread server_thread([&] { (void)server.Run("127.0.0.1", port); }); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + + httplib::Client client("127.0.0.1", port); + + const httplib::Headers rebound{ + {"Host", "rebind.evil.example"}, + {"Origin", "http://rebind.evil.example"}}; + auto refused = client.Post("/mcp", rebound, EchoCall(), "application/json"); + REQUIRE(refused != nullptr); + CHECK(refused->status == 403); + CHECK(refused->get_header_value("Access-Control-Allow-Origin").empty()); + CHECK(refused->body.find("hi") == std::string::npos); // the tool did not run + + // Same request with no Origin at all — the other half of the shape, and + // the one an Origin-only check waves straight through. + const httplib::Headers rebound_no_origin{{"Host", "rebind.evil.example"}}; + auto refused_silent = + client.Post("/mcp", rebound_no_origin, EchoCall(), "application/json"); + REQUIRE(refused_silent != nullptr); + CHECK(refused_silent->status == 403); + + // Loopback and the configured name still work. + const httplib::Headers loopback{{"Host", "127.0.0.1"}}; + auto allowed = client.Post("/mcp", loopback, EchoCall(), "application/json"); + REQUIRE(allowed != nullptr); + CHECK(allowed->status == 200); + + const httplib::Headers configured{{"Host", "mcp.internal.example"}}; + auto named = client.Post("/mcp", configured, EchoCall(), "application/json"); + REQUIRE(named != nullptr); + CHECK(named->status == 200); + + server.Stop(); + server_thread.join(); +} + +TEST_CASE("McpHttpServer: enforcing hosts leaves the web UI's own origin alone", + "[mcp][http][security]") { + // The embedded UI is reached at whatever address the browser used, and + // its POSTs are same-origin. Enforcement must not cost it anything. + HttpSecurityOptions security; + security.allowed_hosts = {"catalog.internal"}; + security.enforce_hosts = true; + + McpHttpServer server(MakeEchoRegistry(), false, security); + auto port = static_cast(TestPort() + 32); + + std::thread server_thread([&] { (void)server.Run("127.0.0.1", port); }); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + + httplib::Client client("127.0.0.1", port); + const httplib::Headers same_origin{{"Host", "catalog.internal"}, + {"Origin", "http://catalog.internal"}}; + auto response = client.Post("/mcp", same_origin, EchoCall(), "application/json"); + REQUIRE(response != nullptr); + CHECK(response->status == 200); + CHECK(response->get_header_value("Access-Control-Allow-Origin") == + "http://catalog.internal"); + + server.Stop(); + server_thread.join(); +} From d346e8c1d62584875992c945fcfbb96a4932f4fc Mon Sep 17 00:00:00 2001 From: Joachim Rosskopf Date: Tue, 1 Sep 2026 04:12:21 +0200 Subject: [PATCH 2/2] test(mcp): give the new Host tests their own ports TestPort()+30..32 already belong to the HTTP status-conformance tests, so the new cases were binding ports a neighbour had just used. Windows CI crashed in a nearby test because of it. --- test/mcp/test_mcp_http_server.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/mcp/test_mcp_http_server.cpp b/test/mcp/test_mcp_http_server.cpp index 1e5f928..7b3a4b5 100644 --- a/test/mcp/test_mcp_http_server.cpp +++ b/test/mcp/test_mcp_http_server.cpp @@ -491,7 +491,7 @@ TEST_CASE("McpHttpServer: an unknown Host is served by default", // Warn-only until the operator opts in: a deployment reached through a // DNS name or a reverse proxy keeps working with no new flag. McpHttpServer server(MakeEchoRegistry()); - auto port = static_cast(TestPort() + 30); + auto port = static_cast(TestPort() + 40); std::thread server_thread([&] { (void)server.Run("127.0.0.1", port); }); std::this_thread::sleep_for(std::chrono::milliseconds(200)); @@ -517,7 +517,7 @@ TEST_CASE("McpHttpServer: with --allowed-hosts, a rebound Host is refused", security.enforce_hosts = true; McpHttpServer server(MakeEchoRegistry(), false, security); - auto port = static_cast(TestPort() + 31); + auto port = static_cast(TestPort() + 41); std::thread server_thread([&] { (void)server.Run("127.0.0.1", port); }); std::this_thread::sleep_for(std::chrono::milliseconds(200)); @@ -565,7 +565,7 @@ TEST_CASE("McpHttpServer: enforcing hosts leaves the web UI's own origin alone", security.enforce_hosts = true; McpHttpServer server(MakeEchoRegistry(), false, security); - auto port = static_cast(TestPort() + 32); + auto port = static_cast(TestPort() + 42); std::thread server_thread([&] { (void)server.Run("127.0.0.1", port); }); std::this_thread::sleep_for(std::chrono::milliseconds(200));