From 0fb486ea7c6d3a44efef36adc3a5d6f377a4ef4e Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:08:32 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F=20Only=20accept=20presenc?= =?UTF-8?q?e=20for=20the=20doc=20on=20its=20channel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/share/sharedb/issues/738 At the moment, the server checks read access against a presence's `c` and `d`, but broadcasts it on its `ch`: 1. Deny `docs/secret` in `readSnapshots` 2. Send presence on `docs.secret` with the `c`/`d` of a readable doc 3. `docs/secret` subscribers apply it This change rejects doc presence whose `c` and `d` don't match its channel, and `DocPresence` ignores presence for any other doc, which also covers untyped presence and dotted-name channel collisions. Forwarded `sendPresence` errors no longer reach a local presence with the same ID either: unlike replies to the client's own submissions, they carry no `pv`. Subscribing, write access and presence ID ownership are left to middleware, which the new "Access control" docs cover. The server check only holds once every instance is upgraded, and the client filter only protects upgraded clients. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- docs/api/backend.md | 11 ++ docs/api/presence.md | 2 + docs/middleware/actions.md | 6 + docs/presence.md | 73 ++++++++++ lib/agent.js | 12 ++ lib/client/presence/doc-presence.js | 6 + lib/client/presence/presence.js | 3 +- test/client/presence/doc-presence.js | 192 +++++++++++++++++++++++++-- test/client/presence/presence.js | 21 +++ 9 files changed, 317 insertions(+), 9 deletions(-) diff --git a/docs/api/backend.md b/docs/api/backend.md index b3efa8b04..f6657783b 100644 --- a/docs/api/backend.md +++ b/docs/api/backend.md @@ -113,6 +113,17 @@ Optional {: .d-inline-block } +`doNotForwardSendPresenceErrorsToClient` -- boolean + +Optional +{: .label .label-grey } + +> Default: `false` + +> If set to `true`, errors from the [`'sendPresence'`]({{ site.baseurl }}{% link middleware/actions.md %}#sendpresence) middleware will be passed to the `errorHandler` instead of being sent to the client. Sending these errors to the client is deprecated + +{: .d-inline-block } + `doNotCommitNoOps` -- boolean Optional diff --git a/docs/api/presence.md b/docs/api/presence.md index d8768a7a9..2cc136662 100644 --- a/docs/api/presence.md +++ b/docs/api/presence.md @@ -92,6 +92,8 @@ Optional {: .warn } > Depending on use-case, the same client may have **multiple** presences, so a user or client ID may not be appropriate to use as a presence ID. +> +> Presence IDs are sent to every subscriber, so the server should check that a client only uses its own. See [Access control]({{ site.baseurl }}{% link presence.md %}#presence-ids). Return value diff --git a/docs/middleware/actions.md b/docs/middleware/actions.md index 3d34dc6a3..ef3e7cb21 100644 --- a/docs/middleware/actions.md +++ b/docs/middleware/actions.md @@ -69,6 +69,9 @@ This action has these additional `context` properties: Presence information has just been received from a client. The presence has not yet been transformed against any ops it has missed. +{: .warn } +Presence is not covered by document permissions. See [Access control]({{ site.baseurl }}{% link presence.md %}#access-control). + This action has these additional `context` properties: `collection` -- string @@ -83,6 +86,9 @@ This action has these additional `context` properties: Presence information is about to be sent to a client. +{: .warn } +Presence is not covered by document permissions. See [Access control]({{ site.baseurl }}{% link presence.md %}#access-control). + This action has these additional `context` properties: `collection` -- string diff --git a/docs/presence.md b/docs/presence.md index 9567446e2..4db987e58 100644 --- a/docs/presence.md +++ b/docs/presence.md @@ -76,3 +76,76 @@ const localPresence = presence.create() // The presence value depends on the type localPresence.submit(value) ``` + +## Access control + +{: .warn } +Presence is **not** covered by document permissions: being able to read or write a document does not control who can see or send presence on it. Use [middleware]({{ site.baseurl }}{% link middleware/index.md %}) to restrict presence. + +### Receiving presence + +Subscribing to presence doesn't trigger any presence middleware, so any client can subscribe to any presence channel -- including the presence of a document it can't read. + +To stop a client receiving presence, reject the update in the [`'sendPresence'`]({{ site.baseurl }}{% link middleware/actions.md %}#sendpresence) middleware. For typed presence, ShareDB makes sure that `presence.c` and `presence.d` match the presence channel. This middleware runs for every update about to be sent to every client, so keep the check cheap -- for example, by caching permissions on [`agent.custom`]({{ site.baseurl }}{% link api/agent.md %}#custom--object). + +{: .warn } +Rejecting updates in `'sendPresence'` only stops them reaching the client if you set the [`doNotForwardSendPresenceErrorsToClient`]({{ site.baseurl }}{% link api/backend.md %}#options) option. Otherwise, the client is sent an error for each rejected update, including its presence ID. With the option set, each rejected update is passed to the `errorHandler` instead. + +### Sending presence + +ShareDB rejects typed presence whose collection and document ID don't match its channel. It also only broadcasts typed presence if the client can read the document, as determined by the [`'readSnapshots'`]({{ site.baseurl }}{% link middleware/actions.md %}#readsnapshots) middleware. This is checked when the client starts sending presence on the document, not on every update. + +ShareDB doesn't check: + + - whether the client can write to the document + - `null` presence, which is sent when a client leaves + - untyped presence + +Check these in the [`'receivePresence'`]({{ site.baseurl }}{% link middleware/actions.md %}#receivepresence) middleware if you need to. + +If you only use typed presence, reject untyped presence in `'receivePresence'`: older clients still apply untyped presence sent on a document's channel. + +### Presence IDs + +Presence IDs are chosen by the client, and sent to every subscriber, so a client could use another client's presence ID to overwrite or clear its presence. + +To prevent this, tie presence IDs to the user in `'receivePresence'` -- for example by requiring them to start with the user ID: + +```js +const localPresence = presence.create(`${userId}:${randomId}`) +``` + +{: .info } +Keep a random suffix, so that the same user can have more than one presence -- for example, in multiple browser tabs. + +### Example + +```js +backend.use('receivePresence', (context, next) => { + // agent.custom is usually set in the 'connect' hook + const userId = context.agent.custom.userId + const presence = context.presence + if (!presence.id.startsWith(`${userId}:`)) { + return next(new Error('Unauthorized')) + } + // This app only uses typed presence + if (!presence.c) { + return next(new Error('Unauthorized')) + } + // Let users clear their own presence, even if they've lost access + if (presence.p === null) return next() + if (!userCanChangeDoc(userId, presence.c, presence.d)) { + return next(new Error('Unauthorized')) + } + next() +}) + +backend.use('sendPresence', (context, next) => { + const userId = context.agent.custom.userId + const presence = context.presence + if (!userCanReadDoc(userId, presence.c, presence.d)) { + return next(new Error('Unauthorized')) + } + next() +}) +``` diff --git a/lib/agent.js b/lib/agent.js index cf7f0d759..4d3d302d2 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -5,6 +5,7 @@ var ACTIONS = require('./message-actions').ACTIONS; var types = require('./types'); var util = require('./util'); var protocol = require('./protocol'); +var DocPresence = require('./client/presence/doc-presence'); var ERROR_CODE = ShareDBError.CODES; @@ -423,6 +424,9 @@ Agent.prototype._checkRequest = function(request) { if (typeof request.id !== 'string' || util.isDangerousProperty(request.id)) { return 'Invalid presence ID'; } + if (isDocPresence(request) && !matchesDocPresenceChannel(request)) { + return 'Invalid presence channel'; + } } } else if ( request.a === ACTIONS.bulkFetch || @@ -444,6 +448,14 @@ Agent.prototype._checkRequest = function(request) { } }; +function isDocPresence(request) { + return request.c != null || request.d != null; +} + +function matchesDocPresenceChannel(request) { + return !!request.c && !!request.d && request.ch === DocPresence.channel(request.c, request.d); +} + // Handle an incoming message from the client Agent.prototype._handleMessage = function(request, callback) { try { diff --git a/lib/client/presence/doc-presence.js b/lib/client/presence/doc-presence.js index 612893c59..ced87d817 100644 --- a/lib/client/presence/doc-presence.js +++ b/lib/client/presence/doc-presence.js @@ -17,6 +17,12 @@ DocPresence.channel = function(collection, id) { return collection + '.' + id; }; +DocPresence.prototype._receiveUpdate = function(error, message) { + var isForThisDoc = message.c === this.collection && message.d === this.id; + if (!error && !isForThisDoc) return; + Presence.prototype._receiveUpdate.call(this, error, message); +}; + DocPresence.prototype._createLocalPresence = function(id) { return new LocalDocPresence(this, id); }; diff --git a/lib/client/presence/presence.js b/lib/client/presence/presence.js index 7b3fccabc..9c173ae0d 100644 --- a/lib/client/presence/presence.js +++ b/lib/client/presence/presence.js @@ -120,8 +120,9 @@ Presence.prototype._handleUnsubscribe = function(error, seq) { }; Presence.prototype._receiveUpdate = function(error, message) { + var isForwardedError = !!error && message.pv == null; var localPresence = util.dig(this.localPresences, message.id); - if (localPresence) return localPresence._ack(error, message.pv); + if (localPresence && !isForwardedError) return localPresence._ack(error, message.pv); if (error) return this.emit('error', error); var presence = this; diff --git a/test/client/presence/doc-presence.js b/test/client/presence/doc-presence.js index 5cb4b5ef7..e2d6ea45c 100644 --- a/test/client/presence/doc-presence.js +++ b/test/client/presence/doc-presence.js @@ -601,6 +601,175 @@ describe('DocPresence', function() { connection1.send(message); }); + it('rejects a presence message for a different doc than its channel', function(done) { + var localPresence1 = presence1.create('presence-1'); + localPresence1.on('error', function(error) { + expect(error.code).to.eql('ERR_MESSAGE_BADLY_FORMED'); + done(); + }); + + var message = localPresence1._message(); + message.d = 'subtle-knife'; + message.v = 1; + message.t = presenceTestType.type.uri; + connection1.send(message); + }); + + it('rejects a presence message with a doc ID but no collection', function(done) { + var localPresence1 = presence1.create('presence-1'); + localPresence1.on('error', function(error) { + expect(error.code).to.eql('ERR_MESSAGE_BADLY_FORMED'); + done(); + }); + + var message = localPresence1._message(); + message.c = null; + message.v = 1; + message.t = presenceTestType.type.uri; + connection1.send(message); + }); + + it('rejects a presence message with a collection but no doc ID', function(done) { + var localPresence1 = presence1.create('presence-1'); + localPresence1.on('error', function(error) { + expect(error.code).to.eql('ERR_MESSAGE_BADLY_FORMED'); + done(); + }); + + var message = localPresence1._message(); + message.d = null; + message.v = 1; + message.t = presenceTestType.type.uri; + connection1.send(message); + }); + + it('rejects a presence message with an empty collection', function(done) { + var localPresence1 = connection1.getDocPresence('', 'northern-lights').create('presence-1'); + localPresence1.on('error', function(error) { + expect(error.code).to.eql('ERR_MESSAGE_BADLY_FORMED'); + done(); + }); + + var message = localPresence1._message(); + message.v = 1; + message.t = presenceTestType.type.uri; + connection1.send(message); + }); + + it('rejects presence on a doc the client cannot read', function(done) { + backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots, function(context, next) { + next(new Error('Forbidden')); + }); + + var localPresence1 = presence1.create('presence-1'); + localPresence1.submit({index: 1}, function(error) { + expect(error.message).to.equal('Forbidden'); + done(); + }); + }); + + it('ignores untyped presence on its channel', function(done) { + var connection3 = backend.connect(); + var untypedPresence3 = connection3.getPresence('books.northern-lights'); + var localPresence1 = presence1.create('presence-1'); + + async.series([ + presence2.subscribe.bind(presence2), + untypedPresence3.subscribe.bind(untypedPresence3), + function(next) { + connection2.on('receive', function(request) { + if (request.data.src !== connection3.id) return; + localPresence1.submit({index: 1}, errorHandler(done)); + }); + presence2.once('receive', function(id, presence) { + expect(id).to.equal('presence-1'); + expect(presence).to.eql({index: 1}); + next(); + }); + connection3.send({ + a: 'p', ch: 'books.northern-lights', id: 'presence-1', p: {index: 9}, pv: 0, v: doc1.version + }); + } + ], done); + }); + + it('does not let untyped presence with the same ID ack its local presence', function(done) { + var connection3 = backend.connect(); + var untypedPresence3 = connection3.getPresence('books.northern-lights'); + var localPresence1 = presence1.create('presence-1'); + var hasReceivedOwnPresence = false; + + async.series([ + presence1.subscribe.bind(presence1), + untypedPresence3.subscribe.bind(untypedPresence3), + function(next) { + connection1.on('receive', function(request) { + if (request.data.src === connection1.id) hasReceivedOwnPresence = true; + if (request.data.src !== connection3.id) return; + localPresence1.submit({index: 1}, function(error) { + if (error) return done(error); + expect(hasReceivedOwnPresence).to.be.true; + next(); + }); + }); + connection3.send({ + a: 'p', ch: 'books.northern-lights', id: 'presence-1', p: {index: 9}, pv: 0, v: doc1.version + }); + } + ], done); + }); + + it('emits a forwarded error for a presence with the same ID on the presence, not its local presence', function(done) { + backend.use(backend.MIDDLEWARE_ACTIONS.sendPresence, function(context, next) { + next(new Error('uh-oh!')); + }); + + var connection3 = backend.connect(); + var doc3 = connection3.get('books', 'northern-lights'); + var localPresence3 = connection3.getDocPresence('books', 'northern-lights').create('presence-1'); + var localPresence1 = presence1.create('presence-1'); + localPresence1.on('error', done); + + async.series([ + doc3.subscribe.bind(doc3), + presence1.subscribe.bind(presence1), + function(next) { + presence1.once('error', function(error) { + expect(error.message).to.equal('uh-oh!'); + next(); + }); + localPresence3.submit({index: 1}, errorHandler(done)); + } + ], done); + }); + + it('ignores presence for a doc whose collection and id join to the same channel', function(done) { + var connection3 = backend.connect(); + var notesDoc1 = connection1.get('notes', 'private.alice'); + var notesDoc3 = connection3.get('notes', 'private.alice'); + var diaryDoc2 = connection2.get('notes.private', 'alice'); + var localNotesPresence1 = connection1.getDocPresence('notes', 'private.alice').create('notes-presence'); + var localDiaryPresence2 = connection2.getDocPresence('notes.private', 'alice').create('diary-presence'); + var notesPresence3 = connection3.getDocPresence('notes', 'private.alice'); + + async.series([ + notesDoc1.create.bind(notesDoc1, 'Hello', presenceTestType.type.name), + diaryDoc2.create.bind(diaryDoc2, 'Dear diary', presenceTestType.type.name), + notesDoc3.subscribe.bind(notesDoc3), + notesPresence3.subscribe.bind(notesPresence3), + function(next) { + notesPresence3.once('receive', function(id) { + expect(id).to.equal('notes-presence'); + next(); + }); + localDiaryPresence2.submit({index: 1}, function(error) { + if (error) return done(error); + localNotesPresence1.submit({index: 1}, errorHandler(done)); + }); + } + ], done); + }); + it('only sends presence responses for the associated doc', function(done) { var localPresence1 = presence1.create('presence-1'); var localPresence2 = presence2.create('presence-2'); @@ -976,12 +1145,19 @@ describe('DocPresence', function() { var localPresence1 = presence1.create('presence-1'); var catchUpCount = 0; - function sendUntypedPresenceAtVersion(version) { - var message = localPresence1._message(); - message.d = null; - message.p = {index: 0}; - message.v = version; - connection1.send(message); + function receiveTypelessPresenceAtVersion(version) { + connection2.handleMessage({ + a: ACTIONS.presence, + ch: 'books.northern-lights', + src: connection1.id, + id: 'presence-1', + p: {index: 0}, + pv: version, + c: 'books', + d: 'northern-lights', + v: version, + t: null + }); } async.series([ @@ -992,7 +1168,7 @@ describe('DocPresence', function() { catchUpCount++; if (catchUpCount === 1) next(); }); - sendUntypedPresenceAtVersion(0); + receiveTypelessPresenceAtVersion(0); }, function(next) { doc2.once('op', function() { @@ -1006,7 +1182,7 @@ describe('DocPresence', function() { expect(error.code).to.equal('ERR_DOC_TYPE_NOT_RECOGNIZED'); next(); }); - sendUntypedPresenceAtVersion(1); + receiveTypelessPresenceAtVersion(1); }, function(next) { doc2.once('op', function() { diff --git a/test/client/presence/presence.js b/test/client/presence/presence.js index f972327e2..22089a35a 100644 --- a/test/client/presence/presence.js +++ b/test/client/presence/presence.js @@ -795,6 +795,27 @@ describe('Presence', function() { ], done); }); + it('emits an error for a presence with the same ID on the presence, not its local presence', function(done) { + var localPresence1 = presence1.create('presence-1'); + var localPresence2 = presence2.create('presence-1'); + localPresence2.on('error', done); + + backend.use(backend.MIDDLEWARE_ACTIONS.sendPresence, function(context, next) { + next(new Error('uh-oh!')); + }); + + async.series([ + presence2.subscribe.bind(presence2), + function(next) { + localPresence1.submit({index: 3}, errorHandler(done)); + presence2.once('error', function(error) { + expect(error.message).to.equal('uh-oh!'); + next(); + }); + } + ], done); + }); + it('emits errors on the server instead of sending to the client', function(done) { var localPresence1 = presence1.create('presence-1');