From 85ebca532e4f934b9ba7a7fed3095260d263e8b1 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Tue, 7 May 2024 15:10:30 -0700 Subject: [PATCH] Fix not rejecting a frame with an empty endpoint ID. PiperOrigin-RevId: 631563161 --- .../implementation/base_pcp_handler.cc | 3 +- .../implementation/base_pcp_handler_test.cc | 50 +++++++++++++++++++ .../offline_frames_validator.cc | 4 +- .../offline_frames_validator_test.cc | 19 ++++++- 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/connections/implementation/base_pcp_handler.cc b/connections/implementation/base_pcp_handler.cc index 29a3cdca..711f208f 100644 --- a/connections/implementation/base_pcp_handler.cc +++ b/connections/implementation/base_pcp_handler.cc @@ -1756,8 +1756,7 @@ Exception BasePcpHandler::OnIncomingConnection( << "with error: " << wrapped_frame.exception(); ProcessPreConnectionInitiationFailure( client, medium, "", channel.get(), - /* is_incoming= */ false, start_time, {Status::kError}, nullptr); - return {Exception::kSuccess}; + /* is_incoming= */ true, start_time, {Status::kError}, nullptr); } return wrapped_frame.GetException(); } diff --git a/connections/implementation/base_pcp_handler_test.cc b/connections/implementation/base_pcp_handler_test.cc index 8067829a..314f8ad5 100644 --- a/connections/implementation/base_pcp_handler_test.cc +++ b/connections/implementation/base_pcp_handler_test.cc @@ -2400,6 +2400,56 @@ TEST_F(BasePcpHandlerTest, TestDeviceFilterForConnectionsWithPresence) { env_.Stop(); } +TEST_F(BasePcpHandlerTest, IncomingConnectionFailsWithEmptyEndpointId) { + env_.Start(); + ClientProxy client; + Mediums m; + EndpointChannelManager ecm; + EndpointManager em(&ecm); + BwuManager bwu(m, em, ecm, {}, {}); + MockPcpHandler pcp_handler(&m, &em, &ecm, &bwu); + v3::ConnectionListeningOptions options = { + .strategy = Strategy::kP2pCluster, + .enable_ble_listening = true, + .enable_bluetooth_listening = true, + .enable_wlan_listening = true, + .listening_endpoint_type = NearbyDevice::Type::kConnectionsDevice}; + EXPECT_CALL(pcp_handler, StartListeningForIncomingConnectionsImpl) + .WillOnce(Return( + MockPcpHandler::StartOperationResult{.status = {Status::kSuccess}})); + EXPECT_CALL(pcp_handler, CanReceiveIncomingConnection) + .WillRepeatedly(Return(true)); + EXPECT_TRUE( + pcp_handler + .StartListeningForIncomingConnections(&client, "service", options, {}) + .first.Ok()); + ASSERT_TRUE(client.IsListeningForIncomingConnections()); + ASSERT_TRUE(pcp_handler.CanReceiveIncomingConnection(&client)); + auto channel_pair = SetupConnection(Medium::BLUETOOTH); + ByteArray serialized_frame = parser::ForConnectionRequestConnections( + {}, { + .local_endpoint_id = "", + .local_endpoint_info = ByteArray("local endpoint"), + }); + // At this point the connection request doesn't have an endpoint ID field set, + // so we do that here. + location::nearby::connections::OfflineFrame frame; + frame.ParseFromString(serialized_frame.AsStringView()); + frame.mutable_v1()->mutable_connection_request()->set_endpoint_id(""); + ASSERT_TRUE(frame.v1().connection_request().has_endpoint_id()); + // do a dummy write to get to the actual write. + channel_pair.first->Write(ByteArray()); + channel_pair.first->Write(ByteArray(frame.SerializeAsString())); + EXPECT_EQ(pcp_handler + .OnIncomingConnection(&client, ByteArray("remote endpoint"), + std::move(channel_pair.second), + Medium::BLUETOOTH, + NearbyDevice::Type::kConnectionsDevice) + .value, + Exception::Value::kIo); + env_.Stop(); +} + TEST_F(BasePcpHandlerTest, TestNeedsToTurnOffAdvertisingMedium) { Mediums m; EndpointChannelManager ecm; diff --git a/connections/implementation/offline_frames_validator.cc b/connections/implementation/offline_frames_validator.cc index 9be7d18d..a71b4c51 100644 --- a/connections/implementation/offline_frames_validator.cc +++ b/connections/implementation/offline_frames_validator.cc @@ -73,8 +73,8 @@ inline bool WithinRange(int value, int min, int max) { Exception EnsureValidConnectionRequestFrame( const ConnectionRequestFrame& frame) { - if (!frame.has_endpoint_id()) return {Exception::kInvalidProtocolBuffer}; - if (!frame.has_endpoint_name()) return {Exception::kInvalidProtocolBuffer}; + if (frame.endpoint_id().empty()) return {Exception::kInvalidProtocolBuffer}; + if (frame.endpoint_name().empty()) return {Exception::kInvalidProtocolBuffer}; // For backwards compatibility reasons, no other fields should be // null-checked for this frame. Parameter checking (eg. must be within this diff --git a/connections/implementation/offline_frames_validator_test.cc b/connections/implementation/offline_frames_validator_test.cc index b76f30ab..02e89ca3 100644 --- a/connections/implementation/offline_frames_validator_test.cc +++ b/connections/implementation/offline_frames_validator_test.cc @@ -110,7 +110,24 @@ TEST_F(OfflineFramesConnectionRequestTest, auto ret_value = EnsureValidOfflineFrame(offline_frame); - ASSERT_FALSE(ret_value.Ok()); + EXPECT_FALSE(ret_value.Ok()); +} + +TEST_F(OfflineFramesConnectionRequestTest, + ValidatesAsFailWithEmptyEndpointIdInConnectionRequestFrame) { + connection_info_.local_endpoint_id = ""; + ByteArray bytes = ForConnectionRequestConnections({}, connection_info_); + location::nearby::connections::OfflineFrame frame; + frame.ParseFromString(bytes.AsStringView()); + frame.mutable_v1()->mutable_connection_request()->set_endpoint_id(""); + ASSERT_TRUE(frame.v1().connection_request().has_endpoint_id()); + + OfflineFrame offline_frame; + offline_frame.ParseFromString(frame.SerializeAsString()); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + EXPECT_FALSE(ret_value.Ok()); } TEST_F(OfflineFramesConnectionRequestTest,