🛡️ Sentinel: [CRITICAL] Fix SSRF in bulk lookup API#210
Conversation
Removed `fetch` calls dynamically constructed with `request.nextUrl.origin` inside `src/app/api/lookup/bulk/route.ts` which relied on the client-controlled `Host` header. It now securely directly invokes the local API handlers via synthetic NextRequest objects. 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 fixes an SSRF vulnerability in the bulk lookup API by replacing outbound ChangesFix SSRF in Bulk Lookup API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BulkRoute
participant UrlPOST
participant DoiPOST
participant IsbnPOST
Client->>BulkRoute: POST /api/lookup/bulk (items)
loop each item
BulkRoute->>BulkRoute: detect type, build reqBody, select handler
alt url
BulkRoute->>UrlPOST: handler(syntheticReq)
UrlPOST-->>BulkRoute: NextResponse
else doi
BulkRoute->>DoiPOST: handler(syntheticReq)
DoiPOST-->>BulkRoute: NextResponse
else isbn
BulkRoute->>IsbnPOST: handler(syntheticReq)
IsbnPOST-->>BulkRoute: NextResponse
end
end
BulkRoute-->>Client: aggregated results and summary
Related Issues: None referenced. Related PRs: None referenced. Suggested labels: security, bug Suggested reviewers: None specified. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (1)
src/app/api/lookup/bulk/route.test.ts (1)
65-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the request body forwarded to each handler.
Tests only check
toHaveBeenCalled(); they don't verify what was passed to the handler (e.g., thaturlPOSTactually received{ url: ... }rather than some other field). Since the mocked functions ignore their arguments, a bug that sends the wrong JSON field to the correct handler wouldn't be caught here.As per path instructions,
src/app/api/**/*.test.tsshould "Write tests for citation formatting and API route logic."✅ Example strengthened assertion
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 [syntheticReq] = (urlPOST as ReturnType<typeof vi.fn>).mock.calls[0]; + expect(await syntheticReq.json()).toEqual({ url: 'https://example.com' });🤖 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 65 - 96, The bulk lookup route tests only verify that the correct handler functions are called, but not that they receive the right request payload. In `POST` coverage for `urlPOST`, `doiPOST`, and `isbnPOST`, strengthen the assertions to check the forwarded body/arguments so a wrong field mapping (for example, sending the wrong key to the right handler) is caught. Use the existing handler mocks in `src/app/api/lookup/bulk/route.test.ts` to assert the exact payload passed into each route-specific POST handler.Source: Path instructions
🤖 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 11: The markdown in this sentinel note has a malformed inline code span
because backslashes do not escape backticks inside single-backtick code spans,
which breaks rendering. Update the affected sentence in .jules/sentinel.md to
use a valid inline code span format (for example, a double-backtick fence or
reworded code formatting) so the literal `fetch(...)`/`request.nextUrl.origin`
snippet renders correctly and complies with markdownlint-cli2 MD038.
---
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 65-96: The bulk lookup route tests only verify that the correct
handler functions are called, but not that they receive the right request
payload. In `POST` coverage for `urlPOST`, `doiPOST`, and `isbnPOST`, strengthen
the assertions to check the forwarded body/arguments so a wrong field mapping
(for example, sending the wrong key to the right handler) is caught. Use the
existing handler mocks in `src/app/api/lookup/bulk/route.test.ts` to assert the
exact payload passed into each route-specific POST handler.
🪄 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: 8d986047-191f-4b57-b016-d3fb1fdfc7d4
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| **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`. | ||
| ## 2025-03-08 - Fixed SSRF in bulk lookup API | ||
|
|
||
| **Vulnerability:** Server-Side Request Forgery (SSRF) risk in `src/app/api/lookup/bulk/route.ts` where it was using `fetch(\`\${request.nextUrl.origin}/api/lookup/...\`)`. `request.nextUrl.origin` is dynamically derived from the potentially attacker-controllable `Host` header. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix malformed code span (breaks rendering).
Backslash doesn't escape backticks inside a single-backtick code span per CommonMark, so this span likely terminates early and renders with stray backticks/backslashes.
📝 Proposed fix using a double-backtick fence
-**Vulnerability:** Server-Side Request Forgery (SSRF) risk in `src/app/api/lookup/bulk/route.ts` where it was using `fetch(\`\${request.nextUrl.origin}/api/lookup/...\`)`. `request.nextUrl.origin` is dynamically derived from the potentially attacker-controllable `Host` header.
+**Vulnerability:** Server-Side Request Forgery (SSRF) risk in `src/app/api/lookup/bulk/route.ts` where it was using ``fetch(`${request.nextUrl.origin}/api/lookup/...`)``. `request.nextUrl.origin` is dynamically derived from the potentially attacker-controllable `Host` header.Flagged by markdownlint-cli2: "Spaces inside code span elements (MD038, no-space-in-code)".
📝 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.
| **Vulnerability:** Server-Side Request Forgery (SSRF) risk in `src/app/api/lookup/bulk/route.ts` where it was using `fetch(\`\${request.nextUrl.origin}/api/lookup/...\`)`. `request.nextUrl.origin` is dynamically derived from the potentially attacker-controllable `Host` header. | |
| **Vulnerability:** Server-Side Request Forgery (SSRF) risk in `src/app/api/lookup/bulk/route.ts` where it was using ``fetch(`${request.nextUrl.origin}/api/lookup/...`)``. `request.nextUrl.origin` is dynamically derived from the potentially attacker-controllable `Host` header. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 11-11: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 11, The markdown in this sentinel note has a
malformed inline code span because backslashes do not escape backticks inside
single-backtick code spans, which breaks rendering. Update the affected sentence
in .jules/sentinel.md to use a valid inline code span format (for example, a
double-backtick fence or reworded code formatting) so the literal
`fetch(...)`/`request.nextUrl.origin` snippet renders correctly and complies
with markdownlint-cli2 MD038.
Source: Linters/SAST tools
🚨 Severity: CRITICAL
💡 Vulnerability: Server-Side Request Forgery (SSRF) risk in
src/app/api/lookup/bulk/route.ts. The code was using loopbackfetch()requests built withrequest.nextUrl.origin, which depends on the user-controlledHostheader.🎯 Impact: An attacker could manipulate the
Hostheader to route backend internal lookups to an external attacker-controlled domain or arbitrary internal services, potentially leading to unauthorized data exposure, internal service mapping, or other SSRF exploitation.🔧 Fix: Refactored
src/app/api/lookup/bulk/route.tsto directly import the internal Next.js App Router POST handlers (urlPOST,doiPOST,isbnPOST). Construct a minimal syntheticNextRequestwith only safe required headers, rather than forwarding or performing network lookups. Test assertions updated to mock the new handler architecture appropriately.✅ Verification: Verified by passing all unit tests, confirming
pnpm test:runsucceeds without breaking. Confirmednpx eslintpasses on the changed files. Added findings cleanly into.jules/sentinel.md.PR created automatically by Jules for task 8878284910696671916 started by @aicoder2009
Summary by CodeRabbit