Merge pull request #524 from share/no-send-presence-errors

🔒 Prevent sending `sendPresence` middleware errors to subscribers

docs/api/backend.md

@@ -111,6 +111,21 @@ Optional
 
 > If set to `true`, enables [Presence]({{ site.baseurl }}{% link presence.md %}) functionality
 
+{: .d-inline-block }
+
+`errorHandler` -- Function
+
+Optional
+{: .label .label-grey }
+
+> ```js
+> function(error, context) {
+>   logger.error(error)
+> }
+> ```
+
+> Non-fatal ShareDB server errors will be passed to this handler. By default, ShareDB will log the error
+
 ## Properties
 
 ### `MIDDLEWARE_ACTIONS` -- Object

lib/agent.js

@@ -823,7 +823,9 @@ Agent.prototype._handlePresenceData = function(presence) {
   var agent = this;
   backend.trigger(backend.MIDDLEWARE_ACTIONS.sendPresence, this, context, function(error) {
     if (error) {
-      return agent.send({a: 'p', ch: presence.ch, id: presence.id, error: getReplyErrorObject(error)});
+      if (backend.doNotForwardSendPresenceErrorsToClient) backend.errorHandler(error, {agent: agent});
+      else agent.send({a: 'p', ch: presence.ch, id: presence.id, error: getReplyErrorObject(error)});
+      return;
     }
     agent.send(presence);
   });

lib/backend.js

@@ -14,6 +14,7 @@ var StreamSocket = require('./stream-socket');
 var SubmitRequest = require('./submit-request');
 var ReadSnapshotsRequest = require('./read-snapshots-request');
 var util = require('./util');
+var logger = require('./logger');
 
 var ERROR_CODE = ShareDBError.CODES;
 
@@ -34,13 +35,28 @@ function Backend(options) {
   this.suppressPublish = !!options.suppressPublish;
   this.maxSubmitRetries = options.maxSubmitRetries || null;
   this.presenceEnabled = !!options.presence;
+  this.doNotForwardSendPresenceErrorsToClient = !!options.doNotForwardSendPresenceErrorsToClient;
+  if (!this.doNotForwardSendPresenceErrorsToClient) {
+    logger.warn(
+      'Broadcasting "sendPresence" middleware errors to clients is deprecated ' +
+      'and will be removed in a future release. Disable this behaviour with:\n\n' +
+      'new Backend({doNotForwardSendPresenceErrorsToClient: true})\n\n'
+    );
+  }
 
   // Map from event name to a list of middleware
   this.middleware = {};
 
   // The number of open agents for monitoring and testing memory leaks
   this.agentsCount = 0;
   this.remoteAgentsCount = 0;
+
+  this.errorHandler = typeof options.errorHandler === 'function' ?
+    options.errorHandler :
+    // eslint-disable-next-line no-unused-vars
+    function(error, context) {
+      logger.error(error);
+    };
 }
 module.exports = Backend;
 emitter.mixin(Backend);

test/backend.js

@@ -1,29 +1,51 @@
 var Backend = require('../lib/backend');
 var expect = require('chai').expect;
 var sinon = require('sinon');
+var logger = require('../lib/logger');
 
 describe('Backend', function() {
   var backend;
-  var agent = {
-    custom: {
-      foo: 'bar'
-    }
-  };
-  var fetchOptions = {
-    snapshotOptions: {
-      fizz: 'buzz'
-    }
-  };
-
-  beforeEach(function() {
-    backend = new Backend();
-  });
 
   afterEach(function(done) {
     backend.close(done);
   });
 
+  describe('options', function() {
+    describe('errorHandler', function() {
+      it('logs by default', function() {
+        backend = new Backend();
+        var error = new Error('foo');
+        sinon.spy(logger, 'error');
+        backend.errorHandler(error);
+        expect(logger.error.callCount).to.equal(1);
+      });
+
+      it('overrides with another function', function() {
+        var handler = sinon.spy();
+        backend = new Backend({errorHandler: handler});
+        var error = new Error('foo');
+        backend.errorHandler(error);
+        expect(handler.callCount).to.equal(1);
+      });
+    });
+  });
+
   describe('a simple document', function() {
+    var agent = {
+      custom: {
+        foo: 'bar'
+      }
+    };
+    var fetchOptions = {
+      snapshotOptions: {
+        fizz: 'buzz'
+      }
+    };
+
+    beforeEach(function() {
+      backend = new Backend();
+    });
+
     beforeEach(function(done) {
       var doc = backend.connect().get('books', '1984');
       doc.create({title: '1984'}, function(error) {

test/client/presence/presence.js

@@ -541,5 +541,48 @@ describe('Presence', function() {
         ], done);
       });
     });
+
+    describe('sendPresence', function() {
+      // TODO: This functionality is deprecated
+      it('sends an error to a subscribed client', function(done) {
+        var localPresence1 = presence1.create('presence-1');
+
+        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');
+
+        backend.doNotForwardSendPresenceErrorsToClient = true;
+        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.on('error', errorHandler(done));
+            backend.errorHandler = function(error) {
+              expect(error.message).to.equal('uh-oh!');
+              next();
+            };
+          }
+        ], done);
+      });
+    });
   });
 });