From c4d15ec1367731b110d6f7a414d6e8ba3b6d08b7 Mon Sep 17 00:00:00 2001 From: yeoleobun Date: Fri, 6 Mar 2026 18:51:39 +0800 Subject: [PATCH 1/2] fix incorrect reqeust uri --- src/transaction/message.rs | 100 ++++++++++++++++++++++----- src/transaction/tests/test_client.rs | 89 ++++++++++++++++++++---- src/transaction/transaction.rs | 11 ++- 3 files changed, 162 insertions(+), 38 deletions(-) diff --git a/src/transaction/message.rs b/src/transaction/message.rs index bc97606b..6562e465 100644 --- a/src/transaction/message.rs +++ b/src/transaction/message.rs @@ -1,9 +1,11 @@ use super::{endpoint::EndpointInner, make_call_id}; -use crate::{transaction::make_via_branch, Result}; +use crate::{rsip_ext::extract_uri_from_contact, transaction::make_via_branch, Result}; use rsip::{ + common::uri::{UriWithParams, UriWithParamsList}, header, - headers::{ContentLength, Route}, - prelude::{ToTypedHeader, UntypedHeader}, + headers::ContentLength, + prelude::{HeadersExt, ToTypedHeader, UntypedHeader as _}, + typed::Route as TypedRoute, Error, Header, Request, Response, StatusCode, }; @@ -244,10 +246,23 @@ impl EndpointInner { } } - pub fn make_ack(&self, resp: &Response, request_uri: rsip::Uri) -> Result { + // make ack from response, for ack to non-200 reponse, should pass the original invite + pub fn make_ack(&self, invite: &Request, resp: &Response) -> Result { let mut headers = resp.headers.clone(); - if matches!(resp.status_code.kind(), rsip::StatusCodeKind::Successful) { - //For non-2xx final responses (3xx–6xx), the ACK stays within the original INVITE client transaction. + let request_uri; + if resp.status_code.kind() != rsip::StatusCodeKind::Successful { + // Non-2xx ACK stays in the original INVITE transaction. + request_uri = invite.uri.clone(); + headers.extend( + invite + .headers + .iter() + .filter(|header| matches!(header, Header::Route(_))) + .cloned() + .collect::>(), + ); + } else { + // 2xx ACK is a separate request built from the dialog remote target and route set. if let Ok(top_most_via) = header!( headers.iter_mut(), Header::Via, @@ -259,19 +274,72 @@ impl EndpointInner { *top_most_via = typed_via.into(); } } - } - // update route set from Record-Route header (support comma-separated lists) - let mut route_set = Vec::new(); - for header in resp.headers.iter() { - if let Header::RecordRoute(record_route) = header { - let value = record_route.value(); - for token in value.split(',').map(|t| t.trim()).filter(|t| !t.is_empty()) { - route_set.push(Header::Route(Route::from(token))); + let mut route_set: Vec = Vec::new(); + for header in resp.headers.iter() { + if let Header::RecordRoute(record_route) = header { + let typed = record_route.typed()?; + for uri in typed.uris() { + route_set.push(uri.clone()); + } } } + route_set.reverse(); + + let contact = resp.contact_header()?; + + // work around for rsip parsing bug + let remote_target_uri = if let Ok(typed_contact) = contact.typed() { + typed_contact.uri + } else { + let mut uri = extract_uri_from_contact(contact.value())?; + uri.headers.clear(); + uri + }; + + let route_headers = match route_set.as_slice() { + [] => { + request_uri = remote_target_uri; + Vec::new() + } + [head, rest @ ..] => { + // loose rooting + if head + .uri + .params + .iter() + .any(|param| matches!(param, rsip::Param::Lr)) + { + request_uri = remote_target_uri; + route_set + } else { + // Strict routing promotes the first route URI into the Request-URI + // and appends the remote target as the last Route value. + let mut request_uri_value = head.uri.clone(); + request_uri_value.headers.clear(); + request_uri = request_uri_value; + + let mut strict_routes = rest.to_vec(); + strict_routes.push(UriWithParams { + uri: remote_target_uri.clone(), + params: vec![], + }); + + strict_routes + } + } + }; + + headers.extend( + route_headers + .iter() + .cloned() + .map(|route| { + let typed_route = TypedRoute::from(UriWithParamsList::from(vec![route])); + Header::Route(typed_route.into()) + }) + .collect::>(), + ); } - route_set.reverse(); - headers.extend(route_set); headers.retain(|h| { matches!( diff --git a/src/transaction/tests/test_client.rs b/src/transaction/tests/test_client.rs index d8092acb..0f7e6059 100644 --- a/src/transaction/tests/test_client.rs +++ b/src/transaction/tests/test_client.rs @@ -1,8 +1,7 @@ -use crate::rsip_ext::{destination_from_request, RsipResponseExt}; +use crate::rsip_ext::destination_from_request; use crate::transaction::key::{TransactionKey, TransactionRole}; use crate::transaction::transaction::Transaction; use crate::transport::udp::UdpConnection; -use crate::transport::SipAddr; use crate::{transport::TransportEvent, Result}; use rsip::{headers::*, Header, Response, SipMessage, Uri}; use std::convert::TryFrom; @@ -10,6 +9,24 @@ use std::time::Duration; use tokio::{select, sync::mpsc::unbounded_channel, time::sleep}; use tracing::info; +fn make_invite_request(uri: &str) -> Result { + Ok(rsip::Request { + method: rsip::Method::Invite, + uri: Uri::try_from(uri)?, + headers: vec![ + Via::new("SIP/2.0/TCP uac.example.com:5060;branch=z9hG4bK1").into(), + CSeq::new("1 INVITE").into(), + From::new(";tag=from-tag").into(), + To::new("").into(), + CallId::new("callid@example.com").into(), + MaxForwards::new("70").into(), + ] + .into(), + version: rsip::Version::V2, + body: vec![], + }) +} + #[tokio::test] async fn test_client_transaction() -> Result<()> { let endpoint = super::create_test_endpoint(Some("127.0.0.1:0")).await?; @@ -128,8 +145,8 @@ Contact: \r\n\ Content-Length: 0\r\n\r\n"; let response = Response::try_from(raw_response)?; - let request_uri = response.remote_uri(None)?; - let ack = endpoint.inner.make_ack(&response, request_uri)?; + let invite = make_invite_request("sip:bob@example.com")?; + let ack = endpoint.inner.make_ack(&invite, &response)?; let expected_uri = Uri::try_from("sip:uas@192.0.2.55:5080;transport=tcp")?; assert_eq!(ack.uri, expected_uri, "ACK must target the remote Contact"); @@ -158,8 +175,8 @@ Content-Length: 0\r\n\r\n"; assert_eq!( routes, vec![ - "".to_string(), - "".to_string() + "".to_string(), + "".to_string() ], "ACK Route headers must follow the reversed Record-Route order" ); @@ -182,8 +199,8 @@ Contact: \r\n\ Content-Length: 0\r\n\r\n"; let response = Response::try_from(raw_response)?; - let request_uri = response.remote_uri(None)?; - let ack = endpoint.inner.make_ack(&response, request_uri)?; + let invite = make_invite_request("sip:bob@example.com")?; + let ack = endpoint.inner.make_ack(&invite, &response)?; let routes: Vec = ack .headers @@ -229,13 +246,55 @@ Contact: \r\n\ Content-Length: 0\r\n\r\n"; let response = Response::try_from(raw_response)?; - let dest = SipAddr { - r#type: Some(rsip::transport::Transport::Tcp), - addr: "1.2.3.4:15060".try_into()?, - }; - let request_uri = dest.try_into().expect("to uri failed"); - let ack = endpoint.inner.make_ack(&response, request_uri)?; - let expected_uri = Uri::try_from("sip:1.2.3.4:15060;transport=tcp")?; + let invite = make_invite_request("sip:bob@example.com")?; + let ack = endpoint.inner.make_ack(&invite, &response)?; + let expected_uri = Uri::try_from("sip:uas@192.0.2.55:5080;ob")?; assert_eq!(ack.uri, expected_uri, "ACK must target the remote Contact"); Ok(()) } + +#[tokio::test] +async fn test_make_ack_uses_first_route_without_lr() -> Result<()> { + let endpoint = super::create_test_endpoint(None).await?; + + let raw_response = "SIP/2.0 200 OK\r\n\ +Via: SIP/2.0/TCP uac.example.com:5060;branch=z9hG4bK1\r\n\ +Record-Route: \r\n\ +Record-Route: \r\n\ +From: ;tag=from-tag\r\n\ +To: ;tag=to-tag\r\n\ +Call-ID: callid@example.com\r\n\ +CSeq: 1 INVITE\r\n\ +Contact: \r\n\ +Content-Length: 0\r\n\r\n"; + + let response = Response::try_from(raw_response)?; + let invite = make_invite_request("sip:bob@example.com")?; + let ack = endpoint.inner.make_ack(&invite, &response)?; + + assert_eq!( + ack.uri, + Uri::try_from("sip:strict-proxy-2.example.com:5070")?, + "strict routing must place the first route set URI into the Request-URI" + ); + + let routes: Vec = ack + .headers + .iter() + .filter_map(|header| match header { + Header::Route(route) => Some(route.value().to_string()), + _ => None, + }) + .collect(); + + assert_eq!( + routes, + vec![ + "".to_string(), + "".to_string(), + ], + "strict routing must place the remaining route set first and the remote target last" + ); + + Ok(()) +} diff --git a/src/transaction/transaction.rs b/src/transaction/transaction.rs index 81cc7051..dc897877 100644 --- a/src/transaction/transaction.rs +++ b/src/transaction/transaction.rs @@ -2,7 +2,7 @@ use super::endpoint::EndpointInnerRef; use super::key::TransactionKey; use super::{SipConnection, TransactionState, TransactionTimer, TransactionType}; use crate::dialog::DialogId; -use crate::rsip_ext::{destination_from_request, RsipResponseExt}; +use crate::rsip_ext::destination_from_request; use crate::transaction::key::TransactionRole; use crate::transaction::make_tag; use crate::transport::SipAddr; @@ -466,10 +466,7 @@ impl Transaction { Some(ack) => ack, None => match self.last_response { Some(ref resp) => { - let request_uri = resp - .remote_uri(self.destination.as_ref()) - .unwrap_or_else(|_| self.original.uri.clone()); - self.endpoint_inner.make_ack(resp, request_uri)? + self.endpoint_inner.make_ack(&self.original, resp)? } None => { return Err(Error::TransactionError( @@ -986,8 +983,8 @@ impl Transaction { ) { if self.last_ack.is_none() { if let Some(ref resp) = self.last_response { - let request_uri = self.original.uri.clone(); - if let Ok(ack) = self.endpoint_inner.make_ack(resp, request_uri) { + if let Ok(ack) = self.endpoint_inner.make_ack(&self.original, resp) + { self.last_ack.replace(ack); } } From 11681870a68dbb0b41b877af742f25362e3f0412 Mon Sep 17 00:00:00 2001 From: yeoleobun Date: Fri, 6 Mar 2026 20:37:41 +0800 Subject: [PATCH 2/2] fmt --- src/transaction/transaction.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/transaction/transaction.rs b/src/transaction/transaction.rs index dc897877..6d0ff1e9 100644 --- a/src/transaction/transaction.rs +++ b/src/transaction/transaction.rs @@ -465,9 +465,7 @@ impl Transaction { let ack = match self.last_ack.clone() { Some(ack) => ack, None => match self.last_response { - Some(ref resp) => { - self.endpoint_inner.make_ack(&self.original, resp)? - } + Some(ref resp) => self.endpoint_inner.make_ack(&self.original, resp)?, None => { return Err(Error::TransactionError( "no last response found to send ACK".to_string(),