From 2cdbad0cea4f0b2e7049e174dc382e247825374e Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Sun, 16 Aug 2026 00:15:45 +0100 Subject: [PATCH] =?UTF-8?q?feat:=20SCA-only=20saved-card=20charges=20?= =?UTF-8?q?=E2=80=94=202FA=20charge=20fallback=20removed=20(C6),=20version?= =?UTF-8?q?ed=20consent=20fields,=20token=20provenance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PSR 2017 reg 100 makes SCA mandatory and non-waivable for customer-initiated stored-credential charges; a merchant-side 2FA check cannot legally substitute for it (authorising a token-less charge via 2FA leaves the MERCHANT liable for ECI 7 / SLI 210 chargebacks and reg 77(6) compensation regardless of consent). - payments/twofa.go: the homegrown 2FA fallback for token-less saved-card charges is REMOVED ENTIRELY. requireTwoFactorForCardAccess is now SCA-only: a non-empty Square verification_token (charge surfaces, token forwarded to Square) skips the gate; anything else is refused 402 verification_required. enforceSCAFallbackConsent is a compile-compatible no-op (fallback never runs). - New requireTwoFactorForCardAccessWithTokenValidation distinguishes surfaces where the token IS forwarded to Square (charge — Square validates it) from card-SAVE surfaces (token client-asserted, never forwarded: a non-empty token must NOT skip the save gate, auth-F1). - SCA tokenize-result wire contract (C1): a saved card charged with a fresh one-time tokenize-result sends the token as the charge SOURCE (new_card_token -> source_id) alongside saved_card_id, never a separate verification_token. resolveChargeSource resolves the saved-card branch FIRST (customer from the card row, token as source) so combined token+card requests are SCA-clean. - C6 consent fields (consent_version / consent_accepted) added to the booking/ tip/till/gift-card charge requests, enforced server-side before any fallback charge could reach Square and recorded on the 2fa_fallback_charge audit row; logVerificationTokenProvenance traces minted tokens to their charge. - user 2FA issuance gate refactored into pure build-agnostic functions (twoFAPepperConfigured / twoFADeliveryChannelConfigured / twoFAEnsureIssueAllowedStrict) shared with the payments re-issue path and exercised directly by the test,dev suite; TWO_FACTOR_FALLBACK switch and .env.example entry removed; startup posture notes updated. - Test coverage: fail-closed 2FA production gates (pepper/delivery), token validation on save vs charge surfaces, completion idempotency, idempotency key determinism, refund-policy 72h/24h epsilon boundaries, VAT parity. --- .env.example | 38 +- backend/handlers/payments/charge_helpers.go | 76 +- .../payments/completion_idempotency_test.go | 247 +++++++ backend/handlers/payments/completion_test.go | 85 +++ backend/handlers/payments/errors.go | 6 +- backend/handlers/payments/handlers.go | 289 ++++++-- backend/handlers/payments/handlers_test.go | 492 +++++++++++++ .../handlers/payments/idempotency_helpers.go | 19 + .../payments/idempotency_helpers_test.go | 126 ++++ .../payments/refund_policy_boundary_test.go | 91 +++ .../handlers/payments/terminal_sca_test.go | 93 +-- backend/handlers/payments/twofa.go | 314 ++++----- .../handlers/payments/twofa_delivery_dev.go | 15 + .../handlers/payments/twofa_delivery_prod.go | 44 +- .../payments/twofa_failclosed_test.go | 60 ++ .../payments/twofa_gate_consume_test.go | 156 +++-- backend/handlers/payments/twofa_test.go | 649 ++++++++++-------- backend/handlers/payments/vat_test.go | 39 ++ backend/handlers/user/twofa.go | 48 ++ .../handlers/user/twofa_failclosed_test.go | 67 ++ backend/handlers/user/twofa_prod.go | 42 +- backend/handlers/user/twofa_prod_test.go | 79 +++ .../user/twofa_prod_variant_dev_test.go | 11 + .../user/twofa_prod_variant_prod_test.go | 8 + backend/main.go | 36 +- 25 files changed, 2356 insertions(+), 774 deletions(-) create mode 100644 backend/handlers/payments/completion_idempotency_test.go create mode 100644 backend/handlers/payments/completion_test.go create mode 100644 backend/handlers/payments/handlers_test.go create mode 100644 backend/handlers/payments/idempotency_helpers_test.go create mode 100644 backend/handlers/payments/refund_policy_boundary_test.go create mode 100644 backend/handlers/payments/twofa_failclosed_test.go create mode 100644 backend/handlers/user/twofa_failclosed_test.go create mode 100644 backend/handlers/user/twofa_prod_test.go create mode 100644 backend/handlers/user/twofa_prod_variant_dev_test.go create mode 100644 backend/handlers/user/twofa_prod_variant_prod_test.go diff --git a/.env.example b/.env.example index 18e455a..a304549 100644 --- a/.env.example +++ b/.env.example @@ -55,19 +55,25 @@ SQUARE_ENVIRONMENT=mock # deliberately route a dev build to the real production API. Never set in a # deployed production build. SQUARE_ALLOW_REAL_API= -# 2FA — backup merchant-level authorization gate on saved-card online payments. -# NOT PSD2 SCA: Square buyer verification (3-D Secure / SCA) is the PRIMARY -# authorisation for saved-card charges and is wired for both new-card and -# saved-card charges. The 2FA gate fires only when SCA is unavailable (e.g. a -# customer's bank does not support the in-app approval flow) and every such -# fallback charge is audited (admin_audit_log 2fa_fallback_charge). Enforcement -# is FAIL-CLOSED: +# 2FA — homegrown two-factor authentication for ADMIN/ACCOUNT VERIFICATION ONLY +# (setup, disable, delete-account re-auth). It is NEVER a card-charge +# authorisation. Saved-card online payments are authorised EXCLUSIVELY by +# Square PSD2 SCA (3-D Secure / buyer verification), wired for both new-card +# and saved-card charges. PSR 2017 reg 100 makes SCA mandatory and non-waivable +# for customer-initiated stored-credential charges, so a saved-card charge +# carrying no Square verification token is REFUSED 402 verification_required +# (the payment does not go through; the customer can try again later, or at +# the till they can be invited to pay online later instead) — the homegrown +# 2FA fallback was removed entirely and no TWO_FACTOR_FALLBACK switch exists. +# Enforcement is FAIL-CLOSED: # ON unless REQUIRE_2FA explicitly disables it (false/0/off/no, case-insensitive) # OR SQUARE_ENVIRONMENT explicitly equals one of mock/dev/development/test. # Empty or unknown SQUARE_ENVIRONMENT values are treated as production-enforced # (a mistyped env var can never silently disarm the gate; the backend logs a # startup warning in that case). Set REQUIRE_2FA=false only in controlled -# environments. +# environments. The dev mock simulates SCA +# (SimulateSavedCardVerificationRequired + cnon:sca-... tokenize-results), so +# development has full parity with the SCA-only production posture. # Code delivery: the intended channel is email/SMS (the method chosen at # setup) — NOT wired yet. Until it lands, the verification code is delivered # via the server log (a [2FA]-prefixed line) when the operator opts into @@ -75,16 +81,6 @@ SQUARE_ALLOW_REAL_API= # environments an operator must relay the logged code to the user out-of-band; # the API never returns the code while enforcement is ON. REQUIRE_2FA=true -# TWO_FACTOR_FALLBACK: whether the homegrown 2FA code gate may be used as a -# BACKUP authorisation for saved-card charges when Square SCA (3-D Secure / -# buyer verification) is unavailable — e.g. a customer's bank does not support -# in-app approval. Defaults true. Square 3DS2 SCA is the PRIMARY authorisation; -# when a saved-card charge carries a Square verification_token the 2FA gate is -# skipped entirely. Set TWO_FACTOR_FALLBACK=false for a security-first posture -# in which a saved-card charge without SCA cannot proceed via 2FA (the frontend -# surfaces the SCA challenge; if the bank can't complete it, the charge fails). -# 2FA fallback success always writes an admin_audit_log 2fa_fallback_charge row. -TWO_FACTOR_FALLBACK=true # TWO_FACTOR_PEPPER — server-side pepper for HMAC-hashing 2FA codes. REQUIRED # in production builds: code issuance FAILS CLOSED when it is unset (an # unsalted SHA-256 digest in the 1M code space would be offline-brute-forceable @@ -97,7 +93,7 @@ TWO_FACTOR_PEPPER= # FAILS CLOSED without a delivery channel: there is no email/SMS transport yet, # so the ONLY production channel is the operator's explicit opt-in to the # insecure server-log delivery ([2FA] prefix — anyone with backend log access -# can defeat the gate on saved-card charges). MUST be set to true to deliver +# can defeat the account 2FA gate). MUST be set to true to deliver # 2FA codes via the server log in production until email/SMS lands. Dev/test # builds always deliver via the log and never consult this flag. TWO_FACTOR_ALLOW_LOG_DELIVERY=false @@ -137,6 +133,10 @@ TEST_DB_DSN= GO_TESTING= # CardDAV (SabreDAV) — profile photo sync +# DAV_BASE_URL is retained for reference only: the Go backend no longer reads it +# — updateCardDAV writes through the dav.Service directly (Postgres-backed +# CardDAV store), it never calls this URL. Only the sabredav PHP container needs +# the server-side credential below. DAV_BASE_URL=http://localhost:8080 # DAV_ADMIN_PASSWORD — REQUIRED, FAIL-CLOSED. sabredav/server.php refuses to # start when unset or set to a known weak/default value ('admin' etc.) — this diff --git a/backend/handlers/payments/charge_helpers.go b/backend/handlers/payments/charge_helpers.go index 65c9b76..bb6012b 100644 --- a/backend/handlers/payments/charge_helpers.go +++ b/backend/handlers/payments/charge_helpers.go @@ -43,6 +43,52 @@ import ( // On any error the helper writes the HTTP response and returns ok=false — the // caller must return immediately. func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *PaymentService, userID string, newCardToken, cardID *string, saveCard bool, notFoundMsg string) (sourceID string, savedCardID *string, squareCustomerID string, ok bool) { + // The saved-card branch is resolved FIRST so a request carrying BOTH a + // new-card token and a saved card id — the SCA tokenize-result wire + // contract (Square's CURRENT "Charge a Card on File" flow, where + // card.tokenize(verificationDetails, cardId) returns a token that must be + // sent as source_id, NOT a separate verification_token) — resolves the + // customer from the saved-card row and uses the fresh one-time token as + // the source. Without the token it is a plain saved-card (ccof:) charge. + // Only a token with NO saved card falls through to the new-card path below. + if cardID != nil { + card, err := svc.GetCardByID(ctx, *cardID, userID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + http.Error(w, notFoundMsg, http.StatusNotFound) + return "", nil, "", false + } + log.Printf("Failed to get card: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return "", nil, "", false + } + if card.SquareCustomerID == "" { + if userID == "" { + // Defensive parity with the original saved-card block: a card + // with no bookable owner cannot be provisioned. Unreachable in + // practice — GetCardByID above filters on user_id and would + // have 404'd for an empty owner. + http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest) + return "", nil, "", false + } + provisioned, provErr := svc.EnsureSquareCustomerForSavedCard(ctx, *cardID, userID) + if provErr != nil { + log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *cardID, userID, provErr) + http.Error(w, "Failed to process card", http.StatusInternalServerError) + return "", nil, "", false + } + card.SquareCustomerID = provisioned + } + if newCardToken != nil && *newCardToken != "" { + // SCA tokenize-result for a saved card: the token is the charge + // source (a fresh one-time token minted only after the issuer + // completed buyer verification for THIS card + amount); the stored + // ccof: id is NOT sent. customer_id still derives from the saved + // card row — Square requires it for the card-on-file charge. + return *newCardToken, cardID, card.SquareCustomerID, true + } + return card.SquareCardID, cardID, card.SquareCustomerID, true + } if newCardToken != nil && *newCardToken != "" { if saveCard { sqCustomerID, custErr := svc.EnsureSquareCustomer(ctx, userID) @@ -96,36 +142,6 @@ func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *Paymen } return sourceID, savedCardID, squareCustomerID, true } - if cardID != nil { - card, err := svc.GetCardByID(ctx, *cardID, userID) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - http.Error(w, notFoundMsg, http.StatusNotFound) - return "", nil, "", false - } - log.Printf("Failed to get card: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return "", nil, "", false - } - if card.SquareCustomerID == "" { - if userID == "" { - // Defensive parity with the original saved-card block: a card - // with no bookable owner cannot be provisioned. Unreachable in - // practice — GetCardByID above filters on user_id and would - // have 404'd for an empty owner. - http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest) - return "", nil, "", false - } - provisioned, provErr := svc.EnsureSquareCustomerForSavedCard(ctx, *cardID, userID) - if provErr != nil { - log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *cardID, userID, provErr) - http.Error(w, "Failed to process card", http.StatusInternalServerError) - return "", nil, "", false - } - card.SquareCustomerID = provisioned - } - return card.SquareCardID, cardID, card.SquareCustomerID, true - } // Neither a new-card token nor a saved card — validation upstream // (ValidateCardInfo) guarantees one of them is present. return "", nil, "", false diff --git a/backend/handlers/payments/completion_idempotency_test.go b/backend/handlers/payments/completion_idempotency_test.go new file mode 100644 index 0000000..5f2d56f --- /dev/null +++ b/backend/handlers/payments/completion_idempotency_test.go @@ -0,0 +1,247 @@ +//go:build test && dev + +package payments + +// M15 webhook/sweep completion-asymmetry tests. A Square payment.updated +// webhook (handlers/webhooks/square.go handlePaymentUpdated) and the stale +// pending-payment sweep's rescue path can both try to complete the same +// payment row: the webhook flips `payments.status` with a +// `WHERE ... AND status = 'pending'` guard, and the sweep's rescue +// (rescueStaleRowCompletedTx) flips the row with its own +// `WHERE id = ... AND status = 'pending'` guard before applying the split / +// VAT / fully-paid-booking completion side effects. Both guards make the +// completion idempotent — the first writer wins, the second matches zero rows +// and applies NO side effects. These tests lock that invariant in both race +// orderings. Sequential (no t.Parallel): they swap the package-global +// SquareClient and mutate the shared pool, like the other sweep tests. + +import ( + "context" + "database/sql" + "testing" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" +) + +// TestSweepRescueThenWebhook_CompletesExactlyOnce locks the sweep-first race +// ordering: the sweep rescues a stale pending payment to 'completed' and +// applies the booking-completion side effects exactly once; a webhook-style +// idempotent status flip (the exact `WHERE ... AND status = 'pending'` UPDATE +// handlePaymentUpdated runs) that arrives AFTER the rescue matches zero rows, +// and a re-run of the sweep also does nothing — the side effects are never +// doubled. +func TestSweepRescueThenWebhook_CompletesExactlyOnce(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, serviceID := setupTestData(t, ctx, tx) + // Give the booking a payable total so the rescue's fully-paid check can + // complete it. + if _, err := tx.Exec(ctx, "UPDATE bookings SET total_amount = 2000.00 WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking total: %v", err) + } + // VAT-registered — the rescue applies VAT to the split records. + if _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00, voucher_type = 'SPV'`); err != nil { + t.Fatalf("failed to enable VAT in business_settings: %v", err) + } + + payID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create pending payment: %v", err) + } + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '25 hours' WHERE id = $1", payID); err != nil { + t.Fatalf("failed to age the payment: %v", err) + } + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + pay, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 200000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "seed-asymmetry-completed", + }) + if err != nil { + t.Fatalf("failed to seed completed Square payment: %v", err) + } + if _, err := tx.Exec(ctx, "UPDATE payments SET square_payment_id = $1 WHERE id = $2", pay.SquarePayID, payID); err != nil { + t.Fatalf("failed to set square_payment_id: %v", err) + } + SquareClient = mock + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + _, _ = db.Conn.Exec(pool, `UPDATE business_settings SET is_vat_registered = FALSE, voucher_type = 'SPV'`) + }) + + // 1. The sweep rescues the stale pending row (Square reports COMPLETED). + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var payStatus string + if err := db.Conn.QueryRow(pool, "SELECT status FROM payments WHERE id = $1", payID).Scan(&payStatus); err != nil { + t.Fatalf("failed to query payment: %v", err) + } + if payStatus != "completed" { + t.Fatalf("expected the sweep to rescue the payment to 'completed', got %q", payStatus) + } + + // The rescue must have completed the booking (fully paid) exactly once. + var bookingStatus string + if err := db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus); err != nil { + t.Fatalf("failed to query booking: %v", err) + } + if bookingStatus != "completed" { + t.Errorf("expected the sweep rescue to complete the fully-paid booking, got %q", bookingStatus) + } + + // Exactly the deposit + balance split records exist — no duplicates. + var recordCount int + if err := db.Conn.QueryRow(pool, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&recordCount); err != nil { + t.Fatalf("failed to count payment records: %v", err) + } + if recordCount != 2 { + t.Errorf("expected exactly 2 split payment records after the rescue, got %d", recordCount) + } + + // VAT applied on both split records. + var vatRows int + if err := db.Conn.QueryRow(pool, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND vat_amount IS NOT NULL", bookingID).Scan(&vatRows); err != nil { + t.Fatalf("failed to count VAT'd records: %v", err) + } + if vatRows != 2 { + t.Errorf("expected VAT applied to both split records exactly once, got %d records with VAT", vatRows) + } + + // 2. The webhook's completion path arrives AFTER the rescue: its pending- + // only UPDATE matches zero rows (idempotent status flip). + tag, err := db.Conn.Exec(pool, ` + UPDATE payments SET status = 'completed', updated_at = NOW() + WHERE square_payment_id = $1 AND status = 'pending' + `, pay.SquarePayID) + if err != nil { + t.Fatalf("webhook-style update failed: %v", err) + } + if int(tag.RowsAffected()) != 0 { + t.Errorf("expected the post-rescue webhook completion to match 0 pending rows, got %d", tag.RowsAffected()) + } + + // 3. A re-run of the sweep finds nothing pending — no second rescue, no + // second completion, no duplicate records. + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("second sweep failed: %v", err) + } + if err := db.Conn.QueryRow(pool, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&recordCount); err != nil { + t.Fatalf("failed to re-count payment records: %v", err) + } + if recordCount != 2 { + t.Errorf("expected the second sweep to add no payment records, got %d", recordCount) + } + if err := db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus); err != nil { + t.Fatalf("failed to re-query booking: %v", err) + } + if bookingStatus != "completed" { + t.Errorf("expected the booking to stay completed after the second sweep, got %q", bookingStatus) + } + var stampAwarded sql.NullTime + if err := db.Conn.QueryRow(pool, "SELECT loyalty_stamp_awarded_at FROM bookings WHERE id = $1", bookingID).Scan(&stampAwarded); err != nil { + t.Fatalf("failed to query loyalty stamp marker: %v", err) + } + if !stampAwarded.Valid { + t.Error("expected the loyalty stamp awarded exactly once by the single completion") + } +} + +// TestWebhookThenSweepRescue_CompletesExactlyOnce locks the webhook-first race +// ordering: the webhook's pending-only status flip completes the payment +// first, so the sweep finds no pending row left to rescue and applies NO +// side effects — the booking is not double-completed and no split/VAT records +// are minted by the sweep. +func TestWebhookThenSweepRescue_CompletesExactlyOnce(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, serviceID := setupTestData(t, ctx, tx) + if _, err := tx.Exec(ctx, "UPDATE bookings SET total_amount = 2000.00 WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking total: %v", err) + } + + payID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create pending payment: %v", err) + } + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '25 hours', square_payment_id = 'sqp_webhook_first' WHERE id = $1", payID); err != nil { + t.Fatalf("failed to age the payment: %v", err) + } + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + // 1. The webhook flips the pending row to completed first. + tag, err := db.Conn.Exec(pool, ` + UPDATE payments SET status = 'completed', updated_at = NOW() + WHERE square_payment_id = 'sqp_webhook_first' AND status = 'pending' + `) + if err != nil { + t.Fatalf("webhook-style update failed: %v", err) + } + if int(tag.RowsAffected()) != 1 { + t.Fatalf("expected the webhook-style completion to match exactly 1 pending row, got %d", tag.RowsAffected()) + } + + // 2. The sweep rescue path arrives after: the row is no longer pending, so + // it is never fetched and NO side effects run. + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var recordCount int + if err := db.Conn.QueryRow(pool, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&recordCount); err != nil { + t.Fatalf("failed to count payment records: %v", err) + } + // Exactly ONE row — the webhook only flipped status; the sweep's split + // logic must not have run (the rescue is gated on the row still pending). + if recordCount != 1 { + t.Errorf("expected the sweep to add no records after the webhook completed the row, got %d", recordCount) + } + + var bookingStatus string + if err := db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus); err != nil { + t.Fatalf("failed to query booking: %v", err) + } + if bookingStatus != "in_progress" { + t.Errorf("expected the booking untouched by the sweep (no double-completion), got %q", bookingStatus) + } +} diff --git a/backend/handlers/payments/completion_test.go b/backend/handlers/payments/completion_test.go new file mode 100644 index 0000000..6f0b76e --- /dev/null +++ b/backend/handlers/payments/completion_test.go @@ -0,0 +1,85 @@ +//go:build test && dev + +package payments + +import ( + "context" + "testing" + + "crussell/db" + "crussell/testutils" + "crussell/testutils/fixtures" +) + +// TestCompleteActiveBookingFromPayment_RefusesCancelledBooking locks the M2 +// money-safety boundary of the sweep rescue's completion side-effect: a +// cancelled / lapsed / no-show booking must NEVER be auto-completed by the +// payment path — a charge landing on such a booking is failed + auto-refunded +// by the sweep's F3 gate, and completing the booking would record money against +// a booking the cancellation flow already closed. +func TestCompleteActiveBookingFromPayment_RefusesCancelledBooking(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + + if _, err := tx.Exec(ctx, "UPDATE bookings SET status = 'we_cancelled' WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking we_cancelled: %v", err) + } + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + completeActiveBookingFromPayment(ctx, pgxTx, bookingID) + + var status string + if err := tx.QueryRow(ctx, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&status); err != nil { + t.Fatalf("failed to query booking status: %v", err) + } + if status != "we_cancelled" { + t.Errorf("expected the cancelled booking NOT auto-completed, got %q", status) + } + // The setup tx is rolled back at test end, so no pool-level cleanup is needed. +} + +// TestCompleteFullyPaidBooking_CompletesPayableBooking locks the sweep rescue's +// completion side-effect (applyStaleRescueRecords → bookingIsFullyPaid → +// completeActiveBookingFromPayment): a PAYABLE booking fully covered by +// completed real money is completed by the rescue-completion path, exactly as +// the live payment path would. +func TestCompleteFullyPaidBooking_CompletesPayableBooking(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, serviceID := setupTestData(t, ctx, tx) + + // A full £50 completed payment covers the £50 booking total. + payID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed") + if err != nil { + t.Fatalf("failed to create completed payment: %v", err) + } + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit setup tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_discounts WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE id = $1`, payID) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + completeFullyPaidBooking(pool, bookingID) + + var status string + if err := db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&status); err != nil { + t.Fatalf("failed to query booking status: %v", err) + } + if status != "completed" { + t.Errorf("expected the fully-paid payable booking completed, got %q", status) + } +} diff --git a/backend/handlers/payments/errors.go b/backend/handlers/payments/errors.go index 83c301a..3b9be96 100644 --- a/backend/handlers/payments/errors.go +++ b/backend/handlers/payments/errors.go @@ -71,8 +71,10 @@ func isVerificationRequiredError(err error) bool { // flow (mirrors the overflow_tip_confirmation_required / campaign_fully_redeemed // structured-error pattern — mw.RespondJSON, code + human message). The message // tells the buyer to approve the payment in their banking app. Used both by the -// charge-failure paths (Square returned an SCA-required code) and by the 2FA -// gate when the SCA-only posture has no fallback for a token-less charge. +// charge-failure paths (Square returned an SCA-required code) and by the SCA-only +// saved-card gate, which refuses any token-less charge (the homegrown 2FA +// fallback was removed — a token-less charge is always refused here, never +// authorised by a 2FA code). func writeVerificationRequiredResponse(w http.ResponseWriter) { mw.RespondJSON(w, http.StatusPaymentRequired, map[string]string{ "error": "Your card issuer requires verification. Approve this payment in your banking app.", diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index de6c569..6d1f719 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -75,23 +75,54 @@ func InsertAdminAuditCharge(ctx context.Context, adminID, targetUserID, action s // charge must land an admin_audit_log row (action_type '2fa_fallback_charge', // details {sca_performed:false, fallback_reason:"verification_unavailable"}) so // the operator can distinguish SCA-authorized charges from fallback-authorized -// ones. Mirrors InsertAdminAuditCharge's best-effort, own-transaction, -// non-fatal failure handling (a failed audit write can never abort a completed -// charge). For customer-initiated online charges the actor (adminID) is the -// customer's own userID; for admin surfaces it is the admin from request -// context — the caller passes accordingly. cardLast4 and referenceID are filled -// by the caller at charge success (paymentResult.CardLast4, booking/till/payment -// id), where they are actually known. -func insertTwoFAFallbackAudit(ctx context.Context, adminID, userID, cardLast4, referenceID, notes string) { +// ones. Since C6 the row also captures the customer's versioned consent to the +// fallback (consent_version, consent_accepted, consent_versioned_at) — the +// exact values the server validated before the charge — so an operator/GDPR +// export can reconstruct which dialog version was shown, that it was accepted, +// when, and on which charge. Mirrors InsertAdminAuditCharge's best-effort, +// own-transaction, non-fatal failure handling (a failed audit write can never +// abort a completed charge). For customer-initiated online charges the actor +// (adminID) is the customer's own userID; for admin surfaces it is the admin +// from request context — the caller passes accordingly. cardLast4 and +// referenceID are filled by the caller at charge success +// (paymentResult.CardLast4, booking/till/payment id), where they are actually +// known. +func insertTwoFAFallbackAudit(ctx context.Context, adminID, userID, cardLast4, referenceID, notes string, consentVersion string, consentAccepted bool) { InsertAdminAuditCharge(ctx, adminID, userID, "2fa_fallback_charge", map[string]any{ - "sca_performed": false, - "fallback_reason": "verification_unavailable", - "card_last4": cardLast4, - "reference_id": referenceID, - "notes": notes, + "sca_performed": false, + "fallback_reason": "verification_unavailable", + "card_last4": cardLast4, + "reference_id": referenceID, + "notes": notes, + "consent_version": consentVersion, + "consent_accepted": consentAccepted, + "consent_versioned_at": clock.Now().Format(time.RFC3339), }) } +// logVerificationTokenProvenance records the charge context a legacy SCA +// verification_token arrived with (saved-card reference + booking) so an +// operator can correlate a minted token with the exact charge it authorized. +// The length-only ValidateVerificationToken is deliberately not extended: +// Square mints and validates these tokens server-side, binding them to the +// card + amount, and any stale/reused/mis-bound token is definitively rejected +// by Square with VERIFICATION_TOKEN_INVALID / CARD_DECLINED_VERIFICATION_REQUIRED +// (errors.go classifies those), so Square being the sole arbiter is acceptable +// — the charge fails closed on any mismatch. This log is the minimum provenance +// trace; the token is redacted to a prefix because it is a sensitive credential. +// A no-op for token-less charges (the SCA tokenize-result wire contract sends +// no verification_token at all). +func logVerificationTokenProvenance(flow, bookingID string, savedCardRef *string, token string) { + if token == "" { + return + } + ref := "(new-card)" + if savedCardRef != nil && *savedCardRef != "" { + ref = *savedCardRef + } + log.Printf("SCA verification_token present on %s charge for booking %s (saved card %s) — token %q forwarded to Square", flow, bookingID, ref, square.TokenPrefix(token)) +} + type CreateTerminalPaymentRequest struct { Amount int64 `json:"amount" validate:"required,gt=0"` PaymentType string `json:"payment_type" validate:"required"` @@ -122,12 +153,27 @@ type CreateTerminalPaymentRequest struct { // amount+card key for no-client-key retry safety. Cap ≤45 (Square's // idempotency-key limit for /v2/payments — this key feeds CreatePayment). IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` + // ConsentVersion / ConsentAccepted: the customer's explicit versioned + // consent to the SCA-unavailable → 2FA fallback (C6). The frontend sends + // consent_version:"v1" + consent_accepted:true (scaFallbackConsentFields) + // when the ScaFallbackConsentDialog was accepted; the server enforces it + // (403 consent_required) before a fallback charge can reach Square and + // records it on the 2fa_fallback_charge audit row. + ConsentVersion *string `json:"consent_version,omitempty"` + ConsentAccepted bool `json:"consent_accepted"` } type CreateBookingPaymentRequest struct { - Amount int64 `json:"amount" validate:"required,gt=0"` - PaymentType string `json:"payment_type" validate:"required"` - CardID *string `json:"card_id,omitempty"` + Amount int64 `json:"amount" validate:"required,gt=0"` + PaymentType string `json:"payment_type" validate:"required"` + CardID *string `json:"card_id,omitempty"` + // UserSavedCardID (saved_card_id) is the SCA path's reference to the stored + // saved card (user_saved_cards.id). It coexists with NewCardToken when the + // frontend sends the SCA tokenize-result — card.tokenize(verificationDetails, + // cardId) — as new_card_token: the tokenize-result token is a fresh + // one-time source_id and the saved-card row supplies the Square customer. + // Without a token it behaves exactly like card_id (legacy/2FA-fallback). + UserSavedCardID *string `json:"saved_card_id,omitempty"` NewCardToken *string `json:"new_card_token,omitempty"` SaveCard bool `json:"save_card"` IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` @@ -136,6 +182,11 @@ type CreateBookingPaymentRequest struct { // enforced environment charges a saved card only when this matches the // customer's pending code. VerificationCode string `json:"verification_code,omitempty"` + // ConsentVersion / ConsentAccepted: the customer's explicit versioned + // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side + // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. + ConsentVersion *string `json:"consent_version,omitempty"` + ConsentAccepted bool `json:"consent_accepted"` // ConfirmOverflowTip acknowledges that an overpayment beyond the booking's // remaining balance will be recorded as a tip (M7). Tips cannot be paid in // advance, so a pre-start overpayment is rejected with 400 @@ -167,6 +218,11 @@ type CreateTipPaymentRequest struct { // enforced environment charges a saved card only when this matches the // customer's pending code. VerificationCode string `json:"verification_code,omitempty"` + // ConsentVersion / ConsentAccepted: the customer's explicit versioned + // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side + // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. + ConsentVersion *string `json:"consent_version,omitempty"` + ConsentAccepted bool `json:"consent_accepted"` } type CheckoutResponse struct { @@ -491,6 +547,26 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // Route based on payment method if req.PaymentMethod != nil && (*req.PaymentMethod == "cash" || *req.PaymentMethod == "giftcard") { + // C2: serialize this till cash/giftcard charge under the SAME + // crussell:payment: advisory lock the online booking payment + // path (CreateBookingPayment) and the saved-card/terminal paths take. + // The bookings-row FOR UPDATE below only serializes against OTHER + // transactions that take the same row lock — the online path reads the + // remaining balance under the advisory lock with NO FOR UPDATE, so the + // two primitives do NOT serialize against each other: a concurrent + // online charge + till cash/giftcard charge could both pass their + // remaining-balance checks and both record money (the overflow carved + // into a non-refundable tip by buildSplitRecords). Holding the same + // advisory lock here makes every money-mutating path contend on one + // primitive; the FOR UPDATE stays as a harmless double-guard against + // concurrent cancellations. The bounded try-lock (R6) gives an + // in-flight online charge ~3s to finish, then fails this fast with 409. + pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again") + if !lockOK { + return + } + defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) + // Start transaction before the status and idempotency checks so they // are atomic with the payment insert. tx, err := db.Conn.Begin(r.Context()) @@ -888,6 +964,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { terminalVerificationToken := "" if req.VerificationToken != nil { terminalVerificationToken = *req.VerificationToken + logVerificationTokenProvenance("admin terminal saved-card", bookingID, req.UserSavedCardID, terminalVerificationToken) } // Resolve the saved-card Square source for the booking's user (the @@ -1070,6 +1147,12 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { if !gateOK { return } + // C6: a fallback-authorized charge must carry the customer's + // accepted consent (403 consent_required otherwise) — the server-side + // guard that stops a 2FA-path charge reaching Square without it. + if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + return + } } // B13: the pre-charge discount SET for the post-charge apply-time @@ -1312,7 +1395,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // The 2FA BACKUP authorized this token-less saved-card charge // (SCA was unavailable) — record the strict fallback audit row. if twoFAFallbackUsed { - insertTwoFAFallbackAudit(r.Context(), adminID, bookingUserID.String, paymentResult.CardLast4, bookingID, "admin saved-card charge authorized via 2FA fallback (SCA unavailable)") + insertTwoFAFallbackAudit(r.Context(), adminID, bookingUserID.String, paymentResult.CardLast4, bookingID, "admin saved-card charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) } } @@ -1953,6 +2036,19 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } + // savedCardRef is the effective saved-card reference for this request: + // the legacy card_id field OR the SCA path's saved_card_id (they are the + // same user_saved_cards.id; card_id wins when both are sent). 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. ValidateCardInfo above already rejected card_id + new_card_token + // together, so a coexistence can only be new_card_token + saved_card_id. + savedCardRef := req.CardID + if savedCardRef == nil || *savedCardRef == "" { + savedCardRef = req.UserSavedCardID + } + scaTokenizedSavedCard := req.NewCardToken != nil && *req.NewCardToken != "" && savedCardRef != nil && *savedCardRef != "" + // Serialize payment attempts for this booking to prevent concurrent payments // across browser tabs or duplicate requests. Uses a PostgreSQL session-level // advisory lock so that only one goroutine processes payment for a given @@ -1999,8 +2095,8 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // the dedup lookup below returns it (double-charge protection). if req.IdempotencyKey == "" { cardPart := "new" - if req.CardID != nil && *req.CardID != "" { - cardPart = *req.CardID + if savedCardRef != nil && *savedCardRef != "" { + cardPart = *savedCardRef } key, keyErr := deriveBookingPaymentIdempotencyKey(r.Context(), tx, bookingID, req.PaymentType, req.Amount, cardPart) if keyErr != nil { @@ -2189,21 +2285,39 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // re-rejected as "expired". Pending-reuse and fresh paths still gate — a // new charge may move at Square. The gate also runs before // resolveChargeSource below, so an un-2FA'd request never persists a card. - // SCA-primary: a request carrying a Square verification_token (SCA - // performed) skips the gate; a token-less card-save/charge falls back to - // the customer's 2FA code, and twoFAFallbackUsed records that the 2FA - // BACKUP authorized the operation (the caller audits the charge on success). + // SCA-primary (auth-F1): the SAVE surface never forwards the client's + // verification_token to Square (the card is persisted via CreateCardOnFile, + // which takes no token), so a non-empty token is client-asserted and must + // NOT skip the gate — a token-less save falls back to the customer's 2FA + // code, and twoFAFallbackUsed records that the 2FA BACKUP authorized the + // operation (the caller audits it). Only the call-site's + // scaTokenizedSavedCard tokenize-result flow skips the save gate (the + // combined path never persists a card and Square validates the token as the + // source_id). twoFAFallbackUsed := false bookingVerificationToken := "" if req.VerificationToken != nil { bookingVerificationToken = *req.VerificationToken + logVerificationTokenProvenance("booking", bookingID, savedCardRef, bookingVerificationToken) } - if req.SaveCard { + // scaTokenizedSavedCard (an SCA tokenize-result token charging a saved + // card) skips BOTH 2FA gates exactly like a present verification_token: the + // token only exists after the issuer completed buyer verification for this + // card + amount (SCA-primary), so no homegrown fallback authorization is + // needed. The SAVE gate is skipped because the combined path never persists + // a card (resolveChargeSource uses the token as a one-time source, no + // card-on-file is created). + if req.SaveCard && !scaTokenizedSavedCard { var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, bookingVerificationToken, true) + gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, req.VerificationCode, bookingVerificationToken, true, false) if !gateOK { return } + // C6: a fallback-authorized save/charge must carry the customer's + // accepted consent (403 consent_required otherwise). + if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + return + } } // After the idempotency check (which handles same-key retries), verify @@ -2490,17 +2604,26 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // carrying a Square verification_token (SCA performed) skips the gate; a // token-less charge falls back to 2FA and twoFAFallbackUsed is set for the // charge-success audit. - if req.CardID != nil && *req.CardID != "" { + if savedCardRef != nil && *savedCardRef != "" && !scaTokenizedSavedCard { var gateOK bool gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, bookingVerificationToken, !reusePendingRecord) if !gateOK { return } + // C6: a fallback-authorized charge must carry the customer's accepted + // consent (403 consent_required otherwise). + if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + return + } } // Resolve the new-card-vs-saved-card Square source (shared with // CreateTipPayment, BuyGiftCard, and the saved-card branch of // CreateTerminalPayment — see resolveChargeSource for the R6 rationale). - sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found") + // savedCardRef (card_id OR saved_card_id) is passed as the card reference; + // when an SCA tokenize-result token rides along in NewCardToken, + // resolveChargeSource uses the token as the source and the card row for the + // customer. + sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, savedCardRef, req.SaveCard, "Card not found") if !sourceOK { return } @@ -2622,7 +2745,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // A pending-reuse retry verified WITHOUT consuming, so its code is still // live and no re-issue runs (a re-issue would invalidate the code the // customer already holds). - if (req.CardID != nil && *req.CardID != "") || req.SaveCard { + if (savedCardRef != nil && *savedCardRef != "") || req.SaveCard { reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r) } // SCA-required failures must surface the structured verification_required @@ -2760,9 +2883,9 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // For a PENDING-REUSE retry (gate passed consume=false — the code was // re-issued for this retry) this is where it is burned, so a retry that // fails again keeps its code for one more attempt. Only runs for - // saved-card (CardID) charges — the gate only ran for those, and new-card + // saved-card charges — the gate only ran for those, and new-card // charges have no code to consume. - if req.CardID != nil && *req.CardID != "" && reusePendingRecord { + if savedCardRef != nil && *savedCardRef != "" && reusePendingRecord { if consErr := twofa.ConsumePendingCode(r.Context(), tx2, userID); consErr != nil { log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but consuming the 2FA code for user %s failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, userID, consErr) @@ -2850,7 +2973,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // completed charge). The actor is the customer's own userID // (customer-initiated online charge). if twoFAFallbackUsed { - insertTwoFAFallbackAudit(r.Context(), userID, userID, paymentResult.CardLast4, bookingID, "saved-card charge authorized via 2FA fallback (SCA unavailable)") + insertTwoFAFallbackAudit(r.Context(), userID, userID, paymentResult.CardLast4, bookingID, "saved-card charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) } // B13: a campaign was exhausted between the preview and the apply-time @@ -3343,6 +3466,21 @@ type CreatePaymentMethodRequest struct { // enforced environment persists a card only when this matches the // customer's pending code. VerificationCode string `json:"verification_code,omitempty"` + // VerificationToken is a Square 3DS/SCA verification token. On the add-card + // SAVE surface it is CLIENT-ASSERTED and never forwarded to Square + // (CreateCardOnFile takes no verification_token), so the 2FA gate IGNORES it + // (auth-F1 — a forged value cannot authorise a save; see twofa.go). It is + // carried on the wire for parity with the charge surfaces and passed through + // to the gate, whose save variant applies the token-less SCA-only refusal + // when a genuine SCA proof is absent. The genuine proof for a SAVE is the + // STORE-intent tokenize-result submitted as card_token (see + // isSCATokenizeResultCardToken). + VerificationToken *string `json:"verification_token,omitempty"` + // ConsentVersion / ConsentAccepted mirror the charge structs (C6): the + // add-card endpoint never charges, so they are recorded on the fallback + // audit row (when the client sends them) but not enforced here. + ConsentVersion *string `json:"consent_version,omitempty"` + ConsentAccepted bool `json:"consent_accepted"` } func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { @@ -3371,19 +3509,33 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { service := NewPaymentService() - // 2FA gating (H4): persisting a card via the account "add card" endpoint - // requires 2FA when the feature is enforced — the same gate the booking - // and tip flows apply to req.SaveCard. Persisting a stored credential is - // exactly what the PSD2 SCA stand-in protects, so the dedicated save-card - // endpoint must not be the un-gated side door. consume=true: saving a card - // is a terminal operation with no downstream charge to attach consumption - // to (MEDIUM-2). This request type carries no verification_token field, so - // the 2FA fallback is the only authorization path; a fallback success is - // recorded by the strict audit below. + // SCA-only compliance (M11): the add-card surface's SCA proof is the card + // token itself. The frontend performs the STORE-intent SCA at tokenization + // (SquareCardInput.tokenizeForStore) — the tokenize-result IS the card + // token, and Square validates it as a card-on-file source when + // CreateCardOnFile persists it. A save that carries a genuine SCA + // tokenize-result as card_token is therefore SCA-compliant and skips the + // 2FA gate at the call site — exactly like the charge surfaces' + // scaTokenizedSavedCard skip: the STORE-intent SCA performed at tokenization + // IS the verification (PSR 2017 reg 100 compliant), so no homegrown fallback + // authorization is needed and no fallback audit is written. + // + // Every other save stays gated through the save-surface variant + // (tokenForwardedToSquare=false): a verification_token is client-asserted + // and NEVER forwarded to Square on a SAVE surface, so it must NOT skip the + // gate (auth-F1 — a forged value cannot authorise a save, and no 2FA code + // can either, SCA-only). A token-less legacy save (a raw card.tokenize() + // nonce, or a token the backend cannot recognize as a genuine SCA + // tokenize-result) is refused 402 verification_required in an enforced + // deployment. + saveVerificationToken := "" + if req.VerificationToken != nil { + saveVerificationToken = *req.VerificationToken + } twoFAFallbackUsed := false - { + if !isSCATokenizeResultCardToken(req.CardToken) { var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, "", true) + gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, req.VerificationCode, saveVerificationToken, true, false) if !gateOK { return } @@ -3402,9 +3554,11 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { } // The 2FA BACKUP authorized persisting this card (no SCA was performed on - // the add-card endpoint); the actor is the customer's own userID. + // the add-card endpoint); the actor is the customer's own userID. Unreachable + // under SCA-only (the gate never sets fallbackUsed), retained for parity + // with the charge surfaces. if twoFAFallbackUsed && card != nil { - insertTwoFAFallbackAudit(r.Context(), userID, userID, card.Last4, "", "card persisted via 2FA fallback (SCA unavailable)") + insertTwoFAFallbackAudit(r.Context(), userID, userID, card.Last4, "", "card persisted via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) } if err := json.NewEncoder(w).Encode(card); err != nil { @@ -3412,6 +3566,20 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { } } +// isSCATokenizeResultCardToken reports whether a card token submitted to the +// add-card save surface is a GENUINE Square tokenizeWithVerification result — +// the STORE-intent tokenize-result the account page's tokenizeForStore +// produces (the SCA challenge runs at tokenization, so the returned token is +// the SCA-verified source the backend stores). Real Square returns opaque cnon: +// tokens, so the dev mock's contract marks a genuine tokenize-result with +// "sca-" immediately after the cnon: prefix (internal/square's +// isSCATokenizeResultSource) — the enforcement-parity stand-in the charge +// paths also rely on (money-F2). A raw card.tokenize() nonce or any other +// shape is NOT an SCA proof and never skips the gate. +func isSCATokenizeResultCardToken(cardToken string) bool { + return strings.HasPrefix(cardToken, "cnon:sca-") +} + // isDefinitiveCardSaveFailure reports whether a CreatePaymentMethod error is a // definitive client rejection — an expired/invalid/already-used card source or // a declined card that can never be saved — as opposed to an ambiguous @@ -3987,6 +4155,16 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + // MEDIUM-3a coverage: a manual refund that completes synchronously at + // Square (COMPLETED on the first attempt) records the SAME + // admin_audit_log row the sweep's re-issue writes — reuse the shared + // insertManualRefundAudit helper (refunds.go) so the row shape is + // byte-identical: action 'admin_refund', admin actor, payment id, pence + // amount and reason, best-effort own-tx non-fatal. Written AFTER the + // row is marked completed so a later sweep pass (which only processes + // still-pending rows) can never re-resolve this refund and duplicate + // the audit row. + insertManualRefundAudit(r.Context(), adminID, paymentID, req.Amount, req.Reason) } else { status = "pending" } @@ -4555,9 +4733,12 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // consume=true: saving a card is a terminal operation (the card row is // created right here), so the verified code is single-use immediately — // unlike the saved-card CHARGE gate below, which defers consumption to the - // charge's terminal success (MEDIUM-2). A request carrying a Square - // verification_token (SCA performed) skips the gate; a token-less save falls - // back to 2FA and twoFAFallbackUsed is set for the charge-success audit. + // charge's terminal success (MEDIUM-2). SCA-primary (auth-F1): the SAVE + // surface never forwards the client's verification_token to Square (the + // card is persisted via CreateCardOnFile, which takes no token), so a + // non-empty token is client-asserted and must NOT skip the gate — a + // token-less save falls back to 2FA and twoFAFallbackUsed is set for the + // charge-success audit. twoFAFallbackUsed := false tipVerificationToken := "" if req.VerificationToken != nil { @@ -4565,10 +4746,15 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { } if req.SaveCard { var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, tipVerificationToken, true) + gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, req.VerificationCode, tipVerificationToken, true, false) if !gateOK { return } + // C6: a fallback-authorized save/charge must carry the customer's + // accepted consent (403 consent_required otherwise). + if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + return + } } if err := ValidateAmount(req.Amount); err != nil { @@ -4814,6 +5000,11 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { if !gateOK { return } + // C6: a fallback-authorized charge must carry the customer's accepted + // consent (403 consent_required otherwise). + if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + return + } } if !reusePendingRecord { @@ -5000,7 +5191,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // unavailable) — record the strict fallback audit row. The actor is the // customer's own userID (customer-initiated online charge). if twoFAFallbackUsed { - insertTwoFAFallbackAudit(r.Context(), userID, userID, paymentResult.CardLast4, bookingID, "saved-card tip charge authorized via 2FA fallback (SCA unavailable)") + insertTwoFAFallbackAudit(r.Context(), userID, userID, paymentResult.CardLast4, bookingID, "saved-card tip charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) } if err := json.NewEncoder(w).Encode(PaymentResponse{ diff --git a/backend/handlers/payments/handlers_test.go b/backend/handlers/payments/handlers_test.go new file mode 100644 index 0000000..494ef70 --- /dev/null +++ b/backend/handlers/payments/handlers_test.go @@ -0,0 +1,492 @@ +//go:build test && dev + +package payments + +// Tests for the C2 till-vs-online advisory-lock serialization fix and the +// SCA tokenize-result wire contract (new_card_token + saved_card_id on +// CreateBookingPayment). +// +// These tests COMMIT their setup so the advisory locks work across independent +// pool connections (see cleanupConcurrentTestRows in concurrency_test.go for +// the leak-free deletion order). They swap the SquareClient global and run +// lock-bound waits (~3s each), so they are sequential — never t.Parallel. + +import ( + "context" + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/require" +) + +// enteredCreatePaymentClient blocks inside CreatePayment until the caller +// confirms entry (closing entered), then delays `delay` before forwarding. +// Because the online handler holds the crussell:payment: advisory +// lock from BEFORE the pending insert through to AFTER CreatePayment returns, +// a closed entered channel proves the lock is provably held — letting the test +// deterministically fire a concurrent till cash payment into that window. +type enteredCreatePaymentClient struct { + square.SquareClient + entered chan struct{} + delay time.Duration +} + +func (c *enteredCreatePaymentClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + select { + case <-c.entered: + default: + close(c.entered) + } + time.Sleep(c.delay) + return c.SquareClient.CreatePayment(ctx, req) +} + +// commitSetupTx commits the per-test transaction so the test operates at pool +// level on independent connections (advisory locks only serialize across +// independent connections; a shared per-test tx would mask the race). +func commitSetupTx(t *testing.T, ctx context.Context) { + t.Helper() + innerTx := db.TxFromContext(ctx) + require.NotNil(t, innerTx, "no transaction in context") + require.NoError(t, innerTx.Commit(ctx)) +} + +// TestTerminalCash_BlockedByConcurrentOnlineCharge pins the C2 fix: while an +// online CreateBookingPayment charge holds the crussell:payment: +// advisory lock across its Square round-trip, a till CASH payment on the SAME +// booking must fail fast with 409 — never pass its remaining-balance check and +// record a second amount (which buildSplitRecords would carve into a +// non-refundable tip). Before the fix the till branch serialized only on the +// bookings-row FOR UPDATE (a DIFFERENT primitive the online path never takes), +// so both charges would land and the overflow became a silent tip. +func TestTerminalCash_BlockedByConcurrentOnlineCharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + commitSetupTx(t, ctx) + + // The online charge holds the lock for ~4s (longer than the ~3s bounded + // try-lock), so the concurrent till cash request is guaranteed to hit a + // contended lock and give up with 409. + origClient := SquareClient + slow := &enteredCreatePaymentClient{SquareClient: square.NewDevClient(), entered: make(chan struct{}), delay: 4 * time.Second} + SquareClient = slow + defer func() { SquareClient = origClient }() + + pool := context.Background() + cardToken := "cnon:c2-online-inflight" + onlineReq := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "c2-online-inflight-" + bookingID, + } + + var wg sync.WaitGroup + var onlineRec *httptest.ResponseRecorder + wg.Add(1) + go func() { + defer wg.Done() + onlineRec = makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", onlineReq, userToken, pool) + }() + + // Block until the online charge is inside the Square call — the advisory + // lock is provably held from that point until the handler returns. + <-slow.entered + + cashReq := CreateTerminalPaymentRequest{ + Amount: 2000, + PaymentType: "full", + PaymentMethod: strPtr("cash"), + } + cashRec := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", cashReq, adminToken, pool) + require.Equal(t, http.StatusConflict, cashRec.Code, + "till cash payment must 409 while an online charge holds the payment lock, body: %s", cashRec.Body.String()) + + wg.Wait() + require.Equal(t, http.StatusOK, onlineRec.Code, "the online charge must succeed once the till payment was blocked: %s", onlineRec.Body.String()) + + // No cash payment row, no tip row (the till payment was never recorded), + // and the booking's real-money ledger is exactly the online charge. + assertNoTillMoneyRecorded(t, pool, bookingID, 5000) +} + +// TestTerminalGiftCard_BlockedByConcurrentOnlineCharge is the gift-card half +// of the C2 fix: the till GIFT-CARD branch takes the same advisory lock, so a +// concurrent online charge blocks it too (the pre-fix FOR UPDATE only +// serialized against other row-lock holders). +func TestTerminalGiftCard_BlockedByConcurrentOnlineCharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + commitSetupTx(t, ctx) + + origClient := SquareClient + slow := &enteredCreatePaymentClient{SquareClient: square.NewDevClient(), entered: make(chan struct{}), delay: 4 * time.Second} + SquareClient = slow + defer func() { SquareClient = origClient }() + + pool := context.Background() + cardToken := "cnon:c2-online-inflight-gc" + onlineReq := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "c2-online-inflight-gc-" + bookingID, + } + + var wg sync.WaitGroup + var onlineRec *httptest.ResponseRecorder + wg.Add(1) + go func() { + defer wg.Done() + onlineRec = makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", onlineReq, userToken, pool) + }() + + <-slow.entered + + gcReq := CreateTerminalPaymentRequest{ + Amount: 2000, + PaymentType: "full", + PaymentMethod: strPtr("giftcard"), + GiftCardID: strPtr("GC-0000-0000-0000"), + } + gcRec := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", gcReq, adminToken, pool) + require.Equal(t, http.StatusConflict, gcRec.Code, + "till gift-card payment must 409 while an online charge holds the payment lock, body: %s", gcRec.Body.String()) + + wg.Wait() + require.Equal(t, http.StatusOK, onlineRec.Code, onlineRec.Body.String()) + + assertNoTillMoneyRecorded(t, pool, bookingID, 5000) +} + +// TestOnlineCharge_BlockedWhileTillHoldsLock is the reverse direction of the +// C2 fix: while the till cash/giftcard branch holds the SAME advisory lock +// (here held directly to deterministically simulate a till charge in flight), +// an online CreateBookingPayment charge must fail fast with 409 — the two +// paths now contend on one primitive instead of passing their independent +// remaining-balance checks. +func TestOnlineCharge_BlockedWhileTillHoldsLock(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + commitSetupTx(t, ctx) + + pool := context.Background() + key := "crussell:payment:" + bookingID + + // Simulate a till cash/giftcard charge in flight: hold the same advisory + // lock on a dedicated connection for longer than the ~3s try-lock bound. + holder, err := db.Conn.Acquire(pool) + require.NoError(t, err) + defer holder.Release() + _, err = holder.Exec(pool, `SELECT pg_advisory_lock(hashtext($1))`, key) + require.NoError(t, err) + defer func() { _, _ = holder.Exec(pool, `SELECT pg_advisory_unlock(hashtext($1))`, key) }() + + cardToken := "cnon:c2-online-blocked" + req := CreateBookingPaymentRequest{ + Amount: 2000, + PaymentType: "partial", + NewCardToken: &cardToken, + IdempotencyKey: "c2-online-blocked-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, pool) + require.Equal(t, http.StatusConflict, w.Code, + "online charge must 409 while the till holds the payment lock, body: %s", w.Body.String()) + + var payCount int + require.NoError(t, db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount)) + require.Zero(t, payCount, "no payment record may exist when the online charge was blocked") +} + +// assertNoTillMoneyRecorded pins the "not silently double-record" half of C2: +// after a till cash/giftcard payment was blocked, the booking must carry no +// till-originated row, no tip row (the pre-fix double-charge carved the +// overflow into a non-refundable tip), and exactly wantPence of real-money +// completed payments. +func assertNoTillMoneyRecorded(t *testing.T, ctx context.Context, bookingID string, wantPence int64) { + t.Helper() + var cashRows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'cash'`, bookingID).Scan(&cashRows)) + require.Zero(t, cashRows, "the blocked till cash payment must not be recorded") + var gcRows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'giftcard'`, bookingID).Scan(&gcRows)) + require.Zero(t, gcRows, "the blocked till gift-card payment must not be recorded") + var tipRows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'`, bookingID).Scan(&tipRows)) + require.Zero(t, tipRows, "no non-refundable tip row may be minted by the blocked till payment") + var paidPence int64 + require.NoError(t, db.Conn.QueryRow(ctx, ` + SELECT COALESCE(ROUND(SUM(amount) * 100), 0) FROM payments + WHERE booking_id = $1 AND status = 'completed' AND payment_type <> 'tip' + AND payment_method NOT IN ('discount', 'on_the_house') + `, bookingID).Scan(&paidPence)) + require.Equal(t, wantPence, paidPence, "the booking ledger must reflect exactly the online charge") +} + +// TestCreateBookingPayment_SCATokenizeResult_UsedAsSource pins the SCA wire +// contract: a saved-card charge carrying new_card_token (the SCA +// tokenize-result from card.tokenize(verificationDetails, cardId)) plus +// saved_card_id must call Square with source_id = the tokenize-result token — +// NOT the stored ccof id — and with customer_id derived from the saved card, +// and must NOT require a verification_token for the new path. +func TestCreateBookingPayment_SCATokenizeResult_UsedAsSource(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestData(t, ctx, tx) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sca-tokenize-test", "VISA", "4242") + require.NoError(t, err) + userToken := jwt.GenerateUserToken(userID) + rec := installRecordingClient(t) + + token := "cnon:sca-tokenize-result" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &token, + UserSavedCardID: &cardID, + IdempotencyKey: "sca-tokenize-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + rec.mu.Lock() + last := rec.lastReq + rec.mu.Unlock() + require.Equal(t, token, last.SourceID, "the SCA tokenize-result token must be the charge source_id") + require.NotEqual(t, "ccof:sca-tokenize-test", last.SourceID, "the stored ccof id must NOT be the source for the tokenize-result flow") + require.NotEmpty(t, last.CustomerID, "customer_id must derive from the saved card row") + require.Empty(t, last.VerificationToken, "the tokenize-result flow must not require a legacy verification_token") + + // The recorded payment row must carry the token as its square source and + // reference the saved card row. + var squareSource, uscID sql.NullString + require.NoError(t, tx.QueryRow(ctx, `SELECT square_source_id, user_saved_card_id FROM payments WHERE booking_id = $1`, bookingID).Scan(&squareSource, &uscID)) + require.Equal(t, token, squareSource.String, "the payment row must record the tokenize-result token as its square source") + require.Equal(t, cardID, uscID.String, "the payment row must reference the saved card") +} + +// TestCreateBookingPayment_SCATokenizeResult_Skips2FAGate pins that the SCA +// tokenize-result flow is SCA-primary: with 2FA enforced, a charge carrying +// new_card_token + saved_card_id (no verification_code, no verification_token) +// succeeds — the tokenize-result token itself proves buyer verification, so no +// homegrown fallback authorization is demanded. +func TestCreateBookingPayment_SCATokenizeResult_Skips2FAGate(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestData(t, ctx, tx) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sca-tokenize-2fa", "VISA", "4242") + require.NoError(t, err) + userToken := jwt.GenerateUserToken(userID) + installRecordingClient(t) + + token := "cnon:sca-tokenize-2fa" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &token, + UserSavedCardID: &cardID, + IdempotencyKey: "sca-tokenize-2fa-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, + "an SCA tokenize-result charge must skip the 2FA gate, body: %s", w.Body.String()) +} + +// TestManualRefund_SynchronousCompletion_IssuesAdminAuditLog locks the M8 fix +// for the SYNC path: when the RefundPayment handler's FIRST Square attempt +// returns COMPLETED immediately (no pending row for the sweep to re-issue), the +// synchronous terminal-success path must write the SAME admin_audit_log row the +// sweep's re-issue writes — action 'admin_refund', admin actor, payment id, +// pence amount and reason, via the shared insertManualRefundAudit helper. +// The row is marked completed so the sweep (which only processes still-pending +// rows) can never re-process this refund, guaranteeing exactly one audit row. +// Mirrors TestManualRefund_IssuesAdminAuditLog (refunds_test.go) but drives +// the handler end-to-end instead of the sweep. +func TestManualRefund_SynchronousCompletion_IssuesAdminAuditLog(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed") + require.NoError(t, err) + // The mock refunds any non-"pay_mock_" id leniently and returns COMPLETED + // synchronously, so the handler's FIRST attempt completes immediately. + const chargeID = "sqp_m8_sync_audit" + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = $1 WHERE id = $2", chargeID, paymentID) + require.NoError(t, err) + + // The audit insert runs in its OWN transaction via InsertAdminAuditCharge + // (never the per-test tx), and the refund's advisory-lock serialization + // needs pool-level rows — commit the setup so the sync path executes at + // pool level, exactly like the sweep test. + commitSetupTx(t, ctx) + freshCtx := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_audit_log WHERE action_type = 'admin_refund' AND admin_id = $1`, adminID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE payment_id = $1`, paymentID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, paymentID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, adminID) + }) + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + adminToken := jwt.GenerateTestToken(adminID, "admin") + req := RefundRequest{Amount: 5000, Reason: "customer request"} + rec := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, freshCtx) + require.Equal(t, http.StatusOK, rec.Code, "synchronous refund should complete 200, body: %s", rec.Body.String()) + + var resp RefundResponse + require.NoError(t, parsePaymentResponseBody(rec, &resp)) + require.Equal(t, "completed", resp.Status, "the synchronous COMPLETED refund must resolve to 'completed'") + + // The refund row must be completed so the sweep can never re-process it + // (and duplicate the audit). + var status string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, resp.ID).Scan(&status)) + require.Equal(t, "completed", status) + + // Exactly one admin_refund audit row, carrying payment id, pence amount and + // reason. + var auditCount int + require.NoError(t, db.Conn.QueryRow(freshCtx, ` + SELECT COUNT(*) FROM admin_audit_log + WHERE admin_id = $1 AND action_type = 'admin_refund' + `, adminID).Scan(&auditCount)) + require.Equal(t, 1, auditCount, "expected exactly 1 'admin_refund' audit row for the synchronous refund") + var details string + require.NoError(t, db.Conn.QueryRow(freshCtx, ` + SELECT details::text FROM admin_audit_log + WHERE admin_id = $1 AND action_type = 'admin_refund' + `, adminID).Scan(&details)) + for _, want := range []string{paymentID, "5000", "customer request"} { + require.True(t, strings.Contains(details, want), "expected audit details to carry %q, got %s", want, details) + } +} + +// TestTwoFactorEnforced_CreatePaymentMethod_SCATokenizeResult_Save_Succeeds +// pins the M11 fix: in an ENFORCED (SCA-only) deployment a card save that +// carries a GENUINE SCA tokenize-result as card_token succeeds — the +// STORE-intent SCA performed at tokenization (SquareCardInput.tokenizeForStore) +// IS the verification (PSR 2017 reg 100), so the save skips the 2FA gate at the +// call site exactly like the charge surfaces' scaTokenizedSavedCard path, and +// the card is persisted. Before the fix the handler demanded the (removed) 2FA +// fallback and refused every save 402 verification_required, so add-card could +// not complete in an enforced deployment. +func TestTwoFactorEnforced_CreatePaymentMethod_SCATokenizeResult_Save_Succeeds(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateUserToken(userID) + t.Cleanup(func() { InvalidateSquareCustomerCache(userID) }) + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + req := CreatePaymentMethodRequest{CardToken: "cnon:sca-tokenize-store"} + w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) + require.Equal(t, http.StatusOK, w.Code, + "a genuine SCA tokenize-result card save must succeed in an enforced deployment, body: %s", w.Body.String()) + + var card SavedCard + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &card)) + require.NotEmpty(t, card.ID, "the saved card must be returned") + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount)) + require.Equal(t, 1, cardCount, "the SCA-compliant save must persist exactly one card") +} + +// TestTwoFactorEnforced_CreatePaymentMethod_ForgeVerificationToken_402 pins +// auth-F1 on the add-card surface: a verification_token is CLIENT-ASSERTED and +// never forwarded to Square on a SAVE surface (CreateCardOnFile takes no token), +// so it must NOT skip the gate — enforced + a raw nonce card_token + a (forged) +// non-empty verification_token is refused 402 verification_required and no card +// is persisted. The M11 fix cannot be a client-asserted token bypass. +func TestTwoFactorEnforced_CreatePaymentMethod_ForgeVerificationToken_402(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateUserToken(userID) + + forged := "forged-verification-token" + req := CreatePaymentMethodRequest{ + CardToken: "cnon:2fa-forged-save", + VerificationToken: &forged, + } + w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, + "a forged verification_token must not skip the save gate, body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount)) + require.Zero(t, cardCount, "a refused forged-token save must not persist a card") +} + +// TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_Refused402 pins the M11 +// refusal half in the SCA suite's own staging helper: a legacy token-less save +// (a raw card.tokenize() nonce carrying no SCA proof) is refused 402 +// verification_required in an enforced deployment and no card is persisted. +// Companion to errors_test.go's TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_402. +func TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_Refused402(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateUserToken(userID) + + req := CreatePaymentMethodRequest{CardToken: "cnon:2fa-tokenless-save"} + w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, + "a token-less save must be refused 402 in an enforced deployment, body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount)) + require.Zero(t, cardCount, "a refused token-less save must not persist a card") +} diff --git a/backend/handlers/payments/idempotency_helpers.go b/backend/handlers/payments/idempotency_helpers.go index 0506266..9d944de 100644 --- a/backend/handlers/payments/idempotency_helpers.go +++ b/backend/handlers/payments/idempotency_helpers.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "fmt" "os" + "strconv" "strings" "crussell/internal/square" @@ -36,6 +37,24 @@ func truncateIdempotencyKey(prefix, candidate string) string { return prefix + "-" + hex.EncodeToString(sum[:16]) } +// deriveRefundIdempotencyKey returns the deterministic SERVER-SIDE idempotency +// key for a refund issued WITHOUT a client-supplied key (M1): a retry of the +// same logical refund re-derives the SAME key, so Square's idempotency dedup +// returns the original refund instead of minting a SECOND Square refund — even +// after the sweep has resolved the first attempt (a client keyed to +// (payment_id, amount, refund type) can never regenerate the fresh random +// suffix the old no-key fallback used). The key is derived ONLY from stable +// request fields — never a random value — and routed through +// truncateIdempotencyKey so an over-length candidate stays deterministic and +// inside Square's 45-char /v2/refunds limit (preserving its semantics). The +// distinct refundType (e.g. "manual" vs "cancellation") keeps a partial refund +// of the same payment+amount distinct from a cancellation refund of the same +// size, and the paymentID prefix prevents cross-payment collisions. +func deriveRefundIdempotencyKey(paymentID string, amountPence int64, refundType string) string { + candidate := paymentID + "-refund-" + strconv.FormatInt(amountPence, 10) + "-" + refundType + return truncateIdempotencyKey("refund", candidate) +} + // nextIdempotencyCandidate returns the idempotency-key candidate for slot // sequence seq: the base key itself at seq 0, or "base-seq" at seq >= 1, then // truncated via truncateIdempotencyKey so the final key stays inside Square's diff --git a/backend/handlers/payments/idempotency_helpers_test.go b/backend/handlers/payments/idempotency_helpers_test.go new file mode 100644 index 0000000..6d85724 --- /dev/null +++ b/backend/handlers/payments/idempotency_helpers_test.go @@ -0,0 +1,126 @@ +//go:build test && dev + +package payments + +import ( + "context" + "testing" + + "crussell/internal/square" +) + +// TestDeriveRefundIdempotencyKey_Deterministic locks the M1 property: the +// server-side refund key derived from (payment_id, amount pence, refund type) +// is DETERMINISTIC — a retry of the same logical refund re-derives the SAME +// key, so Square's idempotency dedup returns the original refund instead of +// minting a second one. +func TestDeriveRefundIdempotencyKey_Deterministic(t *testing.T) { + key1 := deriveRefundIdempotencyKey("pay1234567890", 5000, "manual") + key2 := deriveRefundIdempotencyKey("pay1234567890", 5000, "manual") + if key1 != key2 { + t.Errorf("expected the derived refund key to be deterministic, got %q vs %q", key1, key2) + } +} + +// TestDeriveRefundIdempotencyKey_DistinguishesInputs locks the M1 discriminator +// fields: the key MUST change when the payment, the amount, or the refund type +// changes (a distinct partial refund of the same amount must never collide with +// a cancellation refund of the same size). +func TestDeriveRefundIdempotencyKey_DistinguishesInputs(t *testing.T) { + base := deriveRefundIdempotencyKey("pay1234567890", 5000, "manual") + + otherPayment := deriveRefundIdempotencyKey("pay9999999999", 5000, "manual") + if otherPayment == base { + t.Errorf("expected a different payment id to produce a different refund key") + } + + otherAmount := deriveRefundIdempotencyKey("pay1234567890", 5001, "manual") + if otherAmount == base { + t.Errorf("expected a different amount to produce a different refund key") + } + + otherType := deriveRefundIdempotencyKey("pay1234567890", 5000, "cancellation") + if otherType == base { + t.Errorf("expected a different refund type to produce a different refund key") + } +} + +// TestDeriveRefundIdempotencyKey_RespectsSquareLimit locks the +// truncateIdempotencyKey semantics: every derived key stays within Square's +// 45-char /v2/refunds idempotency-key limit, even for an over-length candidate +// (a long payment id / large amount), and the truncation stays deterministic. +func TestDeriveRefundIdempotencyKey_RespectsSquareLimit(t *testing.T) { + cases := []struct { + paymentID string + amount int64 + refundType string + }{ + {"pay1234567890", 5000, "manual"}, + {"pay1234567890", 5000, "cancellation"}, + {"a-very-long-payment-id-that-exceeds-the-45-char-limit-when-combined", 999999999, "manual"}, + {"pay1234567890", 999999999, "a-refund-type-that-is-itself-quite-long"}, + } + for _, c := range cases { + key := deriveRefundIdempotencyKey(c.paymentID, c.amount, c.refundType) + if len(key) > maxIdempotencyKeyLength { + t.Errorf("derived refund key %q (%d chars) exceeds Square's %d-char limit", key, len(key), maxIdempotencyKeyLength) + } + if key != deriveRefundIdempotencyKey(c.paymentID, c.amount, c.refundType) { + t.Errorf("derived refund key %q is not deterministic across calls", key) + } + } +} + +// TestDeriveRefundIdempotencyKey_RetryAfterSweepResolution_NoSecondRefund locks +// the M1 end-to-end dedup: a refund issued with the deterministic server-side +// key, resolved by a sweep pass, and then RETRIED maps to the SAME Square refund +// (the mock's refundByKey dedup returns the original) — a second Square refund +// is never minted. This is the mechanism the refund issuance uses so a +// no-client-key retry after sweep resolution dedups onto the first refund. +func TestDeriveRefundIdempotencyKey_RetryAfterSweepResolution_NoSecondRefund(t *testing.T) { + mock := square.NewDevClient().(*square.MockClient) + payment, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "seed-refund-dedup-payment", + }) + if err != nil { + t.Fatalf("failed to seed the payment at the mock: %v", err) + } + + paymentID := "payrefund000001" + amountPence := int64(5000) + // The no-client-key refund issues with the deterministic server-side key. + key := deriveRefundIdempotencyKey(paymentID, amountPence, "manual") + + first, err := mock.RefundPayment(context.Background(), square.RefundPaymentReq{ + PaymentID: payment.SquarePayID, + Amount: amountPence, + IdempotencyKey: key, + Reason: "test", + }) + if err != nil { + t.Fatalf("first refund failed: %v", err) + } + + // After the sweep resolves the first attempt, the client retries the SAME + // logical refund. The retry re-derives the SAME deterministic key, so the + // mock's Square-style dedup returns the ORIGINAL refund — RefundKeyCount + // stays 1 and no second Square refund is minted. + retry, retryErr := mock.RefundPayment(context.Background(), square.RefundPaymentReq{ + PaymentID: payment.SquarePayID, + Amount: amountPence, + IdempotencyKey: deriveRefundIdempotencyKey(paymentID, amountPence, "manual"), + Reason: "test", + }) + if retryErr != nil { + t.Fatalf("retry after sweep resolution failed: %v", retryErr) + } + if first.ID != retry.ID { + t.Errorf("expected the retry to dedup onto the original refund %s, got a different refund %s", first.ID, retry.ID) + } + if got := mock.RefundKeyCount(); got != 1 { + t.Errorf("expected exactly ONE Square refund after the retry, got %d distinct refund keys", got) + } +} diff --git a/backend/handlers/payments/refund_policy_boundary_test.go b/backend/handlers/payments/refund_policy_boundary_test.go new file mode 100644 index 0000000..a1868c7 --- /dev/null +++ b/backend/handlers/payments/refund_policy_boundary_test.go @@ -0,0 +1,91 @@ +//go:build test && dev + +package payments + +// M18 refund-tier epsilon boundary tests. CalculateRefundForCancellation uses +// STRICT comparisons (`> FullRefundThreshold`, `>= PartialRefundThreshold`), +// so the exact-boundary tier selection and the just-inside/just-outside +// epsilon margins are distinct behaviours. The exact 72h / exact 24h tier +// selection is already locked in refunds_test.go +// (TestCalculateRefundForCancellation_Exact72hBoundary / +// _Exact24hBoundary); these tests pin the epsilon margins AROUND both +// boundaries and assert the refundable/kept amounts at the exact boundaries +// (which the existing exact-boundary tests only check for the tier string). + +import ( + "testing" + "time" +) + +func TestCalculateRefundForCancellation_Boundary72h_EpsilonMargins(t *testing.T) { + t.Parallel() + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + + // 1s past 72h → the > comparison wins: full refund of all pre-payments. + over := CalculateRefundForCancellation(100, 50, start.Add(-72*time.Hour-1*time.Second), start) + if over.Tier != FullRefundTier { + t.Errorf("expected tier %q just past 72h, got %q", FullRefundTier, over.Tier) + } + if over.RefundableAmount != 50 || over.KeptAmount != 0 { + t.Errorf("expected full refund (50 refundable / 0 kept) just past 72h, got %.2f / %.2f", over.RefundableAmount, over.KeptAmount) + } + + // 1s inside 72h → the >= comparison wins: protected deposit kept. + under := CalculateRefundForCancellation(100, 50, start.Add(-72*time.Hour+1*time.Second), start) + if under.Tier != PartialRefundTier { + t.Errorf("expected tier %q 1s inside 72h, got %q", PartialRefundTier, under.Tier) + } + // Protected deposit = min(50, 100*0.5) = 50 → everything kept. + if under.KeptAmount != 50 || under.RefundableAmount != 0 { + t.Errorf("expected 50 kept / 0 refundable 1s inside 72h, got %.2f / %.2f", under.KeptAmount, under.RefundableAmount) + } +} + +func TestCalculateRefundForCancellation_Boundary24h_EpsilonMargins(t *testing.T) { + t.Parallel() + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + + // 1s past 24h → the >= comparison wins: protected deposit kept. + over := CalculateRefundForCancellation(100, 80, start.Add(-24*time.Hour-1*time.Second), start) + if over.Tier != PartialRefundTier { + t.Errorf("expected tier %q 1s past 24h, got %q", PartialRefundTier, over.Tier) + } + // Protected deposit = min(80, 50) = 50 → 30 refundable / 50 kept. + if over.RefundableAmount != 30 || over.KeptAmount != 50 { + t.Errorf("expected 30 refundable / 50 kept 1s past 24h, got %.2f / %.2f", over.RefundableAmount, over.KeptAmount) + } + + // 1s inside 24h → the default wins: nothing refundable, everything kept. + under := CalculateRefundForCancellation(100, 80, start.Add(-24*time.Hour+1*time.Second), start) + if under.Tier != NoRefundTier { + t.Errorf("expected tier %q 1s inside 24h, got %q", NoRefundTier, under.Tier) + } + if under.RefundableAmount != 0 || under.KeptAmount != 80 { + t.Errorf("expected 0 refundable / 80 kept 1s inside 24h, got %.2f / %.2f", under.RefundableAmount, under.KeptAmount) + } +} + +func TestCalculateRefundForCancellation_ExactBoundaries_Amounts(t *testing.T) { + t.Parallel() + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + + // Exactly 72h: NOT > 72h → partial tier. Amounts must match the partial + // tier computation (protected deposit kept), not the full tier. + at72 := CalculateRefundForCancellation(100, 100, start.Add(-72*time.Hour), start) + if at72.Tier != PartialRefundTier { + t.Fatalf("expected tier %q at exactly 72h, got %q", PartialRefundTier, at72.Tier) + } + if at72.ProtectedDeposit != 50 || at72.KeptAmount != 50 || at72.RefundableAmount != 50 { + t.Errorf("expected protected 50 / kept 50 / refundable 50 at exactly 72h, got %.2f / %.2f / %.2f", at72.ProtectedDeposit, at72.KeptAmount, at72.RefundableAmount) + } + + // Exactly 24h: >= 24h → partial tier (protected deposit kept). + at24 := CalculateRefundForCancellation(100, 60, start.Add(-24*time.Hour), start) + if at24.Tier != PartialRefundTier { + t.Fatalf("expected tier %q at exactly 24h, got %q", PartialRefundTier, at24.Tier) + } + // Protected deposit = min(60, 50) = 50 → 10 refundable / 50 kept. + if at24.RefundableAmount != 10 || at24.KeptAmount != 50 { + t.Errorf("expected 10 refundable / 50 kept at exactly 24h, got %.2f / %.2f", at24.RefundableAmount, at24.KeptAmount) + } +} diff --git a/backend/handlers/payments/terminal_sca_test.go b/backend/handlers/payments/terminal_sca_test.go index fc55020..dea0c50 100644 --- a/backend/handlers/payments/terminal_sca_test.go +++ b/backend/handlers/payments/terminal_sca_test.go @@ -2,10 +2,10 @@ package payments -// Tests for the CreateTerminalPayment SCA/2FA decision model: the -// VerificationToken field (handlers.go:114, validation 440-444, extraction -// 843-846, gate skip 1024, forwarding 1113), the admin-actor 2FA-fallback -// audit rows on the terminal and till saved-card surfaces, and the +// Tests for the CreateTerminalPayment SCA decision model: the VerificationToken +// field (handlers.go:114, validation 440-444, extraction 843-846, gate skip +// 1024, forwarding 1113), the SCA-only token-less refusal (402 +// verification_required) on the terminal and till saved-card surfaces, and the // customer_initiated classification (Square's SCA/liability-shift signal) on // every saved-card charge site. // @@ -29,7 +29,9 @@ import ( ) // helperEnvEnforce2FAStaging flips the env to an ENFORCED non-mock Square env -// that still resolves to the in-memory mock client. +// that still resolves to the in-memory mock client. Saved-card charges are +// SCA-only (the 2FA fallback was removed entirely), so an enforced env refuses +// token-less saved-card charges 402 verification_required. func helperEnvEnforce2FAStaging(t *testing.T) { t.Helper() t.Setenv("REQUIRE_2FA", "true") @@ -144,11 +146,12 @@ func TestTerminalSavedCard_VerificationToken_Skips2FA(t *testing.T) { require.Zero(t, auditCount, "an SCA-authorized terminal charge must not write a 2FA-fallback audit row") } -// TestTerminalSavedCard_2FAFallback_Audit_AdminActor pins (c)+(d): a token-less -// terminal saved-card charge falls back to the card owner's 2FA code and writes -// a strict 2fa_fallback_charge audit row for the ADMIN actor (admin_id = the -// charging admin, target_user_id = the card owner). -func TestTerminalSavedCard_2FAFallback_Audit_AdminActor(t *testing.T) { +// TestTerminalSavedCard_Tokenless_402 pins the SCA-only posture on the terminal +// saved-card surface: a token-less terminal saved-card charge is refused 402 +// verification_required even when the card owner holds a valid 2FA code (the +// homegrown 2FA fallback was removed entirely — PSR 2017 reg 100), and no +// 2fa_fallback_charge audit row is written. +func TestTerminalSavedCard_Tokenless_402(t *testing.T) { helperEnvEnforce2FAStaging(t) ctx, tx := testutils.SetupTestTx(t) @@ -160,45 +163,6 @@ func TestTerminalSavedCard_2FAFallback_Audit_AdminActor(t *testing.T) { require.NoError(t, err) adminToken := jwt.GenerateTestToken(adminID, "admin") - rec := installRecordingClient(t) - - req := CreateTerminalPaymentRequest{ - Amount: 5000, - PaymentType: "full", - PaymentMethod: strPtr("saved_card"), - UserSavedCardID: &cardID, - IdempotencyKey: "terminal-2fa-fallback-" + bookingID, - VerificationCode: "334411", - } - - w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) - - rec.mu.Lock() - got := rec.lastReq.VerificationToken - rec.mu.Unlock() - require.Empty(t, got, "a 2FA-fallback charge must not carry a verification token") - - assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, adminID, bookingID, "admin saved-card charge authorized via 2FA fallback (SCA unavailable)", "4242") -} - -// TestTerminalSavedCard_2FAFallbackDisabled_402 pins (d): when the deployment -// opts out of the 2FA fallback (TWO_FACTOR_FALLBACK=false), a token-less saved- -// card terminal charge is denied 402 with the structured verification_required -// body the frontend keys on to run the SCA challenge. -func TestTerminalSavedCard_2FAFallbackDisabled_402(t *testing.T) { - helperEnvEnforce2FAStaging(t) - t.Setenv("TWO_FACTOR_FALLBACK", "false") - ctx, tx := testutils.SetupTestTx(t) - - userID, bookingID, _ := setupTestData(t, ctx, tx) - seedTwoFAPendingCode(t, tx, userID, "998800") - cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_terminal_sca_no_fallback", "VISA", "4242") - require.NoError(t, err) - adminID, err := fixtures.CreateTestAdminUser(tx) - require.NoError(t, err) - adminToken := jwt.GenerateTestToken(adminID, "admin") - installRecordingClient(t) req := CreateTerminalPaymentRequest{ @@ -206,22 +170,26 @@ func TestTerminalSavedCard_2FAFallbackDisabled_402(t *testing.T) { PaymentType: "full", PaymentMethod: strPtr("saved_card"), UserSavedCardID: &cardID, - IdempotencyKey: "terminal-sca-no-fallback-" + bookingID, - VerificationCode: "998800", + IdempotencyKey: "terminal-tokenless-" + bookingID, + VerificationCode: "334411", } w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less charge with the fallback disabled must be denied 402, body: %s", w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less terminal saved-card charge must be refused 402 (SCA-only), body: %s", w.Body.String()) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) require.Equal(t, "verification_required", body["code"]) + + var auditCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM admin_audit_log WHERE action_type = '2fa_fallback_charge'`).Scan(&auditCount)) + require.Zero(t, auditCount, "the 2FA fallback was removed — no fallback audit row may be written") } -// TestTillSavedCard_2FAFallback_Audit_AdminActor pins (c): an admin till -// saved-card sale authorized by the 2FA fallback writes a strict -// 2fa_fallback_charge audit row for the ADMIN actor with the till sale id as -// the reference. -func TestTillSavedCard_2FAFallback_Audit_AdminActor(t *testing.T) { +// TestTillSavedCard_Tokenless_402 pins the SCA-only posture on the till saved- +// card surface: an admin till saved-card sale is refused 402 +// verification_required even with a valid 2FA code (the 2FA fallback was +// removed entirely), and no fallback audit row is written. +func TestTillSavedCard_Tokenless_402(t *testing.T) { helperEnvEnforce2FAStaging(t) ctx, tx := testutils.SetupTestTx(t) @@ -249,11 +217,14 @@ func TestTillSavedCard_2FAFallback_Audit_AdminActor(t *testing.T) { } w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) - require.Contains(t, []int{http.StatusOK, http.StatusCreated}, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less till saved-card sale must be refused 402 (SCA-only), body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) - var tillSaleID string - require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM till_sales WHERE idempotency_key = $1`, key).Scan(&tillSaleID)) - assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, adminID, tillSaleID, "admin till saved-card charge authorized via 2FA fallback (SCA unavailable)", "4242") + var auditCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM admin_audit_log WHERE action_type = '2fa_fallback_charge'`).Scan(&auditCount)) + require.Zero(t, auditCount, "the 2FA fallback was removed — no fallback audit row may be written") } // TestCustomerInitiated_ChargeClassification pins (f): the customer_initiated diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 0cc1de5..f0b80fb 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -15,9 +15,6 @@ import ( "crussell/clock" "crussell/db" "crussell/internal/twofa" - "crussell/mw" - - "github.com/jackc/pgx/v5" ) // require2FADisabled reports whether REQUIRE_2FA explicitly disables 2FA @@ -53,195 +50,174 @@ func (s *PaymentService) TwoFactorEnforced() bool { return twoFactorEnforced() } -// UserTwoFactorEnabled reports whether the user has completed 2FA setup -// (users.two_factor_enabled). It is the source of truth for the card-access -// gate: an enforced environment blocks online card access for users who have -// not enabled 2FA. -func (s *PaymentService) UserTwoFactorEnabled(ctx context.Context, userID string) (bool, error) { - var enabled bool - err := db.Conn.QueryRow(ctx, `SELECT two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&enabled) - if err != nil { - return false, err +// errTwoFAPepperRequired is returned when TWO_FACTOR_PEPPER is unset in a +// production-style re-issue gate. Refusing to re-issue is the only safe outcome: +// without the pepper the fresh code would be persisted as an unsalted SHA-256 +// digest in the 1M code space, which a log/DB leak could brute-force offline +// (mirrors handlers/user's errTwoFAPepperRequired). Defined here (no build tag) +// so the pure gate (twoFAReissueIssueAllowedStrict) and the test,dev suite +// share it. +var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") + +// errTwoFADeliveryUnavailable is returned when a production-style re-issue has +// no delivery channel: email/SMS unwired and the operator has not opted into +// the insecure log-delivery mode (TWO_FACTOR_ALLOW_LOG_DELIVERY=true). A fresh +// code the customer could never receive would strand them on the saved-card +// gate (mirrors handlers/user's errTwoFADeliveryUnavailable). +var errTwoFADeliveryUnavailable = errors.New("2FA requires an email or SMS delivery channel; contact the salon") + +// twoFAPepperConfigured reports whether TWO_FACTOR_PEPPER is set — the pure, +// build-agnostic read behind the strict re-issue gate. +func twoFAPepperConfigured() bool { return os.Getenv("TWO_FACTOR_PEPPER") != "" } + +// twoFADeliveryChannelConfigured reports whether the deployment has explicitly +// configured a 2FA code delivery channel: TWO_FACTOR_ALLOW_LOG_DELIVERY set to +// exactly "true" (the only production channel today — email/SMS unwired, P6). +// Pure env read, build-agnostic: the build-tagged twoFADeliveryAvailable +// (twofa_delivery_dev.go / twofa_delivery_prod.go) is the runtime-facing +// wrapper that turns this into the always-true dev channel or the prod env +// check. +func twoFADeliveryChannelConfigured() bool { return os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" } + +// twoFAReissueIssueAllowedStrict is the pure, build-agnostic production-style +// re-issue gate: a re-issued 2FA code may be minted ONLY when BOTH +// TWO_FACTOR_PEPPER is set (an unsalted digest in the 1M code space would be +// offline-brute-forceable) AND a delivery channel is configured (otherwise the +// fresh code could never reach the customer). Either way it fails closed. The +// build-tagged twoFAReissueIssueAllowed wraps it for production builds; +// dev/test builds always allow re-issue and never consult it — but the test,dev +// suite exercises THIS function directly, so the fail-closed branches are +// CI-visible even though the prod file (!dev && !test) is excluded there. +func twoFAReissueIssueAllowedStrict() error { + if !twoFAPepperConfigured() { + return errTwoFAPepperRequired } - return enabled, nil + if !twoFADeliveryChannelConfigured() { + return errTwoFADeliveryUnavailable + } + return nil } // Single source of truth for 2FA verification: crussell/internal/twofa owns // the code hashing (HMAC-SHA256 keyed by TWO_FACTOR_PEPPER, legacy SHA-256 // fallback), the constant-time compare, the code-lifetime check, and the // per-user brute-force lockout. The user package's interactive endpoints -// (setup/verify/disable) and this saved-card gate all share it; nothing is -// re-implemented locally here. Two agents once shipped a drift-risk duplicate -// of the hash+verify in this file (hashTwoFAVerificationCode + -// verifyPendingTwoFactorCode) — that copy is gone, and any future change to -// the hashing or lockout rules must land in internal/twofa only. +// (setup/verify/disable) and this file's re-issue path share it; nothing is +// re-implemented locally here. The saved-card charge gate no longer verifies +// 2FA codes at all (SCA-only — the 2FA fallback was removed), so the only +// local consumer left is reissueTwoFACodeAfterFailedCharge below. -// verifyPendingTwoFactorCode verifies the submitted code against the user's -// stored pending 2FA code. It is a thin delegation shim over -// twofa.VerifyForUser — the single source of truth for the verification core -// (per-user brute-force lockout, constant-time compare, legacy pre-pepper -// hash fallback, code lifetime). consume=true makes a verified code SINGLE-USE -// immediately (the pending code is NULLed on success); consume=false verifies -// WITHOUT consuming. Since finding 1 the saved-card CHARGE gates pass -// consume=!reusePendingRecord: a FRESH charge consumes at the gate (one code -// authorizes exactly one charge), while a PENDING-REUSE retry passes false and -// defers consumption to the completed-charge transaction via -// twofa.ConsumePendingCode, so a retry that fails again keeps its code for one -// more attempt. The save-card SAVE gates pass true because saving a card is a -// terminal operation with no downstream charge to attach consumption to. It -// returns nil on a valid code, or a classified twofa.ErrIncorrect / -// twofa.ErrLockedOut / twofa.ErrMissingOrExpired (or a wrapped DB error) for -// the caller to map to the correct HTTP status. -func verifyPendingTwoFactorCode(ctx context.Context, userID, code string, consume bool) error { - return twofa.VerifyForUser(ctx, userID, code, consume) +// enforceSCAFallbackConsent is retained as a compile-compatible NO-OP so the +// saved-card charge handlers (handlers.go / till.go / giftcards.go) keep +// compiling unchanged. It used to enforce the C6 consent notice on the +// SCA-unavailable → 2FA fallback path, which is now REMOVED ENTIRELY: PSR 2017 +// reg 100 makes Strong Customer Authentication mandatory and non-waivable for +// customer-initiated stored-credential charges, and the homegrown 2FA (a +// merchant-side check with no bank involvement) cannot legally act as an SCA +// fallback — authorising a token-less charge via 2FA leaves the MERCHANT liable +// for ECI 7 / SLI 210 chargebacks and PSR 2017 reg 77(6) compensation, and +// customer consent does not cure that. The gate now refuses a token-less +// saved-card charge 402 verification_required BEFORE any fallback can be used, +// so fallbackUsed is always false and there is no consent to demand. The +// callers pass it through as before; it always returns true. +func enforceSCAFallbackConsent(w http.ResponseWriter, consentVersion *string, consentAccepted bool, fallbackUsed bool) bool { + return true } -// twoFactorFallbackEnabled reports whether the homegrown 2FA may act as a -// BACKUP authorization for a saved-card charge when SCA is unavailable (the -// charge carries no Square verification_token). The parse is case-insensitive -// and alias-tolerant (false/0/off/no) — a value like "False" or "OFF" never -// silently leaves the fallback ON. Any other value — including empty and -// unknown — keeps the fallback enabled (the shipped default). It is the -// TWO_FACTOR_FALLBACK policy switch read at startup by main.go and exposed via -// PaymentService.TwoFactorFallbackEnabled. -func twoFactorFallbackEnabled() bool { - switch strings.ToLower(strings.TrimSpace(os.Getenv("TWO_FACTOR_FALLBACK"))) { - case "false", "0", "off", "no": - return false - default: - return true +// consentVersionValue normalizes the request's optional consent_version pointer +// to a string ("" when absent). Retained because the saved-card charge handlers +// still read the field when writing their (now unreachable) fallback audit row. +func consentVersionValue(version *string) string { + if version == nil { + return "" } + return *version } -// TwoFactorFallbackEnabled is the exported form of twoFactorFallbackEnabled, so -// main.go can log the SCA-primary/2FA-backup posture at startup without -// re-implementing the env logic. -func (s *PaymentService) TwoFactorFallbackEnabled() bool { - return twoFactorFallbackEnabled() -} - -// requireTwoFactorForCardAccess gates the saved-card online payment paths under -// the SCA-primary / 2FA-backup decision model. It returns (allowed, fallbackUsed): -// allowed is true when the request may proceed; fallbackUsed is true when the -// authorization was granted by the homegrown 2FA BACKUP (SCA was unavailable and -// the customer's 2FA code verified) — the caller must then write a strict -// insertTwoFAFallbackAudit row for the charge. +// requireTwoFactorForCardAccess gates the saved-card online payment paths. +// It returns (allowed, fallbackUsed): allowed is true when the request may +// proceed; fallbackUsed is ALWAYS false — the homegrown 2FA fallback for +// token-less saved-card charges was REMOVED ENTIRELY, so no charge is ever +// authorized by a 2FA code and no fallback audit row is ever written (the +// callers' insertTwoFAFallbackAudit branches are unreachable). +// +// It is a thin wrapper over requireTwoFactorForCardAccessWithTokenValidation +// that passes tokenForwardedToSquare=true — the legacy signature is retained +// because the saved-card CHARGE surfaces (booking, tip, gift-card buy, till) +// forward the verification_token to Square in CreatePaymentReq.VerificationToken, +// so a non-empty token there IS Square-validated SCA and legitimately skips the +// gate. The card-SAVE surfaces call the WithTokenValidation variant directly +// with false (see that helper for the auth-F1 rationale). // // The decision model, in order: // -// - 2FA is not enforced (dev/mock) → allowed, no fallback. +// - 2FA enforcement is not active (dev/mock, or REQUIRE_2FA disabled) → +// allowed. The dev mock simulates SCA (SimulateSavedCardVerificationRequired +// + cnon:sca-... tokenize-results), so development has full parity with the +// SCA-only production posture. // // - The request carries a Square verification_token (SCA performed — the -// issuer has already authenticated the buyer): SKIP the 2FA gate entirely. -// SCA is PRIMARY; the issuer did the job, so the homegrown gate is never -// consulted (fallbackUsed=false). A charge that carries a token passes even -// for a user who has not enabled 2FA. +// issuer has already authenticated the buyer) AND the token is forwarded to +// Square on this surface (tokenForwardedToSquare=true, the charge paths): +// SKIP the gate entirely. SCA is PRIMARY; a charge that carries a token +// passes even for a user who has not enabled 2FA. // -// - Otherwise the gate is the FALLBACK authorization for a ccof charge with -// no verification token. It only runs when the fallback is permitted: -// -// (a) TWO_FACTOR_FALLBACK is enabled (see twoFactorFallbackEnabled) — when -// the deployment opts out, a token-less charge is denied 402 -// verification_required: the frontend shows the SCA challenge, and if the -// bank cannot do SCA the payment cannot proceed (security-first); and -// -// (b) a 2FA code delivery channel exists (twoFADeliveryAvailable, build- -// dependent like the user package's) — a code the customer can never -// receive would silently lock the gate, so it is denied 503 -// ("2FA requires an email or SMS delivery channel"). -// -// - The user has completed 2FA setup (two_factor_enabled) AND the request -// carries a verification_code matching the user's stored pending code. -// -// B10: the setup flag alone must NOT unlock saved-card charges — an enforced -// environment requires an actual one-time code challenge at charge time, so -// merely enabling 2FA (a setup flag) can never unlock saved-card access with -// no challenge. The code is the customer's current pending 2FA code, which an -// operator relays (delivery is the user package's build-dependent [2FA] log / -// email-SMS channel). -// -// consume controls whether a verified code is NULLed immediately (consume=true -// — a FRESH charge's single-use burn at the gate, closing the TOCTOU where a -// verified-but-unconsumed code could authorize a second charge; and the -// save-card SAVE gate, a terminal operation) or left intact for the caller to -// consume when a PENDING-REUSE retry reaches terminal success -// (consume=false — see verifyPendingTwoFactorCode / twofa.ConsumePendingCode, -// finding 1). In every case the 5-attempt lockout and the -// code-destroy-on-lockout semantics are unchanged (twofa.Check). -// -// The code check is delegated to crussell/internal/twofa via -// verifyPendingTwoFactorCode, so this gate participates in the SAME per-user -// brute-force lockout (5 failed attempts invalidate the pending code) as the -// user package's setup/verify/disable flows. Classified errors map to the HTTP -// statuses the frontend expects: incorrect → 400, locked out → 429, missing or -// expired → 400, DB failure → 500. +// - Otherwise the charge is token-less, and there is NO homegrown 2FA +// fallback anymore. PSR 2017 reg 100 makes Strong Customer Authentication +// mandatory and NON-WAIVABLE for customer-initiated stored-credential +// charges, and a merchant-side 2FA check with no bank involvement cannot +// legally act as an SCA substitute: authorising a token-less charge via 2FA +// would leave the MERCHANT liable for ECI 7 / SLI 210 chargebacks and PSR +// 2017 reg 77(6) compensation, and customer consent does not cure that. On +// genuine sca-unavailable the charge is therefore REFUSED 402 +// verification_required and the customer is invited to pay online later. // // On any denial an error JSON is written (parseable by the frontend via // extractErrorMessage) and allowed=false is returned — the caller must abort // the charge. func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationCode, verificationToken string, consume bool) (allowed, fallbackUsed bool) { + return requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, verificationCode, verificationToken, consume, true) +} + +// requireTwoFactorForCardAccessWithTokenValidation is the real gate: +// requireTwoFactorForCardAccess above is the charge-surface wrapper that passes +// tokenForwardedToSquare=true. The extra flag distinguishes surfaces where the +// verification_token WILL be forwarded to Square (charge — Square validates it +// server-side and rejects a forged token, so a non-empty token legitimately +// proves SCA) from surfaces where it is client-asserted and NEVER forwarded +// (card SAVE — the card is persisted via CreateCardOnFile, which takes no +// verification_token, so Square never validates it). +// +// auth-F1: on a SAVE surface a forged non-empty verification_token must NOT +// skip the gate — with tokenForwardedToSquare=false the token is ignored and +// the token-less refusal below applies. An authenticated client can therefore +// no longer persist a card to the account with `verification_token: "anything"` +// and no SCA. The one legitimate SAVE skip is the call-site's scaTokenizedSavedCard +// flow (new_card_token + saved_card_id): there the SCA tokenize-result token IS +// the charge source and Square validates it as source_id, so the gate is skipped +// by the caller before this helper is ever reached. +func requireTwoFactorForCardAccessWithTokenValidation(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationCode, verificationToken string, consume bool, tokenForwardedToSquare bool) (allowed, fallbackUsed bool) { if !twoFactorEnforced() { return true, false } - if service == nil { - service = &PaymentService{} - } // SCA-primary: a Square verification_token means the issuer already - // completed Strong Customer Authentication — the 2FA gate is skipped and no - // fallback audit applies. - if verificationToken != "" { + // completed Strong Customer Authentication — the gate is skipped and no + // fallback applies. This skip is only valid when the token WILL be + // forwarded to Square (tokenForwardedToSquare=true, the charge surfaces): + // Square validates it and rejects a forged value. On a SAVE surface the + // token is client-asserted and never reaches Square, so a forged non-empty + // token must not skip the gate (auth-F1) — the token-less refusal below + // applies instead. + if verificationToken != "" && tokenForwardedToSquare { return true, false } - // 2FA is now the BACKUP authorization for a token-less ccof charge. Fail - // closed when the deployment disabled the fallback (TWO_FACTOR_FALLBACK) or - // has no delivery channel for its codes (twoFADeliveryAvailable). - if !twoFactorFallbackEnabled() { - writeVerificationRequiredResponse(w) - return false, false - } - if !twoFADeliveryAvailable() { - mw.RespondError(w, http.StatusServiceUnavailable, "2FA requires an email or SMS delivery channel; contact the salon") - return false, false - } - enabled, err := service.UserTwoFactorEnabled(r.Context(), userID) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - mw.RespondError(w, http.StatusForbidden, "Two-factor authentication is required to use online card payments. Enable it in your account settings.") - return false, false - } - log.Printf("failed to check two-factor status for user %s: %v", userID, err) - mw.RespondError(w, http.StatusInternalServerError, "failed to check two-factor status") - return false, false - } - if !enabled { - mw.RespondError(w, http.StatusForbidden, "Two-factor authentication is required to use online card payments. Enable it in your account settings.") - return false, false - } - // B10: an enforced charge of a saved card needs a live one-time code, not - // just the enabled setup flag. - if verificationCode == "" { - mw.RespondError(w, http.StatusForbidden, "A two-factor verification code is required to use this saved card. Ask the customer for their current code.") - return false, false - } - switch err := verifyPendingTwoFactorCode(r.Context(), userID, verificationCode, consume); { - case err == nil: - // The 2FA BACKUP authorized this token-less saved-card charge. The - // caller writes the strict fallback audit row on the charge's success. - return true, true - case errors.Is(err, twofa.ErrIncorrect): - mw.RespondError(w, http.StatusBadRequest, "Invalid verification code") - return false, false - case errors.Is(err, twofa.ErrLockedOut): - mw.RespondError(w, http.StatusTooManyRequests, "Too many attempts") - return false, false - case errors.Is(err, twofa.ErrMissingOrExpired): - mw.RespondError(w, http.StatusBadRequest, "Verification code expired — request a new one") - return false, false - default: - log.Printf("failed to check two-factor verification code for user %s: %v", userID, err) - mw.RespondError(w, http.StatusInternalServerError, "failed to check two-factor verification code") - return false, false - } + // SCA-only: a token-less saved-card charge has NO 2FA fallback (the + // homegrown fallback was REMOVED — see the gate doc above for the PSR 2017 + // legal rationale). It is refused 402 verification_required; the customer is + // invited to pay online later (or through Square's buyer-verification flow + // on a retry). fallbackUsed stays false. + writeVerificationRequiredResponse(w) + return false, false } // reissueTwoFACodeAfterFailedCharge mints a fresh 2FA code after a saved-card @@ -363,13 +339,13 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID return } st.SetLastMintAtLocked(clock.Now()) - // Delivery mirrors the user package's build-dependent behaviour (the - // operator relays the [2FA] log line). Production logs the plaintext code - // only when explicitly opted in; dev/test always. - if IsExplicitDevOrMockEnv() || os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { - log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) - log.Printf("[2FA] code: %s", code) - } + // Delivery is build-dependent (twofa_delivery_dev.go / twofa_delivery_prod.go), + // mirroring the user package's twoFADeliverCode: dev/test builds always write + // the [2FA] log line (the operator relays the code); production writes it ONLY + // when the operator explicitly opted into log delivery + // (TWO_FACTOR_ALLOW_LOG_DELIVERY=true) — otherwise the plaintext code is never + // logged. + twoFAReissueDeliverCode(userID, code) } // twoFAMintCooldown bounds how often the re-issue path mints a fresh 2FA code diff --git a/backend/handlers/payments/twofa_delivery_dev.go b/backend/handlers/payments/twofa_delivery_dev.go index b23f2db..1a1a589 100644 --- a/backend/handlers/payments/twofa_delivery_dev.go +++ b/backend/handlers/payments/twofa_delivery_dev.go @@ -2,6 +2,8 @@ package payments +import "log" + // twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in // this build. Dev/test builds always have one — the [2FA] log line is the // documented loose-fake delivery channel — so the 2FA BACKUP authorization @@ -18,3 +20,16 @@ func twoFADeliveryAvailable() bool { return true } // twofa_dev.go). Production builds fail closed here — no pepper, no delivery // channel, no codes (see twofa_delivery_prod.go). func twoFAReissueIssueAllowed() error { return nil } + +// twoFAReissueDeliverCode delivers a re-issued code (a fresh saved-card charge +// consumed the customer's code at the gate and the charge failed at Square). +// Dev/test builds always deliver via the [2FA] log line — the documented +// loose-fake delivery channel — so the operator can relay the fresh code to the +// customer. Mirrors the user package's twoFADeliverCode; production logs it +// ONLY with the explicit TWO_FACTOR_ALLOW_LOG_DELIVERY opt-in (see +// twofa_delivery_prod.go). MEDIUM-3b: the user id and the plaintext code go to +// SEPARATE log lines so a single record cannot trivially pair them. +func twoFAReissueDeliverCode(userID, code string) { + log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) + log.Printf("[2FA] code: %s", code) +} diff --git a/backend/handlers/payments/twofa_delivery_prod.go b/backend/handlers/payments/twofa_delivery_prod.go index f9a2c09..768a498 100644 --- a/backend/handlers/payments/twofa_delivery_prod.go +++ b/backend/handlers/payments/twofa_delivery_prod.go @@ -3,7 +3,7 @@ package payments import ( - "errors" + "log" "os" ) @@ -13,18 +13,13 @@ import ( // (TWO_FACTOR_ALLOW_LOG_DELIVERY=true). Without a channel, codes can never // reach the customer, so the 2FA BACKUP authorization (the saved-card gate // when SCA is unavailable) cannot operate and a token-less saved-card charge is -// denied 503 (see requireTwoFactorForCardAccess). Mirrors -// handlers/user/twofa_prod.go; dev/test builds always deliver (twofa_delivery_dev.go). +// denied 503 (see requireTwoFactorForCardAccess). Delegates to the pure +// build-agnostic twoFADeliveryChannelConfigured (twofa.go); dev/test builds +// always deliver (twofa_delivery_dev.go). func twoFADeliveryAvailable() bool { - return os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" + return twoFADeliveryChannelConfigured() } -// errTwoFAPepperRequired is returned by twoFAReissueIssueAllowed when -// TWO_FACTOR_PEPPER is unset in a production build — the re-issue would -// otherwise persist an offline-brute-forceable unsalted SHA-256 digest in the -// 1M code space (mirrors handlers/user's errTwoFAPepperRequired). -var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") - // twoFAReissueIssueAllowed is the re-issue path's issuance gate // (reissueTwoFACodeAfterFailedCharge, handlers.go), mirroring the user // package's twoFAEnsureIssueAllowed (handlers/user/twofa_prod.go) build-tagged @@ -32,7 +27,10 @@ var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing // Without a channel the code could never reach the customer, and without the // pepper every stored code would be an offline-brute-forceable unsalted digest // — either way the re-issue refuses (fail-closed), exactly like the interactive -// mint paths. Dev/test builds always allow issuance (twofa_delivery_dev.go). +// mint paths. Delegates to the pure build-agnostic gate +// twoFAReissueIssueAllowedStrict (twofa.go), which the test,dev suite also +// exercises directly; dev/test builds always allow issuance +// (twofa_delivery_dev.go). // // The pepper check is the ONLY hard gate on the re-issue (plus the delivery // channel). PEPPER-CHANGE HAZARD (Loop B finding 2): the pepper keys the @@ -43,11 +41,23 @@ var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing // re-run 2FA setup), or a fresh saved-card charge whose code was consumed at // the gate will strand the customer with 400 ErrMissingOrExpired on retry. func twoFAReissueIssueAllowed() error { - if os.Getenv("TWO_FACTOR_PEPPER") == "" { - return errTwoFAPepperRequired + return twoFAReissueIssueAllowedStrict() +} + +// twoFAReissueDeliverCode delivers a re-issued code after a failed fresh +// saved-card charge (the gate consumed the customer's code at verify time). +// Production's ONLY channel is the operator's explicit, insecure opt-in to log +// delivery (TWO_FACTOR_ALLOW_LOG_DELIVERY=true — anyone with backend log access +// could defeat the 2FA gate on saved-card charges). WITHOUT that flag the +// plaintext code is NEVER written to the log (issuance was already refused by +// twoFAReissueIssueAllowed, so this no-op is unreachable); with it, the code +// goes to the [2FA] log line for the operator to relay to the customer, exactly +// like the documented dev flow. MEDIUM-3b: the user id and the plaintext code +// go to SEPARATE log lines so a single record cannot trivially pair them. +func twoFAReissueDeliverCode(userID, code string) { + if os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { + log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) + log.Printf("[2FA] code: %s", code) } - if !twoFADeliveryAvailable() { - return errors.New("2FA requires an email or SMS delivery channel; contact the salon") - } - return nil + // Otherwise: deliberate no-op — never log the plaintext code by default. } diff --git a/backend/handlers/payments/twofa_failclosed_test.go b/backend/handlers/payments/twofa_failclosed_test.go new file mode 100644 index 0000000..09ab6ca --- /dev/null +++ b/backend/handlers/payments/twofa_failclosed_test.go @@ -0,0 +1,60 @@ +//go:build test + +package payments + +// M17 follow-up (ITEM 2): twofa_delivery_prod.go is excluded from the test,dev +// suite (`!dev && !test`), so its fail-closed re-issue branches were never +// exercised in CI — the old twofa_delivery_prod_test.go only runs its +// assertions in a genuine production build and skips under the test tag. The +// pure decision logic now lives build-agnostically in twofa.go +// (twoFAReissueIssueAllowedStrict / twoFAPepperConfigured / +// twoFADeliveryChannelConfigured); these tests exercise those branches in the +// STANDARD test,dev run, so a regression in the prod fail-closed behaviour is +// CI-visible even though the prod file itself is only compiled in a genuine +// production build. + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestTwoFAReissueIssueAllowedStrict_FailClosed pins the production-style +// re-issue gate that twofa_delivery_prod.go's twoFAReissueIssueAllowed +// delegates to (reissueTwoFACodeAfterFailedCharge): (a) pepper unset → +// re-issue refused (errTwoFAPepperRequired — an unsalted digest in the 1M code +// space would be offline-brute-forceable); (b) delivery channel absent → +// re-issue refused (errTwoFADeliveryUnavailable — the 503-style error); +// (c) both configured → re-issue succeeds. +func TestTwoFAReissueIssueAllowedStrict_FailClosed(t *testing.T) { + t.Run("pepper_unset_refuses_reissue", func(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "") + t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") + require.ErrorIs(t, twoFAReissueIssueAllowedStrict(), errTwoFAPepperRequired) + }) + + t.Run("delivery_channel_absent_refuses_reissue", func(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper") + t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "") + require.ErrorIs(t, twoFAReissueIssueAllowedStrict(), errTwoFADeliveryUnavailable) + }) + + t.Run("pepper_and_channel_present_allows_reissue", func(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper") + t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") + require.NoError(t, twoFAReissueIssueAllowedStrict()) + }) +} + +// TestPaymentsTwoFADeliveryChannelConfigured pins the pure delivery-channel +// predicate behind the payments 503 refusal: only the exact value "true" opens +// the channel. +func TestPaymentsTwoFADeliveryChannelConfigured(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper") + for _, v := range []string{"", "1", "yes", "on", "True", "TRUE", "false"} { + t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", v) + require.False(t, twoFADeliveryChannelConfigured(), "value %q must NOT open the delivery channel (exact 'true' only)", v) + } + t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") + require.True(t, twoFADeliveryChannelConfigured()) +} diff --git a/backend/handlers/payments/twofa_gate_consume_test.go b/backend/handlers/payments/twofa_gate_consume_test.go index c8d4665..6670c95 100644 --- a/backend/handlers/payments/twofa_gate_consume_test.go +++ b/backend/handlers/payments/twofa_gate_consume_test.go @@ -2,36 +2,27 @@ package payments -// Tests pinning the Loop B 2FA single-use consume-at-gate fix on the two -// saved-card charge gates that were still consuming AFTER the charge landed: -// the till saved-card charge (till.go) and the gift-card purchase saved-card -// gate (giftcards.go). -// -// Semantics (mirroring CreateBookingPayment, handlers.go): -// - A FRESH charge verifies the 2FA code WITH consumption at the gate -// (consume = !reusePendingRecord). The code is single-use, closing the -// TOCTOU where a verified-but-unconsumed code could authorize a second -// concurrent charge. If Square then fails, a fresh code is re-issued -// (reissueTwoFACodeAfterFailedCharge) so the same-key retry has a live -// code to verify. -// - A PENDING-REUSE retry verifies WITHOUT consuming: the code was re-issued -// for exactly this retry, and the post-charge success path consumes it on -// terminal success, so a retry that fails again keeps its code for one -// more attempt. +// Tests pinning the SCA-only behavior on the till and gift-card saved-card +// charge gates (till.go / giftcards.go) after the 2FA fallback removal: the +// gate NEVER verifies or consumes a 2FA code anymore — a token-less saved-card +// charge is refused 402 verification_required up front (PSR 2017 reg 100), so +// the customer's pending code is left untouched (not consumed at the gate, no +// re-issue on failure). The consume-at-gate / re-issue semantics that these +// tests used to pin are GONE: the homegrown 2FA fallback was removed entirely. // // These tests flip REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv and therefore // must stay sequential (no t.Parallel) — see the note at the top of // twofa_test.go. They use SQUARE_ENVIRONMENT=staging (NOT production) for // enforcement: twoFactorEnforced() is fail-closed, so any non-mock/dev value // enforces the gate, while square.NewDevClient() — which the injected fault -// client and structuredSquareErrorWithCode construct at call time — returns -// the in-memory mock for every env except production/sandbox. The shared -// helperEnvEnforce2FA sets production, which would panic NewDevClient in a -// dev build. +// client constructs at call time — returns the in-memory mock for every env +// except production/sandbox. The shared helperEnvEnforce2FA sets production, +// which would panic NewDevClient in a dev build. import ( "context" "database/sql" + "encoding/json" "net/http" "testing" "time" @@ -46,13 +37,12 @@ import ( "github.com/stretchr/testify/require" ) -// TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure pins the -// till saved-card gate on a FRESH charge: the 2FA code is consumed AT THE GATE -// (single-use), and a failed Square charge re-issues a fresh code so the -// same-key retry can verify again. The stored hash must differ from the seeded -// one — if the gate still used consume=false the seeded hash would survive -// unchanged and there would be no re-issue. -func TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure_ConsumesAtGate_Reissues(t *testing.T) { +// TestTwoFactorEnforced_CreateTillSale_SavedCard_Tokenless_RefusedWithoutConsuming +// pins the till saved-card gate: a token-less charge is refused 402 +// verification_required BEFORE the 2FA code is consulted, so the customer's +// pending code is never consumed and no re-issue runs (the fallback was +// removed — the gate has no code path at all). +func TestTwoFactorEnforced_CreateTillSale_SavedCard_Tokenless_RefusedWithoutConsuming(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") ctx, tx := testutils.SetupTestTx(t) @@ -83,19 +73,24 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure_Consumes } w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less till saved-card charge must be refused 402 verification_required (SCA-only), body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + // The gate never verified the code — the seeded hash must survive untouched + // (no consumption, no re-issue). var hash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a failed fresh saved-card charge must re-issue a live 2FA code for the same-key retry") - require.NotEqual(t, twofa.Hash("556677"), hash.String, "the gate must have consumed the seeded code at verification time (single-use)") + require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") + require.Equal(t, twofa.Hash("556677"), hash.String, "the gate must not consume the code — it was never consulted") } -// TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure pins the -// till saved-card gate on a PENDING-REUSE retry: the code is verified WITHOUT -// consumption, so a retry that fails again keeps its seeded code unchanged and -// no re-issue runs (the fresh-charge-only guard must not fire). -func TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure_KeepsCode(t *testing.T) { +// TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Tokenless_Refused +// pins the same SCA-only refusal on a PENDING-REUSE till retry: the gate still +// refuses 402 verification_required (the code is irrelevant), leaving the +// seeded code untouched. +func TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Tokenless_Refused(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") ctx, tx := testutils.SetupTestTx(t) @@ -142,18 +137,19 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure_KeepsCo } w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less till saved-card charge must be refused 402 verification_required (SCA-only), body: %s", w.Body.String()) var hash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a pending-reuse retry must not consume the code at the gate") - require.Equal(t, twofa.Hash("667788"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)") + require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") + require.Equal(t, twofa.Hash("667788"), hash.String, "the gate must not consume the code — it was never consulted") } -// TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure pins the gift-card -// purchase saved-card gate on a FRESH charge: consume-at-gate + re-issue on -// failure, exactly as the till gate above. -func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure_ConsumesAtGate_Reissues(t *testing.T) { +// TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_RefusedWithoutConsuming +// pins the SCA-only refusal on the gift-card purchase saved-card gate: a +// token-less charge is refused 402 verification_required and the seeded code is +// untouched (no consumption, no re-issue). +func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_RefusedWithoutConsuming(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") ctx, tx := testutils.SetupTestTx(t) @@ -179,18 +175,22 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure_ConsumesAtGate_Re } w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less gift-card saved-card charge must be refused 402 verification_required (SCA-only), body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) var hash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a failed fresh saved-card purchase must re-issue a live 2FA code for the same-key retry") - require.NotEqual(t, twofa.Hash("112233"), hash.String, "the gate must have consumed the seeded code at verification time (single-use)") + require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") + require.Equal(t, twofa.Hash("112233"), hash.String, "the gate must not consume the code — it was never consulted") } -// TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure pins the -// gift-card purchase gate on a PENDING-REUSE retry: verify-without-consume and -// no re-issue, so the seeded code survives a second failed retry unchanged. -func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure_KeepsCode(t *testing.T) { +// TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Tokenless_Refused +// pins the SCA-only refusal on a PENDING-REUSE gift-card purchase retry: the +// gate refuses 402 verification_required regardless of the pending state, +// leaving the seeded code untouched. +func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Tokenless_Refused(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") ctx, tx := testutils.SetupTestTx(t) @@ -225,24 +225,20 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure_KeepsCode( } w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less gift-card saved-card charge must be refused 402 verification_required (SCA-only), body: %s", w.Body.String()) var hash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a pending-reuse retry must not consume the code at the gate") - require.Equal(t, twofa.Hash("334455"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)") + require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") + require.Equal(t, twofa.Hash("334455"), hash.String, "the gate must not consume the code — it was never consulted") } -// TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Failure_Reissues -// pins the save-gate code-burn re-issue for the BOOKING path: a NEW-card -// (cnon) charge with save_card=true passes the SAVE gate (consume=true — the -// single-use code is burned there), so when the subsequent Square charge is -// definitively declined the re-issue guard must fire (req.SaveCard) and mint a -// fresh code — otherwise every same-key retry hits "Verification code expired" -// forever (finding: 2FA code burned by the SAVE gate never re-issued). The -// stored hash must differ from the seeded one: if the re-issue did not run the -// gate's consumption would leave no pending code at all. -func TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Failure_Reissues(t *testing.T) { +// TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Tokenless_Refused +// pins the SCA-only refusal on the booking NEW-card + save_card SAVE gate: a +// token-less save is refused 402 verification_required and the seeded code is +// untouched (no consumption at the save gate, no re-issue — the fallback is +// gone). +func TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Tokenless_Refused(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") ctx, tx := testutils.SetupTestTx(t) @@ -266,20 +262,20 @@ func TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Failure_Reissues } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less new-card + save_card booking charge must be refused 402 verification_required (SCA-only), body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) var hash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a failed new-card + save_card booking charge must re-issue a live 2FA code for the same-key retry") - require.NotEqual(t, twofa.Hash("999001"), hash.String, "the SAVE gate consumed the seeded code at verification time — the failure must re-issue a fresh one") + require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") + require.Equal(t, twofa.Hash("999001"), hash.String, "the save gate must not consume the code — it was never consulted") } -// TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Failure_Reissues pins -// the same save-gate code-burn re-issue for the TIP path (mirrors the booking -// test above): a NEW-card tip with save_card=true burns its code at the SAVE -// gate, so a definitively declined charge must re-issue a fresh code for the -// same-key retry. -func TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Failure_Reissues(t *testing.T) { +// TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Tokenless_Refused pins +// the same SCA-only refusal on the TIP new-card + save_card SAVE gate. +func TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Tokenless_Refused(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") ctx, tx := testutils.SetupTestTx(t) @@ -304,20 +300,22 @@ func TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Failure_Reissues(t * } w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) - require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less new-card + save_card tip charge must be refused 402 verification_required (SCA-only), body: %s", w.Body.String()) var hash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a failed new-card + save_card tip charge must re-issue a live 2FA code for the same-key retry") - require.NotEqual(t, twofa.Hash("999002"), hash.String, "the SAVE gate consumed the seeded code at verification time — the failure must re-issue a fresh one") + require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") + require.Equal(t, twofa.Hash("999002"), hash.String, "the save gate must not consume the code — it was never consulted") } // TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown pins the -// LOW-MEDIUM finding 2 contract on the re-issue path: the re-issue mints a -// live code after a FRESH charge consumed one at the gate, respects the same -// per-user mint cooldown as the interactive mint endpoints (a second re-issue -// inside the window is a no-op), and runs again once the cooldown elapses -// (simulated by clearing the shared stamp the way a successful verify does). +// LOW-MEDIUM finding 2 contract on the retained re-issue helper: the re-issue +// mints a live code after a FRESH charge consumed one at the gate (in the +// pre-removal world), respects the same per-user mint cooldown as the +// interactive mint endpoints (a second re-issue inside the window is a no-op), +// and runs again once the cooldown elapses (simulated by clearing the shared +// stamp). The helper is retained because handlers.go / till.go / giftcards.go +// still call it with their (now always-false) fallbackUsed flag. func TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "staging") diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go index 8e53813..c1d0de6 100644 --- a/backend/handlers/payments/twofa_test.go +++ b/backend/handlers/payments/twofa_test.go @@ -2,22 +2,24 @@ package payments -// Tests for the PSD2 SCA stand-in gate (twofa.go): the twoFactorEnforced() env -// matrix, requireTwoFactorForCardAccess() gating, and the end-to-end -// enforcement of the saved-card payment paths in CreateBookingPayment / -// CreateTillSale. Tests that flip REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv -// must stay sequential (no t.Parallel): os.Getenv is process-global and -// t.Setenv panics under t.Parallel. Sequential tests run before this package's -// parallel batch, so the enforced env never leaks into parallel tests. +// Tests for the PSD2 SCA gate (twofa.go): the twoFactorEnforced() env matrix, +// requireTwoFactorForCardAccess() gating, and the end-to-end enforcement of the +// saved-card payment paths in CreateBookingPayment / CreateTillSale. Saved-card +// charges are SCA-ONLY: the homegrown 2FA fallback was REMOVED entirely (PSR +// 2017 reg 100 — SCA mandatory and non-waivable for customer-initiated +// stored-credential charges), so a token-less saved-card charge is always +// refused 402 verification_required and no 2FA code can authorise it. Tests +// that flip REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv must stay sequential +// (no t.Parallel): os.Getenv is process-global and t.Setenv panics under +// t.Parallel. Sequential tests run before this package's parallel batch, so +// the enforced env never leaks into parallel tests. import ( "bytes" "context" - "database/sql" "encoding/json" "net/http" "net/http/httptest" - "os" "testing" "crussell/db" @@ -37,11 +39,16 @@ func helperEnvEnforce2FA(t *testing.T) { t.Helper() t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "production") + // There is no TWO_FACTOR_FALLBACK switch anymore (the 2FA fallback was + // removed — SCA-only); enforcement now only means token-less saved-card + // charges are refused 402 verification_required. } // seedTwoFAPendingCode stores a pending 2FA code hash + expiry for a user, the // state the user package's deliverTwoFACode writes. The hash uses the shared -// twofa.Hash — the same single source of truth the gate verifies with. +// twofa.Hash — the same single source of truth the re-issue path uses. +// Used by tests that prove even a VALID pending 2FA code cannot authorise a +// token-less saved-card charge (SCA-only). func seedTwoFAPendingCode(t *testing.T, q db.Querier, userID, code string) { t.Helper() _, err := q.Exec(context.Background(), ` @@ -103,7 +110,7 @@ func TestTwoFactorEnforced(t *testing.T) { // TestRequireTwoFactorForCardAccess_VerificationTokenSkips pins the SCA-primary // leg of the decision model: when the request carries a Square verification_token -// (the issuer already completed SCA), the 2FA gate is skipped entirely — even a +// (the issuer already completed SCA), the gate is skipped entirely — even a // user with NO 2FA setup passes, no code is demanded, and no fallback is used. func TestRequireTwoFactorForCardAccess_VerificationTokenSkips(t *testing.T) { helperEnvEnforce2FA(t) @@ -113,39 +120,220 @@ func TestRequireTwoFactorForCardAccess_VerificationTokenSkips(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", "vrf_sca_token_123", false) - require.True(t, allowed, "SCA performed — the 2FA gate must be skipped") - require.False(t, fallbackUsed, "SCA is primary — the 2FA fallback was not used") + require.True(t, allowed, "SCA performed — the gate must be skipped") + require.False(t, fallbackUsed, "SCA is primary — no fallback is ever used") require.Equal(t, http.StatusOK, w.Code, "no denial response may be written when the token skips the gate") } -// TestRequireTwoFactorForCardAccess_FallbackDisabled_402Structured pins the -// SCA-only posture (TWO_FACTOR_FALLBACK=false): a token-less saved-card charge -// has no 2FA fallback, so the gate denies 402 with the structured -// verification_required body the frontend keys on to trigger the SCA challenge. -func TestRequireTwoFactorForCardAccess_FallbackDisabled_402Structured(t *testing.T) { +// TestRequireTwoFactorForCardAccess_TokenForwardedDistinction pins auth-F1 at +// the gate level: the SAME non-empty verification_token must be treated +// differently by surface. On a token-FORWARDED (charge) surface it is +// Square-validated SCA and skips the gate (tokenForwardedToSquare=true). On a +// card-SAVE surface it is client-asserted and never reaches Square, so it must +// NOT skip the gate (tokenForwardedToSquare=false) — a forged token cannot +// authorise a save, exactly like a token-less request, and no 2FA code can +// authorise it either (SCA-only). +func TestRequireTwoFactorForCardAccess_TokenForwardedDistinction(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + + t.Run("forged_token_no_code_refused_402", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) + require.NoError(t, err) + w := httptest.NewRecorder() + allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "", "forged-token", true, false) + require.False(t, allowed, "a forged non-empty token must not skip the gate on the save path") + require.False(t, fallbackUsed) + require.Equal(t, http.StatusPaymentRequired, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + }) + + t.Run("forged_token_with_valid_code_still_refused_402", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "424242") + w := httptest.NewRecorder() + allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "424242", "forged-token", true, false) + require.False(t, allowed, "a valid 2FA code cannot authorise a save (SCA-only — the 2FA fallback was removed)") + require.False(t, fallbackUsed, "the 2FA fallback was removed — fallbackUsed is always false") + require.Equal(t, http.StatusPaymentRequired, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + }) + + t.Run("forged_token_user_not_enabled_refused_402", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + w := httptest.NewRecorder() + allowed, _ := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "424242", "forged-token", true, false) + require.False(t, allowed, "a user without 2FA setup cannot save a card even with a token") + require.Equal(t, http.StatusPaymentRequired, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + }) + + t.Run("token_forwarded_variant_still_skips", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + w := httptest.NewRecorder() + allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "", "vrf_sca_token_123", false, true) + require.True(t, allowed, "a token-forwarded charge path still skips on a non-empty token (SCA)") + require.False(t, fallbackUsed) + require.Equal(t, http.StatusOK, w.Code) + }) +} + +// TestTwoFactorEnforced_CreateBookingPayment_SaveCard_ForgeToken_Blocked pins +// auth-F1 end-to-end on the booking SAVE gate: enforced + save_card=true + a +// forged non-empty verification_token is denied 402 verification_required with +// no payment row and no saved card. A forged token can never skip the SAVE gate +// (Square never validates it there), and no 2FA code can fall back (SCA-only). +func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_ForgeToken_Blocked(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:test-card-nonce" + forged := "forged-verification-token" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "2fa-save-card-forge-token", + VerificationToken: &forged, + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Zero(t, payCount, "blocked save-card request must not create a payment row") + var cardCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1", userID).Scan(&cardCount)) + require.Zero(t, cardCount, "blocked save-card request must not persist a card") +} + +// TestTwoFactorEnforced_CreateBookingPayment_SaveCard_ForgeToken_With2FA_Blocked +// pins that a forged token on the booking SAVE gate cannot authorise a save by +// ANY fallback: even with a valid 2FA code the request is refused 402 +// verification_required (SCA-only — the homegrown 2FA fallback was removed). +func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_ForgeToken_With2FA_Blocked(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + seedTwoFAPendingCode(t, tx, userID, "112233") + + cardToken := "cnon:test-card-nonce" + forged := "forged-verification-token" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "2fa-save-card-forge-token-ok", + VerificationCode: "112233", + VerificationToken: &forged, + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Zero(t, payCount, "blocked save-card request must not create a payment row") +} + +// TestTwoFactorEnforced_CreateBookingPayment_SCATokenizeResult_SaveGate_Skips +// pins auth-F1 scenario (c): the legitimate SCA-primary tokenize-result flow +// (new_card_token + saved_card_id) still skips BOTH 2FA gates on save+charge — +// the SAVE gate is skipped because the combined path never persists a card +// (resolveChargeSource uses the token as the one-time source) and the +// tokenize-result token is validated by Square as the source_id. A user with +// NO 2FA setup and NO code succeeds: the tokenize-result token is the SCA proof. +func TestTwoFactorEnforced_CreateBookingPayment_SCATokenizeResult_SaveGate_Skips(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sca-tokenize-save-2fa", "VISA", "4242") + require.NoError(t, err) + userToken := jwt.GenerateUserToken(userID) + installRecordingClient(t) + + // The fixture above already created one saved-card row for the user; the + // combined SCA tokenize-result path must not add another. + var cardsBefore int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1", userID).Scan(&cardsBefore)) + + token := "cnon:sca-tokenize-save-2fa" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &token, + UserSavedCardID: &cardID, + SaveCard: true, + IdempotencyKey: "sca-tokenize-save-2fa-" + bookingID, + } + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, + "an SCA tokenize-result save+charge must skip both 2FA gates, body: %s", w.Body.String()) + + // The combined path never persists a card — the SCA tokenize-result is the + // one-time charge source, not a new card-on-file. + var cardsAfter int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1", userID).Scan(&cardsAfter)) + require.Equal(t, cardsBefore, cardsAfter, "the SCA tokenize-result save+charge must not persist a new card") +} + +// TestRequireTwoFactorForCardAccess_Tokenless_402Structured pins the SCA-only +// posture: a token-less saved-card charge is ALWAYS refused 402 with the +// structured verification_required body — even when the customer holds a valid +// 2FA code, because the homegrown 2FA fallback was removed entirely (PSR 2017 +// reg 100: SCA is mandatory and non-waivable for customer-initiated +// stored-credential charges). There is no TWO_FACTOR_FALLBACK switch to flip. +func TestRequireTwoFactorForCardAccess_Tokenless_402Structured(t *testing.T) { helperEnvEnforce2FA(t) - t.Setenv("TWO_FACTOR_FALLBACK", "false") ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "123456") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456", "", false) - require.False(t, allowed, "SCA-only posture: no 2FA fallback for a token-less charge") - require.False(t, fallbackUsed) + require.False(t, allowed, "SCA-only: no 2FA fallback for a token-less charge") + require.False(t, fallbackUsed, "fallbackUsed must be false — the 2FA fallback was removed") require.Equal(t, http.StatusPaymentRequired, w.Code) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) require.Equal(t, "verification_required", body["code"]) } -// TestTwoFactorEnforced_BookingSavedCard_Fallback_Audits pins the strict audit -// trail: a 2FA-fallback saved-card charge (no verification token, code verified) -// must write an admin_audit_log row with action_type '2fa_fallback_charge' for -// the customer actor — the operator can distinguish SCA-authorized charges from -// fallback-authorized ones — and the row's details JSON must carry the strict -// shape (sca_performed:false, fallback_reason, card_last4, reference_id, notes). -func TestTwoFactorEnforced_BookingSavedCard_Fallback_Audits(t *testing.T) { +// TestTwoFactorEnforced_BookingSavedCard_Tokenless_402 pins the SCA-only +// posture end-to-end on the booking saved-card charge path: a token-less +// saved-card charge (even with a valid 2FA code) is refused 402 +// verification_required, no payment row is created, and no 2fa_fallback_charge +// audit row is written (the fallback audit path is unreachable — the gate never +// authorises a charge by 2FA). +func TestTwoFactorEnforced_BookingSavedCard_Tokenless_402(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -164,38 +352,22 @@ func TestTwoFactorEnforced_BookingSavedCard_Fallback_Audits(t *testing.T) { } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) - details := assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, userID, bookingID, "saved-card charge authorized via 2FA fallback (SCA unavailable)", "4242") - require.Equal(t, false, details["sca_performed"]) - require.Equal(t, "verification_unavailable", details["fallback_reason"]) -} - -// assertTwoFAFallbackAuditDetails reads the most recent '2fa_fallback_charge' -// admin_audit_log row for (target_user_id, admin_id) and asserts the strict -// insertTwoFAFallbackAudit details shape: sca_performed=false, -// fallback_reason="verification_unavailable", card_last4, reference_id, notes. -func assertTwoFAFallbackAuditDetails(t *testing.T, ctx context.Context, q db.Querier, userID, adminID, referenceID, notes, wantLast4 string) map[string]any { - t.Helper() - var detailsJSON []byte - require.NoError(t, q.QueryRow(ctx, ` - SELECT details FROM admin_audit_log - WHERE action_type = '2fa_fallback_charge' AND target_user_id = $1 AND admin_id = $2 - ORDER BY created_at DESC LIMIT 1 - `, userID, adminID).Scan(&detailsJSON)) - var details map[string]any - require.NoError(t, json.Unmarshal(detailsJSON, &details)) - require.Equal(t, false, details["sca_performed"], "details.sca_performed must be false") - require.Equal(t, "verification_unavailable", details["fallback_reason"], "details.fallback_reason") - require.Equal(t, wantLast4, details["card_last4"], "details.card_last4") - require.Equal(t, referenceID, details["reference_id"], "details.reference_id") - require.Equal(t, notes, details["notes"], "details.notes") - return details + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Zero(t, payCount, "a refused token-less charge must not create a payment row") + var auditCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM admin_audit_log WHERE action_type = '2fa_fallback_charge'").Scan(&auditCount)) + require.Zero(t, auditCount, "the 2FA fallback was removed — no fallback audit row may be written") } // TestTwoFactorEnforced_BookingSavedCard_SCA_Skips_Audit pins that an SCA- // authorized charge (verification token present) writes NO 2fa_fallback_charge -// audit row: SCA is primary and the homegrown fallback was not used. +// audit row: SCA is primary and the fallback no longer exists at all. func TestTwoFactorEnforced_BookingSavedCard_SCA_Skips_Audit(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -224,11 +396,10 @@ func TestTwoFactorEnforced_BookingSavedCard_SCA_Skips_Audit(t *testing.T) { require.Zero(t, auditCount, "an SCA-authorized charge must not write a 2FA-fallback audit row") } -// TestTwoFactorEnforced_TipSavedCard_Fallback_Audit pins the strict audit -// trail on the TIP SAVE gate (CreateTipPayment with save_card=true): the -// token-less save is authorized by the 2FA fallback, and the resulting -// charge writes a 2fa_fallback_charge row for the customer actor. -func TestTwoFactorEnforced_TipSavedCard_Fallback_Audit(t *testing.T) { +// TestTwoFactorEnforced_TipSavedCard_Tokenless_402 pins the SCA-only posture on +// the TIP SAVE gate (CreateTipPayment with save_card=true): a token-less save +// is refused 402 verification_required even with a valid 2FA code. +func TestTwoFactorEnforced_TipSavedCard_Tokenless_402(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -248,16 +419,17 @@ func TestTwoFactorEnforced_TipSavedCard_Fallback_Audit(t *testing.T) { } w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) - - assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, userID, bookingID, "saved-card tip charge authorized via 2FA fallback (SCA unavailable)", "4242") + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) } -// TestTwoFactorEnforced_TipChargeSavedCard_Fallback_Audit pins the strict audit -// trail on the TIP CHARGE gate (CreateTipPayment charging an existing saved -// card): the token-less charge is authorized by the 2FA fallback and writes a -// 2fa_fallback_charge row for the customer actor. -func TestTwoFactorEnforced_TipChargeSavedCard_Fallback_Audit(t *testing.T) { +// TestTwoFactorEnforced_TipChargeSavedCard_Tokenless_402 pins the SCA-only +// posture on the TIP CHARGE gate (CreateTipPayment charging an existing saved +// card): a token-less charge is refused 402 verification_required even with a +// valid 2FA code. +func TestTwoFactorEnforced_TipChargeSavedCard_Tokenless_402(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -277,17 +449,16 @@ func TestTwoFactorEnforced_TipChargeSavedCard_Fallback_Audit(t *testing.T) { } w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) - - assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, userID, bookingID, "saved-card tip charge authorized via 2FA fallback (SCA unavailable)", "4242") + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) } -// TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fallback_Audit pins the strict -// audit trail on the gift-card purchase saved-card gate (giftcards.go): the -// token-less charge is authorized by the 2FA fallback and writes a -// 2fa_fallback_charge row for the customer actor with the payment id as the -// reference. -func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fallback_Audit(t *testing.T) { +// TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_402 pins the SCA-only +// posture on the gift-card purchase saved-card gate (giftcards.go): a token-less +// charge is refused 402 verification_required even with a valid 2FA code. +func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_402(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -308,27 +479,26 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fallback_Audit(t *testing.T) { } w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) - require.Contains(t, []int{http.StatusOK, http.StatusCreated}, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) - var payID string - require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE idempotency_key = $1`, key).Scan(&payID)) - assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, userID, payID, "gift-card purchase authorized via 2FA fallback (SCA unavailable)", "4242") + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE idempotency_key = $1", key).Scan(&payCount)) + require.Zero(t, payCount, "a refused token-less gift-card purchase must not create a payment row") } -// TestTwoFactorEnforced_PaymentMethodSave_Fallback_Audit pins the strict audit -// trail on the add-card save gate (handlers.go CreatePaymentMethod): persisting -// a card via the 2FA fallback writes a 2fa_fallback_charge row for the customer -// actor with an empty reference id (no charge reference exists for a save). -func TestTwoFactorEnforced_PaymentMethodSave_Fallback_Audit(t *testing.T) { +// TestTwoFactorEnforced_PaymentMethodSave_Tokenless_402 pins the SCA-only +// posture on the add-card save gate (handlers.go CreatePaymentMethod): the +// dedicated add-card endpoint carries no verification_token field, so a save is +// refused 402 verification_required even with a valid 2FA code (SCA-only). +func TestTwoFactorEnforced_PaymentMethodSave_Tokenless_402(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) - t.Cleanup(func() { - InvalidateSquareCustomerCache(userID) - _, _ = db.Conn.Exec(context.Background(), `DELETE FROM user_saved_cards WHERE user_id = $1`, userID) - }) token := jwt.GenerateUserToken(userID) seedTwoFAPendingCode(t, tx, userID, "998877") @@ -338,9 +508,14 @@ func TestTwoFactorEnforced_PaymentMethodSave_Fallback_Audit(t *testing.T) { } w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) - assertTwoFAFallbackAuditDetails(t, ctx, tx, userID, userID, "", "card persisted via 2FA fallback (SCA unavailable)", "4242") + var cardCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1", userID).Scan(&cardCount)) + require.Zero(t, cardCount, "a refused token-less save must not persist a card") } // TestRequireTwoFactorForCardAccess_NotEnforced verifies the dev/mock path @@ -354,128 +529,82 @@ func TestRequireTwoFactorForCardAccess_NotEnforced(t *testing.T) { w := httptest.NewRecorder() allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, nil, "000000000001", "", "", false) require.True(t, allowed, "no response must be written when not enforced") - require.False(t, fallbackUsed, "not enforced — the 2FA fallback did not authorize anything") + require.False(t, fallbackUsed, "not enforced — no fallback is ever used") require.Equal(t, http.StatusOK, w.Code) } +// TestRequireTwoFactorForCardAccess_Enforced pins the SCA-only refusal in an +// enforced environment: a token-less saved-card charge is refused 402 +// verification_required regardless of the user's 2FA state or a submitted code — +// the homegrown 2FA fallback was removed entirely and cannot authorise anything. func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) - t.Run("user_not_enabled_writes_403_json", func(t *testing.T) { + t.Run("user_not_enabled_writes_402_json", func(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456", "", false) require.False(t, ok) - require.Equal(t, http.StatusForbidden, w.Code) + require.Equal(t, http.StatusPaymentRequired, w.Code) var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "403 body must be mw.RespondError JSON") - require.NotEmpty(t, body["error"]) + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "402 body must be structured JSON") + require.Equal(t, "verification_required", body["code"]) }) - t.Run("user_enabled_but_no_code_writes_403_json", func(t *testing.T) { + t.Run("user_enabled_but_no_code_writes_402_json", func(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - // B10: the enabled setup flag alone must NOT unlock the gate. ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", "", false) require.False(t, ok) - require.Equal(t, http.StatusForbidden, w.Code) + require.Equal(t, http.StatusPaymentRequired, w.Code) }) - t.Run("user_enabled_with_valid_code_allows", func(t *testing.T) { + t.Run("user_enabled_with_valid_code_still_refused_402", func(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) seedTwoFAPendingCode(t, tx, userID, "424242") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", "", false) - require.True(t, allowed) - require.True(t, fallbackUsed, "a code-verified token-less charge uses the 2FA fallback") - require.Equal(t, http.StatusOK, w.Code) + require.False(t, allowed, "a valid 2FA code cannot authorise a token-less charge (SCA-only)") + require.False(t, fallbackUsed, "fallbackUsed must be false — the 2FA fallback was removed") + require.Equal(t, http.StatusPaymentRequired, w.Code) }) - t.Run("user_enabled_with_wrong_code_writes_400_json", func(t *testing.T) { + t.Run("user_enabled_with_wrong_code_writes_402_json", func(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) seedTwoFAPendingCode(t, tx, userID, "424242") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "000000", "", false) - require.False(t, ok) - require.Equal(t, http.StatusBadRequest, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - require.Equal(t, "Invalid verification code", body["error"]) + require.False(t, ok, "the gate does not verify codes anymore — any token-less charge is refused 402") + require.Equal(t, http.StatusPaymentRequired, w.Code) }) - t.Run("unknown_user_writes_403_json", func(t *testing.T) { + t.Run("unknown_user_writes_402_json", func(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000", "123456", "", false) require.False(t, ok) - require.Equal(t, http.StatusForbidden, w.Code) + require.Equal(t, http.StatusPaymentRequired, w.Code) var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "403 body must be mw.RespondError JSON") - require.NotEmpty(t, body["error"]) + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "402 body must be structured JSON") + require.Equal(t, "verification_required", body["code"]) }) } -// TestRequireTwoFactorForCardAccess_VerifyDoesNotConsume pins the MEDIUM-2 -// contract: the charge gate verifies the code WITHOUT consuming it (consume -// happens later, at the charge's terminal SUCCESS state via -// twofa.ConsumePendingCode), so a failed/ambiguous Square charge does NOT burn -// the operator-relayed code — a same-key retry can re-verify the SAME code. -// Only an explicit ConsumePendingCode (the completed-charge path) NULLs it, -// after which the code is dead ("expired"). -func TestRequireTwoFactorForCardAccess_VerifyDoesNotConsume(t *testing.T) { - helperEnvEnforce2FA(t) - ctx, tx := testutils.SetupTestTx(t) - userID, err := fixtures.CreateTestUser(tx) - require.NoError(t, err) - seedTwoFAPendingCode(t, tx, userID, "424242") - - req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) - w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", "", false) - require.True(t, allowed, "first gate pass must succeed") - require.True(t, fallbackUsed, "the code verification is the 2FA fallback") - require.Equal(t, http.StatusOK, w.Code) - - // The code must still be present — the gate verified WITHOUT consuming. - var pendingHash sql.NullString - require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) - require.True(t, pendingHash.Valid, "the gate must NOT consume the code (MEDIUM-2)") - - // A same-key retry (e.g. after a failed Square charge) re-verifies the SAME code. - w = httptest.NewRecorder() - allowed, _ = requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", "", false) - require.True(t, allowed, "a not-yet-consumed code must pass the gate again on retry") - require.Equal(t, http.StatusOK, w.Code) - - // Consumption happens at the charge's terminal SUCCESS state. - require.NoError(t, twofa.ConsumePendingCode(ctx, tx, userID)) - require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) - require.False(t, pendingHash.Valid, "ConsumePendingCode must NULL the pending code") - - // A further attempt with the consumed code is denied as expired. - w = httptest.NewRecorder() - allowed, _ = requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", "", false) - require.False(t, allowed, "a consumed code must not pass the gate") - require.Equal(t, http.StatusBadRequest, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - require.Equal(t, "Verification code expired — request a new one", body["error"]) -} - // TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked verifies the -// end-to-end gate on the save-card path: enforced + user without 2FA → 403 with -// no payment row and no saved card (Square never called). +// end-to-end gate on the save-card path: enforced + no SCA → 402 +// verification_required with no payment row and no saved card (Square never +// called). func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -493,10 +622,10 @@ func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked(t *testing.T) { } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusForbidden, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - require.Contains(t, body["error"], "Two-factor") + require.Equal(t, "verification_required", body["code"]) var payCount int require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) @@ -507,7 +636,7 @@ func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked(t *testing.T) { } // TestTwoFactorEnforced_CreateBookingPayment_SavedCard_Blocked verifies the -// gate on charging an existing saved card. +// gate on charging an existing saved card without SCA. func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_Blocked(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -525,17 +654,20 @@ func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_Blocked(t *testing.T) } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusForbidden, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - require.Contains(t, body["error"], "Two-factor") + require.Equal(t, "verification_required", body["code"]) var payCount int require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) require.Zero(t, payCount, "blocked saved-card charge must not create a payment row") } -func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Succeeds(t *testing.T) { +// TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Blocked pins that +// a valid 2FA code cannot unlock the booking SAVE gate (SCA-only — the 2FA +// fallback was removed). +func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Blocked(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -554,14 +686,20 @@ func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Succeeds(t *tes } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) var payCount int require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) - require.Equal(t, 1, payCount) + require.Zero(t, payCount, "a valid 2FA code must not create a payment row (SCA-only)") } -func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Succeeds(t *testing.T) { +// TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Blocked pins +// that a valid 2FA code cannot unlock a saved-card charge (SCA-only — the 2FA +// fallback was removed). +func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Blocked(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -580,12 +718,15 @@ func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Succeeds(t *te } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) } // TestTwoFactorEnforced_NewCardCharge_NotGated verifies the gate applies ONLY // to saved-card paths: a new-card (nonce) charge is allowed without 2FA even -// when enforced. +// when enforced (SCA is performed by Square's buyer-verification flow). func TestTwoFactorEnforced_NewCardCharge_NotGated(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -606,8 +747,8 @@ func TestTwoFactorEnforced_NewCardCharge_NotGated(t *testing.T) { } // TestTwoFactorEnforced_CreateTillSale_SavedCard_Blocked verifies the till's -// saved-card charge path: an admin charging a customer's saved card while the -// card's owner has no 2FA is blocked with 403 and no till_sale is created. +// saved-card charge path: an admin charging a customer's saved card without SCA +// is blocked with 402 verification_required and no till_sale is created. func TestTwoFactorEnforced_CreateTillSale_SavedCard_Blocked(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -643,17 +784,20 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_Blocked(t *testing.T) { r.Post("/api/admin/till/sale", CreateTillSale) r.ServeHTTP(w, req) - require.Equal(t, http.StatusForbidden, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - require.Contains(t, body["error"], "Two-factor") + require.Equal(t, "verification_required", body["code"]) var saleCount int require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM till_sales").Scan(&saleCount)) require.Zero(t, saleCount, "blocked till saved-card sale must not create a till_sale row") } -func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Succeeds(t *testing.T) { +// TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Blocked pins that a +// valid 2FA code cannot unlock the till saved-card gate (SCA-only — the 2FA +// fallback was removed). +func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Blocked(t *testing.T) { helperEnvEnforce2FA(t) _, tx := testutils.SetupTestTx(t) @@ -690,145 +834,48 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Succeeds(t *testing. r.Post("/api/admin/till/sale", CreateTillSale) r.ServeHTTP(w, req) - require.Contains(t, []int{http.StatusOK, http.StatusCreated}, w.Code, w.Body.String()) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) } -// TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Retry_ReturnsCompleted -// pins the Loop B MEDIUM gate-ordering fix: the save-card 2FA gate runs AFTER -// the idempotency dedup's completed short-circuit. A same-key lost-response -// retry re-sends the SAME single-use verification code that the original -// attempt already consumed; if the gate ran first it would 400 "Verification -// code expired". With the gate below the dedup, the retry returns the -// already-completed payment instead of re-entering the gate. -func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Retry_ReturnsCompleted(t *testing.T) { +// TestSCASuccess_BookingSavedCard_NeverRequiresConsent pins the SCA-primary +// carve-out: a charge carrying a Square verification_token (SCA performed) is +// never gated on the (removed) fallback consent — with no consent fields and no +// 2FA setup it succeeds and writes no fallback audit row. +func TestSCASuccess_BookingSavedCard_NeverRequiresConsent(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) userID, bookingID, _ := setupTestData(t, ctx, tx) userToken := jwt.GenerateUserToken(userID) - seedTwoFAPendingCode(t, tx, userID, "778899") + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:consent_sca", "VISA", "4242") + require.NoError(t, err) - cardToken := "cnon:2fa-save-card-retry" + vrf := "vrf_consent_sca_success" req := CreateBookingPaymentRequest{ - Amount: 2500, - PaymentType: "deposit", - NewCardToken: &cardToken, - SaveCard: true, - IdempotencyKey: "2fa-save-card-retry", - VerificationCode: "778899", + Amount: 5000, + PaymentType: "full", + CardID: &cardID, + IdempotencyKey: "consent-sca", + VerificationToken: &vrf, } - handler := withNonGuest(CreateBookingPayment) - w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "SCA-success must never demand consent, body: %s", w.Body.String()) - // Same-key retry re-sends the identical request, whose code is now - // consumed. The completed-dedup must return the payment before the gate. - w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - require.Equal(t, http.StatusOK, w2.Code, "same-key retry must dedup to the completed payment, not re-run the gate: %s", w2.Body.String()) - - var payCount int - require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) - require.Equal(t, 1, payCount, "the retry must not create a second payment") -} - -// TestTwoFactorEnforced_CreateTipPayment_SavedCard_Retry_ReturnsCompleted pins -// the Loop B MEDIUM gate-ordering fix on the tip endpoint: the saved-card -// charge 2FA gate runs AFTER the tip idempotency dedup's completed -// short-circuit. A same-key lost-response retry re-sends the SAME single-use -// verification code the original attempt consumed; with the gate first it would -// 400 "expired", with the gate below the dedup the retry returns the completed -// tip instead. -func TestTwoFactorEnforced_CreateTipPayment_SavedCard_Retry_ReturnsCompleted(t *testing.T) { - helperEnvEnforce2FA(t) - ctx, tx := testutils.SetupTestTx(t) - - userID, bookingID, _ := setupTestDataPast(t, ctx, tx) - userToken := jwt.GenerateUserToken(userID) - seedTwoFAPendingCode(t, tx, userID, "667788") - - _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") - require.NoError(t, err) - cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_tip_retry", "VISA", "4321") - require.NoError(t, err) - - req := CreateTipPaymentRequest{ - Amount: 500, - CardID: &cardID, - IdempotencyKey: "2fa-tip-saved-card-retry", - VerificationCode: "667788", - } - - handler := withNonGuest(CreateTipPayment) - w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) - require.Equal(t, http.StatusOK, w.Code, w.Body.String()) - - // Same-key retry re-sends the identical request, whose code is now - // consumed. The completed-dedup must return the tip before the gate. - w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) - require.Equal(t, http.StatusOK, w2.Code, "same-key retry must dedup to the completed tip, not re-run the gate: %s", w2.Body.String()) - - var tipCount int - require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'", bookingID).Scan(&tipCount)) - require.Equal(t, 1, tipCount, "the retry must not create a second tip") -} - -// TestTwoFactorFallbackEnabled pins the TWO_FACTOR_FALLBACK policy-switch -// parse matrix (twofa.go:100-107): false/0/off/no are case-insensitive -// DISABLES; every other value — empty, unset, unknown, true/1/on/yes — keeps -// the fallback enabled (the shipped default). The exported -// PaymentService.TwoFactorFallbackEnabled wrapper must match the unexported -// predicate. Sequential (no t.Parallel): flips process-global env vars. -func TestTwoFactorFallbackEnabled(t *testing.T) { - tests := []struct { - name string - val string - want bool - }{ - {"false disables", "false", false}, - {"zero disables", "0", false}, - {"off disables", "off", false}, - {"no disables", "no", false}, - {"case_insensitive_False_disables", "False", false}, - {"case_insensitive_OFF_disables", "OFF", false}, - {"case_insensitive_No_disables", "No", false}, - {"empty_keeps_enabled", "", true}, - {"unknown_keeps_enabled", "enable", true}, - {"true_keeps_enabled", "true", true}, - {"one_keeps_enabled", "1", true}, - {"on_keeps_enabled", "on", true}, - {"yes_keeps_enabled", "yes", true}, - {"case_insensitive_TRUE_keeps_enabled", "TRUE", true}, - {"case_insensitive_ON_keeps_enabled", "ON", true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Setenv("TWO_FACTOR_FALLBACK", tt.val) - require.Equal(t, tt.want, twoFactorFallbackEnabled()) - require.Equal(t, tt.want, NewPaymentService().TwoFactorFallbackEnabled(), "exported wrapper must match twoFactorFallbackEnabled") - }) - } - t.Run("unset_keeps_enabled_default", func(t *testing.T) { - prev, had := os.LookupEnv("TWO_FACTOR_FALLBACK") - os.Unsetenv("TWO_FACTOR_FALLBACK") - defer func() { - if had { - os.Setenv("TWO_FACTOR_FALLBACK", prev) - } else { - os.Unsetenv("TWO_FACTOR_FALLBACK") - } - }() - require.True(t, twoFactorFallbackEnabled()) - require.True(t, NewPaymentService().TwoFactorFallbackEnabled()) - }) + var auditCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM admin_audit_log WHERE action_type = '2fa_fallback_charge'").Scan(&auditCount)) + require.Zero(t, auditCount, "an SCA-success charge must not write a fallback audit row") } // TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue documents the dev/test // delivery predicate: twofa_delivery_dev.go (`dev || test`) always reports a // delivery channel (the [2FA] log relay), so the 503 "2FA requires an email or -// SMS delivery channel" branch (twofa.go:193) is UNREACHABLE in this build. -// The production predicate — TWO_FACTOR_ALLOW_LOG_DELIVERY gating — is covered -// by twofa_delivery_prod_test.go in a !dev build (see its header for the +// SMS delivery channel" branch is UNREACHABLE in this build. The production +// predicate — TWO_FACTOR_ALLOW_LOG_DELIVERY gating — is covered by +// twofa_delivery_prod_test.go in a !dev build (see its header for the // documented limitation). func TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue(t *testing.T) { require.True(t, twoFADeliveryAvailable()) diff --git a/backend/handlers/payments/vat_test.go b/backend/handlers/payments/vat_test.go index 045f141..34affc3 100644 --- a/backend/handlers/payments/vat_test.go +++ b/backend/handlers/payments/vat_test.go @@ -3537,3 +3537,42 @@ func TestGetVATConfig_QueryError(t *testing.T) { assert.Error(t, err) assert.Nil(t, cfg) } + +// TestApplyVATToTillSale_Idempotent locks the M5 safety property the sweep +// rescue relies on: apply_vat_to_till_sale is guarded on vat_amount IS NULL, so +// applying VAT to a till sale that already has VAT is a no-op — a rescue that +// re-applies VAT inside its transaction (the sweep rescues rows that may carry +// VAT from a prior synchronous completion attempt) can never double-book it. +func TestApplyVATToTillSale_Idempotent(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + if _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00, voucher_type = 'SPV'`); err != nil { + t.Fatalf("failed to update business_settings: %v", err) + } + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + + var saleID string + if err := tx.QueryRow(ctx, ` + INSERT INTO till_sales (item_type, description, quantity, unit_price, total_amount, payment_method, status, created_by, created_at, updated_at) + VALUES ('gift_card', 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending', $1, NOW(), NOW()) + RETURNING id + `, adminID).Scan(&saleID); err != nil { + t.Fatalf("failed to seed till sale: %v", err) + } + + ApplyVATToTillSale(ctx, tx, saleID) + ApplyVATToTillSale(ctx, tx, saleID) + + var vatAmount, netAmount sql.NullFloat64 + var isVATApplicable bool + if err := tx.QueryRow(ctx, `SELECT is_vat_applicable, vat_amount, net_amount FROM till_sales WHERE id = $1`, saleID).Scan(&isVATApplicable, &vatAmount, &netAmount); err != nil { + t.Fatalf("failed to query till sale VAT fields: %v", err) + } + if !isVATApplicable || !vatAmount.Valid || vatAmount.Float64 != 8.33 || !netAmount.Valid || netAmount.Float64 != 41.67 { + t.Errorf("expected the twice-applied VAT to stay single (8.33/41.67), got applicable=%v vat=%v net=%v", isVATApplicable, vatAmount, netAmount) + } +} diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 5ef48d8..b6f7a58 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -10,6 +10,7 @@ import ( "log" "math/big" "net/http" + "os" "time" "crussell/clock" @@ -60,6 +61,53 @@ const twoFAPepperEnv = "TWO_FACTOR_PEPPER" // channel and never return it (see twofa_dev.go). var errTwoFADeliveryUnavailable = errors.New("2FA requires an email or SMS delivery channel; contact the salon") +// twoFAAllowLogDeliveryEnv is the explicit operator opt-in that makes a +// production build deliver 2FA codes via the server log ([2FA] prefix) — the +// documented INSECURE stand-in for the not-yet-wired email/SMS transport (P6). +// Production builds fail closed without it (see twoFAEnsureIssueAllowedStrict); +// dev/test builds always deliver via the log and never consult this flag. +const twoFAAllowLogDeliveryEnv = "TWO_FACTOR_ALLOW_LOG_DELIVERY" + +// errTwoFAPepperRequired is returned when TWO_FACTOR_PEPPER is unset in a +// production-style issuance gate. Refusing to issue is the only safe outcome: +// without the pepper a pending code would be persisted as an unsalted SHA-256 +// digest in the 1M code space, which a log/DB leak could brute-force offline. +var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") + +// twoFAPepperConfigured reports whether TWO_FACTOR_PEPPER is set — the pure, +// build-agnostic read behind every strict issuance gate. Dev/test builds fall +// back to the legacy unsalted SHA-256 digest when it is unset (twofa_dev.go); +// production-style issuance refuses instead (twoFAEnsureIssueAllowedStrict). +func twoFAPepperConfigured() bool { return os.Getenv(twoFAPepperEnv) != "" } + +// twoFADeliveryChannelConfigured reports whether the deployment has explicitly +// configured a 2FA code delivery channel: TWO_FACTOR_ALLOW_LOG_DELIVERY set to +// exactly "true" (the only production channel today — email/SMS unwired, P6). +// Pure env read, build-agnostic: the build-tagged twoFADeliveryAvailable +// (twofa_dev.go / twofa_prod.go) is the runtime-facing wrapper that turns this +// into the always-true dev channel or the prod env check. +func twoFADeliveryChannelConfigured() bool { return os.Getenv(twoFAAllowLogDeliveryEnv) == "true" } + +// twoFAEnsureIssueAllowedStrict is the pure, build-agnostic production-style +// issuance gate: code issuance is allowed ONLY when BOTH TWO_FACTOR_PEPPER is +// set (an unsalted digest in the 1M code space would be offline-brute-forceable) +// AND a delivery channel is configured (otherwise a minted code could never +// reach the user and would silently dead-end the enforced saved-card-payments +// gate). Either way it fails closed with the actionable errors the handlers map +// to a 503. The build-tagged twoFAEnsureIssueAllowed wraps it for production +// builds; dev/test builds always allow issuance and never consult it — but the +// test,dev suite exercises THIS function directly, so the fail-closed branches +// are CI-visible even though the prod file (!dev && !test) is excluded there. +func twoFAEnsureIssueAllowedStrict() error { + if !twoFAPepperConfigured() { + return errTwoFAPepperRequired + } + if !twoFADeliveryChannelConfigured() { + return errTwoFADeliveryUnavailable + } + return nil +} + // twoFAPepper reads TWO_FACTOR_PEPPER (plus the one-time unset warning) and // twoFAEnsureIssueAllowed guards code issuance; both are build-dependent. // Dev/test builds keep the documented loose-fake fallback (twofa_dev.go); diff --git a/backend/handlers/user/twofa_failclosed_test.go b/backend/handlers/user/twofa_failclosed_test.go new file mode 100644 index 0000000..8bf0c4a --- /dev/null +++ b/backend/handlers/user/twofa_failclosed_test.go @@ -0,0 +1,67 @@ +//go:build test + +package user + +// M17 follow-up (ITEM 2): twofa_prod.go is excluded from the test,dev suite +// (`!dev && !test`), so its fail-closed branches were never exercised in CI — +// the old twofa_prod_test.go only runs its assertions in a genuine production +// build and skips under the test tag. The pure decision logic now lives +// build-agnostically in twofa.go (twoFAEnsureIssueAllowedStrict / +// twoFAPepperConfigured / twoFADeliveryChannelConfigured); these tests exercise +// those branches in the STANDARD test,dev run, so a regression in the prod +// fail-closed behaviour is CI-visible even though the prod file itself is only +// compiled in a genuine production build. + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestTwoFAEnsureIssueAllowedStrict_FailClosed pins the production-style +// issuance gate that twofa_prod.go's twoFAEnsureIssueAllowed delegates to: +// (a) pepper unset → issuance refused (errTwoFAPepperRequired — an unsalted +// digest in the 1M code space would be offline-brute-forceable); +// (b) delivery channel absent → issuance refused (errTwoFADeliveryUnavailable — +// the 503-style error the handlers surface as StatusServiceUnavailable); +// (c) both configured → issuance succeeds. +func TestTwoFAEnsureIssueAllowedStrict_FailClosed(t *testing.T) { + t.Run("pepper_unset_refuses_issuance", func(t *testing.T) { + t.Setenv(twoFAPepperEnv, "") + t.Setenv(twoFAAllowLogDeliveryEnv, "true") + require.ErrorIs(t, twoFAEnsureIssueAllowedStrict(), errTwoFAPepperRequired) + }) + + t.Run("delivery_channel_absent_refuses_issuance", func(t *testing.T) { + t.Setenv(twoFAPepperEnv, "test-pepper") + t.Setenv(twoFAAllowLogDeliveryEnv, "") + require.ErrorIs(t, twoFAEnsureIssueAllowedStrict(), errTwoFADeliveryUnavailable) + }) + + t.Run("pepper_and_channel_present_allows_issuance", func(t *testing.T) { + t.Setenv(twoFAPepperEnv, "test-pepper") + t.Setenv(twoFAAllowLogDeliveryEnv, "true") + require.NoError(t, twoFAEnsureIssueAllowedStrict()) + }) +} + +// TestTwoFADeliveryChannelConfigured pins the pure delivery-channel predicate +// behind the 503 refusal: only the exact value "true" opens the channel. +func TestTwoFADeliveryChannelConfigured(t *testing.T) { + t.Setenv(twoFAPepperEnv, "test-pepper") + for _, v := range []string{"", "1", "yes", "on", "True", "TRUE", "false"} { + t.Setenv(twoFAAllowLogDeliveryEnv, v) + require.False(t, twoFADeliveryChannelConfigured(), "value %q must NOT open the delivery channel (exact 'true' only)", v) + } + t.Setenv(twoFAAllowLogDeliveryEnv, "true") + require.True(t, twoFADeliveryChannelConfigured()) +} + +// TestTwoFAPepperConfigured pins the pure pepper predicate behind the +// pepper-required refusal. +func TestTwoFAPepperConfigured(t *testing.T) { + t.Setenv(twoFAPepperEnv, "") + require.False(t, twoFAPepperConfigured()) + t.Setenv(twoFAPepperEnv, "test-pepper") + require.True(t, twoFAPepperConfigured()) +} diff --git a/backend/handlers/user/twofa_prod.go b/backend/handlers/user/twofa_prod.go index 7da1788..ce572a8 100644 --- a/backend/handlers/user/twofa_prod.go +++ b/backend/handlers/user/twofa_prod.go @@ -25,29 +25,14 @@ package user import ( "crussell/internal/twofa" - "errors" "log" "os" ) -// twoFAAllowLogDeliveryEnv is the explicit operator opt-in that makes this -// production build deliver 2FA codes via the server log ([2FA] prefix) — the -// documented INSECURE stand-in for the not-yet-wired email/SMS transport (P6). -// Production builds fail closed without it: no delivery channel is configured, -// so code issuance is refused (see twoFAEnsureIssueAllowed) and setup surfaces -// errTwoFADeliveryUnavailable. Set it ONLY to keep the operator-relays-the-code -// flow working in a deployment that understands the risk (anyone with backend -// log access can defeat the 2FA gate on saved-card charges). Defined here in -// the prod build only — dev/test builds always deliver via the log and never -// consult this flag. -const twoFAAllowLogDeliveryEnv = "TWO_FACTOR_ALLOW_LOG_DELIVERY" - -// errTwoFAPepperRequired is returned by twoFAEnsureIssueAllowed when -// TWO_FACTOR_PEPPER is unset in a production build. Refusing to issue is the -// only safe outcome: without the pepper a pending code would be persisted as an -// unsalted SHA-256 digest in the 1M code space, which a log/DB leak could -// brute-force offline. -var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") +// twoFAAllowLogDeliveryEnv (the TWO_FACTOR_ALLOW_LOG_DELIVERY opt-in) and +// errTwoFAPepperRequired are defined in twofa.go — shared by the pure issuance +// gate (twoFAEnsureIssueAllowedStrict), which the test,dev suite exercises +// directly, and this production build. // init registers the production pepper reader into the shared verification // core (crussell/internal/twofa): raw env read, no fallback — code issuance @@ -62,9 +47,11 @@ func init() { // insecure log-delivery mode (TWO_FACTOR_ALLOW_LOG_DELIVERY=true) or a real // email/SMS transport is wired (not yet — P6). Default false: no channel, so // code issuance is refused and setup surfaces errTwoFADeliveryUnavailable -// instead of a silent dead-end. +// instead of a silent dead-end. Delegates to the pure build-agnostic +// twoFADeliveryChannelConfigured (twofa.go); dev/test builds always return true +// (twofa_dev.go). func twoFADeliveryAvailable() bool { - return os.Getenv(twoFAAllowLogDeliveryEnv) == "true" + return twoFADeliveryChannelConfigured() } // twoFAEnsureIssueAllowed reports whether a 2FA code may be issued in this @@ -73,8 +60,9 @@ func twoFADeliveryAvailable() bool { // code could never reach the user — issuing one would silently lock the user // out of the enforced saved-card-payments gate; and without the pepper every // stored code would be an offline-brute-forceable unsalted digest. Either way -// issuance is refused (fail-closed). Dev/test builds always allow issuance -// (twofa_dev.go). +// issuance is refused (fail-closed). Delegates to the pure build-agnostic gate +// twoFAEnsureIssueAllowedStrict (twofa.go), which the test,dev suite also +// exercises directly; dev/test builds always allow issuance (twofa_dev.go). // // The pepper check is the ONLY hard gate here (plus the delivery channel), and // it is also the ONLY hard gate on the payments re-issue path @@ -86,13 +74,7 @@ func twoFADeliveryAvailable() bool { // (or have each user re-run 2FA setup), or enforced saved-card charges will // strand customers with 400 ErrMissingOrExpired forever. func twoFAEnsureIssueAllowed() error { - if os.Getenv(twoFAPepperEnv) == "" { - return errTwoFAPepperRequired - } - if !twoFADeliveryAvailable() { - return errTwoFADeliveryUnavailable - } - return nil + return twoFAEnsureIssueAllowedStrict() } // twoFADeliverCode delivers a fresh verification code to the user. Production diff --git a/backend/handlers/user/twofa_prod_test.go b/backend/handlers/user/twofa_prod_test.go new file mode 100644 index 0000000..a21855b --- /dev/null +++ b/backend/handlers/user/twofa_prod_test.go @@ -0,0 +1,79 @@ +//go:build !dev + +package user + +// Tests for the PRODUCTION 2FA issuance gate (twofa_prod.go). +// +// LIMITATION (documented — M17): twofa_prod.go is compiled only in a genuine +// production build (`!dev && !test`). Under BOTH required test runs — the +// "test,dev" run and the "test,!dev" prod-shape run — the dev/test variant +// (twofa_dev.go, build tag `dev || test`) is the compiled function and its +// fail-closed branches (no TWO_FACTOR_PEPPER → refuse; no delivery channel → +// 503) are unreachable. These tests compile in every `!dev` build and run +// their assertions ONLY when the prod variant marker reports the real prod +// functions are live; under the test tag they skip with the same documented +// rationale the payments package uses (twofa_delivery_prod_test.go). + +import ( + "os" + "testing" +) + +// twoFAAllowLogDeliveryEnv is only defined in twofa_prod.go (!dev && !test); +// use the literal env name so this test also compiles under `test,!dev`. +const allowLogDeliveryEnv = "TWO_FACTOR_ALLOW_LOG_DELIVERY" + +// TestTwoFAEnsureIssueAllowed_ProdPredicate pins the production issuance gate: +// it fails closed without TWO_FACTOR_PEPPER (an unsalted digest in the 1M code +// space would be offline-brute-forceable) or without a delivery channel, and +// allows issuance only when both are configured. +func TestTwoFAEnsureIssueAllowed_ProdPredicate(t *testing.T) { + if !twofaProdVariant { + t.Skip("twoFAEnsureIssueAllowed() is the dev/test build's always-allowed variant (twofa_dev.go, `dev || test`); the prod fail-closed branches are unreachable under the test tag — see the file header for the documented limitation") + } + + os.Unsetenv(twoFAPepperEnv) + os.Unsetenv(allowLogDeliveryEnv) + if err := twoFAEnsureIssueAllowed(); err == nil { + t.Error("expected issuance refused without TWO_FACTOR_PEPPER in a production build") + } else if err.Error() != "TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)" { + t.Errorf("expected the pepper-required error without the pepper, got %v", err) + } + + os.Setenv(twoFAPepperEnv, "test-pepper") + os.Unsetenv(allowLogDeliveryEnv) + if err := twoFAEnsureIssueAllowed(); err == nil { + t.Error("expected issuance refused without a delivery channel in a production build") + } else if err != errTwoFADeliveryUnavailable { + t.Errorf("expected errTwoFADeliveryUnavailable without a channel, got %v", err) + } + + os.Setenv(allowLogDeliveryEnv, "true") + if err := twoFAEnsureIssueAllowed(); err != nil { + t.Errorf("expected issuance allowed with both the pepper and a delivery channel, got %v", err) + } +} + +// TestTwoFADeliveryAvailable_ProdPredicate pins the production delivery +// predicate: TWO_FACTOR_ALLOW_LOG_DELIVERY unset → no channel (false), exactly +// "true" → channel (true), any other value → no channel. +func TestTwoFADeliveryAvailable_ProdPredicate(t *testing.T) { + if !twofaProdVariant { + t.Skip("twoFADeliveryAvailable() is the dev/test build's trivially-true variant (twofa_dev.go, `dev || test`); the 503 delivery-unavailable branch is unreachable under the test tag — see the file header for the documented limitation") + } + + os.Unsetenv(allowLogDeliveryEnv) + if twoFADeliveryAvailable() { + t.Error("production without the explicit opt-in must have NO 2FA delivery channel") + } + for _, v := range []string{"", "1", "yes", "on", "True", "TRUE", "false"} { + os.Setenv(allowLogDeliveryEnv, v) + if twoFADeliveryAvailable() { + t.Errorf("value %q must NOT open the delivery channel (exact 'true' only)", v) + } + } + os.Setenv(allowLogDeliveryEnv, "true") + if !twoFADeliveryAvailable() { + t.Error("the explicit insecure log-delivery opt-in must open the channel") + } +} diff --git a/backend/handlers/user/twofa_prod_variant_dev_test.go b/backend/handlers/user/twofa_prod_variant_dev_test.go new file mode 100644 index 0000000..acb1df3 --- /dev/null +++ b/backend/handlers/user/twofa_prod_variant_dev_test.go @@ -0,0 +1,11 @@ +//go:build dev || test + +package user + +// twofaProdVariant reports whether the PRODUCTION 2FA issuance/delivery +// functions (twofa_prod.go, !dev && !test) are the compiled ones in this +// build. Under `dev` OR `test` tags the dev/test variants (twofa_dev.go) are +// compiled instead — always-allowed issuance, trivially-true delivery — so +// the prod fail-closed branches are unreachable there. +//lint:ignore U1000 referenced only from the prod-tag test (twofa_prod_test.go, !dev && !test); deliberately unused under dev/test tags +const twofaProdVariant = false diff --git a/backend/handlers/user/twofa_prod_variant_prod_test.go b/backend/handlers/user/twofa_prod_variant_prod_test.go new file mode 100644 index 0000000..c47f238 --- /dev/null +++ b/backend/handlers/user/twofa_prod_variant_prod_test.go @@ -0,0 +1,8 @@ +//go:build !dev && !test + +package user + +// twofaProdVariant reports whether the PRODUCTION 2FA issuance/delivery +// functions (twofa_prod.go, !dev && !test) are the compiled ones in this +// build. True only in a genuine production build — neither dev nor test tag. +const twofaProdVariant = true diff --git a/backend/main.go b/backend/main.go index ef5311c..e681446 100644 --- a/backend/main.go +++ b/backend/main.go @@ -191,11 +191,11 @@ func initSquare() { // 2FA enforcement is fail-closed (see payments.twoFactorEnforced): it is // OFF only when REQUIRE_2FA explicitly disables it (false/0/off/no, // case-insensitive) or SQUARE_ENVIRONMENT explicitly selects the dev/mock - // stack. Warn loudly when a non-dev env (empty/unknown — a likely - // misconfiguration) leaves the gate disabled, so saved-card charges can - // never silently ship without this merchant-level authorization gate (an - // additional fraud control; NOT PSD2 SCA — Square buyer verification is the - // SCA mechanism, wired for new-card charges). + // stack. In an enforced env a token-less saved-card charge is refused 402 + // verification_required (SCA-only — see the posture note below); in a + // dev/mock env the gate lifts and the mock simulates SCA. Warn loudly when + // a non-dev env (empty/unknown — a likely misconfiguration) leaves the gate + // disabled. enforced := payments.NewPaymentService().TwoFactorEnforced() if enforced { // Delivery is build-dependent (handlers/user/twofa_dev.go / @@ -206,24 +206,26 @@ func initSquare() { // misled into thinking codes are reaching users when issuance is // actually failing closed. if os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { - log.Printf("WARNING: 2FA codes are delivered in PLAINTEXT via the server log ([2FA] prefix) — anyone with backend log access can defeat the 2FA gate. Restrict log access and relay codes out-of-band; replace this loose-fake delivery with email/SMS (P6) before launch.") + log.Printf("WARNING: 2FA codes are delivered in PLAINTEXT via the server log ([2FA] prefix) — anyone with backend log access can defeat the account 2FA gate. Restrict log access and relay codes out-of-band; replace this loose-fake delivery with email/SMS (P6) before launch.") } else { - log.Printf("WARNING: 2FA enforcement is ON but TWO_FACTOR_ALLOW_LOG_DELIVERY is unset: in a production build there is NO code-delivery channel (email/SMS is not wired — P6), so 2FA code issuance FAILS CLOSED and no user can complete setup or disable. Every enforced saved-card online payment for a user without 2FA will 403 with no way to enable it. Set TWO_FACTOR_ALLOW_LOG_DELIVERY=true to opt into the insecure [2FA] log-delivery channel (plaintext codes in the server log — restrict log access), or wire email/SMS (P6).") + log.Printf("WARNING: 2FA enforcement is ON but TWO_FACTOR_ALLOW_LOG_DELIVERY is unset: in a production build there is NO code-delivery channel (email/SMS is not wired — P6), so 2FA code issuance FAILS CLOSED and no user can complete 2FA setup or disable. Set TWO_FACTOR_ALLOW_LOG_DELIVERY=true to opt into the insecure [2FA] log-delivery channel (plaintext codes in the server log — restrict log access), or wire email/SMS (P6).") } } if !enforced && !payments.IsExplicitDevOrMockEnv() { log.Printf("WARNING: 2FA enforcement is OFF (REQUIRE_2FA=%q) with SQUARE_ENVIRONMENT=%q (not an explicit mock/dev value). Online saved-card payments will NOT require 2FA.", os.Getenv("REQUIRE_2FA"), env) } - // SCA-primary / 2FA-backup posture (TWO_FACTOR_FALLBACK, default "true"): - // the homegrown 2FA gate is the BACKUP authorization for saved-card charges - // that carry no Square verification_token. TWO_FACTOR_FALLBACK=false makes - // the deployment SCA-only: a token-less saved-card charge is then rejected - // 402 verification_required (the frontend shows the 3DS challenge; if the - // bank cannot do SCA the payment cannot proceed). Warn when the operator - // opted out so the SCA-only posture is a deliberate, visible choice. - if !payments.NewPaymentService().TwoFactorFallbackEnabled() && !payments.IsExplicitDevOrMockEnv() { - log.Printf("WARNING: TWO_FACTOR_FALLBACK=false with SQUARE_ENVIRONMENT=%q — the homegrown 2FA is DISABLED as a saved-card charge fallback. Only charges carrying a Square verification_token (SCA performed) can proceed; a token-less charge is rejected 402 verification_required.", env) - } + // SCA-only posture (permanent — no switch): saved-card charges are + // authorised exclusively by Square PSD2 SCA. PSR 2017 reg 100 makes SCA + // mandatory and non-waivable for customer-initiated stored-credential + // charges, and the homegrown 2FA cannot legally act as an SCA fallback: a + // token-less saved-card charge is refused 402 verification_required ("pay + // online later") — a fallback-authorised charge would leave the MERCHANT + // liable for ECI 7 / SLI 210 chargebacks and PSR 2017 reg 77(6) + // compensation, and consent does not cure it. The dev mock simulates SCA + // (SimulateSavedCardVerificationRequired + cnon:sca-... tokenize-results), + // so development has full parity. There is no TWO_FACTOR_FALLBACK switch to + // warn about — it was removed; a token-less charge can never be authorised + // by 2FA in any environment. // The mirror-image confusion: enforcement is ON but the Square client fell // back to the in-memory mock (internal/square.NewDevClient only picks the // real API for sandbox/production) because SQUARE_ENVIRONMENT is empty or