mirrors/etherpad-lite
1
0
Fork 0
mirror of https://github.com/ether/etherpad-lite.git synced 2025-04-22 08:26:16 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 26

PadMessageHandler: Move handleMessage hooks after access check

Browse source 
Move the handleMessageSecurity and handleMessage hooks after the call
to securityManager.checkAccess.

Benefits:

  * A handleMessage plugin can safely assume the message will be
    handled unless the plugin itself drops the message, so it doesn't
    need to repeat the access checks done by the `handleMessage`
    function.
  * This paves the way for a future enhancement: pass the author ID to
    the hooks.

Note: The handleMessageSecurity hook is broken in several ways:

  * The hook result is ignored for `CLIENT_READY` and `SWITCH_TO_PAD`
    messages because the `handleClientReady` function overwrites the
    hook result. This causes the client to receive client vars with
    `readonly` set to true, which causes the client to display an
    immutable pad even though the pad is technically writable.
  * The formatting toolbar buttons are removed for read-only pads
    before the handleMessageSecurity hook even runs.
  * It is awkwardly named: Without reading the documentation, how is
    one supposed to know that "handle message security" actually means
    "grant one-time write access to a read-only pad"?
  * It is called for every message even though calls after a
    `CLIENT_READY` or `SWITCH_TO_PAD` are mostly pointless.
  * Why would anyone want to grant write access when the user visits a
    read-only pad URL? The user should just visit the writable pad URL
    instead.
  * Why would anyone want to grant write access that only lasts for a
    single socket.io connection?
  * There are better ways to temporarily grant write access (e.g., the
    authorize hook).
  * This hook is inviting bugs because it breaks a core assumption
    about `/p/r.*` URLs.

I think the hook should be deprecated and eventually removed.
This commit is contained in:
Richard Hansen 2020-09-10 13:27:59 -04:00 • committed by John McLear
parent 0f6baac7b5
commit 1bb44098df
2 changed files with 15 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

33
src/node/handler/PadMessageHandler.js
View file

@ -199,23 +199,6 @@ exports.handleMessage = async function(client, message)
return;
}
// Allow plugins to bypass the readonly message blocker
if ((await hooks.aCallAll('handleMessageSecurity', {client, message})).some((w) => w === true)) {
thisSession.readonly = false;
}
// Call handleMessage hook. If a plugin returns null, the message will be dropped. Note that for
// all messages handleMessage will be called, even if the client is not authorized
if ((await hooks.aCallAll('handleMessage', {client, message})).some((m) => m === null)) {
return;
}
// Drop the message if the client disconnected while the hooks were running.
if (sessioninfos[client.id] !== thisSession) {
messageLogger.warn("Dropping message from a connection that has gone away.")
return;
}
if (message.type === "CLIENT_READY") {
// client tried to auth for the first time (first msg from the client)
createSessionInfoAuth(thisSession, message);
@ -245,7 +228,21 @@ exports.handleMessage = async function(client, message)
return;
}
// access was granted
// Allow plugins to bypass the readonly message blocker
if ((await hooks.aCallAll('handleMessageSecurity', {client, message})).some((w) => w === true)) {
thisSession.readonly = false;
}
// Call handleMessage hook. If a plugin returns null, the message will be dropped.
if ((await hooks.aCallAll('handleMessage', {client, message})).some((m) => m === null)) {
return;
}
// Drop the message if the client disconnected during the above processing.
if (sessioninfos[client.id] !== thisSession) {
messageLogger.warn('Dropping message from a connection that has gone away.')
return;
}
// Check what type of message we get and delegate to the other methods
if (message.type === "CLIENT_READY") {