From 379d8ee8b66c498e2d0ef438a8c7c2c27daa8d26 Mon Sep 17 00:00:00 2001 From: Michael Behr Date: Mon, 24 Aug 2026 19:40:57 +0000 Subject: [PATCH] mcp: Never reject requests during passthrough mode. Signed-off-by: Michael Behr --- .../extensions/filters/http/mcp/v3/mcp.proto | 8 +- .../mcp__no_passthrough_reject.rst | 1 + .../extensions/filters/common/mcp/constants.h | 1 + .../extensions/filters/http/mcp/mcp_filter.cc | 40 +++- .../extensions/filters/http/mcp/mcp_filter.h | 1 + .../http/mcp/mcp_filter_integration_test.cc | 42 +++- .../filters/http/mcp/mcp_filter_test.cc | 194 ++++++++++++++---- 7 files changed, 226 insertions(+), 61 deletions(-) create mode 100644 changelogs/current/minor_behavior_changes/mcp__no_passthrough_reject.rst diff --git a/api/envoy/extensions/filters/http/mcp/v3/mcp.proto b/api/envoy/extensions/filters/http/mcp/v3/mcp.proto index b1f383094dcab..1b8992993be56 100644 --- a/api/envoy/extensions/filters/http/mcp/v3/mcp.proto +++ b/api/envoy/extensions/filters/http/mcp/v3/mcp.proto @@ -116,9 +116,11 @@ message Mcp { // If unset (default), do not extract or inject baggage. BaggagePropagationConfig propagate_baggage = 7; - // When true, reject requests that contain duplicate JSON keys at any - // nesting level. RFC 8259 Section 4 states that names within an object SHOULD be - // unique. Defaults to false (last-key-wins / last-win). + // When true, disallow requests that contain duplicate JSON keys at any + // nesting level. RFC 8259 Section 4 states that names within an object SHOULD + // be unique. Defaults to false (last-key-wins / last-win). If traffic mode is + // REJECT_NO_MCP, reject these requests; if PASS_THROUGH, log an error status + // but don't reject the request. google.protobuf.BoolValue reject_duplicate_keys = 8; } diff --git a/changelogs/current/minor_behavior_changes/mcp__no_passthrough_reject.rst b/changelogs/current/minor_behavior_changes/mcp__no_passthrough_reject.rst new file mode 100644 index 0000000000000..74970e863fb82 --- /dev/null +++ b/changelogs/current/minor_behavior_changes/mcp__no_passthrough_reject.rst @@ -0,0 +1 @@ +``McpFilter`` no longer rejects any requests in ``PASS_THROUGH`` mode. Previously, the filter could reject requests in some cases when it failed to parse the request as an MCP request. Parse failures are now recorded in the filter's dynamic metadata, under ``passthrough_reason``. diff --git a/source/extensions/filters/common/mcp/constants.h b/source/extensions/filters/common/mcp/constants.h index 1287d022d3553..4f9e3e1744153 100644 --- a/source/extensions/filters/common/mcp/constants.h +++ b/source/extensions/filters/common/mcp/constants.h @@ -54,6 +54,7 @@ constexpr absl::string_view DEFAULT_SERVER_VERSION = "1.0.0"; constexpr absl::string_view IS_MCP_REQUEST = "is_mcp_request"; constexpr absl::string_view IS_EXCEEDING_LIMIT = "is_exceeding_limit"; constexpr absl::string_view STATUS = "status"; +constexpr absl::string_view PASSTHROUGH_REASON = "passthrough_reason"; namespace StatusValues { constexpr absl::string_view OK = "mcp_ok"; diff --git a/source/extensions/filters/http/mcp/mcp_filter.cc b/source/extensions/filters/http/mcp/mcp_filter.cc index 4c7d8c9e311d6..5547b7ae2b0e7 100644 --- a/source/extensions/filters/http/mcp/mcp_filter.cc +++ b/source/extensions/filters/http/mcp/mcp_filter.cc @@ -372,8 +372,13 @@ Http::FilterDataStatus McpFilter::decodeData(Buffer::Instance& data, bool end_st if (!status.ok()) { config_->stats().invalid_json_.inc(); - sendErrorReply("not a valid JSON", Filters::Common::Mcp::Status::NotJsonRpc); - return Http::FilterDataStatus::StopIterationNoBuffer; + if (shouldRejectRequest()) { + sendErrorReply("not a valid JSON", Filters::Common::Mcp::Status::NotJsonRpc); + return Http::FilterDataStatus::StopIterationNoBuffer; + } else { + passthrough_reason_ = Filters::Common::Mcp::Status::NotJsonRpc; + return completeParsing(); + } } if (parser_->isParsingComplete()) { @@ -398,9 +403,14 @@ Http::FilterDataStatus McpFilter::decodeData(Buffer::Instance& data, bool end_st } auto final_status = parser_->finishParse(); if (!final_status.ok()) { - if (truncated_by_limit && !shouldRejectRequest()) { - // PASS_THROUGH mode: size limit caused truncation, allow through. - ENVOY_LOG(debug, "size limit hit in PASS_THROUGH mode; proceeding with partial parse"); + if (!shouldRejectRequest()) { + if (truncated_by_limit) { + ENVOY_LOG(debug, "size limit hit in PASS_THROUGH mode; proceeding with partial parse"); + passthrough_reason_ = Filters::Common::Mcp::Status::BodyTooLarge; + } else { + ENVOY_LOG(debug, "parse error in PASS_THROUGH mode; proceeding"); + passthrough_reason_ = Filters::Common::Mcp::Status::ParseError; + } return completeParsing(); } Filters::Common::Mcp::Status status = Filters::Common::Mcp::Status::ParseError; @@ -450,11 +460,15 @@ Http::FilterDataStatus McpFilter::completeParsing() { ENVOY_LOG(debug, "parsing complete: is_mcp={}, bytes_parsed={}", is_mcp_request_, bytes_parsed_); - // Check for duplicate keys — reject if configured. + // Check for duplicate keys — reject if configured and we are in reject mode. if (parser_->hasDuplicateKeys() && rejectDuplicateKeys()) { - config_->stats().duplicate_keys_rejected_.inc(); - sendErrorReply("duplicate JSON keys detected", Filters::Common::Mcp::Status::DuplicateKeys); - return Http::FilterDataStatus::StopIterationNoBuffer; + if (shouldRejectRequest()) { + config_->stats().duplicate_keys_rejected_.inc(); + sendErrorReply("duplicate JSON keys detected", Filters::Common::Mcp::Status::DuplicateKeys); + return Http::FilterDataStatus::StopIterationNoBuffer; + } else if (!passthrough_reason_.has_value()) { + passthrough_reason_ = Filters::Common::Mcp::Status::DuplicateKeys; + } } if (!is_mcp_request_ && shouldRejectRequest()) { @@ -497,7 +511,9 @@ Http::FilterDataStatus McpFilter::completeParsing() { } const bool has_metadata = !metadata.fields().empty(); - const bool should_store_metadata = has_metadata || is_exceeding_limit_; + const bool should_store_metadata = has_metadata || is_exceeding_limit_ || + status_ != Filters::Common::Mcp::Status::Ok || + passthrough_reason_.has_value(); if (should_store_metadata) { if (shouldStoreToFilterState()) { @@ -532,6 +548,10 @@ void McpFilter::setDynamicMetadataStatus(Protobuf::Struct metadata) { (*metadata.mutable_fields())[Filters::Common::Mcp::McpConstants::IS_EXCEEDING_LIMIT] .set_bool_value(true); } + if (passthrough_reason_.has_value()) { + (*metadata.mutable_fields())[Filters::Common::Mcp::McpConstants::PASSTHROUGH_REASON] + .set_string_value(std::string(statusToString(passthrough_reason_.value()))); + } decoder_callbacks_->streamInfo().setDynamicMetadata(config_->metadataNamespace(), metadata); ENVOY_STREAM_LOG(debug, "MCP filter set dynamic metadata: {}", *decoder_callbacks_, metadata.DebugString()); diff --git a/source/extensions/filters/http/mcp/mcp_filter.h b/source/extensions/filters/http/mcp/mcp_filter.h index b78642ca57753..bf90779e9fb0e 100644 --- a/source/extensions/filters/http/mcp/mcp_filter.h +++ b/source/extensions/filters/http/mcp/mcp_filter.h @@ -188,6 +188,7 @@ class McpFilter : public Http::PassThroughFilter, public Logger::Loggable passthrough_reason_; }; } // namespace Mcp diff --git a/test/extensions/filters/http/mcp/mcp_filter_integration_test.cc b/test/extensions/filters/http/mcp/mcp_filter_integration_test.cc index 288b84b7b4c86..dcad8d0b68004 100644 --- a/test/extensions/filters/http/mcp/mcp_filter_integration_test.cc +++ b/test/extensions/filters/http/mcp/mcp_filter_integration_test.cc @@ -119,8 +119,8 @@ TEST_P(McpFilterIntegrationTest, ValidJsonRpcPostRequest) { EXPECT_TRUE(metadata_verified); } -// Test that an MCP request with malformed JSON is rejected with a 400. -TEST_P(McpFilterIntegrationTest, InvalidJsonBodyRejected) { +// Test that an MCP request with malformed JSON is passed through in PASS_THROUGH mode. +TEST_P(McpFilterIntegrationTest, InvalidJsonBodyPassedThrough) { FakeAccessLogFactory factory; Registry::InjectFactory factory_register(factory); @@ -135,7 +135,8 @@ TEST_P(McpFilterIntegrationTest, InvalidJsonBodyRejected) { if (it != dynamic_metadata.end()) { Protobuf::Struct expected_metadata; MessageUtil::loadFromJson(R"json({ - "status": "mcp_parse_error", + "status": "mcp_ok", + "passthrough_reason": "mcp_parse_error", "is_mcp_request": false })json", expected_metadata); @@ -167,10 +168,12 @@ TEST_P(McpFilterIntegrationTest, InvalidJsonBodyRejected) { {"content-type", "application/json"}}, R"({"jsonrpc": "2.0",)"); // Malformed JSON + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + ASSERT_TRUE(response->waitForEndStream()); - // The upstream should NOT receive a request because the filter sends a local reply. - EXPECT_FALSE(upstream_request_ != nullptr); - EXPECT_EQ("400", response->headers().getStatusValue()); + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); EXPECT_TRUE(metadata_verified); } @@ -240,6 +243,31 @@ TEST_P(McpFilterIntegrationTest, WrongContentTypePostRequestIgnored) { EXPECT_EQ("200", response->headers().getStatusValue()); } +// Test that invalid JSON is allowed in PASS_THROUGH mode and forwarded to upstream. +TEST_P(McpFilterIntegrationTest, PassThroughModeAllowsInvalidJson) { + initializeFilter(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + // Invalid JSON (unclosed brace) + const std::string request_body = R"({"jsonrpc": "2.0", "method": "test")"; + auto response = codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"accept", "application/json"}, + {"accept", "text/event-stream"}, + {"content-type", "application/json"}}, + request_body); + + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); +} + // Test no-MCP traffic is passed through without both accept headers TEST_P(McpFilterIntegrationTest, NoAcceptHeaderReject) { initializeFilter(R"EOF( @@ -482,6 +510,8 @@ TEST_P(McpFilterIntegrationTest, PerRouteRejectDuplicateKeysOverride) { envoy::extensions::filters::http::mcp::v3::McpOverride mcp_override; mcp_override.set_reject_duplicate_keys(true); + mcp_override.set_traffic_mode( + envoy::extensions::filters::http::mcp::v3::Mcp::REJECT_NO_MCP); std::ignore = (*route->mutable_typed_per_filter_config())["envoy.filters.http.mcp"].PackFrom( mcp_override); diff --git a/test/extensions/filters/http/mcp/mcp_filter_test.cc b/test/extensions/filters/http/mcp/mcp_filter_test.cc index b79c54a893cc1..819d351fb4231 100644 --- a/test/extensions/filters/http/mcp/mcp_filter_test.cc +++ b/test/extensions/filters/http/mcp/mcp_filter_test.cc @@ -260,7 +260,7 @@ TEST_F(McpFilterTest, NoopModePassesThroughNonMcp) { Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {"accept", "text/html"}}; - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); } @@ -299,7 +299,7 @@ TEST_F(McpFilterTest, NoopModePerRouteOverride) { Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {"accept", "text/html"}}; // Global REJECT_NO_MCP would reject this, but the NOOP override lets it through. - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); } @@ -356,7 +356,7 @@ TEST_F(McpFilterTest, DynamicMetadataContainsIsMcpRequest) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); } -// Test that malformed JSON is always rejected regardless of traffic mode +// Test that malformed JSON is allowed in PASS_THROUGH mode TEST_F(McpFilterTest, PartialNoJsonData) { Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, {"content-type", "application/json"}, @@ -367,6 +367,34 @@ TEST_F(McpFilterTest, PartialNoJsonData) { Buffer::OwnedImpl buffer("partial data"); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); + EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) + .WillOnce([](const std::string&, const Protobuf::Struct& metadata) { + EXPECT_THAT( + metadata.fields(), + AllOf(Contains(Pair("status", Property(&Protobuf::Value::string_value, "mcp_ok"))), + Contains(Pair("passthrough_reason", + Property(&Protobuf::Value::string_value, "mcp_not_jsonrpc"))), + Contains(Pair("is_mcp_request", Property(&Protobuf::Value::bool_value, false))))); + }); + + // Malformed JSON — allowed in PASS_THROUGH mode. + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); +} + +// Test that malformed JSON is rejected in REJECT_NO_MCP mode +TEST_F(McpFilterTest, PartialNoJsonDataRejectMode) { + setupRejectMode(); + + Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, + {"content-type", "application/json"}, + {"accept", "application/json"}, + {"accept", "text/event-stream"}}; + + filter_->decodeHeaders(headers, false); + + Buffer::OwnedImpl buffer("partial data"); + EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::BadRequest, "not a valid JSON", _, _, "mcp_not_jsonrpc")); EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) @@ -378,12 +406,11 @@ TEST_F(McpFilterTest, PartialNoJsonData) { Contains(Pair("is_mcp_request", Property(&Protobuf::Value::bool_value, false))))); }); - // Malformed JSON — always rejected even in PASS_THROUGH mode. EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_->decodeData(buffer, true)); } // Test that incomplete JSON (which is valid so far but incomplete at end_stream) -// triggers a parse error. +// is allowed in PASS_THROUGH mode. TEST_F(McpFilterTest, IncompleteJsonParseError) { Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, {"content-type", "application/json"}, @@ -395,6 +422,34 @@ TEST_F(McpFilterTest, IncompleteJsonParseError) { // Send incomplete JSON body Buffer::OwnedImpl buffer("{\"jsonrpc\": \"2.0\""); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); + EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) + .WillOnce([](const std::string&, const Protobuf::Struct& metadata) { + EXPECT_THAT( + metadata.fields(), + AllOf(Contains(Pair("status", Property(&Protobuf::Value::string_value, "mcp_ok"))), + Contains(Pair("passthrough_reason", + Property(&Protobuf::Value::string_value, "mcp_parse_error"))), + Contains(Pair("is_mcp_request", Property(&Protobuf::Value::bool_value, false))))); + }); + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); +} + +// Test that incomplete JSON triggers a parse error and rejection in REJECT_NO_MCP mode. +TEST_F(McpFilterTest, IncompleteJsonParseErrorRejectMode) { + setupRejectMode(); + + Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, + {"content-type", "application/json"}, + {"accept", "application/json"}, + {"accept", "text/event-stream"}}; + + filter_->decodeHeaders(headers, false); + + // Send incomplete JSON body + Buffer::OwnedImpl buffer("{\"jsonrpc\": \"2.0\""); + EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::BadRequest, "reached end_stream or configured body size, don't get enough data.", @@ -628,8 +683,9 @@ TEST_F(McpFilterTest, RejectModeNonJsonRpcPopulatesFilterState) { EXPECT_EQ(filter_state_obj->status(), Filters::Common::Mcp::Status::NotJsonRpc); } -// Test that truncated JSON with end_stream is rejected even in PASS_THROUGH mode. -// Truncation here is caused by the client (not by size limit), so data is bad. +// Test that truncated JSON with end_stream is allowed in PASS_THROUGH mode. +// Truncation here is caused by the client (not by size limit), so data is bad, but we pass it +// through. TEST_F(McpFilterTest, PartialJsonEndStreamPassThroughMode) { envoy::extensions::filters::http::mcp::v3::Mcp proto_config; proto_config.set_traffic_mode(envoy::extensions::filters::http::mcp::v3::Mcp::PASS_THROUGH); @@ -648,18 +704,14 @@ TEST_F(McpFilterTest, PartialJsonEndStreamPassThroughMode) { std::string json = R"({"jsonrpc": "2.0", "method": "tools/call", "params": {"name": "test")"; Buffer::OwnedImpl buffer(json); - // Client-sent truncated data — rejected even in PASS_THROUGH mode. - EXPECT_CALL(decoder_callbacks_, - sendLocalReply(Http::Code::BadRequest, - "reached end_stream or configured body size, don't get enough data.", - _, _, _)); + // Client-sent truncated data — allowed in PASS_THROUGH mode. + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_->decodeData(buffer, true)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); } // Test that malformed JSON with length exactly equal to max_request_body_size and end_stream=true -// is rejected in PASS_THROUGH mode, because the client terminated the stream and no further data -// exists. +// is allowed in PASS_THROUGH mode. TEST_F(McpFilterTest, PartialJsonEndStreamPassThroughModeExactlyAtLimit) { envoy::extensions::filters::http::mcp::v3::Mcp proto_config; proto_config.set_traffic_mode(envoy::extensions::filters::http::mcp::v3::Mcp::PASS_THROUGH); @@ -682,15 +734,10 @@ TEST_F(McpFilterTest, PartialJsonEndStreamPassThroughModeExactlyAtLimit) { Buffer::OwnedImpl buffer(json); - // Even in PASS_THROUGH mode, it must be rejected because the total body length matches - // the limit, end_stream is true, and the JSON is genuinely malformed (not truncated by the - // limit). - EXPECT_CALL(decoder_callbacks_, - sendLocalReply(Http::Code::BadRequest, - "reached end_stream or configured body size, don't get enough data.", - _, _, _)); + // Even in PASS_THROUGH mode, it must be allowed. + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); - EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_->decodeData(buffer, true)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); } // Test that if a request in PASS_THROUGH mode exceeds the size limit before extracting any MCP @@ -722,7 +769,7 @@ TEST_F(McpFilterTest, BodyLimitPassThroughWithoutMetadata) { Buffer::OwnedImpl buffer(json); // In PASS_THROUGH mode, the request is allowed through. - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); Protobuf::Struct captured_metadata; EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) @@ -731,13 +778,15 @@ TEST_F(McpFilterTest, BodyLimitPassThroughWithoutMetadata) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); // Verify that dynamic metadata still contains is_exceeding_limit, is_mcp_request, and status - EXPECT_THAT(captured_metadata.fields(), - AllOf(Contains(Pair(std::string(IS_EXCEEDING_LIMIT), - Property(&Protobuf::Value::bool_value, true))), - Contains(Pair(std::string(IS_MCP_REQUEST), - Property(&Protobuf::Value::bool_value, false))), - Contains(Pair(std::string(STATUS), - Property(&Protobuf::Value::string_value, "mcp_ok"))))); + EXPECT_THAT( + captured_metadata.fields(), + AllOf(Contains(Pair(std::string(IS_EXCEEDING_LIMIT), + Property(&Protobuf::Value::bool_value, true))), + Contains( + Pair(std::string(IS_MCP_REQUEST), Property(&Protobuf::Value::bool_value, false))), + Contains(Pair(std::string(STATUS), Property(&Protobuf::Value::string_value, "mcp_ok"))), + Contains(Pair("passthrough_reason", + Property(&Protobuf::Value::string_value, "mcp_body_too_large"))))); // Verify that FilterStateObject still exists and is populated with the exceeding limit state and // status @@ -751,7 +800,7 @@ TEST_F(McpFilterTest, BodyLimitPassThroughWithoutMetadata) { } // Test that if reject_duplicate_keys is explicitly set to true in the config, -// the filter rejects requests containing duplicate JSON keys. +// the filter still allows requests containing duplicate JSON keys in PASS_THROUGH mode. TEST_F(McpFilterTest, DuplicateKeyRejectionEnabledConfig) { envoy::extensions::filters::http::mcp::v3::Mcp proto_config; proto_config.mutable_reject_duplicate_keys()->set_value(true); @@ -771,6 +820,43 @@ TEST_F(McpFilterTest, DuplicateKeyRejectionEnabledConfig) { R"({"jsonrpc": "2.0", "method": "tools/call", "id": 1, "params": {"name": "tool1"}, "params": {"name": "tool2"}})"; Buffer::OwnedImpl buffer(json); + // Expect request to be allowed in PASS_THROUGH mode + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); + EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) + .WillOnce([](const std::string&, const Protobuf::Struct& metadata) { + EXPECT_THAT( + metadata.fields(), + AllOf(Contains(Pair("status", Property(&Protobuf::Value::string_value, "mcp_ok"))), + Contains(Pair("passthrough_reason", + Property(&Protobuf::Value::string_value, "mcp_duplicate_keys"))), + Contains(Pair("is_mcp_request", Property(&Protobuf::Value::bool_value, true))))); + }); + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); + EXPECT_EQ(0u, config_->stats().duplicate_keys_rejected_.value()); +} + +// Test that duplicate keys are rejected in REJECT_NO_MCP mode if reject_duplicate_keys is true. +TEST_F(McpFilterTest, DuplicateKeyRejectionEnabledConfigRejectMode) { + envoy::extensions::filters::http::mcp::v3::Mcp proto_config; + proto_config.set_traffic_mode(envoy::extensions::filters::http::mcp::v3::Mcp::REJECT_NO_MCP); + proto_config.mutable_reject_duplicate_keys()->set_value(true); + config_ = std::make_shared(proto_config, "test.", factory_context_.scope()); + filter_ = std::make_unique(config_); + filter_->setDecoderFilterCallbacks(decoder_callbacks_); + + Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, + {"content-type", "application/json"}, + {"accept", "application/json"}, + {"accept", "text/event-stream"}}; + + filter_->decodeHeaders(headers, false); + + // Send JSON body containing duplicate keys + std::string json = + R"({"jsonrpc": "2.0", "method": "tools/call", "id": 1, "params": {"name": "tool1"}, "params": {"name": "tool2"}})"; + Buffer::OwnedImpl buffer(json); + // Expect request to be rejected with BadRequest due to duplicate keys EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::BadRequest, "duplicate JSON keys detected", _, _, @@ -809,7 +895,7 @@ TEST_F(McpFilterTest, DuplicateKeyAllowedByDefault) { Buffer::OwnedImpl buffer(json); // Expect request to be allowed through (no rejection calls) - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); @@ -817,7 +903,7 @@ TEST_F(McpFilterTest, DuplicateKeyAllowedByDefault) { } // Test that a complete MCP JSON object followed by trailing garbage in the same data chunk -// is correctly rejected with BadRequest by the filter. +// is allowed in PASS_THROUGH mode. TEST_F(McpFilterTest, TrailingGarbageRejectedInFilter) { Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, {"content-type", "application/json"}, @@ -830,6 +916,29 @@ TEST_F(McpFilterTest, TrailingGarbageRejectedInFilter) { R"({"jsonrpc": "2.0", "method": "tools/call", "params": {"name": "calculator"}, "id": 1} trailing garbage)"; Buffer::OwnedImpl buffer(json); + // In PASS_THROUGH mode, it must be allowed. + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, true)); + EXPECT_EQ(1u, config_->stats().invalid_json_.value()); +} + +// Test that a complete MCP JSON object followed by trailing garbage in the same data chunk +// is correctly rejected with BadRequest in REJECT_NO_MCP mode. +TEST_F(McpFilterTest, TrailingGarbageRejectedInFilterRejectMode) { + setupRejectMode(); + + Http::TestRequestHeaderMapImpl headers{{":method", "POST"}, + {"content-type", "application/json"}, + {"accept", "application/json"}, + {"accept", "text/event-stream"}}; + + filter_->decodeHeaders(headers, false); + + std::string json = + R"({"jsonrpc": "2.0", "method": "tools/call", "params": {"name": "calculator"}, "id": 1} trailing garbage)"; + Buffer::OwnedImpl buffer(json); + // The filter must reject the request because of trailing garbage in the chunk EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::BadRequest, "not a valid JSON", _, _, _)); @@ -896,7 +1005,7 @@ TEST_F(McpFilterTest, PartialParsingSucceedsWithOptionalFieldConfig) { std::string json = prefix + padding + R"("}})"; Buffer::OwnedImpl buffer(json); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) .WillOnce([](const std::string&, const Protobuf::Struct& metadata) { EXPECT_THAT( @@ -947,7 +1056,7 @@ TEST_F(McpFilterTest, OptionalMetaFieldExtractedWithPartialParsing) { std::string json = json_with_meta + padding + R"("}})"; Buffer::OwnedImpl buffer(json); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) .WillOnce([](const std::string&, const Protobuf::Struct& metadata) { EXPECT_THAT( @@ -1008,7 +1117,7 @@ TEST_F(McpFilterTest, ChunkByChunkParsingWithOptionalFields) { std::string chunk2 = R"("_meta": {"trace_id": "abc123"}}})"; Buffer::OwnedImpl buffer2(chunk2); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) .WillOnce([](const std::string&, const Protobuf::Struct& metadata) { EXPECT_THAT( @@ -1156,7 +1265,7 @@ TEST_F(McpFilterTest, BodySizeLimitInPassThroughMode) { Buffer::OwnedImpl buffer(json); // In PASS_THROUGH mode, request is allowed through (no rejection). - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); Protobuf::Struct captured_metadata; EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) .WillOnce(testing::SaveArg<1>(&captured_metadata)); @@ -1211,7 +1320,7 @@ TEST_F(McpFilterTest, BodySizeLimitInPassThroughModeMultiChunk) { ASSERT_EQ(chunk2.size(), 20); Buffer::OwnedImpl buffer2(chunk2); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); Protobuf::Struct captured_metadata; EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata("envoy.filters.http.mcp", _)) .WillOnce(testing::SaveArg<1>(&captured_metadata)); @@ -1410,7 +1519,7 @@ TEST_F(McpFilterTest, NonMcpJsonCompletesInPassThroughMode) { std::string json = R"({"foo": "bar", "nested": {"deep": 123}, "baz": true})"; Buffer::OwnedImpl buffer(json); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, false)); } @@ -1427,7 +1536,7 @@ TEST_F(McpFilterTest, NonMcpJsonMultiChunkCompletion) { EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(buffer1, false)); Buffer::OwnedImpl buffer2(R"("baz": 123})"); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(0); + EXPECT_CALL(decoder_callbacks_, sendLocalReply).Times(0); EXPECT_CALL(decoder_callbacks_.stream_info_, setDynamicMetadata(_, _)).Times(0); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer2, false)); } @@ -1578,9 +1687,10 @@ TEST_F(McpFilterTest, PerRouteRejectDuplicateKeys) { filter_ = std::make_unique(config_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); - // Per-route config overrides reject duplicate keys = true + // Per-route config overrides reject duplicate keys = true and traffic mode = REJECT_NO_MCP envoy::extensions::filters::http::mcp::v3::McpOverride override_config; override_config.set_reject_duplicate_keys(true); + override_config.set_traffic_mode(envoy::extensions::filters::http::mcp::v3::Mcp::REJECT_NO_MCP); auto route_config = std::make_shared(override_config); EXPECT_CALL(decoder_callbacks_, mostSpecificPerFilterConfig())