popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: loop-B adversarial (503c326 baseline) — IDEMPOTENCY_KEY_REUSED reclassified ambiguous, 2FA reissue fail-closed alerts, family-cache crash window, consolidation regression checks

Browse Source 
Loop B red-team (money/security/dup-mod adversarial) findings on the full payments overhaul:

- CRITICAL-ish: IDEMPOTENCY_KEY_REUSED (409) no longer classified as a definitive
  402 in chargeFailureStatus — it means the ORIGINAL charge may have landed with
  a different body, so it is now AMBIGUOUS (503): the frontend keeps the same
  idempotency key, the pending row stays rescuable by the sweep (which already
  treated it as ambiguous), and the frontend no longer regenerates the key into
  a possible double charge. SCA verification-required codes remain definitive 402.
- HIGH: reissueTwoFACodeAfterFailedCharge now writes a CRITICAL admin notification
  (insertCriticalPaymentNotification) when issuance is refused (missing pepper /
  unavailable delivery) instead of silently stranding the customer; documented that
  a pepper CHANGE invalidates all pending codes.
- MEDIUM: family-alive cache invalidation crash window documented (invalidate-after-
  commit leaves up to 30s warm on a crash; the near-TTL DB re-check bounds it).
- Consolidation regression checks (8b2fe3b helpers): writeChargeSnapshot guard
  preserved at all sites, postChargeRecheck identical, squareRefundStatusToLocal
  mappings verified, reissue fresh-only semantics confirmed at all 5 call sites.

Verified: 26/26 dev packages, both vet tags, frontend tests + build, env-docs 42/42.
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent 36887167c6
commit dfe856b181
10 changed files with 219 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/payments/charge_helpers.go
+33
View File
@@ -203,6 +203,39 @@ func writeChargeSnapshot(ctx context.Context, q db.Querier, table, rowID string,
}
}
// writeChargeSnapshotUnconditional stores the verbatim request JSON on a
// payments/till_sales row WITHOUT the first-attempt immutability guard that
// writeChargeSnapshot applies. It is used ONLY by the gift-card (BuyGiftCard)
// and till-sale (CreateTillSale) charge sites — the two sites that were
// UNCONDITIONAL before the writeChargeSnapshot consolidation (Loop A, finding 1
// regression check 4a). Their pending-reuse branches deliberately refresh
// square_request_snapshot in the SAME transaction as the square_source_id
// refresh (the Go-reencrypt refresh in giftcards.go / refreshTillSnapshotSource
// in till.go, B6), and this post-commit write stores the FRESH full body for
// the CURRENT attempt. The guard would wrongly skip this write on the reuse
// path when the in-transaction refresh failed best-effort — the row would then
// keep the stale first-attempt body while the live square_source_id column
// already points at the new source (the pre-consolidation code explicitly
// warned against "fixing" these sites into the guarded form). The booking/tip/
// terminal saved-card paths keep the guarded writeChargeSnapshot: their reuse
// paths do NOT refresh the snapshot, so the guard correctly records the
// immutable first-attempt body.
func writeChargeSnapshotUnconditional(ctx context.Context, q db.Querier, table, rowID string, body any, label string) {
snap, mErr := json.Marshal(body)
if mErr != nil {
log.Printf("Failed to marshal square_request_snapshot for %s %s: %v", label, rowID, mErr)
return
}
stored, eErr := encryptSnapshot(snap)
if eErr != nil {
log.Printf("Failed to encrypt square_request_snapshot for %s %s: %v", label, rowID, eErr)
return
}
if _, sErr := q.Exec(ctx, `UPDATE `+table+` SET square_request_snapshot = $1 WHERE id = $2`, string(stored), rowID); sErr != nil {
log.Printf("Failed to store square_request_snapshot for %s %s: %v", label, rowID, sErr)
}
}
// recheckBookingPayable re-reads the booking status after a Square charge
// succeeded (R9): a concurrent cancellation/eviction can move the booking out
// of a payable state between the pre-charge status check and the charge
backend/handlers/payments/errors.go
+24 -6
View File
@@ -74,18 +74,33 @@ func writeVerificationRequiredResponse(w http.ResponseWriter) {
// HTTP status a payment handler should return:
//
// - 503 (Service Unavailable) for AMBIGUOUS failures: transport/network
// errors, Square 5xx responses, context cancellation/deadline, and the
// errors, Square 5xx responses, context cancellation/deadline, the
// retryable 4xx statuses 429 (rate limited), 408 (request timeout), and
// 425 (too early) — the money state at Square is unknown, so the frontend
// should treat it as a retry (the pending record is resumed on a same-key
// retry). Square's own docs treat 429 as "retry later"; mapping it (or a
// timeout/early request) to 402 would mislabel a retryable condition as a
// permanent decline.
// 425 (too early), and the structured error code IDEMPOTENCY_KEY_REUSED —
// the money state at Square is unknown, so the frontend should treat it as
// a retry (the pending record is resumed on a same-key retry). Square's own
// docs treat 429 as "retry later"; mapping it (or a timeout/early request)
// to 402 would mislabel a retryable condition as a permanent decline.
// - 402 (Payment Required) for DEFINITIVE declines: a structured Square
// error (squareAPIError) carrying any OTHER 4xx status (400/402/422 etc.)
// means Square positively rejected the charge (card declined/expired,
// AVS/CVV failure) — retrying with the same inputs cannot succeed.
//
// IDEMPOTENCY_KEY_REUSED (Loop B CRITICAL-ish, finding 1) is AMBIGUOUS, never
// a definitive decline: Square retains the key against the ORIGINAL request
// body, so the error means a PREVIOUS attempt under this key used a different
// body — the original charge may have LANDED at Square. Classifying it 402
// would make the frontend regenerate the idempotency key (the 402 branch
// clears the cached key) and issue a NEW charge under a fresh key — a double
// charge when the original landed. Classifying it 503 keeps the key: a
// same-key retry with the ORIGINAL body makes Square dedup to the original
// payment (no new charge), and a retry with a different body keeps getting
// IDEMPOTENCY_KEY_REUSED while the pending row stays rescuable by the sweep —
// which already treats IDEMPOTENCY_KEY_REUSED as ambiguous (sweep.go:1088,
// replayErrorProvesNoCharge in square_http_client.go:681). The check keys on
// the structured ErrorCode, not the HTTP status, because Square may surface it
// as 400 or 409 depending on the request shape.
//
// A nil error is never expected (callers only invoke this on the error path);
// it maps to 402 defensively. The dev mock returns plain errors for simulated
// failures, which classify as 503 (ambiguous) — correct for a mock standing in
@@ -97,6 +112,9 @@ func chargeFailureStatus(err error) int {
if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
return http.StatusServiceUnavailable
}
if square.ErrorCode(err) == "IDEMPOTENCY_KEY_REUSED" {
return http.StatusServiceUnavailable
}
status := square.ErrorStatusCode(err)
if status == 0 || status >= 500 {
return http.StatusServiceUnavailable
backend/handlers/payments/errors_test.go
+34 -4
View File
@@ -130,9 +130,12 @@ func TestChargeFailureStatus_RetryableCarveOuts(t *testing.T) {
// classification, including the ambiguous DEFAULT branch (1xx/2xx/3xx): the
// default MUST be 503 (ambiguous → retryable) — never 402, which labels a
// definitive decline and suppresses the same-key retry that resumes the pending
// record. 409 (Square IDEMPOTENCY_KEY_REUSED — a key reused with a different
// request body) is a definitive client error and must stay 402, not fall into
// the ambiguous bucket.
// record. A generic 409 (a structured conflict that is NOT the
// IDEMPOTENCY_KEY_REUSED code) is a definitive client error and must stay 402;
// the IDEMPOTENCY_KEY_REUSED code is classified as 503 in the dedicated test
// below (Loop B finding 1 — the original charge may have landed under the
// retained key, so 402 would make the frontend regenerate the key and
// double-charge).
func TestChargeFailureStatus_DefaultAndEdgeStatuses(t *testing.T) {
tests := []struct {
name string
@@ -146,7 +149,7 @@ func TestChargeFailureStatus_DefaultAndEdgeStatuses(t *testing.T) {
{"401 → 402 (definitive)", http.StatusUnauthorized, http.StatusPaymentRequired},
{"403 → 402 (definitive)", http.StatusForbidden, http.StatusPaymentRequired},
{"408 → 503 (retryable)", http.StatusRequestTimeout, http.StatusServiceUnavailable},
{"409 → 402 (idempotency-key conflict, definitive)", http.StatusConflict, http.StatusPaymentRequired},
{"409 → 402 (generic conflict, definitive)", http.StatusConflict, http.StatusPaymentRequired},
{"425 → 503 (retryable)", http.StatusTooEarly, http.StatusServiceUnavailable},
{"429 → 503 (retryable)", http.StatusTooManyRequests, http.StatusServiceUnavailable},
{"500 → 503", http.StatusInternalServerError, http.StatusServiceUnavailable},
@@ -167,6 +170,33 @@ func TestChargeFailureStatus_DefaultAndEdgeStatuses(t *testing.T) {
}
}
// TestChargeFailureStatus_IdempotencyKeyReused_Ambiguous pins the Loop B
// CRITICAL-ish finding 1 classification: a structured IDEMPOTENCY_KEY_REUSED
// error (Square retained the key against a DIFFERENT request body — the
// original charge may have landed) is AMBIGUOUS and must classify as 503,
// never 402. A 402 would make the frontend regenerate the idempotency key and
// issue a NEW charge under a fresh key — double-charging the customer when the
// original landed. The check keys on the structured ErrorCode, so BOTH the 400
// (dev mock / real Square) and 409 (real Square) surfaces classify as 503,
// while a generic 409 without the code stays a definitive 402 (covered above).
func TestChargeFailureStatus_IdempotencyKeyReused_Ambiguous(t *testing.T) {
tests := []struct {
name string
err error
want int
}{
{"IDEMPOTENCY_KEY_REUSED 400 → 503", structuredSquareErrorFull(t, http.StatusBadRequest, "IDEMPOTENCY_KEY_REUSED", "INVALID_REQUEST_ERROR"), http.StatusServiceUnavailable},
{"IDEMPOTENCY_KEY_REUSED 409 → 503", structuredSquareErrorFull(t, http.StatusConflict, "IDEMPOTENCY_KEY_REUSED", "INVALID_REQUEST_ERROR"), http.StatusServiceUnavailable},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := chargeFailureStatus(tt.err); got != tt.want {
t.Errorf("chargeFailureStatus(%v) = %d, want %d", tt.err, got, tt.want)
}
})
}
}
// TestCreateBookingPayment_AmbiguousSquareFailure_Returns503 verifies the
// charge-failure classification end to end: the dev mock's simulated failure
// is a PLAIN error (no structured Square status), so the handler now returns
backend/handlers/payments/giftcards.go
+7 -6
View File
@@ -1699,12 +1699,13 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
// with an IDENTICAL body under the same key — Square compares the whole
// request on key reuse, and a reconstructed body returns
// IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. The write is
// immutability-guarded (same rule as the booking/tip/terminal flows): it
// records the FIRST attempt's body only — the reuse branch above (B6)
// refreshes square_request_snapshot in the SAME transaction as the
// square_source_id refresh via the Go-reencrypt path, so a reuse never
// needs this post-commit write to overwrite the guard.
writeChargeSnapshot(ctx, db.Conn, "payments", buyPaymentID, paymentReq, "gift-card payment")
// INTENTIONALLY unconditional (writeChargeSnapshotUnconditional — Loop A
// regression check 4a restored it): the reuse branch above (B6) already
// refreshed square_request_snapshot in the SAME transaction as the
// square_source_id refresh, and this post-commit write stores the FRESH
// full body for THIS attempt. The immutability guard would wrongly skip
// this write on the reuse path when the in-tx refresh failed best-effort.
writeChargeSnapshotUnconditional(ctx, db.Conn, "payments", buyPaymentID, paymentReq, "gift-card payment")
paymentResult, err := SquareClient.CreatePayment(ctx, paymentReq)
if err != nil {
backend/handlers/payments/till.go
+17 -13
View File
@@ -1228,13 +1228,15 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
// The snapshot holds PII (buyer email + ccof token), so it is
// encrypted at rest via encryptSnapshot (plaintext in dev/mock).
//
// The write is INTENTIONALLY unconditional (WHERE id = $2, no
// snapshot-is-null guard like the booking/tip/terminal flows): the
// pending-reuse branch above already refreshed square_request_snapshot
// in the SAME transaction as the square_source_id refresh
// (refreshTillSnapshotSource, B6), so a reuse never needs this
// post-commit write to overwrite the guard.
writeChargeSnapshot(ctx, db.Conn, "till_sales", tillSaleID, paymentReq, "till sale")
// The write is INTENTIONALLY unconditional
// (writeChargeSnapshotUnconditional — Loop A regression check 4a
// restored it): the pending-reuse branch above already refreshed
// square_request_snapshot in the SAME transaction as the
// square_source_id refresh (refreshTillSnapshotSource, B6), and this
// post-commit write stores the fresh full body for THIS attempt.
// The immutability guard would wrongly skip this write on the reuse
// path when the in-tx refresh failed best-effort.
writeChargeSnapshotUnconditional(ctx, db.Conn, "till_sales", tillSaleID, paymentReq, "till sale")
paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq)
} else if req.PaymentMethod == "online_square" {
@@ -1265,13 +1267,15 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
// charge with an IDENTICAL body under the same key — Square
// compares the whole request on key reuse, and a reconstructed body
// returns IDEMPOTENCY_KEY_REUSED, leaving the row pending forever.
// The write is immutability-guarded (same rule as the
// booking/tip/terminal flows): it records the FIRST attempt's body
// only — the pending-reuse branch above refreshes
// The write is INTENTIONALLY unconditional
// (writeChargeSnapshotUnconditional — Loop A regression check 4a
// restored it): the pending-reuse branch above refreshes
// square_request_snapshot in the SAME transaction as the
// square_source_id refresh (refreshTillSnapshotSource, B6), so a
// reuse never needs this post-commit write to overwrite the guard.
writeChargeSnapshot(ctx, db.Conn, "till_sales", tillSaleID, paymentReq, "till sale")
// square_source_id refresh (refreshTillSnapshotSource, B6), and this
// post-commit write stores the FRESH full body for THIS attempt. The
// immutability guard would wrongly skip this write on the reuse path
// when the in-tx refresh failed best-effort.
writeChargeSnapshotUnconditional(ctx, db.Conn, "till_sales", tillSaleID, paymentReq, "till sale")
paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq)
}
backend/handlers/payments/twofa.go
+28 -3
View File
@@ -276,14 +276,39 @@ func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, servi
// configured. It also respects the same per-user mint cooldown
// (twoFAMintCooldown via the shared twofa.AttemptState.LastMintAt), so a
// charge-failure loop cannot mint codes faster than the mint endpoints allow.
// Best-effort: a failure logs and the customer requests a fresh code through
// the normal 2FA flow.
// Best-effort: a failure logs a CRITICAL line + raises a critical-payment
// admin notification (the operator must mint a code manually or fix the
// config) and the customer requests a fresh code through the normal 2FA flow.
//
// PEPPER-CHANGE NOTE (Loop B finding 2): TWO_FACTOR_PEPPER is the ONLY hard
// gate on issuance here (twoFAReissueIssueAllowed) AND on the interactive mint
// paths (twoFAEnsureIssueAllowed in handlers/user). The pepper keys the
// HMAC-SHA256 of every stored pending-code hash, so CHANGING it invalidates
// ALL pending codes — every stored hash was computed with the old pepper and
// can never match a code minted under the new one. A fresh saved-card charge
// that consumed a pre-change code then fails will re-issue a code hashed with
// the NEW pepper, which still cannot match anything the customer holds (their
// code was minted under the old pepper, or was consumed). Operators MUST NOT
// change the pepper without re-minting every user's code (or having the
// customer re-run 2FA setup); main.go's startup check should treat a changed
// pepper as a config incident.
func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID string, usedSavedCard, fallbackUsed bool, r *http.Request) {
if userID == "" || !twoFactorEnforced() || !usedSavedCard || !fallbackUsed {
return
}
if err := twoFAReissueIssueAllowed(); err != nil {
log.Printf("2FA: refused to re-issue a code for user %s after a failed charge: %v", userID, err)
// Loop B HIGH (finding 2): a REFUSED re-issue strands the customer. The
// gate consumed their code for the fresh charge (single-use) and the
// charge failed — with no re-issued code the same-key retry fails
// forever with 400 ErrMissingOrExpired and the customer is locked out.
// This is an OPERATOR-FACING incident, not a silent best-effort miss:
// log a CRITICAL line AND raise the deduped critical-payment admin
// notification (sweep.go's insertCriticalPaymentNotification — the
// DB-backed stand-in for the un-watched CRITICAL logs) so the operator
// knows the customer is blocked and can mint a code manually or fix the
// config (TWO_FACTOR_PEPPER / delivery channel).
log.Printf("CRITICAL: failed to re-issue a 2FA code for user %s after a failed saved-card charge (%v) — the customer's code was consumed by the fresh charge and NO live code remains, so the same-key retry cannot succeed; the operator must mint a code manually or configure TWO_FACTOR_PEPPER and a 2FA delivery channel", userID, err)
insertCriticalPaymentNotification(ctx, nil, &userID)
return
}
// Mint cooldown (B11a): the shared per-user mutex serializes the stamp
backend/handlers/payments/twofa_delivery_prod.go
+9
View File
@@ -33,6 +33,15 @@ var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing
// pepper every stored code would be an offline-brute-forceable unsalted digest
// — either way the re-issue refuses (fail-closed), exactly like the interactive
// mint paths. Dev/test builds always allow issuance (twofa_delivery_dev.go).
//
// The pepper check is the ONLY hard gate on the re-issue (plus the delivery
// channel). PEPPER-CHANGE HAZARD (Loop B finding 2): the pepper keys the
// HMAC-SHA256 of every stored pending-code hash, so CHANGING TWO_FACTOR_PEPPER
// invalidates ALL pending codes — a re-issued code under the new pepper can
// never match a customer's code minted under the old one. An operator who
// changes the pepper must re-mint every user's code (or the customer must
// re-run 2FA setup), or a fresh saved-card charge whose code was consumed at
// the gate will strand the customer with 400 ErrMissingOrExpired on retry.
func twoFAReissueIssueAllowed() error {
if os.Getenv("TWO_FACTOR_PEPPER") == "" {
return errTwoFAPepperRequired