Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,27 @@ app.use((req, res, next) => {
next();
});

io.of('/queued-run').on('connection', (socket) => {
const userId = socket.handshake.query.userId as string;

if (userId) {
socket.join(`user-${userId}`);
logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);

socket.on('disconnect', () => {
logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
});
} else {
logger.log('warn', `Client connected to queued-run namespace without userId: ${socket.id}`);
socket.disconnect();
}
});
Comment on lines +163 to +177
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Do not trust client-supplied userId; validate and type-guard handshake input before joining rooms

Currently, a client can arbitrarily provide any userId via query and subscribe to another user's room, which is a PII leakage risk. Also, socket.handshake.query.userId as string may be an array at runtime. Add runtime guards and avoid trusting client-provided identifiers without authentication.

Refactor to bind the namespace to a variable, export it for emitters, and add strict parsing of userId:

-io.of('/queued-run').on('connection', (socket) => {
-  const userId = socket.handshake.query.userId as string;
-  
-  if (userId) {
-    socket.join(`user-${userId}`);
-    logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);
-    
-    socket.on('disconnect', () => {
-      logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
-    });
-  } else {
-    logger.log('warn', `Client connected to queued-run namespace without userId: ${socket.id}`);
-    socket.disconnect();
-  }
-});
+const queuedRunNsp = io.of('/queued-run');
+export const queuedRunNamespace = queuedRunNsp;
+
+queuedRunNsp.on('connection', (socket) => {
+  const raw = socket.handshake.query?.userId as unknown;
+  const userId = Array.isArray(raw) ? raw[0] : raw;
+
+  if (typeof userId === 'string' && userId.trim().length > 0) {
+    socket.join(`user-${userId}`);
+    logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);
+
+    socket.on('disconnect', () => {
+      logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
+    });
+  } else {
+    logger.log('warn', `Client connected to queued-run namespace without a valid userId: ${socket.id}`);
+    socket.disconnect();
+  }
+});

Follow-up: Strongly consider authenticating the socket (e.g., via a signed token in handshake.auth, or reading the server session and deriving userId on the server) instead of accepting it from the client. This prevents room hijacking. If you want, I can provide a middleware example for JWT/session validation.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
io.of('/queued-run').on('connection', (socket) => {
const userId = socket.handshake.query.userId as string;
if (userId) {
socket.join(`user-${userId}`);
logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);
socket.on('disconnect', () => {
logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
});
} else {
logger.log('warn', `Client connected to queued-run namespace without userId: ${socket.id}`);
socket.disconnect();
}
});
const queuedRunNsp = io.of('/queued-run');
export const queuedRunNamespace = queuedRunNsp;
queuedRunNsp.on('connection', (socket) => {
const raw = socket.handshake.query?.userId as unknown;
const userId = Array.isArray(raw) ? raw[0] : raw;
if (typeof userId === 'string' && userId.trim().length > 0) {
socket.join(`user-${userId}`);
logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);
socket.on('disconnect', () => {
logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
});
} else {
logger.log('warn', `Client connected to queued-run namespace without a valid userId: ${socket.id}`);
socket.disconnect();
}
});


setInterval(() => {
processQueuedRuns();
}, 5000);


server.listen(SERVER_PORT, '0.0.0.0', async () => {
try {
await connectDB();
Expand Down
68 changes: 66 additions & 2 deletions src/context/socket.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
import React, { createContext, useCallback, useContext, useMemo, useState } from 'react';
import React, { createContext, useCallback, useContext, useState, useRef, useEffect } from 'react';
import { io, Socket } from 'socket.io-client';
import { apiUrl } from "../apiConfig";

const SERVER_ENDPOINT = apiUrl;

interface SocketState {
socket: Socket | null;
queueSocket: Socket | null;
id: string;
setId: (id: string) => void;
connectToQueueSocket: (userId: string, onRunCompleted?: (data: any) => void) => void;
disconnectQueueSocket: () => void;
};

class SocketStore implements Partial<SocketState> {
socket = null;
socket: Socket | null = null;
queueSocket: Socket | null = null;
id = '';
};

Expand All @@ -22,7 +26,9 @@ export const useSocketStore = () => useContext(socketStoreContext);

export const SocketProvider = ({ children }: { children: JSX.Element }) => {
const [socket, setSocket] = useState<Socket | null>(socketStore.socket);
const [queueSocket, setQueueSocket] = useState<Socket | null>(socketStore.queueSocket);
const [id, setActiveId] = useState<string>(socketStore.id);
const runCompletedCallbackRef = useRef<((data: any) => void) | null>(null);

const setId = useCallback((id: string) => {
// the socket client connection is recomputed whenever id changes -> the new browser has been initialized
Expand All @@ -39,12 +45,70 @@ export const SocketProvider = ({ children }: { children: JSX.Element }) => {
setActiveId(id);
}, [setSocket]);

const connectToQueueSocket = useCallback((userId: string, onRunCompleted?: (data: any) => void) => {
runCompletedCallbackRef.current = onRunCompleted || null;

const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
transports: ["websocket"],
rejectUnauthorized: false,
query: { userId }
});

Comment on lines +51 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove ineffective/insecure option and prefer authenticated handshake over query

rejectUnauthorized: false has no effect in the browser and is misleading. Also, passing userId in the query is easily spoofed; prefer a signed token via auth or rely on server-side session.

Apply this minimal cleanup:

-    const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
-      transports: ["websocket"],
-      rejectUnauthorized: false,
-      query: { userId }
-    });
+    const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
+      transports: ["websocket"],
+      query: { userId }
+    });

Longer-term: move to auth: { token } and validate on the server in a namespace middleware.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
transports: ["websocket"],
rejectUnauthorized: false,
query: { userId }
});
const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
transports: ["websocket"],
query: { userId }
});
🤖 Prompt for AI Agents
In src/context/socket.tsx around lines 51 to 56, remove the ineffective and
misleading rejectUnauthorized: false option and stop sending userId in the query
(it is easily spoofed); instead, pass authentication via the socket auth payload
(e.g. auth: { token }) or rely on server-side session authentication—update the
io(...) options to remove rejectUnauthorized and move/replace query: { userId }
with a secure auth field or omit client-side identity entirely, and ensure
server-side namespace middleware validates the token/session.

newQueueSocket.on('connect', () => {
console.log('Queue socket connected for user:', userId);
});

newQueueSocket.on('connect_error', (error) => {
console.log('Queue socket connection error:', error);
});

newQueueSocket.on('run-completed', (completionData) => {
console.log('Run completed event received:', completionData);
if (runCompletedCallbackRef.current) {
runCompletedCallbackRef.current(completionData);
}
});

setQueueSocket(currentSocket => {
if (currentSocket) {
currentSocket.disconnect();
}
return newQueueSocket;
});

socketStore.queueSocket = newQueueSocket;
}, []);

const disconnectQueueSocket = useCallback(() => {
setQueueSocket(currentSocket => {
if (currentSocket) {
currentSocket.disconnect();
}
return null;
});

socketStore.queueSocket = null;
runCompletedCallbackRef.current = null;
}, []);

// Cleanup on unmount
useEffect(() => {
return () => {
if (queueSocket) {
queueSocket.disconnect();
}
};
}, [queueSocket]);

return (
<socketStoreContext.Provider
value={{
socket,
queueSocket,
id,
setId,
connectToQueueSocket,
disconnectQueueSocket,
}}
>
{children}
Expand Down
76 changes: 33 additions & 43 deletions src/pages/MainPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { ScheduleSettings } from "../components/robot/ScheduleSettings";
import { apiUrl } from "../apiConfig";
import { useNavigate } from 'react-router-dom';
import { AuthContext } from '../context/auth';
import { useSocketStore } from '../context/socket';

interface MainPageProps {
handleEditRecording: (id: string, fileName: string) => void;
Expand Down Expand Up @@ -54,6 +55,8 @@ export const MainPage = ({ handleEditRecording, initialContent }: MainPageProps)
const { state } = useContext(AuthContext);
const { user } = state;

const { connectToQueueSocket, disconnectQueueSocket } = useSocketStore();

const abortRunHandler = (runId: string, robotName: string, browserId: string) => {
notify('info', t('main_page.notifications.abort_initiated', { name: robotName }));

Expand Down Expand Up @@ -138,50 +141,7 @@ export const MainPage = ({ handleEditRecording, initialContent }: MainPageProps)
navigate(`/runs/${robotMetaId}/run/${runId}`);

if (queued) {
console.log('Creating queue socket for queued run:', runId);

setQueuedRuns(prev => new Set([...prev, runId]));

const queueSocket = io(`${apiUrl}/queued-run`, {
transports: ["websocket"],
rejectUnauthorized: false,
query: { userId: user?.id }
});

queueSocket.on('connect', () => {
console.log('Queue socket connected for user:', user?.id);
});

queueSocket.on('connect_error', (error) => {
console.log('Queue socket connection error:', error);
});

queueSocket.on('run-completed', (completionData) => {
if (completionData.runId === runId) {
setRunningRecordingName('');
setCurrentInterpretationLog('');
setRerenderRuns(true);

setQueuedRuns(prev => {
const newSet = new Set(prev);
newSet.delete(runId);
return newSet;
});

const robotName = completionData.robotName || runningRecordingName;

if (completionData.status === 'success') {
notify('success', t('main_page.notifications.interpretation_success', { name: robotName }));
} else {
notify('error', t('main_page.notifications.interpretation_failed', { name: robotName }));
}

queueSocket.disconnect();
}
});

setSockets(sockets => [...sockets, queueSocket]);

notify('info', `Run queued: ${runningRecordingName}`);
} else {
const socket = io(`${apiUrl}/${browserId}`, {
Expand Down Expand Up @@ -245,6 +205,36 @@ export const MainPage = ({ handleEditRecording, initialContent }: MainPageProps)
return message === 'success';
}

useEffect(() => {
if (user?.id) {
const handleRunCompleted = (completionData: any) => {
setRerenderRuns(true);

if (queuedRuns.has(completionData.runId)) {
setQueuedRuns(prev => {
const newSet = new Set(prev);
newSet.delete(completionData.runId);
return newSet;
});
}

const robotName = completionData.robotName || 'Unknown Robot';

if (completionData.status === 'success') {
notify('success', t('main_page.notifications.interpretation_success', { name: robotName }));
} else {
notify('error', t('main_page.notifications.interpretation_failed', { name: robotName }));
}
};

connectToQueueSocket(user.id, handleRunCompleted);

return () => {
disconnectQueueSocket();
};
}
}, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, queuedRuns, setQueuedRuns]);

Comment on lines +208 to +237
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Prevent unnecessary socket reconnections caused by queuedRuns in deps; use an idempotent delete

Including queuedRuns in the dependency array will reconnect the queue socket on every state change. Instead, remove the membership check and always attempt to delete within a functional setter, then drop queuedRuns from deps.

Apply this diff:

   useEffect(() => {
     if (user?.id) {
       const handleRunCompleted = (completionData: any) => {
         setRerenderRuns(true);
-        
-        if (queuedRuns.has(completionData.runId)) {
-          setQueuedRuns(prev => {
-            const newSet = new Set(prev);
-            newSet.delete(completionData.runId);
-            return newSet;
-          });
-        }
+        setQueuedRuns(prev => {
+          const newSet = new Set(prev);
+          newSet.delete(completionData.runId); // idempotent
+          return newSet;
+        });
         
         const robotName = completionData.robotName || 'Unknown Robot';
         
         if (completionData.status === 'success') {
           notify('success', t('main_page.notifications.interpretation_success', { name: robotName }));
         } else {
           notify('error', t('main_page.notifications.interpretation_failed', { name: robotName }));
         }
       };
       
-      connectToQueueSocket(user.id, handleRunCompleted);
+      connectToQueueSocket(String(user.id), handleRunCompleted);
       
       return () => {
         disconnectQueueSocket();
       };
     }
-  }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, queuedRuns, setQueuedRuns]);
+  }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, setQueuedRuns]);

Notes:

  • Casting user.id to string prevents accidental type mismatches with the server expectation.
  • Dropping queuedRuns from deps avoids reconnect churn while still correctly updating the set.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (user?.id) {
const handleRunCompleted = (completionData: any) => {
setRerenderRuns(true);
if (queuedRuns.has(completionData.runId)) {
setQueuedRuns(prev => {
const newSet = new Set(prev);
newSet.delete(completionData.runId);
return newSet;
});
}
const robotName = completionData.robotName || 'Unknown Robot';
if (completionData.status === 'success') {
notify('success', t('main_page.notifications.interpretation_success', { name: robotName }));
} else {
notify('error', t('main_page.notifications.interpretation_failed', { name: robotName }));
}
};
connectToQueueSocket(user.id, handleRunCompleted);
return () => {
disconnectQueueSocket();
};
}
}, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, queuedRuns, setQueuedRuns]);
useEffect(() => {
if (user?.id) {
const handleRunCompleted = (completionData: any) => {
setRerenderRuns(true);
setQueuedRuns(prev => {
const newSet = new Set(prev);
newSet.delete(completionData.runId); // idempotent
return newSet;
});
const robotName = completionData.robotName || 'Unknown Robot';
if (completionData.status === 'success') {
notify('success', t('main_page.notifications.interpretation_success', { name: robotName }));
} else {
notify('error', t('main_page.notifications.interpretation_failed', { name: robotName }));
}
};
connectToQueueSocket(String(user.id), handleRunCompleted);
return () => {
disconnectQueueSocket();
};
}
}, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, setQueuedRuns]);
🤖 Prompt for AI Agents
In src/pages/MainPage.tsx around lines 208 to 237, the effect reconnects the
socket on every queuedRuns state change and uses a membership check before
deleting; remove queuedRuns from the dependency array, change the handler to
always delete the runId using the functional state setter (no prior has()
check), and cast user.id to string when calling connectToQueueSocket to avoid
type mismatches; keep all other dependencies (connectToQueueSocket,
disconnectQueueSocket, t, setRerenderRuns, setQueuedRuns) in the array and
ensure disconnectQueueSocket is still called in the cleanup.

const DisplayContent = () => {
switch (content) {
case 'robots':
Expand Down