Fix security warnings reported by CodeQL #2641
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| res | ||
| .status(400) | ||
| .type("text/plain") | ||
| .send((error as Error).message); |
Check warning
Code scanning / CodeQL
Exception text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem, we should ensure that any error message sent in the response cannot be used to inject malicious content, even if the content type is changed or ignored. The best way to do this is to escape or sanitize the error message before sending it. Since the response is "text/plain", escaping is straightforward: we can ensure that the message does not contain control characters that could be interpreted as HTML or script. The simplest and most robust approach is to use a library like he (HTML Entities) to encode the message, or to manually escape <, >, and & characters. Alternatively, we can ensure that the error message is never directly derived from user input by sending a generic error message, but this may reduce debuggability.
The best fix is to escape the error message before sending it in the response. This can be done directly in the catch block in app.ts (lines 268-273). We can use the he library for robust escaping, or implement a minimal escape function if external dependencies are not desired.
Required changes:
- Add an import for the
helibrary at the top ofapp.ts. - In the catch block, escape the error message before sending it in the response.
| @@ -15,6 +15,7 @@ | ||
| import bcd from "@mdn/browser-compat-data" with {type: "json"}; | ||
| const bcdBrowsers = bcd.browsers; | ||
| import esMain from "es-main"; | ||
| import he from "he"; | ||
| import express, {Request, Response, NextFunction} from "express"; | ||
| import expressCache from "cache-express"; | ||
| import {expressCspHeader, INLINE, SELF, EVAL} from "express-csp-header"; | ||
| @@ -269,7 +270,7 @@ | ||
| res | ||
| .status(400) | ||
| .type("text/plain") | ||
| .send((error as Error).message); | ||
| .send(he.encode((error as Error).message)); | ||
| return; | ||
| } | ||
|
|
| @@ -71,7 +71,8 @@ | ||
| "ua-parser-js": "2.0.4", | ||
| "wasm-feature-detect": "^1.8.0", | ||
| "winston": "3.17.0", | ||
| "yargs": "18.0.0" | ||
| "yargs": "18.0.0", | ||
| "he": "^1.2.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@babel/core": "7.28.0", |
| Package | Version | Security advisories |
| he (npm) | 1.2.0 | None |
| res | ||
| .status(400) | ||
| .type("text/plain") | ||
| .send((error as Error).message); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem, we need to ensure that any user-controlled data included in error messages sent to the client is properly escaped to prevent XSS. The best way to do this is to escape the error message before sending it in the response. Since the response is sent as text/plain, escaping HTML entities is sufficient, but for defense-in-depth, we should use a well-known escaping library such as escape-html to sanitize the message. This change should be made in app.ts at the point where the error message is sent in the response (line 272). We need to import the escape-html library at the top of the file and use it to escape (error as Error).message before sending it.
| @@ -10,6 +10,7 @@ | ||
| import http from "node:http"; | ||
| import querystring from "node:querystring"; | ||
| import readline from "node:readline"; | ||
| import escapeHtml from "escape-html"; | ||
|
|
||
| import fs from "fs-extra"; | ||
| import bcd from "@mdn/browser-compat-data" with {type: "json"}; | ||
| @@ -269,7 +270,7 @@ | ||
| res | ||
| .status(400) | ||
| .type("text/plain") | ||
| .send((error as Error).message); | ||
| .send(escapeHtml((error as Error).message)); | ||
| return; | ||
| } | ||
|
|
| @@ -71,7 +71,8 @@ | ||
| "ua-parser-js": "2.0.4", | ||
| "wasm-feature-detect": "^1.8.0", | ||
| "winston": "3.17.0", | ||
| "yargs": "18.0.0" | ||
| "yargs": "18.0.0", | ||
| "escape-html": "^1.0.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@babel/core": "7.28.0", |
| Package | Version | Security advisories |
| escape-html (npm) | 1.0.3 | None |
| async (req: Request, res: Response) => { | ||
| const docs: Record<string, string> = {}; | ||
| for (const f of await fs.readdir(new URL("./docs", import.meta.url))) { | ||
| const readable = fs.createReadStream( | ||
| new URL(`./docs/${f}`, import.meta.url), | ||
| ); | ||
| const reader = readline.createInterface({input: readable}); | ||
| const line: string = await new Promise((resolve) => { | ||
| reader.on("line", (line) => { | ||
| reader.close(); | ||
| resolve(line); | ||
| }); | ||
| }); | ||
| }); | ||
| readable.close(); | ||
| readable.close(); | ||
|
|
||
| docs[f] = line.replace("# ", ""); | ||
| } | ||
| res.render("docs", { | ||
| docs, | ||
| }); | ||
| }); | ||
| docs[f] = line.replace("# ", ""); | ||
| } | ||
| res.render("docs", { | ||
| docs, | ||
| }); | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem, we should add a rate-limiting middleware to the /docs route to restrict the number of requests that can be made to this endpoint within a given time window. The best way to do this in an Express application is to use the well-known express-rate-limit package. This package allows us to easily configure rate limits and apply them to specific routes.
The fix involves:
- Importing the
express-rate-limitpackage at the top of the file. - Creating a rate limiter instance with reasonable defaults (e.g., 100 requests per 15 minutes).
- Applying the rate limiter as middleware to the
/docsroute (and optionally to other routes that perform expensive operations). - Ensuring that the fix does not change existing functionality except for adding rate limiting.
All changes should be made within the shown code in app.ts.
| @@ -16,6 +16,7 @@ | ||
| const bcdBrowsers = bcd.browsers; | ||
| import esMain from "es-main"; | ||
| import express, {Request, Response, NextFunction} from "express"; | ||
| import rateLimit from "express-rate-limit"; | ||
| import expressCache from "cache-express"; | ||
| import {expressCspHeader, INLINE, SELF, EVAL} from "express-csp-header"; | ||
| import cookieParser from "cookie-parser"; | ||
| @@ -38,6 +39,12 @@ | ||
| import getSecrets from "./lib/secrets.js"; | ||
| import {Report, ReportStore, Extensions, Exposure} from "./types/types.js"; | ||
|
|
||
| // Rate limiter for expensive routes | ||
| const docsRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| }); | ||
|
|
||
| type RequestWithSession = Request & { | ||
| session: expressSession.Session; | ||
| sessionID: string; | ||
| @@ -360,6 +367,7 @@ | ||
|
|
||
| app.get( | ||
| "/docs", | ||
| docsRateLimiter, | ||
| expressCache({ | ||
| timeOut: 60000, | ||
| }), |
| @@ -71,7 +71,8 @@ | ||
| "ua-parser-js": "2.0.4", | ||
| "wasm-feature-detect": "^1.8.0", | ||
| "winston": "3.17.0", | ||
| "yargs": "18.0.0" | ||
| "yargs": "18.0.0", | ||
| "express-rate-limit": "^8.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@babel/core": "7.28.0", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.0.1 | None |
This PR fixes a few of the security warnings reported by CodeQL.