diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 487463b..eaf5771 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -2958,6 +2958,19 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } log.Printf("Payment %s for booking %s was already resolved to %q by a concurrent resolver (Square webhook/sweep) before the sync completion flip — skipping split records, VAT and booking-completion side-effects", paymentID, bookingID, curStatus) + // R10 belt-and-braces: verify the booking IS actually completed. The + // webhook path runs ApplyBookingCompletionSideEffects (which applies + // campaigns at completion time) only when the booking is fully paid and + // transitions to 'completed'. If the booking is NOT completed despite + // the payment being completed, the booking is stuck in a non-terminal + // state with a completed payment — manual reconciliation required. + var bookingStatus string + if bErr := tx2.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus); bErr != nil || bookingStatus != "completed" { + log.Printf("CRITICAL: Square payment %s (ID=%s) was processed and payment row %s is completed, but booking %s is in status %q (not 'completed') — the booking is stuck in a non-terminal state with a completed payment — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, paymentID, bookingID, bookingStatus) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } // MEDIUM-2: burn the re-issued 2FA code anyway (idempotent) — the // charge reached terminal success, so a single-use code re-issued for // this retry must not authorize another charge. @@ -4906,7 +4919,17 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { if req.VerificationToken != nil { tipVerificationToken = *req.VerificationToken } - if req.SaveCard { + // savedCardRef is the effective saved-card reference for this request: + // card_id is the saved-card ref; when a NEW-card token arrives alongside it + // (SCA tokenize-result wire contract), the token is the one-time charge + // source and this row supplies the customer. + savedCardRef := req.CardID + scaTokenizedSavedCard := req.NewCardToken != nil && *req.NewCardToken != "" && savedCardRef != nil && *savedCardRef != "" + saveGateToken := "" + if req.NewCardToken != nil { + saveGateToken = *req.NewCardToken + } + if req.SaveCard && !scaTokenizedSavedCard && !isSCATokenizeResultShape(saveGateToken) { if gateOK, _ := requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, tipVerificationToken, true, false); !gateOK { return } @@ -5158,7 +5181,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // re-entering the gate. A charge carrying a Square verification_token (SCA // performed) skips the gate; a token-less charge is refused 402 // verification_required (SCA-only — the homegrown 2FA fallback was removed). - if req.CardID != nil && *req.CardID != "" { + if savedCardRef != nil && *savedCardRef != "" && !scaTokenizedSavedCard { if gateOK, _ := requireTwoFactorForCardAccess(w, r, service, userID, tipVerificationToken, !reusePendingRecord); !gateOK { return } diff --git a/backend/handlers/payments/idempotency_helpers.go b/backend/handlers/payments/idempotency_helpers.go index 9d944de..dde0807 100644 --- a/backend/handlers/payments/idempotency_helpers.go +++ b/backend/handlers/payments/idempotency_helpers.go @@ -109,7 +109,7 @@ func scanIdempotencySlot(ctx context.Context, baseKey string, occupied func(cand // snapshot handling (giftcards.go), and main.go's startup warnings. The // exported name is stable for main.go; in-package callers use it directly. func IsExplicitDevOrMockEnv() bool { - switch os.Getenv("SQUARE_ENVIRONMENT") { + switch strings.ToLower(strings.TrimSpace(os.Getenv("SQUARE_ENVIRONMENT"))) { case "mock", "dev", "development", "test": return true default: diff --git a/backend/handlers/payments/idempotency_helpers_test.go b/backend/handlers/payments/idempotency_helpers_test.go index 6d85724..0ba8ccf 100644 --- a/backend/handlers/payments/idempotency_helpers_test.go +++ b/backend/handlers/payments/idempotency_helpers_test.go @@ -124,3 +124,46 @@ func TestDeriveRefundIdempotencyKey_RetryAfterSweepResolution_NoSecondRefund(t * t.Errorf("expected exactly ONE Square refund after the retry, got %d distinct refund keys", got) } } + +// TestIsExplicitDevOrMockEnv_NormalizedPins the FIX 5 normalization: the env +// value is lowercased and trimmed before comparison, so "Mock", " MOCK ", +// "Production " (space), and "PROD" all map correctly. Empty/unknown stays +// fail-closed (false). +func TestIsExplicitDevOrMockEnv_Normalized(t *testing.T) { + cases := []struct { + env string + want bool + }{ + // Exact matches (unchanged behavior) + {"mock", true}, + {"dev", true}, + {"development", true}, + {"test", true}, + // Case normalization + {"Mock", true}, + {"MOCK", true}, + {"Dev", true}, + {"DEVELOPMENT", true}, + // Trailing/leading whitespace + {" mock ", true}, + {" mock ", true}, + {"mock ", true}, + {"", false}, + {"production", false}, + {"PROD", false}, + {"Production ", false}, + {" PRODUCTION ", false}, + {"sandbox", false}, + {"staging", false}, + {"unknown", false}, + } + for _, tc := range cases { + t.Run(tc.env, func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", tc.env) + got := IsExplicitDevOrMockEnv() + if got != tc.want { + t.Errorf("IsExplicitDevOrMockEnv(%q) = %v, want %v", tc.env, got, tc.want) + } + }) + } +} diff --git a/backend/handlers/payments/money_safety_fixes_test.go b/backend/handlers/payments/money_safety_fixes_test.go index 03c4dfe..6f3c4ad 100644 --- a/backend/handlers/payments/money_safety_fixes_test.go +++ b/backend/handlers/payments/money_safety_fixes_test.go @@ -404,10 +404,84 @@ func TestBuyGiftCard_ForeignIdempotencyKey_NotReused(t *testing.T) { } // ============================================================================= -// H4 — a COMPLETED provisional terminal checkout must be recorded, not just -// released +// Fix 1 — tip 2FA gate asymmetry: saved-card tip with SCA tokenize-result +// must skip the gate in an enforced deployment // ============================================================================= +// TestCreateTipPayment_EnforcedSavedCard_SCATokenizeResult_Succeeds locks the +// Fix 1 gate skip: a saved-card tip carrying an SCA tokenize-result token +// (new_card_token alongside card_id) must skip the 2FA gate and complete, +// matching the CreateBookingPayment scaTokenizedSavedCard pattern. Without the +// fix, the tip path gates on card_id alone and refuses 402 +// verification_required because the legacy verification_token field is empty. +func TestCreateTipPayment_EnforcedSavedCard_SCATokenizeResult_Succeeds(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + // A completed payment is required before a tip can be added. + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_tip_sca_ok", "VISA", "4242") + require.NoError(t, err) + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.SimulateSavedCardVerificationRequired = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + scaToken := "cnon:sca-4242_500_ok" + req := CreateTipPaymentRequest{ + Amount: 500, + CardID: &cardID, + NewCardToken: &scaToken, + IdempotencyKey: "enforced-tip-scatokenized", + } + + w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "an SCA tokenize-result tip must skip the enforced gate and complete, body: %s", w.Body.String()) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'tip'`, bookingID).Scan(&payCount)) + require.Equal(t, 1, payCount, "the SCA-tokenized tip must record exactly one completed tip payment") +} + +// TestCreateTipPayment_EnforcedSavedCard_SCATokenizeResult_SaveCard_Succeeds +// locks the Fix 1 SAVE gate skip: a save-card tip carrying an SCA tokenize-result +// token must skip the SAVE gate and persist the card, matching the +// CreateBookingPayment isSCATokenizeResultShape pattern. +func TestCreateTipPayment_EnforcedSavedCard_SCATokenizeResult_SaveCard_Succeeds(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + // A cnon:sca-... token with save_card=true and no card_id is the + // NEW-card SCA tokenize-result save shape — isSCATokenizeResultShape + // must recognise it and skip the SAVE gate. Use the regular mock + // (no SimulateSavedCardVerificationRequired) so the ccof charge from + // CreateCardOnFile succeeds. + scaToken := "cnon:sca-round9-save-tip" + req := CreateTipPaymentRequest{ + Amount: 500, + NewCardToken: &scaToken, + SaveCard: true, + IdempotencyKey: "enforced-tip-scasave", + } + + w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "an SCA tokenize-result tip with save_card=true must skip the enforced SAVE gate and complete, body: %s", w.Body.String()) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'tip'`, bookingID).Scan(&payCount)) + require.Equal(t, 1, payCount, "the SCA-tokenized save-card tip must record exactly one completed tip payment") +} + // TestActiveTerminalCheckoutID_ProvisionalCompleted_RecordsPayment locks the // H4 fix: when activeTerminalCheckoutID discovers a provisional (tmp-) // checkout COMPLETED at Square, it must RECORD the payment (mirroring the diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 8cd54f0..07e63af 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -80,6 +80,7 @@ func (s *PaymentService) TwoFactorEnforced() bool { // extractErrorMessage) and allowed=false is returned — the caller must abort // the charge. func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationToken string, consume bool) (allowed, fallbackUsed bool) { + _ = consume // TODO: remove when callers updated — the SCA-only gate never reads verification codes return requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, verificationToken, consume, true) } @@ -107,6 +108,7 @@ func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, servi // the gate never read it (SCA-only), the request structs no longer carry it, // and there is no homegrown fallback to authorise anything with it. func requireTwoFactorForCardAccessWithTokenValidation(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationToken string, consume bool, tokenForwardedToSquare bool) (allowed, fallbackUsed bool) { + _ = consume // TODO: remove when callers updated — the SCA-only gate never reads verification codes if !twoFactorEnforced() { return true, false } diff --git a/backend/handlers/scheduling/time-blockers.go b/backend/handlers/scheduling/time-blockers.go index 8a07388..e9a3a71 100644 --- a/backend/handlers/scheduling/time-blockers.go +++ b/backend/handlers/scheduling/time-blockers.go @@ -765,11 +765,20 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { // deletion calls themselves never block the local erasure.) var cardsByUser map[string][]string var customers map[string]string + // FIX 1: durable Square erasure outbox — capture row IDs BEFORE the UPDATE + // NULLs square_card_id/square_customer_id, so we can restore them on the + // soft-deleted rows inside the tx (crash-safe via retry-square-erasures job). + type squareErasureRow struct { + rowID string + cardID string + customerID string + } + var erasureRows []squareErasureRow if payments.SquareClient != nil { cardsByUser = map[string][]string{} customers = map[string]string{} rows, err := db.Conn.Query(ctx, ` - SELECT usc.user_id, usc.square_card_id, usc.square_customer_id + SELECT usc.id, usc.user_id, usc.square_card_id, usc.square_customer_id FROM user_saved_cards usc JOIN users u ON u.id = usc.user_id WHERE u.account_role = 'guest' @@ -782,8 +791,8 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { } userSeen := map[string]bool{} for rows.Next() { - var userID, cardID, customerID sql.NullString - if err := rows.Scan(&userID, &cardID, &customerID); err != nil { + var rowID, userID, cardID, customerID sql.NullString + if err := rows.Scan(&rowID, &userID, &cardID, &customerID); err != nil { rows.Close() return 0, fmt.Errorf("failed to scan stale-guest saved card: %w", err) } @@ -803,6 +812,14 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { customers[customerID.String] = userID.String } } + // FIX 1: capture row-level data for the durable outbox restore. + if rowID.Valid && rowID.String != "" { + erasureRows = append(erasureRows, squareErasureRow{ + rowID: rowID.String, + cardID: cardID.String, + customerID: customerID.String, + }) + } } rows.Close() if err := rows.Err(); err != nil { @@ -961,6 +978,30 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { } totalRows += int(tag.RowsAffected()) + // FIX 1: durable Square erasure outbox — restore the Square references on the + // just-scrubbed, soft-deleted rows INSIDE the tx, BEFORE the commit. If the + // process crashes after the commit but before the post-commit Square deletion, + // the retry-square-erasures job finds these rows (deleted_at IS NOT NULL AND + // last_4 = 'XXXX' AND square_card_id IS NOT NULL) and completes the erasure. + if len(erasureRows) > 0 { + for _, er := range erasureRows { + var cardID, customerID any + if er.cardID != "" { + cardID = er.cardID + } + if er.customerID != "" { + customerID = er.customerID + } + if _, err := tx.Exec(ctx, ` + UPDATE user_saved_cards + SET square_card_id = $2, square_customer_id = $3, user_id = NULL + WHERE id = $1 AND deleted_at IS NOT NULL + `, er.rowID, cardID, customerID); err != nil { + return 0, fmt.Errorf("failed to persist Square erasure outbox for row %s: %w", er.rowID, err) + } + } + } + // Scrub Square CreatePayment request snapshots (payments / till_sales): // the stored replay JSON embeds the guest's email as BuyerEmail (PII, GDPR // Art 17 / Art 5(1)(e)). The financial rows MUST survive the 7-year diff --git a/backend/handlers/scheduling/time_blockers_test.go b/backend/handlers/scheduling/time_blockers_test.go index 4ed6a84..e353a6f 100644 --- a/backend/handlers/scheduling/time_blockers_test.go +++ b/backend/handlers/scheduling/time_blockers_test.go @@ -4413,3 +4413,75 @@ func TestCleanupIdleAccounts_S3OutboxPersisted(t *testing.T) { t.Errorf("expected object_key %q, got %q", "profiles/"+userID+".jpg", objectKey) } } + +// TestAnonymizeStaleGuestAccounts_SquareErasureOutboxPersisted verifies FIX 1: +// the stale-guest erasure restores Square references on the soft-deleted +// user_saved_cards rows INSIDE the transaction (crash-safe outbox). After the +// anonymize commit, the retry-square-erasures job finds these rows and completes +// the Square deletion. Deliberately NOT t.Parallel: swaps the package-level +// payments.SquareClient. +func TestAnonymizeStaleGuestAccounts_SquareErasureOutboxPersisted(t *testing.T) { + ctx, tx := resetTestData(t) + + guestID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create guest: %v", err) + } + if _, err := tx.Exec(ctx, `UPDATE users SET account_role = 'guest' WHERE id = $1`, guestID); err != nil { + t.Fatalf("failed to set guest role: %v", err) + } + if _, err := tx.Exec(ctx, ` + INSERT INTO bookings (user_id, start_time, status, deposit_required) + VALUES ($1, NOW() - INTERVAL '7 months', 'completed', false) + `, guestID); err != nil { + t.Fatalf("failed to create stale booking: %v", err) + } + var cardRowID string + if err := tx.QueryRow(ctx, ` + INSERT INTO user_saved_cards (user_id, square_card_id, square_customer_id, brand, last_4, exp_month, exp_year, fingerprint, is_default) + VALUES ($1, 'ccof:stale_card_outbox', 'cus_stale_outbox', 'Visa', '4242', 12, 2030, 'fp_outbox', true) + RETURNING id + `, guestID).Scan(&cardRowID); err != nil { + t.Fatalf("failed to insert saved card: %v", err) + } + + origSquare := payments.SquareClient + rec := &recordingDisableClient{} + payments.SquareClient = rec + defer func() { payments.SquareClient = origSquare }() + + if _, err := AnonymizeStaleGuestAccounts(ctx); err != nil { + t.Fatalf("AnonymizeStaleGuestAccounts failed: %v", err) + } + + // After the anonymize tx commits, the soft-deleted row must still carry + // the Square references (the outbox restore ran before commit). + var squareCardID, squareCustomerID *string + if err := tx.QueryRow(ctx, ` + SELECT square_card_id, square_customer_id FROM user_saved_cards WHERE id = $1 + `, cardRowID).Scan(&squareCardID, &squareCustomerID); err != nil { + t.Fatalf("failed to query outbox row: %v", err) + } + if squareCardID == nil || *squareCardID != "ccof:stale_card_outbox" { + t.Errorf("expected square_card_id to be preserved on the outbox row, got %v", squareCardID) + } + if squareCustomerID == nil || *squareCustomerID != "cus_stale_outbox" { + t.Errorf("expected square_customer_id to be preserved on the outbox row, got %v", squareCustomerID) + } + + // The row must be soft-deleted (deleted_at set) and last_4 = 'XXXX' so the + // retry-square-erasures job's query finds it. + var deletedAt *time.Time + var last4 string + if err := tx.QueryRow(ctx, ` + SELECT deleted_at, last_4 FROM user_saved_cards WHERE id = $1 + `, cardRowID).Scan(&deletedAt, &last4); err != nil { + t.Fatalf("failed to query outbox row state: %v", err) + } + if deletedAt == nil { + t.Error("expected the outbox row to be soft-deleted (deleted_at set)") + } + if last4 != "XXXX" { + t.Errorf("expected last_4 to be 'XXXX' on the outbox row, got %q", last4) + } +} diff --git a/backend/handlers/user/account.go b/backend/handlers/user/account.go index 0f59b74..fed1876 100644 --- a/backend/handlers/user/account.go +++ b/backend/handlers/user/account.go @@ -360,19 +360,30 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { // FIX 2: apply the shared current-password failed-attempt budget BEFORE // the compare — a stolen session token must not be able to brute-force // the current password with unlimited guesses. + // + // FIX 1 (round-9): a user locked out by LOGIN attacks (shared + // failed_attempts/locked_until columns) can still recover by providing + // the CORRECT current password here — the lockout is cleared on + // success. When locked out we still run the bcrypt compare (one + // attempt), and if the password is correct the lockout is lifted. If + // the password is wrong while locked out, no additional failure is + // recorded (the lockout stands). + lockedOut := false if err := checkCurrentPasswordLockout(ctx, userID); err != nil { if errors.Is(err, errCurrentPasswordLockedOut) { - // FIX 4: uniform 401 — the same status as a wrong password, so - // locked-vs-wrong is never distinguishable; the body text still - // tells the UI which one happened. + lockedOut = true + } else { + log.Printf("Failed to check current-password lockout for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + } + if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { + if lockedOut { + // FIX 1: already locked out — don't increment further. http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized) return } - log.Printf("Failed to check current-password lockout for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) - return - } - if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { // FIX 3: the failure record is ONE atomic UPDATE ... RETURNING // (increment + escalation) — concurrent wrong-password requests // cannot race a check-then-increment and lose updates. @@ -387,9 +398,17 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "current password is incorrect", http.StatusUnauthorized) return } + // FIX 1: a correct current password clears the shared lockout, so a + // login-locked-out user can self-recover by deleting their account. resetCurrentPasswordFailures(ctx, userID) } - if !hasPassword || (twoFARequired() && twoFactorEnabled) { + // FIX 2 (round-9): passwordless accounts need 2FA only when enforcement is + // active (twoFARequired()). In unenforced environments (dev/test) the code + // cannot be minted (no delivery channel), so skip the 2FA gate — the + // passwordless property and the authenticated session are the protection. + // Has-password accounts need 2FA only when enforcement is active AND the + // user has 2FA enabled. + if twoFARequired() && (!hasPassword || twoFactorEnabled) { if req.VerificationCode == "" { http.Error(w, "a two-factor verification code is required to delete the account", http.StatusBadRequest) return diff --git a/backend/handlers/user/account_round9_fixes_test.go b/backend/handlers/user/account_round9_fixes_test.go index f0ebd8f..fe8c2cb 100644 --- a/backend/handlers/user/account_round9_fixes_test.go +++ b/backend/handlers/user/account_round9_fixes_test.go @@ -35,6 +35,10 @@ import ( // password is rejected with 429), and a cleared lockout lets the correct // password through. Sequential (no t.Parallel): the handler reads the // process-global s3.Client / payments.SquareClient. +// +// FIX 1 (round-9): a locked-out user who provides the CORRECT current password +// clears the lockout and succeeds — the test verifies that the 6th attempt +// with the correct password now succeeds (the lockout is lifted on success). func TestDeleteAccount_CurrentPasswordLockout(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -49,28 +53,19 @@ func TestDeleteAccount_CurrentPasswordLockout(t *testing.T) { require.Equal(t, http.StatusUnauthorized, rr.Code, "wrong current password must be rejected (attempt %d)", i+1) } - // The budget is now locked: even the correct password is rejected. FIX 4: - // a locked account returns the SAME uniform 401 as a wrong password (never - // distinguishable), with a distinct body the UI can surface. + // FIX 1: the correct password now clears the lockout and succeeds (the + // locked-out user can self-recover). req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") rr := httptest.NewRecorder() DeleteAccountHandler(rr, req) - require.Equal(t, http.StatusUnauthorized, rr.Code, "delete-account must be rejected with a lockout after 5 wrong current passwords") - require.Contains(t, rr.Body.String(), "too many failed attempts", "the locked body must stay distinct for the UI") + require.Equal(t, http.StatusNoContent, rr.Code, "a locked-out user with the correct current password must be able to recover (FIX 1)") - // The account survives the lockout. - var firstName string - require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) - require.Equal(t, "Test", firstName) - - // Clearing the lockout (the documented operator / password-reset recovery) - // lets the correct password through. - _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID) - require.NoError(t, err) - req = deleteAccountRequest(t, ctx, userID, "testpassword123", "") - rr = httptest.NewRecorder() - DeleteAccountHandler(rr, req) - require.Equal(t, http.StatusNoContent, rr.Code, "correct password must succeed after the lockout is reset") + // The lockout was cleared on success. + var failedAttempts int + var lockedUntil *time.Time + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil)) + require.Zero(t, failedAttempts, "failed_attempts must be reset to 0 after a successful recovery") + require.Nil(t, lockedUntil, "locked_until must be NULL after a successful recovery") } // ============================================================================ @@ -96,6 +91,9 @@ func changePasswordRequest(t *testing.T, ctx context.Context, userID, currentPas // the same failed-attempt/lockout columns as delete-account: 5 wrong current // passwords lock the change-password flow (correct password → 429), and a // cleared lockout lets it through. +// +// FIX 1 (round-9): a locked-out user who provides the CORRECT current password +// clears the lockout and succeeds. func TestPasswordChange_CurrentPasswordLockout(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -108,21 +106,18 @@ func TestPasswordChange_CurrentPasswordLockout(t *testing.T) { require.Equal(t, http.StatusUnauthorized, rr.Code, "wrong current password must be rejected (attempt %d)", i+1) } - // Locked out: the correct current password is rejected too. FIX 4: uniform - // 401 (never distinguishable from a wrong password), distinct body text. + // FIX 1: the correct password now clears the lockout and succeeds. req := changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456") rr := httptest.NewRecorder() ChangePasswordHandler(rr, req) - require.Equal(t, http.StatusUnauthorized, rr.Code, "change-password must be rejected with a lockout after 5 wrong current passwords") - require.Contains(t, rr.Body.String(), "too many failed attempts", "the locked body must stay distinct for the UI") + require.Equal(t, http.StatusOK, rr.Code, "a locked-out user with the correct current password must be able to recover (FIX 1)") - // The correct password works again once the lockout is cleared. - _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID) - require.NoError(t, err) - req = changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456") - rr = httptest.NewRecorder() - ChangePasswordHandler(rr, req) - require.Equal(t, http.StatusOK, rr.Code, "correct current password must succeed after the lockout is reset") + // The lockout was cleared on success. + var failedAttempts int + var lockedUntil *time.Time + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil)) + require.Zero(t, failedAttempts, "failed_attempts must be reset to 0 after a successful recovery") + require.Nil(t, lockedUntil, "locked_until must be NULL after a successful recovery") } // ============================================================================ @@ -302,44 +297,6 @@ func TestDeleteAccount_ConcurrentWrongPassword_NoLostUpdates(t *testing.T) { // FIX 5 — passwordless (NULL password_hash) accounts // ============================================================================ -// TestDeleteAccount_Passwordless_Requires2FAUnconditionally verifies FIX 5a: a -// NULL-password-hash (social-only) account has no current password to -// re-verify, so deleting it requires the 2FA code gate UNCONDITIONALLY — even -// when 2FA is not otherwise enforced — so a session holder cannot erase a -// passwordless account with zero credential proof. -func TestDeleteAccount_Passwordless_Requires2FAUnconditionally(t *testing.T) { - ctx, tx := testutils.SetupTestTx(t) - userID, err := fixtures.CreateTestUser(tx) - require.NoError(t, err) - _, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID) - require.NoError(t, err) - - // No code → rejected with the exact message the frontend uses to reveal the - // 2FA step (deleteRevealTwoFactor). - req := deleteAccountRequest(t, ctx, userID, "", "") - rr := httptest.NewRecorder() - DeleteAccountHandler(rr, req) - require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) - require.Contains(t, rr.Body.String(), "a two-factor verification code is required to delete the account") - - // A wrong code is rejected too (the account survives). - seedPendingTwoFA(t, ctx, tx, userID, "424242") - req = deleteAccountRequest(t, ctx, userID, "", "000000") - rr = httptest.NewRecorder() - DeleteAccountHandler(rr, req) - require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) - - var firstName string - require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) - require.Equal(t, "Test", firstName, "the account must survive a rejected code") - - // A correct fresh code is the sole credential — it deletes the account. - req = deleteAccountRequest(t, ctx, userID, "", "424242") - rr = httptest.NewRecorder() - DeleteAccountHandler(rr, req) - require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) -} - // TestPasswordChange_NullHash_NoPasswordToChange verifies FIX 5b: changing the // password on a passwordless (NULL hash) account is a clear 400 with an // actionable message — not the old 500 from scanning NULL into a plain string. @@ -396,3 +353,183 @@ func TestDeleteAccount_DavCardDeletedInErasureTx(t *testing.T) { require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM dav_cards WHERE uri = $1`, uri).Scan(&countAfter)) require.Zero(t, countAfter, "the dav_cards row must be deleted inside the erasure transaction") } + +// ============================================================================ +// FIX 1 (round-9) — current-password clears login lockout on success +// ============================================================================ + +// TestPasswordChange_LockedOutUserCanRecover verifies FIX 1: a user with +// locked_until set (locked out by LOGIN attacks) can still change their +// password by providing the CORRECT current password. The handler runs the +// bcrypt compare even when locked out, and on success clears the shared +// lockout (failed_attempts = 0, locked_until = NULL). +func TestPasswordChange_LockedOutUserCanRecover(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // Simulate a login lockout: set locked_until in the future. + future := clock.Now().Add(30 * time.Minute) + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future) + require.NoError(t, err) + + // The user is locked out but provides the CORRECT current password. + req := changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456") + rr := httptest.NewRecorder() + ChangePasswordHandler(rr, req) + require.Equal(t, http.StatusOK, rr.Code, "a locked-out user with the correct current password must be able to change their password") + + // The lockout was cleared on success. + var failedAttempts int + var lockedUntil *time.Time + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil)) + require.Zero(t, failedAttempts, "failed_attempts must be reset to 0 after a successful password change") + require.Nil(t, lockedUntil, "locked_until must be NULL after a successful password change") +} + +// TestPasswordChange_LockedOutUserWrongPassword verifies FIX 1: a locked-out +// user who provides a WRONG current password is rejected without incrementing +// the counter further (the lockout stands). +func TestPasswordChange_LockedOutUserWrongPassword(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + future := clock.Now().Add(30 * time.Minute) + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future) + require.NoError(t, err) + + req := changePasswordRequest(t, ctx, userID, "wrong-password", "newpassword456") + rr := httptest.NewRecorder() + ChangePasswordHandler(rr, req) + require.Equal(t, http.StatusUnauthorized, rr.Code, "a locked-out user with a wrong password must be rejected") + require.Contains(t, rr.Body.String(), "too many failed attempts") + + // The counter was NOT incremented (still 5). + var failedAttempts int + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts FROM users WHERE id = $1`, userID).Scan(&failedAttempts)) + require.Equal(t, 5, failedAttempts, "failed_attempts must NOT be incremented when already locked out") +} + +// TestDeleteAccount_LockedOutUserCanRecover verifies FIX 1: a user with +// locked_until set (locked out by LOGIN attacks) can still delete their +// account by providing the CORRECT current password. The lockout is cleared +// on success. +func TestDeleteAccount_LockedOutUserCanRecover(t *testing.T) { + savedClient := s3.Client + s3.Client = nil + t.Cleanup(func() { s3.Client = savedClient }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // Simulate a login lockout. + future := clock.Now().Add(30 * time.Minute) + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future) + require.NoError(t, err) + + // The user is locked out but provides the CORRECT current password. + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, "a locked-out user with the correct current password must be able to delete their account") + + // The lockout was cleared on success. + var failedAttempts int + var lockedUntil *time.Time + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil)) + require.Zero(t, failedAttempts, "failed_attempts must be reset to 0 after a successful delete") + require.Nil(t, lockedUntil, "locked_until must be NULL after a successful delete") +} + +// TestDeleteAccount_LockedOutUserWrongPassword verifies FIX 1: a locked-out +// user who provides a WRONG current password is rejected without incrementing +// the counter further. +func TestDeleteAccount_LockedOutUserWrongPassword(t *testing.T) { + savedClient := s3.Client + s3.Client = nil + t.Cleanup(func() { s3.Client = savedClient }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + future := clock.Now().Add(30 * time.Minute) + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future) + require.NoError(t, err) + + req := deleteAccountRequest(t, ctx, userID, "wrong-password", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusUnauthorized, rr.Code, "a locked-out user with a wrong password must be rejected") + require.Contains(t, rr.Body.String(), "too many failed attempts") + + // The counter was NOT incremented (still 5). + var failedAttempts int + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts FROM users WHERE id = $1`, userID).Scan(&failedAttempts)) + require.Equal(t, 5, failedAttempts, "failed_attempts must NOT be incremented when already locked out") +} + +// ============================================================================ +// FIX 2 (round-9) — passwordless delete-account 2FA condition +// ============================================================================ + +// TestDeleteAccount_Passwordless_Requires2FAInEnforcedEnv verifies FIX 2: a +// NULL-password-hash (social-only) account requires a 2FA code ONLY when 2FA +// enforcement is active. In enforced env, the code gate protects against a +// stolen session token erasing the account with zero credential proof. +func TestDeleteAccount_Passwordless_Requires2FAInEnforcedEnv(t *testing.T) { + twofaEnvEnforced(t) + savedClient := s3.Client + s3.Client = nil + t.Cleanup(func() { s3.Client = savedClient }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID) + require.NoError(t, err) + + // No code → rejected with the 2FA-required message. + req := deleteAccountRequest(t, ctx, userID, "", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) + require.Contains(t, rr.Body.String(), "a two-factor verification code is required to delete the account") + + // A correct fresh code deletes the account. + seedPendingTwoFA(t, ctx, tx, userID, "424242") + req = deleteAccountRequest(t, ctx, userID, "", "424242") + rr = httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) +} + +// TestDeleteAccount_Passwordless_No2FARequiredInUnenforcedEnv verifies FIX 2: +// in an unenforced environment (dev/test), a passwordless account can delete +// without a 2FA code — the code cannot be minted (no delivery channel), and +// the passwordless property plus the authenticated session are the protection. +func TestDeleteAccount_Passwordless_No2FARequiredInUnenforcedEnv(t *testing.T) { + twofaEnvUnenforced(t) + savedClient := s3.Client + s3.Client = nil + t.Cleanup(func() { s3.Client = savedClient }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID) + require.NoError(t, err) + + // No code required — the delete succeeds with just the authenticated session. + req := deleteAccountRequest(t, ctx, userID, "", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) + + // The account was anonymized (anonymize_user() sets name to 'Deleted'). + var firstName string + require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) + require.Equal(t, "Deleted", firstName, "the passwordless account must be anonymized") +} diff --git a/backend/handlers/user/profile.go b/backend/handlers/user/profile.go index d5ab0f6..c7860fa 100644 --- a/backend/handlers/user/profile.go +++ b/backend/handlers/user/profile.go @@ -725,18 +725,29 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { // users.failed_attempts/locked_until). Without it a stolen session token // would let an attacker brute-force the current password with unlimited // guesses. + // + // FIX 1 (round-9): a user locked out by LOGIN attacks (shared + // failed_attempts/locked_until columns) can still recover by providing the + // CORRECT current password here — the lockout is cleared on success. When + // the user is locked out we still run the bcrypt compare (one attempt), and + // if the password is correct the lockout is lifted. If the password is wrong + // while locked out, no additional failure is recorded (the lockout stands). + lockedOut := false if err := checkCurrentPasswordLockout(r.Context(), userID); err != nil { if errors.Is(err, errCurrentPasswordLockedOut) { - // FIX 4: uniform 401 — locked-vs-wrong is never distinguishable; - // the body text still tells the UI which one happened. + lockedOut = true + } else { + log.Printf("Failed to check current-password lockout for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + } + if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { + if lockedOut { + // FIX 1: already locked out — don't increment further, just reject. http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized) return } - log.Printf("Failed to check current-password lockout for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) - return - } - if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { // FIX 3: one atomic UPDATE ... RETURNING (increment + escalation) — // concurrent wrong-password requests cannot race a check-then-increment. newCount, _, recordErr := recordCurrentPasswordFailure(r.Context(), userID) @@ -750,6 +761,8 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "current password is incorrect", http.StatusUnauthorized) return } + // FIX 1: a correct current password clears the shared lockout, so a + // login-locked-out user can self-recover by changing their password. resetCurrentPasswordFailures(r.Context(), userID) newHash, err := bcrypt.GenerateFromPassword([]byte(req.NewPassword), bcrypt.DefaultCost) diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 179d21c..40ace04 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -91,6 +91,48 @@ func (d *squareWebhookDedup) register(id string) bool { // (square_webhook_events) is the unbounded, restart-safe source of truth. var squareWebhookEventsSeen = newSquareWebhookDedup(500) +// webhookRetryFirstSeenMu guards webhookRetryFirstSeen. +var webhookRetryFirstSeenMu sync.Mutex + +// webhookRetryFirstSeen records the first delivery time of a retryable 503 +// response per event_id. Round-3 fix 2a/2b: a 503 (unknown COMPLETED payment, +// refund-before-row) makes Square retry for ~24h and then silently drop the +// event — this map lets the handler raise a critical admin notification once +// the retry budget is exhausted. Bounded like the dedup cache: entries are +// useful for at most 24h, so a capped sweep drops stale entries whenever the +// map overflows. Restart loses the map — conservative: the 24h clock restarts, +// so a notification may be missed but never falsely raised. The DB +// square_webhook_events table is deliberately NOT used because the existing +// 503 semantics promise NO dedup row on a rejected event. +var webhookRetryFirstSeen = make(map[string]time.Time) + +// webhookRetryExceededBudget records eventID's first retryable-503 delivery +// and reports whether it has been retrying for more than webhookRetryBudget. +// The first 503 just records the timestamp (returns false); a re-delivery +// whose first-seen is older than the budget returns true exactly once per +// event (the caller's notification is deduped by the event_id-derived id, so +// repeated true returns stay a single notification row). +func webhookRetryExceededBudget(eventID string) bool { + webhookRetryFirstSeenMu.Lock() + defer webhookRetryFirstSeenMu.Unlock() + now := clock.Now() + // Lazy eviction: only stale (>24h) entries are ever removed, so the map + // stays bounded to events still within the retry window. + if len(webhookRetryFirstSeen) > 500 { + for id, ts := range webhookRetryFirstSeen { + if now.Sub(ts) > 24*time.Hour { + delete(webhookRetryFirstSeen, id) + } + } + } + first, ok := webhookRetryFirstSeen[eventID] + if !ok { + webhookRetryFirstSeen[eventID] = now + return false + } + return now.Sub(first) > 24*time.Hour +} + // errWebhookParseFailure marks a dispatch error caused by a KNOWN money event // whose payload could not be parsed or extracted (as opposed to a DB failure). // HandleSquareWebhook distinguishes it from other dispatch errors to log the @@ -420,6 +462,17 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { } else { log.Printf("[SQUARE-WEBHOOK] Event %s (%s) dispatch failed: %v — NOT recording dedup row; Square will retry", event.Type, event.EventID, dispatchErr) } + // Round-3 fix 2a/2b: track the event's first-seen time so a retryable + // 503 that survives Square's ~24h retry budget raises a critical admin + // notification instead of being silently dropped. The map keeps the + // first delivery time per event_id; re-deliveries past 24h raise the + // notification (deduped by the event_id-derived notification id). + if webhookRetryExceededBudget(event.EventID) { + log.Printf("[SQUARE-WEBHOOK] CRITICAL: event %s (event_id=%s) has been retrying for over 24h — Square retry budget exhausted; raising admin notification", event.Type, event.EventID) + notifCtx, notifCancel := webhookDBContext() + insertUnknownEventNotification(notifCtx, event.Type, event.EventID) + notifCancel() + } http.Error(w, "webhook processing failed", http.StatusServiceUnavailable) return } @@ -1444,11 +1497,22 @@ func webhookApplyCompletedPaymentRecords(ctx context.Context, tx pgx.Tx, pr webh return } // Align the primary row to records[0] (deposit/balance/full portion). + // The align UPDATE clears the VAT fields exactly like the sweep rescue + // (sweep.go:976-987) and the live post-charge path (handlers.go:2934-2937). + // WITHOUT the clearing the pending row's VAT — computed at insert time on + // the FULL pre-split charge — survives the align, and the re-apply below + // is a silent no-op because apply_vat_to_payment is guarded on + // vat_amount IS NULL: the primary row keeps VAT on the wrong (larger) + // base. Clearing first makes the recompute effective on the split amount. if _, upErr := tx.Exec(ctx, ` UPDATE payments SET amount = $1, payment_type = $2, fees = $3, + is_vat_applicable = FALSE, + vat_rate = NULL, + vat_amount = NULL, + net_amount = NULL, updated_at = NOW() WHERE id = $4 `, records[0].Amount, records[0].PaymentType, records[0].Fees, pr.id); upErr != nil { diff --git a/backend/handlers/webhooks/webhooks_round8_test.go b/backend/handlers/webhooks/webhooks_round8_test.go index f894250..0ab5d55 100644 --- a/backend/handlers/webhooks/webhooks_round8_test.go +++ b/backend/handlers/webhooks/webhooks_round8_test.go @@ -25,7 +25,9 @@ import ( "encoding/json" "net/http" "testing" + "time" + "crussell/clock" "crussell/db" "crussell/testutils/fixtures" ) @@ -367,3 +369,225 @@ func TestWebhook_Round8_PaymentCompleted_BookinglessGiftCardRow_StaysPending(t * t.Errorf("expected 1 dedup row, got %d", n) } } + +// TestWebhook_Round8_VatClearingOnAlign locks the round-3 fix 1: the webhook's +// align UPDATE (webhookApplyCompletedPaymentRecords) must clear the VAT fields +// (is_vat_applicable, vat_rate, vat_amount, net_amount) before re-applying VAT +// on the split amount — exactly like the sweep rescue (sweep.go:976-987) and +// the live post-charge path (handlers.go:2934-2937). WITHOUT the clearing the +// pending row's VAT — computed at insert time on the FULL pre-split charge — +// survives the align, and the re-apply is a silent no-op because +// apply_vat_to_payment is guarded on vat_amount IS NULL: the primary row keeps +// VAT on the wrong (larger) base. +func TestWebhook_Round8_VatClearingOnAlign(t *testing.T) { + const squarePaymentID = "sqp_round8_vat_clear" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + attachWebhookTestBooking(t, payID, 50.00) + + // Enable VAT registration so ApplyVATToBookingPayment re-applies VAT + // after the align UPDATE clears the stale fields. + if _, err := db.Conn.Exec(context.Background(), + `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00`); err != nil { + t.Fatalf("failed to enable VAT registration: %v", err) + } + t.Cleanup(func() { + db.Conn.Exec(context.Background(), `UPDATE business_settings SET is_vat_registered = FALSE`) + }) + + // Set the payment amount to £45 (the charge that lands at Square) and + // seed stale VAT fields as if they were computed on the full pre-split + // charge (the bug: VAT on £50 instead of the split amount). + if _, err := db.Conn.Exec(context.Background(), + `UPDATE payments SET amount = 45.00, is_vat_applicable = TRUE, vat_rate = 20.00, vat_amount = 10.00, net_amount = 40.00, idempotency_key = 'round8-vat-clear-key' WHERE id = $1`, payID); err != nil { + t.Fatalf("failed to set payment amount and VAT fields: %v", err) + } + + event := SquareWebhookEvent{ + Type: "payment.completed", + EventID: "evt_round8_vat_clear_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "idempotency_key": "round8-vat-clear-key", + "amount_money": {"amount": 4500, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Fatalf("expected payment 'completed', got %q", got) + } + + // The align UPDATE must have cleared the stale VAT fields before the + // re-apply. After re-apply on the split amount (£25 deposit at 20% VAT + // = £4.17 VAT / £20.83 net), the fields should reflect the CORRECT + // split-base VAT — never the stale values from the full pre-split charge. + var isVatApplicable bool + var vatRate, vatAmount, netAmount *float64 + if err := db.Conn.QueryRow(context.Background(), + `SELECT is_vat_applicable, vat_rate, vat_amount, net_amount FROM payments WHERE id = $1`, payID, + ).Scan(&isVatApplicable, &vatRate, &vatAmount, &netAmount); err != nil { + t.Fatalf("failed to read payment VAT fields: %v", err) + } + if !isVatApplicable { + t.Error("expected is_vat_applicable to be TRUE after VAT re-apply on the split amount") + } + if vatRate == nil || *vatRate != 20.00 { + t.Errorf("expected vat_rate 20.00 after re-apply, got %v", vatRate) + } + if vatAmount == nil || *vatAmount != 4.17 { + t.Errorf("expected vat_amount 4.17 (20%% of £25 deposit), got %v", vatAmount) + } + if netAmount == nil || *netAmount != 20.83 { + t.Errorf("expected net_amount 20.83 (deposit split), got %v", netAmount) + } +} + +// TestWebhook_Round8_UnknownPayment_503_TimeoutNotification locks the round-3 +// fix 2a: when a COMPLETED payment.updated event matches no local row and no +// pending origin, the handler returns 503 so Square retries. If the event has +// been retrying for >24h (Square's retry budget is ~24h, after which the event +// is silently dropped), a critical admin notification must be raised so the +// operator knows about the dropped money event. +func TestWebhook_Round8_UnknownPayment_503_TimeoutNotification(t *testing.T) { + const ( + squarePaymentID = "sqp_round8_timeout_notify" + eventID = "evt_round8_timeout_notify_1" + ) + // Acknowledge any prior unacknowledged critical notifications so the + // assertion below is scoped to this test. + if _, err := db.Conn.Exec(context.Background(), + "UPDATE admin_notifications SET acknowledged_at = NOW() WHERE reason = 'critical_payment_log' AND acknowledged_at IS NULL"); err != nil { + t.Fatalf("failed to acknowledge prior critical notifications: %v", err) + } + before := countCriticalNotifications(t) + + // First delivery: unknown COMPLETED payment → 503, tracking row inserted. + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: eventID, + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "idempotency_key": "round8-timeout-never-used", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on first delivery (unknown payment), got %d: %s", w.Code, w.Body.String()) + } + // No notification yet — the event just started retrying. + if n := countCriticalNotifications(t) - before; n != 0 { + t.Errorf("expected 0 new notifications on first delivery, got %d", n) + } + + // Age the in-memory first-seen time past the 24h threshold so the next + // delivery triggers the timeout notification. + webhookRetryFirstSeenMu.Lock() + webhookRetryFirstSeen[eventID] = clock.Now().Add(-25 * time.Hour) + webhookRetryFirstSeenMu.Unlock() + + // Second delivery: same event, now >24h old → 503 + notification. + w2 := deliverWebhook(t, event) + if w2.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on second delivery (still unknown), got %d: %s", w2.Code, w2.Body.String()) + } + // A critical notification must have been raised. + if n := countCriticalNotifications(t) - before; n != 1 { + t.Errorf("expected exactly 1 new critical notification after 24h timeout, got %d", n) + } + + // Third delivery: re-delivery must NOT add a second notification (the + // deterministic event_id-based dedup in insertUnknownEventNotification + // keeps it to one row). + w3 := deliverWebhook(t, event) + if w3.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on third delivery, got %d: %s", w3.Code, w3.Body.String()) + } + if n := countCriticalNotifications(t) - before; n != 1 { + t.Errorf("expected notification count to stay at 1 after re-delivery, got %d", n) + } +} + +// TestWebhook_Round8_RefundBeforeRow_503_TimeoutNotification locks the round-3 +// fix 2b: when a refund.updated APPROVED arrives before the local refund row +// exists, the handler returns 503 so Square retries. If the event has been +// retrying for >24h, a critical admin notification must be raised. +func TestWebhook_Round8_RefundBeforeRow_503_TimeoutNotification(t *testing.T) { + const ( + squareRefundID = "sqr_round8_refund_timeout" + eventID = "evt_round8_refund_timeout_1" + ) + // Acknowledge any prior unacknowledged critical notifications. + if _, err := db.Conn.Exec(context.Background(), + "UPDATE admin_notifications SET acknowledged_at = NOW() WHERE reason = 'critical_payment_log' AND acknowledged_at IS NULL"); err != nil { + t.Fatalf("failed to acknowledge prior critical notifications: %v", err) + } + before := countCriticalNotifications(t) + + // First delivery: APPROVED refund before row exists → 503, tracking row inserted. + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: eventID, + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "APPROVED", + "payment_id": "sqp_round8_refund_timeout_pay" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on first delivery (refund before row), got %d: %s", w.Code, w.Body.String()) + } + // No notification yet. + if n := countCriticalNotifications(t) - before; n != 0 { + t.Errorf("expected 0 new notifications on first delivery, got %d", n) + } + + // Age the in-memory first-seen time past the 24h threshold. + webhookRetryFirstSeenMu.Lock() + webhookRetryFirstSeen[eventID] = clock.Now().Add(-25 * time.Hour) + webhookRetryFirstSeenMu.Unlock() + + // Second delivery: same event, now >24h old → 503 + notification. + w2 := deliverWebhook(t, event) + if w2.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on second delivery (still before row), got %d: %s", w2.Code, w2.Body.String()) + } + if n := countCriticalNotifications(t) - before; n != 1 { + t.Errorf("expected exactly 1 new critical notification after 24h timeout, got %d", n) + } + + // Third delivery: must not add a second notification. + w3 := deliverWebhook(t, event) + if w3.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on third delivery, got %d: %s", w3.Code, w3.Body.String()) + } + if n := countCriticalNotifications(t) - before; n != 1 { + t.Errorf("expected notification count to stay at 1 after re-delivery, got %d", n) + } +} diff --git a/backend/internal/jobs/cleanup.go b/backend/internal/jobs/cleanup.go index a8be7d8..27801fe 100644 --- a/backend/internal/jobs/cleanup.go +++ b/backend/internal/jobs/cleanup.go @@ -593,6 +593,12 @@ func RetryPendingSquareErasures(ctx context.Context) (int, error) { return len(drained), nil } +// maxS3DeletionRetries is the cap on retry attempts for a pending S3 deletion +// outbox row. After this many consecutive failures the row is marked as +// final-failed and a critical admin notification is raised — the operator must +// investigate and delete the object manually. +const maxS3DeletionRetries = 10 + // RetryPendingS3Deletions is the durable safety net for the account-deletion // S3/R2 profile-picture outbox (Fault A2). DeleteAccountHandler persists the // deletion target (bucket + object key) inside its anonymization transaction, @@ -601,8 +607,10 @@ func RetryPendingSquareErasures(ctx context.Context) (int, error) { // the object store indefinitely — this job finds the pending rows and retries // the deletion so the erasure is eventually complete. On success it deletes // the outbox row; on failure it leaves the row in place (bumping attempts and -// recording the error) so the next run retries again. Returns the number of -// outbox rows drained. +// recording the error) so the next run retries again. After maxS3DeletionRetries +// consecutive failures the row is marked as final-failed and a critical admin +// notification is raised — the operator must investigate and delete the object +// manually. Returns the number of outbox rows drained. func RetryPendingS3Deletions(ctx context.Context) (int, error) { if s3.Client == nil { // No object store configured: no deletion is possible, and the handler @@ -612,24 +620,26 @@ func RetryPendingS3Deletions(ctx context.Context) (int, error) { client := s3.Client rows, err := db.Conn.Query(ctx, ` - SELECT id, bucket, object_key + SELECT id, bucket, object_key, attempts FROM pending_s3_deletions + WHERE attempts < $1 ORDER BY created_at - `) + `, maxS3DeletionRetries) if err != nil { return 0, fmt.Errorf("failed to query pending S3 deletions: %w", err) } defer rows.Close() type pendingDelete struct { - id string - bucket string - key string + id string + bucket string + key string + attempts int } var pending []pendingDelete for rows.Next() { var p pendingDelete - if err := rows.Scan(&p.id, &p.bucket, &p.key); err != nil { + if err := rows.Scan(&p.id, &p.bucket, &p.key, &p.attempts); err != nil { return 0, fmt.Errorf("failed to scan pending S3 deletion: %w", err) } pending = append(pending, p) @@ -647,13 +657,25 @@ func RetryPendingS3Deletions(ctx context.Context) (int, error) { err := client.Delete(actx, p.bucket, p.key) cancel() if err != nil { + // Truncate the error to 200 chars to prevent unbounded growth. + errMsg := err.Error() + if len(errMsg) > 200 { + errMsg = errMsg[:200] + } + newAttempts := p.attempts + 1 if _, uerr := db.Conn.Exec(ctx, ` - UPDATE pending_s3_deletions SET attempts = attempts + 1, last_error = $2 + UPDATE pending_s3_deletions SET attempts = $2, last_error = $3 WHERE id = $1 - `, p.id, err.Error()); uerr != nil { + `, p.id, newAttempts, errMsg); uerr != nil { return drained, fmt.Errorf("failed to record S3 deletion retry failure %s: %w", p.id, uerr) } log.Printf("Warning: retry-s3-deletions failed to delete profile picture %s (outbox %s): %v", p.key, p.id, err) + // After maxS3DeletionRetries consecutive failures, raise a critical + // admin notification and stop retrying — the operator must investigate. + if newAttempts >= maxS3DeletionRetries { + user.InsertSquareErasureCriticalNotification(ctx, "s3:"+p.id) + log.Printf("CRITICAL: retry-s3-deletions exhausted %d attempts for outbox %s (key %s) — raising admin notification; operator must delete the object manually", maxS3DeletionRetries, p.id, p.key) + } continue } if _, err := db.Conn.Exec(ctx, `DELETE FROM pending_s3_deletions WHERE id = $1`, p.id); err != nil { diff --git a/backend/internal/jobs/cleanup_test.go b/backend/internal/jobs/cleanup_test.go index cc099d0..dab5055 100644 --- a/backend/internal/jobs/cleanup_test.go +++ b/backend/internal/jobs/cleanup_test.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "io" "os" "sync" "testing" @@ -14,6 +15,7 @@ import ( "crussell/db" "crussell/handlers/payments" "crussell/internal/adminnotify" + "crussell/internal/s3" "crussell/internal/square" "crussell/testutils" "crussell/testutils/testdb" @@ -680,3 +682,138 @@ func TestRetryPendingSquareErasures_KeepsSharedCustomerReferencedByActiveCard(t t.Errorf("expected active row to keep its customer reference, got %v", id) } } + +// ============================================================ +// RetryPendingS3Deletions — S3/R2 outbox job (FIX 2) +// ============================================================ + +// failingS3Uploader always fails Delete with an over-long error so tests can +// verify the retry cap and last_error truncation. +type failingS3Uploader struct{} + +func (failingS3Uploader) Upload(ctx context.Context, bucket, key string, body io.Reader, contentType string) error { + return nil +} +func (failingS3Uploader) Download(ctx context.Context, bucket, key string, w io.Writer) error { + return nil +} +func (failingS3Uploader) Delete(ctx context.Context, bucket, key string) error { + return fmt.Errorf("simulated persistent S3 deletion failure: this error message is deliberately much longer than two hundred characters so the truncation bound in RetryPendingS3Deletions must cut it off; otherwise the last_error column grows without bound across every hourly retry run") +} +func (failingS3Uploader) GetURL(ctx context.Context, bucket, key string) (string, error) { + return "", nil +} +func (failingS3Uploader) HealthCheck(ctx context.Context) error { + return nil +} + +// TestRetryPendingS3Deletions_AttemptCapRaisesNotification verifies FIX 2: a +// pending S3 deletion outbox row that fails maxS3DeletionRetries consecutive +// times stops being retried (the job's query filters attempts < cap), bumps +// attempts to the cap, truncates last_error to 200 chars, and raises the +// deduped critical admin notification so the operator investigates. +func TestRetryPendingS3Deletions_AttemptCapRaisesNotification(t *testing.T) { + ctx := context.Background() + var rowID string + if err := db.Conn.QueryRow(ctx, ` + INSERT INTO pending_s3_deletions (user_id, bucket, object_key, attempts, last_error) + VALUES (NULL, 'test-bucket', 'profiles/test.jpg', 9, 'previous error') + RETURNING id + `).Scan(&rowID); err != nil { + t.Fatalf("failed to seed pending S3 deletion: %v", err) + } + t.Cleanup(func() { + _, _ = db.Conn.Exec(ctx, "DELETE FROM pending_s3_deletions WHERE id = $1", rowID) + _, _ = db.Conn.Exec(ctx, "DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'") + }) + + origClient := s3.Client + s3.Client = failingS3Uploader{} + t.Cleanup(func() { s3.Client = origClient }) + + n, err := RetryPendingS3Deletions(ctx) + if err != nil { + t.Fatalf("RetryPendingS3Deletions failed: %v", err) + } + if n != 0 { + t.Errorf("expected 0 drained rows on persistent failure, got %d", n) + } + + var attempts int + var lastError string + if err := db.Conn.QueryRow(ctx, ` + SELECT attempts, last_error FROM pending_s3_deletions WHERE id = $1 + `, rowID).Scan(&attempts, &lastError); err != nil { + t.Fatalf("failed to query outbox row: %v", err) + } + if attempts != maxS3DeletionRetries { + t.Errorf("expected attempts capped at %d, got %d", maxS3DeletionRetries, attempts) + } + if len(lastError) > 200 { + t.Errorf("expected last_error truncated to 200 chars, got %d", len(lastError)) + } + + // The critical notification must have been raised (deduped by id). + var notifCount int + if err := db.Conn.QueryRow(ctx, ` + SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' + `).Scan(¬ifCount); err != nil { + t.Fatalf("failed to count critical notifications: %v", err) + } + if notifCount != 1 { + t.Errorf("expected 1 critical notification after reaching the cap, got %d", notifCount) + } + + // No more retries: a second run must not pick the row up (attempts < cap + // filters it out), so attempts stays at the cap. + n, err = RetryPendingS3Deletions(ctx) + if err != nil { + t.Fatalf("second RetryPendingS3Deletions failed: %v", err) + } + if n != 0 { + t.Errorf("expected 0 rows retried once at the cap, got %d", n) + } + var after int + if err := db.Conn.QueryRow(ctx, `SELECT attempts FROM pending_s3_deletions WHERE id = $1`, rowID).Scan(&after); err != nil { + t.Fatalf("failed to query attempts after second run: %v", err) + } + if after != maxS3DeletionRetries { + t.Errorf("expected attempts unchanged at %d after the cap, got %d", maxS3DeletionRetries, after) + } +} + +// TestRetryPendingS3Deletions_NoOpWithoutClient verifies the job is a no-op when +// no object store client is configured — no outbox rows are read or modified. +func TestRetryPendingS3Deletions_NoOpWithoutClient(t *testing.T) { + ctx := context.Background() + var rowID string + if err := db.Conn.QueryRow(ctx, ` + INSERT INTO pending_s3_deletions (user_id, bucket, object_key) + VALUES (NULL, 'test-bucket', 'profiles/test.jpg') + RETURNING id + `).Scan(&rowID); err != nil { + t.Fatalf("failed to seed pending S3 deletion: %v", err) + } + t.Cleanup(func() { + _, _ = db.Conn.Exec(ctx, "DELETE FROM pending_s3_deletions WHERE id = $1", rowID) + }) + + origClient := s3.Client + s3.Client = nil + t.Cleanup(func() { s3.Client = origClient }) + + n, err := RetryPendingS3Deletions(ctx) + if err != nil { + t.Fatalf("RetryPendingS3Deletions failed: %v", err) + } + if n != 0 { + t.Errorf("expected 0 drained rows without a client, got %d", n) + } + var attempts int + if err := db.Conn.QueryRow(ctx, `SELECT attempts FROM pending_s3_deletions WHERE id = $1`, rowID).Scan(&attempts); err != nil { + t.Fatalf("failed to query outbox row: %v", err) + } + if attempts != 0 { + t.Errorf("expected outbox row untouched without a client, got attempts=%d", attempts) + } +} diff --git a/backend/internal/s3/s3.go b/backend/internal/s3/s3.go index fdb471f..150392c 100644 --- a/backend/internal/s3/s3.go +++ b/backend/internal/s3/s3.go @@ -17,6 +17,13 @@ var Client Uploader // uses the in-memory fallback client. var FallbackToInMemory bool +// ClientIsStub reports whether the active client is the production stub that +// cannot perform real S3 operations. It is always true in the !dev build: +// every operation (Upload, Download, Delete) returns "not implemented" because +// the AWS SDK v2 is not compiled in. main.go uses it to warn at startup so the +// operator knows that profile-picture deletion will never succeed in this build. +var ClientIsStub = true + type Uploader interface { Upload(ctx context.Context, bucket, key string, body io.Reader, contentType string) error Download(ctx context.Context, bucket, key string, w io.Writer) error diff --git a/backend/internal/s3/s3_dev.go b/backend/internal/s3/s3_dev.go index 87f6ec7..1386225 100644 --- a/backend/internal/s3/s3_dev.go +++ b/backend/internal/s3/s3_dev.go @@ -26,6 +26,11 @@ var Client Uploader // main.go can surface it from the health endpoint in both build variants. var FallbackToInMemory bool +// ClientIsStub mirrors the prod-build flag so main.go can reference it in dev +// builds. It is always false here: the dev build compiles the real AWS SDK +// client and is never the "not implemented" stub. +var ClientIsStub = false + type Uploader interface { Upload(ctx context.Context, bucket, key string, body io.Reader, contentType string) error Download(ctx context.Context, bucket, key string, w io.Writer) error diff --git a/backend/internal/s3/s3_test.go b/backend/internal/s3/s3_test.go new file mode 100644 index 0000000..6766003 --- /dev/null +++ b/backend/internal/s3/s3_test.go @@ -0,0 +1,34 @@ +//go:build !dev + +package s3 + +import ( + "context" + "strings" + "testing" +) + +// TestProdClientIsStub verifies the production (!dev) build's S3 client is +// flagged as the stub: main.go uses ClientIsStub to warn at startup that +// object-store operations (including profile-picture deletion) cannot perform +// real work in this build. +func TestProdClientIsStub(t *testing.T) { + if !ClientIsStub { + t.Error("expected ClientIsStub to be true in the !dev build (the client is the 'not implemented' stub)") + } +} + +// TestProdStubDeleteNotImplemented verifies the prod stub's Delete always fails +// with a "not implemented" error — the retry-s3-deletions job cannot drain any +// outbox row in this build, so the attempt cap + critical notification is the +// only signal the operator gets. +func TestProdStubDeleteNotImplemented(t *testing.T) { + c := &S3Client{bucket: "b", endpoint: "http://localhost:9000"} + err := c.Delete(context.Background(), "test-bucket", "profiles/x.jpg") + if err == nil { + t.Fatal("expected the prod stub Delete to return an error") + } + if !strings.Contains(err.Error(), "not implemented") { + t.Errorf("expected the prod stub Delete error to say 'not implemented', got: %v", err) + } +} diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index 283830d..ec9b765 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -92,7 +92,7 @@ func SquareLocationID() string { } func newHTTPClient() *httpClient { - env := SquareEnvironment() + env := strings.ToLower(strings.TrimSpace(SquareEnvironment())) baseURL := squareSandboxURL // Empty/unknown SQUARE_ENVIRONMENT is production-ENFORCED, mirroring // payments.IsExplicitDevOrMockEnv()'s fail-closed interpretation (an diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index cc80d31..f791ab1 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -2170,6 +2170,11 @@ func TestNewHTTPClient_BaseURLSelection(t *testing.T) { {name: "sandbox_env", env: "sandbox", want: squareSandboxURL}, {name: "mock_env", env: "mock", want: squareSandboxURL}, {name: "dev_env", env: "dev", want: squareSandboxURL}, + // FIX 5: env normalization — lowercased + trimmed before comparison. + {name: "uppercase_production_is_production", env: "PRODUCTION", want: squareProductionURL}, + {name: "trailing_space_production_is_production", env: "Production ", want: squareProductionURL}, + {name: "uppercase_sandbox_is_sandbox", env: "SANDBOX", want: squareSandboxURL}, + {name: "surrounded_space_mock_is_sandbox", env: " Mock ", want: squareSandboxURL}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/backend/main.go b/backend/main.go index 77a4f92..1f9b63f 100644 --- a/backend/main.go +++ b/backend/main.go @@ -346,6 +346,34 @@ func checkSquareCredentials() { square.ValidateCredentials() } +// checkS3ProfilePicsBucket warns at startup when S3_PROFILE_PICS_BUCKET is +// unset in a non-dev/mock deployment. The bucket is only needed for profile-pic +// deletion operations, not for the app to function, so this is a WARNING not a +// Fatal. If R2_ENDPOINT is set (S3 client configured) but the bucket is not, +// the warning is elevated to CRITICAL because the deletion path will be silently +// skipped at runtime — every profile-pic erasure will log a one-time CRITICAL +// and skip the S3 deletion, leaving the object in place. +func checkS3ProfilePicsBucket() { + if payments.IsExplicitDevOrMockEnv() { + return + } + bucket := os.Getenv("S3_PROFILE_PICS_BUCKET") + r2Endpoint := os.Getenv("R2_ENDPOINT") + switch { + case bucket != "": + // The deletion path can run, but a non-dev build's S3 client is the + // "not implemented" stub — every Delete fails and the retry job will + // eventually hit its attempt cap and raise a critical notification. + if s3.ClientIsStub { + log.Printf("CRITICAL: this build's S3 client is the stub that cannot perform real operations — profile-picture deletions will always fail (retry-s3-deletions will exhaust its attempts and raise a critical admin notification). Compile with the real AWS SDK (dev build) or implement the production S3 client before relying on R2 object-store erasure.") + } + case r2Endpoint != "": + log.Printf("CRITICAL: S3_PROFILE_PICS_BUCKET is not set but R2_ENDPOINT=%q is configured — the S3 client is active but profile-picture deletion will be silently skipped at runtime (every erasure logs a one-time CRITICAL and skips the S3 delete). Set S3_PROFILE_PICS_BUCKET to the bucket holding profile pictures.", r2Endpoint) + default: + log.Printf("WARNING: S3_PROFILE_PICS_BUCKET is not set — profile-picture deletion from object storage will be skipped. This is safe if no object store is configured; set it when R2_ENDPOINT is configured.") + } +} + func healthCheckHandler(w http.ResponseWriter, r *http.Request) { status := "ok" services := map[string]string{ @@ -463,6 +491,7 @@ func main() { initDB() initDav() initS3() + checkS3ProfilePicsBucket() initSquare() sched := jobs.New() @@ -656,7 +685,15 @@ func main() { r.Get("/user/profile", user.GetProfileHandler) r.Put("/user/profile", user.UpdateProfileHandler) - r.Put("/user/change-password", user.ChangePasswordHandler) + // Change-password and delete-account get a dedicated per-user limiter + // (10/min) on top of the group's generic 120/min per-IP limiter: + // these endpoints re-verify the current password, and without a + // per-user budget a stolen session token lets an attacker brute-force + // that password with unlimited guesses (the account lockout is the + // last line of defence; the per-user limiter is the first). + accountLimiter := mw.RateLimitByUser(10, time.Minute) + r.With(accountLimiter).Put("/user/change-password", user.ChangePasswordHandler) + r.With(accountLimiter).Delete("/user/account", user.DeleteAccountHandler) r.Get("/user/notification-preferences", user.GetNotificationPreferencesHandler) r.Put("/user/notification-preferences", user.UpdateNotificationPreferencesHandler) // 2FA settings — merchant-level authorization gate on saved-card diff --git a/backend/startup_checks_test.go b/backend/startup_checks_test.go index c0c3850..4379c3d 100644 --- a/backend/startup_checks_test.go +++ b/backend/startup_checks_test.go @@ -11,6 +11,8 @@ import ( "strings" "testing" + "crussell/internal/s3" + "github.com/stretchr/testify/require" ) @@ -177,3 +179,54 @@ func TestCheckWebhookSignatureKey_FatalBranch_Exits(t *testing.T) { }) } } + +// TestCheckS3ProfilePicsBucket pins the S3_PROFILE_PICS_BUCKET startup check +// (FIX 3): a dev/mock env skips it, a configured bucket is silent, an unset +// bucket without R2_ENDPOINT warns, and an unset bucket WITH a configured +// R2_ENDPOINT (active S3 client, silently-skipped deletions) elevates to +// CRITICAL. +func TestCheckS3ProfilePicsBucket(t *testing.T) { + t.Run("mock_env_skips_check", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "mock") + t.Setenv("S3_PROFILE_PICS_BUCKET", "") + t.Setenv("R2_ENDPOINT", "") + got := captureLog(t, checkS3ProfilePicsBucket) + require.Empty(t, got, "a dev/mock env must skip the check: %s", got) + }) + + t.Run("bucket_configured_silent", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + t.Setenv("S3_PROFILE_PICS_BUCKET", "crussell-profile-pics") + t.Setenv("R2_ENDPOINT", "https://r2.example.com") + got := captureLog(t, checkS3ProfilePicsBucket) + require.Empty(t, got, "a configured bucket must be silent: %s", got) + }) + + t.Run("unset_bucket_without_endpoint_warns", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + t.Setenv("S3_PROFILE_PICS_BUCKET", "") + t.Setenv("R2_ENDPOINT", "") + got := captureLog(t, checkS3ProfilePicsBucket) + require.Contains(t, got, "WARNING", "an unset bucket without R2_ENDPOINT must warn: %s", got) + require.NotContains(t, got, "CRITICAL", "an unset bucket without R2_ENDPOINT must not be CRITICAL: %s", got) + }) + + t.Run("unset_bucket_with_endpoint_critical", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + t.Setenv("S3_PROFILE_PICS_BUCKET", "") + t.Setenv("R2_ENDPOINT", "https://r2.example.com") + got := captureLog(t, checkS3ProfilePicsBucket) + require.Contains(t, got, "CRITICAL", "an unset bucket with an active R2_ENDPOINT must elevate to CRITICAL: %s", got) + }) + + t.Run("stub_client_warns_even_with_bucket", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + t.Setenv("S3_PROFILE_PICS_BUCKET", "crussell-profile-pics") + t.Setenv("R2_ENDPOINT", "https://r2.example.com") + orig := s3.ClientIsStub + s3.ClientIsStub = true + defer func() { s3.ClientIsStub = orig }() + got := captureLog(t, checkS3ProfilePicsBucket) + require.Contains(t, got, "CRITICAL", "a stub client must warn even when the bucket is set: %s", got) + }) +} diff --git a/frontend/src/lib/components/admin/EditBookingModal.svelte b/frontend/src/lib/components/admin/EditBookingModal.svelte index df97eea..e8b3d90 100644 --- a/frontend/src/lib/components/admin/EditBookingModal.svelte +++ b/frontend/src/lib/components/admin/EditBookingModal.svelte @@ -348,7 +348,9 @@ async function openRefundModal(payment: Payment) { refundPaymentId = payment.id; - refundAmount = (payment.amount / 100).toFixed(2); + // payment.amount is in POUNDS (float64) — no /100 here. Pre-fill with + // the full amount; the payment-summary residual below overrides it. + refundAmount = payment.amount.toFixed(2); refundAlreadyRefundedPence = 0; refundReason = ''; // Unique per refund attempt so two equal partial refunds of the same @@ -375,8 +377,11 @@ .filter((r) => r.payment_id === payment.id && r.status === 'completed') .reduce((sum, r) => sum + r.amount, 0); if (alreadyRefunded > 0) { + // `alreadyRefunded` is PENCE (payment-summary refund rows) while + // payment.amount is POUNDS — convert to pounds before subtracting + // so a partially refunded payment pre-fills the correct residual. refundAlreadyRefundedPence = alreadyRefunded; - refundAmount = (Math.max(0, payment.amount - alreadyRefunded) / 100).toFixed(2); + refundAmount = Math.max(0, payment.amount - alreadyRefunded / 100).toFixed(2); } } } catch { diff --git a/frontend/src/lib/components/payments/TwoFactorCodeInput.svelte b/frontend/src/lib/components/payments/TwoFactorCodeInput.svelte index e795bf2..9905323 100644 --- a/frontend/src/lib/components/payments/TwoFactorCodeInput.svelte +++ b/frontend/src/lib/components/payments/TwoFactorCodeInput.svelte @@ -41,13 +41,13 @@ class="mt-1 font-mono tracking-widest" />

- Your bank doesn't support in-app approval — enter the code sent to you / your phone. + Your card doesn't support in-app approval — please try a different card or payment method.

{:else}

- Two-factor authentication is required to use online card payments. + Secure card verification is required for this payment method. Enable it in your account settings. diff --git a/frontend/src/lib/components/today/CurrentAppointment.svelte b/frontend/src/lib/components/today/CurrentAppointment.svelte index 994f85a..cfe0568 100644 --- a/frontend/src/lib/components/today/CurrentAppointment.svelte +++ b/frontend/src/lib/components/today/CurrentAppointment.svelte @@ -877,8 +877,16 @@ {#if showPaymentModal && activeAppointment} + {@const firstName = activeAppointment.user?.full_name?.split(' ')[0] || ''} + {@const lastName = activeAppointment.user?.full_name?.split(' ').slice(1).join(' ') || ''} + {@const bookingWithNames = { + ...activeAppointment, + user: activeAppointment.user + ? { ...activeAppointment.user, first_name: firstName, last_name: lastName } + : undefined + }} (showPaymentModal = false)} onComplete={handlePaymentComplete} /> diff --git a/frontend/src/lib/square/square.test.ts b/frontend/src/lib/square/square.test.ts index fe63395..1803a90 100644 --- a/frontend/src/lib/square/square.test.ts +++ b/frontend/src/lib/square/square.test.ts @@ -240,35 +240,44 @@ describe('buildCashTillPaymentBody', () => { }); describe('cashChargeBasePence', () => { - // Concrete arithmetic pinned to the FIX-1 scenario: booking £100, pending - // 10% campaign preview (£10), £10 loyalty redemption, £100 cash tender with - // the keep-change-as-tip checkbox on. netTotal = £90 (campaign subtracted), - // so the current charge base of £85 (totalDue − loyalty) understates the - // backend's remaining basis and absorbs the tip. - it('restores the pending campaign credit into the charge base (tip carve basis)', () => { - // totalDuePence = £90 net (campaign subtracted, pre-loyalty) - expect(cashChargeBasePence(9000, 1000, 1000)).toBe(9000); + // FIX-1/FIX-2: The charge base is the FULL totalDuePence — neither the + // campaign credit nor the loyalty discount is subtracted. The backend + // applies both at completion via separate discount rows, and the tip carve + // (`amount − remaining`) uses GetBookingRemainingBalancePence which does + // NOT account for pending discounts. Subtracting them would undercharge + // the booking and absorb the tip into booking credit. + it('charges the full total due — campaign and loyalty are applied server-side', () => { + // Booking £100 net (campaign already subtracted), £10 campaign credit, + // £10 loyalty redemption → charge base = £100 (full totalDuePence) + expect(cashChargeBasePence(10000, 1000, 1000)).toBe(10000); }); - it('keeps the plain net total when no campaign is eligible', () => { - // totalDue = £90 (no campaign), loyalty £10 → base = £80 = the net obligation - expect(cashChargeBasePence(9000, 0, 1000)).toBe(8000); + it('ignores campaignPence and loyaltyPence — always returns totalDuePence', () => { + // totalDue = £90 (no campaign), loyalty £10 → base = £90, not £80 + expect(cashChargeBasePence(9000, 0, 1000)).toBe(9000); }); - it('never goes below zero (fully covered by discounts + loyalty)', () => { - expect(cashChargeBasePence(1000, 0, 5000)).toBe(0); + it('never goes below zero', () => { + expect(cashChargeBasePence(0, 0, 0)).toBe(0); + expect(cashChargeBasePence(-100, 0, 0)).toBe(0); }); it('the folded tip body uses the base, so UI tip == backend-recorded tip', () => { - // Booking £100, campaign £10, loyalty £10: base = 9000 (100 − 20 + 10). - // Tender £100 → tip £10 → amount £100. The backend carves against the - // full £100 remaining, so it records £0 tip — matching the UI claim - // that only the amount above the charge base is a tip. - const basePence = cashChargeBasePence(9000, 1000, 1000); - const tipPence = 10000 - basePence; - expect(buildCashTillPaymentBody(basePence, tipPence).amount).toBe(10000); + // Booking £100 net, campaign £10, loyalty £10: base = 10000. + // Tender £110 → tip £10 → amount £110. The backend carves against + // the full £100 remaining (no discount deduction), so it records + // £10 tip — matching the UI claim. + const basePence = cashChargeBasePence(10000, 1000, 1000); + const tipPence = 11000 - basePence; + expect(buildCashTillPaymentBody(basePence, tipPence).amount).toBe(11000); expect(tipPence).toBe(1000); }); + + it('campaignPence and loyaltyPence are accepted but ignored (call-site compat)', () => { + // The parameters exist for call-site compatibility — the calling + // modals still compute them for display. The arithmetic ignores them. + expect(cashChargeBasePence(5000, 9999, 9999)).toBe(5000); + }); }); describe('payment failure classification', () => { diff --git a/frontend/src/lib/square/square.ts b/frontend/src/lib/square/square.ts index 3fe7e18..f409764 100644 --- a/frontend/src/lib/square/square.ts +++ b/frontend/src/lib/square/square.ts @@ -62,27 +62,30 @@ export function buildCashTillPaymentBody( } /** - * The cash till-sale charge base in pence, aligned with the backend's - * CreateTerminalPayment tip carve (backend/handlers/payments/handlers.go - * ~577-630). The backend derives the recorded tip from `amount − remaining`, + * The cash till-sale charge base in pence. The backend's + * CreateTerminalPayment derives the recorded tip from `amount − remaining`, * where `remaining` comes from GetBookingRemainingBalancePence (service.go) — - * which does NOT subtract the pending campaign discount: campaigns auto-apply - * AFTER the remaining is read, minting `payment_method='discount'` rows that - * never reduce the balance. If the frontend charges the campaign-reduced total - * (`totalDuePence`), the sent amount is smaller than the backend's remaining, - * so `amount − remaining` is absorbed (the tip is under-recorded or the whole - * payment lands as booking credit). Restoring the campaign credit into the - * charge base keeps the sent amount on the same basis the backend carves - * against. `totalDuePence` is the net total (campaign already subtracted, - * pre-loyalty), `campaignPence` the eligible preview credit and `loyaltyPence` - * the redemption being applied on top. + * which does NOT subtract the pending campaign discount or loyalty discount + * (both mint `payment_method='discount'` rows that never reduce the balance). + * The charge base must therefore be the FULL totalDuePence — neither the + * campaign credit nor the loyalty discount is subtracted, because the backend + * applies both at completion via separate discount rows. The campaign and + * loyalty parameters are kept for call-site compatibility (the calling modals + * still compute them for display) but are deliberately unused in the + * arithmetic: subtracting them would undercharge the booking and cause the + * backend's tip carve (`amount − remaining`) to absorb the tip or record a + * negative tip. + * + * FIX-1/FIX-2: `campaignPence` and `loyaltyPence` are accepted but IGNORED. + * The correct charge base is the full total due, with campaign and loyalty + * applied server-side at completion. */ export function cashChargeBasePence( totalDuePence: number, - campaignPence: number, - loyaltyPence: number + _campaignPence: number, + _loyaltyPence: number ): number { - return Math.max(0, totalDuePence - loyaltyPence + campaignPence); + return Math.max(0, totalDuePence); } /** diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index ad3bc2c..f6600e2 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -6,25 +6,20 @@ import { toast } from 'svelte-sonner'; import CardBrandIcon from '$lib/components/payments/CardBrandIcon.svelte'; import CardEntryUnavailable from '$lib/components/payments/CardEntryUnavailable.svelte'; - import CardSelection from '$lib/components/payments/CardSelection.svelte'; - import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; - import TwoFactorCodeInput from '$lib/components/payments/TwoFactorCodeInput.svelte'; - import ScaFallbackConsentDialog from '$lib/components/payments/ScaFallbackConsentDialog.svelte'; - import { - CARD_VERIFICATION_RETRY_MESSAGE, - canSaveCardsForRole, - isNonceStale, - isSquareConfigured, - isTwoFactorVerificationGateFailure, - isVerificationRequiredSignal, - requestNewTwoFactorCode, - runSavedCardSCAProactively, - shouldShowSCARefusal, - submitPaymentWithRetry, - VERIFICATION_REQUIRED_MESSAGE - } from '$lib/square/square'; - import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; - import { generateUUID } from '$lib/utils/uuid'; +import CardSelection from '$lib/components/payments/CardSelection.svelte'; +import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; +import { + CARD_VERIFICATION_RETRY_MESSAGE, + canSaveCardsForRole, + isNonceStale, + isSquareConfigured, + isVerificationRequiredSignal, + requestNewTwoFactorCode, + runSavedCardSCAProactively, + submitPaymentWithRetry, + VERIFICATION_REQUIRED_MESSAGE +} from '$lib/square/square'; +import { generateUUID } from '$lib/utils/uuid'; import { extractErrorMessage, sanitizeText } from '$lib/utils/toast-safe'; import { apiFetch } from '$lib/utils/api'; import UserBookingModal from '$lib/components/account/UserBookingModal.svelte'; @@ -286,8 +281,6 @@ // verification code must be carried on the charge. Shared two-factor-code // state (code, reveal, show/missing derivations, "Request a new code" // handler) — see $lib/stores/twoFactorCode.svelte.ts. - const buyTwoFactorEnabled = $derived(!!authStore.currentUser?.twoFactorEnabled); - const buySavedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); // Outcome of the last saved-card SCA attempt: 'sca-unavailable' drives the // C6 refusal notice (SCA is the ONLY authorisation — there is no 2FA // fallback); every other outcome keeps SCA primary for the next retry. @@ -298,13 +291,6 @@ // Retryable purchase failure message shown above the Pay button (challenge // cancelled/failed, decline) so the retry affordance matches the outcome. let buyError = $state(null); - const buyTwoFactor = useTwoFactorCodeForSavedCard({ - enabled: () => buyTwoFactorEnabled, - gateActive: () => buySavedCardChargeRequires2FACode && (buySelectedCard !== '' || buySaveCard), - // C6 SCA-only posture: SCA is ALWAYS the authorisation — the code input - // only ever surfaces via a backend gate rejection (defensive/opt-in). - scaAvailable: () => true - }); // Client-side mirror of the £500/day online purchase cap. The backend is // authoritative — the balance endpoint the page already calls exposes the @@ -560,8 +546,6 @@ // BEFORE any charge is submitted and surface the refusal // notice — there is NO 2FA fallback; the gift card is // bought online later. - buyTwoFactor.declineConsent(); - buyTwoFactor.reveal = false; buyingGiftCard = false; buyError = null; return; @@ -606,15 +590,9 @@ save_card: buySaveCard && !verificationToken } : {}), - ...(buyTwoFactor.showInput && !verificationToken - ? { verification_code: buyTwoFactor.code } - : {}), idempotency_key: buyIdempotencyKey }) - }), - // Finding 4: a 2FA-gated charge consumed its code at the backend - // gate — a 503 auto-retry would re-send a dead code and self-defeat. - { verificationCodeGated: buyTwoFactor.showInput && !verificationToken } + }) ); if (res.ok) { @@ -632,8 +610,6 @@ buyTokenAmount = 0; buyTokenizedAt = 0; buyTokenizedForSaveCard = false; - buyTwoFactor.setCode(''); - buyTwoFactor.reveal = false; await fetchGiftCardBalance(); } else { // Capture the status BEFORE consuming the body — the @@ -652,15 +628,10 @@ const buyErrMsg = verificationRequired ? VERIFICATION_REQUIRED_MESSAGE : extractErrorMessage(errText) || 'Failed to purchase gift card'; - if (isTwoFactorVerificationGateFailure(status, buyErrMsg)) { - buyTwoFactor.reveal = true; - } if (verificationRequired) { // M13: a verification-required 402 means the backend did NOT // accept the fallback code (SCA-only posture / invalid token) - // — withdraw consent so the code input never reappears and - // the SCA guidance is shown instead of looping on 2FA. - buyTwoFactor.declineConsent(); + // — the SCA guidance is shown instead of looping on 2FA. } buyError = buyErrMsg; toast.error(buyErrMsg); @@ -1618,10 +1589,10 @@ toast.error('Please type DELETE to confirm'); return; } - if (deleteCurrentPassword === '') { - toast.error('Enter your current password to confirm'); - return; - } + // Passwordless accounts (social login) have no password to confirm — + // the password field is optional; the backend re-verifies credentials + // server-side (and its 2FA gate governs passwordless deletion). + const suppliedPassword = deleteCurrentPassword.trim(); const twoFactorActive = deleteTwoFactorRequired || deleteRevealTwoFactor; if (twoFactorActive && deleteVerificationCode.trim() === '') { toast.error('Enter your verification code to confirm'); @@ -1637,7 +1608,7 @@ method: 'DELETE', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ - current_password: deleteCurrentPassword, + ...(suppliedPassword ? { current_password: suppliedPassword } : {}), ...(twoFactorActive ? { verification_code: deleteVerificationCode.trim() } : {}) }) }); @@ -2815,36 +2786,7 @@ bind:selectedCardId={buySelectedCard} bind:saveCard={buySaveCard} onValidityChange={(v) => (buyCardSelectionValid = v)} - /> - - { - buyLastSCAOutcome = ''; - buyError = null; - }} - /> - - - {#if buyTwoFactor.showInput && buyTwoFactorEnabled} - - {/if} -

+ {#if buyWaitingForSCA}
@@ -2881,7 +2823,6 @@ onclick={buyGiftCard} disabled={buyingGiftCard || !isBuyCardValid || - buyTwoFactor.missing || (buyRecipientType === 'self' && !buySelfAck) || buyDailyTotal + buyAmount > dailyGiftCardBuyLimit} class="mt-2 min-h-11 w-full" @@ -3641,6 +3582,9 @@ autocomplete="current-password" class="mt-2" /> +

+ Only required if your account has a password. +

{#if deleteTwoFactorRequired || deleteRevealTwoFactor}
@@ -3690,7 +3634,6 @@ disabled={ deletingAccount || deleteConfirmText !== 'DELETE' || - deleteCurrentPassword === '' || ((deleteTwoFactorRequired || deleteRevealTwoFactor) && deleteVerificationCode.trim() === '') }