Client Payload Validation and Automation Safety Enforcement Module#71
Client Payload Validation and Automation Safety Enforcement Module#71aniket866 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughInputHandler now enforces server-side input limits and validation: new private constants and a clamp helper, with early-return checks that clamp or reject out-of-range move, scroll, zoom, key, combo, and text inputs before invoking existing action calls. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/InputHandler.ts (2)
63-96:⚠️ Potential issue | 🟡 MinorWrap
case 'scroll'body in a block to scope its declarations.Biome correctly flags
const safeScrollYandconst safeScrollX(lines 67, 71) as accessible from sibling switch clauses. While thebreakprevents actual fallthrough, wrapping the case body in braces is the standard fix and prevents future accidents if cases are reordered.This same pattern applies to other cases with
constdeclarations (zoom,key,combo,text), butscrollis the one flagged.Proposed fix
case 'scroll': + { const promises: Promise<void>[] = []; //Clamp scroll values ... if (promises.length) { await Promise.all(promises); } break; + }
147-178:⚠️ Potential issue | 🟠 MajorDeduplication happens before lowercasing, so case-variant duplicates (e.g.
"Shift"and"shift") both survive and press the same physical key twice.Lowercase first, then deduplicate to avoid double key-presses. Additionally:
- Same single-char case loss as the
keyhandler — line 174 typeslowerKeyinstead of originalk.- Dead code on lines 175-177 — the
elsebranch is unreachable because the guard on line 164 already filters out keys that aren't inKEY_MAPand aren't single-char.🐛 Proposed fix: lowercase-then-dedup, preserve original char, remove dead code
- // Remove duplicate keys - const uniqueKeys = [...new Set(msg.keys)]; + // Normalize to lowercase, then remove duplicates + const uniqueKeys = [...new Set(msg.keys.map(k => k.toLowerCase()))]; const nutKeys: (Key | string)[] = []; - for (const k of uniqueKeys) { - const lowerKey = k.toLowerCase(); - - if (!(lowerKey in KEY_MAP) && lowerKey.length !== 1) { - console.warn(`Blocked unknown key in combo: ${k}`); + for (const lowerKey of uniqueKeys) { + if (!(lowerKey in KEY_MAP) && lowerKey.length !== 1) { + console.warn(`Blocked unknown key in combo: ${lowerKey}`); continue; } const nutKey = KEY_MAP[lowerKey]; if (nutKey !== undefined) { nutKeys.push(nutKey); } else if (lowerKey.length === 1) { nutKeys.push(lowerKey); - } else { - console.warn(`Unknown key in combo: ${k}`); } }Note: lowering single-char combo keys is acceptable here since combos like
Ctrl+Aconventionally use lowercasea. If original case matters for the single-char path, you'd need to carry both the lowered and original values.
🤖 Fix all issues with AI agents
In `@src/server/InputHandler.ts`:
- Around line 129-143: The single-character fallback currently lowercases the
key and loses original case; change the logic in InputHandler.ts so KEY_MAP
lookup still uses lowerKey but when falling back to typing a raw single
character use the original msg.key (preserving case) instead of lowerKey — i.e.,
compute lowerKey for the KEY_MAP lookup and nutKey resolution, call
keyboard.type(nutKey) if defined, otherwise call keyboard.type(msg.key) when
msg.key.length === 1.
- Around line 29-31: The clamp method currently allows NaN through; update
InputHandler.clamp to be static and to guard non-finite values by returning 0
when value is NaN or not finite (Number.isFinite(value) check), otherwise
perform the existing Math.max/Math.min clamp; then change call-sites that use
this.clamp(…) to InputHandler.clamp(…) (affects calls before mouse.setPosition,
mouse.scrollDown, mouse.* zoom/move callers) so NaN no longer propagates into
mouse methods.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 (1)
src/server/InputHandler.ts (1)
174-175:⚠️ Potential issue | 🟠 MajorSame lowercase-typing bug as the
keyhandler — single-char combo keys lose case.Line 175 uses
lowerKeyinstead of the originalk, so an uppercase letter in a combo is silently downcased.Proposed fix
} else if (lowerKey.length === 1) { - nutKeys.push(lowerKey); + nutKeys.push(k); } else {
🤖 Fix all issues with AI agents
In `@src/server/InputHandler.ts`:
- Around line 157-158: The current dedup logic uses const uniqueKeys = [...new
Set(msg.keys)] which is case-sensitive and lets logical duplicates like "Shift"
and "shift" pass through; update the InputHandler handling to normalize keys
(e.g., toLowerCase or a canonical mapping) before deduplication so duplicates
compare in a case-insensitive form, then map back to the original/canonical Key
enum values (e.g., Key.LeftShift) used elsewhere; specifically, transform
msg.keys to a normalized array, run new Set(...) on that normalized form, and
then convert those normalized entries to the canonical Key tokens used by the
rest of the code (refer to uniqueKeys, msg.keys, and the Key enum /
Key.LeftShift resolution).
🧹 Nitpick comments (1)
src/server/InputHandler.ts (1)
64-97: Wrapscrollcase body in a block to fixconstleaking across switch clauses.Biome correctly flags
constdeclarations inside an unbracedcase— they are technically accessible from othercasebranches, which can cause confusing bugs if the switch is later extended.Proposed fix
- case 'scroll': - const promises: Promise<void>[] = []; + case 'scroll': { + const promises: Promise<void>[] = []; //Clamp scroll values ... if (promises.length) { await Promise.all(promises); } - break; + break; + }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@imxade Please check this out , a small fix but important Thankyou! |
|
I'm not sure about this much restriction |
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 (1)
src/server/InputHandler.ts (1)
189-195:⚠️ Potential issue | 🟠 MajorCombo single-character fallback also lowercases the original key.
Same issue as the
keyhandler: line 192 typeslowerKeyinstead of the originalk, so uppercase single characters are silently downcased. The dedup logic on lines 167-175 correctly preserves the original key inuniqueKeys, but that's undone here.🐛 Proposed fix
if (nutKey !== undefined) { nutKeys.push(nutKey); } else if (lowerKey.length === 1) { - nutKeys.push(lowerKey); + nutKeys.push(k); } else { console.warn(`Unknown key in combo: ${k}`); }
🤖 Fix all issues with AI agents
In `@src/server/InputHandler.ts`:
- Around line 38-52: Resolve the Git conflict markers in InputHandler.ts by
removing the<<<<<<<, =======, and >>>>>>> lines and merging the two sides: first
validate msg.dx and msg.dy are numbers (use typeof checks and Number.isFinite)
and then apply the existing clamp logic via this.clamp(...,
-InputHandler.MAX_DELTA, InputHandler.MAX_DELTA) to produce safeDx/safeDy;
ensure references to InputHandler.MAX_DELTA, this.clamp, and safeDx/safeDy
remain consistent and no conflict markers remain in the file so it parses
correctly.
🧹 Nitpick comments (1)
src/server/InputHandler.ts (1)
189-195: Theelsebranch on line 193-195 is unreachable dead code.After the allowlist guard on lines 182-185, the remaining keys are either in
KEY_MAP(line 189) or single-character (line 191). Theelseon line 193 can never be reached. Consider removing it to avoid confusion.
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/InputHandler.ts (1)
75-101:⚠️ Potential issue | 🔴 Critical
safeScrollY/safeScrollXare dead variables — scroll is completely unprotected.The safe, clamped values are computed but never referenced again. The scroll conditions and amount variables still read the raw
msg.dy/msg.dx:
input ( msg.dy)safeScrollYMath.round(msg.dy) !== 0amountpassed to nut-jsInfinity0(clamped)true(Infinity !== 0)Infinity← crash riskNaN0(clamped)true(NaN !== 0)NaN← crash risk999999500(clamped)true999999← un-clamped
mouse.scrollDown(Infinity)/mouse.scrollUp(NaN)may crash or freeze nut-js — the exact class of failures this PR was introduced to prevent.🐛 Proposed fix: use the clamped values throughout the scroll conditions and amounts
// Vertical scroll - if (typeof msg.dy === 'number' && Math.round(msg.dy) !== 0) { - const amount = Math.round(msg.dy); + if (Math.round(safeScrollY) !== 0) { + const amount = Math.round(safeScrollY); if (amount > 0) { promises.push(mouse.scrollDown(amount).then(() => { })); } else { promises.push(mouse.scrollUp(-amount).then(() => { })); } } // Horizontal scroll - if (typeof msg.dx === 'number' && Math.round(msg.dx) !== 0) { - const amount = Math.round(msg.dx); + if (Math.round(safeScrollX) !== 0) { + const amount = Math.round(safeScrollX); if (amount > 0) { promises.push(mouse.scrollRight(amount).then(() => { })); } else { promises.push(mouse.scrollLeft(-amount).then(() => { })); } }The
typeofguard is redundant becausesafeScrollY/safeScrollXalready default to0whenmsg.dy/msg.dxare not numbers (lines 75-81).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 75 - 101, safeScrollY and safeScrollX are computed but never used, so untrusted msg.dy/msg.dx are still passed to nut-js; replace the typeof/msg.dy and typeof/msg.dx checks and Math.round(msg.*) uses with the clamped values: use Math.round(safeScrollY) and Math.round(safeScrollX) for the conditions and to compute amount, and pass that amount to mouse.scrollDown/scrollUp/scrollLeft/scrollRight; you can also remove the redundant typeof guards because safeScrollY/safeScrollX default to 0 when msg values are invalid.
🧹 Nitpick comments (2)
src/server/InputHandler.ts (2)
196-198: Unreachableelsebranch — dead code.The guard at lines 185–188 ensures every key reaching this block is either in
KEY_MAPor haslowerKey.length === 1. The first two branches in theif/else-iftherefore cover all reachable paths; theelsecan never execute.♻️ Remove dead branch
if (nutKey !== undefined) { nutKeys.push(nutKey); } else if (lowerKey.length === 1) { nutKeys.push(lowerKey); - } else { - console.warn(`Unknown key in combo: ${k}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 196 - 198, The else branch containing console.warn("Unknown key in combo: ${k}") in the combo parsing code is unreachable because prior guards (checking KEY_MAP and lowerKey.length === 1) already cover all possible keys; remove that dead else block from InputHandler's combo handling (the conditional that inspects KEY_MAP and lowerKey) and either delete the console.warn line or replace it with an explicit invariant/assert if you want a runtime sanity check; target the conditional handling around KEY_MAP and the lowerKey variable when editing.
38-57: Redundant guards:clampis called before thetypeof/Number.isFinitechecks, making non-finite handling inconsistent.
safeDx/safeDyare computed unconditionally (lines 41-42) and theclamputility correctly mapsInfinity/NaNto0. However, the innerNumber.isFinite(msg.dx)guard on line 47 then checks the original raw value, blocking all non-finite inputs entirely rather than moving by the clamped amount. This leftover from the merge-conflict resolution makes non-finite inputs no-ops while the PR description says they should be clamped.Simplest cleanup: remove the
Number.isFiniteredundancy and rely on the already-clamped values, mirroring howzoomhandles it:♻️ Suggested simplification
case 'move': if (msg.dx !== undefined && msg.dy !== undefined) { + if (typeof msg.dx !== 'number' || typeof msg.dy !== 'number') break; // Clamp dx/dy const safeDx = this.clamp(msg.dx, -InputHandler.MAX_DELTA, InputHandler.MAX_DELTA); const safeDy = this.clamp(msg.dy, -InputHandler.MAX_DELTA, InputHandler.MAX_DELTA); - if ( - typeof msg.dx === 'number' && - typeof msg.dy === 'number' && - Number.isFinite(msg.dx) && - Number.isFinite(msg.dy) - ) { - const currentPos = await mouse.getPosition(); - await mouse.setPosition(new Point( - Math.round(currentPos.x + safeDx), - Math.round(currentPos.y + safeDy) - )); - } + const currentPos = await mouse.getPosition(); + await mouse.setPosition(new Point( + Math.round(currentPos.x + safeDx), + Math.round(currentPos.y + safeDy) + )); } break;
clampalready returns0forInfinity/NaN, so theNumber.isFinitechecks on the raw values are superfluous after this restructuring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 38 - 57, The current code computes safeDx/safeDy via InputHandler.clamp but then still blocks non-finite inputs by checking Number.isFinite on the original msg.dx/msg.dy; remove the redundant Number.isFinite checks so the condition becomes only typeof msg.dx === 'number' && typeof msg.dy === 'number', and rely on clamp/ InputHandler.MAX_DELTA to sanitize Infinity/NaN into 0; keep using safeDx/safeDy when calling mouse.getPosition and mouse.setPosition with new Point to perform the move.
🤖 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/InputHandler.ts`:
- Around line 75-101: safeScrollY and safeScrollX are computed but never used,
so untrusted msg.dy/msg.dx are still passed to nut-js; replace the typeof/msg.dy
and typeof/msg.dx checks and Math.round(msg.*) uses with the clamped values: use
Math.round(safeScrollY) and Math.round(safeScrollX) for the conditions and to
compute amount, and pass that amount to
mouse.scrollDown/scrollUp/scrollLeft/scrollRight; you can also remove the
redundant typeof guards because safeScrollY/safeScrollX default to 0 when msg
values are invalid.
---
Duplicate comments:
In `@src/server/InputHandler.ts`:
- Around line 156-157: The single-character key handling uses lowerKey (losing
case) when it should use the original msg.key and the combo branch pushes
lowerKey into nutKeys; update the single-char branch in InputHandler (where
keyboard.type(lowerKey) is called) to use keyboard.type(msg.key) so case is
preserved, and in the combo loop change nutKeys.push(lowerKey) to
nutKeys.push(k) while still using lowerKey only for KEY_MAP lookups.
---
Nitpick comments:
In `@src/server/InputHandler.ts`:
- Around line 196-198: The else branch containing console.warn("Unknown key in
combo: ${k}") in the combo parsing code is unreachable because prior guards
(checking KEY_MAP and lowerKey.length === 1) already cover all possible keys;
remove that dead else block from InputHandler's combo handling (the conditional
that inspects KEY_MAP and lowerKey) and either delete the console.warn line or
replace it with an explicit invariant/assert if you want a runtime sanity check;
target the conditional handling around KEY_MAP and the lowerKey variable when
editing.
- Around line 38-57: The current code computes safeDx/safeDy via
InputHandler.clamp but then still blocks non-finite inputs by checking
Number.isFinite on the original msg.dx/msg.dy; remove the redundant
Number.isFinite checks so the condition becomes only typeof msg.dx === 'number'
&& typeof msg.dy === 'number', and rely on clamp/ InputHandler.MAX_DELTA to
sanitize Infinity/NaN into 0; keep using safeDx/safeDy when calling
mouse.getPosition and mouse.setPosition with new Point to perform the move.
2 participants
Addressed Issues: 30
In simple words this PR prevents any unkonwn input like infinite(9999999999999999999999999....) and other inputs by user ,
if not prevented it will crash the whole application because can't able to parse infinite, -infinite etc and all other input protection has bee packed into a allowed limit.
PR Summary
This PR strengthens the security and stability of
InputHandler.tsby introducing strict input validation and boundary enforcement.It prevents extreme client-sent values from crashing or freezing the host system.
All high-risk fields (dx, dy, delta, text, keys) are now properly clamped or validated.
These additions ensure predictable behavior while maintaining full backward compatibility.
What This Fix Prevents
Screenshots/Recordings:
Additional Notes:
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit