From f51236f016264d97cf2aff3ecdd754415c6ed9ff Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 10 Aug 2026 17:29:39 +0100 Subject: [PATCH 01/24] Fixed the IndexNow development unit test (#29859) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Fixes the IndexNow development guard test on `main`. - Updates the assertion to use the current direct URL-service dependency rather than the removed `facade` shape. - Contains no runtime behavior changes. ## Context The stale assertion was introduced when #29781 merged after #29792 changed the URL-service dependency shape. The `main` CI run for #29781 was cancelled by the following merge, so the interaction surfaced in [the next `main` run](https://github.com/TryGhost/Ghost/actions/runs/31405973089/job/93513219875). ## Testing - [x] `pnpm test:single test/unit/server/services/indexnow-ping/indexnow-ping-service.test.js` — 28 passed - [x] ESLint pre-commit check --- .../server/services/indexnow-ping/indexnow-ping-service.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ghost/core/test/unit/server/services/indexnow-ping/indexnow-ping-service.test.js b/ghost/core/test/unit/server/services/indexnow-ping/indexnow-ping-service.test.js index ba095f41cda..2c18a4d56b1 100644 --- a/ghost/core/test/unit/server/services/indexnow-ping/indexnow-ping-service.test.js +++ b/ghost/core/test/unit/server/services/indexnow-ping/indexnow-ping-service.test.js @@ -270,7 +270,7 @@ describe('IndexNow', function () { await service.ping(testPost); - sinon.assert.notCalled(deps.urlService.facade.getUrlForResource); + sinon.assert.notCalled(deps.urlService.getUrlForResource); }); it('with a post should execute ping', async function () { From 1691d77240eb6370922c933e842bb2c74c86d10d Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 10 Aug 2026 12:01:11 -0500 Subject: [PATCH 02/24] Fixed test:types failures in kg-converters and adapter-base-scheduling (#29860) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref Two packages fail their `test:types` target on a fresh local `pnpm test` run. CI only runs the `test:unit` target for workspace packages, so these type-check failures never surfaced there — but every local full test run hits them. --- .changeset/tidy-types-check.md | 5 +++++ .changeset/typed-logging-args.md | 5 +++++ koenig/kg-converters/tsconfig.test.json | 1 + packages/adapters/scheduling-base/test/base.test.ts | 5 +++-- 4 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 .changeset/tidy-types-check.md create mode 100644 .changeset/typed-logging-args.md diff --git a/.changeset/tidy-types-check.md b/.changeset/tidy-types-check.md new file mode 100644 index 00000000000..e319b5f8836 --- /dev/null +++ b/.changeset/tidy-types-check.md @@ -0,0 +1,5 @@ +--- +"@tryghost/kg-converters": patch +--- + +Fixed the test suite type-check failing under TypeScript 7. diff --git a/.changeset/typed-logging-args.md b/.changeset/typed-logging-args.md new file mode 100644 index 00000000000..8e3c5ec1423 --- /dev/null +++ b/.changeset/typed-logging-args.md @@ -0,0 +1,5 @@ +--- +"@tryghost/adapter-base-scheduling": patch +--- + +Fixed the test suite type-check against the typed @tryghost/logging error signature. diff --git a/koenig/kg-converters/tsconfig.test.json b/koenig/kg-converters/tsconfig.test.json index b092804ad88..51b4b789fee 100644 --- a/koenig/kg-converters/tsconfig.test.json +++ b/koenig/kg-converters/tsconfig.test.json @@ -2,6 +2,7 @@ "extends": "./tsconfig.json", "compilerOptions": { "rootDir": ".", + "types": ["node"], "outDir": null, "noEmit": true, "declaration": false, diff --git a/packages/adapters/scheduling-base/test/base.test.ts b/packages/adapters/scheduling-base/test/base.test.ts index a3ab63f8cce..f6bd072b8c7 100644 --- a/packages/adapters/scheduling-base/test/base.test.ts +++ b/packages/adapters/scheduling-base/test/base.test.ts @@ -90,13 +90,14 @@ describe('SchedulingBase', function () { await base.rescheduleAll({}); assert.equal(errorStub.callCount, 2, 'only the failing reschedulers are logged'); - const [meta, message] = errorStub.firstCall.args; + type FailureLogArgs = [{event: {name: string}; err: Error; rescheduler: string}, string]; + const [meta, message] = errorStub.firstCall.args as FailureLogArgs; assert.equal(meta.event.name, 'scheduler.reschedule_all.failed'); assert.equal(meta.rescheduler, 'PostScheduling'); assert.equal(meta.err.message, 'post failed'); assert.equal(message, 'Rescheduler failed'); - const [meta2, message2] = errorStub.secondCall.args; + const [meta2, message2] = errorStub.secondCall.args as FailureLogArgs; assert.equal(meta2.event.name, 'scheduler.reschedule_all.failed'); assert.equal(meta2.rescheduler, 'unknown'); assert.equal(meta2.err.message, 'anonymous failed'); From 6f6f19518a819f6119f2ba912d536beb5777fab5 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Mon, 10 Aug 2026 18:29:41 +0100 Subject: [PATCH 03/24] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20theme=20settings?= =?UTF-8?q?=20reset=20when=20editing=20Source=20or=20Casper=20(#29839)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://linear.app/ghost/issue/ONC-1936/ Editing a default theme (Source/Casper) in the built-in theme editor forces a "save as new theme" flow because default themes can't be overwritten. Custom theme settings are stored against the theme name, so the newly-named copy starts with the theme's default settings — the moment it was activated, all customised design settings (fonts, colours, header styles, etc.) silently reverted to defaults and unexpectedly broke the site's appearance. Fixed it so the editor asks the API to carry the settings over: - `POST /themes/upload` now accepts an optional `copy_settings_from` query option. After the theme is stored, the custom theme settings service duplicates the source theme's stored settings under the new theme name, so activating the copy keeps the site's design. - Copied values are reconciled by the existing activation sync (unknown keys pruned, invalid select values reset), which handles the case where the edit also changed the theme's settings definition. - Copying no-ops when the destination theme already has stored settings, so saving over an existing theme never clobbers its customisations. - The theme editor sends `copy_settings_from` whenever a save results in a new theme name — the forced save-as for default themes, and any rename of a custom theme. --- .../site/theme/theme-code-editor-modal.tsx | 6 +- .../settings/site/theme.acceptance.test.tsx | 5 +- .../core/core/server/api/endpoints/themes.js | 7 +- .../core/server/services/themes/storage.js | 21 ++++- .../custom-theme-settings-bread-service.js | 4 + .../custom-theme-settings-service.js | 39 ++++++++ ghost/core/test/e2e-api/admin/themes.test.js | 73 ++++++++++++++- .../service.test.js | 90 +++++++++++++++++++ 8 files changed, 239 insertions(+), 6 deletions(-) diff --git a/apps/admin/src/settings/app/components/settings/site/theme/theme-code-editor-modal.tsx b/apps/admin/src/settings/app/components/settings/site/theme/theme-code-editor-modal.tsx index 4fb74948047..3e21887985b 100644 --- a/apps/admin/src/settings/app/components/settings/site/theme/theme-code-editor-modal.tsx +++ b/apps/admin/src/settings/app/components/settings/site/theme/theme-code-editor-modal.tsx @@ -764,7 +764,11 @@ const ThemeCodeEditorModal: React.FC<{themeName: string}> = ({themeName}) => { const formData = new FormData(); formData.append('file', blob, `${nextThemeName}.zip`); - const response = await fetch(`${getGhostPaths().apiRoot}/themes/upload/`, { + // when saving under a new name, carry over the original theme's + // settings so activating the copy keeps the site's design + const uploadQuery = isSaveAs ? `?copy_settings_from=${encodeURIComponent(previousThemeName)}` : ''; + + const response = await fetch(`${getGhostPaths().apiRoot}/themes/upload/${uploadQuery}`, { method: 'POST', credentials: 'include', headers: { diff --git a/apps/admin/src/settings/site/theme.acceptance.test.tsx b/apps/admin/src/settings/site/theme.acceptance.test.tsx index b178f3c938d..2858508bd12 100644 --- a/apps/admin/src/settings/site/theme.acceptance.test.tsx +++ b/apps/admin/src/settings/site/theme.acceptance.test.tsx @@ -241,7 +241,10 @@ describe("Theme settings", () => { fakeThemeWorld(); await fakeThemeDownload("casper"); await fakeThemeDownload("casper-edited"); - const uploadApi = fakeAdminEndpoint("POST", "/themes/upload/", { themes: [theme({ name: "casper-edited" })] }); + // saving under a new name carries over the original theme's settings + const uploadApi = fakeAdminEndpoint("POST", "/themes/upload/?copy_settings_from=casper", { + themes: [theme({ name: "casper-edited" })], + }); await renderAdminApp("/settings/theme/edit/casper"); const editor = await editorTextbox(); diff --git a/ghost/core/core/server/api/endpoints/themes.js b/ghost/core/core/server/api/endpoints/themes.js index 7b2cd4c19a9..817ef206ebe 100644 --- a/ghost/core/core/server/api/endpoints/themes.js +++ b/ghost/core/core/server/api/endpoints/themes.js @@ -106,6 +106,9 @@ const controller = { headers: { cacheInvalidate: false }, + options: [ + 'copy_settings_from' + ], permissions: { method: 'add' }, @@ -123,7 +126,9 @@ const controller = { name: frame.file.originalname }; - const {theme, themeOverridden} = await themeService.api.setFromZip(zip); + const {theme, themeOverridden} = await themeService.api.setFromZip(zip, { + copySettingsFrom: frame.options.copy_settings_from + }); if (themeOverridden) { frame.setHeader('X-Cache-Invalidate', '/*'); } diff --git a/ghost/core/core/server/services/themes/storage.js b/ghost/core/core/server/services/themes/storage.js index ff854aa568b..345d31d7603 100644 --- a/ghost/core/core/server/services/themes/storage.js +++ b/ghost/core/core/server/services/themes/storage.js @@ -8,6 +8,7 @@ const errors = require('@tryghost/errors'); const validate = require('./validate'); const list = require('./list'); +const customThemeSettings = require('../custom-theme-settings'); const ThemeStorage = require('./theme-storage'); const themeLoader = require('./loader'); const activator = require('./activation-bridge'); @@ -24,7 +25,8 @@ const messages = { invalidThemeName: 'Please select a valid theme.', overrideDefaultTheme: 'Please rename your zip, it\'s not allowed to override the default theme.', destroyDefaultTheme: 'Deleting the default theme is not allowed.', - destroyActive: 'Deleting the active theme is not allowed.' + destroyActive: 'Deleting the active theme is not allowed.', + copySettingsFromDoesNotExist: 'Theme to copy settings from is not installed.' }; const INVALID_THEME_REGEX = /^[./]*$/; @@ -51,7 +53,7 @@ module.exports = { name: themeName }); }, - setFromZip: async (zip) => { + setFromZip: async (zip, {copySettingsFrom} = {}) => { const themeName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]); const backupName = `${themeName}_${ObjectID()}`; @@ -69,12 +71,27 @@ module.exports = { }); } + if (copySettingsFrom && !list.get(copySettingsFrom)) { + throw new errors.ValidationError({ + message: tpl(messages.copySettingsFromDoesNotExist) + }); + } + let checkedTheme; let overrideTheme; let renamedExisting = false; try { checkedTheme = await validate.checkSafe(themeName, zip, true); + + // CASE: theme uploaded as a copy of another theme, carry over that + // theme's settings so activating the copy keeps the site's design. + // Happens before any file changes so a failed copy leaves the + // installed themes untouched + if (copySettingsFrom) { + await customThemeSettings.api.copySettingsBetweenThemes(copySettingsFrom, themeName); + } + const themeExists = await getStorage().exists(themeName); // CASE: move the existing theme to a backup folder if (themeExists) { diff --git a/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-bread-service.js b/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-bread-service.js index dfeb634563a..6d90d10b312 100644 --- a/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-bread-service.js +++ b/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-bread-service.js @@ -26,4 +26,8 @@ module.exports = class CustomThemeSettingsBREADService { async destroy(data, options = {}) { return this.Model.destroy(data, options); } + + async transaction(fn) { + return this.Model.transaction(fn); + } }; diff --git a/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js b/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js index 3c909b02817..050c78e1e1f 100644 --- a/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js +++ b/ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js @@ -164,6 +164,45 @@ module.exports = class CustomThemeSettingsService { return settingsObjects; } + /** + * Duplicate stored settings from one theme to another, e.g. when a theme + * is saved as a copy under a new name. + * + * No-ops if the destination theme already has stored settings so existing + * customisations are never overwritten. Values are copied verbatim - they + * are reconciled against the destination theme's settings definition by + * the sync that runs when that theme is activated. + * + * @param {string} fromThemeName + * @param {string} toThemeName + */ + async copySettingsBetweenThemes(fromThemeName, toThemeName) { + const sourceCollection = await this._repository.browse({filter: `theme:'${fromThemeName}'`}); + + // single transaction so a failure part-way leaves no partial copy + // behind that would make later attempts skip the copy. The + // destination check locks inside the same transaction so concurrent + // copies can't both see an empty destination and insert duplicates + await this._repository.transaction(async (transacting) => { + const destinationCollection = await this._repository.browse({filter: `theme:'${toThemeName}'`, transacting, forUpdate: true}); + + if (destinationCollection.toJSON().length > 0) { + debug(`Skipping copy of custom theme settings from '${fromThemeName}' to '${toThemeName}' - destination already has settings`); + return; + } + + for (const setting of sourceCollection.toJSON()) { + debug(`Copying custom theme setting '${fromThemeName}.${setting.key}' to '${toThemeName}'`); + await this._repository.add({ + theme: toThemeName, + key: setting.key, + type: setting.type, + value: setting.value + }, {transacting}); + } + }); + } + // Private ----------------------------------------------------------------- /** diff --git a/ghost/core/test/e2e-api/admin/themes.test.js b/ghost/core/test/e2e-api/admin/themes.test.js index 290f6ea54e1..7942e69d7ac 100644 --- a/ghost/core/test/e2e-api/admin/themes.test.js +++ b/ghost/core/test/e2e-api/admin/themes.test.js @@ -3,6 +3,7 @@ const {assertExists} = require('../../utils/assertions'); const sinon = require('sinon'); const path = require('path'); const fs = require('fs'); +const os = require('os'); const _ = require('lodash'); const supertest = require('supertest'); const nock = require('nock'); @@ -19,9 +20,10 @@ describe('Themes API', function () { const themePath = options.themePath; const fieldName = 'file'; const request = options.request || ownerRequest; + const query = options.query || ''; return request - .post(localUtils.API.getApiQuery('themes/upload')) + .post(localUtils.API.getApiQuery(`themes/upload${query}`)) .set('Origin', config.get('url')) .attach(fieldName, themePath); }; @@ -425,6 +427,75 @@ describe('Themes API', function () { mockManager.restoreLimitService(); }); + it('Can copy custom theme settings when uploading a theme under a new name', async function () { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'theme-settings-copy-')); + + try { + // start from a known active theme and customise its settings + await ownerRequest + .put(localUtils.API.getApiQuery('themes/source/activate')) + .set('Origin', config.get('url')) + .expect(200); + + await ownerRequest + .put(localUtils.API.getApiQuery('custom_theme_settings/')) + .set('Origin', config.get('url')) + .send({custom_theme_settings: [ + {key: 'title_font', value: 'Elegant serif'}, + {key: 'site_background_color', value: '#123456'} + ]}) + .expect(200); + + // save a copy of the default theme under a new name, as the theme editor does + const zipPath = path.join(tmpDir, 'source-edited.zip'); + fs.copyFileSync(path.join(__dirname, '..', '..', 'utils', 'fixtures', 'themes', 'source.zip'), zipPath); + + const uploadRes = await uploadTheme({ + themePath: zipPath, + query: '?copy_settings_from=source' + }); + assert.equal(uploadRes.statusCode, 200); + assert.equal(uploadRes.body.themes[0].name, 'source-edited'); + + await ownerRequest + .put(localUtils.API.getApiQuery('themes/source-edited/activate')) + .set('Origin', config.get('url')) + .expect(200); + + // customised values survived the switch to the renamed copy + const settingsRes = await ownerRequest + .get(localUtils.API.getApiQuery('custom_theme_settings/')) + .set('Origin', config.get('url')) + .expect(200); + + const settingsByKey = Object.fromEntries(settingsRes.body.custom_theme_settings.map(setting => [setting.key, setting.value])); + assert.equal(settingsByKey.title_font, 'Elegant serif'); + assert.equal(settingsByKey.site_background_color, '#123456'); + } finally { + fs.rmSync(tmpDir, {recursive: true, force: true}); + + // best-effort restore of the pre-test theme state, no assertions so + // cleanup completes even when the test fails part-way through + await ownerRequest + .put(localUtils.API.getApiQuery('themes/source/activate')) + .set('Origin', config.get('url')); + + await ownerRequest + .del(localUtils.API.getApiQuery('themes/source-edited')) + .set('Origin', config.get('url')); + } + }); + + it('Errors when asked to copy settings from an unknown theme', async function () { + const res = await uploadTheme({ + themePath: path.join(__dirname, '..', '..', 'utils', 'fixtures', 'themes', 'valid.zip'), + query: '?copy_settings_from=unknown-theme' + }); + + assert.equal(res.statusCode, 422); + assert.equal(res.body.errors[0].type, 'ValidationError'); + }); + it('Can re-upload the active theme to override', async function () { // The tricky thing about this test is the default active theme is Source and you're not allowed to override it. // So we upload a valid theme, activate it, and then upload again. diff --git a/ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js b/ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js index 0b7fa868874..191fccc9204 100644 --- a/ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js +++ b/ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js @@ -64,6 +64,18 @@ class ModelStub { this.knownSettings = this.knownSettings.filter(setting => setting !== destroyedSetting); return destroyedSetting; } + + async transaction(fn) { + const snapshot = this.knownSettings.slice(); + this.lastTransacting = {}; + + try { + return await fn(this.lastTransacting); + } catch (error) { + this.knownSettings = snapshot; + throw error; + } + } } describe('Service', function () { @@ -485,6 +497,84 @@ describe('Service', function () { }); }); + describe('copySettingsBetweenThemes()', function () { + const settingsForTheme = async (themeName) => { + const collection = await model.findAll({filter: `theme:'${themeName}'`}); + return collection.toJSON().map(({theme, key, type, value}) => ({theme, key, type, value})); + }; + + it('copies settings rows to the new theme name', async function () { + await service.copySettingsBetweenThemes('test', 'test-edited'); + + // the destination check is locked inside the transaction so + // concurrent copies can't both see an empty destination + const destinationCheck = model.findAll.getCalls().find(call => call.firstArg.filter === `theme:'test-edited'`); + assert.equal(destinationCheck.firstArg.transacting, model.lastTransacting); + assert.equal(destinationCheck.firstArg.forUpdate, true); + + // every insert runs in the same transaction + model.add.getCalls().forEach((call) => { + assert.equal(call.args[1].transacting, model.lastTransacting); + }); + + assert.deepEqual(await settingsForTheme('test-edited'), [{ + theme: 'test-edited', + key: 'one', + type: 'select', + value: '1' + }, { + theme: 'test-edited', + key: 'two', + type: 'select', + value: '2' + }]); + + // source theme rows are untouched + assert.equal((await settingsForTheme('test')).length, 2); + }); + + it('does not overwrite existing settings for the destination theme', async function () { + await model.add({theme: 'test-edited', key: 'one', type: 'select', value: 'existing'}); + + await service.copySettingsBetweenThemes('test', 'test-edited'); + + assert.deepEqual(await settingsForTheme('test-edited'), [{ + theme: 'test-edited', + key: 'one', + type: 'select', + value: 'existing' + }]); + }); + + it('is a no-op when the source theme has no settings', async function () { + await service.copySettingsBetweenThemes('unknown', 'test-edited'); + + assert.equal((await settingsForTheme('test-edited')).length, 0); + sinon.assert.notCalled(model.add); + }); + + it('leaves no partial copy behind when the copy fails part-way', async function () { + const originalAdd = model.add; + let addCalls = 0; + model.add = async function (data, options) { + addCalls += 1; + if (addCalls === 2) { + throw new Error('second add failed'); + } + return originalAdd.call(model, data, options); + }; + // atomicity must not depend on individual deletes succeeding + model.destroy = async () => { + throw new Error('destroy failed'); + }; + + await assert.rejects(service.copySettingsBetweenThemes('test', 'test-edited'), /second add failed/); + + model.add = originalAdd; + assert.equal((await settingsForTheme('test-edited')).length, 0); + }); + }); + describe('updateSettings()', function () { it('saves new values', async function () { // activate theme so settings are loaded in internal cache From af0da3d776b940c6edcb23f2cb57f0532c2d6434 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Mon, 10 Aug 2026 18:30:38 +0100 Subject: [PATCH 04/24] Renamed `gift-reminders` controller to `gifts` (#29848) ref https://linear.app/ghost/issue/BER-3851/ - the specificity of the `gift-reminders` controller naming made it harder to expand gift subscriptions with additional endpoints, renaming to just `gifts` matches the service naming and provides a single file to house upcoming gift delivery work --- .../server/api/endpoints/{gift-reminders.js => gifts.js} | 0 ghost/core/core/server/api/endpoints/index.js | 4 ++-- ghost/core/core/server/web/api/endpoints/admin/routes.js | 4 ++-- .../api/endpoints/{gift-reminders.test.js => gifts.test.js} | 6 +++--- 4 files changed, 7 insertions(+), 7 deletions(-) rename ghost/core/core/server/api/endpoints/{gift-reminders.js => gifts.js} (100%) rename ghost/core/test/unit/api/endpoints/{gift-reminders.test.js => gifts.test.js} (76%) diff --git a/ghost/core/core/server/api/endpoints/gift-reminders.js b/ghost/core/core/server/api/endpoints/gifts.js similarity index 100% rename from ghost/core/core/server/api/endpoints/gift-reminders.js rename to ghost/core/core/server/api/endpoints/gifts.js diff --git a/ghost/core/core/server/api/endpoints/index.js b/ghost/core/core/server/api/endpoints/index.js index 9f649d1fe6a..48d63adff79 100644 --- a/ghost/core/core/server/api/endpoints/index.js +++ b/ghost/core/core/server/api/endpoints/index.js @@ -316,8 +316,8 @@ module.exports = { return apiFramework.pipeline(require('./gift-links'), localUtils); }, - get giftReminders() { - return apiFramework.pipeline(require('./gift-reminders'), localUtils); + get gifts() { + return apiFramework.pipeline(require('./gifts'), localUtils); }, get recommendationsPublic() { diff --git a/ghost/core/core/server/web/api/endpoints/admin/routes.js b/ghost/core/core/server/web/api/endpoints/admin/routes.js index b85df3f07f5..b57a10c687d 100644 --- a/ghost/core/core/server/web/api/endpoints/admin/routes.js +++ b/ghost/core/core/server/web/api/endpoints/admin/routes.js @@ -85,8 +85,8 @@ module.exports = function apiRoutes() { // ## Schedules router.put('/schedules/:resource/:id', mw.authAdminApiWithUrl, http(api.schedules.publish)); - // ## Gift Reminders - router.put('/gifts/flush_reminders', mw.authAdminApiWithUrl, http(api.giftReminders.flushReminders)); + // ## Gifts + router.put('/gifts/flush_reminders', mw.authAdminApiWithUrl, http(api.gifts.flushReminders)); // ## Settings router.get('/settings/routes/yaml', mw.authAdminApi, http(api.settings.download)); diff --git a/ghost/core/test/unit/api/endpoints/gift-reminders.test.js b/ghost/core/test/unit/api/endpoints/gifts.test.js similarity index 76% rename from ghost/core/test/unit/api/endpoints/gift-reminders.test.js rename to ghost/core/test/unit/api/endpoints/gifts.test.js index 90f3de61dbe..c7415eb555a 100644 --- a/ghost/core/test/unit/api/endpoints/gift-reminders.test.js +++ b/ghost/core/test/unit/api/endpoints/gifts.test.js @@ -1,10 +1,10 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); const domainEvents = require('@tryghost/domain-events'); -const giftRemindersController = require('../../../../core/server/api/endpoints/gift-reminders'); +const giftsController = require('../../../../core/server/api/endpoints/gifts'); const StartGiftReminderFlushEvent = require('../../../../core/server/services/gifts/events/start-gift-reminder-flush-event'); -describe('Gift Reminders controller', function () { +describe('Gifts controller', function () { afterEach(function () { sinon.restore(); }); @@ -13,7 +13,7 @@ describe('Gift Reminders controller', function () { it('dispatches a StartGiftReminderFlushEvent', function () { const dispatchStub = sinon.stub(domainEvents, 'dispatch'); - const result = giftRemindersController.flushReminders.query({}); + const result = giftsController.flushReminders.query({}); sinon.assert.calledOnceWithExactly( dispatchStub, From a121383a718016c95bb961b06f100ec61ca941e9 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 10 Aug 2026 12:54:15 -0500 Subject: [PATCH 05/24] Added "automation run analytics" private flag (#29857) closes https://linear.app/ghost/issue/NY-1517 ![Screenshot](https://github.com/user-attachments/assets/b2048ab4-dfc2-422d-84cf-2c39caa745ed) --- .../components/settings/advanced/labs/private-features.tsx | 4 ++++ ghost/core/core/shared/labs.js | 1 + .../core/test/e2e-api/admin/__snapshots__/config.test.js.snap | 1 + 3 files changed, 6 insertions(+) diff --git a/apps/admin/src/settings/app/components/settings/advanced/labs/private-features.tsx b/apps/admin/src/settings/app/components/settings/advanced/labs/private-features.tsx index db5abefecac..126fb555377 100644 --- a/apps/admin/src/settings/app/components/settings/advanced/labs/private-features.tsx +++ b/apps/admin/src/settings/app/components/settings/advanced/labs/private-features.tsx @@ -15,6 +15,10 @@ const features: Feature[] = [{ title: 'Automations', description: 'Toggle the automations beta. Unexpected problems can occur if you turn this off after previously turning it on.', flag: 'automations' +}, { + title: 'Automation run analytics', + description: 'Track run-level analytics for automations.', + flag: 'automationRunAnalytics' }, { title: 'Stripe Automatic Tax (private beta)', description: 'Use Stripe Automatic Tax at Stripe Checkout. Needs to be enabled in Stripe', diff --git a/ghost/core/core/shared/labs.js b/ghost/core/core/shared/labs.js index 1f90304d88e..f629ae07dcb 100644 --- a/ghost/core/core/shared/labs.js +++ b/ghost/core/core/shared/labs.js @@ -48,6 +48,7 @@ const PUBLIC_BETA_FEATURES = [ // Which is only visible if the developer experiments flag is enabled const PRIVATE_FEATURES = [ 'automations', + 'automationRunAnalytics', 'stripeAutomaticTax', 'importMemberTier', 'csvContentImporter', diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap index 88f1f07547d..f63b2d1dfae 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap @@ -16,6 +16,7 @@ Object { "additionalPaymentMethods": true, "adminUIRefresh": true, "automationAnalytics": true, + "automationRunAnalytics": true, "automations": true, "commentsPinning": true, "commentsThreads": true, From dc4d1c8220e7474ff8700deeaa4f3a46cd0b394c Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 10 Aug 2026 13:15:16 -0500 Subject: [PATCH 06/24] Quieted ghost-admin dev-server noise (#28997) no ref Three focused cleanups that remove expected noise from the legacy Ember Admin development server without hiding the same diagnostics from production builds or tests. --- apps/ember-admin/.ember-cli | 11 ++++++++++- apps/ember-admin/lib/asset-delivery/index.js | 6 +++++- apps/ember-admin/package.json | 4 ++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/apps/ember-admin/.ember-cli b/apps/ember-admin/.ember-cli index 53f3969d16d..83cbe4cd3eb 100644 --- a/apps/ember-admin/.ember-cli +++ b/apps/ember-admin/.ember-cli @@ -7,5 +7,14 @@ Setting `disableAnalytics` to true will prevent any data from being sent. */ - "disableAnalytics": true + "disableAnalytics": true, + + /** + Default to Node's native FS watcher instead of Watchman. Watchman + repeatedly trips its "MustScanSubDirs UserDropped" recrawl warning + in this monorepo (Docker bind mounts + Vite dev writes), spamming + ~6 lines per rebuild. The node watcher is quieter and good enough + for our tree size. + */ + "watcher": "node" } diff --git a/apps/ember-admin/lib/asset-delivery/index.js b/apps/ember-admin/lib/asset-delivery/index.js index 502cb1ad0e5..7a1ee3abaf9 100644 --- a/apps/ember-admin/lib/asset-delivery/index.js +++ b/apps/ember-admin/lib/asset-delivery/index.js @@ -101,7 +101,11 @@ module.exports = { } else { fs.ensureSymlinkSync(adminXPath, assetsAdminXPath); } - } else { + } else if (this.env === 'production') { + // In dev the admin-x apps may not have finished their first + // build yet and Nx will trigger another Ember rebuild once + // they do. Only flag a missing dist for production where it + // indicates a real pipeline failure. console.log(`${app} folder not found`); } } diff --git a/apps/ember-admin/package.json b/apps/ember-admin/package.json index c5977435038..f6981043685 100644 --- a/apps/ember-admin/package.json +++ b/apps/ember-admin/package.json @@ -16,7 +16,7 @@ "test": "tests" }, "scripts": { - "dev": "SKIP_DEPENDENCY_CHECKER=true ember serve", + "dev": "SKIP_DEPENDENCY_CHECKER=true NODE_OPTIONS=--disable-warning=DEP0179 ember serve", "build": "ember build --environment=production --silent", "build:dev": "SKIP_DEPENDENCY_CHECKER=true pnpm build --environment=development", "test": "ember exam --split 2 --parallel", @@ -188,7 +188,7 @@ "executor": "nx:run-commands", "options": { "cwd": "apps/ember-admin", - "command": "JOBS=4 SKIP_DEPENDENCY_CHECKER=true ember serve" + "command": "JOBS=4 SKIP_DEPENDENCY_CHECKER=true NODE_OPTIONS=--disable-warning=DEP0179 ember serve" }, "dependsOn": [ "^build" From 77f26859fe39c5aec19a8f78948726238fc6ac48 Mon Sep 17 00:00:00 2001 From: Cathy Sarisky <42299862+cathysarisky@users.noreply.github.com> Date: Mon, 10 Aug 2026 14:20:06 -0400 Subject: [PATCH 07/24] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20missing=20tiers=20?= =?UTF-8?q?in=20previous=20part=20of=20webhook=20(#29810)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #29805 Currently, switching between tiers does not cause a webhook to fire. This makes it hard for an integration to detect plan changes without doing a full scan of members. This PR makes sure that tiers are emitted, and provides a test for the corrected behavior. --- .../server/services/webhooks/serialize.js | 10 +++++- .../services/webhooks/serialize.test.js | 36 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/ghost/core/core/server/services/webhooks/serialize.js b/ghost/core/core/server/services/webhooks/serialize.js index 1fbc4bd20c0..a0729a9af74 100644 --- a/ghost/core/core/server/services/webhooks/serialize.js +++ b/ghost/core/core/server/services/webhooks/serialize.js @@ -5,6 +5,14 @@ // already carries (e.g. authors) would strip its nested roles from the // payload. `getRequiredRelations()` is [] when the routing config reads // none, so this is a no-op on a default routes.yaml. +// `model._changed` carries raw model keys, but `previous` is picked off the +// API-serialized payload, where some keys are renamed (members `products` → `tiers`). +const SERIALIZED_KEYS = { + members: { + products: 'tiers' + } +}; + const loadRequiredUrlRelations = async (model, urlService) => { const required = urlService.getRequiredRelations(); const missing = required.filter(relation => !model.relations[relation]); @@ -80,7 +88,7 @@ module.exports = ({urlService}) => async (event, model) => { .serializers .handle .output(model, {docName: docName, method: 'read'}, api.serializers.output, frame); - previous = _.pick(frame.response[docName][0], changed); + previous = _.pick(frame.response[docName][0], changed.map(key => SERIALIZED_KEYS[docName]?.[key] ?? key)); } diff --git a/ghost/core/test/unit/server/services/webhooks/serialize.test.js b/ghost/core/test/unit/server/services/webhooks/serialize.test.js index 8667b6de400..db552fe93e7 100644 --- a/ghost/core/test/unit/server/services/webhooks/serialize.test.js +++ b/ghost/core/test/unit/server/services/webhooks/serialize.test.js @@ -3,6 +3,7 @@ const sinon = require('sinon'); const {Post} = require('../../../../../core/server/models/post'); const {Member} = require('../../../../../core/server/models/member'); +const {Product} = require('../../../../../core/server/models/product'); const createSerialize = require('../../../../../core/server/services/webhooks/serialize'); @@ -160,4 +161,39 @@ describe('WebhookService - Serialize', function () { assert.deepEqual(result.member.previous.updated_at, previousUpdatedAt); assert.deepEqual(Object.keys(result.member.previous).sort(), ['status', 'updated_at']); }); + + it('includes the previous tiers when a member switches between paid tiers', async function () { + // bookshelf only populates `_previousAttributes` on fetch, not construction + const asFetched = (model) => { + model._previousAttributes = {...model.attributes}; + return model; + }; + const oldTier = asFetched(new Product({id: 'tier-old', name: 'Bronze', slug: 'bronze', type: 'paid', active: true})); + const newTier = asFetched(new Product({id: 'tier-new', name: 'Gold', slug: 'gold', type: 'paid', active: true})); + + const memberModel = new Member({ + id: 'member-id', + uuid: 'member-uuid', + email: 'member@example.com', + status: 'paid', + created_at: new Date('2026-01-01T00:00:00.000Z'), + updated_at: new Date('2026-01-01T00:00:00.000Z') + }); + + // mirrors bookshelf-relations' extendChanged + attachPreviousRelations output + // for a tier switch that touches no members column + memberModel._previousAttributes = {...memberModel.attributes}; + memberModel._changed = { + products: {attached: [newTier], detached: [oldTier]} + }; + memberModel._previousRelations = {products: {models: [oldTier]}}; + memberModel.related('products').models = [newTier]; + + sinon.stub(memberModel, 'load').resolves(memberModel); + + const result = await serialize('member.edited', memberModel); + + assert.deepEqual(result.member.current.tiers.map(tier => tier.slug), ['gold']); + assert.deepEqual(result.member.previous.tiers.map(tier => tier.slug), ['bronze']); + }); }); From 73157742038b001d754a5b028929871596e69d5c Mon Sep 17 00:00:00 2001 From: Minho Yoo Date: Tue, 11 Aug 2026 03:30:31 +0900 Subject: [PATCH 08/24] Removed unused events map from member count history (#29831) no ref `_generateCompleteRange` in `members-stats-service.js` builds a `Map` of events by date and then never reads it. --- .../core/server/services/stats/members-stats-service.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ghost/core/core/server/services/stats/members-stats-service.js b/ghost/core/core/server/services/stats/members-stats-service.js index 95e24c3128d..54901602c26 100644 --- a/ghost/core/core/server/services/stats/members-stats-service.js +++ b/ghost/core/core/server/services/stats/members-stats-service.js @@ -111,13 +111,6 @@ class MembersStatsService { const startDateMoment = moment.utc(startDate).startOf('day'); const endDateMoment = moment.utc(today).startOf('day'); - // Create a map of events by date for fast lookup - const eventsMap = new Map(); - rows.forEach((row) => { - const date = moment(row.date).format('YYYY-MM-DD'); - eventsMap.set(date, row); - }); - // Sort rows chronologically to calculate historical totals rows.sort((a, b) => moment(a.date).valueOf() - moment(b.date).valueOf()); From e18dddb098a67c528a34b9ee6e930b6823a6bc7a Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 10 Aug 2026 14:45:30 -0400 Subject: [PATCH 09/24] Fixed session verification to bind with user id (#29865) no ref - ensure session verification is bound to user id Co-authored-by: Steve Larson <9larsons@gmail.com> --- .../services/auth/session/session-service.js | 21 ++- .../admin/__snapshots__/session.test.js.snap | 18 +++ ghost/core/test/e2e-api/admin/session.test.js | 70 +++++++++ .../auth/session/session-service.test.js | 135 ++++++++++++++++++ 4 files changed, 241 insertions(+), 3 deletions(-) diff --git a/ghost/core/core/server/services/auth/session/session-service.js b/ghost/core/core/server/services/auth/session/session-service.js index 15a7bf64b32..35345aa999e 100644 --- a/ghost/core/core/server/services/auth/session/session-service.js +++ b/ghost/core/core/server/services/auth/session/session-service.js @@ -26,6 +26,7 @@ const AUTH_CODE_CHALLENGE_BYTES = 16; * @prop {string} user_agent * @prop {string} ip * @prop {boolean} verified + * @prop {string} [verified_user_id] * @prop {string} [auth_code_challenge] * @prop {number} [auth_code_generated_at] */ @@ -190,12 +191,14 @@ module.exports = function createSessionService({ if (isAuthCodeVerified) { session.verified = true; + session.verified_user_id = user.id; invalidateAuthCodeChallenge(session); } } if (isStaffDeviceVerificationDisabled()) { session.verified = true; + session.verified_user_id = user.id; } } @@ -214,6 +217,7 @@ module.exports = function createSessionService({ const { user_id: previousUserId, verified: previousVerified, + verified_user_id: previousVerifiedUserId, auth_code_challenge: previousAuthCodeChallenge, auth_code_generated_at: previousAuthCodeGeneratedAt } = previousSession; @@ -231,8 +235,11 @@ module.exports = function createSessionService({ const session = req.session; session.user_id = previousUserId; - // A different user doesn't inherit the previous user's verification - session.verified = previousUserId && previousUserId !== user.id ? undefined : previousVerified; + // Verification is bound to the user who completed it — any other user + // (including sessions with no verified_user_id) must verify again + const carryVerification = previousVerified === true && previousVerifiedUserId === user.id; + session.verified = carryVerification ? true : undefined; + session.verified_user_id = carryVerification ? previousVerifiedUserId : undefined; session.auth_code_challenge = previousAuthCodeChallenge; session.auth_code_generated_at = previousAuthCodeGeneratedAt; @@ -282,6 +289,7 @@ module.exports = function createSessionService({ }); session.verified = true; + session.verified_user_id = user.id; invalidateAuthCodeChallenge(session); } @@ -480,6 +488,7 @@ module.exports = function createSessionService({ async function verifySession(req, res) { const session = await getSession(req, res); session.verified = true; + session.verified_user_id = session.user_id; invalidateAuthCodeChallenge(session); } @@ -491,7 +500,12 @@ module.exports = function createSessionService({ */ async function isVerifiedSession(req, res) { const session = await getSession(req, res); - return session.verified; + // Verification is bound to a user; a session with no verified_user_id + // (e.g. logged out, or predating this field) fails closed rather than + // matching an absent user_id via undefined === undefined + return session.verified === true && + !!session.verified_user_id && + session.verified_user_id === session.user_id; } /** @@ -506,6 +520,7 @@ module.exports = function createSessionService({ if (isVerificationRequired()) { session.verified = undefined; + session.verified_user_id = undefined; } invalidateAuthCodeChallenge(session); diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/session.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/session.test.js.snap index 42e0239e621..c4402805f18 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/session.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/session.test.js.snap @@ -68,6 +68,24 @@ Object { } `; +exports[`Sessions API Staff 2FA requires 2FA again when a different user logs in after logout 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": "2FA_NEW_DEVICE_DETECTED", + "context": "A 6-digit sign-in verification code has been sent to your email to keep your account safe.", + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "User must verify session to login.", + "property": null, + "type": "Needs2FAError", + }, + ], +} +`; + exports[`Sessions API Staff 2FA sends verification email if staffDeviceVerification is enabled 1: [body] 1`] = ` Object { "errors": Array [ diff --git a/ghost/core/test/e2e-api/admin/session.test.js b/ghost/core/test/e2e-api/admin/session.test.js index 14afd29ae94..e1e00f156c0 100644 --- a/ghost/core/test/e2e-api/admin/session.test.js +++ b/ghost/core/test/e2e-api/admin/session.test.js @@ -223,5 +223,75 @@ describe('Sessions API', function () { .expectStatus(401) .expectEmptyBody(); }); + + it('requires 2FA again when a different user logs in after logout', async function () { + // Seed staff users beyond the owner so we have a second account + await fixtureManager.init('users'); + + const owner = await fixtureManager.get('users', 0); + const otherUser = await fixtureManager.get('users', 1); + + // Establish the second user as having logged in before, so their + // later login is subject to device verification rather than the + // first-login skip + await agent + .post('session/') + .body({ + grant_type: 'password', + username: otherUser.email, + password: otherUser.password + }) + .expectStatus(201); + await agent + .delete('session/') + .expectStatus(204); + + // Owner logs in and completes device verification on this session + await agent + .post('session/') + .body({ + grant_type: 'password', + username: owner.email, + password: owner.password + }) + .expectStatus(403); + + const ownerEmail = assert.sentEmail({ + subject: /[0-9]{6} is your Ghost sign in verification code/ + }); + const ownerToken = ownerEmail.subject.match(/[0-9]{6}/)[0]; + + await agent + .put('session/verify') + .body({ + token: ownerToken + }) + .expectStatus(200); + + // Owner logs out — in trusted-device mode logout keeps the + // session's verified flag but clears the user + await agent + .delete('session/') + .expectStatus(204); + + // The second user logging in on the same session must verify again + // rather than inheriting the owner's verification + await agent + .post('session/') + .body({ + grant_type: 'password', + username: otherUser.email, + password: otherUser.password + }) + .expectStatus(403) + .matchBodySnapshot({ + errors: [{ + code: '2FA_NEW_DEVICE_DETECTED', + id: anyUuid, + message: 'User must verify session to login.', + type: 'Needs2FAError' + }] + }); + }); }); }); diff --git a/ghost/core/test/unit/server/services/auth/session/session-service.test.js b/ghost/core/test/unit/server/services/auth/session/session-service.test.js index aadcc717174..e691e445368 100644 --- a/ghost/core/test/unit/server/services/auth/session/session-service.test.js +++ b/ghost/core/test/unit/server/services/auth/session/session-service.test.js @@ -299,6 +299,137 @@ describe('SessionService', function () { assert.equal(req.session.verified, undefined); }); + it('#createSessionForUser does not carry verification to a different user after logout', async function () { + const getSession = createGetSession(); + + const findUserById = sinon.spy(async ({id}) => ({id})); + const getOriginOfRequest = sinon.stub().returns('https://admin.example.com'); + + const isStaffDeviceVerificationDisabled = sinon.stub().returns(false); + + const sessionService = SessionService({ + getSession, + findUserById, + getOriginOfRequest, + getSettingsCache, + isStaffDeviceVerificationDisabled, + urlUtils + }); + + const req = Object.create(express.request, { + ip: { + value: '0.0.0.0' + }, + headers: { + value: { + cookie: 'thing' + } + }, + get: { + value: () => 'https://admin.example.com' + } + }); + const res = Object.create(express.response); + + await sessionService.createSessionForUser(req, res, {id: 'egg'}); + await sessionService.verifySession(req, res); + assert.equal(await sessionService.isVerifiedSession(req, res), true); + + // Trusted-device mode: logout keeps the verified flag + await sessionService.removeUserForSession(req, res); + assert.equal(req.session.user_id, undefined); + assert.equal(req.session.verified, true); + + // A different user signing in must verify again + await sessionService.createSessionForUser(req, res, {id: 'bacon'}); + assert.equal(req.session.user_id, 'bacon'); + assert.equal(req.session.verified, undefined); + assert.equal(await sessionService.isVerifiedSession(req, res), false); + }); + + it('#createSessionForUser keeps trusted-device verification for the same user after logout', async function () { + const getSession = createGetSession(); + + const findUserById = sinon.spy(async ({id}) => ({id})); + const getOriginOfRequest = sinon.stub().returns('https://admin.example.com'); + + const isStaffDeviceVerificationDisabled = sinon.stub().returns(false); + + const sessionService = SessionService({ + getSession, + findUserById, + getOriginOfRequest, + getSettingsCache, + isStaffDeviceVerificationDisabled, + urlUtils + }); + + const req = Object.create(express.request, { + ip: { + value: '0.0.0.0' + }, + headers: { + value: { + cookie: 'thing' + } + }, + get: { + value: () => 'https://admin.example.com' + } + }); + const res = Object.create(express.response); + + await sessionService.createSessionForUser(req, res, {id: 'egg'}); + await sessionService.verifySession(req, res); + + await sessionService.removeUserForSession(req, res); + + await sessionService.createSessionForUser(req, res, {id: 'egg'}); + assert.equal(req.session.user_id, 'egg'); + assert.equal(req.session.verified, true); + assert.equal(await sessionService.isVerifiedSession(req, res), true); + }); + + it('Treats legacy verified sessions without verified_user_id as unverified', async function () { + const getSession = createGetSession({user_id: 'egg', verified: true, origin: 'https://admin.example.com'}); + + const findUserById = sinon.spy(async ({id}) => ({id})); + const getOriginOfRequest = sinon.stub().returns('https://admin.example.com'); + + const isStaffDeviceVerificationDisabled = sinon.stub().returns(false); + + const sessionService = SessionService({ + getSession, + findUserById, + getOriginOfRequest, + getSettingsCache, + isStaffDeviceVerificationDisabled, + urlUtils + }); + + const req = Object.create(express.request, { + ip: { + value: '0.0.0.0' + }, + headers: { + value: { + cookie: 'thing' + } + }, + get: { + value: () => 'https://admin.example.com' + } + }); + const res = Object.create(express.response); + + assert.equal(await sessionService.isVerifiedSession(req, res), false); + + // Verification is not carried into a new login either + await sessionService.createSessionForUser(req, res, {id: 'egg'}); + assert.equal(req.session.verified, undefined); + assert.equal(await sessionService.isVerifiedSession(req, res), false); + }); + it('#createSessionForUser verifies session when valid token is provided on request', async function () { const getSession = createGetSession(); @@ -758,6 +889,10 @@ describe('SessionService', function () { assert.equal(req.session.user_id, 'egg'); assert.equal(req.session.verified, true); + // Verification must be bound to the SSO user, or isVerifiedSession + // would reject it and lock the user into a re-verify loop + assert.equal(req.session.verified_user_id, 'egg'); + assert.equal(await sessionService.isVerifiedSession(req, res), true); }); it('Throws if the user id is invalid', async function () { From ff3354398403019d4b2393e446cf025428de1bbb Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 10 Aug 2026 14:49:05 -0400 Subject: [PATCH 10/24] Applied external `got` client to Slack hooks (#29864) Co-authored-by: UserExistsError <23325451+UserExistsError@users.noreply.github.com> --- .../slack-notifications/slack-notifications.js | 4 ++-- ghost/core/core/server/services/slack-ping/index.ts | 2 +- .../server/services/slack-ping/slack-ping-service.ts | 1 + .../slack-notifications/slack-notifications.test.js | 10 +++++----- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ghost/core/core/server/services/slack-notifications/slack-notifications.js b/ghost/core/core/server/services/slack-notifications/slack-notifications.js index 9fc539f36b6..bde0fad213c 100644 --- a/ghost/core/core/server/services/slack-notifications/slack-notifications.js +++ b/ghost/core/core/server/services/slack-notifications/slack-notifications.js @@ -1,6 +1,6 @@ -const got = require('got').default; const validator = require('@tryghost/validator'); const errors = require('@tryghost/errors'); +const externalRequest = require('../../lib/request-external'); const ghostVersion = require('@tryghost/version'); const moment = require('moment'); @@ -178,7 +178,7 @@ class SlackNotifications { }; } - return await got.post(url, requestOptions); + return await externalRequest.post(url, requestOptions); } /** diff --git a/ghost/core/core/server/services/slack-ping/index.ts b/ghost/core/core/server/services/slack-ping/index.ts index 40780777cb3..1ec1176b6f9 100644 --- a/ghost/core/core/server/services/slack-ping/index.ts +++ b/ghost/core/core/server/services/slack-ping/index.ts @@ -13,7 +13,7 @@ class SlackPingServiceWrapper { const {blogIcon} = require('../../lib/image'); const events = require('../../lib/common/events'); const logging = require('@tryghost/logging'); - const request = require('@tryghost/request'); + const request = require('../../lib/request-external'); const settingsCache = require('../../../shared/settings-cache'); const urlService = require('../url'); const urlUtils = require('../../../shared/url-utils').default; diff --git a/ghost/core/core/server/services/slack-ping/slack-ping-service.ts b/ghost/core/core/server/services/slack-ping/slack-ping-service.ts index 4fd0d4aa6d7..58ab89bef92 100644 --- a/ghost/core/core/server/services/slack-ping/slack-ping-service.ts +++ b/ghost/core/core/server/services/slack-ping/slack-ping-service.ts @@ -232,6 +232,7 @@ export class SlackPingService { } return this.request(slackSettings.url, { + method: 'POST', body: JSON.stringify(slackData), headers: { 'Content-type': 'application/json' diff --git a/ghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js b/ghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js index cb4a8190f09..b2e24183992 100644 --- a/ghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js +++ b/ghost/core/test/unit/server/services/slack-notifications/slack-notifications.test.js @@ -3,7 +3,7 @@ const sinon = require('sinon'); const SlackNotifications = require('../../../../../core/server/services/slack-notifications/slack-notifications'); const nock = require('nock'); const ObjectId = require('bson-objectid').default; -const got = require('got').default; +const externalRequest = require('../../../../../core/server/lib/request-external'); const ghostVersion = require('@tryghost/version'); describe('SlackNotifications', function () { @@ -282,7 +282,7 @@ describe('SlackNotifications', function () { describe('send', function () { it('Sends with correct requestOptions', async function () { - const gotStub = sinon.stub(got, 'post').resolves(); + const externalRequestStub = sinon.stub(externalRequest, 'post').resolves(); sinon.stub(ghostVersion, 'original').value('5.0.0'); const expectedRequestOptions = [ @@ -298,9 +298,9 @@ describe('SlackNotifications', function () { await slackNotifications.send({data: 'test'}, 'https://slack-webhook.com'); assert(loggingErrorStub.callCount === 0); - sinon.assert.calledOnce(gotStub); - const gotStubArgs = gotStub.getCall(0).args; - assert.deepEqual(gotStubArgs, expectedRequestOptions); + sinon.assert.calledOnce(externalRequestStub); + const externalRequestStubArgs = externalRequestStub.getCall(0).args; + assert.deepEqual(externalRequestStubArgs, expectedRequestOptions); }); it('Throws when invalid URL is passed', async function () { From fa232980caa888c2f03239403fc5c2fd26a60719 Mon Sep 17 00:00:00 2001 From: Waqas Ahmed Date: Mon, 10 Aug 2026 23:53:08 +0500 Subject: [PATCH 11/24] Added regression test for content-adapter dependency resolution (#29830) closes https://github.com/TryGhost/Ghost/issues/22883 Adds a regression test proving that custom adapters placed in the documented `content/adapters//` location (per https://ghost.org/docs/config/#location) are correctly detected, and that a genuine missing-dependency error inside the adapter's own code is surfaced as such rather than misreported as "adapter not found." --- .../services/adapter-manager/adapter-paths.ts | 29 +++--- .../adapter-manager/adapter-paths.test.ts | 89 +++++++++++++++++++ 2 files changed, 106 insertions(+), 12 deletions(-) create mode 100644 ghost/core/test/unit/server/services/adapter-manager/adapter-paths.test.ts diff --git a/ghost/core/core/server/services/adapter-manager/adapter-paths.ts b/ghost/core/core/server/services/adapter-manager/adapter-paths.ts index 66fd14ea4de..2da2f9c12c5 100644 --- a/ghost/core/core/server/services/adapter-manager/adapter-paths.ts +++ b/ghost/core/core/server/services/adapter-manager/adapter-paths.ts @@ -1,21 +1,26 @@ import config from '../../../shared/config'; +import type {ConfigInstance} from '../../../shared/config/loader'; /** * Where adapters are looked up, in order. Also read by bin/validate-adapters.ts, * which checks adapter implementations at build time - keep this the only place * the lookup order is declared, so a name resolves there exactly as it does here. */ -export const adapterPaths: string[] = Array.from(new Set([ - '', // A blank path will cause us to check node_modules for the adapter - config.get('paths').internalAdaptersPath, +export function buildAdapterPaths(configInstance: ConfigInstance): string[] { + return Array.from(new Set([ + '', // A blank path will cause us to check node_modules for the adapter + configInstance.get('paths').internalAdaptersPath, - // custom docker builds may install adapters in a separate path from content, - // since the content dir is often bind-mounted into the container. Offering - // an escape hatch here to allow for this - config.get('paths').installedAdaptersPath ?? '', + // custom docker builds may install adapters in a separate path from content, + // since the content dir is often bind-mounted into the container. Offering + // an escape hatch here to allow for this + configInstance.get('paths').installedAdaptersPath ?? '', - // load adapters from content last, so that they don't override any other - // internal or platform-installed adapters - // TODO: potentially deprecate/remove as part of Ghost 7.0 - config.getContentPath('adapters') -])); + // load adapters from content last, so that they don't override any other + // internal or platform-installed adapters + // TODO: potentially deprecate/remove as part of Ghost 7.0 + configInstance.getContentPath('adapters') + ])); +} + +export const adapterPaths: string[] = buildAdapterPaths(config); diff --git a/ghost/core/test/unit/server/services/adapter-manager/adapter-paths.test.ts b/ghost/core/test/unit/server/services/adapter-manager/adapter-paths.test.ts new file mode 100644 index 00000000000..c8f169fe4fc --- /dev/null +++ b/ghost/core/test/unit/server/services/adapter-manager/adapter-paths.test.ts @@ -0,0 +1,89 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import {Provider} from 'nconf'; +import {AdapterManager} from '../../../../../core/server/services/adapter-manager/adapter-manager'; +import {buildAdapterPaths} from '../../../../../core/server/services/adapter-manager/adapter-paths'; +import {bindAll as bindUrlHelpers} from '@tryghost/config-url-helpers'; +import {bindAll as bindHelpers} from '../../../../../core/shared/config/helpers'; +import type {ConfigInstance} from '../../../../../core/shared/config/loader'; +import type {Adapter} from '../../../../../core/server/services/adapter-manager/types'; + +class BaseStorageAdapter implements Adapter { + readonly requiredFns: string[]; + + constructor() { + this.requiredFns = ['someMethod']; + } +} + +// A minimal nconf-backed config instance, seeded with a real `paths:contentPath`, +// mirroring the shape production config provides so `getContentPath('adapters')` +// resolves exactly as it would at runtime. +function makeConfig(contentPath: string, adapters: object = {}): ConfigInstance { + const nconf = new Provider(); + nconf.use('memory'); + nconf.set('paths:contentPath', contentPath); + // No internal/installed adapters path is relevant to this test, so they're + // left unset, matching how `installedAdaptersPath` is optional in production. + nconf.set('paths:internalAdaptersPath', path.join(os.tmpdir(), 'ghost-adapter-test-nonexistent-internal')); + nconf.set('adapters', adapters); + + bindUrlHelpers(nconf); + bindHelpers(nconf); + + return nconf; +} + +describe('adapter-paths', function () { + it('finds an adapter placed in content/adapters// per the documented convention, and reports its own missing dependency rather than "unable to find adapter"', function () { + // Regression test for https://github.com/TryGhost/Ghost/issues/22883: + // a storage adapter placed under content/adapters/storage//, per + // https://ghost.org/docs/config/#location, that itself requires a + // missing npm dependency (e.g. aws-sdk) was misreported as "unable to + // find storage adapter" instead of surfacing the real missing-dependency + // error, because `loadAdapterClass` matched the adapter's own path + // against Node's full MODULE_NOT_FOUND message - which always includes + // a trailing "Require stack" naming that same path - and treated any + // match as "not found here", silently moving on to the next search path. + const contentDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ghost-content-')); + + try { + const adapterName = 'CustomStorageAdapter'; + // Realistic layout: content/adapters/storage//index.js + const adapterDir = path.join(contentDir, 'adapters', 'storage', adapterName); + fs.mkdirSync(adapterDir, {recursive: true}); + fs.writeFileSync( + path.join(adapterDir, 'index.js'), + `require('this-npm-package-does-not-exist-at-all');\nmodule.exports = class {};` + ); + + const config = makeConfig(contentDir, {storage: {active: adapterName}}); + const pathsToAdapters = buildAdapterPaths(config); + + // Sanity check: the content adapters path really is last in the list, + // matching the documented "content wins last" resolution order. + assert.equal(pathsToAdapters[pathsToAdapters.length - 1], config.getContentPath('adapters')); + + const adapterManager = new AdapterManager({ + loadAdapterFromPath: require, + pathsToAdapters, + config, + baseClasses: {storage: BaseStorageAdapter} + }); + + assert.throws(() => { + adapterManager.getAdapter('storage'); + }, { + errorType: 'IncorrectUsageError', + // Must be the specific missing-dependency error naming the + // adapter's own unresolved package, NOT the generic + // "Unable to find storage adapter" fallback the issue reported. + message: /missing a dependency 'this-npm-package-does-not-exist-at-all' in your adapter/ + }); + } finally { + fs.rmSync(contentDir, {recursive: true, force: true}); + } + }); +}); From a5b43cba396a87ed6d9db94ba6692eb56e9333d6 Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 10 Aug 2026 14:53:33 -0400 Subject: [PATCH 12/24] Fixed feedback api to omit member relation for less privileged staff (#29867) fixes https://linear.app/ghost/issue/SC-41/ - this limits which staff users can view member data in feedback. Co-authored-by: UserExistsError <23325451+UserExistsError@users.noreply.github.com> --- .../audience-feedback-controller.js | 11 ++- .../audience-feedback/feedback-repository.js | 5 +- .../audience-feedback-controller.test.js | 67 +++++++++++++++++++ .../feedback-repository.test.js | 44 ++++++++++++ 4 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 ghost/core/test/unit/server/services/audience-feedback/feedback-repository.test.js diff --git a/ghost/core/core/server/services/audience-feedback/audience-feedback-controller.js b/ghost/core/core/server/services/audience-feedback/audience-feedback-controller.js index aad613d44d4..ace2d6b064e 100644 --- a/ghost/core/core/server/services/audience-feedback/audience-feedback-controller.js +++ b/ghost/core/core/server/services/audience-feedback/audience-feedback-controller.js @@ -1,5 +1,6 @@ const Feedback = require('./feedback'); const errors = require('@tryghost/errors'); +const permissions = require('../../services/permissions'); const tpl = require('@tryghost/tpl'); const messages = { @@ -118,7 +119,8 @@ class AudienceFeedbackController { const postId = frame.data.id; const options = { limit: frame.options.limit || 10, - page: frame.options.page || 1 + page: frame.options.page || 1, + withMember: false }; // Add score filter if specified @@ -126,6 +128,13 @@ class AudienceFeedbackController { options.score = parseInt(frame.options.score); } + try { + await permissions.canThis(frame.options.context).browse.member(); + options.withMember = true; + } catch { + // permissions throws; we want to return data but without member info + } + const result = await this.#repository.getForPost(postId, options); return result; } diff --git a/ghost/core/core/server/services/audience-feedback/feedback-repository.js b/ghost/core/core/server/services/audience-feedback/feedback-repository.js index 56a7a105699..a4f4c069ddb 100644 --- a/ghost/core/core/server/services/audience-feedback/feedback-repository.js +++ b/ghost/core/core/server/services/audience-feedback/feedback-repository.js @@ -76,10 +76,13 @@ module.exports = class FeedbackRepository { limit: options.limit || 10, page: options.page || 1, order: 'created_at DESC', - withRelated: ['member'], filter: filter }; + if (options.withMember) { + findOptions.withRelated = ['member']; + } + // Use findPage with filter const results = await this.#MemberFeedback.findPage(findOptions); diff --git a/ghost/core/test/unit/server/services/audience-feedback/audience-feedback-controller.test.js b/ghost/core/test/unit/server/services/audience-feedback/audience-feedback-controller.test.js index db054579bdc..e9895d438e6 100644 --- a/ghost/core/test/unit/server/services/audience-feedback/audience-feedback-controller.test.js +++ b/ghost/core/test/unit/server/services/audience-feedback/audience-feedback-controller.test.js @@ -1,7 +1,12 @@ const sinon = require('sinon'); +const permissions = require('../../../../../core/server/services/permissions'); const AudienceFeedbackController = require('../../../../../core/server/services/audience-feedback/audience-feedback-controller'); describe('AudienceFeedbackController', function () { + afterEach(function () { + sinon.restore(); + }); + describe('redirectToPost', function () { const postId = '634fc3901e0a291855d8b135'; const uuid = '7b11de3c-dff9-4563-82ae-a281122d201d'; @@ -83,4 +88,66 @@ describe('AudienceFeedbackController', function () { sinon.assert.calledOnceWithExactly(next, error); }); }); + + describe('browse', function () { + const postId = '634fc3901e0a291855d8b135'; + const context = {user: 'user-id'}; + + function createController(getForPost) { + return new AudienceFeedbackController({ + repository: {getForPost}, + audienceFeedbackService: {} + }); + } + + it('includes member data when the user can browse members', async function () { + const browseMember = sinon.stub().resolves(); + sinon.stub(permissions, 'canThis').withArgs(context).returns({ + browse: { + member: browseMember + } + }); + const result = {data: [], meta: {}}; + const getForPost = sinon.stub().resolves(result); + const controller = createController(getForPost); + + const response = await controller.browse({ + data: {id: postId}, + options: {context} + }); + + sinon.assert.calledOnce(browseMember); + sinon.assert.calledOnceWithExactly(getForPost, postId, { + limit: 10, + page: 1, + withMember: true + }); + sinon.assert.match(response, result); + }); + + it('excludes member data when the user cannot browse members', async function () { + const browseMember = sinon.stub().rejects(); + sinon.stub(permissions, 'canThis').withArgs(context).returns({ + browse: { + member: browseMember + } + }); + const result = {data: [], meta: {}}; + const getForPost = sinon.stub().resolves(result); + const controller = createController(getForPost); + + const response = await controller.browse({ + data: {id: postId}, + options: {context} + }); + + sinon.assert.calledOnce(browseMember); + sinon.assert.calledOnceWithExactly(getForPost, postId, { + limit: 10, + page: 1, + withMember: false + }); + sinon.assert.match(response, result); + }); + }); }); diff --git a/ghost/core/test/unit/server/services/audience-feedback/feedback-repository.test.js b/ghost/core/test/unit/server/services/audience-feedback/feedback-repository.test.js new file mode 100644 index 00000000000..1b8650c4a9a --- /dev/null +++ b/ghost/core/test/unit/server/services/audience-feedback/feedback-repository.test.js @@ -0,0 +1,44 @@ +const sinon = require('sinon'); +const FeedbackRepository = require('../../../../../core/server/services/audience-feedback/feedback-repository'); + +describe('FeedbackRepository', function () { + const postId = '634fc3901e0a291855d8b135'; + + function createRepository(findPage) { + return new FeedbackRepository({ + Member: {}, + Post: {}, + MemberFeedback: {findPage}, + Feedback: class Feedback {} + }); + } + + it('loads the related member when requested', async function () { + const findPage = sinon.stub().resolves({data: [], meta: {}}); + const repository = createRepository(findPage); + + await repository.getForPost(postId, {withMember: true}); + + sinon.assert.calledOnceWithExactly(findPage, { + limit: 10, + page: 1, + order: 'created_at DESC', + filter: `post_id:'${postId}'`, + withRelated: ['member'] + }); + }); + + it('does not load the related member by default', async function () { + const findPage = sinon.stub().resolves({data: [], meta: {}}); + const repository = createRepository(findPage); + + await repository.getForPost(postId); + + sinon.assert.calledOnceWithExactly(findPage, { + limit: 10, + page: 1, + order: 'created_at DESC', + filter: `post_id:'${postId}'` + }); + }); +}); From 75f1bcac33e6f8b019fd7c12b2b9064b2cbee3db Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 10 Aug 2026 15:02:55 -0400 Subject: [PATCH 13/24] Added feedback confirm prompt if not a newsletter feedback link (#29866) ref https://linear.app/ghost/issue/SC-20/ - prevents a logged-in member from unwittingly submitting feedback. Co-authored-by: UserExistsError <23325451+UserExistsError@users.noreply.github.com> --- apps/portal/src/app.jsx | 4 ++-- .../src/components/pages/feedback-page.jsx | 5 +++-- apps/portal/test/feedback-flow.test.jsx | 18 +++++++++++++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/apps/portal/src/app.jsx b/apps/portal/src/app.jsx index b4cd0744779..cb21601293b 100644 --- a/apps/portal/src/app.jsx +++ b/apps/portal/src/app.jsx @@ -672,8 +672,8 @@ export default class App extends React.Component { showPopup: true, page: 'feedback', pageData: { - uuid: member ? null : hashQuery.get('uuid'), - key: member ? null : hashQuery.get('key'), + uuid: hashQuery.get('uuid'), + key: hashQuery.get('key'), postId, score } diff --git a/apps/portal/src/components/pages/feedback-page.jsx b/apps/portal/src/components/pages/feedback-page.jsx index 1586cc8f5f1..60d70d611c3 100644 --- a/apps/portal/src/components/pages/feedback-page.jsx +++ b/apps/portal/src/components/pages/feedback-page.jsx @@ -311,9 +311,10 @@ export default function FeedbackPage() { const [score, setScore] = useState(initialScore); const positive = score === 1; const isLoggedIn = !!member; + const fromEmailLink = !!(uuid && key); - const [confirmed, setConfirmed] = useState(isLoggedIn); - const [loading, setLoading] = useState(isLoggedIn); + const [confirmed, setConfirmed] = useState(fromEmailLink && isLoggedIn); + const [loading, setLoading] = useState(fromEmailLink && isLoggedIn); const [error, setError] = useState(null); const doSendFeedback = async (selectedScore) => { diff --git a/apps/portal/test/feedback-flow.test.jsx b/apps/portal/test/feedback-flow.test.jsx index 685983d04c1..e681d6f0900 100644 --- a/apps/portal/test/feedback-flow.test.jsx +++ b/apps/portal/test/feedback-flow.test.jsx @@ -64,7 +64,7 @@ describe('Feedback Submission Flow', () => { within(popupIframeDocument).getByText('Your input helps shape what gets published.'); }); - test('Autosubmits feedback w/o uuid or key params', async () => { + test('Requires confirmation w/o uuid or key params', async () => { Object.defineProperty(window, 'location', { value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/`), writable: true @@ -72,9 +72,21 @@ describe('Feedback Submission Flow', () => { const {ghostApi, popupFrame, popupIframeDocument} = await setup(); expect(popupFrame).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText('Give feedback on this post')).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText('More like this')).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText('Less like this')).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(0); + + const submitBtn = within(popupIframeDocument).getByText('Submit feedback'); + fireEvent.click(submitBtn); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(1); - within(popupIframeDocument).getByText('Thanks for the feedback!'); - within(popupIframeDocument).getByText('Your input helps shape what gets published.'); + + // the re-render loop is slow to get to the final state + await waitFor(() => { + within(popupIframeDocument).getByText('Thanks for the feedback!'); + within(popupIframeDocument).getByText('Your input helps shape what gets published.'); + }); }); }); From 8f6a78dd435362fcc4896535c117ced49f04d4e6 Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 10 Aug 2026 15:18:59 -0400 Subject: [PATCH 14/24] Updated benchmarks to run on Blacksmith (#29868) no ref - github actions native runners can have wildly different performance characteristics depending on CPU/load, making benchmarks difficult to evaluate over time - switching benchmarks to run on Blacksmith should in theory provide a more stable set of measurements --- .github/workflows/ci.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c32d7fca9e..808b2d63110 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -484,8 +484,7 @@ jobs: # Changing the runner label or the hyperfine version makes new results # incomparable with the existing history - treat both as pinned. job_perf-tests: - runs-on: - labels: ubuntu-latest-4-cores + runs-on: blacksmith-2vcpu-ubuntu-2404 needs: [job_setup] if: (needs.job_setup.outputs.changed_core == 'true' && needs.job_setup.outputs.is_development == 'true') || needs.job_setup.outputs.has_perf_tests_label == 'true' name: Performance tests @@ -533,8 +532,7 @@ jobs: # bump would land in the code series as a regression with no Ghost commit behind it. job_perf-tests-image: name: Performance tests (production image) - runs-on: - labels: ubuntu-latest-4-cores + runs-on: blacksmith-2vcpu-ubuntu-2404 needs: [job_setup, job_docker] # Registry path only: the core image is pushed to GHCR, never saved as an artifact. if: | From cbdf028bb94c934157926babbf3bd49ba47edd42 Mon Sep 17 00:00:00 2001 From: louisghost Date: Mon, 10 Aug 2026 21:24:32 +0200 Subject: [PATCH 15/24] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20admin=20toolbar=20?= =?UTF-8?q?redirect=20loops=20on=20subdirectory=20sites=20(#29838)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Activating the admin toolbar on a subdirectory site (e.g. `https://ghost.org/changelog/?admin=1`) can redirect-loop when a reverse proxy strips the subdirectory before forwarding the request. Ghost then cleaned the query and redirected to `/`, which bounced back to the public URL with `admin=1` still attached. ## Summary - Build the clean redirect with `urlUtils.createUrl()` so the configured subdirectory is restored when missing (and left alone when already present) - Add unit coverage for stripped paths, already-prefixed paths, retained query params, and the activation redirect ## Test plan - [x] `pnpm test:single test/unit/frontend/web/middleware/admin-toolbar.test.js` - [ ] Manually open `/?admin=1` on a subdirectory install and confirm it redirects once to the public subdirectory path without looping --- .../frontend/web/middleware/admin-toolbar.js | 2 +- .../web/middleware/admin-toolbar.test.js | 49 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/ghost/core/core/frontend/web/middleware/admin-toolbar.js b/ghost/core/core/frontend/web/middleware/admin-toolbar.js index 4d35a2a59d2..c391851f2d8 100644 --- a/ghost/core/core/frontend/web/middleware/admin-toolbar.js +++ b/ghost/core/core/frontend/web/middleware/admin-toolbar.js @@ -135,7 +135,7 @@ function getCleanRedirectUrl(req) { currentUrl.searchParams.delete(QUERY_PARAM); currentUrl.searchParams.delete(HIDE_QUERY_PARAM); - return `${currentUrl.pathname}${currentUrl.search}${currentUrl.hash}`; + return `${urlUtils.createUrl(currentUrl.pathname)}${currentUrl.search}${currentUrl.hash}`; } function getQueryValue(value) { diff --git a/ghost/core/test/unit/frontend/web/middleware/admin-toolbar.test.js b/ghost/core/test/unit/frontend/web/middleware/admin-toolbar.test.js index e3a9d2907e1..5ebe21c7330 100644 --- a/ghost/core/test/unit/frontend/web/middleware/admin-toolbar.test.js +++ b/ghost/core/test/unit/frontend/web/middleware/admin-toolbar.test.js @@ -2,6 +2,7 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); const settingsCache = require('../../../../../core/shared/settings-cache'); +const urlUtils = require('../../../../../core/shared/url-utils').default; const adminToolbar = require('../../../../../core/frontend/web/middleware/admin-toolbar'); describe('admin toolbar middleware', function () { @@ -148,6 +149,54 @@ describe('admin toolbar middleware', function () { assert.equal(adminToolbar._private.getCleanRedirectUrl(req), '/welcome/?ref=test'); }); + describe('subdirectory sites', function () { + beforeEach(function () { + sandbox.stub(urlUtils, 'getSiteUrl').returns('https://example.com/changelog/'); + sandbox.stub(urlUtils, 'getSubdir').returns('/changelog'); + }); + + it('restores the subdirectory when a proxy strips it from the request path', function () { + assert.equal(adminToolbar._private.getCleanRedirectUrl({ + originalUrl: '/?admin=1', + url: '/?admin=1' + }), '/changelog/'); + + assert.equal(adminToolbar._private.getCleanRedirectUrl({ + originalUrl: '/welcome/?admin=1&ref=test', + url: '/welcome/?admin=1&ref=test' + }), '/changelog/welcome/?ref=test'); + }); + + it('keeps the redirect unchanged when the request path already has the subdirectory', function () { + assert.equal(adminToolbar._private.getCleanRedirectUrl({ + originalUrl: '/changelog/?admin=1', + url: '/changelog/?admin=1' + }), '/changelog/'); + + assert.equal(adminToolbar._private.getCleanRedirectUrl({ + originalUrl: '/changelog/welcome/?admin=0&admin_toolbar=0', + url: '/changelog/welcome/?admin=0&admin_toolbar=0' + }), '/changelog/welcome/'); + }); + + it('redirects to the subdirectory when the toolbar is activated', function () { + const req = { + headers: {}, + originalUrl: '/?admin=1', + query: { + admin: '1' + }, + url: '/?admin=1' + }; + const res = createResponse(); + + adminToolbar(req, res, sinon.spy()); + + assert.equal(res.redirectStatus, 302); + assert.equal(res.redirectUrl, '/changelog/'); + }); + }); + it('marks the request when the frontend marker cookie is valid', function () { const token = adminToolbar._private.createToken(); const req = { From c459636a33adb26d6243b928e09b13a577f6aed0 Mon Sep 17 00:00:00 2001 From: Cathy Sarisky <42299862+cathysarisky@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:26:05 -0400 Subject: [PATCH 16/24] =?UTF-8?q?=F0=9F=90=9B=20Made=20the=20member.edited?= =?UTF-8?q?=20webhook=20fire=20on=20cancellation=20and=20reinstatement=20o?= =?UTF-8?q?f=20paid=20plans=20(#29814)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #29806 Currently, if a user clicks 'cancel subscription' or while in cancel_at_period_end: true state clicks 'resume', Ghost does not fire a webhook, which makes it hard for integrations to run retention flows. This PR causes the member.edited webhook to fire, and causes the webhook to include the subscription field in current and previous. --------- Co-authored-by: Steve Larson <9larsons@gmail.com> --- .../server/models/base/plugins/overrides.js | 12 +- .../repositories/member-repository.js | 51 +++ .../server/services/webhooks/serialize.js | 3 +- .../test/e2e-api/members/webhooks.test.js | 324 ++++++++++++++++++ .../repositories/member-repository.test.js | 27 +- .../services/webhooks/serialize.test.js | 54 +++ 6 files changed, 462 insertions(+), 9 deletions(-) diff --git a/ghost/core/core/server/models/base/plugins/overrides.js b/ghost/core/core/server/models/base/plugins/overrides.js index 6589d8bd9a7..bee74152372 100644 --- a/ghost/core/core/server/models/base/plugins/overrides.js +++ b/ghost/core/core/server/models/base/plugins/overrides.js @@ -96,11 +96,13 @@ module.exports = function (Bookshelf) { (value, key) => key.startsWith(PIVOT_PREFIX) ); - if (this.relationships) { - this.relationships.forEach((relation) => { - if (this._previousRelations && Object.prototype.hasOwnProperty.call(this._previousRelations, relation)) { - clonedModel.related(relation).models = this._previousRelations[relation].models; - } + // Iterate `_previousRelations` rather than `relationships` — a relation + // can carry previous state without being managed by bookshelf-relations + // (e.g. a member's stripeSubscriptions). Behaviour is unchanged for + // relations that are in both. + if (this._previousRelations) { + Object.keys(this._previousRelations).forEach((relation) => { + clonedModel.related(relation).models = this._previousRelations[relation].models; }); } diff --git a/ghost/core/core/server/services/members/members-api/repositories/member-repository.js b/ghost/core/core/server/services/members/members-api/repositories/member-repository.js index e9ba66bd877..1be49c07236 100644 --- a/ghost/core/core/server/services/members/members-api/repositories/member-repository.js +++ b/ghost/core/core/server/services/members/members-api/repositories/member-repository.js @@ -1256,6 +1256,11 @@ module.exports = class MemberRepository { }; let eventData = {}; + // A cancellation (or reactivation) changes no `members` column, so the member + // model event would be suppressed by `wasChanged()`. Remember the pre-update + // subscription so the event can be marked and carry its prior state. + let subscriptionBeforeCancelFlagChange = null; + const stripeCustomerSubscriptionModelShouldBeDeleted = stripeSubscriptionData.metadata && !!stripeSubscriptionData.metadata.ghost_migrated_to && stripeSubscriptionData.status === 'canceled'; if (stripeCustomerSubscriptionModelShouldBeDeleted) { logging.warn(`Subscription ${subscriptionData.subscription_id} is marked for deletion, skipping linking.`); @@ -1300,6 +1305,10 @@ module.exports = class MemberRepository { this.dispatchEvent(offerRedemptionEvent, options); } + if (stripeCustomerSubscriptionModel.get('cancel_at_period_end') !== updatedStripeCustomerSubscriptionModel.get('cancel_at_period_end')) { + subscriptionBeforeCancelFlagChange = stripeCustomerSubscriptionModel; + } + if (stripeCustomerSubscriptionModel.get('mrr') !== updatedStripeCustomerSubscriptionModel.get('mrr') || stripeCustomerSubscriptionModel.get('plan_id') !== updatedStripeCustomerSubscriptionModel.get('plan_id') || stripeCustomerSubscriptionModel.get('status') !== updatedStripeCustomerSubscriptionModel.get('status') || stripeCustomerSubscriptionModel.get('cancel_at_period_end') !== updatedStripeCustomerSubscriptionModel.get('cancel_at_period_end')) { const originalMrrDelta = stripeCustomerSubscriptionModel.get('mrr'); const updatedMrrDelta = updatedStripeCustomerSubscriptionModel.get('mrr'); @@ -1550,6 +1559,48 @@ module.exports = class MemberRepository { updatedMember = await this._Member.edit({status: status}, {...options, id: data.id}); } + // Cancelling (or reactivating) touches no `members` column, so mark the + // subscription relation as the change and carry its prior state. This lets the + // webhook payload express before/after — see `services/webhooks/serialize.js`. + // + // No explicit `emitChange` is needed, and adding one would emit twice: inside a + // transaction `emitChange` queues onto the `committed` handler and defers its + // `wasChanged()` check to commit time (see `models/base/plugins/events.js`), so + // the event already queued by `_Member.edit` above is still pending and setting + // `_changed` here is what lets it through. The e2e test asserting exactly one + // `member.edited` per cancellation guards this. + if (subscriptionBeforeCancelFlagChange && updatedMember) { + // Price/tier relations are needed for the serialized subscription shape + // to match the Admin API member resource (price, tier, plan) + await updatedMember.load([ + 'stripeSubscriptions', + 'stripeSubscriptions.stripePrice', + 'stripeSubscriptions.stripePrice.stripeProduct', + 'stripeSubscriptions.stripePrice.stripeProduct.product' + ], options); + await subscriptionBeforeCancelFlagChange.load([ + 'stripePrice', + 'stripePrice.stripeProduct', + 'stripePrice.stripeProduct.product' + ], options); + updatedMember._changed = { + ...updatedMember._changed, + stripeSubscriptions: { + cancel_at_period_end: subscriptionBeforeCancelFlagChange.get('cancel_at_period_end') + } + }; + // `previous` must be the full collection — a member can have multiple + // subscriptions — with only the changed one swapped for its prior state + updatedMember._previousRelations = { + ...updatedMember._previousRelations, + stripeSubscriptions: { + models: updatedMember.related('stripeSubscriptions').models.map((subscription) => { + return subscription.id === subscriptionBeforeCancelFlagChange.id ? subscriptionBeforeCancelFlagChange : subscription; + }) + } + }; + } + const newMemberProductIds = memberProducts.map(product => product.id); const oldMemberProductIds = oldMemberProducts.map(product => product.id); diff --git a/ghost/core/core/server/services/webhooks/serialize.js b/ghost/core/core/server/services/webhooks/serialize.js index a0729a9af74..134a2ea7b47 100644 --- a/ghost/core/core/server/services/webhooks/serialize.js +++ b/ghost/core/core/server/services/webhooks/serialize.js @@ -9,7 +9,8 @@ // API-serialized payload, where some keys are renamed (members `products` → `tiers`). const SERIALIZED_KEYS = { members: { - products: 'tiers' + products: 'tiers', + stripeSubscriptions: 'subscriptions' } }; diff --git a/ghost/core/test/e2e-api/members/webhooks.test.js b/ghost/core/test/e2e-api/members/webhooks.test.js index 7b3312ef563..173113bda65 100644 --- a/ghost/core/test/e2e-api/members/webhooks.test.js +++ b/ghost/core/test/e2e-api/members/webhooks.test.js @@ -6,6 +6,8 @@ const stripe = require('stripe'); const {Product} = require('../../../core/server/models/product'); const {agentProvider, mockManager, fixtureManager, matchers} = require('../../utils/e2e-framework'); const models = require('../../../core/server/models'); +const modelEvents = require('../../../core/server/lib/common/events'); +const createWebhookSerializer = require('../../../core/server/services/webhooks/serialize'); const urlServiceUtils = require('../../utils/url-service-utils'); const urlUtils = require('../../../core/shared/url-utils').default; const DomainEvents = require('@tryghost/domain-events'); @@ -366,12 +368,39 @@ describe('Members API', function () { secret: process.env.WEBHOOK_SECRET }); + // A cancellation changes no members column, so the model event is only + // emitted if the subscription relation is explicitly marked as the change. + // Counting matters: `emitChange` defers its `wasChanged()` check to commit + // time inside a transaction, so it is possible to emit twice by accident. + const memberEditedEvents = []; + const captureMemberEdited = model => memberEditedEvents.push(model); + modelEvents.on('member.edited', captureMemberEdited); + await membersAgent.post('/webhooks/stripe/') .body(webhookPayload) .header('content-type', 'application/json') .header('stripe-signature', webhookSignature) .expectStatus(200); + modelEvents.removeListener('member.edited', captureMemberEdited); + assert.equal(memberEditedEvents.length, 1, 'A cancellation should emit exactly one member.edited'); + assert.ok(memberEditedEvents[0]._changed.stripeSubscriptions, 'The event should be marked as a subscription change'); + + // The payload a webhook subscriber actually receives must show both states + const webhookBody = await createWebhookSerializer({getRequiredRelations: () => []})( + 'member.edited', memberEditedEvents[0] + ); + assert.equal(webhookBody.member.current.subscriptions[0].cancel_at_period_end, true, + 'The webhook payload should show the subscription now cancelling'); + assert.equal(webhookBody.member.previous.subscriptions[0].cancel_at_period_end, false, + 'The webhook payload should show the subscription was not cancelling before'); + // The subscription shape must match the Admin API member resource — + // price and tier only serialize when their relations are loaded + assert.equal(webhookBody.member.current.subscriptions[0].price.amount, 500, + 'The webhook payload subscriptions should include the price'); + assert.equal(webhookBody.member.previous.subscriptions[0].price.amount, 500, + 'The previous subscriptions should include the price'); + // Check that the subscription has been set to cancel and has saved the cancellation reason const {body: body2} = await adminAgent.get('/members/' + initialMember.id + '/'); assert.equal(body2.members.length, 1, 'The member does not exist'); @@ -418,6 +447,301 @@ describe('Members API', function () { }); }); + it('Handles reinstating a subscription that was set to cancel at period end', async function () { + const customer_id = createStripeID('cust'); + const subscription_id = createStripeID('sub'); + + set(subscription, { + id: subscription_id, + customer: customer_id, + status: 'active', + items: { + type: 'list', + data: [{ + id: 'item_123', + price: { + id: 'price_123', + product: 'product_123', + active: true, + nickname: 'Monthly', + currency: 'usd', + recurring: { + interval: 'month' + }, + unit_amount: 500, + type: 'recurring' + } + }] + }, + start_date: Date.now() / 1000, + current_period_end: Date.now() / 1000 + (60 * 60 * 24 * 31), + cancel_at_period_end: false + }); + + set(customer, { + id: customer_id, + name: 'Reinstate me before the cycle ends', + email: 'reinstate-me@test.com', + subscriptions: { + type: 'list', + data: [subscription] + } + }); + + await createMemberFromStripe(); + + const sendSubscriptionUpdated = async () => { + const payload = JSON.stringify({ + type: 'customer.subscription.updated', + data: { + object: subscription + } + }); + const signature = stripe.webhooks.generateTestHeaderString({ + payload, + secret: process.env.WEBHOOK_SECRET + }); + + const captured = []; + const capture = model => captured.push(model); + modelEvents.on('member.edited', capture); + + await membersAgent.post('/webhooks/stripe/') + .body(payload) + .header('content-type', 'application/json') + .header('stripe-signature', signature) + .expectStatus(200); + + modelEvents.removeListener('member.edited', capture); + + // Cancelling dispatches a staff-notification domain event; drain it so + // it does not outlive the test and stall teardown + await DomainEvents.allSettled(); + + return captured; + }; + + // Setup: cancel first, since a subscription can only be reinstated from + // a cancelling state + set(subscription, { + ...subscription, + canceled_at: Date.now() / 1000, + cancel_at_period_end: true + }); + await sendSubscriptionUpdated(); + + // The change under test: flip the flag back + set(subscription, { + ...subscription, + canceled_at: null, + cancel_at_period_end: false + }); + const reinstateEvents = await sendSubscriptionUpdated(); + + assert.equal(reinstateEvents.length, 1, 'Reinstating should emit exactly one member.edited'); + + const webhookBody = await createWebhookSerializer({getRequiredRelations: () => []})( + 'member.edited', reinstateEvents[0] + ); + assert.equal(webhookBody.member.current.subscriptions[0].cancel_at_period_end, false, + 'The webhook payload should show the subscription is no longer cancelling'); + assert.equal(webhookBody.member.previous.subscriptions[0].cancel_at_period_end, true, + 'The webhook payload should show the subscription was cancelling before'); + }); + + it('Includes every subscription in the previous payload when one of several is cancelled', async function () { + const customer_id = createStripeID('cust'); + const first_subscription_id = createStripeID('sub'); + const second_subscription_id = createStripeID('sub'); + + const buildSubscription = (id, itemId) => ({ + id, + customer: customer_id, + status: 'active', + items: { + type: 'list', + data: [{ + id: itemId, + price: { + id: 'price_123', + product: 'product_123', + active: true, + nickname: 'Monthly', + currency: 'usd', + recurring: { + interval: 'month' + }, + unit_amount: 500, + type: 'recurring' + } + }] + }, + start_date: Date.now() / 1000, + current_period_end: Date.now() / 1000 + (60 * 60 * 24 * 31), + cancel_at_period_end: false + }); + + set(subscription, buildSubscription(first_subscription_id, 'item_first')); + const secondSubscription = buildSubscription(second_subscription_id, 'item_second'); + subscriptionOverrides[second_subscription_id] = secondSubscription; + + set(customer, { + id: customer_id, + name: 'Two subscriptions, cancel only one', + email: 'cancel-one-of-two@test.com', + subscriptions: { + type: 'list', + data: [subscription, secondSubscription] + } + }); + + const initialMember = await createMemberFromStripe(); + assert.equal(initialMember.subscriptions.length, 2, 'The member should start with two subscriptions'); + + // Cancel only the second subscription + Object.assign(secondSubscription, { + canceled_at: Date.now() / 1000, + cancel_at_period_end: true + }); + + const payload = JSON.stringify({ + type: 'customer.subscription.updated', + data: { + object: secondSubscription + } + }); + const signature = stripe.webhooks.generateTestHeaderString({ + payload, + secret: process.env.WEBHOOK_SECRET + }); + + const captured = []; + const capture = model => captured.push(model); + modelEvents.on('member.edited', capture); + + await membersAgent.post('/webhooks/stripe/') + .body(payload) + .header('content-type', 'application/json') + .header('stripe-signature', signature) + .expectStatus(200); + + modelEvents.removeListener('member.edited', capture); + await DomainEvents.allSettled(); + + assert.equal(captured.length, 1, 'Cancelling one of two subscriptions should emit exactly one member.edited'); + + const webhookBody = await createWebhookSerializer({getRequiredRelations: () => []})( + 'member.edited', captured[0] + ); + const currentSubs = webhookBody.member.current.subscriptions; + const previousSubs = webhookBody.member.previous.subscriptions; + + assert.equal(currentSubs.length, 2, 'current must carry both subscriptions'); + assert.equal(previousSubs.length, 2, + 'previous must carry the full collection, not only the changed subscription'); + assert.deepEqual(previousSubs.map(s => s.id).sort(), currentSubs.map(s => s.id).sort(), + 'previous and current must describe the same set of subscriptions'); + + assert.equal(currentSubs.find(s => s.id === second_subscription_id).cancel_at_period_end, true, + 'current must show the cancelled subscription as cancelling'); + assert.equal(currentSubs.find(s => s.id === first_subscription_id).cancel_at_period_end, false, + 'current must show the untouched subscription unchanged'); + assert.equal(previousSubs.find(s => s.id === second_subscription_id).cancel_at_period_end, false, + 'previous must show the changed subscription in its prior state'); + assert.equal(previousSubs.find(s => s.id === first_subscription_id).cancel_at_period_end, false, + 'previous must show the untouched subscription unchanged'); + }); + + it('Does not emit member.edited when a subscription update leaves cancel_at_period_end unchanged', async function () { + const customer_id = createStripeID('cust'); + const subscription_id = createStripeID('sub'); + + set(subscription, { + id: subscription_id, + customer: customer_id, + status: 'active', + items: { + type: 'list', + data: [{ + id: 'item_123', + price: { + id: 'price_123', + product: 'product_123', + active: true, + nickname: 'Monthly', + currency: 'usd', + recurring: { + interval: 'month' + }, + unit_amount: 500, + type: 'recurring' + } + }] + }, + start_date: Date.now() / 1000, + current_period_end: Date.now() / 1000 + (60 * 60 * 24 * 31), + cancel_at_period_end: false + }); + + set(customer, { + id: customer_id, + name: 'Renew me quietly', + email: 'renew-me-quietly@test.com', + subscriptions: { + type: 'list', + data: [subscription] + } + }); + + const initialMember = await createMemberFromStripe(); + // Anchor: the webhook path under test is only reached for a linked paid + // member — without this a broken fixture would make the test pass vacuously + assert.equal(initialMember.status, 'paid', 'The member must start paid'); + assert.equal(initialMember.subscriptions.length, 1, 'The member must start with one linked subscription'); + + // A renewal-style update: the billing period moves, the cancel flag does not. + // This must stay silent — otherwise every Stripe sync becomes webhook spam. + const renewedPeriodEnd = Math.floor(Date.now() / 1000) + (60 * 60 * 24 * 62); + set(subscription, { + ...subscription, + current_period_end: renewedPeriodEnd + }); + + const payload = JSON.stringify({ + type: 'customer.subscription.updated', + data: { + object: subscription + } + }); + const signature = stripe.webhooks.generateTestHeaderString({ + payload, + secret: process.env.WEBHOOK_SECRET + }); + + const captured = []; + const capture = model => captured.push(model); + modelEvents.on('member.edited', capture); + + await membersAgent.post('/webhooks/stripe/') + .body(payload) + .header('content-type', 'application/json') + .header('stripe-signature', signature) + .expectStatus(200); + + modelEvents.removeListener('member.edited', capture); + await DomainEvents.allSettled(); + + // Anchor: the update must have been fully processed — proving the silence + // comes from the wasChanged() suppression, not from the webhook + // short-circuiting before linkSubscription + const subscriptionRow = await getSubscription(subscription_id); + assert.equal(Math.floor(subscriptionRow.get('current_period_end').getTime() / 1000), renewedPeriodEnd, + 'The renewal must have been persisted by linkSubscription'); + + assert.equal(captured.length, 0, + 'A subscription update without a cancel_at_period_end change must not emit member.edited'); + }); + it('Handles immediate cancellation of paid subscriptions', async function () { const customer_id = createStripeID('cust'); const subscription_id = createStripeID('sub'); diff --git a/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js b/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js index a19789f784f..b66c125b072 100644 --- a/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js @@ -454,7 +454,11 @@ describe('MemberRepository', function () { }), edit: sinon.stub().resolves({ attributes: {}, - _previousAttributes: {} + _previousAttributes: {}, + // The real _Member.edit resolves a bookshelf model; linkSubscription + // loads relations off it when a subscription cancel flag changes + load: sinon.stub().resolvesThis(), + related: sinon.stub().returns({models: []}) }) }; @@ -561,7 +565,11 @@ describe('MemberRepository', function () { }), edit: sinon.stub().resolves({ attributes: {}, - _previousAttributes: {} + _previousAttributes: {}, + // The real _Member.edit resolves a bookshelf model; linkSubscription + // loads relations off it when a subscription cancel flag changes + load: sinon.stub().resolvesThis(), + related: sinon.stub().returns({models: []}) }) }; MemberPaidSubscriptionEvent = { @@ -696,6 +704,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), get: sinon.stub().withArgs('offer_id').returns(null) }); @@ -1036,6 +1045,7 @@ describe('MemberRepository', function () { // Existing subscription sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), get: sinon.stub().withArgs('offer_id').returns(null) }); @@ -1333,6 +1343,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1380,6 +1391,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1426,6 +1438,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1490,6 +1503,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1552,6 +1566,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1620,6 +1635,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1687,6 +1703,7 @@ describe('MemberRepository', function () { }); sinon.stub(repo, 'getSubscriptionByStripeID').resolves({ + load: sinon.stub().resolvesThis(), id: 'sub_db_id', get: sinon.stub().callsFake((key) => { if (key === 'offer_id') { @@ -1953,7 +1970,11 @@ describe('MemberRepository', function () { }), edit: sinon.stub().resolves({ attributes: {}, - _previousAttributes: {} + _previousAttributes: {}, + // The real _Member.edit resolves a bookshelf model; linkSubscription + // loads relations off it when a subscription cancel flag changes + load: sinon.stub().resolvesThis(), + related: sinon.stub().returns({models: []}) }) }; diff --git a/ghost/core/test/unit/server/services/webhooks/serialize.test.js b/ghost/core/test/unit/server/services/webhooks/serialize.test.js index db552fe93e7..345590dba87 100644 --- a/ghost/core/test/unit/server/services/webhooks/serialize.test.js +++ b/ghost/core/test/unit/server/services/webhooks/serialize.test.js @@ -4,6 +4,7 @@ const sinon = require('sinon'); const {Post} = require('../../../../../core/server/models/post'); const {Member} = require('../../../../../core/server/models/member'); const {Product} = require('../../../../../core/server/models/product'); +const {StripeCustomerSubscription} = require('../../../../../core/server/models/stripe-customer-subscription'); const createSerialize = require('../../../../../core/server/services/webhooks/serialize'); @@ -196,4 +197,57 @@ describe('WebhookService - Serialize', function () { assert.deepEqual(result.member.current.tiers.map(tier => tier.slug), ['gold']); assert.deepEqual(result.member.previous.tiers.map(tier => tier.slug), ['bronze']); }); + + it('includes the previous subscriptions when a cancellation flips cancel_at_period_end', async function () { + // bookshelf only populates _previousAttributes on fetch, and `previous: true` + // cascades into related models — so these have to look fetched, not constructed. + const asFetched = (model) => { + model._previousAttributes = {...model.attributes}; + return model; + }; + const activeSub = asFetched(new StripeCustomerSubscription({ + id: 'sub-1', + subscription_id: 'sub_123', + status: 'active', + cancel_at_period_end: false, + plan_id: 'price_123', + plan_nickname: 'Monthly', + plan_amount: 500, + plan_interval: 'month', + plan_currency: 'usd' + })); + const cancellingSub = asFetched(new StripeCustomerSubscription({ + id: 'sub-1', + subscription_id: 'sub_123', + status: 'active', + cancel_at_period_end: true, + plan_id: 'price_123', + plan_nickname: 'Monthly', + plan_amount: 500, + plan_interval: 'month', + plan_currency: 'usd' + })); + + const memberModel = new Member({ + id: 'member-id', + uuid: 'member-uuid', + email: 'member@example.com', + status: 'paid', + created_at: new Date('2026-01-01T00:00:00.000Z'), + updated_at: new Date('2026-01-01T00:00:00.000Z') + }); + + // Cancelling touches no members column — the subscription relation is the only change + memberModel._previousAttributes = {...memberModel.attributes}; + memberModel._changed = {stripeSubscriptions: {cancel_at_period_end: true}}; + memberModel._previousRelations = {stripeSubscriptions: {models: [activeSub]}}; + memberModel.related('stripeSubscriptions').models = [cancellingSub]; + + sinon.stub(memberModel, 'load').resolves(memberModel); + + const result = await serialize('member.edited', memberModel); + + assert.equal(result.member.current.subscriptions[0].cancel_at_period_end, true); + assert.equal(result.member.previous.subscriptions[0].cancel_at_period_end, false); + }); }); From 4b95c4cd40c49449a390ea3a9b58dc131159b389 Mon Sep 17 00:00:00 2001 From: Evans Juma Date: Mon, 10 Aug 2026 22:26:32 +0300 Subject: [PATCH 17/24] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20contrast=5Ftext=5F?= =?UTF-8?q?color=20returning=20incorrect=20text=20color=20for=20some=20lig?= =?UTF-8?q?ht=20backgrounds=20(#29834)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #27797 Fixes `{{contrast_text_color}}` and every other Ghost consumer of `textColorForBackgroundColor` returning white for light colors such as `#dacafe`, `#ffa5b1`, and `#a3e6ff`. ## Root cause `@tryghost/color-utils@0.2.19` used `.b()` in its YIQ calculation. That accessor is the Lab b-channel, not the RGB blue channel, so many colors were classified incorrectly. --------- Co-authored-by: Steve Larson <9larsons@gmail.com> --- .changeset/warm-hotels-make.md | 6 ++++++ .../helpers/contrast-text-color.test.js | 10 ++++++++++ pnpm-lock.yaml | 20 +++++++++---------- pnpm-workspace.yaml | 2 +- 4 files changed, 27 insertions(+), 11 deletions(-) create mode 100644 .changeset/warm-hotels-make.md diff --git a/.changeset/warm-hotels-make.md b/.changeset/warm-hotels-make.md new file mode 100644 index 00000000000..3dd25e644c1 --- /dev/null +++ b/.changeset/warm-hotels-make.md @@ -0,0 +1,6 @@ +--- +"@tryghost/kg-default-nodes": patch +"@tryghost/koenig-lexical": patch +--- + +Fixed contrast text colors for light backgrounds diff --git a/ghost/core/test/unit/frontend/helpers/contrast-text-color.test.js b/ghost/core/test/unit/frontend/helpers/contrast-text-color.test.js index 8446e027b55..daed1ab172c 100644 --- a/ghost/core/test/unit/frontend/helpers/contrast-text-color.test.js +++ b/ghost/core/test/unit/frontend/helpers/contrast-text-color.test.js @@ -16,6 +16,16 @@ describe('{{contrast_text_color}} helper', function () { assert.equal(contrast_text_color('#FFFFFF'), '#000000'); }); + it('returns black for other light backgrounds', function () { + ['#dacafe', '#ffa5b1', '#a3e6ff'].forEach(color => { + assert.equal(contrast_text_color(color), '#000000'); + }); + }); + + it('returns white for mid-tone backgrounds', function () { + assert.equal(contrast_text_color('#808080'), '#FFFFFF'); + }); + it('falls back to white for invalid colors', function () { assert.equal(contrast_text_color(''), '#FFFFFF'); }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 19b42319b08..1c407e16296 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -136,8 +136,8 @@ catalogs: specifier: 3.2.1 version: 3.2.1 '@tryghost/color-utils': - specifier: 0.2.19 - version: 0.2.19 + specifier: 0.2.20 + version: 0.2.20 '@tryghost/custom-fonts': specifier: 1.0.11 version: 1.0.11 @@ -682,7 +682,7 @@ importers: version: link:../admin-x-framework '@tryghost/color-utils': specifier: 'catalog:' - version: 0.2.19 + version: 0.2.20 '@tryghost/custom-fonts': specifier: 'catalog:' version: 1.0.11 @@ -1231,7 +1231,7 @@ importers: version: link:../admin-x-framework '@tryghost/color-utils': specifier: 'catalog:' - version: 0.2.19 + version: 0.2.20 '@tryghost/ember-promise-modals': specifier: 2.0.1 version: 2.0.1(ember-source@3.24.0(@babel/core@7.29.7(supports-color@10.2.2))(supports-color@10.2.2))(postcss@8.5.16)(supports-color@10.2.2) @@ -2161,7 +2161,7 @@ importers: version: 3.2.1(better-sqlite3@12.11.1)(express@4.22.2(supports-color@10.2.2))(mysql2@3.22.5(@types/node@22.20.0))(sqlite3@5.1.7(bluebird@3.7.2)(supports-color@10.2.2))(supports-color@10.2.2) '@tryghost/color-utils': specifier: 'catalog:' - version: 0.2.19 + version: 0.2.20 '@tryghost/config-url-helpers': specifier: 1.0.27 version: 1.0.27 @@ -2929,7 +2929,7 @@ importers: version: 0.13.1(lexical@0.13.1) '@tryghost/color-utils': specifier: 'catalog:' - version: 0.2.19 + version: 0.2.20 '@tryghost/kg-clean-basic-html': specifier: workspace:~ version: link:../kg-clean-basic-html @@ -3461,7 +3461,7 @@ importers: version: 16.3.2(@testing-library/dom@10.4.1)(@types/react-dom@18.3.7(@types/react@18.3.31))(@types/react@18.3.31)(react-dom@18.3.1(react@18.3.1))(react@18.3.1) '@tryghost/color-utils': specifier: 'catalog:' - version: 0.2.19 + version: 0.2.20 '@tryghost/helpers': specifier: 'catalog:' version: 1.1.106 @@ -8945,8 +8945,8 @@ packages: '@tryghost/bunyan-rotating-filestream@0.0.11': resolution: {integrity: sha512-mXYF/qvL2E13yZBNubkAKL8peFcdqb8JVQSyKbGZn6GOsamD1WQ/OcXz2N4KKcf/MIs0LO84VPE3owa1Ko8PRA==} - '@tryghost/color-utils@0.2.19': - resolution: {integrity: sha512-jKq4/XFs9v5EoMQKJfH1Wy+AbMNn5wha6l15JoWG2utBIw6vksp7S/WKRN/nubR+nQS4rP7jyqeYYvkko50uDA==} + '@tryghost/color-utils@0.2.20': + resolution: {integrity: sha512-0KLCQDX7TbJGKNihs+OB1+3hEvhdP4YXOKpdbqJlUkvEFdY+Gl1FlI4WL8wnn0v6oc1aOjvrKmva5BUT0pWtIg==} '@tryghost/config-url-helpers@1.0.27': resolution: {integrity: sha512-l7Tfy8BwYWu9PEdDWRLbqH3BdBqQhfD9V+1L4YGG3PsBj6V9V7b6yUJj9glZngusceS2bWHllA1ffCFV+XFvtQ==} @@ -28305,7 +28305,7 @@ snapshots: dependencies: long-timeout: 0.1.1 - '@tryghost/color-utils@0.2.19': + '@tryghost/color-utils@0.2.20': dependencies: '@types/color': 4.2.1 color: 3.2.1 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 6723df2361f..1c6dd9878b4 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -77,7 +77,7 @@ catalog: '@testing-library/react': 14.3.1 '@tryghost/api-framework': 3.3.5 '@tryghost/brute-knex': 3.2.1 - '@tryghost/color-utils': 0.2.19 + '@tryghost/color-utils': 0.2.20 '@tryghost/custom-fonts': 1.0.11 '@tryghost/debug': 2.3.5 '@tryghost/domain-events': 3.3.5 From aeaf5449363daaf07c89d840d7118b830cedf59b Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 10 Aug 2026 20:38:24 +0100 Subject: [PATCH 18/24] Consolidate E2E testing guidance and retire ADR experiment (#29844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref - Retire the abandoned, proposed-only E2E ADR experiment so the repository does not imply that it has a second engineering decision system. - Consolidate the useful Arrange–Act–Assert and Page Object guidance into the canonical E2E documentation and agent instructions. - Reconcile selector guidance with current suite practice: prefer semantic locators, then stable test IDs, while allowing stable structural selectors where semantic locators are unavailable. This is the precursor change for a broader contributor-documentation consolidation. It does not introduce a replacement ADR process or begin the wider `/docs` restructuring. --- adr/0001-aaa-test-structure.md | 40 ---------- adr/0002-page-objects-pattern.md | 101 -------------------------- adr/README.md | 22 ------ docs/README.md | 7 +- e2e/.claude/E2E_TEST_WRITING_GUIDE.md | 40 +++++----- e2e/AGENTS.md | 30 +++++--- e2e/README.md | 58 +++++++++++---- 7 files changed, 82 insertions(+), 216 deletions(-) delete mode 100644 adr/0001-aaa-test-structure.md delete mode 100644 adr/0002-page-objects-pattern.md delete mode 100644 adr/README.md diff --git a/adr/0001-aaa-test-structure.md b/adr/0001-aaa-test-structure.md deleted file mode 100644 index 769b6c79cab..00000000000 --- a/adr/0001-aaa-test-structure.md +++ /dev/null @@ -1,40 +0,0 @@ -# Adopt Arrange–Act–Assert (AAA) Pattern for All Tests - -## Status -Proposed - -## Context - -Our tests are currently written in different styles, which makes them harder to read, understand, and maintain. - -To improve **readability** and make it easier to **debug failing tests**, we want to standardize the structure of tests by following the well-known **Arrange–Act–Assert (AAA)** pattern. - -## Decision - -We will adopt the AAA pattern for tests. Every test should follow this structure: - -1. **Arrange**: Set up data, mocks, page state, or environment -2. **Act**: Perform the action being tested -3. **Assert**: Check the expected outcome - -## Guidelines - -- ✅ Multiple actions and assertions are **allowed** as long as they belong to a **single AAA flow** -- 🚫 **Repeating the full AAA structure in a single test is discouraged**, except for performance‑sensitive tests where setup cost is prohibitively high -- ✂️ If a test involves multiple unrelated behaviors, **split it into separate test cases** -- 🧼 Keep tests focused and predictable: one test = one scenario - -## Example - -```ts -test('user can view their post', async ({ page }) => { - // Arrange - const user = await userFactory.create(); - const post = await postFactory.create({ userId: user.id }); - - // Act - await page.goto(`/posts/${post.id}`); - - // Assert - await expect(page.getByText(post.title)).toBeVisible(); -}); diff --git a/adr/0002-page-objects-pattern.md b/adr/0002-page-objects-pattern.md deleted file mode 100644 index dce4506602d..00000000000 --- a/adr/0002-page-objects-pattern.md +++ /dev/null @@ -1,101 +0,0 @@ -# Adopt Page Objects Pattern for E2E Test Organization - -## Status -Proposed - -## Context - -Our Playwright tests currently interact directly with page elements using raw selectors and actions scattered throughout test files. This approach leads to several issues: - -- **Code duplication**: The same selectors and interactions are repeated across multiple tests -- **Maintenance burden**: When UI changes, we need to update selectors in many places -- **Poor readability**: Tests are cluttered with low-level DOM interactions instead of focusing on business logic -- **Fragile tests**: Direct coupling between tests and implementation details makes tests brittle - -To improve **maintainability**, **readability**, and **test stability**, we want to adopt the Page Objects pattern to encapsulate page-specific knowledge and provide a clean API for test interactions. - -The Page Objects pattern was originally described by [Martin Fowler](https://martinfowler.com/bliki/PageObject.html) as a way to "wrap an HTML page, or fragment, with an application-specific API, allowing you to manipulate page elements without digging around in the HTML." - -## Decision - -We will adopt the Page Objects pattern for organizing E2E tests. Every page or major UI component should have a corresponding page object class that: - -1. **Encapsulates locators**: All element selectors are defined in one place -2. **Provides semantic methods**: Expose high-level actions like `login()`, `createPost()`, `navigateToSettings()` -3. **Abstracts implementation details**: Tests interact with business concepts, not DOM elements -4. **Centralizes page-specific logic**: Complex interactions and waits are handled within page objects -5. **Assertions live in test files**: Page Objects may include readiness guards (e.g., locator.waitFor({state: 'visible'})) before actions, business assertions (expect(...)) should be in tests -6. **Expose semantic locators, hide selectors**: Page Objects should surface public readonly Locators for tests to assert on, while keeping selector strings and construction internal - -## Guidelines - -Following both [Fowler's original principles](https://martinfowler.com/bliki/PageObject.html) and modern Playwright best practices: - -- ✅ **One page object per logical page or major component** (e.g., `LoginPage`, `PostEditor`, `AdminDashboard`) -- ✅ **Model the structure that makes sense to the user**: not necessarily the HTML structure -- ✅ **Use descriptive method names** that reflect user actions (e.g., `fillPostTitle()` not `typeInTitleInput()`) -- ✅ **Return elements or data**: for assertions in tests (e.g., `getErrorMessage()` returns locator) -- ✅ **Include wait methods**: for page readiness and async operations (e.g., `waitForErrorMessage()`) -- ✅ **Chain related actions**: in fluent interfaces where it makes sense -- ✅ **Keep assertions in test files**: page objects should return data/elements, tests should assert -- ✅ **Handle concurrency issues** within page objects (async operations, loading states) -- ✅ **Expose Locators (read-only), not raw selector strings**: you can tests assert against public locators (Playwright encourages it, with helpers on assertion) - - `loginPage.saveButton.click` instead of `page.locator('[data-testid="save-button"]')` -- ✅ **Selector priority: prefer getByRole / getByLabel / data-testid over CSS or XPath.**: add data-testid attributes where needed for stability -- ✅ **Use guards, not assertions, in POM**: prefer locator.waitFor({state:'visible'}) -- 🚫 **Don't include expectations/assertions** in page object methods (following Fowler's recommendation) -- 📁 **Organize in `/e2e/helpers/pages/` directory** with clear naming conventions - -## Example - -```ts -// e2e/helpers/pages/admin/LoginPage.ts -export class LoginPage extends BasePage { - public readonly emailInput = this.page.locator('[data-testid="email-input"]'); - public readonly passwordInput = this.page.locator('[data-testid="password-input"]'); - public readonly loginButton = this.page.locator('[data-testid="login-button"]'); - public readonly errorMessage = this.page.locator('[data-testid="login-error"]'); - - constructor(page: Page) { - super(page); - this.pageUrl = '/login'; - } - - async login(email: string, password: string) { - await this.emailInput.fill(email); - await this.passwordInput.fill(password); - await this.loginButton.click(); - } - - async waitForErrorMessage() { - await this.errorMessage.waitFor({ state: 'visible' }); - return this.errorMessage; - } - - getErrorMessage() { - return this.errorMessage; - } -} - -// In test file -test.describe('Login', () => { - test('invalid credentials', async ({page}) => { - // Arrange - const loginPage = new LoginPage(page); - - // Act - await loginPage.goto(); - await loginPage.login('invalid@email.com', 'wrongpassword'); - const errorMessage = await loginPage.waitForErrorMessage(); - - // Assert - await expect(errorMessage).toHaveText('Invalid credentials'); - }); -} -``` - -## References - -- [Page Object - Martin Fowler](https://martinfowler.com/bliki/PageObject.html) - Original pattern definition -- [Selenium Page Objects](https://selenium-python.readthedocs.io/page-objects.html) - Early implementation guidance -- [Playwright Page Object Model](https://playwright.dev/docs/pom) - Modern Playwright-specific approaches diff --git a/adr/README.md b/adr/README.md deleted file mode 100644 index fd1d936154a..00000000000 --- a/adr/README.md +++ /dev/null @@ -1,22 +0,0 @@ -# Architecture Decision Records (ADRs) - -This directory contains Architecture Decision Records (ADRs) specific to the E2E test suite. - -ADRs are short, version-controlled documents that capture important architectural and process decisions, along with the reasoning behind them. -They help document **why** something was decided — not just **what** was done — which improves transparency, consistency, and long-term maintainability. - -Each ADR includes the following sections: - -- `Status` – `Proposed`, `Accepted`, `Rejected`, etc. -- `Context` – Why the decision was needed -- `Decision` – What was decided and why -- `Guidelines` – (Optional) How the decision should be applied -- `Example` – (Optional) Minimal working example to clarify intent -- `References` - (Optional) - Lists documents, links, or resources that informed or support the decision - -## Guidelines for contributing - -- We follow a simplified and slightly adapted version of the [Michael Nygard ADR format](https://github.com/joelparkerhenderson/architecture-decision-record/tree/main/locales/en/templates/decision-record-template-by-michael-nygard) -- Keep ADRs focused, short, and scoped to one decision -- Start with `Status: Proposed` and update to `Status: Accepted` after code review -- Use sequential filenames with a descriptive slug, for example: `0002-page-objects-pattern.md` diff --git a/docs/README.md b/docs/README.md index 2e8c9c88bd6..031c7305e5d 100644 --- a/docs/README.md +++ b/docs/README.md @@ -77,8 +77,7 @@ Ghost/ ├── koenig/ # Ghost editor (Koenig) packages │ ├── koenig-lexical/ # Lexical-based rich text editor UI │ └── kg-*/ # Editor renderers, converters, and support packages -├── e2e/ # End-to-end tests -├── adr/ # Architecture Decision Records +└── e2e/ # End-to-end tests ``` ## Contributing @@ -109,10 +108,6 @@ Before contributing, please read: - **[API Documentation](https://ghost.org/docs/content-api/)** - Content and Admin API reference - **[Theme Documentation](https://ghost.org/docs/themes/)** - Theme development -## Architecture Decision Records - -The [adr/](../adr/) directory contains Architecture Decision Records (ADRs) that document significant architectural decisions made in the project. - ## Getting Help - **Forum**: [forum.ghost.org](https://forum.ghost.org) diff --git a/e2e/.claude/E2E_TEST_WRITING_GUIDE.md b/e2e/.claude/E2E_TEST_WRITING_GUIDE.md index 915c4bd2810..568be79c1ab 100644 --- a/e2e/.claude/E2E_TEST_WRITING_GUIDE.md +++ b/e2e/.claude/E2E_TEST_WRITING_GUIDE.md @@ -54,10 +54,13 @@ e2e/ ## Page Object Pattern ### Core Principles -1. **ALL selectors must be in Page Objects** - Never put selectors in test files -2. **Page Objects encapsulate page structure and interactions** -3. **Reuse existing Page Objects when possible** -4. **Create focused, single-responsibility Page Objects** +1. **Page Objects contain reusable page structure and interactions** +2. **Reuse existing Page Objects when possible** +3. **Create focused, single-responsibility Page Objects** +4. **Keep necessary structural selectors in Page Objects where practical** + +A direct semantic locator in a test is acceptable for a small, one-off assertion or +interaction when a Page Object would add indirection without reuse. ### Creating a Page Object @@ -77,19 +80,19 @@ export class FeaturePage extends AdminPage { this.pageUrl = '/ghost/#/[path]'; // Selector priority (use in this order): - // 1. data-testid - this.elementName = page.getByTestId('element-id'); - - // 2. ARIA roles with accessible names + // 1. ARIA roles with accessible names this.buttonName = page.getByRole('button', {name: 'Button Text'}); - // 3. Labels for form elements + // 2. Labels for form elements this.elementName = page.getByLabel('Field Label'); - // 4. Text content (for unique text) + // 3. Text content (for unique text) this.elementName = page.getByText('Unique text'); - // 5. Avoid CSS/XPath selectors unless absolutely necessary + // 4. Stable test IDs when semantic locators are unavailable + this.elementName = page.getByTestId('element-id'); + + // 5. Stable structural selectors only when necessary } // Action methods @@ -180,7 +183,7 @@ export class PublicHomePage extends BasePage { **Important: Write self-documenting tests without comments. Test names and method names should clearly express intent. If complex logic is needed, extract it to a well-named method in the Page Object.** -Tests should follow the **Arrange-Act-Assert (AAA)** pattern: +Use **Arrange–Act–Assert (AAA)** as a readability heuristic: - **Arrange**: Set up test data and page objects - **Act**: Perform the actions being tested - **Assert**: Verify the expected outcomes @@ -194,16 +197,13 @@ import {createPostFactory} from '../../data-factory'; test.describe('Feature Name', () => { test('should perform expected behavior', async ({page, ghostInstance}) => { - // Arrange const featurePage = new FeaturePage(page); const postFactory = createPostFactory(page.request); const post = await postFactory.create({title: 'Test Post'}); - // Act await featurePage.goto(); await featurePage.performAction(); - // Assert expect(await featurePage.isElementVisible()).toBe(true); expect(await featurePage.getResultText()).toContain('Expected text'); }); @@ -290,7 +290,7 @@ New factories are added as needed. When you need test data that doesn't have a f ## Best Practices ### DO's -✅ **Use Page Objects for all selectors** +✅ **Use Page Objects for reusable UI structure and interactions** ✅ **Write self-documenting tests** with clear method and test names ✅ **Check existing Page Objects before creating new ones** ✅ **Use proper waits** (`waitForLoadState`, `waitFor`, etc.) @@ -301,13 +301,13 @@ New factories are added as needed. When you need test data that doesn't have a f ✅ **Add meaningful assertions** beyond just visibility checks ### DON'Ts -❌ **Never put selectors in test files** +❌ **Don't duplicate reusable selectors and interactions across test files** ❌ **Don't write comments** - make code self-documenting instead ❌ **Don't use hardcoded waits** (`page.waitForTimeout`) ❌ **Don't use networkidle in waits** (`page.waitForLoadState('networkidle')`) - rely on web assertions to assess readiness instead ❌ **Don't depend on test execution order** ❌ **Don't manually log in** - use the pre-authenticated fixture -❌ **Avoid CSS/XPath selectors** - use semantic selectors +❌ **Avoid XPath and selectors coupled to styling or DOM position** ❌ **Don't create test data manually** if a factory exists ## Common Patterns @@ -451,8 +451,8 @@ You don't need to worry about: ## Validation Checklist Before submitting a test: -- [ ] All selectors are in Page Objects -- [ ] Test follows AAA pattern +- [ ] Reusable UI behavior is in Page Objects +- [ ] Arrange, Act, and Assert phases are easy to identify - [ ] Test is deterministic (not flaky) - [ ] Uses proper waits (no arbitrary timeouts) - [ ] Has meaningful assertions diff --git a/e2e/AGENTS.md b/e2e/AGENTS.md index f8820b41db7..0d387e61489 100644 --- a/e2e/AGENTS.md +++ b/e2e/AGENTS.md @@ -2,14 +2,17 @@ E2E testing guidance for AI assistants (Claude, Codex, etc.) working with Ghost tests. -**IMPORTANT**: When creating or modifying E2E tests, always refer to `./.claude/E2E_TEST_WRITING_GUIDE.md` for comprehensive testing guidelines and patterns. +**IMPORTANT**: `README.md` is the canonical human documentation for E2E testing. +When creating or modifying E2E tests, follow it first. Use +`./.claude/E2E_TEST_WRITING_GUIDE.md` for additional agent-oriented examples. ## Critical Rules -1. **Always follow ADRs** in `../adr/` folder (ADR-0001: AAA pattern, ADR-0002: Page Objects) -2. **Always use pnpm**, never npm -3. **Always run after changes**: `pnpm lint` and `pnpm test:types` -4. **Never use CSS/XPath selectors** - only semantic locators or data-testid -5. **Prefer less comments and giving things clear names** +1. **Always use pnpm**, never npm +2. **Always run after changes**: `pnpm lint` and `pnpm test:types` +3. **Prefer semantic locators**, then stable test IDs +4. **Keep reusable UI structure and interactions in Page Objects** +5. **Avoid selectors coupled to styling or DOM position** +6. **Prefer clear names over explanatory comments** ## Running E2E Tests @@ -74,7 +77,8 @@ export class AnalyticsPage extends AdminPage { ``` ### Rules -- Page Objects are located in `helpers/pages/` +- Put reusable page and major-component behavior in `helpers/pages/` +- Direct semantic locators are acceptable for small, one-off test interactions or assertions - Expose locators as `public readonly` when used with assertions - Methods use semantic names (`login()` not `clickLoginButton()`) - Use `waitFor()` for guards, never `expect()` in page objects @@ -91,7 +95,11 @@ export class AnalyticsPage extends AdminPage { - `getByTestId('analytics-card')` - Suggest adding `data-testid` to Ghost codebase when needed -3. **Never use**: CSS selectors, XPath, nth-child, class names +3. **Structural fallback**: stable attributes when semantic locators are unavailable + +Avoid XPath, `nth-child`, styling classes, and other selectors coupled to DOM +position or presentation. Keep necessary structural selectors in Page Objects where +practical. ### Playwright MCP Usage - Use `mcp__playwright__browser_snapshot` to find elements @@ -145,6 +153,6 @@ After writing tests, verify: 2. Linting passes: `pnpm lint` 3. Types check: `pnpm test:types` 4. Follows AAA pattern with clear sections -5. Uses page objects appropriately -6. Uses semantic locators or data-testid only -7. No hard-coded waits or CSS selectors +5. Uses Page Objects for reusable UI behavior +6. Prefers semantic locators, then stable test IDs +7. Has no hard-coded waits or selectors coupled to styling/DOM position diff --git a/e2e/README.md b/e2e/README.md index 302670b4881..f548fe1c24c 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -138,19 +138,19 @@ e2e/ ### Writing Tests -Tests use [Playwright Test](https://playwright.dev/docs/writing-tests) framework with page objects. -Aim to format tests in Arrange Act Assert style - it will help you with directions when writing your tests. +Tests use [Playwright Test](https://playwright.dev/docs/writing-tests). Use +Arrange–Act–Assert (AAA) as a readability heuristic: set up the scenario, perform +the behavior under test, then verify the outcome. Keep those phases clear through +test structure and naming; comments are only useful when the boundaries would +otherwise be unclear. ```typescript test.describe('Ghost Homepage', () => { test('loads correctly', async ({page}) => { - // ARRANGE - setup fixtures, create helpers, prepare things that helps will need to be executed const homePage = new HomePage(page); - - // ACT - do the actions you need to do, to verify certain behaviour + await homePage.goto(); - - // ASSERT + await expect(homePage.title).toBeVisible(); }); }); @@ -158,15 +158,41 @@ test.describe('Ghost Homepage', () => { ### Using Page Objects -Page objects encapsulate page elements, and interactions. To read more about them, check [this link out](https://www.selenium.dev/documentation/test_practices/encouraged/page_object_models/) and [this link](https://martinfowler.com/bliki/PageObject.html). +Page Objects are the default home for reusable knowledge about a page or major UI +component. They encapsulate locators, readiness guards, and semantic interactions +so tests can describe behavior rather than DOM structure. Assertions stay in test +files. + +Prefer an existing Page Object when a test exercises reusable UI behavior. A direct +semantic locator in a test is acceptable for a small, one-off assertion or +interaction when creating a Page Object would add indirection without reuse. +Structural selectors sometimes remain necessary for iframes, editor internals, +generated theme markup, and elements without an accessible role. Keep those inside +Page Objects where practical and prefer, in order: + +1. Accessible roles, labels, and visible text +2. Stable test IDs +3. Stable structural selectors when no semantic locator exists + +Avoid selectors coupled to visual styling, DOM position, or incidental class names. +See [Playwright's locator guidance](https://playwright.dev/docs/locators) and +[Martin Fowler's Page Object description](https://martinfowler.com/bliki/PageObject.html) +for background. ```typescript // Create a page object for admin login +import type {Locator, Page} from '@playwright/test'; + export class AdminLoginPage { - private pageUrl:string; + private readonly pageUrl = '/ghost'; + public readonly emailInput: Locator; + public readonly passwordInput: Locator; + public readonly signInButton: Locator; - constructor(private page: Page) { - this.pageUrl = '/ghost' + constructor(private readonly page: Page) { + this.emailInput = page.getByLabel('Email address'); + this.passwordInput = page.getByLabel('Password'); + this.signInButton = page.getByRole('button', {name: 'Sign in'}); } async goto(urlToVisit = this.pageUrl) { @@ -174,9 +200,9 @@ export class AdminLoginPage { } async login(email: string, password: string) { - await this.page.fill('[name="identification"]', email); - await this.page.fill('[name="password"]', password); - await this.page.click('button[type="submit"]'); + await this.emailInput.fill(email); + await this.passwordInput.fill(password); + await this.signInButton.click(); } } ``` @@ -246,9 +272,9 @@ Modes: ### Best Practices -1. **Use page object patterns** to separate page elements, actions on the pages, complex logic from tests. They should help you make them more readable and UI elements reusable. +1. **Use Page Objects for reusable UI structure and interactions.** Direct semantic locators are fine for small, one-off assertions where a Page Object would not improve reuse or readability. 2. **Add meaningful assertions** beyond just page loads. Keep assertions in tests. -3. **Use `data-testid` attributes** for reliable element selection, in case you **cannot** locate elements in a simple way. Example: `page.getByLabel('User Name')`. Avoid, css, xpath locators - they make tests brittle. +3. **Prefer semantic locators**, such as `getByRole()` and `getByLabel()`. Use stable test IDs when semantic locators are unavailable. Avoid selectors coupled to styling or DOM position. 4. **Clean up test data** when tests modify Ghost state 5. **Group related tests** in describe blocks 6. **Do not use should to describe test scenarios** From 5405426ee80b5be71cfd7021942ae6acde7cc9ab Mon Sep 17 00:00:00 2001 From: Waqas Ahmed Date: Tue, 11 Aug 2026 01:07:43 +0500 Subject: [PATCH 19/24] Removed third-party spacergif.org request from video card poster (#29833) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Ghost/issues/25059 The video card's web-rendering `poster` attribute pointed at a third-party service (`https://img.spacergif.org/v1/{w}x{h}/0a/spacer.png`) to produce an aspect-ratio-shaped placeholder image. Every visitor viewing a post with a video card triggered a request to that external host, leaking IP/referrer/UA data with no consent. This replaces the web-path poster with a local, inline transparent 1x1 GIF `data:` URI — zero third-party network requests, no visual change. --------- Co-authored-by: Steve Larson <9larsons@gmail.com> --- .changeset/spacy-poodles-hunt.md | 6 +++ .../content/__snapshots__/posts.test.js.snap | 40 +++++++++---------- koenig/kg-default-cards/src/cards/video.ts | 20 +++++++++- .../kg-default-cards/test/cards/video.test.ts | 38 +++++++++++++++++- .../src/nodes/video/video-renderer.ts | 20 +++++++++- .../kg-default-nodes/test/nodes/video.test.ts | 10 ++--- .../test/renderers/video-renderer.test.ts | 12 +++++- 7 files changed, 114 insertions(+), 32 deletions(-) create mode 100644 .changeset/spacy-poodles-hunt.md diff --git a/.changeset/spacy-poodles-hunt.md b/.changeset/spacy-poodles-hunt.md new file mode 100644 index 00000000000..516ab568751 --- /dev/null +++ b/.changeset/spacy-poodles-hunt.md @@ -0,0 +1,6 @@ +--- +"@tryghost/kg-default-nodes": patch +"@tryghost/kg-default-cards": patch +--- + +Removed a third-party network request (spacergif.org) from the web-rendered video card's poster image, replacing it with a local transparent placeholder. Email rendering is unaffected. diff --git a/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap b/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap index 7370722dc92..fadfd9af882 100644 --- a/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap +++ b/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap @@ -1499,7 +1499,7 @@ Snippet Docume", "frontmatter": null, "html": "

This is a post containing all media types for testing URL transformations. It includes images, galleries, files, videos, audio, and an inserted snippet.

\\"Inline
An inline image card
\\"Gallery
\\"Gallery
\\"Gallery
A gallery with three images

File card:

Test Document
A test PDF document
document.pdf
2 MB
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
1000 KB
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
0 Byte
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
1000 KB
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
0 Byte
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
1000 KB
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
0 Byte
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
1000 KB
download-circle

Video card:

- +

Audio card:

\\"audio-thumbnail\\"
Test Audio
0:00
/180

Inserted snippet content below:

This snippet contains all media types for testing URL transformations in reusable content.

Image card:

\\"Snippet
Image in snippet

File card:

Snippet Document
A test document file
snippet-doc.pdf
0 Byte
download-circle

Video card:

- +
0:00
/
`); + expect(serializer.serialize(card.render(opts))).toBe(`
0:00
/
`); }); it('renders for email target', function () { @@ -41,6 +41,42 @@ describe('Video card', function () { expect(output).toContain('
+// element so no third-party request (e.g. spacergif.org) is needed. The
0:00
/1:00
Video caption
+
0:00
/1:00
Video caption
`); const nodes = $generateNodesFromDOM(editor, document) as VideoNode[]; expect(nodes.length).toBe(1); @@ -370,7 +370,7 @@ describe('VideoNode', function () { it('parses video card without caption', editorTest(function () { const document = createDocument(html` -
0:00
/1:00
+
0:00
/1:00
`); const nodes = $generateNodesFromDOM(editor, document) as VideoNode[]; expect(nodes.length).toBe(1); @@ -379,7 +379,7 @@ describe('VideoNode', function () { it('parses video card with custom thumbnail', editorTest(function () { const document = createDocument(html` -
0:00
/1:00
+
0:00
/1:00
`); const nodes = $generateNodesFromDOM(editor, document) as VideoNode[]; expect(nodes.length).toBe(1); diff --git a/koenig/kg-default-nodes/test/renderers/video-renderer.test.ts b/koenig/kg-default-nodes/test/renderers/video-renderer.test.ts index d156b9d5911..5fc12579ae5 100644 --- a/koenig/kg-default-nodes/test/renderers/video-renderer.test.ts +++ b/koenig/kg-default-nodes/test/renderers/video-renderer.test.ts @@ -36,7 +36,7 @@ describe('renderers/video-renderer', function () { assertPrettifiesTo(result.html, html`
- +