From 4896ca78161c4dfa2ec467d8cbe485c249a02597 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 26 Aug 2026 11:43:03 +0100 Subject: [PATCH 1/4] Added Stripe checkout collection into member custom fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tier could say what its checkout should collect and where each answer belongs, and nothing acted on it. This connects the two ends: the session Ghost creates now asks for what the tier configured, and the completed session's answers are written into the member's fields. What comes back is read as ports and values, so nothing outside the bindings knows a field key, and each value is written in its own transaction — atomic across the parts of an address, but no wider, because a postcode a processor formatted differently must not cost the phone number beside it. None of it may fail the webhook, which is busy creating the member and linking the subscription. The limits a checkout page enforces were measured against the live API rather than read from the reference, which disagreed with it in three of five probes. One of those measurements matters on its own: Stripe will not collect a tax number for a customer it may not rename, so a tier that asked for one could not be bought by a member who already had a Stripe customer. A site that has configured nothing sends a request identical to the one it sent before, and the whole path stays behind the members custom fields flag. ref https://linear.app/ghost/issue/BER-3872 --- .../members/members-api/members-api.js | 5 + .../members-api/services/payments-service.js | 48 +- .../services/checkout/allowed-countries.ts | 267 +++++++++ .../services/checkout/completed-session.ts | 89 +++ .../services/checkout/session-options.ts | 156 +++++ .../webhook/checkout-session-event-service.js | 49 ++ .../core/server/services/stripe/stripe-api.js | 29 + .../server/services/stripe/stripe-service.js | 7 + .../tier-checkout-config/serializers.ts | 15 +- .../admin/tiers-checkout-config.test.ts | 15 + .../create-stripe-checkout-session.test.js | 535 ++++++++++++++++++ .../test/e2e-api/members/webhooks.test.js | 328 +++++++++++ .../services/stripe/allowed-countries.test.ts | 57 ++ 13 files changed, 1596 insertions(+), 4 deletions(-) create mode 100644 ghost/core/core/server/services/stripe/services/checkout/allowed-countries.ts create mode 100644 ghost/core/core/server/services/stripe/services/checkout/completed-session.ts create mode 100644 ghost/core/core/server/services/stripe/services/checkout/session-options.ts create mode 100644 ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts diff --git a/ghost/core/core/server/services/members/members-api/members-api.js b/ghost/core/core/server/services/members/members-api/members-api.js index 8182fc51f9f..7c429d31f33 100644 --- a/ghost/core/core/server/services/members/members-api/members-api.js +++ b/ghost/core/core/server/services/members/members-api/members-api.js @@ -179,6 +179,11 @@ module.exports = function MembersAPI({ offersAPI, stripeAPIService, settingsCache, + // The service wrapper, not the checkout config it builds: tiers and members are + // initialised in the same Promise.all, so reading the property here would capture + // whatever it was before tiers finished — usually undefined. + tiersService, + labsService, }); const memberController = new MemberController({ diff --git a/ghost/core/core/server/services/members/members-api/services/payments-service.js b/ghost/core/core/server/services/members/members-api/services/payments-service.js index 452914b94f0..feef2be26c4 100644 --- a/ghost/core/core/server/services/members/members-api/services/payments-service.js +++ b/ghost/core/core/server/services/members/members-api/services/payments-service.js @@ -14,6 +14,8 @@ class PaymentsService { * @param {import('../../../offers/application/offers-api')} deps.offersAPI * @param {import('../../../stripe/stripe-api')} deps.stripeAPIService * @param {{get(key: string): any}} deps.settingsCache + * @param {{checkout: import('../../../tier-checkout-config').TierCheckoutConfigService}} deps.tiersService + * @param {{isSet(flag: string): boolean}} deps.labsService */ constructor(deps) { /** @private */ @@ -30,6 +32,10 @@ class PaymentsService { this.stripeAPIService = deps.stripeAPIService; /** @private */ this.settingsCache = deps.settingsCache; + /** @private */ + this.tiersService = deps.tiersService; + /** @private */ + this.labsService = deps.labsService; DomainEvents.subscribe(OfferCreatedEvent, async (event) => { await this.getCouponForOffer(event.data.offer.id); }); @@ -114,11 +120,19 @@ class PaymentsService { const price = await this.getPriceForTierCadence(tier, cadence); const data = { - metadata, + // The tier being bought, recorded on the session so the completed event can find + // the configuration that produced its questions. Nothing else carries it: a + // completed session names prices and products, and mapping those back is a + // lookup that can fail where this cannot. + metadata: { ...metadata, ghostTierId: tier.id.toHexString() }, successUrl: successUrl, cancelUrl: cancelUrl, trialDays: trialDays ?? tier.trialDays, coupon: coupon?.id, + // Resolved here rather than cached with the tier, so a field archived a minute + // ago stops being asked on the next checkout. A failure to resolve must not + // stop a member paying, so it costs the questions and nothing else. + checkout: await this.getCheckoutConfigForTier(tier), }; // If we already have a coupon, we don't want to give trial days over it @@ -135,6 +149,38 @@ class PaymentsService { return session.url; } + /** + * What this tier's checkout should ask for beyond the payment, or nothing. + * + * Undefined on every path but the configured one, including the flag being off: the + * session builder adds no parameters for it, so an unconfigured site's request to + * Stripe is exactly the request it made before this existed. + * + * @private + * @param {import('../../../tiers/tier')} tier + */ + async getCheckoutConfigForTier(tier) { + const checkoutConfig = this.tiersService?.checkout; + if (!this.labsService?.isSet('membersCustomFields') || !checkoutConfig) { + return undefined; + } + try { + return await checkoutConfig.resolve(tier.id.toHexString()); + } catch (err) { + // A checkout that asks one fewer question still takes the money; one that fails + // to be created takes none. This is the whole reason it is caught. + logging.error( + { + event: { name: 'stripe_checkout.tier_config.resolve_failed' }, + err, + tierId: tier.id.toHexString(), + }, + 'Failed to resolve what a tier checkout should collect', + ); + return undefined; + } + } + /** * @param {object} params * @param {Member} [params.member] diff --git a/ghost/core/core/server/services/stripe/services/checkout/allowed-countries.ts b/ghost/core/core/server/services/stripe/services/checkout/allowed-countries.ts new file mode 100644 index 00000000000..75646083734 --- /dev/null +++ b/ghost/core/core/server/services/stripe/services/checkout/allowed-countries.ts @@ -0,0 +1,267 @@ +/** + * The countries Stripe Checkout will accept in `shipping_address_collection`. + * + * Not a general list of countries. This is not a question about which places exist, it is + * a question about where this payment processor will ship, and only the processor's answer + * decides whether a session can be created at all. A general ISO list gets it wrong in both + * directions: it omits places Stripe accepts, and it includes the sanctioned countries + * Stripe refuses. + * + * Measured against the live API at the pinned version by `e2e/scripts/probe-stripe-constraints.ts`, + * not read from the SDK's `AllowedCountry` union, because that union is wrong: it omits + * `SD`, which the live API accepts. Every other code the union carries was confirmed + * accepted, so the union is a subset of this list rather than a different one — which is + * what the accompanying test holds it to, so a country Stripe adds cannot pass unnoticed. + * + * A code Stripe rejects fails the whole session create, which is a publisher who cannot + * sell. So this is checked when a publisher chooses a country rather than when a member + * tries to buy. + */ +export const STRIPE_ALLOWED_COUNTRIES = [ + 'AC', + 'AD', + 'AE', + 'AF', + 'AG', + 'AI', + 'AL', + 'AM', + 'AO', + 'AQ', + 'AR', + 'AT', + 'AU', + 'AW', + 'AX', + 'AZ', + 'BA', + 'BB', + 'BD', + 'BE', + 'BF', + 'BG', + 'BH', + 'BI', + 'BJ', + 'BL', + 'BM', + 'BN', + 'BO', + 'BQ', + 'BR', + 'BS', + 'BT', + 'BV', + 'BW', + 'BY', + 'BZ', + 'CA', + 'CD', + 'CF', + 'CG', + 'CH', + 'CI', + 'CK', + 'CL', + 'CM', + 'CN', + 'CO', + 'CR', + 'CV', + 'CW', + 'CY', + 'CZ', + 'DE', + 'DJ', + 'DK', + 'DM', + 'DO', + 'DZ', + 'EC', + 'EE', + 'EG', + 'EH', + 'ER', + 'ES', + 'ET', + 'FI', + 'FJ', + 'FK', + 'FO', + 'FR', + 'GA', + 'GB', + 'GD', + 'GE', + 'GF', + 'GG', + 'GH', + 'GI', + 'GL', + 'GM', + 'GN', + 'GP', + 'GQ', + 'GR', + 'GS', + 'GT', + 'GU', + 'GW', + 'GY', + 'HK', + 'HN', + 'HR', + 'HT', + 'HU', + 'ID', + 'IE', + 'IL', + 'IM', + 'IN', + 'IO', + 'IQ', + 'IS', + 'IT', + 'JE', + 'JM', + 'JO', + 'JP', + 'KE', + 'KG', + 'KH', + 'KI', + 'KM', + 'KN', + 'KR', + 'KW', + 'KY', + 'KZ', + 'LA', + 'LB', + 'LC', + 'LI', + 'LK', + 'LR', + 'LS', + 'LT', + 'LU', + 'LV', + 'LY', + 'MA', + 'MC', + 'MD', + 'ME', + 'MF', + 'MG', + 'MK', + 'ML', + 'MM', + 'MN', + 'MO', + 'MQ', + 'MR', + 'MS', + 'MT', + 'MU', + 'MV', + 'MW', + 'MX', + 'MY', + 'MZ', + 'NA', + 'NC', + 'NE', + 'NG', + 'NI', + 'NL', + 'NO', + 'NP', + 'NR', + 'NU', + 'NZ', + 'OM', + 'PA', + 'PE', + 'PF', + 'PG', + 'PH', + 'PK', + 'PL', + 'PM', + 'PN', + 'PR', + 'PS', + 'PT', + 'PY', + 'QA', + 'RE', + 'RO', + 'RS', + 'RU', + 'RW', + 'SA', + 'SB', + 'SC', + 'SD', + 'SE', + 'SG', + 'SH', + 'SI', + 'SJ', + 'SK', + 'SL', + 'SM', + 'SN', + 'SO', + 'SR', + 'SS', + 'ST', + 'SV', + 'SX', + 'SZ', + 'TA', + 'TC', + 'TD', + 'TF', + 'TG', + 'TH', + 'TJ', + 'TK', + 'TL', + 'TM', + 'TN', + 'TO', + 'TR', + 'TT', + 'TV', + 'TW', + 'TZ', + 'UA', + 'UG', + 'US', + 'UY', + 'UZ', + 'VA', + 'VC', + 'VE', + 'VG', + 'VN', + 'VU', + 'WF', + 'WS', + 'XK', + 'YE', + 'YT', + 'ZA', + 'ZM', + 'ZW', + 'ZZ', +] as const; + +export type StripeAllowedCountry = (typeof STRIPE_ALLOWED_COUNTRIES)[number]; + +const ALLOWED = new Set(STRIPE_ALLOWED_COUNTRIES); + +export function isStripeAllowedCountry(code: string): code is StripeAllowedCountry { + return ALLOWED.has(code); +} diff --git a/ghost/core/core/server/services/stripe/services/checkout/completed-session.ts b/ghost/core/core/server/services/stripe/services/checkout/completed-session.ts new file mode 100644 index 00000000000..ed076fd5eb1 --- /dev/null +++ b/ghost/core/core/server/services/stripe/services/checkout/completed-session.ts @@ -0,0 +1,89 @@ +import { z } from 'zod'; +import { STRIPE_PORTS, type StripePort } from './field-ports'; + +/** + * What Ghost reads off a completed session. + * + * `shipping` is where the address lives at Ghost's pinned Stripe API version; later + * versions moved it to `collected_information.shipping_details`. An Event is an immutable + * snapshot rendered at the account's version, so nothing in a payload would warn us if + * that pin ever changed. + * + * A tax number is deliberately not read: Stripe keeps it against the customer it invoices + * and Ghost never stores one. + */ + +const Collected = z + .string() + .nullish() + .transform((given) => (given && given.trim() !== '' ? given : undefined)); + +export const CompletedSession = z + .object({ + custom_fields: z + .array( + z + .object({ + key: z.string().nullish(), + text: z.object({ value: Collected }).nullish(), + }) + .nullable(), + ) + .nullish(), + shipping: z + .object({ + name: Collected, + address: z.record(z.string(), Collected).nullish(), + }) + .nullish(), + customer_details: z.object({ phone: Collected }).nullish(), + }) + .loose(); +export type CompletedSession = z.input; + +export interface CollectedByPort { + port: string; + value: unknown; +} + +/** Our address type needs at least one part; an empty value would erase what is there. */ +function addressValue( + address: Record | null | undefined, +): Record | undefined { + const value = Object.fromEntries( + Object.entries(address ?? {}).filter( + (entry): entry is [string, string] => entry[1] !== undefined, + ), + ); + return Object.keys(value).length > 0 ? value : undefined; +} + +const PORT_VALUES: Record) => unknown> = { + // Stripe returns the recipient's name alongside the address rather than as part of it, + // while Ghost keeps a name and an address in two separate custom fields. So the name is + // carried back separately here, under its own name, to be routed to its own field. + shipping_name: (session) => session.shipping?.name, + shipping_address: (session) => addressValue(session.shipping?.address), + phone: (session) => session.customer_details?.phone, +}; + +/** + * Everything a completed checkout gives back, as a list of pairs: the name Stripe used for + * a value, and the value itself. Answers to questions and values Stripe collected on its + * own both come back this way, because both are saved by the same route afterwards. + * + * Order matters. The values Stripe collected come last, so that if a question's answer and + * a collected value are both saved into the same custom field, the collected value is the + * one the field ends up holding. + */ +export const collectedByPort = CompletedSession.transform((session): CollectedByPort[] => { + const answers: CollectedByPort[] = (session.custom_fields ?? []) + .filter((field) => Boolean(field?.key && field.text?.value)) + .map((field) => ({ port: field!.key!, value: field!.text!.value })); + + const ports = STRIPE_PORTS.map((port) => ({ port, value: PORT_VALUES[port](session) })).filter( + (entry) => entry.value !== undefined, + ); + + return [...answers, ...ports]; +}); diff --git a/ghost/core/core/server/services/stripe/services/checkout/session-options.ts b/ghost/core/core/server/services/stripe/services/checkout/session-options.ts new file mode 100644 index 00000000000..775fd506906 --- /dev/null +++ b/ghost/core/core/server/services/stripe/services/checkout/session-options.ts @@ -0,0 +1,156 @@ +import logging from '@tryghost/logging'; +import { + MAX_CHECKOUT_CUSTOM_FIELDS, + MAX_CHECKOUT_LABEL_LENGTH, + isCheckoutEligible, + type CheckoutEligibleFieldType, +} from './field-ports'; +import type { ResolvedCheckout, ResolvedQuestion } from '../../../tier-checkout-config'; + +/** + * The Stripe session parameters a tier's checkout configuration asks for. + * + * Two rules hold everything here together. + * + * **A site that has configured nothing sends exactly what it sent before.** Every key below + * is added only when something asked for it, so an unconfigured site's session-create call + * is byte-identical to the one it made before this existed. Automatic tax has already taken + * Stripe checkout down twice from this code path, both times through a parameter + * combination Stripe rejects, and a rejected session create is a publisher who cannot sell. + * + * **Every limit is applied again here, not just at the settings screen.** A configuration + * written when the rules were laxer, or a field renamed longer since, must not be able to + * fail a session create years later. Anything that would be refused is dropped and logged + * instead — a missing question costs one answer, and a rejected session costs the sale. + * + * `customer_update` is never set *here*, because nothing here knows whether the session has + * a customer, and setting it without one is the exact reproduction of the incident that took + * the automatic tax beta down. Collecting a tax id does require it for an existing customer + * — Stripe will not collect one for a customer it may not rename — so that pairing is made + * where the customer is known, alongside the same rule automatic tax already follows. + */ + +export interface StripeCheckoutCollectionOptions { + custom_fields?: Array<{ + key: string; + label: { type: 'custom'; custom: string }; + type: 'text'; + optional: boolean; + }>; + shipping_address_collection?: { allowed_countries: string[] }; + tax_id_collection?: { enabled: true }; + phone_number_collection?: { enabled: true }; +} + +/** Keyed on the eligible types, so this and the configure-time rule cannot drift apart. */ +const QUESTION_TYPES = { + short_text: 'text', +} as const satisfies Record; + +type AskableQuestion = ResolvedQuestion & { type: CheckoutEligibleFieldType }; + +function askable(question: ResolvedQuestion): question is AskableQuestion { + if (!isCheckoutEligible(question.type)) { + logging.warn( + { + event: { name: 'stripe.checkout.question_skipped' }, + customFieldKey: question.key, + fieldType: question.type, + reason: 'unsupported_type', + }, + 'Skipping a Stripe checkout question', + ); + return false; + } + if (question.prompt.length > MAX_CHECKOUT_LABEL_LENGTH) { + logging.warn( + { + event: { name: 'stripe.checkout.question_skipped' }, + customFieldKey: question.key, + promptLength: question.prompt.length, + reason: 'label_too_long', + }, + 'Skipping a Stripe checkout question', + ); + return false; + } + return true; +} + +/** + * Build the collection parameters for a checkout, or nothing at all. + * + * Returns an object with no keys when a tier asks for nothing, so a caller can spread it + * over its session options unconditionally and change nothing. + */ +export function stripeCheckoutCollectionOptions( + checkout: ResolvedCheckout | undefined, +): StripeCheckoutCollectionOptions { + const options: StripeCheckoutCollectionOptions = {}; + if (!checkout) { + return options; + } + + const eligible = checkout.customFields.filter(askable); + const questions = eligible.slice(0, MAX_CHECKOUT_CUSTOM_FIELDS); + if (questions.length < eligible.length) { + logging.warn( + { + event: { name: 'stripe.checkout.questions_trimmed' }, + asked: questions.length, + configured: eligible.length, + }, + 'Some Stripe checkout questions were not asked', + ); + } + + if (questions.length > 0) { + options.custom_fields = questions.map((question) => ({ + // Ghost sends the custom field's own key as the question's identifier, and Stripe + // returns the buyer's answer labelled with that same key. Using it in both places + // means reading an answer is a direct lookup of the field it belongs to, with + // nothing in between that could map it to the wrong one. + key: question.key, + label: { type: 'custom' as const, custom: question.prompt }, + type: QUESTION_TYPES[question.type], + optional: question.optional, + })); + } + + if (checkout.shipping) { + // The country list is what makes this reach Stripe at all. An empty + // `shipping_address_collection` form-encodes to nothing, so a request built that + // way carries no parameter and Stripe accepts it precisely because it was never + // asked to collect anything — which reads as success and collects no addresses. + // + // Defended again here rather than trusted from the settings screen: this is the + // checkout path, and a malformed configuration must cost the collection rather than + // throw inside a session build. + if (checkout.shipping.allowedCountries.length === 0) { + logging.warn( + { + event: { name: 'stripe.checkout.collection_skipped' }, + port: 'shipping_address', + reason: 'no_allowed_countries', + }, + 'Skipping a Stripe checkout collection', + ); + } else { + options.shipping_address_collection = { + allowed_countries: checkout.shipping.allowedCountries, + }; + } + } + + // Unioned with whatever automatic tax asks for. Both want the same thing, so a site + // running the 2024 tax beta keeps collecting and a site that asked for it starts. + if (checkout.taxNumber) { + options.tax_id_collection = { enabled: true }; + } + + if (checkout.phone) { + options.phone_number_collection = { enabled: true }; + } + + return options; +} diff --git a/ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js b/ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js index 7bcbe767975..bcba8cdbcb7 100644 --- a/ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js +++ b/ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js @@ -2,6 +2,7 @@ const _ = require('lodash'); const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); const { canWelcomeEmailReplaceSignupPaidEmail } = require('../../../lib/member-signup-contexts'); +const { collectedByPort } = require('../checkout/completed-session'); /** @typedef {import('../../../lib/member-signup-contexts').SignupContext} SignupContext */ function isStripeMetadataTrue(value) { @@ -422,6 +423,11 @@ module.exports = class CheckoutSessionEventService { } } + // After the subscription work, and deliberately not part of it: a value the member + // gave us for free must never be able to fail the webhook. A throw here would make + // Stripe retry the event and risk doing the payment work twice. + await this.writeCollectedFields(member.id, session); + if (checkoutType !== 'upgrade') { const ghostSignupContext = /** @type {SignupContext | undefined} */ ( session.metadata?.ghostSignupContext @@ -440,4 +446,47 @@ module.exports = class CheckoutSessionEventService { } } } + + /** + * This service knows how to read a completed Stripe session. It does not know, and must + * not know, which custom field any of those values belongs in — that is what a binding + * decides, so no field key appears anywhere in this code. + * + * Nothing here may be fatal: a throw fails the webhook, which makes Stripe retry it and + * risks doing the payment work twice. + * + * @param {string} memberId + * @param {import('stripe').Stripe.Checkout.Session} session + */ + async writeCollectedFields(memberId, session) { + // Stamped at create time. A session predating this feature carries none. Read + // outside the try so a failure below can name the tier whose answers were lost. + const tierId = session.metadata?.ghostTierId; + + try { + if (!this.deps.labsService.isSet('membersCustomFields')) { + return; + } + + if (!tierId) { + return; + } + + await this.deps.customFieldBindings.writeCollected( + memberId, + tierId, + collectedByPort.parse(session), + ); + } catch (err) { + logging.error( + { + event: { name: 'stripe_checkout.collected_fields.write_failed' }, + err, + memberId, + tierId, + }, + 'Failed to store the fields a checkout collected', + ); + } + } }; diff --git a/ghost/core/core/server/services/stripe/stripe-api.js b/ghost/core/core/server/services/stripe/stripe-api.js index e4d18cc3717..4ab992fe31c 100644 --- a/ghost/core/core/server/services/stripe/stripe-api.js +++ b/ghost/core/core/server/services/stripe/stripe-api.js @@ -3,6 +3,7 @@ const debug = require('@tryghost/debug')('stripe'); const ghostConfig = require('../../../shared/config'); const stripe = require('stripe'); const i18n = require('../i18n'); +const { stripeCheckoutCollectionOptions } = require('./services/checkout/session-options'); /* Stripe has the following rate limits: * - For most APIs, 100 read requests per second in live mode, 25 read requests per second in test mode @@ -556,6 +557,8 @@ module.exports = class StripeAPI { * @param {string} options.customerEmail * @param {number} options.trialDays * @param {string} [options.coupon] + * @param {import('../tier-checkout-config').ResolvedCheckout} [options.checkout] What + * the tier's checkout asks for beyond the payment. Absent, or empty, adds nothing. * * @returns {Promise} */ @@ -636,6 +639,32 @@ module.exports = class StripeAPI { hasCustomer: Boolean(customerId), }); + // Applied after automatic tax so the two agree about `tax_id_collection`: both ask + // for the same thing, and a site with either reason to collect gets it. Nothing is + // added for a tier that has configured nothing, so an unconfigured site's request + // is unchanged. + Object.assign(stripeSessionOptions, stripeCheckoutCollectionOptions(options.checkout)); + + // Stripe refuses to collect a tax id for a customer it may not rename: + // + // Tax ID collection requires updating business name on the customer. To enable tax + // ID collection for an existing customer, please set `customer_update[name]`. + // + // Measured against the live API rather than read — `pnpm --filter @tryghost/e2e + // stripe:probe` — because the same probe shows shipping and phone collection need no + // such thing. Every signed-in checkout carries a customer, so without this a tier + // that collects a tax number cannot be bought by an existing member. + // + // Merged rather than assigned: automatic tax sets this too, and whichever ran first + // must not lose its keys. Still never set without a customer, which is the shape + // that took the tax beta down. + if (stripeSessionOptions.tax_id_collection && customerId) { + stripeSessionOptions.customer_update = { + ...stripeSessionOptions.customer_update, + name: 'auto', + }; + } + const session = await this._stripe.checkout.sessions.create(stripeSessionOptions); return session; diff --git a/ghost/core/core/server/services/stripe/stripe-service.js b/ghost/core/core/server/services/stripe/stripe-service.js index ba1f8f37019..94cb5226871 100644 --- a/ghost/core/core/server/services/stripe/stripe-service.js +++ b/ghost/core/core/server/services/stripe/stripe-service.js @@ -10,6 +10,7 @@ const InvoiceEventService = require('./services/webhook/invoice-event-service'); const CheckoutSessionEventService = require('./services/webhook/checkout-session-event-service'); const ChargeRefundedEventService = require('./services/webhook/charge-refunded-event-service'); const memberWelcomeEmailService = require('../member-welcome-emails/service'); +const customFields = require('../members-custom-fields'); /** * @typedef {object} IStripeServiceConfig @@ -133,6 +134,12 @@ module.exports = class StripeService { memberWelcomeEmailService.init(); return memberWelcomeEmailService.api.isMemberWelcomeEmailActive('paid'); }, + labsService: labs, + // A getter because the custom field services are built during boot: reading the + // binding at construction would capture the empty value it had beforehand. + get customFieldBindings() { + return customFields.bindings; + }, }); const chargeRefundedEventService = new ChargeRefundedEventService({ diff --git a/ghost/core/core/server/services/tier-checkout-config/serializers.ts b/ghost/core/core/server/services/tier-checkout-config/serializers.ts index 48106509e72..b958ffc147e 100644 --- a/ghost/core/core/server/services/tier-checkout-config/serializers.ts +++ b/ghost/core/core/server/services/tier-checkout-config/serializers.ts @@ -1,9 +1,15 @@ import { z } from 'zod'; import { MAX_CHECKOUT_CUSTOM_FIELDS } from '../stripe/services/checkout/field-ports'; +import { + STRIPE_ALLOWED_COUNTRIES, + isStripeAllowedCountry, +} from '../stripe/services/checkout/allowed-countries'; import { TierCheckoutConfig } from './models'; -/** Each code costs three characters once comma-joined, against a 2000-character column. */ -const MAX_ALLOWED_COUNTRIES = 600; +// Every country Stripe will take, sent at once, was measured as accepted — so the only +// ceiling is the list itself, and a request naming more than there are countries is naming +// something twice. +const MAX_ALLOWED_COUNTRIES = STRIPE_ALLOWED_COUNTRIES.length; const QuestionInput = z.object({ key: z.string().min(1, { error: 'Every checkout question needs a custom field key.' }), @@ -18,7 +24,10 @@ const CountryCode = z .string() .trim() .regex(/^[A-Za-z]{2}$/, { error: 'Enter a 2-letter country code, like US.' }) - .toUpperCase(); + .toUpperCase() + .refine(isStripeAllowedCountry, { + error: 'Stripe will not ship to that country, so a checkout cannot offer it.', + }); /** * Where a collected value lands is the request's to state. Ghost keeps no convention about diff --git a/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts b/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts index a4f3e99f54b..699b4635dcb 100644 --- a/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts +++ b/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts @@ -184,6 +184,21 @@ describe('Tier Checkout Admin API', function () { }); describe('What the checkout collects for itself', function () { + // A country Stripe will not ship to fails the whole session create, so a publisher who + // saved one would find every checkout for that tier broken and nothing to tell them + // why. Refused at the point they choose it instead. + it('refuses a country the processor will not ship to', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + // The usual slip for GB: two letters, looks like a country, and Stripe rejects it. + const body = await setCheckout(shipping({ allowed_countries: ['UK'] }), 422); + assert.match(body.errors[0].context, /will not ship to that country/); + + // Sanctioned, so a general list of countries has it and Stripe does not. + const sanctioned = await setCheckout(shipping({ allowed_countries: ['KP'] }), 422); + assert.match(sanctioned.errors[0].context, /will not ship to that country/); + }); + // Collecting and choosing where it lands are one statement, because a publisher // makes them as one choice: a checkbox and the field beside it. it('collects a port and binds its destination in one write', async function () { diff --git a/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js b/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js index 1f6eac0935b..aca99996162 100644 --- a/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js +++ b/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js @@ -8,6 +8,7 @@ const { } = require('../../utils/e2e-framework'); const nock = require('nock'); const models = require('../../../core/server/models'); +const membersService = require('../../../core/server/services/members'); const urlServiceUtils = require('../../utils/url-service-utils'); let membersAgent, adminAgent; @@ -717,4 +718,538 @@ describe('Create Stripe Checkout Session', function () { assert.equal(scope.isDone(), true); }); }); + // What a tier's checkout configuration actually puts on the wire. The parameters + // themselves are settled next to the builder; what is proven here is the whole chain — + // a publisher's configuration reaching Stripe through the payment link. + describe("Collecting a tier's checkout fields", function () { + let paidTier; + + function mockStripe(captureSessionBody) { + nock('https://api.stripe.com') + .persist() + .get(/v1\/.*/) + .reply((uri) => { + const [match, resource, id] = uri.match(/\/v1\/(\w+)\/(.+)\/?/) || [null]; + if (match && resource === 'products') { + return [200, { id, active: true }]; + } + if (match && resource === 'prices') { + return [ + 200, + { + id, + active: true, + currency: 'usd', + unit_amount: 500, + recurring: { interval: 'month' }, + }, + ]; + } + // A signed-in checkout looks its member's customer up before creating a + // session, which is the whole difference between it and an anonymous one. + if (match && resource === 'customers') { + return [ + 200, + { id: id.split('?')[0], email: 'member1@test.com', subscriptions: { data: [] } }, + ]; + } + return [500]; + }); + + nock('https://api.stripe.com') + .persist() + .post(/v1\/.*/) + .reply((uri, body) => { + if (uri === '/v1/checkout/sessions') { + captureSessionBody(querystring.parse(body)); + return [200, { id: 'cs_123', url: 'https://site.com' }]; + } + if (uri === '/v1/prices') { + return [ + 200, + { + id: 'price_1', + active: true, + currency: 'usd', + unit_amount: 500, + recurring: { interval: 'month' }, + }, + ]; + } + if (uri === '/v1/customers') { + return [ + 200, + { id: 'cus_signed_in', email: 'member1@test.com', subscriptions: { data: [] } }, + ]; + } + return [500]; + }); + } + + async function startCheckout() { + let sessionBody; + mockStripe((body) => { + sessionBody = body; + }); + + await membersAgent + .post('/api/create-stripe-checkout-session/') + .body({ tierId: paidTier.id, cadence: 'month' }) + .expectStatus(200); + + return sessionBody; + } + + beforeEach(async function () { + // The tests above register persistent interceptors and never clean them up, so + // one of theirs would answer these requests and the body would never be seen. + nock.cleanAll(); + mockManager.mockLabsEnabled('membersCustomFields'); + const { + body: { tiers }, + } = await adminAgent.get('/tiers/?include=monthly_price&yearly_price'); + paidTier = tiers.find((tier) => tier.type === 'paid'); + }); + + afterEach(async function () { + nock.cleanAll(); + await models.Base.knex('products_checkout_fields').del(); + await models.Base.knex('products_checkout_config').del(); + await models.Base.knex('members_custom_field_bindings').del(); + await models.Base.knex('members_custom_fields').del(); + }); + + it('asks Stripe for the questions and the collection a tier configured', async function () { + const { + body: { + members_custom_fields: [question], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'T-shirt size', type: 'short_text' }] }); + const { + body: { + members_custom_fields: [address], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Delivery address', type: 'address' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [ + { + custom_fields: [{ key: question.key }], + shipping: { + collect: true, + allowed_countries: ['GB', 'IE'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: address.key }, + }, + }, + ], + }); + + const sessionBody = await startCheckout(); + + // Form-encoded, so Stripe's nested parameters arrive as bracketed keys. Our own + // field key is what goes out, which is what makes reading the answer a lookup. + assert.equal(sessionBody['custom_fields[0][key]'], 't_shirt_size'); + assert.equal(sessionBody['custom_fields[0][label][custom]'], 'T-shirt size'); + assert.equal(sessionBody['custom_fields[0][type]'], 'text'); + assert.equal(sessionBody['shipping_address_collection[allowed_countries][0]'], 'GB'); + assert.equal(sessionBody['shipping_address_collection[allowed_countries][1]'], 'IE'); + }); + + // The safety property: a site that configured nothing sends what it always sent. + // + // `tax_id_collection` is excluded because automatic tax already sets it on this + // fixture, which is the point — the two have to agree on that parameter rather than + // one of them owning it. `customer_update` is the parameter that took checkout down + // in 2024, and nothing here may be a new way to reach it. + it('asks for nothing when the tier configured nothing', async function () { + const sessionBody = await startCheckout(); + + const collectionKeys = Object.keys(sessionBody).filter( + (key) => + key.startsWith('custom_fields') || + key.startsWith('shipping_address_collection') || + key.startsWith('phone_number_collection') || + key.startsWith('customer_update'), + ); + assert.deepEqual(collectionKeys, []); + }); + + it('asks Stripe for a tax number and a phone number when a tier collects them', async function () { + const { + body: { + members_custom_fields: [phone], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Phone', type: 'short_text' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [ + { + tax_number: { collect: true }, + phone: { collect: true, custom_field_key: phone.key }, + }, + ], + }); + + const sessionBody = await startCheckout(); + + assert.equal(sessionBody['tax_id_collection[enabled]'], 'true'); + assert.equal(sessionBody['phone_number_collection[enabled]'], 'true'); + }); + + // Every limit is applied again at session-build time rather than trusted from the + // settings screen. A configuration written while the rules were laxer, or a field + // renamed longer since, must cost that one question rather than the whole checkout: + // a rejected session create is a publisher who cannot sell. + it('drops a question renamed longer than a checkout will render, and still sells', async function () { + const { + body: { + members_custom_fields: [asked], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'T-shirt size', type: 'short_text' }] }); + const { + body: { + members_custom_fields: [kept], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Nickname', type: 'short_text' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [{ custom_fields: [{ key: asked.key }, { key: kept.key }] }], + }); + + // Renaming a field does not revisit the checkouts that ask for it, which is how + // an unaskable question comes to exist without anyone writing one. + await adminAgent + .put(`/members/custom_fields/${asked.key}/`) + .body({ + members_custom_fields: [ + { + name: `A question far longer than a payment page will ever render ${'x'.repeat(20)}`, + }, + ], + }) + .expectStatus(200); + + const sessionBody = await startCheckout(); + + assert.equal(sessionBody['custom_fields[0][key]'], 'nickname'); + assert.equal(sessionBody['custom_fields[1][key]'], undefined); + }); + + // Archiving is reversible, so the configuration stays and stops being acted on. + // Whether a field is still active is decided by the join that reads it, so these + // pin what that join is for. + it('stops asking a question whose field was archived, and keeps the rest', async function () { + const { + body: { + members_custom_fields: [archived], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'T-shirt size', type: 'short_text' }] }); + const { + body: { + members_custom_fields: [kept], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Nickname', type: 'short_text' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [{ custom_fields: [{ key: archived.key }, { key: kept.key }] }], + }); + + await adminAgent + .put(`/members/custom_fields/${archived.key}/`) + .body({ members_custom_fields: [{ status: 'archived' }] }) + .expectStatus(200); + + const sessionBody = await startCheckout(); + + assert.equal(sessionBody['custom_fields[0][key]'], 'nickname'); + assert.equal(sessionBody['custom_fields[1][key]'], undefined); + }); + + // Each destination drops out on its own. Neither of the two behind the shipping + // toggle is privileged: whichever is still active is why the step is worth asking + // for, and the other simply goes unkept. + it('keeps asking for shipping while either destination is still active', async function () { + const { + body: { + members_custom_fields: [recipient], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Recipient name', type: 'short_text' }] }); + const { + body: { + members_custom_fields: [address], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Delivery address', type: 'address' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: recipient.key }, + address: { custom_field_key: address.key }, + }, + }, + ], + }); + + // The address is the obvious half, so archiving it is the case that would break + // if the rule keyed off it rather than off anything landing. + await adminAgent + .put(`/members/custom_fields/${address.key}/`) + .body({ members_custom_fields: [{ status: 'archived' }] }) + .expectStatus(200); + + const sessionBody = await startCheckout(); + assert.equal(sessionBody['shipping_address_collection[allowed_countries][0]'], 'GB'); + }); + + // Stripe returns the recipient and the address under one parameter, so what makes + // asking worthwhile is that *something* it returns still has somewhere to land. + async function collectShippingThenArchive(archived) { + const { + body: { + members_custom_fields: [address], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Delivery address', type: 'address' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: address.key }, + }, + }, + ], + }); + + for (const key of archived) { + await adminAgent + .put(`/members/custom_fields/${key === 'address' ? address.key : key}/`) + .body({ members_custom_fields: [{ status: 'archived' }] }) + .expectStatus(200); + } + } + + it('goes on collecting while one destination is left', async function () { + // The recipient's name is kept in the field Ghost provisioned when the collection + // was turned on, and that field is still active, so the ask stands and the address + // is what gets thrown away. + await collectShippingThenArchive(['address']); + + const sessionBody = await startCheckout(); + assert.equal(sessionBody['shipping_address_collection[allowed_countries][0]'], 'GB'); + }); + + it('stops collecting once no destination is left', async function () { + await collectShippingThenArchive(['address', 'shipping_name']); + + // Collecting an address to throw it away is worse than not asking for one. + const sessionBody = await startCheckout(); + assert.deepEqual( + Object.keys(sessionBody).filter((key) => key.startsWith('shipping_address_collection')), + [], + ); + }); + + // Every signed-in checkout carries a Stripe customer — a free member upgrading, or + // anyone buying a second time — and that is the combination the rest of these tests + // never reach, because they all check out anonymously. Stripe requires + // `customer_update` alongside an existing customer for automatic tax, which is why + // `_applyAutomaticTaxSessionOptions` sets it only when there is one. If the same + // holds for collection, turning shipping on breaks checkout for exactly the members + // most likely to buy. Whether it does is measured by `pnpm stripe:probe`; what this + // pins is that the path is exercised at all. + it('collects for a member who already has a Stripe customer', async function () { + const { + body: { + members_custom_fields: [address], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Delivery address', type: 'address' }] }); + + await adminAgent.put(`/tiers/${paidTier.id}/checkout_config/`).body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: address.key }, + }, + }, + ], + }); + + let sessionBody; + mockStripe((body) => { + sessionBody = body; + }); + + const member = await models.Member.findOne({ email: 'member1@test.com' }); + const identity = await membersService.api.getMemberIdentityToken(member.get('transient_id')); + + await membersAgent + .post('/api/create-stripe-checkout-session/') + .body({ identity, tierId: paidTier.id, cadence: 'month' }) + .expectStatus(200); + + // The customer is what makes this different from every other collection test. + assert.ok(sessionBody.customer, 'a signed-in checkout carries a customer'); + assert.equal(sessionBody['shipping_address_collection[allowed_countries][0]'], 'GB'); + }); + + // Stripe will not collect a tax id for a customer it may not rename, so a signed-in + // member could not buy a tier that collects a tax number until Ghost sent the pair. + // Every other collection test here checks out anonymously and would miss it. + it('lets a member with a customer buy a tier that collects a tax number', async function () { + await adminAgent + .put(`/tiers/${paidTier.id}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + tax_number: { collect: true }, + }, + ], + }) + .expectStatus(200); + + // Automatic tax asks for the same pairing, and is on by default here, so it would + // satisfy this whatever collection did. Off, the assertion is about collection. + mockManager.mockLabsDisabled('stripeAutomaticTax'); + + let sessionBody; + mockStripe((body) => { + sessionBody = body; + }); + + const member = await models.Member.findOne({ email: 'member1@test.com' }); + const identity = await membersService.api.getMemberIdentityToken(member.get('transient_id')); + + await membersAgent + .post('/api/create-stripe-checkout-session/') + .body({ identity, tierId: paidTier.id, cadence: 'month' }) + .expectStatus(200); + + assert.ok(sessionBody.customer, 'a signed-in checkout carries a customer'); + assert.equal(sessionBody['tax_id_collection[enabled]'], 'true'); + assert.equal(sessionBody['customer_update[name]'], 'auto'); + }); + + // Both automatic tax and collection write `customer_update`, so the second one to run + // must add to it rather than replace it. Assigning would drop the address automatic + // tax needs, and break tax calculation on a site that had it working. + it('keeps what automatic tax asks for when a tier also collects a tax number', async function () { + await adminAgent + .put(`/tiers/${paidTier.id}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + tax_number: { collect: true }, + }, + ], + }) + .expectStatus(200); + + let sessionBody; + mockStripe((body) => { + sessionBody = body; + }); + + const member = await models.Member.findOne({ email: 'member1@test.com' }); + const identity = await membersService.api.getMemberIdentityToken(member.get('transient_id')); + + await membersAgent + .post('/api/create-stripe-checkout-session/') + .body({ identity, tierId: paidTier.id, cadence: 'month' }) + .expectStatus(200); + + assert.equal(sessionBody['customer_update[address]'], 'auto'); + assert.equal(sessionBody['customer_update[name]'], 'auto'); + }); + + // `customer_update` is only valid alongside `customer`, and sending it without one + // is the exact shape that took the automatic tax beta down. Collection asks for it + // only for tax, and only once there is a customer to update. + it('never sends customer_update for a checkout without a customer', async function () { + const { + body: { + members_custom_fields: [address], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Delivery address', type: 'address' }] }); + + await adminAgent + .put(`/tiers/${paidTier.id}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: address.key }, + }, + }, + ], + }) + .expectStatus(200); + + const sessionBody = await startCheckout(); + + assert.deepEqual( + Object.keys(sessionBody).filter((key) => key.startsWith('customer_update')), + [], + ); + }); + + // Turning the flag off has to stop collection without anyone unpicking the + // configuration first. + it('asks for nothing with the flag off, however the tier is configured', async function () { + const { + body: { + members_custom_fields: [question], + }, + } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name: 'T-shirt size', type: 'short_text' }] }); + await adminAgent + .put(`/tiers/${paidTier.id}/checkout_config/`) + .body({ tiers_checkout_config: [{ custom_fields: [{ key: question.key }] }] }); + + mockManager.mockLabsDisabled('membersCustomFields'); + const sessionBody = await startCheckout(); + + assert.deepEqual( + Object.keys(sessionBody).filter((key) => key.startsWith('custom_fields')), + [], + ); + }); + }); }); diff --git a/ghost/core/test/e2e-api/members/webhooks.test.js b/ghost/core/test/e2e-api/members/webhooks.test.js index fdfe919f3ea..ca9b78a7048 100644 --- a/ghost/core/test/e2e-api/members/webhooks.test.js +++ b/ghost/core/test/e2e-api/members/webhooks.test.js @@ -1366,6 +1366,334 @@ describe('Members API', function () { }); }); + // The other half of the collection work: what Stripe collected on the payment page + // landing on the member's own fields. Driven through the real webhook, because the + // whole point is that this happens on the path that already creates the member. + describe('Fields collected on the checkout page', function () { + // Named here rather than inside the one test that adds it, because the cleanup that + // takes it away again has to name the same thing. + const SECOND_TIER_ID = 'ffffffffffffffffffffffff'; + let fieldKeys; + + async function createField(name, type) { + const { body } = await adminAgent + .post('/members/custom_fields/') + .body({ members_custom_fields: [{ name, type }] }); + return body.members_custom_fields[0].key; + } + + async function sendCheckoutWebhook(email, sessionExtras) { + set(customer, { + id: 'cus_123', + name: 'Test Member', + email, + subscriptions: { type: 'list', data: [subscription] }, + }); + + const webhookPayload = JSON.stringify({ + type: 'checkout.session.completed', + data: { + object: { + mode: 'subscription', + customer: customer.id, + subscription: subscription.id, + // The tier the session was created for, which is how the write + // finds the configuration that produced these questions. + metadata: { ghostTierId: (await getPaidProduct()).id }, + ...sessionExtras, + }, + }, + }); + + await membersAgent + .post('/webhooks/stripe/') + .body(webhookPayload) + .header('content-type', 'application/json') + .header( + 'stripe-signature', + stripe.webhooks.generateTestHeaderString({ + payload: webhookPayload, + secret: process.env.WEBHOOK_SECRET, + }), + ); + + const { body } = await adminAgent.get(`/members/?search=${encodeURIComponent(email)}`); + assert.equal(body.members.length, 1, 'The member was not created'); + const { body: read } = await adminAgent.get(`/members/${body.members[0].id}/`); + return read.members[0]; + } + + beforeEach(async function () { + mockManager.mockLabsEnabled('membersCustomFields'); + fieldKeys = { + question: await createField('T-shirt size', 'short_text'), + address: await createField('Delivery address', 'address'), + vat: await createField('VAT number', 'short_text'), + recipient: await createField('Recipient name', 'short_text'), + }; + // A destination is set by configuring a tier to collect into it, which is + // the only way a publisher gets one: the checkbox and the field are one + // choice. The binding it creates is what the webhook resolves against. + const product = await getPaidProduct(); + await adminAgent.put(`/tiers/${product.id}/checkout_config/`).body({ + tiers_checkout_config: [ + { + // The question has to be configured too: an answer lands because the + // tier asked for it, and asking is what creates the binding it lands + // through. An answer to a question a tier never asked is not ours. + custom_fields: [{ key: fieldKeys.question }], + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: fieldKeys.recipient }, + address: { custom_field_key: fieldKeys.address }, + }, + tax_number: { collect: true }, + }, + ], + }); + }); + + afterEach(async function () { + await models.Base.knex('members_custom_field_values').del(); + await models.Base.knex('members_custom_field_bindings').del(); + await models.Base.knex('products_checkout_fields').del(); + await models.Base.knex('products_checkout_config').del(); + await models.Base.knex('members_custom_fields').del(); + // The second tier one test adds is a paid product, and `getPaidProduct` asks for + // whichever paid product comes first. Leaving it behind would decide that answer + // for every test after this one. + await models.Base.knex('products').where('id', SECOND_TIER_ID).del(); + }); + + it('saves the answers and the collected data onto the member', async function () { + const member = await sendCheckoutWebhook('checkout-collected-fields@email.com', { + custom_fields: [{ key: fieldKeys.question, type: 'text', text: { value: 'Large' } }], + shipping: { + name: 'Bex Jones, c/o Acme Ltd', + address: { + line1: '1 High Street', + line2: null, + city: 'London', + state: null, + postal_code: 'E1 6AN', + country: 'GB', + }, + }, + customer_details: { tax_ids: [{ type: 'gb_vat', value: 'GB123456789' }] }, + }); + + assert.equal(member.custom_fields[fieldKeys.question], 'Large'); + // Stripe returns the recipient beside the address and Ghost keeps them + // apart, so each lands in the field the publisher chose for it. + assert.equal(member.custom_fields[fieldKeys.recipient], 'Bex Jones, c/o Acme Ltd'); + // Stripe's address parts are exactly ours, so nothing is transformed. + assert.deepEqual(member.custom_fields[fieldKeys.address], { + line1: '1 High Street', + city: 'London', + postal_code: 'E1 6AN', + country: 'GB', + }); + // Asked for on the page and kept by Stripe against the customer it invoices. + // Ghost never copies one into a publisher's field, so there is nothing here for it. + assert.equal(member.custom_fields[fieldKeys.vat], undefined); + }); + + // Turning collection off has to stop the collecting, and Stripe keeps returning + // the recipient and address on every completed session whatever a tier asked for, + // so "stopped" can only mean the values stop landing on the member. + // + // A second tier still collects, because that is what makes the difference between + // one tier changing its mind and the site doing so — and a publisher running a + // print tier beside a digital one is the ordinary case, not a corner. + it('keeps a phone number a checkout collected', async function () { + const phone = await createField('Contact number', 'short_text'); + const product = await getPaidProduct(); + await adminAgent + .put(`/tiers/${product.id}/checkout_config/`) + .body({ + tiers_checkout_config: [{ phone: { collect: true, custom_field_key: phone } }], + }) + .expectStatus(200); + + const member = await sendCheckoutWebhook('checkout-phone@email.com', { + customer_details: { phone: '+447700900123' }, + }); + + assert.equal(member.custom_fields[phone], '+447700900123'); + }); + + // The member has already paid by the time this runs, so losing an answer must never + // cost the payment work. A throw would fail the webhook, and Stripe would retry the + // event and risk creating the subscription twice. + it('takes the payment even when the session cannot be read at all', async function () { + const member = await sendCheckoutWebhook('checkout-unreadable@email.com', { + // Stripe would never send this. It fails the codec, so the whole read throws + // rather than one value being dropped. + shipping: 'not an object at all', + }); + + assert.ok(member, 'the member was still created'); + assert.deepEqual( + member.custom_fields, + {}, + 'nothing was collected, and nothing else was disturbed', + ); + }); + + it('stops collecting for a tier that has turned it off, while another still does', async function () { + const [existing] = await models.Base.knex('products').where( + 'id', + (await getPaidProduct()).id, + ); + await models.Base.knex('products').insert({ + ...existing, + id: SECOND_TIER_ID, + name: 'Still shipping', + slug: 'still-shipping-tier', + }); + await adminAgent + .put(`/tiers/${SECOND_TIER_ID}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: fieldKeys.recipient }, + address: { custom_field_key: fieldKeys.address }, + }, + }, + ], + }) + .expectStatus(200); + + // The publisher stops collecting on the tier this session is for. + const product = await getPaidProduct(); + await adminAgent.put(`/tiers/${product.id}/checkout_config/`).body({ + tiers_checkout_config: [{ shipping: { collect: false }, tax_number: { collect: false } }], + }); + + const member = await sendCheckoutWebhook('checkout-collection-off@email.com', { + shipping: { + name: 'Bex Jones', + address: { line1: '1 High Street', country: 'GB' }, + }, + customer_details: { tax_ids: [{ type: 'gb_vat', value: 'GB123456789' }] }, + }); + + assert.equal( + member.custom_fields[fieldKeys.recipient], + undefined, + 'no recipient name was kept', + ); + assert.equal(member.custom_fields[fieldKeys.address], undefined, 'no address was kept'); + }); + + // The acceptance criterion this whole thing turns on: a value Stripe collected + // has to be tellable apart from one a person typed, and only the moment of the + // write can record that. + it('records the binding that wrote every value it collected', async function () { + const member = await sendCheckoutWebhook('checkout-collected-source@email.com', { + custom_fields: [{ key: fieldKeys.question, type: 'text', text: { value: 'Large' } }], + shipping: { name: 'Bex Jones', address: { line1: '1 High Street', country: 'GB' } }, + }); + + const written = await models.Base.knex('members_custom_field_values') + .where('member_id', member.id) + .distinct('written_by_type') + .pluck('written_by_type'); + assert.deepEqual(written, ['binding'], 'a checkout writes through a binding'); + + // The id is the point: it resolves back to the tier that asked, what it was + // collected as, and the field it landed in — which is everything worth + // knowing about how a value got here, and more than a name could say. + const resolved = await models.Base.knex('members_custom_field_values') + .join( + 'members_custom_field_bindings', + 'members_custom_field_bindings.id', + 'members_custom_field_values.written_by_id', + ) + .where('members_custom_field_values.member_id', member.id) + .distinct('members_custom_field_bindings.port') + .pluck('port'); + assert.deepEqual( + resolved.sort(), + ['shipping_address', 'shipping_name', fieldKeys.question].sort(), + ); + }); + + // Several writers may land in one field: a binding says where a value goes, and + // nothing says a field may only be written into once. So what matters is not that it + // cannot happen but that it settles the same way every time, rather than on whichever + // value the payload happened to carry first. What the processor collected under its + // own name is written after the answers the member typed, so it is what remains. + it('settles a field two writers share the same way every time', async function () { + const product = await getPaidProduct(); + await adminAgent + .put(`/tiers/${product.id}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + // Asked for as a question and collected into as the recipient's name, so + // two bindings of this tier point at one field. + custom_fields: [{ key: fieldKeys.recipient }], + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: fieldKeys.recipient }, + address: { custom_field_key: fieldKeys.address }, + }, + }, + ], + }) + .expectStatus(200); + + const member = await sendCheckoutWebhook('checkout-collected-shared@email.com', { + custom_fields: [ + { key: fieldKeys.recipient, type: 'text', text: { value: 'Typed by the member' } }, + ], + shipping: { name: 'Collected by Stripe', address: { country: 'GB' } }, + }); + + assert.equal(member.custom_fields[fieldKeys.recipient], 'Collected by Stripe'); + }); + + // A value the member gave us for free must never fail the webhook: a throw makes + // Stripe retry the event and risks doing the payment work twice. + it('still creates the member when a collected value cannot be saved', async function () { + const member = await sendCheckoutWebhook('checkout-collected-invalid@email.com', { + custom_fields: [{ key: fieldKeys.question, type: 'text', text: { value: 'Large' } }], + // Longer than any postcode our address type will take. The recipient's name + // arrives on the same Stripe parameter and routes through its own binding. + shipping: { + name: 'Ada Lovelace', + address: { postal_code: 'x'.repeat(40), country: 'GB' }, + }, + }); + + assert.equal(member.status, 'paid'); + assert.equal( + member.custom_fields[fieldKeys.question], + 'Large', + 'the answer beside it was kept', + ); + assert.equal( + member.custom_fields[fieldKeys.recipient], + 'Ada Lovelace', + 'and so was the other half of what Stripe returned together', + ); + assert.equal(member.custom_fields[fieldKeys.address], undefined); + }); + + it('leaves the member alone when the checkout collected nothing', async function () { + const member = await sendCheckoutWebhook('checkout-collected-nothing@email.com', {}); + + assert.equal(member.status, 'paid'); + assert.deepEqual(member.custom_fields, {}); + }); + }); + it('Will create a member with default newsletter subscriptions', async function () { set(customer, { id: 'cus_123', diff --git a/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts b/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts new file mode 100644 index 00000000000..75d6d63ab90 --- /dev/null +++ b/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts @@ -0,0 +1,57 @@ +import fs from 'fs'; +import path from 'path'; +import { describe, it, assert } from 'vitest'; +import { + STRIPE_ALLOWED_COUNTRIES, + isStripeAllowedCountry, +} from '../../../../../core/server/services/stripe/services/checkout/allowed-countries'; + +/** + * The list was measured against the live API rather than taken from the SDK, because the + * SDK's `AllowedCountry` union is missing `SD`, which the live API accepts. So the two are + * legitimately different and asserting they are equal would fail for the wrong reason. + * + * What does hold is that the union is a subset. Stripe adding a country shows up in the SDK + * when the pin moves, and this fails until someone re-probes and adds it — so a country a + * publisher could ship to cannot stay quietly unavailable. + */ +function allowedCountriesFromSdk(): string[] { + const types = path.join( + __dirname, + '../../../../../node_modules/stripe/types/2020-08-27/Checkout/Sessions.d.ts', + ); + const source = fs.readFileSync(types, 'utf8'); + const start = source.indexOf('type AllowedCountry ='); + assert.notEqual(start, -1, 'the pinned SDK no longer declares an AllowedCountry union'); + + const union = source.slice(start, source.indexOf(';', start)); + return [...new Set(union.match(/'[A-Z]{2}'/g)?.map((code) => code.slice(1, -1)) ?? [])]; +} + +describe('Stripe allowed countries', function () { + it('offers every country the pinned SDK knows about', function () { + const missing = allowedCountriesFromSdk().filter((code) => !isStripeAllowedCountry(code)); + assert.deepEqual(missing, [], 'the SDK knows countries this list does not offer'); + }); + + it('offers the country the SDK forgot, because the live API takes it', function () { + assert.ok(isStripeAllowedCountry('SD')); + assert.equal( + allowedCountriesFromSdk().includes('SD'), + false, + 'the SDK now lists SD, so this list no longer needs to differ from it', + ); + }); + + it('accepts a country Stripe ships to and refuses one it does not', function () { + assert.ok(isStripeAllowedCountry('GB')); + // Kosovo and Stripe's catch-all: absent from a general ISO list, accepted by Stripe. + assert.ok(isStripeAllowedCountry('XK')); + assert.ok(isStripeAllowedCountry('ZZ')); + // The usual slip for GB. Two letters, looks like a country, and Stripe refuses it. + assert.equal(isStripeAllowedCountry('UK'), false); + // Sanctioned, so present in a general country list and refused by Stripe. + assert.equal(isStripeAllowedCountry('KP'), false); + assert.equal(isStripeAllowedCountry('IR'), false); + }); +}); From 5fe2cb30d91e83912a34af5033419a01a456ce02 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 26 Aug 2026 11:44:52 +0100 Subject: [PATCH 2/4] Changed the Stripe fixture check to typecheck the scripts it runs The scripts that capture fixtures from Stripe and probe what it accepts import the request builder out of ghost/core, so that they send what Ghost sends. Nothing typechecked them, and they had already rotted against a change to that builder without anything noticing. The e2e package now declares the dependency it typechecks against, so the build that runs before the fixture check pulls it in, and the check itself runs through nx rather than as steps written into one workflow. A rule about how a package is built belongs in the package rather than in the one place that happens to build it. Rewriting the capture script leaves the checkout session it used to record behind, unread by anything, so it goes too. ref https://linear.app/ghost/issue/BER-3872 --- .github/workflows/ci.yml | 8 +- .../services/stripe/allowed-countries.ts | 249 ++++++++++++++++++ .../services/stripe/fake-stripe-server.ts | 28 ++ .../fixtures/checkout_session.shipping.json | 104 -------- e2e/package.json | 16 +- e2e/scripts/capture-completed-checkout.ts | 44 ++-- e2e/scripts/capture-stripe-fixtures.ts | 92 +++++-- e2e/scripts/probe-stripe-constraints.ts | 111 +++++++- e2e/scripts/provision-stripe-environment.ts | 17 +- e2e/tsconfig.scripts.json | 10 + .../services/stripe/allowed-countries.test.ts | 20 ++ pnpm-lock.yaml | 6 + 12 files changed, 544 insertions(+), 161 deletions(-) create mode 100644 e2e/helpers/services/stripe/allowed-countries.ts delete mode 100644 e2e/helpers/services/stripe/fixtures/checkout_session.shipping.json create mode 100644 e2e/tsconfig.scripts.json diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2ffdc896ed..ef22601d4fc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1227,11 +1227,11 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile - # Asserts the fake Stripe server against responses captured from Stripe, and - # that it refuses the requests Stripe refuses. Needs no Ghost, no Docker and - # no browser, so it does not belong in the e2e matrix that waits on the image. + # Needs no Ghost, no Docker and no browser, so it does not belong in the e2e + # matrix that waits on the image. Run through nx so the target pulls in the + # build it declares. - name: Check Stripe fixtures - run: pnpm --filter @tryghost/e2e test:fixtures + run: pnpm nx run @tryghost/e2e:test:fixtures - uses: tryghost/actions/actions/slack-build@e7a401946f91165a6426290705f501a377ec1533 # main if: failure() && github.event_name == 'push' && github.ref == 'refs/heads/main' diff --git a/e2e/helpers/services/stripe/allowed-countries.ts b/e2e/helpers/services/stripe/allowed-countries.ts new file mode 100644 index 00000000000..42cace6f40e --- /dev/null +++ b/e2e/helpers/services/stripe/allowed-countries.ts @@ -0,0 +1,249 @@ +/** + * The countries Stripe Checkout accepts in `shipping_address_collection`, measured against + * the live API by `scripts/probe-stripe-constraints.ts`. + * + * Kept here rather than imported from Ghost, because this package models Stripe and Ghost + * is what it is modelling: a fake that shared the product's list could never catch the + * product offering a country Stripe refuses. The two are held together by a test in + * `ghost/core` that reads this file, so they cannot drift quietly. + */ +export const STRIPE_ALLOWED_COUNTRIES = [ + 'AC', + 'AD', + 'AE', + 'AF', + 'AG', + 'AI', + 'AL', + 'AM', + 'AO', + 'AQ', + 'AR', + 'AT', + 'AU', + 'AW', + 'AX', + 'AZ', + 'BA', + 'BB', + 'BD', + 'BE', + 'BF', + 'BG', + 'BH', + 'BI', + 'BJ', + 'BL', + 'BM', + 'BN', + 'BO', + 'BQ', + 'BR', + 'BS', + 'BT', + 'BV', + 'BW', + 'BY', + 'BZ', + 'CA', + 'CD', + 'CF', + 'CG', + 'CH', + 'CI', + 'CK', + 'CL', + 'CM', + 'CN', + 'CO', + 'CR', + 'CV', + 'CW', + 'CY', + 'CZ', + 'DE', + 'DJ', + 'DK', + 'DM', + 'DO', + 'DZ', + 'EC', + 'EE', + 'EG', + 'EH', + 'ER', + 'ES', + 'ET', + 'FI', + 'FJ', + 'FK', + 'FO', + 'FR', + 'GA', + 'GB', + 'GD', + 'GE', + 'GF', + 'GG', + 'GH', + 'GI', + 'GL', + 'GM', + 'GN', + 'GP', + 'GQ', + 'GR', + 'GS', + 'GT', + 'GU', + 'GW', + 'GY', + 'HK', + 'HN', + 'HR', + 'HT', + 'HU', + 'ID', + 'IE', + 'IL', + 'IM', + 'IN', + 'IO', + 'IQ', + 'IS', + 'IT', + 'JE', + 'JM', + 'JO', + 'JP', + 'KE', + 'KG', + 'KH', + 'KI', + 'KM', + 'KN', + 'KR', + 'KW', + 'KY', + 'KZ', + 'LA', + 'LB', + 'LC', + 'LI', + 'LK', + 'LR', + 'LS', + 'LT', + 'LU', + 'LV', + 'LY', + 'MA', + 'MC', + 'MD', + 'ME', + 'MF', + 'MG', + 'MK', + 'ML', + 'MM', + 'MN', + 'MO', + 'MQ', + 'MR', + 'MS', + 'MT', + 'MU', + 'MV', + 'MW', + 'MX', + 'MY', + 'MZ', + 'NA', + 'NC', + 'NE', + 'NG', + 'NI', + 'NL', + 'NO', + 'NP', + 'NR', + 'NU', + 'NZ', + 'OM', + 'PA', + 'PE', + 'PF', + 'PG', + 'PH', + 'PK', + 'PL', + 'PM', + 'PN', + 'PR', + 'PS', + 'PT', + 'PY', + 'QA', + 'RE', + 'RO', + 'RS', + 'RU', + 'RW', + 'SA', + 'SB', + 'SC', + 'SD', + 'SE', + 'SG', + 'SH', + 'SI', + 'SJ', + 'SK', + 'SL', + 'SM', + 'SN', + 'SO', + 'SR', + 'SS', + 'ST', + 'SV', + 'SX', + 'SZ', + 'TA', + 'TC', + 'TD', + 'TF', + 'TG', + 'TH', + 'TJ', + 'TK', + 'TL', + 'TM', + 'TN', + 'TO', + 'TR', + 'TT', + 'TV', + 'TW', + 'TZ', + 'UA', + 'UG', + 'US', + 'UY', + 'UZ', + 'VA', + 'VC', + 'VE', + 'VG', + 'VN', + 'VU', + 'WF', + 'WS', + 'XK', + 'YE', + 'YT', + 'ZA', + 'ZM', + 'ZW', + 'ZZ', +] as const; diff --git a/e2e/helpers/services/stripe/fake-stripe-server.ts b/e2e/helpers/services/stripe/fake-stripe-server.ts index 090c04f0b21..d61e72d94b9 100644 --- a/e2e/helpers/services/stripe/fake-stripe-server.ts +++ b/e2e/helpers/services/stripe/fake-stripe-server.ts @@ -22,6 +22,7 @@ import { buildPrice, buildProduct, } from './builders'; +import { STRIPE_ALLOWED_COUNTRIES } from './allowed-countries'; import { renderFakeCheckoutPage, renderFakeDonationCheckoutPage, @@ -31,6 +32,19 @@ import { const MAX_CUSTOM_FIELDS = 3; const MAX_CUSTOM_FIELD_LABEL_LENGTH = 50; +// What this catches is a request reaching Stripe with a country nothing validated — a +// configuration saved before the rule existed, say — which would fail the session create +// and take the sale with it. +const ALLOWED_COUNTRIES = new Set(STRIPE_ALLOWED_COUNTRIES); + +// Stripe names the offending element and then lists every code it will take. +function allowedCountriesMessage(index: number): string { + const codes = [...STRIPE_ALLOWED_COUNTRIES]; + const last = codes[codes.length - 1]; + const listed = `${codes.slice(0, -1).join(', ')}, or ${last}`; + return `Invalid shipping_address_collection[allowed_countries][${index}]: must be one of ${listed}`; +} + export class FakeStripeServer extends FakeServer { private readonly products: Map = new Map(); private readonly prices: Map = new Map(); @@ -738,6 +752,20 @@ export class FakeStripeServer extends FakeServer { // Read against `true` rather than for truthiness: form decoding delivers the flag as // the string `"false"`, which is truthy, and refusing on that would refuse a checkout // that had switched tax collection off. + const rawCountries = (body.shipping_address_collection as { allowed_countries?: unknown }) + ?.allowed_countries; + const countries = Array.isArray(rawCountries) + ? rawCountries + : rawCountries && typeof rawCountries === 'object' + ? Object.values(rawCountries) + : []; + const refused = countries.findIndex( + (code) => typeof code !== 'string' || !ALLOWED_COUNTRIES.has(code), + ); + if (refused !== -1) { + return allowedCountriesMessage(refused); + } + const taxIdFlag = (body.tax_id_collection as { enabled?: unknown })?.enabled; const collectsTaxId = taxIdFlag === true || taxIdFlag === 'true'; const mayRename = (body.customer_update as { name?: unknown })?.name === 'auto'; diff --git a/e2e/helpers/services/stripe/fixtures/checkout_session.shipping.json b/e2e/helpers/services/stripe/fixtures/checkout_session.shipping.json deleted file mode 100644 index 1c33fc334a7..00000000000 --- a/e2e/helpers/services/stripe/fixtures/checkout_session.shipping.json +++ /dev/null @@ -1,104 +0,0 @@ -{ - "id": "cs_test_a1rlcaFrJ6rRmefuR84dmcYIZC8Wba50apdgswVRAL9rQW5tWhcFePiEHu", - "object": "checkout.session", - "adaptive_pricing": { - "enabled": true - }, - "after_expiration": null, - "allow_promotion_codes": null, - "amount_subtotal": 500, - "amount_total": 500, - "automatic_tax": { - "enabled": false, - "liability": null, - "provider": null, - "status": null - }, - "billing_address_collection": null, - "branding_settings": { - "background_color": "#ffffff", - "border_style": "rounded", - "button_color": "#0074d4", - "display_name": "Counterproof Collective", - "font_family": "default", - "icon": null, - "logo": null - }, - "cancel_url": "https://example.com/cancel", - "client_reference_id": null, - "client_secret": null, - "collected_information": null, - "consent": null, - "consent_collection": null, - "created": 1787146482, - "currency": "usd", - "currency_conversion": null, - "custom_fields": [], - "custom_text": { - "after_submit": null, - "shipping_address": null, - "submit": null, - "terms_of_service_acceptance": null - }, - "customer": null, - "customer_account": null, - "customer_creation": "always", - "customer_details": null, - "customer_email": null, - "discounts": [], - "expires_at": 1787232882, - "integration_identifier": null, - "invoice": null, - "invoice_creation": null, - "livemode": false, - "locale": null, - "managed_payments": { - "enabled": false - }, - "metadata": {}, - "mode": "subscription", - "origin_context": null, - "payment_intent": null, - "payment_link": null, - "payment_method_collection": "always", - "payment_method_configuration_details": { - "id": "pmc_1SAoFrCvhsRvBqJ0uYiAIPaH", - "parent": null - }, - "payment_method_options": { - "card": { - "request_three_d_secure": "automatic" - } - }, - "payment_method_types": ["card", "link", "amazon_pay"], - "payment_status": "unpaid", - "permissions": null, - "phone_number_collection": { - "enabled": false - }, - "recovered_from": null, - "saved_payment_method_options": { - "allow_redisplay_filters": ["always"], - "payment_method_remove": "disabled", - "payment_method_save": null - }, - "setup_intent": null, - "shipping": null, - "shipping_address_collection": { - "allowed_countries": ["GB", "US"] - }, - "shipping_options": [], - "shipping_rate": null, - "status": "open", - "submit_type": null, - "subscription": null, - "success_url": "https://example.com/success", - "total_details": { - "amount_discount": 0, - "amount_shipping": 0, - "amount_tax": 0 - }, - "ui_mode": "hosted", - "url": "https://checkout.stripe.com/c/pay/cs_test_redacted", - "wallet_options": null -} diff --git a/e2e/package.json b/e2e/package.json index 39a41e72e0f..a98808a48aa 100644 --- a/e2e/package.json +++ b/e2e/package.json @@ -19,7 +19,7 @@ "infra:down": "bash ./scripts/infra-down.sh", "tinybird:sync": "node ./scripts/sync-tinybird-state.mjs", "stripe:provision": "node --experimental-strip-types ./scripts/provision-stripe-environment.ts", - "stripe:fixtures": "node --experimental-strip-types ./scripts/capture-stripe-fixtures.ts", + "stripe:fixtures": "tsx ./scripts/capture-stripe-fixtures.ts", "stripe:fixtures:checkout": "node --experimental-strip-types ./scripts/capture-completed-checkout.ts", "stripe:probe": "node --experimental-strip-types ./scripts/probe-stripe-constraints.ts", "preflight:build": "bash ./scripts/prepare-ci-e2e-build-mode.sh", @@ -29,13 +29,14 @@ "test:single": "bash ./scripts/run-playwright-host.sh playwright test --project=main -g", "test:debug": "bash ./scripts/run-playwright-host.sh playwright test --project=main --headed --timeout=60000 -g", "test:fixtures": "playwright test --project=fixtures", - "test:types": "tsc --noEmit", + "test:types": "tsc --noEmit && tsc -p tsconfig.scripts.json", "lint": "eslint . --cache" }, "devDependencies": { "@eslint/js": "catalog:", "@faker-js/faker": "catalog:", "@playwright/test": "catalog:", + "@tryghost/custom-field-types": "workspace:*", "@tryghost/debug": "catalog:", "@tryghost/logging": "catalog:", "@tryghost/test-data": "workspace:*", @@ -52,8 +53,19 @@ "express": "4.22.2", "knex": "3.1.0", "stripe": "8.222.0", + "tsx": "catalog:", "typescript": "catalog:", "typescript-eslint": "catalog:", "zod": "catalog:" + }, + "nx": { + "targets": { + "test:fixtures": { + "dependsOn": [ + "build" + ], + "cache": true + } + } } } diff --git a/e2e/scripts/capture-completed-checkout.ts b/e2e/scripts/capture-completed-checkout.ts index 06fdf975fda..f4bc718e5dc 100644 --- a/e2e/scripts/capture-completed-checkout.ts +++ b/e2e/scripts/capture-completed-checkout.ts @@ -1,7 +1,8 @@ import fs from 'node:fs'; import path from 'node:path'; +import { asSessionCreateParams, provision, stripeClient } from './provision-stripe-environment.ts'; import { fileURLToPath } from 'node:url'; -import { provision, stripeClient } from './provision-stripe-environment.ts'; +import type Stripe from 'stripe'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const fixtureDir = path.resolve(__dirname, '../helpers/services/stripe/fixtures'); @@ -51,22 +52,24 @@ async function main(): Promise { process.exit(1); } - const session = await stripe.checkout.sessions.create({ - mode: 'subscription', - line_items: [{ price: monthly.id, quantity: 1 }], - success_url: 'https://example.com/success', - cancel_url: 'https://example.com/cancel', - shipping_address_collection: { allowed_countries: ['GB', 'US'] }, - tax_id_collection: { enabled: true }, - custom_fields: [ - { - key: 'delivery_notes', - label: { type: 'custom', custom: 'Delivery notes' }, - type: 'text', - optional: true, - }, - ], - }); + const session = await stripe.checkout.sessions.create( + asSessionCreateParams({ + mode: 'subscription', + line_items: [{ price: monthly.id, quantity: 1 }], + success_url: 'https://example.com/success', + cancel_url: 'https://example.com/cancel', + shipping_address_collection: { allowed_countries: ['GB', 'US'] }, + tax_id_collection: { enabled: true }, + custom_fields: [ + { + key: 'delivery_notes', + label: { type: 'custom', custom: 'Delivery notes' }, + type: 'text', + optional: true, + }, + ], + }), + ); log(''); log(' Open this and pay:'); @@ -157,7 +160,10 @@ async function main(): Promise { log(''); } -main().catch((error: Error) => { - log(`Capture failed: ${error.message}`); +main().catch((error: unknown) => { + // A rejection need not be an Error, and the stack is what says which of a dozen + // sequential Stripe calls failed. + const detail = error instanceof Error ? (error.stack ?? error.message) : String(error); + process.stderr.write(`Capture failed: ${detail}\n`); process.exit(1); }); diff --git a/e2e/scripts/capture-stripe-fixtures.ts b/e2e/scripts/capture-stripe-fixtures.ts index 3525bd538f8..7d6a14fcff7 100644 --- a/e2e/scripts/capture-stripe-fixtures.ts +++ b/e2e/scripts/capture-stripe-fixtures.ts @@ -1,7 +1,15 @@ import fs from 'node:fs'; import path from 'node:path'; -import { PRICES, provision, stripeClient } from './provision-stripe-environment.ts'; +import { + PRICES, + asSessionCreateParams, + provision, + stripeClient, +} from './provision-stripe-environment.ts'; import { fileURLToPath } from 'node:url'; +// Reached across the workspace deliberately: the point of this capture is that the request +// is the one Ghost builds, so importing the builder is the coupling rather than a leak. +import { stripeCheckoutCollectionOptions } from '../../ghost/core/core/server/services/stripe/services/checkout/session-options.ts'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const fixtureDir = path.resolve(__dirname, '../helpers/services/stripe/fixtures'); @@ -105,33 +113,62 @@ async function main(): Promise { line_items: [{ price: monthly.id, quantity: 1 }], }), ); + // Built by the same function Ghost builds a real session with, rather than by hand. + // That is what makes this capture check the request as well as the response: if the + // builder ever produces something Stripe refuses, this fails here instead of a fixture + // quietly describing a request production never sends. It is also how we learned that + // an empty shipping_address_collection form-encodes to nothing, so Stripe accepts a + // request it was never actually asked to collect an address by. + const collection = stripeCheckoutCollectionOptions({ + customFields: [ + { + key: 'delivery_notes', + label: null, + optional: true, + prompt: 'Delivery notes', + type: 'short_text', + }, + ], + shipping: { + allowedCountries: ['GB', 'US'], + nameCustomFieldKey: 'recipient_name', + addressCustomFieldKey: 'delivery_address', + }, + taxNumber: true, + phone: null, + }); + save( - 'checkout_session.shipping', - await stripe.checkout.sessions.create({ - ...urls, - mode: 'subscription', - line_items: [{ price: monthly.id, quantity: 1 }], - shipping_address_collection: { allowed_countries: ['GB', 'US'] }, - }), + 'checkout_session.collection', + await stripe.checkout.sessions.create( + asSessionCreateParams({ + ...urls, + mode: 'subscription', + line_items: [{ price: monthly.id, quantity: 1 }], + ...collection, + }), + ), ); save( 'checkout_session.donation', - await stripe.checkout.sessions.create({ - ...urls, - mode: 'payment', - submit_type: 'donate', - line_items: [ - { price_data: { currency: 'usd', unit_amount: 1000, product: product.id }, quantity: 1 }, - ], - custom_fields: [ - { - key: 'donation_message', - label: { type: 'custom', custom: 'Add a personal note' }, - type: 'text', - optional: true, - }, - ], - }), + await stripe.checkout.sessions.create( + asSessionCreateParams({ + ...urls, + mode: 'payment', + submit_type: 'donate', + line_items: [ + { price_data: { currency: 'usd', unit_amount: 1000, product: product.id }, quantity: 1 }, + ], + custom_fields: [ + { + key: 'donation_message', + label: { type: 'custom', custom: 'Add a personal note' }, + type: 'text', + optional: true, + }, + ], + }), + ), ); // Without this there is no way to tell how stale the fixtures are, which makes @@ -147,7 +184,10 @@ async function main(): Promise { log('its hosted page, so checkout_session.completed must be captured by hand.'); } -main().catch((error: Error) => { - log(`Capture failed: ${error.message}`); +main().catch((error: unknown) => { + // A rejection need not be an Error, and the stack is what says which of a dozen + // sequential Stripe calls failed. + const detail = error instanceof Error ? (error.stack ?? error.message) : String(error); + process.stderr.write(`Capture failed: ${detail}\n`); process.exit(1); }); diff --git a/e2e/scripts/probe-stripe-constraints.ts b/e2e/scripts/probe-stripe-constraints.ts index 5d0ddb46a01..344982937cd 100644 --- a/e2e/scripts/probe-stripe-constraints.ts +++ b/e2e/scripts/probe-stripe-constraints.ts @@ -1,4 +1,5 @@ -import { provision, stripeClient } from './provision-stripe-environment.ts'; +import { asSessionCreateParams, provision, stripeClient } from './provision-stripe-environment.ts'; +import { readFileSync } from 'node:fs'; /** * Measures the checkout constraints the fake Stripe server enforces. @@ -14,6 +15,19 @@ import { provision, stripeClient } from './provision-stripe-environment.ts'; const API_VERSION = '2020-08-27'; +// Read from the pinned SDK's own union at probe time, so this measures whatever that +// artefact currently claims rather than a copy of it that could already have drifted. +const ALL_UNION_CODES: string[] = (() => { + const types = new URL( + '../../ghost/core/node_modules/stripe/types/2020-08-27/Checkout/Sessions.d.ts', + import.meta.url, + ); + const source = readFileSync(types, 'utf8'); + const start = source.indexOf('type AllowedCountry ='); + const union = source.slice(start, source.indexOf(';', start)); + return [...new Set(union.match(/'[A-Z]{2}'/g)?.map((code) => code.slice(1, -1)) ?? [])]; +})(); + function log(message: string): void { process.stdout.write(`${message}\n`); } @@ -30,7 +44,7 @@ const field = (overrides: Record = {}) => ({ async function probe(name: string, params: Record): Promise { try { - await stripe.checkout.sessions.create(params as Stripe.Checkout.SessionCreateParams); + await stripe.checkout.sessions.create(asSessionCreateParams(params)); log(` ACCEPTED ${name}`); } catch (error) { log(` REJECTED ${name}`); @@ -83,10 +97,99 @@ async function main(): Promise { shipping_address_collection: {}, }); + // Whether the SDK's own `AllowedCountry` union can be trusted as the list Ghost enforces + // when a publisher saves. The union is a published artefact, and published artefacts have + // already disagreed with this API three times above, so it is measured rather than read. + // + // Two directions matter. A code the union carries but Stripe refuses would have Ghost + // accept a setting that breaks every checkout for that tier. A code Stripe accepts but the + // union omits would have Ghost refuse a country a publisher can legitimately ship to. + const country = (code: string) => ({ + ...base, + shipping_address_collection: { allowed_countries: [code] }, + }); + + log('\n -- expected to be accepted --'); + await probe('GB (baseline)', country('GB')); + + log('\n -- in the SDK union, absent from a general ISO country list --'); + for (const code of ['AC', 'BV', 'TA', 'XK', 'ZZ']) { + await probe(`${code}`, country(code)); + } + + log('\n -- expected to be refused --'); + await probe('UK (the usual slip for GB)', country('UK')); + + log('\n -- in a general ISO country list, absent from the SDK union --'); + for (const code of [ + 'AS', + 'CC', + 'CU', + 'CX', + 'FM', + 'HM', + 'IR', + 'KP', + 'MH', + 'MP', + 'NF', + 'PW', + 'SD', + 'SY', + 'UM', + 'VI', + ]) { + await probe(`${code}`, country(code)); + } + + log('\n -- how many countries one session will take --'); + await probe('every country in the union', { + ...base, + shipping_address_collection: { allowed_countries: ALL_UNION_CODES }, + }); + + // Collecting for a member who already has a Stripe customer, which is every signed-in + // checkout: a free member upgrading, or anyone buying a second time. Ghost sends the + // collection parameters and no `customer_update`, because it reads what was collected + // off the completed session rather than off the customer. Stripe may still require the + // pair — it does for automatic tax, which is why `_applyAutomaticTaxSessionOptions` sets + // `customer_update` only when there is a customer. If it does here too, then turning + // shipping on breaks checkout for exactly the members most likely to buy, and every test + // we have would miss it: they all check out anonymously. + const customer = await stripe.customers.create({ email: `probe-${Date.now()}@example.com` }); + const withCustomer = { ...base, customer: customer.id }; + const shipping = { allowed_countries: ['GB'] }; + + log(''); + await probe('shipping_address_collection with customer, no customer_update', { + ...withCustomer, + shipping_address_collection: shipping, + }); + await probe('shipping_address_collection with customer and customer_update', { + ...withCustomer, + shipping_address_collection: shipping, + customer_update: { shipping: 'auto' }, + }); + await probe('tax_id_collection with customer, no customer_update', { + ...withCustomer, + tax_id_collection: { enabled: true }, + }); + await probe('phone_number_collection with customer, no customer_update', { + ...withCustomer, + phone_number_collection: { enabled: true }, + }); + await probe('custom_fields with customer, no customer_update', { + ...withCustomer, + custom_fields: [field()], + }); + log('\nEach REJECTED message is the string the fake server should return.'); } -main().catch((error: Error) => { - log(`Probe failed: ${error.message}`); +main().catch((error: unknown) => { + // A rejection need not be an Error, and the stack is what says which of a dozen + // sequential Stripe calls failed. + const detail = error instanceof Error ? (error.stack ?? error.message) : String(error); + process.stderr.write(`Probe failed: ${detail}\n`); process.exit(1); }); diff --git a/e2e/scripts/provision-stripe-environment.ts b/e2e/scripts/provision-stripe-environment.ts index 800ce388572..6f0b8eef0fc 100644 --- a/e2e/scripts/provision-stripe-environment.ts +++ b/e2e/scripts/provision-stripe-environment.ts @@ -133,8 +133,21 @@ async function main(): Promise { // Only run when invoked directly, so the capture script can import provision(). if (process.argv[1]?.endsWith('provision-stripe-environment.ts')) { - main().catch((error: Error) => { - log(`Provisioning failed: ${error.message}`); + main().catch((error: unknown) => { + // A rejection need not be an Error, and the stack is what says which of a dozen + // sequential Stripe calls failed. + const detail = error instanceof Error ? (error.stack ?? error.message) : String(error); + process.stderr.write(`Provisioning failed: ${detail}\n`); process.exit(1); }); } + +/** + * The pinned Stripe SDK is older than several of the Checkout parameters these scripts + * send, so its types refuse what the API accepts. That gap is the reason the captures and + * the probe exist at all, and it cannot be closed without moving off the pinned version. + * Casting only the parameters keeps the rest of each request typechecked. + */ +export function asSessionCreateParams(params: unknown): Stripe.Checkout.SessionCreateParams { + return params as Stripe.Checkout.SessionCreateParams; +} diff --git a/e2e/tsconfig.scripts.json b/e2e/tsconfig.scripts.json new file mode 100644 index 00000000000..a3b9735cb89 --- /dev/null +++ b/e2e/tsconfig.scripts.json @@ -0,0 +1,10 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "rootDir": "..", + "declaration": false, + "allowImportingTsExtensions": true + }, + "include": ["scripts/**/*.ts", "types.d.ts"] +} diff --git a/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts b/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts index 75d6d63ab90..cf8c8f71f45 100644 --- a/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts +++ b/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts @@ -43,6 +43,26 @@ describe('Stripe allowed countries', function () { ); }); + it('offers exactly what the end-to-end fake Stripe server will accept', function () { + // The harness keeps its own copy on purpose: a fake that shared this list could never + // catch Ghost offering a country Stripe refuses. Read back here because that package + // cannot import this one, and a silent disagreement would make the fake useless. + const harness = path.join( + __dirname, + '../../../../../../../e2e/helpers/services/stripe/allowed-countries.ts', + ); + const codes = [ + ...new Set( + fs + .readFileSync(harness, 'utf8') + .match(/'[A-Z]{2}',/g) + ?.map((code) => code.slice(1, 3)) ?? [], + ), + ].sort(); + + assert.deepEqual(codes, [...STRIPE_ALLOWED_COUNTRIES].sort()); + }); + it('accepts a country Stripe ships to and refuses one it does not', function () { assert.ok(isStripeAllowedCountry('GB')); // Kosovo and Stripe's catch-all: absent from a general ISO list, accepted by Stripe. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7a29a8b1421..1e88aeeccc5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2152,6 +2152,9 @@ importers: '@playwright/test': specifier: 'catalog:' version: 1.61.1 + '@tryghost/custom-field-types': + specifier: workspace:* + version: link:../packages/custom-field-types '@tryghost/debug': specifier: 'catalog:' version: 2.3.9(supports-color@10.2.2) @@ -2200,6 +2203,9 @@ importers: stripe: specifier: 8.222.0 version: 8.222.0 + tsx: + specifier: 'catalog:' + version: 4.23.12 typescript: specifier: 'catalog:' version: '@typescript/typescript6@6.0.2' From d68240ec23447898efb073a9e465003fc65974f8 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 26 Aug 2026 17:03:53 +0100 Subject: [PATCH 3/4] Changed members custom field and import logs to structured events ref https://linear.app/ghost/issue/BER-3872 A log line is only worth writing if an operator can find it again, and these could not be found. Each glued its context into a sentence, so a decode regression dropping a stored value on every member read, an import that rejected half a file, and an export that had slowed down all read as prose rather than as one named event with numbers hanging off it. Two said less than that, handing an error straight to the logger with nothing to say what had been swallowed, which is the whole difficulty with a best-effort write: it is invisible unless the line that gives it up says what it gave up. All of them now carry a dotted event name, a static message, and their context in named fields, so the question can be put to a fleet rather than read out of one server's scrollback. --- .../services/members-custom-fields/actions.ts | 12 ++++++++++- .../members-custom-fields/values-service.ts | 8 +++++++- .../members/import-export/export/exporter.ts | 20 +++++++++++++------ .../members/import-export/import/importer.ts | 20 +++++++++++++++---- .../import-export/import/stripe-utils.js | 7 ++++++- 5 files changed, 54 insertions(+), 13 deletions(-) diff --git a/ghost/core/core/server/services/members-custom-fields/actions.ts b/ghost/core/core/server/services/members-custom-fields/actions.ts index 46bae821599..67e134c45fa 100644 --- a/ghost/core/core/server/services/members-custom-fields/actions.ts +++ b/ghost/core/core/server/services/members-custom-fields/actions.ts @@ -100,6 +100,16 @@ export async function recordCustomFieldAction({ { autoRefresh: false }, ); } catch (err) { - logging.error(err); + logging.error( + { + event: { name: 'members.custom_fields.action_log_failed' }, + err, + verb, + subject, + actorType: context.actor.type, + actorId: context.actor.id, + }, + 'Failed to record a member custom field action', + ); } } diff --git a/ghost/core/core/server/services/members-custom-fields/values-service.ts b/ghost/core/core/server/services/members-custom-fields/values-service.ts index c7721bab412..2e3636b4ad8 100644 --- a/ghost/core/core/server/services/members-custom-fields/values-service.ts +++ b/ghost/core/core/server/services/members-custom-fields/values-service.ts @@ -107,7 +107,13 @@ export class CustomFieldValuesService { leaves.push(DbCustomFieldLeaf.parse(row)); } catch (err) { logging.warn( - `Skipping unreadable custom field value (field '${row.key}', path '${row.path}'): ${err instanceof Error ? err.message : String(err)}`, + { + event: { name: 'members.custom_fields.value_unreadable' }, + err, + customFieldKey: row.key, + path: row.path, + }, + 'Skipping an unreadable custom field value', ); } } diff --git a/ghost/core/core/server/services/members/import-export/export/exporter.ts b/ghost/core/core/server/services/members/import-export/export/exporter.ts index cfa8d26b3cb..2ea596b574d 100644 --- a/ghost/core/core/server/services/members/import-export/export/exporter.ts +++ b/ghost/core/core/server/services/members/import-export/export/exporter.ts @@ -154,7 +154,10 @@ export default class MembersCSVExporter { const hasFilter = options.limit !== 'all' || options.filter || options.search; const ids = hasFilter ? await this._members.findFilteredIds(options) : null; if (ids) { - logging.info(`[MembersExporter] Found ${ids.length} members matching filter criteria`); + logging.info( + { event: { name: 'members-export.filtered' }, matched: ids.length }, + 'Found members matching the export filter', + ); } const reference = await this.fetchReferenceData(); @@ -164,7 +167,10 @@ export default class MembersCSVExporter { membersQuery.whereIn('id', ids); } - logging.info('[MembersExporter] Starting streaming export of members'); + logging.info( + { event: { name: 'members-export.started' } }, + 'Starting streaming export of members', + ); const batchingTransform = this.createBatchingTransform(); const processingTransform = this.createProcessingTransform(reference); @@ -176,9 +182,8 @@ export default class MembersCSVExporter { ); } else { logging.info( - '[MembersExporter] Total time taken for member export: ' + - (Date.now() - start) / 1000 + - 's', + { event: { name: 'members-export.completed' }, durationMs: Date.now() - start }, + 'Members export completed', ); } }); @@ -212,7 +217,10 @@ export default class MembersCSVExporter { }, {}), ); - logging.info('[MembersExporter] Fetched products and labels in ' + (Date.now() - start) + 'ms'); + logging.info( + { event: { name: 'members-export.reference_data_read' }, durationMs: Date.now() - start }, + 'Read the products and labels an export names', + ); const activeCustomFields = await this._customFields.activeDefinitions(); diff --git a/ghost/core/core/server/services/members/import-export/import/importer.ts b/ghost/core/core/server/services/members/import-export/import/importer.ts index e4fa7f97c6e..5c8be8ba1ae 100644 --- a/ghost/core/core/server/services/members/import-export/import/importer.ts +++ b/ghost/core/core/server/services/members/import-export/import/importer.ts @@ -305,7 +305,10 @@ class MembersCSVImporter { const emailRecipient: string = requestUserEmail ?? (await this._email.getDefaultRecipient()); const spooled = await this._spool.write(rows); - logging.info('[Background Job] members-import queued'); + logging.info( + { event: { name: 'members.import.queued' }, rows: rows.length }, + 'Members import queued', + ); this._addJob({ job: () => this.runImportJob(spooled, { labelName, extraLabels, emailRecipient }, verificationTrigger), @@ -326,7 +329,7 @@ class MembersCSVImporter { verificationTrigger: VerificationTrigger, ): Promise { const startedAt = Date.now(); - logging.info('[Background Job] members-import started'); + logging.info({ event: { name: 'members.import.started' } }, 'Members import started'); // Null until the import produces one: parsing and mapping already happened inside // the request, so anything failing from here is ours rather than the file's. let result: ImportResult | null = null; @@ -355,10 +358,19 @@ class MembersCSVImporter { if (result) { logging.info( - `[Background Job] members-import completed in ${Date.now() - startedAt}ms: imported ${result.imported}, ${result.errors.length} row(s) rejected`, + { + event: { name: 'members.import.completed' }, + durationMs: Date.now() - startedAt, + imported: result.imported, + rejected: result.errors.length, + }, + 'Members import completed', ); } else { - logging.info(`[Background Job] members-import failed after ${Date.now() - startedAt}ms`); + logging.info( + { event: { name: 'members.import.failed' }, durationMs: Date.now() - startedAt }, + 'Members import failed', + ); } } diff --git a/ghost/core/core/server/services/members/import-export/import/stripe-utils.js b/ghost/core/core/server/services/members/import-export/import/stripe-utils.js index e661e903606..8529f06b711 100644 --- a/ghost/core/core/server/services/members/import-export/import/stripe-utils.js +++ b/ghost/core/core/server/services/members/import-export/import/stripe-utils.js @@ -175,7 +175,12 @@ module.exports = class MembersCSVImporterStripeUtils { await this.archivePrice(newStripePrice.id); } catch (archiveErr) { logging.warn( - `Failed to archive orphaned Stripe price ${newStripePrice.id} after a failed subscription update: ${archiveErr.message}`, + { + event: { name: 'members.import.orphaned_price_archive_failed' }, + err: archiveErr, + stripePriceId: newStripePrice.id, + }, + 'Failed to archive an orphaned Stripe price after a failed subscription update', ); } throw err; From 0deb391cc0243fd443442e2506ab50432c678c4b Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 26 Aug 2026 17:28:31 +0100 Subject: [PATCH 4/4] Fixed an import reporting its own fault as the row's error A row's custom field values are validated before its transaction opens, so by the time they are written there is nothing left in them that can be refused. A write that fails there is the database saying no to us, not to the publisher, and it was being handled as though it were the publisher's: the driver's message, query text and all, went into the error file they open next to their spreadsheet, and nobody who could act on it was told. The failure still fails the row, because half a member is worse than none, but the publisher now reads a sentence about what did not save and the original goes to the error tracker, where a lock timeout or a constraint is something someone can go and look at. ref https://linear.app/ghost/issue/BER-3872 --- .../members/import-export/import/importer.ts | 12 +++++++++++- .../import-export/import/error-handling.test.ts | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/ghost/core/core/server/services/members/import-export/import/importer.ts b/ghost/core/core/server/services/members/import-export/import/importer.ts index 5c8be8ba1ae..6088da6b9e9 100644 --- a/ghost/core/core/server/services/members/import-export/import/importer.ts +++ b/ghost/core/core/server/services/members/import-export/import/importer.ts @@ -182,6 +182,7 @@ const messages = { giftCannotCombineWithImportTier: 'Cannot specify both gift_id and import_tier.', giftCannotCombineWithComplimentary: 'Cannot specify both gift_id and complimentary_plan.', giftReassignFailed: 'Failed to reassign gift to member.', + customFieldWriteFailed: 'Failed to save the custom field values for this member.', }; // Columns whose presence makes a row slow to import (they reach out to Stripe), so @@ -581,7 +582,16 @@ class MembersCSVImporter { } // On the row's transaction, so the values commit or roll back with the member. - await this._customFields.applyWrite(member.id, customFieldPlan, trx); + try { + await this._customFields.applyWrite(member.id, customFieldPlan, trx); + } catch (writeError) { + // planWrite passed every value before the transaction opened, so a failure + // here is ours and not the row's. Operators get the original, which a driver + // will have written a query into; the publisher gets a sentence instead, in a + // file they open next to a spreadsheet. + this._report(writeError); + throw new errors.DataImportError({ message: tpl(messages.customFieldWriteFailed) }); + } await trx.commit(); imported += 1; diff --git a/ghost/core/test/unit/server/services/members/import-export/import/error-handling.test.ts b/ghost/core/test/unit/server/services/members/import-export/import/error-handling.test.ts index 5b9755fb627..631617ed9f9 100644 --- a/ghost/core/test/unit/server/services/members/import-export/import/error-handling.test.ts +++ b/ghost/core/test/unit/server/services/members/import-export/import/error-handling.test.ts @@ -243,6 +243,23 @@ describe('members import error handling', function () { assert.deepEqual(h.reported, []); }); + // The one row failure whose fault is known: the values were validated before the + // transaction opened, so nothing left in them can be what threw here. + it('keeps a fault of its own out of the error file and sends it to operators', async function () { + const h = harness([row('first@example.com')]); + const fault = new Error('ER_LOCK_WAIT_TIMEOUT: update `members_custom_field_values` set ...'); + h.deps.customFields.applyWrite = async () => { + throw fault; + }; + + await h.run(); + + assert.equal(h.onlyReport(), fault); + const attached = h.onlyEmail().attachments[0].content; + assert.match(attached, /Failed to save the custom field values for this member/); + assert.doesNotMatch(attached, /ER_LOCK_WAIT_TIMEOUT|members_custom_field_values/); + }); + it('reports an import where every row failed as unsuccessful', async function () { const h = harness(); h.deps.members.create = async () => {