-
Notifications
You must be signed in to change notification settings - Fork 0
valdiate urls #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
valdiate urls #25
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
WalkthroughAdds URL validation to proxy and redirect middlewares using a new validateUrl utility function. Both middlewares now validate URLs before processing, returning 400 errors for invalid inputs. Early exit paths replace unconditional processing. Comprehensive test coverage validates the utility and middleware behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/middleware/proxyMiddleware.js (1)
10-14: LGTM! Validation properly gates proxy forwarding.The validation correctly prevents proxying to invalid or non-HTTP(S) URLs, mitigating SSRF risks.
Optional: Avoid double URL parsing
The URL is parsed twice: once in
validateUrl(line 10) and again on line 16. You could optimize by havingvalidateUrlreturn the parsed URL object:- const validation = validateUrl(proxyUrl); - if (!validation.valid) { - return res.status(400).json({ error: validation.error }); - } - - req.url = proxyUrl; - const host = new URL(proxyUrl).host; + const validation = validateUrl(proxyUrl); + if (!validation.valid) { + return res.status(400).json({ error: validation.error }); + } + + req.url = proxyUrl; + const host = validation.parsed.host; // Reuse parsed URLHowever, this would require changing the
validateUrlAPI to return the parsed URL. The current approach is simpler and the performance impact is negligible.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/middleware/proxyMiddleware.js(1 hunks)src/middleware/redirectMiddleware.js(2 hunks)src/tests/proxy.test.js(1 hunks)src/tests/redirect.test.js(1 hunks)src/tests/validateUrl.test.js(1 hunks)src/utils/validateUrl.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/tests/redirect.test.js (2)
src/middleware/redirectMiddleware.js (2)
redirectUrl(5-5)redirectUrl(21-21)src/middleware/proxyMiddleware.js (1)
response(21-21)
src/middleware/proxyMiddleware.js (2)
src/middleware/redirectMiddleware.js (2)
req(20-34)validation(28-28)src/utils/validateUrl.js (1)
validateUrl(6-23)
src/middleware/redirectMiddleware.js (2)
src/middleware/proxyMiddleware.js (2)
req(3-32)validation(10-10)src/utils/validateUrl.js (1)
validateUrl(6-23)
src/tests/proxy.test.js (1)
src/middleware/proxyMiddleware.js (2)
proxyUrl(4-4)response(21-21)
src/tests/validateUrl.test.js (1)
src/utils/validateUrl.js (1)
validateUrl(6-23)
🔇 Additional comments (7)
src/utils/validateUrl.js (1)
6-23: LGTM! Solid validation logic with good security posture.The validation correctly:
- Rejects empty/null/undefined inputs
- Validates URL format via the URL constructor
- Restricts protocols to HTTP/HTTPS, preventing file://, javascript:, data:, and other potentially dangerous protocols
This effectively mitigates SSRF and open redirect risks.
src/middleware/redirectMiddleware.js (2)
1-1: LGTM! Proper import of validation utility.
27-33: LGTM! Validation properly integrated into redirect flow.The validation is correctly positioned after URL resolution (getRedirectUrl), ensuring:
- Relative URLs are resolved against referer first
- Only the final absolute URL is validated
- Invalid URLs return 400 before any redirect occurs
This prevents open redirects to non-HTTP(S) protocols.
src/tests/proxy.test.js (1)
22-28: LGTM! Good test coverage for invalid URL handling.The test properly verifies that invalid proxy URLs return a 400 error response.
src/middleware/proxyMiddleware.js (1)
1-1: LGTM! Proper import of validation utility.src/tests/redirect.test.js (1)
55-62: LGTM! Good test coverage for invalid redirect URL handling.The test properly verifies that invalid redirect URLs return a 400 error response, consistent with the pattern used in other redirect tests.
src/tests/validateUrl.test.js (1)
4-52: LGTM! Comprehensive test coverage for URL validation.The test suite thoroughly validates:
- Valid HTTP/HTTPS URLs ✓
- Edge cases (empty, null, undefined) ✓
- Invalid URL formats ✓
- Security: Dangerous protocols (file://, javascript:) ✓
This provides strong confidence in the validation utility's behavior.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.