From cab655289f94e25c33c0d80b8504a735db390174 Mon Sep 17 00:00:00 2001 From: Sag Date: Wed, 16 Oct 2024 10:42:35 +0200 Subject: [PATCH 1/3] Revert "Fixed fetching and storing bookmark card icons and thumbnails" (#21316) ref https://linear.app/tryghost/issue/ONC-433 ref https://linear.app/tryghost/issue/ENG-904 - the reverted commit (871d21acafafe0caafb6ac93dd54302d5beb6faf) caused a regression for recommendations: incoming recommendations were marked as deleted and did not render in Admin Settings anymore --- .../utils/serializers/output/mappers/index.js | 1 - .../serializers/output/mappers/oembed.js | 5 -- .../utils/serializers/output/oembed.js | 7 -- .../core/server/services/oembed/service.js | 3 +- ghost/core/test/e2e-api/admin/oembed.test.js | 66 ++-------------- ghost/oembed-service/lib/OEmbedService.js | 78 ++----------------- 6 files changed, 14 insertions(+), 146 deletions(-) delete mode 100644 ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/oembed.js diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/index.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/index.js index 8ccf99fd22..72e807acc3 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/index.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/index.js @@ -9,7 +9,6 @@ module.exports = { emailFailures: require('./email-failures'), images: require('./images'), integrations: require('./integrations'), - oembed: require('./oembed'), pages: require('./pages'), posts: require('./posts'), settings: require('./settings'), diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/oembed.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/oembed.js deleted file mode 100644 index 35fef8e62f..0000000000 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/oembed.js +++ /dev/null @@ -1,5 +0,0 @@ -const url = require('../utils/url'); - -module.exports = (path) => { - return url.forImage(path); -}; diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/oembed.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/oembed.js index 1a8f45f30b..aa2eb73abc 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/oembed.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/oembed.js @@ -1,15 +1,8 @@ const debug = require('@tryghost/debug')('api:endpoints:utils:serializers:output:oembed'); -const mappers = require('./mappers'); module.exports = { all(data, apiConfig, frame) { debug('all'); - if (data?.metadata?.thumbnail) { - data.metadata.thumbnail = mappers.oembed(data.metadata.thumbnail); - } - if (data?.metadata?.icon) { - data.metadata.icon = mappers.oembed(data.metadata.icon); - } frame.response = data; } }; diff --git a/ghost/core/core/server/services/oembed/service.js b/ghost/core/core/server/services/oembed/service.js index 25f55e88b1..77b7cd72d2 100644 --- a/ghost/core/core/server/services/oembed/service.js +++ b/ghost/core/core/server/services/oembed/service.js @@ -1,9 +1,8 @@ const config = require('../../../shared/config'); -const storage = require('../../adapters/storage'); const externalRequest = require('../../lib/request-external'); const OEmbed = require('@tryghost/oembed-service'); -const oembed = new OEmbed({config, externalRequest, storage}); +const oembed = new OEmbed({config, externalRequest}); const NFT = require('./NFTOEmbedProvider'); const nft = new NFT({ diff --git a/ghost/core/test/e2e-api/admin/oembed.test.js b/ghost/core/test/e2e-api/admin/oembed.test.js index 65be48d4ab..39a6184baf 100644 --- a/ghost/core/test/e2e-api/admin/oembed.test.js +++ b/ghost/core/test/e2e-api/admin/oembed.test.js @@ -6,8 +6,6 @@ const testUtils = require('../../utils/index'); const config = require('../../../core/shared/config/index'); const localUtils = require('./utils'); const {mockManager} = require('../../utils/e2e-framework'); -const oembed = require('../../../../core/core/server/services/oembed'); -const urlUtils = require('../../../core/shared/url-utils'); // for sinon stubs const dnsPromises = require('dns').promises; @@ -21,18 +19,9 @@ describe('Oembed API', function () { await localUtils.doAuth(request); }); - let processImageFromUrlStub; - beforeEach(function () { // ensure sure we're not network dependent mockManager.disableNetwork(); - processImageFromUrlStub = sinon.stub(oembed, 'processImageFromUrl'); - processImageFromUrlStub.callsFake(async function (imageUrl, imageType) { - if (imageUrl === 'http://example.com/bad-image') { - throw new Error('Failed to process image'); - } - return `/content/images/${imageType}/image-01.png`; - }); }); afterEach(function () { @@ -239,10 +228,15 @@ describe('Oembed API', function () { .get('/page-with-icon') .reply( 200, - 'TESTING', + 'TESTING', {'content-type': 'text/html'} ); + // Mock the icon URL to return 404 + nock('http://example.com/') + .head('/icon.svg') + .reply(404); + const url = encodeURIComponent(' http://example.com/page-with-icon\t '); // Whitespaces are to make sure urls are trimmed const res = await request.get(localUtils.API.getApiQuery(`oembed/?url=${url}&type=bookmark`)) .set('Origin', config.get('url')) @@ -258,54 +252,6 @@ describe('Oembed API', function () { }); }); - it('should fetch and store icons', async function () { - // Mock the page to contain a readable icon URL - const pageMock = nock('http://example.com') - .get('/page-with-icon') - .reply( - 200, - 'TESTING', - {'content-type': 'text/html'} - ); - - const url = encodeURIComponent(' http://example.com/page-with-icon\t '); // Whitespaces are to make sure urls are trimmed - const res = await request.get(localUtils.API.getApiQuery(`oembed/?url=${url}&type=bookmark`)) - .set('Origin', config.get('url')) - .expect('Content-Type', /json/) - .expect('Cache-Control', testUtils.cacheRules.private) - .expect(200); - - // Check that the icon URL mock was loaded - pageMock.isDone().should.be.true(); - - // Check that the substitute icon URL is returned in place of the original - res.body.metadata.icon.should.eql(`${urlUtils.urlFor('home', true)}content/images/icon/image-01.png`); - }); - - it('should fetch and store thumbnails', async function () { - // Mock the page to contain a readable icon URL - const pageMock = nock('http://example.com') - .get('/page-with-thumbnail') - .reply( - 200, - 'TESTING', - {'content-type': 'text/html'} - ); - - const url = encodeURIComponent(' http://example.com/page-with-thumbnail\t '); // Whitespaces are to make sure urls are trimmed - const res = await request.get(localUtils.API.getApiQuery(`oembed/?url=${url}&type=bookmark`)) - .set('Origin', config.get('url')) - .expect('Content-Type', /json/) - .expect('Cache-Control', testUtils.cacheRules.private) - .expect(200); - - // Check that the thumbnail URL mock was loaded - pageMock.isDone().should.be.true(); - - // Check that the substitute thumbnail URL is returned in place of the original - res.body.metadata.thumbnail.should.eql(`${urlUtils.urlFor('home', true)}content/images/thumbnail/image-01.png`); - }); - describe('with unknown provider', function () { it('fetches url and follows redirects', async function () { const redirectMock = nock('http://test.com/') diff --git a/ghost/oembed-service/lib/OEmbedService.js b/ghost/oembed-service/lib/OEmbedService.js index b58b4eaa9c..85e1330b71 100644 --- a/ghost/oembed-service/lib/OEmbedService.js +++ b/ghost/oembed-service/lib/OEmbedService.js @@ -4,7 +4,6 @@ const logging = require('@tryghost/logging'); const _ = require('lodash'); const charset = require('charset'); const iconv = require('iconv-lite'); -const path = require('path'); // Some sites block non-standard user agents so we need to mimic a typical browser const USER_AGENT = 'Mozilla/5.0 (compatible; Ghost/5.0; +https://ghost.org/)'; @@ -50,12 +49,6 @@ const findUrlWithProvider = (url) => { /** * @typedef {Object} IConfig * @prop {(key: string) => string} get - * @prop {(key: string) => string} getContentPath - */ - -/** - * @typedef {Object} IStorage - * @prop {(feature: string) => Object} getStorage */ /** @@ -73,12 +66,10 @@ class OEmbedService { * * @param {Object} dependencies * @param {IConfig} dependencies.config - * @param {IStorage} dependencies.storage * @param {IExternalRequest} dependencies.externalRequest */ - constructor({config, externalRequest, storage}) { + constructor({config, externalRequest}) { this.config = config; - this.storage = storage; /** @type {IExternalRequest} */ this.externalRequest = externalRequest; @@ -127,55 +118,6 @@ class OEmbedService { } } - /** - * Fetches the image buffer from a URL using fetch - * @param {String} imageUrl - URL of the image to fetch - * @returns {Promise} - Promise resolving to the image buffer - */ - async fetchImageBuffer(imageUrl) { - const response = await fetch(imageUrl); - - if (!response.ok) { - throw Error(`Failed to fetch image: ${response.statusText}`); - } - - const arrayBuffer = await response.arrayBuffer(); - - const buffer = Buffer.from(arrayBuffer); - return buffer; - } - - /** - * Process and store image from a URL - * @param {String} imageUrl - URL of the image to process - * @param {String} imageType - What is the image used for. Example - icon, thumbnail - * @returns {Promise} - URL where the image is stored - */ - async processImageFromUrl(imageUrl, imageType) { - // Fetch image buffer from the URL - const imageBuffer = await this.fetchImageBuffer(imageUrl); - const store = this.storage.getStorage('images'); - - // Extract file name from URL - const fileName = path.basename(new URL(imageUrl).pathname); - let ext = path.extname(fileName); - let name; - - if (ext) { - name = store.getSanitizedFileName(path.basename(fileName, ext)); - } else { - name = store.getSanitizedFileName(path.basename(fileName)); - } - - let targetDir = path.join(this.config.getContentPath('images'), imageType); - const uniqueFilePath = await store.generateUnique(targetDir, name, ext, 0); - const targetPath = path.join(imageType, path.basename(uniqueFilePath)); - - const imageStoredUrl = await store.saveRaw(imageBuffer, targetPath); - - return imageStoredUrl; - } - /** * @param {string} url * @param {Object} options @@ -329,20 +271,14 @@ class OEmbedService { }); } - await this.processImageFromUrl(metadata.icon, 'icon') - .then((processedImageUrl) => { - metadata.icon = processedImageUrl; - }).catch((err) => { + if (metadata.icon) { + try { + await this.externalRequest.head(metadata.icon); + } catch (err) { metadata.icon = 'https://static.ghost.org/v5.0.0/images/link-icon.svg'; logging.error(err); - }); - - await this.processImageFromUrl(metadata.thumbnail, 'thumbnail') - .then((processedImageUrl) => { - metadata.thumbnail = processedImageUrl; - }).catch((err) => { - logging.error(err); - }); + } + } return { version: '1.0', From 00f70a445b62610d878c710553a9806bf4c85bdb Mon Sep 17 00:00:00 2001 From: Sag Date: Wed, 16 Oct 2024 11:00:47 +0200 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20missing=20incoming?= =?UTF-8?q?=20recommendations=20(#21317)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ONC-433 - due to a regression introduced in commit 871d21a, incoming recommendations were not rendering in Admin Settings anymore, as they were marked as deleted - this commit updates the refresh logic of incoming recommendations on boot: previously deleted incoming recommendations are refetched, and if now available, restored - when a recommendation is restored, we don't send a staff email notification --- ghost/core/core/server/models/mention.js | 2 +- ghost/recommendations/src/IncomingRecommendationService.ts | 6 ++++-- .../test/IncomingRecommendationService.test.ts | 4 ++++ ghost/webmentions/lib/Mention.js | 1 - 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/ghost/core/core/server/models/mention.js b/ghost/core/core/server/models/mention.js index c5e849594a..269bf71f59 100644 --- a/ghost/core/core/server/models/mention.js +++ b/ghost/core/core/server/models/mention.js @@ -6,7 +6,7 @@ const Mention = ghostBookshelf.Model.extend({ deleted: false, verified: false }, - enforcedFilters() { + defaultFilters() { return 'deleted:false'; } }, { diff --git a/ghost/recommendations/src/IncomingRecommendationService.ts b/ghost/recommendations/src/IncomingRecommendationService.ts index 8d7c3b293b..60aca0c3d3 100644 --- a/ghost/recommendations/src/IncomingRecommendationService.ts +++ b/ghost/recommendations/src/IncomingRecommendationService.ts @@ -95,8 +95,10 @@ export class IncomingRecommendationService { } async #updateIncomingRecommendations() { - // Note: we also recheck recommendations that were not verified (verification could have failed) - const filter = this.#getMentionFilter(); + // We refresh all incoming recommendations, including: + // - recommendations that were not verified, as the verification could have failed + // - recommendations that were deleted previously. Implementation note: given that we have `deleted:false` as default filter in the Mention model, we need to override it here + const filter = this.#getMentionFilter() + '+deleted:[true,false]'; await this.#mentionsApi.refreshMentions({filter, limit: 100}); } diff --git a/ghost/recommendations/test/IncomingRecommendationService.test.ts b/ghost/recommendations/test/IncomingRecommendationService.test.ts index 37c949d956..8511022ee0 100644 --- a/ghost/recommendations/test/IncomingRecommendationService.test.ts +++ b/ghost/recommendations/test/IncomingRecommendationService.test.ts @@ -52,6 +52,10 @@ describe('IncomingRecommendationService', function () { await service.init(); clock.tick(1000 * 60 * 60 * 24); assert(refreshMentions.calledOnce); + assert(refreshMentions.calledWith({ + filter: `source:~$'/.well-known/recommendations.json'+deleted:[true,false]`, + limit: 100 + })); } finally { process.env.NODE_ENV = saved; } diff --git a/ghost/webmentions/lib/Mention.js b/ghost/webmentions/lib/Mention.js index 6fc029e133..2a2c25d2a6 100644 --- a/ghost/webmentions/lib/Mention.js +++ b/ghost/webmentions/lib/Mention.js @@ -33,7 +33,6 @@ module.exports = class Mention { // When an earlier mention is deleted, but then it gets verified again, we need to undelete it if (this.#deleted) { this.#deleted = false; - this.events.push(MentionCreatedEvent.create({mention: this})); } } From 42fcd385a67f113d2ae621516a670ceb4178e10e Mon Sep 17 00:00:00 2001 From: Ghost CI <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 16 Oct 2024 09:31:42 +0000 Subject: [PATCH 3/3] v5.96.2 --- 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 34d38d9ed2..a0c4ed3971 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -1,6 +1,6 @@ { "name": "ghost-admin", - "version": "5.96.1", + "version": "5.96.2", "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 2ca167c32a..9436b4c3ea 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "5.96.1", + "version": "5.96.2", "description": "The professional publishing platform", "author": "Ghost Foundation", "homepage": "https://ghost.org",