From 2b4c50b4b0d706081c7b9047e3c2422b7d8a7bb9 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Tue, 4 Aug 2026 22:43:37 +0100 Subject: [PATCH] Block guest-role tokens from online payment routes (RequireNonGuest middleware) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guests (account_role='guest') have no login flow and never receive a JWT normally, so this is defense-in-depth: any token whose role claim is 'guest' (forged/minted guest tokens or future changes) is refused 403 before the money handlers run. The check reads role from context ONLY — real guests are seeded with account_type='email', so account_type is never the discriminator. A missing role passes through (RequireAuth guarantees presence; same trust model as isVerifiedRole). Wired onto all user-facing money routes: booking payment, apply-redemption, payment-lock POST/DELETE, tip, gift-card redeem and gift-card buy. Admin and till routes (RequireAdmin) are untouched — admin can never be guest. The payment-methods routes gain RequireVerified (verified_email, admin) alongside, so only verified accounts can manage saved cards. Tests: 6 middleware tests (reject guest, allow verified/unverified/admin/ affiliate/missing-role) + 4 integration tests (guest 403 on booking payment with zero side effects, tip, gift-card buy; verified user still pays 200). --- .../payments/guest_payment_block_test.go | 150 ++++++++++++++++++ backend/main.go | 26 +-- backend/mw/auth.go | 19 +++ backend/mw/auth_test.go | 75 +++++++++ 4 files changed, 260 insertions(+), 10 deletions(-) create mode 100644 backend/handlers/payments/guest_payment_block_test.go diff --git a/backend/handlers/payments/guest_payment_block_test.go b/backend/handlers/payments/guest_payment_block_test.go new file mode 100644 index 0000000..36f4da1 --- /dev/null +++ b/backend/handlers/payments/guest_payment_block_test.go @@ -0,0 +1,150 @@ +//go:build test && dev + +package payments + +import ( + "context" + "net/http" + "testing" + "time" + + "crussell/db" + "crussell/mw" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/require" +) + +// withNonGuest wraps a payment handler in the RequireNonGuest middleware, the +// same chain main.go mounts for user-facing money routes (after RequireAuth). +func withNonGuest(handler http.HandlerFunc) http.HandlerFunc { + return mw.RequireNonGuest(handler).ServeHTTP +} + +// createGuestBooking sets up a guest user with an in_progress booking so a +// charge would otherwise succeed if the guest guard did not block it. +func createGuestBooking(t *testing.T, ctx context.Context, q db.Querier) (string, string) { + guestID, err := fixtures.CreateTestGuestUser(q) + require.NoError(t, err) + + serviceID, err := fixtures.CreateTestService(q) + require.NoError(t, err) + + bookingID, err := fixtures.CreateTestBookingAtTime(q, guestID, serviceID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + _, err = q.Exec(ctx, "UPDATE bookings SET status = 'in_progress' WHERE id = $1", bookingID) + require.NoError(t, err) + + return guestID, bookingID +} + +func TestGuestBlocked_CreateBookingPayment(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + guestID, bookingID := createGuestBooking(t, ctx, tx) + guestToken := jwt.GenerateTestToken(guestID, "guest") + + cardToken := "cnon:test-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "guest-block-payment-key", + } + + handler := withNonGuest(CreateBookingPayment) + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, guestToken, ctx) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for guest payment, got %d. body: %s", w.Code, w.Body.String()) + } + + var count int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&count); err != nil { + t.Fatalf("failed to query payments: %v", err) + } + if count != 0 { + t.Errorf("expected 0 payment rows for blocked guest, got %d", count) + } +} + +func TestGuestBlocked_CreateTipPayment(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + guestID, bookingID := createGuestBooking(t, ctx, tx) + guestToken := jwt.GenerateTestToken(guestID, "guest") + + cardToken := "cnon:tip-card" + req := CreateTipPaymentRequest{ + Amount: 500, + NewCardToken: &cardToken, + } + + handler := withNonGuest(CreateTipPayment) + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, guestToken, ctx) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for guest tip, got %d. body: %s", w.Code, w.Body.String()) + } +} + +func TestGuestBlocked_BuyGiftCard(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + guestID, err := fixtures.CreateTestGuestUser(tx) + require.NoError(t, err) + guestToken := jwt.GenerateTestToken(guestID, "guest") + + newToken := "cnon:test-card" + req := BuyGiftCardRequest{ + Amount: 1000, + RecipientType: "self", + NewCardToken: &newToken, + IdempotencyKey: "guest-block-gc-key", + } + + handler := withNonGuest(BuyGiftCard) + w := makePaymentRequest(handler, "POST", "/api/user/giftcards/buy", req, guestToken, ctx) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for guest gift card purchase, got %d. body: %s", w.Code, w.Body.String()) + } +} + +func TestGuestBlocked_VerifiedUserStillPays(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:test-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "guest-block-verified-key", + } + + handler := withNonGuest(CreateBookingPayment) + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for verified user through RequireNonGuest, got %d. body: %s", w.Code, w.Body.String()) + } + + var count int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&count); err != nil { + t.Fatalf("failed to query payments: %v", err) + } + if count != 1 { + t.Errorf("expected 1 payment for verified user, got %d", count) + } +} diff --git a/backend/main.go b/backend/main.go index 0db3fd7..c240341 100644 --- a/backend/main.go +++ b/backend/main.go @@ -385,22 +385,28 @@ func main() { r.Delete("/bookings/reserve", bookings.CancelReservationHandler) // User payment routes - r.Post("/bookings/{id}/payment", payments.CreateBookingPayment) - r.Post("/bookings/{id}/apply-redemption", payments.ApplyLoyaltyRedemption) - r.Post("/bookings/{id}/payment-lock", payments.AcquirePaymentLock) - r.Delete("/bookings/{id}/payment-lock", payments.ReleasePaymentLock) - r.Get("/user/payment-methods", payments.GetUserPaymentMethods) - r.Post("/user/payment-methods", payments.CreatePaymentMethod) - r.Delete("/user/payment-methods/{id}", payments.DeletePaymentMethod) - r.Post("/bookings/{id}/tip", payments.CreateTipPayment) + // Product rule (security): guests (account_role='guest') must not + // pay online — RequireNonGuest 403s any token with a guest role + // claim before the money handlers run. + r.With(mw.RequireNonGuest).Post("/bookings/{id}/payment", payments.CreateBookingPayment) + r.With(mw.RequireNonGuest).Post("/bookings/{id}/apply-redemption", payments.ApplyLoyaltyRedemption) + r.With(mw.RequireNonGuest).Post("/bookings/{id}/payment-lock", payments.AcquirePaymentLock) + r.With(mw.RequireNonGuest).Delete("/bookings/{id}/payment-lock", payments.ReleasePaymentLock) + // Product rule (security): cards may only be saved by verified + // accounts — RequireAuth (group mw above) runs first and injects + // userID/role into ctx, then RequireVerified 403s the rest. + r.With(mw.RequireVerified).Get("/user/payment-methods", payments.GetUserPaymentMethods) + r.With(mw.RequireVerified).Post("/user/payment-methods", payments.CreatePaymentMethod) + r.With(mw.RequireVerified).Delete("/user/payment-methods/{id}", payments.DeletePaymentMethod) + r.With(mw.RequireNonGuest).Post("/bookings/{id}/tip", payments.CreateTipPayment) r.Get("/bookings/{id}/payment-summary", payments.GetBookingPaymentSummary) r.With(mw.RateLimit(10, time.Minute), limitBody(defaultBodyLimit)). Get("/bookings/{id}/discount-preview", payments.GetDiscountPreviewHandler) // User gift card routes - r.Post("/user/giftcards/redeem", payments.RedeemGiftCard) + r.With(mw.RequireNonGuest).Post("/user/giftcards/redeem", payments.RedeemGiftCard) r.Get("/user/giftcards/balance", payments.GetGiftCardBalance) - r.Post("/user/giftcards/buy", payments.BuyGiftCard) + r.With(mw.RequireNonGuest).Post("/user/giftcards/buy", payments.BuyGiftCard) }) r.With(mw.RequireAuth, mw.RequireVerified, limitBody(uploadBodyLimit)).Post("/user/profile-picture", user.UploadProfilePictureHandler) diff --git a/backend/mw/auth.go b/backend/mw/auth.go index 4ce1363..47ce572 100644 --- a/backend/mw/auth.go +++ b/backend/mw/auth.go @@ -95,6 +95,25 @@ func RequireVerified(next http.Handler) http.Handler { return RequireRole("verified_email", "admin")(next) } +// RequireNonGuest middleware - blocks guest-role tokens from money routes. +// Guests are created with account_role='guest' (account_type is NOT checked — +// real guests are seeded with account_type='email'), and they have no login +// flow, so a legitimate guest never holds a JWT. This is a defense-in-depth +// guard: any token whose role claim is 'guest' (forged/minted guest tokens or +// future changes) is refused. A missing role is treated as not-guest and passes +// through — RequireAuth guarantees the role is present when this runs, matching +// the trust model of the in-handler isVerifiedRole checks. +func RequireNonGuest(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + role, ok := r.Context().Value(UserRoleKey).(string) + if ok && role == "guest" { + RespondJSON(w, http.StatusForbidden, map[string]string{"error": "forbidden"}) + return + } + next.ServeHTTP(w, r) + }) +} + // RequireAdmin middleware - only allows admin and validates the user ID is present // in context, so handlers don't need to re-check. The user ID is always set by // RequireAuth before this runs, but this adds a defensive safety net. diff --git a/backend/mw/auth_test.go b/backend/mw/auth_test.go index 747f063..5895e56 100644 --- a/backend/mw/auth_test.go +++ b/backend/mw/auth_test.go @@ -307,6 +307,81 @@ func TestRequireVerified_Unverified(t *testing.T) { } } +// ============================================================================= +// RequireNonGuest (blocks account_role='guest' tokens from money routes) +// ============================================================================= + +func TestRequireNonGuest_AllowsVerifiedEmail(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + ctx := context.WithValue(req.Context(), UserRoleKey, "verified_email") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + RequireNonGuest(testHandler()).ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for verified_email, got %d", w.Code) + } +} + +func TestRequireNonGuest_AllowsUnverifiedEmail(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + ctx := context.WithValue(req.Context(), UserRoleKey, "unverified_email") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + RequireNonGuest(testHandler()).ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for unverified_email, got %d", w.Code) + } +} + +func TestRequireNonGuest_AllowsAdmin(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + ctx := context.WithValue(req.Context(), UserRoleKey, "admin") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + RequireNonGuest(testHandler()).ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for admin, got %d", w.Code) + } +} + +func TestRequireNonGuest_AllowsAffiliate(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + ctx := context.WithValue(req.Context(), UserRoleKey, "affiliate") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + RequireNonGuest(testHandler()).ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for affiliate, got %d", w.Code) + } +} + +func TestRequireNonGuest_RejectsGuest(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + ctx := context.WithValue(req.Context(), UserRoleKey, "guest") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + RequireNonGuest(testHandler()).ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for guest, got %d", w.Code) + } +} + +func TestRequireNonGuest_NoRoleInContext(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + // No UserRoleKey in context — treated as not-guest, passes through + w := httptest.NewRecorder() + RequireNonGuest(testHandler()).ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 when no role in context, got %d", w.Code) + } +} + // ============================================================================= // RequireAdmin (RequireRole("admin") + UserIDKey guard) // =============================================================================