fix: validate roomId in WebRTC signaling handlers#107
fix: validate roomId in WebRTC signaling handlers#107LakshmiSravyaVedantham wants to merge 2 commits intogbowne1:masterfrom
Conversation
|
@LakshmiSravya123 This is great but this wont work without #61 and or #68 for live chat and live stream functionality applied to master branch Thanks for the attempt With this change there would be a lot of overlap between this PR and #61 #68. Am currently not supporting merge of this PR. It also appears that this PR was generated by Claude AI. While I'm mostly okay with that, I'd rather have actual working code first then use AI to debug the code and its issues rather than generate the entire solution |
shishir-21
left a comment
There was a problem hiding this comment.
@LakshmiSravya123
@gbowne1
I reviewed the implementation focusing on signaling validation and architectural impact.
The validateRoom() helper is a good addition and improves signaling security by ensuring that:
-The room exists in activeStreams
-The socket has joined the room before forwarding WebRTC messages
However, I noticed a few blocking concerns:
1.) This PR introduces WebRTC signaling logic that overlaps with #61 and #68, which are not merged into master yet. This may cause duplication or architectural conflicts.
2.)The server is started using both app.listen and httpServer.listen, which can cause runtime errors.
3.)The use of the error event for custom error messages may conflict with reserved Socket.IO behavior.
4.)There is no authentication middleware applied to the Socket.IO connection.
Add roomId validation to offer, answer, and ice-candidate socket handlers. Before forwarding signaling messages, the server now verifies that the roomId exists in activeStreams and that the socket belongs to the room, emitting an error event back to the client on failure.
9d700c5 to
d3565e9
Compare
shishir-21
left a comment
There was a problem hiding this comment.
@LakshmiSravya123
@gbowne1
Thanks for the update.
I see that #61 is now merged, but since #68 is still pending and this branch has conflicts in server.js, I recommend resolving the conflicts and rebasing against the latest master first.
Given the architectural overlap and lack of tests, it may be better to revisit this implementation after #68 is merged to avoid duplication and instability.
|
@shishir-21 @glenjaysondmello @Ved178 I've completed the first couple rounds of refactoring this app and just released #117 so this work done in this PR will have to be completed once the refactoring changes have been merged closing this also due to merge conflict's that wouldn't work without significant work to it |
Summary
server.js, including avalidateRoom()helper that checks bothactiveStreams.has(roomId)andsocket.rooms.has(roomId)before forwarding messagesoffer,answer, andice-candidatehandlers now reject signaling attempts for non-existent rooms or sockets that have not joined the room, emitting a descriptiveerrorevent back to the callersocket.ioas a production dependency inpackage.jsonCloses #106
Test plan
ws://localhost:3000offer/answer/ice-candidatewith an invalidroomIdand confirm anerrorevent is receivederrorevent is received