From b62afe616bcedd1af4a2fcc462c2e4048ac556f5 Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Tue, 12 Mar 2024 10:36:34 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20HTTP=20500=20responses?= =?UTF-8?q?=20when=20oembed=20endpoint=20receives=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/TryGhost/Product/issues/4237 - this fixes the fact that we return a HTTP 500 response when the oembed library receives an error, such as a 401 or 403 - includes special handling for cases where we want to return a slightly different error message - also adds unit tests for @tryghost/oembed-service package --- ghost/oembed-service/lib/OEmbedService.js | 17 ++-- ghost/oembed-service/test/hello.test.js | 7 -- .../test/oembed-service.test.js | 99 +++++++++++++++++++ 3 files changed, 109 insertions(+), 14 deletions(-) delete mode 100644 ghost/oembed-service/test/hello.test.js create mode 100644 ghost/oembed-service/test/oembed-service.test.js diff --git a/ghost/oembed-service/lib/OEmbedService.js b/ghost/oembed-service/lib/OEmbedService.js index 738ff6895dc2..7715cb1facb3 100644 --- a/ghost/oembed-service/lib/OEmbedService.js +++ b/ghost/oembed-service/lib/OEmbedService.js @@ -10,6 +10,7 @@ const messages = { noUrlProvided: 'No url provided.', insufficientMetadata: 'URL contains insufficient metadata.', unknownProvider: 'No provider found for supplied URL.', + unableToFetchOembed: 'Unable to fetch requested embed.', unauthorized: 'URL contains a private resource.' }; @@ -101,15 +102,17 @@ class OEmbedService { try { return await extract(url); } catch (err) { - if (err.message === 'Request failed with error code 401') { - throw new errors.UnauthorizedError({ - message: messages.unauthorized - }); - } else { - throw new errors.InternalServerError({ - message: err.message + if (err.message === 'Request failed with error code 401' || err.message === 'Request failed with error code 403') { + throw new errors.ValidationError({ + message: tpl(messages.unableToFetchOembed), + context: messages.unauthorized }); } + + throw new errors.ValidationError({ + message: tpl(messages.unableToFetchOembed), + context: err.message + }); } } diff --git a/ghost/oembed-service/test/hello.test.js b/ghost/oembed-service/test/hello.test.js deleted file mode 100644 index d9b992702844..000000000000 --- a/ghost/oembed-service/test/hello.test.js +++ /dev/null @@ -1,7 +0,0 @@ -const assert = require('assert/strict'); - -describe('Hello world', function () { - it('Runs a test', function () { - assert.ok(require('../index')); - }); -}); diff --git a/ghost/oembed-service/test/oembed-service.test.js b/ghost/oembed-service/test/oembed-service.test.js new file mode 100644 index 000000000000..1ada49a6fa75 --- /dev/null +++ b/ghost/oembed-service/test/oembed-service.test.js @@ -0,0 +1,99 @@ +const assert = require('assert/strict'); +const nock = require('nock'); + +const OembedService = require('../'); + +describe('oembed-service', function () { + /** @type {OembedService} */ + let oembedService; + + before(function () { + oembedService = new OembedService({}); + + nock.disableNetConnect(); + }); + + afterEach(function () { + nock.cleanAll(); + }); + + describe('known provider', function () { + it('should return data if successful', async function () { + nock('https://www.youtube.com') + .get('/oembed') + .query(true) + .reply(200, { + title: 'Test Title', + author_name: 'Test Author', + author_url: 'https://www.youtube.com/user/testauthor', + html: '' + }); + + const response = await oembedService.knownProvider('https://www.youtube.com/watch?v=1234'); + assert.equal(response.title, 'Test Title'); + assert.equal(response.author_name, 'Test Author'); + assert.equal(response.author_url, 'https://www.youtube.com/user/testauthor'); + assert.equal(response.html, ''); + }); + + it('should return a ValidationError if upstream 401s', async function () { + nock('https://www.youtube.com') + .get('/oembed') + .query(true) + .reply(401); + + try { + await oembedService.knownProvider('https://www.youtube.com/watch?v=1234'); + } catch (error) { + assert.equal(error.name, 'ValidationError'); + assert.equal(error.statusCode, 422); + assert.equal(error.context, 'URL contains a private resource.'); + } + }); + + it('should return a ValidationError if upstream 403s', async function () { + nock('https://www.youtube.com') + .get('/oembed') + .query(true) + .reply(403); + + try { + await oembedService.knownProvider('https://www.youtube.com/watch?v=1234'); + } catch (error) { + assert.equal(error.name, 'ValidationError'); + assert.equal(error.statusCode, 422); + assert.equal(error.context, 'URL contains a private resource.'); + } + }); + + it('should return a ValidationError if upstream 404s', async function () { + nock('https://www.youtube.com') + .get('/oembed') + .query(true) + .reply(404); + + try { + await oembedService.knownProvider('https://www.youtube.com/watch?v=1234'); + } catch (error) { + assert.equal(error.name, 'ValidationError'); + assert.equal(error.statusCode, 422); + assert.equal(error.context, 'Request failed with error code 404'); + } + }); + + it('should return a ValidationError if upstream 500s', async function () { + nock('https://www.youtube.com') + .get('/oembed') + .query(true) + .reply(500); + + try { + await oembedService.knownProvider('https://www.youtube.com/watch?v=1234'); + } catch (error) { + assert.equal(error.name, 'ValidationError'); + assert.equal(error.statusCode, 422); + assert.equal(error.context, 'Request failed with error code 500'); + } + }); + }); +});