🛡️ Sentinel: [HIGH] Fix SSRF loopback vulnerability in bulk lookup API#150
🛡️ Sentinel: [HIGH] Fix SSRF loopback vulnerability in bulk lookup API#150aicoder2009 wants to merge 1 commit into
Conversation
Directly invoke route handlers instead of performing loopback HTTP requests using `request.nextUrl.origin`, mitigating potential SSRF vulnerabilities. Co-authored-by: aicoder2009 <127642633+aicoder2009@users.noreply.github.com>
|
👋 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 PR remediates an SSRF vulnerability in the bulk lookup route by replacing fetch-based internal API calls (which derived the origin from the client's ChangesSSRF Vulnerability Remediation
Sequence DiagramsequenceDiagram
participant Client
participant BulkRoute as POST /bulk
participant URLHandler as URL Handler
participant DOIHandler as DOI Handler
Client->>BulkRoute: POST bulk request (items)
loop For each item
alt URL item
BulkRoute->>BulkRoute: Select URLHandler, create payload
BulkRoute->>URLHandler: invoke with NextRequest
URLHandler-->>BulkRoute: NextResponse
else DOI item
BulkRoute->>BulkRoute: Select DOIHandler, create payload
BulkRoute->>DOIHandler: invoke with NextRequest
DOIHandler-->>BulkRoute: NextResponse
end
end
BulkRoute-->>Client: bulk results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
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/lookup/bulk/route.ts (1)
30-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate item type before calling
trim()to prevent batch-wide 500s.Line 31 assumes every entry is a string. A payload like
{ items: [123] }throws before per-item error handling, rejectsPromise.all, and returns a 500 for the entire request.🛠️ Suggested fix
const lookupPromises = items.map(async (item) => { + if (typeof item !== "string") { + return { + input: String(item), + success: false, + error: "Each item must be a string" + }; + } + const trimmedItem = item.trim(); if (!trimmedItem) { return { input: item, success: false, error: "Empty input" }; }🤖 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/lookup/bulk/route.ts` around lines 30 - 33, The map callback for lookupPromises calls item.trim() without validating the type, which can throw for non-strings and cause Promise.all to reject; update the items.map async callback (the function producing lookupPromises) to first check typeof item === "string" (or safely coerce with String(item) if desired) and if it's not a string immediately return the per-item error shape ({ input: item, success: false, error: "Invalid input type" }) instead of calling trim(); ensure the branch that calls trim() only runs for string values so individual bad items produce per-item errors rather than crashing the whole batch.
🧹 Nitpick comments (1)
src/app/api/lookup/bulk/route.test.ts (1)
61-133: ⚡ Quick winAdd a dedicated hostile-origin regression test for the SSRF fix.
Current tests validate routing outcomes, but none explicitly lock in that a malicious request origin/Host cannot influence internal dispatch again.
✅ Suggested test addition
+ it('ignores request origin when dispatching internal lookups', async () => { + (urlPOST as ReturnType<typeof vi.fn>).mockResolvedValueOnce( + NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 }) + ); + + const request = new NextRequest('http://attacker.example/api/lookup/bulk', { + method: 'POST', + body: JSON.stringify({ items: ['https://example.com'] }), + headers: { host: 'attacker.example' }, + }); + + const response = await POST(request); + const data = await response.json(); + + expect(response.status).toBe(200); + expect(data.results[0].success).toBe(true); + expect(urlPOST).toHaveBeenCalledTimes(1); + });As per coding guidelines,
src/app/api/**/*.ts: Write tests for API route logic.🤖 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/lookup/bulk/route.test.ts` around lines 61 - 133, Tests exercise routing but lack a regression test ensuring a hostile Host/origin header cannot influence dispatch; add a new test in route.test.ts that constructs a request via makeRequest with items (e.g., a URL and a DOI) while setting a malicious Host/Origin header, then calls POST and asserts routing still calls the correct handlers (urlPOST, doiPOST, isbnPOST as appropriate), that results reflect the mocked responses, and that no routing decision depends on the Host header (e.g., urlPOST/doiPOST are invoked regardless of the hostile header) to lock in the SSRF fix.Source: Coding guidelines
🤖 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 @.jules/sentinel.md:
- Line 10: The header line "## 2024-06-07 - SSRF via Host-header in Next.js
Loopback APIs" has the wrong year; update that date string to "2026-06-07"
(matching the PR created on June 7, 2026) keeping the same header format so it
reads "## 2026-06-07 - SSRF via Host-header in Next.js Loopback APIs".
---
Outside diff comments:
In `@src/app/api/lookup/bulk/route.ts`:
- Around line 30-33: The map callback for lookupPromises calls item.trim()
without validating the type, which can throw for non-strings and cause
Promise.all to reject; update the items.map async callback (the function
producing lookupPromises) to first check typeof item === "string" (or safely
coerce with String(item) if desired) and if it's not a string immediately return
the per-item error shape ({ input: item, success: false, error: "Invalid input
type" }) instead of calling trim(); ensure the branch that calls trim() only
runs for string values so individual bad items produce per-item errors rather
than crashing the whole batch.
---
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 61-133: Tests exercise routing but lack a regression test ensuring
a hostile Host/origin header cannot influence dispatch; add a new test in
route.test.ts that constructs a request via makeRequest with items (e.g., a URL
and a DOI) while setting a malicious Host/Origin header, then calls POST and
asserts routing still calls the correct handlers (urlPOST, doiPOST, isbnPOST as
appropriate), that results reflect the mocked responses, and that no routing
decision depends on the Host header (e.g., urlPOST/doiPOST are invoked
regardless of the hostile header) to lock in the SSRF fix.
🪄 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: c113b861-d6f7-48d2-9e83-18c0a7785c0f
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| **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-07 - SSRF via Host-header in Next.js Loopback APIs |
There was a problem hiding this comment.
Correct the SSRF note date to match the remediation timeline.
Line 10 uses 2024-06-07, but this fix is tracked in a PR created on June 7, 2026. Keeping the date aligned avoids audit/incident confusion.
✏️ Suggested edit
-## 2024-06-07 - SSRF via Host-header in Next.js Loopback APIs
+## 2026-06-07 - SSRF via Host-header in Next.js Loopback APIs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2024-06-07 - SSRF via Host-header in Next.js Loopback APIs | |
| ## 2026-06-07 - SSRF via Host-header in Next.js Loopback APIs |
🤖 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 @.jules/sentinel.md at line 10, The header line "## 2024-06-07 - SSRF via
Host-header in Next.js Loopback APIs" has the wrong year; update that date
string to "2026-06-07" (matching the PR created on June 7, 2026) keeping the
same header format so it reads "## 2026-06-07 - SSRF via Host-header in Next.js
Loopback APIs".
🚨 Severity: HIGH
💡 Vulnerability: The bulk API endpoint (
src/app/api/lookup/bulk/route.ts) was usingrequest.nextUrl.originto construct an absolute URL for afetchcall to other internal APIs. Becauserequest.nextUrl.originis derived from the HTTPHostheader sent by the client, an attacker could spoof theHostheader to cause the server to make requests to arbitrary external servers or internal IP addresses (SSRF).🎯 Impact: This could be exploited to bypass security controls, interact with internal networks, or launch DoS attacks.
🔧 Fix: Instead of using
fetchto make a loopback HTTP request, the code now imports the other route handler functions (e.g.,POSTfrom../url/route) directly and invokes them as normal asynchronous function calls. A syntheticNextRequestis passed to satisfy the handler's parameters.✅ Verification: Ran
pnpm test:runto ensure the bulk API works securely and passes all tests.PR created automatically by Jules for task 1604677428229134358 started by @aicoder2009
Summary by CodeRabbit
Bug Fixes
Refactor