From 849dc5cf722b42f15b6d95f2bd9f7ef58958edbb Mon Sep 17 00:00:00 2001 From: Pavlo Buidenkov Date: Fri, 22 Jan 2021 18:20:28 +0200 Subject: [PATCH] fix sharing session docs, comments on code --- app/client/src/features/PeerConnection/index.ts | 2 +- .../src/features/PeerConnection/peerConnectionHandleSocket.ts | 4 +++- app/features/PeerConnection/handleRecieveEncryptedMessage.ts | 1 + app/features/PeerConnection/handleSocketUserEnter.ts | 1 + app/server/darkwireSocket.ts | 4 +++- ...tc-screen-sharing-session-initiation-pavlobu-22012021.html | 2 +- ...rtc-screen-sharing-session-initiation-pavlobu-22012021.svg | 2 +- 7 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/client/src/features/PeerConnection/index.ts b/app/client/src/features/PeerConnection/index.ts index b7d4402..a3bc96f 100644 --- a/app/client/src/features/PeerConnection/index.ts +++ b/app/client/src/features/PeerConnection/index.ts @@ -142,7 +142,7 @@ export default class PeerConnection { this.socket.emit('USER_ENTER', { username: user.username, publicKey: user.publicKey, - ip: myIP, + ip: myIP, // TODO: remove as it is not used }); } diff --git a/app/client/src/features/PeerConnection/peerConnectionHandleSocket.ts b/app/client/src/features/PeerConnection/peerConnectionHandleSocket.ts index 8ae4d80..7ab961b 100644 --- a/app/client/src/features/PeerConnection/peerConnectionHandleSocket.ts +++ b/app/client/src/features/PeerConnection/peerConnectionHandleSocket.ts @@ -60,6 +60,7 @@ export default (peerConnection: PeerConnection) => { if (!peerConnection.partner) return; + // TODO: ADD_USER is actually not used, so will remove this code from host and client, this is no use... peerConnection.sendEncryptedMessage({ type: 'ADD_USER', payload: { @@ -72,8 +73,9 @@ export default (peerConnection: PeerConnection) => { peerConnection.sendEncryptedMessage({ type: 'DEVICE_DETAILS', + // TODO: add deviceIP in this payload payload: { - socketID: peerConnection.socket.io.engine.id, + socketID: peerConnection.socket.io.engine.id, // TODO: maybe this socketID can be actually retrieved by host? so there will be no use for client to send it? need to check os: peerConnection.myDeviceDetails.myOS, deviceType: peerConnection.myDeviceDetails.myDeviceType, browser: peerConnection.myDeviceDetails.myBrowser, diff --git a/app/features/PeerConnection/handleRecieveEncryptedMessage.ts b/app/features/PeerConnection/handleRecieveEncryptedMessage.ts index 510bcb4..d7bbde2 100644 --- a/app/features/PeerConnection/handleRecieveEncryptedMessage.ts +++ b/app/features/PeerConnection/handleRecieveEncryptedMessage.ts @@ -34,6 +34,7 @@ export default async function handleRecieveEncryptedMessage( 'GET_IP_BY_SOCKET_ID', message.payload.socketID, (deviceIP: string) => { + // TODO: need to add myIP in client message.payload.myIP, then if retrieved deviceIP and myIP from client don't match, we were spoofed, then we can interrupt connection immediately! handleDeviceIPMessage(deviceIP, peerConnection, message); } ); diff --git a/app/features/PeerConnection/handleSocketUserEnter.ts b/app/features/PeerConnection/handleSocketUserEnter.ts index 4025b83..21df50c 100644 --- a/app/features/PeerConnection/handleSocketUserEnter.ts +++ b/app/features/PeerConnection/handleSocketUserEnter.ts @@ -10,6 +10,7 @@ export default ( [peerConnection.partner] = filteredPartner; + // TODO: ADD_USER is actually not used, so will remove this code from host and client, this is no use... peerConnection.sendEncryptedMessage({ type: 'ADD_USER', payload: { diff --git a/app/server/darkwireSocket.ts b/app/server/darkwireSocket.ts index 5136b47..26fb24c 100644 --- a/app/server/darkwireSocket.ts +++ b/app/server/darkwireSocket.ts @@ -90,6 +90,7 @@ export default class Socket implements SocketOPTS { }); this.socket.on('GET_IP_BY_SOCKET_ID', (socketID, acknowledgeFunction) => { + // TODO: for security only allow localhost to use this socket event! right now it may be emitted by client which may be not secure. The purpose of this event is for host to get the actual IP of connected client socket and compare them with what was sent by client in DEVICE_DETAILS. acknowledgeFunction(socketsIPService.getSocketIPByID(socketID)); }); @@ -142,7 +143,7 @@ export default class Socket implements SocketOPTS { isOwner: this.socket.request.connection.remoteAddress.includes( LOCALHOST_SOCKET_IP ), - ip: payload.ip ? payload.ip : '', + ip: payload.ip ? payload.ip : '', // TODO: remove as it is not used }, ], }; @@ -158,6 +159,7 @@ export default class Socket implements SocketOPTS { }); this.socket.on('TOGGLE_LOCK_ROOM', async () => { + // TODO: in here if there is somehow already more than ONE client connected, then we were spoofed! Need to add code to interrupt connection immediately. const room: Room = (await this.fetchRoom()) as Room; const user = (room.users || []).find( (u) => u.socketId === this.socket.id && u.isOwner diff --git a/doc/init-sharing-session/deskreen-webrtc-screen-sharing-session-initiation-pavlobu-22012021.html b/doc/init-sharing-session/deskreen-webrtc-screen-sharing-session-initiation-pavlobu-22012021.html index 16e06b2..feec600 100644 --- a/doc/init-sharing-session/deskreen-webrtc-screen-sharing-session-initiation-pavlobu-22012021.html +++ b/doc/init-sharing-session/deskreen-webrtc-screen-sharing-session-initiation-pavlobu-22012021.html @@ -13,7 +13,7 @@