🛡️ Sentinel: [HIGH] Fix CSRF vulnerability via Content-Type enforcement#100
🛡️ Sentinel: [HIGH] Fix CSRF vulnerability via Content-Type enforcement#100Dexploarer wants to merge 1 commit intomainfrom
Conversation
… to prevent CSRF - Updated `readJsonBody` in `src/api/http-helpers.ts` to reject requests without `Content-Type: application/json` by default. - Added `ReadJsonBodyOptions.requireContentType` (defaults to "application/json") to allow opting out if needed. - Updated test helpers `createMockIncomingMessage` to auto-inject Content-Type for JSON requests to fix regressions in existing tests. - Manually updated tests in `src/api/cloud-routes.test.ts` that were using partial mocks without correct headers. - Added `src/api/server.csrf.test.ts` to verify the security fix. - This prevents CSRF attacks where a malicious site could send a "simple request" (e.g. text/plain) with a JSON body that would be parsed and executed by the server.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Summary of ChangesHello @Dexploarer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a critical security fix to prevent Cross-Site Request Forgery (CSRF) attacks. Previously, the server's JSON body parsing logic did not validate the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| export function createMockReq(method: string, body?: string): IncomingMessage { | ||
| const headers: Record<string, string> = {}; | ||
| if (body) { | ||
| headers["content-type"] = "application/json"; | ||
| } | ||
| return createMockIncomingMessage({ | ||
| method, | ||
| headers: {}, | ||
| headers, | ||
| body, | ||
| }) as IncomingMessage; | ||
| } |
There was a problem hiding this comment.
Lack of Input Validation:
The createMockReq function does not validate the method or body parameters. Passing an invalid HTTP method or malformed body could result in misleading test outcomes or errors that are difficult to diagnose. Consider adding validation for the method (e.g., ensuring it is a recognized HTTP verb) and for the body (e.g., checking if it is valid JSON when the content-type is set to 'application/json').
Recommended Solution:
Add input validation logic before constructing the mock request:
const validMethods = ["GET", "POST", "PUT", "DELETE", ...];
if (!validMethods.includes(method)) {
throw new Error(`Invalid HTTP method: ${method}`);
}
if (body && headers["content-type"] === "application/json") {
try {
JSON.parse(body);
} catch {
throw new Error("Body is not valid JSON");
}
}| const contentType = req.headers["content-type"] || ""; | ||
| if (!contentType.toLowerCase().includes(requireContentType.toLowerCase())) { |
There was a problem hiding this comment.
Content-Type Validation Permissiveness
The content-type check uses a substring match (includes) which may allow unintended content types, such as application/json+xml, to pass validation. This could result in the server attempting to parse non-JSON payloads as JSON, potentially causing errors or unexpected behavior.
Recommendation:
Consider using a stricter comparison, such as checking for an exact match or using a regular expression to match only valid JSON content types:
if (!/^application\/json($|;)/i.test(contentType)) {
// ...
}| setTimeout(() => { | ||
| req.emit("data", Buffer.from(JSON.stringify({ command: "whoami" }))); | ||
| req.emit("end"); | ||
| }, 10); |
There was a problem hiding this comment.
The use of setTimeout to emit 'data' and 'end' events introduces a race condition: if readJsonBody does not properly await these events, the test may pass or fail inconsistently depending on timing. To ensure deterministic behavior, consider emitting the events synchronously before awaiting readJsonBody, or refactor the test to guarantee that the function under test is listening for events before they are emitted. Example:
process.nextTick(() => {
req.emit("data", Buffer.from(JSON.stringify({ command: "whoami" })));
req.emit("end");
});This approach ensures the events are emitted after the function has started listening, reducing the risk of race conditions.
|
|
||
| // Auto-inject Content-Type for JSON requests if missing, to satisfy CSRF protection in readJsonBody. | ||
| if (json && !headers["content-type"] && !headers["Content-Type"]) { | ||
| headers["content-type"] = "application/json"; |
There was a problem hiding this comment.
The code directly mutates the headers object by assigning to headers["content-type"] if the JSON flag is set and the header is missing. If the headers object is shared or reused elsewhere, this can lead to unintended side effects and test pollution.
Recommendation:
Clone the headers object before mutation to ensure test isolation:
const headersCopy = { ...headers };
if (json && !headersCopy["content-type"] && !headersCopy["Content-Type"]) {
headersCopy["content-type"] = "application/json";
}
req.headers = headersCopy;![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical CSRF vulnerability by enforcing the Content-Type header in readJsonBody. The implementation is robust, using a case-insensitive substring match to accommodate headers with character sets, and includes sensible defaults for error responses. The addition of dedicated tests for this new behavior in server.csrf.test.ts is excellent, and the updates to existing tests and test helpers ensure the test suite remains consistent with the new security requirement. My review includes a couple of suggestions to improve the maintainability and cleanliness of the test code.
| import { describe, it, expect, vi } from "vitest"; | ||
| import { EventEmitter } from "node:events"; | ||
| import { readJsonBody } from "./http-helpers"; | ||
|
|
||
| describe("CSRF Protection (Content-Type enforcement)", () => { | ||
| it("should REJECT JSON with text/plain (CSRF Protection)", async () => { | ||
| // Mock IncomingMessage | ||
| const req: any = new EventEmitter(); | ||
| req.headers = { "content-type": "text/plain" }; | ||
| req.off = vi.fn(); | ||
| req.destroy = vi.fn(); | ||
|
|
||
| // Mock ServerResponse | ||
| const res: any = { | ||
| statusCode: 200, | ||
| setHeader: vi.fn(), | ||
| end: vi.fn(), | ||
| writableEnded: false, | ||
| }; | ||
|
|
||
| // Simulate body stream (though it shouldn't be read if header check fails first, | ||
| // but our implementation checks header first) | ||
| // We don't strictly need to emit data if it fails early, but good for robustness. | ||
| setTimeout(() => { | ||
| req.emit("data", Buffer.from(JSON.stringify({ command: "whoami" }))); | ||
| req.emit("end"); | ||
| }, 10); | ||
|
|
||
| const body = await readJsonBody(req, res); | ||
|
|
||
| expect(body).toBeNull(); | ||
| expect(res.statusCode).toBe(415); | ||
| expect(res.end).toHaveBeenCalledWith(expect.stringContaining("Unsupported Media Type")); | ||
| }); | ||
|
|
||
| it("should ACCEPT JSON with application/json", async () => { | ||
| const req: any = new EventEmitter(); | ||
| req.headers = { "content-type": "application/json" }; | ||
| req.off = vi.fn(); | ||
| req.destroy = vi.fn(); | ||
|
|
||
| const res: any = { | ||
| statusCode: 200, | ||
| setHeader: vi.fn(), | ||
| end: vi.fn(), | ||
| writableEnded: false, | ||
| }; | ||
|
|
||
| setTimeout(() => { | ||
| req.emit("data", Buffer.from(JSON.stringify({ ok: true }))); | ||
| req.emit("end"); | ||
| }, 10); | ||
|
|
||
| const body = await readJsonBody(req, res); | ||
| expect(body).toEqual({ ok: true }); | ||
| }); | ||
|
|
||
| it("should ACCEPT JSON with application/json; charset=utf-8", async () => { | ||
| const req: any = new EventEmitter(); | ||
| req.headers = { "content-type": "application/json; charset=utf-8" }; | ||
| req.off = vi.fn(); | ||
| req.destroy = vi.fn(); | ||
|
|
||
| const res: any = { | ||
| statusCode: 200, | ||
| setHeader: vi.fn(), | ||
| end: vi.fn(), | ||
| writableEnded: false, | ||
| }; | ||
|
|
||
| setTimeout(() => { | ||
| req.emit("data", Buffer.from(JSON.stringify({ ok: true }))); | ||
| req.emit("end"); | ||
| }, 10); | ||
|
|
||
| const body = await readJsonBody(req, res); | ||
| expect(body).toEqual({ ok: true }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
For consistency with other tests in the codebase and to improve readability, consider refactoring these tests to use the existing createMockIncomingMessage and createMockHttpResponse test helpers from src/test-support/test-helpers.ts. This will make the tests more concise and easier to maintain by removing the manual setup of mock request and response objects.
import { describe, it, expect } from "vitest";
import { createMockHttpResponse, createMockIncomingMessage } from "../test-support/test-helpers";
import { readJsonBody } from "./http-helpers";
describe("CSRF Protection (Content-Type enforcement)", () => {
it("should REJECT JSON with text/plain (CSRF Protection)", async () => {
const req = createMockIncomingMessage({
headers: { "content-type": "text/plain" },
body: JSON.stringify({ command: "whoami" }),
});
const { res, getStatus, getJson } = createMockHttpResponse();
const body = await readJsonBody(req, res);
expect(body).toBeNull();
expect(getStatus()).toBe(415);
expect(getJson<{ error: string }>().error).toContain("Unsupported Media Type");
});
it("should ACCEPT JSON with application/json", async () => {
const req = createMockIncomingMessage({
headers: { "content-type": "application/json" },
body: JSON.stringify({ ok: true }),
});
const { res } = createMockHttpResponse();
const body = await readJsonBody(req, res);
expect(body).toEqual({ ok: true });
});
it("should ACCEPT JSON with application/json; charset=utf-8", async () => {
const req = createMockIncomingMessage({
headers: { "content-type": "application/json; charset=utf-8" },
body: JSON.stringify({ ok: true }),
});
const { res } = createMockHttpResponse();
const body = await readJsonBody(req, res);
expect(body).toEqual({ ok: true });
});
});| headers["content-type"] = "application/json"; | ||
| } | ||
|
|
||
| // console.log('[test-helper] Request headers:', headers); |
There was a problem hiding this comment.
This commented-out console.log appears to be a leftover from debugging. It should be removed to keep the code clean.
1 participant
🛡️ Sentinel: [HIGH] Fix CSRF vulnerability via Content-Type enforcement
Severity: HIGH
Vulnerability:
The
readJsonBodyhelper blindly parsed request bodies as JSON without verifying theContent-Typeheader. This allowed "simple requests" (e.g.,text/plainPOSTs) sent from a malicious origin (CSRF) to be processed by the server, bypassing CORS preflight checks. If the server was running locally (default unauthenticated mode), this could lead to Remote Code Execution (RCE) via endpoints like/api/terminal/run.Fix:
Content-Type: application/json(case-insensitive substring match) inreadJsonBody.415 Unsupported Media Type.Verification:
npx vitest run src/api/server.csrf.test.tsto verifying thattext/plainrequests are rejected andapplication/jsonrequests are accepted.npx vitest run src/api/to ensure no regressions in existing endpoints.PR created automatically by Jules for task 4413514494244013636 started by @Dexploarer