Skip to content

Commit

Permalink
Allow router.redirect() to accept external destinations (#110)
Browse files Browse the repository at this point in the history
* Allow router.redirect() to accept external destinations

* Remove redundant case-insensitive flag

* Added tests for router.redirect() with external destinations

* Simplify external destination detection
  • Loading branch information
apancutt committed Nov 2, 2020
1 parent 56735f0 commit 89b7c02
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 1 deletion.
2 changes: 1 addition & 1 deletion lib/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ Router.prototype.redirect = function (source, destination, code) {
if (source[0] !== '/') source = this.url(source);

// lookup destination route by name
if (destination[0] !== '/') destination = this.url(destination);
if (destination[0] !== '/' && !destination.includes('://')) destination = this.url(destination);

return this.all(source, ctx => {
ctx.redirect(destination);
Expand Down
30 changes: 30 additions & 0 deletions test/lib/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,36 @@ describe('Router', function () {
done();
});
});

it('redirects to external sites', function (done) {
const app = new Koa();
const router = new Router();
app.use(router.routes());
router.redirect('/', 'https://www.example.com');
request(http.createServer(app.callback()))
.post('/')
.expect(301)
.end(function (err, res) {
if (err) return done(err);
res.header.should.have.property('location', 'https://www.example.com');
done();
});
});

it('redirects to any external protocol', function (done) {
const app = new Koa();
const router = new Router();
app.use(router.routes());
router.redirect('/', 'my-custom-app-protocol://www.example.com/foo');
request(http.createServer(app.callback()))
.post('/')
.expect(301)
.end(function (err, res) {
if (err) return done(err);
res.header.should.have.property('location', 'my-custom-app-protocol://www.example.com/foo');
done();
});
});
});

describe('Router#route()', function () {
Expand Down

2 comments on commit 89b7c02

@niftylettuce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better to use url internal Node module to parse the destination (instead of .includes approach you did; which is fine for now). So if you want to improve it you could submit another PR with that (e.g. use the destination value and if it's valid then return the href, see https://nodejs.org/api/url.html#url_url_href).

@niftylettuce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries though, thanks for this, releasing new major semver bump now

Please sign in to comment.