From 2eb4c8e05fccac13e5da3a9c6c6b5691646ae0be Mon Sep 17 00:00:00 2001
From: Julian Rother <julian@jrother.eu>
Date: Sun, 28 May 2023 11:55:34 +0200
Subject: [PATCH] Fix premature note cleanup on error

Connection forbidden errors cause cleanup of note state without first
checking if other clients are still connected to the note. This leads
to inconsistent pad content and changes not being saved properly.

This change reverts parts of 725e982 (Fix realtime on forbidden not clean
up properly ...). The call to `interruptConnection()` on permission errors
is redundant, since `failConnection()` and `disconnect()` already perform
all required cleanup in this case. The other call to `interruptConnection()`
only happens when a client (the first client for a note) disconnects while
the note is being loaded from the database. It is refactored for clarity.

Fixes #3894

Co-authored-by: David Mehren <git@herrmehren.de>
Signed-off-by: Julian Rother <julian@jrother.eu>
---
 lib/realtime.js              | 19 +++++++------------
 public/docs/release-notes.md |  3 +++
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/lib/realtime.js b/lib/realtime.js
index 1269e4116..66d606fa4 100644
--- a/lib/realtime.js
+++ b/lib/realtime.js
@@ -363,13 +363,6 @@ function connectNextSocket () {
   }, 1)
 }
 
-function interruptConnection (socket, noteId, socketId) {
-  if (notes[noteId]) delete notes[noteId]
-  if (users[socketId]) delete users[socketId]
-  if (socket) { clearSocketQueue(connectionSocketQueue, socket) } else { connectionSocketQueue.shift() }
-  connectNextSocket()
-}
-
 function checkViewPermission (req, note) {
   if (note.permission === 'private') {
     if (req.user && req.user.logged_in && req.user.id === note.owner) { return true } else { return false }
@@ -386,13 +379,8 @@ let isDisconnectBusy = false
 const disconnectSocketQueue = []
 
 function finishConnection (socket, noteId, socketId) {
-  // if no valid info provided will drop the client
-  if (!socket || !notes[noteId] || !users[socketId]) {
-    return interruptConnection(socket, noteId, socketId)
-  }
   // check view permission
   if (!checkViewPermission(socket.request, notes[noteId])) {
-    interruptConnection(socket, noteId, socketId)
     return failConnection(403, 'connection forbidden', socket)
   }
   const note = notes[noteId]
@@ -460,6 +448,13 @@ function startConnection (socket) {
       },
       include
     }).then(function (note) {
+      // if client disconnected while we waited for the note, disconnect() cleaned up users[socket.id]
+      if (!users[socket.id]) {
+        clearSocketQueue(connectionSocketQueue, socket)
+        connectNextSocket()
+        return
+      }
+
       if (!note) {
         return failConnection(404, 'note not found', socket)
       }
diff --git a/public/docs/release-notes.md b/public/docs/release-notes.md
index 2ec84b72c..34fccdd0c 100644
--- a/public/docs/release-notes.md
+++ b/public/docs/release-notes.md
@@ -10,6 +10,9 @@ You now need Node 16 to run HedgeDoc. We don't support more recent versions of N
 - Allow setting of `documentMaxLength` via `CMD_DOCUMENT_MAX_LENGTH` environment variable.
 - Add dedicated healthcheck endpoint at /_health that is less resource intensive than /status.
 
+### Bugfixes
+- Fix that permission errors can break existing connections to a note, causing inconsistent note content and changes not being saved
+
 ## <i class="fa fa-tag"></i> 1.9.7 <i class="fa fa-calendar-o"></i> 2023-02-19
 
 ### Bugfixes