Conversation
📝 WalkthroughWalkthroughThe WebSocket server integration has been refactored to use HTTP upgrade path handling instead of relying on the server's connection event. Path-based filtering and ArcJet protection are now applied during upgrade, with HTTP status codes returned for denied requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Server as HTTP Server
participant AJ as ArcJet Protection
participant WSS as WebSocket Server
participant Socket as Socket/Connection
Client->>Server: HTTP Upgrade Request
Server->>Server: Check path (must be /ws)
alt Path is /ws
Server->>AJ: Validate request
alt Protection allows
AJ->>Server: Request approved
Server->>WSS: handleUpgrade()
WSS->>Socket: Establish WebSocket
Socket->>Client: WebSocket established
else Protection denies
AJ->>Server: Request denied (rate-limited/forbidden)
Server->>Socket: Send 429/403 + close
Socket->>Client: HTTP error response
end
else Path is not /ws
Server->>Socket: Destroy socket
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In `@src/ws/server.js`:
- Around line 30-53: Before calling wss.handleUpgrade(req, socket, head, ...)
add a liveness guard after the await wsArcjet.protect() (and after the
try/catch) to ensure the socket is still usable; check socket.destroyed (or
socket.writable) and if the socket was closed/destroyed, skip calling
wss.handleUpgrade and return early (optionally log). This change touches the
wsArcjet.protect call site and the wss.handleUpgrade invocation—ensure you
perform the socket liveness check between them and avoid calling
wss.handleUpgrade on a destroyed socket.
- Around line 26-29: Wrap the URL parsing inside the async upgrade handler in a
try/catch to prevent the URL constructor from throwing an unhandled rejection;
specifically, surround the line that constructs new URL(req.url,
`http://${req.headers.host}`) and the extraction of pathname with a try/catch in
the server.on('upgrade', async (req, socket, head) => { ... }) handler, and on
catch send a 400 response on the socket (write an HTTP/1.1 400 Bad Request
response and destroy the socket) and return so further handling (e.g., the if
(pathname === '/ws') block) does not run.
| server.on('upgrade', async (req, socket, head) => { | ||
| const { pathname } = new URL(req.url, `http://${req.headers.host}`); | ||
|
|
||
| if (pathname === '/ws') { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "server.js" -path "*/ws/*" -type fRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 95
🏁 Script executed:
head -40 src/ws/server.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 1794
🏁 Script executed:
wc -l src/ws/server.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 96
🏁 Script executed:
sed -n '26,82p' src/ws/server.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 2007
🏁 Script executed:
cat > /tmp/verify_url.js << 'EOF'
// Test 1: Does new URL throw on invalid input?
try {
new URL('not a valid url', 'http://localhost');
console.log("Test 1 PASS: No throw");
} catch (e) {
console.log("Test 1 FAIL: Throws " + e.constructor.name);
}
// Test 2: Empty/null string handling
try {
new URL('', 'http://localhost');
console.log("Test 2 PASS: Empty string accepted");
} catch (e) {
console.log("Test 2 FAIL: Empty string throws");
}
// Test 3: null base
try {
new URL('/path', null);
console.log("Test 3 PASS: null base accepted");
} catch (e) {
console.log("Test 3 FAIL: null base throws");
}
// Test 4: undefined headers.host
try {
const req = { url: '/ws', headers: {} };
const url = new URL(req.url, `http://${req.headers.host}`);
console.log("Test 4 RESULT: Created URL with undefined host, base is:", `http://${req.headers.host}`);
} catch (e) {
console.log("Test 4 FAIL: Throws " + e.constructor.name);
}
EOF
node /tmp/verify_url.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 237
🏁 Script executed:
cat > /tmp/check_async_handler.js << 'EOF'
// Simulate async upgrade handler with unhandled error
const http = require('http');
const server = http.createServer();
let errorCaught = false;
// Track unhandled rejections
process.on('unhandledRejection', (reason, promise) => {
console.log('Unhandled rejection detected:', reason.message);
errorCaught = true;
});
server.on('upgrade', async (req, socket, head) => {
try {
// This would throw in the async context
const url = new URL('not a valid url', 'http://localhost');
} catch (e) {
console.log('Sync throw caught in async handler:', e.message);
// Without try/catch, this becomes an unhandled rejection
}
});
console.log('Async handler error handling: If error is thrown without try/catch, it becomes unhandled rejection');
EOF
node /tmp/check_async_handler.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 175
🏁 Script executed:
cat > /tmp/test_malformed_urls.js << 'EOF'
// Test various potentially problematic URL combinations
const tests = [
{ url: null, host: 'localhost', desc: 'null url' },
{ url: undefined, host: 'localhost', desc: 'undefined url' },
{ url: '/ws', host: 'localhost', desc: 'normal path' },
{ url: '/ws', host: undefined, desc: 'undefined host' },
{ url: '/ws', host: '', desc: 'empty host' },
{ url: '/ws', host: 'invalid[host]', desc: 'invalid host chars' },
{ url: '/ws\r\nHost: evil.com', host: 'localhost', desc: 'CRLF injection attempt' },
];
tests.forEach(test => {
try {
const base = `http://${test.host || 'localhost'}`;
const result = new URL(test.url || '/', base);
console.log(`✓ ${test.desc}: OK`);
} catch (e) {
console.log(`✗ ${test.desc}: THROWS - ${e.message}`);
}
});
EOF
node /tmp/test_malformed_urls.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 239
🏁 Script executed:
cat > /tmp/test_upgrade_context.js << 'EOF'
// Check if async handler without try/catch can crash process
const http = require('http');
const server = http.createServer();
let rejectionHandled = false;
process.on('unhandledRejection', (reason) => {
console.log('⚠ Process would crash on unhandledRejection:', reason.message);
rejectionHandled = true;
});
// Simulate current code structure
server.on('upgrade', async (req, socket, head) => {
// Synchronous throw in async context = unhandled rejection IF not caught
throw new Error('Sync error in async handler');
});
// Emit test upgrade event
server.emit('upgrade',
{ url: '/ws', headers: { host: 'localhost' } },
{ write: () => {}, destroy: () => {} },
Buffer.from('')
);
setTimeout(() => {
if (rejectionHandled) {
console.log('✓ Unhandled rejection confirmed - can crash process');
}
}, 100);
EOF
node /tmp/test_upgrade_context.jsRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 201
Guard URL parsing in async upgrade handler to prevent unhandled rejection crashes.
While req.url is always a string and req.headers.host missing gracefully becomes "http://undefined", the URL constructor can still throw on certain malformed inputs (e.g., invalid host characters). Since this is in an async upgrade handler without try/catch, any thrown error becomes an unhandled rejection that will crash the process.
Wrap the URL parsing in try/catch and respond with a 400 error:
🔧 Suggested fix
- const { pathname } = new URL(req.url, `http://${req.headers.host}`);
+ let pathname;
+ try {
+ ({ pathname } = new URL(req.url, `http://${req.headers.host || 'localhost'}`));
+ } catch (err) {
+ socket.write('HTTP/1.1 400 Bad Request\r\nConnection: close\r\n\r\n');
+ socket.destroy();
+ return;
+ }🤖 Prompt for AI Agents
In `@src/ws/server.js` around lines 26 - 29, Wrap the URL parsing inside the async
upgrade handler in a try/catch to prevent the URL constructor from throwing an
unhandled rejection; specifically, surround the line that constructs new
URL(req.url, `http://${req.headers.host}`) and the extraction of pathname with a
try/catch in the server.on('upgrade', async (req, socket, head) => { ... })
handler, and on catch send a 400 response on the socket (write an HTTP/1.1 400
Bad Request response and destroy the socket) and return so further handling
(e.g., the if (pathname === '/ws') block) does not run.
| if (wsArcjet) { | ||
| try { | ||
| const decision = await wsArcjet.protect(req); | ||
| if (decision.isDenied()) { | ||
| const statusCode = decision.reason.isRateLimit() ? 429 : 403; | ||
| const statusMessage = decision.reason.isRateLimit() ? "Too Many Requests" : "Forbidden"; | ||
| socket.write(`HTTP/1.1 ${statusCode} ${statusMessage}\r\nConnection: close\r\n\r\n`); | ||
| socket.destroy(); | ||
| return; | ||
| } | ||
| } catch (e) { | ||
| console.log('Ws security error during upgrade', e); | ||
| socket.write('HTTP/1.1 500 Internal Server Error\r\nConnection: close\r\n\r\n'); | ||
| socket.destroy(); | ||
| return; | ||
| } | ||
| }catch (e) { | ||
| console.log('Ws connection error', e); | ||
| socket.close(1011, 'Server security error') | ||
| return; | ||
|
|
||
| } | ||
|
|
||
| wss.handleUpgrade(req, socket, head, (ws) => { | ||
| wss.emit('connection', ws, req); | ||
| }); | ||
| } else { | ||
| socket.destroy(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "server.js" -path "*/ws/*" | head -5Repository: SanManjiro/Real-Time-Sport-Engine
Length of output: 95
🏁 Script executed:
cat -n src/ws/server.js | head -70Repository: SanManjiro/Real-Time-Sport-Engine
Length of output: 3206
🏁 Script executed:
web_search
Node.js Socket.destroyed property behavior and when it is setRepository: SanManjiro/Real-Time-Sport-Engine
Length of output: 173
🌐 Web query:
Node.js Socket destroyed property documentation
💡 Result:
In Node.js, socket.destroyed is documented on net.Socket as:
- Type:
boolean - Meaning: indicates whether the connection has been destroyed; once destroyed, no further data can be transferred. It also notes to “see
writable.destroyedfor further details.” [1]
Because net.Socket is a stream (Duplex), the underlying stream docs define writable.destroyed as:
- Type:
boolean - Meaning:
trueafterwritable.destroy()has been called. [2]
And similarly readable.destroyed is:
- Type:
boolean - Meaning:
trueafterreadable.destroy()has been called. [3]
Sources: Node.js net docs for socket.destroyed [1], and Node.js stream docs for writable.destroyed / readable.destroyed [2] [3].
🌐 Web query:
Node.js net.Socket destroyed property when set client disconnect
💡 Result:
socket.destroyed becomes true when the socket’s underlying stream has been destroyed/closed, meaning no more I/O is possible on that net.Socket. It’s not something you typically “set”; Node flips it as part of teardown (local destroy() / end() leading to close, remote hangup, or errors). Once destroyed === true, further write()/end() attempts will fail. [1]
What happens on a client disconnect (server-side)
- If the client closes gracefully (FIN), the server socket emits:
'end': remote sent FIN (no more incoming data). [2]- then
'close': socket is fully closed (callback arg tells if it closed due to error). [2]
After/around'close',socket.destroyedwill betrue. [1][2]
- If the client drops abruptly (RST / network issue), you’ll usually see
'error'then'close', anddestroyedwill becometrue. [2]
Best practice: detect disconnect via events (not polling destroyed)
server.on('connection', (socket) => {
socket.on('end', () => { /* client half-closed (FIN) */ });
socket.on('close', (hadError) => { /* fully closed; socket.destroyed is true */ });
socket.on('error', (err) => { /* handle/reset */ });
});Related nuance: allowHalfOpen
By default (allowHalfOpen: false), after 'end' Node will finish pending writes and then close/destroy the socket automatically. If you set allowHalfOpen: true, you must call socket.end() yourself to close your side. [2]
Sources: Node.js net.Socket.destroyed docs [1]; Node.js net.Socket events 'end', 'error', 'close' docs [2].
References:
[1] https://nodejs.org/api/net.html
[2] https://r2.nodejs.org/docs/v4.2.2/api/net.html
Add a socket liveness check before handleUpgrade to prevent errors on client disconnect during protection.
During the async wsArcjet.protect() call, a client can disconnect and destroy the socket. If this happens, handleUpgrade will fail on a destroyed socket. Add a guard check before upgrading.
🛡️ Suggested guard
if (wsArcjet) {
try {
const decision = await wsArcjet.protect(req);
if (decision.isDenied()) {
const statusCode = decision.reason.isRateLimit() ? 429 : 403;
const statusMessage = decision.reason.isRateLimit() ? "Too Many Requests" : "Forbidden";
socket.write(`HTTP/1.1 ${statusCode} ${statusMessage}\r\nConnection: close\r\n\r\n`);
socket.destroy();
return;
}
} catch (e) {
console.log('Ws security error during upgrade', e);
socket.write('HTTP/1.1 500 Internal Server Error\r\nConnection: close\r\n\r\n');
socket.destroy();
return;
}
}
+ if (socket.destroyed) return;
wss.handleUpgrade(req, socket, head, (ws) => {
wss.emit('connection', ws, req);
});🤖 Prompt for AI Agents
In `@src/ws/server.js` around lines 30 - 53, Before calling wss.handleUpgrade(req,
socket, head, ...) add a liveness guard after the await wsArcjet.protect() (and
after the try/catch) to ensure the socket is still usable; check
socket.destroyed (or socket.writable) and if the socket was closed/destroyed,
skip calling wss.handleUpgrade and return early (optionally log). This change
touches the wsArcjet.protect call site and the wss.handleUpgrade
invocation—ensure you perform the socket liveness check between them and avoid
calling wss.handleUpgrade on a destroyed socket.
1 participant
Summary by CodeRabbit