fix(koa): sanitize protocol-relative redirects#5951
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds formatRedirectUrl() to normalize/sanitize redirect targets (protocol-relative, leading backslashes, double slashes) and calls it from Response.redirect(); expands tests to cover these unsafe patterns and a malformed absolute URL, ensuring redirects set Location and status 302 without throwing. ChangesRedirect URL sanitization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Deploying egg with
|
| Latest commit: |
f5c17f1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f14f31a4.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-262092a7.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5951 +/- ##
=======================================
Coverage 85.21% 85.21%
=======================================
Files 669 669
Lines 19304 19317 +13
Branches 3787 3790 +3
=======================================
+ Hits 16449 16461 +12
- Misses 2463 2464 +1
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
f5c17f1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11ba90f5.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-262092a7.egg-v3.pages.dev |
![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)
packages/koa/test/response/redirect.test.ts (1)
38-43: 💤 Low valueConsider adding test coverage for double backslash and malformed URL edge cases.
The current tests cover the primary security vectors. For completeness, consider adding:
\\evil.com(UNC-style double backslash) → should become/%2Fevil.com- Malformed URL like
http://[invalid→ should not throw (once the fix is applied)🤖 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 `@packages/koa/test/response/redirect.test.ts` around lines 38 - 43, Add two tests to redirect.test.ts: one that calls ctx.redirect(String.raw`\\evil.com`) and asserts ctx.response.header.location === '/%2Fevil.com' and ctx.status === 302, and another that calls ctx.redirect('http://[invalid') and asserts it does not throw (wrap call in a try/catch or let the test runner surface exceptions) and that ctx.response.header.location and ctx.status are set (e.g., header.location equals the provided input and status === 302) to ensure malformed URLs don't crash the redirect logic; locate and modify the existing test block that uses context() and ctx.redirect to add these cases.
🤖 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 `@packages/koa/src/response.ts`:
- Around line 22-25: The URL normalization branch that currently does "new
URL(url).toString()" must be made robust: wrap the new URL(url) call in a
try/catch inside the same conditional that checks url.startsWith('https://') ||
url.startsWith('http://') (the block in response.ts handling URL formatting)
and, on catch, return the original input "url" (or a safe fallback) instead of
letting the TypeError bubble; keep the existing comment and only change the
block that constructs the URL so malformed inputs no longer throw.
---
Nitpick comments:
In `@packages/koa/test/response/redirect.test.ts`:
- Around line 38-43: Add two tests to redirect.test.ts: one that calls
ctx.redirect(String.raw`\\evil.com`) and asserts ctx.response.header.location
=== '/%2Fevil.com' and ctx.status === 302, and another that calls
ctx.redirect('http://[invalid') and asserts it does not throw (wrap call in a
try/catch or let the test runner surface exceptions) and that
ctx.response.header.location and ctx.status are set (e.g., header.location
equals the provided input and status === 302) to ensure malformed URLs don't
crash the redirect logic; locate and modify the existing test block that uses
context() and ctx.redirect to add these cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cced03f-1100-4a0e-9abc-2e1dbedff97d
📒 Files selected for processing (2)
packages/koa/src/response.tspackages/koa/test/response/redirect.test.ts
There was a problem hiding this comment.
Pull request overview
This PR hardens ctx.redirect() in @eggjs/koa by sanitizing redirect targets that could be interpreted by user agents as protocol-relative (cross-origin) URLs, while preserving the existing behavior for absolute http(s):// URLs.
Changes:
- Introduce a
formatRedirectUrl()helper to normalize/sanitize redirect targets before setting theLocationheader. - Block protocol-relative and backslash-based redirect variants by rewriting them into same-origin absolute-path redirects with percent-encoded leading characters.
- Add regression tests covering
//host,/\host, and mixed slash/backslash prefixes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/koa/src/response.ts | Adds redirect URL normalization/sanitization and applies it in Response.redirect() before Location is encoded. |
| packages/koa/test/response/redirect.test.ts | Adds regression tests ensuring protocol-relative and backslash-prefixed redirect inputs are rewritten to safe, same-origin paths. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a formatRedirectUrl utility to sanitize redirect URLs, preventing potential open redirect vulnerabilities by handling protocol-relative and backslash-prefixed paths. New tests were added to verify these security fixes. A review comment suggests refactoring the URL prefix validation logic to use explicit startsWith checks instead of a regular expression to improve clarity and maintainability.
Summary
Test
Summary by CodeRabbit
Bug Fixes
Tests