Implement WebSocket Authentication for Enhanced Security#33
Implement WebSocket Authentication for Enhanced Security#33DeveloperAmrit wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a PIN-based WebSocket authentication flow: server generates a 6‑digit session PIN, auto-authenticates local clients, and requires remote clients to authenticate. Client hook and UI updated to handle auth handshake, state, and error handling; server enforces authentication before processing control messages. Changes
Sequence Diagram(s)sequenceDiagram
participant Mobile as Mobile Client
participant Server as WebSocket Server
participant LocalApp as Local Client (Desktop)
Mobile->>Server: CONNECT ws://host:port/ws
Server->>Server: generate 6-digit PIN\nidentify remote vs local
alt remote client
Server->>Mobile: { type: "connected", authRequired: true }
Mobile->>Mobile: show PIN entry UI
Mobile->>Server: { type: "authenticate", pin: "XXXXXX" }
alt PIN matches
Server->>Mobile: { type: "auth-success" }
Mobile->>Mobile: set isAuthenticated = true
else PIN mismatch
Server->>Mobile: { type: "auth-failed" }
Mobile->>Mobile: show auth error
end
else local client
Server->>LocalApp: { type: "connected", pin: "XXXXXX", authRequired: false }
LocalApp->>LocalApp: display PIN to user (settings)
end
Mobile->>Server: { type: "text" / "mouse" / ... }
Server->>Server: verify authenticated -> process message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useRemoteConnection.ts (1)
65-68:⚠️ Potential issue | 🟠 MajorStale
wsreference in cleanup closure.The cleanup function captures
wsfrom the initial render (alwaysnull) because the dependency array is[]. TheclearTimeoutworks correctly sincereconnectTimeris scoped inside the effect, butws?.close()will never actually close the socket on unmount.Proposed fix — use a ref or close via the local `socket` variable
One approach is to track the socket in a ref:
+ const wsRef = useRef<WebSocket | null>(null); + useEffect(() => { ... const connect = () => { ... const socket = new WebSocket(wsUrl); + wsRef.current = socket; ... - setWs(socket); + setWs(socket); }; connect(); return () => { clearTimeout(reconnectTimer); - ws?.close(); + wsRef.current?.close(); }; }, []);
🤖 Fix all issues with AI agents
In `@src/hooks/useRemoteConnection.ts`:
- Around line 50-55: The socket.onclose handler currently clears status,
isAuthenticated and authRequired but leaves authError true; update the onclose
block in useRemoteConnection (socket.onclose) to also reset authError by calling
setAuthError(false) so the PIN screen doesn’t show a stale "Incorrect PIN" after
reconnect; optionally also ensure socket.onopen/handleConnect clears authError
as well to be safe during successful reconnects.
In `@src/routes/settings.tsx`:
- Around line 98-109: The SVG used inside the PIN alert (the JSX block that
renders when the variable `pin` is truthy) is missing an accessible title; add a
unique <title> element and reference it from the SVG via aria-labelledby (or
aria-label) so screen readers can announce the icon—e.g., insert a descriptive
title like "Info" (with an id such as "session-pin-icon-title") and set
aria-labelledby="session-pin-icon-title" (and keep role="img" if needed) on the
same <svg> element that currently renders the info icon in the Session PIN
alert.
In `@src/routes/trackpad.tsx`:
- Line 37: The two inline SVG elements in src/routes/trackpad.tsx (the
disconnected icon around line 37 and the error icon around line 78) are missing
accessible labels; update each SVG element to include an appropriate aria-label
(e.g., aria-label="Disconnected" and aria-label="Error") and role="img" so
assistive tech can read them (do not leave aria-hidden). Locate the <svg ...>
elements for those icons in the JSX and add these attributes to the SVG tags.
- Around line 68-74: The Connect button currently relies on the default button
type which is "submit" and can cause unintended form submissions; update the
button element that calls authenticate(pinInput) (the onClick handler
referencing authenticate and pinInput) to include an explicit type="button"
attribute so it won't submit any enclosing form implicitly.
- Around line 55-65: The PIN input currently accepts non-digits and allows Enter
to trigger authenticate without length validation; update the input handlers so
onChange for the input (bound to pinInput via setPinInput) filters out any
non-digit characters and truncates to 6 characters before calling setPinInput,
and update the onKeyDown handler to only call authenticate(pinInput) when
pinInput.length >= 6 (and optionally preventDefault for Enter when length < 6)
to match the button disabled logic; reference the input element using the
existing value={pinInput}, onChange={e => setPinInput(...)} and onKeyDown={(e)
=> { if (e.key === 'Enter') authenticate(pinInput); }} handlers and the
authenticate function.
In `@src/server/websocket.ts`:
- Around line 76-87: Add per-connection rate limiting for PIN authentication:
track failed attempts for each WebSocket (e.g., a Map like authFailCount keyed
by ws) when handling msg.type === 'authenticate'; increment the count on each
wrong PIN (compare against PIN), send the auth-failed message, and if count
exceeds a MAX_FAILED_ATTEMPTS constant (e.g., 5) then send a final failure
message and close the socket (ws.close()) and/or set a short timeout before
allowing further attempts; optionally add a small delay (e.g., setTimeout) after
each failed attempt to slow brute-force. Ensure successful auth resets/clears
the failure count and still sets clientAuth.set(ws, true).
🧹 Nitpick comments (2)
src/server/websocket.ts (2)
26-29:Math.random()is not cryptographically secure.For a session PIN,
Math.random()is predictable if the attacker can observe or infer the PRNG state. Consider usingcrypto.randomIntfrom Node'scryptomodule for better security, especially since this is a security-critical feature.Proposed fix
+import { randomInt } from 'crypto'; + -const PIN = Math.floor(100000 + Math.random() * 900000).toString(); +const PIN = randomInt(100000, 1000000).toString();
89-93: Silently dropping messages from unauthenticated clients — consider sending an error response.The commented-out error response (line 91) would help clients understand why their commands are being ignored. Without it, debugging connection issues for remote clients will be difficult.
Proposed fix
if (!clientAuth.get(ws)) { - // ws.send(JSON.stringify({ type: 'error', message: 'Authentication required' })); + ws.send(JSON.stringify({ type: 'error', message: 'Authentication required' })); return; }
| socket.onclose = () => { | ||
| setStatus('disconnected'); | ||
| setIsAuthenticated(false); | ||
| setAuthRequired(false); | ||
| reconnectTimer = setTimeout(connect, 3000); | ||
| }; |
There was a problem hiding this comment.
authError is not reset on reconnection.
When the WebSocket closes and reconnects, isAuthenticated and authRequired are cleared but authError remains true. After reconnection, the PIN entry screen will immediately show the "Incorrect PIN" error from the previous session.
Proposed fix
socket.onclose = () => {
setStatus('disconnected');
setIsAuthenticated(false);
setAuthRequired(false);
+ setAuthError(false);
reconnectTimer = setTimeout(connect, 3000);
};📝 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.
| socket.onclose = () => { | |
| setStatus('disconnected'); | |
| setIsAuthenticated(false); | |
| setAuthRequired(false); | |
| reconnectTimer = setTimeout(connect, 3000); | |
| }; | |
| socket.onclose = () => { | |
| setStatus('disconnected'); | |
| setIsAuthenticated(false); | |
| setAuthRequired(false); | |
| setAuthError(false); | |
| reconnectTimer = setTimeout(connect, 3000); | |
| }; |
🤖 Prompt for AI Agents
In `@src/hooks/useRemoteConnection.ts` around lines 50 - 55, The socket.onclose
handler currently clears status, isAuthenticated and authRequired but leaves
authError true; update the onclose block in useRemoteConnection (socket.onclose)
to also reset authError by calling setAuthError(false) so the PIN screen doesn’t
show a stale "Incorrect PIN" after reconnect; optionally also ensure
socket.onopen/handleConnect clears authError as well to be safe during
successful reconnects.
| {pin && ( | ||
| <div className="alert alert-info shadow-lg"> | ||
| <div> | ||
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" className="stroke-current flex-shrink-0 w-6 h-6"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" d="M13 16h-1v-4h-1m1-4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z"></path></svg> | ||
| <div> | ||
| <h3 className="font-bold">Session PIN</h3> | ||
| <div className="text-xs">Enter this PIN on your mobile device</div> | ||
| </div> | ||
| </div> | ||
| <div className="font-mono text-4xl font-black">{pin}</div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Add accessible title to the SVG icon.
The SVG on line 101 lacks an accessible title element, flagged by Biome's noSvgWithoutTitle rule.
Proposed fix
-<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" className="stroke-current flex-shrink-0 w-6 h-6"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" d="M13 16h-1v-4h-1m1-4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z"></path></svg>
+<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" className="stroke-current flex-shrink-0 w-6 h-6" aria-label="Info"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" d="M13 16h-1v-4h-1m1-4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z"></path></svg>📝 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.
| {pin && ( | |
| <div className="alert alert-info shadow-lg"> | |
| <div> | |
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" className="stroke-current flex-shrink-0 w-6 h-6"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" d="M13 16h-1v-4h-1m1-4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z"></path></svg> | |
| <div> | |
| <h3 className="font-bold">Session PIN</h3> | |
| <div className="text-xs">Enter this PIN on your mobile device</div> | |
| </div> | |
| </div> | |
| <div className="font-mono text-4xl font-black">{pin}</div> | |
| </div> | |
| )} | |
| {pin && ( | |
| <div className="alert alert-info shadow-lg"> | |
| <div> | |
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" className="stroke-current flex-shrink-0 w-6 h-6" aria-label="Info"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" d="M13 16h-1v-4h-1m1-4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z"></path></svg> | |
| <div> | |
| <h3 className="font-bold">Session PIN</h3> | |
| <div className="text-xs">Enter this PIN on your mobile device</div> | |
| </div> | |
| </div> | |
| <div className="font-mono text-4xl font-black">{pin}</div> | |
| </div> | |
| )} |
🧰 Tools
🪛 Biome (2.3.13)
[error] 101-101: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
🤖 Prompt for AI Agents
In `@src/routes/settings.tsx` around lines 98 - 109, The SVG used inside the PIN
alert (the JSX block that renders when the variable `pin` is truthy) is missing
an accessible title; add a unique <title> element and reference it from the SVG
via aria-labelledby (or aria-label) so screen readers can announce the
icon—e.g., insert a descriptive title like "Info" (with an id such as
"session-pin-icon-title") and set aria-labelledby="session-pin-icon-title" (and
keep role="img" if needed) on the same <svg> element that currently renders the
info icon in the Session PIN alert.
| <div className="h-full flex items-center justify-center p-6 bg-base-100"> | ||
| <div className="text-center space-y-4"> | ||
| <div className="text-error"> | ||
| <svg xmlns="http://www.w3.org/2000/svg" className="h-16 w-16 mx-auto" fill="none" viewBox="0 0 24 24" stroke="currentColor"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" d="M12 8v4m0 4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z" /></svg> |
There was a problem hiding this comment.
Add accessible labels to SVG icons.
Both SVGs (disconnected icon on line 37 and error icon on line 78) lack accessible text, flagged by Biome's noSvgWithoutTitle. Add aria-label attributes.
Also applies to: 78-78
🧰 Tools
🪛 Biome (2.3.13)
[error] 37-37: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
🤖 Prompt for AI Agents
In `@src/routes/trackpad.tsx` at line 37, The two inline SVG elements in
src/routes/trackpad.tsx (the disconnected icon around line 37 and the error icon
around line 78) are missing accessible labels; update each SVG element to
include an appropriate aria-label (e.g., aria-label="Disconnected" and
aria-label="Error") and role="img" so assistive tech can read them (do not leave
aria-hidden). Locate the <svg ...> elements for those icons in the JSX and add
these attributes to the SVG tags.
| <input | ||
| type="tel" | ||
| className="input input-bordered w-full text-center text-4xl tracking-[0.5em] font-mono h-16" | ||
| placeholder="000000" | ||
| maxLength={6} | ||
| value={pinInput} | ||
| onChange={e => setPinInput(e.target.value)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter') authenticate(pinInput); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
PIN input allows non-digit characters, and Enter bypasses the 6-digit length check.
Two issues with the PIN input:
type="tel"doesn't restrict to digits only on all browsers — non-numeric characters can be entered.- Pressing Enter (line 63) calls
authenticate(pinInput)without checkingpinInput.length >= 6, unlike the button which is disabled until 6 characters.
Proposed fix
<input
type="tel"
className="input input-bordered w-full text-center text-4xl tracking-[0.5em] font-mono h-16"
placeholder="000000"
maxLength={6}
value={pinInput}
- onChange={e => setPinInput(e.target.value)}
+ onChange={e => setPinInput(e.target.value.replace(/\D/g, ''))}
onKeyDown={(e) => {
- if (e.key === 'Enter') authenticate(pinInput);
+ if (e.key === 'Enter' && pinInput.length === 6) authenticate(pinInput);
}}
/>📝 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.
| <input | |
| type="tel" | |
| className="input input-bordered w-full text-center text-4xl tracking-[0.5em] font-mono h-16" | |
| placeholder="000000" | |
| maxLength={6} | |
| value={pinInput} | |
| onChange={e => setPinInput(e.target.value)} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter') authenticate(pinInput); | |
| }} | |
| /> | |
| <input | |
| type="tel" | |
| className="input input-bordered w-full text-center text-4xl tracking-[0.5em] font-mono h-16" | |
| placeholder="000000" | |
| maxLength={6} | |
| value={pinInput} | |
| onChange={e => setPinInput(e.target.value.replace(/\D/g, ''))} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' && pinInput.length === 6) authenticate(pinInput); | |
| }} | |
| /> |
🤖 Prompt for AI Agents
In `@src/routes/trackpad.tsx` around lines 55 - 65, The PIN input currently
accepts non-digits and allows Enter to trigger authenticate without length
validation; update the input handlers so onChange for the input (bound to
pinInput via setPinInput) filters out any non-digit characters and truncates to
6 characters before calling setPinInput, and update the onKeyDown handler to
only call authenticate(pinInput) when pinInput.length >= 6 (and optionally
preventDefault for Enter when length < 6) to match the button disabled logic;
reference the input element using the existing value={pinInput}, onChange={e =>
setPinInput(...)} and onKeyDown={(e) => { if (e.key === 'Enter')
authenticate(pinInput); }} handlers and the authenticate function.
| <button | ||
| className="btn btn-primary w-full max-w-xs btn-lg" | ||
| onClick={() => authenticate(pinInput)} | ||
| disabled={pinInput.length < 6} | ||
| > | ||
| Connect | ||
| </button> |
There was a problem hiding this comment.
Add explicit type="button" to prevent implicit form submission.
Biome flags this button as missing an explicit type prop. Without it, the default is "submit", which can cause unintended form submission if the DOM structure changes.
Proposed fix
<button
className="btn btn-primary w-full max-w-xs btn-lg"
onClick={() => authenticate(pinInput)}
disabled={pinInput.length < 6}
+ type="button"
>📝 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.
| <button | |
| className="btn btn-primary w-full max-w-xs btn-lg" | |
| onClick={() => authenticate(pinInput)} | |
| disabled={pinInput.length < 6} | |
| > | |
| Connect | |
| </button> | |
| <button | |
| className="btn btn-primary w-full max-w-xs btn-lg" | |
| onClick={() => authenticate(pinInput)} | |
| disabled={pinInput.length < 6} | |
| type="button" | |
| > | |
| Connect | |
| </button> |
🧰 Tools
🪛 Biome (2.3.13)
[error] 68-72: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In `@src/routes/trackpad.tsx` around lines 68 - 74, The Connect button currently
relies on the default button type which is "submit" and can cause unintended
form submissions; update the button element that calls authenticate(pinInput)
(the onClick handler referencing authenticate and pinInput) to include an
explicit type="button" attribute so it won't submit any enclosing form
implicitly.
| // Handle Authentication | ||
| if (msg.type === 'authenticate') { | ||
| if (msg.pin === PIN) { | ||
| clientAuth.set(ws, true); | ||
| ws.send(JSON.stringify({ type: 'auth-success' })); | ||
| console.log('Client authenticated successfully.'); | ||
| } else { | ||
| ws.send(JSON.stringify({ type: 'auth-failed' })); | ||
| console.log('Client failed authentication.'); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
No rate limiting on PIN authentication attempts — brute-force risk.
A 6-digit PIN has only 10⁶ possible values. Without rate limiting or lockout, a remote attacker can rapidly enumerate all PINs over the WebSocket. At thousands of attempts per second, the PIN could be cracked in under a minute.
Consider adding per-connection attempt limits (e.g., disconnect after 5 failures) or a short delay after each failed attempt.
Proposed fix — disconnect after max failed attempts
+const MAX_AUTH_ATTEMPTS = 5;
+const authAttempts = new WeakMap<WebSocket, number>();
// In the connection handler:
+authAttempts.set(ws, 0);
// In the authenticate handler:
if (msg.type === 'authenticate') {
+ const attempts = (authAttempts.get(ws) || 0) + 1;
+ authAttempts.set(ws, attempts);
if (msg.pin === PIN) {
clientAuth.set(ws, true);
ws.send(JSON.stringify({ type: 'auth-success' }));
console.log('Client authenticated successfully.');
} else {
ws.send(JSON.stringify({ type: 'auth-failed' }));
- console.log('Client failed authentication.');
+ console.log(`Client failed authentication. Attempt ${attempts}/${MAX_AUTH_ATTEMPTS}`);
+ if (attempts >= MAX_AUTH_ATTEMPTS) {
+ console.log('Max auth attempts reached. Disconnecting client.');
+ ws.close();
+ }
}
return;
}🤖 Prompt for AI Agents
In `@src/server/websocket.ts` around lines 76 - 87, Add per-connection rate
limiting for PIN authentication: track failed attempts for each WebSocket (e.g.,
a Map like authFailCount keyed by ws) when handling msg.type === 'authenticate';
increment the count on each wrong PIN (compare against PIN), send the
auth-failed message, and if count exceeds a MAX_FAILED_ATTEMPTS constant (e.g.,
5) then send a final failure message and close the socket (ws.close()) and/or
set a short timeout before allowing further attempts; optionally add a small
delay (e.g., setTimeout) after each failed attempt to slow brute-force. Ensure
successful auth resets/clears the failure count and still sets
clientAuth.set(ws, true).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/settings.tsx (1)
197-223:⚠️ Potential issue | 🟠 MajorReuse the authenticated WebSocket connection to send
update-config.The settings page creates a new, unauthenticated WebSocket and immediately sends
update-config. The server enforces authentication (lines 89–92 ofsrc/server/websocket.ts) and silently drops unauthenticated messages. For remote clients, this means the config update fails silently with no error feedback.The
useRemoteConnectionhook already provides an authenticated connection with proper handshake handling—reuse it instead of creating a new socket. This ensures the command is processed and handles both local and remote scenarios correctly.
🧹 Nitpick comments (2)
src/routes/settings.tsx (1)
63-73: Socket not closed after receivingconnectedmessage.The
server-iphandler (line 78) closes the socket after receiving the IP, but theconnectedhandler does not. This leaves the detection socket open until component unmount. Since this socket is only used for initial detection, close it after extracting the data.Proposed fix
if (data.type === 'connected') { if (data.serverIp) { console.log('Auto-detected IP:', data.serverIp); setIp(data.serverIp); } if (data.pin) { console.log('Received session PIN'); setPin(data.pin); } + socket.close(); }src/routes/trackpad.tsx (1)
47-85:authErroralert persists while user retypes — consider clearing on input change.When the user enters an incorrect PIN, the error alert stays visible as they type a new PIN. Clearing
authErroron input change would improve UX. This would require the hook to expose aclearAuthErroror reset it internally whenauthenticateis called.
|
create a new pr as per feedback in the issue itself |
2 participants
Fixes #25
Summary
This PR addresses a critical security vulnerability in the WebSocket server by implementing an authentication mechanism to prevent unauthorized access and control. The changes ensure that only authenticated clients can send commands to the server, mitigating the risk of malicious actors exploiting the open WebSocket connection.
Changes Implemented
Testing
Screenshot (server side)
Video (client side)
WhatsApp.Video.2026-02-08.at.07.53.53.mp4
Summary by CodeRabbit
New Features
Bug Fixes / UX