From 60e93aafd7ef6ee0035422ccea9b617442cd858c Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 10:30:32 +0200 Subject: [PATCH] Put in the key everything the call actually sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A key says "I already sent this call", not "this is what the object is". The catalogue calls sent metadata that the key knew nothing about, so adding the tax_treatment field in 9da1358 poisoned yesterday's key for a day — and AddonPrices::ensure() runs inside a customer's module booking, not only in the sweep. createPrice and createProduct now fold a fingerprint of their parameters into the key. refund, cancelSubscription, addSubscriptionItem and the checkout deliberately do not: there a second object is the customer's money taken twice, and Stripe's refusal is the thing worth keeping. The fake could not see any of this. Its ledger replayed the first id for a repeated key without ever comparing what the second call asked for. Co-Authored-By: Claude Opus 5 --- app/Services/Stripe/FakeStripeClient.php | 107 +++++++++++---- app/Services/Stripe/HttpStripeClient.php | 17 ++- app/Services/Stripe/IdempotencyKey.php | 86 ++++++++++++ .../Billing/StripeIdempotencyKeyTest.php | 122 ++++++++++++++++++ 4 files changed, 301 insertions(+), 31 deletions(-) create mode 100644 app/Services/Stripe/IdempotencyKey.php create mode 100644 tests/Feature/Billing/StripeIdempotencyKeyTest.php diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 5a5398d..c6bb114 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -19,7 +19,7 @@ class FakeStripeClient implements StripeClient /** @var array */ public array $products = []; - /** @var array */ + /** @var array */ public array $prices = []; /** @var array */ @@ -34,7 +34,17 @@ class FakeStripeClient implements StripeClient */ public array $activated = []; - /** Idempotency key → the id first returned for it. @var array */ + /** + * Idempotency key → what was first answered for it, and a fingerprint of + * what it was first sent with. + * + * The fingerprint is the half that was missing. This ledger replayed the + * first id for a repeated key without ever looking at what the second call + * was asking for — so it was unrealistic in precisely the way that mattered, + * and no test could see the 2026-07-29 blockade. + * + * @var array + */ public array $keys = []; /** @@ -150,17 +160,20 @@ class FakeStripeClient implements StripeClient public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string { + $key = IdempotencyKey::forProduct($idempotencyKey, $name, $metadata); + $parameters = ['name' => $name, 'metadata' => $metadata]; + // Replays the first answer for a repeated key, as Stripe does. - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { - return $this->keys[$idempotencyKey]; + $replayed = $this->replay($key, $parameters); + + if ($replayed !== null) { + return $replayed; } $id = 'prod_'.substr(sha1($name.count($this->products)), 0, 12); $this->products[$id] = ['name' => $name, 'metadata' => $metadata]; - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $id; - } + $this->rememberKey($key, $id, $parameters); return $id; } @@ -173,8 +186,19 @@ class FakeStripeClient implements StripeClient array $metadata = [], ?string $idempotencyKey = null, ): string { - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { - return $this->keys[$idempotencyKey]; + $key = IdempotencyKey::forPrice($idempotencyKey, $productId, $amountCents, $currency, $interval, $metadata); + $parameters = [ + 'product' => $productId, + 'unit_amount' => $amountCents, + 'currency' => strtolower($currency), + 'interval' => $interval, + 'metadata' => $metadata, + ]; + + $replayed = $this->replay($key, $parameters); + + if ($replayed !== null) { + return $replayed; } $id = 'price_'.substr(sha1($productId.$amountCents.$interval.count($this->prices)), 0, 12); @@ -184,11 +208,13 @@ class FakeStripeClient implements StripeClient 'currency' => $currency, 'interval' => $interval, 'metadata' => $metadata, + // Stripe stamps every object with its creation time and + // AdoptStripePrice takes the OLDEST of several orphans. A counter + // is enough and beats a clock: it cannot tie. + 'created' => count($this->prices) + 1, ]; - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $id; - } + $this->rememberKey($key, $id, $parameters); return $id; } @@ -308,9 +334,12 @@ class FakeStripeClient implements StripeClient throw new RuntimeException("Unknown cancellation timing: {$when}"); } - // Replays the first answer for a repeated key, as Stripe does, so a retry - // after a timeout records one order to stop rather than two. - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { + // No fingerprint: for a subscription cancellation, a changed parameter + // under a used key is exactly what must fail loudly. See IdempotencyKey. + $parameters = ['subscription' => $subscriptionId, 'when' => $when]; + $replayed = $this->replay($idempotencyKey, $parameters); + + if ($replayed !== null) { return; } @@ -320,9 +349,7 @@ class FakeStripeClient implements StripeClient 'key' => $idempotencyKey, ]; - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $subscriptionId; - } + $this->rememberKey($idempotencyKey, $subscriptionId, $parameters); } public function refund( @@ -332,10 +359,13 @@ class FakeStripeClient implements StripeClient ): string { $this->failIfAsked(); - // Replays the first answer for a repeated key, as Stripe does — which - // is the whole point of sending one on a refund. - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { - return $this->keys[$idempotencyKey]; + // No fingerprint: for a refund, a changed parameter under a used key is + // exactly what must fail loudly. See IdempotencyKey. + $parameters = ['payment' => $paymentReference, 'amount' => (string) $amountCents]; + $replayed = $this->replay($idempotencyKey, $parameters); + + if ($replayed !== null) { + return $replayed; } $this->refunds[] = [ @@ -346,9 +376,7 @@ class FakeStripeClient implements StripeClient $id = 're_'.substr(sha1($paymentReference.count($this->refunds)), 0, 12); - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $id; - } + $this->rememberKey($idempotencyKey, $id, $parameters); return $id; } @@ -382,4 +410,33 @@ class FakeStripeClient implements StripeClient throw new RuntimeException($this->failWith); } } + + /** + * The id Stripe already answered for this key, or null when it is new. + * + * Throws on a key that comes back with different parameters, which is what + * Stripe does — with this wording — and what makes a test able to see the + * failure at all. + */ + private function replay(?string $key, array $parameters): ?string + { + if ($key === null || ! isset($this->keys[$key])) { + return null; + } + + if ($this->keys[$key]['fingerprint'] !== IdempotencyKey::fingerprint($parameters)) { + throw new RuntimeException( + 'Keys for idempotent requests can only be used with the same parameters they were first used with.', + ); + } + + return $this->keys[$key]['id']; + } + + private function rememberKey(?string $key, string $id, array $parameters): void + { + if ($key !== null) { + $this->keys[$key] = ['id' => $id, 'fingerprint' => IdempotencyKey::fingerprint($parameters)]; + } + } } diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index a88a1a0..c24e581 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -86,7 +86,7 @@ class HttpStripeClient implements StripeClient public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string { - return (string) $this->request($idempotencyKey) + return (string) $this->request(IdempotencyKey::forProduct($idempotencyKey, $name, $metadata)) ->asForm() ->post($this->url('products'), array_filter([ 'name' => $name, @@ -104,7 +104,9 @@ class HttpStripeClient implements StripeClient array $metadata = [], ?string $idempotencyKey = null, ): string { - return (string) $this->request($idempotencyKey) + return (string) $this->request(IdempotencyKey::forPrice( + $idempotencyKey, $productId, $amountCents, $currency, $interval, $metadata, + )) ->asForm() ->post($this->url('prices'), [ 'product' => $productId, @@ -345,10 +347,13 @@ class HttpStripeClient implements StripeClient $request = Http::withToken($secret)->acceptJson()->timeout(20); // Stripe replays the original response for a repeated key instead of - // creating a second object. That covers the gap this cannot close on - // its own: a crash between Stripe creating a Price and us storing its - // id. Their keys expire after 24 hours, so it protects a retry, not a - // sync re-run next week — for which the stored ids are the guard. + // creating a second object — but ONLY for a call repeated exactly; a + // changed parameter under a used key is HTTP 400 for twenty-four hours. + // The catalogue calls therefore fold a fingerprint of their parameters + // into the key (see IdempotencyKey); the money calls deliberately do + // not. Their keys expire after 24 hours, so this protects a retry, not + // a sync re-run next week — for which the stored ids and + // AdoptStripePrice are the guard. return $idempotencyKey !== null ? $request->withHeaders(['Idempotency-Key' => $idempotencyKey]) : $request; diff --git a/app/Services/Stripe/IdempotencyKey.php b/app/Services/Stripe/IdempotencyKey.php new file mode 100644 index 0000000..f42b79d --- /dev/null +++ b/app/Services/Stripe/IdempotencyKey.php @@ -0,0 +1,86 @@ + $productId, + 'unit_amount' => $amountCents, + 'currency' => strtolower($currency), + 'interval' => $interval, + 'metadata' => $metadata, + ]); + } + + /** The key for a Product, for the same reason and against the same trap. */ + public static function forProduct(?string $key, string $name, array $metadata): ?string + { + return self::with($key, ['name' => $name, 'metadata' => $metadata]); + } + + /** + * Eight hex characters over the parameters, canonically ordered. + * + * Ordered, because PHP keeps insertion order and two callers writing the + * same metadata in a different order would otherwise send two keys for one + * call — which would mint two Prices and be a worse bug than the one this + * closes. + */ + public static function fingerprint(array $parameters): string + { + return substr(sha1(self::canonical($parameters)), 0, 8); + } + + private static function with(?string $key, array $parameters): ?string + { + // Null stays null: a caller who sends no key wants no replay, and + // inventing one here would change what the call means. + return $key === null ? null : $key.'-'.self::fingerprint($parameters); + } + + private static function canonical(array $parameters): string + { + ksort($parameters); + + foreach ($parameters as $name => $value) { + $parameters[$name] = is_array($value) ? self::canonical($value) : (string) $value; + } + + return json_encode($parameters, JSON_THROW_ON_ERROR); + } +} diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php new file mode 100644 index 0000000..36b3c26 --- /dev/null +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -0,0 +1,122 @@ +set('services.stripe.secret', 'sk_test_plan_task_one'); +}); + +it('sends a different key once the metadata changes', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_x'])]); + + $client = new HttpStripeClient; + $spoken = 'clupilot-addon-price-priority_support-month-3480-EUR'; + + // The call as it stood before 9da1358, and the call after it: same money, + // same interval, one metadata field more. + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], $spoken); + + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'tax_treatment' => 'domestic'], $spoken); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent[0])->toStartWith($spoken) + ->and($sent[1])->toStartWith($spoken) + ->and($sent[1])->not->toBe($sent[0]); +}); + +it('sends the same key for the very same call', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_x'])]); + + $client = new HttpStripeClient; + + foreach ([1, 2] as $ignored) { + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], 'clupilot-addon-price'); + } + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->unique() + ->all(); + + expect($sent)->toHaveCount(1); +}); + +it('fingerprints the product call too, where the same trap was waiting', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'prod_x'])]); + + $client = new HttpStripeClient; + + $client->createProduct('Priority Support', ['addon' => 'priority_support'], 'clupilot-addon-product-x'); + $client->createProduct('Priority Support', ['addon' => 'priority_support', 'sold_as' => 'entitlement'], 'clupilot-addon-product-x'); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent[1])->not->toBe($sent[0]); +}); + +it('leaves the money calls their bare key, so Stripe still refuses a changed one', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'x'])]); + + $client = new HttpStripeClient; + + $client->refund('pi_1', 500, 'clupilot-refund-7'); + $client->cancelSubscription('sub_1', 'at_period_end', 'clupilot-cancel-7'); + $client->addSubscriptionItem('sub_1', 'price_1', 1, 'none', 'clupilot-item-7'); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent)->toBe(['clupilot-refund-7', 'clupilot-cancel-7', 'clupilot-item-7']); +}); + +it('reproduces the refusal Stripe makes, which the fake used to swallow', function () { + $fake = new FakeStripeClient; + + $fake->refund('pi_1', 500, 'clupilot-refund-7'); + + // Same key, different amount. Stripe answers 400; the fake said nothing and + // replayed the first refund's id, which is how a test could pass over the + // very failure that stopped production. + expect(fn () => $fake->refund('pi_1', 900, 'clupilot-refund-7')) + ->toThrow(RuntimeException::class, 'same parameters'); +}); + +it('mints a second price rather than blocking when the metadata moved', function () { + $fake = new FakeStripeClient; + + $first = $fake->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], 'clupilot-addon-price'); + + $second = $fake->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'tax_treatment' => 'domestic'], 'clupilot-addon-price'); + + // Two objects, no exception. That the second one is not WANTED is the job of + // AdoptStripePrice, not of the key — see StripePriceAdoptionTest. + expect($second)->not->toBe($first); +});