diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index ad802f327..818f04d9d 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -60,15 +60,15 @@ exports.checkAccess = async function(padID, sessionCookie, token, password, user let canCreate = !settings.editOnly; - if (settings.requireAuthentication) { - // Make sure the user has authenticated if authentication is required. The caller should have - // already performed this check, but it is repeated here just in case. + // Authentication and authorization checks. + if (settings.loadTest) { + console.warn( + 'bypassing socket.io authentication and authorization checks due to settings.loadTest'); + } else if (settings.requireAuthentication) { if (userSettings == null) { authLogger.debug('access denied: authentication is required'); return DENY; } - - // Check whether the user is authorized to create the pad if it doesn't exist. if (userSettings.canCreate != null && !userSettings.canCreate) canCreate = false; if (userSettings.readOnly) canCreate = false; // Note: userSettings.padAuthorizations should still be populated even if diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index 8ec3b25e3..b1e07ba91 100644 --- a/src/node/hooks/express/socketio.js +++ b/src/node/hooks/express/socketio.js @@ -40,15 +40,6 @@ exports.expressCreateServer = function (hook_name, args, cb) { cookie: false, }); - // REQUIRE a signed express-session cookie to be present, then load the session. See - // http://www.danielbaulig.de/socket-ioexpress for more info. After the session is loaded, ensure - // that the user has authenticated (if authentication is required). - // - // !!!WARNING!!! Requests to /socket.io are NOT subject to the checkAccess middleware in - // webaccess.js. If this handler fails to check for a signed express-session cookie or fails to - // 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). const cookieParserFn = util.promisify(cookieParser(settings.sessionKey, {})); const getSession = util.promisify(webaccess.sessionStore.get).bind(webaccess.sessionStore); io.use(async (socket, next) => { @@ -58,24 +49,14 @@ exports.expressCreateServer = function (hook_name, args, cb) { // token and express_sid cookies have to be passed via a query parameter for unit tests. req.headers.cookie = socket.handshake.query.cookie; } - if (!req.headers.cookie && settings.loadTest) { - console.warn('bypassing socket.io authentication check due to settings.loadTest'); - return 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); + await cookieParserFn(req, {}); + const expressSid = req.signedCookies.express_sid; + if (expressSid) { + const session = await getSession(expressSid); + if (session) req.session = new sessionModule.Session(req, session); } + // Note: PadMessageHandler.handleMessage calls SecurityMananger.checkAccess which will perform + // authentication and authorization checks. return next(null, true); }); diff --git a/tests/backend/specs/socketio.js b/tests/backend/specs/socketio.js index e89d48725..6a4541d74 100644 --- a/tests/backend/specs/socketio.js +++ b/tests/backend/specs/socketio.js @@ -155,20 +155,17 @@ describe('socket.io access checks', function() { describe('Normal accesses', function() { it('!authn anonymous cookie /p/pad -> 200, ok', async function() { const res = await agent.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 function() { - // 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 function() { const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); - // Should not throw. socket = await connect(res); const clientVars = await handshake(socket, 'pad'); assert.equal(clientVars.type, 'CLIENT_VARS'); @@ -176,7 +173,6 @@ describe('socket.io access checks', function() { it('authn user /p/pad -> 200, ok', async function() { settings.requireAuthentication = true; const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); - // Should not throw. socket = await connect(res); const clientVars = await handshake(socket, 'pad'); assert.equal(clientVars.type, 'CLIENT_VARS'); @@ -185,7 +181,6 @@ describe('socket.io access checks', function() { settings.requireAuthentication = true; settings.requireAuthorization = true; const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); - // Should not throw. socket = await connect(res); const clientVars = await handshake(socket, 'pad'); assert.equal(clientVars.type, 'CLIENT_VARS'); @@ -199,7 +194,6 @@ describe('socket.io access checks', function() { settings.requireAuthorization = true; const encodedPadId = encodeURIComponent('päd'); const res = await agent.get(`/p/${encodedPadId}`).auth('user', 'user-password').expect(200); - // Should not throw. socket = await connect(res); const clientVars = await handshake(socket, 'päd'); assert.equal(clientVars.type, 'CLIENT_VARS'); @@ -211,11 +205,15 @@ describe('socket.io access checks', function() { settings.requireAuthentication = true; const res = await agent.get('/p/pad').expect(401); // Despite the 401, try to create the pad via a socket.io connection anyway. - await assert.rejects(connect(res), {message: /authentication required/i}); + socket = await connect(res); + const message = await handshake(socket, 'pad'); + assert.equal(message.accessStatus, 'deny'); }); it('authn !cookie -> error', async function() { settings.requireAuthentication = true; - await assert.rejects(connect(null), {message: /signed express_sid cookie is required/i}); + socket = await connect(null); + const message = await handshake(socket, 'pad'); + assert.equal(message.accessStatus, 'deny'); }); it('authorization bypass attempt -> error', async function() { // Only allowed to access /p/pad. @@ -224,7 +222,6 @@ describe('socket.io access checks', function() { settings.requireAuthorization = true; // First authenticate and establish a session. const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); - // Connecting should work because the user successfully authenticated. socket = await connect(res); // Accessing /p/other-pad should fail, despite the successful fetch of /p/pad. const message = await handshake(socket, 'other-pad');