security: Don't require express_sid if authn not required

This should make it possible to embed a pad in an iframe from another
site as long as `settings.requireAuthentication` is false.
This commit is contained in:
Richard Hansen 2020-09-23 17:13:07 -04:00 committed by John McLear
parent 53fd0b4f98
commit 94f944160d
2 changed files with 31 additions and 22 deletions

View file

@ -8,6 +8,7 @@ var padMessageHandler = require("../../handler/PadMessageHandler");
var cookieParser = require('cookie-parser');
var sessionModule = require('express-session');
const util = require('util');
exports.expressCreateServer = function (hook_name, args, cb) {
//init socket.io and redirect all requests to the MessageHandler
@ -48,32 +49,34 @@ exports.expressCreateServer = function (hook_name, args, cb) {
// check whether the user has authenticated, then any random person on the Internet can read,
// modify, or create any pad (unless the pad is password protected or an HTTP API session is
// required).
var cookieParserFn = cookieParser(webaccess.secret, {});
io.use((socket, next) => {
var data = socket.request;
if (!data.headers.cookie) {
const cookieParserFn = util.promisify(cookieParser(webaccess.secret, {}));
const getSession = util.promisify(args.app.sessionStore.get).bind(args.app.sessionStore);
io.use(async (socket, next) => {
const req = socket.request;
if (!req.headers.cookie) {
// socketio.js-client on node.js doesn't support cookies (see https://git.io/JU8u9), so the
// token and express_sid cookies have to be passed via a query parameter for unit tests.
data.headers.cookie = socket.handshake.query.cookie;
req.headers.cookie = socket.handshake.query.cookie;
}
if (!data.headers.cookie && settings.loadTest) {
if (!req.headers.cookie && settings.loadTest) {
console.warn('bypassing socket.io authentication check due to settings.loadTest');
return next(null, true);
}
const fail = (msg) => { return next(new Error(msg), false); };
cookieParserFn(data, {}, function(err) {
if (err) return fail('access denied: unable to parse express_sid cookie');
const expressSid = data.signedCookies.express_sid;
if (!expressSid) return fail ('access denied: signed express_sid cookie is required');
args.app.sessionStore.get(expressSid, (err, session) => {
if (err || !session) return fail('access denied: bad session or session has expired');
data.session = new sessionModule.Session(data, session);
if (settings.requireAuthentication && data.session.user == null) {
return fail('access denied: authentication required');
}
next(null, true);
});
});
try {
await cookieParserFn(req, {});
const expressSid = req.signedCookies.express_sid;
const needAuthn = settings.requireAuthentication;
if (needAuthn && !expressSid) throw new Error('signed express_sid cookie is required');
if (expressSid) {
const session = await getSession(expressSid);
if (!session) throw new Error('bad session or session has expired');
req.session = new sessionModule.Session(req, session);
if (needAuthn && req.session.user == null) throw new Error('authentication required');
}
} catch (err) {
return next(new Error(`access denied: ${err}`), false);
}
return next(null, true);
});
// var socketIOLogger = log4js.getLogger("socket.io");

View file

@ -130,13 +130,19 @@ describe('socket.io access checks', () => {
});
// Normal accesses.
it('!authn anonymous /p/pad -> 200, ok', async () => {
it('!authn anonymous cookie /p/pad -> 200, ok', async () => {
const res = await client.get('/p/pad').expect(200);
// Should not throw.
socket = await connect(res);
const clientVars = await handshake(socket, 'pad');
assert.equal(clientVars.type, 'CLIENT_VARS');
});
it('!authn !cookie -> ok', async () => {
// Should not throw.
socket = await connect(null);
const clientVars = await handshake(socket, 'pad');
assert.equal(clientVars.type, 'CLIENT_VARS');
});
it('!authn user /p/pad -> 200, ok', async () => {
const res = await client.get('/p/pad').auth('user', 'user-password').expect(200);
// Should not throw.
@ -160,7 +166,7 @@ describe('socket.io access checks', () => {
// Despite the 401, try to create the pad via a socket.io connection anyway.
await assert.rejects(connect(res), {message: /authentication required/i});
});
it('socket.io connection without express-session cookie -> error', async () => {
it('authn !cookie -> error', async () => {
settings.requireAuthentication = true;
await assert.rejects(connect(null), {message: /signed express_sid cookie is required/i});
});