From b46927336b2ca78fa4319742bcd42bc4e13db45f Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 14 Aug 2026 16:17:19 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20dup/mod=20secondary=20round=20=E2=80=94?= =?UTF-8?q?=20till=202FA=20gate=20parity,=20mint-cooldown=20single=20sourc?= =?UTF-8?q?e,=20notification=20parity,=20pence=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loop B dup/mod attack findings: - TillPurchases admin 2FA gate now mirrors PaymentModal and the backend: gateActive = twoFactorEnforced && customerTwoFactorEnabled && paymentMethod='saved_card'; the customer's setup flag is fetched from GET /api/admin/users/{id} on selection. A 2FA-disabled customer in an enforced env no longer hits a dead-end blocked input — the charge 403 surfaces the actionable message via the existing self-heal. - Extracted twoFAMintThrottled helper shared by SetupTwoFAHandler and ensurePendingTwoFACode — mint-cooldown rule can no longer drift between setup and disable-flow paths - Notification-helper drift documented: sweep copy states the intentional booking+user scoping vs the canonical webhook copy (cross-referenced); auth refresh_token_reuse insert verified to carry the same NOT EXISTS acknowledged_at IS NULL guard; no import cycle (webhooks→payments one-way) - Pence convention: 'rounded to the cent' corrected to 'pence' (handlers.go:2726) 26/26 backend packages; 72/72 frontend tests + build; env-docs 41/41. --- backend/handlers/payments/handlers.go | 2 +- backend/handlers/payments/sweep.go | 15 +++++- backend/handlers/user/twofa.go | 24 ++++++--- backend/handlers/webhooks/square.go | 6 ++- .../lib/components/admin/TillPurchases.svelte | 49 +++++++++++++++---- 5 files changed, 74 insertions(+), 22 deletions(-) diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index d5f9675..44c5806 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -2723,7 +2723,7 @@ func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingI // // MONEY INVARIANT (deliberately kept exact): the returned records always // partition paymentAmount — deposit + balance + tip === paymentAmount exactly -// (every component is rounded to the cent and the parts are derived from one +// (every component is rounded to the pence and the parts are derived from one // another, so no rounding residue exists). The sum of the split records can // therefore never exceed the amount actually charged at Square. When deposit // AND balance are both zero (booking already fully paid) the tip record alone diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index cc8579e..ac0d9cf 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -1245,8 +1245,19 @@ func leaveGiftCardPurchasePending(ctx context.Context, r staleRow) { // ties to a user (gift-card purchases). The NOT EXISTS guard keeps ONE // notification per issue instead of one per sweep run, and requires the prior // notification to be unacknowledged (acknowledged_at IS NULL) so that after an -// admin acknowledges it, a NEW event for the same booking/user re-notifies — -// matching the webhook copy's guard (handlers/webhooks/square.go) exactly. +// admin acknowledges it, a NEW event for the same booking/user re-notifies. +// +// This is the SWEEP-specific variant of a same-named helper in the webhooks +// package (handlers/webhooks/square.go, insertCriticalPaymentNotification) with +// an INTENTIONAL scoping difference: this one dedups on (reason, booking_id, +// user_id) — booking and/or user — because sweep-originated events may be +// user-attributed without a booking (gift-card purchases) or booking-attributed +// without a user (untracked terminal charges). The webhook variant takes +// (ctx, bookingID, disputeID string), never writes user_id, and dedups on +// (reason, booking_id) or a deterministic per-dispute id. Both share the +// admin_notifications table and the 'critical_payment_log' reason but serve +// different call paths (sweep vs webhook) — do not merge them, and if the +// NOT EXISTS guard shape ever changes, update BOTH copies. func insertCriticalPaymentNotification(ctx context.Context, bookingID, userID *string) { tag, err := db.Conn.Exec(ctx, ` INSERT INTO admin_notifications (reason, booking_id, user_id, created_at) diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 587136a..dd5216e 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -106,6 +106,15 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { return twofa.StateFor(userID) } +// twoFAMintThrottled reports whether a fresh 2FA code mint for the user is +// still inside the per-user cooldown window (twoFAMintCooldown): a previous +// mint within the window throttles the request (429) instead of minting +// another code. Shared by SetupTwoFAHandler and ensurePendingTwoFACode so the +// cooldown rule cannot drift between the setup and disable-flow call paths. +func twoFAMintThrottled(st *twoFAAttemptState, now time.Time) bool { + return !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown +} + // twoFAResetAttempts zeroes the shared per-user attempt counter in place. // Called on successful verify only — a fresh code mint must NOT reset it (B11b). func twoFAResetAttempts(userID string) { @@ -260,13 +269,14 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - // Mint cooldown (B11a): the same twoFAMintCooldown guard the disable flow - // applies via ensurePendingTwoFACode now bounds setup re-mints too. A fresh - // setup code no longer resets the failed-attempt counter (B11b), so without - // this a setup-spam loop could mint fresh codes (each invalidating the - // prior lockout state) and keep a guessing budget alive indefinitely. + // Mint cooldown (B11a): the shared twoFAMintThrottled helper bounds setup + // re-mints — the same guard the disable flow applies via + // ensurePendingTwoFACode. A fresh setup code no longer resets the + // failed-attempt counter (B11b), so without this a setup-spam loop could + // mint fresh codes (each invalidating the prior lockout state) and keep a + // guessing budget alive indefinitely. now := clock.Now() - if !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown { + if twoFAMintThrottled(st, now) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return } @@ -821,7 +831,7 @@ func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptStat return "", pendingExpires.Time.Sub(clock.Now()), nil } now := clock.Now() - if !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown { + if twoFAMintThrottled(st, now) { return "", 0, errTwoFAMintThrottled } code, err := deliverTwoFACode(r, userID, "", purpose) diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 41040a0..8757f77 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -667,8 +667,10 @@ func disputeNotificationID(squareDisputeID string) string { // an.booking_id IS NOT DISTINCT FROM $1 AND an.acknowledged_at IS NULL` (a // notification blocks a re-notify until the admin acknowledges it, then a NEW // event re-arms). sweep.go's insertCriticalPaymentNotification mirrors this -// predicate exactly (its copy is the same guard over (reason, booking_id, -// user_id)); if the guard ever changes, sweep.go must be updated to match. +// predicate in shape (its copy applies the same NOT EXISTS/acknowledged_at +// IS NULL guard over (reason, booking_id, user_id) — an intentional +// user-scoping addition for sweep-originated events, documented on the sweep +// copy itself); if the guard ever changes, sweep.go must be updated to match. // // Untracked disputes (no local payment row, booking_id NULL) pass disputeID // instead: each DISTINCT square dispute gets its OWN notification under the diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte index 72be188..3657a7e 100644 --- a/frontend/src/lib/components/admin/TillPurchases.svelte +++ b/frontend/src/lib/components/admin/TillPurchases.svelte @@ -112,18 +112,26 @@ // B6/B10: charging a customer's saved card via the till requires the // customer's current 2FA verification code when the backend enforces the - // gate. The backend keys on the CARD OWNER (not the admin), so the input is - // surfaced whenever the gate is enforced — the operator relays the - // customer's code. Cash, card machine, and online (new-card nonce) payments - // are unaffected. Shared two-factor-code state (code, reveal, show/missing - // derivations, "Request a new code" handler) — see - // $lib/stores/twoFactorCode.svelte.ts. The admin always supplies the - // CUSTOMER's code — the admin's own 2FA flag is irrelevant to the backend - // gate, so `enabled` is always true. - const savedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); + // gate. The backend keys on the CARD OWNER (not the admin) and only gates + // customers who have actually ENABLED 2FA (requireTwoFactorForCardAccess: + // twoFactorEnforced() && UserTwoFactorEnabled(cardUserID)), so the input is + // surfaced only when BOTH hold — mirroring PaymentModal. The customer's + // setup flag is not carried by the till customer search, so it is fetched + // from GET /api/admin/users/{id} when a customer is selected (see + // fetchCustomerTwoFactor). For a 2FA-disabled customer in an enforced + // environment the input stays hidden so the charge can be attempted; the + // backend then returns the clear "Enable it in your account settings" 403, + // which the isTwoFactorVerificationGateFailure self-heal surfaces. Cash, + // card machine, and online (new-card nonce) payments are unaffected. Shared + // two-factor-code state (code, reveal, show/missing derivations, "Request a + // new code" handler) — see $lib/stores/twoFactorCode.svelte.ts. The admin + // always supplies the CUSTOMER's code — the admin's own 2FA flag is + // irrelevant to the backend gate, so `enabled` is always true. + const twoFactorEnforced = $derived(!!authStore.currentUser?.twoFactorRequired); + let customerTwoFactorEnabled = $state(false); const twoFactor = useTwoFactorCodeForSavedCard({ enabled: () => true, - gateActive: () => savedCardChargeRequires2FACode && paymentMethod === 'saved_card', + gateActive: () => twoFactorEnforced && customerTwoFactorEnabled && paymentMethod === 'saved_card', mint: () => selectedCustomer?.id ? adminRequestNewTwoFactorCode(selectedCustomer.id) : requestNewTwoFactorCode() }); @@ -178,6 +186,7 @@ customerResults = []; showCustomerResults = false; fetchSavedCards(customer.id); + fetchCustomerTwoFactor(customer.id); } function clearSelectedCustomer() { @@ -187,6 +196,7 @@ selectedSavedCardId = null; customerResults = []; showCustomerResults = false; + customerTwoFactorEnabled = false; } async function fetchSavedCards(userId: string) { @@ -208,6 +218,25 @@ } } + // B6/B10: the till customer search (GET /api/admin/users) carries no 2FA + // state, so the selected customer's setup flag is fetched from the admin + // user detail endpoint — the same source PaymentModal's fetchCustomerTwoFactor + // keys on. A failure leaves the flag false; the charge 403 self-heal still + // reveals the input. + async function fetchCustomerTwoFactor(userId: string) { + try { + const res = await apiFetch(`/api/admin/users/${userId}`); + if (res.ok) { + const data = await res.json(); + customerTwoFactorEnabled = data?.twoFactorEnabled === true; + } else { + customerTwoFactorEnabled = false; + } + } catch { + customerTwoFactorEnabled = false; + } + } + const subtotal = $derived(cart.reduce((sum, item) => sum + item.price * item.qty, 0)); const itemCount = $derived(cart.reduce((sum, item) => sum + item.qty, 0));