feat: add token-based WebSocket authentication (#25)#108
Merged
imxade merged 3 commits intoAOSSIE-Org:mainfrom Feb 18, 2026
Merged
feat: add token-based WebSocket authentication (#25)#108imxade merged 3 commits intoAOSSIE-Org:mainfrom
imxade merged 3 commits intoAOSSIE-Org:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds token-based authentication: a file-backed server token store, WebSocket upgrade checks requiring tokens for remote clients (localhost exempt), client-side token generation/persistence, and inclusion of tokens in connection URLs and QR/share links. Changes
Sequence Diagram(s)sequenceDiagram
participant SettingsPage
participant RemoteClient
participant WS_Server
participant TokenStore
SettingsPage->>WS_Server: WS connect /ws (localhost)
SettingsPage->>WS_Server: send {"type":"generate-token"}
WS_Server->>TokenStore: generateToken() / getActiveToken()
TokenStore-->>WS_Server: token
WS_Server->>TokenStore: storeToken(token)
WS_Server-->>SettingsPage: {"type":"token-generated","token":...}
SettingsPage->>SettingsPage: save to localStorage, regen QR/share URL (includes token)
RemoteClient->>WS_Server: WS connect /ws?token=<token>
WS_Server->>TokenStore: isKnownToken(token)
alt token valid
TokenStore-->>WS_Server: true
WS_Server->>TokenStore: touchToken(token)
WS_Server-->>RemoteClient: accept connection
else invalid
WS_Server-->>RemoteClient: reject (401 / close)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/websocket.ts (1)
112-126:⚠️ Potential issue | 🟠 Major
update-configis not restricted to localhost — any authenticated remote client can change server configuration.The
generate-tokenhandler (line 102) correctly gates onisLocal, butupdate-configdoes not. An authenticated remote client (e.g., a phone, or an attacker with a stolen token) can write arbitrary config toserver-config.json, changing the frontend port or potentially injecting other keys.Suggested fix
if (msg.type === 'update-config') { + if (!isLocal) { + ws.send(JSON.stringify({ type: 'auth-error', error: 'Only localhost can update config' })); + return; + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 112 - 126, The update-config WebSocket handler (the if (msg.type === 'update-config') block) accepts config changes from any authenticated client; restrict it to local-only like the generate-token handler does by checking the same isLocal flag before applying changes. Modify the update-config branch to first verify isLocal (or call the same isLocal-checking function used by generate-token), return/send an error message and do not write to server-config.json if not local, and optionally log the unauthorized attempt; keep the existing try/catch and success/error response behavior for allowed requests.src/hooks/useRemoteConnection.ts (1)
48-56:⚠️ Potential issue | 🟠 MajorInfinite reconnect loop when server rejects the connection with 401.
When a remote client connects without a valid token, the server destroys the socket during upgrade with a 401. This fires
onerror→close()→onclose→ 3s reconnect, looping indefinitely against a server that will always reject it.Consider tracking consecutive failures or checking if the close was due to an auth rejection (e.g., WebSocket close code) and stopping reconnection in that case.
Suggested approach
+ let consecutiveFailures = 0; + const connect = () => { if (!isMounted) return; // Close any existing socket before creating a new one if (wsRef.current) { wsRef.current.onopen = null; wsRef.current.onclose = null; wsRef.current.onerror = null; wsRef.current.close(); wsRef.current = null; } setStatus('connecting'); const socket = new WebSocket(wsUrl); socket.onopen = () => { - if (isMounted) setStatus('connected'); + if (isMounted) { + setStatus('connected'); + consecutiveFailures = 0; + } }; socket.onclose = () => { if (isMounted) { setStatus('disconnected'); - reconnectTimer = setTimeout(connect, 3000); + consecutiveFailures++; + if (consecutiveFailures < 5) { + reconnectTimer = setTimeout(connect, 3000); + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` around lines 48 - 56, The current handlers (socket.onclose and socket.onerror) cause an infinite reconnect loop when the server rejects auth; modify the logic in connect so socket.onerror does not unconditionally call socket.close(), and change socket.onclose to accept the CloseEvent (e) and bail out of reconnection when the close indicates authentication rejection or when a consecutive-failure limit is exceeded: add a closure-scoped counter like failedAttempts (increment in onclose when close code/reason indicates auth or any non-successful close), reset failedAttempts to 0 in socket.onopen, and only schedule reconnectTimer = setTimeout(connect, 3000) if isMounted && failedAttempts < MAX_FAILED_ATTEMPTS (e.g., 3) and the close code/reason is not an auth failure (check e.code/e.reason for your server's auth code or policy-violation 1008); reference symbols: connect, socket.onclose, socket.onerror, socket.onopen, setStatus, reconnectTimer, isMounted, failedAttempts, MAX_FAILED_ATTEMPTS.
🧹 Nitpick comments (4)
src/server/tokenStore.ts (1)
16-24: No validation on loaded token data — malformed JSON could cause runtime errors.
load()trusts the file contents entirely. A corrupt or tamperedtokens.json(e.g., missingtoken,lastUsed, orcreatedAtfields, or wrong types) would inject invalid entries into the in-memory array, potentially causingtimingSafeEqualto throw orpurgeExpiredto behave incorrectly.Consider adding basic schema validation (e.g., check that each entry has string
tokenand numericlastUsed/createdAt).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tokenStore.ts` around lines 16 - 24, The load() function currently trusts TOKENS_FILE contents; parse the file then validate the shape before assigning to tokens: ensure the top-level value is an array and each entry has a string 'token' and numeric 'lastUsed' and 'createdAt' (or coerce/skip entries that fail validation). Implement this validation inside load() after JSON.parse and before setting the module-level tokens array; drop or filter out malformed entries (or reset to an empty array) and optionally persist the cleaned array back to TOKENS_FILE so timingSafeEqual and purgeExpired only operate on valid entries.src/server/websocket.ts (2)
27-27: Avoidany— keep the original type parameter.Changing
server: Servertoserver: anyremoves type safety. If the originalhttp.Serverimport caused issues, consider usingimport type { Server } from 'http'orimport type { Server } from 'node:http'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 27, The createWsServer function lost type safety by changing the server parameter to any; restore the explicit Server type (e.g., import type { Server } from 'http' or 'node:http') and update the signature back to createWsServer(server: Server) so TypeScript enforces the correct HTTP server shape (adjust imports to use an import type to avoid runtime import issues).
77-78:storeTokenis called on every connection, causing disk I/O on each reconnect.Line 78 calls
storeToken(token)for every connection event (including reconnections).storeTokencallspurgeExpired()+save(), writing to disk each time. For a remote client with intermittent connectivity that reconnects frequently, this is redundant I/O.Consider using
touchTokenhere instead (which only updates in-memory) and relying on the existingstoreTokencall during the upgrade phase for first-time storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 77 - 78, The connection handler currently calls storeToken(token) on every 'connection', causing unnecessary disk writes; change that call to touchToken(token) so only in-memory state is updated on routine reconnects, and ensure the existing storeToken usage during the upgrade/first-storage flow still persists tokens to disk; update the wss.on('connection', ...) callback to call touchToken(token) (not storeToken) and leave storeToken for the upgrade path that handles initial saves.src/routes/settings.tsx (1)
124-151: Consider consolidating the two WebSocket connections into one.The settings page opens two separate WebSocket connections: one for token generation (line 66) and one for IP detection (line 130). Both connect to the same
/wsendpoint from localhost. These could share a single connection to reduce overhead and simplify cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 124 - 151, The component currently opens a dedicated WebSocket in the useEffect that requests the server IP (creating socket, socket.onopen, socket.onmessage, and calling setIp) while another WebSocket is opened elsewhere for token generation; consolidate by creating a single shared WebSocket instance (same /ws URL) and reusing it for both purposes: lift the socket creation out of this useEffect into a shared place/state (or a useRef) used by the token-generation logic and by this IP logic, register a single socket.onmessage handler that dispatches on message.type (e.g., 'server-ip' vs token messages) to call setIp or token handlers, ensure socket.onopen sends any queued messages (or explicitly send the 'get-ip' request when open), and perform a single cleanup that closes the shared socket on unmount to avoid duplicate connections and conflicting handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/settings.tsx`:
- Around line 57-88: The current useEffect returns early if localStorage has
rein_auth_token, causing stale client tokens to persist; instead always open the
WebSocket and request a token so the server can idempotently refresh or issue a
new one. Remove the early-return that checks
localStorage.getItem('rein_auth_token') in the useEffect, keep creating the
WebSocket (wsUrl), send JSON { type: 'generate-token' } on socket.onopen, and in
socket.onmessage handle 'token-generated' by calling setAuthToken(data.token)
and overwriting localStorage.setItem('rein_auth_token', data.token') so expired
tokens are replaced; keep the socket.close() and cleanup logic as-is.
- Around line 85-87: The cleanup returned by the effect currently only closes
the socket when socket.readyState === WebSocket.OPEN, which misses the
CONNECTING state and allows the connection to open later and call onmessage on
an unmounted component; update the effect's cleanup to also close sockets in
WebSocket.CONNECTING (e.g., treat CONNECTING the same as OPEN for closure) and
remove or nullify the socket event handlers (socket.onmessage, socket.onopen,
socket.onerror, socket.onclose) to prevent callbacks after unmount; reference
socket.readyState, WebSocket.CONNECTING, WebSocket.OPEN, and the cleanup
function returned by the effect to locate where to implement these changes.
In `@src/server/tokenStore.ts`:
- Around line 73-81: touchToken currently updates tokens[].lastUsed but never
persists to disk; implement the commented periodic-persist behavior by tracking
a module-level timestamp (e.g., lastPersistedAt) and, inside touchToken(token:
string), after setting entry.lastUsed check if Date.now() - lastPersistedAt >
60_000 and if so call save() and update lastPersistedAt; reference the existing
touchToken function, the tokens array, entry.lastUsed, and the save() function
when adding this throttled persistence logic.
- Line 11: The TOKENS_FILE constant currently uses
path.resolve('./src/tokens.json') which resolves against process.cwd() and is
fragile; change TOKENS_FILE to resolve relative to the module using
import.meta.url (e.g. build the module directory from new URL('.',
import.meta.url) and join with 'tokens.json') so the path is correct in ESM and
production builds; update the TOKENS_FILE definition and any code that reads it
(reference symbol: TOKENS_FILE in tokenStore.ts) to use the
import.meta.url-based resolution.
In `@src/server/websocket.ts`:
- Around line 56-63: The current branch in websocket.ts that accepts any token
when hasTokens() is false must be removed: stop auto-storing arbitrary tokens
via storeToken(token) and do not upgrade the socket for remote clients in that
case. Instead, always validate incoming tokens against the server's generated
token store (replace the hasTokens() bypass with a validation check using the
existing token store) and if no valid token exists (including when hasTokens()
is false) reject the upgrade/close the socket and do not call
wss.emit('connection', ...). Remove the special-case logic that calls
storeToken(token) and ensure the upgrade path only proceeds for tokens already
present/valid in the server token store.
---
Outside diff comments:
In `@src/hooks/useRemoteConnection.ts`:
- Around line 48-56: The current handlers (socket.onclose and socket.onerror)
cause an infinite reconnect loop when the server rejects auth; modify the logic
in connect so socket.onerror does not unconditionally call socket.close(), and
change socket.onclose to accept the CloseEvent (e) and bail out of reconnection
when the close indicates authentication rejection or when a consecutive-failure
limit is exceeded: add a closure-scoped counter like failedAttempts (increment
in onclose when close code/reason indicates auth or any non-successful close),
reset failedAttempts to 0 in socket.onopen, and only schedule reconnectTimer =
setTimeout(connect, 3000) if isMounted && failedAttempts < MAX_FAILED_ATTEMPTS
(e.g., 3) and the close code/reason is not an auth failure (check
e.code/e.reason for your server's auth code or policy-violation 1008); reference
symbols: connect, socket.onclose, socket.onerror, socket.onopen, setStatus,
reconnectTimer, isMounted, failedAttempts, MAX_FAILED_ATTEMPTS.
In `@src/server/websocket.ts`:
- Around line 112-126: The update-config WebSocket handler (the if (msg.type ===
'update-config') block) accepts config changes from any authenticated client;
restrict it to local-only like the generate-token handler does by checking the
same isLocal flag before applying changes. Modify the update-config branch to
first verify isLocal (or call the same isLocal-checking function used by
generate-token), return/send an error message and do not write to
server-config.json if not local, and optionally log the unauthorized attempt;
keep the existing try/catch and success/error response behavior for allowed
requests.
---
Nitpick comments:
In `@src/routes/settings.tsx`:
- Around line 124-151: The component currently opens a dedicated WebSocket in
the useEffect that requests the server IP (creating socket, socket.onopen,
socket.onmessage, and calling setIp) while another WebSocket is opened elsewhere
for token generation; consolidate by creating a single shared WebSocket instance
(same /ws URL) and reusing it for both purposes: lift the socket creation out of
this useEffect into a shared place/state (or a useRef) used by the
token-generation logic and by this IP logic, register a single socket.onmessage
handler that dispatches on message.type (e.g., 'server-ip' vs token messages) to
call setIp or token handlers, ensure socket.onopen sends any queued messages (or
explicitly send the 'get-ip' request when open), and perform a single cleanup
that closes the shared socket on unmount to avoid duplicate connections and
conflicting handlers.
In `@src/server/tokenStore.ts`:
- Around line 16-24: The load() function currently trusts TOKENS_FILE contents;
parse the file then validate the shape before assigning to tokens: ensure the
top-level value is an array and each entry has a string 'token' and numeric
'lastUsed' and 'createdAt' (or coerce/skip entries that fail validation).
Implement this validation inside load() after JSON.parse and before setting the
module-level tokens array; drop or filter out malformed entries (or reset to an
empty array) and optionally persist the cleaned array back to TOKENS_FILE so
timingSafeEqual and purgeExpired only operate on valid entries.
In `@src/server/websocket.ts`:
- Line 27: The createWsServer function lost type safety by changing the server
parameter to any; restore the explicit Server type (e.g., import type { Server }
from 'http' or 'node:http') and update the signature back to
createWsServer(server: Server) so TypeScript enforces the correct HTTP server
shape (adjust imports to use an import type to avoid runtime import issues).
- Around line 77-78: The connection handler currently calls storeToken(token) on
every 'connection', causing unnecessary disk writes; change that call to
touchToken(token) so only in-memory state is updated on routine reconnects, and
ensure the existing storeToken usage during the upgrade/first-storage flow still
persists tokens to disk; update the wss.on('connection', ...) callback to call
touchToken(token) (not storeToken) and leave storeToken for the upgrade path
that handles initial saves.
Rozerxshashank
force-pushed
the
fix/ws-auth-25
branch
from
dfa3972 to
f25e711
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/websocket.ts (2)
112-126:⚠️ Potential issue | 🟠 MajorRemote clients can overwrite server config — missing locality/authorization check.
The
update-confighandler has noisLocalguard, unlikegenerate-token(Line 102). Any authenticated remote client can send anupdate-configmessage to write arbitrary keys intosrc/server-config.json. Depending on what the server reads from this config, this could allow a remote user to alter server behavior in unintended ways.Suggested fix
if (msg.type === 'update-config') { + if (!isLocal) { + ws.send(JSON.stringify({ type: 'config-updated', success: false, error: 'Only localhost can update config' })); + return; + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 112 - 126, The update-config handler (the block handling msg.type === 'update-config' in src/server/websocket.ts) lacks the same locality/authorization check used by the generate-token path, allowing remote clients to overwrite server-config.json; add a guard that verifies the connection is local or the client is authorized (reuse the existing isLocal check/logic used for "generate-token" or the same auth method you use elsewhere), and only perform the fs.writeFileSync when that check passes; if the check fails, send back a JSON response like { type: 'config-updated', success: false, error: 'unauthorized' } and do not write to disk.
128-128:⚠️ Potential issue | 🔴 CriticalAdd input sanitization for
textinput type — currently accepts arbitrary unvalidated text.Remote authenticated clients can execute input commands as intended. However,
InputHandler.handleMessage()has a critical gap: thetextcase (line 156 in InputHandler.ts) directly callskeyboard.type(msg.text)without any validation or sanitization. While most other input types have validation (move hasNumber.isFinite(), zoom hasMAX_ZOOM_STEP, key/combo useKEY_MAPwhitelist), thetextcase accepts arbitrary text, allowing a remote client to inject any string—potentially into terminals, vulnerable applications, or for UI-based attacks.The 10KB size check at the WebSocket layer provides only DoS protection, not payload validation. Add bounds checking and character/pattern validation for the
textinput type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 128, InputHandler.handleMessage currently forwards arbitrary text to keyboard.type (via inputHandler.handleMessage and keyboard.type) without validation; add input sanitization for the "text" case by enforcing a configurable MAX_TEXT_LENGTH (e.g., 1k) and rejecting/triming inputs that exceed it, strip or reject non-printable/control characters (allow only a safe charset such as printable ASCII/Unicode categories you support), and normalize/escape problematic characters before calling keyboard.type; return a clear error/ignore the input when validation fails and log via the existing logger so remote clients cannot inject arbitrary payloads.src/hooks/useRemoteConnection.ts (1)
48-56:⚠️ Potential issue | 🟠 MajorInfinite reconnect loop on persistent 401 rejection.
When the server rejects a token (e.g., expired or revoked), the WebSocket handshake fails →
onerrorfires →socket.close()→onclosefires → reconnect in 3 seconds with the same stale token. This loops indefinitely, generating server-side noise and wasting client resources.Consider either:
- Detecting auth failures (e.g., via a close code/reason) and clearing the stored token / stopping retries.
- Adding exponential backoff with a max retry cap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` around lines 48 - 56, The current socket.onerror -> socket.close() causes onclose to always schedule reconnects which loops on persistent auth failures; update useRemoteConnection to detect authentication failures in socket.onclose (inspect closeEvent.code and closeEvent.reason) and if the server indicates a 401/authorization failure, clear the stored token and stop further reconnects (cancel reconnectTimer and avoid calling connect), otherwise implement exponential backoff in the reconnect logic used by connect (track attempt count in a retryAttempt variable, increase delay up to a max, and cap max attempts) and ensure setStatus('disconnected') is preserved for normal closes; reference socket.onclose, socket.onerror, connect, reconnectTimer, setStatus, and useRemoteConnection when making the changes.
🧹 Nitpick comments (3)
src/server/websocket.ts (2)
27-27:serverparameter typed asany— type safety regression.The AI summary notes the signature changed from
server: Server(fromhttp) toserver: any. This drops type checking on the argument. If the broader typing was relaxed to support both HTTP and HTTPS servers, usehttp.Server | https.Serverornet.Serverinstead.Suggested fix
-export function createWsServer(server: any) { +export function createWsServer(server: http.Server | https.Server) {(Requires importing
httpsif not already present, or useimport { Server } from 'net'for a broader compatible type.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 27, The createWsServer function's parameter is typed as any, causing a type-safety regression; change the signature of createWsServer(server: any) to use a concrete server type such as createWsServer(server: http.Server | https.Server) or createWsServer(server: net.Server) and update imports accordingly (import https or import { Server as NetServer } from 'net' / import { Server as HttpServer } from 'http') so the parameter is strongly typed while keeping compatibility with both HTTP and HTTPS servers; ensure any internal usages of server still accept the chosen type.
21-25:isLocalhostcheck is sound for direct connections but fragile behind reverse proxies.The check covers the standard loopback addresses. If this app is ever deployed behind a reverse proxy (e.g., nginx, Caddy), all connections will appear as localhost, bypassing token auth entirely. This is likely fine for a desktop app, but worth a brief inline comment to document the assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 21 - 25, The isLocalhost(request: IncomingMessage) function currently treats requests from 127.0.0.1/::1/::ffff:127.0.0.1 as trusted which is unsafe behind reverse proxies; add a concise inline comment next to the isLocalhost function documenting the assumption that the app is not behind a reverse proxy and warning that proxies (e.g., nginx/Caddy) will make all connections appear local and bypass token auth, and suggest alternatives (check X-Forwarded-For or remove localhost bypass) so future maintainers know why this check exists and when to change it.src/hooks/useRemoteConnection.ts (1)
12-26: Token persists in browser URL/history after extraction.After reading the token from
window.location.searchand saving it tolocalStorage, the token remains in the address bar, browser history, and could leak viaRefererheaders. Consider stripping it from the URL after extraction:Suggested fix
const urlParams = new URLSearchParams(window.location.search); const urlToken = urlParams.get('token'); const storedToken = localStorage.getItem('rein_auth_token'); const token = urlToken || storedToken; // Persist URL token to localStorage for future reconnections if (urlToken && urlToken !== storedToken) { localStorage.setItem('rein_auth_token', urlToken); } + + // Remove token from URL to prevent leakage via history/Referer + if (urlToken) { + urlParams.delete('token'); + const cleanSearch = urlParams.toString(); + const newUrl = window.location.pathname + (cleanSearch ? `?${cleanSearch}` : '') + window.location.hash; + window.history.replaceState({}, '', newUrl); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` around lines 12 - 26, The token extracted in useRemoteConnection (variables urlToken, storedToken, token) is left in window.location.search — after persisting urlToken to localStorage, remove the token from the address bar and browser history by replacing the current history entry with a new URL that omits the token query parameter (use window.history.replaceState to avoid a reload and preserve other query params and state). Locate the block where urlToken is checked and stored, construct a new URL without the token param, and call history.replaceState(null, '', newUrl) to strip the token while keeping the rest of the app state intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/websocket.ts`:
- Line 94: touchToken currently updates in-memory lastUsed but never persists
it; modify touchToken to track a lastSave timestamp (module-level or inside
TokenStore) and after updating lastUsed call save() only if now - lastSave > 60s
(update lastSave when save completes) so frequent websocket messages won't cause
excessive disk I/O but recent activity is persisted across restarts; reference
touchToken, save, and storeToken (use the same token store instance) and ensure
you only call save when the token was found/updated to avoid unnecessary writes.
---
Outside diff comments:
In `@src/hooks/useRemoteConnection.ts`:
- Around line 48-56: The current socket.onerror -> socket.close() causes onclose
to always schedule reconnects which loops on persistent auth failures; update
useRemoteConnection to detect authentication failures in socket.onclose (inspect
closeEvent.code and closeEvent.reason) and if the server indicates a
401/authorization failure, clear the stored token and stop further reconnects
(cancel reconnectTimer and avoid calling connect), otherwise implement
exponential backoff in the reconnect logic used by connect (track attempt count
in a retryAttempt variable, increase delay up to a max, and cap max attempts)
and ensure setStatus('disconnected') is preserved for normal closes; reference
socket.onclose, socket.onerror, connect, reconnectTimer, setStatus, and
useRemoteConnection when making the changes.
In `@src/server/websocket.ts`:
- Around line 112-126: The update-config handler (the block handling msg.type
=== 'update-config' in src/server/websocket.ts) lacks the same
locality/authorization check used by the generate-token path, allowing remote
clients to overwrite server-config.json; add a guard that verifies the
connection is local or the client is authorized (reuse the existing isLocal
check/logic used for "generate-token" or the same auth method you use
elsewhere), and only perform the fs.writeFileSync when that check passes; if the
check fails, send back a JSON response like { type: 'config-updated', success:
false, error: 'unauthorized' } and do not write to disk.
- Line 128: InputHandler.handleMessage currently forwards arbitrary text to
keyboard.type (via inputHandler.handleMessage and keyboard.type) without
validation; add input sanitization for the "text" case by enforcing a
configurable MAX_TEXT_LENGTH (e.g., 1k) and rejecting/triming inputs that exceed
it, strip or reject non-printable/control characters (allow only a safe charset
such as printable ASCII/Unicode categories you support), and normalize/escape
problematic characters before calling keyboard.type; return a clear error/ignore
the input when validation fails and log via the existing logger so remote
clients cannot inject arbitrary payloads.
---
Duplicate comments:
In `@src/server/websocket.ts`:
- Around line 56-63: The block that accepts and stores any incoming token when
hasTokens() is false must be removed: delete the if (!hasTokens()) {
storeToken(token); wss.handleUpgrade(...); wss.emit('connection', ...) return; }
branch so the server never auto-registers arbitrary remote tokens; instead
ensure incoming upgrades are rejected when no server-generated tokens exist
(i.e., do not call storeToken or emit connection), and keep the existing
generate-token message handler restriction (the "generate-token" handler) as the
only path for creating tokens from localhost. Locate references to hasTokens(),
storeToken, wss.handleUpgrade and wss.emit in websocket.ts to apply this change.
---
Nitpick comments:
In `@src/hooks/useRemoteConnection.ts`:
- Around line 12-26: The token extracted in useRemoteConnection (variables
urlToken, storedToken, token) is left in window.location.search — after
persisting urlToken to localStorage, remove the token from the address bar and
browser history by replacing the current history entry with a new URL that omits
the token query parameter (use window.history.replaceState to avoid a reload and
preserve other query params and state). Locate the block where urlToken is
checked and stored, construct a new URL without the token param, and call
history.replaceState(null, '', newUrl) to strip the token while keeping the rest
of the app state intact.
In `@src/server/websocket.ts`:
- Line 27: The createWsServer function's parameter is typed as any, causing a
type-safety regression; change the signature of createWsServer(server: any) to
use a concrete server type such as createWsServer(server: http.Server |
https.Server) or createWsServer(server: net.Server) and update imports
accordingly (import https or import { Server as NetServer } from 'net' / import
{ Server as HttpServer } from 'http') so the parameter is strongly typed while
keeping compatibility with both HTTP and HTTPS servers; ensure any internal
usages of server still accept the chosen type.
- Around line 21-25: The isLocalhost(request: IncomingMessage) function
currently treats requests from 127.0.0.1/::1/::ffff:127.0.0.1 as trusted which
is unsafe behind reverse proxies; add a concise inline comment next to the
isLocalhost function documenting the assumption that the app is not behind a
reverse proxy and warning that proxies (e.g., nginx/Caddy) will make all
connections appear local and bypass token auth, and suggest alternatives (check
X-Forwarded-For or remove localhost bypass) so future maintainers know why this
check exists and when to change it.
3e2047e to
8744195
Compare
imxade
reviewed
Feb 17, 2026
Rozerxshashank
force-pushed
the
fix/ws-auth-25
branch
from
8744195 to
ef8008a
Compare
Contributor
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/server/websocket.ts (1)
110-124:⚠️ Potential issue | 🟠 MajorGate
update-configbehindisLocalto match the protection level ofgenerate-token.
generate-tokenrequiresisLocal(line 94), butupdate-configaccepts writes from any authenticated remote client. This allows remote clients with valid tokens to arbitrarily modifyserver-config.json, potentially altering server behavior. The asymmetry creates a security gap.Apply the same
isLocalcheck toupdate-config:🛡️ Proposed fix
if (msg.type === 'update-config') { + if (!isLocal) { + ws.send(JSON.stringify({ type: 'config-updated', success: false, error: 'Only localhost can update config' })); + return; + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 110 - 124, The update-config message handler currently allows any authenticated client to modify server-config.json; mirror the protection used for generate-token by gating the 'update-config' branch with the same isLocal check: inside the message handling for msg.type === 'update-config', verify isLocal (the same boolean used for generate-token) and if not local respond with an error/failure message, otherwise proceed with reading, merging, and writing server-config.json and sending the config-updated success message; ensure the error path still logs and returns the config-updated failure response when an exception occurs.
🧹 Nitpick comments (7)
src/server/tokenStore.ts (4)
31-41: Synchronous file write blocks the event loop.
writeFileSyncblocks the Node.js event loop when it fires. For a desktop app with a small token file this is unlikely to cause noticeable lag, but if you ever want to harden this,fs.writeFile(async) with a write-in-progress guard would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tokenStore.ts` around lines 31 - 41, The save function currently blocks the event loop by using fs.writeFileSync; change it to use fs.writeFile (async) and add a write-in-progress guard (e.g., a boolean like writeInProgress) to prevent concurrent writes, keeping the existing throttle using SAVE_THROTTLE_MS and the force parameter; on successful async write update lastSaveTime and clear the guard, on error log the error and clear the guard, and ensure TOKENS_FILE and tokens are serialized with JSON.stringify as before so callers of save behave non-blockingly.
91-98:getActiveTokenreturns the plaintext token — ensure it is not logged or leaked.The returned token is sent over the WebSocket in
websocket.tsline 106. Ensure no middleware or logging layer inadvertently records the token value in logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tokenStore.ts` around lines 91 - 98, getActiveToken currently returns the plaintext token which can be sent over the WebSocket and potentially logged; change the implementation to avoid returning raw secrets — return a non-sensitive identifier or a masked token (e.g., last4 or hash) instead and update callers (notably the WebSocket send in websocket.ts around the send at line ~106) to use that safe value; additionally ensure any logging/middleware filters out or redacts token fields (add a redact filter for keys like "token" or "authToken") so the raw token is never emitted to logs or telemetry.
35-37: Token file written with default permissions — other OS users can read it.
writeFileSynccreatestokens.jsonwith default permissions (typically0o644on Unix), meaning any user on the system can read the authentication tokens. On a shared machine, this could allow another user to extract a valid token.Consider restricting file permissions:
🔒 Proposed fix — restrict file permissions
try { - fs.writeFileSync(TOKENS_FILE, JSON.stringify(tokens, null, 2)); + fs.writeFileSync(TOKENS_FILE, JSON.stringify(tokens, null, 2), { mode: 0o600 }); lastSaveTime = now; } catch (e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tokenStore.ts` around lines 35 - 37, The token file is being written with default (world-readable) permissions; update the write to explicitly restrict permissions and harden existing files by using fs.writeFileSync with the mode option (e.g., { mode: 0o600 }) when writing TOKENS_FILE, and after writing call fs.chmodSync(TOKENS_FILE, 0o600) to ensure previously-created files are corrected; locate the write in tokenStore.ts (the try block that calls fs.writeFileSync and sets lastSaveTime) and apply these changes so tokens.json is only user-readable/writable.
21-29:load()does not validate the parsed JSON structure.If
tokens.jsonis corrupted or tampered with (e.g., contains non-array data, entries missingtoken/lastUsedfields, or entries with wrong types), the module silently loads malformed data. Downstream calls totimingSafeEqual(t.token, ...)or comparisons ont.lastUsedwould then throw at runtime.Consider adding basic schema validation after parsing:
🛡️ Proposed fix — validate loaded data
function load(): void { try { if (fs.existsSync(TOKENS_FILE)) { - tokens = JSON.parse(fs.readFileSync(TOKENS_FILE, 'utf-8')); + const data = JSON.parse(fs.readFileSync(TOKENS_FILE, 'utf-8')); + if (Array.isArray(data)) { + tokens = data.filter( + (t): t is TokenEntry => + typeof t === 'object' && t !== null && + typeof t.token === 'string' && + typeof t.createdAt === 'number' && + typeof t.lastUsed === 'number' + ); + } else { + tokens = []; + } } } catch { tokens = []; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tokenStore.ts` around lines 21 - 29, The load() function should validate the parsed JSON from TOKENS_FILE before assigning to tokens: after JSON.parse but inside load(), ensure the root value is an array and each element has the required properties (e.g., t.token is a string or Buffer-representable value and t.lastUsed is a number), otherwise treat the file as invalid (reset tokens = []), log the problem and avoid assigning malformed data so later calls like timingSafeEqual(t.token, ...) or numeric comparisons on t.lastUsed cannot throw; implement this validation inside load() (and keep TOKENS_FILE, tokens, and timingSafeEqual references intact) and surface/log parsing/validation errors.src/server/websocket.ts (3)
74-86:touchTokenis called after JSON parsing but before message-type dispatch — consider its placement.Currently
touchToken(token)runs for every parsed message regardless of type, includingget-ipandgenerate-token. This is fine for the throttled save mechanism, but note thattouchTokenperforms an O(n) timing-safe comparison scan on every message. For most deployments with a small token list this is negligible, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 74 - 86, touchToken is currently invoked for every parsed message inside the ws.on('message') handler which forces an O(n) timing-safe scan on every message (including unauthenticated types like "get-ip" and "generate-token"); restrict calls to touchToken(token) so it only runs for message types that require token authentication. Move or guard the touchToken call to occur after JSON.parse and after you inspect msg.type (or inside the authenticated-case branches in your dispatch logic), e.g., only invoke touchToken when token is present AND msg.type is not "get-ip" or "generate-token" (or when handling auth-protected handlers), leaving MAX_PAYLOAD_SIZE checks and JSON parsing as-is.
27-27:server: anyweakens type safety — preferhttp.Server.The AI summary notes this was changed from
Servertoany. The original typed parameter was safer.♻️ Restore type annotation
-export function createWsServer(server: any) { +export function createWsServer(server: import('http').Server) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 27, The createWsServer function's parameter is currently typed as any which reduces type safety; change the signature export function createWsServer(server: any) to accept the proper HTTP server type (e.g., http.Server or Node's Server) so callers and internals get correct typings; update the parameter type in createWsServer and add the necessary import or fully qualified type reference for http.Server, ensuring any usages inside createWsServer (and tests) still compile against the stronger type.
69-70: Localhost connections can store arbitrary tokens via line 70.When a localhost client connects with a
tokenquery param (e.g.,ws://localhost:PORT/ws?token=anything), that token is stored unconditionally at line 70 without validation. Since localhost is fully trusted by design, this is acceptable in the current threat model. However, it means any process on the local machine can inject tokens into the store.If you want to tighten this: only call
storeTokenfor tokens that were either generated by the server or already known.♻️ Optional: only store tokens that are already known or were just generated
- wss.on('connection', (ws: WebSocket, _request: IncomingMessage, token: string | null, isLocal: boolean) => { - if (token) storeToken(token); + wss.on('connection', (ws: WebSocket, _request: IncomingMessage, token: string | null, isLocal: boolean) => { + if (token && !isLocal) storeToken(token); // Remote tokens already validated; localhost tokens stored via generate-token flow🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 69 - 70, The current connection handler (wss.on('connection')) unconditionally calls storeToken(token) for localhost connections, allowing any local process to inject tokens; change it to only call storeToken when the token is either already known in the server's token store or was generated by the server (e.g., check existingTokenStore.has(token) or verifyTokenGenerated(token) before storing). Update the connection callback around the token parameter handling to perform a predicate like isKnownOrServerGenerated(token) and only then call storeToken(token); keep the existing behavior for legitimately generated tokens but reject or ignore arbitrary tokens from local clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/websocket.ts`:
- Around line 110-124: The update-config message handler currently allows any
authenticated client to modify server-config.json; mirror the protection used
for generate-token by gating the 'update-config' branch with the same isLocal
check: inside the message handling for msg.type === 'update-config', verify
isLocal (the same boolean used for generate-token) and if not local respond with
an error/failure message, otherwise proceed with reading, merging, and writing
server-config.json and sending the config-updated success message; ensure the
error path still logs and returns the config-updated failure response when an
exception occurs.
---
Nitpick comments:
In `@src/server/tokenStore.ts`:
- Around line 31-41: The save function currently blocks the event loop by using
fs.writeFileSync; change it to use fs.writeFile (async) and add a
write-in-progress guard (e.g., a boolean like writeInProgress) to prevent
concurrent writes, keeping the existing throttle using SAVE_THROTTLE_MS and the
force parameter; on successful async write update lastSaveTime and clear the
guard, on error log the error and clear the guard, and ensure TOKENS_FILE and
tokens are serialized with JSON.stringify as before so callers of save behave
non-blockingly.
- Around line 91-98: getActiveToken currently returns the plaintext token which
can be sent over the WebSocket and potentially logged; change the implementation
to avoid returning raw secrets — return a non-sensitive identifier or a masked
token (e.g., last4 or hash) instead and update callers (notably the WebSocket
send in websocket.ts around the send at line ~106) to use that safe value;
additionally ensure any logging/middleware filters out or redacts token fields
(add a redact filter for keys like "token" or "authToken") so the raw token is
never emitted to logs or telemetry.
- Around line 35-37: The token file is being written with default
(world-readable) permissions; update the write to explicitly restrict
permissions and harden existing files by using fs.writeFileSync with the mode
option (e.g., { mode: 0o600 }) when writing TOKENS_FILE, and after writing call
fs.chmodSync(TOKENS_FILE, 0o600) to ensure previously-created files are
corrected; locate the write in tokenStore.ts (the try block that calls
fs.writeFileSync and sets lastSaveTime) and apply these changes so tokens.json
is only user-readable/writable.
- Around line 21-29: The load() function should validate the parsed JSON from
TOKENS_FILE before assigning to tokens: after JSON.parse but inside load(),
ensure the root value is an array and each element has the required properties
(e.g., t.token is a string or Buffer-representable value and t.lastUsed is a
number), otherwise treat the file as invalid (reset tokens = []), log the
problem and avoid assigning malformed data so later calls like
timingSafeEqual(t.token, ...) or numeric comparisons on t.lastUsed cannot throw;
implement this validation inside load() (and keep TOKENS_FILE, tokens, and
timingSafeEqual references intact) and surface/log parsing/validation errors.
In `@src/server/websocket.ts`:
- Around line 74-86: touchToken is currently invoked for every parsed message
inside the ws.on('message') handler which forces an O(n) timing-safe scan on
every message (including unauthenticated types like "get-ip" and
"generate-token"); restrict calls to touchToken(token) so it only runs for
message types that require token authentication. Move or guard the touchToken
call to occur after JSON.parse and after you inspect msg.type (or inside the
authenticated-case branches in your dispatch logic), e.g., only invoke
touchToken when token is present AND msg.type is not "get-ip" or
"generate-token" (or when handling auth-protected handlers), leaving
MAX_PAYLOAD_SIZE checks and JSON parsing as-is.
- Line 27: The createWsServer function's parameter is currently typed as any
which reduces type safety; change the signature export function
createWsServer(server: any) to accept the proper HTTP server type (e.g.,
http.Server or Node's Server) so callers and internals get correct typings;
update the parameter type in createWsServer and add the necessary import or
fully qualified type reference for http.Server, ensuring any usages inside
createWsServer (and tests) still compile against the stronger type.
- Around line 69-70: The current connection handler (wss.on('connection'))
unconditionally calls storeToken(token) for localhost connections, allowing any
local process to inject tokens; change it to only call storeToken when the token
is either already known in the server's token store or was generated by the
server (e.g., check existingTokenStore.has(token) or verifyTokenGenerated(token)
before storing). Update the connection callback around the token parameter
handling to perform a predicate like isKnownOrServerGenerated(token) and only
then call storeToken(token); keep the existing behavior for legitimately
generated tokens but reject or ignore arbitrary tokens from local clients.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/websocket.ts (1)
118-132: Consider restrictingupdate-configto localhost-only connections.Now that remote clients are authenticated and can send arbitrary messages, any mobile device that scanned the QR code can overwrite
server-config.jsonviaupdate-config. There's also no validation onmsg.configkeys/values. Since config changes affect server behavior, this should likely be a localhost-only operation — similar togenerate-token.Suggested guard
if (msg.type === 'update-config') { + if (!isLocal) { + ws.send(JSON.stringify({ type: 'config-updated', success: false, error: 'Only localhost can update config' })); + return; + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 118 - 132, The update-config handler currently accepts arbitrary remote messages and writes to server-config.json without validation; restrict this to localhost-only connections and validate incoming config keys/values. Before processing msg.type === 'update-config' (same block that writes to configPath './src/server-config.json'), check the WebSocket peer address (e.g., ws._socket.remoteAddress or equivalent) and only allow requests from '127.0.0.1' and '::1' (or mirror the same guard used for generate-token); if not localhost, return an error message. Additionally, implement a whitelist of allowed config keys and basic value validation for msg.config (or use a small JSON schema) and merge only permitted keys into newConfig before writing to disk and sending the config-updated response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/websocket.ts`:
- Around line 118-132: The update-config handler currently accepts arbitrary
remote messages and writes to server-config.json without validation; restrict
this to localhost-only connections and validate incoming config keys/values.
Before processing msg.type === 'update-config' (same block that writes to
configPath './src/server-config.json'), check the WebSocket peer address (e.g.,
ws._socket.remoteAddress or equivalent) and only allow requests from '127.0.0.1'
and '::1' (or mirror the same guard used for generate-token); if not localhost,
return an error message. Additionally, implement a whitelist of allowed config
keys and basic value validation for msg.config (or use a small JSON schema) and
merge only permitted keys into newConfig before writing to disk and sending the
config-updated response.
Contributor
|
if the token is stored in RAM on the PC, then every time the application restarts: • A new token is generated That can feel inconvenient for daily user? What's your view on this @imxade should we look for other method of authentication? or am I interpreting it wrong? |
aniket866
reviewed
Feb 17, 2026
Contributor
aniket866
left a comment
aniket866
left a comment
There was a problem hiding this comment.
waiting for clarification
Contributor
Author
That's exactly why I've implemented file-based persistence in this PR. If you check tokenStore.ts, you'll see:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #25
Additional Notes:
crypto.randomUUID()and timing-safe validation.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores