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

fix: loop-A fresh review (503c326 baseline) — overflow-guard bypass, discounted-deposit retry, GDPR audit scrub, till cap, sweep rescue, 2FA reissue + SCA retry, consolidation round

Browse Source 
Loop A fresh money/security/dup-mod review of the whole payments overhaul. 28 consolidated findings fixed:

MONEY:
- HIGH-1: B12 overflow guard now uses the discounted obligation — a pre-start deposit can never mint an unintended tip; the discount is never truncated to £0 when the customer pays the discounted deposit
- HIGH-2: discounted-deposit pending-reuse retry compares pendingStoredAmountPence vs chargeAmount (the actual Square amount), not req.Amount — no more permanent amount_mismatch 400 on lost-response retries
- MEDIUM-3: sweep rescue now carves overflow as a tip record + runs completion side-effects (was booking overflow as service revenue, skipping completion)
- MEDIUM-4 (shared w/ security): admin_audit_log.admin_id made nullable + anonymize_user/delete_guest_user NULL it + scrub details.card_last4 — 2fa_fallback_charge PII no longer survives account deletion
- MEDIUM-5: till gift-card payment now passes the £5,000/day admin cap (giftcard_limits)
- LOW-6: expired gift-card balance surfaced as expired/zero in GetUserGiftCardBalance

SECURITY:
- 2FA single-use consume made atomic at verify time for all 5 saved-card gates (fresh charges consume; pending-reuse retries don't); deferred consumption removed
- reissueTwoFACodeAfterFailedCharge routed through the fail-closed issuance gate (pepper check, cooldown) + fresh-only semantics (only when a code was actually consumed)
- family-alive cache invalidated on the stale-family cleanup DELETE (no 30s warm window after expiry)
- frontend 503-retry no longer reuses a consumed 2FA code — aligns with backend re-issue

DUP/MOD:
- reissue helper single-sourced (5 call sites), squareRefundStatusToLocal (10 inline switches), writeChargeSnapshot (7 sites, immutability guard on gift-card/till), postChargeRecheck (3+1 sites), scanIdempotencySlot (2), applyVATToChargeRecord (3 patterns), user_saved_cards upsert (2), BuyGiftCard pending INSERT via service
- till completed-dedup now re-validates paymentHasLiveRefund (aligns with booking/tip/gift-card)
- frontend 402 idempotency-key regeneration added to PaymentModal (aligns with other CIT surfaces)
- PAYMENT_METHOD_SAVED_CARD constant standardised ('saved_card' everywhere)
- admin audit coverage added for AdminRefundBooking + gift-card buy/top-up
- audit-helper cross-package dedup (user/twofa.go now calls payments' exported insert)

Verified: 26/26 dev + 24/24 prod packages, both vet tags, frontend tests + build, gitleaks clean.
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent b7122be3a0
commit 36887167c6
32 changed files with 1565 additions and 590 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/user/gdpr_test.go
+21 -11
View File
@@ -503,11 +503,11 @@ func TestAnonymizeUser_RetainsEditRequestNotes(t *testing.T) {
// TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback closes the GDPR erasure gap
// for admin_audit_log: a '2fa_fallback_charge' row (insertTwoFAFallbackAudit,
// handlers/payments) carries target_user_id = the erased user PLUS
// details.card_last4 — the audit row MUST survive erasure (GDPR Art 30 records
// of processing / financial audit trail) but de-identified: the user link is
// NULLed exactly as delete_guest_user() does (which scrubs target_user_id only,
// leaving details untouched — anonymize_user mirrors that consistency).
// handlers/payments) carries target_user_id = the erased user, admin_id = the
// customer's own userID (the CIT actor), AND details.card_last4 — the audit row
// MUST survive erasure (GDPR Art 30 records of processing / financial audit
// trail) but be de-identified: the user links (both target_user_id and
// admin_id) are NULLed and the card PII in details is scrubbed.
func TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback(t *testing.T) {
t.Parallel()
ctx, tx := testutils.SetupTestTx(t)
@@ -536,27 +536,37 @@ func TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback(t *testing.T) {
}
// The audit row survives erasure (audit retention) but is de-identified:
// target_user_id is NULLed. details is left untouched, mirroring
// delete_guest_user() exactly (it scrubs target_user_id only).
var targetUserID interface{}
// both the target_user_id and the admin_id (the CIT actor was the erased
// customer) are NULLed, and the card_last4 PII is scrubbed from details.
var targetUserID, adminID interface{}
var details json.RawMessage
err = tx.QueryRow(ctx, `
SELECT target_user_id, details FROM admin_audit_log WHERE id = $1
`, auditID).Scan(&targetUserID, &details)
SELECT target_user_id, admin_id, details FROM admin_audit_log WHERE id = $1
`, auditID).Scan(&targetUserID, &adminID, &details)
if err != nil {
t.Fatalf("failed to query audit row after anonymization: %v", err)
}
if targetUserID != nil {
t.Errorf("expected target_user_id to be NULL after anonymization, got %v", targetUserID)
}
if adminID != nil {
t.Errorf("expected admin_id to be NULL after anonymization (the CIT actor is the erased user), got %v", adminID)
}
if len(details) == 0 {
t.Error("expected the audit row to survive erasure (retained, de-identified)")
}
var detailsMap map[string]any
if err := json.Unmarshal(details, &detailsMap); err != nil {
t.Fatalf("failed to parse retained audit details: %v", err)
}
if last4, ok := detailsMap["card_last4"]; ok && last4 != nil {
t.Errorf("expected details.card_last4 to be scrubbed after anonymization, got %v", last4)
}
// No residual audit rows may still reference the erased user.
var remaining int
err = tx.QueryRow(ctx, `
SELECT COUNT(*) FROM admin_audit_log WHERE target_user_id = $1
SELECT COUNT(*) FROM admin_audit_log WHERE target_user_id = $1 OR admin_id = $1
`, userID).Scan(&remaining)
if err != nil {
t.Fatalf("failed to count residual audit rows: %v", err)
backend/handlers/user/twofa.go
+2 -41
View File
@@ -8,7 +8,6 @@ import (
"errors"
"fmt"
"log"
"log/slog"
"math/big"
"net/http"
"time"
@@ -605,44 +604,6 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) {
}
}
// insertAdminAudit records an admin action in admin_audit_log. Mirrors the
// insertAdminAuditCharge pattern (handlers/payments/handlers.go) — same table,
// same columns, same best-effort non-fatal failure handling. The insert runs in
// its OWN transaction (a savepoint in the test harness) so an audit-write
// failure — e.g. a synthetic admin id in tests violating the admin_id FK —
// rolls back only the audit write and can never abort the caller's transaction.
func insertAdminAudit(ctx context.Context, adminID, targetUserID, action string, details map[string]any) {
detailsJSON, err := json.Marshal(details)
if err != nil {
log.Printf("Failed to marshal admin_audit_log details (non-critical): %v", err)
return
}
var target any
if targetUserID != "" {
target = targetUserID
}
auditTx, err := db.Conn.Begin(ctx)
if err != nil {
log.Printf("Failed to record admin_audit_log (non-critical): %v", err)
return
}
defer func() {
if err := auditTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) {
slog.Error("failed to rollback admin audit transaction", "err", err)
}
}()
if _, err := auditTx.Exec(ctx, `
INSERT INTO admin_audit_log (admin_id, action_type, target_user_id, details)
VALUES ($1, $2, $3, $4::jsonb)
`, adminID, action, target, string(detailsJSON)); err != nil {
log.Printf("Failed to record admin_audit_log (non-critical): %v", err)
return
}
if err := auditTx.Commit(ctx); err != nil {
log.Printf("Failed to record admin_audit_log (non-critical): %v", err)
}
}
// AdminSendVerificationCodeHandler mints (or reuses) a 2FA code for a TARGET
// user, not the session user. The saved-card charge gate verifies the code
// against the CARD OWNER (customer) — never the admin session (till.go:951,
@@ -703,8 +664,8 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) {
// `code` return discriminates reuse from a fresh mint — the plaintext is
// only returned for a fresh delivery, never on reuse (EnsurePendingTwoFACode).
if adminID, ok := mw.GetUserID(r.Context()); ok {
insertAdminAudit(r.Context(), adminID, targetUserID, "2fa_code_mint", map[string]any{
"reused": code == "",
payments.InsertAdminAuditCharge(r.Context(), adminID, targetUserID, "2fa_code_mint", map[string]any{
"reused": code == "",
"remaining_seconds": int(remaining.Seconds()),
})
}