Allow SIPServer to be configured with interceptors. - #777
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| func (s *Server) wrapHandler(handler sipgo.RequestHandler) sipgo.RequestHandler { | ||
| ret := handler | ||
| for _, interceptor := range slices.Backward(s.interceptors) { | ||
| ret = interceptor(ret) | ||
| } | ||
| return ret | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 A single failing call can now crash the whole SIP service
The safety net that caught unexpected failures while handling incoming SIP messages was removed (wrapHandler at pkg/sip/server.go:324-331) and nothing replaces it in the production startup path, so one bad call can take down the entire service and every other call on it.
Impact: An unexpected error while processing one incoming call terminates the process, dropping all in-progress calls on that node.
Removal of the panic-recovery wrapper with no default replacement
Previously every handler registered in Start was wrapped by withRecovery, which deferred a recover() and logged the panic with method/Call-ID/stack. The new wrapHandler only applies the caller-supplied s.interceptors, and WithInterceptors (pkg/sip/server.go:212-216) is never used by any production caller: cmd/livekit-sip/main.go:102, test/cloud/service.go:21, and test/integration/sip_test.go:98 all call sip.NewService without options. Consequently a panic inside onInvite/onAck/onBye/onNotify/onOptions/OnNoRoute propagates up the sipgo serving goroutine and crashes the process.
Either keep a recovery interceptor applied by default inside wrapHandler, or export a RecoveryInterceptor and install it at the NewService/main.go level.
Prompt for agents
The PR removed withRecovery, which wrapped every SIP request handler in a deferred recover() so that a panic in handling an INVITE/ACK/BYE/NOTIFY/OPTIONS did not kill the process. The new wrapHandler in pkg/sip/server.go only applies user-supplied interceptors, and no production caller (cmd/livekit-sip/main.go, test/cloud/service.go, test/integration/sip_test.go) passes any interceptors, so panic protection is now completely gone in deployed builds. Restore the protection either by always applying a recovery interceptor as the outermost wrapper inside wrapHandler, or by exporting the recovery logic as a reusable HandlerInterceptor and wiring it in at NewService/main.go so default behaviour is unchanged.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func (s *Server) wrapHandler(handler sipgo.RequestHandler) sipgo.RequestHandler { | ||
| ret := handler | ||
| for _, interceptor := range slices.Backward(s.interceptors) { | ||
| ret = interceptor(ret) | ||
| } | ||
| return ret | ||
| } | ||
|
|
There was a problem hiding this comment.
🟨 Removal of default panic recovery enables remote denial of service via malformed SIP requests
The PR removes the default panic-recovery wrapper around all SIP request handlers (wrapHandler at pkg/sip/server.go:324-331) and no production caller supplies a replacement interceptor (cmd/livekit-sip/main.go:102, test/cloud/service.go:21). Any unauthenticated remote peer that can reach the SIP listening port and trigger a panic in INVITE/ACK/BYE/NOTIFY/OPTIONS processing now crashes the entire process, terminating all concurrent calls.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Currently, the SIP server is configured so that each handler is automatically wrapped with a panic recovery interceptor. This PR removes the panic interceptors and allows SIP servers to be configured with arbitrary interceptors (so as to provide greater flexibility to users). Also, allow update
NewServiceso that callers might specify options to be passed to the underlying server.