regression: sip integration logs with duplicate references and server connection instance #37204
Conversation
… and server connection details
|
Looks like this PR is ready to merge! 🎉 |
|
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: QA assured
WalkthroughIntroduces a helper to strip internal Drachtio fields from SIP messages and updates Incoming/Outgoing SIP call providers to log sanitized request/response objects and negotiation identifiers. No control flow, error handling, or public API behavior changes beyond the added utility method. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ee/packages/media-calls/src/sip/Session.ts (1)
112-116: Add null/undefined guard for robustness.The method doesn't handle null or undefined inputs, which could cause a runtime error if called with an invalid argument. Consider adding a guard clause or type narrowing.
Apply this diff to add input validation:
public stripDrachtioServerDetails(reqOrRes: Srf.SipMessage): Record<string, any> { + if (!reqOrRes || typeof reqOrRes !== 'object') { + return {}; + } const { _agent, socket: _socket, _req, _res, ...data } = reqOrRes as Record<string, any>; return data; }ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (1)
264-264: Consider clarifying the property name in error log.The error log uses
localNegotiationas the property name but assigns it the valuelocalNegotiation.id. For clarity, consider renaming the property tonegotiationIdto match the actual data being logged.Apply this diff for better clarity:
- logger.error({ msg: 'Invalid Negotiation reference on OutgoingSipCall.', localNegotiation: localNegotiation.id }); + logger.error({ msg: 'Invalid Negotiation reference on OutgoingSipCall.', negotiationId: localNegotiation.id });ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (1)
310-310: Consider clarifying the property name in error log.Similar to OutgoingSipCall line 264, the error log uses
localNegotiationas the property name but assigns it the valuelocalNegotiation.id. For consistency and clarity, consider renaming the property tonegotiationId.Apply this diff for better clarity:
- logger.error({ msg: 'Invalid Negotiation reference on IncomingSipCall.', localNegotiation: localNegotiation.id }); + logger.error({ msg: 'Invalid Negotiation reference on IncomingSipCall.', negotiationId: localNegotiation.id });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ee/packages/media-calls/src/sip/Session.ts(1 hunks)ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts(3 hunks)ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (2)
149-165: LGTM! Proper sanitization of Drachtio server details.The changes correctly sanitize SIP message objects before logging by removing internal connection details, which improves security and reduces log verbosity.
226-226: LGTM! Consistent sanitization pattern.The modify handler now correctly sanitizes the request before logging, consistent with other log statements in the file.
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (2)
174-174: LGTM! Consistent sanitization in modify handler.The debug log now sanitizes the request object before logging, consistent with the pattern used in OutgoingSipCall and improving log security.
204-204: LGTM! Proper sanitization in cancel handler.The cancel method now sanitizes the SIP response before logging, preventing potential leakage of server connection details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/media-calls/src/sip/Session.ts (1)
112-116: Consider adding input validation and clarifying variable naming.The method serves its purpose well, but consider these improvements:
- The destructuring uses
socket: _socketwhere_socketis just discarded. Consider using a plain underscore or leaving it assocketfor clarity.- Add a null/undefined guard to prevent runtime errors if called with invalid input.
Apply this diff for improved robustness:
public stripDrachtioServerDetails(reqOrRes: Srf.SipMessage): Record<string, any> { + if (!reqOrRes) { + return {}; + } - const { _agent, socket: _socket, _req, _res, ...data } = reqOrRes as Record<string, any>; + const { _agent, socket, _req, _res, ...data } = reqOrRes as Record<string, any>; return data; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ee/packages/media-calls/src/sip/Session.ts(1 hunks)ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts(3 hunks)ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (3)
174-174: LGTM!The logging sanitization correctly strips internal Drachtio server details from the request object without affecting control flow.
204-204: LGTM!The logging sanitization correctly strips internal Drachtio server details from the response object in the cancel handler.
310-310: LGTM!Logging only the negotiation ID instead of the full object prevents exposing unnecessary internal details while still providing useful debugging information.
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (5)
149-152: LGTM!The guard check
provRes &&combined with sanitization properly handles the optional provisional response without exposing internal server details.
155-155: LGTM!The request object is correctly sanitized before logging, preventing exposure of internal Drachtio server connection details.
159-164: LGTM!The logging sanitization for both request and response objects is well-implemented with appropriate guard checks, while preserving the
ackparameter for debugging purposes.
226-226: LGTM!The request sanitization in the modify handler is consistent with other logging improvements in this file.
264-264: LGTM!Logging only the negotiation ID prevents exposing the full negotiation object's internal details while maintaining debugging utility.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37204 +/- ##
==================================================
- Coverage 66.37% 66.35% -0.02%
==================================================
Files 3386 3386
Lines 115619 115619
Branches 21351 21351
==================================================
- Hits 76739 76724 -15
- Misses 36275 36289 +14
- Partials 2605 2606 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores