Add Origin header validation for DNS-rebind protection

[JAORMX]

Summary

ToolHive's proxy layer had no Origin header validation, and the legacy HTTP+SSE transport emitted Access-Control-Allow-Origin: *, leaving both modes open to DNS-rebinding attacks from browser clients. MCP 2025-11-25 §"Security Warning" requires servers to validate Origin on all connections and respond with 403 when the value is invalid.

• Add pkg/transport/middleware/origin/ with a factory (for the RunConfig-driven chain used by thv run / thv-proxyrunner / vMCP) and a bare constructor (for the inline chain used by thv proxy)
• New --allowed-origins flag on both thv run and thv proxy; when empty and the bind is loopback, a default loopback-only allowlist is derived automatically. Non-loopback binds without an explicit list log a warning (bind opt-in hardening follows in a separate PR)
• Case-insensitive scheme+host match per RFC 6454 §4; requests with multiple Origin headers are rejected outright; 403 responses carry a canonical JSON-RPC error body
• Removed Access-Control-Allow-Origin: * from the httpsse SSE handler — the wildcard would have neutered the enforcement via preflight response inheritance

Closes audit row 5 (Origin validation absent) from the MCP 2025-11-25 spec-compliance audit.

Type of change

• Bug fix

Test plan

• Unit tests (task test) — pkg/transport/middleware/origin/..., pkg/runner/..., pkg/transport/proxy/httpsse/... all green
• Linting (task lint-fix) — 0 issues
• Manual testing — verified thv run and thv proxy with --host 127.0.0.1 auto-derive the loopback allowlist; --host 0.0.0.0 without --allowed-origins logs the expected warning; explicit --allowed-origins takes precedence over the auto-derivation. Verified the 403 JSON-RPC error body shape with curl + crafted Origin headers.

Does this introduce a user-facing change?

Yes. Two related changes:

1. New flag: --allowed-origins on thv run and thv proxy accepts a repeatable exact-match list for the HTTP Origin header. Default behavior for loopback binds preserves existing browser-client flows (same-origin from localhost / 127.0.0.1 / [::1] is auto-allowed). Non-loopback binds continue to work as before but now log a warning recommending explicit origins.
2. Browser CORS behavior: the * wildcard on the httpsse /sse endpoint is gone. Any browser client that relied on cross-origin requests to the legacy SSE transport will need to migrate to the streamable transport or be added to --allowed-origins.

Implementation plan

Approved implementation plan

PR-1 from the MCP proxy spec-compliance Phase 2 plan (~/.claude/plans/yes-let-s-plan-phase-abundant-dragonfly.md, personal / not in repo).

Scope: implement and wire the Origin-header validation middleware, remove the httpsse Access-Control-Allow-Origin: * reflector. Uses the bare-middleware + factory pattern modelled on pkg/transport/middleware/header_forward.go. Shared default-allowlist derivation (origin.ResolveAllowedOrigins) keeps the runner and thv proxy call sites from drifting.

Deferred to follow-up PRs:

• PR-12 (audit row 22): warn on non-loopback bind + --allow-public-bind opt-in — this PR only adds the warn path through the middleware absence
• Operator CRD wiring: MCPServer CRD does not yet expose allowedOrigins for the operator-reconciled proxyrunner pods; a follow-up PR will add the CRD field and serialize it into runconfig
• Recovery middleware scope: the pre-existing Recovery middleware wraps only the backend handler rather than the full middleware chain; untouched here

Special notes for reviewers

• The loopback-default derivation uses net.ParseIP(host).IsLoopback() so 127.0.0.2 and similar variants are handled; the earlier literal-switch approach missed them.
• origin.ResolveAllowedOrigins is exported specifically so cmd/thv/app/proxy.go (the inline chain) can share logic with pkg/runner/middleware.go. Doc comment explains the contract.
• The empty-allowlist path registers a pass-through middleware (not a no-op at the factory) so the middleware slot stays occupied; a WARN log makes the disabled state visible.
• Part of a broader Phase 2 plan that fixes 16 audit findings across 15 PRs. Subsequent PRs (event-IDs + primer, protocol-version allowlist, session-identity binding, etc.) will follow once this lands.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 88.61789% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.82%. Comparing base (6f63ac0) to head (322b09f).

Files with missing lines	Patch %	Lines
pkg/runner/config_builder.go	0.00%	4 Missing ⚠️
pkg/runner/middleware.go	78.94%	2 Missing and 2 partials ⚠️
pkg/transport/middleware/origin/origin.go	95.83%	2 Missing and 2 partials ⚠️
pkg/runner/runner.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4908      +/-   ##
==========================================
+ Coverage   68.77%   68.82%   +0.04%     
==========================================
  Files         629      630       +1     
  Lines       63914    64032     +118     
==========================================
+ Hits        43955    44068     +113     
- Misses      16703    16705       +2     
- Partials     3256     3259       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jhrozek]

A few clarifying questions on wiring/coverage and a couple of style questions. Nothing blocking — just want to make sure the integration paths and API shape are intentional. Implementation itself looks solid; the spec compliance is correct.

[jhrozek]

The new addOriginMiddleware is added to PopulateMiddlewareConfigs, but WithMiddlewareFromFlags / addCoreMiddlewares (the path used by thv run) doesn't include it. Since runner.Run skips PopulateMiddlewareConfigs when MiddlewareConfigs is pre-populated (runner.go:232), thv run --allowed-origins=... plumbs the flag into RunConfig.AllowedOrigins — which is exactly what this builder option does — but the middleware never registers at runtime.

Is this intentional, or an omission? If intentional, what's the rationale for excluding thv run from the protection?

[JAORMX]

Good catch — and the fix needed to go deeper than moving the call into WithMiddlewareFromFlags. The builder resolves the effective port in validateConfig (via WithPorts), which runs after all options, so b.config.Port is still 0 during WithMiddlewareFromFlags — ResolveAllowedOrigins would have returned nil and silently disabled loopback defaults for thv run. Fixed by wiring Origin validation centrally in runner.Run (new prependOriginMiddleware), after both population paths, where Host/Port/AllowedOrigins are fully resolved. Removed it from PopulateMiddlewareConfigs so there's one wiring site, and prepended it so it still runs first (rejects before auth). Added TestPrependOriginMiddleware covering the thv run path.

[jhrozek]

The operator path uses PopulateMiddlewareConfigs so the factory side is wired, but MCPServerSpec / MCPRemoteProxySpec / VirtualMCPServerSpec have no AllowedOrigins field. Combined with operator-deployed pods binding to non-loopback addresses, ResolveAllowedOrigins returns nil and addOriginMiddleware skips registration with a WARN — so K8s deployments ship with Origin validation disabled.

Is this expected, planned for a follow-up PR, or considered out of scope for the CRDs? If a follow-up, would it be worth a // TODO or a note in the PR description so it's tracked?

[JAORMX]

Deferring CRD wiring to a follow-up to keep this PR focused on CLI/proxyrunner. Documented explicitly in the commit message and in the prependOriginMiddleware doc comment: operator non-loopback pods log the WARN rather than enforcing until the CRD field lands. Tracked in a follow-up issue: "Add allowedOrigins to MCPServer/MCPRemoteProxy/VirtualMCPServer CRDs + operator wiring."

[jhrozek]

The commit message says the middleware is wired into "thv run / thv-proxyrunner / vMCP", but vMCP composes its own chain via factory.NewIncomingAuthMiddleware and pkg/vmcp/server/server.go Handler — neither references the new origin package. Grep across pkg/vmcp/ and cmd/vmcp/ finds no usage.

Should vMCP be wired here too, or should the commit message be corrected to drop the vMCP claim and tracked as a follow-up?

[JAORMX]

Correct — vMCP composes its own chain and doesn't reference this package. Dropped the "vMCP" claim from the commit message and called out vMCP wiring as a follow-up. Tracked in a follow-up issue: "Wire Origin validation into the vMCP middleware chain."

[jhrozek]

Was net/url.Parse considered for this function, and if so, what made hand-rolled parsing the better choice?

Asking because the current implementation has a few RFC-6454 edge cases that url.Parse would handle for free: userinfo (https://user:pass@host) is preserved rather than rejected per §6, IPv6 bracket detection is hand-rolled, and IDN/punycode normalization isn't applied. Curious if these are deliberate tradeoffs (e.g., avoiding a dep on golang.org/x/net/idna, or the cost of full URL parsing on the hot path).

[JAORMX]

Switched to net/url.Parse. It now rejects userinfo per RFC 6454 §6 (closing an Origin-smuggling vector like http://localhost:8080@evil.example), handles IPv6 brackets and host/port via Hostname()/Port(), and lowercases scheme+host per §4. Unparseable / scheme-less / host-less / userinfo inputs map to a sentinel that can never match (fail closed). Added tests for these cases. IDN/punycode normalization is intentionally not applied — matching stays on the literal serialized form; can add x/net/idna in a follow-up if we want host equivalence.

[jhrozek]

What's the purpose of having both CreateOriginMiddleware (exported, single-line forwarder) and createOriginHandler (unexported, the actual implementation)? The factory path bypasses the public API and calls the unexported one directly, and tests also use the unexported one — so the only caller of CreateOriginMiddleware is cmd/thv/app/proxy.go.

Could one of them be removed, or is there a reason to keep both as separate entry points (e.g., a planned divergence in behavior)?

[JAORMX]

Collapsed into a single exported entry point NewHandler, used by both the factory (CreateMiddleware) and cmd/thv/app/proxy.go. Removed the duplicate forwarder.

[JAORMX]

Filed the two deferred follow-ups referenced in the threads above:

• Add allowedOrigins to workload CRDs + operator wiring #5399 — Add allowedOrigins to workload CRDs + operator wiring (the K8s enforcement gap)
• Wire Origin validation into the vMCP middleware chain #5400 — Wire Origin validation into the vMCP middleware chain