fix: admin notification flood caps (C5) at every remaining insert site; gift-card expiry-sweep TOCTOU (M4)

- adminnotify: MaxUnacknowledgedCriticalLogs global cap exposed as
  CriticalLogsCapExceeded — a pre-check helper every insert site pairs with the
  atomic fold inside its INSERT (count-then-insert is atomic, closing the
  TOCTOU where concurrent inserts could both read a below-cap count).
- jobs/cleanup.go ScanCriticalPaymentLogs: capped at the shared cap, pre-check
  skips the scan and logs the suppression.
- scheduling: 1_week_no_pay, 1_month_no_pay, default_hours_changed,
  deposit_not_paid_by_deadline and the Square-erasure critical notification all
  flood-capped with pre-check + atomic fold (per-booking/per-user dedup kept).
- time-blockers.go CleanupExpiredGiftCards (M4): the expiry SELECT now runs
  under FOR UPDATE row locks so the read-expired-then-zero window is atomic —
  a concurrent top-up either commits before the SELECT (refreshed last_used_at
  drops the card out of the predicate) or blocks until the sweep's tx ends and
  revives the zeroed card via its own expiry refresh; the top-up value can
  never be destroyed by the sweep.
- flood-cap tests added for 1_week_no_pay; adminnotify unit coverage added.
This commit is contained in:
2026-08-22 00:34:50 +01:00
parent 16304bd295
commit 69a854d857
8 changed files with 553 additions and 30 deletions
+51 -8
View File
@@ -18,6 +18,7 @@ import (
"crussell/clock"
"crussell/db"
"crussell/handlers/payments"
"crussell/internal/adminnotify"
"crussell/internal/dav"
"crussell/internal/s3"
"crussell/internal/square"
@@ -506,11 +507,24 @@ func squareCleanupNotificationID(userID string) string {
func insertSquareCleanupCriticalNotification(ctx context.Context, userID string) {
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 flood cap as every other (webhooks, account
// erasure, jwt reuse). The pre-check logs the suppression; the fold inside
// the INSERT enforces it atomically (count-then-insert). The per-user
// deterministic id (ON CONFLICT (id) DO NOTHING) is preserved, so each
// affected user still gets exactly one notification.
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", "user", userID)
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
`, squareCleanupNotificationID(userID))
`, squareCleanupNotificationID(userID), adminnotify.MaxUnacknowledgedCriticalLogs)
if err != nil {
slog.Error("failed to insert critical notification for failed Square erasure", "user", userID, "err", err)
return
@@ -1274,15 +1288,23 @@ func CleanupExpiredDeposits(ctx context.Context) (int, error) {
userIDs[i] = b.userID
}
_, err = tx.Exec(ctx, `
// C5: flood-cap the unacknowledged 'deposit_not_paid_by_deadline' queue
// (pre-check logs the suppression; the fold inside the INSERT enforces it
// atomically). The per-booking NOT EXISTS dedup is preserved. No early
// return — the time_blocker cleanup below must still run.
if adminnotify.CriticalLogsCapExceeded(ctx, tx, "deposit_not_paid_by_deadline") {
slog.Warn("suppressed deposit_not_paid_by_deadline admin notification — unacknowledged queue at the cap")
} else if _, err = tx.Exec(ctx, `
INSERT INTO admin_notifications (reason, booking_id, user_id)
SELECT 'deposit_not_paid_by_deadline', unnest($1::text[]), unnest($2::text[])
WHERE NOT EXISTS (
SELECT 1 FROM admin_notifications an
WHERE an.booking_id = ANY($1) AND an.reason = 'deposit_not_paid_by_deadline'
)
`, ids, userIDs)
if err != nil {
AND (SELECT COUNT(*) FROM admin_notifications _an
WHERE _an.reason = 'deposit_not_paid_by_deadline'
AND _an.acknowledged_at IS NULL) < $3
`, ids, userIDs, adminnotify.MaxUnacknowledgedCriticalLogs); err != nil {
return 0, fmt.Errorf("failed to create pending_release notifications: %w", err)
}
@@ -1316,14 +1338,27 @@ func CleanupExpiredDeposits(ctx context.Context) (int, error) {
// drift apart.
//
// This function:
// 1. Finds unredeemed cards (redeemed_by IS NULL) unused for the window
// 1. Zeroes unredeemed cards (redeemed_by IS NULL) unused for the window
// 2. Inserts into gift_card_expired_balances for recovery claims
// 3. Sets amount_remaining to 0
// 4. Records transaction in gift_card_transactions
// 3. Records transaction in gift_card_transactions
//
// Once redeemed to an account, the balance doesn't expire (but the account can
// be deleted after idle time per GDPR - see CleanupIdleAccounts).
//
// M4 (expiry-sweep TOCTOU): the expiry decision is made ATOMICALLY with the
// zeroing — the SELECT reads the rolling-expiry predicate under FOR UPDATE row
// locks. A concurrent top-up either commits BEFORE the SELECT (its refreshed
// last_used_at drops the card out of the predicate → the card is never
// selected), or it blocks on the row lock until this sweep's transaction ends
// and lands on the ALREADY-zeroed card, reviving it via its own
// last_used_at/expiry refresh (→ the top-up value is preserved on the card).
// Either way the top-up's value is never destroyed. The old sweep SELECTed the
// expired cards with NO lock and zeroed them with an unconditional
// `UPDATE ... WHERE id = ANY($1)` a moment later, so a top-up committing in
// between was read as expired and then clobbered — the freshly topped-up value
// was lost. The row lock also pins the read balance until commit, so the
// recovery/audit rows below can never disagree with the zeroed value.
//
// Pre-expiry email warnings are intentionally omitted: gift cards are unowned
// (bought as gifts, change hands) until redeemed to an account. After redemption,
// the balance is covered by CleanupIdleAccounts warnings.
@@ -1342,12 +1377,17 @@ func CleanupExpiredGiftCards(ctx context.Context) (int, error) {
return 0, fmt.Errorf("failed to read gift card expiry months: %w", monthsErr)
}
// FOR UPDATE (M4): the predicate is re-evaluated against the latest
// committed row state while the row lock is held, so a concurrent top-up
// can neither be read as expired and clobbered, nor interleave a balance
// change between this read and the zeroing UPDATE below.
rows, err := tx.Query(ctx, `
SELECT id, amount_remaining
FROM gift_cards
WHERE redeemed_by IS NULL
AND amount_remaining > 0
AND last_used_at < NOW() - ($1 * INTERVAL '1 month')
FOR UPDATE
`, expiryMonths)
if err != nil {
return 0, fmt.Errorf("failed to query expired gift cards: %w", err)
@@ -1369,6 +1409,9 @@ func CleanupExpiredGiftCards(ctx context.Context) (int, error) {
}
expiredCards = append(expiredCards, card)
}
if err := rows.Err(); err != nil {
return 0, fmt.Errorf("failed to iterate expired gift cards: %w", err)
}
if len(expiredCards) > 0 {
ids := make([]string, len(expiredCards))