prevent outside modifications to the thread Y Map

[YousefED]

BlockNote-demo-nextjs-hocuspocus/hocuspocus-server/src/index.ts

Line 32 in a55cd0e

// TODO: for good security, you'd want to make sure that either:

[nperez0111]

Two approaches we discussed with Kevin:

The server can reject the change (by inspecting what is inside of the update), by not applying it to the document, but we would need to implement something on the protocol level to say that the change failed to apply and give the client a way to move from this invalid state (because further changes to the document from this point would all also be invalid).

Or we can "rollback" the change on the server making the user's change effectively moot, by running the inverse of whatever change the client made. We should take care to apply both the invalid change, and it's inverse within a single "transaction" so that no other clients can see this invalid state.

Kevin expressed preference towards the first option, though we could not figure out a better idea to move a client from an invalid state than "reload the window". We'll want to put some thought towards this to see what we actually expect in this sort of a scenario.

[YousefED]

@dmonad could you share here what's a good way to inspect how we see which map an update is affecting? Then we can detect the "invalid" update and explore either the two approaches nick suggested.

PS: if you're interested, this is the POC code for a comments REST server: https://github.com/TypeCellOS/BlockNote-demo-nextjs-hocuspocus/blob/a55cd0e4ca635d812bd3b119c856215065c2e1cb/hocuspocus-server/src/threads.ts

[dmonad]

Yjs does not support type-level permissions. Permissions can only be applied on the global document. Rejecting changes based on the observe event opens up several opportunities for exploits and bugs. I suggest implementing a REST API if you need control over certain actions. The server then can manipulate the Yjs document for the client.

But if you want to roll-back changes anyway, this is how I would do it:

const ydoc = new Y.Doc()
const restrictedType = ydoc.getArray('restricted')

/**
* Assume this function handles incoming updates via a communication channel like websockets.
* Changes to the `ydoc.getMap('restricted')` type should be rejected.
*
* - set up undo manager on the restricted types
* - cache pending* updates from the Ydoc to avoid certain attacks
* - apply received update and check whether the restricted type (or any of its children) has been changed.
* - catch errors that might try to circumvent the restrictions
* - undo changes on restricted types
* - reapply pending* updates
*
* @param {Uint8Array} update
*/
const updateHandler = (update) => {
// don't handle changes of the local undo manager, which is used to undo invalid changes
const um = new Y.UndoManager(restrictedType, { trackedOrigins: new Set(['remote change']) })
const beforePendingDs = ydoc.store.pendingDs
const beforePendingStructs = ydoc.store.pendingStructs?.update
try {
  Y.applyUpdate(ydoc, update, 'remote change')
} finally {
  while (um.undoStack.length) { um.undo() }
  um.destroy()
  ydoc.store.pendingDs = beforePendingDs
  ydoc.store.pendingStructs = null
  if (beforePendingStructs) {
    Y.applyUpdateV2(ydoc, beforePendingStructs)
  }
}
}

// whenever you receive updates from a remote peer:
updateHandler(update)

[YousefED]

Thanks! Is there also an easy way to detect beforehand if the update affects restrictedType? (so that we don't need to instantiate this code (UndoManager) etc if it's not necessary?

Asking because if we were to implement a workaround like this, I think it's best to keep it as isolated as possible

[dmonad]

The UndoManager has a very small footprint. This is probably as good as it gets. The advantage is that it properly reverts changes using existing technology.

We could inspect the update directly, read the content using the binary decoder, and check whether any operation affects restrictedType. But that's fairly complex code to write and to maintain. Furthermore, it would be as "expensive" (in terms of big O) as the above solution.

[YousefED]

The UndoManager has a very small footprint. This is probably as good as it gets. The advantage is that it properly reverts changes using existing technology.

We could inspect the update directly, read the content using the binary decoder, and check whether any operation affects restrictedType. But that's fairly complex code to write and to maintain. Furthermore, it would be as "expensive" (in terms of big O) as the above solution.

Ok, thanks!

[nperez0111]

Just to be clear, if a client made a change to this restrictedType they would be in the "broken" state that we talked about where their next updates would not apply too correctly?

I'm curious on how (if possible) we could do the inverse of this (i.e. instead of protecting a single restrictedType we could only allow updates to a allowedType). Because with this solution, we'd have to check all top-level types to ensure that the client who only has comment permissions only modifies the comment map, right?

[YousefED]

I'm curious on how (if possible) we could do the inverse of this (i.e. instead of protecting a single restrictedType we could only allow updates to a allowedType). Because with this solution, we'd have to check all top-level types to ensure that the client who only has comment permissions only modifies the comment map, right?

We need to distinguish two cases:

RestYjsThreadStore
All comment operations should be done from the backend (and the client requests the backend to do this via an API call). That means ANY outside operations to the threads map are invalid. For this, I think Kevin's solution should work, right?

YjsThreadStore
This is a more advanced scenario I think you're talking about. But this would not only require checking whether an update has been made to a specific type, but also to check certain business (auth) rules (e.g.: a user can only edit his/her own comments). One of this auth-rules is what you mentioned (comment only-users should not be able to make changes to other fields in the Y.Doc).

[nperez0111]

We need to distinguish two cases

Good to explicitly state, I was just curious if there was a "cheap" way to do the YjsThreadStore.

For this, I think Kevin's solution should work, right?

Yep, I think it would, too.

[dmonad]

Just to be clear, if a client made a change to this restrictedType they would be in the "broken" state that we talked about where their next updates would not apply too correctly?

The above solution uses the undo manager to revert the change. So the document wouldn't be in a broken state.

YjsThreadStore
This is a more advanced scenario I think you're talking about. But this would not only require checking whether an update has been made to a specific type, but also to check certain business (auth) rules (e.g.: a user can only edit his/her own comments). One of this auth-rules is what you mentioned (comment only-users should not be able to make changes to other fields in the Y.Doc).

In this case, I suggest that the undo manager still observes the "comments" type or "restricted" type. Then you register an observer on the type and only trigger um.undo() if an invalid change happened.

Something like..

let revertChanges = false
comments.observeDeep(events => {
    revertChanges = events.some(event => !isAllowedChange(event))
})

Y.applyUpdate(ydoc, update)

comments.unobserveDeep(..)
if (revertChanges) {
  um.undo()
}

[nperez0111]

I'm unsure how helpful this is, but I felt like it was valuable to diagram how the system should work, in the interest of aligning expectations.

REST Proposal for Commenting

This is an overview of how the REST architecture could work for a commenting system based on Y.js

This approach uses different permissions for comments that are separate from the document content. It side-steps issues with comments by only granting read access to those who should have access to the comments. To make updates to the comments document, the only trusted entity to do so is the server, which can check authentication(AuthN) and authorization(AuthZ) with an external auth system.

Benefits

• By splitting out comments to a separate data structure with a different permissions system, we can safely assume that the content in the comments document is valid & untampered with. Saving us from possible issues from malicious actors like impersonation or otherwise tampering with the document
• Using a Y.js document to synchronize changes keeps the system "real-time" and re-uses existing infrastructure & services

Downsides

• In the current BlockNote implementation, comments are stored into the document as marks, which means that a user with a role of commenter actually needs some permissions to be able to create a comment.
  • There are three possible resolutions for this. 1) not storing comments as marks, and exploring using some way to reference to content in the document from an external structure (like using relative positions). 2) disallow commenters from creating comments at a specific point in the document, maybe they can comment on a specific block ID only. 3) implement a system that allows commenters to add marks via a REST endpoint and allow the document to make the modification to the document
• This requires the server to be able to inspect the contents of a document in clear text, which is contrary to an end-to-end encrypted (E2EE) system

Future E2EE System for Comments

In the interest of thinking of things ahead of time, it is probably valuable to see how an E2EE system for comments might work.

In an E2EE system, each user would have 1 or more public keys which are securely shared to other users within the system. Their device would maintain the private key, which they can use to sign messages they make. For the moment, we will gloss over the implementation of a E2E for Y.js and focus on a possible implementation for commenting.

The main idea of how comments might work in a system like this is the fact that each operation a user wants to make for comments would be signed by their private keys, and other users in the system would validate the comments to make certain that they are authentic to the associated public key.

For a system like this, it is safe to give write access to the comments document, because each message much be signed and can be independently validated for authenticity.

Benefits

• Can be considered E2EE, independent verification of comment authenticity, no trust in server operators

Downsides

• Using Y.js is probably the wrong choice for a system like this, it would need a separate mechanism for distributing these comments, likely built on the same mechanism as what would distribute Y.js updates in an E2EE version of a provider

Conclusion

Unfortunately, there isn't a clear way to build towards the future for this system as the main mechanism for validating authenticity is not present in the current system. Theoretically, for a migration between the two systems, it should be relatively straight-forward to re-sign existing comments (either by users or more likely by the docs server as a migration step).

[YousefED]

Thanks for the write-up! My remarks will be about the first part as I think indeed E2EE is out of scope for now.

• 3. implement a system that allows commenters to add marks via a REST endpoint and allow the document to make the modification to the document

fyi, this has already been implemented here: server, client

Separate doc vs same doc for comments:

In your diagram, you describe a separate Y.Doc for Comments. We didn't discuss the se

Let's explore the pros / cons of using a separate Y.Doc vs storing the Comment data inside the same Y.Doc as the document (as a Y.Map):

Separate Doc

• + matches the Yjs model nicely (authZ scoped per document)
• - requires more orchestration

Same Doc

• + easy to set up
• +- requires the UndoManager solution above to prevent unauthorized writes to the Comment data
• - "View"-only users that connect to the document via Yjs will always be able to read Comment data (no way to separate this)

Securing YjsThreadStore

Note that in your comment above, we didn't discuss YjsThreadStore. To reiterate this, we could also secure this according to this suggestion by @dmonad. My suggestion would be to leave that out of scope for now (possibly link this thread from the YjsThreadStore if users really want this)

Next steps

I'd propose to:

• in this repo, implement the UndoManager for the "same doc" solution (mostly to keep things simple)
• add the documentation from this thread (either move to separate .md or link from the main readme to this GH issue)
• don't implement the YjsThreadStore AuthZ for now