🛡️ Sentinel: Harden image processing routes against SSRF and injection#35
🛡️ Sentinel: Harden image processing routes against SSRF and injection#35Sumit-5002 wants to merge 2 commits into
Conversation
This change improves the security of the image processing API by: - Mitigating SSRF in `/html-to-image` by disabling automatic redirects and manually rejecting 3xx responses. - Ensuring IPv6 addresses are correctly formatted in target URLs for SSRF protection. - Implementing a strict whitelist for allowed image formats (`ALLOWED_IMAGE_FORMATS`) in resize, compress, convert, and crop operations. - Sanitizing error responses to prevent internal library details from leaking to clients.
|
👋 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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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 |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/image.js`:
- Around line 64-67: Validation paths currently await fs.unlink(req.file.path)
which can throw and prevent sending the 400 response; change each occurrence
(the checks using ALLOWED_IMAGE_FORMATS and the other validation blocks that
call fs.unlink before res.status(400).json) to perform best-effort cleanup: call
fs.unlink(req.file.path) inside a try/catch (or attach .catch(()=>{}) if using
promises) and ignore any unlink errors so the code always proceeds to return
res.status(400).json({ error: ... }); ensure you update all places that
reference fs.unlink(req.file.path) in the file so cleanup failures never block
the response.
- Around line 308-311: The rebuilt fetch target currently drops parsed.port;
update construction of targetUrl (where ipPart and targetUrl are created) to
include the port when present (e.g., append `:${parsed.port}` after ipPart)
while preserving IPv6 brackets logic in ipPart, and ensure the Host header sent
uses parsed.host (which includes hostname:port) instead of hostname only so
requests to hosts like https://example.com:8443/... use the correct port and
Host header.
- Line 8: Normalize any 'jpg' occurrences to 'jpeg' before using the format
value in response headers and filenames: update the logic that uses
ALLOWED_IMAGE_FORMATS and the format variable (the place that builds
Content-Type as `image/${format}` and the Content-Disposition filename) to map
'jpg' -> 'jpeg' (e.g., if (format === 'jpg') format = 'jpeg') so headers use the
standards-compliant MIME type and filenames use 'jpeg' while keeping
ALLOWED_IMAGE_FORMATS unchanged for input validation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 329b3e81-e4c2-4856-ad6b-2d14cf800708
📒 Files selected for processing (1)
server/routes/image.js
@jules check the comments of coderabbit |
I have implemented all the requested changes:
|
|
Request timed out after 900000ms (requestId=7bae6a79-a35b-412d-9a09-b5e3759f4790) |
This commit addresses the following:
- Updated `fs.unlink` cleanup in validation paths to be best-effort (`.catch(() => {})`) to avoid blocking error responses.
- Fixed SSRF mitigation in `html-to-image` to correctly handle custom ports and use the full `Host` header (hostname + port).
- Normalized 'jpg' format to 'jpeg' for response headers and filenames for better standard compliance.
- Removed sensitive error messages from 500 responses across all image processing endpoints.
1 participant
I have hardened the
server/routes/image.jsfile to address several security risks.Key improvements:
html-to-imageendpoint now explicitly blocks redirects (3xx status codes) when fetching target URLs. This prevents attackers from bypassing the initial internal IP check by redirecting the server to a local resource. I also ensured that IPv6 addresses are correctly wrapped in square brackets during URL construction to prevent parsing errors.Verification:
pnpm lint.PR created automatically by Jules for task 3256619706500965542 started by @Sumit-5002
Summary by CodeRabbit
Release Notes
Bug Fixes
Security