Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a security issue by normalizing and validating the referrer before performing a "back" redirect to prevent scheme-relative URLs (e.g., //evil.com) from causing open redirects.
- Removes the naive early-return for referrers starting with "/" so all referrer values are resolved against the request origin and validated as same-origin.
- Updates tests to set the host explicitly, adds a test for the double-slash scheme-relative URL case, and removes a test for the "Referer" header variant.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/response.js | Removes early return for relative referrers to ensure same-origin validation via URL parsing. |
| tests/response/redirect.js | Updates test setup to include host, adds a test for scheme-relative referrer, and removes test for the "Referer" header variant. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -243,11 +243,6 @@ module.exports = { | |||
| _getBackReferrer() { | |||
| const referrer = this.ctx.get('Referrer'); | |||
There was a problem hiding this comment.
Only reading the 'Referrer' header misses the de facto standard HTTP header 'Referer'. To maintain compatibility with real-world clients and browsers, read both headers, for example: const referrer = this.ctx.get('Referrer') || this.ctx.get('Referer');
| const referrer = this.ctx.get('Referrer'); | |
| const referrer = this.ctx.get('Referrer') || this.ctx.get('Referer'); |
| assert.equal(ctx.response.header.location, '/'); | ||
|
|
||
| ctx.redirect('back', '/home'); | ||
| assert.equal(ctx.response.header.location, '/home'); |
There was a problem hiding this comment.
Use assert.strictEqual for consistency with the rest of the file and to avoid loose equality: replace assert.equal(...) with assert.strictEqual(...).
| assert.equal(ctx.response.header.location, '/'); | |
| ctx.redirect('back', '/home'); | |
| assert.equal(ctx.response.header.location, '/home'); | |
| assert.strictEqual(ctx.response.header.location, '/'); | |
| ctx.redirect('back', '/home'); | |
| assert.strictEqual(ctx.response.header.location, '/home'); |
pick from #1908