From 3bc5eb8cf9fe394fbd5c180b5996317a6efa114a Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 20 Jun 2024 21:41:24 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20Bluesky=20URLs=20creatin?= =?UTF-8?q?g=20bookmarks=20rather=20than=20embeds=20(#20435)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Ghost/issues/20028 It's fairly common practice for oembed providers to skip some of the "required" fields from the oembed spec such as `height` when it doesn't make sense for the embeddable content, this was the case with Bluesky embeds which return `height: null` - removed validation for `height` being present in the response for it to be recognised as an embed because we don't use it anywhere and the validation is blocking otherwise valid embeds --- ghost/oembed-service/lib/OEmbedService.js | 6 +-- .../test/oembed-service.test.js | 38 ++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/ghost/oembed-service/lib/OEmbedService.js b/ghost/oembed-service/lib/OEmbedService.js index 7715cb1fac..977a67edd6 100644 --- a/ghost/oembed-service/lib/OEmbedService.js +++ b/ghost/oembed-service/lib/OEmbedService.js @@ -238,11 +238,11 @@ class OEmbedService { scraperResponse = await metascraper({ html, url, - // In development, allow non-standard tlds + // In development, allow non-standard TLDs validateUrl: this.config.get('env') !== 'development' }); } catch (err) { - // Log to avoid being blind to errors happenning in metascraper + // Log to avoid being blind to errors happening in metascraper logging.error(err); return this.unknownProvider(url); } @@ -334,7 +334,7 @@ class OEmbedService { if (oembed.type === 'photo' && !oembed.url) { return; } - if ((oembed.type === 'video' || oembed.type === 'rich') && (!oembed.html || !oembed.width || !oembed.height)) { + if ((oembed.type === 'video' || oembed.type === 'rich') && (!oembed.html || !oembed.width)) { return; } diff --git a/ghost/oembed-service/test/oembed-service.test.js b/ghost/oembed-service/test/oembed-service.test.js index 1ada49a6fa..d87d5fcdb9 100644 --- a/ghost/oembed-service/test/oembed-service.test.js +++ b/ghost/oembed-service/test/oembed-service.test.js @@ -1,5 +1,6 @@ const assert = require('assert/strict'); const nock = require('nock'); +const got = require('got'); const OembedService = require('../'); @@ -8,7 +9,12 @@ describe('oembed-service', function () { let oembedService; before(function () { - oembedService = new OembedService({}); + oembedService = new OembedService({ + config: {get() { + return true; + }}, + externalRequest: got + }); nock.disableNetConnect(); }); @@ -96,4 +102,34 @@ describe('oembed-service', function () { } }); }); + + describe('fetchOembedDataFromUrl', function () { + it('allows rich embeds to skip height field', async function () { + nock('https://www.example.com') + .get('/') + .query(true) + .reply(200, ``); + + nock('https://www.example.com') + .get('/oembed') + .query(true) + .reply(200, { + type: 'rich', + version: '1.0', + title: 'Test Title', + author_name: 'Test Author', + author_url: 'https://example.com/user/testauthor', + html: '', + width: 640, + height: null + }); + + const response = await oembedService.fetchOembedDataFromUrl('https://www.example.com'); + + assert.equal(response.title, 'Test Title'); + assert.equal(response.author_name, 'Test Author'); + assert.equal(response.author_url, 'https://example.com/user/testauthor'); + assert.equal(response.html, ''); + }); + }); });