diff --git a/internal/handlers/billing.go b/internal/handlers/billing.go index 346c444..f92e4b8 100644 --- a/internal/handlers/billing.go +++ b/internal/handlers/billing.go @@ -189,8 +189,8 @@ func detectBillingMisconfiguration(cfgEnvironment, razorpayKeyID string) (code, // BillingHandler handles billing and Razorpay webhook endpoints. type BillingHandler struct { - db *sql.DB - cfg *config.Config + db *sql.DB + cfg *config.Config // email is the Mailer used for all webhook-triggered sends (payment // receipts, payment-failed dunning, etc.). The interface lets main.go // wrap the underlying *email.Client in a *email.BreakingClient — a @@ -1115,12 +1115,12 @@ type rzpSubscriptionEntity struct { } type rzpPaymentEntity struct { - ID string `json:"id"` - Amount int64 `json:"amount"` - Currency string `json:"currency"` - Email string `json:"email"` - AttemptCount int `json:"attempt_count"` - ErrorDescription string `json:"error_description"` + ID string `json:"id"` + Amount int64 `json:"amount"` + Currency string `json:"currency"` + Email string `json:"email"` + AttemptCount int `json:"attempt_count"` + ErrorDescription string `json:"error_description"` // SubscriptionID + OrderID + Notes (B11-P1, 2026-05-20): used to // resolve the team server-side instead of trusting payload.email // verbatim. A payment.failed entity carries `subscription_id` for @@ -2100,11 +2100,48 @@ func (h *BillingHandler) handleSubscriptionCancelled(ctx context.Context, c *fib return fmt.Errorf("subscription.cancelled team resolve: %w", err) } - // Snapshot the prior tier so the audit row can capture from→to. Failure - // to read it is non-fatal — we just emit with from_tier="". + // Snapshot the prior tier so the audit row can capture from→to, and the + // team's CURRENT live subscription id so we can reject a stale/superseded + // cancellation (see the guard below). Failure to read the team is non-fatal + // — we just emit with from_tier="" and an empty live sub id (which falls + // through to the historical always-downgrade behaviour). fromTier := "" + liveSubID := "" if team, lookupErr := models.GetTeamByID(ctx, h.db, teamID); lookupErr == nil && team != nil { fromTier = team.PlanTier + if team.RazorpaySubscriptionID.Valid { + liveSubID = strings.TrimSpace(team.RazorpaySubscriptionID.String) + } + } + + // STALE-SUB GUARD (MONEY-SENSITIVE, 2026-06-04): a cancel/halt/deauth + // webhook carries notes.team_id verbatim from WHATEVER subscription fired + // it — including a SUPERSEDED one. After a hobby→pro plan change the old + // hobby subscription stays alive in Razorpay carrying the same notes.team_id; + // its eventual cancellation (or a halt/deauth) would otherwise downgrade the + // team that is now actively paying on a DIFFERENT live Pro subscription. + // + // Mirror the charged-path lower_tier guard (handleSubscriptionCharged): if + // the webhook's sub.ID does NOT match the team's stored live subscription id + // AND that live id is non-empty, this is a stale event for an old + // subscription. Skip the downgrade entirely, log a loud WARN + a + // billing.charge_undeliverable audit row for operator reconciliation, and + // keep the higher tier. An empty live id (never stored a sub id, or a + // lookup miss above) falls through to the historical behaviour — we cannot + // prove the event is stale, and a never-paid team should still downgrade. + if liveSubID != "" && sub.ID != "" && sub.ID != liveSubID { + slog.Warn("billing.subscription.cancelled.stale_subscription_skip", + "team_id", teamID, + "event_subscription_id", sub.ID, + "live_subscription_id", liveSubID, + "current_tier", fromTier, + "action", "cancel/halt/deauth carried a subscription_id that is NOT the team's live subscription — "+ + "NOT downgrading (likely a superseded subscription from a prior plan change). Operator: verify "+ + "the team's live subscription is healthy; if this WAS the live sub, reconcile manually", + ) + emitChargeUndeliverableAudit(ctx, h.db, teamID, sub, event, + chargeUndeliverableReasonStaleSubscriptionCancel, fromTier) + return nil } // Downgrade behaviour: a cancellation with zero paid invoices means the @@ -3236,6 +3273,13 @@ const ( // downgrades flow through cancellation/plan-change), so the tier was kept // and the charge flagged for operator reconciliation. chargeUndeliverableReasonLowerTierCharge = "lower_tier_charge" + // chargeUndeliverableReasonStaleSubscriptionCancel — 2026-06-04: a + // subscription.cancelled/halted/deauthenticated webhook carried a + // subscription_id that does NOT match the team's stored live subscription. + // The event is for a superseded subscription (e.g. the old sub left alive + // after a hobby→pro plan change), so the downgrade was SKIPPED to protect an + // actively-paying customer. Flagged for operator reconciliation. + chargeUndeliverableReasonStaleSubscriptionCancel = "stale_subscription_cancel" ) // F11 (billing-trust audit 2026-05-19) — cancellation copy. diff --git a/internal/handlers/billing_stale_sub_cancel_test.go b/internal/handlers/billing_stale_sub_cancel_test.go new file mode 100644 index 0000000..31131ea --- /dev/null +++ b/internal/handlers/billing_stale_sub_cancel_test.go @@ -0,0 +1,153 @@ +package handlers_test + +import ( + "net/http" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/testhelpers" +) + +// Stale-sub downgrade guard (MONEY-SENSITIVE, 2026-06-04). +// +// A subscription.cancelled/halted/deauthenticated webhook carries +// notes.team_id verbatim from WHATEVER subscription fired it — including a +// SUPERSEDED one. After a hobby→pro plan change the old hobby subscription +// stays alive in Razorpay; its eventual cancellation must NOT downgrade the +// team that is now actively paying on a different live Pro subscription. +// +// These tests pin three behaviours exercised through the real webhook handler +// path (signature verify → dispatch → handleSubscriptionCancelled): +// (a) a cancel for the team's LIVE subscription still downgrades. +// (b) a cancel for a STALE/non-matching subscription id is IGNORED (no +// UpdatePlanTier, a billing.charge_undeliverable audit row IS emitted). +// (c) an empty live subscription id falls through to historical behaviour +// (the team is downgraded — we cannot prove the event is stale). + +// TestBillingWebhook_SubscriptionCancelled_LiveSub_StillDowngrades verifies +// behaviour (a): when the cancelled webhook's subscription_id MATCHES the +// team's stored live subscription, the historical downgrade still happens. +func TestBillingWebhook_SubscriptionCancelled_LiveSub_StillDowngrades(t *testing.T) { + db, cleanDB := billingStateNeedsDB(t) + defer cleanDB() + + app, _ := billingWebhookDBApp(t, db) + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + defer db.Exec(`DELETE FROM teams WHERE id = $1::uuid`, teamID) + + liveSub := "sub_live_" + uuid.NewString() + _, err := db.Exec(`UPDATE teams SET stripe_customer_id = $1 WHERE id = $2::uuid`, liveSub, teamID) + require.NoError(t, err) + + // Webhook fires for the SAME (live) subscription → downgrade proceeds. + payload := makeSubscriptionCancelledPayload(t, teamID, liveSub) + req := signedWebhookRequest(t, payload) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + // Tier dropped to hobby (paid_count omitted → courtesy floor). + var newTier string + require.NoError(t, db.QueryRow(`SELECT plan_tier FROM teams WHERE id = $1::uuid`, teamID).Scan(&newTier)) + assert.Equal(t, "hobby", newTier, "cancel for the LIVE sub must downgrade") + + // No stale-skip audit row — this was a legitimate cancellation. + var staleCount int + require.NoError(t, db.QueryRow(` + SELECT count(*) FROM audit_log + WHERE team_id = $1::uuid + AND kind = 'billing.charge_undeliverable' + AND metadata->>'reason' = 'stale_subscription_cancel'`, teamID).Scan(&staleCount)) + assert.Equal(t, 0, staleCount, "live-sub cancel must NOT emit a stale-skip audit row") +} + +// TestBillingWebhook_SubscriptionCancelled_StaleSub_IsIgnored verifies +// behaviour (b): a cancelled webhook whose subscription_id does NOT match the +// team's stored live subscription is treated as a superseded event — the tier +// is KEPT and a billing.charge_undeliverable (reason=stale_subscription_cancel) +// audit row is emitted for operator reconciliation. +func TestBillingWebhook_SubscriptionCancelled_StaleSub_IsIgnored(t *testing.T) { + db, cleanDB := billingStateNeedsDB(t) + defer cleanDB() + + app, _ := billingWebhookDBApp(t, db) + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + defer db.Exec(`DELETE FROM teams WHERE id = $1::uuid`, teamID) + + liveSub := "sub_live_pro_" + uuid.NewString() + _, err := db.Exec(`UPDATE teams SET stripe_customer_id = $1 WHERE id = $2::uuid`, liveSub, teamID) + require.NoError(t, err) + + // Webhook fires for a DIFFERENT (superseded hobby) subscription. + staleSub := "sub_stale_hobby_" + uuid.NewString() + payload := makeSubscriptionCancelledPayload(t, teamID, staleSub) + req := signedWebhookRequest(t, payload) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + // Non-retryable skip keeps the 200 — Razorpay must not redeliver. + require.Equal(t, http.StatusOK, resp.StatusCode) + + // Tier UNCHANGED — the actively-paying Pro customer must not be downgraded. + var newTier string + require.NoError(t, db.QueryRow(`SELECT plan_tier FROM teams WHERE id = $1::uuid`, teamID).Scan(&newTier)) + assert.Equal(t, "pro", newTier, "stale-sub cancel must NOT downgrade a live paying customer") + + // A stale-skip audit row IS emitted for operator reconciliation. + var staleCount int + require.NoError(t, db.QueryRow(` + SELECT count(*) FROM audit_log + WHERE team_id = $1::uuid + AND kind = 'billing.charge_undeliverable' + AND metadata->>'reason' = 'stale_subscription_cancel'`, teamID).Scan(&staleCount)) + assert.Equal(t, 1, staleCount, "stale-sub cancel must emit exactly one stale-skip audit row") + + // No subscription.canceled row — the cancellation email must NOT fire. + var canceledCount int + require.NoError(t, db.QueryRow(` + SELECT count(*) FROM audit_log + WHERE team_id = $1::uuid AND kind = 'subscription.canceled'`, teamID).Scan(&canceledCount)) + assert.Equal(t, 0, canceledCount, "stale-sub cancel must NOT emit a customer cancellation row") +} + +// TestBillingWebhook_SubscriptionCancelled_EmptyLiveSub_FallsThrough verifies +// behaviour (c): when the team has NO stored live subscription id (never paid, +// or a sub-id write that never landed), the guard cannot prove the event is +// stale, so it falls through to the historical downgrade behaviour. +func TestBillingWebhook_SubscriptionCancelled_EmptyLiveSub_FallsThrough(t *testing.T) { + db, cleanDB := billingStateNeedsDB(t) + defer cleanDB() + + app, _ := billingWebhookDBApp(t, db) + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + defer db.Exec(`DELETE FROM teams WHERE id = $1::uuid`, teamID) + + // stripe_customer_id left NULL (MustCreateTeamDB does not set it). + payload := makeSubscriptionCancelledPayload(t, teamID, "sub_any_"+uuid.NewString()) + req := signedWebhookRequest(t, payload) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + // Historical behaviour: tier downgraded to the hobby courtesy floor. + var newTier string + require.NoError(t, db.QueryRow(`SELECT plan_tier FROM teams WHERE id = $1::uuid`, teamID).Scan(&newTier)) + assert.Equal(t, "hobby", newTier, "empty live sub must fall through to historical downgrade") + + // No stale-skip audit row — the guard did not trip. + var staleCount int + require.NoError(t, db.QueryRow(` + SELECT count(*) FROM audit_log + WHERE team_id = $1::uuid + AND kind = 'billing.charge_undeliverable' + AND metadata->>'reason' = 'stale_subscription_cancel'`, teamID).Scan(&staleCount)) + assert.Equal(t, 0, staleCount, "empty-live-sub fall-through must NOT emit a stale-skip audit row") +}