diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index eb95f20..71497cb 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -13,6 +13,7 @@ import ( "crussell/clock" "crussell/db" + "crussell/internal/adminnotify" "github.com/jackc/pgx/v5" @@ -662,8 +663,18 @@ func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, } // (ii) Surface the theft in the admin notification centre. The NOT // EXISTS guard keeps ONE alert per reused family until an admin - // acknowledges it — mirroring insertCriticalPaymentNotification. - if _, err := tx.Exec(ctx, ` + // acknowledges it — mirroring insertCriticalPaymentNotification — and + // the GLOBAL cap (adminnotify.MaxUnacknowledgedCriticalLogs) bounds the + // unacknowledged 'refresh_token_reuse' queue ATOMICALLY (Round 2 Loop B + // finding 1): without it a register-botnet — N accounts, each rotated + // once and replayed past the 60s grace — could bury the single-operator + // notification centre under unbounded alerts. The cap is folded into + // the INSERT's WHERE clause (count-then-insert is atomic, closing the + // TOCTOU), and the pre-check logs the suppression for operator + // visibility. + if adminnotify.CriticalLogsCapExceeded(ctx, tx, "refresh_token_reuse") { + slog.Error("CRITICAL: refresh token reuse detected but admin alert suppressed — unacknowledged 'refresh_token_reuse' queue at the cap", "userID", reusedUserID, "cap", adminnotify.MaxUnacknowledgedCriticalLogs) + } else if _, err := tx.Exec(ctx, ` INSERT INTO admin_notifications (reason, user_id, created_at) SELECT 'refresh_token_reuse', $1, NOW() WHERE NOT EXISTS ( @@ -672,7 +683,10 @@ func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, AND an.user_id = $1 AND an.acknowledged_at IS NULL ) - `, reusedUserID); err != nil { + AND (SELECT COUNT(*) FROM admin_notifications _an + WHERE _an.reason = 'refresh_token_reuse' + AND _an.acknowledged_at IS NULL) < $2 + `, reusedUserID, adminnotify.MaxUnacknowledgedCriticalLogs); err != nil { slog.Error("CRITICAL: refresh token reuse detected but admin alert insert failed", "userID", reusedUserID, "err", err) } // Commit the family revocation + alert — NOT the deferred rollback. diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index 3cc2c3b..136fc8e 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -45,29 +45,40 @@ const maxLoginInProgress = 20 // live before it is stale and evictable. const loginInProgressWindow = 30 * time.Second -// maxConcurrentLoginBcrypt bounds how many login requests may run their bcrypt -// comparison concurrently (Round 2 Loop A finding 4b). The progressive per-IP -// middleware sleeps BEFORE this handler, so without the cap a flood of -// throttled login requests could stack an unbounded number of goroutines that -// all hit bcrypt the moment their sleeps elapse — a CPU-amplification vector. -// Beyond the cap the login is rejected 429 immediately (nothing has been -// processed, so nothing leaks). -const maxConcurrentLoginBcrypt = 20 +// maxConcurrentBcrypt bounds how many bcrypt operations may run concurrently +// across BOTH /login and /register (Round 2 Loop A finding 4b + Round 2 Loop B +// finding 4). The progressive per-IP middleware sleeps BEFORE this handler, so +// without the cap a flood of throttled requests could stack an unbounded number +// of goroutines that all hit bcrypt the moment their sleeps elapse — a +// CPU-amplification vector (a register-botnet also burns CPU on +// bcrypt.GenerateFromPassword). Beyond the cap the request is rejected 429 +// immediately (nothing has been processed, so nothing leaks). +// +// Round 2 Loop B finding 5b — ACCEPTED BOUNDED-DoS TRADE-OFF: the 20-slot +// global bound is shared by login AND register AND, by extension, every +// authenticated user. A sustained flood at either endpoint can therefore +// starve bcrypt for everyone for up to one request at a time (429 "server +// busy"). That is the intended trade-off: 20 genuinely concurrent bcrypt +// operations (~20 × ~60ms ≈ 1.2s of wall time) is far more than a single +// salon ever produces, and bounding the CPU is the point of the cap. +const maxConcurrentBcrypt = 20 // Login state management var ( loginStateMu sync.Mutex loginInProgress = make(map[string]time.Time) - // loginBcryptSlots is the counting semaphore backing maxConcurrentLoginBcrypt. - loginBcryptSlots = make(chan struct{}, maxConcurrentLoginBcrypt) + // authBcryptSlots is the counting semaphore backing maxConcurrentBcrypt, + // shared by LoginHandler and RegisterHandler. + authBcryptSlots = make(chan struct{}, maxConcurrentBcrypt) ) -// acquireLoginBcryptSlot tries to reserve a concurrent bcrypt slot. ok=false -// means the handler must respond 429. -func acquireLoginBcryptSlot() (release func(), ok bool) { +// acquireBcryptSlot tries to reserve a concurrent bcrypt slot. ok=false +// means the handler must respond 429. Shared by login and register so the +// bcrypt CPU budget is global, not per-endpoint. +func acquireBcryptSlot() (release func(), ok bool) { select { - case loginBcryptSlots <- struct{}{}: - return func() { <-loginBcryptSlots }, true + case authBcryptSlots <- struct{}{}: + return func() { <-authBcryptSlots }, true default: return nil, false } @@ -227,6 +238,21 @@ func RegisterHandler(w http.ResponseWriter, r *http.Request) { mw.RespondError(w, http.StatusBadRequest, "password must be at least 6 characters") return } + // Round 2 Loop B finding 4: /register previously ran the zxcvbn strength + // scoring AND bcrypt.GenerateFromPassword with NO concurrency cap — a + // register-botnet could stack unbounded goroutines burning CPU, and each + // registered account also fuels the notification-flood and attempt-map + // findings (1/3). Share the login bcrypt slot budget (acquireBcryptSlot — + // the global 20-slot cap, see maxConcurrentBcrypt): beyond it the + // registration is rejected 429 immediately. The slot wraps the expensive + // part (zxcvbn + bcrypt) and is released via defer on every path. + release, ok := acquireBcryptSlot() + if !ok { + mw.RespondError(w, http.StatusTooManyRequests, "server busy, try again later") + return + } + defer release() + // Server-side password strength check using the same @zxcvbn-ts/core as the frontend // via goja (ExecJS-style). Guarantees exact parity with frontend scoring. // Skipped when GO_TESTING=1 (dev/test environments) to allow weaker passwords. @@ -499,7 +525,7 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { // released immediately after the compare — bcrypt is the expensive, // amplifier-prone part; the DB work below is cheap. The deferred delete // above releases this user's in-flight slot on every path. - release, ok := acquireLoginBcryptSlot() + release, ok := acquireBcryptSlot() if !ok { mw.RespondError(w, http.StatusTooManyRequests, "server busy, try again later") return diff --git a/backend/handlers/bookings/bookings.go b/backend/handlers/bookings/bookings.go index b4eb00b..69ccc38 100644 --- a/backend/handlers/bookings/bookings.go +++ b/backend/handlers/bookings/bookings.go @@ -3967,6 +3967,19 @@ func AdminRescheduleBookingHandler(w http.ResponseWriter, r *http.Request) { forgiveFees := req.ForgiveFees != nil && *req.ForgiveFees if forgiveFees { log.Printf("[AUDIT] Admin %s rescheduled booking %s with fee forgiveness (start_time: %s)", adminID, bookingID, req.StartTime.Format(time.RFC3339)) + // MEDIUM-3a coverage: fee forgiveness on a reschedule is an admin money + // action (the deposit-protection retention that would have applied is + // waived) — record it in admin_audit_log (best-effort, own tx, mirrors + // the InsertAdminAuditCharge pattern). The forgiven amount is the + // booking's total paid, which is what the retention would have drawn on. + if payInfo, payErr := payments.NewPaymentService().GetBookingPaymentInfo(r.Context(), bookingID); payErr == nil { + payments.InsertAdminAuditCharge(r.Context(), adminID, bookingUserID, "admin_reschedule_fee_forgiven", map[string]any{ + "booking_id": bookingID, + "forgiven_amount": payInfo.TotalPaid, + }) + } else { + log.Printf("Failed to fetch payment info for reschedule fee-forgiveness audit on booking %s: %v", bookingID, payErr) + } } var durationMinutes int diff --git a/backend/handlers/payments/giftcard_clawback.go b/backend/handlers/payments/giftcard_clawback.go index 2ff8d80..1bd0252 100644 --- a/backend/handlers/payments/giftcard_clawback.go +++ b/backend/handlers/payments/giftcard_clawback.go @@ -115,9 +115,32 @@ func RevertGiftCardFunding(ctx context.Context, action, giftCardID string, amoun if err := tx.Commit(ctx); err != nil { return fmt.Errorf("failed to commit clawback transaction: %w", err) } + + // MEDIUM-3a coverage: every funding clawback is a money reversal that must + // be auditable. Record it in admin_audit_log (best-effort, own tx — + // InsertAdminAuditCharge's separate transaction keeps a write failure from + // aborting the committed clawback). The create branch DELETES the card, so + // target_gift_card_id must stay NULL (the FK would otherwise block the card + // deletion); the card id is carried in the details. admin_id is NULL too — + // this helper is the shared implementation for the till handler, the + // stale-pending sweep and the Square webhook, which carry no admin actor. + insertGiftCardClawbackAudit(ctx, giftCardID, tillSaleID, action, amount) + return nil } +// insertGiftCardClawbackAudit records a funding clawback in admin_audit_log +// via the shared InsertAdminAuditCharge helper. Best-effort and non-fatal — +// a failed audit write can never abort the already-committed money reversal. +func insertGiftCardClawbackAudit(ctx context.Context, giftCardID, tillSaleID, action string, amount float64) { + InsertAdminAuditCharge(ctx, "", "", "giftcard_clawback", map[string]any{ + "gift_card_id": giftCardID, + "till_sale_id": tillSaleID, + "action": action, + "amount": amount, + }) +} + // IsTillSaleNotPending reports whether err is the claim-first sentinel // (errTillSaleNotPending): the gating `status='pending'` UPDATE matched zero // rows, so the till sale is no longer pending and its gift card must be left diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index d37a18b..b5f4b05 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -919,6 +919,17 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-3a coverage: an admin transferring gift-card value is an admin + // money action — record it in admin_audit_log (best-effort, own tx, mirrors + // the top-up audit above). A transfer is only permitted between + // unredeemed cards (both rows' redeemed_by must be NULL), so no cardholder + // is determinable — the acting admin is recorded as the target. + InsertAdminAuditCharge(ctx, adminID, adminID, "gift_card_transfer", map[string]any{ + "from_card_id": fromCardID, + "to_card_id": req.ToCardID, + "amount": req.Amount, + }) + w.WriteHeader(http.StatusOK) if err := json.NewEncoder(w).Encode(map[string]string{"status": "success"}); err != nil { log.Printf("Failed to encode JSON response: %v", err) diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 0b7489d..de6c569 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -588,6 +588,10 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { bookingPortionPounds := float64(bookingPortion) / 100.0 tipPounds := float64(tipPortion) / 100.0 var paymentID string + // auditTargetUserID is the booking's customer for the MEDIUM-3a audit + // row (empty = guest booking, audited with a NULL target). Captured per + // branch and used AFTER the money commits below. + var auditTargetUserID string if *req.PaymentMethod == "cash" { if bookingPortionPounds > roundingEpsilon { @@ -632,17 +636,16 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } } - // MEDIUM-3a: record the admin-initiated CASH charge in - // admin_audit_log (best-effort, non-fatal — an audit-write failure - // can never roll back a completed charge). + // MEDIUM-3a: the admin-initiated CASH charge is audited in + // admin_audit_log AFTER the money commits below (best-effort, + // non-fatal). Capture the booking's customer now for the audit; a + // guest booking has no user_id and audits with a NULL target. var cashCustomerID sql.NullString - if cuErr := tx.QueryRow(r.Context(), "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&cashCustomerID); cuErr == nil && cashCustomerID.Valid { - InsertAdminAuditCharge(r.Context(), adminID, cashCustomerID.String, "admin_cash_charge", map[string]any{ - "booking_id": bookingID, - "payment_id": paymentID, - "amount": amountPounds, - "payment_type": req.PaymentType, - }) + if cuErr := tx.QueryRow(r.Context(), "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&cashCustomerID); cuErr != nil { + log.Printf("Failed to query booking user for cash charge audit: %v", cuErr) + } + if cashCustomerID.Valid { + auditTargetUserID = cashCustomerID.String } } else { // giftcard var customerID sql.NullString @@ -801,16 +804,11 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } } - // MEDIUM-3a: record the admin-initiated gift-card payment in - // admin_audit_log (best-effort, non-fatal — an audit-write failure - // can never roll back a completed charge). + // The admin-initiated gift-card payment is audited AFTER the money + // commits below (best-effort). Capture the customer for the audit + // row; a guest booking has no user_id and audits with a NULL target. if customerID.Valid { - InsertAdminAuditCharge(r.Context(), adminID, customerID.String, "admin_giftcard_payment", map[string]any{ - "booking_id": bookingID, - "payment_id": paymentID, - "amount": amountPounds, - "payment_type": req.PaymentType, - }) + auditTargetUserID = customerID.String } } @@ -820,6 +818,27 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-3a (best-effort, non-fatal — an audit-write failure can never + // roll back a completed charge). The audit runs AFTER the money commits: + // the OLD position wrote the row BEFORE tx.Commit, so a failed commit + // left a false audit row for a charge that never landed. Guest bookings + // audit with a NULL target_user_id (matching the till flow). + if *req.PaymentMethod == "cash" { + InsertAdminAuditCharge(r.Context(), adminID, auditTargetUserID, "admin_cash_charge", map[string]any{ + "booking_id": bookingID, + "payment_id": paymentID, + "amount": amountPounds, + "payment_type": req.PaymentType, + }) + } else { + InsertAdminAuditCharge(r.Context(), adminID, auditTargetUserID, "admin_giftcard_payment", map[string]any{ + "booking_id": bookingID, + "payment_id": paymentID, + "amount": amountPounds, + "payment_type": req.PaymentType, + }) + } + if err := json.NewEncoder(w).Encode(CheckoutResponse{ CheckoutID: paymentID, Status: "COMPLETED", @@ -2295,26 +2314,26 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // against a client that does not. chargeAmount := req.Amount if req.PaymentType == "deposit" && eligibleDiscountPence > 0 { - chargeAmount = req.Amount - eligibleDiscountPence - // A6: when the eligible discount is >= the deposit itself, chargeAmount - // clamps UP — the customer still pays something up front — but NEVER - // beyond the discounted obligation: the cap max(0, (totalPence - - // eligibleDiscountPence) - realPaidPence) equals remainingPence - - // eligibleDiscountPence (remainingPence is the tip-excluded unpaid - // balance, total - realPaid). Without the cap, chargeAmount clamps to - // the full undiscounted deposit and the headroom computation - // (discountHeadroomPence, which counts this pending charge) truncates - // the discount — the booking auto-completes with the customer overpaying - // by the truncated difference. - if chargeAmount <= 0 { - chargeAmount = req.Amount - cap := remainingPence - eligibleDiscountPence - if cap < 0 { - cap = 0 - } - if chargeAmount > cap { - chargeAmount = cap - } + // A6: the deposit is charged net of the eligible campaign credit, then + // clamped DOWN to the discounted obligation max(0, remainingPence - + // eligibleDiscountPence). The OLD clamp only fired when chargeAmount<=0; + // a raw deposit between remaining and remaining+discount was charged at + // the discounted RAW amount while the headroom computation + // (discountHeadroomPence, which counts this in-flight charge) truncated + // the discount — the booking auto-completed with the customer overpaying + // by the truncated difference (Loop-B finding). Clamping ALWAYS to the + // discounted obligation guarantees the headroom always fits the full + // discount: the customer can never be charged beyond the discounted + // price, and no discount is ever truncated. + discounted := req.Amount - eligibleDiscountPence + obligation := remainingPence - eligibleDiscountPence + if obligation < 0 { + obligation = 0 + } + if discounted > obligation { + chargeAmount = obligation + } else { + chargeAmount = discounted } } @@ -2325,19 +2344,39 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // + discount preview could then mint an unintended tip. When the flag is // absent the request is rejected with overflow_tip_confirmation_required so // the frontend can prompt, regardless of booking state. The comparison is - // chargeAmount vs the REAL remaining (see the M4/M7 note above): a deposit - // charge is already net of the campaign credit, so a pre-start deposit can - // never exceed the remaining obligation and never mints an unconfirmed tip. - if req.PaymentType != "tip" && chargeAmount > remainingPence { - if !req.ConfirmOverflowTip { - log.Printf("Overflow requires confirmation: charge %d exceeds remaining %d for booking %s (requested %d)", chargeAmount, remainingPence, bookingID, req.Amount) - mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ - "error": "The extra amount will be recorded as a tip. Confirm to continue.", - "code": "overflow_tip_confirmation_required", - }) - return + // the RAW req.Amount vs the obligation: for a full/balance/partial charge + // (chargeAmount == req.Amount) the obligation is the REAL remaining (see the + // M4/M7 note above), so a charge beyond it requires confirmation. For a + // deposit-with-discount the discounted obligation is remainingPence - + // eligibleDiscountPence — the A6 clamp has already capped chargeAmount to it, + // so the guard compares req.Amount against it (Loop-B finding: a raw deposit + // between remaining and remaining+discount used to slip past the old + // chargeAmount-vs-remaining guard and silently truncate the discount). A + // chargeAmount of 0 (fully discount-covered deposit — the skip path below) + // has no money at all and never overflows. On confirmation the charge is the + // full req.Amount so buildSplitRecords carves the excess beyond the booking's + // real remaining as a tip record — the excess is never absorbed as service + // revenue. + if req.PaymentType != "tip" && chargeAmount > 0 { + overflowThreshold := remainingPence + if req.PaymentType == "deposit" && eligibleDiscountPence > 0 { + overflowThreshold = remainingPence - eligibleDiscountPence + if overflowThreshold < 0 { + overflowThreshold = 0 + } + } + if req.Amount > overflowThreshold { + if !req.ConfirmOverflowTip { + log.Printf("Overflow requires confirmation: requested %d exceeds obligation %d for booking %s (discount credit %d pence)", req.Amount, overflowThreshold, bookingID, eligibleDiscountPence) + mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ + "error": "The extra amount will be recorded as a tip. Confirm to continue.", + "code": "overflow_tip_confirmation_required", + }) + return + } + chargeAmount = req.Amount + log.Printf("Overflow accepted as tip: requested %d exceeds obligation %d for booking %s (discount credit %d pence, confirmed=%v)", req.Amount, overflowThreshold, bookingID, eligibleDiscountPence, req.ConfirmOverflowTip) } - log.Printf("Overflow accepted as tip: charge %d exceeds remaining %d for booking %s (requested %d, confirmed=%v)", chargeAmount, remainingPence, bookingID, req.Amount, req.ConfirmOverflowTip) } // HIGH-2: a pending-reuse retry must match the CHARGE amount stored on the @@ -2371,10 +2410,24 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { if applyErr := applyEligibleCampaignsAtPayment(r.Context(), tx, bookingID, userID, preChargeDiscounts); applyErr != nil { var exErr *campaignExhaustedAtApplyError if errors.As(applyErr, &exErr) { - log.Printf("B13: campaign %s exhausted between preview and apply for booking %s — the deposit is NOT discount-covered; no charge issued; the retry will charge the full deposit", exErr.campaignID, bookingID) - } else { - log.Printf("Failed to apply eligible campaigns on the discount-covered deposit for booking %s: %v", bookingID, applyErr) + // B13 (Loop-B finding): the campaign was exhausted between the + // preview and the apply — the deposit is NOT discount-covered. + // The old code logged B13 and returned a success-shaped 200 + // (status=completed, amount=0) with no discount row, so the + // frontend treated the deposit as PAID and a same-key retry + // could charge the full deposit. The exhaustion reserved nothing + // (the reservation is atomic and matched zero rows), so rolling + // back is clean. Mirror the real-charge path's 400 + // (campaign_fully_redeemed) so the frontend prompts for the + // full amount. + log.Printf("B13: campaign %s exhausted between preview and apply for booking %s — the deposit is NOT discount-covered; no charge issued; returning 400 campaign_fully_redeemed", exErr.campaignID, bookingID) + mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ + "error": "The discount campaign has been fully redeemed. The full amount applies.", + "code": "campaign_fully_redeemed", + }) + return } + log.Printf("Failed to apply eligible campaigns on the discount-covered deposit for booking %s: %v", bookingID, applyErr) } discountAfter := bookingDiscountPence(r.Context(), tx, bookingID) discountApplied := discountAfter > discountBefore @@ -2385,6 +2438,24 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { if bookingIsFullyPaid(r.Context(), tx, bookingID) { completeActiveBookingFromPayment(r.Context(), tx, bookingID) } + // Loop-B finding (idempotency): the skip path writes no row bound to the + // request's idempotency key, so a lost-response same-key retry re-runs + // the handler — and if the campaign has since exhausted, the retry would + // charge the FULL deposit. Attach the request key to the applied + // discount row (the ledger-correct record of the covered deposit) so the + // retry's completed-idempotency short-circuit dedups cleanly. Every + // 200-success skip path applied at least one discount row; when none + // exists nothing was credited and the idempotency gap is benign (the + // retry re-evaluates the same no-charge state). + if _, upErr := tx.Exec(r.Context(), ` + UPDATE payments SET idempotency_key = $1 + WHERE id = (SELECT id FROM payments + WHERE booking_id = $2 AND status = 'completed' + AND payment_method = 'discount' AND idempotency_key IS NULL + ORDER BY created_at DESC LIMIT 1) + `, req.IdempotencyKey, bookingID); upErr != nil { + log.Printf("Failed to attach idempotency key %q to the discount-covered deposit row for booking %s: %v", req.IdempotencyKey, bookingID, upErr) + } // Commit the discount rows (and any completion) — the deferred // rollback must not undo them. if err := tx.Commit(r.Context()); err != nil { @@ -3632,17 +3703,26 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { } reissueResult, reissueErr := SquareClient.RefundPayment(r.Context(), reissueReq) switch { - case reissueErr == nil: - // Resolve by Square's status: terminal COMPLETED/APPROVED - // resolves, FAILED/REJECTED is definitive; non-terminal states - // (PENDING — sweep reconciles — CANCELED, unknown) leave the - // refund pending. - reissueStatus, reissueTerminal := SquareRefundStatusToLocal(reissueResult.Status) - if !reissueTerminal { - log.Printf("Square reissue %s is non-terminal (%s) — leaving refund %s pending for the sweep", reissueResult.ID, reissueResult.Status, existingRefundID.String) - } else if reissueStatus == "failed" { - log.Printf("Square reissue %s FAILED — marking refund %s failed", reissueResult.ID, existingRefundID.String) + case reissueErr == nil: + // Resolve by Square's status: terminal COMPLETED resolves, + // FAILED/REJECTED is definitive; non-terminal states (PENDING — sweep + // reconciles — APPROVED, CANCELED, unknown) leave the refund pending. + reissueStatus, reissueTerminal := SquareRefundStatusToLocal(reissueResult.Status) + if reissueResult.Status == "APPROVED" { + // Loop-B finding (MED-HIGH): APPROVED is NON-terminal — a later + // FAILED/CANCELED must still be able to demote the row. + reissueTerminal = false + } + if !reissueTerminal { + if reissueResult.Status == "APPROVED" { + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET square_refund_id = $1 WHERE id = $2`, reissueResult.ID, existingRefundID.String); upErr != nil { + log.Printf("Failed to record square refund id %s on pending refund %s: %v", reissueResult.ID, existingRefundID.String, upErr) + } } + log.Printf("Square reissue %s is non-terminal (%s) — leaving refund %s pending for the sweep", reissueResult.ID, reissueResult.Status, existingRefundID.String) + } else if reissueStatus == "failed" { + log.Printf("Square reissue %s FAILED — marking refund %s failed", reissueResult.ID, existingRefundID.String) + } if reissueTerminal { if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`, @@ -3878,11 +3958,21 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { // synchronous refund response can be PENDING (money in flight, e.g. an // async card network): marking it completed while Square later fails it // would permanently block that amount in the over-refund guard. Only a - // terminal COMPLETED/APPROVED resolves to completed; non-terminal states - // (PENDING — sweep reconciles — CANCELED, unknown) stay pending; + // terminal COMPLETED resolves to completed; non-terminal states + // (PENDING — sweep reconciles — APPROVED, CANCELED, unknown) stay pending; // FAILED/REJECTED is a real failure. status, terminal := SquareRefundStatusToLocal(refundResult.Status) + if refundResult.Status == "APPROVED" { + // Loop-B finding (MED-HIGH): APPROVED is NON-terminal — a later + // FAILED/CANCELED must still be able to demote the row. + terminal = false + } if !terminal { + if refundResult.Status == "APPROVED" { + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET square_refund_id = $1 WHERE id = $2`, refundResult.ID, refundID); upErr != nil { + log.Printf("Failed to record square refund id %s on pending refund %s: %v", refundResult.ID, refundID, upErr) + } + } log.Printf("Square refund %s is non-terminal (%s) — leaving refund %s pending for the sweep to resolve", refundResult.ID, refundResult.Status, refundID) } else if status == "failed" { log.Printf("Square refund %s FAILED — marking refund %s failed", refundResult.ID, refundID) @@ -3974,11 +4064,21 @@ func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID return } // Resolve by Square's status — a non-terminal resume (PENDING — money in - // flight — CANCELED, unknown) stays pending for the sweep (marking it - // completed while Square later fails it would block the amount in the - // over-refund guard forever); FAILED/REJECTED is definitive. + // flight — APPROVED, CANCELED, unknown) stays pending for the sweep + // (marking it completed while Square later fails it would block the amount + // in the over-refund guard forever); FAILED/REJECTED is definitive. status, terminal := SquareRefundStatusToLocal(resumeResult.Status) + if resumeResult.Status == "APPROVED" { + // Loop-B finding (MED-HIGH): APPROVED is NON-terminal — a later + // FAILED/CANCELED must still be able to demote the row. + terminal = false + } if !terminal { + if resumeResult.Status == "APPROVED" { + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET square_refund_id = $1 WHERE id = $2`, resumeResult.ID, refundID); upErr != nil { + log.Printf("Failed to record square refund id %s on pending refund %s: %v", resumeResult.ID, refundID, upErr) + } + } log.Printf("Square refund %s is non-terminal (%s) — leaving refund %s pending for the sweep", resumeResult.ID, resumeResult.Status, refundID) } else if status == "failed" { log.Printf("Square refund %s FAILED — marking refund %s failed", resumeResult.ID, refundID) @@ -4348,7 +4448,17 @@ func AdminRefundBooking(w http.ResponseWriter, r *http.Request) { switch { case rErr == nil: sqStatus, terminal := SquareRefundStatusToLocal(result.Status) + if result.Status == "APPROVED" { + // Loop-B finding (MED-HIGH): APPROVED is NON-terminal — a later + // FAILED/CANCELED must still be able to demote the row. + terminal = false + } if !terminal { + if result.Status == "APPROVED" { + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET square_refund_id = $1 WHERE id = $2`, result.ID, cf.refundID); upErr != nil { + log.Printf("Failed to record square refund id %s on pending refund %s: %v", result.ID, cf.refundID, upErr) + } + } log.Printf("Square refund %s is non-terminal (%s) — leaving refund %s pending for the sweep", result.ID, result.Status, cf.refundID) } else if sqStatus == "failed" { log.Printf("Square refund %s FAILED — marking refund %s failed", result.ID, cf.refundID) diff --git a/backend/handlers/payments/loop_a_money_fixes_test.go b/backend/handlers/payments/loop_a_money_fixes_test.go index 8502dcb..8dd219e 100644 --- a/backend/handlers/payments/loop_a_money_fixes_test.go +++ b/backend/handlers/payments/loop_a_money_fixes_test.go @@ -64,51 +64,57 @@ func TestLoopA_PreStartFullWithPendingDiscount_RequiresConfirmation(t *testing.T assert.Zero(t, payCount, "the unconfirmed overflow must not create any payment record") } -// TestLoopA_PreStartDepositWithDiscount_NeverMintsUnintendedTip locks the HIGH-1 -// deposit side: a deposit charge is net of the campaign credit, so a pre-start -// deposit can never exceed the real remaining and never mints an unconfirmed -// tip. A £60 deposit on the £50 booking with a 20% campaign (£10 credit) -// charges exactly the £50 remaining — no tip record. A separate booking with a -// deposit that WOULD overflow (discounted charge > remaining) still requires -// confirmation. -func TestLoopA_PreStartDepositWithDiscount_NeverMintsUnintendedTip(t *testing.T) { +// TestLoopA_PreStartDepositWithDiscount_OverflowRequiresConfirmation locks the +// Loop-B A6 finding on the HIGH-1 deposit side: a deposit-with-discount charge +// is clamped DOWN to the discounted obligation (remaining − discount), and the +// overflow guard compares the RAW request against that obligation. A £60 +// deposit on the £50 booking with a 20% campaign (£10 credit) requests £60 +// against a £40 discounted obligation — it MUST require confirmation (the old +// guard compared the discounted charge against the real £50 remaining, accepted +// it and silently truncated the discount). On confirmation the full £60 is +// charged and the £10 excess (beyond the real remaining) is carved out as a tip +// record — never absorbed as service revenue. +func TestLoopA_PreStartDepositWithDiscount_OverflowRequiresConfirmation(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, bookingID, _ := setupTestData(t, ctx, tx) userToken := jwt.GenerateUserToken(userID) seedActiveCampaign(t, ctx, tx, 20) - cardToken := "cnon:loop-a-deposit-no-tip" + cardToken := "cnon:loop-a-deposit-overflow" req := CreateBookingPaymentRequest{ - Amount: 6000, // £60 deposit; chargeAmount = £50 (remaining) + Amount: 6000, // £60 deposit; discounted obligation is £40 PaymentType: "deposit", NewCardToken: &cardToken, - IdempotencyKey: "loop-a-deposit-no-tip-" + bookingID, + IdempotencyKey: "loop-a-deposit-overflow-" + bookingID, } handler := CreateBookingPayment w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, "a deposit whose discounted charge equals the remaining obligation must be accepted, body: %s", w.Body.String()) + require.Equal(t, http.StatusBadRequest, w.Code, "a deposit beyond the discounted obligation must require confirmation, body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "overflow_tip_confirmation_required") + + // No payment record may be written for the unconfirmed overflow. + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount)) + assert.Zero(t, payCount, "the unconfirmed overflow must not create any payment record") + + // Confirmed: the full £60 is charged and the £10 excess (60 − 50 real + // remaining) is carved out as a tip record, never absorbed as service + // revenue. + req.ConfirmOverflowTip = true + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "a confirmed deposit overflow must proceed, body: %s", w2.Body.String()) + + var bookingPortion float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type != 'tip'`, bookingID).Scan(&bookingPortion)) + assert.InDelta(t, 50.0, bookingPortion, 0.001, "the booking portion must total the £50 obligation") - // No tip may be carved: the discounted charge (£50) is fully booking money. var tipCount int - require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'`, bookingID).Scan(&tipCount)) - assert.Zero(t, tipCount, "a pre-start deposit-with-discount must never mint a tip") - - // A deposit that WOULD overflow into a tip requires confirmation: £61 - // deposit → chargeAmount £51 > remaining £50 → confirmation needed. - userID2, bookingID2, _ := setupTestData(t, ctx, tx) - userToken2 := jwt.GenerateUserToken(userID2) - seedActiveCampaign(t, ctx, tx, 20) - req2 := CreateBookingPaymentRequest{ - Amount: 6100, - PaymentType: "deposit", - NewCardToken: &cardToken, - IdempotencyKey: "loop-a-deposit-overflow-" + bookingID2, - } - w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID2+"/payment", req2, userToken2, ctx) - require.Equal(t, http.StatusBadRequest, w2.Code, "a deposit whose discounted charge exceeds the remaining must require confirmation, body: %s", w2.Body.String()) - assert.Contains(t, w2.Body.String(), "overflow_tip_confirmation_required") + var tipAmount float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*), COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'tip'`, bookingID).Scan(&tipCount, &tipAmount)) + assert.Equal(t, 1, tipCount, "the £10 excess must be carved out as a tip record") + assert.InDelta(t, 10.0, tipAmount, 0.001, "the tip must equal the £10 excess") } // ============================================================================= diff --git a/backend/handlers/payments/loop_b_money_fixes_round2_test.go b/backend/handlers/payments/loop_b_money_fixes_round2_test.go new file mode 100644 index 0000000..291061d --- /dev/null +++ b/backend/handlers/payments/loop_b_money_fixes_round2_test.go @@ -0,0 +1,289 @@ +//go:build test && dev + +package payments + +// ============================================================================= +// LOOP B — Round-2 money findings. Each test pins a fixed behaviour and would +// fail on the pre-fix code. +// ============================================================================= + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// Finding 1 — B1 re-poll race: a webhook-promoted 'completed' sweepdup refund +// whose PARENT payment row is still pending must still be resolved by the B1 +// re-poll pass, and the sweep's in-flight guard must keep treating such a +// completed refund as in-flight (never re-replaying the expired key). +// ============================================================================= + +// seedCompletedB1RefundAndPendingParent seeds a payments-table B1 sweep +// auto-refund row with status 'completed' (as the webhook's COMPLETED +// promotion leaves it) on a still-pending parent payment — the stranded +// state the re-poll pass must resolve. +func seedCompletedB1RefundAndPendingParent(t *testing.T, ctx context.Context, tx db.Querier, userID, bookingID string, amount float64, squareRefundID, refundKey string) (paymentID, refundID string) { + t.Helper() + pid, err := fixtures.CreateTestPayment(tx, bookingID, amount, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create pending parent payment: %v", err) + } + var rid string + err = tx.QueryRow(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, square_refund_id, status, origin, reason, idempotency_key, created_by, created_at) + VALUES ($1, $2, $3, $4, 'completed', 'manual', $5, $6, $7, NOW()) + RETURNING id + `, pid, bookingID, amount, squareRefundID, sweepDuplicateRefundReason, refundKey, userID).Scan(&rid) + if err != nil { + t.Fatalf("failed to insert webhook-completed B1 refund row: %v", err) + } + return pid, rid +} + +// TestHasInFlightSweepDuplicateRefund_CompletedRefund_StillInFlight locks the +// in-flight guard widening (sweep.go hasInFlightSweepDuplicateRefund): a B1 +// sweepdup refund a webhook promoted to 'completed' — while the parent payment +// row is still pending — must STILL count as in-flight. Before the fix the +// guard matched only 'pending', so the next sweep re-replayed the expired key +// and minted ANOTHER charge before the re-poll pass resolved the parent. The +// failed-refund guard must stay false for a completed (not failed) refund so +// the row is never blind-failed on the replay path. +func TestHasInFlightSweepDuplicateRefund_CompletedRefund_StillInFlight(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID) + require.NoError(t, err) + + paymentID, _ := seedCompletedB1RefundAndPendingParent(t, ctx, tx, userID, bookingID, 50.00, "ref_b1_wbhk_inflight", "sweepdup-pay_dup_inflight") + + assert.True(t, hasInFlightSweepDuplicateRefund(ctx, "payments", paymentID), + "a webhook-completed sweepdup refund on a pending parent must still count as in-flight") + assert.False(t, hasFailedSweepDuplicateRefund(ctx, "payments", paymentID), + "a completed (not failed) sweepdup refund must NOT trip the failed-refund guard") +} + +// TestSweepPendingB1Refunds_WebhookCompletedRefund_ResolvesParentPayment locks +// the re-poll query widening (refunds.go sweepPendingB1Refunds): a sweepdup +// refund the webhook promoted to 'completed' — whose PARENT payment row is +// still pending — is re-polled and, when Square confirms the refund COMPLETED, +// the parent is finally marked failed. Before the fix the query matched only +// 'pending' refunds, so the completed refund was never resolved and the parent +// stayed pending forever (feeding the sweep replay loop). +func TestSweepPendingB1Refunds_WebhookCompletedRefund_ResolvesParentPayment(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + const squareRefundID = "ref_b1_wbhk_completed" + const refundKey = "sweepdup-pay_dup_wbhk" + paymentID, refundID := seedCompletedB1RefundAndPendingParent(t, ctx, tx, userID, bookingID, 50.00, squareRefundID, refundKey) + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx, "no transaction in context") + require.NoError(t, pgxTx.Commit(ctx), "failed to commit setup tx") + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE id = $1`, refundID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, paymentID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + origClient := SquareClient + SquareClient = &b1RePollStatusClient{SquareClient: square.NewDevClient(), refundID: squareRefundID, status: "COMPLETED"} + defer func() { SquareClient = origClient }() + + freshCtx := context.Background() + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("SweepPendingSquareRefunds failed: %v", err) + } + + var refundStatus string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&refundStatus)) + assert.Equal(t, "completed", refundStatus, "the webhook-completed refund stays completed") + + var parentStatus string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM payments WHERE id = $1`, paymentID).Scan(&parentStatus)) + assert.Equal(t, "failed", parentStatus, "the pending parent must be resolved to failed once the refund settled") +} + +// ============================================================================= +// Finding 3 — a Square APPROVED refund is NON-terminal: the call sites must +// keep the refunds row pending (with the square_refund_id recorded) so a later +// FAILED/CANCELED can demote it, instead of resolving it to 'completed' and +// stranding it. +// ============================================================================= + +// approvedRefundClient answers RefundPayment with Square status APPROVED — +// the ambiguous authorization-only state that must stay pending locally. +type approvedRefundClient struct { + square.SquareClient +} + +func (c *approvedRefundClient) RefundPayment(ctx context.Context, req square.RefundPaymentReq) (*square.RefundResult, error) { + return &square.RefundResult{ID: "ref_approved_test", Status: "APPROVED", Amount: req.Amount, PaymentID: req.PaymentID}, nil +} + +// TestRefundPayment_ApprovedStatus_LeavesRowPending pins the MED-HIGH finding +// at the RefundPayment handler call site: a Square APPROVED refund is NOT +// terminal — resolving it to 'completed' would strand the row (the FAILED +// demotion only demotes 'pending'). The row must stay 'pending' with the +// square_refund_id recorded, and the response must report 'pending'. +func TestRefundPayment_ApprovedStatus_LeavesRowPending(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID) + require.NoError(t, err) + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 100.00, "online_square", "full", "completed") + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_approved' WHERE id = $1", paymentID) + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &approvedRefundClient{SquareClient: square.NewDevClient()} + defer func() { SquareClient = origClient }() + + adminToken := jwt.GenerateTestToken(adminID, "admin") + req := RefundRequest{Amount: 5000, Reason: "customer request", IdempotencyKey: "approved-refund-" + bookingID} + w := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "an APPROVED refund is non-terminal but the handler must still respond 200, body: %s", w.Body.String()) + + var body RefundResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, "pending", body.Status, "the response must report pending for an APPROVED refund") + + var status, sqRefundID string + require.NoError(t, tx.QueryRow(ctx, `SELECT status, COALESCE(square_refund_id, '') FROM refunds WHERE payment_id = $1`, paymentID).Scan(&status, &sqRefundID)) + assert.Equal(t, "pending", status, "an APPROVED refund must leave the row pending, never completed") + assert.Equal(t, "ref_approved_test", sqRefundID, "the square_refund_id must be recorded on the pending row so the re-poll can settle it") +} + +// ============================================================================= +// Finding 6 — guest-booking cash/gift-card terminal charges must write an +// admin_audit_log row (target_user_id NULL) AFTER the money commits. +// ============================================================================= + +// TestCreateTerminalPayment_GuestCash_AuditsWithNullTarget pins the MEDIUM +// finding: a CASH terminal charge on a GUEST booking (user_id NULL) previously +// wrote NO audit row (the `if customerID.Valid` guard skipped it). The audit +// must run with a NULL target_user_id — matching the till flow — after the +// money transaction commits. +func TestCreateTerminalPayment_GuestCash_AuditsWithNullTarget(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + // Guest booking: no account behind it. + _, err = tx.Exec(ctx, `UPDATE bookings SET user_id = NULL, status = 'in_progress' WHERE id = $1`, bookingID) + require.NoError(t, err) + + adminToken := jwt.GenerateTestToken(adminID, "admin") + pm := "cash" + req := CreateTerminalPaymentRequest{ + Amount: 5000, + PaymentType: "full", + PaymentMethod: &pm, + IdempotencyKey: "guest-cash-audit-" + bookingID, + } + w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a guest cash terminal charge must complete, body: %s", w.Body.String()) + + var auditCount int + require.NoError(t, tx.QueryRow(ctx, ` + SELECT COUNT(*) FROM admin_audit_log + WHERE action_type = 'admin_cash_charge' AND target_user_id IS NULL + AND details->>'booking_id' = $1 + `, bookingID).Scan(&auditCount)) + assert.Equal(t, 1, auditCount, "a guest cash charge must audit with a NULL target_user_id") +} + +// ============================================================================= +// Finding 8 — the A6 skip path (deposit fully covered by discount) must bind +// the request's idempotency key to a row so a same-key retry dedups instead of +// re-running and potentially charging the full deposit. +// ============================================================================= + +// TestBookingPayment_DiscountCoveredDeposit_SameKeyRetry_Dedups pins the +// idempotency fix: after a discount-covered deposit skips the Square charge, +// the discount row carries the request's idempotency key. A lost-response +// same-key retry then short-circuits on the completed row — no second skip, no +// Square call, no second discount redemption — where before it re-ran the +// handler and could charge the full deposit once the campaign had exhausted. +func TestBookingPayment_DiscountCoveredDeposit_SameKeyRetry_Dedups(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + seedActiveCampaign(t, ctx, tx, 100) + + origClient := SquareClient + SquareClient = &failOnChargeClient{SquareClient: square.NewDevClient(), t: t} + defer func() { SquareClient = origClient }() + + cardToken := "cnon:deposit-covered-dedup" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "deposit-covered-dedup-" + bookingID, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "the discount-covered deposit must complete, body: %s", w.Body.String()) + + // The skip path bound the request key to the applied discount row. + var keyedDiscountCount int + require.NoError(t, tx.QueryRow(ctx, ` + SELECT COUNT(*) FROM payments + WHERE booking_id = $1 AND payment_method = 'discount' AND idempotency_key = $2 + `, bookingID, req.IdempotencyKey).Scan(&keyedDiscountCount)) + assert.Equal(t, 1, keyedDiscountCount, "the request's idempotency key must be bound to the discount row") + + // Same-key retry: short-circuits on the completed row — no Square charge, + // no additional discount redemption. + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "a same-key retry must dedup to the completed result, body: %s", w2.Body.String()) + + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + assert.Equal(t, 1, discountCount, "the retry must not re-apply (double-redeem) the campaign discount") +} diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index ff2e4bc..5f94c38 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -1356,14 +1356,33 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar } switch { case sqErr == nil: - // Resolve by Square's status: COMPLETED/APPROVED resolves the group; + // Resolve by Square's status: only COMPLETED resolves the group; // non-terminal states (PENDING — a later sweep reconciles them via - // ListPaymentRefunds — CANCELED, unknown) leave the rows pending; - // FAILED/REJECTED is a definitive failure that must not be marked - // completed (that would block the amount in the over-refund guard - // forever). + // ListPaymentRefunds — APPROVED, CANCELED, unknown) leave the rows + // pending; FAILED/REJECTED is a definitive failure that must not be + // marked completed (that would block the amount in the over-refund + // guard forever). sqStatus, sqTerminal := SquareRefundStatusToLocal(sqResult.Status) + if sqResult.Status == "APPROVED" { + // Loop-B finding (MED-HIGH): APPROVED is NON-terminal — the refund + // can still transition to FAILED/CANCELED at Square. Resolving it to + // 'completed' here would strand the row (the FAILED demotion only + // demotes 'pending'). Keep the rows pending with the square_refund_id + // recorded so the sweep re-poll and a later FAILED/CANCELED webhook + // can settle them. COMPLETED stays terminal-completed. + sqTerminal = false + } if !sqTerminal { + if sqResult.Status == "APPROVED" { + // Record the square refund id on the still-pending rows so the + // sweep's re-poll can look the refund up when it settles. + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE refunds SET square_refund_id = $1 + WHERE id = ANY($2) AND status = 'pending' + `, sqResult.ID, idsOf(pending)); upErr != nil { + log.Printf("Failed to record square refund id %s on pending group for charge %s: %v", sqResult.ID, chargeID, upErr) + } + } log.Printf("Square refund %s for charge %s is non-terminal (%s) — leaving refunds pending for the sweep", sqResult.ID, chargeID, sqResult.Status) } else if sqStatus == "failed" { log.Printf("Square refund %s for charge %s FAILED — marking refunds failed", sqResult.ID, chargeID) @@ -1688,7 +1707,9 @@ func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { } // b1PendingRefund is one refunds row for a sweep auto-refund of a -// replay-induced duplicate charge (B1) that Square left PENDING. +// replay-induced duplicate charge (B1) that Square left PENDING — or that a +// webhook promoted to 'completed' while its PARENT payment row is still +// pending (the parent is only ever resolved by this re-poll pass). type b1PendingRefund struct { RefundID string PaymentID string // refunds.payment_id — the parent payment row @@ -1700,11 +1721,17 @@ type b1PendingRefund struct { CreatedBy string CreatedAt time.Time // refunds.created_at — when the auto-refund was recorded (age for escalation) BookingID string // parent payment's booking_id ("" when the row has none) + Status string // refunds.status — 'pending', or 'completed' (webhook-promoted, parent unresolved) } // sweepPendingB1Refunds re-polls refunds rows for sweep auto-refunds of // replay-induced duplicate charges (B1, refundSweepDuplicateCharge in sweep.go) -// that Square left PENDING. The refund is NON-terminal: the parent row must +// that Square left PENDING, AND refunds a webhook promoted to 'completed' +// while the PARENT row is still pending (Loop-B finding: the webhook promotes +// the refund status but the parent is resolved only here — querying only +// 'pending' refunds let a promoted refund slip through, the sweep's in-flight +// guard then missed it and re-replayed the expired key, minting ANOTHER charge). +// The refund is NON-terminal while the parent is pending: the parent row must // stay pending (never marked failed, a funded gift card never clawed back) // until Square settles. When Square reports the refund COMPLETED the parent is // finally resolved — the pending payment marked failed, and a till sale's @@ -1718,11 +1745,20 @@ func sweepPendingB1Refunds(ctx context.Context) (int, error) { rows, err := db.Conn.Query(ctx, ` SELECT r.id, r.payment_id, r.amount, r.square_refund_id, COALESCE(r.idempotency_key, ''), r.reason, COALESCE(p.square_payment_id, ''), COALESCE(r.created_by, ''), - r.created_at, COALESCE(p.booking_id, '') + r.created_at, COALESCE(p.booking_id, ''), r.status FROM refunds r LEFT JOIN payments p ON p.id = r.payment_id - WHERE r.status = 'pending' AND r.square_refund_id IS NOT NULL + WHERE r.square_refund_id IS NOT NULL AND r.origin = 'manual' AND r.reason LIKE 'duplicate charge — sweep replay%' + AND ( + r.status = 'pending' + OR (r.status = 'completed' AND ( + p.status = 'pending' + OR EXISTS (SELECT 1 FROM till_sales ts + WHERE ts.status = 'pending' + AND ts.id = SUBSTRING(r.reason FROM '\(till_sale ([^)]+)\)')) + )) + ) ORDER BY r.id `) if err != nil { @@ -1733,7 +1769,7 @@ func sweepPendingB1Refunds(ctx context.Context) (int, error) { var pending []b1PendingRefund for rows.Next() { var pr b1PendingRefund - if err := rows.Scan(&pr.RefundID, &pr.PaymentID, &pr.Amount, &pr.SquareRefundID, &pr.IdempotencyKey, &pr.Reason, &pr.SquarePaymentID, &pr.CreatedBy, &pr.CreatedAt, &pr.BookingID); err != nil { + if err := rows.Scan(&pr.RefundID, &pr.PaymentID, &pr.Amount, &pr.SquareRefundID, &pr.IdempotencyKey, &pr.Reason, &pr.SquarePaymentID, &pr.CreatedBy, &pr.CreatedAt, &pr.BookingID, &pr.Status); err != nil { log.Printf("Failed to scan B1 sweep auto-refund: %v", err) continue } @@ -1753,14 +1789,19 @@ func sweepPendingB1Refunds(ctx context.Context) (int, error) { // A row already escalated (pending past stalePendingB1RefundAge) is no // longer re-polled — it either resolved terminal (and left the pending // query) or stays pending under the deduped CRITICAL notification for - // manual reconciliation. + // manual reconciliation. A refund a webhook promoted to 'completed' + // AFTER the escalation is re-polled once: the parent can finally be + // resolved (resolveB1RefundCompleted clears the escalation flag). b1EscalatedMu.Lock() escalated := b1Escalated[pr.RefundID] b1EscalatedMu.Unlock() - if escalated { + if escalated && pr.Status == "pending" { log.Printf("B1 refund %s was already escalated (pending over %s) — not re-polling; manual reconciliation holds the parent pending", pr.RefundID, stalePendingB1RefundAge) continue } + if escalated { + log.Printf("B1 refund %s was escalated but a webhook promoted it to completed — re-polling to resolve the parent", pr.RefundID) + } // The Square payment id the refund targets. A till_sale's refund is // attached to the synthetic payments row (square_payment_id = the // duplicate charge); a payments-table refund is attached to the @@ -2263,12 +2304,31 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man // Resolve by Square's status: a synchronous refund response can be // PENDING (money in flight, e.g. an async card network) — marking it // completed while Square later fails it would permanently block that - // amount in the over-refund guard. Only a terminal COMPLETED/APPROVED - // resolves to completed; non-terminal states (PENDING, CANCELED, - // unknown) stay pending for the sweep to reconcile; FAILED/REJECTED - // is a real failure. Mirrors processChargeGroup and the RefundPayment - // handler (handlers.go). + // amount in the over-refund guard. Only a terminal COMPLETED + // resolves to completed; non-terminal states (PENDING, APPROVED, + // CANCELED, unknown) stay pending for the sweep to reconcile; + // FAILED/REJECTED is a real failure. Mirrors processChargeGroup and + // the RefundPayment handler (handlers.go). sqStatus, sqTerminal := SquareRefundStatusToLocal(sqResult.Status) + if sqResult.Status == "APPROVED" { + // Loop-B finding (MED-HIGH): APPROVED is NON-terminal — a later + // FAILED/CANCELED must still be able to demote the row, so it + // stays pending with the square_refund_id recorded. + sqTerminal = false + } + if !sqTerminal { + if sqResult.Status == "APPROVED" { + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE refunds SET square_refund_id = $1 + WHERE id = $2 + `, sqResult.ID, pr.ID); upErr != nil { + log.Printf("Failed to record square refund id %s on pending manual refund %s: %v", sqResult.ID, pr.ID, upErr) + } + } + log.Printf("Square refund %s for manual refund %s is non-terminal (%s) — leaving the row pending for the sweep to resolve", sqResult.ID, pr.ID, sqResult.Status) + } else if sqStatus == "failed" { + log.Printf("Square refund %s for manual refund %s FAILED — marking the row failed", sqResult.ID, pr.ID) + } if !sqTerminal { log.Printf("Square refund %s for manual refund %s is non-terminal (%s) — leaving the row pending for the sweep to resolve", sqResult.ID, pr.ID, sqResult.Status) } else if sqStatus == "failed" { diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 8e5b3f7..845c1f9 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -434,8 +434,8 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r } case staleReconcileDefinitivelyFailedNoClawback: // B1 (CRITICAL-HIGH): the auto-refund of the replay-induced - // duplicate charge was definitively REJECTED at Square — the - // duplicate charge stands and the money is REAL at Square, so a + // duplicate charge was definitively REJECTED (or errored) at Square — + // the duplicate charge stands and the money is REAL at Square, so a // till sale's funded gift card is NOT clawed back. Mark the parent // row failed, raise the CRITICAL notification, and set b1_attempts // to the cap so the expired key is never replayed (each replay @@ -445,6 +445,15 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r } notifyStaleRowCritical(ctx, r) setB1AttemptsToCap(ctx, table, r.ID) + // Loop-B finding (MED): a capped-fail on a till_sale leaves its + // funded gift card outstanding — the duplicate charge at Square is + // REAL and a manual Square refund is the only reversal. Surface the + // outstanding funding with a gift_card_transactions trace so the + // operator sees it (see insertOutstandingGiftCardFundingTrace for + // the manual-resolution path). + if table == "till_sales" && r.HasGiftCard { + insertOutstandingGiftCardFundingTrace(ctx, r) + } } } return resolved, completed, unverifiable, nil @@ -853,6 +862,27 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { return true } +// insertOutstandingGiftCardFundingTrace surfaces a till sale's gift-card funding +// that remains outstanding after a B1 capped-fail (the auto-refund of the +// replay-induced duplicate charge was REJECTED/errored — the duplicate charge +// stands at Square, so the funding is NOT auto-clawed back: reversing money the +// merchant still holds would lose it). The trace row lets the operator see the +// outstanding funding and drives the manual-resolution path: +// +// 1. refund the duplicate charge at Square FIRST (it is real money — the +// customer never authorized it), +// 2. then run clawbackTillSaleFunding (revertGiftCardFunding with the sale's +// action, gift_card_id and amount) to reverse the funding atomically once +// the duplicate is gone. +func insertOutstandingGiftCardFundingTrace(ctx context.Context, r staleRow) { + if _, err := db.Conn.Exec(ctx, ` + INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) + VALUES ($1, 'awaiting_reversal', $2, 'till_sale', $3, $4, $5) + `, r.ItemID, r.TotalAmount, r.ID, r.RedeemToUserID, "B1 capped-fail: duplicate charge stands at Square — refund it there FIRST, then run the till-sale funding clawback manually"); err != nil { + log.Printf("Failed to insert outstanding gift-card funding trace for till sale %s: %v", r.ID, err) + } +} + // replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed // COMPLETED payment to still be treated as the ORIGINAL charge under a retained // key rather than a provably-new duplicate. Rows are inserted pending-first, so @@ -1221,7 +1251,21 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( log.Printf("stale pending %s row %s: the auto-refund of the replay-induced duplicate charge %s was DEFINITIVELY REJECTED at Square (%v) — marking the row failed with a CRITICAL notification; the expired key will never be replayed", table, r.ID, pr.ID, refundErr) return staleReconcileDefinitivelyFailedNoClawback, "" } else { - return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created after the pending row %s (lag %s) — a NEW charge under an expired idempotency key; auto-refund FAILED (%v) — leaving the row PENDING — MANUAL RECONCILIATION REQUIRED: check Square for both charges and refund the duplicate", table, pr.ID, r.ID, lag, refundErr) + // Loop-B finding (MED): the auto-refund ERRORED (transport / + // any non-pending, non-rejected error) — the money state at + // Square is unknown, NO refunds row was written, and the row's + // b1_attempts was already incremented. The OLD code left the + // row pending, so the next run re-replayed the SAME expired key + // and minted ANOTHER charge — the b1_attempts cap of 3 allowed + // up to 3 stacked unauthorized charges. Do NOT re-replay a key + // whose refund attempt errored: fail the parent row + CRITICAL + // immediately (the NoClawback branch — the duplicate charge may + // stand at Square, so a till sale's funding is NOT clawed back; + // the b1_attempts cap is pinned so the key is never replayed). + // A genuinely-ambiguous refund (accepted by Square, left PENDING) + // is already handled by the B1 re-poll pass. + log.Printf("stale pending %s row %s: the auto-refund of the replay-induced duplicate charge %s ERRORED (%v) — marking the row failed with a CRITICAL notification; the expired key will never be replayed", table, r.ID, pr.ID, refundErr) + return staleReconcileDefinitivelyFailedNoClawback, "" } } return staleReconcileCompleted, pr.ID @@ -1422,13 +1466,18 @@ func recordSweepDuplicateRefundRow(ctx context.Context, table string, r staleRow // hasInFlightSweepDuplicateRefund reports whether a stale pending row carries a // sweep auto-refund of a replay-induced duplicate charge (B1) that Square left -// PENDING. While the refund is in flight the row must NOT be replayed (a replay +// PENDING — or that a webhook promoted to 'completed' while the parent is still +// pending. While the refund is in flight the row must NOT be replayed (a replay // of the expired-key row could land ANOTHER charge), blind-failed or clawed -// back (the refund is non-terminal — money may still reverse). The B1 re-poll -// pass (sweepPendingB1Refunds, refunds.go) resolves the parent row when Square -// settles. For a till_sale the refund row's reason carries the sale id (see -// sweepDuplicateRefundReasonFor); a payments-table refund's payment_id IS the -// parent row. +// back (the refund is non-terminal — money may still reverse). Loop-B finding: +// matching only 'pending' let a webhook-promoted 'completed' refund fall off +// the guard, so the next sweep re-replayed the expired key and minted a second +// charge before the re-poll pass could resolve the parent. The parent being +// still pending is implied: the sweep only processes pending rows. The B1 +// re-poll pass (sweepPendingB1Refunds, refunds.go) resolves the parent row when +// Square settles. For a till_sale the refund row's reason carries the sale id +// (see sweepDuplicateRefundReasonFor); a payments-table refund's payment_id IS +// the parent row. func hasInFlightSweepDuplicateRefund(ctx context.Context, table, id string) bool { var exists bool var err error @@ -1436,7 +1485,7 @@ func hasInFlightSweepDuplicateRefund(ctx context.Context, table, id string) bool err = db.Conn.QueryRow(ctx, ` SELECT EXISTS ( SELECT 1 FROM refunds - WHERE status = 'pending' AND square_refund_id IS NOT NULL + WHERE status IN ('pending', 'completed') AND square_refund_id IS NOT NULL AND reason = $1 ) `, sweepDuplicateRefundReasonFor(id)).Scan(&exists) @@ -1444,7 +1493,7 @@ func hasInFlightSweepDuplicateRefund(ctx context.Context, table, id string) bool err = db.Conn.QueryRow(ctx, ` SELECT EXISTS ( SELECT 1 FROM refunds - WHERE payment_id = $1 AND status = 'pending' AND square_refund_id IS NOT NULL + WHERE payment_id = $1 AND status IN ('pending', 'completed') AND square_refund_id IS NOT NULL AND reason = $2 ) `, id, sweepDuplicateRefundReason).Scan(&exists) diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 93b50c0..0cc1de5 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -302,26 +302,23 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID // 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 + // log a CRITICAL line AND raise the per-issue-capped 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) - // Finding 1 (Round 2 Loop A): bound the operator-facing flood. The - // deduped insert (sweep.go's insertCriticalPaymentNotification, keyed on - // reason/booking_id/user_id) is unbounded across attacker-registered - // accounts, so a hostile flood of failed re-issues could bury the - // single-operator notification centre. The global cap skips the insert — - // the CRITICAL log line above still fires, so no alert information is - // lost to the operator's log pipeline — once - // maxUnacknowledgedNotifications unacknowledged 'critical_payment_log' - // rows exist. Acknowledging rows re-arms inserts. - if notificationsCapExceeded(ctx, "critical_payment_log") { - log.Printf("2FA: critical-payment admin notification suppressed for user %s — %d unacknowledged 'critical_payment_log' notifications already exist; acknowledge outstanding notifications to re-arm", userID, maxUnacknowledgedNotifications) - return - } - insertCriticalPaymentNotification(ctx, nil, &userID) + // Round 2 Loop B findings 2 + 7 — the alert is capped PER-ISSUE, NOT + // globally (see alertReissueFail below): at most ONE unacknowledged row + // per stranded customer, so one customer's alert can never be + // suppressed by OTHER users' rows filling the 'critical_payment_log' + // bucket, and the count-then-insert is atomic (a single INSERT ... WHERE + // NOT EXISTS — no TOCTOU). An attacker also cannot FLOOD the alert: + // raising it requires a real saved-card charge that consumed a real code + // AND a failed re-issue, and the per-issue dedup holds each user to one + // row until acknowledged. Fail-closed behaviour is unchanged (the + // CRITICAL log always fires); only the notification INSERT is bounded. + alertReissueFail(ctx, q, userID) return } // Mint cooldown (B11a + Round 2 Loop A finding 2): the shared per-user mutex @@ -337,7 +334,18 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID st.Mu.Lock() defer st.Mu.Unlock() if !st.LastMintAt.IsZero() && clock.Now().Sub(st.LastMintAt) < twoFAMintCooldown { - log.Printf("2FA: re-issue skipped for user %s after a failed charge (mint cooldown)", userID) + // Round 2 Loop B finding 6b: this skip was SILENT before. A FRESH + // charge consumed the customer's code at the gate (single-use) and the + // charge failed; the re-issue is now skipped by the per-user mint + // cooldown — the customer holds NO live code for the same-key retry + // until the cooldown lapses. That is a stranded customer, so raise the + // same per-issue-capped reissue-fail alert the refused-issue branch + // above uses (alertReissueFail, deduped on reason+user_id) so the + // operator knows to mint a code manually. The alert is per-issue + // (finding 2): repeated skips for the same customer stay ONE row until + // acknowledged and can never be suppressed by other users' rows. + log.Printf("2FA: re-issue skipped for user %s after a failed charge (mint cooldown) — the customer's code was consumed by the fresh charge and NO live code remains until the cooldown lapses", userID) + alertReissueFail(ctx, q, userID) return } code, err := generatePaymentsTwoFACode() @@ -354,7 +362,7 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID log.Printf("2FA: failed to store a re-issued code for user %s after a failed charge: %v", userID, err) return } - st.LastMintAt = clock.Now() + st.SetLastMintAtLocked(clock.Now()) // Delivery mirrors the user package's build-dependent behaviour (the // operator relays the [2FA] log line). Production logs the plaintext code // only when explicitly opted in; dev/test always. @@ -375,16 +383,49 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID // actually bounds a charge-failure loop: after a fresh charge consumed a code // at the gate and failed, the re-issue is skipped while the last mint is // inside the cooldown (logged, not silent), bounding code churn + dev log -// flooding. COORDINATION (money agent): the FRESH-charge terminal-success path -// in handlers.go does NOT call ConsumePendingCode (the gate already burned the -// code with consume=true), so its stamp survives — a customer who completes a -// fresh charge within the cooldown of their last mint and immediately requests -// a new code gets 429 until the window elapses. That is the intended bounded -// behaviour; if immediate re-mint after a completed fresh charge is wanted, -// the money agent should clear the stamp there (twofa.ClearMintCooldownForUser -// or ConsumePendingCode) on terminal success. +// flooding. Round 2 Loop B finding 6a — COORDINATION (money agent): the +// FRESH-charge terminal-success path in handlers.go does NOT call +// ConsumePendingCode (the gate already burned the code with consume=true), so +// its mint-cooldown stamp survives — a customer who completes a fresh charge +// within the cooldown of their last mint and immediately requests a new code +// gets 429 until the window elapses. To re-arm immediate re-minting after a +// completed fresh charge, the money agent should call +// twofa.ClearMintCooldownForUser(userID) on that terminal-success path (the +// exported, coordination-actionable entry point documented on +// internal/twofa.ClearMintCooldownForUser). const twoFAMintCooldown = 1 * time.Minute +// alertReissueFail surfaces a stranded-customer incident in the admin +// notification centre (reason 'critical_payment_log'). Round 2 Loop B finding +// 2: it is capped PER-ISSUE, NOT globally — the NOT EXISTS guard keeps exactly +// ONE unacknowledged row per (reason, user_id) (the booking is unresolvable at +// re-issue time), so one customer's alert is never suppressed by other users' +// rows filling the 'critical_payment_log' bucket, and the count-then-insert is +// atomic (a single INSERT ... WHERE NOT EXISTS — no TOCTOU). It is a dedicated +// local insert rather than the sweep's insertCriticalPaymentNotification so +// that the money agent's upcoming GLOBAL cap on the sweep insert (finding 1) +// can never swallow this alert — a stranded customer must always surface. +// Best-effort: a failure logs and the caller's CRITICAL log line still fires. +func alertReissueFail(ctx context.Context, q db.Querier, userID string) { + tag, err := q.Exec(ctx, ` + INSERT INTO admin_notifications (reason, user_id, created_at) + SELECT 'critical_payment_log', $1, NOW() + WHERE NOT EXISTS ( + SELECT 1 FROM admin_notifications an + WHERE an.reason = 'critical_payment_log' + AND an.user_id = $1 + AND an.acknowledged_at IS NULL + ) + `, userID) + if err != nil { + log.Printf("2FA: failed to insert critical-payment admin notification for reissue failure (user=%s): %v", userID, err) + return + } + if tag.RowsAffected() > 0 { + log.Printf("2FA: inserted critical-payment admin notification for reissue failure (user=%s) — customer stranded after a failed fresh saved-card charge", userID) + } +} + // generatePaymentsTwoFACode returns a random 6-digit verification code, // mirroring the user package's generator (crypto/rand, uniform 0-999999). func generatePaymentsTwoFACode() (string, error) { @@ -399,54 +440,41 @@ func generatePaymentsTwoFACode() (string, error) { // mirroring the user package's pending-code expiry. const twoFAPendingCodeLifetime = 10 * time.Minute -// maxUnacknowledgedNotifications is the GLOBAL cap on unacknowledged -// admin_notifications rows for one reason (Round 2 Loop A finding 1). The -// dedup guards keyed on (reason, booking_id, user_id) are bounded per issue but -// UNBOUNDED across attacker-registered accounts, so a hostile flood could bury -// the single-operator notification centre. Insert sites check -// notificationsCapExceeded before inserting and skip the row (the CRITICAL log -// line still fires) once the unacknowledged queue for that reason is at the -// cap — the operator acknowledges rows to re-arm. -const maxUnacknowledgedNotifications = 100 - -// notificationsCapExceeded reports whether the number of unacknowledged -// admin_notifications rows for reason has reached maxUnacknowledgedNotifications. -// Best-effort and fail-OPEN: a count error is logged and the cap is NOT -// enforced (a money alert must never be dropped because the count query failed). -func notificationsCapExceeded(ctx context.Context, reason string) bool { - var n int - err := db.Conn.QueryRow(ctx, ` - SELECT COUNT(*) FROM admin_notifications - WHERE reason = $1::admin_notification_reason AND acknowledged_at IS NULL - `, reason).Scan(&n) - if err != nil { - log.Printf("2FA: failed to count unacknowledged %s admin notifications: %v", reason, err) - return false - } - return n >= maxUnacknowledgedNotifications -} - -// COORDINATION NOTE (Round 2 Loop A finding 1) — the cap pattern must be -// applied by the other two insert sites this finding calls out, which live in -// files owned by other agents: +// COORDINATION NOTE (Round 2 Loop B finding 1) — the shared notification flood +// cap now lives in crussell/internal/adminnotify +// (MaxUnacknowledgedCriticalLogs = 100 + CriticalLogsCapExceeded), applied +// ATOMICALLY (a conditional `INSERT ... SELECT ... WHERE (SELECT COUNT(*) ...) +// < $cap`) at every insert site this finding calls out. Status of each site: // -// - handlers/payments/sweep.go:1414 insertCriticalPaymentNotification (the -// money agent): its INSERT ... WHERE NOT EXISTS dedup is keyed on (reason, -// booking_id, user_id) and is the same unbounded-across-accounts shape. -// Fold the global guard into the SELECT: `AND (SELECT COUNT(*) FROM -// admin_notifications WHERE reason = 'critical_payment_log' AND -// acknowledged_at IS NULL) < 100`. +// - THIS package's reissue-fail alert (reissueTwoFACodeAfterFailedCharge): +// capped PER-ISSUE instead (Round 2 Loop B finding 2) — the local +// alertReissueFail helper dedups atomically on (reason, user_id), so one +// customer's alert is never suppressed by other users' rows. NOT globally +// capped, and deliberately decoupled from sweep's generic insert so the +// money agent's global cap there can never swallow it. // -// - auth/jwt.go:666 VerifyRefreshToken's 'refresh_token_reuse' alert (the -// auth agent): dedups on (reason, user_id) only, so a single attacker -// replaying MANY rotated families can flood the same single-operator -// centre. Apply the identical cap for reason 'refresh_token_reuse' (its -// insert is also `INSERT ... SELECT ... WHERE NOT EXISTS`, so the same -// COUNT subquery folds in). +// - auth/jwt.go VerifyRefreshToken's 'refresh_token_reuse' alert: NOW capped +// (Round 2 Loop B finding 1) — folded into its INSERT via the shared +// adminnotify cap. // -// This helper lives in payments/twofa.go because that is where this task's -// reissue-fail alert (reissueTwoFACodeAfterFailedCharge, above) applies it; the -// other sites copy the pattern since the auth/jobs packages cannot import -// payments. Fail-closed behaviour (a REFUSED re-issue still CRITICAL-logs and -// leaves the operator to mint manually) is unchanged — only the notification -// INSERT is bounded. +// - handlers/webhooks/square.go (dispute/booking/unknown-event/orphan-replay) +// and handlers/user/account.go InsertSquareErasureCriticalNotification: +// NOW capped — same atomic fold. +// +// - handlers/payments/sweep.go:1556 insertCriticalPaymentNotification (the +// MONEY agent): STILL NEEDS the fold. Its INSERT ... SELECT ... WHERE NOT +// EXISTS is the same unbounded-across-accounts shape. Add +// `AND (SELECT COUNT(*) FROM admin_notifications _an WHERE _an.reason = +// 'critical_payment_log' AND _an.acknowledged_at IS NULL) < $N` (N = +// adminnotify.MaxUnacknowledgedCriticalLogs) to its WHERE clause. +// +// - handlers/scheduling/time-blockers.go:511 and internal/jobs/cleanup.go:333 +// (jobs/scheduling agent): same shape on the GDPR-cleanup and log-scan +// paths — apply the identical fold. +// +// - main.go has NO admin_notifications insert sites (it only mounts the +// notification read/ack routes), so nothing to cap there. +// +// Fail-closed behaviour (a REFUSED re-issue still CRITICAL-logs and leaves the +// operator to mint manually) is unchanged — only the notification INSERT is +// bounded. diff --git a/backend/handlers/payments/twofa_gate_consume_test.go b/backend/handlers/payments/twofa_gate_consume_test.go index 8296477..c8d4665 100644 --- a/backend/handlers/payments/twofa_gate_consume_test.go +++ b/backend/handlers/payments/twofa_gate_consume_test.go @@ -336,6 +336,15 @@ func TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown(t *testing.T) { require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) require.Equal(t, firstHash, hash.String, "a re-issue inside the mint cooldown must be a no-op (the stored code is untouched)") + // Round 2 Loop B finding 6b: the cooldown-skipped re-issue must NOT be + // silent — the fresh charge consumed the customer's code at the gate, so + // they have NO live code for the same-key retry until the cooldown lapses. + // The per-issue-capped reissue-fail alert raises so the operator knows the + // customer is stranded (deduped on reason+user_id: one row per customer). + var alertCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1 AND acknowledged_at IS NULL`, userID).Scan(&alertCount)) + require.Equal(t, 1, alertCount, "a cooldown-skipped re-issue after a fresh consumed charge must raise the reissue-fail alert (finding 6b)") + st := twofa.StateFor(userID) st.Mu.Lock() st.LastMintAt = time.Time{} diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go index 6896e84..8e53813 100644 --- a/backend/handlers/payments/twofa_test.go +++ b/backend/handlers/payments/twofa_test.go @@ -21,6 +21,7 @@ import ( "testing" "crussell/db" + "crussell/internal/adminnotify" "crussell/internal/twofa" "crussell/mw" "crussell/testutils" @@ -833,33 +834,39 @@ func TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue(t *testing.T) { require.True(t, twoFADeliveryAvailable()) } -// TestNotificationsCapExceeded pins finding 1: the GLOBAL cap on unacknowledged -// 'critical_payment_log' admin notifications (maxUnacknowledgedNotifications) -// suppresses new inserts once the unacknowledged queue reaches the cap, so a -// hostile flood of attacker-registered accounts cannot bury the single-operator -// notification centre. Acknowledging rows re-arms inserts. The reissue-fail -// alert site (twofa.go) checks this helper before calling the sweep's -// insertCriticalPaymentNotification; the same pattern is documented for the -// sweep.go and auth/jwt.go insert sites (see the coordination note in twofa.go). +// TestNotificationsCapExceeded pins Round 2 Loop B finding 1: the GLOBAL cap on +// unacknowledged 'critical_payment_log' admin notifications +// (adminnotify.MaxUnacknowledgedCriticalLogs, the shared cap living in +// crussell/internal/adminnotify) suppresses new inserts once the unacknowledged +// queue reaches the cap, so a hostile flood of attacker-registered accounts +// cannot bury the single-operator notification centre. Acknowledging rows +// re-arms inserts. The same cap is now applied atomically (a conditional +// INSERT ... SELECT ... WHERE (SELECT COUNT(*) ...) < $cap) at every +// 'critical_payment_log' / 'refresh_token_reuse' insert site (webhooks, +// account erasure, jwt reuse); this test pins the shared count helper. NOTE: +// the REISSUE-FAIL alert (reissueTwoFACodeAfterFailedCharge) is intentionally +// NOT globally capped — Round 2 Loop B finding 2 caps it PER-ISSUE (dedup on +// reason+user_id) so one customer's alert is never suppressed by other users' +// rows. func TestNotificationsCapExceeded(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) // Empty queue → below the cap, inserts allowed. - require.False(t, notificationsCapExceeded(ctx, "critical_payment_log")) + require.False(t, adminnotify.CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log")) // Fill the unacknowledged queue to the cap. _, err = tx.Exec(ctx, `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'`) require.NoError(t, err) - for i := 0; i < maxUnacknowledgedNotifications; i++ { + for i := 0; i < adminnotify.MaxUnacknowledgedCriticalLogs; i++ { _, err = tx.Exec(ctx, ` INSERT INTO admin_notifications (reason, user_id, created_at) VALUES ('critical_payment_log', $1, NOW()) `, userID) require.NoError(t, err) } - require.True(t, notificationsCapExceeded(ctx, "critical_payment_log"), "at the cap the insert must be suppressed") + require.True(t, adminnotify.CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log"), "at the cap the insert must be suppressed") // Acknowledging one row drops below the cap → inserts re-arm. _, err = tx.Exec(ctx, ` @@ -871,5 +878,5 @@ func TestNotificationsCapExceeded(t *testing.T) { ) `) require.NoError(t, err) - require.False(t, notificationsCapExceeded(ctx, "critical_payment_log"), "acknowledging one row must re-arm inserts") + require.False(t, adminnotify.CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log"), "acknowledging one row must re-arm inserts") } diff --git a/backend/handlers/user/account.go b/backend/handlers/user/account.go index 1e93446..4bc5ede 100644 --- a/backend/handlers/user/account.go +++ b/backend/handlers/user/account.go @@ -18,6 +18,7 @@ import ( "crussell/auth" "crussell/db" "crussell/handlers/payments" + "crussell/internal/adminnotify" "crussell/internal/dav" "crussell/internal/s3" "crussell/internal/square" @@ -142,11 +143,21 @@ func InsertSquareErasureCriticalNotification(ctx context.Context, key string) { // the admin alert. actx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() + // Round 2 Loop B finding 1: this is a 'critical_payment_log' insert site — + // the same atomic global cap as every other. The pre-check logs the + // suppression; the fold inside the INSERT enforces it atomically. + if adminnotify.CriticalLogsCapExceeded(actx, db.Conn, "critical_payment_log") { + slog.Error("failed to insert critical notification for failed Square erasure — unacknowledged 'critical_payment_log' queue at the cap", "key", key) + return + } tag, err := db.Conn.Exec(actx, ` INSERT INTO admin_notifications (id, reason, booking_id, user_id, created_at) - VALUES ($1, 'critical_payment_log'::admin_notification_reason, NULL, NULL, NOW()) + SELECT $1, 'critical_payment_log'::admin_notification_reason, NULL, NULL, NOW() + WHERE (SELECT COUNT(*) FROM admin_notifications _an + WHERE _an.reason = 'critical_payment_log' + AND _an.acknowledged_at IS NULL) < $2 ON CONFLICT (id) DO NOTHING - `, squareErasureNotificationID(key)) + `, squareErasureNotificationID(key), adminnotify.MaxUnacknowledgedCriticalLogs) if err != nil { slog.Error("failed to insert critical notification for failed Square erasure", "key", key, "err", err) return diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 433ed37..5ef48d8 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -277,7 +277,7 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "server error", http.StatusInternalServerError) return } - st.LastMintAt = now + st.SetLastMintAtLocked(now) resp := map[string]any{"message": "Code sent"} if !twoFARequired() { @@ -840,7 +840,7 @@ func EnsurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptStat if err != nil { return "", 0, err } - st.LastMintAt = now + st.SetLastMintAtLocked(now) return code, twoFAPendingExpiry, nil } diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 572162d..34577d4 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -20,6 +20,7 @@ import ( "crussell/db" "crussell/handlers/payments" + "crussell/internal/adminnotify" "github.com/jackc/pgx/v5" ) @@ -670,13 +671,26 @@ func disputeNotificationID(squareDisputeID string) string { // collapse every untracked dispute onto one unacknowledged NULL-booking row. // The caller supplies a bounded context (webhookDBContext). func insertCriticalPaymentNotification(ctx context.Context, bookingID, disputeID string) { + // Round 2 Loop B finding 1: apply the global cap to this insert site too + // (the pre-check logs the suppression; the fold inside each INSERT enforces + // it atomically so concurrent events cannot overshoot together). The + // unacknowledged 'critical_payment_log' queue is shared by every insert + // site in the codebase (sweep, webhook, account-erasure, jobs), so a flood + // at any of them must not bury the single-operator notification centre. + if adminnotify.CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log") { + log.Printf("[SQUARE-WEBHOOK] critical_payment_log admin notification suppressed (booking=%q dispute=%q) — %d unacknowledged rows at the cap; acknowledge outstanding notifications to re-arm", bookingID, disputeID, adminnotify.MaxUnacknowledgedCriticalLogs) + return + } if disputeID != "" { id := disputeNotificationID(disputeID) tag, err := db.Conn.Exec(ctx, ` INSERT INTO admin_notifications (id, reason, booking_id, created_at) - VALUES ($1, 'critical_payment_log'::admin_notification_reason, NULL, NOW()) + SELECT $1, 'critical_payment_log'::admin_notification_reason, NULL, NOW() + WHERE (SELECT COUNT(*) FROM admin_notifications _an + WHERE _an.reason = 'critical_payment_log' + AND _an.acknowledged_at IS NULL) < $2 ON CONFLICT (id) DO NOTHING - `, id) + `, id, adminnotify.MaxUnacknowledgedCriticalLogs) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to insert critical_payment_log admin notification: %v", err) return @@ -699,7 +713,10 @@ func insertCriticalPaymentNotification(ctx context.Context, bookingID, disputeID AND an.booking_id IS NOT DISTINCT FROM $1 AND an.acknowledged_at IS NULL ) - `, bid) + AND (SELECT COUNT(*) FROM admin_notifications _an + WHERE _an.reason = 'critical_payment_log' + AND _an.acknowledged_at IS NULL) < $2 + `, bid, adminnotify.MaxUnacknowledgedCriticalLogs) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to insert critical_payment_log admin notification: %v", err) return @@ -735,12 +752,22 @@ func unknownEventNotificationID(eventID string) string { // failure is logged, never a dispatch error (the 501 is already the response). // The caller supplies a bounded context (webhookDBContext). func insertUnknownEventNotification(ctx context.Context, eventType, eventID string) { + // Round 2 Loop B finding 1: same atomic global cap as every other + // 'critical_payment_log' insert site — the pre-check logs the suppression, + // the INSERT fold enforces it atomically. + if adminnotify.CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log") { + log.Printf("[SQUARE-WEBHOOK] critical_payment_log admin notification suppressed for unhandled event %q (event_id=%s) — %d unacknowledged rows at the cap; acknowledge outstanding notifications to re-arm", eventType, eventID, adminnotify.MaxUnacknowledgedCriticalLogs) + return + } id := unknownEventNotificationID(eventID) tag, err := db.Conn.Exec(ctx, ` INSERT INTO admin_notifications (id, reason, booking_id, created_at) - VALUES ($1, 'critical_payment_log'::admin_notification_reason, NULL, NOW()) + SELECT $1, 'critical_payment_log'::admin_notification_reason, NULL, NOW() + WHERE (SELECT COUNT(*) FROM admin_notifications _an + WHERE _an.reason = 'critical_payment_log' + AND _an.acknowledged_at IS NULL) < $2 ON CONFLICT (id) DO NOTHING - `, id) + `, id, adminnotify.MaxUnacknowledgedCriticalLogs) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to insert critical_payment_log admin notification for unhandled event %q (event_id=%s): %v", eventType, eventID, err) return @@ -874,6 +901,12 @@ func insertOrphanReplayChargeNotification(ctx context.Context, paymentID, bookin if paymentID == "" { return } + // Round 2 Loop B finding 1: same atomic global cap as every other + // 'critical_payment_log' insert site. + if adminnotify.CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log") { + log.Printf("[SQUARE-WEBHOOK] orphaned-replay admin notification suppressed (origin payment=%s) — %d unacknowledged rows at the cap; acknowledge outstanding notifications to re-arm", paymentID, adminnotify.MaxUnacknowledgedCriticalLogs) + return + } id := orphanReplayChargeNotificationID(paymentID) var bid any if bookingID != "" { @@ -881,9 +914,12 @@ func insertOrphanReplayChargeNotification(ctx context.Context, paymentID, bookin } tag, err := db.Conn.Exec(ctx, ` INSERT INTO admin_notifications (id, reason, booking_id, created_at) - VALUES ($1, 'critical_payment_log'::admin_notification_reason, $2, NOW()) + SELECT $1, 'critical_payment_log'::admin_notification_reason, $2, NOW() + WHERE (SELECT COUNT(*) FROM admin_notifications _an + WHERE _an.reason = 'critical_payment_log' + AND _an.acknowledged_at IS NULL) < $3 ON CONFLICT (id) DO NOTHING - `, id, bid) + `, id, bid, adminnotify.MaxUnacknowledgedCriticalLogs) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to insert orphaned-replay admin notification: %v", err) return @@ -1149,7 +1185,20 @@ func handleRefundUpdated(data json.RawMessage) error { return fmt.Errorf("refund.updated payload for data.id=%q missing/invalid refund object (id=%q status=%q): %w", env.ID, refund.ID, refund.Status, errWebhookParseFailure) } // Single shared Square → local refund-status mapping (payments package) so - // the webhook and the synchronous refund handlers can never drift. + // the webhook and the synchronous refund handlers can never drift. One + // deliberate webhook-only override: APPROVED. The shared mapping maps + // APPROVED→('completed', true) for the SYNCHRONOUS refund handlers, whose + // blocking APPROVED result is final. The webhook is event-driven — an + // APPROVED refund is still in flight at Square and may settle COMPLETED or + // FAILED afterwards. Promoting on APPROVED would stick the row at + // 'completed', and the FAILED demotion below only demotes 'pending' rows + // (the over-refund guard counts completed refunds — demoting would exclude + // money that already moved). Leave the row 'pending'; the Square status is + // logged for the audit trail. + if refund.Status == "APPROVED" { + log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s status APPROVED is non-terminal for the webhook (Square may still settle COMPLETED or FAILED) — leaving the local row pending", refund.ID) + return nil + } localStatus, terminal := payments.SquareRefundStatusToLocal(refund.Status) if !terminal { log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s status %q is non-terminal — no local state change", refund.ID, refund.Status) @@ -1170,6 +1219,18 @@ func handleRefundUpdated(data json.RawMessage) error { if tag.RowsAffected() > 0 { log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s → local status %s", refund.ID, localStatus) } + // B1 sweep-dup refunds: a COMPLETED promotion must ALSO resolve the + // parent payment/till_sale row. The B1 re-poll pass (sweepPendingB1Refunds, + // refunds.go) only queries refunds rows still 'pending' — once this + // promotion flips the row to 'completed' the re-poll can never see it + // again, so the parent would stay pending forever and the stale-pending + // sweep would re-replay the expired idempotency key each run, minting a + // new charge every 5 minutes (HIGH, B1). Runs on every COMPLETED event + // so a row stranded by an earlier path is healed too; idempotent — the + // parent UPDATE is status='pending'-guarded and the clawback is claim-first. + if err := resolveSweepDupRefundParent(ctx, refund.ID); err != nil { + return err + } return nil case "failed": // A5d: this webhook demotes a 'pending' row to 'failed' BEFORE the sweep @@ -1199,6 +1260,91 @@ func handleRefundUpdated(data json.RawMessage) error { return nil } +// resolveSweepDupRefundParent resolves the parent row of a B1 sweep auto-refund +// of a replay-induced duplicate charge when the webhook promotes the refund to +// COMPLETED. The B1 re-poll pass (sweepPendingB1Refunds, refunds.go) only +// processes refunds rows still 'pending', so once THIS handler promotes the +// row to 'completed' the re-poll can never resolve the parent again — and a +// still-pending parent lets the stale-pending sweep re-replay its expired +// idempotency key every 5-minute run, minting a NEW charge each time (HIGH). +// This replicates the re-poll's resolveB1ParentFailed semantics here: +// +// - a payments-table refund (reason exactly "duplicate charge — sweep replay") +// is attached to the still-pending parent payment row — payment_id IS that +// row, so it is marked failed; +// - a till_sale refund carries the parent sale id in its reason ("(till_sale +// )"), so the sale's funded gift card is clawed back and the sale marked +// failed, exactly like clawbackFailedTillSales. +// +// Idempotent: the parent UPDATE is status='pending'-guarded and the clawback is +// claim-first (payments.RevertGiftCardFunding), so a concurrent resolution by +// the sweep's own re-poll is a no-op. A non-nil error means a DB failure left +// the parent unresolved — the caller rejects the webhook so Square retries. +func resolveSweepDupRefundParent(ctx context.Context, squareRefundID string) error { + var paymentID, reason string + err := db.Conn.QueryRow(ctx, ` + SELECT payment_id, reason FROM refunds WHERE square_refund_id = $1 + `, squareRefundID).Scan(&paymentID, &reason) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + // Refund row already gone (deleted) — nothing to resolve. + return nil + } + log.Printf("[SQUARE-WEBHOOK] Failed to read refund %s for parent resolution: %v", squareRefundID, err) + return err + } + if !strings.HasPrefix(reason, "duplicate charge — sweep replay") { + return nil + } + if reason == "duplicate charge — sweep replay" { + tag, err := db.Conn.Exec(ctx, ` + UPDATE payments SET status = 'failed', updated_at = NOW() + WHERE id = $1 AND status = 'pending' + `, paymentID) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to mark B1 parent payment %s failed: %v", paymentID, err) + return err + } + if tag.RowsAffected() > 0 { + log.Printf("[SQUARE-WEBHOOK] B1 sweep-dup refund %s COMPLETED — marked parent payment %s failed", squareRefundID, paymentID) + } + return nil + } + if idx := strings.Index(reason, "(till_sale "); idx >= 0 { + tillSaleID := strings.TrimSuffix(reason[idx+len("(till_sale "):], ")") + return clawbackOneTillSaleByID(ctx, tillSaleID) + } + log.Printf("[SQUARE-WEBHOOK] B1 sweep-dup refund %s COMPLETED but parent could not be identified from reason %q — MANUAL RECONCILIATION REQUIRED", squareRefundID, reason) + return nil +} + +// clawbackOneTillSaleByID loads a pending till sale by id and resolves it like +// a definitively failed charge: a gift-card sale has its funding reverted +// atomically with the failed mark; a sale with no gift card is only marked +// failed. Shares clawbackOneTillSale with clawbackFailedTillSales so the B1 +// parent resolution and the failed-payment clawback can never drift. +func clawbackOneTillSaleByID(ctx context.Context, saleID string) error { + var ( + itemType string + itemID sql.NullString + totalAmount float64 + redeemedBy sql.NullString + isCreate *bool + ) + err := db.Conn.QueryRow(ctx, ` + SELECT ts.item_type, ts.item_id, ts.total_amount, gc.redeemed_by, + (ts.created_at = gc.created_at) AS is_create + FROM till_sales ts + LEFT JOIN gift_cards gc ON gc.id = ts.item_id + WHERE ts.id = $1 + `, saleID).Scan(&itemType, &itemID, &totalAmount, &redeemedBy, &isCreate) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to load till sale %s for B1 parent clawback: %v", saleID, err) + return err + } + return clawbackOneTillSale(ctx, saleID, itemType, itemID, totalAmount, redeemedBy, isCreate) +} + // truncateDisputeReason caps a Square dispute reason at the disputes.reason // VARCHAR(192) column width. An over-long reason would fail the disputes // INSERT; the handler treats that as a dispatch error (no dedup row, 5xx), so diff --git a/backend/handlers/webhooks/webhooks_state_test.go b/backend/handlers/webhooks/webhooks_state_test.go index df122bf..5fe90a6 100644 --- a/backend/handlers/webhooks/webhooks_state_test.go +++ b/backend/handlers/webhooks/webhooks_state_test.go @@ -38,13 +38,21 @@ func createWebhookTestPayment(t *testing.T, squarePaymentID, status string) stri } func createWebhookTestRefund(t *testing.T, paymentID, squareRefundID, status string) string { + t.Helper() + return createWebhookTestRefundWithReason(t, paymentID, squareRefundID, status, "webhook test refund") +} + +// createWebhookTestRefundWithReason is createWebhookTestRefund with an explicit +// reason — the B1 sweep-dup parent-resolution tests need the +// "duplicate charge — sweep replay" reason to exercise finding 1. +func createWebhookTestRefundWithReason(t *testing.T, paymentID, squareRefundID, status, reason string) string { t.Helper() var id string err := db.Conn.QueryRow(context.Background(), ` INSERT INTO refunds (payment_id, amount, reason, status, square_refund_id, created_at) - VALUES ($1, 5.00, 'webhook test refund', $3, $2, NOW()) + VALUES ($1, 5.00, $4, $3, $2, NOW()) RETURNING id - `, paymentID, squareRefundID, status).Scan(&id) + `, paymentID, squareRefundID, status, reason).Scan(&id) if err != nil { t.Fatalf("failed to create webhook test refund: %v", err) } @@ -1289,6 +1297,176 @@ func TestWebhook_RefundUpdated_NonTerminal_LeavesPending(t *testing.T) { } } +// TestWebhook_RefundUpdated_Approved_IsNonTerminal locks the webhook-only +// APPROVED override (finding 2): an APPROVED Square refund is still in flight +// and may settle COMPLETED or FAILED afterwards, so the webhook must leave the +// local row 'pending' — NOT promote it to 'completed' the way the synchronous +// refund handlers resolve a blocking APPROVED result. A later FAILED event +// must then be able to demote it; promoting on APPROVED would make that +// demotion a no-op (the FAILED path only demotes 'pending' rows) and the +// over-refund guard would count money that never actually moved. +func TestWebhook_RefundUpdated_Approved_IsNonTerminal(t *testing.T) { + const ( + squarePaymentID = "sqp_refund_pay_approved" + squareRefundID = "sqr_updated_approved" + ) + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + refundID := createWebhookTestRefund(t, payID, squareRefundID, "pending") + + approved := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_updated_approved_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "APPROVED", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w := deliverWebhook(t, approved) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 on APPROVED, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "pending" { + t.Fatalf("expected APPROVED to leave the refund 'pending', got %q", got) + } + + // A later FAILED event (Square declined the refund) must demote the still- + // pending row — the exact transition promoting on APPROVED would have broken. + failed := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_updated_approved_fail_1", + CreatedAt: "2025-01-01T00:00:01Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "FAILED", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w2 := deliverWebhook(t, failed) + if w2.Code != http.StatusOK { + t.Fatalf("expected 200 on FAILED after APPROVED, got %d: %s", w2.Code, w2.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "failed" { + t.Errorf("expected the APPROVED-then-FAILED refund to be demoted to 'failed', got %q", got) + } +} + +// TestWebhook_RefundUpdated_Completed_SweepDupResolvesParent locks finding 1: +// a COMPLETED webhook for a B1 sweep auto-refund of a replay-induced duplicate +// charge (reason "duplicate charge — sweep replay") must ALSO resolve the +// parent payment row — the B1 re-poll pass (sweepPendingB1Refunds, refunds.go) +// only queries refunds rows still 'pending', so once the webhook promotes this +// row to 'completed' the re-poll can never resolve the parent again. A +// still-pending parent would let the stale-pending sweep re-replay the expired +// idempotency key each run, minting a NEW charge (HIGH). The parent payment +// must be marked failed by the same webhook that promoted the refund. +func TestWebhook_RefundUpdated_Completed_SweepDupResolvesParent(t *testing.T) { + const ( + squarePaymentID = "sqp_refund_sweepdup_parent" + squareRefundID = "sqr_sweepdup_parent" + ) + // The parent is a still-PENDING payment row (the sweep-failed row the + // duplicate was minted for). The refund is attached to it with the B1 + // sweep-dup reason. + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + refundID := createWebhookTestRefundWithReason(t, payID, squareRefundID, "pending", "duplicate charge — sweep replay") + + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_sweepdup_parent_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "COMPLETED", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "completed" { + t.Errorf("expected sweep-dup refund 'completed', got %q", got) + } + if got := getPaymentStatus(t, payID); got != "failed" { + t.Errorf("expected the sweep-dup refund's parent payment to be resolved to 'failed', got %q", got) + } +} + +// TestWebhook_RefundUpdated_Completed_SweepDupResolvesTillSale locks the +// till_sale branch of finding 1: a COMPLETED webhook for a B1 sweep-dup refund +// whose reason carries "(till_sale )" must claw back the till sale's funded +// gift card and mark the sale failed — mirroring the sweep's re-poll resolution. +func TestWebhook_RefundUpdated_Completed_SweepDupResolvesTillSale(t *testing.T) { + const squareRefundID = "sqr_sweepdup_tillsale" + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, "sqp_sweepdup_tillsale_pay", "2025-01-01T00:00:00Z", "2025-01-01T00:00:00Z", 40.00) + + // A synthetic completed payments row anchors the refund (mirrors + // recordSweepDuplicateRefundRow); the till_sale id lives in the reason. + adminID, err := fixtures.CreateTestAdminUser(db.Conn) + if err != nil { + t.Fatalf("failed to create admin for sweep-dup till sale: %v", err) + } + var payID string + if err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO payments (payment_type, payment_method, status, amount, created_by, created_at) + VALUES ('full', 'in_person_card', 'completed', 40.00, $1, NOW()) + RETURNING id + `, adminID).Scan(&payID); err != nil { + t.Fatalf("failed to create synthetic payment for sweep-dup till sale: %v", err) + } + refundID := createWebhookTestRefundWithReason(t, payID, squareRefundID, "pending", "duplicate charge — sweep replay (till_sale "+saleID+")") + + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_sweepdup_tillsale_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "COMPLETED", + "payment_id": "sqp_sweepdup_tillsale_pay" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "completed" { + t.Errorf("expected sweep-dup refund 'completed', got %q", got) + } + if got := getTillSaleStatus(t, saleID); got != "failed" { + t.Errorf("expected the sweep-dup refund's till sale to be marked 'failed', got %q", got) + } + if exists := giftCardExists(t, giftCardID); exists { + t.Error("expected the created gift card to be deleted by the till-sale clawback") + } +} + // TestWebhook_RefundUpdated_DoesNotDemoteCompleted guards the FAILED // transition: a completed refund must never be demoted to 'failed' by a late // webhook, since the over-refund guard counts 'completed' refunds — demoting diff --git a/backend/internal/adminnotify/adminnotify.go b/backend/internal/adminnotify/adminnotify.go new file mode 100644 index 0000000..17edb90 --- /dev/null +++ b/backend/internal/adminnotify/adminnotify.go @@ -0,0 +1,77 @@ +// Package adminnotify owns the shared flood cap for the DB-backed admin +// notification centre (admin_notifications). That table is the single +// operator's ONLY pager for money events, so every site that inserts an +// operator-facing alert must bound its unacknowledged queue — a hostile flood +// (attacker-registered accounts triggering refresh_token_reuse / reissue-fail / +// webhook alerts) must not be able to bury the notification centre under rows +// the operator can never work through. +// +// Coordination contract (Round 2 Loop B finding 1): the cap is applied +// atomically at every insert site in this codebase: +// +// - handlers/payments/twofa.go — the reissue-fail alert (per-issue capped, +// see finding 2; NOT globally capped). +// - auth/jwt.go VerifyRefreshToken — the 'refresh_token_reuse' alert. +// - handlers/webhooks/square.go — the three critical_payment_log inserts +// (dispute, booking, unknown-event, orphan-replay). +// - handlers/user/account.go InsertSquareErasureCriticalNotification. +// +// Sites owned by OTHER agents that still need the fold (coordination notes): +// +// - handlers/payments/sweep.go:1556 insertCriticalPaymentNotification (money +// agent) — its INSERT ... SELECT ... WHERE NOT EXISTS is the same +// unbounded-across-accounts shape; fold `AND (SELECT COUNT(*) FROM +// admin_notifications _an WHERE _an.reason = 'critical_payment_log' AND +// _an.acknowledged_at IS NULL) < $N` (N = MaxUnacknowledgedCriticalLogs) +// into its WHERE clause. +// - handlers/scheduling/time-blockers.go:511 insertSquareCleanupCriticalNotification +// and internal/jobs/cleanup.go:333 ScanCriticalPaymentLogs — the same +// 'critical_payment_log' insert shape on the GDPR-cleanup and log-scan +// paths (owner: the jobs/scheduling agent). +// - main.go has NO admin_notifications insert sites (it only mounts the +// notification read/ack routes), so nothing to cap there. +// +// Every site folds the cap INTO its INSERT (a conditional +// `INSERT ... SELECT ... WHERE (SELECT COUNT(*) ...) < $cap`) so the +// count-then-insert is ATOMIC — closing the TOCTOU where two concurrent +// inserts both read a below-cap count and overshoot together (finding 2). +package adminnotify + +import ( + "context" + "log" + + "crussell/db" +) + +// MaxUnacknowledgedCriticalLogs is the GLOBAL cap on unacknowledged +// admin_notifications rows for one reason (named after the money-critical +// reason 'critical_payment_log' that the cap exists to protect). Once the +// unacknowledged queue for a reason reaches the cap, further inserts for that +// reason are dropped (with a suppression log for operator visibility) until +// the operator acknowledges outstanding rows. 100 is far beyond anything a +// single salon produces legitimately, so it only ever suppresses an abnormal +// flood. +const MaxUnacknowledgedCriticalLogs = 100 + +// CriticalLogsCapExceeded reports whether the unacknowledged admin-notification +// queue for reason has reached MaxUnacknowledgedCriticalLogs. q routes through +// the caller's transaction when one is active (the ctx-routed db.Conn proxy, or +// an explicit pgx.Tx). +// +// Best-effort and fail-OPEN: a count error is logged and false is returned, so +// a money alert is never dropped because the count query failed (the insert's +// own atomic cap condition below still guards the row in that case — the +// pre-check only decides whether to log a suppression). +func CriticalLogsCapExceeded(ctx context.Context, q db.Querier, reason string) bool { + var n int + err := q.QueryRow(ctx, ` + SELECT COUNT(*) FROM admin_notifications + WHERE reason = $1::admin_notification_reason AND acknowledged_at IS NULL + `, reason).Scan(&n) + if err != nil { + log.Printf("adminnotify: failed to count unacknowledged %s admin notifications: %v", reason, err) + return false + } + return n >= MaxUnacknowledgedCriticalLogs +} diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index 112ce7f..241b897 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -143,6 +143,27 @@ func (st *AttemptState) SetLastActive(t time.Time) { st.LastAt.Store(t.UnixNano()) } +// SetLastMintAtLocked records the per-user mint-cooldown stamp. The caller +// MUST already hold st.Mu (matching how the stamp is read by +// twoFAMintThrottled in handlers/user and the payments re-issue cooldown). +// +// Round 2 Loop B finding 3a: writing to the SHARED saturated state is a +// NO-OP. saturatedLockedState is returned by StateFor for EVERY untracked user +// once the map is at capacity, so a mint stamped on it would throttle every +// untracked user for the whole cooldown (one user's mint blocks everyone for +// 60s) and ClearMintCooldownForUser would clear it for all of them. The +// singleton's stamp therefore stays permanently zeroed: under saturation, +// per-user mints are unthrottled, which is SAFE because a mint never grants a +// fresh guessing budget (B11b) and the saturated state is already permanently +// locked out for verification. The stamp must also never be written by the +// reissue/mint paths when st IS the singleton — the no-op below guarantees it. +func (st *AttemptState) SetLastMintAtLocked(t time.Time) { + if st == saturatedLockedState { + return + } + st.LastMintAt = t +} + // LockedOut reports whether the state is inside its lockout window: the attempt // counter has reached the cap and the window has not yet elapsed. Such a record // is the rate limit's source of truth for its user and must never be evicted @@ -199,9 +220,14 @@ func StateFor(userID string) *AttemptState { delete(Map, id) continue } - if st.LockedOut(now) { - // Inside its lockout window — the rate limit's source of truth - // for this user. Never evict (finding-e fix). + if st.LockedOut(now) || st.Count.Load() > 0 { + // Round 2 Loop B finding 3b: an in-window record with a + // NON-ZERO attempt counter is the rate limit's IN-PROGRESS + // state for a genuine user (a locked-out record, or a user + // mid-window with failed attempts banked). Evicting it would + // silently reset the counter and grant a fresh guessing + // budget. Only count==0 in-window records (fresh lookups / + // idle mint-cooldown stamps) are evictable. continue } if at := st.LastActive(); oldestID == "" || at.Before(oldestAt) { @@ -212,18 +238,30 @@ func StateFor(userID string) *AttemptState { delete(Map, oldestID) } if len(Map) >= MaxTrackedAttempts { - // Every entry is a locked-out in-window record. Do not evict one - // (that would reset its rate limit) and do not grow past the cap: - // return the SHARED permanently-locked state (finding 3, Round 2 - // Loop A). Previously a fresh transient state was returned per - // call, so every untracked user received a fresh 5-guess budget - // per request — silently disabling the brute-force lockout exactly + // Every entry is a protected in-window record (locked out, or + // carrying an in-progress counter). Do not evict one (that would + // reset its rate limit) and do not grow past the cap: return the + // SHARED permanently-locked state (finding 3, Round 2 Loop A). + // Previously a fresh transient state was returned per call, so + // every untracked user received a fresh 5-guess budget per + // request — silently disabling the brute-force lockout exactly // under the hostile flood that saturated the map. The shared state // treats every untracked user as locked out instead. It is never // stored in Map (so the eviction scan / ResetAttempts / // DeleteAttempts never touch it) and self-heals: as soon as one of // the real locked-out records lapses out of its window, StateFor // evicts it and normal per-user tracking resumes. + // + // Round 2 Loop B finding 3c — ACCEPTED RESIDUAL: an attacker can + // still fill the map with up to MaxTrackedAttempts (~10k) + // in-window locked-out records, forcing every other user into the + // shared saturated state. That is a bounded, FAIL-CLOSED outcome: + // the fallback is a locked-out-everyone state (brute force + // impossible, availability reduced), never a locked-out-nobody one. + // Mint cooldowns are unthrottled under saturation (the shared + // stamp is zeroed — see SetLastMintAtLocked), which is safe + // because a mint grants no guessing budget (B11b). The map drains + // as locked-out windows lapse. return saturatedLockedState } } @@ -492,13 +530,26 @@ func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error } // ClearMintCooldownForUser zeroes the user's mint-cooldown stamp (LastMintAt) -// under the per-user mutex — LastMintAt is only ever touched under Mu. Exported -// so the payments re-issue path's coordination contract is actionable (Round 2 -// Loop A finding 2): a FRESH-charge terminal-success path that consumed the -// code at the gate (consume=true, so ConsumePendingCode is not called) can call -// this to re-arm immediate re-minting after a completed charge. +// under the per-user mutex — LastMintAt is only ever touched under Mu. A no-op +// when the user's state IS the shared saturated singleton (Round 2 Loop B +// finding 3a: the shared stamp must not be cleared for every untracked user by +// one user's terminal success). +// +// Round 2 Loop B finding 6a — COORDINATION CONTRACT (money agent): the FRESH +// saved-card charge path consumes the customer's 2FA code AT THE GATE +// (consume=true), so twofa.ConsumePendingCode is NOT called on its terminal +// success and the mint-cooldown stamp survives — a customer who completes a +// fresh charge within 60s of their last code mint and immediately requests a +// new code gets 429 for the rest of the window. The money agent's fresh-charge +// terminal-success path in handlers/payments/handlers.go should call +// ClearMintCooldownForUser(userID) at the same point a completed charge is +// recorded, so a just-completed charge re-arms immediate re-minting. This +// function is the exported, coordination-actionable entry point for that call. func ClearMintCooldownForUser(userID string) { st := StateFor(userID) + if st == saturatedLockedState { + return + } st.Mu.Lock() st.LastMintAt = time.Time{} st.Mu.Unlock() diff --git a/backend/internal/twofa/twofa_test.go b/backend/internal/twofa/twofa_test.go index a1e56dc..a6cdac8 100644 --- a/backend/internal/twofa/twofa_test.go +++ b/backend/internal/twofa/twofa_test.go @@ -223,3 +223,83 @@ func TestConsumePendingCode_ClearsMintCooldownStamp(t *testing.T) { defer st.Mu.Unlock() require.True(t, st.LastMintAt.IsZero(), "terminal-success consumption must clear the mint-cooldown stamp (finding 2)") } + +// TestStateFor_SaturatedMintStampIsNoOp pins Round 2 Loop B finding 3a: the +// SHARED saturated state must never carry a per-user mint-cooldown stamp. +// StateFor returns the package singleton to every untracked user once the map +// is at capacity, so a mint stamped on it would throttle all of them for the +// whole cooldown (one user's mint blocks everyone for 60s) and +// ClearMintCooldownForUser would clear it for everyone. The no-op keeps the +// shared stamp permanently zeroed. +func TestStateFor_SaturatedMintStampIsNoOp(t *testing.T) { + t.Cleanup(func() { + MapMu.Lock() + Map = make(map[string]*AttemptState) + MaxTrackedAttempts = 10_000 + MapMu.Unlock() + }) + + MapMu.Lock() + Map = make(map[string]*AttemptState) + MaxTrackedAttempts = 1 + MapMu.Unlock() + + now := clock.Now() + MapMu.Lock() + victim := &AttemptState{} + victim.SetLastActive(now) + victim.Count.Store(MaxAttempts) // in-window locked-out — protected from eviction + Map["victim"] = victim + MapMu.Unlock() + + st := StateFor("untracked") // saturated — shared permanently-locked state + require.Same(t, st, saturatedLockedState, "a saturated map must return the shared permanently-locked state") + + st.SetLastMintAtLocked(clock.Now()) + require.True(t, st.LastMintAt.IsZero(), "a mint stamp written to the saturated state must be a no-op (cross-user throttle)") + + ClearMintCooldownForUser("untracked") + require.True(t, st.LastMintAt.IsZero(), "clearing the cooldown for one saturated user must not touch the shared stamp") +} + +// TestStateFor_NeverEvictsInWindowCounter pins Round 2 Loop B finding 3b: the +// cap-driven eviction must never drop an in-window record carrying a NON-ZERO +// attempt counter — a genuine user mid-window with failed attempts banked. +// Evicting it would silently reset the counter and grant a fresh guessing +// budget, so only count==0 in-window records (idle mint-cooldown stamps / fresh +// lookups) are evictable. When every in-window record is protected, a new key +// falls back to the shared saturated state instead. +func TestStateFor_NeverEvictsInWindowCounter(t *testing.T) { + t.Cleanup(func() { + MapMu.Lock() + Map = make(map[string]*AttemptState) + MaxTrackedAttempts = 10_000 + MapMu.Unlock() + }) + + MapMu.Lock() + Map = make(map[string]*AttemptState) + MaxTrackedAttempts = 2 + MapMu.Unlock() + + now := clock.Now() + for _, id := range []string{"genuine_a", "genuine_b"} { + st := &AttemptState{} + st.SetLastActive(now) + st.Count.Store(2) // in-progress counter, NOT locked out + Map[id] = st + } + MapMu.Lock() + require.Len(t, Map, 2) + MapMu.Unlock() + + st := StateFor("new_user") + require.True(t, st.LockedOut(clock.Now()), "with every in-window record protected, a new key must fall back to the shared locked state") + + MapMu.Lock() + defer MapMu.Unlock() + require.Len(t, Map, 2, "in-window records with count>0 must never be evicted (finding 3b)") + for _, id := range []string{"genuine_a", "genuine_b"} { + require.NotNil(t, Map[id], "%s must survive cap pressure", id) + } +} diff --git a/backend/mw/ratelimit.go b/backend/mw/ratelimit.go index 0228ef6..e63a1b0 100644 --- a/backend/mw/ratelimit.go +++ b/backend/mw/ratelimit.go @@ -102,14 +102,16 @@ func (prl *ProgressiveRateLimiter) Check(ip string) (delayMs int) { } } -// maxProgressiveSleepDelayMs is the largest delay the progressive per-IP -// limiter still absorbs by sleeping. Beyond it the request is rejected 429 -// immediately instead (Round 2 Loop A finding 4a): sleeping 5-10s ties up a -// goroutine per throttled request while the client keeps hammering, so one -// client can stack many sleeping goroutines in front of the bcrypt wall on -// /login and /register. The small progressive tiers (500ms / 2s) are -// unchanged. -const maxProgressiveSleepDelayMs = 2000 +// progressiveRejectDelayMs is the delay at which the progressive per-IP limiter +// stops sleeping and rejects the request 429 immediately (Round 2 Loop B +// finding 5). ONLY the TOP abuse tier (10s) rejects: the 500ms / 2s / 5s tiers +// keep sleeping (backoff). Previously every tier past 2s hard-rejected, which +// under a shared NAT — or any TRUST_PROXY_HEADERS=false deployment where every +// client collapses onto the proxy's IP — locked out the whole surface behind +// one abusive client. Now a moderate sustained rate still sleeps (throttling +// the offender with backoff) instead of hard-rejecting everyone, while the 10s +// abuse tier keeps the goroutine-parking amplifier bound. +const progressiveRejectDelayMs = 10000 func ProgressiveRateLimit(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -117,10 +119,10 @@ func ProgressiveRateLimit(next http.Handler) http.Handler { delay := globalProgressiveLimiter.Check(ip) switch { - case delay > maxProgressiveSleepDelayMs: - // Far past the sustained budget — reject now instead of parking a - // goroutine for 5-10s. The rejection is a clean 429; the client - // retries after the burst window lapses. + case delay >= progressiveRejectDelayMs: + // Top abuse tier only — far past the sustained budget, reject now + // instead of parking a goroutine for 10s. The rejection is a clean + // 429; the client retries after the burst window lapses. RespondJSON(w, http.StatusTooManyRequests, map[string]string{"error": "Rate limit exceeded"}) return case delay > 0: diff --git a/backend/mw/ratelimit_test.go b/backend/mw/ratelimit_test.go index 0c14162..cccd592 100644 --- a/backend/mw/ratelimit_test.go +++ b/backend/mw/ratelimit_test.go @@ -652,12 +652,14 @@ func TestProgressiveRateLimiter_DelayEscalatesWithSustainedRate(t *testing.T) { } } -// TestProgressiveRateLimit_RejectsBeyondSleepCap pins finding 4a: once the -// computed delay exceeds maxProgressiveSleepDelayMs (2s), the middleware -// rejects the request 429 immediately instead of sleeping a goroutine for -// 5-10s (a per-client goroutine-parking amplifier in front of bcrypt). The -// small progressive tiers (500ms / 2s) still sleep. -func TestProgressiveRateLimit_RejectsBeyondSleepCap(t *testing.T) { +// TestProgressiveRateLimit_RejectsOnlyTopTier pins finding 4a + Round 2 Loop B +// finding 5: ONLY the top 10s abuse tier rejects the request 429 immediately +// instead of sleeping a goroutine (a per-client goroutine-parking amplifier in +// front of bcrypt); the 500ms / 2s / 5s tiers keep sleeping (backoff). Before +// finding 5 every tier past 2s hard-rejected, which under a shared NAT / +// TRUST_PROXY_HEADERS=false deployment locked out the whole surface behind one +// abusive client. +func TestProgressiveRateLimit_RejectsOnlyTopTier(t *testing.T) { globalProgressiveLimiter.mu.Lock() saved := globalProgressiveLimiter.requests globalProgressiveLimiter.requests = make(map[string]*ipProgressiveState) @@ -668,28 +670,48 @@ func TestProgressiveRateLimit_RejectsBeyondSleepCap(t *testing.T) { globalProgressiveLimiter.mu.Unlock() }) - // Seed the 10s abuse tier (sustained > 300) for this IP. - seedProgressiveTimestamps(t, globalProgressiveLimiter, "198.51.100.99", 31, 320) - + // 5s tier (sustained 201-300): sleeps (backoff), never rejects — the NAT / + // proxy-collapse case where a moderate sustained rate must not hard-lock + // the whole shared surface. + seedProgressiveTimestamps(t, globalProgressiveLimiter, "198.51.100.98", 31, 250) nextCalled := false handler := ProgressiveRateLimit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { nextCalled = true w.WriteHeader(http.StatusOK) })) - req := httptest.NewRequest(http.MethodPost, "/login", nil) - req.RemoteAddr = "198.51.100.99:1234" + req.RemoteAddr = "198.51.100.98:1234" w := httptest.NewRecorder() start := time.Now() handler.ServeHTTP(w, req) + if !nextCalled { + t.Error("the 5s tier must sleep (backoff), not reject — next handler must be called") + } + if w.Code != http.StatusOK { + t.Errorf("expected the 5s tier to sleep and pass through, got %d", w.Code) + } + if got := w.Header().Get("X-RateLimit-Delay"); got != "5000" { + t.Errorf("expected X-RateLimit-Delay=5000 for the 5s tier, got %q", got) + } + if elapsed := time.Since(start); elapsed < 4*time.Second { + t.Errorf("the 5s tier must actually sleep (took %s)", elapsed) + } + // 10s abuse tier (sustained > 300): rejects 429 immediately. + seedProgressiveTimestamps(t, globalProgressiveLimiter, "198.51.100.99", 31, 320) + nextCalled = false + req = httptest.NewRequest(http.MethodPost, "/login", nil) + req.RemoteAddr = "198.51.100.99:1234" + w = httptest.NewRecorder() + start = time.Now() + handler.ServeHTTP(w, req) if nextCalled { - t.Error("next handler must NOT be called when the delay exceeds the sleep cap") + t.Error("next handler must NOT be called at the 10s abuse tier") } if w.Code != http.StatusTooManyRequests { - t.Errorf("expected 429, got %d", w.Code) + t.Errorf("expected 429 at the 10s abuse tier, got %d", w.Code) } if elapsed := time.Since(start); elapsed >= 5*time.Second { - t.Errorf("the 5-10s tiers must reject immediately, not sleep (took %s)", elapsed) + t.Errorf("the 10s tier must reject immediately, not sleep (took %s)", elapsed) } } diff --git a/frontend/src/lib/components/admin/BookingModal.svelte b/frontend/src/lib/components/admin/BookingModal.svelte index 55747e4..44f3fd5 100644 --- a/frontend/src/lib/components/admin/BookingModal.svelte +++ b/frontend/src/lib/components/admin/BookingModal.svelte @@ -856,9 +856,13 @@ {apptHours > 72 ? "Over 72 hours' notice means no deposit protection applies — a full refund is given regardless." : apptHours >= 24 - ? "Between 24–72 hours' notice, up to 50% of the total (" + + ? "Between 24–72 hours' notice, up to " + + POLICY.PROTECTED_DEPOSIT_MAX_PCT * 100 + + '% of the total (' + '£' + - (selectedBooking.total_amount * 0.5).toFixed(2) + + (selectedBooking.total_amount * POLICY.PROTECTED_DEPOSIT_MAX_PCT).toFixed( + 2 + ) + ') is treated as a protected deposit to cover the lost slot. The remaining balance above that is refunded.' : "Under 24 hours' notice, the full amount paid (" + '£' + diff --git a/frontend/src/lib/components/admin/EditBookingModal.svelte b/frontend/src/lib/components/admin/EditBookingModal.svelte index d8b72b3..df97eea 100644 --- a/frontend/src/lib/components/admin/EditBookingModal.svelte +++ b/frontend/src/lib/components/admin/EditBookingModal.svelte @@ -5,6 +5,7 @@ import { formatDuration } from '$lib/utils/format'; import { formatUserName } from '$lib/utils/nameDisplay'; import { parseWallClockDate } from '$lib/utils/timeSlots'; + import { generateUUID } from '$lib/utils/uuid'; import * as Modal from '$lib/components/ui/dialog'; import * as AlertDialog from '$lib/components/ui/alert-dialog'; import { Button } from '$lib/components/ui/button'; @@ -353,7 +354,7 @@ // Unique per refund attempt so two equal partial refunds of the same // payment don't collide on the backend's amount-derived key; reused on // retry (the backend dedups on it) so a timeout can't double-refund. - refundIdempotencyKey = crypto.randomUUID(); + refundIdempotencyKey = generateUUID(); showRefundModal = true; // Pre-fill the refund with the RESIDUAL (payment.amount − already diff --git a/frontend/src/lib/components/admin/RescheduleModal.svelte b/frontend/src/lib/components/admin/RescheduleModal.svelte index 1d1f86c..f17bc7d 100644 --- a/frontend/src/lib/components/admin/RescheduleModal.svelte +++ b/frontend/src/lib/components/admin/RescheduleModal.svelte @@ -7,6 +7,7 @@ import { Checkbox } from '$lib/components/ui/checkbox'; import PolicyPopover from '$lib/components/ui/policyPopover.svelte'; import { formatUserName } from '$lib/utils/nameDisplay'; + import { POLICY } from '$lib/constants/policy'; import * as Modal from '$lib/components/ui/dialog'; import { Button } from '$lib/components/ui/button'; @@ -446,8 +447,9 @@ What this means

"Rescheduling within {Math.round(hoursUntilAppointment)}h of the original time with - payments present means deposit protection applies — up to 50% of the total (up to £{( - booking.total_amount * 0.5 + payments present means deposit protection applies — up to {POLICY.PROTECTED_DEPOSIT_MAX_PCT * + 100}% of the total (up to £{( + booking.total_amount * POLICY.PROTECTED_DEPOSIT_MAX_PCT ).toFixed(2)}) could be retained depending on notice period."

diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index c348092..3728877 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -327,7 +327,7 @@ } function calculateDepositAmount(): number { - return Math.round(getTotalPrice() * 0.2 * 100) / 100; + return Math.round(getTotalPrice() * POLICY.REQUIRED_DEPOSIT_PCT * 100) / 100; } async function fetchDiscountPreview() { @@ -2607,8 +2607,8 @@

Deposit Paid

- Your deposit of {formatCurrency(calculateDepositAmount())} has been - paid successfully. See you at your appointment! + Your deposit of {formatCurrency(calculateDepositAmount())} has + been paid successfully. See you at your appointment!

{:else if depositRequired} diff --git a/frontend/src/lib/components/payments/PaymentModal.svelte b/frontend/src/lib/components/payments/PaymentModal.svelte index e75f5d9..66c4a7b 100644 --- a/frontend/src/lib/components/payments/PaymentModal.svelte +++ b/frontend/src/lib/components/payments/PaymentModal.svelte @@ -12,6 +12,7 @@ import { CARD_VERIFICATION_RETRY_MESSAGE, campaignDiscountPence, + isOverflowTipConfirmationRequired, isTwoFactorVerificationGateFailure, isVerificationRequiredSignal, PAYMENT_METHOD_SAVED_CARD, @@ -26,10 +27,10 @@ } from '$lib/square/square'; import { authStore } from '$lib/stores/auth.svelte'; import TwoFactorCodeInput from '$lib/components/payments/TwoFactorCodeInput.svelte'; + import OverflowTipConfirm from '$lib/components/payments/OverflowTipConfirm.svelte'; import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; import { generateUUID } from '$lib/utils/uuid'; - - const LOYALTY_DISCOUNT_RATE = 0.1; + import { POLICY } from '$lib/constants/policy'; interface Props { booking: Booking; @@ -62,7 +63,7 @@ amount: number; }; - type PaymentMethod = 'card' | 'cash' | 'giftcard' | (typeof PAYMENT_METHOD_SAVED_CARD) | null; + type PaymentMethod = 'card' | 'cash' | 'giftcard' | typeof PAYMENT_METHOD_SAVED_CARD | null; let status = $state('idle'); let selectedMethod = $state(null); @@ -75,6 +76,65 @@ // reactive flag is checked synchronously at the start of every handler. let isProcessingPaymentSync = false; + // Overpayment confirmation (mirrors UserPaymentModal/BookingFlow). The + // backend rejects a payment that exceeds the booking's remaining balance + // unless the request carries `confirm_overflow_tip: true` — a tip is + // gratuity for service already rendered. The guard fires on STALE booking + // data (multi-tab, admin-changed totals) where the operator would otherwise + // be stuck with an unresolvable 400; the rejected request body is parked + // here and a Confirm/Cancel prompt is shown, with Confirm resending the SAME + // body plus the flag. + let overflowConfirm = $state<{ + amountPence: number; + overflowPence: number; + body: Record; + } | null>(null); + + function confirmOverflowPayment() { + const pending = overflowConfirm; + if (!pending || status === 'saved-card-processing') return; + status = 'saved-card-processing'; + error = null; + isProcessingPaymentSync = true; + apiFetch(`/api/admin/bookings/${booking.id}/payment`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ ...pending.body, confirm_overflow_tip: true }) + }) + .then(async (response) => { + if (!response.ok) { + const errData = await response.text(); + throw new Error(extractErrorMessage(errData) || 'Failed to process payment'); + } + const data = await response.json(); + status = 'success'; + paymentResult = { + checkout_id: data.payment_id || data.checkout_id || data.id || '', + status: 'COMPLETED', + card_brand: data.card_brand, + last4: data.card_last4, + amount: data.amount + }; + overflowConfirm = null; + toast.success('Payment successful'); + onComplete(paymentResult); + }) + .catch((_err) => { + status = 'error'; + error = _err instanceof Error ? _err.message : 'Failed to process payment'; + toast.error(error ?? 'Unknown error'); + }) + .finally(() => { + isProcessingPaymentSync = false; + }); + } + + function cancelOverflowConfirmation() { + overflowConfirm = null; + status = 'idle'; + selectedMethod = null; + } + // B6/B10: charging a customer's saved card requires the customer's current // 2FA verification code when the backend enforces the gate. The backend keys // on the CARD OWNER (the booking's user), so the input is surfaced whenever @@ -168,7 +228,7 @@ ); const loyaltyDiscount = $derived( - useLoyalty ? Math.round(booking.total_amount * 100 * LOYALTY_DISCOUNT_RATE) : 0 + useLoyalty ? Math.round(booking.total_amount * 100 * POLICY.LOYALTY_DISCOUNT_RATE) : 0 ); // Campaign discount preview — fetched on mount, mirroring the customer flow @@ -324,11 +384,10 @@ const tipPercentages = $derived.by(() => { if (netTotal <= 0) return []; - return [ - { pct: 10, amount: Math.round(netTotal * 0.1 * 100) / 100 }, - { pct: 15, amount: Math.round(netTotal * 0.15 * 100) / 100 }, - { pct: 20, amount: Math.round(netTotal * 0.2 * 100) / 100 } - ]; + return POLICY.TIP_PRESET_PCTS.map((pct) => ({ + pct, + amount: Math.round(netTotal * (pct / 100) * 100) / 100 + })); }); const tipMultiplier = $derived( @@ -342,7 +401,7 @@ const totalWithTip = $derived(tipEnabled ? netTotal * tipMultiplier : netTotal); const tipDisplay = $derived( selectedTipPercent !== null - ? `${selectedTipPercent}%` + ? `${selectedTipPercent}%` : customTipAmount && parseFloat(customTipAmount) > 0 ? `${formatCurrency(parseFloat(customTipAmount))}` : '' @@ -404,6 +463,24 @@ if (!response.ok) { const errData = await response.text(); + // Overflow guard (defensive — the admin terminal path currently + // clamps instead, but a 400 carrying the code must surface the + // Confirm/Cancel prompt like the customer modal, not a dead-end). + if (isOverflowTipConfirmationRequired(errData)) { + overflowConfirm = { + amountPence: Math.round(finalAmount * 100) - loyaltyDiscount, + overflowPence: Math.max( + 0, + Math.round(finalAmount * 100) - loyaltyDiscount - Math.round(netTotal * 100) + ), + body: { + amount: Math.round(finalAmount * 100) - loyaltyDiscount, + payment_type: 'full', + tip_enabled: tipEnabled + } + }; + return; + } throw new Error(extractErrorMessage(errData) || 'Failed to initiate payment'); } @@ -841,9 +918,7 @@ let verificationToken = ''; status = 'saved-card-waiting-sca'; try { - const squareCardId = savedCards.find( - (c) => c.id === selectedSavedCardId - )?.square_card_id; + const squareCardId = savedCards.find((c) => c.id === selectedSavedCardId)?.square_card_id; const proactive = await runSavedCardSCAProactively({ amountPence: chargeAmount, squareCardId: squareCardId ?? '', @@ -898,6 +973,27 @@ if (!response.ok) { responseStatus = response.status; const errData = await response.text(); + // Overflow guard: a 400 carrying the backend's + // `overflow_tip_confirmation_required` code means the charge + // exceeds the booking's remaining balance (stale data). Park the + // rejected request and surface the Confirm/Cancel prompt instead + // of a dead-end 400; Confirm resends the SAME body with the flag. + if (isOverflowTipConfirmationRequired(errData)) { + overflowConfirm = { + amountPence: chargeAmount, + overflowPence: Math.max(0, chargeAmount - Math.round(netTotal * 100)), + body: { + amount: chargeAmount, + payment_type: 'full', + payment_method: 'saved_card', + saved_card_id: selectedSavedCardId, + ...(verificationToken ? { verification_token: verificationToken } : {}), + ...(twoFactor.showInput ? { verification_code: twoFactor.code } : {}), + idempotency_key: savedCardIdempotencyKey + } + }; + return; + } // A 402 verification-required here means the fresh proactive // token was stale/expired at Square — the charge did NOT land. // Surface the SCA-first guidance; the operator taps Pay again and @@ -981,7 +1077,15 @@ open={true} onOpenChange={(open) => { if (open) return; + // ESC/overlay while a charge is in flight must not close the modal — the + // charge may still land. ESC while the overflow-confirm prompt is showing + // dismisses the prompt (back to the amount-editing form), mirroring the + // customer modal, instead of closing the whole flow. if (isChargeInFlight(status)) return; + if (overflowConfirm) { + cancelOverflowConfirmation(); + return; + } handleClose(); }} > @@ -990,7 +1094,23 @@ Take Payment - {#if status === 'idle'} + {#if overflowConfirm} +
+ + + +
+ {:else if status === 'idle'}
Services
@@ -1039,8 +1159,8 @@
Use Loyalty Stamp Card
{Math.floor(stamps / 10)} full card{Math.floor(stamps / 10) === 1 ? '' : 's'} available - · {Math.round(LOYALTY_DISCOUNT_RATE * 100)}% off ({formatCurrency( - Math.round(booking.total_amount * 100 * LOYALTY_DISCOUNT_RATE) / 100 + · {Math.round(POLICY.LOYALTY_DISCOUNT_RATE * 100)}% off ({formatCurrency( + Math.round(booking.total_amount * 100 * POLICY.LOYALTY_DISCOUNT_RATE) / 100 )})
@@ -1132,7 +1252,8 @@ class="flex items-center justify-between rounded-md border border-green-200 bg-green-50 p-3" > Already paid - {formatCurrency(amountPaidPence / 100)}{formatCurrency(amountPaidPence / 100)}
{/if} @@ -1224,14 +1345,14 @@
@@ -1009,7 +1013,8 @@
Total - {formatCurrency(Math.round(booking.total_amount * 100) / 100)}{formatCurrency(Math.round(booking.total_amount * 100) / 100)}
{#if discountPreview?.eligible} @@ -1029,7 +1034,9 @@ {#if useLoyalty && loyaltyDiscount > 0}
Loyalty Stamp Card (10% Off) - -{formatCurrency(loyaltyDiscount / 100)} + -{formatCurrency(loyaltyDiscount / 100)}
{/if}
@@ -1141,7 +1148,7 @@ depositChargePence( booking.deposit_amount ? Math.round(booking.deposit_amount * 100) - : Math.round(booking.total_amount * 0.2 * 100), + : Math.round(booking.total_amount * POLICY.REQUIRED_DEPOSIT_PCT * 100), campaignDiscountPence(discountPreview) ) / 100 )}) diff --git a/frontend/src/lib/constants/policy.ts b/frontend/src/lib/constants/policy.ts index fdfe6c4..4e8454c 100644 --- a/frontend/src/lib/constants/policy.ts +++ b/frontend/src/lib/constants/policy.ts @@ -7,5 +7,14 @@ export const POLICY = { RESCHEDULE_BLOCK_HOURS_WITH_PAYMENTS: 72, RESCHEDULE_BLOCK_HOURS_NO_PAYMENTS: 24, PROTECTED_DEPOSIT_MAX_PCT: 0.5, - REQUIRED_DEPOSIT_PCT: 0.2 + REQUIRED_DEPOSIT_PCT: 0.2, + // Loyalty redemption: 10 stamps → 10% off the booking total. Shared by the + // admin and customer payment modals so the discount rate can't drift. + LOYALTY_DISCOUNT_RATE: 0.1, + // Gratuity suggestion presets offered by the tip surfaces (admin payment + // modal + customer tip page). Deliberately SEPARATE from the deposit policy + // constants above — a tip suggestion is gratuity, not a deposit percentage, + // and coupling them would silently change the tip buttons if the deposit + // rate ever changed. + TIP_PRESET_PCTS: [10, 15, 20] } as const; diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index ee31224..b734a52 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -283,8 +283,13 @@ // value instead of a hardcoded copy and survives a page reload. The local // counter still only reflects confirmed purchases made in this session; any // rejection the counter can't foresee surfaces through the backend's error - // toast. - let dailyGiftCardBuyLimit = $state(500); + // toast. The fallback below exists ONLY for old servers that don't expose + // daily_buy_limit yet — it is always overwritten by the server value when + // the balance fetch succeeds (see fetchGiftCardBalance). + // Cross-reference: backend/handlers/payments/giftcard_limits.go + // maxUserGiftCardDailyPence = 500_00 pence (£500). + const DAILY_GIFT_CARD_BUY_LIMIT_FALLBACK_GBP = 500; + let dailyGiftCardBuyLimit = $state(DAILY_GIFT_CARD_BUY_LIMIT_FALLBACK_GBP); let buyDailyTotal = $state(0); const buyLimitReached = $derived(buyDailyTotal >= dailyGiftCardBuyLimit);