From 9e6d3f3f636e37a93055df349b3806ebaa388e6f Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 14 Sep 2020 00:39:10 -0400 Subject: [PATCH] tests: Add authentication, authorization bypass tests --- src/node/hooks/express/socketio.js | 5 + src/package-lock.json | 6 + src/package.json | 3 +- tests/backend/specs/socketio.js | 197 +++++++++++++++++++++++++++++ 4 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 tests/backend/specs/socketio.js diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index b1a4d4f22..b1406afd2 100644 --- a/src/node/hooks/express/socketio.js +++ b/src/node/hooks/express/socketio.js @@ -51,6 +51,11 @@ exports.expressCreateServer = function (hook_name, args, cb) { var cookieParserFn = cookieParser(webaccess.secret, {}); io.use((socket, next) => { var data = socket.request; + if (!data.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; + } if (!data.headers.cookie && settings.loadTest) { console.warn('bypassing socket.io authentication check due to settings.loadTest'); return next(null, true); diff --git a/src/package-lock.json b/src/package-lock.json index b26c8c54f..6b9b9b0c8 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -7781,6 +7781,12 @@ "resolved": "https://registry.npmjs.org/set-blocking/-/set-blocking-2.0.0.tgz", "integrity": "sha1-BF+XgtARrppoA93TgrJDkrPYkPc=" }, + "set-cookie-parser": { + "version": "2.4.6", + "resolved": "https://registry.npmjs.org/set-cookie-parser/-/set-cookie-parser-2.4.6.tgz", + "integrity": "sha512-mNCnTUF0OYPwYzSHbdRdCfNNHqrne+HS5tS5xNb6yJbdP9wInV0q5xPLE0EyfV/Q3tImo3y/OXpD8Jn0Jtnjrg==", + "dev": true + }, "setprototypeof": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/setprototypeof/-/setprototypeof-1.1.1.tgz", diff --git a/src/package.json b/src/package.json index 2556b4c33..7247b7c4e 100644 --- a/src/package.json +++ b/src/package.json @@ -78,6 +78,7 @@ "mocha": "7.1.2", "mocha-froth": "^0.2.10", "nyc": "15.0.1", + "set-cookie-parser": "^2.4.6", "supertest": "4.0.2", "wd": "1.12.1" }, @@ -90,7 +91,7 @@ "url": "https://github.com/ether/etherpad-lite.git" }, "scripts": { - "test": "nyc mocha --timeout 5000 ../tests/backend/specs/api", + "test": "nyc mocha --timeout 5000 --recursive ../tests/backend/specs", "test-contentcollector": "nyc mocha --timeout 5000 ../tests/backend/specs", "test-container": "nyc mocha --timeout 5000 ../tests/container/specs/api" }, diff --git a/tests/backend/specs/socketio.js b/tests/backend/specs/socketio.js new file mode 100644 index 000000000..b38aa1f6b --- /dev/null +++ b/tests/backend/specs/socketio.js @@ -0,0 +1,197 @@ +function m(mod) { return __dirname + '/../../../src/' + mod; } + +const assert = require('assert').strict; +const db = require(m('node/db/DB')); +const express = require(m('node_modules/express')); +const http = require('http'); +const log4js = require(m('node_modules/log4js')); +let padManager; +const plugins = require(m('static/js/pluginfw/plugin_defs')); +const setCookieParser = require(m('node_modules/set-cookie-parser')); +const settings = require(m('node/utils/Settings')); +const io = require(m('node_modules/socket.io-client')); +const stats = require(m('node/stats')); +const supertest = require(m('node_modules/supertest')); +const util = require('util'); + +const logger = log4js.getLogger('test'); +const app = express(); +const server = http.createServer(app); +let client; +let baseUrl; + +before(async () => { + await util.promisify(server.listen).bind(server)(0, 'localhost'); + baseUrl = `http://localhost:${server.address().port}`; + logger.debug(`HTTP server at ${baseUrl}`); + client = supertest(baseUrl); + const npm = require(m('node_modules/npm/lib/npm.js')); + await util.promisify(npm.load)(); + settings.users = { + admin: {password: 'admin-password', is_admin: true}, + user: {password: 'user-password'}, + }; + await db.init(); + padManager = require(m('node/db/PadManager')); + const webaccess = require(m('node/hooks/express/webaccess')); + webaccess.expressConfigure('expressConfigure', {app}); + const socketio = require(m('node/hooks/express/socketio')); + socketio.expressCreateServer('expressCreateServer', {app, server}); + app.get(/./, (req, res) => { res.status(200).send('OK'); }); +}); + +after(async () => { + stats.end(); + await Promise.all([ + db.doShutdown(), + util.promisify(server.close).bind(server)(), + ]); +}); + +// Waits for and returns the next named socket.io event. Rejects if there is any error while waiting +// (unless waiting for that error event). +const getSocketEvent = async (socket, event) => { + const errorEvents = [ + 'error', + 'connect_error', + 'connect_timeout', + 'reconnect_error', + 'reconnect_failed', + ]; + const handlers = {}; + let timeoutId; + return new Promise((resolve, reject) => { + timeoutId = setTimeout(() => reject(new Error(`timed out waiting for ${event} event`)), 1000); + for (const event of errorEvents) { + handlers[event] = (errorString) => { + logger.debug(`socket.io ${event} event: ${errorString}`); + reject(new Error(errorString)); + }; + } + // This will overwrite one of the above handlers if the user is waiting for an error event. + handlers[event] = (...args) => { + logger.debug(`socket.io ${event} event`); + if (args.length > 1) return resolve(args); + resolve(args[0]); + } + Object.entries(handlers).forEach(([event, handler]) => socket.on(event, handler)); + }).finally(() => { + clearTimeout(timeoutId); + Object.entries(handlers).forEach(([event, handler]) => socket.off(event, handler)); + }); +}; + +// Establishes a new socket.io connection. Passes the cookies from the `set-cookie` header(s) in +// `res` (which may be nullish) to the server. Returns a socket.io Socket object. +const connect = async (res) => { + // Convert the `set-cookie` header(s) into a `cookie` header. + const resCookies = (res == null) ? {} : setCookieParser.parse(res, {map: true}); + const reqCookieHdr = Object.entries(resCookies).map(([name, cookie]) => { + return `${name}=${encodeURIComponent(cookie.value)}`; + }).join('; '); + + logger.debug('socket.io connecting...'); + const socket = io(`${baseUrl}/`, { + forceNew: true, // Different tests will have different query parameters. + path: '/socket.io', + // socketio.js-client on node.js doesn't support cookies (see https://git.io/JU8u9), so the + // express_sid cookie must be passed as a query parameter. + query: {cookie: reqCookieHdr}, + }); + try { + await getSocketEvent(socket, 'connect'); + } catch (e) { + socket.close(); + throw e; + } + logger.debug('socket.io connected'); + + return socket; +}; + +// Helper function to exchange CLIENT_READY+CLIENT_VARS messages for the named pad. +// Returns the CLIENT_VARS message from the server. +const handshake = async (socket, padID) => { + logger.debug('sending CLIENT_READY...'); + socket.send({ + component: 'pad', + type: 'CLIENT_READY', + padId: padID, + sessionID: null, + password: null, + token: 't.12345', + protocolVersion: 2, + }); + logger.debug('waiting for CLIENT_VARS response...'); + const msg = await getSocketEvent(socket, 'message'); + logger.debug('received CLIENT_VARS message'); + return msg; +}; + +describe('socket.io access checks', () => { + let socket; + beforeEach(async () => { + assert(socket == null); + settings.requireAuthentication = false; + settings.requireAuthorization = false; + Promise.all(['pad', 'other-pad'].map(async (pad) => { + if (await padManager.doesPadExist(pad)) (await padManager.getPad(pad)).remove(); + })); + }); + afterEach(async () => { + if (socket) socket.close(); + socket = null; + }); + + // Normal accesses. + it('!authn anonymous /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 user /p/pad -> 200, ok', async () => { + const res = await client.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'); + }); + it('authn user /p/pad -> 200, ok', async () => { + settings.requireAuthentication = true; + const res = await client.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'); + }); + + // Abnormal access attempts. + it('authn anonymous /p/pad -> 401, error', async () => { + settings.requireAuthentication = true; + const res = await client.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}); + }); + it('socket.io connection without express-session cookie -> error', async () => { + settings.requireAuthentication = true; + await assert.rejects(connect(null), {message: /signed express_sid cookie is required/i}); + }); + xit('authorization bypass attempt -> error', async () => { + plugins.hooks.authorize = [{hook_fn: (hookName, {req}, cb) => { + if (req.session.user == null) return cb([]); // Hasn't authenticated yet. + // Only allowed to access /p/pad. + return cb([req.path === '/p/pad']); + }}]; + settings.requireAuthentication = true; + settings.requireAuthorization = true; + // First authenticate and establish a session. + const res = await client.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'); + assert.equal(message.accessStatus, 'deny'); + }); +});