From bd857e1695ffe7009647ec3e2589b7c0f76fa358 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 4 Jun 2026 12:33:18 +0100 Subject: [PATCH 01/10] Reverted admin flexsearch dependency (#28361) Admin search performance is under investigation after the dependency bump in ecab07e6efb49e8315a10db5ebd7d284cbb5a842. This reverts only the admin package while that investigation continues. --- ghost/admin/package.json | 2 +- pnpm-lock.yaml | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/ghost/admin/package.json b/ghost/admin/package.json index c65ef80c93c..9d1f736c1c4 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -120,7 +120,7 @@ "ember-truth-helpers": "3.1.1", "eslint": "catalog:", "eslint-plugin-babel": "5.3.1", - "flexsearch": "0.8.212", + "flexsearch": "0.7.43", "fs-extra": "catalog:", "ghost": "workspace:*", "google-caja-bower": "https://github.com/acburdine/google-caja-bower#ghost", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b28514bba1c..c1105b2a27c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2207,8 +2207,8 @@ importers: specifier: 5.3.1 version: 5.3.1(eslint@8.57.1) flexsearch: - specifier: 0.8.212 - version: 0.8.212 + specifier: 0.7.43 + version: 0.7.43 fs-extra: specifier: 'catalog:' version: 11.3.5 @@ -14217,6 +14217,9 @@ packages: resolution: {integrity: sha512-dVsPA/UwQ8+2uoFe5GHtiBMu48dWLTdsuEd7CKGlZlD78r1TTWBvDuFaFGKCo/ZfEr95Uk56vZoX86OsHkUeIg==} deprecated: flatten is deprecated in favor of utility frameworks such as lodash. + flexsearch@0.7.43: + resolution: {integrity: sha512-c5o/+Um8aqCSOXGcZoqZOm+NqtVwNsvVpWv6lfmSclU954O3wvQKxxK8zj74fPaSJbXpSLTs4PRhh+wnoCXnKg==} + flexsearch@0.8.212: resolution: {integrity: sha512-wSyJr1GUWoOOIISRu+X2IXiOcVfg9qqBRyCPRUdLMIGJqPzMo+jMRlvE83t14v1j0dRMEaBbER/adQjp6Du2pw==} @@ -36942,6 +36945,8 @@ snapshots: flatten@1.0.3: {} + flexsearch@0.7.43: {} + flexsearch@0.8.212: {} flush-write-stream@1.1.1: From 25736d4402e8b968fa6515ce820cab59184583a0 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Thu, 4 Jun 2026 12:40:46 +0100 Subject: [PATCH 02/10] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Added=20shared=20not?= =?UTF-8?q?ification=20email=20primitive=20(#28127)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduced a shared notification email primitive. It takes a resolved recipient list, sanitises the content, renders it inside a styled shell per recipient, and sends. The shell holds the chrome so notifications stay short and semantic. The sanitiser strips scripts, inline handlers and unsafe links so an untrusted upstream source cannot ship a phishing or injection payload through this path. --- .../services/mail/templates/notification.html | 157 ++++++++++++++++++ .../notifications/notification-email.ts | 66 ++++++++ .../notifications/sanitize-email-html.ts | 35 ++++ .../server/services/update-check/index.js | 13 +- .../update-check/update-check-service.js | 43 ++--- ghost/core/package.json | 1 + .../sanitize-email-html.test.ts.snap | 17 ++ .../notifications/notification-email.test.ts | 70 ++++++++ .../notifications/sanitize-email-html.test.ts | 22 +++ .../unit/server/services/update-check.test.js | 54 +++--- pnpm-lock.yaml | 17 +- pnpm-workspace.yaml | 1 + 12 files changed, 440 insertions(+), 56 deletions(-) create mode 100644 ghost/core/core/server/services/mail/templates/notification.html create mode 100644 ghost/core/core/server/services/notifications/notification-email.ts create mode 100644 ghost/core/core/server/services/notifications/sanitize-email-html.ts create mode 100644 ghost/core/test/unit/server/services/notifications/__snapshots__/sanitize-email-html.test.ts.snap create mode 100644 ghost/core/test/unit/server/services/notifications/notification-email.test.ts create mode 100644 ghost/core/test/unit/server/services/notifications/sanitize-email-html.test.ts diff --git a/ghost/core/core/server/services/mail/templates/notification.html b/ghost/core/core/server/services/mail/templates/notification.html new file mode 100644 index 00000000000..e0ea70983b4 --- /dev/null +++ b/ghost/core/core/server/services/mail/templates/notification.html @@ -0,0 +1,157 @@ + + + + + +Ghost notification + + + + + + + + + + +
  + +
+ + + + + + + + + + +
+ + + + + + + +
Ghost
+ {{message}} +
+
+
+

This email was sent from {{siteUrl}} to {{recipientEmail}}

+
+
+ +
+ 		 
+ + diff --git a/ghost/core/core/server/services/notifications/notification-email.ts b/ghost/core/core/server/services/notifications/notification-email.ts new file mode 100644 index 00000000000..1b7a830c595 --- /dev/null +++ b/ghost/core/core/server/services/notifications/notification-email.ts @@ -0,0 +1,66 @@ +import {sanitizeEmailHtml} from './sanitize-email-html'; + +interface Mailer { + send(options: {to: string; subject: string; html: string; text?: string}): Promise; +} + +interface GenerateEmailContent { + (options: {template: string; data: Record}): Promise<{html: string; text?: string}>; +} + +export interface NotificationEmailDeps { + mailer: Mailer; + generateEmailContent: GenerateEmailContent; + getSiteUrl: () => string; +} + +export interface NotificationEmailMessage { + /** + * Resolved recipient addresses. The service does not decide who receives + * the email; callers compose recipients via a separate audience helper. + */ + to: string[]; + subject: string; + /** + * Untrusted HTML content. Sanitised before being rendered into the shell, + * so the email cannot ship scripts, event handlers or unsafe links even + * if the upstream source is compromised. + */ + content: string; +} + +/** + * Renders a notification email in the shared shell and sends it to each + * recipient. Recipient resolution is a separate concern handled by the caller. + */ +export class NotificationEmailService { + private readonly mailer: Mailer; + private readonly generateEmailContent: GenerateEmailContent; + private readonly getSiteUrl: () => string; + + constructor(deps: NotificationEmailDeps) { + this.mailer = deps.mailer; + this.generateEmailContent = deps.generateEmailContent; + this.getSiteUrl = deps.getSiteUrl; + } + + async send({to, subject, content}: NotificationEmailMessage): Promise { + if (!to.length) { + return; + } + const message = sanitizeEmailHtml(content); + const siteUrl = this.getSiteUrl(); + for (const recipient of to) { + const {html, text} = await this.generateEmailContent({ + template: 'notification', + data: {message, siteUrl, recipientEmail: recipient} + }); + await this.mailer.send({ + to: recipient, + subject, + html, + ...(text ? {text} : {}) + }); + } + } +} diff --git a/ghost/core/core/server/services/notifications/sanitize-email-html.ts b/ghost/core/core/server/services/notifications/sanitize-email-html.ts new file mode 100644 index 00000000000..d2db0fa4279 --- /dev/null +++ b/ghost/core/core/server/services/notifications/sanitize-email-html.ts @@ -0,0 +1,35 @@ +import sanitizeHtml from 'sanitize-html'; + +// Even though the upstream feed is operated by Ghost org, the resulting HTML +// is rendered into admin inboxes on the receiving install. A compromised or +// malformed feed entry must not be able to ship scripts, event handlers, or +// non-http(s) URLs to recipients via this path. +const ALLOWED_TAGS = [ + 'p', 'br', 'hr', + 'strong', 'b', 'em', 'i', 'u', 'code', + 'a', + 'ul', 'ol', 'li', + 'blockquote', + 'h1', 'h2', 'h3', 'h4' +]; + +const ALLOWED_SCHEMES = ['http', 'https', 'mailto']; + +const SANITIZE_OPTIONS: sanitizeHtml.IOptions = { + allowedTags: ALLOWED_TAGS, + allowedAttributes: { + a: ['href', 'title', 'target', 'rel'] + }, + allowedSchemes: ALLOWED_SCHEMES, + allowProtocolRelative: false, + transformTags: { + a: sanitizeHtml.simpleTransform('a', { + target: '_blank', + rel: 'noopener noreferrer' + }) + } +}; + +export function sanitizeEmailHtml(html: string): string { + return sanitizeHtml(html, SANITIZE_OPTIONS); +} diff --git a/ghost/core/core/server/services/update-check/index.js b/ghost/core/core/server/services/update-check/index.js index beeb8d8b728..9379f849dfd 100644 --- a/ghost/core/core/server/services/update-check/index.js +++ b/ghost/core/core/server/services/update-check/index.js @@ -11,6 +11,7 @@ const databaseInfo = require('../../data/db/info'); const request = require('@tryghost/request'); const ghostVersion = require('@tryghost/version'); const UpdateCheckService = require('./update-check-service'); +const {NotificationEmailService} = require('../notifications/notification-email'); /** * Initializes and triggers update check @@ -34,8 +35,14 @@ module.exports = async ({ } } - const {GhostMailer} = require('../mail'); - const ghostMailer = new GhostMailer(); + const mailService = require('../mail'); + const ghostMailer = new mailService.GhostMailer(); + + const notificationEmailService = new NotificationEmailService({ + mailer: ghostMailer, + generateEmailContent: mailService.utils.generateContent, + getSiteUrl: () => urlUtils.urlFor('home', true) + }); const updateChecker = new UpdateCheckService({ api: { @@ -66,7 +73,7 @@ module.exports = async ({ rethrowErrors }, request, - sendEmail: ghostMailer.send.bind(ghostMailer) + notificationEmailService }); await updateChecker.check(); diff --git a/ghost/core/core/server/services/update-check/update-check-service.js b/ghost/core/core/server/services/update-check/update-check-service.js index 0318b31fbcd..ccc88a84c77 100644 --- a/ghost/core/core/server/services/update-check/update-check-service.js +++ b/ghost/core/core/server/services/update-check/update-check-service.js @@ -50,14 +50,14 @@ class UpdateCheckService { * @param {boolean} [options.config.forceUpdate] * @param {string} options.config.ghostVersion - Ghost instance version * @param {Function} options.request - a HTTP request proxy function - * @param {Function} options.sendEmail - function handling sending an email + * @param {Object} options.notificationEmailService - service that sends a sanitised, shell-rendered notification email to a recipient list */ - constructor({api, config, request, sendEmail}) { + constructor({api, config, request, notificationEmailService}) { this.api = api; this.config = config; this.logging = logging; this.request = request; - this.sendEmail = sendEmail; + this.notificationEmailService = notificationEmailService; } nextCheckTimestamp() { @@ -315,15 +315,6 @@ class UpdateCheckService { } debug(`creating custom notifications for ${notification.messages.length} notifications`); - const {users} = await this.api.users.browse(Object.assign({ - limit: 'all', - include: ['roles'], - filter: 'status:active' - }, internal)); - - const adminEmails = users - .filter(user => ['Owner', 'Administrator'].includes(user.roles[0].name)) - .map(user => user.email); const siteUrl = this.config.siteUrl; @@ -340,19 +331,21 @@ class UpdateCheckService { }; if (toAdd.type === 'alert') { - for (const email of adminEmails) { - try { - this.sendEmail({ - to: email, - subject: `Action required: Critical alert from Ghost instance ${siteUrl}`, - html: toAdd.message, - forceTextContent: true - }); - } catch (err) { - this.logging.error(err); - if (this.config.rethrowErrors) { - throw err; - } + try { + const {users} = await this.api.users.browse({ + filter: 'status:active+roles.name:[Owner,Administrator]', + ...internal + }); + const to = users.map(user => user.email); + await this.notificationEmailService.send({ + to, + subject: `Action required: Critical alert from Ghost instance ${siteUrl}`, + content: toAdd.message + }); + } catch (err) { + this.logging.error(err); + if (this.config.rethrowErrors) { + throw err; } } } diff --git a/ghost/core/package.json b/ghost/core/package.json index ebb8061df35..13b53c73aad 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -268,6 +268,7 @@ "@types/nodemailer": "6.4.23", "@types/on-headers": "1.0.4", "@types/papaparse": "5.5.2", + "@types/sanitize-html": "catalog:", "@types/sinon": "17.0.4", "@types/supertest": "6.0.3", "@typescript-eslint/parser": "catalog:", diff --git a/ghost/core/test/unit/server/services/notifications/__snapshots__/sanitize-email-html.test.ts.snap b/ghost/core/test/unit/server/services/notifications/__snapshots__/sanitize-email-html.test.ts.snap new file mode 100644 index 00000000000..0ddd160f316 --- /dev/null +++ b/ghost/core/test/unit/server/services/notifications/__snapshots__/sanitize-email-html.test.ts.snap @@ -0,0 +1,17 @@ +// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing + +exports[`sanitizeEmailHtml preserves safe formatting and neutralises dangerous content 1 1`] = ` +Object { + "output": " +

Ghost 6.50.0 is now available

+

This release includes a critical fix for an authentication issue. Please update as soon as possible.

+ +

Contact security if you have questions.

+ +

do not click

+", +} +`; diff --git a/ghost/core/test/unit/server/services/notifications/notification-email.test.ts b/ghost/core/test/unit/server/services/notifications/notification-email.test.ts new file mode 100644 index 00000000000..099feb3c100 --- /dev/null +++ b/ghost/core/test/unit/server/services/notifications/notification-email.test.ts @@ -0,0 +1,70 @@ +import assert from 'node:assert/strict'; +import sinon from 'sinon'; +import {NotificationEmailService} from '../../../../../core/server/services/notifications/notification-email'; + +function fakeMailer() { + return {send: sinon.stub().resolves()}; +} + +describe('NotificationEmailService', function () { + it('sends one email per recipient, shell-rendered with that recipient interpolated', async function () { + const mailer = fakeMailer(); + const generateEmailContent = sinon.stub().callsFake(async (opts: {data: Record}) => ({ + html: `shell:${opts.data.recipientEmail}:${opts.data.message}`, + text: 'plain' + })); + const service = new NotificationEmailService({ + mailer, + generateEmailContent, + getSiteUrl: () => 'https://example.com' + }); + + await service.send({ + to: ['owner@example.com', 'admin@example.com'], + subject: 'Security update', + content: '

Update now

' + }); + + sinon.assert.calledTwice(mailer.send); + assert.equal(mailer.send.args[0][0].to, 'owner@example.com'); + assert.equal(mailer.send.args[1][0].to, 'admin@example.com'); + assert.equal(mailer.send.args[0][0].subject, 'Security update'); + assert.match(mailer.send.args[0][0].html, /shell:owner@example.com:/); + assert.match(mailer.send.args[1][0].html, /shell:admin@example.com:/); + }); + + it('sanitises the content before passing it to the shell', async function () { + const mailer = fakeMailer(); + const generateEmailContent = sinon.stub().resolves({html: ''}); + const service = new NotificationEmailService({ + mailer, + generateEmailContent, + getSiteUrl: () => 'https://example.com' + }); + + await service.send({ + to: ['owner@example.com'], + subject: 'x', + content: '

hi

' + }); + + const renderedMessage = generateEmailContent.args[0][0].data.message; + assert.equal(renderedMessage.includes(' +

do not click

+`; + +describe('sanitizeEmailHtml', function () { + it('preserves safe formatting and neutralises dangerous content', function () { + assertMatchSnapshot({output: sanitizeEmailHtml(FIXTURE_MESSAGE_HTML)}); + }); +}); diff --git a/ghost/core/test/unit/server/services/update-check.test.js b/ghost/core/test/unit/server/services/update-check.test.js index 9fc8a9020b8..928c39298c2 100644 --- a/ghost/core/test/unit/server/services/update-check.test.js +++ b/ghost/core/test/unit/server/services/update-check.test.js @@ -290,17 +290,10 @@ describe('Update Check', function () { assert.equal(targetNotification.type, 'info'); assert.equal(targetNotification.message, notification.messages[0].content); - sinon.assert.calledTwice(usersBrowseStub); - - // Second (non statistical) call should be looking for admin users with an 'active' status only - assert.deepEqual(usersBrowseStub.args[1][0], { - limit: 'all', - include: ['roles'], - filter: 'status:active', - context: { - internal: true - } - }); + // users.browse is called once here, for stats reporting: this + // notification is non-alert, so the admin-lookup query in the + // alert branch never runs. + sinon.assert.calledOnce(usersBrowseStub); }); it('preserves custom flag value from update check response', async function () { @@ -364,7 +357,15 @@ describe('Update Check', function () { }); const notificationsAPIAddStub = sinon.stub().resolves(); - const sendEmailStub = sinon.stub().resolves(); + const emailSendStub = sinon.stub().resolves(); + const usersBrowseStub = sinon.stub().resolves({ + users: [{ + email: 'jbloggs@example.com', + roles: [{ + name: 'Owner' + }] + }] + }); const updateCheckService = new UpdateCheckService({ api: { @@ -373,14 +374,7 @@ describe('Update Check', function () { edit: settingsStub }, users: { - browse: sinon.stub().resolves({ - users: [{ - email: 'jbloggs@example.com', - roles: [{ - name: 'Owner' - }] - }] - }) + browse: usersBrowseStub }, posts: { browse: sinon.stub().resolves() @@ -396,16 +390,24 @@ describe('Update Check', function () { ghostVersion: '0.8.0' }, request: request, - sendEmail: sendEmailStub + notificationEmailService: {send: emailSendStub} }); await updateCheckService.check(); - sinon.assert.called(sendEmailStub); - assert.equal(sendEmailStub.args[0][0].to, 'jbloggs@example.com'); - assert.equal(sendEmailStub.args[0][0].subject, 'Action required: Critical alert from Ghost instance http://127.0.0.1:2369'); - assert.equal(sendEmailStub.args[0][0].html, '

Critical message. Upgrade your site!

'); - assert.equal(sendEmailStub.args[0][0].forceTextContent, true); + sinon.assert.calledOnce(emailSendStub); + assert.deepEqual(emailSendStub.args[0][0].to, ['jbloggs@example.com']); + assert.equal(emailSendStub.args[0][0].subject, 'Action required: Critical alert from Ghost instance http://127.0.0.1:2369'); + assert.equal(emailSendStub.args[0][0].content, '

Critical message. Upgrade your site!

'); + + // users.browse is called twice: once for stats reporting, once for + // the admin lookup. The second call pushes the role filter into + // NQL so the DB returns only Owner/Administrator users. + sinon.assert.calledTwice(usersBrowseStub); + assert.deepEqual(usersBrowseStub.args[1][0], { + filter: 'status:active+roles.name:[Owner,Administrator]', + context: {internal: true} + }); sinon.assert.calledOnce(notificationsAPIAddStub); assert.equal(notificationsAPIAddStub.args[0][0].notifications.length, 1); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c1105b2a27c..9e0982d76db 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -129,6 +129,9 @@ catalogs: '@types/react-dom': specifier: 18.3.7 version: 18.3.7 + '@types/sanitize-html': + specifier: 2.16.1 + version: 2.16.1 '@types/validator': specifier: 13.15.10 version: 13.15.10 @@ -2837,6 +2840,9 @@ importers: '@types/papaparse': specifier: 5.5.2 version: 5.5.2 + '@types/sanitize-html': + specifier: 'catalog:' + version: 2.16.1 '@types/sinon': specifier: 17.0.4 version: 17.0.4 @@ -9220,6 +9226,9 @@ packages: '@types/rimraf@2.0.5': resolution: {integrity: sha512-YyP+VfeaqAyFmXoTh3HChxOQMyjByRMsHU7kc5KOJkSlXudhMhQIALbYV7rHh/l8d2lX3VUQzprrcAgWdRuU8g==} + '@types/sanitize-html@2.16.1': + resolution: {integrity: sha512-n9wjs8bCOTyN/ynwD8s/nTcTreIHB1vf31vhLMGqUPNHaweKC4/fAl4Dj+hUlCTKYgm4P3k83fmiFfzkZ6sgMA==} + '@types/send@0.17.6': resolution: {integrity: sha512-Uqt8rPBE8SY0RK8JB1EzVOIZ32uqy8HwdxCnoCOsYrvnswqmFZ/k+9Ikidlk/ImhsdvBsloHbAlewb2IEBV/Og==} @@ -14624,7 +14633,7 @@ packages: csstype: ^3.0.10 google-caja-bower@https://codeload.github.com/acburdine/google-caja-bower/tar.gz/275cb75249f038492094a499756a73719ae071fd: - resolution: {gitHosted: true, tarball: https://codeload.github.com/acburdine/google-caja-bower/tar.gz/275cb75249f038492094a499756a73719ae071fd} + resolution: {gitHosted: true, integrity: sha512-mmCXdxGKGKDznjgkNzVqzTslaldslk5KMb/A7l8rxWnqyxzwsdPhuBJ6oT1Kh/Y3k4jN54ISee/2AgjFyCBxYw==, tarball: https://codeload.github.com/acburdine/google-caja-bower/tar.gz/275cb75249f038492094a499756a73719ae071fd} version: 6011.0.0 gopd@1.2.0: @@ -16020,7 +16029,7 @@ packages: engines: {node: '>= 0.6'} keymaster@https://codeload.github.com/madrobby/keymaster/tar.gz/f8f43ddafad663b505dc0908e72853bcf8daea49: - resolution: {gitHosted: true, tarball: https://codeload.github.com/madrobby/keymaster/tar.gz/f8f43ddafad663b505dc0908e72853bcf8daea49} + resolution: {gitHosted: true, integrity: sha512-/WVovQslVEqPGNoD97TbqNHuCDPYu2v4/ggrZj0a+9PVPw3Rud4Ut2K7fOi0kMqzoJINkgP68e9m09Al/wFZ8g==, tarball: https://codeload.github.com/madrobby/keymaster/tar.gz/f8f43ddafad663b505dc0908e72853bcf8daea49} version: 1.6.3 keypair@1.0.4: @@ -29565,6 +29574,10 @@ snapshots: '@types/glob': 9.0.0 '@types/node': 25.9.1 + '@types/sanitize-html@2.16.1': + dependencies: + htmlparser2: 10.1.0 + '@types/send@0.17.6': dependencies: '@types/mime': 1.3.5 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 75d9b964c1c..25cd0fe9272 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -67,6 +67,7 @@ catalog: '@types/node': 22.19.19 '@types/react': 18.3.29 '@types/react-dom': 18.3.7 + '@types/sanitize-html': 2.16.1 '@types/validator': 13.15.10 '@typescript-eslint/eslint-plugin': 8.49.0 '@typescript-eslint/parser': 8.49.0 From 8eafcb8150889e9c38cdf38046e7ca948e414924 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Thu, 4 Jun 2026 14:49:52 +0100 Subject: [PATCH 03/10] Improve llms-full generation bounds (#28366) /llms-full.txt previously walked the full public post catalogue while building a single bounded response. On large publications that made generation latency scale with total post count, even though the response is already constrained by a 5 MiB output budget. This bounds the full export to public pages plus the latest 500 public posts. That keeps /llms-full.txt useful as a consolidated context file while giving generation a predictable upper bound of five post pages. The complete public content index remains available in /llms.txt, and a footer is added when the recent-post cap is reached so consumers know where to discover older content. The budget check now tracks generated byte length incrementally instead of repeatedly measuring the whole growing output for every candidate entry. Both truncation and recent-post footer sizes are reserved up front, so the generated response stays within the configured budget no matter which footer is needed. --- .../core/frontend/services/llms/service.js | 47 +++++++++++++++---- .../frontend/services/llms/service.test.js | 45 ++++++++++++++++++ 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/ghost/core/core/frontend/services/llms/service.js b/ghost/core/core/frontend/services/llms/service.js index daebdc7b773..a664eee443e 100644 --- a/ghost/core/core/frontend/services/llms/service.js +++ b/ghost/core/core/frontend/services/llms/service.js @@ -6,10 +6,16 @@ const { const DEFAULT_BUDGET = 5 * 1024 * 1024; const TRUNCATION_FOOTER = '\n_Truncated after 5 MiB. Use `/llms.txt` for the complete index of older public content._\n'; +const RECENT_POSTS_FOOTER = '\n_Includes the latest 500 public posts. Use `/llms.txt` for the complete index of older public content._\n'; const FULL_PAGE_SIZE = 100; +const FULL_POST_LIMIT = 500; function createLlmsService({settingsCache, labs, config, urlServiceFacade, urlUtils, models, routing, api, fullTxtBudget}) { - const BUDGET = (fullTxtBudget || DEFAULT_BUDGET) - Buffer.byteLength(TRUNCATION_FOOTER, 'utf8'); + const footerBudget = Math.max( + Buffer.byteLength(TRUNCATION_FOOTER, 'utf8'), + Buffer.byteLength(RECENT_POSTS_FOOTER, 'utf8') + ); + const BUDGET = (fullTxtBudget || DEFAULT_BUDGET) - footerBudget; function isEnabled() { return labs.isSet('llmsTxt') && !settingsCache.get('is_private') && settingsCache.get('llms_enabled') !== false; } @@ -67,46 +73,55 @@ function createLlmsService({settingsCache, labs, config, urlServiceFacade, urlUt let output = header; let wasTruncated = false; + let wasLimited = false; const pageResult = await appendBoundedSectionPaginated(output, 'Pages', 'page'); output = pageResult.output; wasTruncated = pageResult.wasTruncated; if (!wasTruncated) { - const postResult = await appendBoundedSectionPaginated(output, 'Posts', 'post'); + const postResult = await appendBoundedSectionPaginated(output, 'Posts', 'post', {maxEntries: FULL_POST_LIMIT}); output = postResult.output; wasTruncated = postResult.wasTruncated; + wasLimited = postResult.wasLimited; } if (wasTruncated) { output += TRUNCATION_FOOTER; + } else if (wasLimited) { + output += RECENT_POSTS_FOOTER; } return output.trimEnd() + '\n'; } - async function appendBoundedSectionPaginated(prefix, heading, type) { + async function appendBoundedSectionPaginated(prefix, heading, type, {maxEntries = null} = {}) { const headingBlock = `${prefix}## ${heading}\n`; if (Buffer.byteLength(headingBlock, 'utf8') > BUDGET) { - return {output: prefix, wasTruncated: true}; + return {output: prefix, wasTruncated: true, wasLimited: false}; } let output = headingBlock; + let outputBytes = Buffer.byteLength(output, 'utf8'); let wasTruncated = false; + let wasLimited = false; let page = 1; let hasMore = true; + let entriesRendered = 0; - while (hasMore && !wasTruncated) { + while (hasMore && !wasTruncated && !wasLimited) { const result = await fetchFullEntries(type, page); const entries = result.entries; hasMore = result.hasMore; if (!entries.length && page === 1) { const emptySection = `${output}_No public content available._\n`; + const emptySectionBytes = Buffer.byteLength(emptySection, 'utf8'); - if (Buffer.byteLength(emptySection, 'utf8') <= BUDGET) { + if (emptySectionBytes <= BUDGET) { output = emptySection; + outputBytes = emptySectionBytes; } else { wasTruncated = true; } @@ -115,21 +130,33 @@ function createLlmsService({settingsCache, labs, config, urlServiceFacade, urlUt } for (const entry of entries) { + if (maxEntries && entriesRendered >= maxEntries) { + wasLimited = true; + break; + } + const formattedEntry = buildFullEntry(entry); - const candidate = `${output}${formattedEntry}\n`; + const entryBlock = `${formattedEntry}\n`; + const entryBytes = Buffer.byteLength(entryBlock, 'utf8'); - if (Buffer.byteLength(candidate, 'utf8') > BUDGET) { + if (outputBytes + entryBytes > BUDGET) { wasTruncated = true; break; } - output = candidate; + output = `${output}${entryBlock}`; + outputBytes += entryBytes; + entriesRendered += 1; + } + + if (maxEntries && entriesRendered >= maxEntries && hasMore) { + wasLimited = true; } page += 1; } - return {output, wasTruncated}; + return {output, wasTruncated, wasLimited}; } function buildHeader() { diff --git a/ghost/core/test/unit/frontend/services/llms/service.test.js b/ghost/core/test/unit/frontend/services/llms/service.test.js index a9322b5ea42..ea03252956f 100644 --- a/ghost/core/test/unit/frontend/services/llms/service.test.js +++ b/ghost/core/test/unit/frontend/services/llms/service.test.js @@ -210,6 +210,51 @@ describe('Unit: frontend/services/llms/service', function () { assert.match(llmsFullTxt, /Truncated after 5 MiB/); }); + it('limits llms-full.txt to the latest 500 posts', async function () { + const calls = []; + const models = { + Post: { + findPage: async function (options) { + calls.push(options); + + if (options.filter.includes('type:page')) { + return {data: []}; + } + + const pageStart = (options.page - 1) * 100; + return { + data: Array.from({length: 100}, (_, index) => { + const postIndex = pageStart + index; + return { + toJSON: () => ({ + id: `post-${postIndex}`, + title: `Post ${postIndex}`, + slug: `post-${postIndex}`, + plaintext: `Post ${postIndex} body`, + type: 'post' + }) + }; + }) + }; + } + } + }; + const urlMap = Object.fromEntries(Array.from({length: 600}, (_, index) => [ + `post-${index}`, + `https://example.com/post-${index}/` + ])); + const service = createService({models, urlMap}); + + const llmsFullTxt = await service.getLlmsFullTxt(); + + const postCalls = calls.filter(call => call.filter.includes('type:post')); + assert.equal(postCalls.length, 5); + assert.match(llmsFullTxt, /### Post 499/); + assert.doesNotMatch(llmsFullTxt, /### Post 500/); + assert.match(llmsFullTxt, /Includes the latest 500 public posts/); + assert.match(llmsFullTxt, /Use `\/llms\.txt` for the complete index/); + }); + it('computes fresh output on every call (no cache)', async function () { let callCount = 0; From 405c2623ff9060295d4a87d3a0eb4ef766c8d2d3 Mon Sep 17 00:00:00 2001 From: Jonatan Svennberg Date: Thu, 4 Jun 2026 16:04:31 +0200 Subject: [PATCH 04/10] Refactor comment lookup handling (#28297) ref https://linear.app/ghost/issue/BER-3700/refactor-comment-api-querying Consolidates the comments service's scattered comment-by-ID fetches into a small set of shared, status-aware lookups. Each filters status in the query and selects the internal columns the follow-up logic relies on, so field-limited API requests still resolve correctly. --- .../server/api/endpoints/comment-replies.js | 3 +- .../services/comments/comments-controller.js | 142 +++--- .../services/comments/comments-service.js | 328 +++++++++---- .../__snapshots__/comments.test.js.snap | 48 -- .../e2e-api/members-comments/comments.test.js | 58 ++- .../comments/comments-controller.test.js | 85 +++- .../comments/comments-service.test.js | 437 +++++++++++++++++- 7 files changed, 875 insertions(+), 226 deletions(-) diff --git a/ghost/core/core/server/api/endpoints/comment-replies.js b/ghost/core/core/server/api/endpoints/comment-replies.js index ddc308df5e0..3241c5a0def 100644 --- a/ghost/core/core/server/api/endpoints/comment-replies.js +++ b/ghost/core/core/server/api/endpoints/comment-replies.js @@ -53,8 +53,7 @@ const controller = { }, permissions: true, query(frame) { - frame.options.isAdmin = true; - return commentsService.controller.read(frame); + return commentsService.controller.adminRead(frame); } } }; diff --git a/ghost/core/core/server/services/comments/comments-controller.js b/ghost/core/core/server/services/comments/comments-controller.js index 9128af18b4c..2bc1466f13e 100644 --- a/ghost/core/core/server/services/comments/comments-controller.js +++ b/ghost/core/core/server/services/comments/comments-controller.js @@ -12,7 +12,8 @@ const tpl = require('@tryghost/tpl'); const messages = { cannotDestroyComments: 'You cannot destroy comments.', - memberNotFound: 'Unable to find member' + memberNotFound: 'Unable to find member', + unsupportedReportCountFilter: 'Unsupported count.reports filter' }; module.exports = class CommentsController { @@ -25,12 +26,51 @@ module.exports = class CommentsController { this.stats = stats; } - async #setImpersonationContext(options) { - if (options.impersonate_member_uuid) { - options.context = options.context || {}; - options.context.member = options.context.member || {}; - options.context.member.id = await this.service.getMemberIdByUUID(options.impersonate_member_uuid); + #withPostFilter(options) { + const nextOptions = {...options}; + + if (!options.post_id) { + return nextOptions; + } + + if (options.filter) { + const postId = options.post_id; + const existingTransformer = options.mongoTransformer; + nextOptions.mongoTransformer = function (query) { + const transformedQuery = existingTransformer ? existingTransformer(query) : query; + + return { + $and: [ + { + post_id: postId + }, + transformedQuery + ] + }; + }; + } else { + nextOptions.filter = `post_id:${options.post_id}`; + } + + return nextOptions; + } + + async #withImpersonationContext(options) { + const nextOptions = {...options}; + + if (!options.impersonate_member_uuid) { + return nextOptions; } + + nextOptions.context = { + ...options.context, + member: { + ...options.context?.member, + id: await this.service.getMemberIdByUUID(options.impersonate_member_uuid) + } + }; + + return nextOptions; } #checkMember(frame) { @@ -73,6 +113,12 @@ module.exports = class CommentsController { } else if (val.$ne !== undefined) { reportCount = {op: '!=', value: val.$ne}; } + + if (!reportCount) { + throw new errors.BadRequestError({ + message: tpl(messages.unsupportedReportCountFilter) + }); + } } // If there's remaining filter, use transformer to replace parsed result @@ -92,53 +138,23 @@ module.exports = class CommentsController { * @param {Frame} frame */ async browse(frame) { - if (frame.options.post_id) { - if (frame.options.filter) { - frame.options.mongoTransformer = function (query) { - return { - $and: [ - { - post_id: frame.options.post_id - }, - query - ] - }; - }; - } else { - frame.options.filter = `post_id:${frame.options.post_id}`; - } - } - - return await this.service.getComments(frame.options); + return await this.service.getComments(this.#withPostFilter(frame.options)); } async adminBrowse(frame) { - if (frame.options.post_id) { - if (frame.options.filter) { - frame.options.mongoTransformer = function (query) { - return { - $and: [ - { - post_id: frame.options.post_id - }, - query - ] - }; - }; - } else { - frame.options.filter = `post_id:${frame.options.post_id}`; - } - } - - frame.options.isAdmin = true; + let options = this.#withPostFilter(frame.options); + options = { + ...options, + isAdmin: true + }; // Admin routes in Comments-UI lack member context due to cross-domain constraints (CORS), which prevents // credentials from being passed. This causes issues like the inability to determine if a // logged-in admin (acting on behalf of a member) has already liked a comment. // To resolve this, we retrieve the `impersonate_member_uuid` from the request params and // explicitly set it in the context options as the acting member's ID. // Note: This approach is applied to several admin routes where member context is required. - await this.#setImpersonationContext(frame.options); - return await this.service.getAdminComments(frame.options); + options = await this.#withImpersonationContext(options); + return await this.service.getAdminComments(options); } /** @@ -221,19 +237,34 @@ module.exports = class CommentsController { * @param {Frame} frame */ async adminReplies(frame) { - frame.options.isAdmin = true; - frame.options.order = 'created_at asc'; // we always want to load replies from oldest to newest - await this.#setImpersonationContext(frame.options); + let options = { + ...frame.options, + order: 'created_at asc' // we always want to load replies from oldest to newest + }; + options = await this.#withImpersonationContext(options); - return this.service.getReplies(frame.options.id, _.omit(frame.options, 'id')); + return this.service.getAdminReplies(frame.options.id, _.omit(options, 'id')); } /** * @param {Frame} frame */ async read(frame) { - await this.#setImpersonationContext(frame.options); - return await this.service.getCommentByID(frame.data.id, frame.options); + const options = await this.#withImpersonationContext(frame.options); + return await this.service.getCommentByID(frame.data.id, options); + } + + /** + * @param {Frame} frame + */ + async adminRead(frame) { + let options = { + ...frame.options, + isAdmin: true + }; + options = await this.#withImpersonationContext(options); + + return await this.service.getCommentByID(frame.data.id, options); } /** @@ -337,7 +368,7 @@ module.exports = class CommentsController { frame.options ); - const comment = await this.service.getCommentByID(frame.options.id); + const comment = await this.service.getCommentByID(frame.options.id, frame.options); if (comment) { const postId = comment.get('post_id'); @@ -364,7 +395,7 @@ module.exports = class CommentsController { frame.options ); - const comment = await this.service.getCommentByID(frame.options.id); + const comment = await this.service.getCommentByID(frame.options.id, frame.options); if (comment) { const postId = comment.get('post_id'); @@ -387,7 +418,8 @@ module.exports = class CommentsController { return await this.service.reportComment( frame.options.id, - frame.options?.context?.member + frame.options?.context?.member, + frame.options ); } @@ -427,7 +459,7 @@ module.exports = class CommentsController { frame.options ); - const comment = await this.service.getCommentByID(frame.options.id); + const comment = await this.service.getCommentByID(frame.options.id, frame.options); if (comment) { const postId = comment.get('post_id'); @@ -454,7 +486,7 @@ module.exports = class CommentsController { frame.options ); - const comment = await this.service.getCommentByID(frame.options.id); + const comment = await this.service.getCommentByID(frame.options.id, frame.options); if (comment) { const postId = comment.get('post_id'); diff --git a/ghost/core/core/server/services/comments/comments-service.js b/ghost/core/core/server/services/comments/comments-service.js index f654bdb2fa7..91922111a43 100644 --- a/ghost/core/core/server/services/comments/comments-service.js +++ b/ghost/core/core/server/services/comments/comments-service.js @@ -23,13 +23,55 @@ const messages = { const COMMENT_LIKE_SCORE = 1; const COMMENT_DISLIKE_SCORE = -1; +const COMMENT_STATUS_PUBLISHED = 'published'; +const COMMENT_STATUS_HIDDEN = 'hidden'; +const COMMENT_STATUS_DELETED = 'deleted'; +const COMMENT_STATUSES_READABLE = [COMMENT_STATUS_PUBLISHED, COMMENT_STATUS_HIDDEN]; +const COMMENT_STATUSES_REPLY_PARENT = [COMMENT_STATUS_PUBLISHED, COMMENT_STATUS_HIDDEN, COMMENT_STATUS_DELETED]; +const COMMENT_STATUSES_IN_REPLY_TO = [COMMENT_STATUS_PUBLISHED, COMMENT_STATUS_HIDDEN]; -function withPinnedSelect(options = {}) { - if (!options.columns?.includes('pinned')) { - return options; +// Columns each action reads from the comment row beyond `id`/`status`, which the +// lookup helpers always select. Keeping these lists together makes the coupling +// between "fields read after the fetch" and "columns requested" explicit, so a +// new `comment.get(...)` call has one obvious place to register its column. +const REPLY_PARENT_REQUIRED_COLUMNS = ['parent_id', 'post_id']; +const IN_REPLY_TO_REQUIRED_COLUMNS = ['parent_id']; +const OWNERSHIP_REQUIRED_COLUMNS = ['member_id']; +const REPORT_REQUIRED_COLUMNS = ['post_id', 'member_id', 'html', 'created_at']; + +function getColumnList(columns) { + if (!columns) { + return null; + } + + if (Array.isArray(columns)) { + return columns; + } + + return columns.split(',').map(column => column.trim()).filter(Boolean); +} + +function withRequiredColumns(options = {}, requiredColumns = []) { + const columns = getColumnList(options.columns); + + if (!columns) { + return {...options}; + } + + return { + ...options, + columns: Array.from(new Set(['id', ...columns, ...requiredColumns])) + }; +} + +function withPinnedSelect(options = {}, {includeHidden = false} = {}) { + const columns = getColumnList(options.columns); + + if (!columns?.includes('pinned')) { + return {...options}; } - const statusClause = options.isAdmin ? '' : ' AND comments.status = \'published\''; + const statusClause = includeHidden ? '' : ' AND comments.status = \'published\''; const pinnedSelect = `CASE WHEN comments.parent_id IS NULL AND comments.pinned_at IS NOT NULL${statusClause} THEN 1 ELSE 0 END AS pinned`; return { @@ -38,6 +80,19 @@ function withPinnedSelect(options = {}) { }; } +function getSafeFetchOptions(options = {}, columns = []) { + return { + ...(options.transacting ? {transacting: options.transacting} : {}), + columns: Array.from(new Set(['id', ...columns])) + }; +} + +function getSafeWriteOptions(options = {}) { + return { + ...(options.transacting ? {transacting: options.transacting} : {}) + }; +} + class CommentsService { constructor({config, logging, models, mailer, settingsCache, settingsHelpers, urlService, urlUtils, contentGating, labs}) { /** @private */ @@ -119,14 +174,148 @@ class CommentsService { }); } + /** + * @private + * Primary comment lookup: a single keyed fetch with the allowed statuses + * applied in the query (`WHERE id = ? AND status IN (...)`), optionally locked + * with `FOR UPDATE` inside a transaction. It deliberately does not load the + * member-facing relation graph; the one read that needs those relations + * (#getReadableCommentByID, backing getCommentByID) uses `findOne` instead. + */ + async #fetchCommentByID(id, options = {}, {requiredColumns = [], statuses = [], forUpdate = false} = {}) { + const model = this.models.Comment.forge(); + model.query((qb) => { + qb.where('comments.id', id); + + if (statuses.length === 1) { + qb.where('comments.status', statuses[0]); + } else if (statuses.length > 1) { + qb.whereIn('comments.status', statuses); + } + + if (forUpdate && options.transacting) { + qb.forUpdate(); + } + }); + + return await model.fetch(getSafeFetchOptions(options, requiredColumns)); + } + /** @private */ - async #getMemberCommentVotes(commentId, memberId, options) { - const votes = await this.models.CommentLike.findAll({ - ...options, - filter: `comment_id:'${commentId}'+member_id:'${memberId}'`, - order: 'created_at asc' + async #getPublishedCommentForAction(id, options = {}, requiredColumns = [], {forUpdate = false} = {}) { + const model = await this.#fetchCommentByID(id, options, { + requiredColumns, + statuses: [COMMENT_STATUS_PUBLISHED], + forUpdate + }); + + if (!model) { + throw new errors.NotFoundError({ + message: tpl(messages.commentNotFound) + }); + } + + return model; + } + + /** + * @private + * Member-facing read: goes through `findOne` (not the lean primitive) so the + * serializer's default relation graph (member, counts, in_reply_to, replies) + * is loaded. Readable statuses are constrained in the query via an NQL filter, + * so a non-readable comment is never fetched and its relations never loaded — a + * missing or non-readable id is a 404. `status` stays selected for the + * serializer's hidden/deleted redaction. + */ + async #getReadableCommentByID(id, options = {}, requiredColumns = []) { + const readableFilter = `status:[${COMMENT_STATUSES_READABLE.join(',')}]`; + const model = await this.models.Comment.findOne( + {id}, + withRequiredColumns( + { + ...options, + filter: options.filter ? `(${options.filter})+${readableFilter}` : readableFilter + }, + ['status', ...requiredColumns] + ) + ); + + if (!model) { + throw new errors.NotFoundError({ + message: tpl(messages.commentNotFound) + }); + } + + return model; + } + + /** @private */ + async #getReplyParentCommentByID(id, options = {}) { + // Deleted parent comments intentionally remain valid thread anchors: a reply + // can still be posted under a top-level comment whose root was removed. + const model = await this.#fetchCommentByID(id, options, { + requiredColumns: REPLY_PARENT_REQUIRED_COLUMNS, + statuses: COMMENT_STATUSES_REPLY_PARENT + }); + + if (!model) { + throw new errors.NotFoundError({ + message: tpl(messages.commentNotFound) + }); + } + + return model; + } + + /** @private */ + async #getInReplyToCommentByID(id, parent, options = {}) { + const model = await this.#fetchCommentByID(id, options, { + requiredColumns: IN_REPLY_TO_REQUIRED_COLUMNS, + statuses: COMMENT_STATUSES_IN_REPLY_TO + }); + + // A target that is missing, deleted, or not a readable sibling is simply not + // a valid direct-reply anchor: drop the reference and let the reply post + // anyway (you can reply within the thread, just not "to" a dead comment). The + // query already excluded disallowed statuses, so only the cross-thread + // relationship check remains here. Hidden-target snippets are redacted + // downstream. + if (!model || model.get('parent_id') !== parent) { + return null; + } + + return model; + } + + /** @private */ + async #assertCommentExists(commentId, options = {}) { + const model = await this.#fetchCommentByID(commentId, options, { + requiredColumns: ['id'] + }); + + if (!model) { + throw new errors.NotFoundError({ + message: tpl(messages.commentNotFound) + }); + } + } + + /** @private */ + async #getMemberCommentVotes({commentId, memberId, score}, options) { + const collection = this.models.CommentLike.forge(); + collection.query((qb) => { + qb.where('comment_likes.comment_id', commentId) + .where('comment_likes.member_id', memberId); + + if (score !== undefined) { + qb.where('comment_likes.score', score); + } + + qb.orderBy('comment_likes.created_at', 'asc'); }); + const votes = await collection.fetchAll(options); + return votes.models || []; } @@ -148,7 +337,12 @@ class CommentsService { this.checkCommentAccess(memberModel); return await this.#withTransaction(options, async (transactionOptions) => { - const votes = await this.#getMemberCommentVotes(commentId, memberModel.id, transactionOptions); + await this.#getPublishedCommentForAction(commentId, transactionOptions, [], {forUpdate: true}); + + const votes = await this.#getMemberCommentVotes({ + commentId, + memberId: memberModel.id + }, transactionOptions); const alreadyHasSingleTargetVote = votes.length === 1 && Number(votes[0].get('score')) === targetScore; if (alreadyHasSingleTargetVote) { @@ -170,8 +364,13 @@ class CommentsService { /** @private */ async #clearCommentVote(commentId, member, targetScore, notFoundMessage, options = {}) { await this.#withTransaction(options, async (transactionOptions) => { - const votes = await this.#getMemberCommentVotes(commentId, member.id, transactionOptions); - const votesToRemove = votes.filter(vote => Number(vote.get('score')) === targetScore); + await this.#getPublishedCommentForAction(commentId, transactionOptions, [], {forUpdate: true}); + + const votesToRemove = await this.#getMemberCommentVotes({ + commentId, + memberId: member.id, + score: targetScore + }, transactionOptions); if (votesToRemove.length === 0) { throw new errors.NotFoundError({ @@ -219,15 +418,16 @@ class CommentsService { await this.#clearCommentVote(commentId, member, COMMENT_DISLIKE_SCORE, messages.dislikeNotFound, options); } - async reportComment(commentId, reporter) { + async reportComment(commentId, reporter, options = {}) { this.checkEnabled(); - const comment = await this.models.Comment.findOne({id: commentId}, {require: true}); + const comment = await this.#getPublishedCommentForAction(commentId, options, REPORT_REQUIRED_COLUMNS); + const writeOptions = getSafeWriteOptions(options); // Check if this reporter already reported this comment (then don't send an email)? const existing = await this.models.CommentReport.findOne({ comment_id: comment.id, member_id: reporter.id - }); + }, writeOptions); if (existing) { // Ignore silently for now @@ -238,7 +438,7 @@ class CommentsService { await this.models.CommentReport.add({ comment_id: comment.id, member_id: reporter.id - }); + }, writeOptions); await this.emails.notifyReport(comment, reporter); } @@ -295,7 +495,12 @@ class CommentsService { async getAdminComments(options) { this.checkEnabled(); const pinnedFirst = this.labs?.isSet('commentsPinning'); - const page = await this.models.Comment.findPage(withPinnedSelect({...options, parentId: null, isAdmin: true, pinnedFirst})); + const page = await this.models.Comment.findPage(withPinnedSelect({ + ...options, + parentId: null, + isAdmin: true, + pinnedFirst + }, {includeHidden: true})); return page; } @@ -306,7 +511,7 @@ class CommentsService { if (Object.prototype.hasOwnProperty.call(data, 'status')) { editData.status = data.status; - if (data.status === 'deleted') { + if (data.status === COMMENT_STATUS_DELETED) { editData.pinned_at = null; } } @@ -334,7 +539,7 @@ class CommentsService { }); } - if (existingComment.get('status') === 'deleted' || data.status === 'deleted') { + if (existingComment.get('status') === COMMENT_STATUS_DELETED || data.status === COMMENT_STATUS_DELETED) { throw new errors.BadRequestError({ message: tpl(messages.cannotPinDeletedComment) }); @@ -355,25 +560,27 @@ class CommentsService { * @param {string} id - The ID of the Comment to get replies from * @param {any} options */ - async getReplies(id, options) { + async getReplies(id, options, {includeHidden = false} = {}) { this.checkEnabled(); - const page = await this.models.Comment.findPage(withPinnedSelect({...options, parentId: id})); + const page = await this.models.Comment.findPage(withPinnedSelect({...options, parentId: id}, {includeHidden})); return page; } + async getAdminReplies(id, options) { + return await this.getReplies(id, { + ...options, + isAdmin: true + }, {includeHidden: true}); + } + /** * Get reporters for a comment (admin only) * @param {string} commentId - The ID of the Comment to get reporters for * @param {any} options - Query options (page, limit) */ async getCommentReporters(commentId, options = {}) { - const comment = await this.models.Comment.findOne({id: commentId}); - if (!comment) { - throw new errors.NotFoundError({ - message: tpl(messages.commentNotFound) - }); - } + await this.#assertCommentExists(commentId); const {page, limit} = options; const result = await this.models.CommentReport.findPage({ @@ -393,12 +600,7 @@ class CommentsService { * @param {any} options - Query options (page, limit) */ async getCommentLikes(commentId, options = {}) { - const comment = await this.models.Comment.findOne({id: commentId}); - if (!comment) { - throw new errors.NotFoundError({ - message: tpl(messages.commentNotFound) - }); - } + await this.#assertCommentExists(commentId); const {page, limit} = options; const result = await this.models.CommentLike.findPage({ @@ -418,12 +620,7 @@ class CommentsService { * @param {any} options - Query options (page, limit) */ async getCommentDislikes(commentId, options = {}) { - const comment = await this.models.Comment.findOne({id: commentId}); - if (!comment) { - throw new errors.NotFoundError({ - message: tpl(messages.commentNotFound) - }); - } + await this.#assertCommentExists(commentId); const {page, limit} = options; const result = await this.models.CommentLike.findPage({ @@ -437,21 +634,10 @@ class CommentsService { return result; } - /** - * @param {string} id - The ID of the Comment to get - * @param {any} options - */ - async getCommentByID(id, options) { + async getCommentByID(id, options = {}) { this.checkEnabled(); - const model = await this.models.Comment.findOne({id}, withPinnedSelect(options)); - - if (!model) { - throw new errors.NotFoundError({ - message: tpl(messages.commentNotFound) - }); - } - return model; + return await this.#getReadableCommentByID(id, options); } /** @@ -488,7 +674,7 @@ class CommentsService { member_id: member, parent_id: null, html: comment, - status: 'published' + status: COMMENT_STATUS_PUBLISHED }; if (createdAt) { @@ -531,12 +717,7 @@ class CommentsService { this.checkCommentAccess(memberModel); - const parentComment = await this.getCommentByID(parent, options); - if (!parentComment) { - throw new errors.BadRequestError({ - message: tpl(messages.commentNotFound) - }); - } + const parentComment = await this.#getReplyParentCommentByID(parent, options); if (parentComment.get('parent_id') !== null) { throw new errors.BadRequestError({ @@ -556,18 +737,7 @@ class CommentsService { let inReplyToComment; if (parent && inReplyTo) { - inReplyToComment = await this.getCommentByID(inReplyTo, options); - - // we only allow references to published comments to avoid leaking - // hidden data via the snippet included in API responses - if (inReplyToComment && inReplyToComment.get('status') !== 'published') { - inReplyToComment = null; - } - - // we don't allow in_reply_to references across different parents - if (inReplyToComment && inReplyToComment.get('parent_id') !== parent) { - inReplyToComment = null; - } + inReplyToComment = await this.#getInReplyToCommentByID(inReplyTo, parent, options); } const commentData = { @@ -576,7 +746,7 @@ class CommentsService { parent_id: parentComment.id, in_reply_to_id: inReplyToComment && inReplyToComment.get('id'), html: comment, - status: 'published' + status: COMMENT_STATUS_PUBLISHED }; if (createdAt) { @@ -606,7 +776,7 @@ class CommentsService { */ async deleteComment(id, member, options) { this.checkEnabled(); - const existingComment = await this.getCommentByID(id, options); + const existingComment = await this.#getPublishedCommentForAction(id, options, OWNERSHIP_REQUIRED_COLUMNS); if (existingComment.get('member_id') !== member) { throw new errors.NoPermissionError({ @@ -616,7 +786,7 @@ class CommentsService { } const model = await this.models.Comment.edit({ - status: 'deleted', + status: COMMENT_STATUS_DELETED, pinned_at: null }, { id, @@ -635,11 +805,7 @@ class CommentsService { */ async editCommentContent(id, member, comment, options) { this.checkEnabled(); - const existingComment = await this.getCommentByID(id, options); - - if (!comment) { - return existingComment; - } + const existingComment = await this.#getPublishedCommentForAction(id, options, OWNERSHIP_REQUIRED_COLUMNS); if (existingComment.get('member_id') !== member) { throw new errors.NoPermissionError({ @@ -647,6 +813,10 @@ class CommentsService { }); } + if (!comment) { + return existingComment; + } + const model = await this.models.Comment.edit({ html: comment, edited_at: new Date() diff --git a/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap b/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap index 1c1a8fe9f84..1c186ea7d86 100644 --- a/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap +++ b/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap @@ -2731,54 +2731,6 @@ Object { } `; -exports[`Comments API when commenting enabled for all when authenticated replies to replies cannot set in_reply_to_id to a hidden comment 1: [body] 1`] = ` -Object { - "comments": Array [ - Object { - "count": Object { - "direct_replies": 0, - "likes": Any, - "replies": 0, - }, - "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, - "disliked": false, - "edited_at": null, - "html": "

This is a reply to a reply

", - "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, - "in_reply_to_id": null, - "in_reply_to_snippet": null, - "liked": Any, - "member": Object { - "avatar_image": null, - "expertise": null, - "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, - "name": null, - "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, - }, - "parent_id": Nullable, - "pinned": Any, - "replies": Array [], - "status": "published", - }, - ], -} -`; - -exports[`Comments API when commenting enabled for all when authenticated replies to replies cannot set in_reply_to_id to a hidden comment 2: [headers] 1`] = ` -Object { - "access-control-allow-credentials": "true", - "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": "505", - "content-type": "application/json; charset=utf-8", - "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, - "location": StringMatching /https\\?:\\\\/\\\\/\\.\\*\\?\\\\/comments\\\\/\\[a-f0-9\\]\\{24\\}\\\\//, - "vary": "Origin, Accept-Encoding", - "x-cache-invalidate": StringMatching /\\\\/api\\\\/members\\\\/comments\\\\/post\\\\/\\[0-9a-f\\]\\{24\\}\\\\/, \\\\/api\\\\/members\\\\/comments\\\\/\\[0-9a-f\\]\\{24\\}\\\\/replies\\\\//, - "x-powered-by": "Express", -} -`; - exports[`Comments API when commenting enabled for all when authenticated replies to replies has redacted in_reply_to_snippet when referenced comment is deleted 1: [body] 1`] = ` Object { "comments": Array [ diff --git a/ghost/core/test/e2e-api/members-comments/comments.test.js b/ghost/core/test/e2e-api/members-comments/comments.test.js index 80e8fb271fb..09722b900b0 100644 --- a/ghost/core/test/e2e-api/members-comments/comments.test.js +++ b/ghost/core/test/e2e-api/members-comments/comments.test.js @@ -2087,30 +2087,54 @@ describe('Comments API', function () { }); }); - ['deleted', 'hidden'].forEach((status) => { - it(`cannot set in_reply_to_id to a ${status} comment`, async function () { - const {replies: [reply]} = await dbFns.addCommentWithReplies({ - member_id: fixtureManager.get('members', 1).id, - replies: [{ - member_id: fixtureManager.get('members', 2).id, - status - }] - }); + it('cannot set in_reply_to_id to a deleted comment', async function () { + const {replies: [reply]} = await dbFns.addCommentWithReplies({ + member_id: fixtureManager.get('members', 1).id, + replies: [{ + member_id: fixtureManager.get('members', 2).id, + status: 'deleted' + }] + }); - const {body: {comments: [newComment]}} = await testPostComment({ + const {body: {comments: [newComment]}} = await testPostComment({ + post_id: postId, + parent_id: reply.get('parent_id'), + in_reply_to_id: reply.get('id'), + html: '

This is a reply to a reply

' + }); + + // in_reply_to is not set + assert.equal(newComment.in_reply_to_id, null); + assert.equal(newComment.in_reply_to_snippet, null); + + // only author and parent email sent + emailMockReceiver.assertSentEmailCount(2); + }); + + it('can set in_reply_to_id to a hidden comment with a redacted snippet', async function () { + const {replies: [reply]} = await dbFns.addCommentWithReplies({ + member_id: fixtureManager.get('members', 1).id, + replies: [{ + member_id: fixtureManager.get('members', 2).id, + status: 'hidden' + }] + }); + + const {body: {comments: [newComment]}} = await membersAgent + .post(`/api/comments/`) + .body({comments: [{ post_id: postId, parent_id: reply.get('parent_id'), in_reply_to_id: reply.get('id'), html: '

This is a reply to a reply

' - }); + }]}) + .expectStatus(201); - // in_reply_to is not set - assert.equal(newComment.in_reply_to_id, null); - assert.equal(newComment.in_reply_to_snippet, null); + assert.equal(newComment.in_reply_to_id, reply.get('id')); + assert.equal(newComment.in_reply_to_snippet, '[removed]'); - // only author and parent email sent - emailMockReceiver.assertSentEmailCount(2); - }); + // only author and parent email sent + emailMockReceiver.assertSentEmailCount(2); }); it('in_reply_to_id is ignored when no parent specified', async function () { diff --git a/ghost/core/test/unit/server/services/comments/comments-controller.test.js b/ghost/core/test/unit/server/services/comments/comments-controller.test.js index 521550a0410..ab23cee10d2 100644 --- a/ghost/core/test/unit/server/services/comments/comments-controller.test.js +++ b/ghost/core/test/unit/server/services/comments/comments-controller.test.js @@ -25,12 +25,17 @@ describe('Comments Service: CommentsController', function () { get: key => comment[key] }; const service = { + getComments: sinon.stub().resolves({data: []}), + getAdminAllComments: sinon.stub().resolves({data: []}), + getReplies: sinon.stub().resolves({data: []}), + getAdminReplies: sinon.stub().resolves({data: []}), likeComment: sinon.stub().resolves({id: 'like-id'}), unlikeComment: sinon.stub().resolves(), dislikeComment: sinon.stub().resolves({id: 'dislike-id'}), undislikeComment: sinon.stub().resolves(), getCommentByID: sinon.stub().resolves(commentModel), - getCommentDislikes: sinon.stub().resolves({data: []}) + getCommentDislikes: sinon.stub().resolves({data: []}), + getMemberIdByUUID: sinon.stub().resolves('impersonated-member-id') }; const stats = {}; @@ -107,4 +112,82 @@ describe('Comments Service: CommentsController', function () { limit: 15 }); }); + + it('composes post scoping with existing mongo transformers', async function () { + const {controller, service} = createController(); + const frame = createFrame({ + options: { + post_id: 'post-id', + filter: 'status:published', + mongoTransformer: query => ({existing: query}) + } + }); + + await controller.browse(frame); + + const options = service.getComments.firstCall.args[0]; + assert.deepEqual(options.mongoTransformer({status: 'published'}), { + $and: [ + { + post_id: 'post-id' + }, + { + existing: { + status: 'published' + } + } + ] + }); + }); + + it('uses one service method for admin reply visibility', async function () { + const {controller, service} = createController(); + const frame = createFrame(); + + await controller.adminReplies(frame); + + sinon.assert.notCalled(service.getReplies); + sinon.assert.calledWith(service.getAdminReplies, 'comment-id', sinon.match({ + order: 'created_at asc' + })); + }); + + it('uses explicit admin read options without mutating frame options', async function () { + const {controller, service} = createController(); + const frame = createFrame({ + options: { + impersonate_member_uuid: 'member-uuid' + }, + data: { + id: 'comment-id' + } + }); + + await controller.adminRead(frame); + + assert.equal(frame.options.isAdmin, undefined); + sinon.assert.calledWith(service.getCommentByID, 'comment-id', sinon.match({ + isAdmin: true, + context: { + member: { + id: 'impersonated-member-id' + } + } + })); + }); + + it('rejects unsupported count.reports filters instead of widening the query', async function () { + const {controller, service} = createController(); + const frame = createFrame({ + options: { + filter: 'count.reports:\'many\'' + } + }); + + await assert.rejects( + () => controller.adminBrowseAll(frame), + errors.BadRequestError + ); + sinon.assert.notCalled(service.getAdminAllComments); + }); }); diff --git a/ghost/core/test/unit/server/services/comments/comments-service.test.js b/ghost/core/test/unit/server/services/comments/comments-service.test.js index 27c1e11dc99..af003528296 100644 --- a/ghost/core/test/unit/server/services/comments/comments-service.test.js +++ b/ghost/core/test/unit/server/services/comments/comments-service.test.js @@ -12,10 +12,96 @@ describe('Comments Service: CommentsService', function () { }; } - function createClassInstance({labs = {}, commentsEnabled = 'all'} = {}) { + function buildCommentModel(attributes) { + return { + id: attributes.id, + get: sinon.stub().callsFake(key => attributes[key]) + }; + } + + function createClassInstance({labs = {}, commentsEnabled = 'all', commentStatus = 'published'} = {}) { + let lastCommentColumns; + const commentFetchModels = []; const memberModel = { id: 'member-id', - get: sinon.stub().withArgs('status').returns('paid') + get: sinon.stub().withArgs('status').returns('paid'), + toJSON: sinon.stub().returns({status: 'paid'}) + }; + const postModel = { + id: 'post-id', + toJSON: sinon.stub().returns({id: 'post-id'}) + }; + const commentModel = { + id: 'comment-id', + get: sinon.stub().callsFake((key) => { + const values = { + id: 'comment-id', + post_id: 'post-id', + parent_id: null, + member_id: 'member-id', + html: '

Comment

', + created_at: new Date('2026-01-01T00:00:00.000Z'), + status: commentStatus + }; + + if (lastCommentColumns && !lastCommentColumns.includes(key)) { + return key === 'status' ? 'published' : undefined; + } + + return values[key]; + }) + }; + const commentLikeQuery = { + where: sinon.stub().returnsThis(), + orderBy: sinon.stub().returnsThis() + }; + const commentLikeCollection = { + query: sinon.stub().callsFake((callback) => { + callback(commentLikeQuery); + }), + fetchAll: sinon.stub().resolves({models: []}) + }; + const createCommentFetchModel = () => { + const filters = {}; + const commentFetchQuery = { + where: sinon.stub().callsFake((column, value) => { + filters[column] = value; + return commentFetchQuery; + }), + whereIn: sinon.stub().callsFake((column, value) => { + filters[column] = value; + return commentFetchQuery; + }), + forUpdate: sinon.stub().returnsThis() + }; + const commentFetchModel = { + filters, + query: sinon.stub().callsFake((callback) => { + callback(commentFetchQuery); + return commentFetchModel; + }), + fetch: sinon.stub().callsFake(async (options = {}) => { + lastCommentColumns = options.columns; + const expectedId = filters['comments.id']; + const expectedStatus = filters['comments.status']; + + if (expectedId && expectedId !== commentModel.id) { + return null; + } + + if (Array.isArray(expectedStatus) && !expectedStatus.includes(commentStatus)) { + return null; + } + + if (expectedStatus && !Array.isArray(expectedStatus) && expectedStatus !== commentStatus) { + return null; + } + + return commentModel; + }) + }; + commentFetchModels.push(commentFetchModel); + return commentFetchModel; }; const models = { Base: { @@ -27,13 +113,42 @@ describe('Comments Service: CommentsService', function () { findOne: sinon.stub().resolves(memberModel) }, Comment: { - findPage: sinon.stub().resolves({data: [], meta: {}}) + findOne: sinon.stub().callsFake(async (data, options = {}) => { + lastCommentColumns = options.columns; + if (data.id && data.id !== commentModel.id) { + return null; + } + + // Readable reads constrain status in the query via an NQL filter + // (e.g. `status:[published,hidden]`); mirror that here so a + // non-readable status is excluded just like the real query. + if (options.filter && options.filter.includes('status:') && !options.filter.includes(commentStatus)) { + return null; + } + + if (data.status && data.status !== commentStatus) { + return null; + } + + return commentModel; + }), + forge: sinon.stub().callsFake(createCommentFetchModel), + findPage: sinon.stub().resolves({data: [], meta: {}}), + add: sinon.stub().resolves(commentModel), + edit: sinon.stub().resolves(commentModel) + }, + Post: { + findOne: sinon.stub().resolves(postModel) }, CommentLike: { - findAll: sinon.stub().resolves({models: []}), + forge: sinon.stub().returns(commentLikeCollection), add: sinon.stub().resolves({id: 'like-id'}), edit: sinon.stub().resolves({id: 'like-id'}), destroy: sinon.stub().resolves() + }, + CommentReport: { + findOne: sinon.stub().resolves(null), + add: sinon.stub().resolves({id: 'report-id'}) } }; @@ -52,22 +167,26 @@ describe('Comments Service: CommentsService', function () { settingsHelpers: {}, urlService: {}, urlUtils: {}, - contentGating: {}, + contentGating: { + BLOCK_ACCESS: 'block', + checkPostAccess: sinon.stub().returns('allow') + }, labs: labsStub }); + instance.emails.notifyReport = sinon.stub().resolves(); - return {instance, models, memberModel, labs: labsStub}; + return {instance, models, memberModel, commentModel, commentFetchModels, commentLikeQuery, commentLikeCollection, labs: labsStub}; } describe('likeComment', function () { it('adds a like score when there is no existing vote', async function () { - const {instance, models} = createClassInstance(); + const {instance, models, commentLikeQuery} = createClassInstance(); await instance.likeComment('comment-id', {id: 'member-id'}, {context: {member: {id: 'member-id'}}}); - sinon.assert.calledWith(models.CommentLike.findAll, sinon.match({ - filter: 'comment_id:\'comment-id\'+member_id:\'member-id\'' - })); + sinon.assert.calledWith(commentLikeQuery.where, 'comment_likes.comment_id', 'comment-id'); + sinon.assert.calledWith(commentLikeQuery.where, 'comment_likes.member_id', 'member-id'); + sinon.assert.calledWith(commentLikeQuery.orderBy, 'comment_likes.created_at', 'asc'); sinon.assert.calledWith(models.CommentLike.add, { member_id: 'member-id', comment_id: 'comment-id', @@ -75,14 +194,28 @@ describe('Comments Service: CommentsService', function () { }); }); + it('checks comment status with a lean lookup inside the vote transaction', async function () { + const {instance, models, commentFetchModels} = createClassInstance(); + + await instance.likeComment('comment-id', {id: 'member-id'}, {context: {member: {id: 'member-id'}}}); + + sinon.assert.notCalled(models.Comment.findOne); + assert.equal(commentFetchModels[0].filters['comments.id'], 'comment-id'); + assert.equal(commentFetchModels[0].filters['comments.status'], 'published'); + sinon.assert.calledWith(commentFetchModels[0].fetch, sinon.match({ + transacting: 'transaction', + columns: ['id'] + })); + }); + it('replaces duplicate existing votes with one like score', async function () { - const {instance, models} = createClassInstance(); + const {instance, models, commentLikeCollection} = createClassInstance(); const votes = [ voteModel({id: 'vote-1', score: -1}), voteModel({id: 'vote-2', score: -1}), voteModel({id: 'vote-3', score: 1}) ]; - models.CommentLike.findAll.resolves({models: votes}); + commentLikeCollection.fetchAll.resolves({models: votes}); await instance.likeComment('comment-id', {id: 'member-id'}, {context: {member: {id: 'member-id'}}}); @@ -100,9 +233,9 @@ describe('Comments Service: CommentsService', function () { }); it('rejects one existing like without rewriting the row', async function () { - const {instance, models} = createClassInstance(); + const {instance, models, commentLikeCollection} = createClassInstance(); const vote = voteModel({id: 'vote-1', score: 1}); - models.CommentLike.findAll.resolves({models: [vote]}); + commentLikeCollection.fetchAll.resolves({models: [vote]}); await assert.rejects( () => instance.likeComment('comment-id', {id: 'member-id'}, {context: {member: {id: 'member-id'}}}), @@ -116,9 +249,9 @@ describe('Comments Service: CommentsService', function () { describe('dislikeComment', function () { it('replaces an existing like score with a dislike score', async function () { - const {instance, models} = createClassInstance(); + const {instance, models, commentLikeCollection} = createClassInstance(); const vote = voteModel({id: 'vote-id', score: 1}); - models.CommentLike.findAll.resolves({models: [vote]}); + commentLikeCollection.fetchAll.resolves({models: [vote]}); await instance.dislikeComment('comment-id', {id: 'member-id'}, {context: {member: {id: 'member-id'}}}); @@ -146,42 +279,298 @@ describe('Comments Service: CommentsService', function () { describe('unlikeComment', function () { it('removes every positive duplicate vote', async function () { - const {instance, models} = createClassInstance(); + const {instance, commentLikeCollection, commentLikeQuery} = createClassInstance(); const positiveVotes = [ voteModel({id: 'vote-1', score: 1}), voteModel({id: 'vote-2', score: 1}) ]; - const negativeVote = voteModel({id: 'vote-3', score: -1}); - models.CommentLike.findAll.resolves({models: [...positiveVotes, negativeVote]}); + commentLikeCollection.fetchAll.resolves({models: positiveVotes}); await instance.unlikeComment('comment-id', {id: 'member-id'}); + sinon.assert.calledWith(commentLikeQuery.where, 'comment_likes.comment_id', 'comment-id'); + sinon.assert.calledWith(commentLikeQuery.where, 'comment_likes.member_id', 'member-id'); + sinon.assert.calledWith(commentLikeQuery.where, 'comment_likes.score', 1); positiveVotes.forEach((vote) => { sinon.assert.calledOnce(vote.destroy); }); - sinon.assert.notCalled(negativeVote.destroy); }); }); describe('undislikeComment', function () { it('removes every negative duplicate vote', async function () { - const {instance, models} = createClassInstance(); - const positiveVote = voteModel({id: 'vote-1', score: 1}); + const {instance, commentLikeCollection, commentLikeQuery} = createClassInstance(); const negativeVotes = [ voteModel({id: 'vote-2', score: -1}), voteModel({id: 'vote-3', score: -1}) ]; - models.CommentLike.findAll.resolves({models: [positiveVote, ...negativeVotes]}); + commentLikeCollection.fetchAll.resolves({models: negativeVotes}); await instance.undislikeComment('comment-id', {id: 'member-id'}); - sinon.assert.notCalled(positiveVote.destroy); + sinon.assert.calledWith(commentLikeQuery.where, 'comment_likes.score', -1); negativeVotes.forEach((vote) => { sinon.assert.calledOnce(vote.destroy); }); }); }); + describe('comment votes on non-public comments', function () { + async function assertVoteActionRejects(status, run) { + const {instance, models, commentLikeCollection} = createClassInstance({commentStatus: status}); + + await assert.rejects( + () => run(instance), + errors.NotFoundError + ); + + sinon.assert.notCalled(commentLikeCollection.fetchAll); + sinon.assert.notCalled(models.CommentLike.add); + } + + it('does not add votes to hidden or deleted comments', async function () { + for (const status of ['hidden', 'deleted']) { + await assertVoteActionRejects(status, (instance) => { + return instance.likeComment('comment-id', {id: 'member-id'}, { + columns: ['id'], + context: {member: {id: 'member-id'}} + }); + }); + } + }); + + it('does not clear votes from hidden or deleted comments', async function () { + for (const status of ['hidden', 'deleted']) { + await assertVoteActionRejects(status, (instance) => { + return instance.unlikeComment('comment-id', {id: 'member-id'}, { + columns: ['id'], + context: {member: {id: 'member-id'}} + }); + }); + } + }); + }); + + describe('reportComment', function () { + it('creates a report for a published comment', async function () { + const {instance, models, commentModel} = createClassInstance(); + const reporter = {id: 'member-id'}; + instance.emails.notifyReport.callsFake((comment) => { + assert.equal(comment.get('post_id'), 'post-id'); + assert.equal(comment.get('member_id'), 'member-id'); + assert.equal(comment.get('html'), '

Comment

'); + assert.deepEqual(comment.get('created_at'), new Date('2026-01-01T00:00:00.000Z')); + }); + + await instance.reportComment('comment-id', reporter, {columns: ['id'], context: {member: reporter}}); + + sinon.assert.calledWith(models.CommentReport.findOne, { + comment_id: 'comment-id', + member_id: 'member-id' + }); + sinon.assert.calledWith(models.CommentReport.add, { + comment_id: 'comment-id', + member_id: 'member-id' + }); + sinon.assert.calledWith(instance.emails.notifyReport, commentModel, reporter); + }); + + it('does not report hidden or deleted comments', async function () { + for (const status of ['hidden', 'deleted']) { + const {instance, models} = createClassInstance({commentStatus: status}); + const reporter = {id: 'member-id'}; + + await assert.rejects( + () => instance.reportComment('comment-id', reporter, {columns: ['id'], context: {member: reporter}}), + errors.NotFoundError + ); + + sinon.assert.notCalled(models.CommentReport.findOne); + sinon.assert.notCalled(models.CommentReport.add); + sinon.assert.notCalled(instance.emails.notifyReport); + } + }); + }); + + describe('getCommentByID', function () { + it('uses an exact comment lookup for member-facing direct reads', async function () { + const {instance, models} = createClassInstance(); + const id = 'missing\',id:\'comment-id'; + + await assert.rejects( + () => instance.getCommentByID(id, {context: {member: {id: 'member-id'}}}), + errors.NotFoundError + ); + + // The id is still matched via the data hash (not interpolated into the + // status filter), so a crafted id stays inert: one keyed findOne, no findPage. + sinon.assert.calledOnce(models.Comment.findOne); + sinon.assert.calledWith(models.Comment.findOne, { + id + }); + sinon.assert.notCalled(models.Comment.findPage); + }); + + it('returns hidden comments to member-facing direct reads', async function () { + const {instance, models, commentModel} = createClassInstance({commentStatus: 'hidden'}); + + const result = await instance.getCommentByID('comment-id', {columns: ['id'], context: {member: {id: 'member-id'}}}); + + assert.equal(result, commentModel); + // A single row is fetched and its status checked in memory, rather than + // one query per allowed status. + sinon.assert.calledOnce(models.Comment.findOne); + sinon.assert.calledWith(models.Comment.findOne, { + id: 'comment-id' + }); + }); + + it('does not return deleted comments to member-facing direct reads', async function () { + const {instance} = createClassInstance({commentStatus: 'deleted'}); + + await assert.rejects( + () => instance.getCommentByID('comment-id', {columns: ['id'], context: {member: {id: 'member-id'}}}), + errors.NotFoundError + ); + }); + }); + + describe('replyToComment', function () { + it('allows replies to hidden parent comments even when requested fields omit internal columns', async function () { + const {instance, models} = createClassInstance({commentStatus: 'hidden'}); + + await instance.replyToComment('comment-id', undefined, 'member-id', '

Reply

', { + columns: ['id'], + context: { + internal: true, + member: {id: 'member-id'} + } + }); + + sinon.assert.calledWith(models.Comment.add, sinon.match({ + post_id: 'post-id', + member_id: 'member-id', + parent_id: 'comment-id', + html: '

Reply

', + status: 'published' + })); + }); + + it('keeps hidden in_reply_to links so live descendants stay threaded', async function () { + const {instance, models} = createClassInstance(); + const parentComment = buildCommentModel({ + id: 'parent-id', + post_id: 'post-id', + parent_id: null, + status: 'published' + }); + const hiddenReply = buildCommentModel({ + id: 'reply-id', + post_id: 'post-id', + parent_id: 'parent-id', + status: 'hidden' + }); + const commentsById = {'parent-id': parentComment, 'reply-id': hiddenReply}; + + // Parent and in_reply_to are resolved through the lean lookup, which + // applies the allowed statuses in the query — so the fake filters on the + // captured `whereIn`/`where` values rather than returning blindly by id. + models.Comment.forge.callsFake(() => { + const filters = {}; + const query = { + where: sinon.stub().callsFake((column, value) => { + filters[column] = value; + return query; + }), + whereIn: sinon.stub().callsFake((column, value) => { + filters[column] = value; + return query; + }), + forUpdate: sinon.stub().returnsThis() + }; + const model = { + query: sinon.stub().callsFake((callback) => { + callback(query); + return model; + }), + fetch: sinon.stub().callsFake(async () => { + const target = commentsById[filters['comments.id']]; + if (!target) { + return null; + } + + const statuses = filters['comments.status']; + if (Array.isArray(statuses) && !statuses.includes(target.get('status'))) { + return null; + } + + return target; + }) + }; + return model; + }); + + await instance.replyToComment('parent-id', 'reply-id', 'member-id', '

Reply

', { + columns: ['id'], + context: { + internal: true, + member: {id: 'member-id'} + } + }); + + sinon.assert.calledWith(models.Comment.add, sinon.match({ + post_id: 'post-id', + member_id: 'member-id', + parent_id: 'parent-id', + in_reply_to_id: 'reply-id', + html: '

Reply

', + status: 'published' + })); + }); + + it('drops the in_reply_to link instead of rejecting when the target does not exist', async function () { + // The concession: a deleted/missing/cross-thread target is simply not a + // valid anchor, so the reply still posts without the reference rather than + // failing outright. (Deleted and missing now behave identically.) + const {instance, models} = createClassInstance(); + + await instance.replyToComment('comment-id', 'does-not-exist', 'member-id', '

Reply

', { + context: {internal: true, member: {id: 'member-id'}} + }); + + sinon.assert.calledWith(models.Comment.add, sinon.match({ + parent_id: 'comment-id', + in_reply_to_id: null, + html: '

Reply

', + status: 'published' + })); + }); + }); + + describe('editCommentContent', function () { + it('checks ownership before returning an unchanged comment', async function () { + const {instance, commentModel} = createClassInstance(); + commentModel.get.withArgs('member_id').returns('owner-id'); + + await assert.rejects( + () => instance.editCommentContent('comment-id', 'member-id', undefined, {context: {member: {id: 'member-id'}}}), + errors.NoPermissionError + ); + }); + + it('selects member_id before checking ownership when requested fields omit it', async function () { + const {instance, models, commentModel} = createClassInstance(); + + const result = await instance.editCommentContent('comment-id', 'member-id', undefined, { + columns: ['id'], + context: {member: {id: 'member-id'}} + }); + + assert.equal(result, commentModel); + sinon.assert.notCalled(models.Comment.edit); + }); + }); + describe('getComments', function () { it('preserves net score ordering without a labs flag', async function () { const {instance, models} = createClassInstance(); From 3188b993c4efa0ebca992ae635390c7a08be0ad0 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 4 Jun 2026 14:18:15 +0000 Subject: [PATCH 05/10] Changed "toPlain" helper to TypeScript (#28365) no ref --- .../lib/common/{to-plain.js => to-plain.ts} | 22 +++++++++++-------- .../audience-feedback-service.js | 2 +- .../comments/comments-service-emails.js | 2 +- .../member-attribution/url-translator.js | 2 +- .../unit/server/lib/common/to-plain.test.js | 2 +- 5 files changed, 17 insertions(+), 13 deletions(-) rename ghost/core/core/server/lib/common/{to-plain.js => to-plain.ts} (52%) diff --git a/ghost/core/core/server/lib/common/to-plain.js b/ghost/core/core/server/lib/common/to-plain.ts similarity index 52% rename from ghost/core/core/server/lib/common/to-plain.js rename to ghost/core/core/server/lib/common/to-plain.ts index 5c5e4d4ab19..f73aa953453 100644 --- a/ghost/core/core/server/lib/common/to-plain.js +++ b/ghost/core/core/server/lib/common/to-plain.ts @@ -8,14 +8,18 @@ * * const data = toPlain(post); * urlService.facade.getUrlForResource({...data, type: 'posts'}, ...) - * - * @template T - * @param {T | {toJSON(): T}} modelOrObj - * @returns {T} */ -module.exports = function toPlain(modelOrObj) { - if (modelOrObj && typeof modelOrObj.toJSON === 'function') { - return modelOrObj.toJSON(); - } - return modelOrObj; +type JsonSerializable = { + toJSON(): T; }; + +const isJsonSerializable = (modelOrObj: T | JsonSerializable): modelOrObj is JsonSerializable => !!( + modelOrObj + && typeof modelOrObj === 'object' + && 'toJSON' in modelOrObj + && typeof modelOrObj.toJSON === 'function' +); + +export const toPlain = (modelOrObj: T | JsonSerializable): T => ( + isJsonSerializable(modelOrObj) ? modelOrObj.toJSON() : modelOrObj +); diff --git a/ghost/core/core/server/services/audience-feedback/audience-feedback-service.js b/ghost/core/core/server/services/audience-feedback/audience-feedback-service.js index cfeb50bc699..93a42221091 100644 --- a/ghost/core/core/server/services/audience-feedback/audience-feedback-service.js +++ b/ghost/core/core/server/services/audience-feedback/audience-feedback-service.js @@ -1,4 +1,4 @@ -const toPlain = require('../../lib/common/to-plain'); +const {toPlain} = require('../../lib/common/to-plain'); class AudienceFeedbackService { /** @type URL */ diff --git a/ghost/core/core/server/services/comments/comments-service-emails.js b/ghost/core/core/server/services/comments/comments-service-emails.js index f7c774f4461..e42c6b35221 100644 --- a/ghost/core/core/server/services/comments/comments-service-emails.js +++ b/ghost/core/core/server/services/comments/comments-service-emails.js @@ -2,7 +2,7 @@ const moment = require('moment'); const htmlToPlaintext = require('@tryghost/html-to-plaintext'); const emailService = require('../email-service'); const CommentsServiceEmailRenderer = require('./comments-service-email-renderer'); -const toPlain = require('../../lib/common/to-plain'); +const {toPlain} = require('../../lib/common/to-plain'); const {t} = require('../i18n'); class CommentsServiceEmails { diff --git a/ghost/core/core/server/services/member-attribution/url-translator.js b/ghost/core/core/server/services/member-attribution/url-translator.js index 30d6c732cf8..8642aaeee93 100644 --- a/ghost/core/core/server/services/member-attribution/url-translator.js +++ b/ghost/core/core/server/services/member-attribution/url-translator.js @@ -6,7 +6,7 @@ * }} facade */ -const toPlain = require('../../lib/common/to-plain'); +const {toPlain} = require('../../lib/common/to-plain'); const TYPE_TO_RESOURCE = { post: 'posts', diff --git a/ghost/core/test/unit/server/lib/common/to-plain.test.js b/ghost/core/test/unit/server/lib/common/to-plain.test.js index dc530bebaf1..5d50b847ac0 100644 --- a/ghost/core/test/unit/server/lib/common/to-plain.test.js +++ b/ghost/core/test/unit/server/lib/common/to-plain.test.js @@ -1,5 +1,5 @@ const assert = require('node:assert/strict'); -const toPlain = require('../../../../../core/server/lib/common/to-plain'); +const {toPlain} = require('../../../../../core/server/lib/common/to-plain'); describe('toPlain', function () { it('returns a plain object unchanged', function () { From 958534cd73e6050acb691819561d56a31afa218e Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 4 Jun 2026 14:18:48 +0000 Subject: [PATCH 06/10] Convert frontend routing config to TypeScript (#28363) no ref This change should have no user impact. --- .../frontend/services/routing/{config.js => config.ts} | 8 ++++---- .../core/core/server/services/route-settings/validate.js | 3 ++- .../frontend/services/routing/taxonomy-router.test.js | 3 ++- 3 files changed, 8 insertions(+), 6 deletions(-) rename ghost/core/core/frontend/services/routing/{config.js => config.ts} (93%) diff --git a/ghost/core/core/frontend/services/routing/config.js b/ghost/core/core/frontend/services/routing/config.ts similarity index 93% rename from ghost/core/core/frontend/services/routing/config.js rename to ghost/core/core/frontend/services/routing/config.ts index a45f03b389b..71aa21c6d6c 100644 --- a/ghost/core/core/frontend/services/routing/config.js +++ b/ghost/core/core/frontend/services/routing/config.ts @@ -1,4 +1,4 @@ -module.exports.QUERY = { +export const QUERY = { tag: { controller: 'tagsPublic', type: 'read', @@ -43,9 +43,9 @@ module.exports.QUERY = { slug: '%s' } } -}; +} as const; -module.exports.TAXONOMIES = { +export const TAXONOMIES = { tag: { filter: 'tags:\'%s\'+tags.visibility:public', editRedirect: '#/tags/:slug/', @@ -56,4 +56,4 @@ module.exports.TAXONOMIES = { editRedirect: '#/settings/staff/:slug/', resource: 'authors' } -}; +} as const; diff --git a/ghost/core/core/server/services/route-settings/validate.js b/ghost/core/core/server/services/route-settings/validate.js index 8d2e71bc154..7cbb1b5e874 100644 --- a/ghost/core/core/server/services/route-settings/validate.js +++ b/ghost/core/core/server/services/route-settings/validate.js @@ -429,7 +429,8 @@ module.exports = function validate(object) { } // TODO: extract this config outta here! the config should be passed into this module - RESOURCE_CONFIG = require('../../../frontend/services/routing/config'); + const {QUERY, TAXONOMIES} = require('../../../frontend/services/routing/config'); + RESOURCE_CONFIG = {QUERY, TAXONOMIES}; object.routes = _private.validateRoutes(object.routes); object.collections = _private.validateCollections(object.collections); diff --git a/ghost/core/test/unit/frontend/services/routing/taxonomy-router.test.js b/ghost/core/test/unit/frontend/services/routing/taxonomy-router.test.js index fed26704766..245a23d7408 100644 --- a/ghost/core/test/unit/frontend/services/routing/taxonomy-router.test.js +++ b/ghost/core/test/unit/frontend/services/routing/taxonomy-router.test.js @@ -5,7 +5,8 @@ const settingsCache = require('../../../../../core/shared/settings-cache'); const controllers = require('../../../../../core/frontend/services/routing/controllers'); const TaxonomyRouter = require('../../../../../core/frontend/services/routing/taxonomy-router'); -const RESOURCE_CONFIG = require('../../../../../core/frontend/services/routing/config'); +const {QUERY, TAXONOMIES} = require('../../../../../core/frontend/services/routing/config'); +const RESOURCE_CONFIG = {QUERY, TAXONOMIES}; describe('UNIT - services/routing/TaxonomyRouter', function () { let req; From 0f7cecefb0208c7e8d3614e6ab9cc4eccee6736a Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 4 Jun 2026 15:02:13 +0000 Subject: [PATCH 07/10] Convert role utils model helper to TypeScript (#28367) no ref This change should have no user impact. --- .../models/{role-utils.js => role-utils.ts} | 30 +++++++++++++++---- ...-is-roles.test.js => set-is-roles.test.ts} | 8 ++--- 2 files changed, 27 insertions(+), 11 deletions(-) rename ghost/core/core/server/models/{role-utils.js => role-utils.ts} (67%) rename ghost/core/test/unit/server/models/{set-is-roles.test.js => set-is-roles.test.ts} (95%) diff --git a/ghost/core/core/server/models/role-utils.js b/ghost/core/core/server/models/role-utils.ts similarity index 67% rename from ghost/core/core/server/models/role-utils.js rename to ghost/core/core/server/models/role-utils.ts index 307449c55bb..61fe10a5844 100644 --- a/ghost/core/core/server/models/role-utils.js +++ b/ghost/core/core/server/models/role-utils.ts @@ -1,8 +1,29 @@ +import type {ReadonlyDeep} from 'type-fest'; + +type LoadedPermissionsWithRoles = ReadonlyDeep<{ + user?: { + roles?: Array<{name: string}>; + } | null; +}>; + +type RoleFlags = { + isOwner: boolean; + isAdmin: boolean; + isEditor: boolean; + isAuthor: boolean; + isContributor: boolean; + isSuperEditor: boolean; + isEitherEditor: boolean; +}; + // check if the user has an assigned role // so that we can stop writing this everywhere: //_.some(loadedPermissions.user.roles, {name: 'Administrator'}) -function checkUserPermissionsForRole(loadedPermissions, roleName) { +export function checkUserPermissionsForRole( + loadedPermissions: LoadedPermissionsWithRoles | null | undefined, + roleName: string +): boolean { if (!loadedPermissions?.user?.roles) { return false; } @@ -10,9 +31,9 @@ function checkUserPermissionsForRole(loadedPermissions, roleName) { return loadedPermissions.user.roles.some(role => role.name === roleName); } -function setIsRoles(loadedPermissions) { +export function setIsRoles(loadedPermissions: LoadedPermissionsWithRoles | null | undefined): RoleFlags { // utility function to parse the permissions object and set up all the "is" variables. - let resultsObject = { + const resultsObject: RoleFlags = { isOwner: false, isAdmin: false, isEditor: false, @@ -33,6 +54,3 @@ function setIsRoles(loadedPermissions) { resultsObject.isEitherEditor = resultsObject.isEditor || resultsObject.isSuperEditor; return resultsObject; } - -exports.setIsRoles = setIsRoles; -exports.checkUserPermissionsForRole = checkUserPermissionsForRole; \ No newline at end of file diff --git a/ghost/core/test/unit/server/models/set-is-roles.test.js b/ghost/core/test/unit/server/models/set-is-roles.test.ts similarity index 95% rename from ghost/core/test/unit/server/models/set-is-roles.test.js rename to ghost/core/test/unit/server/models/set-is-roles.test.ts index 6c5798b543b..34a3e37c1f3 100644 --- a/ghost/core/test/unit/server/models/set-is-roles.test.js +++ b/ghost/core/test/unit/server/models/set-is-roles.test.ts @@ -1,7 +1,6 @@ -const assert = require('node:assert/strict'); -const {assertExists} = require('../../../utils/assertions'); -const {setIsRoles} = require('../../../../core/server/models/role-utils'); -const _ = require('lodash'); +import assert from 'node:assert/strict'; +import _ from 'lodash'; +import {setIsRoles} from '../../../../core/server/models/role-utils'; describe('setIsRoles function behavior', function () { // create a fake 'loadedpermissions' object and then confirm the behavior of setIsRoles with it @@ -61,7 +60,6 @@ describe('setIsRoles function behavior', function () { it('returns an object', function () { let result = setIsRoles(loadedPermissionsEditor); - assertExists(result); assert(_.isPlainObject(result)); }); From 97f6da0b2953a961d40aa000117467d316a77acc Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Thu, 4 Jun 2026 10:23:12 -0500 Subject: [PATCH 08/10] Named the missing module in adapter dependency errors (#28355) Follow-up to #44af74370c, which made the adapter "missing dependency" error reachable again. That error said dependencies were missing but not *which* one. This parses the module name from the `MODULE_NOT_FOUND` first line and includes it: > You are missing a dependency `'superagent'` in your adapter `core/server/adapters/scheduling/SchedulingPro` Falls back to the existing wording if no specifier can be parsed. Minor actionability improvement, no behavior change. - `adapter-manager.js`: extract and include the unresolved module name - `adapter-manager.test.js`: assert the name appears in the error --- .../core/server/services/adapter-manager/adapter-manager.js | 6 +++++- .../server/services/adapter-manager/adapter-manager.test.js | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/ghost/core/core/server/services/adapter-manager/adapter-manager.js b/ghost/core/core/server/services/adapter-manager/adapter-manager.js index ea94daf3c75..158e0370f5a 100644 --- a/ghost/core/core/server/services/adapter-manager/adapter-manager.js +++ b/ghost/core/core/server/services/adapter-manager/adapter-manager.js @@ -140,8 +140,12 @@ module.exports = class AdapterManager { // that includes the adapter's own path, which would false-positive. const firstLine = err.message.split('\n')[0]; if (!firstLine.includes(pathToAdapter)) { + // Name the unresolved module so the error is actionable, e.g. + // "Cannot find module 'superagent'" -> 'superagent'. + const missingMatch = /Cannot find module '([^']+)'/.exec(firstLine); + const missingModule = missingMatch ? ` '${missingMatch[1]}'` : ''; throw new errors.IncorrectUsageError({ - message: `You are missing dependencies in your adapter ${pathToAdapter}`, + message: `You are missing a dependency${missingModule} in your adapter ${pathToAdapter}`, err }); } diff --git a/ghost/core/test/unit/server/services/adapter-manager/adapter-manager.test.js b/ghost/core/test/unit/server/services/adapter-manager/adapter-manager.test.js index c68974062a3..75ceced45f0 100644 --- a/ghost/core/test/unit/server/services/adapter-manager/adapter-manager.test.js +++ b/ghost/core/test/unit/server/services/adapter-manager/adapter-manager.test.js @@ -105,7 +105,8 @@ describe('AdapterManager', function () { adapterManager.getAdapter('scheduling', 'BrokenAdapter', {}); }, { errorType: 'IncorrectUsageError', - message: /missing dependencies/ + // The error names the unresolved module so it's actionable + message: /missing a dependency 'this-package-does-not-exist-at-all' in your adapter/ }); } finally { fs.rmSync(tmpDir, {recursive: true, force: true}); From 4ec28b7d339980c9124e2b67a95551569a14e0a5 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 4 Jun 2026 16:47:09 +0100 Subject: [PATCH 09/10] Fixed admin search index refetching every keystroke (#28370) closes https://linear.app/ghost/issue/ONC-1808/ ## Summary - Keeps admin search index refreshes alive across restartable search keystrokes - Expires cached admin search content when indexed post fields change or posts are deleted - Adds the direct chalk dependency required by the admin Ember build under pnpm ## Root cause `searchTask` is restartable, so refresh task instances started from it were cancelled on later keystrokes while the underlying AJAX requests continued. That left the cache stale and allowed another full search-index refresh to start. The fix starts refreshes with `refreshContentTask.unlinked().perform()` so the refresh task is not linked to the restartable search task that triggered it. That lets the in-flight index refresh complete and update the shared cache, while the provider's `drop` task behavior still prevents duplicate refreshes from starting. --- ghost/admin/app/models/post.js | 13 +++- ghost/admin/app/services/search.js | 2 +- ghost/admin/package.json | 1 + .../tests/integration/models/post-test.js | 25 ++++++- .../tests/integration/services/search-test.js | 69 +++++++++++++++++++ pnpm-lock.yaml | 6 ++ pnpm-workspace.yaml | 1 + 7 files changed, 111 insertions(+), 6 deletions(-) diff --git a/ghost/admin/app/models/post.js b/ghost/admin/app/models/post.js index 6fa731f5018..b61faa128cf 100644 --- a/ghost/admin/app/models/post.js +++ b/ghost/admin/app/models/post.js @@ -11,6 +11,7 @@ import {on} from '@ember/object/evented'; import {inject as service} from '@ember/service'; const BLANK_LEXICAL = '{"root":{"children":[{"children":[],"direction":null,"format":"","indent":0,"type":"paragraph","version":1}],"direction":null,"format":"","indent":0,"type":"root","version":1}}'; +const SEARCH_INDEXED_FIELDS = ['title', 'slug', 'status', 'visibility', 'publishedAtUTC']; // ember-cli-shims doesn't export these so we must get them manually const {Comparable} = Ember; @@ -440,12 +441,18 @@ export default Model.extend(Comparable, ValidationEngine, { this.set('publishedAtUTC', publishedAtUTC); }, - // when a published post is updated, unpublished, or deleted we expire the search content cache + // when indexed post fields are updated or deleted we expire the search content cache save() { - const [oldStatus] = this.changedAttributes().status || []; + const changedAttributes = this.changedAttributes(); + const previousUrl = this.url; + const previousPublishedAtUTC = this.publishedAtUTC?.valueOf(); + const searchIndexedFieldChanged = SEARCH_INDEXED_FIELDS.some(field => changedAttributes[field]); return this._super(...arguments).then((res) => { - if (this.status === 'published' || oldStatus === 'published') { + const urlChanged = previousUrl !== this.url; + const publishedAtUTCChanged = previousPublishedAtUTC !== this.publishedAtUTC?.valueOf(); + + if (this.isDeleted || searchIndexedFieldChanged || urlChanged || publishedAtUTCChanged) { this.search.expireContent(); } diff --git a/ghost/admin/app/services/search.js b/ghost/admin/app/services/search.js index f8e3f9e5009..2a93d1d6d5f 100644 --- a/ghost/admin/app/services/search.js +++ b/ghost/admin/app/services/search.js @@ -32,7 +32,7 @@ export default class SearchService extends Service { } // start loading immediately in the background - this.refreshContentTask.perform(); + this.refreshContentTask.unlinked().perform(); // debounce searches to 200ms to avoid thrashing CPU yield timeout(200); diff --git a/ghost/admin/package.json b/ghost/admin/package.json index 9d1f736c1c4..e844220b902 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -65,6 +65,7 @@ "broccoli-terser-sourcemap": "4.1.1", "chai": "catalog:", "chai-dom": "1.12.1", + "chalk": "catalog:", "codemirror": "5.65.21", "cssnano": "4.1.10", "element-resize-detector": "1.2.4", diff --git a/ghost/admin/tests/integration/models/post-test.js b/ghost/admin/tests/integration/models/post-test.js index 506521b0d65..9f26a6dce0b 100644 --- a/ghost/admin/tests/integration/models/post-test.js +++ b/ghost/admin/tests/integration/models/post-test.js @@ -21,10 +21,21 @@ describe('Integration: Model: post', function () { search.isContentStale = false; }); - it('expires on published save', async function () { + it('expires when published title changes', async function () { const serverPost = this.server.create('post', {status: 'published'}); const postModel = await store.find('post', serverPost.id); + postModel.title = 'New title'; + await postModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('expires when draft title changes', async function () { + const serverPost = this.server.create('post', {status: 'draft'}); + + const postModel = await store.find('post', serverPost.id); + postModel.title = 'New title'; await postModel.save(); expect(search.isContentStale, 'stale flag after save').to.be.true; @@ -68,12 +79,22 @@ describe('Integration: Model: post', function () { expect(search.isContentStale, 'stale flag after save').to.be.false; }); - it('does not expire on draft delete', async function () { + it('expires on draft delete', async function () { const serverPost = this.server.create('post', {status: 'draft'}); const postModel = await store.find('post', serverPost.id); await postModel.destroyRecord(); + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('does not expire when non-search content changes', async function () { + const serverPost = this.server.create('post', {status: 'published'}); + + const postModel = await store.find('post', serverPost.id); + postModel.html = '

Updated content

'; + await postModel.save(); + expect(search.isContentStale, 'stale flag after save').to.be.false; }); }); diff --git a/ghost/admin/tests/integration/services/search-test.js b/ghost/admin/tests/integration/services/search-test.js index 587c2ab87c9..1625138e19c 100644 --- a/ghost/admin/tests/integration/services/search-test.js +++ b/ghost/admin/tests/integration/services/search-test.js @@ -1,3 +1,4 @@ +import sinon from 'sinon'; import {authenticateSession} from 'ember-simple-auth/test-support'; import {describe, it} from 'mocha'; import {expect} from 'chai'; @@ -29,6 +30,16 @@ const suites = [{ } }]; +function searchIndexRequests(server) { + return server.pretender.handledRequests.filter(request => request.url.includes('/search-index/')); +} + +function wait(ms) { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + suites.forEach((suite) => { describe(suite.name, function () { const hooks = setupTest(); @@ -57,6 +68,10 @@ suites.forEach((suite) => { firstUser = this.server.create('user', {name: 'First user', slug: 'first-user'}); }); + afterEach(function () { + sinon.restore(); + }); + it('is using correct provider', async function () { suite.confirmProvider.bind(this)(); }); @@ -76,5 +91,59 @@ suites.forEach((suite) => { expect(results[0].options[0].visibility).to.equal('members'); expect(results[0].options[0].publishedAt).to.equal('2024-05-08T16:21:07.000Z'); }); + + it('does not refresh cached content for subsequent searches', async function () { + await search.searchTask.perform('first'); + + expect(searchIndexRequests(this.server), 'initial search index requests').to.have.length(4); + + await search.searchTask.perform('post'); + + expect(searchIndexRequests(this.server), 'later search index requests').to.have.length(4); + }); + + it('keeps the content cache fresh after a restarted search waits for an in-flight refresh', async function () { + this.server.timing = 500; + + search.searchTask.perform('fir'); + await search.searchTask.perform('first'); + + expect(searchIndexRequests(this.server), 'initial search index requests').to.have.length(4); + + await search.searchTask.perform('post'); + + expect(searchIndexRequests(this.server), 'later search index requests').to.have.length(4); + }); + + it('keeps one provider refresh alive across restarted searches', async function () { + let resolveRefresh; + const provider = search.provider; + const refreshPromise = new Promise((resolve) => { + resolveRefresh = resolve; + }); + + sinon.stub(provider.refreshContentTask, 'perform').returns(refreshPromise); + sinon.stub(provider.searchTask, 'perform').returns([]); + + search.searchTask.perform('t'); + await wait(250); + + search.searchTask.perform('te'); + await wait(250); + + const finalSearch = search.searchTask.perform('tes'); + await wait(250); + + expect(provider.refreshContentTask.perform, 'provider refresh starts').to.have.been.calledOnce; + + resolveRefresh(); + await finalSearch; + + expect(search.isContentStale, 'stale flag after shared refresh').to.be.false; + + await search.searchTask.perform('post'); + + expect(provider.refreshContentTask.perform, 'provider refresh starts after cached search').to.have.been.calledOnce; + }); }); }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9e0982d76db..f6be1e15a70 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -156,6 +156,9 @@ catalogs: chai: specifier: 4.5.0 version: 4.5.0 + chalk: + specifier: 4.1.2 + version: 4.1.2 clsx: specifier: 2.1.1 version: 2.1.1 @@ -2044,6 +2047,9 @@ importers: chai-dom: specifier: 1.12.1 version: 1.12.1(chai@4.5.0) + chalk: + specifier: 'catalog:' + version: 4.1.2 codemirror: specifier: 5.65.21 version: 5.65.21 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 25cd0fe9272..6fcd9b1a6be 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -76,6 +76,7 @@ catalog: bson-objectid: 2.0.4 c8: 11.0.0 chai: 4.5.0 + chalk: 4.1.2 clsx: 2.1.1 concurrently: 10.0.0 cross-fetch: 4.1.0 From 4f9062007c9cf5f956d5cf49dfdd539437604c94 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Thu, 4 Jun 2026 10:47:26 -0500 Subject: [PATCH 10/10] Gated self-hosted Renovate workflow to TryGhost/Ghost (#28372) no ref --- .github/workflows/renovate.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/renovate.yml b/.github/workflows/renovate.yml index e6120a29cb0..82b46768ced 100644 --- a/.github/workflows/renovate.yml +++ b/.github/workflows/renovate.yml @@ -48,6 +48,11 @@ permissions: jobs: renovate: + # Never run on forks: both `schedule` and `workflow_dispatch` can fire on a + # fork that has Actions enabled, and this job mints the Renovate GitHub App + # token. Matches the fork guard used across the rest of the repo's + # privileged workflows. + if: github.repository == 'TryGhost/Ghost' runs-on: ubuntu-latest timeout-minutes: 45