diff --git a/docs/api/backend.md b/docs/api/backend.md index b3efa8b0..f6657783 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 d8768a7a..2cc13666 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 3d34dc6a..ef3e7cb2 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 9567446e..4db987e5 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 cf7f0d75..4d3d302d 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 612893c5..ced87d81 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 7b3fccab..9c173ae0 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 5cb4b5ef..e2d6ea45 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 f972327e..22089a35 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');