🛡️ Sentinel: [HIGH] Fix Host-header SSRF in bulk lookup API#155
🛡️ Sentinel: [HIGH] Fix Host-header SSRF in bulk lookup API#155aicoder2009 wants to merge 1 commit into
Conversation
Removed loopback fetch calls utilizing user-controllable Host headers in `/api/lookup/bulk/route.ts` which presented a Host-header SSRF vulnerability. Mitigated this by importing and directly invoking internal route handlers (`../url/route`, `../doi/route`, and `../isbn/route`) utilizing a synthetic NextRequest, increasing security and lowering request latency. 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 eliminates a Host-header SSRF vulnerability in the bulk lookup API by replacing loopback HTTP fetch calls with direct in-process handler invocation. The security issue, mitigation approach, implementation refactoring, and test updates are coordinated to remove the fetch-based origin dependency while maintaining identical external behavior. ChangesSSRF Vulnerability Mitigation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
🧹 Nitpick comments (1)
src/app/api/lookup/bulk/route.test.ts (1)
69-77: ⚡ Quick winAssert dispatched payloads, not only handler invocation.
These tests currently prove routing path selection, but not JSON contract correctness (
{ url },{ doi },{ isbn }). Add body assertions on the mockedNextRequestargument to catch silent contract regressions.Example assertion pattern
it('routes URLs to /api/lookup/url', async () => { (urlPost as ReturnType<typeof vi.fn>).mockResolvedValueOnce( NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 }) ); const response = await POST(makeRequest({ items: ['https://example.com'] })); const data = await response.json(); expect(data.results[0].success).toBe(true); expect(data.results[0].data.title).toBe('Example Page'); expect(urlPost).toHaveBeenCalled(); + const req = (urlPost as ReturnType<typeof vi.fn>).mock.calls[0][0] as NextRequest; + await expect(req.json()).resolves.toEqual({ url: 'https://example.com' }); });Also applies to: 80-87, 90-97
🤖 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 69 - 77, Update the tests to assert the dispatched request payloads (not just that handlers were called): for the URL case, grab the first argument passed to the mocked urlPost (e.g., const req = (urlPost as vi.Mock).mock.calls[0][0]), await req.json() and assert it equals { url: 'https://example.com' }; do the same for the DOI and ISBN tests by inspecting doiPost and isbnPost calls respectively and asserting their JSON bodies equal { doi: '...' } and { isbn: '...' } using the existing POST and makeRequest helpers so any contract regressions are caught.
🤖 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.ts`:
- Around line 28-31: The map over items (lookupPromises) calls item.trim()
without guarding non-string values, causing a 500 on bad inputs; update the
items.map callback (the lookupPromises function) to first check typeof item ===
"string" (or otherwise validate the type) and immediately return a per-item
failure object like { input: item, success: false, error: "Invalid input type" }
for non-strings, then continue with const trimmedItem = item.trim() for valid
strings so each bad entry yields a per-item validation error instead of crashing
the whole request.
---
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 69-77: Update the tests to assert the dispatched request payloads
(not just that handlers were called): for the URL case, grab the first argument
passed to the mocked urlPost (e.g., const req = (urlPost as
vi.Mock).mock.calls[0][0]), await req.json() and assert it equals { url:
'https://example.com' }; do the same for the DOI and ISBN tests by inspecting
doiPost and isbnPost calls respectively and asserting their JSON bodies equal {
doi: '...' } and { isbn: '...' } using the existing POST and makeRequest helpers
so any contract regressions are caught.
🪄 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: 92ef7d38-6fe4-4316-b0de-125a5bf687e5
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| const lookupPromises = items.map(async (item) => { | ||
| const trimmedItem = item.trim(); | ||
| if (!trimmedItem) { | ||
| return { input: item, success: false, error: "Empty input" }; |
There was a problem hiding this comment.
Handle non-string items entries before trimming.
Line 29 calls .trim() before any guard; if a client sends a non-string element, the whole request falls into the outer catch and returns 500 instead of a per-item validation failure.
Proposed fix
- const lookupPromises = items.map(async (item) => {
- const trimmedItem = item.trim();
+ 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" };
}📝 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.
| const lookupPromises = items.map(async (item) => { | |
| const trimmedItem = item.trim(); | |
| if (!trimmedItem) { | |
| return { input: item, success: false, error: "Empty input" }; | |
| 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 28 - 31, The map over items
(lookupPromises) calls item.trim() without guarding non-string values, causing a
500 on bad inputs; update the items.map callback (the lookupPromises function)
to first check typeof item === "string" (or otherwise validate the type) and
immediately return a per-item failure object like { input: item, success: false,
error: "Invalid input type" } for non-strings, then continue with const
trimmedItem = item.trim() for valid strings so each bad entry yields a per-item
validation error instead of crashing the whole request.
🚨 Severity: HIGH
💡 Vulnerability: The
/api/lookup/bulk/route.tsendpoint performed internal API loopback HTTPfetchrequests constructed using a dynamically derived hostname (request.nextUrl.origin). This relies on the HTTPHostheader, which is user-controllable. An attacker could manipulate this header to route the internal lookup requests to an arbitrary internal IP or domain (Host-header Server-Side Request Forgery).🎯 Impact: An attacker could potentially bypass SSRF protections, access restricted internal network endpoints, or perform cache poisoning depending on the environment architecture.
🔧 Fix: Refactored the endpoint to avoid loopback
fetchcalls entirely. Internal route handlers (urlPost,doiPost,isbnPost) are now directly imported and invoked using a syntheticNextRequestobject (new NextRequest(new URL('http://localhost'), ...)). This avoids DNS/network boundaries, effectively neutralizing the SSRF vector while simultaneously improving latency.✅ Verification: Unit tests for
/api/lookup/bulk/route.test.tshave been updated to mock the local route files instead ofglobal.fetchand pass successfully (pnpm test).PR created automatically by Jules for task 10779170663556295043 started by @aicoder2009
Summary by CodeRabbit
Bug Fixes
Tests
Documentation