Skip to content

Commit

Permalink
fix: formatting redirect url on http(s) protocol url (#1803)
Browse files Browse the repository at this point in the history
avoid security escapes
  • Loading branch information
fengmk2 committed Mar 15, 2024
1 parent a08b386 commit 2510b0b
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 10 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/node.js.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:

strategy:
matrix:
node-version: [12.x, 14.x, 16.x, 18.x]
node-version: [12.x, 14.x, 16.x, 18.x, 20.x, 21.x]

steps:
- uses: actions/checkout@v2
Expand Down
25 changes: 16 additions & 9 deletions __tests__/response/redirect.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ describe('ctx.redirect(url)', () => {
it('should redirect to the given url', () => {
const ctx = context();
ctx.redirect('http://google.com');
assert.strictEqual(ctx.response.header.location, 'http://google.com');
assert.strictEqual(ctx.response.header.location, 'http://google.com/');
assert.strictEqual(ctx.status, 302);
});

it('should formatting url before redirect', () => {
const ctx = context();
ctx.redirect('http://google.com\\@apple.com');
assert.strictEqual(ctx.response.header.location, 'http://google.com/@apple.com');
assert.strictEqual(ctx.status, 302);
});

Expand Down Expand Up @@ -66,7 +73,7 @@ describe('ctx.redirect(url)', () => {
ctx.header.accept = 'text/html';
ctx.redirect(url);
assert.strictEqual(ctx.response.header['content-type'], 'text/html; charset=utf-8');
assert.strictEqual(ctx.body, `Redirecting to <a href="${url}">${url}</a>.`);
assert.strictEqual(ctx.body, `Redirecting to <a href="${url}/">${url}/</a>.`);
});

it('should escape the url', () => {
Expand All @@ -86,17 +93,17 @@ describe('ctx.redirect(url)', () => {
const url = 'http://google.com';
ctx.header.accept = 'text/plain';
ctx.redirect(url);
assert.strictEqual(ctx.body, `Redirecting to ${url}.`);
assert.strictEqual(ctx.body, `Redirecting to ${url}/.`);
});
});

describe('when status is 301', () => {
it('should not change the status code', () => {
const ctx = context();
const url = 'http://google.com';
const url = 'http://google.com/';
ctx.status = 301;
ctx.header.accept = 'text/plain';
ctx.redirect('http://google.com');
ctx.redirect(url);
assert.strictEqual(ctx.status, 301);
assert.strictEqual(ctx.body, `Redirecting to ${url}.`);
});
Expand All @@ -108,19 +115,19 @@ describe('ctx.redirect(url)', () => {
const url = 'http://google.com';
ctx.status = 304;
ctx.header.accept = 'text/plain';
ctx.redirect('http://google.com');
ctx.redirect(url);
assert.strictEqual(ctx.status, 302);
assert.strictEqual(ctx.body, `Redirecting to ${url}.`);
assert.strictEqual(ctx.body, `Redirecting to ${url}/.`);
});
});

describe('when content-type was present', () => {
it('should overwrite content-type', () => {
const ctx = context();
ctx.body = {};
const url = 'http://google.com';
const url = 'http://google.com/foo/bar';
ctx.header.accept = 'text/plain';
ctx.redirect('http://google.com');
ctx.redirect(url);
assert.strictEqual(ctx.status, 302);
assert.strictEqual(ctx.body, `Redirecting to ${url}.`);
assert.strictEqual(ctx.type, 'text/plain');
Expand Down
4 changes: 4 additions & 0 deletions lib/response.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,10 @@ module.exports = {
redirect(url, alt) {
// location
if ('back' === url) url = this.ctx.get('Referrer') || alt || '/';
if (url.startsWith('https://') || url.startsWith('http://')) {
// formatting url again avoid security escapes
url = new URL(url).toString();
}
this.set('Location', encodeUrl(url));

// status
Expand Down

0 comments on commit 2510b0b

Please sign in to comment.