public-offering
/
pad.pub0.org
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

socketio: Delete redundant authentication check 

There's no need to perform an authentication check in the socket.io
middleware because `PadMessageHandler.handleMessage` calls
`SecurityMananger.checkAccess` and that now performs authentication
and authorization checks.

This change also improves the user experience: Before, access denials
caused socket.io error events in the client, which `pad.js` mostly
ignores (the user doesn't see anything). Now a deny message is sent
back to the client, which causes `pad.js` to display an obvious
permission denied message.

This also fixes a minor bug: `settings.loadTest` is supposed to bypass
authentication and authorization checks, but they weren't bypassed
because `SecurityManager.checkAccess` did not check
`settings.loadTest`.
pull/4383/head
Richard Hansen 2020-10-03 17:52:10 -04:00 committed by John McLear
parent 3f8365a995
commit f7953ece85
3 changed files with 18 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/node/db/SecurityManager.js
View File

@ -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

25
src/node/hooks/express/socketio.js
View File

@ -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);
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);
});

15
tests/backend/specs/socketio.js
View File

@ -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');