From 263c493e2b3429d56ac183f90b26d493f6ed2e44 Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 20 Nov 2024 12:12:23 +0700 Subject: [PATCH 1/3] Refactored webhook restriction to WebhookTrigger ref https://linear.app/ghost/issue/AP-598 This is not a change in functionality. The ActivityPub integration is an `internal` integration, because we want it to be available regardless of the plan a Ghost(Pro) site is on. However the webhooks service is not able to differentiate between webhooks for a custom integration and an internal one. Rather than disable webhooks entirely when the custom integrations limit active, we want to allow webhooks for internal integrations to work. The first step towards that is keeping the listener for the model events and have the limiting happen in the WebhookTrigger which allows us to be more specific as to which webhooks should be triggered or not. --- .../services/webhooks/WebhookTrigger.js | 19 ++++++++- .../core/server/services/webhooks/listen.js | 14 +------ ghost/core/test/e2e-webhooks/site.test.js | 39 +++++++++++++++++++ .../server/services/webhooks/trigger.test.js | 33 +++++++++++++++- .../test/utils/e2e-framework-mock-manager.js | 11 ++++++ 5 files changed, 99 insertions(+), 17 deletions(-) diff --git a/ghost/core/core/server/services/webhooks/WebhookTrigger.js b/ghost/core/core/server/services/webhooks/WebhookTrigger.js index 68b92395233b..e88545f541f4 100644 --- a/ghost/core/core/server/services/webhooks/WebhookTrigger.js +++ b/ghost/core/core/server/services/webhooks/WebhookTrigger.js @@ -9,16 +9,31 @@ class WebhookTrigger { * @param {Object} options * @param {Object} options.models - Ghost models * @param {Function} options.payload - Function to generate payload + * @param {import('../../services/limits')} options.limitService - Function to generate payload * @param {Object} [options.request] - HTTP request handling library */ - constructor({models, payload, request}){ + constructor({models, payload, request, limitService}){ this.models = models; this.payload = payload; this.request = request ?? require('@tryghost/request'); + this.limitService = limitService; } - getAll(event) { + async getAll(event) { + if (this.limitService.isLimited('customIntegrations')) { + // NOTE: using "checkWouldGoOverLimit" instead of "checkIsOverLimit" here because flag limits don't have + // a concept of measuring if the limit has been surpassed + const overLimit = await this.limitService.checkWouldGoOverLimit('customIntegrations'); + + if (overLimit) { + logging.info(`Skipping all webhooks for event ${event}. The "customIntegrations" plan limit is enabled.`); + return { + models: [] + }; + } + } + return this.models .Webhook .findAllByEvent(event, {context: {internal: true}}); diff --git a/ghost/core/core/server/services/webhooks/listen.js b/ghost/core/core/server/services/webhooks/listen.js index d76e6fcd6210..4ce8817a8aa2 100644 --- a/ghost/core/core/server/services/webhooks/listen.js +++ b/ghost/core/core/server/services/webhooks/listen.js @@ -1,6 +1,5 @@ const _ = require('lodash'); const limitService = require('../../services/limits'); -const logging = require('@tryghost/logging'); const WebhookTrigger = require('./WebhookTrigger'); const models = require('../../models'); const payload = require('./payload'); @@ -46,18 +45,7 @@ const WEBHOOKS = [ ]; const listen = async () => { - if (limitService.isLimited('customIntegrations')) { - // NOTE: using "checkWouldGoOverLimit" instead of "checkIsOverLimit" here because flag limits don't have - // a concept of measuring if the limit has been surpassed - const overLimit = await limitService.checkWouldGoOverLimit('customIntegrations'); - - if (overLimit) { - logging.info(`Skipped subscribing webhooks to events. The "customIntegrations" plan limit is enabled.`); - return; - } - } - - const webhookTrigger = new WebhookTrigger({models, payload}); + const webhookTrigger = new WebhookTrigger({models, payload, limitService}); _.each(WEBHOOKS, (event) => { // @NOTE: The early exit makes sure the listeners are only registered once. // During testing the "events" instance is kept around with all the diff --git a/ghost/core/test/e2e-webhooks/site.test.js b/ghost/core/test/e2e-webhooks/site.test.js index 86278fa4690c..0d1158352d07 100644 --- a/ghost/core/test/e2e-webhooks/site.test.js +++ b/ghost/core/test/e2e-webhooks/site.test.js @@ -48,4 +48,43 @@ describe('site.* events', function () { }) .matchBodySnapshot(); }); + + it('site.changed event is triggered but the custom integrations are limited', async function () { + const webhookURL = 'https://test-webhook-receiver.com/site-changed'; + await webhookMockReceiver.mock(webhookURL); + await fixtureManager.insertWebhook({ + event: 'site.changed', + url: webhookURL + }); + + mockManager.mockLimitService('customIntegrations', { + isLimited: true, + wouldGoOverLimit: true + }); + + await adminAPIAgent + .post('posts/') + .body({ + posts: [{ + title: 'webhookz', + status: 'published', + mobiledoc: fixtureManager.get('posts', 1).mobiledoc + }] + }) + .expectStatus(201); + + const receivedRequest = webhookMockReceiver.receivedRequest().then(() => true); + const wait = new Promise((resolve) => { + setTimeout(resolve, 2000, false); + }); + + const requestWasRecieved = await Promise.race([ + receivedRequest, + wait + ]); + + if (requestWasRecieved) { + throw new Error('The webhook should not have been sent.'); + } + }); }); diff --git a/ghost/core/test/unit/server/services/webhooks/trigger.test.js b/ghost/core/test/unit/server/services/webhooks/trigger.test.js index b4791ab7d02f..f6146b368d84 100644 --- a/ghost/core/test/unit/server/services/webhooks/trigger.test.js +++ b/ghost/core/test/unit/server/services/webhooks/trigger.test.js @@ -1,6 +1,7 @@ const assert = require('assert/strict'); const crypto = require('crypto'); const sinon = require('sinon'); +const LimitService = require('@tryghost/limit-service'); const WebhookTrigger = require('../../../../../core/server/services/webhooks/WebhookTrigger'); @@ -12,7 +13,7 @@ describe('Webhook Service', function () { const WEBHOOK_TARGET_URL = 'http://example.com'; const WEBHOOK_SECRET = 'abc123dontstealme'; - let models, payload, request, webhookTrigger; + let models, payload, request, webhookTrigger, limitService; beforeEach(function () { models = { @@ -34,7 +35,12 @@ describe('Webhook Service', function () { payload = sinon.stub(); request = sinon.stub().resolves({}); - webhookTrigger = new WebhookTrigger({models, payload, request}); + const realLimitService = new LimitService(); + limitService = sinon.stub(realLimitService); + limitService.isLimited.withArgs('customIntegrations').returns(false); + limitService.checkWouldGoOverLimit.withArgs('customIntegrations').resolves(false); + + webhookTrigger = new WebhookTrigger({models, payload, request, limitService}); sinon.stub(webhookTrigger, 'onSuccess').callsFake(() => Promise.resolve()); sinon.stub(webhookTrigger, 'onError').callsFake(() => Promise.resolve()); @@ -53,6 +59,29 @@ describe('Webhook Service', function () { assert.equal(request.called, false); }); + it('does not trigger payload handler when there are hooks registered for an event, but the custom integrations limit is active', async function () { + const webhookModel = { + get: sinon.stub() + }; + + webhookModel.get + .withArgs('event').returns(WEBHOOK_EVENT) + .withArgs('target_url').returns(WEBHOOK_TARGET_URL); + + models.Webhook.findAllByEvent + .withArgs(WEBHOOK_EVENT, {context: {internal: true}}) + .resolves({models: [webhookModel]}); + + limitService.isLimited.withArgs('customIntegrations').returns(true); + limitService.checkWouldGoOverLimit.withArgs('customIntegrations').resolves(true); + + await webhookTrigger.trigger(WEBHOOK_EVENT); + + assert.equal(models.Webhook.findAllByEvent.called, false); + assert.equal(payload.called, false); + assert.equal(request.called, false); + }); + it('triggers payload handler when there are hooks registered for an event', async function () { const webhookModel = { get: sinon.stub() diff --git a/ghost/core/test/utils/e2e-framework-mock-manager.js b/ghost/core/test/utils/e2e-framework-mock-manager.js index 3d5eec4021b5..8bbba5d30154 100644 --- a/ghost/core/test/utils/e2e-framework-mock-manager.js +++ b/ghost/core/test/utils/e2e-framework-mock-manager.js @@ -19,6 +19,7 @@ const originalMailServiceSendMail = mailService.GhostMailer.prototype.sendMail; const labs = require('../../core/shared/labs'); const events = require('../../core/server/lib/common/events'); const settingsCache = require('../../core/shared/settings-cache'); +const limitService = require('../../core/server/services/limits'); const dns = require('dns'); const dnsPromises = dns.promises; const StripeMocker = require('./stripe-mocker'); @@ -283,6 +284,15 @@ const mockLabsDisabled = (flag, alpha = true) => { fakedLabsFlags[flag] = false; }; +const mockLimitService = (limit, options) => { + if (!mocks.limitService) { + mocks.limitService = sinon.stub(limitService); + } + + mocks.limitService.isLimited.withArgs(limit).returns(options.isLimited); + mocks.limitService.checkWouldGoOverLimit.withArgs(limit).resolves(options.wouldGoOverLimit); +}; + const restore = () => { // eslint-disable-next-line no-console configUtils.restore().catch(console.error); @@ -319,6 +329,7 @@ module.exports = { mockLabsDisabled, mockWebhookRequests, mockSetting, + mockLimitService, disableNetwork, restore, stripeMocker, From 3265e257b7575595df50835045108ca3c6f9554d Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 20 Nov 2024 17:54:30 +0700 Subject: [PATCH 2/3] Fixed webhooks not firing for internal integrations ref https://linear.app/ghost/issue/AP-598 We want to make sure that webhooks for internal integrations are fired even when the custom integrations limit has been hit. In order to test this we've had to update the fixtureManager to allow passing the integration type when inserting fixture webhooks into the db. --- .../services/webhooks/WebhookTrigger.js | 13 +++++-- .../__snapshots__/site.test.js.snap | 12 +++++++ ghost/core/test/e2e-webhooks/site.test.js | 36 +++++++++++++++++++ .../server/services/webhooks/trigger.test.js | 10 ++++-- ghost/core/test/utils/e2e-framework.js | 4 ++- ghost/core/test/utils/fixture-utils.js | 10 ++++-- 6 files changed, 77 insertions(+), 8 deletions(-) diff --git a/ghost/core/core/server/services/webhooks/WebhookTrigger.js b/ghost/core/core/server/services/webhooks/WebhookTrigger.js index e88545f541f4..a12d98e98a01 100644 --- a/ghost/core/core/server/services/webhooks/WebhookTrigger.js +++ b/ghost/core/core/server/services/webhooks/WebhookTrigger.js @@ -27,9 +27,18 @@ class WebhookTrigger { const overLimit = await this.limitService.checkWouldGoOverLimit('customIntegrations'); if (overLimit) { - logging.info(`Skipping all webhooks for event ${event}. The "customIntegrations" plan limit is enabled.`); + logging.info(`Skipping all non-internal webhooks for event ${event}. The "customIntegrations" plan limit is enabled.`); + const result = await this.models + .Webhook + .findAllByEvent(event, { + context: {internal: true}, + withRelated: ['integration'] + }); + return { - models: [] + models: result?.models?.filter((model) => { + return model.related('integration')?.get('type') === 'internal'; + }) || [] }; } } diff --git a/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap b/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap index 7ececc9f6509..67dbdaceb844 100644 --- a/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap +++ b/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap @@ -11,3 +11,15 @@ Object { `; exports[`site.* events site.changed event is triggered 2: [body] 1`] = `Object {}`; + +exports[`site.* events site.changed event is triggered, custom integrations are limited but we have an internal webhook 1: [headers] 1`] = ` +Object { + "accept-encoding": "gzip, deflate, br", + "content-length": Any, + "content-type": "application/json", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "user-agent": StringMatching /Ghost\\\\/\\\\d\\+\\\\\\.\\\\d\\+\\\\\\.\\\\d\\+\\\\s\\\\\\(https:\\\\/\\\\/github\\.com\\\\/TryGhost\\\\/Ghost\\\\\\)/, +} +`; + +exports[`site.* events site.changed event is triggered, custom integrations are limited but we have an internal webhook 2: [body] 1`] = `Object {}`; diff --git a/ghost/core/test/e2e-webhooks/site.test.js b/ghost/core/test/e2e-webhooks/site.test.js index 0d1158352d07..0472ba9e00a7 100644 --- a/ghost/core/test/e2e-webhooks/site.test.js +++ b/ghost/core/test/e2e-webhooks/site.test.js @@ -87,4 +87,40 @@ describe('site.* events', function () { throw new Error('The webhook should not have been sent.'); } }); + + it('site.changed event is triggered, custom integrations are limited but we have an internal webhook', async function () { + const webhookURL = 'https://test-webhook-receiver.com/site-changed'; + await webhookMockReceiver.mock(webhookURL); + await fixtureManager.insertWebhook({ + event: 'site.changed', + url: webhookURL, + integrationType: 'internal' + }); + + mockManager.mockLimitService('customIntegrations', { + isLimited: true, + wouldGoOverLimit: true + }); + + await adminAPIAgent + .post('posts/') + .body({ + posts: [{ + title: 'webhookz', + status: 'published', + mobiledoc: fixtureManager.get('posts', 1).mobiledoc + }] + }) + .expectStatus(201); + + await webhookMockReceiver.receivedRequest(); + + webhookMockReceiver + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + 'content-length': anyNumber, + 'user-agent': anyGhostAgent + }) + .matchBodySnapshot(); + }); }); diff --git a/ghost/core/test/unit/server/services/webhooks/trigger.test.js b/ghost/core/test/unit/server/services/webhooks/trigger.test.js index f6146b368d84..776b2969f931 100644 --- a/ghost/core/test/unit/server/services/webhooks/trigger.test.js +++ b/ghost/core/test/unit/server/services/webhooks/trigger.test.js @@ -61,15 +61,19 @@ describe('Webhook Service', function () { it('does not trigger payload handler when there are hooks registered for an event, but the custom integrations limit is active', async function () { const webhookModel = { - get: sinon.stub() + get: sinon.stub(), + related: sinon.stub() }; webhookModel.get .withArgs('event').returns(WEBHOOK_EVENT) .withArgs('target_url').returns(WEBHOOK_TARGET_URL); + webhookModel.related + .withArgs('integration').returns(null); + models.Webhook.findAllByEvent - .withArgs(WEBHOOK_EVENT, {context: {internal: true}}) + .withArgs(WEBHOOK_EVENT, {context: {internal: true}, withRelated: ['integration']}) .resolves({models: [webhookModel]}); limitService.isLimited.withArgs('customIntegrations').returns(true); @@ -77,7 +81,7 @@ describe('Webhook Service', function () { await webhookTrigger.trigger(WEBHOOK_EVENT); - assert.equal(models.Webhook.findAllByEvent.called, false); + assert.equal(models.Webhook.findAllByEvent.called, true); assert.equal(payload.called, false); assert.equal(request.called, false); }); diff --git a/ghost/core/test/utils/e2e-framework.js b/ghost/core/test/utils/e2e-framework.js index bcf727ae4845..6b2efae1d309 100644 --- a/ghost/core/test/utils/e2e-framework.js +++ b/ghost/core/test/utils/e2e-framework.js @@ -409,10 +409,12 @@ const getAgentsWithFrontend = async () => { }; }; -const insertWebhook = ({event, url}) => { +const insertWebhook = ({event, url, integrationType = undefined}) => { return fixtureUtils.fixtures.insertWebhook({ event: event, target_url: url + }, { + integrationType }); }; diff --git a/ghost/core/test/utils/fixture-utils.js b/ghost/core/test/utils/fixture-utils.js index 9f4e3f5b5607..b154b3558c07 100644 --- a/ghost/core/test/utils/fixture-utils.js +++ b/ghost/core/test/utils/fixture-utils.js @@ -426,10 +426,16 @@ const fixtures = { })); }, - insertWebhook: function (attributes) { + insertWebhook: function (attributes, options = {}) { + let integration = DataGenerator.forKnex.integrations[0]; + if (options.integrationType) { + integration = DataGenerator.forKnex.integrations.find( + props => props.type === options.integrationType + ); + } const webhook = DataGenerator.forKnex.createWebhook( Object.assign(attributes, { - integration_id: DataGenerator.forKnex.integrations[0].id + integration_id: integration.id }) ); From a2389607269a952f38c670e5c6aa5bfb4a04c670 Mon Sep 17 00:00:00 2001 From: Ghost CI <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 21 Nov 2024 09:19:08 +0000 Subject: [PATCH 3/3] v5.101.3 --- ghost/admin/package.json | 2 +- ghost/core/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ghost/admin/package.json b/ghost/admin/package.json index eae466200ac4..21e6b061b321 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -1,6 +1,6 @@ { "name": "ghost-admin", - "version": "5.101.2", + "version": "5.101.3", "description": "Ember.js admin client for Ghost", "author": "Ghost Foundation", "homepage": "http://ghost.org", diff --git a/ghost/core/package.json b/ghost/core/package.json index 690d79fe14b1..610a337894dd 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "5.101.2", + "version": "5.101.3", "description": "The professional publishing platform", "author": "Ghost Foundation", "homepage": "https://ghost.org",