security: enforce MCP Origin allowlist to prevent DNS-rebinding attacks - #75
Conversation
There was a problem hiding this comment.
Thanks for adding early Origin enforcement and focused coverage. I am requesting changes because the implementation does not yet satisfy the MCP 2026-07-28 Streamable HTTP security requirement.
Blocking issues:
- Origin validation is opt-in and disabled by default, although the specification requires validation on all incoming connections.
- The custom parser reimplements standards-sensitive URL/origin behavior and treats a portless origin as a wildcard for every port. Please use the workspace existing
url = 2.5.8crate and compare typedurl::Originvalues instead. - The new CLI/environment flag is unnecessary and creates a supported insecure state for a mandatory transport invariant.
Please remove the optional configuration surface, enforce validation automatically for every request carrying Origin while continuing to allow requests without it, use Url::parse(...).origin() for canonical typed comparison, and add regressions for the empty-policy and non-default-port cases. Update the security and request-flow documentation to cite and describe MCP 2026-07-28.
The early middleware ordering and HTTP 403 behavior are directionally correct. This review is limited to the Origin-validation scope; the broader stateless MCP migration remains separate.
dawid-nowak
left a comment
There was a problem hiding this comment.
See comments, but generally speaking ideally we should be re-using the Rust crates as much as possible especially the ones that are widely used and trusted.
In this is case, a lot of code can be simplified/removed if url::Url is used.
919bb6e to
969ec23
Compare
|
[P1] Keep Origin validation active when the allowlist is empty Fixed. The early-return bypass is gone. When [P1] Replace the custom Origin parser with the Fixed. The hand-rolled [P1] Do not expose mandatory validation as a new CLI opt-out Fixed. There is no disable flag. The two new fields — [P2] Document MCP 2026-07-28, remove weakened SHOULD language Fixed.
|
lucarlig
left a comment
There was a problem hiding this comment.
Thanks for the updates. CI is green, but four issues remain:
-
[P1] Default DNS-rebinding protection is ineffective.
Origin == Hostcompares two attacker-controlled values, so a rebinding domain can pass. Remove this fallback: missingOriginmay pass, but a presentOriginmust match an explicit trusted allowlist; an empty allowlist should return 403. -
[P1] HTTPS is inferred as HTTP. Normal HTTP/1 requests contain
/mcp, not a URI scheme, soscheme_str().unwrap_or("http")rejects valid HTTPS origins. Do not synthesize a URL fromHost; parse Host ashttp::uri::Authority, and use only explicitly configured origins for Origin validation. -
[P2] Malformed Origins are normalized and accepted.
url::Urlrepairs values such ashttps:\app.example.comand empty userinfo. Strictly validate serialized-origin syntax, reject userinfo/path/query/fragment/backslashes and opaque origins, then compare typedurl::Originvalues. Parse configured origins once at startup and reject invalid configuration. -
[P2] The PR description is stale. It still references MCP
2025-11-25, describes validation as opt-in, says Host validation is excluded, and claims RMCP Origin validation is enabled. Update it to match the final2026-07-28behavior.
Please add regressions for matching attacker-controlled Host/Origin, HTTPS origin-form requests, and malformed-but-normalizable Origin values. After these changes, this should be ready for another review.
60d1f7d to
44b2d35
Compare
There was a problem hiding this comment.
Most of the prior feedback is now addressed: an empty allowlist rejects every present Origin, Host is no longer used to synthesize an Origin, HTTPS origin-form requests work, typed url::Origin equality handles default/non-default ports, and the PR description is current.
Three items remain, so I am requesting changes:
-
[P2] Strict serialized-origin validation is incomplete.
url::Urlstill repairs malformed values such ashttps:////app.example.com,https:///app.example.com,https://app.example.com:, and leading-whitespace values. Each becomes the same typed tuple ashttps://app.example.com, so a malformed present Origin can be accepted when the canonical origin is allowlisted. Enforce exactscheme://authoritysyntax beforeUrl::parseand add regressions. -
[P2] Invalid configured origins do not reject startup configuration.
parse_allowed_originsusesfilter_map, logs, and silently drops invalid entries. Return aResultfrom this parser/Config::finalizeand propagate the error fromrun_gateway. -
[P2]
request-flow.mdstill describes the removed same-origin fallback. The stack summary and middleware table must say that an empty allowlist rejects every presentOrigin.
Reproduction
Run against commit 44b2d35c1fa04791bcba531c07d81d42fb43bcc6:
cargo test -p contextforge-gateway-rs-lib mcp_origin -- --nocapture
cargo test -p contextforge-gateway-rs-lib invalid_configured_origin_is_skipped -- --nocapture|
@prakhar-singh1928 Thanks for the latest update. I retested head
After those changes, this should be ready for another review. |
dawid-nowak
left a comment
There was a problem hiding this comment.
I left some comments in-line.
I am not a big fan of long comments which are there to describe to what the code is doing. Most of the times, the comments are longer than the actual code.
So for example, instead of describing that the string contains host: port, why not just using Vec instead of Vec
Fixes #421. - Add mcp_origin_layer middleware that validates the Origin header on all incoming MCP Streamable HTTP requests per MCP spec 2025-11-25. - Missing Origin is accepted (native/non-browser clients). - Present Origin must match CONTEXTFORGE_GATEWAY_RS_MCP_ALLOWED_ORIGINS; any non-matching, malformed, or null Origin returns HTTP 403 before JWT auth, session creation, or backend fan-out. - Empty allowlist (default) disables validation for backward compatibility. - Align Tower CORS layer and RMCP StreamableHttpServerConfig with the same allowlist for defence in depth. - Add --mcp-allowed-origins CLI flag / CONTEXTFORGE_GATEWAY_RS_MCP_ALLOWED_ORIGINS env var to Config. - Update security-model.md with the new Origin validation section. - 27 unit/integration tests covering all acceptance criteria.
…t-flow docs - Reject https:///…, https:////…, trailing colon (https://host:), and leading/trailing whitespace in parse_origin before passing to url::Url; all four were silently normalized to a valid tuple by the url crate - Config::finalize() now returns Result<(), ConfigValidationError>; invalid configured origins abort startup with an error naming every bad entry - Add ConfigValidationError::InvalidMcpAllowedOrigins variant - run_gateway propagates finalize() error via ? - Remove parse_allowed_origins (replaced by finalize + parse_origin_str) - Add regressions: extra_slashes_after_scheme_returns_none, trailing_colon_without_port_returns_none, leading_whitespace_returns_none, finalize_with_invalid_origin_returns_error, finalize_reports_all_invalid_origins_in_error - request-flow.md: remove same-origin fallback from stack diagram and middleware table Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…ding Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
655c37c to
ed2a62a
Compare
…op value_parser Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…time Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Summary
Implements the MCP 2026-07-28 Streamable HTTP
Origin validation requirement
to prevent DNS-rebinding attacks on the gateway MCP endpoint.
Closes #421.
Changes
mcp_origin_layer— outermost middleware inlayers/mcp_origin.rs.Validates
Originbefore JWT auth, session creation, or any backend call.Returns HTTP 403 for non-allowlisted, malformed, or
nullOrigins.Missing
Originis always accepted (native MCP clients).Strict serialized-origin validation — pre-parse checks reject backslash,
userinfo (
@), query, and fragment before theurlcrate ever sees thevalue, preventing WHATWG normalization from silently repairing malformed input.
Post-parse checks confirm no userinfo/path/query/fragment survived.
Uses typed
url::Origin(notUrl) for equality comparison.Empty allowlist rejects every present Origin — there is no same-origin
fallback. A DNS-rebinding attacker can control both
HostandOriginsimultaneously; comparing them provides no protection.
Host check uses
http::uri::Authority— theHostheader is parsed asa bare authority (no scheme inference). Origin validation uses the allowlist
only; it never synthesizes a URL from
Host.Config.mcp_allowed_origins— allowlist wired to--mcp-allowed-origins/CONTEXTFORGE_GATEWAY_RS_MCP_ALLOWED_ORIGINS.Parsed once at startup via
Config::finalize(); pre-parsedVec<url::Origin>stored in
Config.mcp_parsed_originsfor zero-cost per-request lookup.Config.mcp_allowed_hosts— companion Host allowlist wired to--mcp-allowed-hosts/CONTEXTFORGE_GATEWAY_RS_MCP_ALLOWED_HOSTS.RMCP disabled —
StreamableHttpServerConfigusesdisable_allowed_origins()unconditionally;mcp_origin_layeris thesole enforcement point.
docs/book/src/security-model.md— updated MCP Origin and HostValidation section citing 2026-07-28; empty-allowlist behavior documented.
docs/book/src/request-flow.md—mcp_origin_layeradded to theHTTP middleware stack order.
Testing
78 unit and router integration tests including: