Fix #28: Implement 3-layer defensive shield for rate limiting and input validation #93
Fix #28: Implement 3-layer defensive shield for rate limiting and input validation #93b-u-g-g wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds rate-limiting, validation, and sanitization middleware plus config limits; integrates them into websocket message handling with per-connection client IDs; removes debug logging from InputHandler; and adds a test harness exercising the new pipeline. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebSocket
participant RateLimiter as RateLimiter<br/>(Layer 1)
participant Validator as InputValidator<br/>(Layer 2)
participant Sanitizer as InputSanitizer<br/>(Layer 3)
participant InputHandler
Client->>WebSocket: Send message (JSON)
WebSocket->>WebSocket: Parse & assign clientId
WebSocket->>RateLimiter: shouldProcess(clientId, msg.type)
alt Rate limit exceeded
RateLimiter-->>WebSocket: false
WebSocket-->>Client: Drop / throttle response
else Rate limit OK
RateLimiter-->>WebSocket: true
WebSocket->>Validator: isValid(msg)
alt Validation fails
Validator-->>WebSocket: false
WebSocket-->>Client: Reject invalid message
else Validation passes
Validator-->>WebSocket: true
WebSocket->>Sanitizer: sanitize(msg)
Sanitizer->>Sanitizer: Clamp coords, clamp zoom, truncate strings/arrays
Sanitizer-->>WebSocket: Normalized message
WebSocket->>InputHandler: handle(normalized message)
InputHandler-->>WebSocket: Processed
WebSocket-->>Client: Acknowledge
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/websocket.ts (1)
63-84:⚠️ Potential issue | 🟠 Major
get-ipandupdate-configbypass all three middleware layers.These message types are handled before the rate-limit/validation/sanitization pipeline. A client can spam
get-ipwithout throttling, andupdate-configaccepts unvalidatedmsg.configthat gets written to disk viafs.writeFileSync. At minimum,update-configshould be rate-limited and its payload validated/sanitized — it writes arbitrary JSON to the filesystem.src/server/InputHandler.ts (1)
4-14:⚠️ Potential issue | 🟡 Minor
InputMessage.typeis missing'paste'and'copy'variants that the middleware accepts.
VALID_MESSAGE_TYPESinsrc/server/config/limits.ts(Lines 27-37) includes'paste'and'copy', and they pass validation. After sanitization, the message is cast toInputMessageinwebsocket.tsLine 101 and routed tohandleMessage, which has no matching cases for them — they silently fall through the switch statement. Either add them to theInputMessageunion and handle them, or reject them earlier in the validation pipeline.
🤖 Fix all issues with AI agents
In `@src/server/middleware/InputSanitizer.ts`:
- Around line 6-10: The scroll handling currently unconditionally calls
this.clamp on msg.dx and msg.dy which yields NaN when an axis is absent; update
the code in InputSanitizer (the switch case handling 'move' and 'scroll') to
only call this.clamp and assign back to msg.dx or msg.dy if the value is a
number (e.g., typeof msg.dx === 'number'), leaving the property untouched when
undefined/null; use the existing clamp method and INPUT_LIMITS constants so
behavior for present axes is unchanged.
In `@src/server/middleware/InputValidator.ts`:
- Around line 14-22: The scroll validation in InputValidator.ts currently groups
'scroll' with 'move' and requires both dx and dy, which rejects valid one-axis
scrolls; split the 'scroll' case from 'move' and change validation so that for
type === 'scroll' at least one of msg.dx or msg.dy is a number and finite (use
typeof/isFinite checks on each present value), while leaving 'move' to require
both. Also update InputSanitizer (where dx/dy are unconditionally clamped) to
treat dx/dy as optional for scroll — only call the clamping logic when the
property is present to avoid clamp(undefined, ...) → NaN; align these changes
with InputHandler.handleMessage which expects independent dx/dy for scroll.
In `@src/server/middleware/RateLimiter.ts`:
- Around line 3-31: The RateLimiter is fine but using a shared fallback clientId
('unknown') causes collisions; instead, ensure each WebSocket connection
supplies a truly unique clientId (e.g., generate a UUID or use an incrementing
connection counter when creating the socket) and pass that into
RateLimiter.shouldProcess and RateLimiter.cleanup; update the websocket
connection setup to assign the unique id on connect and call cleanup with that
id on close so lastMessageTime (Map in class RateLimiter) keys are
per-connection and not a shared 'unknown' bucket.
In `@src/server/websocket.ts`:
- Around line 42-43: The current connection handler (wss.on('connection') where
clientId is set from request.socket.remoteAddress) falls back to the literal
'unknown', causing different connections to share the same rate-limit bucket;
change clientId assignment to generate a per-connection unique identifier when
request.socket.remoteAddress is falsy (e.g., create a UUID or an incrementing
counter) and use that value for rate-limiting. Locate the clientId logic in the
wss.on('connection', (ws: WebSocket, request: IncomingMessage) => { ... }) block
and replace the fallback 'unknown' with a generated unique id (and optionally
attach it to the ws object like ws.id) so each proxied/undefined-remoteAddress
connection gets its own bucket. Ensure the generated id is stable for the
connection lifetime and used wherever clientId is referenced for rate-limiting.
In `@test-shield.ts`:
- Around line 42-213: The tests only print SUCCESS/FAILURE but never fail the
process; modify the IIFE that runs tests to track failures (e.g., a failures
counter updated when expected outcomes for processMessage results,
sanitizer.sanitize effects, and validator.isValid checks don't match
expectations) and at the end call process.exit(1) if failures > 0 (or use a test
framework like vitest/jest and convert each block into assertions). Update
checks around processMessage, sanitizer.sanitize, and validator.isValid (and the
large-case comparisons such as text length, coordinate clamping, combo key
length, zoom delta, and throttling summaries) to increment the failures counter
when conditions are not met so CI can detect test failures.
- Around line 17-36: The test calls processMessage which invokes
handler.handleMessage (an InputHandler that moves the mouse/presses keys),
causing unsafe side effects in CI; modify tests to avoid touching native
automation by either mocking/stubbing handler.handleMessage (replace the
global/instantiated InputHandler used by processMessage with a test double) or
refactor processMessage to accept a handler argument or expose the middleware
calls so tests exercise rateLimiter, validator, and sanitizer in isolation;
target symbols: processMessage, handler.handleMessage, InputHandler,
rateLimiter, validator, sanitizer.
🧹 Nitpick comments (4)
src/server/config/limits.ts (1)
1-38: Well-structured centralized config — looks good.Clean use of
as constfor immutability and literal types. One consideration: there's no limit on the overall raw WebSocket message size. A malicious client could send a very large JSON payload (e.g., acombomessage with 10 valid keys but megabytes of extra properties) before the validator even runs. Consider adding a max raw message size check inwebsocket.tsbeforeJSON.parse, or configuring thewsserver'smaxPayloadoption.src/server/middleware/InputValidator.ts (1)
39-48: No length bounds checked fortextandkeyin the validator.The validator accepts any string regardless of length. While the sanitizer truncates oversized values, an extremely large string (e.g., several MB) will pass validation and be held in memory until sanitization. For defense-in-depth, consider rejecting obviously abusive lengths here (e.g., text > 10×
MAX_TEXT_LENGTH).src/server/websocket.ts (1)
91-94: Logging unvalidatedmsg.typeis a minor log-injection vector.
msg.typecomes from raw client JSON and could contain control characters or very long strings, potentially polluting logs or exploiting log viewers. Consider sanitizing or truncating it before logging:Proposed fix
- console.warn('[Security] Invalid input:', msg.type); + console.warn('[Security] Invalid input:', String(msg.type).substring(0, 50));test-shield.ts (1)
1-1: Unused imports:mouse,Point,keyboard,Keyare imported but never used in this file.Proposed fix
-import { mouse, Point, keyboard, Key } from '@nut-tree-fork/nut-js';
| export class RateLimiter { | ||
| private lastMessageTime: Map<string, Map<string, number>>; | ||
|
|
||
| constructor() { | ||
| this.lastMessageTime = new Map(); | ||
| } | ||
|
|
||
| shouldProcess(clientId: string, messageType: string): boolean { | ||
| const now = Date.now(); | ||
|
|
||
| if (!this.lastMessageTime.has(clientId)) { | ||
| this.lastMessageTime.set(clientId, new Map()); | ||
| } | ||
|
|
||
| const clientTimes = this.lastMessageTime.get(clientId)!; | ||
| const lastTime = clientTimes.get(messageType) || 0; | ||
| const interval = RATE_LIMITS[messageType as keyof typeof RATE_LIMITS] || RATE_LIMITS.default; | ||
|
|
||
| if (now - lastTime < interval) { | ||
| return false; | ||
| } | ||
|
|
||
| clientTimes.set(messageType, now); | ||
| return true; | ||
| } | ||
|
|
||
| cleanup(clientId: string): void { | ||
| this.lastMessageTime.delete(clientId); | ||
| } |
There was a problem hiding this comment.
Clean implementation. Note potential 'unknown' client-ID collision.
The rate limiter itself is well-structured. However, in websocket.ts Line 43, clientId falls back to 'unknown' when remoteAddress is undefined (e.g., behind certain proxies). Multiple such clients would share a single throttle bucket, causing cross-client rate-limit interference. Consider using a unique identifier per connection (e.g., an incrementing counter or UUID) instead of relying solely on remoteAddress.
🤖 Prompt for AI Agents
In `@src/server/middleware/RateLimiter.ts` around lines 3 - 31, The RateLimiter is
fine but using a shared fallback clientId ('unknown') causes collisions;
instead, ensure each WebSocket connection supplies a truly unique clientId
(e.g., generate a UUID or use an incrementing connection counter when creating
the socket) and pass that into RateLimiter.shouldProcess and
RateLimiter.cleanup; update the websocket connection setup to assign the unique
id on connect and call cleanup with that id on close so lastMessageTime (Map in
class RateLimiter) keys are per-connection and not a shared 'unknown' bucket.
| (async () => { | ||
| // TEST 1: Rate Limiting | ||
| console.log("TEST 1: Rate Limiting - Move Events"); | ||
| console.log("Status: Dispatching 100 move events..."); | ||
|
|
||
| let processed = 0; | ||
| const startTime = Date.now(); | ||
|
|
||
| for (let i = 0; i < 100; i++) { | ||
| const result = await processMessage({ type: 'move', dx: 1, dy: 0 }); | ||
| if (result) processed++; | ||
| } | ||
|
|
||
| const duration = Date.now() - startTime; | ||
| console.log(`Summary: Processed ${processed}/100 in ${duration}ms`); | ||
| console.log(`Analysis: Expected approx 6 events for 60fps logic`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 1.5: Scroll Throttling | ||
| console.log("TEST 1.5: Scroll Throttling"); | ||
| console.log("Status: Dispatching 50 scroll events..."); | ||
| let scrollsProcessed = 0; | ||
| for (let i = 0; i < 50; i++) { | ||
| // Updated to include both axes to satisfy Validator schema | ||
| const result = await processMessage({ type: 'scroll', dx: 0, dy: 1 }); | ||
| if (result) scrollsProcessed++; | ||
| } | ||
| console.log(`Summary: Processed ${scrollsProcessed}/50`); | ||
| console.log(`Analysis: Expected approx 3 events to match move throttle`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 2: Text Truncation | ||
| console.log("TEST 2: Text Length Sanitization"); | ||
|
|
||
| const longText = 'A'.repeat(5000); | ||
| const textMsg = { type: 'text', text: longText }; | ||
|
|
||
| console.log(`Original: ${textMsg.text.length} characters`); | ||
| sanitizer.sanitize(textMsg); | ||
| console.log(`Sanitized: ${textMsg.text.length} characters`); | ||
| console.log(`Result: ${textMsg.text.length === 1000 ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 3: Coordinate Clamping | ||
| console.log("TEST 3: Coordinate Clamping"); | ||
|
|
||
| const hugeMove = { type: 'move', dx: 999999, dy: -999999 }; | ||
| console.log(`Input: dx=${hugeMove.dx}, dy=${hugeMove.dy}`); | ||
|
|
||
| sanitizer.sanitize(hugeMove); | ||
| console.log(`Output: dx=${hugeMove.dx}, dy=${hugeMove.dy}`); | ||
| console.log(`Result: ${hugeMove.dx === 5000 && hugeMove.dy === -5000 ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 4: Invalid Input Rejection | ||
| console.log("TEST 4: Invalid Input Validation"); | ||
|
|
||
| const invalidInputs = [ | ||
| { type: 'move', dx: NaN, dy: 5 }, | ||
| { type: 'move', dx: Infinity, dy: 0 }, | ||
| { type: 'text', text: 123 }, | ||
| { type: 'click', button: 'invalid' }, | ||
| { type: 'combo', keys: 'not-an-array' }, | ||
| { type: 'invalid-type' }, | ||
| ]; | ||
|
|
||
| let rejected = 0; | ||
| for (const msg of invalidInputs) { | ||
| if (!validator.isValid(msg)) { | ||
| rejected++; | ||
| } | ||
| } | ||
|
|
||
| console.log(`Summary: Rejected ${rejected}/${invalidInputs.length} inputs`); | ||
| console.log(`Result: ${rejected === invalidInputs.length ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 5: Valid Inputs Still Work | ||
| console.log("TEST 5: Valid Input Persistence"); | ||
|
|
||
| const validInputs = [ | ||
| { type: 'move', dx: 10, dy: 10 }, | ||
| { type: 'click', button: 'left', press: true }, | ||
| { type: 'text', text: 'Hello' }, | ||
| { type: 'scroll', dx: 5, dy: -5 }, | ||
| { type: 'combo', keys: ['ctrl', 'c'] }, | ||
| ]; | ||
|
|
||
| let validCount = 0; | ||
| for (const msg of validInputs) { | ||
| if (validator.isValid(msg)) { | ||
| validCount++; | ||
| } | ||
| } | ||
|
|
||
| console.log(`Summary: Accepted ${validCount}/${validInputs.length} inputs`); | ||
| console.log(`Result: ${validCount === validInputs.length ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 6: Combo Key Array Limit | ||
| console.log("TEST 6: Keyboard Combo Array Limit"); | ||
|
|
||
| const hugeCombo = { type: 'combo', keys: Array(50).fill('a') }; | ||
| console.log(`Original: ${hugeCombo.keys.length} keys`); | ||
|
|
||
| sanitizer.sanitize(hugeCombo); | ||
| console.log(`Sanitized: ${hugeCombo.keys.length} keys`); | ||
| console.log(`Result: ${hugeCombo.keys.length === 10 ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 7: Zoom Delta Clamping | ||
| console.log("TEST 7: Zoom Delta Clamping"); | ||
|
|
||
| const hugeZoom = { type: 'zoom', delta: 999 }; | ||
| console.log(`Input: ${hugeZoom.delta}`); | ||
|
|
||
| sanitizer.sanitize(hugeZoom); | ||
| console.log(`Output: ${hugeZoom.delta}`); | ||
| console.log(`Result: ${hugeZoom.delta === 100 ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 8: Click Throttling | ||
| console.log("TEST 8: Click Event Throttling"); | ||
| console.log("Status: Dispatching 30 click events..."); | ||
|
|
||
| let clicksProcessed = 0; | ||
| const clickStart = Date.now(); | ||
|
|
||
| for (let i = 0; i < 30; i++) { | ||
| const result = await processMessage({ type: 'click', button: 'left', press: true }); | ||
| if (result) clicksProcessed++; | ||
| } | ||
|
|
||
| const clickDuration = Date.now() - clickStart; | ||
| console.log(`Summary: Processed ${clicksProcessed}/30 in ${clickDuration}ms`); | ||
| console.log(`Analysis: Expected approx 2-4 events for 50ms interval`); | ||
| console.log(""); | ||
|
|
||
| await sleep(1000); | ||
|
|
||
| // TEST 9: Domain Isolation | ||
| console.log("TEST 9: Independent Throttling Per Type"); | ||
|
|
||
| const moveResult = await processMessage({ type: 'move', dx: 1, dy: 1 }); | ||
| const clickResult = await processMessage({ type: 'click', button: 'left', press: true }); | ||
| const textResult = await processMessage({ type: 'text', text: 'test' }); | ||
|
|
||
| console.log(`Move: ${moveResult ? 'PASS' : 'FAIL'}`); | ||
| console.log(`Click: ${clickResult ? 'PASS' : 'FAIL'}`); | ||
| console.log(`Text: ${textResult ? 'PASS' : 'FAIL'}`); | ||
| console.log(`Result: ${moveResult && clickResult && textResult ? 'SUCCESS' : 'FAILURE'}`); | ||
| console.log(""); | ||
| console.log("Integrated Security and Performance Tests Complete"); | ||
|
|
||
|
|
||
| })(); No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tests lack proper assertions and exit codes — failures are only visible in console output.
The script prints SUCCESS/FAILURE but always exits with code 0. Consider using a proper test framework (e.g., vitest or jest) or at minimum tracking failures and calling process.exit(1) on any failure. Without this, the test suite can't gate a CI pipeline.
🤖 Prompt for AI Agents
In `@test-shield.ts` around lines 42 - 213, The tests only print SUCCESS/FAILURE
but never fail the process; modify the IIFE that runs tests to track failures
(e.g., a failures counter updated when expected outcomes for processMessage
results, sanitizer.sanitize effects, and validator.isValid checks don't match
expectations) and at the end call process.exit(1) if failures > 0 (or use a test
framework like vitest/jest and convert each block into assertions). Update
checks around processMessage, sanitizer.sanitize, and validator.isValid (and the
large-case comparisons such as text length, coordinate clamping, combo key
length, zoom delta, and throttling summaries) to increment the failures counter
when conditions are not met so CI can detect test failures.
b-u-g-g
changed the title
b-u-g-g
changed the title
b-u-g-g
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/server/middleware/InputValidator.ts`:
- Around line 71-73: VALID_MESSAGE_TYPES currently allows 'paste' and 'copy' but
InputMessage (in InputHandler.ts) lacks those types, so messages pass
validation, get cast in websocket.ts, and then silently fall through
InputHandler.handleMessage; either add 'paste' and 'copy' to the InputMessage
union and implement handling in InputHandler.handleMessage (and ensure
websocket.ts casting and any rate-limit/UI behavior in ControlBar.tsx are
consistent), or remove 'paste' and 'copy' from VALID_MESSAGE_TYPES to prevent
them being validated; update whichever files you change (VALID_MESSAGE_TYPES,
InputHandler.ts InputMessage type, InputHandler.handleMessage, and websocket.ts
cast) so the type system, validator, and handler are aligned.
In `@src/server/websocket.ts`:
- Around line 92-95: When validator.isValid(msg) returns false, avoid logging
unsanitized msg.type; instead stringify and sanitize/truncate it before logging
to prevent log injection and huge entries. Update the invalid-input branch
around validator.isValid(msg) (the block that currently calls
console.warn('[Security] Invalid input:', msg.type)) to: obtain a safe string
from msg.type (e.g., String(msg.type) or JSON.stringify with try/catch), remove
newlines/control characters, and truncate to a reasonable length (e.g., 100
chars) before passing to console.warn so only the sanitized/truncated type is
logged.
In `@test-shield.ts`:
- Around line 36-51: The test's "expected approx 6 events" assumption is
incorrect because the loop dispatches 100 move events synchronously and the
actual number passing the 16ms throttle depends on real handler latency; update
the test in test-shield.ts to make the expectation deterministic: either (A)
stub or instrument processMessage/handler.handleMessage and add an explicit
sleep(16) between iterations so ~6 events pass in ~100ms, or (B) keep the tight
loop but change the expected result to 1 event (or assert <= N) to match a
synchronous fast path; locate the loop using processMessage and the TEST 1 Rate
Limiting comments and apply one of these fixes so the test no longer relies on
env timing.
🧹 Nitpick comments (3)
src/server/middleware/InputValidator.ts (1)
1-1: Unused import:INPUT_LIMITSis never referenced in this file.
INPUT_LIMITSis imported but not used anywhere inInputValidator. OnlyVALID_BUTTONSandVALID_MESSAGE_TYPESare consumed.Proposed fix
-import { INPUT_LIMITS, VALID_BUTTONS, VALID_MESSAGE_TYPES } from '../config/limits'; +import { VALID_BUTTONS, VALID_MESSAGE_TYPES } from '../config/limits';src/server/websocket.ts (1)
65-99:get-ipandupdate-configbypass rate limiting, validation, and sanitization.The early-return paths for
get-ip(Line 66) andupdate-config(Line 71) execute before the middleware pipeline. Whileget-ipis low-risk,update-configwrites arbitrarymsg.configto disk without any validation or rate limiting. A malicious client can callupdate-configat an unbounded rate with arbitrary payloads.This is pre-existing code, but now that a defensive pipeline is in place, consider at minimum rate-limiting these message types.
test-shield.ts (1)
1-1: Unused imports from@nut-tree-fork/nut-js.
mouse,Point,keyboard, andKeyare imported but never used directly in this file. They are only consumed transitively throughInputHandler.Proposed fix
-import { mouse, Point, keyboard, Key } from '@nut-tree-fork/nut-js';
| case 'paste': | ||
| case 'copy': | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the relevant files mentioned
fd -t f -e ts | grep -E "(InputValidator|InputMessage|websocket|InputHandler)" | head -20Repository: AOSSIE-Org/Rein
Length of output: 149
🏁 Script executed:
# Search for VALID_MESSAGE_TYPES definition
rg "VALID_MESSAGE_TYPES" -A 10 -B 2 --type=tsRepository: AOSSIE-Org/Rein
Length of output: 2001
🏁 Script executed:
# Search for paste and copy in the codebase
rg "'paste'|'copy'" --type=ts -B 2 -A 2Repository: AOSSIE-Org/Rein
Length of output: 665
🏁 Script executed:
# Check the InputMessage interface definition
rg "interface InputMessage|type InputMessage" -A 15 --type=tsRepository: AOSSIE-Org/Rein
Length of output: 901
🏁 Script executed:
# Check websocket.ts around line 99
sed -n '90,110p' src/server/websocket.tsRepository: AOSSIE-Org/Rein
Length of output: 658
🏁 Script executed:
# Check InputHandler.handleMessage method
rg "handleMessage" -A 30 --type=ts src/server/InputHandler.tsRepository: AOSSIE-Org/Rein
Length of output: 1274
🏁 Script executed:
# Get full InputHandler.ts to understand message handling
wc -l src/server/InputHandler.tsRepository: AOSSIE-Org/Rein
Length of output: 89
🏁 Script executed:
# Check if there's any usage of 'paste' or 'copy' type anywhere in the codebase
rg "paste|copy" --type=ts --type=js -i | grep -v node_modulesRepository: AOSSIE-Org/Rein
Length of output: 431
🏁 Script executed:
# Get the full handleMessage method
sed -n '50,145p' src/server/InputHandler.tsRepository: AOSSIE-Org/Rein
Length of output: 3479
🏁 Script executed:
# Also check the limits.ts to see the structure for paste/copy
cat -n src/server/config/limits.tsRepository: AOSSIE-Org/Rein
Length of output: 1035
paste and copy pass validation but are absent from InputMessage.
VALID_MESSAGE_TYPES includes 'paste' and 'copy', and the validator accepts them. However, the InputMessage interface (in InputHandler.ts) defines type as 'move' | 'click' | 'scroll' | 'key' | 'text' | 'zoom' | 'combo' — it does not include 'paste' or 'copy'. In websocket.ts Line 99, msg is cast to InputMessage, so a paste/copy message will pass validation, get cast, and then silently fall through the InputHandler.handleMessage switch with no action. Rate limits and UI controls for these types exist in ControlBar.tsx, suggesting they were intended. Either complete the implementation by adding 'paste' and 'copy' to the InputMessage type and handling them in InputHandler.handleMessage, or remove them from VALID_MESSAGE_TYPES.
🤖 Prompt for AI Agents
In `@src/server/middleware/InputValidator.ts` around lines 71 - 73,
VALID_MESSAGE_TYPES currently allows 'paste' and 'copy' but InputMessage (in
InputHandler.ts) lacks those types, so messages pass validation, get cast in
websocket.ts, and then silently fall through InputHandler.handleMessage; either
add 'paste' and 'copy' to the InputMessage union and implement handling in
InputHandler.handleMessage (and ensure websocket.ts casting and any
rate-limit/UI behavior in ControlBar.tsx are consistent), or remove 'paste' and
'copy' from VALID_MESSAGE_TYPES to prevent them being validated; update
whichever files you change (VALID_MESSAGE_TYPES, InputHandler.ts InputMessage
type, InputHandler.handleMessage, and websocket.ts cast) so the type system,
validator, and handler are aligned.
| if (!validator.isValid(msg)) { | ||
| console.warn('[Security] Invalid input:', msg.type); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Logging unsanitized msg.type from untrusted input.
When validator.isValid(msg) returns false, msg.type may not be from the allowed set (e.g., it could be an arbitrary string or object). Logging it directly could pollute logs or be exploited for log injection if logs are rendered in a web UI.
Truncate and stringify the type before logging:
Proposed fix
if (!validator.isValid(msg)) {
- console.warn('[Security] Invalid input:', msg.type);
+ console.warn('[Security] Invalid input:', String(msg.type).substring(0, 50));
return;
}📝 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.
| if (!validator.isValid(msg)) { | |
| console.warn('[Security] Invalid input:', msg.type); | |
| return; | |
| } | |
| if (!validator.isValid(msg)) { | |
| console.warn('[Security] Invalid input:', String(msg.type).substring(0, 50)); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@src/server/websocket.ts` around lines 92 - 95, When validator.isValid(msg)
returns false, avoid logging unsanitized msg.type; instead stringify and
sanitize/truncate it before logging to prevent log injection and huge entries.
Update the invalid-input branch around validator.isValid(msg) (the block that
currently calls console.warn('[Security] Invalid input:', msg.type)) to: obtain
a safe string from msg.type (e.g., String(msg.type) or JSON.stringify with
try/catch), remove newlines/control characters, and truncate to a reasonable
length (e.g., 100 chars) before passing to console.warn so only the
sanitized/truncated type is logged.
| // TEST 1: Rate Limiting | ||
| console.log("TEST 1: Rate Limiting - Move Events"); | ||
| console.log("Status: Dispatching 100 move events..."); | ||
|
|
||
| let processed = 0; | ||
| const startTime = Date.now(); | ||
|
|
||
| for (let i = 0; i < 100; i++) { | ||
| const result = await processMessage({ type: 'move', dx: 1, dy: 0 }); | ||
| if (result) processed++; | ||
| } | ||
|
|
||
| const duration = Date.now() - startTime; | ||
| console.log(`Summary: Processed ${processed}/100 in ${duration}ms`); | ||
| console.log(`Analysis: Expected approx 6 events for 60fps logic`); | ||
| console.log(""); |
There was a problem hiding this comment.
Rate-limiting test expectation ("approx 6 events") is environment-dependent and likely wrong.
The loop sends 100 move events in rapid succession. With a 16 ms throttle interval, the number of events that pass depends entirely on how long handler.handleMessage takes per call (real mouse I/O). Without the handler (if stubbed), the loop completes in under 1 ms, so only 1 event would pass. The "expected approx 6" comment is misleading.
If you stub the handler, either insert explicit sleep(16) delays between sends or adjust the expectation to 1 event for a tight loop.
🤖 Prompt for AI Agents
In `@test-shield.ts` around lines 36 - 51, The test's "expected approx 6 events"
assumption is incorrect because the loop dispatches 100 move events
synchronously and the actual number passing the 16ms throttle depends on real
handler latency; update the test in test-shield.ts to make the expectation
deterministic: either (A) stub or instrument
processMessage/handler.handleMessage and add an explicit sleep(16) between
iterations so ~6 events pass in ~100ms, or (B) keep the tight loop but change
the expected result to 1 event (or assert <= N) to match a synchronous fast
path; locate the loop using processMessage and the TEST 1 Rate Limiting comments
and apply one of these fixes so the test no longer relies on env timing.
1 participant
Problem Statement
The WebSocket server currently trusts all incoming JSON payloads without verification. This leads to cursor lag during high-frequency swipes and leaves the host vulnerable to crashes from malicious numeric data or memory exhaustion via oversized text strings.
Proposed Solution
I implemented a 3-layer defensive pipeline to harden the server:
NaN,Infinity, or malformed schemas before processing.Key Improvements
limits.tsfor easier maintainability.Evidence of Testing
I verified the integrity of the shield using a dedicated test suite (
test-shield.ts).NaN,Infinity, invalid enums) were rejected.How to Run the Tests
To verify these changes locally, run the following command from the project root:
Summary by CodeRabbit