Fix: Secure file parsing – prevent SSRF by validating external URLs - issue #961 #1129
Conversation
Added SSRF protection tests for file parsing API.
Added functions to validate external URLs and handle secure fetching with DNS resolution and IP address checks. Removed the old handleExternalUrl function for improved security and error handling.
|
@Hemanth-Kumar-04 is attempting to deploy a commit to the Sim Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR addresses a critical SSRF (Server-Side Request Forgery) vulnerability in the file parsing API by implementing comprehensive URL validation and secure HTTP fetching mechanisms. The changes center around two main files:
Security Implementation (route.ts):
The PR introduces a multi-layered security approach by adding three key functions: isPrivateOrReservedAddress() checks if IP addresses fall into private/reserved ranges (192.168.x.x, 10.x.x.x, 127.x.x.x, etc.), validateExternalUrl() performs DNS resolution and validates all resolved IP addresses against these ranges, and secureFetch() replaces direct fetch calls with manual redirect handling that validates each redirect URL. The original handleExternalUrl() function has been updated to use these secure mechanisms instead of making unvalidated HTTP requests.
Test Suite Updates (route.test.ts):
The testing approach has been restructured to support the new security features. All tests now use dynamic imports (await import('@/app/api/files/parse/route')) instead of static imports to ensure proper module isolation and mocking. Two new comprehensive SSRF protection tests have been added: one verifying that URLs resolving to private IP addresses (192.168.1.50) are properly rejected, and another confirming that legitimate public IPs (93.184.216.34) are allowed. The test suite includes DNS lookup mocking to simulate hostname resolution scenarios and implements proper cleanup of global fetch mocks in afterEach hooks.
This security fix integrates seamlessly with the existing file parsing infrastructure, maintaining all current functionality while preventing malicious actors from using the API to make requests to internal network resources, cloud metadata endpoints, or other sensitive services that should not be accessible from the application.
Confidence score: 4/5
- This PR addresses a critical security vulnerability with thorough implementation and comprehensive test coverage
- Score reflects robust security controls and proper testing, though complex security logic always requires careful review
- Pay close attention to the SSRF validation logic in
route.tsand ensure all private IP ranges are properly covered
2 files reviewed, 2 comments
| expect(response.status).toBe(200) | ||
| expect(data).toHaveProperty('success', false) | ||
| expect(data.error).toMatch(/private|reserved|Localhost/i) | ||
| expect(fetchMock.mock.calls.length === 0 || fetchMock.mock.calls.length >= 0).toBe(true) |
There was a problem hiding this comment.
logic: This assertion is tautological - length === 0 || length >= 0 is always true
| expect(fetchMock.mock.calls.length === 0 || fetchMock.mock.calls.length >= 0).toBe(true) | |
| expect(fetchMock).not.toHaveBeenCalled() |
| // lookup with all: true returns an array of { address, family } | ||
| const results = await lookup(hostname, { all: true }) | ||
| // results may be objects or strings depending on environment - normalize | ||
| return results.map((r: any) => (typeof r === 'string' ? r : r.address)) |
There was a problem hiding this comment.
style: Consider adding type annotation for the results parameter to avoid using any
| return results.map((r: any) => (typeof r === 'string' ? r : r.address)) | |
| return results.map((r: { address: string; family: number } | string) => (typeof r === 'string' ? r : r.address)) |
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
|
resolved in #1149 |
2 participants
Summary
This PR fixes an issue with file parsing that could allow SSRF (Server-Side Request Forgery).
The fix ensures external URLs are properly validated, and replaces unsafe
delete (globalThis as any).fetchusage with a safer override.Fixes #961
Type of Change
Testing
route.test.tsto reflect the new safer behavior.Checklist
Screenshots/Videos
N/A – backend fix (no UI impact).