🛡️ Sentinel: [HIGH] Fix Missing CSRF Protection on Mutating API Routes#143
Open
aicoder2009 wants to merge 1 commit into
Open
🛡️ Sentinel: [HIGH] Fix Missing CSRF Protection on Mutating API Routes#143aicoder2009 wants to merge 1 commit into
aicoder2009 wants to merge 1 commit into
Conversation
Added `isSameOrigin` validation from `@/lib/security/rate-limit` to the following state-mutating POST endpoints: - `/api/lists` - `/api/lists/[id]/citations` - `/api/projects` - `/api/projects/[id]/lists` - `/api/share` - `/api/share/[code]/clone` This prevents cross-origin requests from forging state changes on behalf of an authenticated user. Included mocked tests to ensure proper execution. Co-authored-by: aicoder2009 <127642633+aicoder2009@users.noreply.github.com>
Contributor
|
👋 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request adds same-origin validation to six mutating API route handlers to prevent CSRF attacks. An ChangesCSRF Protection via Same-Origin Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens several authenticated, state-mutating Next.js Route Handlers against CSRF by enforcing the existing isSameOrigin check before processing POST bodies, returning 403 Forbidden for cross-origin requests.
Changes:
- Enforced
isSameOriginon multiple mutating API endpoints (lists, projects, citations, share links, and share cloning). - Updated Vitest route-handler tests to mock the new CSRF guard so existing test cases continue to run.
- Documented the CSRF finding and prevention guidance in the Jules Sentinel log.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/api/share/route.ts | Adds same-origin CSRF enforcement to share-link creation. |
| src/app/api/share/route.test.ts | Mocks CSRF guard for share route tests (needs explicit 403-path coverage). |
| src/app/api/share/[code]/clone/route.ts | Adds CSRF enforcement and changes handler request type to NextRequest (currently introduces a duplicate import lint error). |
| src/app/api/projects/route.ts | Adds same-origin CSRF enforcement to project creation. |
| src/app/api/projects/route.test.ts | Mocks CSRF guard for projects route tests (needs explicit 403-path coverage). |
| src/app/api/projects/[id]/lists/route.ts | Adds same-origin CSRF enforcement to list creation under a project. |
| src/app/api/projects/[id]/lists/route.test.ts | Mocks CSRF guard for project-lists route tests (needs explicit 403-path coverage). |
| src/app/api/lists/route.ts | Adds same-origin CSRF enforcement to list creation. |
| src/app/api/lists/route.test.ts | Mocks CSRF guard for lists route tests (needs explicit 403-path coverage). |
| src/app/api/lists/[id]/citations/route.ts | Adds same-origin CSRF enforcement to adding a citation to a list. |
| src/app/api/lists/[id]/citations/route.test.ts | Mocks CSRF guard for citations route tests (needs explicit 403-path coverage). |
| .jules/sentinel.md | Adds Sentinel entry documenting the CSRF issue and prevention guidance (date appears to have an incorrect year). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
20
| import { isSameOrigin } from "@/lib/security/rate-limit"; | ||
| import { NextRequest } from "next/server"; | ||
|
|
| **Learning:** `marked` does not sanitize HTML by default. While this may seem safe for trusted inputs (like internal docs or GitHub releases), if malicious input manages to enter these sources, it leads directly to an XSS vulnerability. | ||
| **Prevention:** The output of `marked` (or any markdown parser) must always be wrapped with `DOMPurify.sanitize()` (using `isomorphic-dompurify` for SSR) before being passed to `dangerouslySetInnerHTML`. | ||
|
|
||
| ## 2024-06-03 - Missing CSRF Protection on Mutating API Routes |
Comment on lines
+5
to
+9
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| rateLimit: vi.fn(() => ({ ok: true })), | ||
| getClientKey: vi.fn(), | ||
| })); |
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/share/[code]/clone/route.ts (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMerge duplicate
next/serverimport.
NextRequestis imported separately on Line 19 whileNextResponseis already imported fromnext/serveron Line 2. ESLint flags this (no-duplicate-imports/import/no-duplicates); consolidate into a single import.🔧 Proposed fix
-import { NextResponse } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; import { getShareLink, findListById, @@ import { parseShareSegment } from "`@/lib/share-utils`"; import { verifySharePassword } from "`@/lib/share-password`"; import { isSameOrigin } from "`@/lib/security/rate-limit`"; -import { NextRequest } from "next/server";Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/share/`[code]/clone/route.ts at line 2, Consolidate the duplicate imports from "next/server" by importing both NextRequest and NextResponse in a single statement (replace the separate NextRequest import with a combined import like import { NextRequest, NextResponse } from "next/server"); remove the redundant import so only one import from "next/server" remains and update any references to NextRequest/NextResponse accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/lists/`[id]/citations/route.test.ts:
- Around line 5-7: Tests currently mock isSameOrigin to always return true, so
there is no coverage that cross-origin POSTs are rejected; add a test in the
POST /api/lists/[id]/citations suite that temporarily mocks the isSameOrigin
guard to return false (using vi.fn or vi.mock), performs a POST to the route
handler, and asserts the response status is 403 Forbidden and body contains the
CSRF error. Target the isSameOrigin mock from "`@/lib/security/rate-limit`" and
update/restore the mock inside the test so other tests are unaffected.
In `@src/app/api/lists/route.test.ts`:
- Around line 5-7: Add a test in src/app/api/lists/route.test.ts that verifies
CSRF protection by making a cross-origin POST to the POST /api/lists route with
isSameOrigin mocked to return false; specifically, temporarily override the
existing vi.mock/vi.fn for isSameOrigin (or use vi.spyOn) to return false,
perform the POST call used by the suite, and assert the response status is 403
Forbidden; restore/reset the mock after the test to avoid affecting other tests.
In `@src/app/api/projects/`[id]/lists/route.test.ts:
- Around line 5-7: Add a test in the POST /api/projects/[id]/lists suite in
route.test.ts that verifies CSRF/origin protection by setting the mocked
isSameOrigin to return false (use the same "`@/lib/security/rate-limit`" mock and
mutate its isSameOrigin mock to false), perform a POST to the route under test,
and assert the response is rejected (e.g., status 403 and an error body
indicating CSRF/origin failure); restore or reset the isSameOrigin mock to its
original true behavior after the test so other tests remain unaffected.
In `@src/app/api/projects/route.test.ts`:
- Around line 5-7: Add a test in src/app/api/projects/route.test.ts inside the
"POST /api/projects" suite that verifies the CSRF guard rejects cross-origin
requests: update or add a mock for the isSameOrigin function (used in the
vi.mock("`@/lib/security/rate-limit`") setup) to return false for the cross-origin
case, perform a POST to the /api/projects route with a non-same-origin header,
and assert the response status is 403 Forbidden; ensure the test uses the
existing test helpers/setup in the file and restores the original mock behavior
after the case so other tests remain unaffected.
In `@src/app/api/share/route.test.ts`:
- Around line 5-9: The test suite currently mocks isSameOrigin to always return
true, so add a test in the POST /api/share suite that sets the isSameOrigin mock
to return false (e.g., vi.mock or isSameOrigin.mockReturnValueOnce(false)),
issues a POST to the /api/share route, and asserts the handler returns 403
Forbidden; reference the mocked symbols isSameOrigin, rateLimit, and
getClientKey and ensure the isSameOrigin mock is reset/restored after the test
so other tests are unaffected.
---
Outside diff comments:
In `@src/app/api/share/`[code]/clone/route.ts:
- Line 2: Consolidate the duplicate imports from "next/server" by importing both
NextRequest and NextResponse in a single statement (replace the separate
NextRequest import with a combined import like import { NextRequest,
NextResponse } from "next/server"); remove the redundant import so only one
import from "next/server" remains and update any references to
NextRequest/NextResponse accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc34c6f8-a336-46ab-ac71-2e6052f8e2a2
📒 Files selected for processing (12)
.jules/sentinel.mdsrc/app/api/lists/[id]/citations/route.test.tssrc/app/api/lists/[id]/citations/route.tssrc/app/api/lists/route.test.tssrc/app/api/lists/route.tssrc/app/api/projects/[id]/lists/route.test.tssrc/app/api/projects/[id]/lists/route.tssrc/app/api/projects/route.test.tssrc/app/api/projects/route.tssrc/app/api/share/[code]/clone/route.tssrc/app/api/share/route.test.tssrc/app/api/share/route.ts
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
There was a problem hiding this comment.
Add test coverage for the CSRF protection.
The mock allows existing tests to pass, but no test verifies that cross-origin POST requests are rejected with 403 Forbidden. The new isSameOrigin guard is a critical security feature and should be tested.
🧪 Proposed test case
Add this test to the POST /api/lists/[id]/citations suite:
+ it("should return 403 for cross-origin requests", async () => {
+ const { isSameOrigin } = await import("`@/lib/security/rate-limit`");
+ const mockIsSameOrigin = isSameOrigin as unknown as ReturnType<typeof vi.fn>;
+
+ mockAuth.mockResolvedValue({ userId: "user-123" });
+ mockIsSameOrigin.mockReturnValueOnce(false);
+
+ const request = new NextRequest("http://localhost/api/lists/list-123/citations", {
+ method: "POST",
+ body: JSON.stringify({
+ fields: sampleCitationFields,
+ style: "apa",
+ formattedText: "text",
+ formattedHtml: "<p>html</p>",
+ }),
+ });
+ const response = await POST(request, createParams("list-123"));
+ const data = await response.json();
+
+ expect(response.status).toBe(403);
+ expect(data.success).toBe(false);
+ expect(data.error).toBe("Forbidden");
+ });As per coding guidelines, API routes should have comprehensive test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/lists/`[id]/citations/route.test.ts around lines 5 - 7, Tests
currently mock isSameOrigin to always return true, so there is no coverage that
cross-origin POSTs are rejected; add a test in the POST
/api/lists/[id]/citations suite that temporarily mocks the isSameOrigin guard to
return false (using vi.fn or vi.mock), performs a POST to the route handler, and
asserts the response status is 403 Forbidden and body contains the CSRF error.
Target the isSameOrigin mock from "`@/lib/security/rate-limit`" and update/restore
the mock inside the test so other tests are unaffected.
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
There was a problem hiding this comment.
Add test coverage for the CSRF protection.
The mock bypasses the origin check, but no test verifies that cross-origin POST requests receive a 403 Forbidden response. The CSRF guard should be tested explicitly.
🧪 Proposed test case
Add this test to the POST /api/lists suite:
+ it("should return 403 for cross-origin requests", async () => {
+ const { isSameOrigin } = await import("`@/lib/security/rate-limit`");
+ const mockIsSameOrigin = isSameOrigin as unknown as ReturnType<typeof vi.fn>;
+
+ mockAuth.mockResolvedValue({ userId: "user-123" });
+ mockIsSameOrigin.mockReturnValueOnce(false);
+
+ const request = new NextRequest("http://localhost/api/lists", {
+ method: "POST",
+ body: JSON.stringify({ name: "New List" }),
+ });
+
+ const response = await POST(request);
+ const data = await response.json();
+
+ expect(response.status).toBe(403);
+ expect(data.success).toBe(false);
+ expect(data.error).toBe("Forbidden");
+ });As per coding guidelines, API routes should have comprehensive test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/lists/route.test.ts` around lines 5 - 7, Add a test in
src/app/api/lists/route.test.ts that verifies CSRF protection by making a
cross-origin POST to the POST /api/lists route with isSameOrigin mocked to
return false; specifically, temporarily override the existing vi.mock/vi.fn for
isSameOrigin (or use vi.spyOn) to return false, perform the POST call used by
the suite, and assert the response status is 403 Forbidden; restore/reset the
mock after the test to avoid affecting other tests.
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
There was a problem hiding this comment.
Add test coverage for the CSRF protection.
The mock allows existing tests to pass without triggering the origin check, but no test confirms that cross-origin POST requests are rejected. This security feature should be tested.
🧪 Proposed test case
Add this test to the POST /api/projects/[id]/lists suite:
+ it("should return 403 for cross-origin requests", async () => {
+ const { isSameOrigin } = await import("`@/lib/security/rate-limit`");
+ const mockIsSameOrigin = isSameOrigin as unknown as ReturnType<typeof vi.fn>;
+
+ mockAuth.mockResolvedValue({ userId: "user-123" });
+ mockIsSameOrigin.mockReturnValueOnce(false);
+
+ const request = new NextRequest("http://localhost/api/projects/project-123/lists", {
+ method: "POST",
+ body: JSON.stringify({ name: "New List" }),
+ });
+ const response = await POST(request, createParams("project-123"));
+ const data = await response.json();
+
+ expect(response.status).toBe(403);
+ expect(data.success).toBe(false);
+ expect(data.error).toBe("Forbidden");
+ });As per coding guidelines, API routes should have comprehensive test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/projects/`[id]/lists/route.test.ts around lines 5 - 7, Add a test
in the POST /api/projects/[id]/lists suite in route.test.ts that verifies
CSRF/origin protection by setting the mocked isSameOrigin to return false (use
the same "`@/lib/security/rate-limit`" mock and mutate its isSameOrigin mock to
false), perform a POST to the route under test, and assert the response is
rejected (e.g., status 403 and an error body indicating CSRF/origin failure);
restore or reset the isSameOrigin mock to its original true behavior after the
test so other tests remain unaffected.
Comment on lines
+5
to
+7
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| })); |
There was a problem hiding this comment.
Add test coverage for the CSRF protection.
The mock bypasses the origin validation, but no test verifies that cross-origin POST requests are rejected with 403 Forbidden. The CSRF guard should be tested.
🧪 Proposed test case
Add this test to the POST /api/projects suite:
+ it("should return 403 for cross-origin requests", async () => {
+ const { isSameOrigin } = await import("`@/lib/security/rate-limit`");
+ const mockIsSameOrigin = isSameOrigin as unknown as ReturnType<typeof vi.fn>;
+
+ mockAuth.mockResolvedValue({ userId: "user-123" });
+ mockIsSameOrigin.mockReturnValueOnce(false);
+
+ const request = new NextRequest("http://localhost/api/projects", {
+ method: "POST",
+ body: JSON.stringify({ name: "New Project" }),
+ });
+
+ const response = await POST(request);
+ const data = await response.json();
+
+ expect(response.status).toBe(403);
+ expect(data.success).toBe(false);
+ expect(data.error).toBe("Forbidden");
+ });As per coding guidelines, API routes should have comprehensive test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/projects/route.test.ts` around lines 5 - 7, Add a test in
src/app/api/projects/route.test.ts inside the "POST /api/projects" suite that
verifies the CSRF guard rejects cross-origin requests: update or add a mock for
the isSameOrigin function (used in the vi.mock("`@/lib/security/rate-limit`")
setup) to return false for the cross-origin case, perform a POST to the
/api/projects route with a non-same-origin header, and assert the response
status is 403 Forbidden; ensure the test uses the existing test helpers/setup in
the file and restores the original mock behavior after the case so other tests
remain unaffected.
Comment on lines
+5
to
+9
| vi.mock("@/lib/security/rate-limit", () => ({ | ||
| isSameOrigin: vi.fn(() => true), | ||
| rateLimit: vi.fn(() => ({ ok: true })), | ||
| getClientKey: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
Add test coverage for the CSRF protection.
The mock bypasses the origin check, but no test verifies that cross-origin POST requests are rejected with 403 Forbidden. This critical security feature should be tested.
🧪 Proposed test case
Add this test to the POST /api/share suite:
+ it("should return 403 for cross-origin requests", async () => {
+ const { isSameOrigin } = await import("`@/lib/security/rate-limit`");
+ const mockIsSameOrigin = isSameOrigin as unknown as ReturnType<typeof vi.fn>;
+
+ mockAuth.mockResolvedValue({ userId: "user-123" });
+ mockIsSameOrigin.mockReturnValueOnce(false);
+
+ const request = new NextRequest("http://localhost/api/share", {
+ method: "POST",
+ body: JSON.stringify({ type: "list", targetId: "list-123" }),
+ });
+ const response = await POST(request);
+ const data = await response.json();
+
+ expect(response.status).toBe(403);
+ expect(data.success).toBe(false);
+ expect(data.error).toBe("Forbidden");
+ });As per coding guidelines, API routes should have comprehensive test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/share/route.test.ts` around lines 5 - 9, The test suite currently
mocks isSameOrigin to always return true, so add a test in the POST /api/share
suite that sets the isSameOrigin mock to return false (e.g., vi.mock or
isSameOrigin.mockReturnValueOnce(false)), issues a POST to the /api/share route,
and asserts the handler returns 403 Forbidden; reference the mocked symbols
isSameOrigin, rateLimit, and getClientKey and ensure the isSameOrigin mock is
reset/restored after the test so other tests are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚨 Severity: HIGH
💡 Vulnerability: Next.js Route Handlers do not automatically protect against CSRF attacks. Several core authenticated POST endpoints (creating lists, projects, citations, and share links) lacked the
isSameOrigincheck, meaning an attacker could potentially forge a cross-origin request using an authenticated user's session to perform unauthorized state mutations.🎯 Impact: An attacker could trick a logged-in user into visiting a malicious site that sends POST requests to the OpenCitation API, creating lists, projects, or share links without their explicit consent.
🔧 Fix: Imported and enforced the existing
isSameOriginutility check from@/lib/security/rate-limit.tsto block cross-origin POSTs across all vulnerable endpoints, returning a403 Forbiddenresponse.✅ Verification: Verified via
pnpm test(tests updated with mocked CSRF check) and visual inspection of route handlers.PR created automatically by Jules for task 8950450425632066895 started by @aicoder2009
Summary by CodeRabbit
New Features
Tests