diff --git a/app/controllers/consent.js b/app/controllers/consent.js index 7425977b5..8629337f9 100644 --- a/app/controllers/consent.js +++ b/app/controllers/consent.js @@ -112,7 +112,7 @@ exports.controller = async function (request, getParams, response) { r.html = mainTemplate.renderWhydPage(r); } // call the adequate renderer - if (r.redirect) response.redirect(r.redirect); + if (r.redirect) response.safeRedirect(r.redirect); else if (r.html) response.renderHTML(r.html); else response.renderJSON(r); // and track visit to that page diff --git a/app/models/logging.js b/app/models/logging.js index e366d5aa3..fff3fbeee 100644 --- a/app/models/logging.js +++ b/app/models/logging.js @@ -255,10 +255,17 @@ http.ServerResponse.prototype.renderText = function (json, statusCode) { ); }; +// TODO: this function is overrided by Express => delete it to prevent ambiguity http.ServerResponse.prototype.redirect = function (url) { return this.renderHTML(loggingTemplate.htmlRedirect(url)); }; +http.ServerResponse.prototype.safeRedirect = function (url) { + const fullURL = new URL(url, config.urlPrefix); + if (!fullURL.toString().startsWith(config.urlPrefix)) return this.forbidden(); + this.redirect(url); +}; + http.ServerResponse.prototype.redirectWithTracking = function (url, title) { return this.renderHTML( loggingTemplate.renderRedirectPageWithTracking(url, title) diff --git a/test/api-client.js b/test/api-client.js index 5634a63bf..56b68fc16 100644 --- a/test/api-client.js +++ b/test/api-client.js @@ -87,6 +87,13 @@ exports.get = function (jar, url, callback) { }); }; +exports.postRaw = function (jar, url, body, callback) { + request.post( + { jar, url: `${URL_PREFIX}${url}`, body, json: typeof body === 'object' }, + (error, response, body) => callback(error, { response, body }) + ); +}; + // USER /* diff --git a/test/api/security.api.tests.js b/test/api/security.api.tests.js new file mode 100644 index 000000000..4061fed10 --- /dev/null +++ b/test/api/security.api.tests.js @@ -0,0 +1,47 @@ +/* global describe, it, before */ + +const { promisify } = require('util'); +const assert = require('assert'); + +const { ADMIN_USER, cleanup, URL_PREFIX } = require('../fixtures.js'); +const apiClient = require('../api-client.js'); + +const postRaw = promisify(apiClient.postRaw); +const loginAs = promisify(apiClient.loginAs); + +before(cleanup); + +describe('security', () => { + describe('Open Redirect', () => { + it('should allow redirect to /stream', async () => { + const target = `/stream`; + const { jar } = await loginAs(ADMIN_USER); + const { response } = await postRaw(jar, `/consent`, { + lang: 'en', + redirect: target, + }); + assert.equal(response.statusCode, 302); + assert.equal(response.headers.location, target); + }); + + it(`should allow redirect to ${URL_PREFIX}/stream`, async () => { + const target = `${URL_PREFIX}/stream`; + const { jar } = await loginAs(ADMIN_USER); + const { response } = await postRaw(jar, `/consent`, { + lang: 'en', + redirect: target, + }); + assert.equal(response.statusCode, 302); + assert.equal(response.headers.location, target); + }); + + it('should NOT allow redirect to other domain', async () => { + const { jar } = await loginAs(ADMIN_USER); + const { response } = await postRaw(jar, `/consent`, { + lang: 'en', + redirect: `http://google.com`, + }); + assert.equal(response.statusCode, 403); // forbidden + }); + }); +});