From fee402a340c539291df889f2f389fab2b71901f5 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Fri, 3 Nov 2023 15:02:45 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20adding=20recommendation?= =?UTF-8?q?=20with=20URL=20redirect=20breaking=20one-click-subscribe=20(#1?= =?UTF-8?q?8863)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/TryGhost/Product/issues/4102 E.g. you recommend myghostsite.com, while that site redirects all traffic to [www.myghostsite.com](#): The redirect causes CORS issues, which means we cannot detect one-click-subscribe support. - This is fixed by moving the whole detection to the backend, which has the additional benefit that we can update it in the background without the frontend, and update it on every recommendation change. - This change also fixes existing recommendations by doing a check on boot (we can move this to a background job in the future). --- .../src/api/external-ghost-site.ts | 107 -------- apps/admin-x-settings/src/api/oembed.ts | 39 --- .../src/api/recommendations.ts | 29 +-- .../AddRecommendationModal.tsx | 69 ++---- .../membership/recommendations.test.ts | 6 +- .../server/api/endpoints/recommendations.js | 18 ++ .../RecommendationServiceWrapper.js | 15 +- .../server/web/api/endpoints/admin/routes.js | 1 + .../recommendations.test.js.snap | 33 +++ .../e2e-api/admin/recommendations.test.js | 39 +++ ghost/recommendations/src/Recommendation.ts | 7 +- .../src/RecommendationController.ts | 32 ++- .../src/RecommendationMetadataService.ts | 119 +++++++++ .../src/RecommendationService.ts | 71 +++++- ghost/recommendations/src/index.ts | 1 + .../test/Recommendation.test.ts | 24 ++ .../test/RecommendationController.test.ts | 78 ++++++ .../RecommendationMetadataService.test.ts | 215 ++++++++++++++++ .../test/RecommendationService.test.ts | 231 +++++++++++++++++- 19 files changed, 903 insertions(+), 231 deletions(-) delete mode 100644 apps/admin-x-settings/src/api/external-ghost-site.ts delete mode 100644 apps/admin-x-settings/src/api/oembed.ts create mode 100644 ghost/recommendations/src/RecommendationMetadataService.ts create mode 100644 ghost/recommendations/test/RecommendationMetadataService.test.ts diff --git a/apps/admin-x-settings/src/api/external-ghost-site.ts b/apps/admin-x-settings/src/api/external-ghost-site.ts deleted file mode 100644 index b6465eab84..0000000000 --- a/apps/admin-x-settings/src/api/external-ghost-site.ts +++ /dev/null @@ -1,107 +0,0 @@ -import {useFetchApi} from '../utils/api/hooks'; - -export type GhostSiteResponse = { - site: { - title: string, - description: string | null, - logo: URL | null, - icon: URL | null, - cover_image : URL | null, - allow_external_signup: boolean, - url: URL, - } -} - -export const apiUrl = (root: string, path: string, searchParams: Record = {}) => { - const url = new URL(`${root}${path}`, window.location.origin); - url.search = new URLSearchParams(searchParams).toString(); - return url.toString(); -}; - -export const useExternalGhostSite = () => { - const fetchApi = useFetchApi(); - const path = '/members/api/site'; - - return { - async query(root: string) { - // Remove trailing slash - root = root.replace(/\/$/, ''); - const url = apiUrl(root, path); - try { - const result = await fetchApi(url, { - method: 'GET', - credentials: 'omit', // Allow CORS wildcard, - timeout: 5000, - retry: false - }); - - // We need to validate all data types here for extra safety - if (typeof result !== 'object' || !result.site || typeof result.site !== 'object') { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result); - return null; - } - - // Temporary mapping (should get removed!) - // allow_self_signup was replaced by allow_external_signup - if (typeof result.site.allow_self_signup === 'boolean' && typeof result.site.allow_external_signup !== 'boolean') { - result.site.allow_external_signup = result.site.allow_self_signup; - } - - // We need to validate all data types here for extra safety - if (typeof result.site.title !== 'string' || typeof result.site.allow_external_signup !== 'boolean' || typeof result.site.url !== 'string') { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result); - return null; - } - - if (result.site.description !== null && typeof result.site.description !== 'string') { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result); - return null; - } - - if (result.site.logo !== null && typeof result.site.logo !== 'string') { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result); - return null; - } - - if (result.site.icon !== null && typeof result.site.icon !== 'string') { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result); - return null; - } - - if (result.site.cover_image !== null && typeof result.site.cover_image !== 'string') { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result); - return null; - } - - // Validate URLs - try { - return { - site: { - title: result.site.title, - description: result.site.description, - logo: result.site.logo ? new URL(result.site.logo) : null, - icon: result.site.icon ? new URL(result.site.icon) : null, - cover_image: result.site.cover_image ? new URL(result.site.cover_image) : null, - allow_external_signup: result.site.allow_external_signup, - url: new URL(result.site.url) - } - }; - } catch (e) { - // eslint-disable-next-line no-console - console.warn('Received invalid response from external Ghost site API', result, e); - return null; - } - } catch (e) { - // eslint-disable-next-line no-console - console.error(e); - return null; - } - } - }; -}; diff --git a/apps/admin-x-settings/src/api/oembed.ts b/apps/admin-x-settings/src/api/oembed.ts deleted file mode 100644 index 19a9954cb0..0000000000 --- a/apps/admin-x-settings/src/api/oembed.ts +++ /dev/null @@ -1,39 +0,0 @@ -import {apiUrl, useFetchApi} from '../utils/api/hooks'; - -export type OembedResponse = { - metadata: { - title: string | null, - description:string | null, - author: string | null, - publisher: string | null, - thumbnail: string | null, - icon: string | null - } -} - -export type OembedRequest = { - url: string, - type: 'mention' -} - -export const useGetOembed = () => { - const fetchApi = useFetchApi(); - const path = '/oembed/'; - - return { - async query(searchParams: OembedRequest) { - const url = apiUrl(path, searchParams); - try { - const result = await fetchApi(url, { - method: 'GET', - timeout: 5000 - }); - return result as OembedResponse; - } catch (e) { - // eslint-disable-next-line no-console - console.error(e); - return null; - } - } - }; -}; diff --git a/apps/admin-x-settings/src/api/recommendations.ts b/apps/admin-x-settings/src/api/recommendations.ts index adcb49b579..e203a864f9 100644 --- a/apps/admin-x-settings/src/api/recommendations.ts +++ b/apps/admin-x-settings/src/api/recommendations.ts @@ -1,5 +1,5 @@ import {InfiniteData} from '@tanstack/react-query'; -import {Meta, apiUrl, createInfiniteQuery, createMutation, useFetchApi} from '../utils/api/hooks'; +import {Meta, createInfiniteQuery, createMutation} from '../utils/api/hooks'; export type Recommendation = { id: string @@ -77,28 +77,11 @@ export const useAddRecommendation = createMutation { - const fetchApi = useFetchApi(); - const path = '/recommendations/'; - - return { - async query(url: URL): Promise { - const urlFilter = `url:~'${url.host.replace('www.', '')}${url.pathname.replace(/\/$/, '')}'`; - const endpoint = apiUrl(path, {filter: urlFilter}); - try { - const result = await fetchApi(endpoint, { - method: 'GET', - timeout: 5000 - }); - return result as RecommendationResponseType; - } catch (e) { - // eslint-disable-next-line no-console - console.error(e); - return null; - } - } - }; -}; +export const useCheckRecommendation = createMutation({ + method: 'POST', + path: () => '/recommendations/check/', + body: url => ({recommendations: [{url: url.toString()}]}) +}); export type IncomingRecommendation = { id: string diff --git a/apps/admin-x-settings/src/components/settings/membership/recommendations/AddRecommendationModal.tsx b/apps/admin-x-settings/src/components/settings/membership/recommendations/AddRecommendationModal.tsx index 773a7e6995..e8b4e06084 100644 --- a/apps/admin-x-settings/src/components/settings/membership/recommendations/AddRecommendationModal.tsx +++ b/apps/admin-x-settings/src/components/settings/membership/recommendations/AddRecommendationModal.tsx @@ -7,14 +7,12 @@ import TextField from '../../../../admin-x-ds/global/form/TextField'; import useForm, {ErrorMessages} from '../../../../hooks/useForm'; import useRouting from '../../../../hooks/useRouting'; import {AlreadyExistsError} from '../../../../utils/errors'; -import {EditOrAddRecommendation, RecommendationResponseType, useGetRecommendationByUrl} from '../../../../api/recommendations'; +import {EditOrAddRecommendation, useCheckRecommendation} from '../../../../api/recommendations'; import {LoadingIndicator} from '../../../../admin-x-ds/global/LoadingIndicator'; import {RoutingModalProps} from '../../../providers/RoutingProvider'; -import {arePathsEqual, trimSearchAndHash} from '../../../../utils/url'; import {dismissAllToasts, showToast} from '../../../../admin-x-ds/global/Toast'; import {formatUrl} from '../../../../admin-x-ds/global/form/URLTextField'; -import {useExternalGhostSite} from '../../../../api/external-ghost-site'; -import {useGetOembed} from '../../../../api/oembed'; +import {trimSearchAndHash} from '../../../../utils/url'; interface AddRecommendationModalProps { recommendation?: EditOrAddRecommendation, @@ -45,9 +43,7 @@ const AddRecommendationModal: React.FC 0) { - const existing = recommendations.find(r => arePathsEqual(r.url, validatedUrl.toString())); - - if (existing) { - throw new AlreadyExistsError('A recommendation with this URL already exists.'); - } - } - - // Check if it's a Ghost site or not: - // 1. Check the full path first. This is the most common use case, and also helps to cover Ghost sites that are hosted on a subdirectory - // 2. If needed, check the origin. This helps to cover cases where the recommendation URL is a subpage or a post URL of the Ghost site - let externalGhostSite = null; - externalGhostSite = await queryExternalGhostSite(validatedUrl.toString()); - - if (!externalGhostSite && validatedUrl.pathname !== '' && validatedUrl.pathname !== '/') { - externalGhostSite = await queryExternalGhostSite(validatedUrl.origin); - } - // Use the hostname as fallback title const defaultTitle = validatedUrl.hostname.replace('www.', ''); @@ -100,25 +76,28 @@ const AddRecommendationModal: React.FC { test('can add a recommendation', async ({page}) => { const {lastApiRequests} = await mockApi({page, requests: { ...globalDataRequests, - getRecommendationByUrl: {method: 'GET', path: '/recommendations/?filter=url%3A%7E%27example.com%2Fa-cool-website%27', response: {recommendations: [], meta: {}}}, + checkRecommendation: {method: 'POST', path: '/recommendations/check/', response: {recommendations: [{url: '', one_click_subscribe: true}], meta: {}}}, addRecommendation: {method: 'POST', path: '/recommendations/', response: {}} }}); @@ -74,7 +74,7 @@ test.describe('Recommendations', async () => { {excerpt: null, favicon: null, featured_image: null, - one_click_subscribe: false, + one_click_subscribe: true, description: 'This is a description', title: 'This is a title', url: 'https://example.com/a-cool-website'} @@ -85,7 +85,7 @@ test.describe('Recommendations', async () => { test('errors when adding an existing URL', async ({page}) => { await mockApi({page, requests: { ...globalDataRequests, - getRecommendationByUrl: {method: 'GET', path: '/recommendations/?filter=url%3A%7E%27recommendation1.com%27', response: responseFixtures.recommendations} + checkRecommendation: {method: 'POST', path: '/recommendations/check/', response: {recommendations: [{url: 'https://recommendation1.com', one_click_subscribe: true, id: 'exists'}], meta: {}}} }}); await page.goto('/'); diff --git a/ghost/core/core/server/api/endpoints/recommendations.js b/ghost/core/core/server/api/endpoints/recommendations.js index 368156f120..afad5b4d65 100644 --- a/ghost/core/core/server/api/endpoints/recommendations.js +++ b/ghost/core/core/server/api/endpoints/recommendations.js @@ -48,6 +48,24 @@ module.exports = { } }, + /** + * Fetch metadata for a recommendation URL + */ + check: { + headers: { + cacheInvalidate: true + }, + options: [], + validation: {}, + permissions: { + // Everyone who has add permissions, can 'check' + method: 'add' + }, + async query(frame) { + return await recommendations.controller.check(frame); + } + }, + edit: { headers: { cacheInvalidate: true diff --git a/ghost/core/core/server/services/recommendations/RecommendationServiceWrapper.js b/ghost/core/core/server/services/recommendations/RecommendationServiceWrapper.js index 6c00e54dc8..dd9bf714a6 100644 --- a/ghost/core/core/server/services/recommendations/RecommendationServiceWrapper.js +++ b/ghost/core/core/server/services/recommendations/RecommendationServiceWrapper.js @@ -50,6 +50,7 @@ class RecommendationServiceWrapper { const sentry = require('../../../shared/sentry'); const settings = require('../settings'); const RecommendationEnablerService = require('./RecommendationEnablerService'); + const { BookshelfRecommendationRepository, RecommendationService, @@ -58,7 +59,8 @@ class RecommendationServiceWrapper { BookshelfClickEventRepository, IncomingRecommendationController, IncomingRecommendationService, - IncomingRecommendationEmailRenderer + IncomingRecommendationEmailRenderer, + RecommendationMetadataService } = require('@tryghost/recommendations'); const mentions = require('../mentions'); @@ -87,13 +89,22 @@ class RecommendationServiceWrapper { sentry }); + const oembedService = require('../oembed'); + const externalRequest = require('../../../server/lib/request-external.js'); + + const recommendationMetadataService = new RecommendationMetadataService({ + oembedService, + externalRequest + }); + this.service = new RecommendationService({ repository: this.repository, recommendationEnablerService, wellknownService, mentionSendingService: mentions.sendingService, clickEventRepository: this.clickEventRepository, - subscribeEventRepository: this.subscribeEventRepository + subscribeEventRepository: this.subscribeEventRepository, + recommendationMetadataService }); const mail = require('../mail'); diff --git a/ghost/core/core/server/web/api/endpoints/admin/routes.js b/ghost/core/core/server/web/api/endpoints/admin/routes.js index 8dfd990d3e..4fb7177954 100644 --- a/ghost/core/core/server/web/api/endpoints/admin/routes.js +++ b/ghost/core/core/server/web/api/endpoints/admin/routes.js @@ -351,6 +351,7 @@ module.exports = function apiRoutes() { router.get('/recommendations', mw.authAdminApi, http(api.recommendations.browse)); router.get('/recommendations/:id', mw.authAdminApi, http(api.recommendations.read)); router.post('/recommendations', mw.authAdminApi, http(api.recommendations.add)); + router.post('/recommendations/check', mw.authAdminApi, http(api.recommendations.check)); router.put('/recommendations/:id', mw.authAdminApi, http(api.recommendations.edit)); router.del('/recommendations/:id', mw.authAdminApi, http(api.recommendations.destroy)); diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap index d6cf409117..76a2963aab 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap @@ -2184,6 +2184,39 @@ Object { } `; +exports[`Recommendations Admin API check Can check a recommendation url 1: [body] 1`] = ` +Object { + "recommendations": Array [ + Object { + "created_at": null, + "description": null, + "excerpt": "Because dogs are cute", + "favicon": "https://dogpictures.com/favicon.ico", + "featured_image": "https://dogpictures.com/dog.jpg", + "id": null, + "one_click_subscribe": true, + "title": "Dog Pictures", + "updated_at": null, + "url": "https://dogpictures.com/", + }, + ], +} +`; + +exports[`Recommendations Admin API check Can check a recommendation url 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "304", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-cache-invalidate": "/*", + "x-powered-by": "Express", +} +`; + exports[`Recommendations Admin API delete Can delete recommendation 1: [body] 1`] = `Object {}`; exports[`Recommendations Admin API delete Can delete recommendation 2: [headers] 1`] = ` diff --git a/ghost/core/test/e2e-api/admin/recommendations.test.js b/ghost/core/test/e2e-api/admin/recommendations.test.js index 8bf2bb29c1..699aa84c10 100644 --- a/ghost/core/test/e2e-api/admin/recommendations.test.js +++ b/ghost/core/test/e2e-api/admin/recommendations.test.js @@ -3,6 +3,7 @@ const {anyObjectId, anyErrorId, anyISODateTime, anyContentVersion, anyLocationFo const assert = require('assert/strict'); const recommendationsService = require('../../../core/server/services/recommendations'); const {Recommendation, ClickEvent, SubscribeEvent} = require('@tryghost/recommendations'); +const nock = require('nock'); async function addDummyRecommendation(i = 0) { const recommendation = Recommendation.create({ @@ -666,6 +667,44 @@ describe('Recommendations Admin API', function () { }); }); + describe('check', function () { + it('Can check a recommendation url', async function () { + nock('https://dogpictures.com') + .get('/members/api/site') + .reply(200, { + site: { + title: 'Dog Pictures', + description: 'Because dogs are cute', + cover_image: 'https://dogpictures.com/dog.jpg', + icon: 'https://dogpictures.com/favicon.ico', + allow_external_signup: true + } + }); + + const {body} = await agent.post('recommendations/check/') + .body({ + recommendations: [{ + url: 'https://dogpictures.com' + }] + }) + .expectStatus(200) + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }) + .matchBodySnapshot({}); + + // Check everything is set correctly + assert.equal(body.recommendations[0].title, 'Dog Pictures'); + assert.equal(body.recommendations[0].url, 'https://dogpictures.com/'); + assert.equal(body.recommendations[0].description, null); + assert.equal(body.recommendations[0].excerpt, 'Because dogs are cute'); + assert.equal(body.recommendations[0].featured_image, 'https://dogpictures.com/dog.jpg'); + assert.equal(body.recommendations[0].favicon, 'https://dogpictures.com/favicon.ico'); + assert.equal(body.recommendations[0].one_click_subscribe, true); + }); + }); + describe('delete', function () { it('Can delete recommendation', async function () { const id = await addDummyRecommendation(); diff --git a/ghost/recommendations/src/Recommendation.ts b/ghost/recommendations/src/Recommendation.ts index 8b06805c80..4ff593af7c 100644 --- a/ghost/recommendations/src/Recommendation.ts +++ b/ghost/recommendations/src/Recommendation.ts @@ -181,13 +181,18 @@ export class Recommendation { edit(properties: EditRecommendation) { // Delete undefined properties const newProperties = this.plain; + let didChange = false; for (const key of Object.keys(properties) as (keyof EditRecommendation)[]) { - if (Object.prototype.hasOwnProperty.call(properties, key) && properties[key] !== undefined) { + if (Object.prototype.hasOwnProperty.call(properties, key) && properties[key] !== undefined && properties[key] !== newProperties[key]) { (newProperties as Record)[key] = properties[key] as unknown; + didChange = true; } } + if (!didChange) { + return; + } newProperties.updatedAt = new Date(); const created = Recommendation.create(newProperties); diff --git a/ghost/recommendations/src/RecommendationController.ts b/ghost/recommendations/src/RecommendationController.ts index 97af8adb8a..e9505a771a 100644 --- a/ghost/recommendations/src/RecommendationController.ts +++ b/ghost/recommendations/src/RecommendationController.ts @@ -65,6 +65,22 @@ export class RecommendationController { ); } + /** + * Given a recommendation URL, returns either an existing recommendation with that url and updated metadata, + * or the metadata from that URL as if it would create a new one (without creating a new one) + * + * This can be used in the frontend when creating a new recommendation (duplication checking + showing a preview before saving) + */ + async check(frame: Frame) { + const data = new UnsafeData(frame.data); + const recommendation = data.key('recommendations').index(0); + const url = recommendation.key('url').url; + + return this.#serialize( + [await this.service.checkRecommendation(url)] + ); + } + async edit(frame: Frame) { const options = new UnsafeData(frame.options); const data = new UnsafeData(frame.data); @@ -201,19 +217,19 @@ export class RecommendationController { return null; } - #serialize(recommendations: RecommendationPlain[], meta?: any) { + #serialize(recommendations: Partial[], meta?: any) { return { data: recommendations.map((entity) => { const d = { - id: entity.id, - title: entity.title, - description: entity.description, - excerpt: entity.excerpt, + id: entity.id ?? null, + title: entity.title ?? null, + description: entity.description ?? null, + excerpt: entity.excerpt ?? null, featured_image: entity.featuredImage?.toString() ?? null, favicon: entity.favicon?.toString() ?? null, - url: entity.url.toString(), - one_click_subscribe: entity.oneClickSubscribe, - created_at: entity.createdAt.toISOString(), + url: entity.url?.toString() ?? null, + one_click_subscribe: entity.oneClickSubscribe ?? null, + created_at: entity.createdAt?.toISOString() ?? null, updated_at: entity.updatedAt?.toISOString() ?? null, count: entity.clickCount !== undefined || entity.subscriberCount !== undefined ? { clicks: entity.clickCount, diff --git a/ghost/recommendations/src/RecommendationMetadataService.ts b/ghost/recommendations/src/RecommendationMetadataService.ts new file mode 100644 index 0000000000..9d6e5f947c --- /dev/null +++ b/ghost/recommendations/src/RecommendationMetadataService.ts @@ -0,0 +1,119 @@ +type OembedMetadata = { + version: '1.0', + type: Type, + url: string, + metadata: { + title: string|null, + description: string|null, + publisher: string|null, + author: string|null, + thumbnail: string|null, + icon: string|null + }, + body?: Type extends 'mention' ? string : unknown, + contentType?: Type extends 'mention' ? string : unknown +} + +type OEmbedService = { + fetchOembedDataFromUrl(url: string, type: Type, options?: {timeout?: number}): Promise> +} + +type ExternalRequest = { + get(url: string, options: object): Promise<{statusCode: number, body: string}> +} + +export type RecommendationMetadata = { + title: string|null, + excerpt: string|null, + featuredImage: URL|null, + favicon: URL|null, + oneClickSubscribe: boolean +} + +export class RecommendationMetadataService { + #oembedService: OEmbedService; + #externalRequest: ExternalRequest; + + constructor(dependencies: {oembedService: OEmbedService, externalRequest: ExternalRequest}) { + this.#oembedService = dependencies.oembedService; + this.#externalRequest = dependencies.externalRequest; + } + + async #fetchJSON(url: URL, options?: {timeout?: number}) { + // default content type is application/x-www-form-encoded which is what we need for the webmentions spec + const response = await this.#externalRequest.get(url.toString(), { + throwHttpErrors: false, + maxRedirects: 10, + followRedirect: true, + timeout: 15000, + retry: { + // Only retry on network issues, or specific HTTP status codes + limit: 3 + }, + ...options + }); + + if (response.statusCode >= 200 && response.statusCode < 300) { + try { + return JSON.parse(response.body); + } catch (e) { + return undefined; + } + } + } + + #castUrl(url: string|null|undefined): URL|null { + if (!url) { + return null; + } + try { + return new URL(url); + } catch (e) { + return null; + } + } + + async fetch(url: URL, options: {timeout: number} = {timeout: 5000}): Promise { + // Make sure url path ends with a slash (urls should be resolved relative to the path) + if (!url.pathname.endsWith('/')) { + url.pathname += '/'; + } + + // 1. Check if it is a Ghost site + let ghostSiteData = await this.#fetchJSON( + new URL('members/api/site', url), + options + ); + + if (!ghostSiteData && url.pathname !== '' && url.pathname !== '/') { + // Try root relative URL + ghostSiteData = await this.#fetchJSON( + new URL('members/api/site', url.origin), + options + ); + } + + if (ghostSiteData && typeof ghostSiteData === 'object' && ghostSiteData.site && typeof ghostSiteData.site === 'object') { + // Check if the Ghost site returns allow_external_signup, otherwise it is an old Ghost version that returns unreliable data + if (typeof ghostSiteData.site.allow_external_signup === 'boolean') { + return { + title: ghostSiteData.site.title || null, + excerpt: ghostSiteData.site.description || null, + featuredImage: this.#castUrl(ghostSiteData.site.cover_image), + favicon: this.#castUrl(ghostSiteData.site.icon || ghostSiteData.site.logo), + oneClickSubscribe: !!ghostSiteData.site.allow_external_signup + }; + } + } + + // Use the oembed service to fetch metadata + const oembed = await this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention'); + return { + title: oembed?.metadata?.title || null, + excerpt: oembed?.metadata?.description || null, + featuredImage: this.#castUrl(oembed?.metadata?.thumbnail), + favicon: this.#castUrl(oembed?.metadata?.icon), + oneClickSubscribe: false + }; + } +} diff --git a/ghost/recommendations/src/RecommendationService.ts b/ghost/recommendations/src/RecommendationService.ts index dcd9f0373c..8326d54be8 100644 --- a/ghost/recommendations/src/RecommendationService.ts +++ b/ghost/recommendations/src/RecommendationService.ts @@ -8,6 +8,7 @@ import {AddRecommendation, Recommendation, RecommendationPlain} from './Recommen import {RecommendationRepository} from './RecommendationRepository'; import {SubscribeEvent} from './SubscribeEvent'; import {WellknownService} from './WellknownService'; +import {RecommendationMetadataService} from './RecommendationMetadataService'; type MentionSendingService = { sendAll(options: {url: URL, links: URL[]}): Promise @@ -30,6 +31,7 @@ export class RecommendationService { wellknownService: WellknownService; mentionSendingService: MentionSendingService; recommendationEnablerService: RecommendationEnablerService; + recommendationMetadataService: RecommendationMetadataService; constructor(deps: { repository: RecommendationRepository, @@ -37,7 +39,8 @@ export class RecommendationService { subscribeEventRepository: InMemoryRepository, wellknownService: WellknownService, mentionSendingService: MentionSendingService, - recommendationEnablerService: RecommendationEnablerService + recommendationEnablerService: RecommendationEnablerService, + recommendationMetadataService: RecommendationMetadataService }) { this.repository = deps.repository; this.wellknownService = deps.wellknownService; @@ -45,11 +48,37 @@ export class RecommendationService { this.recommendationEnablerService = deps.recommendationEnablerService; this.clickEventRepository = deps.clickEventRepository; this.subscribeEventRepository = deps.subscribeEventRepository; + this.recommendationMetadataService = deps.recommendationMetadataService; } async init() { const recommendations = await this.#listRecommendations(); await this.updateWellknown(recommendations); + + // Do a slow update of all the recommendation metadata (keeping logo up to date, one-click-subscribe, etc.) + // We better move this to a job in the future + if (!process.env.NODE_ENV?.startsWith('test')) { + setTimeout(async () => { + try { + await this.updateAllRecommendationsMetadata(); + } catch (e) { + logging.error('[Recommendations] Failed to update all recommendations metadata on boot', e); + } + }, 2 * 60 * 1000 + Math.random() * 5 * 60 * 1000); + } + } + + async updateAllRecommendationsMetadata() { + const recommendations = await this.#listRecommendations(); + logging.info('[Recommendations] Updating recommendations metadata'); + for (const recommendation of recommendations) { + try { + await this._updateRecommendationMetadata(recommendation); + await this.repository.save(recommendation); + } catch (e) { + logging.error('[Recommendations] Failed to save updated metadata for recommendation ' + recommendation.url.toString(), e); + } + } } async updateWellknown(recommendations: Recommendation[]) { @@ -112,6 +141,45 @@ export class RecommendationService { return recommendation.plain; } + async checkRecommendation(url: URL): Promise> { + // If a recommendation with this URL already exists, return it, but with updated metadata + const existing = await this.repository.getByUrl(url); + if (existing) { + this._updateRecommendationMetadata(existing); + await this.repository.save(existing); + return existing.plain; + } + + const metadata = await this.recommendationMetadataService.fetch(url); + return { + url: url, + title: metadata.title ?? undefined, + excerpt: metadata.excerpt ?? undefined, + featuredImage: metadata.featuredImage ?? undefined, + favicon: metadata.favicon ?? undefined, + oneClickSubscribe: !!metadata.oneClickSubscribe + }; + } + + async _updateRecommendationMetadata(recommendation: Recommendation) { + // Fetch data + try { + const metadata = await this.recommendationMetadataService.fetch(recommendation.url); + + // Set null values to undefined so we don't trigger an update + recommendation.edit({ + // Don't set title if it's already set on the recommendation + title: recommendation.title ? undefined : (metadata.title ?? undefined), + excerpt: metadata.excerpt ?? undefined, + featuredImage: metadata.featuredImage ?? undefined, + favicon: metadata.favicon ?? undefined, + oneClickSubscribe: !!metadata.oneClickSubscribe + }); + } catch (e) { + logging.error('[Recommendations] Failed to update metadata for recommendation ' + recommendation.url.toString(), e); + } + } + async editRecommendation(id: string, recommendationEdit: Partial): Promise { // Check if it exists const existing = await this.repository.getById(id); @@ -122,6 +190,7 @@ export class RecommendationService { } existing.edit(recommendationEdit); + await this._updateRecommendationMetadata(existing); await this.repository.save(existing); const recommendations = await this.#listRecommendations(); diff --git a/ghost/recommendations/src/index.ts b/ghost/recommendations/src/index.ts index 784a6c80b3..482b9d62ae 100644 --- a/ghost/recommendations/src/index.ts +++ b/ghost/recommendations/src/index.ts @@ -12,3 +12,4 @@ export * from './BookshelfSubscribeEventRepository'; export * from './IncomingRecommendationController'; export * from './IncomingRecommendationService'; export * from './IncomingRecommendationEmailRenderer'; +export * from './RecommendationMetadataService'; diff --git a/ghost/recommendations/test/Recommendation.test.ts b/ghost/recommendations/test/Recommendation.test.ts index e3afbc541e..a88e645d70 100644 --- a/ghost/recommendations/test/Recommendation.test.ts +++ b/ghost/recommendations/test/Recommendation.test.ts @@ -173,6 +173,30 @@ describe('Recommendation', function () { assert.notEqual(recommendation.updatedAt, null); }); + it('does not change updatedAt if nothing changed', function () { + const recommendation = Recommendation.create({ + title: 'Test', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + url: 'https://example.com', + oneClickSubscribe: false, + createdAt: new Date('2021-01-01T00:00:05Z'), + updatedAt: null + }); + assert.equal(recommendation.updatedAt, null); + + recommendation.edit({ + title: 'Test', + url: undefined + } as any); + + assert.equal(recommendation.title, 'Test'); + assert.equal(recommendation.url.toString(), 'https://example.com/'); + assert.equal(recommendation.updatedAt, null); + }); + it('can not edit unknown properties', function () { const recommendation = Recommendation.create({ title: 'Test', diff --git a/ghost/recommendations/test/RecommendationController.test.ts b/ghost/recommendations/test/RecommendationController.test.ts index 31bdb09d51..fb27a22453 100644 --- a/ghost/recommendations/test/RecommendationController.test.ts +++ b/ghost/recommendations/test/RecommendationController.test.ts @@ -156,6 +156,84 @@ describe('RecommendationController', function () { }); }); + describe('check', function () { + it('should return url metadata', async function () { + service.checkRecommendation = async (url) => { + return { + excerpt: 'Updated excerpt', + url + }; + }; + + const result = await controller.check({ + data: { + recommendations: [ + { + url: 'https://example.com/' + } + ] + }, + options: {}, + user: {} + }); + + assert.deepEqual(result, { + data: [{ + excerpt: 'Updated excerpt', + created_at: null, + updated_at: null, + description: null, + favicon: null, + featured_image: null, + id: null, + one_click_subscribe: null, + title: null, + url: 'https://example.com/', + count: undefined + }], + meta: undefined + }); + }); + + it('should serialize undefined url', async function () { + service.checkRecommendation = async () => { + return { + excerpt: 'Updated excerpt', + url: undefined + }; + }; + + const result = await controller.check({ + data: { + recommendations: [ + { + url: 'https://example.com/' + } + ] + }, + options: {}, + user: {} + }); + + assert.deepEqual(result, { + data: [{ + excerpt: 'Updated excerpt', + created_at: null, + updated_at: null, + description: null, + favicon: null, + featured_image: null, + id: null, + one_click_subscribe: null, + title: null, + url: null, + count: undefined + }], + meta: undefined + }); + }); + }); + describe('edit', function () { it('should edit a recommendation', async function () { service.editRecommendation = async (id, edit) => { diff --git a/ghost/recommendations/test/RecommendationMetadataService.test.ts b/ghost/recommendations/test/RecommendationMetadataService.test.ts new file mode 100644 index 0000000000..dd58cbffea --- /dev/null +++ b/ghost/recommendations/test/RecommendationMetadataService.test.ts @@ -0,0 +1,215 @@ +import assert from 'assert/strict'; +import got from 'got'; +import nock from 'nock'; +import {RecommendationMetadataService} from '../src'; +import sinon from 'sinon'; + +describe('RecommendationMetadataService', function () { + let service: RecommendationMetadataService; + let fetchOembedDataFromUrl: sinon.SinonStub; + + beforeEach(function () { + nock.disableNetConnect(); + fetchOembedDataFromUrl = sinon.stub().resolves({ + version: '1.0', + type: 'webmention', + metadata: { + title: 'Oembed Site Title', + description: 'Oembed Site Description', + publisher: 'Oembed Site Publisher', + author: 'Oembed Site Author', + thumbnail: 'https://example.com/oembed/thumbnail.png', + icon: 'https://example.com/oembed/icon.png' + } + }); + service = new RecommendationMetadataService({ + externalRequest: got, + oembedService: { + fetchOembedDataFromUrl + } + }); + }); + + afterEach(function () { + nock.cleanAll(); + }); + + it('Returns metadata from the Ghost site', async function () { + nock('https://exampleghostsite.com') + .get('/subdirectory/members/api/site') + .reply(200, { + site: { + title: 'Example Ghost Site', + description: 'Example Ghost Site Description', + cover_image: 'https://exampleghostsite.com/cover.png', + icon: 'https://exampleghostsite.com/favicon.ico', + allow_external_signup: true + } + }); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + assert.deepEqual(metadata, { + title: 'Example Ghost Site', + excerpt: 'Example Ghost Site Description', + featuredImage: new URL('https://exampleghostsite.com/cover.png'), + favicon: new URL('https://exampleghostsite.com/favicon.ico'), + oneClickSubscribe: true + }); + }); + + it('Nulifies empty data from Ghost site response', async function () { + nock('https://exampleghostsite.com') + .get('/subdirectory/members/api/site') + .reply(200, { + site: { + title: '', + description: '', + cover_image: '', + icon: '', + allow_external_signup: false + } + }); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + assert.deepEqual(metadata, { + title: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + }); + + it('Ignores ghost site if allow_external_signup is missing', async function () { + nock('https://exampleghostsite.com') + .get('/members/api/site') + .reply(200, { + site: { + title: '', + description: '', + cover_image: '', + icon: '' + } + }); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com')); + assert.deepEqual(metadata, { + // oembed + title: 'Oembed Site Title', + excerpt: 'Oembed Site Description', + featuredImage: new URL('https://example.com/oembed/thumbnail.png'), + favicon: new URL('https://example.com/oembed/icon.png'), + oneClickSubscribe: false + }); + }); + + it('Returns metadata from the Ghost site root if not found on subdirectory', async function () { + nock('https://exampleghostsite.com') + .get('/subdirectory/members/api/site') + .reply(404, {}); + + nock('https://exampleghostsite.com') + .get('/members/api/site') + .reply(200, { + site: { + title: 'Example Ghost Site', + description: 'Example Ghost Site Description', + cover_image: 'https://exampleghostsite.com/cover.png', + icon: 'https://exampleghostsite.com/favicon.ico', + allow_external_signup: true + } + }); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + assert.deepEqual(metadata, { + title: 'Example Ghost Site', + excerpt: 'Example Ghost Site Description', + featuredImage: new URL('https://exampleghostsite.com/cover.png'), + favicon: new URL('https://exampleghostsite.com/favicon.ico'), + oneClickSubscribe: true + }); + }); + + it('Skips ghost metadata if json is invalid', async function () { + nock('https://exampleghostsite.com') + .get('/subdirectory/members/api/site') + .reply(200, 'invalidjson'); + + nock('https://exampleghostsite.com') + .get('/members/api/site') + .reply(200, 'invalidjson'); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + assert.deepEqual(metadata, { + title: 'Oembed Site Title', + excerpt: 'Oembed Site Description', + featuredImage: new URL('https://example.com/oembed/thumbnail.png'), + favicon: new URL('https://example.com/oembed/icon.png'), + oneClickSubscribe: false + }); + }); + + it('Ignores invalid urls', async function () { + nock('https://exampleghostsite.com') + .get('/subdirectory/members/api/site') + .reply(404, 'invalidjson'); + + nock('https://exampleghostsite.com') + .get('/members/api/site') + .reply(404, 'invalidjson'); + + fetchOembedDataFromUrl.resolves({ + version: '1.0', + type: 'webmention', + metadata: { + title: 'Oembed Site Title', + description: 'Oembed Site Description', + publisher: 'Oembed Site Publisher', + author: 'Oembed Site Author', + thumbnail: 'invalid', + icon: 'invalid' + } + }); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + assert.deepEqual(metadata, { + title: 'Oembed Site Title', + excerpt: 'Oembed Site Description', + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + }); + + it('Nullifies empty oembed data', async function () { + nock('https://exampleghostsite.com') + .get('/subdirectory/members/api/site') + .reply(404, 'invalidjson'); + + nock('https://exampleghostsite.com') + .get('/members/api/site') + .reply(404, 'invalidjson'); + + fetchOembedDataFromUrl.resolves({ + version: '1.0', + type: 'webmention', + metadata: { + title: '', + description: '', + publisher: '', + author: '', + thumbnail: '', + icon: '' + } + }); + + const metadata = await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + assert.deepEqual(metadata, { + title: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + }); +}); diff --git a/ghost/recommendations/test/RecommendationService.test.ts b/ghost/recommendations/test/RecommendationService.test.ts index 67198a51ed..c7af99693f 100644 --- a/ghost/recommendations/test/RecommendationService.test.ts +++ b/ghost/recommendations/test/RecommendationService.test.ts @@ -1,5 +1,5 @@ import assert from 'assert/strict'; -import {ClickEvent, InMemoryRecommendationRepository, Recommendation, RecommendationService, SubscribeEvent, WellknownService} from '../src'; +import {ClickEvent, InMemoryRecommendationRepository, Recommendation, RecommendationService, SubscribeEvent, WellknownService, RecommendationMetadata, RecommendationMetadataService} from '../src'; import {InMemoryRepository} from '@tryghost/in-memory-repository'; import sinon from 'sinon'; @@ -12,9 +12,18 @@ class InMemoryClickEventRepository extends describe('RecommendationService', function () { let service: RecommendationService; let enabled = false; + let clock: sinon.SinonFakeTimers; + let fetchMetadataStub: sinon.SinonStub>; beforeEach(function () { enabled = false; + fetchMetadataStub = sinon.stub().resolves({ + title: 'Test', + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); service = new RecommendationService({ repository: new InMemoryRecommendationRepository(), clickEventRepository: new InMemoryClickEventRepository(), @@ -43,8 +52,17 @@ describe('RecommendationService', function () { enabled = e === 'true'; return Promise.resolve(); } - } + }, + recommendationMetadataService: { + fetch: fetchMetadataStub + } as unknown as RecommendationMetadataService }); + clock = sinon.useFakeTimers(); + }); + + afterEach(function () { + sinon.restore(); + clock.restore(); }); describe('init', function () { @@ -53,6 +71,215 @@ describe('RecommendationService', function () { await service.init(); assert(updateWellknown.calledOnce); }); + + it('should update recommendations on boot', async function () { + const recommendation = Recommendation.create({ + id: '2', + url: 'http://localhost/1', + title: 'Test', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + await service.repository.save(recommendation); + + // Sandbox time + const saved = process.env.NODE_ENV; + try { + process.env.NODE_ENV = 'development'; + const spy = sinon.spy(service, 'updateAllRecommendationsMetadata'); + await service.init(); + await clock.tick(1000 * 60 * 60 * 24); + assert(spy.calledOnce); + } finally { + process.env.NODE_ENV = saved; + } + }); + + it('ignores errors when update recommendations on boot', async function () { + // Sandbox time + const saved = process.env.NODE_ENV; + try { + process.env.NODE_ENV = 'development'; + const spy = sinon.stub(service, 'updateAllRecommendationsMetadata'); + spy.rejects(new Error('test')); + await service.init(); + clock.tick(1000 * 60 * 60 * 24); + assert(spy.calledOnce); + } finally { + process.env.NODE_ENV = saved; + } + }); + + it('should errors when update recommendations on boot (invidiual)', async function () { + const recommendation = Recommendation.create({ + id: '2', + url: 'http://localhost/1', + title: 'Test', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + await service.repository.save(recommendation); + + // Sandbox time + const saved = process.env.NODE_ENV; + try { + process.env.NODE_ENV = 'development'; + const spy = sinon.stub(service, '_updateRecommendationMetadata'); + spy.rejects(new Error('This is a test')); + await service.init(); + clock.tick(1000 * 60 * 60 * 24); + clock.restore(); + // This assert doesn't work without a timeout because the timeout in boot is async + // eslint-disable-next-line no-promise-executor-return + await new Promise((resolve) => { + setTimeout(() => resolve(true), 50); + }); + assert(!!spy.calledOnce); + } finally { + process.env.NODE_ENV = saved; + } + }); + }); + + describe('checkRecommendation', function () { + it('Returns existing recommendation if found', async function () { + const recommendation = Recommendation.create({ + id: '2', + url: 'http://localhost/existing', + title: 'Test', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + await service.repository.save(recommendation); + + const response = await service.checkRecommendation(new URL('http://localhost/existing')); + assert.deepEqual(response, recommendation.plain); + }); + + it('Returns updated recommendation if found', async function () { + const recommendation = Recommendation.create({ + id: '2', + url: 'http://localhost/existing', + title: 'Test', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + // Force an empty title (shouldn't be possible) + recommendation.title = ''; + await service.repository.save(recommendation); + + fetchMetadataStub.resolves({ + title: 'Test 2', + excerpt: 'Test excerpt', + featuredImage: new URL('https://example.com/image.png'), + favicon: new URL('https://example.com/favicon.ico'), + oneClickSubscribe: true + }); + + const response = await service.checkRecommendation(new URL('http://localhost/existing')); + assert.deepEqual(response, { + ...recommendation.plain, + // Note: Title only changes if it was empty + title: 'Test 2', + description: null, + excerpt: 'Test excerpt', + featuredImage: new URL('https://example.com/image.png'), + favicon: new URL('https://example.com/favicon.ico'), + oneClickSubscribe: true + }); + }); + + it('Returns updated recommendation if found but keeps empty title if no title found', async function () { + const recommendation = Recommendation.create({ + id: '2', + url: 'http://localhost/existing', + title: 'Test', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + // Force an empty title (shouldn't be possible) + recommendation.title = ''; + await service.repository.save(recommendation); + + fetchMetadataStub.resolves({ + title: null, + excerpt: 'Test excerpt', + featuredImage: new URL('https://example.com/image.png'), + favicon: new URL('https://example.com/favicon.ico'), + oneClickSubscribe: true + }); + + const response = await service.checkRecommendation(new URL('http://localhost/existing')); + + // No changes here, because validation failed with an empty title + assert.deepEqual(response, { + ...recommendation.plain + }); + }); + + it('Returns existing recommendation if found and fetch failes', async function () { + const recommendation = Recommendation.create({ + id: '2', + url: 'http://localhost/existing', + title: 'Outdated title', + description: null, + excerpt: null, + featuredImage: null, + favicon: null, + oneClickSubscribe: false + }); + await service.repository.save(recommendation); + + fetchMetadataStub.rejects(new Error('Test')); + const response = await service.checkRecommendation(new URL('http://localhost/existing')); + assert.deepEqual(response, recommendation.plain); + }); + + it('Returns recommendation metadata if not found', async function () { + const response = await service.checkRecommendation(new URL('http://localhost/newone')); + assert.deepEqual(response, { + title: 'Test', + excerpt: undefined, + featuredImage: undefined, + favicon: undefined, + oneClickSubscribe: false, + url: new URL('http://localhost/newone') + }); + }); + + it('Returns recommendation metadata if not found with all data except title', async function () { + fetchMetadataStub.resolves({ + title: null, + excerpt: 'Test excerpt', + featuredImage: new URL('https://example.com/image.png'), + favicon: new URL('https://example.com/favicon.ico'), + oneClickSubscribe: true + }); + const response = await service.checkRecommendation(new URL('http://localhost/newone')); + assert.deepEqual(response, { + title: undefined, + excerpt: 'Test excerpt', + featuredImage: new URL('https://example.com/image.png'), + favicon: new URL('https://example.com/favicon.ico'), + oneClickSubscribe: true, + url: new URL('http://localhost/newone') + }); + }); }); describe('updateRecommendationsEnabledSetting', function () {