Repository navigation
Accept whitespace and extra params in the NOTIFY Event header - #922
RaphaelFakhri wants to merge 1 commit into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if len(m) == 0 { | ||
| // Event: event-type *( SEMI event-param ). SEMI and EQUAL allow whitespace | ||
| // around them, and generic params other than id may follow (RFC 6665). | ||
| params := strings.Split(event.Value(), ";") |
There was a problem hiding this comment.
🔴 Quoted parameters lose transfer results
For Event: refer;id=12;foo="tag;id=13;rest", handleNotify replaces the real id with 13. handleReferNotify then discards the final status for REFER 12, leaving the transfer to fail.
Learn more
An Event header permits generic parameters whose values are quoted strings. A semicolon inside such a string belongs to the value, not to the parameter list. Splitting the entire header on every semicolon turns quoted content into new parameters. handleReferNotify uses the parsed id to decide whether a NOTIFY belongs to the current REFER, so a spurious id can discard its final result. Both inbound NOTIFY handling and outbound NOTIFY handling call this parser.
Example: After REFER CSeq 12 receives a successful response, its peer sends NOTIFY with Event: refer;id=12;foo="tag;id=13;rest" and a SIP/2.0 200 OK sipfrag. The parser overwrites id 12 with 13, and the success is ignored rather than completing the transfer.
Recommended fix: Tokenize Event parameters while respecting quoted strings and escaped quotes; only treat semicolons outside quoted values as delimiters. Add a test with an id-like substring inside a quoted generic parameter.
Was this helpful? React with 👍 or 👎 to provide feedback.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #922 +/- ##
==========================================
+ Coverage 65.25% 70.75% +5.50%
==========================================
Files 51 45 -6
Lines 6588 8984 +2396
==========================================
+ Hits 4299 6357 +2058
- Misses 1915 2118 +203
- Partials 374 509 +135 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
handleNotifymatches theEventheader value against^refer(;id=(\d+))?$. RFC 3261 definesSEMIandEQUALwith optional whitespace around them, and RFC 6665 allows genericevent-params besidesid. SoEvent: refer; id=2,Event: refer ;id=2andEvent: refer;id=2;foo=barare all valid NOTIFYs for a REFER subscription, but each one fails the match withunknown event.The NOTIFY is then answered
501 Not Implementedand its sipfrag status is dropped. For a transfer, the 200 that reports success is lost.TransferSIPParticipantwaits until the transfer deadline and reports a failure, even though the transfer completed.This change splits the header on
;, compares the event type and parameter names case-insensitively after trimming whitespace, and ignores parameters other thanid. A non-numericidreturns400 Bad Requestinstead of501. The regular expression is removed.Testing
TestHandleNotifygains three cases:refer ; id = 1234,Refer;foo=bar;ID=1234;bazandrefer;id=abc. On currentmainthe first two fail withunknown event. With the change,go test ./pkg/sip -run '^TestHandleNotify$'passes (15 of 15 subtests), andgo test ./pkg/sippasses.Fixes #921