feat: implements update rollback mechanism #3

nperez0111 · 2025-03-11T14:06:19Z

This resolves #1 by implementing a rollback mechanism for hocuspocus servers.

This will inspect a hocuspocus sync, which contains a ysync update, and check that there are no modifications to the threads map.
This will ensure that a malicious client cannot modify the threads map and it can only ever be manipulated through the REST server.

nperez0111 · 2025-03-11T14:07:30Z

hocuspocus-server/src/rejectUnauthorized.ts

+      // TODO, we can close their connection or just let them continue, since we've already undone their changes (and our changes are newer than theirs)
+      const error = {
+        reason: `Modification of a restricted type: ${this.threadsMapKey} was rejected`,
+      } satisfies Partial<CloseEvent>;
+      throw error;


We can close their connection or let them continue. Closing their connection does have the benefit that it forces them to re-sync so I went with that.

Since you are undoing, the client will receive a message that reverses their changes (Assuming Hocuspocus distributes all changes that were applied to a Y.Doc).

But it would be good to know somehow when this happens, because it shouldn't!

I'm sorry I don't fully understand what you mean by:

But it would be good to know somehow when this happens, because it shouldn't!

But, to address your first point: hocuspocus would distribute this change if it was modified. But, with this implementation, it bails out early and closes the connection with that client. The hocuspocus provider notices that the connection has been closed, and will automatically retry to open a new connection. I think that what happens here is that they attempt to sync again, and the document on the hocuspocus server already includes that "invalid" update and so they can continue to sync from there.

So, either way, the connection seems to continue, so maybe we should not kill the connection? Though, I don't really like just taking the update and not letting the user know that it was invalid. At least closing the connection makes it clear that the client did something invalid.

Happy to take opinions on this though

We can close their connection or let them continue.

I don't think we can let them continue because HocusPocus will apply the update again as per https://github.com/TypeCellOS/BlockNote-demo-nextjs-hocuspocus/pull/3/files#r1990990264

Update: I'm probably wrong here, because the second update will be a noop as it's already been applied (even though it has been "undone")

nperez0111 · 2025-03-11T14:08:00Z

next-app/components/Editor.tsx

+  useEffect(() => {
+    provider.document.on("update", (update) => {
+      console.log(provider.document.getMap("threads").toJSON());
+    });
+  }, [provider.document]);


This just shows the current client state in an easy to inspect way

nperez0111 · 2025-03-11T14:08:15Z

next-app/components/Editor.tsx

+        onClick={() => {
+          const comments = provider.document.getMap("threads");
+          comments.set("unauthorized-thread", {


simulating a malicious client

nperez0111 · 2025-03-11T14:09:06Z

hocuspocus-server/src/rejectUnauthorized.ts

+  private getYUpdate(message: Uint8Array) {
+    /**
+     * The messages we are interested in are of the following format:
+     * [docIdLength: number, ...docIdString: string, hocuspocusMessageType: number,  ySyncMessageType: number, ...yjsUpdate: Uint8Array]
+     *
+     * We check that the hocuspocusMessageType is Sync and that the ySyncMessageType is messageYjsUpdate.
+     */
+    const incomingMessage = new IncomingMessage(message);
+    // Read the docID string, but don't use it
+    incomingMessage.readVarString();
+
+    // Read the hocuspocusMessageType
+    const hocuspocusMessageType = incomingMessage.readVarUint();
+    // If the hocuspocusMessageType is not Sync, we don't handle the message, since it is not an update
+    if (hocuspocusMessageType !== MessageType.Sync) {
+      return;
+    }
+
+    // Read the ySyncMessageType
+    const ySyncMessageType = incomingMessage.readVarUint();
+
+    // If the ySyncMessageType is not messageYjsUpdate, we don't handle the message, since it is not an update
+    if (ySyncMessageType !== syncProtocol.messageYjsUpdate) {
+      // not an update
+      return;
+    }
+
+    // Read the yjsUpdate
+    const yUpdate = incomingMessage.readVarUint8Array();
+
+    return yUpdate;
+  }


This took forever to figure out 😮‍💨

YousefED

Nice you got this to work. Not sure yet whether this approach is feasible though:

see my comment in beforeHandleMessage; if we're applying updates twice that would be a problem
if we're also missing other messages (see comments about SyncReply and messageYjsSyncStep1) this approach will get quite error-prone

let's discuss

hocuspocus-server/src/rejectUnauthorized.ts

YousefED · 2025-03-12T08:21:13Z

hocuspocus-server/src/rejectUnauthorized.ts

+    // Read the hocuspocusMessageType
+    const hocuspocusMessageType = incomingMessage.readVarUint();
+    // If the hocuspocusMessageType is not Sync, we don't handle the message, since it is not an update
+    if (hocuspocusMessageType !== MessageType.Sync) {


do we also need SyncReply? https://github.com/ueberdosis/hocuspocus/blob/6fe902a7902e98b6fba3981179ae56b64ad6d4ac/packages/server/src/MessageReceiver.ts#L38

Maybe? I'm not familiar with what this bit is doing totally

Looks specific to the Redis integration, but doesn't hurt to add: https://github.com/ueberdosis/hocuspocus/blob/05f7980ef8840c20be9145805c466f14b4dd3939/packages/server/src/types.ts#L16

hocuspocus-server/src/rejectUnauthorized.ts

YousefED · 2025-03-12T08:24:57Z

hocuspocus-server/src/rejectUnauthorized.ts

+    update,
+    document: ydoc,
+  }: beforeHandleMessagePayload) {
+    const yUpdate = this.getYUpdate(update);


I see we process the update here (and in rollbackUpdateIfNeeded manually). Wouldn't we now apply the update twice? i.e.: the regular hocuspocus code will still process the message, right? so everything would be processed twice?

The only way that I could think of not applying the update twice would be to make a whole copy of the ydoc.

Applying updates twice should be fine, no? It is a CRDT 😄

It's fine to apply the update twice.

to be clear; this means all updates will be applied twice. Wouldn't that double the load?

hocuspocus-server/src/rejectUnauthorized.ts

YousefED · 2025-03-25T09:13:58Z

@nperez0111 wdyt of something like this? https://github.com/ueberdosis/hocuspocus/compare/main...YousefED:hocuspocus:feat/custom-messagereceiver?expand=1

Not sure it will land of course, but I think this should make it possible to customize the messagereceiver and wrap / overload the apply method with the checks we require?

nperez0111 · 2025-03-25T12:18:00Z

@nperez0111 wdyt of something like this? https://github.com/ueberdosis/hocuspocus/compare/main...YousefED:hocuspocus:feat/custom-messagereceiver?expand=1

Not sure it will land of course, but I think this should make it possible to customize the messagereceiver and wrap / overload the apply method with the checks we require?

Yea, I think that something like that would work, and probably worthwhile to try to get in.

YousefED · 2025-03-26T08:52:37Z

Nice. @nperez0111 do you want to try first whether it works before I (or you) fire up a PR?

feat: implements update rollback mechanism

74d777f

nperez0111 commented Mar 11, 2025

View reviewed changes

nperez0111 requested a review from YousefED March 11, 2025 14:10

YousefED reviewed Mar 12, 2025

View reviewed changes

dmonad reviewed Mar 12, 2025

View reviewed changes

hocuspocus-server/src/rejectUnauthorized.ts Outdated Show resolved Hide resolved

fix: include hocuspocus SyncReply and y-sync messageYjsSyncStep2

f4bf873

nperez0111 mentioned this pull request Mar 26, 2025

feat: add beforeSync hook ueberdosis/hocuspocus#919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implements update rollback mechanism #3

feat: implements update rollback mechanism #3

nperez0111 commented Mar 11, 2025

nperez0111 Mar 11, 2025

dmonad Mar 12, 2025

nperez0111 Mar 12, 2025

YousefED Mar 12, 2025 •

edited

Loading

nperez0111 Mar 11, 2025

nperez0111 Mar 11, 2025

nperez0111 Mar 11, 2025

YousefED left a comment

YousefED Mar 12, 2025

nperez0111 Mar 12, 2025

nperez0111 Mar 12, 2025

YousefED Mar 12, 2025

nperez0111 Mar 12, 2025

dmonad Mar 12, 2025

YousefED Mar 12, 2025

YousefED commented Mar 25, 2025

nperez0111 commented Mar 25, 2025

YousefED commented Mar 26, 2025

feat: implements update rollback mechanism #3

Are you sure you want to change the base?

feat: implements update rollback mechanism #3

Conversation

nperez0111 commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YousefED Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YousefED left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YousefED commented Mar 25, 2025

nperez0111 commented Mar 25, 2025

YousefED commented Mar 26, 2025

YousefED Mar 12, 2025 •

edited

Loading