From e1e70efb4cd8592e655333c1993207eb394860b2 Mon Sep 17 00:00:00 2001 From: Paul van Brenk <5273975+paulvanbrenk@users.noreply.github.com> Date: Sat, 5 Sep 2026 21:24:07 -0400 Subject: [PATCH 1/3] fix(api): stop dropping subscription and invoice webhook events The Stripe webhook only acted on events whose data object carried metadata["app"] == "patchnotes". That tag was set in one place, on the Checkout Session, and Stripe does not copy a session's own metadata onto the subscription it creates -- subscription_data.metadata is a separate parameter that was never set. Invoices never carry it at all. So four of the five handled event types were dropped: customer.subscription.updated, customer.subscription.deleted, invoice.payment_failed and invoice.payment_succeeded. A subscriber activated correctly at checkout and was then frozen: renewals never extended SubscriptionExpiresAt, so a paying customer lost Pro after one period; cancellations never recorded; failed payments never set past_due. It was invisible because the ignore path returns 200, so Stripe's dashboard showed a healthy endpoint with full delivery success. Events are now recognised two ways. subscription_data.metadata is set at checkout so new subscriptions and their events carry the tag regardless of delivery order, and anything untagged is matched by a customer id we already store -- without that fallback every subscription created before this commit would stay broken forever, since the tag cannot be applied retroactively. Also stop trusting the snapshot payload in the two subscription handlers. Snapshot events are eventually consistent, so the subscription is re-read from the API and the payload is used only to identify it. Re-fetching does not help when the event refers to a subscription the user has already replaced -- a cancelled subscription still reads back as cancelled -- so a delayed "deleted" for a superseded subscription is declined explicitly rather than cancelling the current one. Recognising events by customer also means a checkout session with no metadata at all now reaches its handler, where the old filter would have dropped it first. Reading stytch_user_id there is made null-safe to match: without it the handler throws, returns 500, and Stripe retries until it disables the endpoint. Tests cover the ownership decision through the real signature path. Three of them fail against the previous implementation. The re-fetch itself is not covered: it calls SubscriptionService directly, which would need the Stripe client injected to fake. Co-Authored-By: Claude Opus 5 (1M context) --- PatchNotes.Api/Routes/SubscriptionRoutes.cs | 11 + PatchNotes.Api/Webhooks/StripeWebhook.cs | 115 ++++++-- PatchNotes.Tests/StripeWebhookTests.cs | 281 ++++++++++++++++++++ 3 files changed, 383 insertions(+), 24 deletions(-) create mode 100644 PatchNotes.Tests/StripeWebhookTests.cs diff --git a/PatchNotes.Api/Routes/SubscriptionRoutes.cs b/PatchNotes.Api/Routes/SubscriptionRoutes.cs index 73651617..142e5203 100644 --- a/PatchNotes.Api/Routes/SubscriptionRoutes.cs +++ b/PatchNotes.Api/Routes/SubscriptionRoutes.cs @@ -103,6 +103,17 @@ public static WebApplication MapSubscriptionRoutes(this WebApplication app) { "stytch_user_id", stytchUserId }, { "app", "patchnotes" }, }, + // Stripe does not copy the session's metadata onto the subscription it creates, + // so the tag is set again here. Without it the subscription and its invoices + // arrive at our webhook carrying nothing that identifies them as ours. + SubscriptionData = new SessionSubscriptionDataOptions + { + Metadata = new Dictionary + { + { "stytch_user_id", stytchUserId }, + { "app", "patchnotes" }, + }, + }, }; // If user already has a Stripe customer ID, use it diff --git a/PatchNotes.Api/Webhooks/StripeWebhook.cs b/PatchNotes.Api/Webhooks/StripeWebhook.cs index 3fae56ab..ebbaec99 100644 --- a/PatchNotes.Api/Webhooks/StripeWebhook.cs +++ b/PatchNotes.Api/Webhooks/StripeWebhook.cs @@ -52,20 +52,24 @@ public static WebApplication MapStripeWebhook(this WebApplication app) return Results.Ok(new { received = true, duplicate = true }); } - // Filter events to only those for our app - if (stripeEvent.Data.Object is IHasMetadata objWithMetadata) + // Filter events to only those for our app. Two checks, because neither alone + // covers every event we handle: + // + // - Metadata. Stripe never copies a Checkout Session's own metadata onto the + // subscription it creates, so only sessions carried the "app" tag until + // SubscriptionData.Metadata was added at checkout. Subscriptions created since + // then carry it too, which makes them recognisable no matter what order Stripe + // delivers the events in. + // - Known customer. Invoices never carry the tag, and subscriptions created before + // that change never will. Those are matched by the customer we already stored. + // + // Anything we cannot place is acknowledged and dropped. + if (!HasAppMetadata(stripeEvent.Data.Object) + && !await IsKnownCustomerAsync(stripeEvent.Data.Object, db)) { - var metadata = objWithMetadata.Metadata; - if (metadata == null || !metadata.TryGetValue("app", out var appValue) || appValue != "patchnotes") - { - // Not our event, ignore but acknowledge - return Results.Ok(new { received = true, ignored = true }); - } - } - else - { - // Unknown object type without metadata — skip to be safe - logger.LogWarning("Stripe event {EventType} data object does not support metadata, skipping", stripeEvent.Type); + logger.LogInformation( + "Ignoring Stripe event {EventId} ({EventType}): not associated with this app", + stripeEvent.Id, stripeEvent.Type); return Results.Ok(new { received = true, ignored = true }); } @@ -124,15 +128,74 @@ public static WebApplication MapStripeWebhook(this WebApplication app) return app; } + private const string AppMetadataKey = "app"; + private const string AppMetadataValue = "patchnotes"; + + /// Metadata we set ourselves, on sessions and on subscriptions created at checkout. + private static bool HasAppMetadata(object? data) => + data is IHasMetadata { Metadata: { } metadata } + && metadata.TryGetValue(AppMetadataKey, out var app) + && app == AppMetadataValue; + + /// + /// The customer this event concerns, for the event types we handle. Stripe.net has no common + /// interface for it, so the types are listed explicitly; anything else is not ours to judge. + /// + private static string? GetCustomerId(object? data) => data switch + { + Stripe.Checkout.Session session => session.CustomerId, + Subscription subscription => subscription.CustomerId, + Invoice invoice => invoice.CustomerId, + _ => null, + }; + + private static async Task IsKnownCustomerAsync(object? data, PatchNotesDbContext db) + { + var customerId = GetCustomerId(data); + return !string.IsNullOrEmpty(customerId) + && await db.Users.AnyAsync(u => u.StripeCustomerId == customerId); + } + + /// + /// Re-reads the subscription from Stripe. Snapshot payloads are eventually consistent and can + /// arrive out of order, so the payload identifies the subscription but never decides its state. + /// + private static Task FetchCurrentAsync(Subscription payload) => + new SubscriptionService().GetAsync(payload.Id); + + /// + /// True when the event concerns a subscription the user has already replaced. Re-fetching does + /// not catch this on its own: a cancelled subscription still reads back as cancelled, so a + /// delayed "deleted" for a previous subscription would cancel the current one. + /// + private static bool IsSupersededSubscription(User user, Subscription payload, ILogger logger) + { + if (string.IsNullOrEmpty(user.StripeSubscriptionId) + || user.StripeSubscriptionId == payload.Id) + { + return false; + } + + logger.LogInformation( + "Ignoring event for superseded subscription {EventSubscriptionId}; user is on {CurrentSubscriptionId}", + payload.Id, user.StripeSubscriptionId); + return true; + } + private static async Task HandleCheckoutSessionCompleted(Event stripeEvent, PatchNotesDbContext db, ILogger logger) { var session = stripeEvent.Data.Object as Stripe.Checkout.Session; if (session == null) return; - // Get the Stytch user ID from session metadata - if (!session.Metadata.TryGetValue("stytch_user_id", out var stytchUserId)) + // Get the Stytch user ID from session metadata. Metadata is not guaranteed here: a + // session now reaches this handler when it merely belongs to a customer we know, so the + // null check the ownership filter used to provide has to live at the point of use. + if (session.Metadata is not { } metadata + || !metadata.TryGetValue("stytch_user_id", out var stytchUserId)) { - logger.LogWarning("Checkout session completed but no stytch_user_id in metadata"); + logger.LogWarning( + "Checkout session {SessionId} completed with no stytch_user_id in metadata", + session.Id); return; } @@ -163,16 +226,18 @@ private static async Task HandleCheckoutSessionCompleted(Event stripeEvent, Patc private static async Task HandleSubscriptionUpdated(Event stripeEvent, PatchNotesDbContext db, ILogger logger) { - var subscription = stripeEvent.Data.Object as Subscription; - if (subscription == null) return; + if (stripeEvent.Data.Object is not Subscription payload) return; - var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == subscription.CustomerId); + var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == payload.CustomerId); if (user == null) { - logger.LogWarning("User not found for Stripe customer: {CustomerId}", subscription.CustomerId); + logger.LogWarning("User not found for Stripe customer: {CustomerId}", payload.CustomerId); return; } + if (IsSupersededSubscription(user, payload, logger)) return; + + var subscription = await FetchCurrentAsync(payload); user.StripeSubscriptionId = subscription.Id; user.SubscriptionStatus = subscription.Status; user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd; @@ -183,16 +248,18 @@ private static async Task HandleSubscriptionUpdated(Event stripeEvent, PatchNote private static async Task HandleSubscriptionDeleted(Event stripeEvent, PatchNotesDbContext db, ILogger logger) { - var subscription = stripeEvent.Data.Object as Subscription; - if (subscription == null) return; + if (stripeEvent.Data.Object is not Subscription payload) return; - var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == subscription.CustomerId); + var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == payload.CustomerId); if (user == null) { - logger.LogWarning("User not found for Stripe customer: {CustomerId}", subscription.CustomerId); + logger.LogWarning("User not found for Stripe customer: {CustomerId}", payload.CustomerId); return; } + if (IsSupersededSubscription(user, payload, logger)) return; + + var subscription = await FetchCurrentAsync(payload); user.SubscriptionStatus = "canceled"; // Keep the expiration date so user has access until end of paid period user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd; diff --git a/PatchNotes.Tests/StripeWebhookTests.cs b/PatchNotes.Tests/StripeWebhookTests.cs new file mode 100644 index 00000000..4c595a44 --- /dev/null +++ b/PatchNotes.Tests/StripeWebhookTests.cs @@ -0,0 +1,281 @@ +using System.Net; +using System.Security.Cryptography; +using System.Text; +using FluentAssertions; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.DependencyInjection; +using PatchNotes.Data; +using Stripe; + +namespace PatchNotes.Tests; + +public class StripeWebhookTests : IAsyncLifetime +{ + private const string TestWebhookSecret = "whsec_testsecretvalueforsigning"; + private const string KnownCustomerId = "cus_known123"; + + private PatchNotesApiFixture _fixture = null!; + private HttpClient _client = null!; + + public async Task InitializeAsync() + { + _fixture = new PatchNotesApiFixture(); + _fixture.ConfigureSettings(builder => + { + builder.UseSetting("Stripe:WebhookSecret", TestWebhookSecret); + }); + await _fixture.InitializeAsync(); + _client = _fixture.CreateClient(); + } + + public async Task DisposeAsync() + { + _client.Dispose(); + await _fixture.DisposeAsync(); + _fixture.Dispose(); + } + + #region Event ownership + + [Fact] + public async Task HandleWebhook_GivenInvoiceForKnownCustomer_IsProcessed() + { + // Invoices never carry our "app" metadata — Stripe does not propagate it from the + // Checkout Session or the subscription. They have to be recognised by their customer, + // or every renewal and payment failure is silently dropped. + await SeedUserAsync(KnownCustomerId, status: "active"); + + var response = await PostEventAsync("invoice.payment_failed", $$""" + { + "id": "in_test1", + "object": "invoice", + "customer": "{{KnownCustomerId}}", + "metadata": {} + } + """); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + (await response.Content.ReadAsStringAsync()).Should().NotContain("ignored"); + (await GetUserAsync(KnownCustomerId))!.SubscriptionStatus.Should().Be("past_due"); + } + + [Fact] + public async Task HandleWebhook_GivenInvoiceForUnknownCustomer_IsIgnored() + { + await SeedUserAsync(KnownCustomerId, status: "active"); + + var response = await PostEventAsync("invoice.payment_failed", """ + { + "id": "in_test2", + "object": "invoice", + "customer": "cus_someoneelse", + "metadata": {} + } + """); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + (await response.Content.ReadAsStringAsync()).Should().Contain("ignored"); + (await GetUserAsync(KnownCustomerId))!.SubscriptionStatus.Should().Be("active"); + + // An ignored event is not recorded, so it stays eligible if we later learn the customer. + using var scope = _fixture.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + db.ProcessedWebhookEvents.Count().Should().Be(0); + } + + [Fact] + public async Task HandleWebhook_GivenAppMetadataButUnknownCustomer_IsProcessed() + { + // The metadata path has to keep working on its own: a checkout session arrives before + // we have stored a customer id for that user. + await SeedUserAsync(KnownCustomerId, status: "active"); + + var response = await PostEventAsync("invoice.payment_failed", $$""" + { + "id": "in_test3", + "object": "invoice", + "customer": "{{KnownCustomerId}}", + "metadata": { "app": "patchnotes" } + } + """); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + (await response.Content.ReadAsStringAsync()).Should().NotContain("ignored"); + } + + [Fact] + public async Task HandleWebhook_GivenForeignAppMetadata_IsIgnored() + { + var response = await PostEventAsync("invoice.payment_failed", """ + { + "id": "in_test4", + "object": "invoice", + "customer": "cus_othertenant", + "metadata": { "app": "someoneelse" } + } + """); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + (await response.Content.ReadAsStringAsync()).Should().Contain("ignored"); + } + + #endregion + + #region Stale and superseded events + + [Fact] + public async Task HandleWebhook_GivenDeleteForSupersededSubscription_LeavesUserAlone() + { + // A delayed "deleted" for a subscription the user already replaced must not cancel the + // current one. Re-fetching cannot catch this: the old subscription really is cancelled. + await SeedUserAsync(KnownCustomerId, status: "active", subscriptionId: "sub_current"); + + var response = await PostEventAsync("customer.subscription.deleted", $$""" + { + "id": "sub_old", + "object": "subscription", + "customer": "{{KnownCustomerId}}", + "metadata": {}, + "items": { "object": "list", "data": [] } + } + """); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + var user = await GetUserAsync(KnownCustomerId); + user!.SubscriptionStatus.Should().Be("active"); + user.StripeSubscriptionId.Should().Be("sub_current"); + + // Recorded, not ignored: the event was recognised as ours and deliberately declined. + // Without this the test would also pass on an implementation that drops every + // subscription event before it reaches a handler. + using var scope = _fixture.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + db.ProcessedWebhookEvents.Count().Should().Be(1); + } + + [Fact] + public async Task HandleWebhook_GivenSessionWithoutMetadata_IsHandledNotCrashed() + { + // Recognising events by customer means a session with no metadata at all now reaches the + // handler, where the old filter would have dropped it first. Reading metadata there has + // to tolerate its absence, or Stripe retries a 500 until the endpoint is disabled. + await SeedUserAsync(KnownCustomerId, status: "active"); + + var response = await PostEventAsync("checkout.session.completed", $$""" + { + "id": "cs_nometa", + "object": "checkout.session", + "customer": "{{KnownCustomerId}}" + } + """); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + (await GetUserAsync(KnownCustomerId))!.SubscriptionStatus.Should().Be("active"); + } + + #endregion + + #region Transport + + [Fact] + public async Task HandleWebhook_GivenInvalidSignature_ReturnsBadRequest() + { + var request = new HttpRequestMessage(HttpMethod.Post, "/webhooks/stripe") + { + Content = new StringContent("{}", Encoding.UTF8, "application/json"), + }; + request.Headers.TryAddWithoutValidation("Stripe-Signature", "t=123,v1=deadbeef"); + + var response = await _client.SendAsync(request); + + response.StatusCode.Should().Be(HttpStatusCode.BadRequest); + } + + [Fact] + public async Task HandleWebhook_GivenDuplicateEventId_ReturnsDuplicateFlag() + { + await SeedUserAsync(KnownCustomerId, status: "active"); + var data = $$""" + { + "id": "in_dup", + "object": "invoice", + "customer": "{{KnownCustomerId}}", + "metadata": {} + } + """; + + var first = await PostEventAsync("invoice.payment_failed", data, eventId: "evt_dup"); + first.StatusCode.Should().Be(HttpStatusCode.OK); + + var second = await PostEventAsync("invoice.payment_failed", data, eventId: "evt_dup"); + + second.StatusCode.Should().Be(HttpStatusCode.OK); + (await second.Content.ReadAsStringAsync()).Should().Contain("duplicate"); + } + + #endregion + + #region Helpers + + private Task PostEventAsync( + string type, string dataObject, string? eventId = null) + { + // api_version has to match the SDK's, because ConstructEvent is called with + // throwOnApiVersionMismatch: true. Reading it at runtime keeps these tests working + // across SDK upgrades instead of pinning a version string that goes stale. + var body = $$""" + { + "id": "{{eventId ?? "evt_" + Guid.NewGuid().ToString("N")[..16]}}", + "object": "event", + "api_version": "{{StripeConfiguration.ApiVersion}}", + "created": {{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}}, + "type": "{{type}}", + "livemode": false, + "pending_webhooks": 0, + "request": { "id": null, "idempotency_key": null }, + "data": { "object": {{dataObject}} } + } + """; + + var request = new HttpRequestMessage(HttpMethod.Post, "/webhooks/stripe") + { + Content = new StringContent(body, Encoding.UTF8, "application/json"), + }; + request.Headers.TryAddWithoutValidation("Stripe-Signature", Sign(body)); + return _client.SendAsync(request); + } + + private static string Sign(string body) + { + var timestamp = DateTimeOffset.UtcNow.ToUnixTimeSeconds().ToString(); + using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(TestWebhookSecret)); + var hash = hmac.ComputeHash(Encoding.UTF8.GetBytes($"{timestamp}.{body}")); + return $"t={timestamp},v1={Convert.ToHexString(hash).ToLowerInvariant()}"; + } + + private async Task SeedUserAsync( + string customerId, string status, string? subscriptionId = null) + { + using var scope = _fixture.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + db.Users.Add(new User + { + StytchUserId = "stytch_" + customerId, + Email = $"{customerId}@example.com", + StripeCustomerId = customerId, + StripeSubscriptionId = subscriptionId, + SubscriptionStatus = status, + }); + await db.SaveChangesAsync(); + } + + private async Task GetUserAsync(string customerId) + { + using var scope = _fixture.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + return await Task.FromResult( + db.Users.FirstOrDefault(u => u.StripeCustomerId == customerId)); + } + + #endregion +} From 7713f2352ace687a7bd4a3349c6b38c1105fc073 Mon Sep 17 00:00:00 2001 From: Paul van Brenk <5273975+paulvanbrenk@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:39:29 -0400 Subject: [PATCH 2/3] fix(api): let each Stripe handler admit its own events Review feedback on the ownership filter, which was widened to "carries our app tag OR belongs to a customer we store". The second half is a weaker signal -- a stored customer is not proof an event concerns us -- and worse, it kept the shape of a gate, so handlers went on assuming admission had been decided for them when it had not. Several bugs followed from that. The global check is now only a negative one: reject events explicitly tagged for another app. Everything else reaches a handler, and each handler resolves its own subject and returns without writing when it cannot. Fixed along the way, all reachable in production: - A checkout session with no metadata reached an unguarded session.Metadata.TryGetValue, threw, returned 500, and Stripe retried until it would have disabled the endpoint. - Events were recorded in ProcessedWebhookEvents even when no handler applied them. Since subscription events can arrive before checkout.session.completed has stored a customer id, an event could be consumed on first delivery and never be replayable. Only applied events are recorded now, and subscription events resolve their user by stytch_user_id from subscription_data.metadata when the customer id is not stored yet. - The superseded-subscription guard treated any id mismatch as "older", but the stored id is not a high-water mark: only checkout advances it and cancellation leaves it pointing at the dead subscription. A resubscribe was therefore misfiled as stale, stranding the account on the cancelled subscription. Adoption now keys off the subscription's own status. - The guard covered two of the four handlers that write subscription state. A retried invoice for a replaced subscription could flip a paying customer to past_due, or rewind SubscriptionExpiresAt to the old subscription's period end. Both invoice handlers now check the invoice belongs to the subscription the user is actually on. - Writing the expiry unconditionally set it to null whenever Items came back empty. IsPro reads null as "no paid period remaining", so that ended a cancelled user's access immediately rather than at period end. - The delete handler no longer re-fetches. The status is known and the period end is in the signature-verified payload, so fetching only made recording a cancellation depend on Stripe being reachable. The metadata test asserted a claim it did not test: it used a seeded customer, so it passed through the customer branch and stayed green with the metadata check deleted. ShouldAdopt and IsForCurrentSubscription are internal and unit-tested directly; both sit behind a Stripe API call, so the endpoint cannot reach them without live network. Each fix above was verified by reverting it and confirming the matching test fails. Co-Authored-By: Claude Opus 5 (1M context) --- PatchNotes.Api/PatchNotes.Api.csproj | 6 + PatchNotes.Api/Webhooks/StripeWebhook.cs | 294 ++++++++++------- PatchNotes.Tests/StripeWebhookTests.cs | 388 ++++++++++++++++------- 3 files changed, 474 insertions(+), 214 deletions(-) diff --git a/PatchNotes.Api/PatchNotes.Api.csproj b/PatchNotes.Api/PatchNotes.Api.csproj index d5c7a7d7..c8da4948 100644 --- a/PatchNotes.Api/PatchNotes.Api.csproj +++ b/PatchNotes.Api/PatchNotes.Api.csproj @@ -6,6 +6,12 @@ enable + + + + + diff --git a/PatchNotes.Api/Webhooks/StripeWebhook.cs b/PatchNotes.Api/Webhooks/StripeWebhook.cs index ebbaec99..63603ae6 100644 --- a/PatchNotes.Api/Webhooks/StripeWebhook.cs +++ b/PatchNotes.Api/Webhooks/StripeWebhook.cs @@ -52,65 +52,68 @@ public static WebApplication MapStripeWebhook(this WebApplication app) return Results.Ok(new { received = true, duplicate = true }); } - // Filter events to only those for our app. Two checks, because neither alone - // covers every event we handle: + // The only global check is a negative one: reject what is provably another app's. // - // - Metadata. Stripe never copies a Checkout Session's own metadata onto the - // subscription it creates, so only sessions carried the "app" tag until - // SubscriptionData.Metadata was added at checkout. Subscriptions created since - // then carry it too, which makes them recognisable no matter what order Stripe - // delivers the events in. - // - Known customer. Invoices never carry the tag, and subscriptions created before - // that change never will. Those are matched by the customer we already stored. - // - // Anything we cannot place is acknowledged and dropped. - if (!HasAppMetadata(stripeEvent.Data.Object) - && !await IsKnownCustomerAsync(stripeEvent.Data.Object, db)) + // There is deliberately no positive "this looks like ours" gate. Metadata alone misses + // invoices and any subscription predating subscription_data.metadata, and widening it + // with "the customer exists in our Users table" trades a strong signal for a weak one + // -- a stored customer is not proof the event concerns us -- while giving handlers a + // false assurance that admission had already been decided for them. Each handler below + // resolves its own subject and returns false without writing when it cannot. + if (IsTaggedForAnotherApp(stripeEvent.Data.Object)) { logger.LogInformation( - "Ignoring Stripe event {EventId} ({EventType}): not associated with this app", + "Ignoring Stripe event {EventId} ({EventType}): tagged for another app", stripeEvent.Id, stripeEvent.Type); return Results.Ok(new { received = true, ignored = true }); } try { + bool applied; switch (stripeEvent.Type) { case "checkout.session.completed": - await HandleCheckoutSessionCompleted(stripeEvent, db, logger); + applied = await HandleCheckoutSessionCompleted(stripeEvent, db, logger); break; case "customer.subscription.updated": - await HandleSubscriptionUpdated(stripeEvent, db, logger); + applied = await HandleSubscriptionUpdated(stripeEvent, db, logger); break; case "customer.subscription.deleted": - await HandleSubscriptionDeleted(stripeEvent, db, logger); + applied = await HandleSubscriptionDeleted(stripeEvent, db, logger); break; case "invoice.payment_failed": - await HandlePaymentFailed(stripeEvent, db, logger); + applied = await HandlePaymentFailed(stripeEvent, db, logger); break; case "invoice.payment_succeeded": - await HandlePaymentSucceeded(stripeEvent, db, logger); + applied = await HandlePaymentSucceeded(stripeEvent, db, logger); break; default: logger.LogInformation("Unhandled Stripe event type: {EventType}", stripeEvent.Type); + applied = false; break; } - // Record event as processed for idempotency - db.ProcessedWebhookEvents.Add(new ProcessedWebhookEvent + // Only events that actually changed something are recorded. This table means "do + // not process this again", and claiming that for an event no handler applied burns + // it permanently: a subscription event that arrives before we can resolve its user + // would be consumed on first delivery and could never be replayed. + if (applied) { - EventId = stripeEvent.Id, - ProcessedAt = DateTimeOffset.UtcNow - }); - await db.SaveChangesAsync(); + db.ProcessedWebhookEvents.Add(new ProcessedWebhookEvent + { + EventId = stripeEvent.Id, + ProcessedAt = DateTimeOffset.UtcNow + }); + await db.SaveChangesAsync(); + } - return Results.Ok(new { received = true }); + return Results.Ok(new { received = true, applied }); } catch (StripeException ex) { @@ -130,80 +133,140 @@ public static WebApplication MapStripeWebhook(this WebApplication app) private const string AppMetadataKey = "app"; private const string AppMetadataValue = "patchnotes"; + private const string StytchUserIdMetadataKey = "stytch_user_id"; + + /// + /// Subscription statuses that mean the customer is actually on this subscription. Anything + /// outside this set is history, which is what distinguishes a replacement subscription from a + /// stale echo of a previous one. + /// + private static readonly HashSet LiveSubscriptionStatuses = + new(StringComparer.Ordinal) { "active", "trialing", "past_due", "unpaid", "incomplete" }; - /// Metadata we set ourselves, on sessions and on subscriptions created at checkout. - private static bool HasAppMetadata(object? data) => + /// + /// True only when the event carries an explicit app tag naming someone else. An absent tag is + /// not evidence either way -- invoices never carry one -- so it is not treated as rejection. + /// + private static bool IsTaggedForAnotherApp(object? data) => data is IHasMetadata { Metadata: { } metadata } && metadata.TryGetValue(AppMetadataKey, out var app) - && app == AppMetadataValue; + && app != AppMetadataValue; /// - /// The customer this event concerns, for the event types we handle. Stripe.net has no common - /// interface for it, so the types are listed explicitly; anything else is not ours to judge. + /// Re-reads the subscription from Stripe. Snapshot payloads are eventually consistent, so the + /// payload identifies the subscription but never decides its state. /// - private static string? GetCustomerId(object? data) => data switch - { - Stripe.Checkout.Session session => session.CustomerId, - Subscription subscription => subscription.CustomerId, - Invoice invoice => invoice.CustomerId, - _ => null, - }; + private static Task FetchCurrentAsync(Subscription payload) => + new SubscriptionService().GetAsync(payload.Id); - private static async Task IsKnownCustomerAsync(object? data, PatchNotesDbContext db) + /// + /// The user a subscription event concerns. The customer id is the usual route, but Stripe can + /// deliver subscription events before checkout.session.completed has stored one, so the + /// stytch_user_id written to subscription_data.metadata at checkout is the fallback. + /// + private static async Task ResolveUserAsync(Subscription payload, PatchNotesDbContext db) { - var customerId = GetCustomerId(data); - return !string.IsNullOrEmpty(customerId) - && await db.Users.AnyAsync(u => u.StripeCustomerId == customerId); + if (!string.IsNullOrEmpty(payload.CustomerId)) + { + var byCustomer = await db.Users + .FirstOrDefaultAsync(u => u.StripeCustomerId == payload.CustomerId); + if (byCustomer != null) + { + return byCustomer; + } + } + + if (payload.Metadata is { } metadata + && metadata.TryGetValue(StytchUserIdMetadataKey, out var stytchUserId) + && !string.IsNullOrEmpty(stytchUserId)) + { + return await db.Users.FirstOrDefaultAsync(u => u.StytchUserId == stytchUserId); + } + + return null; } /// - /// Re-reads the subscription from Stripe. Snapshot payloads are eventually consistent and can - /// arrive out of order, so the payload identifies the subscription but never decides its state. + /// Whether an event about this subscription may write the user's subscription state. + /// + /// The stored id is not a high-water mark: only checkout advances it and cancellation leaves it + /// pointing at the dead subscription, so "a different id" cannot be read as "an older one". A + /// resubscribe would be misfiled as stale on that reading. The subscription's own status is the + /// signal that works -- a live subscription is the one the customer is on. /// - private static Task FetchCurrentAsync(Subscription payload) => - new SubscriptionService().GetAsync(payload.Id); + internal static bool ShouldAdopt(User user, Subscription subscription, ILogger logger) + { + if (string.IsNullOrEmpty(user.StripeSubscriptionId) + || user.StripeSubscriptionId == subscription.Id + || LiveSubscriptionStatuses.Contains(subscription.Status)) + { + return true; + } + + logger.LogInformation( + "Ignoring {Status} subscription {EventSubscriptionId}; user is on {CurrentSubscriptionId}", + subscription.Status, subscription.Id, user.StripeSubscriptionId); + return false; + } /// - /// True when the event concerns a subscription the user has already replaced. Re-fetching does - /// not catch this on its own: a cancelled subscription still reads back as cancelled, so a - /// delayed "deleted" for a previous subscription would cancel the current one. + /// Whether an invoice belongs to the subscription the user is currently on. Stripe retries a + /// failed invoice for weeks, so a replaced subscription keeps producing invoice events long + /// after the customer resubscribed; acting on those reports dunning for an account in good + /// standing, or rewinds the paid period to the old subscription's. An invoice with no + /// subscription is not ours to act on either. /// - private static bool IsSupersededSubscription(User user, Subscription payload, ILogger logger) + internal static bool IsForCurrentSubscription( + User user, Invoice invoice, ILogger logger, string action) { - if (string.IsNullOrEmpty(user.StripeSubscriptionId) - || user.StripeSubscriptionId == payload.Id) + var invoiceSubscriptionId = invoice.Parent?.SubscriptionDetails?.SubscriptionId; + + if (!string.IsNullOrEmpty(invoiceSubscriptionId) + && invoiceSubscriptionId == user.StripeSubscriptionId) { - return false; + return true; } logger.LogInformation( - "Ignoring event for superseded subscription {EventSubscriptionId}; user is on {CurrentSubscriptionId}", - payload.Id, user.StripeSubscriptionId); - return true; + "Ignoring {Action} for invoice {InvoiceId} on subscription {InvoiceSubscriptionId}; user is on {CurrentSubscriptionId}", + action, invoice.Id, invoiceSubscriptionId ?? "(none)", user.StripeSubscriptionId ?? "(none)"); + return false; } - private static async Task HandleCheckoutSessionCompleted(Event stripeEvent, PatchNotesDbContext db, ILogger logger) + /// + /// Copies subscription state onto the user. The expiry is overwritten only when the + /// subscription carries one: Items can come back empty, and reads a + /// null expiry as "no paid period remaining", so writing null ends access immediately. + /// + private static void ApplySubscription(User user, Subscription subscription) { - var session = stripeEvent.Data.Object as Stripe.Checkout.Session; - if (session == null) return; + user.StripeSubscriptionId = subscription.Id; + user.SubscriptionStatus = subscription.Status; + user.SubscriptionExpiresAt = + subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd ?? user.SubscriptionExpiresAt; + } + + private static async Task HandleCheckoutSessionCompleted( + Event stripeEvent, PatchNotesDbContext db, ILogger logger) + { + if (stripeEvent.Data.Object is not Stripe.Checkout.Session session) return false; - // Get the Stytch user ID from session metadata. Metadata is not guaranteed here: a - // session now reaches this handler when it merely belongs to a customer we know, so the - // null check the ownership filter used to provide has to live at the point of use. + // Metadata is not guaranteed: nothing upstream vets it, so the check lives at the point + // of use rather than being assumed from an admission filter. if (session.Metadata is not { } metadata - || !metadata.TryGetValue("stytch_user_id", out var stytchUserId)) + || !metadata.TryGetValue(StytchUserIdMetadataKey, out var stytchUserId)) { logger.LogWarning( "Checkout session {SessionId} completed with no stytch_user_id in metadata", session.Id); - return; + return false; } var user = await db.Users.FirstOrDefaultAsync(u => u.StytchUserId == stytchUserId); if (user == null) { logger.LogWarning("User not found for Stytch ID: {StytchUserId}", stytchUserId); - return; + return false; } // Update user with Stripe customer ID @@ -212,104 +275,121 @@ private static async Task HandleCheckoutSessionCompleted(Event stripeEvent, Patc // Fetch the subscription to get status and period end if (!string.IsNullOrEmpty(session.SubscriptionId)) { - var subscriptionService = new SubscriptionService(); - var subscription = await subscriptionService.GetAsync(session.SubscriptionId); - - user.StripeSubscriptionId = subscription.Id; - user.SubscriptionStatus = subscription.Status; - user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd; + var subscription = await new SubscriptionService().GetAsync(session.SubscriptionId); + ApplySubscription(user, subscription); } await db.SaveChangesAsync(); logger.LogInformation("Updated subscription for user {StytchUserId}: status={Status}", stytchUserId, user.SubscriptionStatus); + return true; } - private static async Task HandleSubscriptionUpdated(Event stripeEvent, PatchNotesDbContext db, ILogger logger) + private static async Task HandleSubscriptionUpdated( + Event stripeEvent, PatchNotesDbContext db, ILogger logger) { - if (stripeEvent.Data.Object is not Subscription payload) return; + if (stripeEvent.Data.Object is not Subscription payload) return false; - var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == payload.CustomerId); + var user = await ResolveUserAsync(payload, db); if (user == null) { logger.LogWarning("User not found for Stripe customer: {CustomerId}", payload.CustomerId); - return; + return false; } - if (IsSupersededSubscription(user, payload, logger)) return; - var subscription = await FetchCurrentAsync(payload); - user.StripeSubscriptionId = subscription.Id; - user.SubscriptionStatus = subscription.Status; - user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd; + if (!ShouldAdopt(user, subscription, logger)) return false; + + // Reached via subscription metadata when checkout has not landed yet, so the customer id + // may still be missing. + if (string.IsNullOrEmpty(user.StripeCustomerId)) + { + user.StripeCustomerId = subscription.CustomerId; + } + + ApplySubscription(user, subscription); await db.SaveChangesAsync(); logger.LogInformation("Updated subscription for customer {CustomerId}: status={Status}", subscription.CustomerId, subscription.Status); + return true; } - private static async Task HandleSubscriptionDeleted(Event stripeEvent, PatchNotesDbContext db, ILogger logger) + private static async Task HandleSubscriptionDeleted( + Event stripeEvent, PatchNotesDbContext db, ILogger logger) { - if (stripeEvent.Data.Object is not Subscription payload) return; + if (stripeEvent.Data.Object is not Subscription payload) return false; - var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == payload.CustomerId); + var user = await ResolveUserAsync(payload, db); if (user == null) { logger.LogWarning("User not found for Stripe customer: {CustomerId}", payload.CustomerId); - return; + return false; } - if (IsSupersededSubscription(user, payload, logger)) return; + // A cancellation only concerns the subscription the user is actually on. + if (!string.IsNullOrEmpty(user.StripeSubscriptionId) + && user.StripeSubscriptionId != payload.Id) + { + logger.LogInformation( + "Ignoring cancellation of subscription {EventSubscriptionId}; user is on {CurrentSubscriptionId}", + payload.Id, user.StripeSubscriptionId); + return false; + } - var subscription = await FetchCurrentAsync(payload); + // No re-fetch here. The status is already known, and the period end is in the payload that + // was signature-verified, so re-reading would only add a dependency on Stripe being + // reachable for a cancellation to be recorded at all. user.SubscriptionStatus = "canceled"; // Keep the expiration date so user has access until end of paid period - user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd; + user.SubscriptionExpiresAt = + payload.Items.Data.FirstOrDefault()?.CurrentPeriodEnd ?? user.SubscriptionExpiresAt; await db.SaveChangesAsync(); - logger.LogInformation("Subscription canceled for customer {CustomerId}", subscription.CustomerId); + logger.LogInformation("Subscription canceled for customer {CustomerId}", payload.CustomerId); + return true; } - private static async Task HandlePaymentFailed(Event stripeEvent, PatchNotesDbContext db, ILogger logger) + private static async Task HandlePaymentFailed( + Event stripeEvent, PatchNotesDbContext db, ILogger logger) { - var invoice = stripeEvent.Data.Object as Invoice; - if (invoice == null) return; + if (stripeEvent.Data.Object is not Invoice invoice) return false; var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == invoice.CustomerId); if (user == null) { logger.LogWarning("User not found for Stripe customer: {CustomerId}", invoice.CustomerId); - return; + return false; } + if (!IsForCurrentSubscription(user, invoice, logger, "payment failure")) return false; + user.SubscriptionStatus = "past_due"; await db.SaveChangesAsync(); logger.LogWarning("Payment failed for customer {CustomerId}, marked as past_due", invoice.CustomerId); + return true; } - private static async Task HandlePaymentSucceeded(Event stripeEvent, PatchNotesDbContext db, ILogger logger) + private static async Task HandlePaymentSucceeded( + Event stripeEvent, PatchNotesDbContext db, ILogger logger) { - var invoice = stripeEvent.Data.Object as Invoice; - if (invoice == null) return; + if (stripeEvent.Data.Object is not Invoice invoice) return false; var user = await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == invoice.CustomerId); if (user == null) { logger.LogWarning("User not found for Stripe customer: {CustomerId}", invoice.CustomerId); - return; + return false; } - // Update subscription expiry on successful renewal payment - var invoiceSubscriptionId = invoice.Parent?.SubscriptionDetails?.SubscriptionId; - if (!string.IsNullOrEmpty(invoiceSubscriptionId)) - { - var subscriptionService = new SubscriptionService(); - var subscription = await subscriptionService.GetAsync(invoiceSubscriptionId); + if (!IsForCurrentSubscription(user, invoice, logger, "payment success")) return false; - user.SubscriptionStatus = subscription.Status; - user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd; + // Update subscription expiry on successful renewal payment + var invoiceSubscriptionId = invoice.Parent!.SubscriptionDetails!.SubscriptionId; + var subscription = await new SubscriptionService().GetAsync(invoiceSubscriptionId); + ApplySubscription(user, subscription); - await db.SaveChangesAsync(); - logger.LogInformation("Payment succeeded for customer {CustomerId}, updated expiry to {ExpiresAt}", invoice.CustomerId, user.SubscriptionExpiresAt); - } + await db.SaveChangesAsync(); + logger.LogInformation("Payment succeeded for customer {CustomerId}, updated expiry to {ExpiresAt}", invoice.CustomerId, user.SubscriptionExpiresAt); + return true; } } diff --git a/PatchNotes.Tests/StripeWebhookTests.cs b/PatchNotes.Tests/StripeWebhookTests.cs index 4c595a44..cbb21baf 100644 --- a/PatchNotes.Tests/StripeWebhookTests.cs +++ b/PatchNotes.Tests/StripeWebhookTests.cs @@ -3,7 +3,10 @@ using System.Text; using FluentAssertions; using Microsoft.AspNetCore.Hosting; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using PatchNotes.Api.Webhooks; using PatchNotes.Data; using Stripe; @@ -13,6 +16,10 @@ public class StripeWebhookTests : IAsyncLifetime { private const string TestWebhookSecret = "whsec_testsecretvalueforsigning"; private const string KnownCustomerId = "cus_known123"; + private const string CurrentSubscriptionId = "sub_current"; + private const long PeriodEndUnix = 1798761600; // 2027-01-01T00:00:00Z + private static readonly DateTimeOffset PeriodEnd = + DateTimeOffset.FromUnixTimeSeconds(PeriodEndUnix); private PatchNotesApiFixture _fixture = null!; private HttpClient _client = null!; @@ -35,131 +42,170 @@ public async Task DisposeAsync() _fixture.Dispose(); } - #region Event ownership + #region Admission [Fact] - public async Task HandleWebhook_GivenInvoiceForKnownCustomer_IsProcessed() + public async Task HandleWebhook_GivenUntaggedInvoiceForCurrentSubscription_IsApplied() { - // Invoices never carry our "app" metadata — Stripe does not propagate it from the - // Checkout Session or the subscription. They have to be recognised by their customer, - // or every renewal and payment failure is silently dropped. - await SeedUserAsync(KnownCustomerId, status: "active"); + // The original bug. Invoices never carry our "app" metadata, because Stripe propagates it + // neither from the Checkout Session nor from the subscription, so dropping untagged events + // silently discarded every renewal and payment failure. + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); - var response = await PostEventAsync("invoice.payment_failed", $$""" - { - "id": "in_test1", - "object": "invoice", - "customer": "{{KnownCustomerId}}", - "metadata": {} - } - """); + var response = await PostEventAsync("invoice.payment_failed", InvoiceJson()); response.StatusCode.Should().Be(HttpStatusCode.OK); - (await response.Content.ReadAsStringAsync()).Should().NotContain("ignored"); - (await GetUserAsync(KnownCustomerId))!.SubscriptionStatus.Should().Be("past_due"); + (await response.Content.ReadAsStringAsync()).Should().Contain("\"applied\":true"); + (await GetUserAsync())!.SubscriptionStatus.Should().Be("past_due"); } [Fact] - public async Task HandleWebhook_GivenInvoiceForUnknownCustomer_IsIgnored() + public async Task HandleWebhook_GivenForeignAppTag_IsIgnoredBeforeAnyHandler() { - await SeedUserAsync(KnownCustomerId, status: "active"); + // The only global check left, and the only one that is actual evidence: an explicit tag + // naming a different app. + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); - var response = await PostEventAsync("invoice.payment_failed", """ - { - "id": "in_test2", - "object": "invoice", - "customer": "cus_someoneelse", - "metadata": {} - } - """); + var response = await PostEventAsync("invoice.payment_failed", + InvoiceJson(metadata: """{ "app": "someoneelse" }""")); response.StatusCode.Should().Be(HttpStatusCode.OK); (await response.Content.ReadAsStringAsync()).Should().Contain("ignored"); - (await GetUserAsync(KnownCustomerId))!.SubscriptionStatus.Should().Be("active"); + (await GetUserAsync())!.SubscriptionStatus.Should().Be("active"); + } - // An ignored event is not recorded, so it stays eligible if we later learn the customer. - using var scope = _fixture.Services.CreateScope(); - var db = scope.ServiceProvider.GetRequiredService(); - db.ProcessedWebhookEvents.Count().Should().Be(0); + [Fact] + public async Task HandleWebhook_GivenInvoiceForUnknownCustomer_ChangesNothing() + { + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); + + var response = await PostEventAsync("invoice.payment_failed", + InvoiceJson(customerId: "cus_someoneelse")); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + (await response.Content.ReadAsStringAsync()).Should().Contain("\"applied\":false"); + (await GetUserAsync())!.SubscriptionStatus.Should().Be("active"); + } + + #endregion + + #region Recording only what was applied + + [Fact] + public async Task HandleWebhook_GivenUnappliedEvent_IsNotRecordedSoItStaysReplayable() + { + // A row here means "never process this again". Claiming that for an event no handler acted + // on burns it: one that arrives before its user is resolvable would be consumed on first + // delivery and could never be replayed from the dashboard. + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); + + await PostEventAsync("invoice.payment_failed", InvoiceJson(customerId: "cus_someoneelse")); + + (await CountRecordedAsync()).Should().Be(0); } [Fact] - public async Task HandleWebhook_GivenAppMetadataButUnknownCustomer_IsProcessed() + public async Task HandleWebhook_GivenAppliedEvent_IsRecorded() { - // The metadata path has to keep working on its own: a checkout session arrives before - // we have stored a customer id for that user. - await SeedUserAsync(KnownCustomerId, status: "active"); + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); - var response = await PostEventAsync("invoice.payment_failed", $$""" - { - "id": "in_test3", - "object": "invoice", - "customer": "{{KnownCustomerId}}", - "metadata": { "app": "patchnotes" } - } - """); + await PostEventAsync("invoice.payment_failed", InvoiceJson()); + + (await CountRecordedAsync()).Should().Be(1); + } + + #endregion + + #region Invoices belong to a subscription + + [Fact] + public async Task HandleWebhook_GivenInvoiceForReplacedSubscription_IsIgnored() + { + // Stripe retries a failed invoice for weeks. A customer who cancelled sub_old with an open + // invoice and resubscribed would otherwise be flipped to past_due by every retry against + // the dead subscription, reporting dunning for an account in good standing. + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); + + var response = await PostEventAsync("invoice.payment_failed", + InvoiceJson(subscriptionId: "sub_old")); response.StatusCode.Should().Be(HttpStatusCode.OK); - (await response.Content.ReadAsStringAsync()).Should().NotContain("ignored"); + (await GetUserAsync())!.SubscriptionStatus.Should().Be("active"); + (await CountRecordedAsync()).Should().Be(0); } [Fact] - public async Task HandleWebhook_GivenForeignAppMetadata_IsIgnored() + public async Task HandleWebhook_GivenInvoiceWithNoSubscription_IsIgnored() { - var response = await PostEventAsync("invoice.payment_failed", """ - { - "id": "in_test4", - "object": "invoice", - "customer": "cus_othertenant", - "metadata": { "app": "someoneelse" } - } - """); + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); + + var response = await PostEventAsync("invoice.payment_failed", + InvoiceJson(subscriptionId: null)); response.StatusCode.Should().Be(HttpStatusCode.OK); - (await response.Content.ReadAsStringAsync()).Should().Contain("ignored"); + (await GetUserAsync())!.SubscriptionStatus.Should().Be("active"); } #endregion - #region Stale and superseded events + #region Cancellation + + [Fact] + public async Task HandleWebhook_GivenDeleteForCurrentSubscription_KeepsThePaidPeriod() + { + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); + + var response = await PostEventAsync("customer.subscription.deleted", SubscriptionJson()); + + response.StatusCode.Should().Be(HttpStatusCode.OK); + var user = await GetUserAsync(); + user!.SubscriptionStatus.Should().Be("canceled"); + user.SubscriptionExpiresAt.Should().Be(PeriodEnd); + } + + [Fact] + public async Task HandleWebhook_GivenDeleteWithNoPeriodEnd_KeepsTheExistingExpiry() + { + // IsPro reads a null expiry as "no paid period remaining", so overwriting unconditionally + // would end a cancelled user's access the moment a payload came back without items. + var existing = DateTimeOffset.UtcNow.AddDays(20); + await SeedUserAsync( + status: "active", subscriptionId: CurrentSubscriptionId, expiresAt: existing); + + await PostEventAsync("customer.subscription.deleted", SubscriptionJson(items: "[]")); + + var user = await GetUserAsync(); + user!.SubscriptionStatus.Should().Be("canceled"); + user.SubscriptionExpiresAt.Should().BeCloseTo(existing, TimeSpan.FromSeconds(1)); + } [Fact] public async Task HandleWebhook_GivenDeleteForSupersededSubscription_LeavesUserAlone() { - // A delayed "deleted" for a subscription the user already replaced must not cancel the - // current one. Re-fetching cannot catch this: the old subscription really is cancelled. - await SeedUserAsync(KnownCustomerId, status: "active", subscriptionId: "sub_current"); + // A delayed cancellation for a subscription the user already replaced must not cancel the + // current one, and must stay replayable rather than being marked processed. + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); - var response = await PostEventAsync("customer.subscription.deleted", $$""" - { - "id": "sub_old", - "object": "subscription", - "customer": "{{KnownCustomerId}}", - "metadata": {}, - "items": { "object": "list", "data": [] } - } - """); + var response = await PostEventAsync("customer.subscription.deleted", + SubscriptionJson(subscriptionId: "sub_old")); response.StatusCode.Should().Be(HttpStatusCode.OK); - var user = await GetUserAsync(KnownCustomerId); + var user = await GetUserAsync(); user!.SubscriptionStatus.Should().Be("active"); - user.StripeSubscriptionId.Should().Be("sub_current"); - - // Recorded, not ignored: the event was recognised as ours and deliberately declined. - // Without this the test would also pass on an implementation that drops every - // subscription event before it reaches a handler. - using var scope = _fixture.Services.CreateScope(); - var db = scope.ServiceProvider.GetRequiredService(); - db.ProcessedWebhookEvents.Count().Should().Be(1); + user.StripeSubscriptionId.Should().Be(CurrentSubscriptionId); + (await CountRecordedAsync()).Should().Be(0); } + #endregion + + #region Checkout sessions + [Fact] public async Task HandleWebhook_GivenSessionWithoutMetadata_IsHandledNotCrashed() { - // Recognising events by customer means a session with no metadata at all now reaches the - // handler, where the old filter would have dropped it first. Reading metadata there has - // to tolerate its absence, or Stripe retries a 500 until the endpoint is disabled. - await SeedUserAsync(KnownCustomerId, status: "active"); + // Nothing upstream guarantees metadata, so the handler has to tolerate its absence. + // Throwing here returns 500 and Stripe retries until it disables the endpoint. + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); var response = await PostEventAsync("checkout.session.completed", $$""" { @@ -170,7 +216,82 @@ public async Task HandleWebhook_GivenSessionWithoutMetadata_IsHandledNotCrashed( """); response.StatusCode.Should().Be(HttpStatusCode.OK); - (await GetUserAsync(KnownCustomerId))!.SubscriptionStatus.Should().Be("active"); + (await GetUserAsync())!.SubscriptionStatus.Should().Be("active"); + } + + #endregion + + #region Admission predicates + + // ShouldAdopt and IsForCurrentSubscription decide whether an event may write user state. Both + // sit behind a Stripe API call in their handlers, so they are exercised directly rather than + // through the endpoint, which would need live network. + + [Fact] + public void ShouldAdopt_GivenReplacementSubscriptionAfterCancellation_AdoptsIt() + { + // The stored id is not a high-water mark: cancellation leaves it pointing at the dead + // subscription. Reading "different id" as "older" would misfile a resubscribe as stale and + // strand the account on the cancelled subscription. + var user = NewUser(subscriptionId: "sub_old", status: "canceled"); + + StripeWebhook.ShouldAdopt(user, new Subscription { Id = "sub_new", Status = "active" }, + NullLogger.Instance).Should().BeTrue(); + } + + [Fact] + public void ShouldAdopt_GivenStaleEchoOfAReplacedSubscription_DeclinesIt() + { + var user = NewUser(subscriptionId: CurrentSubscriptionId, status: "active"); + + StripeWebhook.ShouldAdopt(user, new Subscription { Id = "sub_old", Status = "canceled" }, + NullLogger.Instance).Should().BeFalse(); + } + + [Theory] + [InlineData("active")] + [InlineData("trialing")] + [InlineData("past_due")] + public void ShouldAdopt_GivenAnyLiveStatus_AdoptsIt(string status) + { + var user = NewUser(subscriptionId: "sub_old", status: "canceled"); + + StripeWebhook.ShouldAdopt(user, new Subscription { Id = "sub_new", Status = status }, + NullLogger.Instance).Should().BeTrue(); + } + + [Fact] + public void ShouldAdopt_GivenUserWithNoSubscriptionYet_AdoptsIt() + { + StripeWebhook.ShouldAdopt(NewUser(), new Subscription { Id = "sub_new", Status = "canceled" }, + NullLogger.Instance).Should().BeTrue(); + } + + [Fact] + public void IsForCurrentSubscription_GivenInvoiceForAnotherSubscription_IsFalse() + { + var user = NewUser(subscriptionId: CurrentSubscriptionId); + + StripeWebhook.IsForCurrentSubscription( + user, InvoiceFor("sub_old"), NullLogger.Instance, "x").Should().BeFalse(); + } + + [Fact] + public void IsForCurrentSubscription_GivenInvoiceWithNoSubscription_IsFalse() + { + var user = NewUser(subscriptionId: CurrentSubscriptionId); + + StripeWebhook.IsForCurrentSubscription( + user, new Invoice { Id = "in_x" }, NullLogger.Instance, "x").Should().BeFalse(); + } + + [Fact] + public void IsForCurrentSubscription_GivenMatchingSubscription_IsTrue() + { + var user = NewUser(subscriptionId: CurrentSubscriptionId); + + StripeWebhook.IsForCurrentSubscription( + user, InvoiceFor(CurrentSubscriptionId), NullLogger.Instance, "x").Should().BeTrue(); } #endregion @@ -194,20 +315,14 @@ public async Task HandleWebhook_GivenInvalidSignature_ReturnsBadRequest() [Fact] public async Task HandleWebhook_GivenDuplicateEventId_ReturnsDuplicateFlag() { - await SeedUserAsync(KnownCustomerId, status: "active"); - var data = $$""" - { - "id": "in_dup", - "object": "invoice", - "customer": "{{KnownCustomerId}}", - "metadata": {} - } - """; + await SeedUserAsync(status: "active", subscriptionId: CurrentSubscriptionId); - var first = await PostEventAsync("invoice.payment_failed", data, eventId: "evt_dup"); + var first = await PostEventAsync( + "invoice.payment_failed", InvoiceJson(), eventId: "evt_dup"); first.StatusCode.Should().Be(HttpStatusCode.OK); - var second = await PostEventAsync("invoice.payment_failed", data, eventId: "evt_dup"); + var second = await PostEventAsync( + "invoice.payment_failed", InvoiceJson(), eventId: "evt_dup"); second.StatusCode.Should().Be(HttpStatusCode.OK); (await second.Content.ReadAsStringAsync()).Should().Contain("duplicate"); @@ -217,12 +332,70 @@ public async Task HandleWebhook_GivenDuplicateEventId_ReturnsDuplicateFlag() #region Helpers + private static User NewUser(string? subscriptionId = null, string? status = null) => new() + { + StytchUserId = "stytch_user", + Email = "user@example.com", + StripeCustomerId = KnownCustomerId, + StripeSubscriptionId = subscriptionId, + SubscriptionStatus = status, + }; + + private static string InvoiceJson( + string customerId = KnownCustomerId, + string? subscriptionId = CurrentSubscriptionId, + string metadata = "{}") + { + var parent = subscriptionId is null + ? "null" + : $$"""{ "type": "subscription_details", "subscription_details": { "subscription": "{{subscriptionId}}" } }"""; + + return $$""" + { + "id": "in_test", + "object": "invoice", + "customer": "{{customerId}}", + "metadata": {{metadata}}, + "parent": {{parent}} + } + """; + } + + private static string SubscriptionJson( + string subscriptionId = CurrentSubscriptionId, + string? items = null) + { + items ??= $$"""[ { "id": "si_1", "object": "subscription_item", "current_period_end": {{PeriodEndUnix}} } ]"""; + + return $$""" + { + "id": "{{subscriptionId}}", + "object": "subscription", + "customer": "{{KnownCustomerId}}", + "metadata": {}, + "items": { "object": "list", "data": {{items}} } + } + """; + } + + private static Invoice InvoiceFor(string subscriptionId) => new() + { + Id = "in_x", + Parent = new InvoiceParent + { + SubscriptionDetails = new InvoiceParentSubscriptionDetails + { + SubscriptionId = subscriptionId, + }, + }, + }; + private Task PostEventAsync( string type, string dataObject, string? eventId = null) { - // api_version has to match the SDK's, because ConstructEvent is called with - // throwOnApiVersionMismatch: true. Reading it at runtime keeps these tests working - // across SDK upgrades instead of pinning a version string that goes stale. + // api_version must match the SDK's, because ConstructEvent is called with + // throwOnApiVersionMismatch: true. Reading it at runtime keeps these tests working across + // SDK upgrades instead of pinning a version string that goes stale. var body = $$""" { "id": "{{eventId ?? "evt_" + Guid.NewGuid().ToString("N")[..16]}}", @@ -254,27 +427,28 @@ private static string Sign(string body) } private async Task SeedUserAsync( - string customerId, string status, string? subscriptionId = null) + string status, string? subscriptionId = null, DateTimeOffset? expiresAt = null) { using var scope = _fixture.Services.CreateScope(); var db = scope.ServiceProvider.GetRequiredService(); - db.Users.Add(new User - { - StytchUserId = "stytch_" + customerId, - Email = $"{customerId}@example.com", - StripeCustomerId = customerId, - StripeSubscriptionId = subscriptionId, - SubscriptionStatus = status, - }); + var user = NewUser(subscriptionId, status); + user.SubscriptionExpiresAt = expiresAt; + db.Users.Add(user); await db.SaveChangesAsync(); } - private async Task GetUserAsync(string customerId) + private async Task GetUserAsync() + { + using var scope = _fixture.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + return await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == KnownCustomerId); + } + + private async Task CountRecordedAsync() { using var scope = _fixture.Services.CreateScope(); var db = scope.ServiceProvider.GetRequiredService(); - return await Task.FromResult( - db.Users.FirstOrDefault(u => u.StripeCustomerId == customerId)); + return await db.ProcessedWebhookEvents.CountAsync(); } #endregion From 8a0acef49feda94dffbe9453ca7a65c5b949c406 Mon Sep 17 00:00:00 2001 From: Paul van Brenk <5273975+paulvanbrenk@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:57:23 -0400 Subject: [PATCH 3/3] refactor(api): share the Stripe fetch and metadata, and keep tests offline Second round of review feedback on this PR. FetchCurrentAsync now takes a subscription id rather than a Subscription, so the checkout and payment-succeeded handlers use it too instead of each hand-rolling new SubscriptionService().GetAsync(...). One place to change when that service finally becomes injectable. The checkout metadata is built once and assigned to both the session and subscription_data. The tags have to agree -- that is the point of this PR -- and building the dictionary twice is how they would come to disagree. Tests point the Stripe SDK at a closed loopback port once the host is built. SubscriptionService is constructed inline and cannot be faked, so any regression that reintroduces a call would otherwise dial api.stripe.com from CI with the fixture's placeholder key: slow, network-dependent, and it reports a 503 rather than the assertion that actually broke. Verified by temporarily reintroducing a fetch: the request goes to 127.0.0.1:1 and is refused immediately. It has to be set after the host builds, because Program.cs assigns StripeConfiguration.ApiKey, which discards the client. Co-Authored-By: Claude Opus 5 (1M context) --- PatchNotes.Api/Routes/SubscriptionRoutes.cs | 23 +++++++++------------ PatchNotes.Api/Webhooks/StripeWebhook.cs | 14 ++++++------- PatchNotes.Tests/PatchNotesApiFixture.cs | 10 +++++++++ 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/PatchNotes.Api/Routes/SubscriptionRoutes.cs b/PatchNotes.Api/Routes/SubscriptionRoutes.cs index 142e5203..59cdfdd1 100644 --- a/PatchNotes.Api/Routes/SubscriptionRoutes.cs +++ b/PatchNotes.Api/Routes/SubscriptionRoutes.cs @@ -83,6 +83,14 @@ public static WebApplication MapSubscriptionRoutes(this WebApplication app) // Determine the base URL for success/cancel redirects var origin = GetValidatedOrigin(httpContext, configuration); + // One dictionary for both: the session tag and the subscription tag have to agree, + // and building it twice is how they would come to disagree. + var appMetadata = new Dictionary + { + { "stytch_user_id", stytchUserId }, + { "app", "patchnotes" }, + }; + var sessionOptions = new SessionCreateOptions { Mode = "subscription", @@ -98,22 +106,11 @@ public static WebApplication MapSubscriptionRoutes(this WebApplication app) SuccessUrl = $"{origin}/subscription-success?session_id={{CHECKOUT_SESSION_ID}}", CancelUrl = $"{origin}/subscription-canceled", CustomerEmail = user.Email, - Metadata = new Dictionary - { - { "stytch_user_id", stytchUserId }, - { "app", "patchnotes" }, - }, + Metadata = appMetadata, // Stripe does not copy the session's metadata onto the subscription it creates, // so the tag is set again here. Without it the subscription and its invoices // arrive at our webhook carrying nothing that identifies them as ours. - SubscriptionData = new SessionSubscriptionDataOptions - { - Metadata = new Dictionary - { - { "stytch_user_id", stytchUserId }, - { "app", "patchnotes" }, - }, - }, + SubscriptionData = new SessionSubscriptionDataOptions { Metadata = appMetadata }, }; // If user already has a Stripe customer ID, use it diff --git a/PatchNotes.Api/Webhooks/StripeWebhook.cs b/PatchNotes.Api/Webhooks/StripeWebhook.cs index 63603ae6..ae8725d4 100644 --- a/PatchNotes.Api/Webhooks/StripeWebhook.cs +++ b/PatchNotes.Api/Webhooks/StripeWebhook.cs @@ -156,8 +156,8 @@ private static bool IsTaggedForAnotherApp(object? data) => /// Re-reads the subscription from Stripe. Snapshot payloads are eventually consistent, so the /// payload identifies the subscription but never decides its state. /// - private static Task FetchCurrentAsync(Subscription payload) => - new SubscriptionService().GetAsync(payload.Id); + private static Task FetchCurrentAsync(string subscriptionId) => + new SubscriptionService().GetAsync(subscriptionId); /// /// The user a subscription event concerns. The customer id is the usual route, but Stripe can @@ -275,8 +275,7 @@ private static async Task HandleCheckoutSessionCompleted( // Fetch the subscription to get status and period end if (!string.IsNullOrEmpty(session.SubscriptionId)) { - var subscription = await new SubscriptionService().GetAsync(session.SubscriptionId); - ApplySubscription(user, subscription); + ApplySubscription(user, await FetchCurrentAsync(session.SubscriptionId)); } await db.SaveChangesAsync(); @@ -296,7 +295,7 @@ private static async Task HandleSubscriptionUpdated( return false; } - var subscription = await FetchCurrentAsync(payload); + var subscription = await FetchCurrentAsync(payload.Id); if (!ShouldAdopt(user, subscription, logger)) return false; // Reached via subscription metadata when checkout has not landed yet, so the customer id @@ -384,9 +383,8 @@ private static async Task HandlePaymentSucceeded( if (!IsForCurrentSubscription(user, invoice, logger, "payment success")) return false; // Update subscription expiry on successful renewal payment - var invoiceSubscriptionId = invoice.Parent!.SubscriptionDetails!.SubscriptionId; - var subscription = await new SubscriptionService().GetAsync(invoiceSubscriptionId); - ApplySubscription(user, subscription); + ApplySubscription( + user, await FetchCurrentAsync(invoice.Parent!.SubscriptionDetails!.SubscriptionId)); await db.SaveChangesAsync(); logger.LogInformation("Payment succeeded for customer {CustomerId}, updated expiry to {ExpiresAt}", invoice.CustomerId, user.SubscriptionExpiresAt); diff --git a/PatchNotes.Tests/PatchNotesApiFixture.cs b/PatchNotes.Tests/PatchNotesApiFixture.cs index 88a7d433..08db5b2e 100644 --- a/PatchNotes.Tests/PatchNotesApiFixture.cs +++ b/PatchNotes.Tests/PatchNotesApiFixture.cs @@ -1,6 +1,7 @@ using System.Data.Common; using System.Net; using Microsoft.AspNetCore.Hosting; +using Stripe; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; @@ -98,6 +99,15 @@ public async Task InitializeAsync() { // Force creation of the WebApplication and database using var scope = Services.CreateScope(); + + // Nothing under test may reach Stripe. SubscriptionService is constructed inline in the + // webhook handlers and cannot be faked, so the SDK is pointed at a closed loopback port: + // a regression that reintroduces a call then fails immediately and locally, instead of + // dialling api.stripe.com from CI and reporting a 503 in place of the real assertion. + // This runs after the host is built because Program.cs assigns StripeConfiguration.ApiKey, + // which discards any client set before it. + StripeConfiguration.StripeClient = new StripeClient( + "sk_test_placeholder", null, null, "http://127.0.0.1:1"); var db = scope.ServiceProvider.GetRequiredService(); await db.Database.EnsureCreatedAsync(); }