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/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..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 @@ -305,7 +306,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 +330,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 +359,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', + ); } } @@ -569,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/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; 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/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 () => { 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..cf8c8f71f45 --- /dev/null +++ b/ghost/core/test/unit/server/services/stripe/allowed-countries.test.ts @@ -0,0 +1,77 @@ +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('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. + 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); + }); +}); 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'