From ddb2beced9c2840a17f72fb53d2706629cb461b0 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Tue, 18 Aug 2026 23:41:16 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20donation=20checkout?= =?UTF-8?q?=20failing=20when=20the=20note=20label=20translation=20is=20too?= =?UTF-8?q?=20long?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stripe rejects checkout session creation when a custom field label exceeds 50 characters, but the donation personal-note label (a translated Portal string) was only capped at 255 — a limit copied from the donation_message column, which stores the member's answer, a different Stripe field with a different limit. A translation between 51 and 255 characters would therefore break the entire donation checkout for that locale. The label is now bounded on both sides of sanitising: before, because this is a public endpoint and nothing past the limit can survive, so there is no reason to sanitise an unbounded string; and after, because sanitising can lengthen the string and the post-sanitise length is what Stripe measures. Truncating rather than rejecting keeps the label translated, as falling back to the English default would defeat the purpose of the parameter. --- .../controllers/router-controller.js | 16 +-- .../controllers/router-controller.test.js | 102 +++++++++++++++++- packages/i18n/locales/context.json | 2 +- 3 files changed, 110 insertions(+), 10 deletions(-) diff --git a/ghost/core/core/server/services/members/members-api/controllers/router-controller.js b/ghost/core/core/server/services/members/members-api/controllers/router-controller.js index 57e3a5dc8f2..e340b7dcd8b 100644 --- a/ghost/core/core/server/services/members/members-api/controllers/router-controller.js +++ b/ghost/core/core/server/services/members/members-api/controllers/router-controller.js @@ -1261,20 +1261,24 @@ module.exports = class RouterController { } }; +// Stripe's limit on `custom_fields[].label.custom` — longer labels make session creation fail +const STRIPE_CUSTOM_FIELD_LABEL_MAX_LENGTH = 50; + function parsePersonalNote(rawText) { if (rawText && typeof rawText !== 'string') { logging.warn('Donation personal note is not a string, ignoring'); return ''; } - if (rawText && rawText.length > 255) { - logging.warn('Donation personal note is too long, ignoring:', rawText); - return ''; - } - const safeInput = sanitizeHtml(rawText, { + // Bounded twice, for two different reasons. Before sanitising because this is a + // public endpoint and nothing past the limit can survive anyway, so there is no + // reason to sanitise more than that. After, because sanitising can lengthen the + // string (`&` becomes `&`), and the second bound is the one Stripe measures. + const bounded = (rawText ?? '').slice(0, STRIPE_CUSTOM_FIELD_LABEL_MAX_LENGTH); + const safeInput = sanitizeHtml(bounded, { allowedTags: [], allowedAttributes: {} }); - return safeInput; + return safeInput.slice(0, STRIPE_CUSTOM_FIELD_LABEL_MAX_LENGTH); } diff --git a/ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js b/ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js index b1cb614d9bc..f49bde1eb24 100644 --- a/ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js @@ -584,7 +584,7 @@ describe('RouterController', function () { } })); }); - it('silently discards too-long personal notes', async function () { + it('truncates too-long personal notes', async function () { const routerController = new RouterController({ tiersService, paymentsService, @@ -604,7 +604,7 @@ describe('RouterController', function () { type: 'donation', successUrl: 'https://example.com/?type=success', cancelUrl: 'https://example.com/?type=cancel', - personalNote: 'a'.repeat(1000), + personalNote: 'a'.repeat(51), metadata: { test: 'hello', urlHistory: [ @@ -626,7 +626,103 @@ describe('RouterController', function () { sinon.assert.calledWith(getDonationLinkSpy, sinon.match({ successUrl: 'https://example.com/?type=success', cancelUrl: 'https://example.com/?type=cancel', - personalNote: '', + personalNote: 'a'.repeat(50), + metadata: { + test: 'hello' + } + })); + }); + it('bounds a personal note far past the limit', async function () { + const routerController = new RouterController({ + tiersService, + paymentsService, + offersAPI, + stripeAPIService, + labsService, + settingsCache, + settingsHelpers, + urlUtils, + memberAttributionService: { + getAttribution: sinon.stub().resolves({}) + } + }); + + await routerController.createCheckoutSession({ + body: { + type: 'donation', + successUrl: 'https://example.com/?type=success', + cancelUrl: 'https://example.com/?type=cancel', + personalNote: 'a'.repeat(10000), + metadata: { + test: 'hello', + urlHistory: [ + { + path: 'https://example.com/', + time: Date.now(), + referrerMedium: null, + referrerSource: 'ghost-explore', + referrerUrl: 'https://example.com/blog/' + } + ] + } + } + }, { + writeHead: () => {}, + end: () => {} + }); + sinon.assert.calledOnce(getDonationLinkSpy); + sinon.assert.calledWith(getDonationLinkSpy, sinon.match({ + successUrl: 'https://example.com/?type=success', + cancelUrl: 'https://example.com/?type=cancel', + personalNote: 'a'.repeat(50), + metadata: { + test: 'hello' + } + })); + }); + it('truncates personal notes that grow past the limit when sanitised', async function () { + const routerController = new RouterController({ + tiersService, + paymentsService, + offersAPI, + stripeAPIService, + labsService, + settingsCache, + settingsHelpers, + urlUtils, + memberAttributionService: { + getAttribution: sinon.stub().resolves({}) + } + }); + + await routerController.createCheckoutSession({ + body: { + type: 'donation', + successUrl: 'https://example.com/?type=success', + cancelUrl: 'https://example.com/?type=cancel', + personalNote: 'a'.repeat(30) + '&'.repeat(10), + metadata: { + test: 'hello', + urlHistory: [ + { + path: 'https://example.com/', + time: Date.now(), + referrerMedium: null, + referrerSource: 'ghost-explore', + referrerUrl: 'https://example.com/blog/' + } + ] + } + } + }, { + writeHead: () => {}, + end: () => {} + }); + sinon.assert.calledOnce(getDonationLinkSpy); + sinon.assert.calledWith(getDonationLinkSpy, sinon.match({ + successUrl: 'https://example.com/?type=success', + cancelUrl: 'https://example.com/?type=cancel', + personalNote: 'a'.repeat(30) + '&'.repeat(4), metadata: { test: 'hello' } diff --git a/packages/i18n/locales/context.json b/packages/i18n/locales/context.json index 602df897d02..3f6159e504c 100644 --- a/packages/i18n/locales/context.json +++ b/packages/i18n/locales/context.json @@ -10,7 +10,7 @@ "Account": "A label in Portal for your account area", "Account details updated successfully": "Popover message in Portal", "Account settings": "A label in Portal for your account settings", - "Add a personal note": "Becomes the field label for donations in Stripe", + "Add a personal note": "Becomes the field label for donations in Stripe, which limits it to 50 characters", "Add comment": "Button text to post a comment", "Add context to your comment, share your name and expertise to foster a healthy discussion.": "Invitation to include additional info when commenting", "Add reply": "Button text to post your reply", From 16869380152bf2ce438ceaf7dadb82efa6fd3100 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 19 Aug 2026 00:15:19 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20negated=20newsletter?= =?UTF-8?q?=20filters=20being=20read=20as=20their=20opposite?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A members filter like (newsletters.slug:-weekly+email_disabled:0) says not subscribed to the weekly newsletter, but Admin read it as subscribed, and because Admin writes the parsed filter back on save, opening and saving such a segment silently inverted its meaning. The parser now takes the subscription state from the slug clause's own polarity and treats the email_disabled clause purely as the marker of a newsletter-subscription compound, matching how the legacy Ember admin read these filters. --- .../src/members/member-filter-query.test.ts | 22 ++++++++++++ apps/admin/src/members/member-filter-query.ts | 35 ++++++++----------- 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/apps/admin/src/members/member-filter-query.test.ts b/apps/admin/src/members/member-filter-query.test.ts index bbbb058fdef..ccb8a08071b 100644 --- a/apps/admin/src/members/member-filter-query.test.ts +++ b/apps/admin/src/members/member-filter-query.test.ts @@ -36,6 +36,28 @@ describe('member-filter-query', () => { ]); }); + it('reads newsletter subscription state from the slug clause polarity', () => { + expect(stripIds(parseMemberFilter('(newsletters.slug:-weekly,email_disabled:1)', 'UTC'))).toEqual([ + {field: 'newsletters.weekly', operator: 'is', values: ['unsubscribed']} + ]); + + // Hand-written shapes pairing the slug with the "wrong" join and + // email_disabled value still follow the slug's polarity. + expect(stripIds(parseMemberFilter('(newsletters.slug:-weekly+email_disabled:0)', 'UTC'))).toEqual([ + {field: 'newsletters.weekly', operator: 'is', values: ['unsubscribed']} + ]); + + expect(stripIds(parseMemberFilter('(newsletters.slug:weekly,email_disabled:1)', 'UTC'))).toEqual([ + {field: 'newsletters.weekly', operator: 'is', values: ['subscribed']} + ]); + }); + + it('round-trips a mismatched unsubscribed compound to the canonical shape', () => { + const parsed = parseMemberFilter('(newsletters.slug:-weekly+email_disabled:0)', 'UTC'); + + expect(serializeMemberFilters(parsed, 'UTC')).toBe('(newsletters.slug:-weekly,email_disabled:1)'); + }); + it('parses legacy scalar set filters and preserves singleton offer ids', () => { const parsed = parseMemberFilter('offer_redemptions:\'offer_123\'', 'UTC'); diff --git a/apps/admin/src/members/member-filter-query.ts b/apps/admin/src/members/member-filter-query.ts index 6b1917d004a..9e7826b9a80 100644 --- a/apps/admin/src/members/member-filter-query.ts +++ b/apps/admin/src/members/member-filter-query.ts @@ -103,13 +103,15 @@ function matchNewsletterGroupedNode(node: AstNode): ParsedPredicate | null { } let slug: string | undefined; - let emailDisabledValue: number | undefined; + let slugNegated = false; + let hasEmailDisabled = false; for (const child of compound.children) { const newsletterSlug = child['newsletters.slug']; if (typeof newsletterSlug === 'string') { slug = newsletterSlug; + slugNegated = false; } if ( @@ -119,34 +121,27 @@ function matchNewsletterGroupedNode(node: AstNode): ParsedPredicate | null { typeof (newsletterSlug as Record).$ne === 'string' ) { slug = (newsletterSlug as Record).$ne; + slugNegated = true; } if (typeof child.email_disabled === 'number') { - emailDisabledValue = child.email_disabled; + hasEmailDisabled = true; } } - if (!slug) { + if (!slug || !hasEmailDisabled) { return null; } - if (compound.operator === '$and' && emailDisabledValue === 0) { - return { - field: `newsletters.${slug}`, - operator: 'is', - values: ['subscribed'] - }; - } - - if (compound.operator === '$or' && emailDisabledValue === 1) { - return { - field: `newsletters.${slug}`, - operator: 'is', - values: ['unsubscribed'] - }; - } - - return null; + // The slug clause's polarity is the subscription state. Serialize pairs it + // with a fixed join + email_disabled shape, but hand-written filters may + // pair them differently; the email_disabled clause only marks the compound + // as a newsletter subscription filter and never flips its meaning. + return { + field: `newsletters.${slug}`, + operator: 'is', + values: [slugNegated ? 'unsubscribed' : 'subscribed'] + }; } function matchFeedbackGroupedNode(node: AstNode): ParsedPredicate | null { From 5ace62033e0578eca7c303450503035c78237c62 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Wed, 19 Aug 2026 10:01:58 +0100 Subject: [PATCH 3/4] Moved Mailgun message ID handling to shared services (#30103) ref https://linear.app/ghost/issue/BER-3851 Gift delivery also needs to interpret Mailgun send results, so keeping the helper under automations would make a cross-service transport concern appear automation-owned. Moving it to the shared services area gives existing and future mail callers one neutral implementation without changing behavior. --- ghost/core/core/server/services/automations/poll.ts | 2 +- .../automation-email-analytics-batch-processor.ts | 2 +- .../server/services/{automations => lib}/mailgun-message-id.ts | 0 .../services/{automations => lib}/mailgun-message-id.test.ts | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename ghost/core/core/server/services/{automations => lib}/mailgun-message-id.ts (100%) rename ghost/core/test/unit/server/services/{automations => lib}/mailgun-message-id.test.ts (95%) diff --git a/ghost/core/core/server/services/automations/poll.ts b/ghost/core/core/server/services/automations/poll.ts index 29e40def1df..c3753e383ab 100644 --- a/ghost/core/core/server/services/automations/poll.ts +++ b/ghost/core/core/server/services/automations/poll.ts @@ -1,5 +1,5 @@ import type {AutomationStepToRun, AutomationsRepository} from './automations-repository'; -import {getMailgunMessageId} from './mailgun-message-id'; +import {getMailgunMessageId} from '../lib/mailgun-message-id'; import logging from '@tryghost/logging'; import errors from '@tryghost/errors'; import {MEMBER_WELCOME_EMAIL_ELIGIBLE_STATUSES, MEMBER_WELCOME_EMAIL_SLUGS} from '../member-welcome-emails/constants'; diff --git a/ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.ts b/ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.ts index 47ee038fe4b..aff4e3e5e2d 100644 --- a/ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.ts +++ b/ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.ts @@ -1,4 +1,4 @@ -import {normalizeMailgunMessageId} from '../automations/mailgun-message-id'; +import {normalizeMailgunMessageId} from '../lib/mailgun-message-id'; import type * as automationsApi from '../automations/automations-api'; import type {AutomatedEmailEvents, AutomatedEmailRecipientWithMailgunId} from '../automations/automations-repository'; import type {BatchEventProcessor} from './batch-event-processor'; diff --git a/ghost/core/core/server/services/automations/mailgun-message-id.ts b/ghost/core/core/server/services/lib/mailgun-message-id.ts similarity index 100% rename from ghost/core/core/server/services/automations/mailgun-message-id.ts rename to ghost/core/core/server/services/lib/mailgun-message-id.ts diff --git a/ghost/core/test/unit/server/services/automations/mailgun-message-id.test.ts b/ghost/core/test/unit/server/services/lib/mailgun-message-id.test.ts similarity index 95% rename from ghost/core/test/unit/server/services/automations/mailgun-message-id.test.ts rename to ghost/core/test/unit/server/services/lib/mailgun-message-id.test.ts index c67ccb8de4b..4bcf0ea2c50 100644 --- a/ghost/core/test/unit/server/services/automations/mailgun-message-id.test.ts +++ b/ghost/core/test/unit/server/services/lib/mailgun-message-id.test.ts @@ -1,6 +1,6 @@ import assert from 'node:assert/strict'; -import {getMailgunMessageId, normalizeMailgunMessageId} from '../../../../../core/server/services/automations/mailgun-message-id'; +import {getMailgunMessageId, normalizeMailgunMessageId} from '../../../../../core/server/services/lib/mailgun-message-id'; describe('Mailgun message ID handling', function () { describe('normalizeMailgunMessageId', function () { From e17a544aee11bbd8ddc9721bde3538c87305be94 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 19 Aug 2026 09:46:33 +0100 Subject: [PATCH 4/4] Fixed flaky onboarding E2E test Admin fills in a missing user preference by writing the whole accessibility blob merged over the user it read when the page loaded, and it queues one such write per mounted consumer, so the state the test set through the API landed in between those queued writes and the next one reverted it. Setting the onboarding state now detaches the page from Admin first so the queue is discarded, then retries the write until the server reports the state back, leaving the caller to navigate and load Admin against it. --- e2e/tests/admin/onboarding.test.ts | 50 +++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/e2e/tests/admin/onboarding.test.ts b/e2e/tests/admin/onboarding.test.ts index 764f95efab2..f0b8391b8eb 100644 --- a/e2e/tests/admin/onboarding.test.ts +++ b/e2e/tests/admin/onboarding.test.ts @@ -4,6 +4,12 @@ import type {Page} from '@playwright/test'; type ChecklistState = 'pending' | 'started' | 'completed' | 'dismissed'; +interface OnboardingPreferences { + completedSteps: string[]; + checklistState: ChecklistState; + startedAt?: string; +} + const allSteps = ['customize-design', 'first-post', 'build-audience', 'share-publication']; const activeStartedAt = '2026-05-01T00:00:00.000Z'; const legacyNavigationSteps: Array<[string, RegExp]> = [ @@ -21,18 +27,18 @@ async function getCurrentUser(page: Page) { return body.users[0]; } -async function setOnboardingState(page: Page, checklistState: ChecklistState, completedSteps: string[] = [], startedAt: string | null | undefined = checklistState === 'started' ? activeStartedAt : undefined) { +async function getOnboardingPreferences(page: Page) { const user = await getCurrentUser(page); const preferences = user.accessibility ? JSON.parse(user.accessibility) : {}; - preferences.onboarding = { - completedSteps, - checklistState - }; + return preferences.onboarding; +} - if (startedAt) { - preferences.onboarding.startedAt = startedAt; - } +async function putOnboardingPreferences(page: Page, onboarding: OnboardingPreferences) { + const user = await getCurrentUser(page); + const preferences = user.accessibility ? JSON.parse(user.accessibility) : {}; + + preferences.onboarding = onboarding; const response = await page.request.put(`/ghost/api/admin/users/${user.id}/?include=roles`, { data: { @@ -43,15 +49,31 @@ async function setOnboardingState(page: Page, checklistState: ChecklistState, co } }); expect(response.ok()).toBe(true); - - await page.reload({waitUntil: 'load'}); } -async function getOnboardingPreferences(page: Page) { - const user = await getCurrentUser(page); - const preferences = user.accessibility ? JSON.parse(user.accessibility) : {}; +/** + * Sets the onboarding preferences through the API and leaves the page detached + * from Admin, so the caller navigates to load Admin against them. + * + * Admin fills in a missing user preference by writing the whole accessibility + * blob, merged over the user it read when the page loaded, and it queues one + * such write per mounted consumer. A write from here lands between those queued + * writes and the next one reverts it, so detach first to discard the queue and + * write until the server agrees to cover a write already on the wire. + */ +async function setOnboardingState(page: Page, checklistState: ChecklistState, completedSteps: string[] = [], startedAt: string | null | undefined = checklistState === 'started' ? activeStartedAt : undefined) { + const onboarding: OnboardingPreferences = {completedSteps, checklistState}; - return preferences.onboarding; + if (startedAt) { + onboarding.startedAt = startedAt; + } + + await page.goto('about:blank'); + + await expect(async () => { + await putOnboardingPreferences(page, onboarding); + expect(await getOnboardingPreferences(page)).toEqual(onboarding); + }).toPass({timeout: 15000}); } async function expectOnboardingRoute(page: Page, {returnTo = '/analytics'}: {returnTo?: string} = {}) {