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); +});