From e2b3b009e1ba928b29dcbe1539ef31793f7a11dc Mon Sep 17 00:00:00 2001 From: webzwo0i Date: Wed, 9 Sep 2020 22:40:53 +0200 Subject: [PATCH 01/16] tests: skip responsivness test on firefox 52.0/windows (#4275) --- tests/frontend/specs/responsiveness.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/frontend/specs/responsiveness.js b/tests/frontend/specs/responsiveness.js index 3eb7a1919..053ade217 100644 --- a/tests/frontend/specs/responsiveness.js +++ b/tests/frontend/specs/responsiveness.js @@ -23,7 +23,12 @@ describe('Responsiveness of Editor', function() { // And the test needs to be fixed to work in Firefox 52 on Windows 7. I am not sure why it fails on this specific platform // The errors show this.timeout... then crash the browser but I am sure something is actually causing the stack trace and // I just need to narrow down what, offers to help accepted. - xit('Fast response to keypress in pad with large amount of contents', function(done) { + it('Fast response to keypress in pad with large amount of contents', function(done) { + + //skip on Windows Firefox 52.0 + if(window.bowser && window.bowser.windows && window.bowser.firefox && window.bowser.version == "52.0") { + this.skip(); + } var inner$ = helper.padInner$; var chrome$ = helper.padChrome$; var chars = '0000000000'; // row of placeholder chars From 24978daeb021ef3a5e98f3e495c586f70000b7de Mon Sep 17 00:00:00 2001 From: "translatewiki.net" Date: Thu, 10 Sep 2020 18:36:59 +0200 Subject: [PATCH 02/16] Localisation updates from https://translatewiki.net. --- src/locales/pl.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locales/pl.json b/src/locales/pl.json index 77d4fae4b..68f293708 100644 --- a/src/locales/pl.json +++ b/src/locales/pl.json @@ -4,6 +4,7 @@ "DeRudySoulStorm", "Macofe", "Mateon1", + "Matlin", "Pan Cube", "Rezonansowy", "Teeed", @@ -51,6 +52,7 @@ "pad.settings.fontType.normal": "Normalna", "pad.settings.language": "Język:", "pad.settings.about": "O aplikacji", + "pad.settings.poweredBy": "Dostarczane przez $1", "pad.importExport.import_export": "Import/eksport", "pad.importExport.import": "Prześlij dowolny plik tekstowy lub dokument", "pad.importExport.importSuccessful": "Sukces!", From ed3c82e8c3c435f682b2936fa3c7d1070aaf7b5d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Sep 2020 18:54:06 -0400 Subject: [PATCH 03/16] Use `null`, not `"null"`, if `sessionID` cookie doesn't exist `decodeURIComponent(null)` returns the string `'null'`, which we don't want. --- src/static/js/pad.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/static/js/pad.js b/src/static/js/pad.js index cfc58f18e..2e774a38a 100644 --- a/src/static/js/pad.js +++ b/src/static/js/pad.js @@ -156,7 +156,8 @@ function sendClientReady(isReconnect, messageType) createCookie("token", token, 60); } - var sessionID = decodeURIComponent(readCookie("sessionID")); + var encodedSessionID = readCookie('sessionID'); + var sessionID = encodedSessionID == null ? null : decodeURIComponent(encodedSessionID); var password = readCookie("password"); var msg = { From d4db091d1d50c86f12987b6710436d9d81463d93 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 2 Sep 2020 19:33:08 -0400 Subject: [PATCH 04/16] PadMessageHandler: Simplify handleClientReady a bit Before, this function referred to the same author ID in different ways in different places. Use one spelling to make the code easier to read. --- src/node/handler/PadMessageHandler.js | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index fc3b0a19a..27dfd88a0 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -894,8 +894,8 @@ async function handleClientReady(client, message) let padIds = await readOnlyManager.getIds(message.padId); // FIXME: Allow to override readwrite access with readonly - let statusObject = await securityManager.checkAccess(padIds.padId, message.sessionID, message.token, message.password); - let accessStatus = statusObject.accessStatus; + const {accessStatus, authorID} = await securityManager.checkAccess( + padIds.padId, message.sessionID, message.token, message.password); // no access, send the client a message that tells him why if (accessStatus !== "grant") { @@ -903,11 +903,9 @@ async function handleClientReady(client, message) return; } - let author = statusObject.authorID; - // get all authordata of this new user - assert(author); - let value = await authorManager.getAuthor(author); + assert(authorID); + let value = await authorManager.getAuthor(authorID); let authorColorId = value.colorId; let authorName = value.name; @@ -933,7 +931,7 @@ async function handleClientReady(client, message) })); let thisUserHasEditedThisPad = false; - if (historicalAuthorData[statusObject.authorID]) { + if (historicalAuthorData[authorID]) { /* * This flag is set to true when a user contributes to a specific pad for * the first time. It is used for deciding if importing to that pad is @@ -954,7 +952,7 @@ async function handleClientReady(client, message) for (let client of roomClients) { let sinfo = sessioninfos[client.id]; - if (sinfo && sinfo.author == author) { + if (sinfo && sinfo.author == authorID) { // fix user's counter, works on page refresh or if user closes browser window and then rejoins sessioninfos[client.id] = {}; client.leave(padIds.padId); @@ -1104,7 +1102,7 @@ async function handleClientReady(client, message) "readOnlyId": padIds.readOnlyPadId, "readonly": padIds.readonly, "serverTimestamp": Date.now(), - "userId": author, + "userId": authorID, "abiwordAvailable": settings.abiwordAvailable(), "sofficeAvailable": settings.sofficeAvailable(), "exportAvailable": settings.exportAvailable(), @@ -1149,7 +1147,7 @@ async function handleClientReady(client, message) // Save the current revision in sessioninfos, should be the same as in clientVars sessioninfos[client.id].rev = pad.getHeadRevisionNumber(); - sessioninfos[client.id].author = author; + sessioninfos[client.id].author = authorID; // prepare the notification for the other users on the pad, that this user joined let messageToTheOtherUsers = { @@ -1160,7 +1158,7 @@ async function handleClientReady(client, message) "ip": "127.0.0.1", "colorId": authorColorId, "userAgent": "Anonymous", - "userId": author + "userId": authorID, } } }; From 7f0770d684e265cb00496e78b0367ba5589db70a Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 2 Sep 2020 20:44:26 -0400 Subject: [PATCH 05/16] PadMessageHandler: Invert logic to improve readability --- src/node/handler/PadMessageHandler.js | 74 +++++++++++++-------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 27dfd88a0..32752bce5 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -260,45 +260,45 @@ exports.handleMessage = async function(client, message) } let dropMessage = await handleMessageHook(); - if (!dropMessage) { - if (message.type == "CLIENT_READY") { - // client tried to auth for the first time (first msg from the client) - createSessionInfo(client, message); - } + if (dropMessage) return; - // the session may have been dropped during earlier processing - if (!sessioninfos[client.id]) { - messageLogger.warn("Dropping message from a connection that has gone away.") - return; - } - - // Simulate using the load testing tool - if (!sessioninfos[client.id].auth) { - console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") - return; - } - - let auth = sessioninfos[client.id].auth; - - // check if pad is requested via readOnly - let padId = auth.padID; - - if (padId.indexOf("r.") === 0) { - // Pad is readOnly, first get the real Pad ID - padId = await readOnlyManager.getPadId(padId); - } - - let { accessStatus } = await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password); - - if (accessStatus !== "grant") { - // no access, send the client a message that tells him why - client.json.send({ accessStatus }); - return; - } - - // access was granted - finalHandler(); + if (message.type == "CLIENT_READY") { + // client tried to auth for the first time (first msg from the client) + createSessionInfo(client, message); } + + // the session may have been dropped during earlier processing + if (!sessioninfos[client.id]) { + messageLogger.warn("Dropping message from a connection that has gone away.") + return; + } + + // Simulate using the load testing tool + if (!sessioninfos[client.id].auth) { + console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") + return; + } + + let auth = sessioninfos[client.id].auth; + + // check if pad is requested via readOnly + let padId = auth.padID; + + if (padId.indexOf("r.") === 0) { + // Pad is readOnly, first get the real Pad ID + padId = await readOnlyManager.getPadId(padId); + } + + let { accessStatus } = await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password); + + if (accessStatus !== "grant") { + // no access, send the client a message that tells him why + client.json.send({ accessStatus }); + return; + } + + // access was granted + finalHandler(); } From de792559cb024b777563146a067425a3d03acd80 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 3 Sep 2020 13:30:16 -0400 Subject: [PATCH 06/16] PadMessageHandler: Use `===` instead of `==` for comparison --- src/node/handler/PadMessageHandler.js | 60 +++++++++++++-------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 32752bce5..9eb9c7ba8 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -106,7 +106,7 @@ exports.kickSessionsFromPad = function(padID) return; // skip if there is nobody on this pad - if(_getRoomClients(padID).length == 0) + if(_getRoomClients(padID).length === 0) return; // disconnect everyone from this pad @@ -227,32 +227,32 @@ exports.handleMessage = async function(client, message) function finalHandler() { // Check what type of message we get and delegate to the other methods - if (message.type == "CLIENT_READY") { + if (message.type === "CLIENT_READY") { handleClientReady(client, message); - } else if (message.type == "CHANGESET_REQ") { + } else if (message.type === "CHANGESET_REQ") { handleChangesetRequest(client, message); - } else if(message.type == "COLLABROOM") { + } else if(message.type === "COLLABROOM") { if (thisSession.readonly) { messageLogger.warn("Dropped message, COLLABROOM for readonly pad"); - } else if (message.data.type == "USER_CHANGES") { + } else if (message.data.type === "USER_CHANGES") { stats.counter('pendingEdits').inc() padChannels.emit(message.padId, {client: client, message: message}); // add to pad queue - } else if (message.data.type == "USERINFO_UPDATE") { + } else if (message.data.type === "USERINFO_UPDATE") { handleUserInfoUpdate(client, message); - } else if (message.data.type == "CHAT_MESSAGE") { + } else if (message.data.type === "CHAT_MESSAGE") { handleChatMessage(client, message); - } else if (message.data.type == "GET_CHAT_MESSAGES") { + } else if (message.data.type === "GET_CHAT_MESSAGES") { handleGetChatMessages(client, message); - } else if (message.data.type == "SAVE_REVISION") { + } else if (message.data.type === "SAVE_REVISION") { handleSaveRevisionMessage(client, message); - } else if (message.data.type == "CLIENT_MESSAGE" && + } else if (message.data.type === "CLIENT_MESSAGE" && message.data.payload != null && - message.data.payload.type == "suggestUserName") { + message.data.payload.type === "suggestUserName") { handleSuggestUserName(client, message); } else { messageLogger.warn("Dropped message, unknown COLLABROOM Data Type " + message.data.type); } - } else if(message.type == "SWITCH_TO_PAD") { + } else if(message.type === "SWITCH_TO_PAD") { handleSwitchToPad(client, message); } else { messageLogger.warn("Dropped message, unknown Message Type " + message.type); @@ -262,7 +262,7 @@ exports.handleMessage = async function(client, message) let dropMessage = await handleMessageHook(); if (dropMessage) return; - if (message.type == "CLIENT_READY") { + if (message.type === "CLIENT_READY") { // client tried to auth for the first time (first msg from the client) createSessionInfo(client, message); } @@ -461,7 +461,7 @@ function handleSuggestUserName(client, message) // search the author and send him this message roomClients.forEach(function(client) { var session = sessioninfos[client.id]; - if (session && session.author == message.data.payload.unnamedId) { + if (session && session.author === message.data.payload.unnamedId) { client.json.send(message); } }); @@ -621,7 +621,7 @@ async function handleUserChanges(data) if (!attr) return; // the empty author is used in the clearAuthorship functionality so this should be the only exception - if ('author' == attr[0] && (attr[1] != thisSession.author && attr[1] != '')) { + if ('author' === attr[0] && (attr[1] !== thisSession.author && attr[1] !== '')) { throw new Error("Trying to submit changes as another author in changeset " + changeset); } }); @@ -660,7 +660,7 @@ async function handleUserChanges(data) // a changeset can be based on an old revision with the same changes in it // prevent eplite from accepting it TODO: better send the client a NEW_CHANGES // of that revision - if (baseRev + 1 == r && c == changeset) { + if (baseRev + 1 === r && c === changeset) { client.json.send({disconnect:"badChangeset"}); stats.meter('failedChangesets').mark(); throw new Error("Won't apply USER_CHANGES, because it contains an already accepted changeset"); @@ -676,7 +676,7 @@ async function handleUserChanges(data) let prevText = pad.text(); - if (Changeset.oldLen(changeset) != prevText.length) { + if (Changeset.oldLen(changeset) !== prevText.length) { client.json.send({disconnect:"badChangeset"}); stats.meter('failedChangesets').mark(); throw new Error("Can't apply USER_CHANGES "+changeset+" with oldLen " + Changeset.oldLen(changeset) + " to document of length " + prevText.length); @@ -696,7 +696,7 @@ async function handleUserChanges(data) } // Make sure the pad always ends with an empty line. - if (pad.text().lastIndexOf("\n") != pad.text().length-1) { + if (pad.text().lastIndexOf("\n") !== pad.text().length-1) { var nlChangeset = Changeset.makeSplice(pad.text(), pad.text().length - 1, 0, "\n"); pad.appendRevision(nlChangeset); } @@ -714,7 +714,7 @@ exports.updatePadClients = async function(pad) // skip this if no-one is on this pad let roomClients = _getRoomClients(pad.id); - if (roomClients.length == 0) { + if (roomClients.length === 0) { return; } @@ -749,7 +749,7 @@ exports.updatePadClients = async function(pad) continue; } - if (author == sessioninfos[sid].author) { + if (author === sessioninfos[sid].author) { client.json.send({ "type": "COLLABROOM", "data":{ type: "ACCEPT_COMMIT", newRev: r }}); } else { let forWire = Changeset.prepareForWire(revChangeset, pad.pool); @@ -794,7 +794,7 @@ function _correctMarkersInPad(atext, apool) { if (hasMarker) { for (var i = 0; i < op.chars; i++) { - if (offset > 0 && text.charAt(offset-1) != '\n') { + if (offset > 0 && text.charAt(offset-1) !== '\n') { badMarkers.push(offset); } offset++; @@ -804,7 +804,7 @@ function _correctMarkersInPad(atext, apool) { } } - if (badMarkers.length == 0) { + if (badMarkers.length === 0) { return null; } @@ -831,7 +831,7 @@ function handleSwitchToPad(client, message) roomClients.forEach(client => { let sinfo = sessioninfos[client.id]; - if (sinfo && sinfo.author == currentSession.author) { + if (sinfo && sinfo.author === currentSession.author) { // fix user's counter, works on page refresh or if user closes browser window and then rejoins sessioninfos[client.id] = {}; client.leave(padId); @@ -883,7 +883,7 @@ async function handleClientReady(client, message) return; } - if (message.protocolVersion != 2) { + if (message.protocolVersion !== 2) { messageLogger.warn("Dropped message, CLIENT_READY Message has a unknown protocolVersion '" + message.protocolVersion + "'!"); return; } @@ -952,7 +952,7 @@ async function handleClientReady(client, message) for (let client of roomClients) { let sinfo = sessioninfos[client.id]; - if (sinfo && sinfo.author == authorID) { + if (sinfo && sinfo.author === authorID) { // fix user's counter, works on page refresh or if user closes browser window and then rejoins sessioninfos[client.id] = {}; client.leave(padIds.padId); @@ -975,7 +975,7 @@ async function handleClientReady(client, message) if (pad.head > 0) { accessLogger.info('[ENTER] Pad "' + padIds.padId + '": Client ' + client.id + ' with IP "' + ip + '" entered the pad'); - } else if (pad.head == 0) { + } else if (pad.head === 0) { accessLogger.info('[CREATE] Pad "' + padIds.padId + '": Client ' + client.id + ' with IP "' + ip + '" created the pad'); } @@ -1036,7 +1036,7 @@ async function handleClientReady(client, message) client.json.send(wireMsg); } - if (startNum == endNum) { + if (startNum === endNum) { var Msg = {"type":"COLLABROOM", "data":{type:"CLIENT_RECONNECT", noChanges: true, @@ -1176,7 +1176,7 @@ async function handleClientReady(client, message) await Promise.all(_getRoomClients(pad.id).map(async roomClient => { // Jump over, if this session is the connection session - if (roomClient.id == client.id) { + if (roomClient.id === client.id) { return; } @@ -1316,7 +1316,7 @@ async function getChangesetInfo(padId, startNum, endNum, granularity) compositesChangesetNeeded.push({ start, end }); // add the t1 time we need - revTimesNeeded.push(start == 0 ? 0 : start - 1); + revTimesNeeded.push(start === 0 ? 0 : start - 1); // add the t2 time we need revTimesNeeded.push(end - 1); @@ -1371,7 +1371,7 @@ async function getChangesetInfo(padId, startNum, endNum, granularity) let forwards2 = Changeset.moveOpsToNewPool(forwards, pad.apool(), apool); let backwards2 = Changeset.moveOpsToNewPool(backwards, pad.apool(), apool); - let t1 = (compositeStart == 0) ? revisionDate[0] : revisionDate[compositeStart - 1]; + let t1 = (compositeStart === 0) ? revisionDate[0] : revisionDate[compositeStart - 1]; let t2 = revisionDate[compositeEnd - 1]; timeDeltas.push(t2 - t1); From 3262ff1cb912bc2083e6b5fa13d482e36031beed Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 2 Sep 2020 20:32:54 -0400 Subject: [PATCH 07/16] PadMessageHandler: Rename createSessionInfo to createSessionInfoAuth The function doesn't create the session info -- it creates the auth property of existing session info. --- src/node/handler/PadMessageHandler.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 9eb9c7ba8..ed1c970d6 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -264,7 +264,7 @@ exports.handleMessage = async function(client, message) if (message.type === "CLIENT_READY") { // client tried to auth for the first time (first msg from the client) - createSessionInfo(client, message); + createSessionInfoAuth(client, message); } // the session may have been dropped during earlier processing @@ -839,11 +839,13 @@ function handleSwitchToPad(client, message) }); // start up the new pad - createSessionInfo(client, message); + createSessionInfoAuth(client, message); handleClientReady(client, message); } -function createSessionInfo(client, message) +// Creates/replaces the auth object in the client's session info. Session info for the client must +// already exist. +function createSessionInfoAuth(client, message) { // Remember this information since we won't // have the cookie in further socket.io messages. From 8756fed80dd9b97a418ce292843b389b79e348b0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Sep 2020 18:12:20 -0400 Subject: [PATCH 08/16] PadMessageHandler: Use `await` instead of `p.then()` --- src/node/handler/PadMessageHandler.js | 68 +++++++++++++-------------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index ed1c970d6..77f6d39ce 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -1193,48 +1193,46 @@ async function handleClientReady(client, message) let cached = historicalAuthorData[author]; // reuse previously created cache of author's data - let p = cached ? Promise.resolve(cached) : authorManager.getAuthor(author); + const authorInfo = await (cached ? Promise.resolve(cached) : authorManager.getAuthor(author)); - return p.then(authorInfo => { - // default fallback color to use if authorInfo.colorId is null - const defaultColor = "#daf0b2"; + // default fallback color to use if authorInfo.colorId is null + const defaultColor = "#daf0b2"; - if (!authorInfo) { - console.warn(`handleClientReady(): no authorInfo parameter was received. Default values are going to be used. See issue #3612. This can be caused by a user clicking undo after clearing all authorship colors see #2802`); - authorInfo = {}; - } + if (!authorInfo) { + console.warn(`handleClientReady(): no authorInfo parameter was received. Default values are going to be used. See issue #3612. This can be caused by a user clicking undo after clearing all authorship colors see #2802`); + authorInfo = {}; + } - // For some reason sometimes name isn't set - // Catch this issue here and use a fixed name. - if (!authorInfo.name) { - console.warn(`handleClientReady(): client submitted no author name. Using "Anonymous". See: issue #3612`); - authorInfo.name = "Anonymous"; - } + // For some reason sometimes name isn't set + // Catch this issue here and use a fixed name. + if (!authorInfo.name) { + console.warn(`handleClientReady(): client submitted no author name. Using "Anonymous". See: issue #3612`); + authorInfo.name = "Anonymous"; + } - // For some reason sometimes colorId isn't set - // Catch this issue here and use a fixed color. - if (!authorInfo.colorId) { - console.warn(`handleClientReady(): author "${authorInfo.name}" has no property colorId. Using the default color ${defaultColor}. See issue #3612`); - authorInfo.colorId = defaultColor; - } + // For some reason sometimes colorId isn't set + // Catch this issue here and use a fixed color. + if (!authorInfo.colorId) { + console.warn(`handleClientReady(): author "${authorInfo.name}" has no property colorId. Using the default color ${defaultColor}. See issue #3612`); + authorInfo.colorId = defaultColor; + } - // Send the new User a Notification about this other user - let msg = { - "type": "COLLABROOM", - "data": { - type: "USER_NEWINFO", - userInfo: { - "ip": "127.0.0.1", - "colorId": authorInfo.colorId, - "name": authorInfo.name, - "userAgent": "Anonymous", - "userId": author - } + // Send the new User a Notification about this other user + let msg = { + "type": "COLLABROOM", + "data": { + type: "USER_NEWINFO", + userInfo: { + "ip": "127.0.0.1", + "colorId": authorInfo.colorId, + "name": authorInfo.name, + "userAgent": "Anonymous", + "userId": author } - }; + } + }; - client.json.send(msg); - }); + client.json.send(msg); })); } } From 8b0baa96797718985b0557d25d4696c19220c309 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Sep 2020 12:47:59 -0400 Subject: [PATCH 09/16] SecurityManager: Refactor checkAccess for readability, correctness * Move session validity check and session author ID fetch to a separate function. This separate function can be used by hooks, making it easier for them to properly determine the author ID. * Rewrite the remainder of checkAccess. Benefits: - The function is more readable and maintainable now. - Vulnerability fix: Before, the session IDs in sessionCookie were not validated when checking settings.requireSession. Now, sessionCookie must identify a valid session for the settings.requireSession test to pass. - Bug fix: Before, checkAccess would sometimes use the author ID associated with the token even if sessionCookie identified a valid session. Now it always uses the author ID associated with the session if available. --- src/node/db/SecurityManager.js | 253 +++++++-------------------------- src/node/db/SessionManager.js | 54 +++++++ src/node/utils/promises.js | 39 ++++- 3 files changed, 136 insertions(+), 210 deletions(-) diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index cb7830968..fd381f3a8 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -48,234 +48,77 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'}); * * WARNING: Tokens and session IDs MUST be kept secret, otherwise users will be able to impersonate * each other (which might allow them to gain privileges). - * - * TODO: Add a hook so that plugins can make access decisions. */ exports.checkAccess = async function(padID, sessionCookie, token, password) { if (!padID) { + authLogger.debug('access denied: missing padID'); return DENY; } // allow plugins to deny access - var deniedByHook = hooks.callAll("onAccessCheck", {'padID': padID, 'password': password, 'token': token, 'sessionCookie': sessionCookie}).indexOf(false) > -1; - if (deniedByHook) { + const isFalse = (x) => x === false; + if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) { + authLogger.debug('access denied: an onAccessCheck hook function returned false'); return DENY; } - // start to get author for this token - let p_tokenAuthor = authorManager.getAuthor4Token(token); + // start fetching the info we may need + const p_sessionAuthorID = sessionManager.findAuthorID(padID.split('$')[0], sessionCookie); + const p_tokenAuthorID = authorManager.getAuthor4Token(token); + const p_padExists = padManager.doesPadExist(padID); - // start to check if pad exists - let p_padExists = padManager.doesPadExist(padID); + const padExists = await p_padExists; + if (!padExists && settings.editOnly) { + authLogger.debug('access denied: user attempted to create a pad, which is prohibited'); + return DENY; + } - if (settings.requireSession) { - // a valid session is required (api-only mode) - if (!sessionCookie) { - // without sessionCookie, access is denied + const sessionAuthorID = await p_sessionAuthorID; + if (settings.requireSession && !sessionAuthorID) { + authLogger.debug('access denied: HTTP API session is required'); + return DENY; + } + + const grant = { + accessStatus: 'grant', + authorID: (sessionAuthorID != null) ? sessionAuthorID : await p_tokenAuthorID, + }; + + if (!padID.includes('$')) { + // Only group pads can be private or have passwords, so there is nothing more to check for this + // non-group pad. + return grant; + } + + if (!padExists) { + if (sessionAuthorID == null) { + authLogger.debug('access denied: must have an HTTP API session to create a group pad'); return DENY; } - } else { - // a session is not required, so we'll check if it's a public pad - if (padID.indexOf("$") === -1) { - // it's not a group pad, means we can grant access - if (settings.editOnly && !(await p_padExists)) return DENY; - return {accessStatus: 'grant', authorID: await p_tokenAuthor}; - } + // Creating a group pad, so there is no password or public status to check. + return grant; } - let validSession = false; - let sessionAuthor; - let isPublic; - let isPasswordProtected; - let passwordStatus = password == null ? "notGiven" : "wrong"; // notGiven, correct, wrong + const pad = await padManager.getPad(padID); - // get information about all sessions contained in this cookie - if (sessionCookie) { - let groupID = padID.split("$")[0]; - - /* - * Sometimes, RFC 6265-compliant web servers may send back a cookie whose - * value is enclosed in double quotes, such as: - * - * Set-Cookie: sessionCookie="s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"; Version=1; Path=/; Domain=localhost; Discard - * - * Where the double quotes at the start and the end of the header value are - * just delimiters. This is perfectly legal: Etherpad parsing logic should - * cope with that, and remove the quotes early in the request phase. - * - * Somehow, this does not happen, and in such cases the actual value that - * sessionCookie ends up having is: - * - * sessionCookie = '"s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"' - * - * As quick measure, let's strip the double quotes (when present). - * Note that here we are being minimal, limiting ourselves to just removing - * quotes at the start and the end of the string. - * - * Fixes #3819. - * Also, see #3820. - */ - let sessionIDs = sessionCookie.replace(/^"|"$/g, '').split(','); - - // was previously iterated in parallel using async.forEach - try { - let sessionInfos = await Promise.all(sessionIDs.map(sessionID => { - return sessionManager.getSessionInfo(sessionID); - })); - - // seperated out the iteration of sessioninfos from the (parallel) fetches from the DB - for (let sessionInfo of sessionInfos) { - // is it for this group? - if (sessionInfo.groupID != groupID) { - authLogger.debug("Auth failed: wrong group"); - continue; - } - - // is validUntil still ok? - let now = Math.floor(Date.now() / 1000); - if (sessionInfo.validUntil <= now) { - authLogger.debug("Auth failed: validUntil"); - continue; - } - - // fall-through - there is a valid session - validSession = true; - sessionAuthor = sessionInfo.authorID; - break; - } - } catch (err) { - // skip session if it doesn't exist - if (err.message == "sessionID does not exist") { - authLogger.debug("Auth failed: unknown session"); - } else { - throw err; - } - } + if (!pad.getPublicStatus() && sessionAuthorID == null) { + authLogger.debug('access denied: must have an HTTP API session to access private group pads'); + return DENY; } - let padExists = await p_padExists; - - if (padExists) { - let pad = await padManager.getPad(padID); - - // is it a public pad? - isPublic = pad.getPublicStatus(); - - // is it password protected? - isPasswordProtected = pad.isPasswordProtected(); - - // is password correct? - if (isPasswordProtected && password && pad.isCorrectPassword(password)) { - passwordStatus = "correct"; - } - } - - // - a valid session for this group is avaible AND pad exists - if (validSession && padExists) { - let authorID = sessionAuthor; - let grant = Object.freeze({ accessStatus: "grant", authorID }); - - if (!isPasswordProtected) { - // - the pad is not password protected - - // --> grant access - return grant; - } - - if (settings.sessionNoPassword) { - // - the setting to bypass password validation is set - - // --> grant access - return grant; - } - - if (isPasswordProtected && passwordStatus === "correct") { - // - the pad is password protected and password is correct - - // --> grant access - return grant; - } - - if (isPasswordProtected && passwordStatus === "wrong") { - // - the pad is password protected but wrong password given - - // --> deny access, ask for new password and tell them that the password is wrong - return WRONG_PASSWORD; - } - - if (isPasswordProtected && passwordStatus === "notGiven") { - // - the pad is password protected but no password given - - // --> ask for password + const passwordExempt = settings.sessionNoPassword && sesionAuthorID != null; + const requirePassword = pad.isPasswordProtected() && !passwordExempt; + if (requirePassword) { + if (password == null) { + authLogger.debug('access denied: password required'); return NEED_PASSWORD; } - - throw new Error("Oops, something wrong happend"); - } - - if (validSession && !padExists) { - // - a valid session for this group avaible but pad doesn't exist - - // --> grant access by default - let accessStatus = "grant"; - let authorID = sessionAuthor; - - // --> deny access if user isn't allowed to create the pad - if (settings.editOnly) { - authLogger.debug("Auth failed: valid session & pad does not exist"); - return DENY; - } - - return { accessStatus, authorID }; - } - - if (!validSession && padExists) { - // there is no valid session avaiable AND pad exists - - let authorID = await p_tokenAuthor; - let grant = Object.freeze({ accessStatus: "grant", authorID }); - - if (isPublic && !isPasswordProtected) { - // -- it's public and not password protected - - // --> grant access, with author of token - return grant; - } - - if (isPublic && isPasswordProtected && passwordStatus === "correct") { - // - it's public and password protected and password is correct - - // --> grant access, with author of token - return grant; - } - - if (isPublic && isPasswordProtected && passwordStatus === "wrong") { - // - it's public and the pad is password protected but wrong password given - - // --> deny access, ask for new password and tell them that the password is wrong + if (!password || !pad.isCorrectPassword(password)) { + authLogger.debug('access denied: wrong password'); return WRONG_PASSWORD; } - - if (isPublic && isPasswordProtected && passwordStatus === "notGiven") { - // - it's public and the pad is password protected but no password given - - // --> ask for password - return NEED_PASSWORD; - } - - if (!isPublic) { - // - it's not public - - authLogger.debug("Auth failed: invalid session & pad is not public"); - // --> deny access - return DENY; - } - - throw new Error("Oops, something wrong happend"); } - // there is no valid session avaiable AND pad doesn't exist - authLogger.debug("Auth failed: invalid session & pad does not exist"); - return DENY; -} + return grant; +}; diff --git a/src/node/db/SessionManager.js b/src/node/db/SessionManager.js index 9161205d7..5f7df1e24 100644 --- a/src/node/db/SessionManager.js +++ b/src/node/db/SessionManager.js @@ -19,11 +19,65 @@ */ var customError = require("../utils/customError"); +const promises = require('../utils/promises'); var randomString = require("../utils/randomstring"); var db = require("./DB"); var groupManager = require("./GroupManager"); var authorManager = require("./AuthorManager"); +/** + * Finds the author ID for a session with matching ID and group. + * + * @param groupID identifies the group the session is bound to. + * @param sessionCookie contains a comma-separated list of IDs identifying the sessions to search. + * @return If there is a session that is not expired, has an ID matching one of the session IDs in + * sessionCookie, and is bound to a group with the given ID, then this returns the author ID + * bound to the session. Otherwise, returns undefined. + */ +exports.findAuthorID = async (groupID, sessionCookie) => { + if (!sessionCookie) return undefined; + /* + * Sometimes, RFC 6265-compliant web servers may send back a cookie whose + * value is enclosed in double quotes, such as: + * + * Set-Cookie: sessionCookie="s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"; Version=1; Path=/; Domain=localhost; Discard + * + * Where the double quotes at the start and the end of the header value are + * just delimiters. This is perfectly legal: Etherpad parsing logic should + * cope with that, and remove the quotes early in the request phase. + * + * Somehow, this does not happen, and in such cases the actual value that + * sessionCookie ends up having is: + * + * sessionCookie = '"s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"' + * + * As quick measure, let's strip the double quotes (when present). + * Note that here we are being minimal, limiting ourselves to just removing + * quotes at the start and the end of the string. + * + * Fixes #3819. + * Also, see #3820. + */ + const sessionIDs = sessionCookie.replace(/^"|"$/g, '').split(','); + const sessionInfoPromises = sessionIDs.map(async (id) => { + try { + return await exports.getSessionInfo(id); + } catch (err) { + if (err.message === 'sessionID does not exist') { + console.debug(`SessionManager getAuthorID: no session exists with ID ${id}`); + } else { + throw err; + } + } + return undefined; + }); + const now = Math.floor(Date.now() / 1000); + const isMatch = (si) => (si != null && si.groupID === groupID && si.validUntil <= now); + const sessionInfo = await promises.firstSatisfies(sessionInfoPromises, isMatch); + if (sessionInfo == null) return undefined; + return sessionInfo.authorID; +}; + exports.doesSessionExist = async function(sessionID) { //check if the database entry of this session exists diff --git a/src/node/utils/promises.js b/src/node/utils/promises.js index 4fb2e8318..a754823a0 100644 --- a/src/node/utils/promises.js +++ b/src/node/utils/promises.js @@ -2,7 +2,40 @@ * Helpers to manipulate promises (like async but for promises). */ -var timesLimit = function (ltMax, concurrency, promiseCreator) { +// Returns a Promise that resolves to the first resolved value from `promises` that satisfies +// `predicate`. Resolves to `undefined` if none of the Promises satisfy `predicate`, or if +// `promises` is empty. If `predicate` is nullish, the truthiness of the resolved value is used as +// the predicate. +exports.firstSatisfies = (promises, predicate) => { + if (predicate == null) predicate = (x) => x; + + // Transform each original Promise into a Promise that never resolves if the original resolved + // value does not satisfy `predicate`. These transformed Promises will be passed to Promise.race, + // yielding the first resolved value that satisfies `predicate`. + const newPromises = promises.map( + (p) => new Promise((resolve, reject) => p.then((v) => predicate(v) && resolve(v), reject))); + + // If `promises` is an empty array or if none of them resolve to a value that satisfies + // `predicate`, then `Promise.race(newPromises)` will never resolve. To handle that, add another + // Promise that resolves to `undefined` after all of the original Promises resolve. + // + // Note: If all of the original Promises simultaneously resolve to a value that satisfies + // `predicate` (perhaps they were already resolved when this function was called), then this + // Promise will resolve too, and with a value of `undefined`. There is no concern that this + // Promise will win the race and thus cause an erroneous `undefined` result. This is because + // a resolved Promise's `.then()` function is scheduled for execution -- not executed right away + // -- and ES guarantees in-order execution of the enqueued invocations. Each of the above + // transformed Promises has a `.then()` chain of length one, while the Promise added here has a + // `.then()` chain of length two or more (at least one `.then()` that is internal to + // `Promise.all()`, plus the `.then()` function added here). By the time the `.then()` function + // added here executes, all of the above transformed Promises will have already resolved and one + // will have been chosen as the winner. + newPromises.push(Promise.all(promises).then(() => {})); + + return Promise.race(newPromises); +}; + +exports.timesLimit = function(ltMax, concurrency, promiseCreator) { var done = 0 var current = 0 @@ -26,7 +59,3 @@ var timesLimit = function (ltMax, concurrency, promiseCreator) { addAnother() } } - -module.exports = { - timesLimit: timesLimit -} From 4434e54368d8ba4be23d665caa09c216ea16d9ea Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 12 Sep 2020 11:00:05 +0100 Subject: [PATCH 10/16] Update responsiveness.js Changing allowed delay from 300 to 400 because Safari OSX is consistently slow compared to every other modern browser. --- tests/frontend/specs/responsiveness.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/frontend/specs/responsiveness.js b/tests/frontend/specs/responsiveness.js index 053ade217..e6552642f 100644 --- a/tests/frontend/specs/responsiveness.js +++ b/tests/frontend/specs/responsiveness.js @@ -77,7 +77,7 @@ describe('Responsiveness of Editor', function() { var end = Date.now(); // get the current time var delay = end - start; // get the delay as the current time minus the start time - expect(delay).to.be.below(300); + expect(delay).to.be.below(400); done(); }, 1000); From d0a16d23cbcbf4b835467fbc6eff0be50948971d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 11 Sep 2020 15:07:33 -0400 Subject: [PATCH 11/16] security: Fix authentication bypass vulnerability Before, anyone who could create a socket.io connection to Etherpad could read, modify, and create pads at will without authenticating first. The `checkAccess` middleware in `webaccess.js` normally handles authentication and authorization, but it does not run for `/socket.io` requests. This means that the connection handler in `socketio.js` must handle authentication and authorization. However, before this change: * The handler did not require a signed `express_sid` cookie. * After loading the express-session state, the handler did not check to see if the user had authenticated. Now the handler requires a signed `express_sid` cookie, and it ensures that `socket.request.session.user` is non-null if authentication is required. (`socket.request.session.user` is non-null if and only if the user has authenticated.) --- src/node/hooks/express/socketio.js | 57 +++++++++++++----------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index 03fa7bbe6..3eef7a92b 100644 --- a/src/node/hooks/express/socketio.js +++ b/src/node/hooks/express/socketio.js @@ -39,43 +39,36 @@ exports.expressCreateServer = function (hook_name, args, cb) { cookie: false, }); - /* Require an express session cookie to be present, and load the - * session. See http://www.danielbaulig.de/socket-ioexpress for more - * info */ + // 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). var cookieParserFn = cookieParser(webaccess.secret, {}); - io.use(function(socket, accept) { var data = socket.request; - // Use a setting if we want to allow load Testing - - // Sometimes browsers might not have cookies at all, for example Safari in iFrames Cross domain - // https://github.com/ether/etherpad-lite/issues/4031 - // if requireSession is false we can allow them to still get on the pad. - // Note that this does make security less tight because any socketIO connection can be established without - // any logic on the client to do any handshaking.. I am not concerned about this though, the real solution - // here is to implement rateLimiting on SocketIO ACCEPT_COMMIT messages. - - if(!data.headers.cookie && (settings.loadTest || !settings.requireSession)){ - accept(null, true); - }else{ - if (!data.headers.cookie) return accept('No session cookie transmitted.', false); - } - if(data.headers.cookie){ - cookieParserFn(data, {}, function(err){ - if(err) { - console.error(err); - accept("Couldn't parse request cookies. ", false); - return; + if (!data.headers.cookie && settings.loadTest) return accept(null, true); + cookieParserFn(data, {}, function(err) { + if (err) { + console.error(err); + accept("Couldn't parse request cookies.", false); + return; + } + data.sessionID = data.signedCookies.express_sid; + if (!data.sessionID) return accept('Signed express_sid cookie is required', false); + args.app.sessionStore.get(data.sessionID, function(err, session) { + if (err || !session) return accept('Bad session / session has expired', false); + data.session = new sessionModule.Session(data, session); + if (settings.requireAuthentication && data.session.user == null) { + return accept('Authentication required', false); } - - data.sessionID = data.signedCookies.express_sid; - args.app.sessionStore.get(data.sessionID, function (err, session) { - if (err || !session) return accept('Bad session / session has expired', false); - data.session = new sessionModule.Session(data, session); - accept(null, true); - }); + accept(null, true); }); - } + }); }); // var socketIOLogger = log4js.getLogger("socket.io"); From ec6b983917a21912afca2305c174dea92b143ee7 Mon Sep 17 00:00:00 2001 From: webzwo0i Date: Sun, 13 Sep 2020 20:01:28 +0200 Subject: [PATCH 12/16] packaging: remove pad_docbar.js (#4286) package to reduce http requests: nice-select, pad_automatic_reconnect, skin_variants, scroll, caretPosition rename unorm in tar.json so it can be included --- src/node/utils/tar.json | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/node/utils/tar.json b/src/node/utils/tar.json index 1c1102f46..80d645dd0 100644 --- a/src/node/utils/tar.json +++ b/src/node/utils/tar.json @@ -6,8 +6,9 @@ , "pad_cookie.js" , "pad_editor.js" , "pad_editbar.js" - , "pad_docbar.js" + , "vendors/nice-select.js" , "pad_modals.js" + , "pad_automatic_reconnect.js" , "ace.js" , "collab_client.js" , "pad_userlist.js" @@ -19,6 +20,7 @@ , "$tinycon/tinycon.js" , "excanvas.js" , "farbtastic.js" + , "skin_variants.js" ] , "timeslider.js": [ "timeslider.js" @@ -29,8 +31,9 @@ , "pad_cookie.js" , "pad_editor.js" , "pad_editbar.js" - , "pad_docbar.js" + , "vendors/nice-select.js" , "pad_modals.js" + , "pad_automatic_reconnect.js" , "pad_savedrevs.js" , "pad_impexp.js" , "AttributePool.js" @@ -52,12 +55,14 @@ , "cssmanager.js" , "colorutils.js" , "undomodule.js" - , "$unorm.js" + , "$unorm/lib/unorm.js" , "contentcollector.js" , "changesettracker.js" , "linestylefilter.js" , "domline.js" , "AttributeManager.js" + , "scroll.js" + , "caretPosition.js" ] , "ace2_common.js": [ "ace2_common.js" From 0a836ced2911d9df95cf30fff9919aed08c3e608 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Tue, 15 Sep 2020 00:35:59 -0400 Subject: [PATCH 13/16] css: Line up line numbers with their rows Tested with both `no-skin` and `colibris`. --- src/static/css/iframe_editor.css | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/static/css/iframe_editor.css b/src/static/css/iframe_editor.css index a68ca3527..7267375a4 100644 --- a/src/static/css/iframe_editor.css +++ b/src/static/css/iframe_editor.css @@ -25,7 +25,8 @@ html.inner-editor { /* ACE-PAD Container (i.e. where the text is displayed) */ #innerdocbody { - padding: 15px; + padding-left: 15px; + padding-right: 15px; overflow: hidden; background-color: white; line-height: 1.6; @@ -39,6 +40,13 @@ html.inner-editor { overflow-wrap: break-word; } +#innerdocbody, #sidediv { + /* Both must have the same top padding to line up line numbers */ + padding-top: 15px; + /* Some space when we scroll to the bottom */ + padding-bottom: 15px; +} + #innerdocbody a { color: #2e96f3; } @@ -144,4 +152,4 @@ body.mozilla, body.safari { } body.grayedout { background-color: #eee !important -} \ No newline at end of file +} From 259b8d891d866bf1b6ddfcd6a174a31574ca3924 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 14 Sep 2020 00:49:16 -0400 Subject: [PATCH 14/16] socketio: Use Error objects for socket.io connection errors socket.io expects Error objects, otherwise it won't propagate the message to the client. Also do some cleanup. --- src/node/hooks/express/socketio.js | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index 3eef7a92b..b1a4d4f22 100644 --- a/src/node/hooks/express/socketio.js +++ b/src/node/hooks/express/socketio.js @@ -49,24 +49,24 @@ exports.expressCreateServer = function (hook_name, args, cb) { // 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(function(socket, accept) { + io.use((socket, next) => { var data = socket.request; - if (!data.headers.cookie && settings.loadTest) return accept(null, true); + if (!data.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) { - console.error(err); - accept("Couldn't parse request cookies.", false); - return; - } - data.sessionID = data.signedCookies.express_sid; - if (!data.sessionID) return accept('Signed express_sid cookie is required', false); - args.app.sessionStore.get(data.sessionID, function(err, session) { - if (err || !session) return accept('Bad session / session has expired', false); + 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 accept('Authentication required', false); + return fail('access denied: authentication required'); } - accept(null, true); + next(null, true); }); }); }); From f9087fabd684cd555883ef99cb207219fd34a681 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 11 Sep 2020 17:12:29 -0400 Subject: [PATCH 15/16] security: Check authentication in SecurityManager checkAccess In addition to providing defense in depth, this change makes it easier to implement future enhancements such as support for read-only users. --- src/node/db/SecurityManager.js | 10 +++++++++- src/node/handler/PadMessageHandler.js | 7 +++++-- src/node/handler/SocketIORouter.js | 4 +++- src/node/hooks/express/importexport.js | 3 ++- src/node/padaccess.js | 4 +++- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index fd381f3a8..f4c7af880 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -42,6 +42,7 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'}); * with this token then a new author object is created (including generating an author ID) and * associated with this token. * @param password is the password the user has given to access this pad. It can be null. + * @param userSettings is the settings.users[username] object (or equivalent from an authn plugin). * @return {accessStatus: grant|deny|wrongPassword|needPassword, authorID: a.xxxxxx}. The caller * must use the author ID returned in this object when making any changes associated with the * author. @@ -49,13 +50,20 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'}); * WARNING: Tokens and session IDs MUST be kept secret, otherwise users will be able to impersonate * each other (which might allow them to gain privileges). */ -exports.checkAccess = async function(padID, sessionCookie, token, password) +exports.checkAccess = async function(padID, sessionCookie, token, password, userSettings) { if (!padID) { authLogger.debug('access denied: missing padID'); return DENY; } + // 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. + if (settings.requireAuthentication && userSettings == null) { + authLogger.debug('access denied: authentication is required'); + return DENY; + } + // allow plugins to deny access const isFalse = (x) => x === false; if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) { diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 77f6d39ce..624db6829 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -289,7 +289,9 @@ exports.handleMessage = async function(client, message) padId = await readOnlyManager.getPadId(padId); } - let { accessStatus } = await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password); + const {session: {user} = {}} = client.client.request; + const {accessStatus} = + await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password, user); if (accessStatus !== "grant") { // no access, send the client a message that tells him why @@ -896,8 +898,9 @@ async function handleClientReady(client, message) let padIds = await readOnlyManager.getIds(message.padId); // FIXME: Allow to override readwrite access with readonly + const {session: {user} = {}} = client.client.request; const {accessStatus, authorID} = await securityManager.checkAccess( - padIds.padId, message.sessionID, message.token, message.password); + padIds.padId, message.sessionID, message.token, message.password, user); // no access, send the client a message that tells him why if (accessStatus !== "grant") { diff --git a/src/node/handler/SocketIORouter.js b/src/node/handler/SocketIORouter.js index 077a62beb..a5220d2f4 100644 --- a/src/node/handler/SocketIORouter.js +++ b/src/node/handler/SocketIORouter.js @@ -97,7 +97,9 @@ exports.setSocketIO = function(_socket) { padId = await readOnlyManager.getPadId(message.padId); } - let { accessStatus } = await securityManager.checkAccess(padId, message.sessionID, message.token, message.password); + const {session: {user} = {}} = client.client.request; + const {accessStatus} = await securityManager.checkAccess( + padId, message.sessionID, message.token, message.password, user); if (accessStatus === "grant") { // access was granted, mark the client as authorized and handle the message diff --git a/src/node/hooks/express/importexport.js b/src/node/hooks/express/importexport.js index 85ab6e129..4aa06ecb8 100644 --- a/src/node/hooks/express/importexport.js +++ b/src/node/hooks/express/importexport.js @@ -58,8 +58,9 @@ exports.expressCreateServer = function (hook_name, args, cb) { return next(); } + const {session: {user} = {}} = req; const {accessStatus, authorID} = await securityManager.checkAccess( - req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password); + req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user); if (accessStatus !== 'grant') return res.status(403).send('Forbidden'); assert(authorID); diff --git a/src/node/padaccess.js b/src/node/padaccess.js index 3449f7d16..6e294403e 100644 --- a/src/node/padaccess.js +++ b/src/node/padaccess.js @@ -3,7 +3,9 @@ var securityManager = require('./db/SecurityManager'); // checks for padAccess module.exports = async function (req, res) { try { - let accessObj = await securityManager.checkAccess(req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password); + const {session: {user} = {}} = req; + const accessObj = await securityManager.checkAccess( + req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user); if (accessObj.accessStatus === "grant") { // there is access, continue From 2bc26b8ef89ea0a5b447ad12586b694c6a830d4e Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 12 Sep 2020 17:23:48 -0400 Subject: [PATCH 16/16] webaccess: Factor out common code --- src/node/hooks/express/webaccess.js | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 761a46ab8..e8caab566 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -17,18 +17,21 @@ exports.checkAccess = (req, res, next) => { // This may be called twice per access: once before authentication is checked and once after (if // settings.requireAuthorization is true). - const authorize = (cb) => { + const authorize = (fail) => { // Do not require auth for static paths and the API...this could be a bit brittle - if (req.path.match(/^\/(static|javascripts|pluginfw|api)/)) return cb(true); + if (req.path.match(/^\/(static|javascripts|pluginfw|api)/)) return next(); if (req.path.toLowerCase().indexOf('/admin') !== 0) { - if (!settings.requireAuthentication) return cb(true); - if (!settings.requireAuthorization && req.session && req.session.user) return cb(true); + if (!settings.requireAuthentication) return next(); + if (!settings.requireAuthorization && req.session && req.session.user) return next(); } - if (req.session && req.session.user && req.session.user.is_admin) return cb(true); + if (req.session && req.session.user && req.session.user.is_admin) return next(); - hooks.aCallFirst('authorize', {req, res, next, resource: req.path}, hookResultMangle(cb)); + hooks.aCallFirst('authorize', {req, res, next, resource: req.path}, hookResultMangle((ok) => { + if (ok) return next(); + return fail(); + )); }; /* Authentication OR authorization failed. */ @@ -59,12 +62,7 @@ exports.checkAccess = (req, res, next) => { let step1PreAuthenticate, step2Authenticate, step3Authorize; - step1PreAuthenticate = () => { - authorize((ok) => { - if (ok) return next(); - step2Authenticate(); - }); - }; + step1PreAuthenticate = () => authorize(step2Authenticate); step2Authenticate = () => { const ctx = {req, res, next}; @@ -98,12 +96,7 @@ exports.checkAccess = (req, res, next) => { })); }; - step3Authorize = () => { - authorize((ok) => { - if (ok) return next(); - failure(); - }); - }; + step3Authorize = () => authorize(failure); step1PreAuthenticate(); };