From caefa51ac951cc1c56d0e4db2b67759a8954c058 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Tue, 1 Sep 2026 11:43:26 +0100 Subject: [PATCH 01/36] Fixed the portal date picker tests failing once the real month moved on The calendar opens on the current month, so these tests depended on the wall clock as well as their August 2026 fixtures: they passed all August because the two agreed, then failed everywhere on September 1st. Pinning the test clock inside the fixture month makes the suite deterministic whatever the real date is. Only Date is faked, so timers and rendering behave as before. --- .../test/unit/components/common/date-picker.test.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/apps/portal/test/unit/components/common/date-picker.test.tsx b/apps/portal/test/unit/components/common/date-picker.test.tsx index 51f52f2874c..cf9c7e3e320 100644 --- a/apps/portal/test/unit/components/common/date-picker.test.tsx +++ b/apps/portal/test/unit/components/common/date-picker.test.tsx @@ -53,6 +53,17 @@ const dayButton = ({ getByText }: RenderPickerUtils, day: number) => getByText(String(day), { selector: 'button.gh-portal-datepicker-day-button' }); describe('DatePicker', () => { + // The calendar opens on the current month, so these specs depend on the clock as + // well as their fixtures. Pin it inside the fixture month or they only pass while + // the real month happens to agree. + beforeAll(() => { + vi.useFakeTimers({ toFake: ['Date'], now: new Date('2026-08-15T12:00:00Z') }); + }); + + afterAll(() => { + vi.useRealTimers(); + }); + it('shows the minLabel while the field sits on the minimum', () => { const utils = renderPicker({ minLabel: 'Now' }); From 833f194b9491b496f647e2cb975d1a14aef0f45b Mon Sep 17 00:00:00 2001 From: Sag Date: Tue, 1 Sep 2026 13:18:11 +0200 Subject: [PATCH 02/36] Removed stale dependents from allow_self_signup (#30379) no ref This change should not change behaviour. The calculated `allow_self_signup` setting has only read `members_signup_access` since its formula was simplified in e67e2411f2 (Dec 2024), but `portal_plans` and the Stripe keys were left in the `dependents` list. Dependents only control when the field is recalculated, so edits to any of those settings have been triggering a pointless recalculation that always produced the identical value. --- .../core/server/services/settings/settings-service.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/ghost/core/core/server/services/settings/settings-service.js b/ghost/core/core/server/services/settings/settings-service.js index 4a439a33dea..b6c9929c67d 100644 --- a/ghost/core/core/server/services/settings/settings-service.js +++ b/ghost/core/core/server/services/settings/settings-service.js @@ -200,14 +200,7 @@ module.exports = { type: 'boolean', group: 'members', fn: settingsHelpers.allowSelfSignup.bind(settingsHelpers), - dependents: [ - 'members_signup_access', - 'portal_plans', - 'stripe_secret_key', - 'stripe_publishable_key', - 'stripe_connect_secret_key', - 'stripe_connect_publishable_key', - ], + dependents: ['members_signup_access'], }), ); fields.push( From 0160c7c1ead3a18c33f1788d12b87ab60e187079 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Thu, 27 Aug 2026 12:21:35 +0100 Subject: [PATCH 03/36] Fixed dev container stops skipping Ghost's shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The container's process tree (pnpm → sh → nodemon → sh → node) forwarded no signal to Ghost, so a compose down killed it before any registered cleanup task ran. The entrypoint now stays PID 1 and signals the Ghost process directly, waiting for it to exit. --- compose.dev.yaml | 2 ++ docker/ghost-dev/entrypoint.sh | 61 ++++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/compose.dev.yaml b/compose.dev.yaml index af8d62ee90a..386bd354731 100644 --- a/compose.dev.yaml +++ b/compose.dev.yaml @@ -71,6 +71,8 @@ services: container_name: ghost-dev working_dir: /home/ghost/ghost/core command: ['pnpm', 'dev'] + # The entrypoint drains Ghost before exiting; give that longer than Docker's 10s + stop_grace_period: 45s volumes: # Mount the backend source as whole-directory mounts instead of # enumerating each server-graph workspace package. `pnpm dev` runs in diff --git a/docker/ghost-dev/entrypoint.sh b/docker/ghost-dev/entrypoint.sh index 5cb1b33e353..228ee0c7097 100755 --- a/docker/ghost-dev/entrypoint.sh +++ b/docker/ghost-dev/entrypoint.sh @@ -29,6 +29,63 @@ if [ -f /mnt/shared-config/.env.stripe ]; then fi fi -# Execute the CMD -exec "$@" +# When Docker stops this container it sends a stop signal to the first process, which +# is this script. Ghost runs several processes below it (pnpm, nodemon and their +# shells), and none of them pass the signal on. Without the handling below Ghost is +# killed outright and never runs its shutdown work. So this script stays as the first +# process, sends the stop signal straight to Ghost, waits for it to finish, and only +# then lets the others exit. +GHOST_COMMAND='node --conditions=source --import=tsx index.js' # nodemon.json exec +SHUTDOWN_WAIT_SECONDS=30 +ghost_pid() { + local proc cmdline cwd + for proc in /proc/[0-9]*; do + cmdline=$(tr '\0' ' ' < "$proc/cmdline" 2>/dev/null || true) + [ "${cmdline% }" = "$GHOST_COMMAND" ] || continue + cwd=$(readlink "$proc/cwd" 2>/dev/null || true) + [[ "$cwd" == */ghost/core ]] || continue + echo "${proc#/proc/}" + return + done +} + +# A process that has exited but not yet been collected by its parent still has an +# entry under /proc with state Z. Treat it as exited. +has_exited() { + local state + state=$(awk '{print $3}' "/proc/$1/stat" 2>/dev/null || true) + [ -z "$state" ] || [ "$state" = "Z" ] +} + +wait_for_exit() { + local pid=$1 deadline=$2 + while ! has_exited "$pid" && [ "$SECONDS" -lt "$deadline" ]; do + sleep 0.2 + done + has_exited "$pid" +} + +child="" +shutdown() { + # One time limit for everything below, so it finishes within the stop_grace_period + # that compose.dev.yaml gives this container. + local pid deadline=$((SECONDS + SHUTDOWN_WAIT_SECONDS)) + pid=$(ghost_pid || true) + if [ -n "$pid" ]; then + echo "Stopping Ghost (pid $pid) before the container exits" + kill -TERM "$pid" 2>/dev/null || true + wait_for_exit "$pid" "$deadline" || echo "Ghost did not stop within ${SHUTDOWN_WAIT_SECONDS}s" + fi + if [ -n "$child" ]; then + kill -TERM "$child" 2>/dev/null || true + wait_for_exit "$child" "$deadline" || kill -KILL "$child" 2>/dev/null || true + wait "$child" 2>/dev/null || true + fi + exit 0 +} +trap shutdown TERM INT + +"$@" & +child=$! +wait "$child" From 59ffc26346ed6a582bf027ff0677cff7479a6795 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Thu, 27 Aug 2026 12:21:37 +0100 Subject: [PATCH 04/36] Added a dev mode that receives Stripe webhooks at Ghost's pinned API version `stripe listen` renders every event at the account's default API version rather than the version Ghost pins on the endpoint it registers in production, so a payload seen locally can differ in shape from the one production receives. `pnpm dev:stripe:remote` publishes the dev gateway through Tailscale Funnel and lets Ghost register its own pinned endpoint on boot; the endpoint is marked ephemeral so it is deleted on shutdown, and the dev container entrypoint now forwards SIGTERM to the Ghost process so that shutdown actually runs on container stop. --- compose.dev.stripe-remote.yaml | 7 + docker/stripe/entrypoint.sh | 3 + docker/stripe/with-remote-webhooks.sh | 137 ++++++++++++++++++ docs/contributing/development-setup.md | 32 ++++ e2e/README.md | 5 + ghost/core/core/boot.js | 5 + .../core/server/services/stripe/config.js | 20 ++- .../core/server/services/stripe/service.js | 11 +- .../server/services/stripe/stripe-service.js | 29 ++-- .../server/services/stripe/config.test.js | 85 +++++++++++ package.json | 1 + 11 files changed, 318 insertions(+), 17 deletions(-) create mode 100644 compose.dev.stripe-remote.yaml create mode 100755 docker/stripe/with-remote-webhooks.sh diff --git a/compose.dev.stripe-remote.yaml b/compose.dev.stripe-remote.yaml new file mode 100644 index 00000000000..01aa988138f --- /dev/null +++ b/compose.dev.stripe-remote.yaml @@ -0,0 +1,7 @@ +services: + ghost-dev: + environment: + # Ghost registers its own Stripe webhook endpoint, as it does in production, at the + # tunnel URL started by with-remote-webhooks.sh. The site itself stays on localhost. + stripeWebhookUrl: ${GHOST_STRIPE_WEBHOOK_URL:?set by docker/stripe/with-remote-webhooks.sh; run pnpm dev:stripe:remote} + stripeRemoteWebhooks: 'true' diff --git a/docker/stripe/entrypoint.sh b/docker/stripe/entrypoint.sh index 1055fc49393..29a60fae133 100755 --- a/docker/stripe/entrypoint.sh +++ b/docker/stripe/entrypoint.sh @@ -3,6 +3,9 @@ # Entrypoint script for the Stripe CLI service in compose.yml ## This script fetches the webhook secret from Stripe CLI and writes it to a shared config file ## that the Ghost server can read to verify webhook signatures. +## Events forwarded by `stripe listen` use the Stripe account's default API version, not +## the version Ghost registers with in production, so their shape can differ from what +## production receives. Use `pnpm dev:stripe:remote` when the shape matters. # Note: the stripe CLI container is based on alpine, hence `sh` instead of `bash`. set -eu diff --git a/docker/stripe/with-remote-webhooks.sh b/docker/stripe/with-remote-webhooks.sh new file mode 100755 index 00000000000..4f9e5b85d58 --- /dev/null +++ b/docker/stripe/with-remote-webhooks.sh @@ -0,0 +1,137 @@ +#!/bin/bash + +# Runs the development environment with Ghost receiving Stripe webhooks the way it +# does in production. +# +# In production Ghost gives Stripe a URL to send webhooks to, registered with a fixed +# Stripe API version, so the messages always have the same shape. The usual local +# setup forwards webhooks with Stripe's command line tool instead, which uses the +# account's default API version, so messages can have a shape production never sends. +# +# This script makes Ghost's webhook URL reachable from the internet through Tailscale, +# nothing else on the machine, and tells Ghost to register that URL with Stripe at +# boot. Ghost removes the registration when it shuts down. The site and Admin stay on +# localhost. +# +# Usage: ./docker/stripe/with-remote-webhooks.sh +# Example: ./docker/stripe/with-remote-webhooks.sh pnpm nx run ghost-monorepo:docker:dev + +set -euo pipefail + +FUNNEL_PORT=443 +GATEWAY_PORT=2368 +WEBHOOK_PATH=/members/webhooks/stripe + +fail() { + echo "" + echo "================================================================================" + echo "ERROR: $1" + echo "" + shift + for line in "$@"; do + echo "$line" + done + echo "================================================================================" + echo "" + exit 1 +} + +[ "$#" -gt 0 ] || fail "no command given" \ + "Usage: $0 " \ + "Example: $0 pnpm nx run ghost-monorepo:docker:dev" + +# The macOS app bundle does not put its CLI on PATH. +TAILSCALE=$(command -v tailscale || true) +if [ -z "$TAILSCALE" ] && [ -x /Applications/Tailscale.app/Contents/MacOS/Tailscale ]; then + TAILSCALE=/Applications/Tailscale.app/Contents/MacOS/Tailscale +fi +[ -n "$TAILSCALE" ] || fail "tailscale is not installed" \ + "Install it from https://tailscale.com/download and sign in, then re-run." + +status=$("$TAILSCALE" status --json 2>/dev/null || true) +read -r backend hostname < <(node -e ' + const status = JSON.parse(process.argv[1] || "{}"); + const name = ((status.Self || {}).DNSName || "").replace(/\.$/, ""); + process.stdout.write(`${status.BackendState || "Unknown"} ${name}\n`); +' "$status") + +[ "$backend" = "Running" ] || fail "tailscale is not connected (state: $backend)" \ + "Run 'tailscale up' (or open the Tailscale app and sign in), then re-run." +[ -n "$hostname" ] || fail "this node has no MagicDNS name" \ + "Funnel needs MagicDNS and HTTPS certificates enabled for the tailnet." \ + "See https://tailscale.com/kb/1223/funnel" + +if [ "$FUNNEL_PORT" = "443" ]; then + public_origin="https://${hostname}" +else + public_origin="https://${hostname}:${FUNNEL_PORT}" +fi +export GHOST_STRIPE_WEBHOOK_URL="${public_origin}${WEBHOOK_PATH}/" + +# Something is already published on this port: a funnel left running in the +# background, or another copy of this script. Do not take it over. +if "$TAILSCALE" funnel status --json 2>/dev/null | grep -q "\"${hostname}:${FUNNEL_PORT}\""; then + fail "tailscale funnel is already serving port ${FUNNEL_PORT}" \ + "If nothing else needs it: tailscale funnel --https=${FUNNEL_PORT} off" \ + "If another pnpm dev:stripe:remote is running, stop that first." +fi + +echo "Publishing Ghost's Stripe webhook route at ${GHOST_STRIPE_WEBHOOK_URL} via Tailscale Funnel" +echo "Only that path is reachable from the internet, and only while this command runs." +# Run the funnel as a child process without --bg. Tailscale then keeps the URL public +# only while that process lives, so a crash, a closed terminal or a reboot cannot leave +# it published. Tailscale removes the path from the request before forwarding, so the +# target includes the path again for Ghost to route on. +funnel_err=$(mktemp) +"$TAILSCALE" funnel --https="$FUNNEL_PORT" --set-path "$WEBHOOK_PATH" \ + "http://127.0.0.1:${GATEWAY_PORT}${WEBHOOK_PATH}" >/dev/null 2>"$funnel_err" & +funnel_pid=$! + +stop_funnel() { + kill "$funnel_pid" 2>/dev/null || true + wait "$funnel_pid" 2>/dev/null || true + rm -f "$funnel_err" +} +# Bash skips the EXIT trap when a signal kills it, so turn signals into exits. +trap stop_funnel EXIT +trap 'exit 130' INT +trap 'exit 143' TERM HUP + +funnel_ready=false +for _ in $(seq 1 20); do + if "$TAILSCALE" funnel status --json 2>/dev/null | grep -q "\"${hostname}:${FUNNEL_PORT}\""; then + funnel_ready=true + break + fi + kill -0 "$funnel_pid" 2>/dev/null || break + sleep 0.5 +done +if [ "$funnel_ready" != true ]; then + grep -v 'client version' "$funnel_err" >&2 || true + fail "tailscale funnel could not be started" \ + "Funnel must be enabled for your tailnet and this node (Tailscale 1.52 or newer)." \ + "See https://tailscale.com/kb/1223/funnel" +fi + +# The `stripe` compose profile starts Stripe's command line forwarder. With it running, +# Ghost would use the forwarder instead of registering its own URL, and every event +# would also arrive a second time in the other shape. +profiles="${COMPOSE_PROFILES:-}" +if [ -z "$profiles" ] && [ -f .env ]; then + profiles=$(grep -E '^COMPOSE_PROFILES=' .env | tail -n1 | cut -d= -f2- | sed -e 's/[[:space:]]*#.*$//' -e "s/^['\"]//" -e "s/['\"]$//" || true) +fi +if [[ ",${profiles}," == *",stripe,"* ]]; then + echo "Dropping the 'stripe' compose profile: remote webhooks replace stripe listen." + profiles=$(echo "$profiles" | tr ',' '\n' | grep -vx 'stripe' | paste -sd, - || true) +fi +export COMPOSE_PROFILES="$profiles" + +export DEV_COMPOSE_FILES="${DEV_COMPOSE_FILES:-} -f compose.dev.stripe-remote.yaml" + +echo "Ghost registers its webhook endpoint at boot once Stripe is connected in Ghost Admin (Settings > Tiers)." +echo "Open the site and Admin on http://localhost:${GATEWAY_PORT} as usual." +echo "Watch the ghost-dev logs: it warns if Stripe is not connected." + +# The wrapped command stops the containers before it returns, and Ghost removes its +# Stripe registration during that stop. The funnel is closed after that, on exit. +"$@" diff --git a/docs/contributing/development-setup.md b/docs/contributing/development-setup.md index 2380cbfbb6f..9ec17094b73 100644 --- a/docs/contributing/development-setup.md +++ b/docs/contributing/development-setup.md @@ -114,6 +114,7 @@ environment and adds the listed tooling: | `pnpm dev:analytics:local` | Tinybird-backed analytics with your locally running instance of the Traffic Analytics service | | `pnpm dev:storage` | S3-compatible storage through MinIO on ports `9000` and `9001` | | `pnpm dev:stripe` | Stripe webhooks; requires `STRIPE_SECRET_KEY` in the environment or a local `.env` file | +| `pnpm dev:stripe:remote` | Stripe webhooks exactly as production receives them; requires Tailscale, see below | | `pnpm dev:full` | Public app watchers plus analytics, storage, and Stripe | Copy [`.env.example`](../../.env.example) to `.env` only when you need an @@ -123,6 +124,37 @@ To open Ghost on a phone or another computer, or to exercise HTTPS, subdirectory, and separate-Admin URL behaviour, see [Testing development URLs and devices](testing-development-urls.md). +### Stripe webhooks + +`pnpm dev:stripe` forwards events with `stripe listen`. The CLI renders every +event at your Stripe account's default API version, which cannot be pinned, so +an event can carry a different shape from the one Ghost's production endpoint +receives. Ghost pins that endpoint to its own API version when it creates it. +Production keeps a persistent endpoint; a normal development environment never +creates one. + +`pnpm dev:stripe:remote` runs the production path instead. It publishes Ghost's +webhook route, and nothing else, through +[Tailscale Funnel](https://tailscale.com/kb/1223/funnel), and Ghost registers a +pinned webhook endpoint at that address once Stripe is connected in Admin, then +deletes it on shutdown. +The site and Admin stay on `localhost`, so hot reload and the rest of the +development environment work as usual. Use it when the shape of a webhook +payload matters, for example when reading new fields from a checkout session. +Ghost logs an error whenever an event arrives rendered at a different API +version from the one it pins, in any environment. + +The webhook route is reachable from the internet while the command runs; every +request to it must carry a valid Stripe signature. The tunnel is a child +process of the command and ends with it, including on Ctrl-C. Only a forced +kill of the command can leave the tunnel running, and even then it does not +survive a restart of Tailscale or the machine. + +Funnel needs Tailscale 1.52 or newer with MagicDNS, HTTPS certificates and +Funnel enabled for your tailnet and node. The command reports when Tailscale is +missing, not signed in, or has no MagicDNS name; for the other requirements it +shows Tailscale's own error. + ## Data and email After creating the local owner account, populate a development site with stable diff --git a/e2e/README.md b/e2e/README.md index b9d5daf5a4e..a9a13197233 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -363,6 +363,11 @@ renderings: at Stripe's current default the shipping address moves to `collected_information.shipping_details`, which Ghost never sees. Ghost reads only `event.type` and `event.data.object`, so the envelope carries nothing worth pinning. +The same difference applies to `stripe listen`, which `pnpm dev:stripe` uses: it renders +events at the account default too. To see the payloads production receives, run +`pnpm dev:stripe:remote`, which lets Ghost register its own pinned endpoint (see +[Development setup](../docs/contributing/development-setup.md#stripe-webhooks)). + ## Resolving issues ### Test Failures diff --git a/ghost/core/core/boot.js b/ghost/core/core/boot.js index a5f753104a5..eec2dcc815e 100644 --- a/ghost/core/core/boot.js +++ b/ghost/core/core/boot.js @@ -389,6 +389,11 @@ async function initServices({ ghostServer, config, prometheusClient, jobsService }); const giftDeliveryService = giftService.deliveryService; assert(giftDeliveryService, 'Gift delivery service should be initialized'); + if (ghostServer) { + ghostServer.registerCleanupTask(async () => { + await stripe.shutdown(); + }, 'Stripe'); + } await Promise.all([ identityTokens.init(), diff --git a/ghost/core/core/server/services/stripe/config.js b/ghost/core/core/server/services/stripe/config.js index 82c0df595c8..72d4f9c5565 100644 --- a/ghost/core/core/server/services/stripe/config.js +++ b/ghost/core/core/server/services/stripe/config.js @@ -62,16 +62,19 @@ module.exports = { } const env = config.get('env'); - let webhookSecret = process.env.WEBHOOK_SECRET; + const remoteWebhooks = env !== 'production' && config.get('stripeRemoteWebhooks') === true; + let webhookSecret = remoteWebhooks ? undefined : process.env.WEBHOOK_SECRET; - if (env !== 'production') { - if (!webhookSecret) { - webhookSecret = 'DEFAULT_WEBHOOK_SECRET'; - logging.warn(tpl(messages.remoteWebhooksInDevelopment)); - } + if (env !== 'production' && !remoteWebhooks && !webhookSecret) { + webhookSecret = 'DEFAULT_WEBHOOK_SECRET'; + logging.warn(tpl(messages.remoteWebhooksInDevelopment)); } - const webhookHandlerUrl = new URL('members/webhooks/stripe/', urlUtils.getSiteUrl()); + // Development can tunnel the webhook URL alone while the site stays on localhost. + const webhookHandlerUrl = new URL( + config.get('stripeWebhookUrl') || 'members/webhooks/stripe/', + urlUtils.getSiteUrl(), + ); const webhookCustomerIgnoreList = parseIgnoreCustomerList( config.get('stripeWebhookCustomerIgnoreList'), ); @@ -88,6 +91,9 @@ module.exports = { }, webhookSecret: webhookSecret, webhookHandlerUrl: webhookHandlerUrl.href, + // A development registration points at a tunnel that closes with the process, + // so it is removed on shutdown rather than kept for the next boot. + ephemeralWebhook: remoteWebhooks, webhookCustomerIgnoreList, siteUrl, }; diff --git a/ghost/core/core/server/services/stripe/service.js b/ghost/core/core/server/services/stripe/service.js index 11b77afbd62..6c5b78f608f 100644 --- a/ghost/core/core/server/services/stripe/service.js +++ b/ghost/core/core/server/services/stripe/service.js @@ -14,6 +14,12 @@ const giftService = require('../gifts'); const staffService = require('../staff'); const labs = require('../../../shared/labs'); const settingsCache = require('../../../shared/settings-cache'); +const tpl = require('@tryghost/tpl'); + +const messages = { + remoteWebhooksWithoutStripe: + 'stripeRemoteWebhooks is set but Stripe is not connected, so no webhook endpoint was registered. Connect Stripe in Ghost Admin under Settings, then restart.', +}; async function configureApi() { const cfg = getConfig({ settingsHelpers, config, urlUtils }); @@ -84,7 +90,10 @@ function stripeSettingsChanged(model) { module.exports.init = async function init() { try { - await configureApi(); + const configured = await configureApi(); + if (!configured && config.get('stripeRemoteWebhooks') === true) { + logging.warn(tpl(messages.remoteWebhooksWithoutStripe)); + } } catch (err) { logging.error(err); } diff --git a/ghost/core/core/server/services/stripe/stripe-service.js b/ghost/core/core/server/services/stripe/stripe-service.js index 66534ad0a77..2ac721c6b23 100644 --- a/ghost/core/core/server/services/stripe/stripe-service.js +++ b/ghost/core/core/server/services/stripe/stripe-service.js @@ -25,6 +25,7 @@ const customFields = require('../members-custom-fields'); * @prop {boolean} testEnv Whether this is a test environment * @prop {string} webhookSecret The Stripe webhook secret * @prop {string} webhookHandlerUrl The URL to handle Stripe webhooks + * @prop {boolean} [ephemeralWebhook] Whether the webhook endpoint is deleted on shutdown * @prop {string[]} webhookCustomerIgnoreList List of customer IDs for customer.subscription.updated webhook bypass * @prop {string} siteUrl The site URL for billing portal return URL */ @@ -161,6 +162,8 @@ module.exports = class StripeService { this.migrations = migrations; this.webhookController = webhookController; this.billingPortalManager = billingPortalManager; + /** @private */ + this.ephemeralWebhook = false; } async connect() { @@ -212,23 +215,31 @@ module.exports = class StripeService { webhookSecret: config.webhookSecret, webhookHandlerUrl: config.webhookHandlerUrl, }); + this.ephemeralWebhook = config.ephemeralWebhook === true; this.billingPortalManager.configure({ siteUrl: config.siteUrl, }); - // webhookManager.start() already self-guards: configure() above puts it in - // 'local' mode whenever a webhookSecret is set, which is always true outside - // production (config.js defaults it to DEFAULT_WEBHOOK_SECRET), so start() - // returns immediately without touching Stripe. Only billingPortalManager - // needs an explicit test-env skip — in the test env there is no real Stripe - // to register against, and the mock returns 500 for billing_portal/ - // configurations, so this network-registration call only error-logs on - // every boot. Tests never need a registered portal configuration. Skip it - // under test; prod and dev register exactly as before. + // webhookManager.start() registers a webhook URL with Stripe only when no webhook + // secret was supplied. Outside production config.js supplies a placeholder secret + // unless stripeRemoteWebhooks is set, so start() normally returns without touching + // Stripe. billingPortalManager has no such guard: in the test environment the mock + // Stripe answers its registration call with a 500 on every boot, and tests never + // need a registered portal, so skip it under test only. await this.webhookManager.start(); if (!config.testEnv) { await this.billingPortalManager.start(); } } + + /** + * Removes the webhook registration from Stripe, but only when it was marked ephemeral. + * A production registration is kept and reused on the next boot. + */ + async shutdown() { + if (this.ephemeralWebhook) { + await this.webhookManager.stop(); + } + } }; diff --git a/ghost/core/test/unit/server/services/stripe/config.test.js b/ghost/core/test/unit/server/services/stripe/config.test.js index 21bc42af4d4..6f7068f208a 100644 --- a/ghost/core/test/unit/server/services/stripe/config.test.js +++ b/ghost/core/test/unit/server/services/stripe/config.test.js @@ -2,6 +2,7 @@ const assert = require('node:assert/strict'); const { assertExists } = require('../../../../utils/assertions'); const sinon = require('sinon'); const UrlUtils = require('@tryghost/url-utils'); +const logging = require('@tryghost/logging'); const configUtils = require('../../../../utils/config-utils'); @@ -39,6 +40,7 @@ describe('Stripe - config', function () { afterEach(async function () { configUtils.set(ignoreCustomerConfigKey, null); + configUtils.set('stripeWebhookUrl', null); await configUtils.restore(); }); @@ -79,6 +81,89 @@ describe('Stripe - config', function () { assertExists(config.billingPortalReturnUrl); }); + describe('webhook mode', function () { + let webhookSecretEnv; + + beforeEach(function () { + webhookSecretEnv = process.env.WEBHOOK_SECRET; + delete process.env.WEBHOOK_SECRET; + sinon.stub(logging, 'warn'); + }); + + afterEach(function () { + sinon.restore(); + if (webhookSecretEnv === undefined) { + delete process.env.WEBHOOK_SECRET; + } else { + process.env.WEBHOOK_SECRET = webhookSecretEnv; + } + }); + + function getWebhookConfig() { + return getConfig({ + settingsHelpers: createSettingsHelpersMock(), + config: configUtils.config, + urlUtils: createUrlUtilsMock(), + }); + } + + it('Falls back to a placeholder secret outside production', function () { + configUtils.set({ env: 'development' }); + + const config = getWebhookConfig(); + + assert.equal(config.webhookSecret, 'DEFAULT_WEBHOOK_SECRET'); + assert.equal(config.ephemeralWebhook, false); + sinon.assert.calledOnce(logging.warn); + }); + + it('Uses the WEBHOOK_SECRET environment variable outside production', function () { + configUtils.set({ env: 'development' }); + process.env.WEBHOOK_SECRET = 'whsec_from_stripe_listen'; + + const config = getWebhookConfig(); + + assert.equal(config.webhookSecret, 'whsec_from_stripe_listen'); + assert.equal(config.ephemeralWebhook, false); + }); + + it('Registers an ephemeral remote webhook outside production when opted in', function () { + configUtils.set({ env: 'development', stripeRemoteWebhooks: true }); + process.env.WEBHOOK_SECRET = 'whsec_from_stripe_listen'; + + const config = getWebhookConfig(); + + assert.equal(config.webhookSecret, undefined); + assert.equal(config.ephemeralWebhook, true); + sinon.assert.notCalled(logging.warn); + }); + + it('Never treats a production webhook as ephemeral', function () { + configUtils.set({ env: 'production', stripeRemoteWebhooks: true }); + + const config = getWebhookConfig(); + + assert.equal(config.webhookSecret, undefined); + assert.equal(config.ephemeralWebhook, false); + }); + }); + + it('Lets config point the webhook handler at another origin', function () { + configUtils.set({ + url: 'http://site.com/subdir', + stripeWebhookUrl: 'https://tunnel.example/members/webhooks/stripe/', + }); + + const config = getConfig({ + settingsHelpers: createSettingsHelpersMock(), + config: configUtils.config, + urlUtils: createUrlUtilsMock(), + }); + + assert.equal(config.webhookHandlerUrl, 'https://tunnel.example/members/webhooks/stripe/'); + assert.equal(config.siteUrl, 'http://site.com/subdir/'); + }); + it('Parses Stripe webhook customer ignore list from config', function () { configUtils.set(ignoreCustomerConfigKey, ['cust_123', ' cust_456 ']); const settingsHelpers = createSettingsHelpersMock(); diff --git a/package.json b/package.json index 4c5dcf5f9a3..54ffcc72640 100644 --- a/package.json +++ b/package.json @@ -22,6 +22,7 @@ "dev:analytics:local": "ANALYTICS_PROXY_TARGET=traffic-analytics-local:3000 DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml' pnpm nx run ghost-monorepo:docker:dev", "dev:storage": "DEV_COMPOSE_FILES='-f compose.dev.storage.yaml' pnpm nx run ghost-monorepo:docker:dev", "dev:stripe": "./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev", + "dev:stripe:remote": "./docker/stripe/with-remote-webhooks.sh pnpm nx run ghost-monorepo:docker:dev", "dev:all": "DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml -f compose.dev.storage.yaml' ./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev", "dev:daemon": "NX_DAEMON=true NX_TUI=false NX_DEFAULT_OUTPUT_STYLE=stream pnpm nx run ghost-monorepo:docker:dev", "fix": "pnpm store prune && rimraf -g '**/node_modules' && pnpm install && pnpm nx reset", From 6e2244f27f2e469b6aa8384405175f5f8ffd7237 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Thu, 27 Aug 2026 14:14:05 +0100 Subject: [PATCH 05/36] Added an error log for Stripe webhooks rendered at an unexpected API version Stripe renders each event at the API version of the endpoint that received it, and Ghost reads fields from where its pinned version puts them, so an event at another version can be misread without any failure. Ghost now compares the version an event declares with the one it pins and logs an error naming both, while still handling the event so a mismatch never causes Stripe to retry indefinitely. --- .../core/server/services/stripe/stripe-api.js | 2 ++ .../services/stripe/webhook-controller.js | 10 ++++++ .../stripe/webhook-controller.test.js | 33 +++++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/ghost/core/core/server/services/stripe/stripe-api.js b/ghost/core/core/server/services/stripe/stripe-api.js index 4ab992fe31c..4d03d003b41 100644 --- a/ghost/core/core/server/services/stripe/stripe-api.js +++ b/ghost/core/core/server/services/stripe/stripe-api.js @@ -62,6 +62,8 @@ const MANAGED_PAYMENTS_DISABLED = { enabled: false }; */ module.exports = class StripeAPI { + static API_VERSION = STRIPE_API_VERSION; + /** * StripeAPI * @param {object} deps diff --git a/ghost/core/core/server/services/stripe/webhook-controller.js b/ghost/core/core/server/services/stripe/webhook-controller.js index 6f3eda573aa..68e0a94ab01 100644 --- a/ghost/core/core/server/services/stripe/webhook-controller.js +++ b/ghost/core/core/server/services/stripe/webhook-controller.js @@ -1,4 +1,5 @@ const logging = require('@tryghost/logging'); +const StripeAPI = require('./stripe-api'); module.exports = class WebhookController { /** @@ -89,6 +90,15 @@ module.exports = class WebhookController { return res.end(); } + // Stripe shapes each event for the API version of the URL it was sent to. A different + // version means this event did not come through the URL Ghost registered, and fields + // may not be where Ghost reads them. + if (event.api_version !== StripeAPI.API_VERSION) { + logging.error( + `Webhook ${event.type} was rendered at Stripe API version ${event.api_version}, Ghost expects ${StripeAPI.API_VERSION}`, + ); + } + const customerId = this.getEventCustomerId(event); if (this.shouldIgnoreEvent(event, customerId)) { logging.info( diff --git a/ghost/core/test/unit/server/services/stripe/webhook-controller.test.js b/ghost/core/test/unit/server/services/stripe/webhook-controller.test.js index 359836f8932..1385a45492e 100644 --- a/ghost/core/test/unit/server/services/stripe/webhook-controller.test.js +++ b/ghost/core/test/unit/server/services/stripe/webhook-controller.test.js @@ -1,4 +1,5 @@ const sinon = require('sinon'); +const logging = require('@tryghost/logging'); const WebhookController = require('../../../../../core/server/services/stripe/webhook-controller'); describe('WebhookController', function () { @@ -31,6 +32,38 @@ describe('WebhookController', function () { }; }); + afterEach(function () { + sinon.restore(); + }); + + it('logs an error for an event rendered at a different API version and still handles it', async function () { + sinon.stub(logging, 'error'); + deps.webhookManager.parseWebhook.returns({ + type: 'charge.refunded', + api_version: '2025-08-27.basil', + data: { object: { id: 'ch_1' } }, + }); + + await controller.handle(req, res); + + sinon.assert.calledWithMatch(logging.error, /2025-08-27\.basil.*expects 2020-08-27/); + sinon.assert.calledOnce(deps.chargeRefundedEventService.handleEvent); + sinon.assert.calledWith(res.writeHead, 200); + }); + + it('does not log a version error for an event at the pinned API version', async function () { + sinon.stub(logging, 'error'); + deps.webhookManager.parseWebhook.returns({ + type: 'charge.refunded', + api_version: '2020-08-27', + data: { object: { id: 'ch_1' } }, + }); + + await controller.handle(req, res); + + sinon.assert.notCalled(logging.error); + }); + it('should return 400 if request body or signature is missing', async function () { req.body = null; await controller.handle(req, res); From b2b7afcd744d99bdf2568fd4c11e9eaa14c28aa1 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Mon, 31 Aug 2026 23:03:35 +0100 Subject: [PATCH 06/36] Changed custom field filtering to derive a field's operators from its kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pill kept its own hard-coded operator list — the text vocabulary — whatever the field held, so the first non-text field would have offered comparisons its codec cannot write and rewritten operators it can. Nothing about that list was the pill's to decide: the registry owns what each kind of value's comparisons mean, and the addressing layers the presence pair on top, because every custom field is optionally set on a member. A field's filtering is now derived — its kind names a registry type, and that type's vocabulary plus presence is everything the pill offers. The one declaration left is a line per kind, exhaustive, so a kind the registry cannot yet express fails the build instead of borrowing text, while a type of a kind it already speaks — a date alongside created_at — inherits everything and owes nothing. --- .../custom-fields/filter-fields.test.ts | 27 +++++ .../members/custom-fields/filter-fields.ts | 18 +-- .../custom-fields/filter-renderer.test.tsx | 107 ++++++++++++++++++ .../members/custom-fields/filter-renderer.tsx | 36 ++++-- 4 files changed, 167 insertions(+), 21 deletions(-) create mode 100644 apps/admin/src/members/custom-fields/filter-fields.test.ts create mode 100644 apps/admin/src/members/custom-fields/filter-renderer.test.tsx diff --git a/apps/admin/src/members/custom-fields/filter-fields.test.ts b/apps/admin/src/members/custom-fields/filter-fields.test.ts new file mode 100644 index 00000000000..e2c3ee95714 --- /dev/null +++ b/apps/admin/src/members/custom-fields/filter-fields.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest'; +import { KIND_FILTER_TYPE } from './filter-fields'; +import { MEMBER_CUSTOM_FIELD_KINDS } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import type { MemberCustomFieldKind } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import type { FilterTypeId } from '@/shared/filters'; + +// Compile-time assertions: this file is type-checked by `tsc -b`, so each expected +// error going away fails the build. +// @ts-expect-error -- must not compile, or KIND_FILTER_TYPE is no longer exhaustive +const _mappingWithAMissingKindDoesNotCompile: { [K in MemberCustomFieldKind]: FilterTypeId } = { + text: 'text', + date: 'plain_date', + number: 'number', +}; +const _mappingWithAnUnknownKindDoesNotCompile: { [K in MemberCustomFieldKind]: FilterTypeId } = { + ...KIND_FILTER_TYPE, + // @ts-expect-error -- must not compile, or KIND_FILTER_TYPE accepts undeclared kinds + boolean: 'scalar', +}; +void _mappingWithAMissingKindDoesNotCompile; +void _mappingWithAnUnknownKindDoesNotCompile; + +describe('KIND_FILTER_TYPE', () => { + it('maps every kind the shared catalog declares', () => { + expect(Object.keys(KIND_FILTER_TYPE).sort()).toEqual([...MEMBER_CUSTOM_FIELD_KINDS].sort()); + }); +}); diff --git a/apps/admin/src/members/custom-fields/filter-fields.ts b/apps/admin/src/members/custom-fields/filter-fields.ts index 5e554afca14..7a5939e0711 100644 --- a/apps/admin/src/members/custom-fields/filter-fields.ts +++ b/apps/admin/src/members/custom-fields/filter-fields.ts @@ -1,5 +1,4 @@ -import { CUSTOM_FIELD_SET_OPERATORS, customFieldAddressing } from './addressing'; -import { filterType } from '@/shared/filters'; +import { customFieldAddressing } from './addressing'; import { memberCustomFieldKind } from '@tryghost/admin-x-framework/api/member-custom-fields'; import type { FieldDescriptor, FieldProvider, FilterTypeId } from '@/shared/filters'; import type { @@ -7,7 +6,7 @@ import type { MemberCustomFieldKind, } from '@tryghost/admin-x-framework/api/member-custom-fields'; -const FILTER_TYPE_FOR_KIND: Record = { +export const KIND_FILTER_TYPE: { [K in MemberCustomFieldKind]: FilterTypeId } = { text: 'text', date: 'plain_date', number: 'number', @@ -22,25 +21,18 @@ export interface CustomFieldDefinition { type: MemberCustomField['type']; } -function filterTypeFor(type: MemberCustomField['type']): FilterTypeId { - return FILTER_TYPE_FOR_KIND[memberCustomFieldKind(type)]; -} - export function customFieldDescriptor(definition: CustomFieldDefinition): FieldDescriptor { - const type = filterTypeFor(definition.type); - const isRecord = memberCustomFieldKind(definition.type) === 'record'; + const kind = memberCustomFieldKind(definition.type); return { key: `custom_fields.${definition.key}`, icon: 'text', - type, + type: KIND_FILTER_TYPE[kind], addressing: customFieldAddressing(definition.key), ui: { label: definition.name, type: 'custom', - defaultOperator: isRecord - ? CUSTOM_FIELD_SET_OPERATORS[0] - : (filterType(type).defaultOperator ?? CUSTOM_FIELD_SET_OPERATORS[0]), + ...(kind === 'record' ? { defaultOperator: 'is-set' } : {}), }, } as FieldDescriptor; } diff --git a/apps/admin/src/members/custom-fields/filter-renderer.test.tsx b/apps/admin/src/members/custom-fields/filter-renderer.test.tsx new file mode 100644 index 00000000000..fc46db250dc --- /dev/null +++ b/apps/admin/src/members/custom-fields/filter-renderer.test.tsx @@ -0,0 +1,107 @@ +import { describe, expect, it, vi } from 'vitest'; +import { fireEvent, render, screen } from '@testing-library/react'; +import CustomFieldFilterRenderer from './filter-renderer'; +import type { FilterFieldConfig } from '@tryghost/shade/patterns'; + +vi.mock('@/shared/member-custom-fields/use-definitions', () => ({ + useCustomFieldDefinitionsIncludingArchived: () => ({ + data: { + members_custom_fields: [ + { key: 'birthday', name: 'Birthday', type: 'short_text', status: 'active' }, + ], + }, + }), +})); + +const PRESENCE_ONLY = [ + { value: 'is-set', label: 'is set' }, + { value: 'is-not-set', label: 'is not set' }, +]; + +const TEXT_OPERATORS = [ + { value: 'is', label: 'is' }, + { value: 'is-not', label: 'is not' }, + { value: 'contains', label: 'contains' }, + { value: 'does-not-contain', label: 'does not contain' }, + { value: 'starts-with', label: 'starts with' }, + { value: 'ends-with', label: 'ends with' }, + ...PRESENCE_ONLY, +]; + +function renderPill({ + operators, + defaultOperator, + operator, + onOperatorChange = () => {}, +}: { + operators: FilterFieldConfig['operators']; + defaultOperator?: string; + operator: string; + onOperatorChange?: (operator: string) => void; +}) { + return render( + {}} + onOperatorChange={onOperatorChange} + />, + ); +} + +describe('CustomFieldFilterRenderer operators', () => { + it('offers only the operators the field declares', async () => { + renderPill({ operators: PRESENCE_ONLY, defaultOperator: 'is-set', operator: 'is-set' }); + + fireEvent.pointerDown(screen.getByLabelText('Birthday operator')); + await screen.findByRole('menu'); + + expect(screen.getByRole('menuitem', { name: 'is set' })).toBeInTheDocument(); + expect(screen.queryByRole('menuitem', { name: 'contains' })).not.toBeInTheDocument(); + }); + + it('keeps an operator the field declares, even one outside the text vocabulary', () => { + const onOperatorChange = vi.fn(); + renderPill({ + operators: [{ value: 'is-or-less', label: 'is on or before' }, ...PRESENCE_ONLY], + defaultOperator: 'is-or-less', + operator: 'is-or-less', + onOperatorChange, + }); + + expect(onOperatorChange).not.toHaveBeenCalled(); + }); + + it('coerces an undeclared operator to the field default', () => { + const onOperatorChange = vi.fn(); + renderPill({ + operators: PRESENCE_ONLY, + defaultOperator: 'is-set', + operator: 'contains', + onOperatorChange, + }); + + expect(onOperatorChange).toHaveBeenCalledWith('is-set'); + }); + + it('still offers the full text vocabulary to a text field', async () => { + const onOperatorChange = vi.fn(); + renderPill({ + operators: TEXT_OPERATORS, + defaultOperator: 'contains', + operator: 'contains', + onOperatorChange, + }); + + expect(onOperatorChange).not.toHaveBeenCalled(); + fireEvent.pointerDown(screen.getByLabelText('Birthday operator')); + await screen.findByRole('menu'); + expect(screen.getByRole('menuitem', { name: 'contains' })).toBeInTheDocument(); + }); +}); diff --git a/apps/admin/src/members/custom-fields/filter-renderer.tsx b/apps/admin/src/members/custom-fields/filter-renderer.tsx index 8718fede17e..00b3a2e8e1f 100644 --- a/apps/admin/src/members/custom-fields/filter-renderer.tsx +++ b/apps/admin/src/members/custom-fields/filter-renderer.tsx @@ -1,11 +1,28 @@ import React, { useEffect } from 'react'; -import { CUSTOM_FIELDS_PREFIX, CUSTOM_FIELD_OPERATORS } from '@/members/member-fields'; +import { CUSTOM_FIELDS_PREFIX } from '@/members/member-fields'; import { CUSTOM_FIELD_SET_OPERATORS } from './addressing'; import { FilterSegmentInput, FilterSegmentSelect } from '@tryghost/shade/patterns'; import { createOperatorOptions, listsOperator } from '@/shared/filters'; import { memberCustomFieldParts } from '@tryghost/admin-x-framework/api/member-custom-fields'; import { useCustomFieldDefinitionsIncludingArchived } from '@/shared/member-custom-fields/use-definitions'; -import type { CustomRendererProps } from '@tryghost/shade/patterns'; +import type { CustomRendererProps, FilterFieldConfig } from '@tryghost/shade/patterns'; + +// "Is set" and "is not set" apply to a field of any value type. +const PRESENCE_ONLY_OPTIONS = createOperatorOptions(CUSTOM_FIELD_SET_OPERATORS); + +function offeredOperators(field: FilterFieldConfig, wholeComposite: boolean) { + const declared = field.operators?.length ? field.operators : PRESENCE_ONLY_OPTIONS; + const options = wholeComposite + ? declared.filter((option) => listsOperator(CUSTOM_FIELD_SET_OPERATORS, option.value)) + : declared; + const ids = options.map((option) => option.value); + const fallback = + field.defaultOperator && ids.includes(field.defaultOperator) + ? field.defaultOperator + : (ids[0] ?? 'is-set'); + + return { options, ids, fallback }; +} const CustomFieldFilterRenderer: React.FC> = ({ field, @@ -32,15 +49,18 @@ const CustomFieldFilterRenderer: React.FC> = ({ const [subfield = '', value = ''] = values; const isWholeField = subfield === ''; - const operators = - isComposite && isWholeField ? CUSTOM_FIELD_SET_OPERATORS : CUSTOM_FIELD_OPERATORS; + const { + options: operatorOptions, + ids: operators, + fallback: fallbackOperator, + } = offeredOperators(field, isComposite && isWholeField); useEffect(() => { - if (readOnly || !onOperatorChange || listsOperator(operators, operator)) { + if (readOnly || !onOperatorChange || operators.includes(operator)) { return; } - onOperatorChange('is-set'); - }, [readOnly, operator, operators, onOperatorChange]); + onOperatorChange(fallbackOperator); + }, [readOnly, operator, operators, fallbackOperator, onOperatorChange]); const needsValue = !listsOperator(CUSTOM_FIELD_SET_OPERATORS, operator); const partOptions = [{ value: '', label: 'Any' }, ...parts]; @@ -61,7 +81,7 @@ const CustomFieldFilterRenderer: React.FC> = ({ {onOperatorChange && ( Date: Mon, 31 Aug 2026 23:03:57 +0100 Subject: [PATCH 07/36] Fixed a bound custom field addressing claiming other fields' clauses An addressing bound to one field answered for every field: it read the key out of the clause and claimed it, so whichever bound entry was asked first parsed another field's values with its own semantics. Identical results while every field is text, silently wrong the day two fields differ. A bound addressing now refuses clauses naming any other key, leaving them to their own field's entry or to the unbound template. --- .../members/custom-fields/addressing.test.ts | 40 +++++++++++++++++++ .../src/members/custom-fields/addressing.ts | 12 +++++- 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 apps/admin/src/members/custom-fields/addressing.test.ts diff --git a/apps/admin/src/members/custom-fields/addressing.test.ts b/apps/admin/src/members/custom-fields/addressing.test.ts new file mode 100644 index 00000000000..74fa10df178 --- /dev/null +++ b/apps/admin/src/members/custom-fields/addressing.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from 'vitest'; +import { customFieldAddressing } from './addressing'; +import { parseFilterToAst } from '@/shared/filters'; + +function ast(filter: string) { + const node = parseFilterToAst(filter); + + if (!node) { + throw new Error(`could not parse: ${filter}`); + } + + return node; +} + +describe('customFieldAddressing bound to a key', () => { + const bound = customFieldAddressing('shipping'); + + it('claims a compound naming its own key', () => { + expect( + bound.matchCompound?.(ast("(custom_fields.key:'shipping'+custom_fields.value:~'x')")), + ).not.toBeNull(); + }); + + it('refuses a compound naming another field', () => { + expect( + bound.matchCompound?.(ast("(custom_fields.key:'billing'+custom_fields.value:~'x')")), + ).toBeNull(); + }); + + it('refuses a lone key clause naming another field', () => { + expect(bound.matchCompound?.(ast("custom_fields.key:'billing'"))).toBeNull(); + }); + + it('leaves other fields readable by the unbound template', () => { + const template = customFieldAddressing(); + expect( + template.matchCompound?.(ast("(custom_fields.key:'billing'+custom_fields.value:~'x')")), + ).not.toBeNull(); + }); +}); diff --git a/apps/admin/src/members/custom-fields/addressing.ts b/apps/admin/src/members/custom-fields/addressing.ts index adcf2145d86..1cbd83e9f50 100644 --- a/apps/admin/src/members/custom-fields/addressing.ts +++ b/apps/admin/src/members/custom-fields/addressing.ts @@ -81,12 +81,18 @@ export function customFieldAddressing(boundKey?: string): PresenceAddressing { }, matchCompound(node): CompoundMatch | null { + const ownsKey = (candidate: string) => boundKey === undefined || candidate === boundKey; + const children = getCompoundChildren(node, '$and'); if (!children) { const keyValue = node[KEY_ATTRIBUTE]; if (typeof keyValue === 'string') { + if (!ownsKey(keyValue)) { + return null; + } + return { kind: 'predicate', predicate: { @@ -100,6 +106,10 @@ export function customFieldAddressing(boundKey?: string): PresenceAddressing { const negatedKey = readNegatedString(keyValue); if (negatedKey !== null) { + if (!ownsKey(negatedKey)) { + return null; + } + return { kind: 'predicate', predicate: { @@ -147,7 +157,7 @@ export function customFieldAddressing(boundKey?: string): PresenceAddressing { } } - if (!fieldKey) { + if (!fieldKey || !ownsKey(fieldKey)) { return null; } From cf5f7af2df5fbd76ff4a87f9fdcdb63914523fa2 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Mon, 31 Aug 2026 23:04:11 +0100 Subject: [PATCH 08/36] Changed member filter sources to load custom field definitions up front MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The definitions were only requested once the URL's filter already named a custom field, but a pill added through the picker reaches the URL on the first keystroke — so the pill was born under the static template, which types every custom field as text, and only afterwards did the typed catalog exist. The definitions are now wanted whenever the feature flag is on. The filter bar already fetches the same query for its picker, so the common members view makes no extra request. --- .../hooks/use-member-filter-sources.test.tsx | 36 +++++++++++++++++++ .../hooks/use-member-filter-sources.ts | 31 +++++++++------- apps/admin/test-utils/acceptance/boot.ts | 9 +++-- 3 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 apps/admin/src/members/hooks/use-member-filter-sources.test.tsx diff --git a/apps/admin/src/members/hooks/use-member-filter-sources.test.tsx b/apps/admin/src/members/hooks/use-member-filter-sources.test.tsx new file mode 100644 index 00000000000..9e686aa6b3c --- /dev/null +++ b/apps/admin/src/members/hooks/use-member-filter-sources.test.tsx @@ -0,0 +1,36 @@ +import { describe, expect, it, vi } from 'vitest'; +import { renderHook } from '@testing-library/react'; +import { useMemberFilterSources } from './use-member-filter-sources'; + +const BIRTHDAY = { key: 'birthday', name: 'Birthday', type: 'short_text', status: 'active' }; + +const mocks = vi.hoisted(() => ({ + definitionsFailed: false, +})); + +vi.mock('@tryghost/admin-x-framework/api/newsletters', () => ({ + useBrowseNewsletters: () => ({ data: undefined, isError: false }), +})); + +vi.mock('@/shared/member-custom-fields/use-definitions', () => ({ + useCustomFieldDefinitionsIncludingArchived: () => ({ + data: mocks.definitionsFailed ? undefined : { members_custom_fields: [BIRTHDAY] }, + isError: mocks.definitionsFailed, + }), +})); + +describe('useMemberFilterSources custom fields', () => { + it('serves the definitions when no filter names a custom field', () => { + mocks.definitionsFailed = false; + const { result } = renderHook(() => useMemberFilterSources(undefined)); + + expect(result.current.customFields).toEqual([BIRTHDAY]); + }); + + it('serves an empty list when the definitions cannot be fetched', () => { + mocks.definitionsFailed = true; + const { result } = renderHook(() => useMemberFilterSources("custom_fields.birthday:'x'")); + + expect(result.current.customFields).toEqual([]); + }); +}); diff --git a/apps/admin/src/members/hooks/use-member-filter-sources.ts b/apps/admin/src/members/hooks/use-member-filter-sources.ts index f21c98d8b43..cf5893174c0 100644 --- a/apps/admin/src/members/hooks/use-member-filter-sources.ts +++ b/apps/admin/src/members/hooks/use-member-filter-sources.ts @@ -1,4 +1,3 @@ -import { CUSTOM_FIELDS_PREFIX } from '@/members/member-fields'; import { filterNamesKey } from '@/shared/filters'; import { useCustomFieldDefinitionsIncludingArchived } from '@/shared/member-custom-fields/use-definitions'; import { useBrowseNewsletters } from '@tryghost/admin-x-framework/api/newsletters'; @@ -16,32 +15,38 @@ export interface MemberFilterSources { } /** - * The site's own newsletters and custom fields, for whoever is reading a filter. + * The site's newsletters and its custom field definitions. The members page needs both + * to read a filter accurately: which newsletters exist decides what a + * `newsletters.` clause means, and a custom field's definition decides how its + * values compare. * - * "Not here yet" and "not coming at all" are different answers, and the difference is the whole - * point of this: the page waits for the first, and must never wait for the second or it waits - * for good. Undefined is still loading. An empty list means there is nothing to load — the - * feature is off, the request failed, or this filter never mentioned one. + * `undefined` means the answer has not arrived and the caller may wait for it. An empty + * array means no answer is coming and the caller must not wait: newsletters are empty + * when the request failed or the current filter names none, and custom fields are empty + * when the request failed. * - * Not waiting is safe. A filter still reads without these; it is just read less precisely. + * Waiting is optional either way. A filter is still readable without these lists, just + * with less accurate labels and value types. */ export function useMemberFilterSources(filterParam: string | undefined): MemberFilterSources { - const wantsCustomFields = filterNamesKey(filterParam, CUSTOM_FIELDS_PREFIX); const wantsNewsletters = filterNamesKey(filterParam, NEWSLETTERS_PREFIX); const { data: newslettersData, isError: newslettersFailed } = useBrowseNewsletters({ searchParams: { limit: '100' }, enabled: wantsNewsletters, }); + // Requested on every members page, not only when the current filter already names a + // custom field. Picking a custom field in the filter bar puts it into the URL on the + // first keystroke; if the definitions were still unrequested at that moment, the new + // filter would be interpreted by the catch-all "any custom field" entry in + // member-fields.ts, which treats every value as text. The filter bar's picker requests + // the same definitions, so this shares that cached request rather than adding one. const { data: customFieldsData, isError: customFieldsFailed } = - useCustomFieldDefinitionsIncludingArchived({ enabled: wantsCustomFields }); + useCustomFieldDefinitionsIncludingArchived(); return { newsletters: !wantsNewsletters || newslettersFailed ? NO_NEWSLETTERS : newslettersData?.newsletters, - customFields: - !wantsCustomFields || customFieldsFailed - ? NO_CUSTOM_FIELDS - : customFieldsData?.members_custom_fields, + customFields: customFieldsFailed ? NO_CUSTOM_FIELDS : customFieldsData?.members_custom_fields, }; } diff --git a/apps/admin/test-utils/acceptance/boot.ts b/apps/admin/test-utils/acceptance/boot.ts index a307ecf8f4c..f5892bd5870 100644 --- a/apps/admin/test-utils/acceptance/boot.ts +++ b/apps/admin/test-utils/acceptance/boot.ts @@ -11,8 +11,8 @@ import { import { registerAdminApiHandler, registerRoute } from './worker'; /** - * The requests the admin shell fires on boot regardless of route, handled by - * default so specs never mention them. Override per test keyed by entry + * The requests the admin shell fires on boot regardless of route, and the lookups a + * page fires on every mount, handled by default so specs never mention them. Override per test keyed by entry * name: `renderAdminApp("/", {boot: {browseMe: {response: ...}}})`. Canned * responses come from @tryghost/test-data; this harness must not import test * data from admin-x-framework. @@ -54,6 +54,11 @@ export function defaultBootRequests() { path: '/members/?limit=1', response: browseResponse('members', [], { limit: 1 }), }, + browseMemberCustomFieldDefinitions: { + method: 'GET', + path: /^\/members\/custom_fields\/(\?|$)/, + response: browseResponse('members_custom_fields', []), + }, browseActiveTheme: { method: 'GET', path: '/themes/active/', From b147f4c24d2c6b238a2731cfe96c17d84c20a526 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Tue, 1 Sep 2026 12:13:46 +0100 Subject: [PATCH 09/36] Added declared types to a composite field's parts A composite's parts each follow their own rule, but the rule was all a part carried: a country code and a postal code were both anonymous string schemas by the time anything outside the catalog saw them. Each part now declares a type alongside its rule, defined once in a catalog keyed by the type itself, so a part cannot be tagged as something its rule is not, and the parts listing carries the type to whoever renders or filters a part. The scalar short_text field type and the short_text part type are the same thing in different positions, so the field type reads its value from the part catalog rather than describing it twice. --- .../src/api/member-custom-fields.ts | 12 +- .../unit/api/member-custom-fields.test.ts | 14 +-- packages/custom-field-types/src/index.ts | 106 +++++++++++------- .../custom-field-types/test/index.test.ts | 13 +++ 4 files changed, 95 insertions(+), 50 deletions(-) diff --git a/apps/admin-x-framework/src/api/member-custom-fields.ts b/apps/admin-x-framework/src/api/member-custom-fields.ts index ab2c2e7710d..360081ec5d7 100644 --- a/apps/admin-x-framework/src/api/member-custom-fields.ts +++ b/apps/admin-x-framework/src/api/member-custom-fields.ts @@ -1,9 +1,11 @@ import { FIELD_TYPES, FIELD_TYPE_IDS, + partTypesOf, subFieldsOf, type FieldKind, type FieldType, + type PartType, type PartsOf, } from '@tryghost/custom-field-types'; import { csvColumnsForField } from '@tryghost/custom-field-types/csv'; @@ -161,10 +163,13 @@ export const memberCustomFieldCsvColumns = ( }); }; -/** One part of a composite field type: the key the value schema declares, and its label. */ +export type { PartType as MemberCustomFieldPartType } from '@tryghost/custom-field-types'; + +/** One part of a composite field type: the key the value schema declares, its label, and its declared type. */ export type MemberCustomFieldPart = { key: PartsOf; label: string; + type: PartType; }; /** @@ -177,11 +182,12 @@ export const memberCustomFieldParts = ( type: T, ): MemberCustomFieldPart[] | null => { const partKeys = subFieldsOf(type); - if (!partKeys) { + const partTypes = partTypesOf(type); + if (!partKeys || !partTypes) { return null; } const labels = partLabelsFor(type); - return partKeys.map((key) => ({ key, label: labels[key] })); + return partKeys.map((key) => ({ key, label: labels[key], type: partTypes[key] })); }; /** diff --git a/apps/admin-x-framework/test/unit/api/member-custom-fields.test.ts b/apps/admin-x-framework/test/unit/api/member-custom-fields.test.ts index bdc22cd0c0f..f60ca0c4077 100644 --- a/apps/admin-x-framework/test/unit/api/member-custom-fields.test.ts +++ b/apps/admin-x-framework/test/unit/api/member-custom-fields.test.ts @@ -186,14 +186,14 @@ describe('member custom fields api helpers', () => { expect(memberCustomFieldParts('long_text')).toBeNull(); }); - it("names a composite type's parts in the order its value schema declares them", () => { + it("carries each part's key, label and declared type, in schema order", () => { expect(memberCustomFieldParts('address')).toEqual([ - { key: 'line1', label: 'Address line 1' }, - { key: 'line2', label: 'Address line 2' }, - { key: 'city', label: 'City' }, - { key: 'state', label: 'State' }, - { key: 'postal_code', label: 'Postal code' }, - { key: 'country', label: 'Country' }, + { key: 'line1', label: 'Address line 1', type: 'short_text' }, + { key: 'line2', label: 'Address line 2', type: 'short_text' }, + { key: 'city', label: 'City', type: 'short_text' }, + { key: 'state', label: 'State', type: 'short_text' }, + { key: 'postal_code', label: 'Postal code', type: 'postal_code' }, + { key: 'country', label: 'Country', type: 'country_code' }, ]); }); }); diff --git a/packages/custom-field-types/src/index.ts b/packages/custom-field-types/src/index.ts index d8305ec8ad6..531d1a57fe3 100644 --- a/packages/custom-field-types/src/index.ts +++ b/packages/custom-field-types/src/index.ts @@ -93,44 +93,57 @@ const byteLength = (value: string): number => new TextEncoder().encode(value).le */ const text = () => z.string({ error: 'Enter text.' }).trim(); -const shortText = () => text().max(255, { error: 'Use 255 characters or fewer.' }); - const longText = () => text().refine((value) => byteLength(value) <= MAX_LONG_TEXT_BYTES, { error: 'This text is too long to save. Shorten it a little.', }); /** - * A postal code, bounded well under a street address because no country's is long. The - * bound is a sanity limit rather than a format: postal codes vary too much between - * countries to check the shape of one without knowing which country it is for, and the - * country is a sibling part rather than something this can see. + * Both ends are pinned to a string because storage keeps one string per leaf: a type + * parsing to anything else could be written and never read back, which is better learned + * from the compiler than from a 500 on the first save. */ -const postalCode = () => text().max(32, { error: 'Use 32 characters or fewer.' }); +type PartSchema = z.ZodType; /** - * The shape of an ISO 3166-1 alpha-2 code, deliberately not checked against the list of - * them. Membership of that list is contested, and a closed list here would make Ghost the - * arbiter of it for every member of every site. The collection form can offer countries to - * pick from without this deciding which ones exist. - * - * Case is normalized so that `gb` and `GB` are not two values for one place, which a - * filter for either would silently half-miss. - * - * Checked as two ASCII letters on the way in rather than by length on the way out, because - * uppercasing does not preserve length: `ß` becomes `SS` and `aß` becomes `ASS`. + * The types a composite's parts can declare, each defined once with its rule. A country + * code and a postal code both store short text, but they are different things — the + * part's type is what a control or a filter dispatches on, the way a field's own type + * is for a scalar. */ -const countryCode = () => - text() +export const PART_TYPES = { + short_text: text().max(255, { error: 'Use 255 characters or fewer.' }), + + // A postal code, bounded well under a street address because no country's is long. The + // bound is a sanity limit rather than a format: postal codes vary too much between + // countries to check the shape of one without knowing which country it is for, and the + // country is a sibling part rather than something this can see. + postal_code: text().max(32, { error: 'Use 32 characters or fewer.' }), + + // The shape of an ISO 3166-1 alpha-2 code, deliberately not checked against the list of + // them. Membership of that list is contested, and a closed list here would make Ghost + // the arbiter of it for every member of every site. The collection form can offer + // countries to pick from without this deciding which ones exist. + // + // Case is normalized so that `gb` and `GB` are not two values for one place, which a + // filter for either would silently half-miss. + // + // Checked as two ASCII letters on the way in rather than by length on the way out, + // because uppercasing does not preserve length: `ß` becomes `SS` and `aß` becomes `ASS`. + country_code: text() .regex(/^[A-Za-z]{2}$/, { error: 'Enter a 2-letter country code, like US.' }) - .toUpperCase(); + .toUpperCase(), +} satisfies Record; -/** - * Both ends are pinned to a string because storage keeps one string per leaf: a type - * parsing to anything else could be written and never read back, which is better learned - * from the compiler than from a 500 on the first save. - */ -type PartSchema = z.ZodType; +export type PartType = keyof typeof PART_TYPES; +export const PART_TYPE_IDS = Object.keys(PART_TYPES) as PartType[]; + +interface TypedPart { + type: T; + schema: (typeof PART_TYPES)[T]; +} + +const part = (type: T): TypedPart => ({ type, schema: PART_TYPES[type] }); /** * A part as a write may name it: absent, empty, or a value of its own kind. @@ -138,9 +151,9 @@ type PartSchema = z.ZodType; * A rule that is a bound would admit empty on its own; one that is a format would not, * and would leave its part the only one that could be set but never removed. */ -const clearable = (part: T) => +const clearable = (schema: T) => text() - .pipe(z.union([z.literal(''), part])) + .pipe(z.union([z.literal(''), schema])) .optional(); export interface FieldTypeDefinition { @@ -151,7 +164,7 @@ export interface FieldTypeDefinition { * A record type's parts, in declaration order. Each part's own rule, and nothing * about how a write may name it: validate against `value`, never against these. */ - fields?: Record; + fields?: Record; } /** @@ -173,24 +186,24 @@ function defineFieldTypes>(decl * explicitly undefined survives parsing as a key holding undefined, and a bare presence * check would let `{line1: undefined}` through as if it named something. */ -function record>(fields: F, { error }: { error: string }) { +function record>(fields: F, { error }: { error: string }) { // Restated for the type system, which loses the key-to-schema mapping through // `Object.fromEntries`; without it every type built on a record infers as `unknown`. const shape = Object.fromEntries( - Object.entries(fields).map(([key, part]) => [key, clearable(part)]), - ) as { [K in keyof F]: ReturnType> }; + Object.entries(fields).map(([key, declared]) => [key, clearable(declared.schema)]), + ) as { [K in keyof F]: ReturnType> }; // Strict, so a part nobody declared is refused rather than dropped. That refusal keeps // zod's wording, which names the offending key. const value = z .strictObject(shape) - .refine((parts) => Object.values(parts).some((part) => typeof part === 'string'), { error }); + .refine((parts) => Object.values(parts).some((entry) => typeof entry === 'string'), { error }); return { kind: 'record' as const, value, fields }; } export const FIELD_TYPES = defineFieldTypes({ - short_text: { kind: 'text', value: shortText() }, + short_text: { kind: 'text', value: PART_TYPES.short_text }, long_text: { kind: 'text', value: longText() }, // An address is a delivery address, so its bounds are what a courier will accept // rather than what the column could hold. Modeled on Stripe's Address object. @@ -202,12 +215,12 @@ export const FIELD_TYPES = defineFieldTypes({ // also how Stripe hands it back: beside the address rather than inside it. address: record( { - line1: shortText(), - line2: shortText(), - city: shortText(), - state: shortText(), - postal_code: postalCode(), - country: countryCode(), + line1: part('short_text'), + line2: part('short_text'), + city: part('short_text'), + state: part('short_text'), + postal_code: part('postal_code'), + country: part('country_code'), }, { error: 'Enter at least one part of the address.' }, ), @@ -251,3 +264,16 @@ export function subFieldsOf(type: T): PartsOf[] | null { // The keys are `PartsOf` by construction: `fields` is the object it reads `keyof` from. return definition?.fields ? (Object.keys(definition.fields) as PartsOf[]) : null; } + +/** Each part's declared type, keyed by part; null for a type with no parts, and for one this build has never heard of. */ +export function partTypesOf(type: T): Record, PartType> | null { + const definition: FieldTypeDefinition | undefined = FIELD_TYPES[type]; + + if (!definition?.fields) { + return null; + } + + return Object.fromEntries( + Object.entries(definition.fields).map(([key, declared]) => [key, declared.type]), + ) as Record, PartType>; +} diff --git a/packages/custom-field-types/test/index.test.ts b/packages/custom-field-types/test/index.test.ts index 9530c38fc0b..fea8202199d 100644 --- a/packages/custom-field-types/test/index.test.ts +++ b/packages/custom-field-types/test/index.test.ts @@ -4,6 +4,7 @@ import { FIELD_TYPES, FIELD_TYPE_IDS, MAX_LONG_TEXT_BYTES, + partTypesOf, subFieldsOf, type FieldType, } from '../src/index.ts'; @@ -25,6 +26,18 @@ describe('custom-field-types catalog', function () { }); }); + it("declares each part's own type", function () { + assert.deepEqual(partTypesOf('address'), { + line1: 'short_text', + line2: 'short_text', + city: 'short_text', + state: 'short_text', + postal_code: 'postal_code', + country: 'country_code', + }); + assert.equal(partTypesOf('short_text'), null); + }); + it('reads a type it has never heard of as having no parts', function () { // Only reachable by lying about the type, which is what an admin build older than // the server it talks to does: the type is a string off the wire, asserted not From b8ddc7949a72f007db59b794b96497344d33b0f3 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Tue, 1 Sep 2026 12:14:00 +0100 Subject: [PATCH 10/36] Changed custom field filtering to derive from declared types A composite's filter type was declared as text at the kind level, which reads as a claim about composites when it is really a fact about their parts. Scalar kinds keep their map, exhaustive so a kind the registry cannot express fails the build. A composite now derives its filter type from what its parts declare, and refuses parts whose declared types filter differently, because the engine reads a composite with a single semantics; whoever first maps a part type away from its siblings is pointed at the per-part dispatch that change needs rather than left to misread values silently. --- .../custom-fields/filter-fields.test.ts | 55 +++++++++++++++---- .../members/custom-fields/filter-fields.ts | 35 ++++++++++-- .../custom-fields/filter-renderer.test.tsx | 42 +++++++++++++- 3 files changed, 114 insertions(+), 18 deletions(-) diff --git a/apps/admin/src/members/custom-fields/filter-fields.test.ts b/apps/admin/src/members/custom-fields/filter-fields.test.ts index e2c3ee95714..52f52649e54 100644 --- a/apps/admin/src/members/custom-fields/filter-fields.test.ts +++ b/apps/admin/src/members/custom-fields/filter-fields.test.ts @@ -1,27 +1,60 @@ import { describe, expect, it } from 'vitest'; -import { KIND_FILTER_TYPE } from './filter-fields'; +import { PART_FILTER_TYPE, SCALAR_KIND_FILTER_TYPE, customFieldDescriptor } from './filter-fields'; import { MEMBER_CUSTOM_FIELD_KINDS } from '@tryghost/admin-x-framework/api/member-custom-fields'; -import type { MemberCustomFieldKind } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import type { + MemberCustomFieldKind, + MemberCustomFieldPartType, +} from '@tryghost/admin-x-framework/api/member-custom-fields'; import type { FilterTypeId } from '@/shared/filters'; +type ScalarKind = Exclude; + // Compile-time assertions: this file is type-checked by `tsc -b`, so each expected // error going away fails the build. -// @ts-expect-error -- must not compile, or KIND_FILTER_TYPE is no longer exhaustive -const _mappingWithAMissingKindDoesNotCompile: { [K in MemberCustomFieldKind]: FilterTypeId } = { +// @ts-expect-error -- must not compile, or SCALAR_KIND_FILTER_TYPE is no longer exhaustive +const _mappingWithAMissingKindDoesNotCompile: { [K in ScalarKind]: FilterTypeId } = { text: 'text', date: 'plain_date', - number: 'number', }; -const _mappingWithAnUnknownKindDoesNotCompile: { [K in MemberCustomFieldKind]: FilterTypeId } = { - ...KIND_FILTER_TYPE, - // @ts-expect-error -- must not compile, or KIND_FILTER_TYPE accepts undeclared kinds +const _mappingWithAnUnknownKindDoesNotCompile: { [K in ScalarKind]: FilterTypeId } = { + ...SCALAR_KIND_FILTER_TYPE, + // @ts-expect-error -- must not compile, or SCALAR_KIND_FILTER_TYPE accepts undeclared kinds boolean: 'scalar', }; void _mappingWithAMissingKindDoesNotCompile; void _mappingWithAnUnknownKindDoesNotCompile; -describe('KIND_FILTER_TYPE', () => { - it('maps every kind the shared catalog declares', () => { - expect(Object.keys(KIND_FILTER_TYPE).sort()).toEqual([...MEMBER_CUSTOM_FIELD_KINDS].sort()); +describe('SCALAR_KIND_FILTER_TYPE', () => { + it('maps every scalar kind the shared catalog declares', () => { + const scalarKinds = MEMBER_CUSTOM_FIELD_KINDS.filter((kind) => kind !== 'record'); + expect(Object.keys(SCALAR_KIND_FILTER_TYPE).sort()).toEqual([...scalarKinds].sort()); + }); +}); + +describe('a composite field descriptor', () => { + it('filters parts as text and starts the whole field at presence', () => { + const descriptor = customFieldDescriptor({ + key: 'shipping', + name: 'Shipping', + type: 'address', + }); + + expect(descriptor.type).toBe('text'); + expect(descriptor.ui.defaultOperator).toBe('is-set'); + }); +}); + +// @ts-expect-error -- must not compile, or PART_FILTER_TYPE is no longer exhaustive +const _partMappingWithAMissingTypeDoesNotCompile: { + [P in MemberCustomFieldPartType]: FilterTypeId; +} = { + short_text: 'text', + postal_code: 'text', +}; +void _partMappingWithAMissingTypeDoesNotCompile; + +describe('PART_FILTER_TYPE', () => { + it('filters every part type the same way, because a composite is read with one semantics', () => { + expect(new Set(Object.values(PART_FILTER_TYPE)).size).toBe(1); }); }); diff --git a/apps/admin/src/members/custom-fields/filter-fields.ts b/apps/admin/src/members/custom-fields/filter-fields.ts index 7a5939e0711..909d5cd5675 100644 --- a/apps/admin/src/members/custom-fields/filter-fields.ts +++ b/apps/admin/src/members/custom-fields/filter-fields.ts @@ -1,18 +1,45 @@ import { customFieldAddressing } from './addressing'; -import { memberCustomFieldKind } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import { + memberCustomFieldKind, + memberCustomFieldParts, +} from '@tryghost/admin-x-framework/api/member-custom-fields'; import type { FieldDescriptor, FieldProvider, FilterTypeId } from '@/shared/filters'; import type { MemberCustomField, MemberCustomFieldKind, + MemberCustomFieldPartType, } from '@tryghost/admin-x-framework/api/member-custom-fields'; -export const KIND_FILTER_TYPE: { [K in MemberCustomFieldKind]: FilterTypeId } = { +export const SCALAR_KIND_FILTER_TYPE: { + [K in Exclude]: FilterTypeId; +} = { text: 'text', date: 'plain_date', number: 'number', - record: 'text', }; +export const PART_FILTER_TYPE: { [P in MemberCustomFieldPartType]: FilterTypeId } = { + short_text: 'text', + postal_code: 'text', + country_code: 'text', +}; + +function compositeFilterType(type: MemberCustomField['type']): FilterTypeId { + const partFilterTypes = [ + ...new Set((memberCustomFieldParts(type) ?? []).map((p) => PART_FILTER_TYPE[p.type])), + ]; + + if (partFilterTypes.length > 1) { + throw new Error( + `The parts of '${type}' filter as different types (${partFilterTypes.join(', ')}), ` + + 'but the filter engine reads a composite with a single semantics. Build per-part ' + + 'dispatch into the codec before mapping a part type away from its siblings.', + ); + } + + return partFilterTypes[0] ?? 'text'; +} + export const CUSTOM_FIELD_CLAUSE = 'custom_fields.'; export interface CustomFieldDefinition { @@ -27,7 +54,7 @@ export function customFieldDescriptor(definition: CustomFieldDefinition): FieldD return { key: `custom_fields.${definition.key}`, icon: 'text', - type: KIND_FILTER_TYPE[kind], + type: kind === 'record' ? compositeFilterType(definition.type) : SCALAR_KIND_FILTER_TYPE[kind], addressing: customFieldAddressing(definition.key), ui: { label: definition.name, diff --git a/apps/admin/src/members/custom-fields/filter-renderer.test.tsx b/apps/admin/src/members/custom-fields/filter-renderer.test.tsx index fc46db250dc..7893220edb5 100644 --- a/apps/admin/src/members/custom-fields/filter-renderer.test.tsx +++ b/apps/admin/src/members/custom-fields/filter-renderer.test.tsx @@ -8,6 +8,7 @@ vi.mock('@/shared/member-custom-fields/use-definitions', () => ({ data: { members_custom_fields: [ { key: 'birthday', name: 'Birthday', type: 'short_text', status: 'active' }, + { key: 'shipping', name: 'Shipping', type: 'address', status: 'active' }, ], }, }), @@ -33,22 +34,28 @@ function renderPill({ defaultOperator, operator, onOperatorChange = () => {}, + fieldKey = 'custom_fields.birthday', + label = 'Birthday', + values = ['', ''], }: { operators: FilterFieldConfig['operators']; defaultOperator?: string; operator: string; onOperatorChange?: (operator: string) => void; + fieldKey?: string; + label?: string; + values?: string[]; }) { return render( {}} onOperatorChange={onOperatorChange} />, @@ -104,4 +111,33 @@ describe('CustomFieldFilterRenderer operators', () => { await screen.findByRole('menu'); expect(screen.getByRole('menuitem', { name: 'contains' })).toBeInTheDocument(); }); + + it('narrows a whole composite to presence, and opens up once a part is chosen', async () => { + const whole = renderPill({ + operators: TEXT_OPERATORS, + defaultOperator: 'is-set', + operator: 'is-set', + fieldKey: 'custom_fields.shipping', + label: 'Shipping', + }); + + fireEvent.pointerDown(screen.getByLabelText('Shipping operator')); + await screen.findByRole('menu'); + expect(screen.getByRole('menuitem', { name: 'is set' })).toBeInTheDocument(); + expect(screen.queryByRole('menuitem', { name: 'contains' })).not.toBeInTheDocument(); + whole.unmount(); + + renderPill({ + operators: TEXT_OPERATORS, + defaultOperator: 'is-set', + operator: 'contains', + fieldKey: 'custom_fields.shipping', + label: 'Shipping', + values: ['city', 'London'], + }); + + fireEvent.pointerDown(screen.getByLabelText('Shipping operator')); + await screen.findByRole('menu'); + expect(screen.getByRole('menuitem', { name: 'contains' })).toBeInTheDocument(); + }); }); From 13d2f8a123127d756d1aae5c6c23a7536d012e9f Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Tue, 1 Sep 2026 12:25:57 +0100 Subject: [PATCH 11/36] Changed local Stripe development to a single dev:stripe command The default now receives webhooks the way production does, through Ghost's own registered URL over a Tailscale tunnel. The Stripe CLI forwarder sits behind an explicit --listen option that warns at startup, because forwarded events carry the account's default API version and their shape has already misled development once. --- ...ote.yaml => compose.dev.stripe-tunnel.yaml | 4 +- docker/stripe/entrypoint.sh | 3 +- docker/stripe/with-remote-webhooks.sh | 137 ------------- docker/stripe/with-stripe.sh | 193 +++++++++++++++--- docs/contributing/development-setup.md | 20 +- e2e/README.md | 6 +- package.json | 1 - 7 files changed, 183 insertions(+), 181 deletions(-) rename compose.dev.stripe-remote.yaml => compose.dev.stripe-tunnel.yaml (59%) delete mode 100755 docker/stripe/with-remote-webhooks.sh diff --git a/compose.dev.stripe-remote.yaml b/compose.dev.stripe-tunnel.yaml similarity index 59% rename from compose.dev.stripe-remote.yaml rename to compose.dev.stripe-tunnel.yaml index 01aa988138f..0052099c41b 100644 --- a/compose.dev.stripe-remote.yaml +++ b/compose.dev.stripe-tunnel.yaml @@ -2,6 +2,6 @@ services: ghost-dev: environment: # Ghost registers its own Stripe webhook endpoint, as it does in production, at the - # tunnel URL started by with-remote-webhooks.sh. The site itself stays on localhost. - stripeWebhookUrl: ${GHOST_STRIPE_WEBHOOK_URL:?set by docker/stripe/with-remote-webhooks.sh; run pnpm dev:stripe:remote} + # tunnel URL started by with-stripe.sh. The site itself stays on localhost. + stripeWebhookUrl: ${GHOST_STRIPE_WEBHOOK_URL:?set by docker/stripe/with-stripe.sh; run pnpm dev:stripe} stripeRemoteWebhooks: 'true' diff --git a/docker/stripe/entrypoint.sh b/docker/stripe/entrypoint.sh index 29a60fae133..28436ea398c 100755 --- a/docker/stripe/entrypoint.sh +++ b/docker/stripe/entrypoint.sh @@ -5,7 +5,8 @@ ## that the Ghost server can read to verify webhook signatures. ## Events forwarded by `stripe listen` use the Stripe account's default API version, not ## the version Ghost registers with in production, so their shape can differ from what -## production receives. Use `pnpm dev:stripe:remote` when the shape matters. +## production receives. This service only runs with `pnpm dev:stripe --listen`; the +## default `pnpm dev:stripe` receives webhooks exactly as production does. # Note: the stripe CLI container is based on alpine, hence `sh` instead of `bash`. set -eu diff --git a/docker/stripe/with-remote-webhooks.sh b/docker/stripe/with-remote-webhooks.sh deleted file mode 100755 index 4f9e5b85d58..00000000000 --- a/docker/stripe/with-remote-webhooks.sh +++ /dev/null @@ -1,137 +0,0 @@ -#!/bin/bash - -# Runs the development environment with Ghost receiving Stripe webhooks the way it -# does in production. -# -# In production Ghost gives Stripe a URL to send webhooks to, registered with a fixed -# Stripe API version, so the messages always have the same shape. The usual local -# setup forwards webhooks with Stripe's command line tool instead, which uses the -# account's default API version, so messages can have a shape production never sends. -# -# This script makes Ghost's webhook URL reachable from the internet through Tailscale, -# nothing else on the machine, and tells Ghost to register that URL with Stripe at -# boot. Ghost removes the registration when it shuts down. The site and Admin stay on -# localhost. -# -# Usage: ./docker/stripe/with-remote-webhooks.sh -# Example: ./docker/stripe/with-remote-webhooks.sh pnpm nx run ghost-monorepo:docker:dev - -set -euo pipefail - -FUNNEL_PORT=443 -GATEWAY_PORT=2368 -WEBHOOK_PATH=/members/webhooks/stripe - -fail() { - echo "" - echo "================================================================================" - echo "ERROR: $1" - echo "" - shift - for line in "$@"; do - echo "$line" - done - echo "================================================================================" - echo "" - exit 1 -} - -[ "$#" -gt 0 ] || fail "no command given" \ - "Usage: $0 " \ - "Example: $0 pnpm nx run ghost-monorepo:docker:dev" - -# The macOS app bundle does not put its CLI on PATH. -TAILSCALE=$(command -v tailscale || true) -if [ -z "$TAILSCALE" ] && [ -x /Applications/Tailscale.app/Contents/MacOS/Tailscale ]; then - TAILSCALE=/Applications/Tailscale.app/Contents/MacOS/Tailscale -fi -[ -n "$TAILSCALE" ] || fail "tailscale is not installed" \ - "Install it from https://tailscale.com/download and sign in, then re-run." - -status=$("$TAILSCALE" status --json 2>/dev/null || true) -read -r backend hostname < <(node -e ' - const status = JSON.parse(process.argv[1] || "{}"); - const name = ((status.Self || {}).DNSName || "").replace(/\.$/, ""); - process.stdout.write(`${status.BackendState || "Unknown"} ${name}\n`); -' "$status") - -[ "$backend" = "Running" ] || fail "tailscale is not connected (state: $backend)" \ - "Run 'tailscale up' (or open the Tailscale app and sign in), then re-run." -[ -n "$hostname" ] || fail "this node has no MagicDNS name" \ - "Funnel needs MagicDNS and HTTPS certificates enabled for the tailnet." \ - "See https://tailscale.com/kb/1223/funnel" - -if [ "$FUNNEL_PORT" = "443" ]; then - public_origin="https://${hostname}" -else - public_origin="https://${hostname}:${FUNNEL_PORT}" -fi -export GHOST_STRIPE_WEBHOOK_URL="${public_origin}${WEBHOOK_PATH}/" - -# Something is already published on this port: a funnel left running in the -# background, or another copy of this script. Do not take it over. -if "$TAILSCALE" funnel status --json 2>/dev/null | grep -q "\"${hostname}:${FUNNEL_PORT}\""; then - fail "tailscale funnel is already serving port ${FUNNEL_PORT}" \ - "If nothing else needs it: tailscale funnel --https=${FUNNEL_PORT} off" \ - "If another pnpm dev:stripe:remote is running, stop that first." -fi - -echo "Publishing Ghost's Stripe webhook route at ${GHOST_STRIPE_WEBHOOK_URL} via Tailscale Funnel" -echo "Only that path is reachable from the internet, and only while this command runs." -# Run the funnel as a child process without --bg. Tailscale then keeps the URL public -# only while that process lives, so a crash, a closed terminal or a reboot cannot leave -# it published. Tailscale removes the path from the request before forwarding, so the -# target includes the path again for Ghost to route on. -funnel_err=$(mktemp) -"$TAILSCALE" funnel --https="$FUNNEL_PORT" --set-path "$WEBHOOK_PATH" \ - "http://127.0.0.1:${GATEWAY_PORT}${WEBHOOK_PATH}" >/dev/null 2>"$funnel_err" & -funnel_pid=$! - -stop_funnel() { - kill "$funnel_pid" 2>/dev/null || true - wait "$funnel_pid" 2>/dev/null || true - rm -f "$funnel_err" -} -# Bash skips the EXIT trap when a signal kills it, so turn signals into exits. -trap stop_funnel EXIT -trap 'exit 130' INT -trap 'exit 143' TERM HUP - -funnel_ready=false -for _ in $(seq 1 20); do - if "$TAILSCALE" funnel status --json 2>/dev/null | grep -q "\"${hostname}:${FUNNEL_PORT}\""; then - funnel_ready=true - break - fi - kill -0 "$funnel_pid" 2>/dev/null || break - sleep 0.5 -done -if [ "$funnel_ready" != true ]; then - grep -v 'client version' "$funnel_err" >&2 || true - fail "tailscale funnel could not be started" \ - "Funnel must be enabled for your tailnet and this node (Tailscale 1.52 or newer)." \ - "See https://tailscale.com/kb/1223/funnel" -fi - -# The `stripe` compose profile starts Stripe's command line forwarder. With it running, -# Ghost would use the forwarder instead of registering its own URL, and every event -# would also arrive a second time in the other shape. -profiles="${COMPOSE_PROFILES:-}" -if [ -z "$profiles" ] && [ -f .env ]; then - profiles=$(grep -E '^COMPOSE_PROFILES=' .env | tail -n1 | cut -d= -f2- | sed -e 's/[[:space:]]*#.*$//' -e "s/^['\"]//" -e "s/['\"]$//" || true) -fi -if [[ ",${profiles}," == *",stripe,"* ]]; then - echo "Dropping the 'stripe' compose profile: remote webhooks replace stripe listen." - profiles=$(echo "$profiles" | tr ',' '\n' | grep -vx 'stripe' | paste -sd, - || true) -fi -export COMPOSE_PROFILES="$profiles" - -export DEV_COMPOSE_FILES="${DEV_COMPOSE_FILES:-} -f compose.dev.stripe-remote.yaml" - -echo "Ghost registers its webhook endpoint at boot once Stripe is connected in Ghost Admin (Settings > Tiers)." -echo "Open the site and Admin on http://localhost:${GATEWAY_PORT} as usual." -echo "Watch the ghost-dev logs: it warns if Stripe is not connected." - -# The wrapped command stops the containers before it returns, and Ghost removes its -# Stripe registration during that stop. The funnel is closed after that, on exit. -"$@" diff --git a/docker/stripe/with-stripe.sh b/docker/stripe/with-stripe.sh index 7914511fa8f..9cd4d4bf86e 100755 --- a/docker/stripe/with-stripe.sh +++ b/docker/stripe/with-stripe.sh @@ -1,41 +1,180 @@ #!/bin/bash -# Wrapper script to run commands with the Stripe profile enabled -# Checks for STRIPE_SECRET_KEY before starting, failing early with helpful error +# Runs the development environment with Stripe webhooks. # -# Usage: ./docker/stripe/with-stripe.sh -# Example: ./docker/stripe/with-stripe.sh nx run ghost-monorepo:docker:dev - -set -e - -check_stripe_key() { - # Check environment variable first - if [ -n "$STRIPE_SECRET_KEY" ]; then - return 0 - fi +# In production Ghost gives Stripe a URL to send webhooks to, registered with a fixed +# Stripe API version, so the messages always have the same shape. By default this +# script reproduces that: it makes Ghost's webhook URL reachable from the internet +# through Tailscale, nothing else on the machine, and Ghost registers that URL with +# Stripe at boot and removes it on shutdown. The site and Admin stay on localhost. +# +# With --listen, webhooks are instead forwarded by Stripe's command line tool. That +# needs no Tailscale, but the tool delivers events at the Stripe account's default +# API version, not the fixed one, so a payload can have a shape production never +# sends. Use it only when the payload shape does not matter for your work. +# +# Usage: ./docker/stripe/with-stripe.sh [--listen] +# Example: ./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev - # Check .env file for non-empty value - if [ -f .env ] && grep -qE '^STRIPE_SECRET_KEY=.+' .env; then - return 0 - fi +set -euo pipefail - return 1 -} +FUNNEL_PORT=443 +GATEWAY_PORT=2368 +WEBHOOK_PATH=/members/webhooks/stripe -if ! check_stripe_key; then +fail() { echo "" echo "================================================================================" - echo "ERROR: STRIPE_SECRET_KEY is not set" - echo "" - echo "To use the Stripe service, set STRIPE_SECRET_KEY in your .env file or ENV vars:" - echo " STRIPE_SECRET_KEY=sk_test_..." + echo "ERROR: $1" echo "" - echo "You can find your secret key at: https://dashboard.stripe.com/test/apikeys" + shift + for line in "$@"; do + echo "$line" + done echo "================================================================================" echo "" exit 1 +} + + +# pnpm appends extra arguments after the wrapped command, so --listen can appear +# anywhere; take it out of the command before running it. +listen=false +args=() +for arg in "$@"; do + if [ "$arg" = "--listen" ]; then + listen=true + else + args+=("$arg") + fi +done +set -- "${args[@]+"${args[@]}"}" + +[ "$#" -gt 0 ] || fail "no command given" \ + "Usage: $0 [--listen]" \ + "Example: $0 pnpm nx run ghost-monorepo:docker:dev" + +if [ "$listen" = true ]; then + # Forwarding needs a Stripe API key for the command line tool. + key_ok=false + if [ -n "${STRIPE_SECRET_KEY:-}" ]; then + key_ok=true + elif [ -f .env ] && grep -qE '^STRIPE_SECRET_KEY=.+' .env; then + key_ok=true + fi + if [ "$key_ok" != true ]; then + fail "STRIPE_SECRET_KEY is not set" \ + "To forward Stripe webhooks, set STRIPE_SECRET_KEY in your .env file or environment:" \ + " STRIPE_SECRET_KEY=sk_test_..." \ + "You can find your secret key at: https://dashboard.stripe.com/test/apikeys" + fi + + echo "Forwarding Stripe webhooks with the Stripe CLI (--listen)." + echo "WARNING: forwarded events use your Stripe account's default API version, not the" + echo "version Ghost registers with in production, so payloads can have a shape" + echo "production never sends. Ghost logs an error when that happens. Run without" + echo "--listen to receive webhooks exactly as production does." + + export COMPOSE_PROFILES="${COMPOSE_PROFILES:+$COMPOSE_PROFILES,}stripe" + exec "$@" +fi + +FUNNEL_PORT=443 +GATEWAY_PORT=2368 +WEBHOOK_PATH=/members/webhooks/stripe + +# The macOS app bundle does not put its CLI on PATH. +TAILSCALE=$(command -v tailscale || true) +if [ -z "$TAILSCALE" ] && [ -x /Applications/Tailscale.app/Contents/MacOS/Tailscale ]; then + TAILSCALE=/Applications/Tailscale.app/Contents/MacOS/Tailscale +fi +[ -n "$TAILSCALE" ] || fail "tailscale is not installed" \ + "Install it from https://tailscale.com/download and sign in, then re-run." + +status=$("$TAILSCALE" status --json 2>/dev/null || true) +read -r backend hostname < <(node -e ' + const status = JSON.parse(process.argv[1] || "{}"); + const name = ((status.Self || {}).DNSName || "").replace(/\.$/, ""); + process.stdout.write(`${status.BackendState || "Unknown"} ${name}\n`); +' "$status") + +[ "$backend" = "Running" ] || fail "tailscale is not connected (state: $backend)" \ + "Run 'tailscale up' (or open the Tailscale app and sign in), then re-run." +[ -n "$hostname" ] || fail "this node has no MagicDNS name" \ + "Funnel needs MagicDNS and HTTPS certificates enabled for the tailnet." \ + "See https://tailscale.com/kb/1223/funnel" + +if [ "$FUNNEL_PORT" = "443" ]; then + public_origin="https://${hostname}" +else + public_origin="https://${hostname}:${FUNNEL_PORT}" +fi +export GHOST_STRIPE_WEBHOOK_URL="${public_origin}${WEBHOOK_PATH}/" + +# Something is already published on this port: a funnel left running in the +# background, or another copy of this script. Do not take it over. +if "$TAILSCALE" funnel status --json 2>/dev/null | grep -q "\"${hostname}:${FUNNEL_PORT}\""; then + fail "tailscale funnel is already serving port ${FUNNEL_PORT}" \ + "If nothing else needs it: tailscale funnel --https=${FUNNEL_PORT} off" \ + "If another pnpm dev:stripe is running, stop that first." fi -# Run the command with the stripe profile enabled -export COMPOSE_PROFILES="${COMPOSE_PROFILES:+$COMPOSE_PROFILES,}stripe" -exec "$@" +echo "Publishing Ghost's Stripe webhook route at ${GHOST_STRIPE_WEBHOOK_URL} via Tailscale Funnel" +echo "Only that path is reachable from the internet, and only while this command runs." +# Run the funnel as a child process without --bg. Tailscale then keeps the URL public +# only while that process lives, so a crash, a closed terminal or a reboot cannot leave +# it published. Tailscale removes the path from the request before forwarding, so the +# target includes the path again for Ghost to route on. +funnel_err=$(mktemp) +"$TAILSCALE" funnel --https="$FUNNEL_PORT" --set-path "$WEBHOOK_PATH" \ + "http://127.0.0.1:${GATEWAY_PORT}${WEBHOOK_PATH}" >/dev/null 2>"$funnel_err" & +funnel_pid=$! + +stop_funnel() { + kill "$funnel_pid" 2>/dev/null || true + wait "$funnel_pid" 2>/dev/null || true + rm -f "$funnel_err" +} +# Bash skips the EXIT trap when a signal kills it, so turn signals into exits. +trap stop_funnel EXIT +trap 'exit 130' INT +trap 'exit 143' TERM HUP + +funnel_ready=false +for _ in $(seq 1 20); do + if "$TAILSCALE" funnel status --json 2>/dev/null | grep -q "\"${hostname}:${FUNNEL_PORT}\""; then + funnel_ready=true + break + fi + kill -0 "$funnel_pid" 2>/dev/null || break + sleep 0.5 +done +if [ "$funnel_ready" != true ]; then + grep -v 'client version' "$funnel_err" >&2 || true + fail "tailscale funnel could not be started" \ + "Funnel must be enabled for your tailnet and this node (Tailscale 1.52 or newer)." \ + "See https://tailscale.com/kb/1223/funnel" +fi + +# The `stripe` compose profile starts Stripe's command line forwarder. With it running, +# Ghost would use the forwarder instead of registering its own URL, and every event +# would also arrive a second time in the other shape. +profiles="${COMPOSE_PROFILES:-}" +if [ -z "$profiles" ] && [ -f .env ]; then + profiles=$(grep -E '^COMPOSE_PROFILES=' .env | tail -n1 | cut -d= -f2- | sed -e 's/[[:space:]]*#.*$//' -e "s/^['\"]//" -e "s/['\"]$//" || true) +fi +if [[ ",${profiles}," == *",stripe,"* ]]; then + echo "Dropping the 'stripe' compose profile: remote webhooks replace stripe listen." + profiles=$(echo "$profiles" | tr ',' '\n' | grep -vx 'stripe' | paste -sd, - || true) +fi +export COMPOSE_PROFILES="$profiles" + +export DEV_COMPOSE_FILES="${DEV_COMPOSE_FILES:-} -f compose.dev.stripe-tunnel.yaml" + +echo "Ghost registers its webhook endpoint at boot once Stripe is connected in Ghost Admin (Settings > Tiers)." +echo "Open the site and Admin on http://localhost:${GATEWAY_PORT} as usual." +echo "Watch the ghost-dev logs: it warns if Stripe is not connected." + +# The wrapped command stops the containers before it returns, and Ghost removes its +# Stripe registration during that stop. The funnel is closed after that, on exit. +"$@" diff --git a/docs/contributing/development-setup.md b/docs/contributing/development-setup.md index 9ec17094b73..3bdcc1b7727 100644 --- a/docs/contributing/development-setup.md +++ b/docs/contributing/development-setup.md @@ -113,8 +113,7 @@ environment and adds the listed tooling: | `pnpm dev:analytics` | Tinybird-backed analytics with the latest published version of the Traffic Analytics service | | `pnpm dev:analytics:local` | Tinybird-backed analytics with your locally running instance of the Traffic Analytics service | | `pnpm dev:storage` | S3-compatible storage through MinIO on ports `9000` and `9001` | -| `pnpm dev:stripe` | Stripe webhooks; requires `STRIPE_SECRET_KEY` in the environment or a local `.env` file | -| `pnpm dev:stripe:remote` | Stripe webhooks exactly as production receives them; requires Tailscale, see below | +| `pnpm dev:stripe` | Stripe webhooks exactly as production receives them; requires Tailscale, see below | | `pnpm dev:full` | Public app watchers plus analytics, storage, and Stripe | Copy [`.env.example`](../../.env.example) to `.env` only when you need an @@ -126,14 +125,7 @@ subdirectory, and separate-Admin URL behaviour, see ### Stripe webhooks -`pnpm dev:stripe` forwards events with `stripe listen`. The CLI renders every -event at your Stripe account's default API version, which cannot be pinned, so -an event can carry a different shape from the one Ghost's production endpoint -receives. Ghost pins that endpoint to its own API version when it creates it. -Production keeps a persistent endpoint; a normal development environment never -creates one. - -`pnpm dev:stripe:remote` runs the production path instead. It publishes Ghost's +`pnpm dev:stripe` runs the webhook path production runs. It publishes Ghost's webhook route, and nothing else, through [Tailscale Funnel](https://tailscale.com/kb/1223/funnel), and Ghost registers a pinned webhook endpoint at that address once Stripe is connected in Admin, then @@ -155,6 +147,14 @@ Funnel enabled for your tailnet and node. The command reports when Tailscale is missing, not signed in, or has no MagicDNS name; for the other requirements it shows Tailscale's own error. +`pnpm dev:stripe --listen` forwards events with `stripe listen` instead, which +needs `STRIPE_SECRET_KEY` in the environment or a local `.env` file but no +Tailscale. The CLI renders every event at your Stripe account's default API +version, which cannot be pinned, so an event can carry a different shape from +the one production receives; the command warns about this at startup and Ghost +logs an error when a mismatched event arrives. Use it only when the payload +shape does not matter. + ## Data and email After creating the local owner account, populate a development site with stable diff --git a/e2e/README.md b/e2e/README.md index a9a13197233..b8ebfc6fbf6 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -363,9 +363,9 @@ renderings: at Stripe's current default the shipping address moves to `collected_information.shipping_details`, which Ghost never sees. Ghost reads only `event.type` and `event.data.object`, so the envelope carries nothing worth pinning. -The same difference applies to `stripe listen`, which `pnpm dev:stripe` uses: it renders -events at the account default too. To see the payloads production receives, run -`pnpm dev:stripe:remote`, which lets Ghost register its own pinned endpoint (see +The same difference applies to `stripe listen`, which `pnpm dev:stripe --listen` uses: +it renders events at the account default too. The default `pnpm dev:stripe` lets Ghost +register its own pinned endpoint, so it receives the payloads production receives (see [Development setup](../docs/contributing/development-setup.md#stripe-webhooks)). ## Resolving issues diff --git a/package.json b/package.json index 54ffcc72640..4c5dcf5f9a3 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,6 @@ "dev:analytics:local": "ANALYTICS_PROXY_TARGET=traffic-analytics-local:3000 DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml' pnpm nx run ghost-monorepo:docker:dev", "dev:storage": "DEV_COMPOSE_FILES='-f compose.dev.storage.yaml' pnpm nx run ghost-monorepo:docker:dev", "dev:stripe": "./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev", - "dev:stripe:remote": "./docker/stripe/with-remote-webhooks.sh pnpm nx run ghost-monorepo:docker:dev", "dev:all": "DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml -f compose.dev.storage.yaml' ./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev", "dev:daemon": "NX_DAEMON=true NX_TUI=false NX_DEFAULT_OUTPUT_STYLE=stream pnpm nx run ghost-monorepo:docker:dev", "fix": "pnpm store prune && rimraf -g '**/node_modules' && pnpm install && pnpm nx reset", From 51adc5b92041a24986ab17f15b0610f2d86a105e Mon Sep 17 00:00:00 2001 From: Jonatan Svennberg Date: Tue, 1 Sep 2026 14:35:11 +0200 Subject: [PATCH 12/36] Fixed gift form state across Stripe checkout (#30279) fixes https://linear.app/ghost/issue/BER-3890 Persisted the personalised gift checkout draft in session storage so returning from Stripe restores the delivery step without losing buyer or recipient input. Explicit plan and delivery routes preserve browser navigation for direct and in-Portal entry points, while shared abandonment actions and a valid checkout success clear saved personal data. --- apps/portal/src/actions.js | 16 +- apps/portal/src/app.jsx | 30 ++- .../src/components/pages/beta-gift-page.tsx | 248 +++++++++++++++--- .../components/pages/beta-gift/form-state.ts | 101 +++++++ .../components/pages/beta-gift/navigation.ts | 76 ++++++ apps/portal/src/utils/api.js | 2 +- .../src/utils/use-session-storage-state.ts | 48 ++++ apps/portal/test/actions.test.ts | 36 +++ apps/portal/test/api.test.js | 1 + apps/portal/test/app.test.jsx | 17 ++ apps/portal/test/portal-links.test.jsx | 92 +++++++ .../components/pages/beta-gift-page.test.tsx | 169 +++++++++++- .../utils/use-session-storage-state.test.tsx | 75 ++++++ 13 files changed, 867 insertions(+), 44 deletions(-) create mode 100644 apps/portal/src/components/pages/beta-gift/form-state.ts create mode 100644 apps/portal/src/components/pages/beta-gift/navigation.ts create mode 100644 apps/portal/src/utils/use-session-storage-state.ts create mode 100644 apps/portal/test/unit/utils/use-session-storage-state.test.tsx diff --git a/apps/portal/src/actions.js b/apps/portal/src/actions.js index e89d87f6c1e..bd825fe926b 100644 --- a/apps/portal/src/actions.js +++ b/apps/portal/src/actions.js @@ -14,6 +14,8 @@ import { getRefDomain, } from './utils/helpers'; import { t } from './utils/i18n'; +import { clearGiftFormState } from './components/pages/beta-gift/form-state'; +import { restoreGiftEntryRoute } from './components/pages/beta-gift/navigation'; const CANNOT_CHECKOUT_WITH_EXISTING_SUBSCRIPTION = 'CANNOT_CHECKOUT_WITH_EXISTING_SUBSCRIPTION'; @@ -43,6 +45,10 @@ function openPopup({ data }) { } function back({ state }) { + if (state.page === 'gift') { + clearGiftFormState(); + } + if (state.lastPage) { return { page: state.lastPage, @@ -53,7 +59,15 @@ function back({ state }) { } function closePopup({ state }) { - removePortalLinkFromUrl(); + let restoredGiftEntryRoute = false; + if (state.page === 'gift') { + clearGiftFormState(); + restoredGiftEntryRoute = restoreGiftEntryRoute(); + } + + if (!restoredGiftEntryRoute) { + removePortalLinkFromUrl(); + } // Drop any one-shot post-sign-in redirect (e.g. set when sign-in is opened // from a comment "Reply") so a dismissed sign-in can't leak its redirect into // a later, unrelated sign-in on the same page. Other pageData is preserved. diff --git a/apps/portal/src/app.jsx b/apps/portal/src/app.jsx index b190488c9ef..344b5b021df 100644 --- a/apps/portal/src/app.jsx +++ b/apps/portal/src/app.jsx @@ -14,7 +14,8 @@ import { transformPortalAnchorToRelative } from './utils/transform-portal-anchor import { getActivePage, isAccountPage, isOfferPage } from './pages'; import ActionHandler from './actions'; import { getGiftRedemptionErrorMessage } from './utils/gift-redemption-notification'; -import { GIFT_DURATION_CATALOGUE } from './utils/gift-subscriptions'; +import { GIFT_DURATION_CATALOGUE, isGiftCustomizationEnabled } from './utils/gift-subscriptions'; +import { clearGiftFormState } from './components/pages/beta-gift/form-state'; import './app.css'; import { hasRecommendations, @@ -211,7 +212,7 @@ export default class App extends React.Component { event.preventDefault(); const target = event.currentTarget; const pagePath = target && target.dataset.portal; - const linkData = this.getPageFromLinkPath(pagePath); + const linkData = this.getPageFromLinkPath(pagePath, this.state.site); if (!linkData) { return; } @@ -679,6 +680,7 @@ export default class App extends React.Component { 'gift_scheduled_at', ]); if (token) { + clearGiftFormState(); return { showPopup: true, page: 'giftSuccess', @@ -1046,6 +1048,11 @@ export default class App extends React.Component { } const { site: linkSite, ...restLinkData } = linkData; + const isLeavingGiftPage = this.state.page === 'gift' && restLinkData.page !== 'gift'; + if (isLeavingGiftPage) { + clearGiftFormState(); + } + const shouldCloseGiftPopup = isLeavingGiftPage && !restLinkData.page; const updatedState = { site: { @@ -1060,6 +1067,7 @@ export default class App extends React.Component { }, ...restLinkData, ...restPreviewData, + ...(shouldCloseGiftPopup ? { showPopup: false, lastPage: null } : {}), }; this.handleSignupQuery({ site: updatedState.site, pageQuery: updatedState.pageQuery }); this.setState(updatedState); @@ -1142,7 +1150,7 @@ export default class App extends React.Component { } /**Get Portal page from Link/Data-attribute path*/ - getPageFromLinkPath(path) { + getPageFromLinkPath(path, site) { const customPricesSignupRegex = /^signup\/?(?:\/(\w+?))?\/?$/; const customMonthlyProductSignup = /^signup\/?(?:\/(\w+?))\/monthly\/?$/; const customYearlyProductSignup = /^signup\/?(?:\/(\w+?))\/yearly\/?$/; @@ -1246,7 +1254,21 @@ export default class App extends React.Component { signup: false, }, }; - } else if (path === 'gift') { + } else if (path === 'gift' && isGiftCustomizationEnabled({ site })) { + return { + page: 'gift', + pageData: { + giftStep: 'plan', + }, + }; + } else if (path === 'gift/delivery' && isGiftCustomizationEnabled({ site })) { + return { + page: 'gift', + pageData: { + giftStep: 'delivery', + }, + }; + } else if (path === 'gift' || path === 'gift/delivery') { return { page: 'gift', }; diff --git a/apps/portal/src/components/pages/beta-gift-page.tsx b/apps/portal/src/components/pages/beta-gift-page.tsx index a13cd482185..2de49923b21 100644 --- a/apps/portal/src/components/pages/beta-gift-page.tsx +++ b/apps/portal/src/components/pages/beta-gift-page.tsx @@ -19,10 +19,20 @@ import { getGiftDurationAttributiveLabel } from '../../utils/gift-redemption-not import { ValidateInputForm } from '../../utils/form'; import { t } from '../../utils/i18n'; import useCardTilt from '../../utils/use-card-tilt'; +import useSessionStorageState from '../../utils/use-session-storage-state'; import { formatGiftValue } from './gift-page'; import GiftDeliveryStep from './beta-gift/delivery-step'; +import { + GIFT_EMAIL_MAX_LENGTH, + GIFT_FORM_STATE_KEY, + GIFT_MESSAGE_MAX_LENGTH, + GIFT_NAME_MAX_LENGTH, + createGiftFormState, + parseGiftFormState, +} from './beta-gift/form-state'; import GiftPlanStep from './beta-gift/plan-step'; import GiftPreviewPanel from './beta-gift/preview-panel'; +import { ensureGiftPlanRoute, restoreGiftEntryRoute, setGiftRoute } from './beta-gift/navigation'; import type { GiftDeliveryMethod, GiftCadenceDuration, @@ -35,9 +45,6 @@ const validateInputForm = ValidateInputForm as unknown as (data: { fields: GiftInputField[]; }) => GiftFormErrors; -const GIFT_EMAIL_MAX_LENGTH = 191; -const GIFT_NAME_MAX_LENGTH = 191; -const GIFT_MESSAGE_MAX_LENGTH = 250; // Mirrors GIFT_MAX_SCHEDULE_DAYS in ghost/core's gifts constants — change them together. const GIFT_MAX_SCHEDULE_DAYS = 365; @@ -51,6 +58,9 @@ interface GiftPageContext { doAction: (action: string, data?: Record) => void; lastPage: string | null; member: GiftPageMember | null; + pageData?: { + giftStep?: GiftStep; + }; site: Site | null; } @@ -58,31 +68,75 @@ function getTierPriceLabel(product: GiftProduct, months: GiftDuration) { return formatGiftValue(getGiftPrice(product, months)); } +function getPortalHash(page: string | null) { + if (page === 'signup') { + return '#/portal/signup'; + } + if (page === 'accountHome') { + return '#/portal/account'; + } + if (page === 'accountPlan') { + return '#/portal/account/plans'; + } + return null; +} + const BetaGiftPage = () => { - const { site, member, brandColor, action, doAction, lastPage } = useContext( + const { site, member, brandColor, action, doAction, lastPage, pageData } = useContext( AppContext, ) as GiftPageContext; - const [step, setStep] = useState('plan'); - const [selectedDuration, setSelectedDuration] = useState(null); - const [selectedProductId, setSelectedProductId] = useState(null); - const [email, setEmail] = useState(''); - const [recipientEmail, setRecipientEmail] = useState(''); - const [recipientName, setRecipientName] = useState(''); - const [buyerName, setBuyerName] = useState(member?.name || ''); - const [giftMessage, setGiftMessage] = useState(''); - const [deliveryMethod, setDeliveryMethod] = useState('email'); - // null means untouched: the effective date then tracks "today" in the site's timezone on every - // render, so an untouched form still means "send now" after the page sits open across midnight. - const [deliveryDate, setDeliveryDate] = useState(null); + const routeStep: GiftStep = pageData?.giftStep === 'delivery' ? 'delivery' : 'plan'; + const [step, setStep] = useState(routeStep); + const [formState, setFormState] = useSessionStorageState({ + key: GIFT_FORM_STATE_KEY, + initialState: () => createGiftFormState({ buyerName: member?.name || '' }), + parse: parseGiftFormState, + }); const [errors, setErrors] = useState({}); const { cardRef, containerProps: cardTiltProps } = useCardTilt(); + const handledRouteStepRef = useRef(null); + const enteredDeliveryFromPlanRef = useRef(false); + + const { plan, delivery } = formState; + const { selectedDuration, selectedProductId, buyerEmail: email, buyerName } = plan; + const { method: deliveryMethod, emailDraft } = delivery; + const { + recipientEmail, + recipientName, + message: giftMessage, + timing: deliveryTiming, + } = emailDraft; // Prefill the "from" name once the logged-in member loads, without clobbering anything the buyer // has already typed. useEffect(() => { - setBuyerName((current) => current || member?.name || ''); + setFormState((current) => { + if (current.plan.buyerName || !member?.name) { + return current; + } + + return { + ...current, + plan: { ...current.plan, buyerName: member.name }, + }; + }); }, [member?.name]); + useEffect(() => { + if (routeStep === 'delivery' && !plan.completed) { + setStep('plan'); + setGiftRoute({ step: 'plan', replace: true }); + return; + } + + if (handledRouteStepRef.current === routeStep) { + return; + } + handledRouteStepRef.current = routeStep; + + setStep(routeStep); + }, [plan.completed, routeStep]); + // Anchors us to the popup's real (iframe) document for scroll control. const contentRef = useRef(null); @@ -116,25 +170,55 @@ const BetaGiftPage = () => { return () => cancelAnimationFrame(raf); }, [step]); - if (!site) { - return ; - } + const handleExit = () => { + const lastPageHash = getPortalHash(lastPage); + if (!restoreGiftEntryRoute() && lastPageHash) { + window.history.replaceState(window.history.state, '', lastPageHash); + } + doAction('back'); + }; + + const handleClose = () => { + doAction('closePopup'); + }; - const { portal_default_plan: portalDefaultPlan } = site; + const portalDefaultPlan = site?.portal_default_plan ?? null; const offeredDurations = getAvailableGiftDurations({ site }); const activeDuration = getActiveGiftDuration({ availableDurations: offeredDurations, - portalDefaultPlan: portalDefaultPlan ?? null, + portalDefaultPlan, selectedDuration, }); const products = activeDuration ? getGiftProducts({ site, duration: activeDuration }) : []; + const restoredSelectionAvailable = + selectedDuration !== null && + offeredDurations.includes(selectedDuration) && + selectedProductId !== null && + products.some((product) => product.id === selectedProductId); + + useEffect(() => { + if (!site || !plan.completed || restoredSelectionAvailable) { + return; + } + + setFormState((current) => ({ + ...current, + plan: { ...current.plan, completed: false }, + })); + setStep('plan'); + setGiftRoute({ step: 'plan', replace: true }); + }, [plan.completed, restoredSelectionAvailable, site]); + + if (!site) { + return ; + } const siteIcon = site.icon; const siteTitle = site.title || ''; if (!activeDuration || products.length === 0) { return (
- +