🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in bulk lookup API#197
🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in bulk lookup API#197aicoder2009 wants to merge 1 commit into
Conversation
Removed loopback fetches relying on request.nextUrl.origin which is susceptible to Host header spoofing. Instead, route handlers are now directly imported and invoked with synthetic requests. 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.
|
📝 WalkthroughWalkthroughThe bulk lookup API route ( ChangesSSRF Fix: Bulk Lookup Direct Handler Dispatch
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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
🤖 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/lookup/bulk/route.test.ts`:
- Around line 3-10: The mocks for the URL/DOI/ISBN handlers are too permissive
and don’t verify the forwarded request body shape, so regressions in the bulk
route contract can slip through. Update the tests around bulk lookup to use the
mocked POST handlers from urlHandler, doiHandler, and isbnHandler with
mockImplementationOnce and inspect await req.json() inside each mock to assert
the expected payload keys ({ url }, { doi }, { isbn }). Keep the assertions tied
to the bulk route’s forwarding behavior so the tests fail if the wrong JSON
shape is sent.
🪄 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: b8cb0fa8-b061-4b20-90d5-a0e078142dd7
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| import { NextRequest, NextResponse } from 'next/server'; | ||
| import { POST as urlHandler } from '../url/route'; | ||
| import { POST as doiHandler } from '../doi/route'; | ||
| import { POST as isbnHandler } from '../isbn/route'; | ||
|
|
||
| vi.mock('../url/route', () => ({ POST: vi.fn() })); | ||
| vi.mock('../doi/route', () => ({ POST: vi.fn() })); | ||
| vi.mock('../isbn/route', () => ({ POST: vi.fn() })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the forwarded JSON body in these handler mocks.
Right now these stubs succeed for any request, so a regression from { url } / { doi } / { isbn } to the wrong payload shape would still pass even though the real sub-routes reject it. Switching these to mockImplementationOnce and checking await req.json() locks the test to the actual cross-route contract, and it also gives NextResponse at Line 3 a real use.
Proposed test pattern
- (urlHandler as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
- ok: true,
- json: async () => ({ data: { title: 'Example Page' } }),
- });
+ (urlHandler as ReturnType<typeof vi.fn>).mockImplementationOnce(async (req: NextRequest) => {
+ expect(await req.json()).toEqual({ url: 'https://example.com' });
+ return NextResponse.json({ data: { title: 'Example Page' } });
+ });As per coding guidelines, "Write tests for citation formatting and API route logic."
Also applies to: 62-97, 108-115, 126-133
🧰 Tools
🪛 ESLint
[error] 3-3: 'NextResponse' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 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 3 - 10, The mocks for the
URL/DOI/ISBN handlers are too permissive and don’t verify the forwarded request
body shape, so regressions in the bulk route contract can slip through. Update
the tests around bulk lookup to use the mocked POST handlers from urlHandler,
doiHandler, and isbnHandler with mockImplementationOnce and inspect await
req.json() inside each mock to assert the expected payload keys ({ url }, { doi
}, { isbn }). Keep the assertions tied to the bulk route’s forwarding behavior
so the tests fail if the wrong JSON shape is sent.
Sources: Coding guidelines, Linters/SAST tools
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The bulk lookup API (
/api/lookup/bulk) usedfetch()with dynamically derived hostnames (request.nextUrl.origin) for loopback requests to internal route handlers. This is an SSRF vulnerability becauserequest.nextUrl.originis constructed using the user-controllableHost(orX-Forwarded-Host) header.🎯 Impact: An attacker could spoof the
Hostheader to trick the internal server into making HTTPPOSTrequests to arbitrary external domains, potentially exposing internal server behavior or acting as a proxy for malicious activity.🔧 Fix: Removed the HTTP loopback mechanism. Instead, the specific Next.js route handlers (
url,doi,isbn) are directly imported and invoked as functions, passing a syntheticNextRequestobject. This eliminates the network layer parsing of the Host header entirely.✅ Verification:
Hostheader (it should no longer redirect traffic).route.test.tshas been refactored to check thatdoiHandler,urlHandler, andisbnHandlerare directly invoked instead ofglobal.fetch. All tests pass.PR created automatically by Jules for task 9423814406976627706 started by @aicoder2009
Summary by CodeRabbit