Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(security): Make login redirects more secure (#519)
Addresses:
- [Improper Authorization vulnerability found in openwhyd](https://huntr.dev/bounties/d66f90d6-1b5f-440d-8be6-cdffc9d4587e/)
- [Open Redirect vulnerability found in openwhyd](https://huntr.dev/bounties/8852bb22-7312-4af5-b9c2-507a68e37a6a/)
- [Cross-site Scripting (XSS) - Reflected vulnerability found in openwhyd](https://huntr.dev/bounties/84a14deb-5dd8-4cc0-810f-d0987566f845/)

Tests:
* test: /login should allow redirect to /stream
* test: /login should NOT allow javascript in redirect URL
* test: /login should should NOT allow redirect to other domain
* test: /login should NOT allow script element in redirect URL
  • Loading branch information
adrienjoly committed Dec 26, 2021
1 parent b97eb82 commit 102a97b
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 5 deletions.
9 changes: 8 additions & 1 deletion app/controllers/api/login.js
@@ -1,6 +1,8 @@
/**
* login controller, to authenticate users
*/
const snip = require('../../snip.js');
const config = require('../../models/config.js');
var emailModel = require('../../models/email.js');
var userModel = require('../../models/user.js');
var notifEmails = require('../../models/notifEmails.js');
Expand Down Expand Up @@ -60,7 +62,12 @@ exports.handleRequest = function (request, form, response, ignorePassword) {
userModel[form.email.indexOf('@') > -1 ? 'fetchByEmail' : 'fetchByHandle'](
form.email,
function (dbUser) {
if (!dbUser) {
if (
form.redirect &&
snip.getSafeOpenwhydURL(form.redirect, config.urlPrefix) === false
) {
form.error = 'Unsafe redirect target';
} else if (!dbUser) {
form.error = "Are you sure? We don't recognize your email address!";
} else if (form.action == 'forgot') {
notifEmails.sendPasswordReset(dbUser._id, dbUser.pwd, form.redirect);
Expand Down
5 changes: 2 additions & 3 deletions app/models/logging.js
Expand Up @@ -261,9 +261,8 @@ http.ServerResponse.prototype.redirect = function (url) {
};

http.ServerResponse.prototype.safeRedirect = function (url) {
const fullURL = new URL(url, config.urlPrefix);
if (`${fullURL.protocol}//${fullURL.host}` !== config.urlPrefix)
return this.forbidden();
const safeURL = snip.getSafeOpenwhydURL(url, config.urlPrefix);
if (safeURL === false) return this.forbidden();
this.redirect(url);
};

Expand Down
7 changes: 7 additions & 0 deletions app/snip.js
Expand Up @@ -109,6 +109,13 @@ exports.sanitizeJsStringInHtml = function (str) {
return exports.htmlEntities(exports.addSlashes(str || ''));
};

exports.getSafeOpenwhydURL = function (url, safeUrlPrefix) {
if (typeof url !== 'string' || url.includes('<script')) return false;
const fullURL = new URL(url, safeUrlPrefix);
if (`${fullURL.protocol}//${fullURL.host}` !== safeUrlPrefix) return false;
else return fullURL;
};

var timeScales = [
{ 'minute(s)': 60 },
{ 'hour(s)': 60 },
Expand Down
60 changes: 59 additions & 1 deletion test/api/security.api.tests.js
Expand Up @@ -12,7 +12,65 @@ const loginAs = promisify(apiClient.loginAs);
before(cleanup);

describe('security', () => {
describe('Open Redirect', () => {
describe('Open Redirect from /login', () => {
it('should allow redirect to /stream', async () => {
const target = `/stream`;
const { response } = await postRaw(null, `/login`, {
action: 'login',
email: ADMIN_USER.email,
md5: ADMIN_USER.md5,
redirect: target,
});
assert(
response.body.includes(`window.location.href="${target}"`) === true,
`page body should include redirect to ${target}`
);
});

it('should NOT allow redirect to other domain', async () => {
const target = `https://google.com`;
const { response } = await postRaw(null, `/login`, {
action: 'login',
email: ADMIN_USER.email,
md5: ADMIN_USER.md5,
redirect: target,
});
assert(
response.body.includes(`window.location.href="${target}"`) === false,
`page body should NOT include redirect to ${target}`
);
});

it('should NOT allow javascript in redirect URL', async () => {
const target = `javascript:alert()`;
const { response } = await postRaw(null, `/login`, {
action: 'login',
email: ADMIN_USER.email,
md5: ADMIN_USER.md5,
redirect: target,
});
assert(
response.body.includes(`window.location.href="${target}"`) === false,
`page body should NOT include redirect to ${target}`
);
});

it('should NOT allow script element in redirect URL', async () => {
const target = `<script>alert(document.cookie)</script>`;
const { response } = await postRaw(null, `/login`, {
action: 'login',
email: ADMIN_USER.email,
md5: ADMIN_USER.md5,
redirect: target,
});
assert(
response.body.includes(`window.location.href="${target}"`) === false,
`page body should NOT include redirect to ${target}`
);
});
});

describe('Open Redirect from /consent', () => {
it('should allow redirect to /stream', async () => {
const target = `/stream`;
const { jar } = await loginAs(ADMIN_USER);
Expand Down

0 comments on commit 102a97b

Please sign in to comment.