fix incorrect request uri for ack

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<Request> {
+    // 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<Request> {
         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::<Vec<_>>(),
+            );
+        } 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<UriWithParams> = 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::<Vec<_>>(),
+            );
         }
-        route_set.reverse();
-        headers.extend(route_set);
 
         headers.retain(|h| {
             matches!(

src/transaction/tests/test_client.rs

@@ -1,15 +1,32 @@
-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;
 use std::time::Duration;
 use tokio::{select, sync::mpsc::unbounded_channel, time::sleep};
 use tracing::info;
 
+fn make_invite_request(uri: &str) -> Result<rsip::Request> {
+    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("<sip:alice@example.com>;tag=from-tag").into(),
+            To::new("<sip:bob@example.com>").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: <sip:uas@192.0.2.55:5080;transport=tcp>\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![
-            "<sip:proxy2.example.com:5070;transport=tcp;lr>".to_string(),
-            "<sip:proxy1.example.com:5060;transport=tcp;lr>".to_string()
+            "<sip:proxy2.example.com:5070;transport=TCP;lr>".to_string(),
+            "<sip:proxy1.example.com:5060;transport=TCP;lr>".to_string()
         ],
         "ACK Route headers must follow the reversed Record-Route order"
     );
@@ -182,8 +199,8 @@ Contact: <sip:uas@192.0.2.55:5080;transport=udp>\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<String> = ack
         .headers
@@ -229,13 +246,55 @@ Contact: <sip:uas@192.0.2.55:5080;ob>\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: <sip:strict-proxy-1.example.com:5060>\r\n\
+Record-Route: <sip:strict-proxy-2.example.com:5070>\r\n\
+From: <sip:alice@example.com>;tag=from-tag\r\n\
+To: <sip:bob@example.com>;tag=to-tag\r\n\
+Call-ID: callid@example.com\r\n\
+CSeq: 1 INVITE\r\n\
+Contact: <sip:uas@192.0.2.55:5080;transport=tcp>\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<String> = ack
+        .headers
+        .iter()
+        .filter_map(|header| match header {
+            Header::Route(route) => Some(route.value().to_string()),
+            _ => None,
+        })
+        .collect();
+
+    assert_eq!(
+        routes,
+        vec![
+            "<sip:strict-proxy-1.example.com:5060>".to_string(),
+            "<sip:uas@192.0.2.55:5080;transport=TCP>".to_string(),
+        ],
+        "strict routing must place the remaining route set first and the remote target last"
+    );
+
+    Ok(())
+}

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;
@@ -465,12 +465,7 @@ impl Transaction {
         let ack = match self.last_ack.clone() {
             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)?
-                }
+                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(),
@@ -986,8 +981,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);
                                 }
                             }