Conversation
3ba305e to
108939f
Compare
bdd601e to
222eba4
Compare
222eba4 to
2bfb339
Compare
2bfb339 to
fb50fec
Compare
sdk/webpubsub-chat-sdk/examples/teams-lite/client/src/components/Sidebar.tsx
Fixed
Show fixed
Hide fixed
fb50fec to
c4ded8d
Compare
| const sender = lastMessage.sender || 'Unknown'; | ||
| // Strip HTML tags to get plain text | ||
| const rawContent = lastMessage.content; | ||
| const content = rawContent.replace(/<[^>]*>/g, '').replace(/ /g, ' ').trim(); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the problem should be fixed by replacing the ad hoc regex-based HTML stripping with a well-tested sanitization strategy that correctly handles all HTML/script tag edge cases. For a React frontend, the safest approach is to sanitize incoming HTML and then extract plain text in a way that does not rely on fragile multi-character regexes.
The single best way to fix this, without changing visible functionality, is to parse the message content as HTML using the browser’s DOM parser, extract its plain text, and then normalize whitespace. This removes all tags and their attributes, regardless of how they are nested or split, and avoids the multi-character-sanitization problem completely. Specifically, in formatMessagePreview, replace:
const rawContent = lastMessage.content;
const content = rawContent.replace(/<[^>]*>/g, '').replace(/ /g, ' ').trim();with a helper that converts HTML to text via DOMParser (or a fallback <div> element) and then trims and normalizes spaces. This change happens entirely within Sidebar.tsx, does not require new dependencies, and preserves the rest of the logic (length limiting, private vs group handling).
Concretely:
- Add a small helper function
stripHtmlTags(orhtmlToText) insideSidebar.tsxthat:- Accepts a string.
- Uses
DOMParser(if available) to parse HTML and returntextContent. - Falls back to creating a
<div>, settinginnerHTML, and readingtextContentifDOMParseris unavailable. - Replaces non‑breaking spaces (
\u00A0) with normal spaces and trims.
- Call this helper from
formatMessagePreviewinstead of using the regex.replace(/<[^>]*>/g, '').
No new imports are required because both DOMParser and document.createElement are standard browser APIs in this client-side code.
| @@ -10,6 +10,37 @@ | ||
|
|
||
| export const Sidebar: React.FC = () => { | ||
| const settings = useContext(ChatSettingsContext); | ||
|
|
||
| // Safely convert HTML content to plain text without relying on fragile regexes. | ||
| const stripHtmlToText = (input: string | undefined | null): string => { | ||
| if (!input) { | ||
| return ''; | ||
| } | ||
|
|
||
| // Prefer DOMParser when available | ||
| if (typeof DOMParser !== 'undefined') { | ||
| try { | ||
| const parser = new DOMParser(); | ||
| const doc = parser.parseFromString(input, 'text/html'); | ||
| const text = doc.body ? doc.body.textContent || '' : ''; | ||
| // Normalize non-breaking spaces and trim | ||
| return text.replace(/\u00A0/g, ' ').trim(); | ||
| } catch { | ||
| // Fallback to div-based parsing below | ||
| } | ||
| } | ||
|
|
||
| // Fallback: use a detached DOM element | ||
| if (typeof document !== 'undefined' && document.createElement) { | ||
| const div = document.createElement('div'); | ||
| div.innerHTML = input; | ||
| const text = div.textContent || div.innerText || ''; | ||
| return text.replace(/\u00A0/g, ' ').trim(); | ||
| } | ||
|
|
||
| // As a last resort, return the original string trimmed | ||
| return String(input).trim(); | ||
| }; | ||
| const [isCreateDialogOpen, setIsCreateDialogOpen] = useState(false); | ||
| const [isCreating, setIsCreating] = useState(false); | ||
| const [searchQuery, setSearchQuery] = useState(""); | ||
| @@ -70,9 +101,9 @@ | ||
| if (!lastMessage) return 'No messages yet'; | ||
|
|
||
| const sender = lastMessage.sender || 'Unknown'; | ||
| // Strip HTML tags to get plain text | ||
| // Convert potential HTML content to plain text safely | ||
| const rawContent = lastMessage.content; | ||
| const content = rawContent.replace(/<[^>]*>/g, '').replace(/ /g, ' ').trim(); | ||
| const content = stripHtmlToText(rawContent); | ||
| const maxLength = 20; // Adjust based on your UI needs | ||
| const isPrivateChat = roomId.startsWith('private-'); | ||
|
|
| await chatClient.createRoom(`${userId} (You)`, [userId], selfChatRoomId); | ||
| } catch (err) { | ||
| // Room may already exist, ignore error | ||
| console.log(`Self-chat room for ${userId} may already exist:`, err.message); |
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix externally-controlled format string issues with console.log/util.format, don’t let untrusted data participate in the format string. Instead, use a constant format string (with %s placeholders if needed) and pass untrusted values as separate arguments, or pre-concatenate them into a single string that is then logged alone.
For this specific case in sdk/webpubsub-chat-sdk/examples/teams-lite/server/index.js, line 53 currently builds a template literal containing userId and then passes err.message as a second argument:
console.log(`Self-chat room for ${userId} may already exist:`, err.message);We can make the format string constant and pass userId and err.message as arguments to that format string. This preserves the log content while eliminating the untrusted format string. A good, minimal change is:
console.log('Self-chat room for %s may already exist: %s', userId, err.message);No new imports or helpers are required; we only change the single console.log call in the catch block around chatClient.createRoom (around lines 51–54). Functionality remains the same: the log still reports the userId and the error message, but any % characters inside userId or err.message are treated as data, not format specifiers.
| @@ -50,7 +50,7 @@ | ||
| await chatClient.createRoom(`${userId} (You)`, [userId], selfChatRoomId); | ||
| } catch (err) { | ||
| // Room may already exist, ignore error | ||
| console.log(`Self-chat room for ${userId} may already exist:`, err.message); | ||
| console.log('Self-chat room for %s may already exist: %s', userId, err.message); | ||
| } | ||
| } | ||
| res.json({ |
|
|
||
| export async function createTestClient(userId?: string): Promise<ChatClient> { | ||
| if (!userId) { | ||
| userId = `uid-${randomInt()}`; |
Check failure
Code scanning / CodeQL
Insecure randomness High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix insecure randomness you must replace Math.random() with a cryptographically secure source such as crypto.randomBytes in Node.js or crypto.getRandomValues in the browser, and then derive your needed values from that secure source.
Here, the best fix is to change the implementation of randomInt so it no longer uses Math.random() and instead uses Node’s crypto module. We keep the same API (randomInt(): number) and roughly the same output range so existing behavior (tests creating user IDs like uid-<big number>) is preserved except for stronger randomness. A straightforward, bias‑free approach is to use crypto.randomInt(0, 10000000). If we want to avoid relying on newer Node APIs, we can instead use crypto.randomBytes and map bytes into the requested range; however, crypto.randomInt is part of Node’s standard library and avoids manual bias concerns.
Concretely:
- Add an import for
randomInt(renamed to avoid collision) from Node’scryptomodule. - Rewrite the local
randomInthelper to callcryptoRandomInt(0, 10000000)instead ofMath.random() * 10000000. - No other lines need to change because all usages (
getUserIds,createTestClient) will automatically use the stronger randomness.
All changes are within sdk/webpubsub-chat-sdk/tests/testUtils.ts:
- Add
import { randomInt as cryptoRandomInt } from "crypto";near the top. - Change the definition of
randomInton line 10 to usecryptoRandomInt.
| @@ -1,5 +1,6 @@ | ||
| import { WebPubSubClient } from "@azure/web-pubsub-client"; | ||
| import { ChatClient } from "../src/chatClient.js"; | ||
| import { randomInt as cryptoRandomInt } from "crypto"; | ||
|
|
||
| // Test configuration | ||
| export const negotiateUrl = "http://localhost:3000/negotiate"; | ||
| @@ -7,7 +8,7 @@ | ||
| export const LONG_TEST_TIMEOUT = 10 * 1000; | ||
|
|
||
| // Helper functions | ||
| export const randomInt = () => Math.floor(Math.random() * 10000000); | ||
| export const randomInt = () => cryptoRandomInt(0, 10000000); | ||
|
|
||
| export const getUserIds = (count: number): string[] => { | ||
| const userIds: string[] = []; |
c4ded8d to
256538c
Compare
| app.get('{*path}', (req, res, next) => { | ||
| if (req.path.startsWith('/api/') || req.path.startsWith('/eventhandler')) { | ||
| return next(); | ||
| } | ||
| res.sendFile(join(clientPath, 'index.html')); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem in general, add a rate-limiting middleware to the Express application and apply it to the route (or routes) that perform file-system access or other expensive operations. A commonly used solution in the Node/Express ecosystem is the express-rate-limit package, which allows you to define a window and a maximum number of requests per IP within that window. The middleware should be applied before the app.get('{*path}', ...) handler so that excessive requests are blocked or slowed before reaching res.sendFile.
In this specific file, the minimal-impact fix is:
- Import
express-rate-limitat the top (without touching existing imports). - Define a rate limiter instance, e.g.,
fileAccessLimiter, configured with a reasonable window and max requests. - Apply that limiter specifically to the
app.get('{*path}', ...)route (or also toexpress.staticif desired), so that requests for the static index page are constrained. This preserves existing behavior for legitimate users while mitigating abuse. - Keep all existing logic inside the route intact; only prepend the limiter as middleware.
Concretely:
- In
sdk/webpubsub-chat-sdk/examples/teams-lite/server/index.js, add an import forexpress-rate-limitalongside the existing imports. - After creating
const app = express();(or near other configuration constants), defineconst fileAccessLimiter = rateLimit({...}). - Change
app.get('{*path}', (req, res, next) => { ... })toapp.get('{*path}', fileAccessLimiter, (req, res, next) => { ... }).
| @@ -6,6 +6,7 @@ | ||
| import { WebPubSubEventHandler } from '@azure/web-pubsub-express'; | ||
| import { ChatClient } from '@azure/web-pubsub-chat-client'; | ||
| import { WebPubSubClient } from '@azure/web-pubsub-client'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = dirname(__filename); | ||
| @@ -63,7 +64,13 @@ | ||
| // Serve static files from client folder (for production deployment) | ||
| const clientPath = join(__dirname, 'client'); | ||
| app.use(express.static(clientPath)); | ||
| app.get('{*path}', (req, res, next) => { | ||
|
|
||
| const fileAccessLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs for index.html | ||
| }); | ||
|
|
||
| app.get('{*path}', fileAccessLimiter, (req, res, next) => { | ||
| if (req.path.startsWith('/api/') || req.path.startsWith('/eventhandler')) { | ||
| return next(); | ||
| } |
| @@ -18,7 +18,8 @@ | ||
| "@azure/web-pubsub-express": "^1.0.6", | ||
| "cors": "^2.8.5", | ||
| "dotenv": "^16.4.5", | ||
| "express": "^5.2.1" | ||
| "express": "^5.2.1", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "esbuild": "^0.24.0" |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
No description provided.