From 9f0eebdaa61d6ff06491cb871a14fd3bd73ea88e Mon Sep 17 00:00:00 2001 From: eigger Date: Tue, 22 Sep 2026 07:29:37 +0900 Subject: [PATCH 1/3] fix(sip_client): keep the proxy path in responses and ACKs (#323) Calls through a proxy/SBC (3CX) could not complete: the 200 OK to an inbound INVITE echoed only the top Via and dropped Record-Route, so the proxy's ACK never reached us and the call stayed in ANSWERING; and the ACK for a 407 used a fresh Via branch, so the PBX kept retransmitting the 407 while the authenticated INVITE was already in flight. - parse_sip_message: Via / Record-Route / Route / Service-Route rows are joined in wire order instead of keeping the first; header folding. - build_response_: echo every Via; Record-Route + Contact on every dialog-establishing INVITE response (18x and 2xx), RFC 3261 s12.1.1. - build_ack_: non-2xx ACK reuses the response's top Via branch and the INVITE Request-URI (s17.1.1.3); 2xx ACK goes through the route set. - Learn the dialog route set from Record-Route (UAS as-is, UAC reversed) and add Route headers to in-dialog requests (BYE, 2xx ACK) with loose/strict router handling, s12.2.1.1. CANCEL keeps the INVITE URI. - Native tests for the header parsing and routing helpers. Ports the equivalent hass-sip fixes (#22, #38, #40). Co-Authored-By: Claude Opus 5 --- components/sip_client/sip_client.cpp | 108 +++++++++--- components/sip_client/sip_client.h | 5 + components/sip_client/sip_message.cpp | 94 +++++++++- components/sip_client/sip_message.h | 18 +- tests/native/sip_sdp/README.md | 15 ++ tests/native/sip_sdp/run.sh | 6 + tests/native/sip_sdp/test_sip_message.cpp | 198 ++++++++++++++++++++++ 7 files changed, 416 insertions(+), 28 deletions(-) create mode 100644 tests/native/sip_sdp/test_sip_message.cpp diff --git a/components/sip_client/sip_client.cpp b/components/sip_client/sip_client.cpp index f742eb99..7f2c123f 100644 --- a/components/sip_client/sip_client.cpp +++ b/components/sip_client/sip_client.cpp @@ -1,4 +1,5 @@ #include "sip_client.h" +#include #include #include #include @@ -26,19 +27,6 @@ static std::string trim(const std::string &s) { return s.substr(b, e - b + 1); } -static std::string extract_angle_uri(const std::string &value) { - size_t lt = value.find('<'); - size_t gt = value.find('>'); - if (lt != std::string::npos && gt != std::string::npos && gt > lt) { - return value.substr(lt + 1, gt - lt - 1); - } - std::string stripped = trim(value); - if (stripped.rfind("sip", 0) == 0) { - return stripped; - } - return ""; -} - // Render an IPv4 sockaddr to dotted-quad without depending on inet_ntop. static std::string sockaddr_ip(const struct sockaddr_storage &ss, uint16_t *port) { if (ss.ss_family != AF_INET) return ""; @@ -307,6 +295,8 @@ void SipClient::call(const std::string &number) { ">;tag=" + this->d_local_tag_; this->d_remote_ = "domain_ + ">"; this->d_remote_target_ = "sip:" + number + "@" + this->domain_; + this->d_invite_uri_ = this->d_remote_target_; + this->dialog_routes_.clear(); this->send_raw_(this->build_invite_()); this->set_state_(SIP_INVITING); ESP_LOGI(TAG, "Calling %s", number.c_str()); @@ -351,7 +341,7 @@ std::string SipClient::local_sdp_(bool answer) { std::string SipClient::build_invite_() { std::string sdp = this->local_sdp_(); std::string msg; - msg += "INVITE " + this->d_remote_target_ + " SIP/2.0\r\n"; + msg += "INVITE " + this->d_invite_uri_ + " SIP/2.0\r\n"; msg += "Via: SIP/2.0/UDP " + this->local_ip_ + ":" + std::to_string(this->local_port_) + ";branch=" + this->d_branch_ + ";rport\r\n"; msg += "Max-Forwards: 70\r\n"; @@ -369,10 +359,28 @@ std::string SipClient::build_invite_() { std::string SipClient::build_ack_(const SipMessage &resp) { std::string to = resp.header("To"); - std::string contact = resp.header("Contact"); - std::string target = extract_angle_uri(contact); - if (target.empty()) { - target = this->d_remote_target_; + bool success = resp.status_code >= 200 && resp.status_code < 300; + + // RFC 3261 §17.1.1.3: the ACK for a 3xx-6xx belongs to the INVITE + // transaction — same Request-URI and same top Via branch — or the server + // keeps retransmitting the failure (seen with 3CX after a 407). The ACK + // for a 2xx is its own transaction: new branch, sent to the dialog's + // remote target through the route set. + std::string target; + std::string route_block; + std::string branch; + if (success) { + target = extract_angle_uri(resp.header("Contact")); + if (target.empty()) target = this->d_remote_target_; + this->route_request_(target, route_block); + branch = gen_branch(); + } else { + target = this->d_invite_uri_.empty() ? this->d_remote_target_ : this->d_invite_uri_; + // Take the branch from the response itself so a late retransmission of + // an old transaction (e.g. the 407 while the authenticated INVITE is + // already in flight) is acknowledged with its own branch. + branch = via_branch(resp.header("Via")); + if (branch.empty()) branch = this->d_branch_; } uint32_t cseq = (uint32_t) std::atoi(resp.header("CSeq").c_str()); @@ -383,8 +391,9 @@ std::string SipClient::build_ack_(const SipMessage &resp) { std::string msg; msg += "ACK " + target + " SIP/2.0\r\n"; msg += "Via: SIP/2.0/UDP " + this->local_ip_ + ":" + std::to_string(this->local_port_) + - ";branch=" + gen_branch() + ";rport\r\n"; + ";branch=" + branch + ";rport\r\n"; msg += "Max-Forwards: 70\r\n"; + msg += route_block; msg += "From: " + this->d_local_ + "\r\n"; msg += "To: " + (to.empty() ? this->d_remote_ : to) + "\r\n"; msg += "Call-ID: " + this->d_call_id_ + "\r\n"; @@ -451,13 +460,17 @@ void SipClient::handle_invite_response_(const SipMessage &m, const std::string & return; } - // Capture remote tag and target, parse SDP, ACK, start media. + // Capture remote tag, target and route set, parse SDP, ACK, start media. std::string to = m.header("To"); if (!to.empty()) this->d_remote_ = to; std::string contact = m.header("Contact"); std::string target = extract_angle_uri(contact); if (!target.empty()) this->d_remote_target_ = target; + // RFC 3261 §12.1.2: the UAC's route set is the Record-Route list of the + // 2xx in reverse order. + this->dialog_routes_ = split_header_values(m.header("Record-Route")); + std::reverse(this->dialog_routes_.begin(), this->dialog_routes_.end()); SdpInfo sdp = parse_sdp(m.body); this->remote_rtp_ip_ = sdp.connection_ip.empty() ? this->remote_rtp_ip_ : sdp.connection_ip; @@ -516,13 +529,23 @@ std::string SipClient::build_response_(const SipMessage &req, int code, const st std::string sdp = with_sdp ? this->local_sdp_(/*answer=*/true) : ""; std::string msg; msg += "SIP/2.0 " + std::to_string(code) + " " + reason + "\r\n"; - msg += "Via: " + req.header("Via") + "\r\n"; + // Echo every Via, topmost first: a request that crossed a proxy/SBC has + // one per hop and the response must retrace them all (RFC 3261 §8.2.6.2). + for (const auto &via : split_header_values(req.header("Via"))) + msg += "Via: " + via + "\r\n"; msg += "From: " + req.header("From") + "\r\n"; msg += "To: " + to + "\r\n"; msg += "Call-ID: " + req.header("Call-ID") + "\r\n"; msg += "CSeq: " + req.header("CSeq") + "\r\n"; - if (code >= 200 && code < 300 && req.method == "INVITE") + // RFC 3261 §12.1.1: every dialog-establishing response — the early dialog + // of a 18x included — must echo the request's Record-Route and carry a + // Contact the peer can route in-dialog requests to. Dropping either + // strands the proxy outside the dialog and its ACK never reaches us. + if (req.method == "INVITE" && code > 100 && code < 300) { + for (const auto &route : split_header_values(req.header("Record-Route"))) + msg += "Record-Route: " + route + "\r\n"; msg += "Contact: " + this->contact_uri_() + "\r\n"; + } msg += "User-Agent: " + std::string(USER_AGENT) + "\r\n"; if (with_sdp) { msg += "Content-Type: application/sdp\r\n"; @@ -567,6 +590,9 @@ void SipClient::handle_request_(const SipMessage &m, const std::string &raw) { this->d_remote_ = m.header("From"); std::string contact = m.header("Contact"); this->d_remote_target_ = extract_angle_uri(contact); + this->d_invite_uri_.clear(); + // RFC 3261 §12.1.1: the UAS's route set is the Record-Route list as-is. + this->dialog_routes_ = split_header_values(m.header("Record-Route")); this->d_cseq_ = std::atoi(m.header("CSeq").c_str()); this->remote_rtp_ip_ = sdp.connection_ip; this->remote_rtp_port_ = sdp.audio_port; @@ -658,7 +684,7 @@ void SipClient::hangup() { case SIP_RINGING_OUT: { // CANCEL the pending INVITE (same branch/cseq). std::string msg; - msg += "CANCEL " + this->d_remote_target_ + " SIP/2.0\r\n"; + msg += "CANCEL " + this->d_invite_uri_ + " SIP/2.0\r\n"; msg += "Via: SIP/2.0/UDP " + this->local_ip_ + ":" + std::to_string(this->local_port_) + ";branch=" + this->d_branch_ + ";rport\r\n"; msg += "Max-Forwards: 70\r\n"; @@ -684,12 +710,46 @@ void SipClient::hangup() { } } +void SipClient::route_request_(std::string &target, std::string &route_block) const { + route_block.clear(); + std::vector active; + for (const auto &r : this->dialog_routes_) { + if (!trim(r).empty()) active.push_back(r); + } + if (active.empty()) return; + auto join = [](const std::vector &v) { + std::string out; + for (size_t i = 0; i < v.size(); i++) { + if (i) out += ", "; + out += v[i]; + } + return out; + }; + if (is_loose_route(active[0])) { + // Loose router: remote target stays in the Request-URI, whole route set + // travels as Route headers. + route_block = "Route: " + join(active) + "\r\n"; + return; + } + // Strict router: it takes the Request-URI; the remote target moves to the + // tail of the route set so it is not lost. + std::vector remaining(active.begin() + 1, active.end()); + if (!target.empty()) remaining.push_back("<" + target + ">"); + if (!remaining.empty()) route_block = "Route: " + join(remaining) + "\r\n"; + std::string first = extract_angle_uri(active[0]); + target = first.empty() ? active[0] : first; +} + std::string SipClient::build_request_in_dialog_(const std::string &method) { + std::string target = this->d_remote_target_; + std::string route_block; + this->route_request_(target, route_block); std::string msg; - msg += method + " " + this->d_remote_target_ + " SIP/2.0\r\n"; + msg += method + " " + target + " SIP/2.0\r\n"; msg += "Via: SIP/2.0/UDP " + this->local_ip_ + ":" + std::to_string(this->local_port_) + ";branch=" + gen_branch() + ";rport\r\n"; msg += "Max-Forwards: 70\r\n"; + msg += route_block; // For BYE the From/To orientation follows who originates: we are always local. msg += "From: " + this->d_local_ + "\r\n"; msg += "To: " + this->d_remote_ + "\r\n"; diff --git a/components/sip_client/sip_client.h b/components/sip_client/sip_client.h index 0e84b5cd..af12f108 100644 --- a/components/sip_client/sip_client.h +++ b/components/sip_client/sip_client.h @@ -102,6 +102,9 @@ class SipClient : public Component { std::string build_invite_(); std::string build_ack_(const SipMessage &resp); std::string build_request_in_dialog_(const std::string &method); + // Request-URI and "Route: ...\r\n" block (or "") for an in-dialog request + // (RFC 3261 §12.2.1.1: loose vs strict routers). + void route_request_(std::string &target, std::string &route_block) const; std::string build_response_(const SipMessage &req, int code, const std::string &reason, bool with_sdp); // Offer (answer=false): all supported codecs. Answer (true): chosen codec @@ -193,6 +196,8 @@ class SipClient : public Component { std::string d_local_; // our From-style header incl. tag std::string d_remote_; // peer header incl. tag std::string d_remote_target_; // request-URI for in-dialog requests + std::string d_invite_uri_; // Request-URI of our INVITE (non-2xx ACK / CANCEL target) + std::vector dialog_routes_; // route set (RFC 3261 §12.1), first hop first std::string d_local_tag_; std::string d_branch_; // branch of the INVITE transaction uint32_t d_cseq_{0}; diff --git a/components/sip_client/sip_message.cpp b/components/sip_client/sip_message.cpp index be4c3acf..51ebcfbd 100644 --- a/components/sip_client/sip_message.cpp +++ b/components/sip_client/sip_message.cpp @@ -37,6 +37,11 @@ static std::string resolve_header_name(const std::string &name) { return name; } +// Headers that may legally repeat and whose values form one ordered list. +static bool is_comma_list_header(const std::string &name) { + return name == "via" || name == "record-route" || name == "route" || name == "service-route"; +} + static std::vector split_ws(const std::string &s) { std::vector out; size_t i = 0; @@ -81,6 +86,7 @@ SipMessage parse_sip_message(const std::string &raw) { size_t pos = 0; bool first = true; + std::string last_name; while (pos < head.size()) { size_t eol = head.find("\r\n", pos); std::string line = head.substr(pos, eol == std::string::npos ? std::string::npos : eol - pos); @@ -106,16 +112,100 @@ SipMessage parse_sip_message(const std::string &raw) { continue; } + if (!line.empty() && (line[0] == ' ' || line[0] == '\t')) { + // Header folding: continuation of the previous header line. + if (!last_name.empty()) msg.headers[last_name] += " " + trim(line); + continue; + } + size_t colon = line.find(':'); if (colon == std::string::npos) continue; std::string name = resolve_header_name(to_lower(trim(line.substr(0, colon)))); std::string value = trim(line.substr(colon + 1)); - // Keep the first occurrence (topmost Via, etc.). - if (msg.headers.find(name) == msg.headers.end()) msg.headers[name] = value; + last_name = name; + auto it = msg.headers.find(name); + if (it == msg.headers.end()) { + msg.headers[name] = value; + } else if (is_comma_list_header(name)) { + // Repeated rows and one comma-separated row are equivalent on the wire; + // keep the proxy path in order (topmost Via first). + it->second += ", " + value; + } + // Any other repeated header keeps its first occurrence. } return msg; } +std::vector split_header_values(const std::string &value) { + std::vector values; + size_t start = 0; + int angle_depth = 0; + bool quoted = false; + bool escaped = false; + for (size_t i = 0; i < value.size(); i++) { + char c = value[i]; + if (escaped) { + escaped = false; + } else if (quoted && c == '\\') { + escaped = true; + } else if (c == '"') { + quoted = !quoted; + } else if (!quoted && c == '<') { + angle_depth++; + } else if (!quoted && c == '>' && angle_depth > 0) { + angle_depth--; + } else if (!quoted && angle_depth == 0 && c == ',') { + std::string item = trim(value.substr(start, i - start)); + if (!item.empty()) values.push_back(item); + start = i + 1; + } + } + std::string item = trim(value.substr(start)); + if (!item.empty()) values.push_back(item); + return values; +} + +std::string extract_angle_uri(const std::string &value) { + size_t lt = value.find('<'); + size_t gt = value.find('>'); + if (lt != std::string::npos && gt != std::string::npos && gt > lt) { + return value.substr(lt + 1, gt - lt - 1); + } + std::string stripped = trim(value); + if (stripped.rfind("sip", 0) == 0) { + return stripped; + } + return ""; +} + +std::string via_branch(const std::string &via) { + // Only the top Via names our transaction; later ones belong to proxies. + std::string top = via.substr(0, via.find(',')); + std::string lower = to_lower(top); + size_t p = lower.find(";branch="); + if (p == std::string::npos) return ""; + p += 8; + size_t e = top.find_first_of(";, \t\r\n", p); + return top.substr(p, e == std::string::npos ? std::string::npos : e - p); +} + +bool is_loose_route(const std::string &route) { + std::string uri = extract_angle_uri(route); + if (uri.empty()) uri = trim(route); + std::string lower = to_lower(uri); + // ";lr" must be a whole parameter: ";lr", ";lr=...", ";lr;..." or ";lr?..." + // count, but ";lrx" and a query/user part mentioning lr must not. + size_t p = 0; + while ((p = lower.find(";lr", p)) != std::string::npos) { + size_t after = p + 3; + if (after >= lower.size()) return true; + char c = lower[after]; + if (c == ';' || c == '=' || c == '?') return true; + p = after; + } + return false; +} + static void derive_codec_pts_(SdpInfo &info) { for (int pt : info.payload_types) { std::string name; diff --git a/components/sip_client/sip_message.h b/components/sip_client/sip_message.h index fb27c9cf..363d35df 100644 --- a/components/sip_client/sip_message.h +++ b/components/sip_client/sip_message.h @@ -16,8 +16,9 @@ struct SipMessage { std::string reason; // response only // Common headers (raw values, leading/trailing space trimmed). Names are - // stored lowercase in `headers`; the convenience fields below mirror the most - // used ones. + // stored lowercase in `headers`. Repeated list headers (Via, Record-Route, + // Route, Service-Route) are joined with ", " in wire order so the full + // proxy path survives; every other repeated header keeps its first value. std::map headers; std::string body; @@ -45,6 +46,19 @@ struct SdpInfo { SipMessage parse_sip_message(const std::string &raw); SdpInfo parse_sdp(const std::string &body); +// Split a comma-list header value (Via, Record-Route, Route, ...) into its +// field-values without splitting commas nested in <...> or quoted strings. +std::vector split_header_values(const std::string &value); + +// URI inside <...>, or the trimmed value itself when it is a bare sip: URI. +std::string extract_angle_uri(const std::string &value); + +// Branch parameter of the top Via (the transaction the message belongs to). +std::string via_branch(const std::string &via); + +// Whether a Record-Route/Route field-value points at a loose router (;lr). +bool is_loose_route(const std::string &route); + // Extract a quoted-or-token parameter from an auth header value, e.g. // auth_param("Digest realm=\"asterisk\", nonce=\"abc\"", "nonce") -> "abc". std::string auth_param(const std::string &header_value, const std::string &key); diff --git a/tests/native/sip_sdp/README.md b/tests/native/sip_sdp/README.md index 3b9e520e..547346fb 100644 --- a/tests/native/sip_sdp/README.md +++ b/tests/native/sip_sdp/README.md @@ -25,6 +25,21 @@ tests/native/sip_sdp/run.sh | `rejected_audio_stream_port_zero` | `m=audio 0` still parses | | `port_with_number_of_ports_suffix` | `m=audio 12345/2` → port 12345 | +### SIP header parsing / routing helpers (`test_sip_message.cpp`) + +| Test | Intent | +|------|--------| +| `multiple_via_kept_in_order` | INVITE through a proxy/SBC has one Via per hop; all kept, topmost first; `via_branch` reads the top one | +| `record_route_preserved` | Record-Route survives parsing and `;lr` is detected | +| `repeated_record_route_rows_and_comma_list_are_equivalent` | repeated rows vs one comma list → same ordered route set | +| `non_list_header_keeps_first_value` | Contact etc. still keep the first occurrence | +| `compact_via_is_merged` | `v:` and `Via:` rows merge into one chain | +| `header_folding` | leading-whitespace continuation lines are joined | +| `split_ignores_nested_commas` | commas inside `"..."` / `<...>` are not separators | +| `via_branch_variants` | param order, case, comma-joined chain, missing branch | +| `loose_route_detection` | `;lr`, `;lr=`, `;lr;`, case; `;lrx` and user-part `lr` are strict | +| `extract_angle_uri` | `<...>` vs bare `sip:` vs garbage | + ### `build_sdp_body` (`test_sdp_builder.cpp`) | Test | Intent | diff --git a/tests/native/sip_sdp/run.sh b/tests/native/sip_sdp/run.sh index 0d2a78c2..f22b9482 100644 --- a/tests/native/sip_sdp/run.sh +++ b/tests/native/sip_sdp/run.sh @@ -13,6 +13,12 @@ g++ "${CXXFLAGS[@]}" "${INC[@]}" \ -o "$OUT/sip_sdp_parse_test" "$OUT/sip_sdp_parse_test" +g++ "${CXXFLAGS[@]}" "${INC[@]}" \ + "$ROOT/components/sip_client/sip_message.cpp" \ + "$ROOT/tests/native/sip_sdp/test_sip_message.cpp" \ + -o "$OUT/sip_message_test" +"$OUT/sip_message_test" + g++ "${CXXFLAGS[@]}" "${INC[@]}" \ "$ROOT/components/sip_client/sdp_builder.cpp" \ "$ROOT/tests/native/sip_sdp/test_sdp_builder.cpp" \ diff --git a/tests/native/sip_sdp/test_sip_message.cpp b/tests/native/sip_sdp/test_sip_message.cpp new file mode 100644 index 00000000..ab12ba7e --- /dev/null +++ b/tests/native/sip_sdp/test_sip_message.cpp @@ -0,0 +1,198 @@ +#include "sip_message.h" + +#include +#include +#include +#include + +using esphome::sip_client::extract_angle_uri; +using esphome::sip_client::is_loose_route; +using esphome::sip_client::parse_sip_message; +using esphome::sip_client::SipMessage; +using esphome::sip_client::split_header_values; +using esphome::sip_client::via_branch; + +namespace { + +int g_failures = 0; + +void require(bool condition, const char *message) { + if (!condition) { + std::cerr << "FAIL: " << message << '\n'; + g_failures++; + } +} + +void require_eq_str(const std::string &actual, const std::string &expected, const char *message) { + if (actual != expected) { + std::cerr << "FAIL: " << message << " (got \"" << actual << "\", expected \"" << expected + << "\")\n"; + g_failures++; + } +} + +// An INVITE as it arrives through a 3CX SBC: two Via hops plus a +// Record-Route for the proxy. +const char *const PROXIED_INVITE = + "INVITE sip:16@192.168.1.50:5060 SIP/2.0\r\n" + "Via: SIP/2.0/UDP 192.168.1.194:5060;branch=z9hG4bK-sbc-1;rport\r\n" + "Via: SIP/2.0/UDP 10.0.0.5:5060;branch=z9hG4bK-pbx-2\r\n" + "Record-Route: \r\n" + "Max-Forwards: 69\r\n" + "From: \"Alice\" ;tag=abc\r\n" + "To: \r\n" + "Call-ID: call-1@10.0.0.5\r\n" + "CSeq: 1 INVITE\r\n" + "Contact: \r\n" + "Content-Length: 0\r\n\r\n"; + +// ---------------- repeated list headers ---------------- + +void test_multiple_via_kept_in_order() { + SipMessage m = parse_sip_message(PROXIED_INVITE); + std::vector vias = split_header_values(m.header("Via")); + require(vias.size() == 2, "both Via hops are kept"); + if (vias.size() == 2) { + require_eq_str(vias[0], "SIP/2.0/UDP 192.168.1.194:5060;branch=z9hG4bK-sbc-1;rport", + "topmost Via first"); + require_eq_str(vias[1], "SIP/2.0/UDP 10.0.0.5:5060;branch=z9hG4bK-pbx-2", "second Via next"); + } + require_eq_str(via_branch(m.header("Via")), "z9hG4bK-sbc-1", "branch comes from the top Via"); +} + +void test_record_route_preserved() { + SipMessage m = parse_sip_message(PROXIED_INVITE); + std::vector routes = split_header_values(m.header("Record-Route")); + require(routes.size() == 1, "Record-Route is kept"); + if (!routes.empty()) { + require_eq_str(routes[0], "", "Record-Route value"); + require(is_loose_route(routes[0]), "3CX record-route is a loose router"); + } +} + +void test_repeated_record_route_rows_and_comma_list_are_equivalent() { + const char *rows = + "INVITE sip:a@b SIP/2.0\r\n" + "Record-Route: \r\n" + "Record-Route: \r\n" + "Call-ID: x\r\n\r\n"; + const char *comma = + "INVITE sip:a@b SIP/2.0\r\n" + "Record-Route: , \r\n" + "Call-ID: x\r\n\r\n"; + std::vector a = split_header_values(parse_sip_message(rows).header("Record-Route")); + std::vector b = split_header_values(parse_sip_message(comma).header("Record-Route")); + require(a.size() == 2 && b.size() == 2, "two route hops either way"); + require(a == b, "repeated rows and comma list parse identically"); + if (a.size() == 2) { + require_eq_str(a[0], "", "first hop first"); + require_eq_str(a[1], "", "second hop second"); + } +} + +void test_non_list_header_keeps_first_value() { + const char *raw = + "SIP/2.0 200 OK\r\n" + "Contact: \r\n" + "Contact: \r\n" + "Call-ID: x\r\n\r\n"; + SipMessage m = parse_sip_message(raw); + require_eq_str(m.header("Contact"), "", "Contact keeps first occurrence"); +} + +void test_compact_via_is_merged() { + const char *raw = + "INVITE sip:a@b SIP/2.0\r\n" + "v: SIP/2.0/UDP h1;branch=z9hG4bK1\r\n" + "Via: SIP/2.0/UDP h2;branch=z9hG4bK2\r\n" + "i: x\r\n\r\n"; + SipMessage m = parse_sip_message(raw); + require(split_header_values(m.header("Via")).size() == 2, "compact v: merges with Via"); + require_eq_str(m.header("Call-ID"), "x", "compact i: resolves to Call-ID"); +} + +void test_header_folding() { + const char *raw = + "INVITE sip:a@b SIP/2.0\r\n" + "Via: SIP/2.0/UDP h1;branch=z9hG4bK1;\r\n" + " rport\r\n" + "Call-ID: x\r\n\r\n"; + SipMessage m = parse_sip_message(raw); + require_eq_str(m.header("Via"), "SIP/2.0/UDP h1;branch=z9hG4bK1; rport", "folded line joined"); +} + +// ---------------- split_header_values ---------------- + +void test_split_ignores_nested_commas() { + std::vector v = split_header_values( + "\"Doe, John\" ;q=1, ,, "); + require(v.size() == 3, "quoted and angle-nested commas are not separators"); + if (v.size() == 3) { + require_eq_str(v[0], "\"Doe, John\" ;q=1", "display-name comma kept"); + require_eq_str(v[1], "", "second value trimmed"); + require_eq_str(v[2], "", "empty items dropped"); + } +} + +void test_split_empty() { + require(split_header_values("").empty(), "empty header value -> no items"); + require(split_header_values(" , ").empty(), "only separators -> no items"); +} + +// ---------------- via_branch ---------------- + +void test_via_branch_variants() { + require_eq_str(via_branch("SIP/2.0/UDP 1.2.3.4:5060;rport;branch=z9hG4bKabc"), "z9hG4bKabc", + "branch after other params"); + require_eq_str(via_branch("SIP/2.0/UDP 1.2.3.4;BRANCH=z9hG4bKup;rport"), "z9hG4bKup", + "branch name is case-insensitive"); + require_eq_str(via_branch("SIP/2.0/UDP a;branch=z9hG4bK1, SIP/2.0/UDP b;branch=z9hG4bK2"), + "z9hG4bK1", "comma-joined chain -> top Via only"); + require_eq_str(via_branch("SIP/2.0/UDP a;rport"), "", "no branch -> empty"); + require_eq_str(via_branch(""), "", "empty Via -> empty"); +} + +// ---------------- is_loose_route ---------------- + +void test_loose_route_detection() { + require(is_loose_route(""), ";lr at end"); + require(is_loose_route(""), ";lr followed by param"); + require(is_loose_route(""), ";lr=on (Asterisk style)"); + require(is_loose_route(""), "case-insensitive"); + require(is_loose_route("sip:p;lr"), "bare URI"); + require(!is_loose_route(""), "no lr -> strict"); + require(!is_loose_route(""), ";lrx is not ;lr"); + require(!is_loose_route(""), "lr inside the user part is not a URI param"); + require(!is_loose_route(""), "empty -> strict"); +} + +// ---------------- extract_angle_uri ---------------- + +void test_extract_angle_uri() { + require_eq_str(extract_angle_uri("\"A\" ;tag=1"), "sip:a@b", "angle URI"); + require_eq_str(extract_angle_uri(" sip:a@b "), "sip:a@b", "bare sip URI trimmed"); + require_eq_str(extract_angle_uri("nonsense"), "", "not a URI -> empty"); +} + +} // namespace + +int main() { + test_multiple_via_kept_in_order(); + test_record_route_preserved(); + test_repeated_record_route_rows_and_comma_list_are_equivalent(); + test_non_list_header_keeps_first_value(); + test_compact_via_is_merged(); + test_header_folding(); + test_split_ignores_nested_commas(); + test_split_empty(); + test_via_branch_variants(); + test_loose_route_detection(); + test_extract_angle_uri(); + + if (g_failures != 0) { + std::cerr << g_failures << " failure(s)\n"; + return EXIT_FAILURE; + } + std::cout << "sip_message tests passed\n"; + return EXIT_SUCCESS; +} From 89dc5779132089e26c3f806f40e1a900821cd649 Mon Sep 17 00:00:00 2001 From: eigger Date: Tue, 22 Sep 2026 07:46:09 +0900 Subject: [PATCH 2/3] fix(sip_client): route set per response, one Route per hop, userinfo-safe ;lr Review follow-ups on #324: - is_loose_route: only ";lr" after the '@' (hostport + URI params) and before any '?' counts; "" is strict. - 2xx ACK builds its route set from that response's Record-Route instead of the stored dialog's, so a forked 2xx is acknowledged along its own path; the accepted dialog and its retransmissions are unchanged. - Route headers are emitted one per hop, matching the Record-Route echo, for proxies that only read the first value of a comma list. - The route-set logic moves to apply_route_set() in sip_message so it is host-testable; loose/strict/empty and reversed-2xx cases added. Co-Authored-By: Claude Opus 5 --- components/sip_client/sip_client.cpp | 48 +++------------- components/sip_client/sip_client.h | 3 - components/sip_client/sip_message.cpp | 44 ++++++++++++-- components/sip_client/sip_message.h | 8 +++ tests/native/sip_sdp/README.md | 4 ++ tests/native/sip_sdp/test_sip_message.cpp | 70 +++++++++++++++++++++++ 6 files changed, 128 insertions(+), 49 deletions(-) diff --git a/components/sip_client/sip_client.cpp b/components/sip_client/sip_client.cpp index 7f2c123f..bf14034e 100644 --- a/components/sip_client/sip_client.cpp +++ b/components/sip_client/sip_client.cpp @@ -20,13 +20,6 @@ namespace sip_client { static const char *const TAG = "sip_client"; static const char *const USER_AGENT = "ESPHome-sip_client"; -static std::string trim(const std::string &s) { - size_t b = s.find_first_not_of(" \t\r\n"); - if (b == std::string::npos) return ""; - size_t e = s.find_last_not_of(" \t\r\n"); - return s.substr(b, e - b + 1); -} - // Render an IPv4 sockaddr to dotted-quad without depending on inet_ntop. static std::string sockaddr_ip(const struct sockaddr_storage &ss, uint16_t *port) { if (ss.ss_family != AF_INET) return ""; @@ -364,15 +357,18 @@ std::string SipClient::build_ack_(const SipMessage &resp) { // RFC 3261 §17.1.1.3: the ACK for a 3xx-6xx belongs to the INVITE // transaction — same Request-URI and same top Via branch — or the server // keeps retransmitting the failure (seen with 3CX after a 407). The ACK - // for a 2xx is its own transaction: new branch, sent to the dialog's - // remote target through the route set. + // for a 2xx is its own transaction: new branch, sent to that response's + // Contact through that response's route set — not the stored dialog's, + // so a forked 2xx from another leg is acknowledged along its own path. std::string target; std::string route_block; std::string branch; if (success) { target = extract_angle_uri(resp.header("Contact")); if (target.empty()) target = this->d_remote_target_; - this->route_request_(target, route_block); + std::vector routes = split_header_values(resp.header("Record-Route")); + std::reverse(routes.begin(), routes.end()); + apply_route_set(routes, target, route_block); branch = gen_branch(); } else { target = this->d_invite_uri_.empty() ? this->d_remote_target_ : this->d_invite_uri_; @@ -710,40 +706,10 @@ void SipClient::hangup() { } } -void SipClient::route_request_(std::string &target, std::string &route_block) const { - route_block.clear(); - std::vector active; - for (const auto &r : this->dialog_routes_) { - if (!trim(r).empty()) active.push_back(r); - } - if (active.empty()) return; - auto join = [](const std::vector &v) { - std::string out; - for (size_t i = 0; i < v.size(); i++) { - if (i) out += ", "; - out += v[i]; - } - return out; - }; - if (is_loose_route(active[0])) { - // Loose router: remote target stays in the Request-URI, whole route set - // travels as Route headers. - route_block = "Route: " + join(active) + "\r\n"; - return; - } - // Strict router: it takes the Request-URI; the remote target moves to the - // tail of the route set so it is not lost. - std::vector remaining(active.begin() + 1, active.end()); - if (!target.empty()) remaining.push_back("<" + target + ">"); - if (!remaining.empty()) route_block = "Route: " + join(remaining) + "\r\n"; - std::string first = extract_angle_uri(active[0]); - target = first.empty() ? active[0] : first; -} - std::string SipClient::build_request_in_dialog_(const std::string &method) { std::string target = this->d_remote_target_; std::string route_block; - this->route_request_(target, route_block); + apply_route_set(this->dialog_routes_, target, route_block); std::string msg; msg += method + " " + target + " SIP/2.0\r\n"; msg += "Via: SIP/2.0/UDP " + this->local_ip_ + ":" + std::to_string(this->local_port_) + diff --git a/components/sip_client/sip_client.h b/components/sip_client/sip_client.h index af12f108..4858e627 100644 --- a/components/sip_client/sip_client.h +++ b/components/sip_client/sip_client.h @@ -102,9 +102,6 @@ class SipClient : public Component { std::string build_invite_(); std::string build_ack_(const SipMessage &resp); std::string build_request_in_dialog_(const std::string &method); - // Request-URI and "Route: ...\r\n" block (or "") for an in-dialog request - // (RFC 3261 §12.2.1.1: loose vs strict routers). - void route_request_(std::string &target, std::string &route_block) const; std::string build_response_(const SipMessage &req, int code, const std::string &reason, bool with_sdp); // Offer (answer=false): all supported codecs. Answer (true): chosen codec diff --git a/components/sip_client/sip_message.cpp b/components/sip_client/sip_message.cpp index 51ebcfbd..0b49c734 100644 --- a/components/sip_client/sip_message.cpp +++ b/components/sip_client/sip_message.cpp @@ -193,12 +193,18 @@ bool is_loose_route(const std::string &route) { std::string uri = extract_angle_uri(route); if (uri.empty()) uri = trim(route); std::string lower = to_lower(uri); - // ";lr" must be a whole parameter: ";lr", ";lr=...", ";lr;..." or ";lr?..." - // count, but ";lrx" and a query/user part mentioning lr must not. - size_t p = 0; - while ((p = lower.find(";lr", p)) != std::string::npos) { + // Only URI parameters count (RFC 3261 §16.4), and those follow the + // hostport: anything before '@' is userinfo, and ";lr" there is not one. + size_t p = lower.find('@'); + p = (p == std::string::npos) ? 0 : p + 1; + // A "?" starts the headers part; ";lr" after it is not a parameter either. + size_t end = lower.find('?', p); + if (end == std::string::npos) end = lower.size(); + // ";lr" must be a whole parameter: ";lr", ";lr=...", ";lr;..." or ";lr?" + // count, but ";lrx" must not. + while ((p = lower.find(";lr", p)) != std::string::npos && p < end) { size_t after = p + 3; - if (after >= lower.size()) return true; + if (after >= end) return true; char c = lower[after]; if (c == ';' || c == '=' || c == '?') return true; p = after; @@ -206,6 +212,34 @@ bool is_loose_route(const std::string &route) { return false; } +void apply_route_set(const std::vector &routes, std::string &target, + std::string &route_block) { + route_block.clear(); + std::vector active; + for (const auto &r : routes) { + if (!trim(r).empty()) active.push_back(r); + } + if (active.empty()) return; + // One Route header per hop (like the Record-Route echo): some proxies only + // read the first value of a comma-joined list. + auto emit = [&route_block](const std::vector &v) { + for (const auto &r : v) route_block += "Route: " + r + "\r\n"; + }; + if (is_loose_route(active[0])) { + // Loose router: remote target stays in the Request-URI, whole route set + // travels as Route headers. + emit(active); + return; + } + // Strict router: it takes the Request-URI; the remote target moves to the + // tail of the route set so it is not lost. + std::vector remaining(active.begin() + 1, active.end()); + if (!target.empty()) remaining.push_back("<" + target + ">"); + emit(remaining); + std::string first = extract_angle_uri(active[0]); + target = first.empty() ? active[0] : first; +} + static void derive_codec_pts_(SdpInfo &info) { for (int pt : info.payload_types) { std::string name; diff --git a/components/sip_client/sip_message.h b/components/sip_client/sip_message.h index 363d35df..65b63a08 100644 --- a/components/sip_client/sip_message.h +++ b/components/sip_client/sip_message.h @@ -59,6 +59,14 @@ std::string via_branch(const std::string &via); // Whether a Record-Route/Route field-value points at a loose router (;lr). bool is_loose_route(const std::string &route); +// Resolve the Request-URI and "Route: ...\r\n" lines for an in-dialog request +// (RFC 3261 §12.2.1.1). `target` is the remote target on entry; on return it +// is the Request-URI to use (a strict first hop takes it over and the remote +// target moves to the end of the route set). `route_block` is one Route +// header per hop, or empty when there is no route set. +void apply_route_set(const std::vector &routes, std::string &target, + std::string &route_block); + // Extract a quoted-or-token parameter from an auth header value, e.g. // auth_param("Digest realm=\"asterisk\", nonce=\"abc\"", "nonce") -> "abc". std::string auth_param(const std::string &header_value, const std::string &key); diff --git a/tests/native/sip_sdp/README.md b/tests/native/sip_sdp/README.md index 547346fb..f5457d8c 100644 --- a/tests/native/sip_sdp/README.md +++ b/tests/native/sip_sdp/README.md @@ -39,6 +39,10 @@ tests/native/sip_sdp/run.sh | `via_branch_variants` | param order, case, comma-joined chain, missing branch | | `loose_route_detection` | `;lr`, `;lr=`, `;lr;`, case; `;lrx` and user-part `lr` are strict | | `extract_angle_uri` | `<...>` vs bare `sip:` vs garbage | +| `route_set_empty_keeps_target` | no / blank route set → Request-URI untouched, no Route | +| `route_set_loose_router` | `;lr` first hop: Request-URI = remote target, one `Route:` per hop | +| `route_set_strict_router` | strict first hop takes the Request-URI, remote target appended | +| `route_set_from_reversed_2xx_record_route` | UAC route set = 2xx Record-Route reversed | ### `build_sdp_body` (`test_sdp_builder.cpp`) diff --git a/tests/native/sip_sdp/test_sip_message.cpp b/tests/native/sip_sdp/test_sip_message.cpp index ab12ba7e..2e27be6f 100644 --- a/tests/native/sip_sdp/test_sip_message.cpp +++ b/tests/native/sip_sdp/test_sip_message.cpp @@ -5,6 +5,7 @@ #include #include +using esphome::sip_client::apply_route_set; using esphome::sip_client::extract_angle_uri; using esphome::sip_client::is_loose_route; using esphome::sip_client::parse_sip_message; @@ -163,9 +164,74 @@ void test_loose_route_detection() { require(!is_loose_route(""), "no lr -> strict"); require(!is_loose_route(""), ";lrx is not ;lr"); require(!is_loose_route(""), "lr inside the user part is not a URI param"); + require(!is_loose_route(""), "lr=on inside the user part is not a URI param"); + require(!is_loose_route(""), "lr;x inside the user part is not a URI param"); + require(is_loose_route(""), "userinfo lr does not hide a real ;lr param"); + require(!is_loose_route(""), "lr in the headers part is not a URI param"); require(!is_loose_route(""), "empty -> strict"); } +// ---------------- apply_route_set ---------------- + +void test_route_set_empty_keeps_target() { + std::string target = "sip:100@10.0.0.5:5060"; + std::string block = "stale"; + apply_route_set({}, target, block); + require_eq_str(target, "sip:100@10.0.0.5:5060", "no route set: target untouched"); + require_eq_str(block, "", "no route set: no Route header"); + + apply_route_set({"", " "}, target, block); + require_eq_str(block, "", "blank entries count as no route set"); +} + +void test_route_set_loose_router() { + // 3CX / Kamailio style: the whole set travels as Route, one header per + // hop, and the Request-URI stays the remote Contact. + std::string target = "sip:100@10.0.0.5:5060"; + std::string block; + apply_route_set({"", ""}, target, block); + require_eq_str(target, "sip:100@10.0.0.5:5060", "loose: Request-URI is the remote target"); + require_eq_str(block, "Route: \r\nRoute: \r\n", + "loose: one Route line per hop, first hop first"); +} + +void test_route_set_strict_router() { + // RFC 2543-style first hop: it takes the Request-URI and the remote target + // is appended to the route set so it is not lost. + std::string target = "sip:100@10.0.0.5:5060"; + std::string block; + apply_route_set({"", ""}, target, block); + require_eq_str(target, "sip:strict.example", "strict: first hop becomes the Request-URI"); + require_eq_str(block, "Route: \r\nRoute: \r\n", + "strict: remaining hops then the remote target"); + + target = "sip:100@10.0.0.5:5060"; + apply_route_set({""}, target, block); + require_eq_str(target, "sip:strict.example", "single strict hop takes the Request-URI"); + require_eq_str(block, "Route: \r\n", "remote target still travels"); + + target = ""; + apply_route_set({"sip:bare.strict"}, target, block); + require_eq_str(target, "sip:bare.strict", "bare URI hop works without <>"); + require_eq_str(block, "", "no remote target -> nothing to append"); +} + +void test_route_set_from_reversed_2xx_record_route() { + // UAC: Record-Route of the 2xx reversed is the route set (RFC 3261 §12.1.2). + const char *raw = + "SIP/2.0 200 OK\r\n" + "Record-Route: \r\n" + "Record-Route: \r\n" + "Contact: \r\n\r\n"; + std::vector routes = split_header_values(parse_sip_message(raw).header("Record-Route")); + std::vector reversed(routes.rbegin(), routes.rend()); + std::string target = "sip:bob@10.0.0.9"; + std::string block; + apply_route_set(reversed, target, block); + require_eq_str(block, "Route: \r\nRoute: \r\n", + "nearest proxy (last Record-Route) is the first Route"); +} + // ---------------- extract_angle_uri ---------------- void test_extract_angle_uri() { @@ -188,6 +254,10 @@ int main() { test_via_branch_variants(); test_loose_route_detection(); test_extract_angle_uri(); + test_route_set_empty_keeps_target(); + test_route_set_loose_router(); + test_route_set_strict_router(); + test_route_set_from_reversed_2xx_record_route(); if (g_failures != 0) { std::cerr << g_failures << " failure(s)\n"; From 98589433ac2b240f270246481d6dada69970785f Mon Sep 17 00:00:00 2001 From: eigger Date: Tue, 22 Sep 2026 07:53:20 +0900 Subject: [PATCH 3/3] fix(sip_client): is_loose_route cuts at '?' before looking for '@' An '@' inside the URI headers part ("") was taken as the userinfo separator, hiding the real ";lr" parameter. Find the end of the parameters first, then the userinfo boundary within it. Co-Authored-By: Claude Opus 5 --- components/sip_client/sip_message.cpp | 13 +++++++------ tests/native/sip_sdp/test_sip_message.cpp | 3 +++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/components/sip_client/sip_message.cpp b/components/sip_client/sip_message.cpp index 0b49c734..9f4aa0d5 100644 --- a/components/sip_client/sip_message.cpp +++ b/components/sip_client/sip_message.cpp @@ -193,13 +193,14 @@ bool is_loose_route(const std::string &route) { std::string uri = extract_angle_uri(route); if (uri.empty()) uri = trim(route); std::string lower = to_lower(uri); - // Only URI parameters count (RFC 3261 §16.4), and those follow the - // hostport: anything before '@' is userinfo, and ";lr" there is not one. - size_t p = lower.find('@'); - p = (p == std::string::npos) ? 0 : p + 1; - // A "?" starts the headers part; ";lr" after it is not a parameter either. - size_t end = lower.find('?', p); + // Only URI parameters count (RFC 3261 §16.4). They sit between the + // hostport and the "?" that starts the headers part; cut there first so an + // '@' inside a header value is not mistaken for the userinfo separator, + // then skip the userinfo (";lr" before '@' is not a parameter). + size_t end = lower.find('?'); if (end == std::string::npos) end = lower.size(); + size_t p = lower.rfind('@', end); + p = (p == std::string::npos) ? 0 : p + 1; // ";lr" must be a whole parameter: ";lr", ";lr=...", ";lr;..." or ";lr?" // count, but ";lrx" must not. while ((p = lower.find(";lr", p)) != std::string::npos && p < end) { diff --git a/tests/native/sip_sdp/test_sip_message.cpp b/tests/native/sip_sdp/test_sip_message.cpp index 2e27be6f..e354246c 100644 --- a/tests/native/sip_sdp/test_sip_message.cpp +++ b/tests/native/sip_sdp/test_sip_message.cpp @@ -168,6 +168,9 @@ void test_loose_route_detection() { require(!is_loose_route(""), "lr;x inside the user part is not a URI param"); require(is_loose_route(""), "userinfo lr does not hide a real ;lr param"); require(!is_loose_route(""), "lr in the headers part is not a URI param"); + require(is_loose_route(""), + "an '@' inside the headers part is not the userinfo separator"); + require(!is_loose_route(""), "userinfo lr still strict with '@' in headers"); require(!is_loose_route(""), "empty -> strict"); }