fix: normalize referer before redirect#5640
Conversation
Summary of ChangesHello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a critical security enhancement by normalizing and validating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe response back-referrer logic now accepts only same-origin absolute Referrer URLs; relative referrers are ignored. Tests were updated to assert origin checks, relative-path fallbacks, and handling of protocol-relative or unsafe referrers. One test suite is skipped on Windows due to flakiness. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ServerResponse as Response._getBackReferrer
participant URLParser as URL(parse)
Note over ServerResponse,URLParser #DDEEFF: New flow validates origin & absolute URL
Client->>ServerResponse: requests redirect('back')
ServerResponse->>URLParser: parse ctx.get('Referrer') / ctx.get('Referer')
alt Referrer absent or parse fails
URLParser-->>ServerResponse: error / invalid
ServerResponse-->>Client: fallback redirect (e.g., '/')
else Absolute URL parsed
URLParser-->>ServerResponse: { origin, pathname, ... }
alt same-origin
ServerResponse-->>Client: redirect to parsed full URL
else different-origin
ServerResponse-->>Client: fallback redirect (e.g., '/')
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (6)**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/test/**/*.test.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{packages/!(cookies)/**/test/**/*.test.ts,plugins/**/test/**/*.test.ts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{packages/**/test/**,plugins/**/test/**}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/**/test/**/*.test.ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (3)
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: |
e137037
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://85cbdb3f.egg-cci.pages.dev |
| Branch Preview URL: | https://koa-fix-1908.egg-cci.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a security vulnerability in the redirect functionality by normalizing referer values before processing redirects. The change prevents the "Trailing Double-Slash" security issue where URLs like //evil.com could be exploited for open redirects.
Key Changes:
- Removed special handling for relative path referers (starting with
/) - Added test coverage for same-origin URL redirects
- Added security test case demonstrating the fix for double-slash URLs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/koa/src/response.ts | Removed conditional check that treated paths starting with / as safe, now all referers go through URL parsing and origin validation |
| packages/koa/test/response/redirect.test.ts | Added test cases for same-origin redirects and the double-slash security vulnerability fix |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5640 +/- ##
==========================================
- Coverage 87.52% 87.52% -0.01%
==========================================
Files 565 565
Lines 11007 11006 -1
Branches 1243 1243
==========================================
- Hits 9634 9633 -1
Misses 1291 1291
Partials 82 82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request addresses a potential open redirect vulnerability by normalizing the Referer header before using it for a 'back' redirect. The change in packages/koa/src/response.ts removes special handling for relative paths and now consistently uses the URL constructor to parse and validate the referrer against the request's host. This ensures that only same-origin referrers are used for redirection, preventing redirects to malicious external sites. The accompanying changes in packages/koa/test/response/redirect.test.ts are excellent, adding a specific test case for the vulnerability and improving the robustness of existing tests by providing a proper request context. The changes are correct, well-tested, and improve the security of the application.
Deploying egg-v3 with
|
| Latest commit: |
e137037
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fbdbc4c9.egg-v3.pages.dev |
| Branch Preview URL: | https://koa-fix-1908.egg-v3.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/koa/test/response/redirect.test.ts (1)
77-77: Inconsistent assertion method usage.Lines 77 and 85 use
assert.strictEqual, while other tests in this file useassert.equal. Since the file imports fromnode:assert/strict, both methods behave identically (strict equality). For consistency, consider using onlyassert.equalthroughout the file.Apply this diff to make assertions consistent:
- assert.strictEqual(ctx.response.header.location, 'https://example.com/login'); + assert.equal(ctx.response.header.location, 'https://example.com/login');And:
- assert.strictEqual(ctx.response.header.location, '/'); + assert.equal(ctx.response.header.location, '/');Also applies to: 85-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koa/src/response.ts(0 hunks)packages/koa/test/response/redirect.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/koa/src/response.ts
🧰 Additional context used
📓 Path-based instructions (6)
packages/**/test/**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/**/test/**/*.test.ts: Name test files as test/**/*.test.ts and run them with Vitest
Use import { describe, it } from 'vitest' in tests
Use Node.js built-in assert module for test assertions
Files:
packages/koa/test/response/redirect.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Prefer TypeScript and ESM: write sources and exports in .ts (ESM-first) rather than CommonJS
Use two-space indentation, trailing commas, and semicolons (Prettier/oxlint defaults)
Name files in lowercase with hyphens (e.g., loader-context.ts)
Name classes in PascalCase
Name functions and variables in camelCase
Re-export types thoughtfully to keep the public API stable
Files:
packages/koa/test/response/redirect.test.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/test/**/*.test.ts: Place test suites following Vitest discovery: /test//*.test.ts
Mirror the repository test pattern when adding new suites
Files:
packages/koa/test/response/redirect.test.ts
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts: For isolatedDeclarations support, all exported functions/methods/getters must have explicit return type annotations
Avoid computed property names with symbols in class declarations (no get SYM)
Add explicit type annotations for class properties when needed (no inferred exported property types)
Exported symbols must use unique symbol type (e.g., export const X: unique symbol = Symbol('x'))
Files:
packages/koa/test/response/redirect.test.ts
{packages/!(cookies)/**/test/**/*.test.ts,plugins/**/test/**/*.test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest test files must follow the naming pattern test/**/*.test.ts
Files:
packages/koa/test/response/redirect.test.ts
{packages/**/test/**,plugins/**/test/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Node.js assert for assertions in Vitest tests
Files:
packages/koa/test/response/redirect.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Test (windows-latest, 22, 2/5)
- GitHub Check: Test (macos-latest, 22, 2/5)
- GitHub Check: Test (ubuntu-latest, 24, 5/5)
- GitHub Check: Test (macos-latest, 24, 4/5)
- GitHub Check: Test (macos-latest, 22, 1/5)
- GitHub Check: Test (ubuntu-latest, 24, 3/5)
- GitHub Check: Test (macos-latest, 22, 5/5)
- GitHub Check: Test (macos-latest, 22, 4/5)
- GitHub Check: Test (ubuntu-latest, 24, 1/5)
- GitHub Check: Test (windows-latest, 24, 4/5)
- GitHub Check: Test (ubuntu-latest, 22, 1/5)
- GitHub Check: Test (windows-latest, 24, 5/5)
- GitHub Check: Test (windows-latest, 24, 1/5)
- GitHub Check: Test bin (windows-latest, 22, 0/3)
- GitHub Check: Test bin (ubuntu-latest, 22, 1/3)
- GitHub Check: Test bin (ubuntu-latest, 22, 0/3)
- GitHub Check: Test bin (windows-latest, 22, 2/3)
- GitHub Check: Test bin (windows-latest, 22, 1/3)
- GitHub Check: typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/koa/test/response/redirect.test.ts (2)
40-52: LGTM: Proper context initialization for origin-aware redirects.The explicit context initialization with
urlandhostis necessary for the updated redirect logic to perform origin checks. Testing bothreferrerandrefererheaders is correct since HTTP allows both spellings.
88-96: LGTM: Critical security fix properly tested.This test correctly validates the Trailing Double-Slash security fix, ensuring protocol-relative URLs (e.g.,
//evil.com/login/) are not followed during back redirects. The test verifies:
- Protocol-relative referrer falls back to
/- Explicit fallback path (e.g.,
/home) works as expectedThis prevents potential open redirect vulnerabilities.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
pick from koajs/koa#1908
Summary by CodeRabbit
Bug Fixes
Tests