From 858ae87e9c363e4e835b763f7fa8215ca6d09798 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Tue, 4 Aug 2026 22:43:05 +0100 Subject: [PATCH] Block card saving for unverified accounts at the charge-handler level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only verified accounts (account_role in 'verified_email','admin') may save cards. CreateBookingPayment, CreateTipPayment and BuyGiftCard now reject save_card=true for guests, unverified accounts and affiliates with 403 BEFORE charge-source resolution, the pending-payment insert, or any Square call — failing closed with zero side effects. Unverified users may still pay; only card persistence is blocked. The dedicated save endpoints are additionally protected by mw.RequireVerified middleware on the routes (main.go). isVerifiedRole / rejectSaveCardForUnverified mirror the existing isAdminRequest defense-in-depth pattern. Tests cover: unverified save-card 403 with no payment row, verified save-card succeeds, and unverified pay-without-save succeeds. --- backend/handlers/payments/giftcards.go | 8 + backend/handlers/payments/handlers.go | 38 +++ .../payments/save_card_verified_test.go | 243 ++++++++++++++++++ 3 files changed, 289 insertions(+) create mode 100644 backend/handlers/payments/save_card_verified_test.go diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 04985c2..fec1f45 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -983,6 +983,14 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { http.Error(w, "Invalid request", http.StatusBadRequest) return } + + // Product rule (security): only verified accounts may save cards. An + // unverified/guest/affiliate user may still buy a gift card, but + // save_card=true is rejected here — before any charge source resolution. + if rejectSaveCardForUnverified(w, r, req.SaveCard) { + return + } + if req.IdempotencyKey == "" { req.IdempotencyKey = uniqueChargeKey("gc-") } diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index cbb7a21..c409b2a 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -148,6 +148,30 @@ func isAdminRequest(r *http.Request) bool { return ok && role == "admin" } +// isVerifiedRole reports whether the authenticated user may save cards +// (account_role in ('verified_email','admin')). Guests, unverified accounts, +// and affiliates may still pay but must never persist a card. +func isVerifiedRole(r *http.Request) bool { + role, ok := r.Context().Value(mw.UserRoleKey).(string) + return ok && (role == "verified_email" || role == "admin") +} + +// rejectSaveCardForUnverified enforces the save-card product rule at the +// handler level for charge endpoints. When a charge request carries +// save_card=true for a non-verified user it responds 403 and returns true so +// the caller aborts BEFORE resolveChargeSource (where the card would be +// persisted), the pending payment record insert, or the Square call — failing +// closed with no side effects. A non-verified user may still pay; only card +// persistence is blocked. The dedicated save endpoints are additionally +// protected by mw.RequireVerified middleware (main.go). +func rejectSaveCardForUnverified(w http.ResponseWriter, r *http.Request, saveCard bool) bool { + if saveCard && !isVerifiedRole(r) { + http.Error(w, "Only verified accounts can save a card", http.StatusForbidden) + return true + } + return false +} + // GetDiscountPreviewHandler returns eligible discounts for a booking without applying them. // GET /api/bookings/{id}/discount-preview func GetDiscountPreviewHandler(w http.ResponseWriter, r *http.Request) { @@ -1194,6 +1218,13 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } + // Product rule (security): only verified accounts may save cards. An + // unverified/guest/affiliate user may still pay, but save_card=true is + // rejected here — before any charge source resolution or payment record. + if rejectSaveCardForUnverified(w, r, req.SaveCard) { + return + } + // Resolve buyer email for Square receipt delivery (failure is non-fatal). var bookingBuyerEmail string if err := db.Conn.QueryRow(r.Context(), `SELECT email FROM users WHERE id = $1`, userID).Scan(&bookingBuyerEmail); err != nil { @@ -2555,6 +2586,13 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } + // Product rule (security): only verified accounts may save cards. An + // unverified/guest/affiliate user may still tip, but save_card=true is + // rejected here — before any charge source resolution or payment record. + if rejectSaveCardForUnverified(w, r, req.SaveCard) { + return + } + if err := ValidateAmount(req.Amount); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) diff --git a/backend/handlers/payments/save_card_verified_test.go b/backend/handlers/payments/save_card_verified_test.go new file mode 100644 index 0000000..a43f139 --- /dev/null +++ b/backend/handlers/payments/save_card_verified_test.go @@ -0,0 +1,243 @@ +//go:build test && dev + +package payments + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/mw" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/require" +) + +// Product rule (security): cards may only be saved by verified accounts +// (account_role in ('verified_email','admin')). Guests, unverified accounts, +// and affiliates may still pay, but must never persist a card. These tests +// cover both enforcement layers: the RequireVerified middleware on the +// dedicated payment-methods routes, and the handler-level save_card gate on +// the charge endpoints (CreateBookingPayment et al). + +// serveThroughVerifiedChain invokes a handler through the same middleware +// stack the payment-methods routes are mounted under in main.go +// (RequireAuth → RequireVerified), carrying a real JWT, so the test exercises +// the actual route wiring rather than a bare handler call. +func serveThroughVerifiedChain(t *testing.T, handler http.Handler, method, path string, body interface{}, token string, ctx context.Context) *httptest.ResponseRecorder { + t.Helper() + var req *http.Request + if body != nil { + bodyBytes, err := json.Marshal(body) + require.NoError(t, err) + req = httptest.NewRequest(method, path, bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + } else { + req = httptest.NewRequest(method, path, nil) + } + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + + rctx := chi.NewRouteContext() + if id, paramName := extractPaymentIDFromPath(path); id != "" { + rctx.URLParams.Add(paramName, id) + } + req = req.WithContext(context.WithValue(ctx, chi.RouteCtxKey, rctx)) + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + return w +} + +// TestPaymentMethodsRoutes_RequireVerified verifies the mw.RequireVerified +// middleware on all three payment-methods routes: GET/POST/DELETE are all 403 +// for an unverified user (before any handler code runs), while a verified user +// still reaches the handler. +func TestPaymentMethodsRoutes_RequireVerified(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + unverifiedID, err := fixtures.CreateTestUnverifiedUser(tx) + require.NoError(t, err) + unverifiedToken := jwt.GenerateUnverifiedUserToken(unverifiedID) + + cardID, err := fixtures.CreateTestPaymentMethod(tx, unverifiedID, "ccof:unverified_saved", "VISA", "1111") + require.NoError(t, err) + + verifiedID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + verifiedToken := jwt.GenerateUserToken(verifiedID) + + // POST /user/payment-methods is the primary save path — verify no card is + // persisted when an unverified user's request is rejected. + saveCardPath := "/api/user/payment-methods" + + cases := []struct { + name string + method string + path string + body interface{} + token string + want int + }{ + {"GET unverified", http.MethodGet, "/api/user/payment-methods", nil, unverifiedToken, http.StatusForbidden}, + {"POST unverified save blocked", http.MethodPost, saveCardPath, CreatePaymentMethodRequest{CardToken: "cnon:unverified-save"}, unverifiedToken, http.StatusForbidden}, + {"DELETE unverified", http.MethodDelete, "/api/user/payment-methods/" + cardID, nil, unverifiedToken, http.StatusForbidden}, + {"GET verified passes through", http.MethodGet, "/api/user/payment-methods", nil, verifiedToken, http.StatusOK}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var route http.Handler + switch tc.method { + case http.MethodGet: + route = mw.RequireAuth(mw.RequireVerified(http.HandlerFunc(GetUserPaymentMethods))) + case http.MethodPost: + route = mw.RequireAuth(mw.RequireVerified(http.HandlerFunc(CreatePaymentMethod))) + case http.MethodDelete: + route = mw.RequireAuth(mw.RequireVerified(http.HandlerFunc(DeletePaymentMethod))) + } + w := serveThroughVerifiedChain(t, route, tc.method, tc.path, tc.body, tc.token, ctx) + require.Equal(t, tc.want, w.Code, "body: %s", w.Body.String()) + }) + } + + var savedCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, unverifiedID).Scan(&savedCount) + require.NoError(t, err) + require.Equal(t, 1, savedCount, "only the fixture card exists — the rejected save attempt must not create one") +} + +// setupTestDataForUserAt mirrors setupTestDataAtTime but for a caller-supplied +// user (so an unverified or otherwise non-default-role user owns the booking). +func setupTestDataForUserAt(t *testing.T, ctx context.Context, q db.Querier, userID string, startTime time.Time) (string, string) { + t.Helper() + serviceID, err := fixtures.CreateTestService(q) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(q, userID, serviceID, startTime) + require.NoError(t, err) + _, err = q.Exec(ctx, "UPDATE bookings SET status = 'in_progress' WHERE id = $1", bookingID) + require.NoError(t, err) + return bookingID, serviceID +} + +// TestCreateBookingPayment_SaveCard_UnverifiedForbidden verifies the +// handler-level save-card gate: an unverified user sending save_card=true on a +// booking payment gets 403 with NO side effects — no payment record and no +// saved card, and no charge reaching Square. +func TestCreateBookingPayment_SaveCard_UnverifiedForbidden(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + unverifiedID, err := fixtures.CreateTestUnverifiedUser(tx) + require.NoError(t, err) + + bookingID, _ := setupTestDataForUserAt(t, ctx, tx, unverifiedID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + + unverifiedToken := jwt.GenerateUnverifiedUserToken(unverifiedID) + + cardToken := "cnon:unverified-save-attempt" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "unverified-save-card-key-" + bookingID, + } + + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, unverifiedToken, ctx) + + require.Equal(t, http.StatusForbidden, w.Code, "body: %s", w.Body.String()) + + var paymentCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&paymentCount) + require.NoError(t, err) + require.Zero(t, paymentCount, "no payment record may be created for the rejected save-card request") + + var cardCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, unverifiedID).Scan(&cardCount) + require.NoError(t, err) + require.Zero(t, cardCount, "no card may be persisted for the rejected save-card request") +} + +// TestCreateBookingPayment_SaveCard_VerifiedSucceeds verifies the gate does +// not disturb verified users: the same save_card=true request from a +// verified_email account charges AND persists the card, exactly as before. +func TestCreateBookingPayment_SaveCard_VerifiedSucceeds(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:verified-save-card" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "verified-save-card-key-" + bookingID, + } + + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var resp PaymentResponse + require.NoError(t, parsePaymentResponseBody(w, &resp)) + require.Equal(t, "completed", resp.Status) + + var cardCount int + err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount) + require.NoError(t, err) + require.Equal(t, 1, cardCount, "verified user's card must still be persisted") +} + +// TestCreateBookingPayment_NoSaveCard_UnverifiedSucceeds verifies the gate +// does not block payment for non-verified users: an unverified user paying +// WITHOUT save_card still charges successfully and saves nothing. +func TestCreateBookingPayment_NoSaveCard_UnverifiedSucceeds(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + unverifiedID, err := fixtures.CreateTestUnverifiedUser(tx) + require.NoError(t, err) + + bookingID, _ := setupTestDataForUserAt(t, ctx, tx, unverifiedID, clock.Now().Add(-1*time.Hour)) + + unverifiedToken := jwt.GenerateUnverifiedUserToken(unverifiedID) + + cardToken := "cnon:unverified-one-off" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + SaveCard: false, + IdempotencyKey: "unverified-one-off-key-" + bookingID, + } + + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, unverifiedToken, ctx) + + require.Equal(t, http.StatusOK, w.Code, "payment must not be blocked for unverified users paying without save_card: %s", w.Body.String()) + + var paymentCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&paymentCount) + require.NoError(t, err) + require.Equal(t, 1, paymentCount, "the unverified user's one-off payment must be recorded") + + var cardCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, unverifiedID).Scan(&cardCount) + require.NoError(t, err) + require.Zero(t, cardCount, "no card may be saved when save_card is false") +}