Disable Failing Webhooks (#11966)

* todo comment

* add failures field to webhooks and sanitize

* implement logic

* use update instead of save

* specify timeout and maximum number of retries

* add tests

test/api/unit/libs/webhooks.test.js

@@ -1,4 +1,5 @@
 import got from 'got';
+import moment from 'moment';
 import {
   WebhookSender,
   taskScoredWebhook,
@@ -13,6 +14,7 @@ import {
 import {
   generateUser,
   defer,
+  sleep,
 } from '../../../helpers/api-unit.helper';
 
 
@@ -322,6 +324,100 @@ describe('webhooks', () => {
         json: true,
       });
     });
+
+    describe('failures', () => {
+      let sendWebhook;
+
+      beforeEach(async () => {
+        sandbox.restore();
+        sandbox.stub(got, 'post').returns(Promise.reject());
+
+        sendWebhook = new WebhookSender({ type: 'taskActivity' });
+        user.webhooks = [{
+          url: 'http://custom-url.com', enabled: true, type: 'taskActivity',
+        }];
+        await user.save();
+
+        expect(user.webhooks[0].failures).to.equal(0);
+        expect(user.webhooks[0].lastFailureAt).to.equal(undefined);
+      });
+
+      it('does not increase failures counter if request is successfull', async () => {
+        sandbox.restore();
+        sandbox.stub(got, 'post').returns(Promise.resolve());
+
+        const body = {};
+        sendWebhook.send(user, body);
+
+        expect(got.post).to.be.calledOnce;
+        expect(got.post).to.be.calledWithMatch('http://custom-url.com', {
+          json: true,
+          body,
+        });
+
+        await sleep(0.1);
+        user = await User.findById(user._id).exec();
+
+        expect(user.webhooks[0].failures).to.equal(0);
+        expect(user.webhooks[0].lastFailureAt).to.equal(undefined);
+      });
+
+      it('records failures', async () => {
+        const body = {};
+        sendWebhook.send(user, body);
+
+        expect(got.post).to.be.calledOnce;
+        expect(got.post).to.be.calledWithMatch('http://custom-url.com', {
+          json: true,
+          body,
+        });
+
+        await sleep(0.1);
+        user = await User.findById(user._id).exec();
+
+        expect(user.webhooks[0].failures).to.equal(1);
+        expect(user.webhooks[0].lastFailureAt.toLocaleString())
+          .to.equal((new Date()).toLocaleString());
+      });
+
+      it('disables a webhook after 10 failures', async () => {
+        const times = 10;
+        for (let i = 0; i < times; i += 1) {
+          sendWebhook.send(user, {});
+          await sleep(0.1); // eslint-disable-line no-await-in-loop
+          user = await User.findById(user._id).exec(); // eslint-disable-line no-await-in-loop
+        }
+
+        expect(got.post).to.be.callCount(10);
+        expect(got.post).to.be.calledWithMatch('http://custom-url.com');
+
+        await sleep(0.1);
+        user = await User.findById(user._id).exec();
+
+        expect(user.webhooks[0].enabled).to.equal(false);
+        expect(user.webhooks[0].failures).to.equal(0);
+      });
+
+      it('resets failures after a month ', async () => {
+        const oneMonthAgo = moment().subtract(1, 'months').subtract(1, 'days').toDate();
+        user.webhooks[0].lastFailureAt = oneMonthAgo;
+        user.webhooks[0].failures = 9;
+
+        await user.save();
+
+        sendWebhook.send(user, []);
+
+        expect(got.post).to.be.calledOnce;
+        expect(got.post).to.be.calledWithMatch('http://custom-url.com');
+
+        await sleep(0.1);
+        user = await User.findById(user._id).exec();
+
+        expect(user.webhooks[0].failures).to.equal(1);
+        expect(user.webhooks[0].lastFailureAt.toLocaleString())
+          .to.equal((new Date()).toLocaleString());
+      });
+    });
   });
 
   describe('taskScoredWebhook', () => {

test/api/v3/integration/webhook/POST-user_add_webhook.test.js

@@ -81,6 +81,16 @@ describe('POST /user/webhook', () => {
     expect(webhook.type).to.eql('taskActivity');
   });
 
+  it('ignores protected fields', async () => {
+    body.failures = 3;
+    body.lastFailureAt = new Date();
+
+    const webhook = await user.post('/user/webhook', body);
+
+    expect(webhook.failures).to.eql(0);
+    expect(webhook.lastFailureAt).to.eql(undefined);
+  });
+
   it('successfully adds the webhook', async () => {
     expect(user.webhooks).to.eql([]);
 

test/api/v3/integration/webhook/PUT-user_update_webhook.test.js

@@ -63,6 +63,21 @@ describe('PUT /user/webhook/:id', () => {
     expect(webhook.options).to.eql(options);
   });
 
+  it('ignores protected fields', async () => {
+    const failures = 3;
+    const lastFailureAt = new Date();
+
+    await user.put(`/user/webhook/${webhookToUpdate.id}`, {
+      failures, lastFailureAt,
+    });
+
+    await user.sync();
+    const webhook = user.webhooks.find(hook => webhookToUpdate.id === hook.id);
+
+    expect(webhook.failures).to.eql(0);
+    expect(webhook.lastFailureAt).to.eql(undefined);
+  });
+
   it('updates a webhook with empty label', async () => {
     const url = 'http://a-new-url.com';
     const type = 'groupChatReceived';

website/server/controllers/api-v3/webhook.js

@@ -108,7 +108,7 @@ api.addWebhook = {
   url: '/user/webhook',
   async handler (req, res) {
     const { user } = res.locals;
-    const webhook = new Webhook(req.body);
+    const webhook = new Webhook(Webhook.sanitize(req.body));
 
     const existingWebhook = user.webhooks.find(hook => hook.id === webhook.id);
 

website/server/controllers/top-level/dataexport.js

@@ -248,7 +248,7 @@ api.exportUserAvatarPng = {
 
     let response;
     try {
-      response = await got.head(s3url);
+      response = await got.head(s3url); // TODO add timeout and retries
     } catch (gotError) {
       // If the file does not exist AWS S3 can return a 403 error
       if (gotError.code !== 'ENOTFOUND' && gotError.statusCode !== 404 && gotError.statusCode !== 403) {

website/server/libs/email.js

@@ -131,6 +131,8 @@ export async function sendTxn (mailingInfoArray, emailType, variables, personalV
 
   if (IS_PROD && mailingInfoArray.length > 0) {
     return got.post(`${EMAIL_SERVER.url}/job`, {
+      retry: 5, // retry the http request to the email server 5 times
+      timeout: 60000, // wait up to 60s before timing out
       auth: `${EMAIL_SERVER.auth.user}:${EMAIL_SERVER.auth.password}`,
       json: true,
       body: {

website/server/libs/webhook.js

@@ -1,6 +1,7 @@
 import got from 'got';
 import { isURL } from 'validator';
 import nconf from 'nconf';
+import moment from 'moment';
 import logger from './logger';
 import { // eslint-disable-line import/no-cycle
   model as User,
@@ -8,11 +9,59 @@ import { // eslint-disable-line import/no-cycle
 
 const IS_PRODUCTION = nconf.get('IS_PROD');
 
-function sendWebhook (url, body) {
+function sendWebhook (webhook, body, user) {
+  const { url, lastFailureAt } = webhook;
+
   got.post(url, {
     body,
     json: true,
-  }).catch(err => logger.error(err));
+    timeout: 30000, // wait up to 30s before timing out
+    retry: 3, // retry the request up to 3 times
+  }).catch(webhookErr => {
+    // Log the error
+    logger.error(webhookErr);
+
+    let _failuresReset = false;
+
+    // Reset failures if the last one happened more than 1 month ago
+    const oneMonthAgo = moment().subtract(1, 'months');
+    if (!lastFailureAt || moment(lastFailureAt).isBefore(oneMonthAgo)) {
+      webhook.failures = 0;
+      _failuresReset = true;
+    }
+
+    // Increase the number of failures
+    webhook.failures += 1;
+    webhook.lastFailureAt = new Date();
+
+    // Disable a webhook with too many failures
+    if (webhook.failures >= 10) {
+      webhook.enabled = false;
+      webhook.failures = 0;
+      webhook.lastFailureAt = undefined;
+      _failuresReset = true;
+    }
+
+    const update = {
+      $set: {
+        'webhooks.$.lastFailureAt': webhook.lastFailureAt,
+        'webhooks.$.enabled': webhook.enabled,
+      },
+    };
+
+    if (_failuresReset) {
+      update.$set['webhooks.$.failures'] = webhook.failures;
+    } else {
+      update.$inc = {
+        'webhooks.$.failures': 1,
+      };
+    }
+
+    return User.update({
+      _id: user._id,
+      'webhooks.id': webhook.id,
+    }, update).exec();
+  }).catch(err => logger.error(err)); // log errors that might have happened in the previous catch
 }
 
 function isValidWebhook (hook) {
@@ -60,7 +109,7 @@ export class WebhookSender {
     this.attachDefaultData(user, body);
 
     hooks.forEach(hook => {
-      sendWebhook(hook.url, body);
+      sendWebhook(hook, body, user);
     });
   }
 }

website/server/models/webhook.js

@@ -61,6 +61,10 @@ export const schema = new Schema({
     }), shared.i18n.t('invalidUrl')],
   },
   enabled: { $type: Boolean, required: true, default: true },
+  // How many times this webhook has failed, disabled after 10
+  failures: { $type: Number, default: 0 },
+  // When the last failure happened, if older than 1 month the number of failures is reset
+  lastFailureAt: { $type: Date },
   options: {
     $type: Schema.Types.Mixed,
     required: true,
@@ -76,7 +80,7 @@ export const schema = new Schema({
 });
 
 schema.plugin(baseModel, {
-  noSet: ['_id'],
+  noSet: ['_id', 'failures', 'lastFailureAt'],
   timestamps: true,
   _id: false,
 });