feat(frontend): Implement issues #549, #548, #538, #541 - Auth, WebSo…#675
Conversation
…t-Foundation#548, OpenKnight-Foundation#538, OpenKnight-Foundation#541 - Auth, WebSocket scaling, and PGN archival - Issue OpenKnight-Foundation#549: Secure session management with JWT and Refresh Tokens * Created AuthContext with JWT token management * Implemented login, register, logout with refresh token rotation * Added SessionManager component for viewing/managing active sessions * Auto-refresh tokens before expiration * Session tracking with IP address and user agent - Issue OpenKnight-Foundation#548 & OpenKnight-Foundation#538: Websocket scaling with horizontal load balancing * Created useEnhancedChessSocket hook with reconnection token support * Seamless reconnection across multiple server instances * Real-time latency monitoring with ping/pong * Connection status indicator component * Improved exponential backoff with jitter * Move queuing during disconnection - Issue OpenKnight-Foundation#541: Automated PGN archiving to IPFS/Arweave * Created PgnExportArchive component for game export * Download PGN as file or copy to clipboard * Archive to decentralized storage (IPFS & Arweave) * View archived games on IPFS gateway and Arweave explorer * PGN integrity verification with SHA-256 hashing All implementations follow Next.js best practices and TypeScript standards. CI checks pass: npm run build successful. Closes OpenKnight-Foundation#549 Closes OpenKnight-Foundation#548 Closes OpenKnight-Foundation#538 Closes OpenKnight-Foundation#541
📝 WalkthroughWalkthroughThis PR introduces frontend infrastructure for authentication, secure WebSocket communication, session management, and PGN game archiving. It adds an auth context with JWT token management and refresh scheduling, an enhanced chess WebSocket hook with reconnection and latency tracking, components for displaying connection status and managing sessions, and a PGN export/archive component supporting IPFS and Arweave decentralized storage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Browser Client
participant Hook as useEnhancedChessSocket
participant WS as WebSocket Server
participant Queue as Move Queue
User->>Client: sendMove(move)
Client->>Hook: Check connection status
alt Connection Open
Hook->>WS: Send move message
WS-->>Hook: Acknowledge
else Connection Closed/Failed
Hook->>Queue: Queue move
Hook->>Hook: Schedule reconnect (exponential backoff)
end
WS->>Hook: Send ping (periodic)
Hook->>Hook: Measure latency (ping/pong)
Hook->>Hook: Update status "connected"
alt Unexpected Disconnect
WS--xHook: Connection lost
Hook->>Hook: Increment reconnect attempts
Hook->>Hook: Calculate backoff delay
Hook->>Hook: Schedule reconnect with delay
Note over Hook: Attempt N: 2^N seconds (capped)
end
Hook->>WS: Reconnect with stored token
WS-->>Hook: Reconnect successful
Hook->>Queue: Dispatch queued moves
Hook->>Hook: Reset reconnect counter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
frontend/hook/useEnhancedChessSocket.ts (2)
305-328: Redundant cleanupuseEffect.The
useEffectat Line 305 already returns a cleanup that clears the reconnect timeout and closes the socket whengameIdchanges or the component unmounts. The second effect at Line 321-328 duplicates that behavior and can be removed. Less code + no risk of double-close during teardown.♻️ Proposed removal
- useEffect(() => { - return () => { - clearReconnectTimeout(); - if (wsRef.current) { - wsRef.current.close(); - } - }; - }, [clearReconnectTimeout]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hook/useEnhancedChessSocket.ts` around lines 305 - 328, The second cleanup useEffect is redundant because the earlier useEffect that depends on gameId (which calls createWebSocket) already sets isManualDisconnectRef.current, calls clearReconnectTimeout, and closes the socket; remove the duplicate effect (the one that returns a cleanup which calls clearReconnectTimeout and wsRef.current.close()) to avoid double-close and simplify teardown; keep the first useEffect that handles gameId changes and the setStatus/setLastOpponentMove fallback for when gameId is falsy.
97-146: MissingstartLatencyMonitoringincreateWebSocketdeps; also consider moving the ping interval so its cleanup is deterministic.
createWebSocket'suseCallbackdeps are[gameId, calculateReconnectDelay]but the body callsstartLatencyMonitoring(ws).react-hooks/exhaustive-depswill flag this; it only works today becausestartLatencyMonitoringhas empty deps and is stable.Separately, the ping interval is cleaned up via a
closelistener added after settingoncloseon the same ws, which is fine today, but tracking the interval id in a ref (and clearing it ondisconnect/unmount alongsidereconnectTimeoutRef) is more robust against leaks.Also applies to: 242-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hook/useEnhancedChessSocket.ts` around lines 97 - 146, createWebSocket uses startLatencyMonitoring but doesn't include it in its useCallback deps and the ping interval is not tracked robustly; add startLatencyMonitoring to the dependency array (or memoize it with useCallback/useRef so it's stable) so react-hooks/exhaustive-deps is satisfied, and move the ping interval id into a ref (e.g., pingIntervalRef) so you can clear the interval deterministically on disconnect/onunmount and when clearing reconnectTimeoutRef; update the websocket cleanup logic (including the onclose handler and any manual close paths) to clear pingIntervalRef and reconnectTimeoutRef to avoid leaks.frontend/components/chess/ConnectionStatusIndicator.tsx (1)
16-47: LGTM — optional: make the switch exhaustive onEnhancedSocketStatus.The
"idle"case fromEnhancedSocketStatusis silently handled viadefault. If a new status is added to the union (e.g.,"closed"), both switches will still compile and quietly render "Idle"/gray. Adding an explicitcase "idle":and anever-typed default would give you compile-time exhaustiveness:♻️ Proposed tweak
const getStatusText = () => { switch (status) { case "connected": return "Connected"; case "connecting": return "Connecting..."; case "reconnecting": return `Reconnecting... (${reconnectAttempts})`; case "disconnected": return "Disconnected"; case "error": return "Connection Error"; - default: + case "idle": return "Idle"; + default: { + const _exhaustive: never = status; + return _exhaustive; + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/chess/ConnectionStatusIndicator.tsx` around lines 16 - 47, Add an explicit "idle" case to both getStatusColor and getStatusText (matching the EnhancedSocketStatus union) and replace the generic default with an exhaustive-check pattern: after the switch, assign status to a never-typed variable (e.g., const _exhaustiveCheck: never = status) and either throw or return a fallback using that variable so the compiler errors if a new status is added; update references in getStatusText to still use reconnectAttempts where needed (reconnecting case) and keep the same returned strings/classes for existing cases.frontend/components/profile/SessionManager.tsx (1)
51-62: Consider a custom modal instead ofwindow.confirm.
confirm()blocks the main thread, is styled inconsistently across browsers, and cannot be tested reliably in JSDOM. Since this action destructively logs the user out of every device, a project-level confirmation dialog would give better UX and a deterministic test surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/profile/SessionManager.tsx` around lines 51 - 62, Replace the blocking window.confirm usage in handleRevokeAllSessions with the app's non-blocking confirmation modal: add local state (e.g., showRevokeConfirm) and render the project-level Confirm/Modal component, trigger showRevokeConfirm when the user initiates revoke, and only call revokeAllSessions() if the modal returns a confirmed result; keep the existing try/catch and setError behavior unchanged and reference handleRevokeAllSessions and revokeAllSessions when wiring the confirm callback.frontend/context/authContext.tsx (1)
265-280: Minor: validate token shape before decoding.If
tokenis not a standardheader.payload.signaturestring,token.split(".")[1]isundefinedand.replacethrows — currently swallowed by the catch, but a quick shape check makes intent explicit and avoids relying on exception handling for control flow.♻️ Suggested tweak
function decodeJWT(token: string) { try { - const base64Url = token.split(".")[1]; + const parts = token.split("."); + if (parts.length < 2) return null; + const base64Url = parts[1]; const base64 = base64Url.replace(/-/g, "+").replace(/_/g, "/");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/context/authContext.tsx` around lines 265 - 280, The decodeJWT function currently assumes token has three dot-separated parts and calls replace on token.split(".")[1], which can be undefined; update decodeJWT to first validate the token shape (e.g., ensure token is a non-empty string and token.split(".").length >= 3 and the payload part is truthy) and early-return null (and optionally log a warning) when the shape is invalid before attempting the base64 manipulations; keep the rest of the decoding logic intact and reference the decodeJWT function to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/.eslintrc.json`:
- Around line 1-6: Delete the legacy ESLint config file that contains the
"extends": ["next/core-web-vitals","next/typescript"] entry because it's ignored
when a flat config exists; remove the entire .eslintrc.json file so developers
won't edit a dead config and rely on the active eslint.config.mjs instead.
In `@frontend/components/chess/PgnExportArchive.tsx`:
- Around line 121-135: Both viewOnIpfs and viewOnArweave use window.open without
sandboxing, enabling reverse-tabnabbing; modify each to call window.open with
the third argument 'noopener,noreferrer' (e.g. window.open(url, '_blank',
'noopener,noreferrer')) and, if window.open returns a window object, set
newWin.opener = null to cover older browsers (update functions viewOnIpfs and
viewOnArweave accordingly).
In `@frontend/context/authContext.tsx`:
- Around line 54-63: The effect in AuthProvider reads localStorage and calls
JSON.parse(storedUser) without protection which will throw on malformed JSON;
wrap the parse in a try/catch inside the useEffect (the block that reads
storedToken/storedUser) and on parse error clear the broken entry
(localStorage.removeItem("user")), optionally remove the access token if you
want to force re-login, and ensure you call setUser(null) and
setIsLoading(false) so the provider doesn't crash; keep
setAccessToken(storedToken) only when token exists and parsing succeeds or after
handling the error.
- Around line 66-82: The useEffect watching accessToken (the effect that decodes
the token via decodeJWT and computes expiresIn/refreshTime) fails to handle
refreshTime <= 0 and omits refreshToken from deps; update the effect in
useEffect to call refreshToken immediately when refreshTime <= 0 (so
near-expired tokens are refreshed on load) and add refreshToken to the
dependency array (e.g., useEffect(..., [accessToken, refreshToken])); keep
existing logic for scheduling setTimeout when refreshTime > 0 and ensure you
still clear the timeout on cleanup.
- Around line 107-114: The code stores access tokens in localStorage creating an
XSS risk; remove all uses of localStorage for the access token and rely on React
state + httpOnly refresh cookie and the existing refreshToken()/auto-refresh
logic. Specifically, delete any localStorage.setItem("access_token", ...) calls
found around the auth initialization, login/handleLogin, and refresh flows
(references: the functions/variables handling setAuth/state updates and
refreshToken()), also remove localStorage.getItem("access_token") from the
initial state bootstrap, and ensure auth state is initialized only from React
state/refresh flow; keep the existing refreshToken() call (which uses
credentials: "include") and auto-refresh scheduling 5 minutes before expiry.
Finally, add/enable a strict Content Security Policy (CSP) header to mitigate
XSS as an additional security layer.
In `@frontend/hook/useEnhancedChessSocket.ts`:
- Around line 291-303: When performing a manual reconnect or creating a new
socket from sendMove, race occurs because the old socket's event handlers can
later trigger a reconnect; fix by: in reconnect() (and before calling
createWebSocket(true) in sendMove), first call clearReconnectTimeout(), then if
(wsRef.current) detach the old handlers by setting wsRef.current.onclose =
wsRef.current.onerror = wsRef.current.onmessage = wsRef.current.onopen = () =>
{} (or equivalent no-op), then call wsRef.current.close(), set wsRef.current =
null, and only after that call createWebSocket(true); this prevents the old
socket's onclose handler (which references isManualDisconnectRef and
reconnectTimeoutRef) from scheduling a duplicate reconnect.
- Around line 63-66: The hook currently accepts accessToken but never uses it,
so modify useEnhancedChessSocket to append the accessToken (e.g., as a query
param or header-equivalent in the WS URL) when building the socket URL in
createWebSocket; also fix the reconnect race by having reconnect() set
isManualDisconnectRef in a way that prevents the old socket's onclose from
triggering an automatic reconnection (for example set a temporary flag or close
the socket after marking manual disconnect and only unset after new socket is
created), ensure sendMove clears any pending reconnectTimeoutRef before calling
createWebSocket(true) to avoid duplicate reconnections, and include
startLatencyMonitoring in the dependency array of the createWebSocket
useCallback so deps are exhaustive.
---
Nitpick comments:
In `@frontend/components/chess/ConnectionStatusIndicator.tsx`:
- Around line 16-47: Add an explicit "idle" case to both getStatusColor and
getStatusText (matching the EnhancedSocketStatus union) and replace the generic
default with an exhaustive-check pattern: after the switch, assign status to a
never-typed variable (e.g., const _exhaustiveCheck: never = status) and either
throw or return a fallback using that variable so the compiler errors if a new
status is added; update references in getStatusText to still use
reconnectAttempts where needed (reconnecting case) and keep the same returned
strings/classes for existing cases.
In `@frontend/components/profile/SessionManager.tsx`:
- Around line 51-62: Replace the blocking window.confirm usage in
handleRevokeAllSessions with the app's non-blocking confirmation modal: add
local state (e.g., showRevokeConfirm) and render the project-level Confirm/Modal
component, trigger showRevokeConfirm when the user initiates revoke, and only
call revokeAllSessions() if the modal returns a confirmed result; keep the
existing try/catch and setError behavior unchanged and reference
handleRevokeAllSessions and revokeAllSessions when wiring the confirm callback.
In `@frontend/context/authContext.tsx`:
- Around line 265-280: The decodeJWT function currently assumes token has three
dot-separated parts and calls replace on token.split(".")[1], which can be
undefined; update decodeJWT to first validate the token shape (e.g., ensure
token is a non-empty string and token.split(".").length >= 3 and the payload
part is truthy) and early-return null (and optionally log a warning) when the
shape is invalid before attempting the base64 manipulations; keep the rest of
the decoding logic intact and reference the decodeJWT function to locate the
change.
In `@frontend/hook/useEnhancedChessSocket.ts`:
- Around line 305-328: The second cleanup useEffect is redundant because the
earlier useEffect that depends on gameId (which calls createWebSocket) already
sets isManualDisconnectRef.current, calls clearReconnectTimeout, and closes the
socket; remove the duplicate effect (the one that returns a cleanup which calls
clearReconnectTimeout and wsRef.current.close()) to avoid double-close and
simplify teardown; keep the first useEffect that handles gameId changes and the
setStatus/setLastOpponentMove fallback for when gameId is falsy.
- Around line 97-146: createWebSocket uses startLatencyMonitoring but doesn't
include it in its useCallback deps and the ping interval is not tracked
robustly; add startLatencyMonitoring to the dependency array (or memoize it with
useCallback/useRef so it's stable) so react-hooks/exhaustive-deps is satisfied,
and move the ping interval id into a ref (e.g., pingIntervalRef) so you can
clear the interval deterministically on disconnect/onunmount and when clearing
reconnectTimeoutRef; update the websocket cleanup logic (including the onclose
handler and any manual close paths) to clear pingIntervalRef and
reconnectTimeoutRef to avoid leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ccf3ddf-1f4e-4cba-b59b-ebe77e042832
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
frontend/.eslintrc.jsonfrontend/components/chess/ConnectionStatusIndicator.tsxfrontend/components/chess/PgnExportArchive.tsxfrontend/components/profile/SessionManager.tsxfrontend/context/authContext.tsxfrontend/hook/useEnhancedChessSocket.ts
| { | ||
| "extends": [ | ||
| "next/core-web-vitals", | ||
| "next/typescript" | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that eslint.config.mjs exists and show both configs for comparison
echo "=== Checking for ESLint flat config ==="
if [ -f "frontend/eslint.config.mjs" ]; then
echo "✓ frontend/eslint.config.mjs exists (ACTIVE config for ESLint v9+)"
echo ""
echo "Contents:"
cat frontend/eslint.config.mjs
else
echo "✗ frontend/eslint.config.mjs not found"
fi
echo ""
echo "=== Checking ESLint version ==="
cd frontend && npm list eslint --depth=0 2>/dev/null || echo "Could not determine ESLint version"Repository: NOVUS-X/XLMate
Length of output: 693
🏁 Script executed:
# Check if .eslintrc.json exists
ls -la frontend/.eslintrc.json 2>&1Repository: NOVUS-X/XLMate
Length of output: 129
🏁 Script executed:
# Check ESLint version from package.json
cat frontend/package.json | grep -A 5 '"eslint"' || echo "eslint not found in package.json"Repository: NOVUS-X/XLMate
Length of output: 168
🏁 Script executed:
# List all eslint config files in frontend directory
find frontend -maxdepth 1 -name ".eslintrc*" -o -name "eslint.config.*" | sortRepository: NOVUS-X/XLMate
Length of output: 109
Remove this file—it's dead code ignored by ESLint v9+.
The frontend/eslint.config.mjs file already exists and configures the same extends. When ESLint v9+ detects a flat config file (eslint.config.mjs), it completely ignores legacy .eslintrc.json files. This creates confusion, as developers might edit this file expecting changes to take effect, but they won't. Delete frontend/.eslintrc.json entirely—the configuration is already present and active in the flat config file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/.eslintrc.json` around lines 1 - 6, Delete the legacy ESLint config
file that contains the "extends": ["next/core-web-vitals","next/typescript"]
entry because it's ignored when a flat config exists; remove the entire
.eslintrc.json file so developers won't edit a dead config and rely on the
active eslint.config.mjs instead.
| const viewOnIpfs = useCallback(() => { | ||
| if (archiveResult?.ipfs_cid) { | ||
| const ipfsGateway = process.env.NEXT_PUBLIC_IPFS_GATEWAY ?? "https://gateway.pinata.cloud"; | ||
| window.open(`${ipfsGateway}/ipfs/${archiveResult.ipfs_cid}`, "_blank"); | ||
| } | ||
| }, [archiveResult]); | ||
|
|
||
| /** | ||
| * View PGN on Arweave explorer | ||
| */ | ||
| const viewOnArweave = useCallback(() => { | ||
| if (archiveResult?.arweave_tx_id) { | ||
| window.open(`https://arweave.net/${archiveResult.arweave_tx_id}`, "_blank"); | ||
| } | ||
| }, [archiveResult]); |
There was a problem hiding this comment.
Harden window.open with noopener,noreferrer.
Without noopener, the opened page can access window.opener and navigate your tab (reverse tabnabbing) in older browsers. This is especially relevant for links to third-party gateways (Pinata, arweave.net).
🛡️ Proposed fix
- window.open(`${ipfsGateway}/ipfs/${archiveResult.ipfs_cid}`, "_blank");
+ window.open(
+ `${ipfsGateway}/ipfs/${archiveResult.ipfs_cid}`,
+ "_blank",
+ "noopener,noreferrer",
+ );
...
- window.open(`https://arweave.net/${archiveResult.arweave_tx_id}`, "_blank");
+ window.open(
+ `https://arweave.net/${archiveResult.arweave_tx_id}`,
+ "_blank",
+ "noopener,noreferrer",
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/components/chess/PgnExportArchive.tsx` around lines 121 - 135, Both
viewOnIpfs and viewOnArweave use window.open without sandboxing, enabling
reverse-tabnabbing; modify each to call window.open with the third argument
'noopener,noreferrer' (e.g. window.open(url, '_blank', 'noopener,noreferrer'))
and, if window.open returns a window object, set newWin.opener = null to cover
older browsers (update functions viewOnIpfs and viewOnArweave accordingly).
| useEffect(() => { | ||
| const storedToken = localStorage.getItem("access_token"); | ||
| const storedUser = localStorage.getItem("user"); | ||
|
|
||
| if (storedToken && storedUser) { | ||
| setAccessToken(storedToken); | ||
| setUser(JSON.parse(storedUser)); | ||
| } | ||
| setIsLoading(false); | ||
| }, []); |
There was a problem hiding this comment.
Provider will crash on corrupted user in localStorage.
JSON.parse(storedUser) has no try/catch. If the value becomes malformed (older schema, manual edit, partial write), mounting AuthProvider throws synchronously during the effect and takes down the tree — including the login UI needed to recover.
🛡️ Defensive parse + cleanup
- const storedToken = localStorage.getItem("access_token");
- const storedUser = localStorage.getItem("user");
-
- if (storedToken && storedUser) {
- setAccessToken(storedToken);
- setUser(JSON.parse(storedUser));
- }
- setIsLoading(false);
+ try {
+ const storedToken = localStorage.getItem("access_token");
+ const storedUser = localStorage.getItem("user");
+ if (storedToken && storedUser) {
+ setAccessToken(storedToken);
+ setUser(JSON.parse(storedUser) as AuthUser);
+ }
+ } catch (err) {
+ console.error("Failed to restore auth state:", err);
+ localStorage.removeItem("access_token");
+ localStorage.removeItem("user");
+ } finally {
+ setIsLoading(false);
+ }📝 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.
| useEffect(() => { | |
| const storedToken = localStorage.getItem("access_token"); | |
| const storedUser = localStorage.getItem("user"); | |
| if (storedToken && storedUser) { | |
| setAccessToken(storedToken); | |
| setUser(JSON.parse(storedUser)); | |
| } | |
| setIsLoading(false); | |
| }, []); | |
| useEffect(() => { | |
| try { | |
| const storedToken = localStorage.getItem("access_token"); | |
| const storedUser = localStorage.getItem("user"); | |
| if (storedToken && storedUser) { | |
| setAccessToken(storedToken); | |
| setUser(JSON.parse(storedUser) as AuthUser); | |
| } | |
| } catch (err) { | |
| console.error("Failed to restore auth state:", err); | |
| localStorage.removeItem("access_token"); | |
| localStorage.removeItem("user"); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/context/authContext.tsx` around lines 54 - 63, The effect in
AuthProvider reads localStorage and calls JSON.parse(storedUser) without
protection which will throw on malformed JSON; wrap the parse in a try/catch
inside the useEffect (the block that reads storedToken/storedUser) and on parse
error clear the broken entry (localStorage.removeItem("user")), optionally
remove the access token if you want to force re-login, and ensure you call
setUser(null) and setIsLoading(false) so the provider doesn't crash; keep
setAccessToken(storedToken) only when token exists and parsing succeeds or after
handling the error.
| useEffect(() => { | ||
| if (!accessToken) return; | ||
|
|
||
| const tokenData = decodeJWT(accessToken); | ||
| if (!tokenData) return; | ||
|
|
||
| const expiresIn = tokenData.exp - Date.now() / 1000; | ||
| const refreshTime = (expiresIn - 300) * 1000; // Refresh 5 minutes before expiration | ||
|
|
||
| if (refreshTime > 0) { | ||
| const timeout = setTimeout(() => { | ||
| refreshToken(); | ||
| }, refreshTime); | ||
|
|
||
| return () => clearTimeout(timeout); | ||
| } | ||
| }, [accessToken]); |
There was a problem hiding this comment.
Near-expired tokens are never refreshed + stale dependency array.
Two related bugs in the auto-refresh effect:
- When
refreshTime <= 0(stored token has < 5 min remaining on page reload, or a very short-lived token is returned), the effect takes no action — the user keeps a near-expiredaccessToken, all subsequent API calls 401, and nothing triggers recovery. refreshTokenis referenced inside the effect but absent from the dependency array. This violatesreact-hooks/exhaustive-depsand relies onrefreshToken's closure changing transitively withaccessToken; brittle iflogoutor other deps change later.
🛠️ Proposed fix
useEffect(() => {
if (!accessToken) return;
const tokenData = decodeJWT(accessToken);
if (!tokenData) return;
- const expiresIn = tokenData.exp - Date.now() / 1000;
- const refreshTime = (expiresIn - 300) * 1000; // Refresh 5 minutes before expiration
-
- if (refreshTime > 0) {
- const timeout = setTimeout(() => {
- refreshToken();
- }, refreshTime);
-
- return () => clearTimeout(timeout);
- }
- }, [accessToken]);
+ const expiresIn = tokenData.exp - Date.now() / 1000;
+ const refreshTime = Math.max(0, (expiresIn - 300) * 1000);
+
+ const timeout = setTimeout(() => {
+ refreshToken();
+ }, refreshTime);
+
+ return () => clearTimeout(timeout);
+ }, [accessToken, refreshToken]);📝 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.
| useEffect(() => { | |
| if (!accessToken) return; | |
| const tokenData = decodeJWT(accessToken); | |
| if (!tokenData) return; | |
| const expiresIn = tokenData.exp - Date.now() / 1000; | |
| const refreshTime = (expiresIn - 300) * 1000; // Refresh 5 minutes before expiration | |
| if (refreshTime > 0) { | |
| const timeout = setTimeout(() => { | |
| refreshToken(); | |
| }, refreshTime); | |
| return () => clearTimeout(timeout); | |
| } | |
| }, [accessToken]); | |
| useEffect(() => { | |
| if (!accessToken) return; | |
| const tokenData = decodeJWT(accessToken); | |
| if (!tokenData) return; | |
| const expiresIn = tokenData.exp - Date.now() / 1000; | |
| const refreshTime = Math.max(0, (expiresIn - 300) * 1000); | |
| const timeout = setTimeout(() => { | |
| refreshToken(); | |
| }, refreshTime); | |
| return () => clearTimeout(timeout); | |
| }, [accessToken, refreshToken]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/context/authContext.tsx` around lines 66 - 82, The useEffect
watching accessToken (the effect that decodes the token via decodeJWT and
computes expiresIn/refreshTime) fails to handle refreshTime <= 0 and omits
refreshToken from deps; update the effect in useEffect to call refreshToken
immediately when refreshTime <= 0 (so near-expired tokens are refreshed on load)
and add refreshToken to the dependency array (e.g., useEffect(..., [accessToken,
refreshToken])); keep existing logic for scheduling setTimeout when refreshTime
> 0 and ensure you still clear the timeout on cleanup.
| localStorage.setItem("access_token", data.access_token); | ||
| localStorage.setItem("user", JSON.stringify({ | ||
| user_id: data.user_id, | ||
| username: data.username, | ||
| })); | ||
|
|
||
| return data; | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OWASP 2025 guidance on storing JWT access tokens in localStorage vs in-memory with httpOnly refresh cookie
💡 Result:
OWASP guidance strongly discourages storing JWT access tokens in localStorage due to XSS risks, as it is accessible to any JavaScript, including malicious scripts. Instead, OWASP recommends secure storage methods. For session tokens, including JWTs used as such, OWASP ASVS v4.0.3 V3.2.3 requires: "Verify the application only stores session tokens in the browser using secure methods such as appropriately secured cookies (see section 3.4) or HTML 5 session storage." Section 3.4 specifies cookies must be HttpOnly, Secure, and SameSite=Strict/Lax to mitigate XSS, CSRF. The JWT for Java Cheat Sheet suggests sessionStorage (or closures/in-memory) for tokens sent as Bearer headers in SPAs, noting localStorage persists across sessions (less secure), but acknowledges sessionStorage's per-tab limitation and recommends short expirations, token rotation, fingerprints, and CSP if using localStorage. It prefers avoiding JS-accessible storage where possible. Session Management Cheat Sheet praises HttpOnly cookies for preventing XSS access to session IDs. Industry consensus from OWASP-aligned sources (2025-2026 articles citing OWASP): Use short-lived (15-30 min) access tokens in memory/JS variables (lost on refresh, auto-refresh via httpOnly refresh cookie), with refresh tokens in httpOnly Secure SameSite=Strict cookies. This balances security (XSS/CSRF protection) and UX. localStorage: High XSS risk, persists indefinitely, avoid entirely for auth tokens. In-memory: Best for access tokens (no persistence), but requires refresh mechanism. httpOnly refresh cookie: Standard for refresh tokens, browser-auto-sent, JS-inaccessible. No specific "OWASP 2025" document found; guidance from ASVS 4.0.3 (latest), cheat sheets (ongoing). OWASP Top 10 2025 lists Authentication Failures at #7 but no storage specifics.
Citations:
- 1: https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html
- 2: https://cheatsheetseries.owasp.org/cheatsheets/JSON_Web_Token_for_Java_Cheat_Sheet.html
- 3: https://asvs.dev/v4.0.3/V3-Session-management/
- 4: https://cornucopia.owasp.org/taxonomy/asvs-4.0.3/03-session-management/02-session-binding
- 5: https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html
🏁 Script executed:
# First, check the authContext.tsx file structure
wc -l frontend/context/authContext.tsxRepository: NOVUS-X/XLMate
Length of output: 95
🏁 Script executed:
# Read the specific lines mentioned (107-114 and surrounding context)
sed -n '100,150p' frontend/context/authContext.tsxRepository: NOVUS-X/XLMate
Length of output: 1390
🏁 Script executed:
# Search for other localStorage access_token references
rg "localStorage.*access_token|access_token.*localStorage" --type tsx --type ts -nRepository: NOVUS-X/XLMate
Length of output: 86
🏁 Script executed:
# Check if there's a CSP policy in the project
fd -type f \( -name "next.config.*" -o -name ".env*" -o -name "csp*" \) | head -20Repository: NOVUS-X/XLMate
Length of output: 229
🏁 Script executed:
# Search for refresh token implementation
rg "\/v1\/auth\/refresh|refreshToken" --type tsx --type ts -n | head -30Repository: NOVUS-X/XLMate
Length of output: 157
🏁 Script executed:
# Search for refresh token implementation with proper escaping
rg "refresh" frontend/context/authContext.tsx -n -A 3 -B 1Repository: NOVUS-X/XLMate
Length of output: 1832
🏁 Script executed:
# Search for all localStorage usage in the file
rg "localStorage" frontend/context/authContext.tsx -nRepository: NOVUS-X/XLMate
Length of output: 635
🏁 Script executed:
# Search for sessionStorage usage
rg "sessionStorage" . -nRepository: NOVUS-X/XLMate
Length of output: 145
🏁 Script executed:
# Check common CSP config locations
cat next.config.js 2>/dev/null || cat next.config.ts 2>/dev/null || echo "No next config found"Repository: NOVUS-X/XLMate
Length of output: 79
🏁 Script executed:
# Read the full authContext.tsx to understand the architecture
cat -n frontend/context/authContext.tsx | head -100Repository: NOVUS-X/XLMate
Length of output: 3641
🏁 Script executed:
# Check if there's a refresh mechanism elsewhere
rg "auth/refresh" . -nRepository: NOVUS-X/XLMate
Length of output: 392
Remove access token from localStorage; rely on React state with httpOnly refresh token.
The code stores the access token in localStorage (lines 107, 139, 184) in addition to React state. This creates an XSS vulnerability: any injected script can read localStorage.access_token and exfiltrate it.
The app already has the infrastructure for the secure pattern:
refreshToken()calls/v1/auth/refreshwithcredentials: "include"for the httpOnly refresh cookie- Auto-refresh is implemented 5 minutes before expiration
- Access token is already stored in React state
Remove all localStorage.setItem("access_token", ...) calls (lines 107, 139, 184) and the localStorage.getItem("access_token") initialization (line 55). The React state and auto-refresh mechanism are sufficient; on page reload, the httpOnly refresh cookie will automatically re-authenticate.
Additionally, enable a strict Content Security Policy (CSP) to mitigate XSS risks even further.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/context/authContext.tsx` around lines 107 - 114, The code stores
access tokens in localStorage creating an XSS risk; remove all uses of
localStorage for the access token and rely on React state + httpOnly refresh
cookie and the existing refreshToken()/auto-refresh logic. Specifically, delete
any localStorage.setItem("access_token", ...) calls found around the auth
initialization, login/handleLogin, and refresh flows (references: the
functions/variables handling setAuth/state updates and refreshToken()), also
remove localStorage.getItem("access_token") from the initial state bootstrap,
and ensure auth state is initialized only from React state/refresh flow; keep
the existing refreshToken() call (which uses credentials: "include") and
auto-refresh scheduling 5 minutes before expiry. Finally, add/enable a strict
Content Security Policy (CSP) header to mitigate XSS as an additional security
layer.
| export function useEnhancedChessSocket( | ||
| gameId: string | null, | ||
| accessToken?: string | null | ||
| ): UseEnhancedChessSocketReturn { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm accessToken is genuinely unused inside the hook body and that callers pass it.
rg -nP 'useEnhancedChessSocket\s*\(' --type=ts --type=tsx -C2
rg -nP '\baccessToken\b' -C2 -- frontend/hook/useEnhancedChessSocket.tsRepository: NOVUS-X/XLMate
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Search for accessToken usage in the hook file - try without tsx type
echo "=== Searching for accessToken in useEnhancedChessSocket.ts ==="
rg -n "accessToken" frontend/hook/useEnhancedChessSocket.ts
echo ""
echo "=== File size check ==="
wc -l frontend/hook/useEnhancedChessSocket.ts
echo ""
echo "=== Check how useEnhancedChessSocket is called ==="
rg -n "useEnhancedChessSocket" --type ts -B2 -A2Repository: NOVUS-X/XLMate
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Read the hook file to verify the claims
cat -n frontend/hook/useEnhancedChessSocket.ts
echo ""
echo "=== Search for actual function calls with arguments ==="
rg -n "useEnhancedChessSocket\s*\(" -A3 --type tsRepository: NOVUS-X/XLMate
Length of output: 13018
🏁 Script executed:
#!/bin/bash
# Find files that import and call useEnhancedChessSocket
echo "=== Files importing useEnhancedChessSocket ==="
rg -l "useEnhancedChessSocket" --type ts
echo ""
echo "=== Actual calls to useEnhancedChessSocket with full context ==="
rg -n "useEnhancedChessSocket\s*\(" -B2 -A5 --type tsRepository: NOVUS-X/XLMate
Length of output: 925
🏁 Script executed:
#!/bin/bash
# Search for actual hook calls (const/let pattern)
echo "=== Look for const/let patterns calling the hook ==="
rg -n "const\s+.*=\s*useEnhancedChessSocket|let\s+.*=\s*useEnhancedChessSocket" -B1 -A3 --type ts
echo ""
echo "=== Check ConnectionStatusIndicator.tsx ==="
cat -n frontend/components/chess/ConnectionStatusIndicator.tsxRepository: NOVUS-X/XLMate
Length of output: 3672
🏁 Script executed:
#!/bin/bash
# Search more broadly for where the hook is actually invoked
echo "=== Search all ts/tsx files for useEnhancedChessSocket invocation ==="
find frontend -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "useEnhancedChessSocket" 2>/dev/null
echo ""
echo "=== Look for any chess game component that might use it ==="
find frontend -type f \( -name "*.ts" -o -name "*.tsx" \) | grep -i chess | head -20
echo ""
echo "=== Search for 'const.*=.*useEnhanced' pattern across all files ==="
find frontend -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "useEnhanced" {} \;Repository: NOVUS-X/XLMate
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Check if the hook is actually being used or if it's unused code
echo "=== Check useChessSocket.ts (the old hook) ==="
head -100 frontend/hook/useChessSocket.ts
echo ""
echo "=== Search for any component that might be calling useEnhancedChessSocket ==="
rg "useEnhancedChessSocket\s*\(" -C5 --type tsRepository: NOVUS-X/XLMate
Length of output: 4802
🏁 Script executed:
#!/bin/bash
# Final verification: search for actual hook invocations across entire repo
echo "=== Final check: any use of useEnhancedChessSocket() as a hook call ==="
rg "useEnhancedChessSocket\s*\(" -C10 frontend/
echo ""
echo "=== Check git status or PR context to see if this is new code ==="
git log --oneline --all -10 -- frontend/hook/useEnhancedChessSocket.ts 2>/dev/null || echo "Git history not available"Repository: NOVUS-X/XLMate
Length of output: 1902
accessToken parameter is declared but never used — WebSocket is unauthenticated.
The hook accepts accessToken?: string | null, but the token is never appended to the WebSocket URL or sent in any message. According to the PR description, horizontal scaling requires JWT auth for session affinity, but the server has no way to associate this connection with an authenticated user. This will break once the backend enforces auth on /v1/ws/game/:id.
Additionally, the hook has three other issues:
-
Reconnect race condition:
reconnect()closes the old WebSocket and callscreateWebSocket(true), but the old connection'sonclosehandler fires asynchronously. SinceisManualDisconnectRefis set tofalsebefore the old handler runs, it triggers an unintended automatic reconnection, creating duplicate concurrent WebSockets and stacked reconnect timers. -
sendMovedoesn't clear pending reconnect timeouts (lines 270–273): CallingcreateWebSocket(true)without first clearingreconnectTimeoutRefcan cause multiple concurrent reconnection attempts if a reconnect is already scheduled. -
Missing dependency in
createWebSocket:startLatencyMonitoring(called at line 145) is not in the useCallback deps list (line 240), violating exhaustive-deps rules.
🛠️ Fix: Add accessToken to WebSocket URL
let wsUrl = `${WS_BASE}/v1/ws/game/${gameId}`;
const params = new URLSearchParams();
if (reconnectTokenRef.current && attemptReconnect) {
params.set("reconnect", reconnectTokenRef.current);
}
+ if (accessToken) {
+ params.set("token", accessToken);
+ }
if (params.toString()) {
wsUrl += `?${params.toString()}`;
}And add accessToken to the createWebSocket useCallback dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/hook/useEnhancedChessSocket.ts` around lines 63 - 66, The hook
currently accepts accessToken but never uses it, so modify
useEnhancedChessSocket to append the accessToken (e.g., as a query param or
header-equivalent in the WS URL) when building the socket URL in
createWebSocket; also fix the reconnect race by having reconnect() set
isManualDisconnectRef in a way that prevents the old socket's onclose from
triggering an automatic reconnection (for example set a temporary flag or close
the socket after marking manual disconnect and only unset after new socket is
created), ensure sendMove clears any pending reconnectTimeoutRef before calling
createWebSocket(true) to avoid duplicate reconnections, and include
startLatencyMonitoring in the dependency array of the createWebSocket
useCallback so deps are exhaustive.
| const reconnect = useCallback(() => { | ||
| isManualDisconnectRef.current = false; | ||
| clearReconnectTimeout(); | ||
|
|
||
| if (wsRef.current) { | ||
| wsRef.current.close(); | ||
| wsRef.current = null; | ||
| } | ||
|
|
||
| reconnectAttemptsRef.current = 0; | ||
| console.log("[EnhancedWebSocket] Manual reconnection initiated"); | ||
| createWebSocket(true); | ||
| }, [clearReconnectTimeout, createWebSocket]); |
There was a problem hiding this comment.
Race: manual reconnect() produces duplicate concurrent sockets.
Sequence of events in reconnect():
isManualDisconnectRef.current = falsewsRef.current.close()— async;onclosewill fire later.wsRef.current = nullcreateWebSocket(true)— creates a new socket and assigns it towsRef.current.
When the old socket's onclose finally fires (Line 205-232), isManualDisconnectRef.current is false and reconnectAttemptsRef.current is 0, so it takes the reconnect branch and schedules another createWebSocket(true) via reconnectTimeoutRef. You now have two websockets and a stacked backoff timer. The same race can be triggered by sendMove (Line 270-273) when a reconnect is already pending — it does not clearReconnectTimeout() before calling createWebSocket(true).
🛠️ Suggested fix
Guard the old socket's handlers before closing, and clear any pending reconnect before manually connecting:
const reconnect = useCallback(() => {
isManualDisconnectRef.current = false;
clearReconnectTimeout();
if (wsRef.current) {
+ // Detach handlers so the old socket's onclose doesn't trigger auto-reconnect.
+ wsRef.current.onopen = null;
+ wsRef.current.onmessage = null;
+ wsRef.current.onerror = null;
+ wsRef.current.onclose = null;
wsRef.current.close();
wsRef.current = null;
}And in sendMove:
if (status === "disconnected" || status === "idle") {
+ clearReconnectTimeout();
reconnectAttemptsRef.current = 0;
createWebSocket(true);
}📝 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.
| const reconnect = useCallback(() => { | |
| isManualDisconnectRef.current = false; | |
| clearReconnectTimeout(); | |
| if (wsRef.current) { | |
| wsRef.current.close(); | |
| wsRef.current = null; | |
| } | |
| reconnectAttemptsRef.current = 0; | |
| console.log("[EnhancedWebSocket] Manual reconnection initiated"); | |
| createWebSocket(true); | |
| }, [clearReconnectTimeout, createWebSocket]); | |
| const reconnect = useCallback(() => { | |
| isManualDisconnectRef.current = false; | |
| clearReconnectTimeout(); | |
| if (wsRef.current) { | |
| // Detach handlers so the old socket's onclose doesn't trigger auto-reconnect. | |
| wsRef.current.onopen = null; | |
| wsRef.current.onmessage = null; | |
| wsRef.current.onerror = null; | |
| wsRef.current.onclose = null; | |
| wsRef.current.close(); | |
| wsRef.current = null; | |
| } | |
| reconnectAttemptsRef.current = 0; | |
| console.log("[EnhancedWebSocket] Manual reconnection initiated"); | |
| createWebSocket(true); | |
| }, [clearReconnectTimeout, createWebSocket]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/hook/useEnhancedChessSocket.ts` around lines 291 - 303, When
performing a manual reconnect or creating a new socket from sendMove, race
occurs because the old socket's event handlers can later trigger a reconnect;
fix by: in reconnect() (and before calling createWebSocket(true) in sendMove),
first call clearReconnectTimeout(), then if (wsRef.current) detach the old
handlers by setting wsRef.current.onclose = wsRef.current.onerror =
wsRef.current.onmessage = wsRef.current.onopen = () => {} (or equivalent no-op),
then call wsRef.current.close(), set wsRef.current = null, and only after that
call createWebSocket(true); this prevents the old socket's onclose handler
(which references isManualDisconnectRef and reconnectTimeoutRef) from scheduling
a duplicate reconnect.
2 participants
…cket scaling, and PGN archival
Issue Backend: Secure session management with JWT and Refresh Tokens #549: Secure session management with JWT and Refresh Tokens
Issue Backend: Websocket scaling with horizontal load balancing #548 & Backend: Websocket scaling with horizontal load balancing #538: Websocket scaling with horizontal load balancing
Issue Backend: Automated PGN archiving to IPFS/Arweave #541: Automated PGN archiving to IPFS/Arweave
All implementations follow Next.js best practices and TypeScript standards. CI checks pass: npm run build successful.
Closes #549
Closes #548
Closes #538
Closes #541
Summary by CodeRabbit