🛡️ Sentinel: [CRITICAL] Fix loopback fetch SSRF in bulk lookup API#148
🛡️ Sentinel: [CRITICAL] Fix loopback fetch SSRF in bulk lookup API#148aicoder2009 wants to merge 1 commit into
Conversation
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.
|
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. 📝 WalkthroughWalkthroughThis PR fixes an SSRF vulnerability in the bulk lookup API by eliminating host-header-based fetch calls. The endpoint now invokes internal route handlers directly with a fixed localhost origin instead of constructing URLs from request-derived headers. Tests are updated to mock the sub-route handlers instead of global fetch calls. ChangesSSRF Vulnerability Fix and Migration
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🤖 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 61-70: Tests currently miss a regression for Host/X-Forwarded-Host
spoofing; add a new test in route.test.ts that uses the existing POST and
makeRequest helpers to send a request with malicious Host and X-Forwarded-Host
headers and then assert urlLookup was called with a Request/synthetic request
whose URL origin is http://localhost (not the spoofed header). Locate the test
helpers used in the file (POST, makeRequest) and the mocked handler urlLookup
and add an assertion that the argument passed to urlLookup contains a URL
beginning with "http://localhost" (use the same pattern of checking the called
argument via vi/expect to verify the synthetic request origin).
In `@src/app/api/lookup/bulk/route.ts`:
- Around line 28-31: The code calls item.trim() inside the items.map callback
(creating lookupPromises) before any per-item validation, so a non-string entry
will throw and cause Promise.all to reject; update the lookupPromises mapping
(the async callback used to build lookupPromises) to validate the item type
(e.g., ensure typeof item === "string") before calling trim or move the trim
inside the existing per-item try/catch, and for non-string values return the
per-item failure object ({ input: item, success: false, error: "Invalid input
type" }) so malformed entries fail individually instead of triggering a
batch-wide 500.
🪄 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: aa4e142e-e53d-47d0-a77b-92af4aa5f258
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| it('routes URLs to /api/lookup/url', async () => { | ||
| (global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({ | ||
| ok: true, | ||
| json: async () => ({ data: { title: 'Example Page' } }), | ||
| }); | ||
| (urlLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce( | ||
| NextResponse.json({ data: { title: 'Example Page' } }) | ||
| ); | ||
| 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'); | ||
| const [url] = (global.fetch as ReturnType<typeof vi.fn>).mock.calls[0] as [string]; | ||
| expect(url).toContain('/api/lookup/url'); | ||
| expect(urlLookup).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a Host-header spoof regression test for this SSRF fix.
Current tests validate routing/mocks, but they don’t assert behavior when inbound Host / X-Forwarded-Host is attacker-controlled. Add one test that sends spoofed headers and verifies the called synthetic request still uses http://localhost.
As per coding guidelines, src/app/api/**/*.ts should include 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 - 70, Tests currently
miss a regression for Host/X-Forwarded-Host spoofing; add a new test in
route.test.ts that uses the existing POST and makeRequest helpers to send a
request with malicious Host and X-Forwarded-Host headers and then assert
urlLookup was called with a Request/synthetic request whose URL origin is
http://localhost (not the spoofed header). Locate the test helpers used in the
file (POST, makeRequest) and the mocked handler urlLookup and add an assertion
that the argument passed to urlLookup contains a URL beginning with
"http://localhost" (use the same pattern of checking the called argument via
vi/expect to verify the synthetic request origin).
Source: Coding guidelines
| 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.
Validate each items entry before calling .trim() to avoid batch-wide 500s.
At Line 29, item.trim() runs before the inner try block. Since items is only compile-time-cast, a non-string value (e.g., null, number) throws and causes Promise.all to reject, returning a top-level 500 instead of a per-item failure.
Suggested 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();📝 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 code calls
item.trim() inside the items.map callback (creating lookupPromises) before any
per-item validation, so a non-string entry will throw and cause Promise.all to
reject; update the lookupPromises mapping (the async callback used to build
lookupPromises) to validate the item type (e.g., ensure typeof item ===
"string") before calling trim or move the trim inside the existing per-item
try/catch, and for non-string values return the per-item failure object ({
input: item, success: false, error: "Invalid input type" }) so malformed entries
fail individually instead of triggering a batch-wide 500.
🛡️ Sentinel Security Fix
Vulnerability:
The bulk lookup API (
src/app/api/lookup/bulk/route.ts) was using a nativefetch()loopback call to reach internal lookup services (url, doi, isbn). It dynamically constructed the base URL for this fetch by readingrequest.nextUrl.origin. Because Next.js derives the origin from user-controllable request headers (such asHostorX-Forwarded-Host), an attacker could supply a spoofed Host header and force the backend server to proxy the request to an attacker-controlled endpoint.Severity: CRITICAL
Fix:
Removed the vulnerable network loopback entirely. The API now safely and idiomatically directly imports the target
POSThandlers (urlLookup,doiLookup,isbnLookup) and invokes them as functions, passing a securely constructed syntheticNextRequestto satisfy their type signatures. The test suite was also fully updated to replaceglobal.fetchmocks withvi.mockfor the imported handlers.Verification:
pnpm test:run- All 652 tests pass successfully.PR created automatically by Jules for task 4228218702171621260 started by @aicoder2009
Summary by CodeRabbit
Bug Fixes