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 { 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} = {}) { 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/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/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 () { 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",