Update chat.ts #190
Update chat.ts #190
Conversation
Implemented safe message length limiting along with basic sanitization using escapeHTML, to avoid any issues like XSS when sending message
WalkthroughAdded HTML escaping and enhanced input validation to chat message processing in the chat service. Text is now sanitized through trimming, truncation to 1000 characters, and HTML entity escaping before being stored and broadcast. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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
🧹 Nitpick comments (1)
backend/src/chat/chat.ts (1)
78-78: Consider extracting the character limit to a named constant.The 1000-character limit is hardcoded here. Extracting it to a named constant (e.g.,
MAX_MESSAGE_LENGTH) at the top of the file alongsideMAX_HISTORYwould improve maintainability and make the limit easier to adjust in the future.Apply this diff to extract the constant:
const MAX_HISTORY = 300; +const MAX_MESSAGE_LENGTH = 1000; const roomHistories = new Map<string, HistItem[]>();Then update line 78:
- safeText = safeText.slice(0, 1000); + safeText = safeText.slice(0, MAX_MESSAGE_LENGTH);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/chat/chat.ts(3 hunks)
🔇 Additional comments (1)
backend/src/chat/chat.ts (1)
77-79: Excellent security improvements for XSS prevention.The three-step sanitization process (trim → truncate → escape) effectively prevents XSS attacks and limits message size. Using the
escape-htmllibrary is the standard approach for HTML entity escaping, and the defensivetoString()call handles edge cases where non-string values might be passed.
| // server/chat.ts | ||
| import type { Server, Socket } from "socket.io"; | ||
|
|
||
| import escapeHtml from "escape-html"; // npm install escape-html |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the escape-html dependency is installed.
Ensure that escape-html has been added to package.json dependencies. Without it, the application will fail at runtime with a module not found error.
Run the following script to verify the dependency exists:
🏁 Script executed:
#!/bin/bash
# Description: Check if escape-html is listed in package.json dependencies
cat package.json | jq '.dependencies["escape-html"], .devDependencies["escape-html"]'Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find package.json files and check escape-html dependency
# Locate all package.json files
echo "=== Locating package.json files ==="
fd -type f -name "package.json"
echo ""
echo "=== Checking backend/package.json for escape-html ==="
if [ -f "backend/package.json" ]; then
cat backend/package.json | jq '.dependencies["escape-html"], .devDependencies["escape-html"]'
else
echo "backend/package.json not found"
fiLength of output: 271
🏁 Script executed:
#!/bin/bash
# Display backend/package.json to confirm escape-html is missing
echo "=== Contents of backend/package.json ==="
cat backend/package.json | jq '.' | head -30Length of output: 756
Add escape-html to backend/package.json dependencies.
The import statement on line 3 references the escape-html package, but it is not listed in backend/package.json dependencies. This will cause a module not found error at runtime. Add "escape-html": "^1.4.2" (or appropriate version) to the dependencies section in backend/package.json.
🤖 Prompt for AI Agents
In backend/src/chat/chat.ts around line 3, the code imports "escape-html" but
backend/package.json does not list it as a dependency; add "escape-html":
"^1.4.2" (or an appropriate version) to the "dependencies" section of
backend/package.json, run npm install (or yarn) to update package-lock.json (or
yarn.lock), and commit the updated package.json and lockfile so the module is
available at runtime.
| if (!roomId || !text || !from) return; | ||
| let safeText = text.toString().trim(); | ||
| safeText = safeText.slice(0, 1000); | ||
| safeText = escapeHtml(safeText); |
There was a problem hiding this comment.
Validate text after trimming to prevent empty messages.
The current validation checks !text before trimming (line 76), but trimming happens afterward (line 77). This allows messages containing only whitespace (e.g., " ") to pass validation, resulting in empty messages being broadcast and stored after trimming.
Apply this diff to fix the validation order:
-
- if (!roomId || !text || !from) return;
- let safeText = text.toString().trim();
+ if (!roomId || !from) return;
+ let safeText = text?.toString().trim();
+ if (!safeText) return;
safeText = safeText.slice(0, 1000);
safeText = escapeHtml(safeText);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/src/chat/chat.ts around lines 76 to 79, the code validates `text`
before trimming which lets whitespace-only messages pass; change the order so
you convert to string and trim first, then validate the trimmed result (return
if empty), then apply the 1000-char slice and escapeHtml. Ensure the validation
checks the trimmed string (e.g., `if (!roomId || !from || safeText.length === 0)
return`) so messages that are only whitespace are rejected.
|
@ajatshatru01 the issue isn’t mentioned. Please make sure you’ve starred both repositories, joined our Discord server, and read the CONTRIBUTING.md before raising a PR. |
ok i will do it right now |
|
@ajatshatru01 in frontend there is no limit number please do update that as well |
i am not able to understand this, can you properly refer to what i am supposed to fix |
|
@ajatshatru01 like mention that we have the limit of 200 words in frontend something like that |
ok |
2 participants
Implemented safe message length limiting along with basic sanitization using escapeHTML, to avoid any issues like XSS when sending message
🌟 Pre-submission Checklist
#pull-requestchannel to ensure no one else is working on this issue#pull-requestchannelSummary
Brief description of what this PR accomplishes.
Converts text to a string and trims it.
Ensures it’s no longer than 1000 characters.
Escapes HTML (<, >, &, etc.) to prevent injection attacks.
Sends the safe text to all connected clients.
Stores the safe message in history.
Type of Changes
Testing Completed
Development Setup Verification
Code Quality
Related Issues
Closes #192
Screenshots/Videos
Additional Notes
Any additional information or context about the changes.
Note: For faster PR review and approval, ensure you're active in our Discord server!
Summary by CodeRabbit