From 6f1abc610a8b7b8ee95257909dc566edc62907e2 Mon Sep 17 00:00:00 2001 From: Naz Date: Thu, 24 Sep 2020 13:55:25 +1200 Subject: [PATCH] Added check for parent integration_id when creating a webhook refs https://github.com/TryGhost/Ghost/issues/12033 refs https://github.com/TryGhost/Ghost/issues/10567 - Creating a webhook without valid parent integration leads to orphaned webhook records, which shoult not ever happen - This scenario is only possible for non-integration authentication, because in case of integration being authenticated it's id is automatically assigned to creatd webhook --- core/server/api/canary/webhooks.js | 35 +++++++++++++++------- core/server/translations/en.json | 6 +++- test/api-acceptance/admin/webhooks_spec.js | 16 +++++++--- 3 files changed, 42 insertions(+), 15 deletions(-) diff --git a/core/server/api/canary/webhooks.js b/core/server/api/canary/webhooks.js index fd8f639c5f..50297a2592 100644 --- a/core/server/api/canary/webhooks.js +++ b/core/server/api/canary/webhooks.js @@ -14,20 +14,35 @@ module.exports = { options: [], data: [], permissions: true, - query(frame) { - return models.Webhook.getByEventAndTarget( + async query(frame) { + const isIntegrationRequest = frame.options.context && frame.options.context.integration && frame.options.context.integration.id; + + // NOTE: this check can be removed once `webhooks.integration_id` gets foreigh ke constraint (Ghost 4.0) + if (!isIntegrationRequest && frame.data.webhooks[0].integration_id) { + const integration = await models.Integration.findOne({id: frame.data.webhooks[0].integration_id}, {context: {internal: true}}); + + if (!integration) { + throw new errors.ValidationError({ + message: i18n.t('notices.data.validation.index.schemaValidationFailed', { + key: 'integration_id' + }), + context: i18n.t('errors.api.webhooks.nonExistingIntegrationIdProvided.context'), + help: i18n.t('errors.api.webhooks.nonExistingIntegrationIdProvided.help') + }); + } + } + + const webhook = await models.Webhook.getByEventAndTarget( frame.data.webhooks[0].event, frame.data.webhooks[0].target_url, frame.options - ).then((webhook) => { - if (webhook) { - return Promise.reject( - new errors.ValidationError({message: i18n.t('errors.api.webhooks.webhookAlreadyExists')}) - ); - } + ); - return models.Webhook.add(frame.data.webhooks[0], frame.options); - }); + if (webhook) { + throw new errors.ValidationError({message: i18n.t('errors.api.webhooks.webhookAlreadyExists')}); + } + + return models.Webhook.add(frame.data.webhooks[0], frame.options); } }, diff --git a/core/server/translations/en.json b/core/server/translations/en.json index 1f1a71d182..18f423ce4f 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -486,6 +486,10 @@ "noIntegrationIdProvided": { "context": "You may only create webhooks with 'integration_id' when using session authentication." }, + "nonExistingIntegrationIdProvided": { + "context": "'integration_id' value does not match any existing integration.", + "help": "Provide 'integration_id' of existing integration." + }, "webhookAlreadyExists": "Target URL has already been used for this event." }, "oembed": { @@ -711,7 +715,7 @@ "valueIsNotInteger": "Value in [{tableName}.{columnKey}] is not an integer.", "themeCannotBeActivated": "{themeName} cannot be activated because it is not currently installed.", "validationFailed": "Validation ({validationName}) failed for {key}", - "schemaValidationFailed": "Validation failed for '{key}'", + "schemaValidationFailed": "Validation failed for '{key}'.", "validationFailedTypes": { "isLength": "Value in [{tableName}.{key}] exceeds maximum length of {max} characters." } diff --git a/test/api-acceptance/admin/webhooks_spec.js b/test/api-acceptance/admin/webhooks_spec.js index 2c933741ce..1d86bf6db7 100644 --- a/test/api-acceptance/admin/webhooks_spec.js +++ b/test/api-acceptance/admin/webhooks_spec.js @@ -17,18 +17,18 @@ describe('Webhooks API', function () { request = supertest.agent(config.get('url')); }) .then(function () { - return localUtils.doAuth(request); + return localUtils.doAuth(request, 'integrations'); }); }); - it('Can creates a webhook', function () { + it('Can create a webhook', function () { let webhookData = { event: 'test.create', target_url: 'http://example.com/webhooks/test/extra/1', name: 'test', secret: 'thisissecret', api_version: 'v2', - integration_id: '12345' + integration_id: testUtils.DataGenerator.Content.integrations[0].id }; return request.post(localUtils.API.getApiQuery('webhooks/')) @@ -52,6 +52,14 @@ describe('Webhooks API', function () { jsonResponse.webhooks[0].integration_id.should.equal(webhookData.integration_id); should.not.exist(res.headers.location); + }) + .then(() => { + return request.post(localUtils.API.getApiQuery('webhooks/')) + .set('Origin', config.get('url')) + .send({webhooks: [webhookData]}) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422); }); }); @@ -114,7 +122,7 @@ describe('Webhooks API', function () { event: 'test.create', // a different target_url from above is needed to avoid an "already exists" error target_url: 'http://example.com/webhooks/test/2', - integration_id: '123423' + integration_id: testUtils.DataGenerator.Content.integrations[0].id }; // create the webhook that is to be deleted