diff --git a/.env.example b/.env.example index 6cf33f9..52900f5 100644 --- a/.env.example +++ b/.env.example @@ -37,6 +37,7 @@ R2_ENDPOINT= # Square Payment Gateway SQUARE_ACCESS_TOKEN= SQUARE_LOCATION_ID= +SQUARE_TERMINAL_DEVICE_ID= SQUARE_ENVIRONMENT=mock SQUARE_WEBHOOK_SIGNATURE_KEY= SQUARE_WEBHOOK_NOTIFICATION_URL= diff --git a/README.md b/README.md index c075269..db59baf 100644 --- a/README.md +++ b/README.md @@ -4,9 +4,9 @@ Nail salon booking platform — Go 1.26.5 backend + SvelteKit 5 SPA + PostgreSQL ## Features -**Booking**: Self-service (customer), walk-in (admin), call-in (admin). Slot reservations prevent double-booking (4 TTL types). **Self-blocking prevention**: `excludeUserID` parameter filters a user's own `RESERVATION` entries from time blocker overlap checks, allowing re-reservation and booking at overlapping slots. **Explicit cancellation**: `DELETE /api/bookings/reserve` releases a user reservation; `DELETE /api/admin/bookings/reserve` releases an admin walk-in/call-in reservation. **Background cleanup**: Centralised cron scheduler (`backend/internal/jobs/`) runs 21 maintenance jobs: reservation/deposit cleanup every 5min, hourly campaign transitions, daily unpaid-booking notifications, staged default hours auto-apply, GDPR anonymization, financial aggregation, and token/code cleanup. Guest accounts with GDPR-compliant anonymization (including `RESERVATION:edit_request:%` scrubbing). Service eligibility based on age + patch test validity. Overlap checks use `FOR UPDATE` row locks inside transactions. Closing-hours validation (`closing_time.go`) resolves both current and staged default hours. +**Booking**: Self-service (customer), walk-in (admin), call-in (admin). Slot reservations prevent double-booking (4 TTL types). **Self-blocking prevention**: `excludeUserID` parameter filters a user's own `RESERVATION` entries from time blocker overlap checks, allowing re-reservation and booking at overlapping slots. **Explicit cancellation**: `DELETE /api/bookings/reserve` releases a user reservation; `DELETE /api/admin/bookings/reserve` releases an admin walk-in/call-in reservation. **Background cleanup**: Centralised cron scheduler (`backend/internal/jobs/`) runs 22 maintenance jobs: reservation/deposit cleanup every 5min, hourly campaign transitions, daily unpaid-booking notifications, staged default hours auto-apply, GDPR anonymization, financial aggregation, and token/code cleanup. Guest accounts with GDPR-compliant anonymization (including `RESERVATION:edit_request:%` scrubbing). Service eligibility based on age + patch test validity. Overlap checks use `FOR UPDATE` row locks inside transactions. Closing-hours validation (`closing_time.go`) resolves both current and staged default hours. -**Payments**: Square Terminal (in-person, via `CreateTerminalCheckout`) + online card payments via saved cards or new cards tokenized through the Square Web Payments SDK (`cnon:` nonces — gated off in local dev until `VITE_SQUARE_APPLICATION_ID`/`VITE_SQUARE_LOCATION_ID` are set). The backend accepts only tokens, never raw PANs (PCI-DSS parity, mirrored in the dev mock). Cash with change calculation. Gift cards (12-digit code or account balance). Saved cards for faster checkout. Tips on completed bookings. Refunds with notice-period tiers and deposit protection (72h/24h thresholds). All payment types: deposit, full, partial, balance, tip. Payment >20% of total promotes `pending_release` bookings back to `confirmed`. Deposit paid is computed from payments on-the-fly. The first 50% of each payment is always carved out as deposit (via `buildSplitRecords`); any overflow beyond the booking total becomes a tip. A PostgreSQL `pg_advisory_lock` serializes payment attempts per-booking to prevent two-tab double-payment races. Gift card purchases insert a pending payment record with VAT before calling Square — the DB transaction commits first, so Square failures leave a retryable pending record (same-key retries reuse it). +**Payments**: Square Terminal (in-person, via `CreateTerminalCheckout`) + online card payments via saved cards or new cards tokenized through the Square Web Payments SDK (`cnon:` nonces — gated off in local dev until `VITE_SQUARE_APPLICATION_ID`/`VITE_SQUARE_LOCATION_ID` are set). The backend accepts only tokens, never raw PANs (PCI-DSS parity, mirrored in the dev mock). Cash with change calculation. Gift cards (12-digit code or account balance). Saved cards for faster checkout. Tips on completed bookings. Refunds with notice-period tiers and deposit protection (72h/24h thresholds). All payment types: deposit, full, partial, balance, tip. Payment >20% of total promotes `pending_release` bookings back to `confirmed`. Deposit paid is computed from payments on-the-fly. The first 50% of each payment is always carved out as deposit (via `buildSplitRecords`); any overflow beyond the booking total becomes a tip. A PostgreSQL `pg_advisory_lock` serializes payment attempts per-booking to prevent two-tab double-payment races. Gift card purchases insert a pending payment record with VAT before calling Square — the DB transaction commits first, so Square failures leave a retryable pending record (same-key retries reuse it). Two background sweeps close Square's ~24h idempotency-key retention window: `sweep-pending-square-refunds` reconciles/retries stuck refunds (with a 23h age guard), and `sweep-stale-pending-payments` fails stale pending payments/till-sales so a late retry cannot issue a second charge. **Gift Cards**: Multi-method purchase (cash, card machine, online card, giveaway). Inventory cards for stock management. 24-month rolling expiry. Idle account cleanup (2yr/5yr thresholds). Expired balance recovery with admin audit trail. Transaction audit log. Idempotency keys for purchases. @@ -18,7 +18,7 @@ Nail salon booking platform — Go 1.26.5 backend + SvelteKit 5 SPA + PostgreSQL **Loyalty & Discounts**: 1 stamp per paid appointment (max 1/day). 10 stamps → 10% off via opt-in checkbox at payment or till. Stamps refunded on cancellation. Campaigns auto-apply at both payment and completion: time-based, per-user milestone, global milestone (in-person only), anniversary. All discounts stack additively against original total. Discount payment records excluded from refund calculations. -**Compliance**: GDPR Article 15 data export (async, 12h cache, 21-section JSON + PDF — excludes verification codes as authentication tokens). Account deletion with external system scrubbing (S3, Square). Guest PII anonymized 6 months post-appointment. UK financial data retention (7 years). Gift card SPV/MPV VAT treatment configurable. +**Compliance**: GDPR Article 15 data export (async, 12h cache, 23-section JSON + PDF — excludes verification codes as authentication tokens). Account deletion with external system scrubbing (S3, Square). Guest PII anonymized 6 months post-appointment. UK financial data retention (7 years). Gift card SPV/MPV VAT treatment configurable. **Frontend**: Portfolio gallery with fuzzy tag search (relevance-sorted) and exact category filters (date-sorted), multi-format images (AVIF/WebP/JPEG/JXL with WASM client-side encoding), cursor-based pagination. MapLibre GL map on contact page. PhoneInput component with UK validation. CharCounter for long notes. @@ -94,6 +94,26 @@ To bypass: `git commit --no-verify`. CI caches Go modules (`~/go/pkg/mod`) and npm dependencies (`~/.npm`, `node_modules`) via `actions/cache` — keyed on `go.sum` and `package-lock.json` respectively. Cache is served by Gitea's built-in cache server at `git.popertots.com`. First run downloads everything (~3m35s), subsequent runs restore from cache in seconds. +### Database migrations + +The schema lives in `init-scripts/init-script.sql` and is applied automatically on a **fresh** volume via `docker-entrypoint-initdb.d`. Existing deployments must apply the payment-system delta manually (the schema is not migration-managed): + +```sql +-- Refund system columns (refunds table) +ALTER TABLE refunds ADD COLUMN IF NOT EXISTS refund_attempts INT NOT NULL DEFAULT 0; +ALTER TABLE refunds ADD COLUMN IF NOT EXISTS origin VARCHAR(16) NOT NULL DEFAULT 'manual'; +ALTER TABLE refunds ADD COLUMN IF NOT EXISTS idempotency_key VARCHAR(64) UNIQUE; + +-- refund_failed notification reason (admin_notification_reason enum) +-- NOTE: ALTER TYPE ... ADD VALUE cannot run inside a transaction block; run on a connection with autocommit. +ALTER TYPE admin_notification_reason ADD VALUE IF NOT EXISTS 'refund_failed'; + +-- Refunds may now reference non-booking payments (gift-card purchase refunds) +ALTER TABLE refunds ALTER COLUMN booking_id DROP NOT NULL; +``` + +The sweep job (`internal/jobs/cleanup.go`) and `refunds.go` cast `'refund_failed'::admin_notification_reason`, so an un-migrated DB fails at runtime — apply these before deploying the payment changes. + ## Full Documentation Detailed architecture, schema, admin workflows, user journeys, and backlog in [obsidian/Crussell/](obsidian/Crussell/). diff --git a/backend/handlers/payments/concurrency_test.go b/backend/handlers/payments/concurrency_test.go index e7fd8c7..ca21403 100644 --- a/backend/handlers/payments/concurrency_test.go +++ b/backend/handlers/payments/concurrency_test.go @@ -12,6 +12,7 @@ import ( "crussell/db" "crussell/internal/square" + "crussell/mw" "crussell/testutils" "crussell/testutils/fixtures" "crussell/testutils/jwt" @@ -392,7 +393,10 @@ func TestGetCheckoutStatus_ConcurrentPolls_SingleRecord(t *testing.T) { req := httptest.NewRequest("GET", "/api/checkout/"+checkoutID+"/status?booking_id="+bookingID, nil) rctx := chi.NewRouteContext() rctx.URLParams.Add("checkout_id", checkoutID) - req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + // GetCheckoutStatus is admin-only (defense-in-depth S-1 check). + reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "admin") + req = req.WithContext(reqCtx) w := httptest.NewRecorder() GetCheckoutStatus(w, req) recs[idx] = w diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 195e3cb..91d9c35 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -481,7 +481,7 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { var redeemedBy sql.NullString var isInventory bool var currentTotalFunds float64 - err = tx.QueryRow(ctx, "SELECT redeemed_by, is_inventory, total_funds_added FROM gift_cards WHERE id = $1", cardID).Scan(&redeemedBy, &isInventory, ¤tTotalFunds) + err = tx.QueryRow(ctx, "SELECT redeemed_by, is_inventory, total_funds_added FROM gift_cards WHERE id = $1 FOR UPDATE", cardID).Scan(&redeemedBy, &isInventory, ¤tTotalFunds) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "Gift card not found", http.StatusNotFound) @@ -597,7 +597,11 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { var fromRedeemedBy, toRedeemedBy sql.NullString var fromRemaining, toRemaining float64 - err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1", fromCardID).Scan(&fromRedeemedBy, &fromRemaining) + // Lock both rows FOR UPDATE (source first, deterministic order) so a + // concurrent transfer/topup can't interleave a read-then-write on the same + // card — the same check-then-act race RedeemGiftCard and the till path + // already guard against (N-5). + err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", fromCardID).Scan(&fromRedeemedBy, &fromRemaining) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "Source gift card not found", http.StatusNotFound) @@ -608,7 +612,7 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { return } - err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1", req.ToCardID).Scan(&toRedeemedBy, &toRemaining) + err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", req.ToCardID).Scan(&toRedeemedBy, &toRemaining) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "Destination gift card not found", http.StatusNotFound) @@ -953,6 +957,13 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { reusePendingID = existing.ID log.Printf("[PAYMENTS] Reusing pending payment %s for idempotent gift-card retry (key %s)", existing.ID, req.IdempotencyKey) } + if existing.Status == "failed" { + // Swept as stale (>24h) or definitively rejected — a retry would + // risk a second Square charge. Reject cleanly (R2). + log.Printf("Gift card retry rejected: pending record %s was marked failed", existing.ID) + http.Error(w, "This gift card purchase previously failed and can no longer be retried", http.StatusConflict) + return + } } } @@ -1093,8 +1104,26 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } - // Step 3: Square succeeded — update payment, create gift card. - _, upErr := db.Conn.Exec(ctx, + // Step 3: Square succeeded — atomically flip the payment to completed and + // create the gift card + balance + transaction in ONE transaction. If any + // step fails, the whole thing rolls back, the payment stays 'pending', and + // a same-key retry re-attempts the Square charge (Square dedups) before + // delivering the card. Previously these were separate non-transactional + // writes: a failure after the payment-completed update left the customer + // CHARGED but with no card, and the completed-dedup swallowed the retry. + issueTx, err := db.Conn.Begin(ctx) + if err != nil { + log.Printf("Failed to begin gift-card issue transaction: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if err := issueTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("failed to rollback gift-card issue transaction", "err", err) + } + }() + + _, upErr := issueTx.Exec(ctx, `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, paymentResult.SquarePayID, buyPaymentID, ) @@ -1108,7 +1137,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { if req.RecipientType == "self" { var purchaseVoucherType string - err = db.Conn.QueryRow(ctx, `SELECT COALESCE(voucher_type, 'SPV') FROM business_settings LIMIT 1`).Scan(&purchaseVoucherType) + err = issueTx.QueryRow(ctx, `SELECT COALESCE(voucher_type, 'SPV') FROM business_settings LIMIT 1`).Scan(&purchaseVoucherType) if err != nil { log.Printf("Failed to query voucher type: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1117,7 +1146,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { if purchaseVoucherType == "" { purchaseVoucherType = "SPV" } - err = db.Conn.QueryRow(ctx, ` + err = issueTx.QueryRow(ctx, ` INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, redeemed_at, redeemed_by, is_inventory, voucher_type_at_purchase) VALUES ($1, 0, $2, NOW(), $2, FALSE, $3) RETURNING id @@ -1128,7 +1157,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } - _, err = db.Conn.Exec(ctx, ` + _, err = issueTx.Exec(ctx, ` INSERT INTO user_giftcard_balances (user_id, balance, updated_at) VALUES ($1, $2, NOW()) ON CONFLICT (user_id) DO UPDATE SET @@ -1141,7 +1170,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } - _, err = db.Conn.Exec(ctx, ` + _, err = issueTx.Exec(ctx, ` INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) VALUES ($1, 'purchase', $2, 'api', NULL, $3, 'self-purchase, auto-redeemed') `, cardID, amountPounds, userID) @@ -1152,7 +1181,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } } else { var purchaseVoucherType string - err = db.Conn.QueryRow(ctx, `SELECT COALESCE(voucher_type, 'SPV') FROM business_settings LIMIT 1`).Scan(&purchaseVoucherType) + err = issueTx.QueryRow(ctx, `SELECT COALESCE(voucher_type, 'SPV') FROM business_settings LIMIT 1`).Scan(&purchaseVoucherType) if err != nil { log.Printf("Failed to query voucher type: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1161,7 +1190,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { if purchaseVoucherType == "" { purchaseVoucherType = "SPV" } - err = db.Conn.QueryRow(ctx, ` + err = issueTx.QueryRow(ctx, ` INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) VALUES ($1, $1, $2, FALSE, $3) RETURNING id @@ -1175,7 +1204,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { recipient := req.RecipientEmail if recipient == "" { var userEmail string - err = db.Conn.QueryRow(ctx, "SELECT email FROM users WHERE id = $1", userID).Scan(&userEmail) + err = issueTx.QueryRow(ctx, "SELECT email FROM users WHERE id = $1", userID).Scan(&userEmail) if err != nil { log.Printf("Failed to query user email: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1188,7 +1217,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // code anyone can redeem). Log only the value and recipient for audit. log.Printf("Gift card purchased for friend — value: £%.2f, intended for: %s (code stored in DB, not logged)", amountPounds, recipient) - _, err = db.Conn.Exec(ctx, ` + _, err = issueTx.Exec(ctx, ` INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) VALUES ($1, 'purchase', $2, 'api', NULL, $3, 'purchased for friend') `, cardID, amountPounds, userID) @@ -1199,6 +1228,12 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } } + if err := issueTx.Commit(ctx); err != nil { + log.Printf("CRITICAL: Square payment succeeded (ID=%s) but gift-card issue transaction commit failed: %v — manual reconciliation required", paymentResult.SquarePayID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(map[string]any{ "status": "success", diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 393f714..20d7b5f 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -3,6 +3,7 @@ package payments import ( "context" "crypto/rand" + "crypto/sha256" "crussell/clock" "crussell/db" "crussell/internal/square" @@ -31,6 +32,10 @@ type CreateTerminalPaymentRequest struct { TipEnabled bool `json:"tip_enabled"` PaymentMethod *string `json:"payment_method,omitempty"` GiftCardID *string `json:"gift_card_id,omitempty"` + // saved_card_id: the user's saved card (user_saved_cards.id) to charge + // directly, bypassing the terminal. The frontend sends this for the admin + // "Charge Saved Card" action. + UserSavedCardID *string `json:"saved_card_id,omitempty"` } type CreateBookingPaymentRequest struct { @@ -123,6 +128,15 @@ type DiscountPreview struct { Amount float64 `json:"amount"` } +// isAdminRequest is a defense-in-depth role check for admin-only payment +// handlers. The routes are mounted under mw.RequireAdmin, but this in-handler +// guard keeps admin-only actions (refunds, terminal charges, till sales) +// protected even if a route is ever re-registered on a non-admin router (S-1). +func isAdminRequest(r *http.Request) bool { + role, ok := r.Context().Value(mw.UserRoleKey).(string) + return ok && role == "admin" +} + // GetDiscountPreviewHandler returns eligible discounts for a booking without applying them. // GET /api/bookings/{id}/discount-preview func GetDiscountPreviewHandler(w http.ResponseWriter, r *http.Request) { @@ -314,6 +328,12 @@ func calculateDiscountPreview(ctx context.Context, bookingID string, userID stri } func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { + // Defense-in-depth admin check (S-1) — the route is mounted under + // mw.RequireAdmin; this keeps terminal charges admin-only regardless. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } bookingID := chi.URLParam(r, "id") if bookingID == "" || !validators.IsValidID(bookingID) { http.Error(w, "Booking not found", http.StatusNotFound) @@ -339,9 +359,6 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } - // M8 - // L5 - if err := ValidateAmount(req.Amount); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) @@ -561,6 +578,199 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // Admin "Charge Saved Card": charge the customer's saved card directly via + // Square (no terminal). Pending-first with full idempotency: a deterministic + // key derived from booking+type+amount+card means a network retry reuses the + // same key — Square dedups the charge and the pending record is resumed, so + // a lost-response retry can NEVER double-charge. Mirrors CreateTipPayment. + if req.PaymentMethod != nil && *req.PaymentMethod == "saved_card" { + if req.UserSavedCardID == nil || *req.UserSavedCardID == "" { + http.Error(w, "saved_card_id is required for saved_card payment", http.StatusBadRequest) + return + } + + status, err := service.GetBookingStatus(r.Context(), bookingID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + http.Error(w, "Booking not found", http.StatusNotFound) + return + } + log.Printf("Failed to get booking status: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if status != "in_progress" && status != "completed" { + http.Error(w, "Booking must be in_progress or completed to create payment", http.StatusBadRequest) + return + } + + // The saved card is owned by the booking's user, not the admin. + var bookingUserID sql.NullString + if err := db.Conn.QueryRow(r.Context(), `SELECT user_id FROM bookings WHERE id = $1`, bookingID).Scan(&bookingUserID); err != nil { + log.Printf("Failed to get booking user: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + card, err := service.GetCardByID(r.Context(), *req.UserSavedCardID, bookingUserID.String) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + http.Error(w, "Saved card not found", http.StatusNotFound) + return + } + log.Printf("Failed to get saved card: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + // Serialize saved-card charges per booking (same lock as online booking + // payments) so concurrent double-clicks can't both pass the idempotency + // check. Mirrors the CreateBookingPayment lock (R4). + pinConn, err := db.Conn.Acquire(r.Context()) + if err != nil { + log.Printf("Failed to acquire connection for saved-card lock: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer pinConn.Release() + if _, err := pinConn.Exec(r.Context(), ` + SELECT pg_advisory_lock(hashtext('crussell:payment:' || $1)) + `, bookingID); err != nil { + log.Printf("Failed to acquire saved-card serialization lock for %s: %v", bookingID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1)) + `, bookingID); err != nil { + log.Printf("Failed to release saved-card serialization lock for %s: %v", bookingID, err) + } + }() + + // Deterministic idempotency key: booking+type+amount+card. A network + // retry with the same inputs derives the same key → dedup, never a + // second charge. ≤45 chars for Square's limit. + scKey := bookingID + "-sc-" + req.PaymentType + "-" + strconv.FormatInt(amount, 10) + "-" + *req.UserSavedCardID + + // Idempotency switch inside the lock: completed → dedup; pending → + // reuse (re-attempt Square with the same key, which dedups Square-side); + // failed → clean rejection. + var existingID, existingStatus sql.NullString + var existingAmount sql.NullFloat64 + err = db.Conn.QueryRow(r.Context(), ` + SELECT id, status, amount FROM payments WHERE booking_id = $1 AND idempotency_key = $2 + `, bookingID, scKey).Scan(&existingID, &existingStatus, &existingAmount) + + paymentID := "" + switch { + case err == nil && existingStatus.String == "completed": + // Dedup — return the existing completed payment. + if err := json.NewEncoder(w).Encode(CheckoutResponse{ + CheckoutID: existingID.String, + Status: "COMPLETED", + }); err != nil { + log.Printf("Failed to encode JSON response: %v", err) + } + return + case err == nil && existingStatus.String == "pending": + // Reuse the pending record: a prior attempt's Square outcome is + // unknown. Guard the amount — a retry with a different amount must + // not reuse the old record's charge. + if int64(math.Round(existingAmount.Float64*100)) != amount { + log.Printf("Saved-card retry amount mismatch: pending %s has %d pence, request has %d pence", existingID.String, int64(math.Round(existingAmount.Float64*100)), amount) + http.Error(w, "Amount does not match the pending payment", http.StatusBadRequest) + return + } + paymentID = existingID.String + case err == nil && existingStatus.String == "failed": + log.Printf("Saved-card payment %s was previously marked failed (swept) — refusing retry", existingID.String) + http.Error(w, "This payment previously failed and can no longer be retried", http.StatusConflict) + return + case err != nil && !errors.Is(err, pgx.ErrNoRows): + log.Printf("Failed to check saved-card idempotency: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + // Pending-first: insert a pending payment record, commit, then charge. + tx, err := db.Conn.Begin(r.Context()) + if err != nil { + log.Printf("Failed to begin transaction: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if paymentID == "" { + record := PaymentRecord{ + BookingID: bookingID, + PaymentType: req.PaymentType, + PaymentMethod: "online_square", + Status: "pending", + Amount: float64(amount) / 100.0, + IdempotencyKey: &scKey, + UserSavedCardID: req.UserSavedCardID, + CreatedAt: clock.Now(), + UpdatedAt: clock.Now(), + CreatedBy: &adminID, + } + if err := tx.QueryRow(r.Context(), ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, user_saved_card_id, created_by, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) + RETURNING id + `, record.BookingID, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.IdempotencyKey, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt).Scan(&paymentID); err != nil { + log.Printf("Failed to insert pending saved-card payment: %v", err) + _ = tx.Rollback(r.Context()) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } + if err := tx.Commit(r.Context()); err != nil { + log.Printf("Failed to commit pending saved-card payment: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + var buyerEmail string + if bookingUserID.Valid { + _ = db.Conn.QueryRow(r.Context(), `SELECT email FROM users WHERE id = $1`, bookingUserID.String).Scan(&buyerEmail) + } + + paymentResult, err := SquareClient.CreatePayment(r.Context(), square.CreatePaymentReq{ + Amount: amount, + Currency: "GBP", + SourceID: card.SquareCardID, + IdempotencyKey: scKey, + ReferenceID: bookingID, + Note: req.PaymentType, + BuyerEmail: buyerEmail, + }) + if err != nil { + log.Printf("Failed to process saved-card payment: %v", err) + http.Error(w, "Payment failed", http.StatusPaymentRequired) + return + } + + if _, upErr := db.Conn.Exec(r.Context(), + `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, + paymentResult.SquarePayID, paymentID, + ); upErr != nil { + log.Printf("CRITICAL: Square payment %s succeeded but saved-card payment %s update failed: %v — manual reconciliation required", paymentResult.SquarePayID, paymentID, upErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + // Return the card details the frontend reads for the success state + // (MINOR-R2) — CheckoutResponse alone leaves card_brand/card_last4 blank. + if err := json.NewEncoder(w).Encode(map[string]any{ + "checkout_id": paymentID, + "status": "COMPLETED", + "card_brand": paymentResult.CardBrand, + "card_last4": paymentResult.CardLast4, + }); err != nil { + log.Printf("Failed to encode JSON response: %v", err) + } + return + } + // For Square checkout (terminal card reader), validate booking status // and check idempotency. No DB transaction needed since Square handles // the payment — no DB writes occur until GetCheckoutStatus. @@ -617,12 +827,18 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { + // Defense-in-depth admin check (S-1) — terminal completion records a + // payment, so it must stay admin-only. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } checkoutID := chi.URLParam(r, "checkout_id") if checkoutID == "" { http.Error(w, "Checkout ID is required", http.StatusBadRequest) return } - if !validators.IsValidID(checkoutID) { + if !validators.IsValidSquareCheckoutID(checkoutID) { http.Error(w, "not found", http.StatusNotFound) return } @@ -650,6 +866,17 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { return } + // Ownership check: the terminal checkout must reference THIS booking. + // CreateTerminalPayment sets reference_id = bookingID; without this check, + // polling the wrong checkout ID would attach its payment to a different + // booking (admin-only route, but a mis-scoped charge is a data-integrity + // bug worth rejecting). + if paymentResult.ReferenceID != "" && paymentResult.ReferenceID != bookingID { + log.Printf("Checkout %s references booking %s, not %s — refusing to record", checkoutID, paymentResult.ReferenceID, bookingID) + http.Error(w, "Checkout does not belong to this booking", http.StatusBadRequest) + return + } + if paymentResult.Status == "COMPLETED" { service := NewPaymentService() @@ -813,9 +1040,6 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { log.Printf("[SQUARE-PROD] Failed to resolve buyer email for user %s: %v (Square receipts will not be emailed)", userID, err) } - // M8 - // L5 - if err := ValidateAmount(req.Amount); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) @@ -937,17 +1161,29 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } // Check idempotency inside the transaction. + // Only short-circuit when the existing record is 'completed'. A 'pending' + // record means the previous Square call failed — returning it as 200 would + // show a success toast without ever charging. Re-attempt the charge below + // with the same idempotency key (Square dedups safely) and reuse the + // existing record. This mirrors CreateTipPayment exactly. var existingID sql.NullString var existingBookingID sql.NullString var existingPaymentType sql.NullString var existingStatus sql.NullString var existingAmount sql.NullFloat64 var existingCreatedAt sql.NullTime - if err := tx.QueryRow(r.Context(), ` + + err = tx.QueryRow(r.Context(), ` SELECT id, booking_id, payment_type, status, amount, created_at FROM payments WHERE booking_id = $1 AND idempotency_key = $2 - `, bookingID, req.IdempotencyKey).Scan(&existingID, &existingBookingID, &existingPaymentType, &existingStatus, &existingAmount, &existingCreatedAt); err == nil { + `, bookingID, req.IdempotencyKey).Scan(&existingID, &existingBookingID, &existingPaymentType, &existingStatus, &existingAmount, &existingCreatedAt) + + paymentID := "" + reusePendingRecord := false + switch { + case err == nil && existingStatus.String == "completed": + // Idempotent dedup — return the already-completed payment. if err := json.NewEncoder(w).Encode(PaymentResponse{ ID: existingID.String, BookingID: existingBookingID.String, @@ -959,7 +1195,27 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { log.Printf("Failed to encode JSON response: %v", err) } return - } else if !errors.Is(err, pgx.ErrNoRows) { + case err == nil && existingStatus.String == "pending": + // Previous Square call failed — reuse the pending record and re-attempt. + // Guard the amount: a retry with a different amount must not mutate the + // original record or charge the new amount against the old key. Compare + // in pence via math.Round — int64(pounds*100) truncation would reject + // legitimate same-amount retries for non-exact values (see CreateTipPayment). + if int64(math.Round(existingAmount.Float64*100)) != req.Amount { + log.Printf("Payment retry amount mismatch: pending record %s has %d pence, request has %d pence", existingID.String, int64(math.Round(existingAmount.Float64*100)), req.Amount) + http.Error(w, "Amount does not match the pending payment", http.StatusBadRequest) + return + } + paymentID = existingID.String + reusePendingRecord = true + case err == nil && existingStatus.String == "failed": + // Swept as stale (>24h) or definitively rejected — a retry would risk a + // second Square charge. Reject cleanly instead of 500-ing on the + // idempotency_key UNIQUE constraint (R2). + log.Printf("Payment retry rejected: record %s was marked failed", existingID.String) + http.Error(w, "This payment previously failed and can no longer be retried", http.StatusConflict) + return + case err != nil && !errors.Is(err, pgx.ErrNoRows): log.Printf("Failed to check idempotency: %v", err) } @@ -1032,6 +1288,53 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { savedCardID = req.CardID } + // If there is no pending record to reuse, insert one NOW and commit the + // transaction BEFORE calling Square. The committed pending row binds the + // idempotency key in the DB, so a post-charge insert/commit failure leaves + // a retryable pending record instead of an unbound key (a same-key retry + // would otherwise re-charge). It also releases the DB transaction before + // the ~30s Square round-trip instead of holding it open across the call. + if !reusePendingRecord { + fees := service.CalculateFees(req.Amount, "online") + pendingRecord := PaymentRecord{ + BookingID: bookingID, + PaymentType: req.PaymentType, + PaymentMethod: "online_square", + Status: "pending", + Amount: float64(req.Amount) / 100.0, + IdempotencyKey: &req.IdempotencyKey, + Fees: float64(fees) / 100.0, + UserSavedCardID: savedCardID, + CreatedAt: clock.Now(), + UpdatedAt: clock.Now(), + CreatedBy: &userID, + } + paymentID, err = service.CreatePaymentRecordTx(r.Context(), tx, pendingRecord, nil) + if err != nil { + log.Printf("Failed to create pending payment record: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + // Apply VAT to the pending record inside the same transaction — same + // pattern as CreateTipPayment. + ApplyVATToBookingPayment(r.Context(), tx, paymentID) + } + + // Always commit the transaction. In the reuse path no rows were written, + // but the commit is required in the test harness: there the context carries + // an outer test tx, so Begin creates a nested savepoint whose deferred + // rollback would otherwise undo the post-charge UPDATE executed later on + // the same connection. In production Begin is a plain tx and this commit is + // a harmless no-op that keeps both paths identical. + if err := tx.Commit(r.Context()); err != nil { + log.Printf("Failed to commit transaction: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + // Step 2: DB transaction committed — safe to call Square now. If Square + // fails, the record stays 'pending' and a same-key retry reuses it. + paymentReq := square.CreatePaymentReq{ Amount: req.Amount, Currency: "GBP", @@ -1049,15 +1352,32 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } - fees := service.CalculateFees(req.Amount, "online") - paymentAmount := float64(req.Amount) / 100.0 + // Step 3: Square succeeded — record the completed payment state in a NEW + // transaction (split records, VAT, deposit promotion, campaigns). The + // pending row committed in step 1 already holds the primary idempotency + // key, so it IS the primary record: update it to 'completed' with the + // Square payment ID, then insert only the additional -split-N records. + tx2, txErr := db.Conn.Begin(r.Context()) + if txErr != nil { + log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but opening the post-charge transaction failed: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, txErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if err := tx2.Rollback(r.Context()); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("failed to rollback post-charge transaction", "err", err) + } + }() + // Build payment records — may split a single Square charge into // a deposit portion (up to 50% of booking total) plus a balance // portion, so the refund system can correctly track deposit vs // non-deposit money per the deposit protection policy. bookingInfo, bErr := service.GetBookingPaymentInfo(r.Context(), bookingID) + fees := service.CalculateFees(req.Amount, "online") primaryRecord := PaymentRecord{ BookingID: bookingID, PaymentType: req.PaymentType, @@ -1083,41 +1403,62 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { records = []PaymentRecord{primaryRecord} } - // Create all payment records for this Square charge inside the transaction - // so that if any insert fails the entire group rolls back. This prevents - // a data inconsistency where Square charged the customer but only part of - // the split is reflected in the DB. + // The primary split record (records[0]) is the committed pending row. Its + // amount/payment_type may differ from the pending insert (deposit carving + // in buildSplitRecords), so align the row to the computed values. The VAT + // fields are cleared so apply_vat_to_payment recomputes on the final amount + // — the pending record had VAT applied at the pre-split amount. + primary := records[0] + if _, upErr := tx2.Exec(r.Context(), ` + UPDATE payments SET + status = 'completed', + square_payment_id = $1, + amount = $2, + payment_type = $3, + fees = $4, + is_vat_applicable = FALSE, + vat_rate = NULL, + vat_amount = NULL, + net_amount = NULL, + updated_at = NOW() + WHERE id = $5 + `, paymentResult.SquarePayID, primary.Amount, primary.PaymentType, primary.Fees, paymentID); upErr != nil { + log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but updating payment %s to completed failed: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, paymentID, upErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - var primaryPaymentID string + // Insert the additional split records. They carry the derived -split-N + // idempotency keys, which are new rows; if the split produced only one + // record, there is nothing more to insert. var paymentIDs []string - for i, rec := range records { - pid, cErr := service.CreatePaymentRecordTx(r.Context(), tx, rec, nil) + for i, rec := range records[1:] { + pid, cErr := service.CreatePaymentRecordTx(r.Context(), tx2, rec, nil) if cErr != nil { - log.Printf("Failed to create payment record %d/%d: %v", i+1, len(records), cErr) + log.Printf("Failed to create split payment record %d/%d: %v", i+2, len(records), cErr) http.Error(w, "internal server error", http.StatusInternalServerError) return } paymentIDs = append(paymentIDs, pid) - if i == 0 { - primaryPaymentID = pid - } } // Apply VAT to all split records if the business is VAT-registered. // Must be inside the transaction so VAT updates are atomic with inserts. - vatCfg, vatErr := GetVATConfig(r.Context(), tx) + vatCfg, vatErr := GetVATConfig(r.Context(), tx2) if vatErr == nil && vatCfg.IsVATRegistered { - for _, pid := range paymentIDs { - if _, execErr := tx.Exec(r.Context(), "SELECT apply_vat_to_payment($1, $2)", pid, vatCfg.DefaultVATRate); execErr != nil { + vatIDs := append([]string{paymentID}, paymentIDs...) + for _, pid := range vatIDs { + if _, execErr := tx2.Exec(r.Context(), "SELECT apply_vat_to_payment($1, $2)", pid, vatCfg.DefaultVATRate); execErr != nil { log.Printf("Failed to apply VAT to payment %s: %v", pid, execErr) } } } // Promote deposit to confirmed if total paid meets the 20% threshold. - // Check is inside the transaction so it sees the just-inserted payments. + // Check is inside the transaction so it sees the just-completed primary. var depositMet bool - if err := tx.QueryRow(r.Context(), ` + if err := tx2.QueryRow(r.Context(), ` WITH booking_total AS ( SELECT total_amount * 100 AS total_cents FROM bookings WHERE id = $1 ), @@ -1133,7 +1474,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } if depositMet { - if _, err := tx.Exec(r.Context(), ` + if _, err := tx2.Exec(r.Context(), ` UPDATE bookings SET status = 'confirmed', updated_at = NOW() WHERE id = $1 AND status = 'pending_release' `, bookingID); err != nil { @@ -1144,9 +1485,9 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // Apply eligible campaign discounts inside the payment transaction, so // atomicity with the payment inserts is guaranteed. The call is idempotent // — if discounts were already applied, the duplicate check skips them. - applyEligibleCampaignsAtPayment(r.Context(), tx, bookingID, userID) + applyEligibleCampaignsAtPayment(r.Context(), tx2, bookingID, userID) - if cErr := tx.Commit(r.Context()); cErr != nil { + if cErr := tx2.Commit(r.Context()); cErr != nil { log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but DB transaction commit failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, cErr) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1154,7 +1495,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } if err := json.NewEncoder(w).Encode(PaymentResponse{ - ID: primaryPaymentID, + ID: paymentID, BookingID: bookingID, PaymentType: req.PaymentType, Status: "completed", @@ -1560,6 +1901,12 @@ func GetUserPaymentMethods(w http.ResponseWriter, r *http.Request) { } func AdminGetUserPaymentMethods(w http.ResponseWriter, r *http.Request) { + // Defense-in-depth admin check (S-1) — exposing another user's saved cards + // must stay admin-only. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } userID := chi.URLParam(r, "id") if userID == "" || !validators.IsValidID(userID) { http.Error(w, "Invalid user ID", http.StatusBadRequest) @@ -1652,6 +1999,13 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { } func RefundPayment(w http.ResponseWriter, r *http.Request) { + // Defense-in-depth: the route is mounted under mw.RequireAdmin, but this + // in-handler check keeps refund access admin-only even if the route is ever + // re-registered on a non-admin router (S-1). + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } paymentID := chi.URLParam(r, "payment_id") if paymentID == "" || !validators.IsValidID(paymentID) { http.Error(w, "Payment not found", http.StatusNotFound) @@ -1710,10 +2064,15 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { // does not create a second Square refund. When the client supplies an // idempotency key (one per distinct refund attempt, reused on retry), use // it — the amount-derived fallback would collide on two DISTINCT partial - // refunds of the same amount, silently swallowing the second. + // refunds of the same amount, silently swallowing the second. The client + // key is hashed+truncated: Square's idempotency-key limit is 45 chars, and + // paymentID (12) + "-refund-" (8) + a full 36-char UUID (56 total) would + // be rejected with a 400. The hash stays deterministic, so a same-key + // retry still dedups. idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10) if req.IdempotencyKey != "" { - idempotencyKey = paymentID + "-refund-" + req.IdempotencyKey + ikHash := sha256.Sum256([]byte(req.IdempotencyKey)) + idempotencyKey = paymentID + "-refund-" + fmt.Sprintf("%x", ikHash)[:24] } // Serialize refund attempts per payment to prevent two concurrent refunds @@ -1824,9 +2183,19 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { reissueResult, reissueErr := SquareClient.RefundPayment(r.Context(), reissueReq) switch { case reissueErr == nil: + // Resolve by Square's status: PENDING stays pending (sweep + // reconciles), FAILED/REJECTED is definitive, COMPLETED resolves. + reissueStatus := "completed" + if reissueResult.Status == "PENDING" { + reissueStatus = "pending" + log.Printf("Square reissue %s is PENDING — leaving refund %s pending for the sweep", reissueResult.ID, existingRefundID.String) + } else if reissueResult.Status == "FAILED" || reissueResult.Status == "REJECTED" { + reissueStatus = "failed" + log.Printf("Square reissue %s FAILED — marking refund %s failed", reissueResult.ID, existingRefundID.String) + } if _, upErr := db.Conn.Exec(r.Context(), - `UPDATE refunds SET status = 'completed', square_refund_id = $1 WHERE id = $2`, - reissueResult.ID, existingRefundID.String, + `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`, + reissueStatus, reissueResult.ID, existingRefundID.String, ); upErr != nil { log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", reissueResult.ID, existingRefundID.String, upErr) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1836,7 +2205,7 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { ID: existingRefundID.String, PaymentID: paymentID, Amount: req.Amount, - Status: "completed", + Status: reissueStatus, Reason: req.Reason, CreatedAt: clock.Now().Format(time.RFC3339), }); err != nil { @@ -1972,13 +2341,20 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { }() var refundID string + // booking_id is NULL for non-booking payments (gift-card purchase refunds); + // payments without a booking leave it NULL rather than inserting an empty + // string that violates the refunds.booking_id FK/NOT NULL. + var refundBookingID any = payment.BookingID + if payment.BookingID == "" { + refundBookingID = nil + } err = tx.QueryRow(r.Context(), ` INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, created_by, created_at, origin) VALUES ($1, $2, $3, 'pending', $4, $5, $6, $7, 'manual') RETURNING id `, paymentID, - payment.BookingID, + refundBookingID, float64(req.Amount)/100.0, req.Reason, idempotencyKey, @@ -2044,14 +2420,26 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } - squareRefundID := refundResult.ID + // Square succeeded — resolve the refund row by Square's status. A + // synchronous refund response can be PENDING (money in flight, e.g. an + // async card network): marking it completed while Square later fails it + // would permanently block that amount in the over-refund guard. Only a + // definitive COMPLETED resolves to completed; PENDING stays pending for the + // sweep to reconcile; FAILED/REJECTED is a real failure. + status := "completed" + if refundResult.Status == "PENDING" { + status = "pending" + log.Printf("Square refund %s is PENDING (in flight) — leaving refund %s pending for the sweep to resolve", refundResult.ID, refundID) + } else if refundResult.Status == "FAILED" || refundResult.Status == "REJECTED" { + status = "failed" + log.Printf("Square refund %s FAILED — marking refund %s failed", refundResult.ID, refundID) + } - // Square succeeded — update the refund record to completed. if _, upErr := db.Conn.Exec(r.Context(), - `UPDATE refunds SET status = 'completed', square_refund_id = $1 WHERE id = $2`, - squareRefundID, refundID, + `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`, + status, refundResult.ID, refundID, ); upErr != nil { - log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", squareRefundID, refundID, upErr) + log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", refundResult.ID, refundID, upErr) http.Error(w, "internal server error", http.StatusInternalServerError) return } @@ -2060,7 +2448,7 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { ID: refundID, PaymentID: paymentID, Amount: req.Amount, - Status: "completed", + Status: status, Reason: req.Reason, CreatedAt: clock.Now().Format(time.RFC3339), }); err != nil { @@ -2119,9 +2507,21 @@ func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID http.Error(w, "Refund failed", http.StatusInternalServerError) return } + // Resolve by Square's status — a PENDING resume stays pending for the + // sweep (marking it completed while Square later fails it would block the + // amount in the over-refund guard forever); FAILED/REJECTED is definitive. + status := "completed" + if resumeResult.Status == "PENDING" { + status = "pending" + log.Printf("Square refund %s is PENDING — leaving refund %s pending for the sweep", resumeResult.ID, refundID) + } else if resumeResult.Status == "FAILED" || resumeResult.Status == "REJECTED" { + status = "failed" + log.Printf("Square refund %s FAILED — marking refund %s failed", resumeResult.ID, refundID) + } + if _, upErr := db.Conn.Exec(r.Context(), - `UPDATE refunds SET status = 'completed', square_refund_id = $1 WHERE id = $2`, - resumeResult.ID, refundID, + `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`, + status, resumeResult.ID, refundID, ); upErr != nil { log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", resumeResult.ID, refundID, upErr) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -2131,7 +2531,7 @@ func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID ID: refundID, PaymentID: paymentID, Amount: resumeAmount, - Status: "completed", + Status: status, Reason: refundReason, CreatedAt: clock.Now().Format(time.RFC3339), }); err != nil { @@ -2352,6 +2752,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { } paymentID = existingID.String reusePendingRecord = true + case err == nil && existingStatus.String == "failed": + // Swept as stale (>24h, past Square's key retention) or definitively + // rejected. A retry can no longer be replayed against Square without + // risking a second charge — reject cleanly instead of inserting a new + // pending row that 500s on the idempotency_key UNIQUE constraint (R2). + log.Printf("Tip retry rejected: pending record %s was marked failed", existingID.String) + http.Error(w, "This tip payment previously failed and can no longer be retried", http.StatusConflict) + return case err != nil && !errors.Is(err, pgx.ErrNoRows): log.Printf("Failed to check tip idempotency: %v", err) } diff --git a/backend/handlers/payments/loyalty.go b/backend/handlers/payments/loyalty.go index d6af1b4..5d547f2 100644 --- a/backend/handlers/payments/loyalty.go +++ b/backend/handlers/payments/loyalty.go @@ -1,6 +1,7 @@ package payments import ( + "context" "crussell/db" "crussell/internal/validators" "crussell/mw" @@ -83,6 +84,34 @@ func ApplyLoyaltyRedemption(w http.ResponseWriter, r *http.Request) { return } + // Serialize redemption per booking: two concurrent redemptions could both + // pass the checks above and both insert a discount (double-apply). Reuse + // the booking-payment advisory lock so redemption is mutually exclusive + // with payments and other redemptions on the same booking (N-6). + pinConn, err := db.Conn.Acquire(r.Context()) + if err != nil { + log.Printf("Failed to acquire connection for loyalty redemption lock: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer pinConn.Release() + if _, err := pinConn.Exec(r.Context(), ` + SELECT pg_advisory_lock(hashtext('crussell:payment:' || $1)) + `, bookingID); err != nil { + log.Printf("Failed to acquire loyalty redemption serialization lock for %s: %v", bookingID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1)) + `, bookingID); err != nil { + log.Printf("Failed to release loyalty redemption serialization lock for %s: %v", bookingID, err) + } + }() + + // Re-check inside the lock (the checks above ran before acquiring it) so a + // concurrent redemption that completed while we waited is caught. var existingDiscount int if err := db.Conn.QueryRow(r.Context(), ` SELECT 1 FROM booking_discounts WHERE booking_id = $1 AND discount_source = 'loyalty' diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index fcde487..7171831 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -31,6 +31,15 @@ func makePaymentRequest(handler http.HandlerFunc, method, path string, body inte return makePaymentAuthRequest(handler, method, path, body, token, "", ctx) } +// adminRequestCtx wraps a request context with the admin role so handlers that +// run a defense-in-depth isAdminRequest check (S-1) work when called directly +// (bypassing the mw.RequireAdmin middleware that normally injects the role). +func adminRequestCtx(r *http.Request) *http.Request { + reqCtx := context.WithValue(r.Context(), mw.UserRoleKey, "admin") + reqCtx = context.WithValue(reqCtx, mw.UserIDKey, "000000000001") + return r.WithContext(reqCtx) +} + func TestValidateCardInfo(t *testing.T) { empty := "" cardID := "card_123" @@ -2902,6 +2911,7 @@ func TestGetCheckoutStatus_MissingCheckoutID(t *testing.T) { rctx := chi.NewRouteContext() reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) req = req.WithContext(reqCtx) + req = adminRequestCtx(req) w := httptest.NewRecorder() GetCheckoutStatus(w, req) @@ -2914,13 +2924,18 @@ func TestGetCheckoutStatus_MissingCheckoutID(t *testing.T) { func TestGetCheckoutStatus_InvalidCheckoutID(t *testing.T) { _, _ = testutils.SetupTestTx(t) - req := httptest.NewRequest("GET", "/api/checkout/1234567890abc/status?booking_id=abc", nil) + // Must fail the Square-compatible checkout-ID check — the old 12-hex gate + // rejected real Square IDs (UUIDs like "08YceKh7B3ZqO"), so an injection + // attempt (path traversal) is the correct invalid case now. + badID := "../etc/passwd" + req := httptest.NewRequest("GET", "/api/checkout/"+badID+"/status?booking_id=abc", nil) rctx := chi.NewRouteContext() - rctx.URLParams.Add("checkout_id", "1234567890abc") + rctx.URLParams.Add("checkout_id", badID) reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) req = req.WithContext(reqCtx) w := httptest.NewRecorder() + req = adminRequestCtx(req) GetCheckoutStatus(w, req) if w.Code != http.StatusNotFound { @@ -2928,6 +2943,31 @@ func TestGetCheckoutStatus_InvalidCheckoutID(t *testing.T) { } } +func TestGetCheckoutStatus_RealSquareID_PassesValidation(t *testing.T) { + // A real Square checkout ID (13-char UUID, not 12-hex) must pass the + // checkout-ID gate — the C-4 fix. It then 404s at the Square client level + // (mock has no such checkout), proving the gate no longer rejects it. + _, _ = testutils.SetupTestTx(t) + + realID := "08YceKh7B3ZqO" + req := httptest.NewRequest("GET", "/api/checkout/"+realID+"/status?booking_id=abc", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("checkout_id", realID) + reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + req = req.WithContext(reqCtx) + + w := httptest.NewRecorder() + req = adminRequestCtx(req) + GetCheckoutStatus(w, req) + + // Not 404-from-validation: the gate accepted it. The mock returns 500 for + // an unknown checkout (it panics on a missing ID), so assert NOT a 404 + // from the gate — any non-404 is proof the gate passed. + if w.Code == http.StatusNotFound { + t.Errorf("real Square checkout ID %q was rejected by the validation gate — expected it to pass validation", realID) + } +} + func TestGetCheckoutStatus_ValidCheckoutNotFound(t *testing.T) { _, tx := testutils.SetupTestTx(t) @@ -2951,6 +2991,7 @@ func TestGetCheckoutStatus_ValidCheckoutNotFound(t *testing.T) { req = req.WithContext(reqCtx) w := httptest.NewRecorder() + req = adminRequestCtx(req) GetCheckoutStatus(w, req) if w.Code != http.StatusInternalServerError { @@ -3011,6 +3052,7 @@ func TestAdminGetUserPaymentMethods_InvalidUserID(t *testing.T) { req = req.WithContext(reqCtx) w := httptest.NewRecorder() + req = adminRequestCtx(req) AdminGetUserPaymentMethods(w, req) if w.Code != http.StatusBadRequest { @@ -3418,6 +3460,7 @@ func TestGetCheckoutStatus_MissingBookingID(t *testing.T) { req = req.WithContext(reqCtx) w := httptest.NewRecorder() + req = adminRequestCtx(req) GetCheckoutStatus(w, req) if w.Code != http.StatusBadRequest { @@ -3433,6 +3476,7 @@ func TestGetCheckoutStatus_InvalidBookingID(t *testing.T) { rctx.URLParams.Add("checkout_id", "aaaaaaaaaaaa") reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) req = req.WithContext(reqCtx) + req = adminRequestCtx(req) w := httptest.NewRecorder() GetCheckoutStatus(w, req) @@ -3452,6 +3496,7 @@ func TestGetCheckoutStatus_BookingNotFound(t *testing.T) { req = req.WithContext(reqCtx) w := httptest.NewRecorder() + req = adminRequestCtx(req) GetCheckoutStatus(w, req) if w.Code != http.StatusInternalServerError { @@ -3472,8 +3517,10 @@ func TestTerminalPayment_NoAuth(t *testing.T) { PaymentType: "full", }, "", ctx) - if w.Code != http.StatusUnauthorized { - t.Errorf("expected status 401, got %d. body: %s", w.Code, w.Body.String()) + // No auth token → no role → the defense-in-depth isAdminRequest check + // rejects with 403 before the adminID check (S-1). + if w.Code != http.StatusForbidden { + t.Errorf("expected status 403, got %d. body: %s", w.Code, w.Body.String()) } } @@ -3538,3 +3585,200 @@ func TestTerminalPayment_ValidatePaymentTypeFails(t *testing.T) { } + +func TestSweepStalePendingPayments_MarksOldFailed(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + + // A fresh pending payment (should NOT be failed). + freshID, err := fixtures.CreateTestPayment(tx, bookingID, 1000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create fresh pending payment: %v", err) + } + // A stale pending payment (25h old — past Square's ~24h key retention). + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create stale pending payment: %v", err) + } + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '25 hours' WHERE id = $1", staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + _, err = SweepStalePendingPayments(ctx) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + + // Stale pending → failed; fresh pending untouched. + var staleStatus, freshStatus string + if err := tx.QueryRow(ctx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&staleStatus); err != nil { + t.Fatalf("failed to query stale payment: %v", err) + } + if err := tx.QueryRow(ctx, "SELECT status FROM payments WHERE id = $1", freshID).Scan(&freshStatus); err != nil { + t.Fatalf("failed to query fresh payment: %v", err) + } + if staleStatus != "failed" { + t.Errorf("expected stale pending payment to be marked failed, got %q", staleStatus) + } + if freshStatus != "pending" { + t.Errorf("expected fresh pending payment to stay pending, got %q", freshStatus) + } +} + +// TestSavedCardPayment_LostResponseRetry_Dedups verifies the R1 fix: two +// "Charge Saved Card" requests with identical inputs (booking + type + amount + +// card) derive the SAME deterministic idempotency key, so a lost-response +// retry reuses the completed payment instead of charging twice. Before the fix, +// every request used a fresh random key → the second click double-charged. +func TestSavedCardPayment_LostResponseRetry_Dedups(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:saved-card-test", "VISA", "4242") + if err != nil { + t.Fatalf("failed to create saved card: %v", err) + } + + handler := CreateTerminalPayment + reqBody := CreateTerminalPaymentRequest{ + Amount: 5000, + PaymentType: "full", + PaymentMethod: strPtr("saved_card"), + UserSavedCardID: &cardID, + } + + // First charge. + w1 := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", reqBody, adminToken, ctx) + if w1.Code != http.StatusOK { + t.Fatalf("first saved-card charge: expected 200, got %d. body: %s", w1.Code, w1.Body.String()) + } + + // Same-input retry (lost response) — must dedup, not double-charge. + w2 := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", reqBody, adminToken, ctx) + if w2.Code != http.StatusOK { + t.Fatalf("retry saved-card charge: expected 200, got %d. body: %s", w2.Code, w2.Body.String()) + } + + // Exactly ONE payment record for this booking. + var payCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'online_square' AND amount = 50.00`, bookingID).Scan(&payCount); err != nil { + t.Fatalf("failed to count payments: %v", err) + } + if payCount != 1 { + t.Errorf("expected exactly 1 payment record (dedup), got %d — double-charge!", payCount) + } +} + +// TestSavedCardPayment_SweptFailed_Rejected verifies the R2 fix: after the +// sweep marks a pending payment failed, a same-key retry is cleanly rejected +// with 409 instead of 500-ing on the idempotency_key UNIQUE constraint. +func TestSavedCardPayment_SweptFailed_Rejected(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:swept-card-test", "VISA", "1111") + if err != nil { + t.Fatalf("failed to create saved card: %v", err) + } + + // Seed a failed payment with the deterministic key the handler will derive. + scKey := bookingID + "-sc-full-5000-" + cardID + if _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, user_saved_card_id, created_by, created_at, updated_at) + VALUES ($1, 'full', 'online_square', 'failed', 50.00, $2, $3, $4, NOW(), NOW()) + `, bookingID, scKey, cardID, adminID); err != nil { + t.Fatalf("failed to seed failed payment: %v", err) + } + + handler := CreateTerminalPayment + reqBody := CreateTerminalPaymentRequest{ + Amount: 5000, + PaymentType: "full", + PaymentMethod: strPtr("saved_card"), + UserSavedCardID: &cardID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", reqBody, adminToken, ctx) + if w.Code != http.StatusConflict { + t.Fatalf("expected 409 (swept-failed rejection), got %d. body: %s", w.Code, w.Body.String()) + } + + // No new payment row was inserted. + var payCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, scKey).Scan(&payCount); err != nil { + t.Fatalf("failed to count payments: %v", err) + } + if payCount != 1 { + t.Errorf("expected exactly 1 (failed) payment row, got %d", payCount) + } +} + + +// TestSweepStalePendingPayments_CoversTillSales verifies the R3 fix: the sweep +// also marks stale pending till_sales rows (card payments) as failed, so a +// lost-response till sale can't stay pending past Square's key retention. +func TestSweepStalePendingPayments_CoversTillSales(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + + // A stale pending till sale (card payment, 25h old). + var tillSaleID string + 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() - INTERVAL '25 hours', NOW()) + RETURNING id + `, adminID).Scan(&tillSaleID) + if err != nil { + t.Fatalf("failed to seed pending till sale: %v", err) + } + + // A fresh pending till sale that must NOT be swept. + var freshSaleID string + 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, 30.00, 30.00, 'online_square', 'pending', $1, NOW(), NOW()) + RETURNING id + `, adminID).Scan(&freshSaleID) + if err != nil { + t.Fatalf("failed to seed fresh till sale: %v", err) + } + + _, err = SweepStalePendingPayments(ctx) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var staleStatus, freshStatus string + if err := tx.QueryRow(ctx, "SELECT status FROM till_sales WHERE id = $1", tillSaleID).Scan(&staleStatus); err != nil { + t.Fatalf("failed to query stale till sale: %v", err) + } + if err := tx.QueryRow(ctx, "SELECT status FROM till_sales WHERE id = $1", freshSaleID).Scan(&freshStatus); err != nil { + t.Fatalf("failed to query fresh till sale: %v", err) + } + if staleStatus != "failed" { + t.Errorf("expected stale pending till sale to be marked failed, got %q", staleStatus) + } + if freshStatus != "pending" { + t.Errorf("expected fresh pending till sale to stay pending, got %q", freshStatus) + } +} diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index 9b61b6a..5203669 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -1052,13 +1052,26 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar }) switch { case sqErr == nil: + // Resolve by Square's status: COMPLETED resolves the group; PENDING + // leaves the rows pending (a later sweep reconciles them via + // ListPaymentRefunds); FAILED/REJECTED is a definitive failure that + // must not be marked completed (that would block the amount in the + // over-refund guard forever). + sqStatus := "completed" + if sqResult.Status == "PENDING" { + sqStatus = "pending" + log.Printf("Square refund %s for charge %s is PENDING — leaving refunds pending for the sweep", sqResult.ID, chargeID) + } else if sqResult.Status == "FAILED" || sqResult.Status == "REJECTED" { + sqStatus = "failed" + log.Printf("Square refund %s for charge %s FAILED — marking refunds failed", sqResult.ID, chargeID) + } // ATOMIC — one statement for the whole group, never per-row. Keeps // crash-retry amounts identical so Square's key-dedup returns the // original refund. if _, upErr := db.Conn.Exec(ctx, ` - UPDATE refunds SET status = 'completed', square_refund_id = $1 - WHERE id = ANY($2) AND status = 'pending' - `, sqResult.ID, idsOf(pending)); upErr != nil { + UPDATE refunds SET status = $1, square_refund_id = $2 + WHERE id = ANY($3) AND status = 'pending' + `, sqStatus, sqResult.ID, idsOf(pending)); upErr != nil { log.Printf("CRITICAL: Square refund committed (%s) but DB update for charge %s failed — manual reconciliation required: %v", sqResult.ID, chargeID, upErr) } return len(pending), nil diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index 5782b32..b3f1c8f 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -332,6 +332,12 @@ func (s *PaymentService) CheckIdempotencyByKey(ctx context.Context, idempotencyK func (s *PaymentService) GetPaymentByID(ctx context.Context, paymentID string) (*PaymentRecord, error) { var p PaymentRecord + // booking_id / vendor_code / gift_card_id / invoice_number are nullable + // (e.g. gift-card purchases have no booking). Scan into Null* and map so a + // NULL value doesn't 500 the scan (N-3: the same fix class as + // CheckIdempotencyByKey). + var bookingID, vendorCode, giftCardID sql.NullString + var invoiceNumber sql.NullInt64 err := db.Conn.QueryRow(ctx, ` SELECT id, booking_id, payment_type, payment_method, vendor_code, invoice_number, status, amount, is_vat_applicable, vat_rate, vat_amount, net_amount, @@ -340,15 +346,26 @@ func (s *PaymentService) GetPaymentByID(ctx context.Context, paymentID string) ( FROM payments WHERE id = $1 `, paymentID).Scan( - &p.ID, &p.BookingID, &p.PaymentType, &p.PaymentMethod, &p.VendorCode, &p.InvoiceNumber, + &p.ID, &bookingID, &p.PaymentType, &p.PaymentMethod, &vendorCode, &invoiceNumber, &p.Status, &p.Amount, &p.IsVATApplicable, &p.VATRate, &p.VATAmount, &p.NetAmount, &p.UserSavedCardID, &p.SquarePaymentID, &p.IdempotencyKey, &p.Fees, - &p.CreatedAt, &p.UpdatedAt, &p.CreatedBy, &p.GiftCardID, + &p.CreatedAt, &p.UpdatedAt, &p.CreatedBy, &giftCardID, ) if err != nil { return nil, err } + p.BookingID = bookingID.String + if vendorCode.Valid { + p.VendorCode = &vendorCode.String + } + if giftCardID.Valid { + p.GiftCardID = &giftCardID.String + } + if invoiceNumber.Valid { + n := int(invoiceNumber.Int64) + p.InvoiceNumber = &n + } return &p, nil } @@ -536,10 +553,22 @@ func (s *PaymentService) CreatePaymentMethodFromToken(ctx context.Context, userI var savedCardID string var isDefault bool + // ON CONFLICT (square_card_id): a response-lost retry re-tokenizes the same + // card (CreateCardOnFile's deterministic key returns the same ccof: id), so + // the UNIQUE constraint would otherwise 500 on the duplicate. Upsert instead + // so the retry returns the existing saved card (N-8). err = db.Conn.QueryRow(ctx, ` INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint, is_default) SELECT $1, $2, $3, $4, $5, $6, $7, NOT EXISTS(SELECT 1 FROM user_saved_cards WHERE user_id = $1 AND deleted_at IS NULL) + ON CONFLICT (square_card_id) DO UPDATE SET + brand = EXCLUDED.brand, + last_4 = EXCLUDED.last_4, + exp_month = EXCLUDED.exp_month, + exp_year = EXCLUDED.exp_year, + fingerprint = EXCLUDED.fingerprint, + deleted_at = NULL, + retained_until = NULL RETURNING id, is_default `, userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint).Scan(&savedCardID, &isDefault) if err != nil { diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go new file mode 100644 index 0000000..9835e69 --- /dev/null +++ b/backend/handlers/payments/sweep.go @@ -0,0 +1,74 @@ +package payments + +import ( + "context" + "log" + "time" + + "crussell/clock" + "crussell/db" +) + +// SweepStalePendingPayments marks pending payment records that are older than +// Square's idempotency-key retention window (~24h) as 'failed'. A pending +// record means the DB committed but the Square charge outcome is unknown; it +// normally resolves on a same-key client retry. But if the client abandoned +// the attempt, the record stays pending forever — and retrying it after the +// key expires would ISSUE A SECOND CHARGE (Square no longer dedups). Failing +// stale pendings closes that double-charge window: a late retry finds a +// 'failed' record and stops instead of charging again. +// +// Only online/till card payments can be pending — cash/giftcard/on_the_house +// are committed synchronously and never enter this state. Both the payments +// table and till_sales carry pending card-sale rows and are swept here. +// +// A swept row may have been genuinely charged at Square with a lost response — +// it is flagged with a CRITICAL manual-reconciliation log (like the refund +// sweep) so the money is not silently lost in limbo (MINOR-R3). +const stalePendingPaymentAge = 24 * time.Hour + +func SweepStalePendingPayments(ctx context.Context) (int, error) { + cutoff := clock.Now().Add(-stalePendingPaymentAge) + + tag, err := db.Conn.Exec(ctx, ` + UPDATE payments + SET status = 'failed', updated_at = NOW() + WHERE status = 'pending' + AND created_at < $1 + `, cutoff) + if err != nil { + return 0, err + } + payCount := int(tag.RowsAffected()) + + // till_sales rows for card payments (stored as 'online_square' or + // 'in_person_card' in the payment_method enum — saved_card/online_square/ + // card_machine requests all persist as one of those) can also be pending. + // Sweep them too — a lost-response till sale would otherwise stay pending + // and a retry after key retention would reuse the stored key → Square sees + // an expired key → second charge (R3). Cash / on_the_house are committed + // synchronously and never pending. + tillTag, err := db.Conn.Exec(ctx, ` + UPDATE till_sales + SET status = 'failed', updated_at = NOW() + WHERE status = 'pending' + AND created_at < $1 + AND payment_method IN ('online_square', 'in_person_card') + `, cutoff) + if err != nil { + return 0, err + } + tillCount := int(tillTag.RowsAffected()) + + total := payCount + tillCount + if total > 0 { + log.Printf("[SWEEP] Marked %d stale pending payments (%d payments, %d till sales) as failed (older than %s) — late retries will be rejected, preventing a second Square charge", total, payCount, tillCount, stalePendingPaymentAge) + } + if payCount > 0 { + log.Printf("CRITICAL: %d pending payments swept to failed may have been charged at Square with a lost response — manual reconciliation required before refunding/charging", payCount) + } + if tillCount > 0 { + log.Printf("CRITICAL: %d pending till sales swept to failed may have been charged at Square with a lost response — manual reconciliation required", tillCount) + } + return total, nil +} diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 4e42ae1..45cd25f 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -56,6 +56,12 @@ func uniqueTillKey() string { func CreateTillSale(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + // Defense-in-depth admin check (S-1) — a till sale moves money (charges a + // card / funds a gift card), so it must stay admin-only. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } adminID, _ := ctx.Value(mw.UserIDKey).(string) var req TillSaleRequest @@ -70,9 +76,6 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { return } - // M8 - // L5 - if req.ItemType != "gift_card" { http.Error(w, "Unsupported item type", http.StatusBadRequest) return @@ -180,6 +183,13 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { existingPendingID = existingID existingPendingGiftCard = existingItemID } + if existingStatus == "failed" { + // Swept as stale (>24h) or definitively rejected — a retry would + // risk a second Square charge. Reject cleanly (R2). + log.Printf("Till-sale retry rejected: record %s was marked failed", existingID) + http.Error(w, "This till sale previously failed and can no longer be retried", http.StatusConflict) + return + } } } @@ -558,6 +568,11 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { http.Error(w, "card_token is required for online_square payment — use a Square Web Payments nonce", http.StatusBadRequest) return } + // Tokenize the till card. The reference_id is the synthetic + // "till-" namespace, NOT a real user — this card is + // ephemeral (used once for this charge) and is never stored in + // user_saved_cards or re-listed. The prefix can't collide with a + // real CHAR(12)-hex user ID. cardOnFile, cardErr := SquareClient.CreateCardOnFile(ctx, "till-"+giftCardID, req.CardToken) if cardErr != nil { log.Printf("Failed to tokenize card: %v", cardErr) @@ -630,6 +645,11 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) { + // Defense-in-depth admin check (S-1). + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } checkoutID := chi.URLParam(r, "checkout_id") if checkoutID == "" { http.Error(w, "Checkout ID is required", http.StatusBadRequest) @@ -675,6 +695,33 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) { } if paymentResult.Status == "COMPLETED" { + // Serialize terminal-completion records per till sale — the one payment + // writer that previously lacked the advisory-lock pattern every other + // path uses. Two concurrent polls of the same checkout could both run + // the UPDATE + VAT (idempotent today, but a double-apply is a latent + // bug). Lock on the till-sale id so only one goroutine completes it. + pinConn, err := db.Conn.Acquire(r.Context()) + if err != nil { + log.Printf("Failed to acquire connection for till-completion lock: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer pinConn.Release() + if _, err := pinConn.Exec(r.Context(), ` + SELECT pg_advisory_lock(hashtext('crussell:tillcomplete:' || $1)) + `, tillSaleID); err != nil { + log.Printf("Failed to acquire till-completion serialization lock for %s: %v", tillSaleID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext('crussell:tillcomplete:' || $1)) + `, tillSaleID); err != nil { + log.Printf("Failed to release till-completion serialization lock for %s: %v", tillSaleID, err) + } + }() + tx, err := db.Conn.Begin(r.Context()) if err != nil { log.Printf("Failed to begin transaction: %v", err) diff --git a/backend/handlers/payments/till_test.go b/backend/handlers/payments/till_test.go index 5edfbaf..e9042d5 100644 --- a/backend/handlers/payments/till_test.go +++ b/backend/handlers/payments/till_test.go @@ -416,6 +416,7 @@ func TestGetTillCheckoutStatus_NotFound(t *testing.T) { rctx.URLParams.Add("checkout_id", "nonexistent") reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) req = req.WithContext(reqCtx) + req = adminRequestCtx(req) w := httptest.NewRecorder() GetTillCheckoutStatus(w, req) @@ -482,6 +483,8 @@ func TestGetTillCheckoutStatus_Pending(t *testing.T) { statusReqCtx = db.ContextWithTx(statusReqCtx, tx.(pgx.Tx)) statusReq = statusReq.WithContext(statusReqCtx) + statusReq = adminRequestCtx(statusReq) + wStatus := httptest.NewRecorder() GetTillCheckoutStatus(wStatus, statusReq) @@ -555,6 +558,8 @@ func TestGetTillCheckoutStatus_Completed(t *testing.T) { statusReqCtx = db.ContextWithTx(statusReqCtx, tx.(pgx.Tx)) statusReq = statusReq.WithContext(statusReqCtx) + statusReq = adminRequestCtx(statusReq) + wStatus := httptest.NewRecorder() GetTillCheckoutStatus(wStatus, statusReq) @@ -624,6 +629,8 @@ func TestGetTillCheckoutStatus_AlreadyCompleted(t *testing.T) { statusReqCtx := context.WithValue(statusReq.Context(), chi.RouteCtxKey, statusRCtx) statusReq = statusReq.WithContext(statusReqCtx) + statusReq = adminRequestCtx(statusReq) + wStatus := httptest.NewRecorder() GetTillCheckoutStatus(wStatus, statusReq) @@ -640,9 +647,11 @@ func TestGetTillCheckoutStatus_EmptyCheckoutID(t *testing.T) { rctx.URLParams.Add("checkout_id", "") reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) req = req.WithContext(reqCtx) + req = adminRequestCtx(req) w := httptest.NewRecorder() GetTillCheckoutStatus(w, req) + req = adminRequestCtx(req) if w.Code != http.StatusBadRequest { t.Errorf("expected status 400, got %d. body: %s", w.Code, w.Body.String()) diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 91067f7..10bd03d 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -34,23 +34,30 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { // in env vars (see Square Developer Console → Webhooks → Subscription). // Reference: https://developer.squareup.com/docs/webhooks/step3validate + // Fail closed: a missing signing key means the webhook cannot be verified, + // so reject rather than process unauthenticated events (S-4). Square + // always sends the signature header, so an unset key in production is a + // misconfiguration that must not silently accept forged events. signingKey := os.Getenv("SQUARE_WEBHOOK_SIGNATURE_KEY") notificationURL := os.Getenv("SQUARE_WEBHOOK_NOTIFICATION_URL") if notificationURL == "" { notificationURL = "http://localhost:8080/webhooks/square" } - if signingKey != "" { - signature := r.Header.Get("x-square-hmacsha256-signature") - if signature == "" { - log.Printf("Missing Square webhook signature header") - http.Error(w, "Invalid signature", http.StatusForbidden) - return - } - if !verifySquareSignature(body, signature, signingKey, notificationURL) { - log.Printf("Invalid Square webhook signature") - http.Error(w, "Invalid signature", http.StatusForbidden) - return - } + if signingKey == "" { + log.Printf("SQUARE_WEBHOOK_SIGNATURE_KEY is not set — rejecting webhook (fail-closed)") + http.Error(w, "webhook signature verification unavailable", http.StatusServiceUnavailable) + return + } + signature := r.Header.Get("x-square-hmacsha256-signature") + if signature == "" { + log.Printf("Missing Square webhook signature header") + http.Error(w, "Invalid signature", http.StatusForbidden) + return + } + if !verifySquareSignature(body, signature, signingKey, notificationURL) { + log.Printf("Invalid Square webhook signature") + http.Error(w, "Invalid signature", http.StatusForbidden) + return } var event SquareWebhookEvent diff --git a/backend/handlers/webhooks/webhooks_test.go b/backend/handlers/webhooks/webhooks_test.go index 552cb97..9715395 100644 --- a/backend/handlers/webhooks/webhooks_test.go +++ b/backend/handlers/webhooks/webhooks_test.go @@ -111,8 +111,20 @@ func makeWebhookRequest(body []byte, signature string, ctx context.Context) *htt return w } +// webhookTestEnv sets a signing key and returns a valid signature for the body +// (the fail-closed handler requires a verifiable signature on every request). +func webhookTestEnv(t *testing.T, body []byte) (signature string) { + t.Helper() + tKey := "test-signing-key" + tURL := "http://localhost:8080/webhooks/square" + mac := hmac.New(sha256.New, []byte(tKey)) + mac.Write([]byte(tURL)) + mac.Write(body) + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", tKey) + return base64.StdEncoding.EncodeToString(mac.Sum(nil)) +} + func TestHandleSquareWebhook_PaymentUpdated(t *testing.T) { - t.Parallel() event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_1", @@ -120,7 +132,8 @@ func TestHandleSquareWebhook_PaymentUpdated(t *testing.T) { Data: json.RawMessage(`{"id":"payment_1"}`), } body, _ := json.Marshal(event) - w := makeWebhookRequest(body, "", context.Background()) + sig := webhookTestEnv(t, body) + w := makeWebhookRequest(body, sig, context.Background()) if w.Code != http.StatusOK { t.Errorf("expected 200, got %d. body: %s", w.Code, w.Body.String()) } @@ -130,7 +143,6 @@ func TestHandleSquareWebhook_PaymentUpdated(t *testing.T) { } func TestHandleSquareWebhook_RefundUpdated(t *testing.T) { - t.Parallel() event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_1", @@ -138,14 +150,14 @@ func TestHandleSquareWebhook_RefundUpdated(t *testing.T) { Data: json.RawMessage(`{"id":"refund_1"}`), } body, _ := json.Marshal(event) - w := makeWebhookRequest(body, "", context.Background()) + sig := webhookTestEnv(t, body) + w := makeWebhookRequest(body, sig, context.Background()) if w.Code != http.StatusOK { t.Errorf("expected 200, got %d. body: %s", w.Code, w.Body.String()) } } func TestHandleSquareWebhook_DisputeCreated(t *testing.T) { - t.Parallel() event := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispute_1", @@ -153,14 +165,14 @@ func TestHandleSquareWebhook_DisputeCreated(t *testing.T) { Data: json.RawMessage(`{"id":"dispute_1"}`), } body, _ := json.Marshal(event) - w := makeWebhookRequest(body, "", context.Background()) + sig := webhookTestEnv(t, body) + w := makeWebhookRequest(body, sig, context.Background()) if w.Code != http.StatusOK { t.Errorf("expected 200 for dispute.created, got %d. body: %s", w.Code, w.Body.String()) } } func TestHandleSquareWebhook_UnknownEventType(t *testing.T) { - t.Parallel() event := SquareWebhookEvent{ Type: "invoice.created", EventID: "evt_unknown_1", @@ -168,25 +180,27 @@ func TestHandleSquareWebhook_UnknownEventType(t *testing.T) { Data: json.RawMessage(`{"id":"inv_1"}`), } body, _ := json.Marshal(event) - w := makeWebhookRequest(body, "", context.Background()) + sig := webhookTestEnv(t, body) + w := makeWebhookRequest(body, sig, context.Background()) if w.Code != http.StatusOK { t.Errorf("expected 200 for unknown event type, got %d. body: %s", w.Code, w.Body.String()) } } func TestHandleSquareWebhook_InvalidJSON(t *testing.T) { - t.Parallel() - w := makeWebhookRequest([]byte(`{invalid json}`), "", context.Background()) + body := []byte(`{invalid json}`) + sig := webhookTestEnv(t, body) + w := makeWebhookRequest(body, sig, context.Background()) if w.Code != http.StatusBadRequest { t.Errorf("expected 400 for invalid JSON, got %d. body: %s", w.Code, w.Body.String()) } } func TestHandleSquareWebhook_BodyTooLarge(t *testing.T) { - t.Parallel() // 600KB body exceeds the 512KB limit largeBody := []byte(strings.Repeat("a", 600*1024)) - w := makeWebhookRequest(largeBody, "", context.Background()) + sig := webhookTestEnv(t, largeBody) + w := makeWebhookRequest(largeBody, sig, context.Background()) if w.Code != http.StatusRequestEntityTooLarge { t.Errorf("expected 413 for oversized body, got %d. body: %s", w.Code, w.Body.String()) } @@ -236,14 +250,15 @@ func TestHandleSquareWebhook_NoSignatureWhenKeySet(t *testing.T) { } } -func TestHandleSquareWebhook_SignatureSkippedWhenKeyEmpty(t *testing.T) { +func TestHandleSquareWebhook_RejectedWhenKeyEmpty(t *testing.T) { t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") body := []byte(`{"type":"payment.updated","event_id":"evt_1"}`) - // Bad signature but key is empty, so verification should be skipped + // Fail-closed: an unset signing key means the webhook cannot be verified, + // so the request is rejected rather than accepted with a bad signature. w := makeWebhookRequest(body, "some-signature", context.Background()) - if w.Code != http.StatusOK { - t.Errorf("expected 200 when no key configured (dev stub), got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusServiceUnavailable { + t.Errorf("expected 503 when no key configured (fail-closed), got %d. body: %s", w.Code, w.Body.String()) } } diff --git a/backend/internal/jobs/cleanup.go b/backend/internal/jobs/cleanup.go index b8115ea..b9fe852 100644 --- a/backend/internal/jobs/cleanup.go +++ b/backend/internal/jobs/cleanup.go @@ -56,6 +56,16 @@ func RegisterAll(s *Scheduler) { Handler: payments.SweepPendingSquareRefunds, }) + // Offset from the refund sweep (which also writes payments rows) by one + // minute to avoid the two sweeps contending on the same table. + s.Register(Job{ + Name: "sweep-stale-pending-payments", + Schedule: "1,6,11,16,21,26,31,36,41,46,51,56 * * * *", + Timeout: 60 * time.Second, + Concurrency: 1, + Handler: payments.SweepStalePendingPayments, + }) + // === MID FREQUENCY — every minute (progressive rate limiter was on 30s) === s.Register(Job{ diff --git a/backend/internal/jobs/scheduler_test.go b/backend/internal/jobs/scheduler_test.go index 41ed830..3e705ee 100644 --- a/backend/internal/jobs/scheduler_test.go +++ b/backend/internal/jobs/scheduler_test.go @@ -413,8 +413,8 @@ func TestRegisterAll_RegistersExpectedJobs(t *testing.T) { s := New() RegisterAll(s) - if got := len(s.registry); got != 21 { - t.Fatalf("RegisterAll() registered %d jobs, want 21", got) + if got := len(s.registry); got != 22 { + t.Fatalf("RegisterAll() registered %d jobs, want 22", got) } registered := make(map[string]Job, len(s.registry)) @@ -473,6 +473,7 @@ func expectedJobNames() map[string]bool { "cleanup-rate-limiters": true, "cleanup-gdpr-export-cache": true, "sweep-pending-square-refunds": true, + "sweep-stale-pending-payments": true, "cleanup-progressive-rate-limiter": true, "cleanup-expired-loyalty-redemptions": true, "cleanup-old-idempotency-keys": true, diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index f236a6f..cfb47ce 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -108,7 +108,16 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* if m.ShouldFail { return nil, fmt.Errorf("mock: payment declined (simulated failure)") } - log.Printf("[SQUARE-MOCK] CreatePayment: amount=%d, reference=%s, source=%s", req.Amount, req.ReferenceID, req.SourceID) + // Do NOT log the full source token — it is a single-use nonce (cnon:) or a + // card reference (ccof:) that could be replayed. Log only its prefix and + // length for debugging (S-2). + sourcePrefix := "" + if len(req.SourceID) > 8 { + sourcePrefix = req.SourceID[:8] + "..." + } else { + sourcePrefix = req.SourceID + } + log.Printf("[SQUARE-MOCK] CreatePayment: amount=%d, reference=%s, source=%s", req.Amount, req.ReferenceID, sourcePrefix) mockSleep(1 * time.Second) m.mu.Lock() @@ -199,7 +208,6 @@ func (m *MockClient) CreateCheckout(ctx context.Context, req CreateCheckoutReq) Status: "PENDING", AmountMoney: req.Amount, Currency: req.Currency, - DeviceID: req.DeviceID, ReferenceID: req.ReferenceID, Note: req.Note, CreatedAt: now.Format(time.RFC3339), @@ -383,7 +391,11 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken str brand, last4 := detectCardInfo(cardToken) card := &CardOnFile{ ID: cardID, - CardID: fmt.Sprintf("ccof_mock_%d", now.UnixNano()), + // Prefix "ccof:" so the mock's own entry-method detection (and any + // consumer checking the prefix) sees ON_FILE, matching production where + // saved-card tokens are "ccof:xxx". An "ccof_mock_" id would silently + // exercise the KEYED path in tests while prod runs ON_FILE. + CardID: fmt.Sprintf("ccof:mock_%d", now.UnixNano()), Brand: brand, Last4: last4, ExpMonth: 12, diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index 801e235..2582860 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -39,6 +39,7 @@ type httpClient struct { baseURL string token string locationID string + deviceID string http *http.Client } @@ -52,6 +53,7 @@ func newHTTPClient() *httpClient { baseURL: baseURL, token: os.Getenv("SQUARE_ACCESS_TOKEN"), locationID: os.Getenv("SQUARE_LOCATION_ID"), + deviceID: os.Getenv("SQUARE_TERMINAL_DEVICE_ID"), http: &http.Client{Timeout: defaultHTTPTimeout}, } } @@ -174,9 +176,12 @@ type sqCard struct { CreatedAt string `json:"created_at"` } +// sqFee matches Square's processing_fee object. The fee amount lives in +// amount_money.amount, NOT a top-level amount field — reading the wrong shape +// made every PaymentResult.Fees 0 against the real API (N-9). type sqFee struct { - Amount int64 `json:"amount"` - Type string `json:"type"` + AmountMoney sqMoney `json:"amount_money"` + Type string `json:"type"` } // --- Terminal Checkout types --- @@ -206,7 +211,6 @@ type sqTerminalCheckout struct { ID string `json:"id"` Status string `json:"status"` AmountMoney sqMoney `json:"amount_money"` - DeviceID string `json:"device_id,omitempty"` ReferenceID string `json:"reference_id,omitempty"` Note string `json:"note,omitempty"` PaymentIDs []string `json:"payment_ids,omitempty"` @@ -268,7 +272,8 @@ type sqCreateCardResponse struct { } type sqListCardsResponse struct { - Cards []sqCard `json:"cards"` + Cards []sqCard `json:"cards"` + Cursor string `json:"cursor"` } type sqDisableCardResponse struct { @@ -312,6 +317,13 @@ func createCheckoutHTTP(ctx context.Context, req CreateCheckoutReq) (*CheckoutRe } func createCheckoutHTTPWithClient(ctx context.Context, req CreateCheckoutReq, hc *httpClient) (*CheckoutResult, error) { + // device_options is REQUIRED by Square's TerminalCheckout API. Prefer the + // per-request device ID, falling back to the env-configured terminal + // (SQUARE_TERMINAL_DEVICE_ID) so the field is always present. + deviceID := req.DeviceID + if deviceID == "" { + deviceID = hc.deviceID + } body := sqTerminalCheckoutRequest{ IdempotencyKey: req.IdempotencyKey, Checkout: sqTerminalCheckoutPayload{ @@ -319,11 +331,11 @@ func createCheckoutHTTPWithClient(ctx context.Context, req CreateCheckoutReq, hc ReferenceID: req.ReferenceID, Note: req.Note, CustomerID: req.CustomerID, + DeviceOptions: &sqDeviceOptions{ + DeviceID: deviceID, + }, }, } - if req.DeviceID != "" { - body.Checkout.DeviceOptions = &sqDeviceOptions{DeviceID: req.DeviceID} - } var resp sqTerminalCheckoutResponse if err := hc.doJSON(ctx, http.MethodPost, "/v2/terminals/checkouts", body, &resp); err != nil { return nil, err @@ -342,7 +354,11 @@ func getCheckoutHTTPWithClient(ctx context.Context, checkoutID string, hc *httpC } tc := tcResp.Checkout if tc.Status != "COMPLETED" { - if tc.Status == "PENDING" || tc.Status == "IN_PROGRESS" { + // PENDING / IN_PROGRESS / CANCEL_REQUESTED all mean the terminal + // hasn't finished — treat as still-polling. Anything else (e.g. + // CANCELED) is terminal but not a completed payment. + switch tc.Status { + case "PENDING", "IN_PROGRESS", "CANCEL_REQUESTED": return nil, ErrCheckoutPending } return nil, fmt.Errorf("square: checkout %s is %s (not COMPLETED)", checkoutID, tc.Status) @@ -372,13 +388,15 @@ func (e *squareAPIError) Unwrap() error { return e.err } // Definitive Square refund rejection codes — the refund was declined and can // never succeed, so retrying is pointless and the refund record should be // marked 'failed'. Anything else (transport errors, 5xx) is left ambiguous so -// callers leave the refund 'pending' for a scheduler retry. Note that -// PAYMENT_ALREADY_REFUNDED is intentionally absent — the money has already -// moved, so it maps to ErrRefundAlreadyProcessed instead of ErrRefundDeclined. +// callers leave the refund 'pending' for a scheduler retry. Codes match +// Square's documented Refunds error list (REFUND_DECLINED, REFUND_AMOUNT_INVALID, +// PAYMENT_NOT_REFUNDABLE); note PAYMENT_ALREADY_REFUNDED and +// REFUND_ALREADY_PENDING are intentionally absent — money is in flight or has +// moved, so they map to ErrRefundAlreadyProcessed instead of ErrRefundDeclined. var definitiveRefundCodes = map[string]bool{ - "REFUND_DECLINED": true, - "PAYMENT_REFUND_AMOUNT_EXCEEDED": true, - "INVALID_PAYMENT_ID": true, + "REFUND_DECLINED": true, + "REFUND_AMOUNT_INVALID": true, + "PAYMENT_NOT_REFUNDABLE": true, } func refundPaymentHTTP(ctx context.Context, req RefundPaymentReq) (*RefundResult, error) { @@ -398,7 +416,7 @@ func refundPaymentHTTPWithClient(ctx context.Context, req RefundPaymentReq, hc * if errors.As(err, &sqErr) && definitiveRefundCodes[sqErr.Code] { return nil, fmt.Errorf("%w: %v", ErrRefundDeclined, err) } - if errors.As(err, &sqErr) && sqErr.Code == "PAYMENT_ALREADY_REFUNDED" { + if errors.As(err, &sqErr) && (sqErr.Code == "PAYMENT_ALREADY_REFUNDED" || sqErr.Code == "REFUND_ALREADY_PENDING") { return nil, fmt.Errorf("%w: %v", ErrRefundAlreadyProcessed, err) } return nil, err @@ -442,9 +460,11 @@ func createCardOnFileHTTPWithClient(ctx context.Context, userID, cardToken strin // Deterministic idempotency key derived from user + card (not time-based) // so that retries with the same details don't create duplicate cards. // SHA-256 hash prevents recovering the card token from the key itself. + // Truncated to ≤45 chars — Square's documented idempotency-key limit for + // /v2/cards (a full 64-hex hash would be rejected with a 400). ikHash := sha256.Sum256([]byte(userID + "|" + cardToken)) body := sqCreateCardRequest{ - IdempotencyKey: fmt.Sprintf("create-card-%x", ikHash), + IdempotencyKey: "card-" + fmt.Sprintf("%x", ikHash)[:38], SourceID: cardToken, Card: sqCardPayload{ // The app does not provision Square customers, so the local user @@ -469,15 +489,25 @@ func getCardsOnFileHTTPWithClient(ctx context.Context, userID string, hc *httpCl // Filter by reference_id natively: Square's List Cards API supports the // reference_id query param, and cards are created with reference_id = the // local user ID (the app has no Square customers, so customer_id cannot be - // used). This avoids both the invalid customer_id filter and a client-side - // filter across a cursor-paginated list. - var resp sqListCardsResponse - if err := hc.doJSON(ctx, http.MethodGet, "/v2/cards?reference_id="+url.QueryEscape(userID), nil, &resp); err != nil { - return nil, err + // used). List Cards pages at 25 cards, so loop on the cursor to avoid + // silently truncating a large saved-card list (N-10). + var cards []CardOnFile + path := "/v2/cards?reference_id=" + url.QueryEscape(userID) + for page := 0; page < 20; page++ { + var resp sqListCardsResponse + if err := hc.doJSON(ctx, http.MethodGet, path, nil, &resp); err != nil { + return nil, err + } + for i := range resp.Cards { + cards = append(cards, *cardFromSquare(&resp.Cards[i], userID)) + } + if resp.Cursor == "" { + break + } + path = "/v2/cards?reference_id=" + url.QueryEscape(userID) + "&cursor=" + url.QueryEscape(resp.Cursor) } - cards := make([]CardOnFile, 0, len(resp.Cards)) - for i := range resp.Cards { - cards = append(cards, *cardFromSquare(&resp.Cards[i], userID)) + if cards == nil { + cards = []CardOnFile{} } return cards, nil } @@ -515,7 +545,7 @@ func paymentFromSquare(sq *sqPayment) *PaymentResult { r.TipAmount = sq.TipMoney.Amount } for _, f := range sq.ProcessingFee { - r.Fees += f.Amount + r.Fees += f.AmountMoney.Amount } if sq.CardDetails != nil { cd := sq.CardDetails @@ -543,7 +573,6 @@ func checkoutFromSquare(sq *sqTerminalCheckout) *CheckoutResult { Status: sq.Status, AmountMoney: sq.AmountMoney.Amount, Currency: sq.AmountMoney.Currency, - DeviceID: sq.DeviceID, ReferenceID: sq.ReferenceID, Note: sq.Note, PaymentIDs: sq.PaymentIDs, diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index 5402d45..02c96c2 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -284,7 +284,9 @@ func TestCreatePaymentHTTP_TipMoneyAbsentWhenNil(t *testing.T) { } // TestRefundPaymentHTTP_CodeClassification verifies definitive refund rejection -// codes map to ErrRefundDeclined, PAYMENT_ALREADY_REFUNDED maps to +// codes (Square's documented list: REFUND_DECLINED, REFUND_AMOUNT_INVALID, +// PAYMENT_NOT_REFUNDABLE) map to ErrRefundDeclined, the money-in-flight codes +// (PAYMENT_ALREADY_REFUNDED, REFUND_ALREADY_PENDING) map to // ErrRefundAlreadyProcessed, and ambiguous errors pass through unwrapped. func TestRefundPaymentHTTP_CodeClassification(t *testing.T) { cases := []struct { @@ -294,9 +296,10 @@ func TestRefundPaymentHTTP_CodeClassification(t *testing.T) { wantErrNil bool }{ {name: "refund_declined", code: "REFUND_DECLINED", wantErrIs: ErrRefundDeclined}, - {name: "amount_exceeded", code: "PAYMENT_REFUND_AMOUNT_EXCEEDED", wantErrIs: ErrRefundDeclined}, - {name: "invalid_payment_id", code: "INVALID_PAYMENT_ID", wantErrIs: ErrRefundDeclined}, + {name: "amount_invalid", code: "REFUND_AMOUNT_INVALID", wantErrIs: ErrRefundDeclined}, + {name: "payment_not_refundable", code: "PAYMENT_NOT_REFUNDABLE", wantErrIs: ErrRefundDeclined}, {name: "already_refunded", code: "PAYMENT_ALREADY_REFUNDED", wantErrIs: ErrRefundAlreadyProcessed}, + {name: "already_pending", code: "REFUND_ALREADY_PENDING", wantErrIs: ErrRefundAlreadyProcessed}, {name: "ambiguous_code", code: "INTERNAL_SERVER_ERROR", wantErrIs: nil}, {name: "ambiguous_non_json", code: "", wantErrIs: nil}, // raw text body } @@ -551,10 +554,15 @@ func TestCreateCardOnFileHTTP_IdempotencyKey(t *testing.T) { } sum := sha256.Sum256([]byte("user_1|cnon:test-card")) - wantIK := fmt.Sprintf("create-card-%x", sum) + wantIK := "card-" + fmt.Sprintf("%x", sum)[:38] if captured["idempotency_key"] != wantIK { t.Errorf("expected idempotency_key %q, got %v", wantIK, captured["idempotency_key"]) } + // Square's documented idempotency-key limit for /v2/cards is 45 chars — + // the truncated key must never exceed it (C-1 regression guard). + if len(wantIK) > 45 { + t.Errorf("idempotency_key %q is %d chars, exceeds Square's 45-char limit", wantIK, len(wantIK)) + } if captured["source_id"] != "cnon:test-card" { t.Errorf("expected source_id cnon:test-card, got %v", captured["source_id"]) } diff --git a/backend/internal/square/types.go b/backend/internal/square/types.go index c3d9963..d0c41f8 100644 --- a/backend/internal/square/types.go +++ b/backend/internal/square/types.go @@ -100,7 +100,6 @@ type CheckoutResult struct { Status string // "PENDING", "IN_PROGRESS", "COMPLETED", "CANCELED", "FAILED" AmountMoney int64 // checkout amount in pence Currency string // "GBP" - DeviceID string // terminal device ID ReferenceID string // client reference Note string // optional note PaymentIDs []string // payment ID(s) once completed diff --git a/backend/internal/validators/validators.go b/backend/internal/validators/validators.go index 5dee1e0..ae4ff80 100644 --- a/backend/internal/validators/validators.go +++ b/backend/internal/validators/validators.go @@ -35,6 +35,19 @@ func IsValidID(id string) bool { return validIDRegex.MatchString(id) } +// Square checkout IDs are opaque strings (e.g. "08YceKh7B3ZqO") — NOT local +// 12-hex DB IDs, so IsValidID must not gate them (it would 404 every real +// checkout). Accept any non-empty ID matching Square's character set with a +// sane length bound, and reject anything that could inject into the URL path. +var squareCheckoutIDRegex = regexp.MustCompile(`^[A-Za-z0-9_\-]{8,64}$`) + +func IsValidSquareCheckoutID(id string) bool { + if id == "" { + return false + } + return squareCheckoutIDRegex.MatchString(id) +} + // ParseCursor splits a "createdAt|id" cursor string into its components. func ParseCursor(cursor string) (time.Time, string, error) { parts := strings.SplitN(cursor, "|", 2) diff --git a/backend/internal/validators/validators_test.go b/backend/internal/validators/validators_test.go index e1ecad6..58fd6b1 100644 --- a/backend/internal/validators/validators_test.go +++ b/backend/internal/validators/validators_test.go @@ -76,3 +76,18 @@ func TestValidate_ValidStruct(t *testing.T) { err := Validate.Struct(validStruct{Name: "hello"}) assert.NoError(t, err) } + +func TestIsValidSquareCheckoutID_AcceptsRealSquareID(t *testing.T) { + t.Parallel() + // Real Square checkout IDs are opaque UUIDs, not local 12-hex DB IDs. + assert.True(t, IsValidSquareCheckoutID("08YceKh7B3ZqO")) + assert.True(t, IsValidSquareCheckoutID("a1b2c3d4e5f6")) +} + +func TestIsValidSquareCheckoutID_RejectsInvalid(t *testing.T) { + t.Parallel() + assert.False(t, IsValidSquareCheckoutID("")) + assert.False(t, IsValidSquareCheckoutID("../etc/passwd")) + assert.False(t, IsValidSquareCheckoutID("has spaces")) + assert.False(t, IsValidSquareCheckoutID("abc")) // too short +} diff --git a/frontend/src/lib/components/payments/PaymentModal.svelte b/frontend/src/lib/components/payments/PaymentModal.svelte index a7f745c..9d0648f 100644 --- a/frontend/src/lib/components/payments/PaymentModal.svelte +++ b/frontend/src/lib/components/payments/PaymentModal.svelte @@ -67,9 +67,10 @@ let savedCardList = $state< Array<{ id: string; - card_brand: string; - card_last4: string; - card_expiry: string; + brand: string; + last_4: string; + exp_month: number; + exp_year: number; cardholder_name?: string; }> >([]); @@ -559,13 +560,16 @@ } } - // Saved cards + // Saved cards — fields match the backend SavedCard shape (brand, last_4, + // exp_month, exp_year), not the old card_brand/card_last4/card_expiry names + // which rendered blank. let savedCards = $state< Array<{ id: string; - card_brand: string; - card_last4: string; - card_expiry: string; + brand: string; + last_4: string; + exp_month: number; + exp_year: number; cardholder_name?: string; }> >([]); @@ -1183,11 +1187,11 @@ - {card.card_brand} ••••{card.card_last4} + {card.brand} ••••{card.last_4} - {card.card_expiry} + {String(card.exp_month).padStart(2, '0')}/{card.exp_year} {#if card.cardholder_name}
{card.cardholder_name}
diff --git a/frontend/src/lib/square/square.ts b/frontend/src/lib/square/square.ts index 804bd6d..aad0ea1 100644 --- a/frontend/src/lib/square/square.ts +++ b/frontend/src/lib/square/square.ts @@ -22,12 +22,18 @@ export function getSquareConfig(): SquareConfig | null { return { appId: APP_ID, locationId: LOCATION_ID }; } +// Pinned SDK version: Square.js is loaded from the /v1/ path, which is +// Square's stable major version. Square does not publish SRI hashes, so no +// integrity attribute can be set — the /v1/ version pin is the supply-chain +// control (S-3). Monitor Square's release notes before major bumps. +const SQUARE_SDK_VERSION = 'v1'; + function sdkUrl(): string { const env = (import.meta.env.VITE_SQUARE_ENVIRONMENT as string | undefined) ?? ''; const isSandbox = env === 'sandbox' || (APP_ID !== '' && APP_ID.startsWith('sandbox-')); return isSandbox - ? 'https://sandbox.web.squarecdn.com/v1/square.js' - : 'https://web.squarecdn.com/v1/square.js'; + ? `https://sandbox.web.squarecdn.com/${SQUARE_SDK_VERSION}/square.js` + : `https://web.squarecdn.com/${SQUARE_SDK_VERSION}/square.js`; } let sdkPromise: Promise | null = null; diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index ec9b034..093014e 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -1984,7 +1984,9 @@ CREATE INDEX idx_user_saved_cards_active ON user_saved_cards(user_id, deleted_at CREATE TABLE refunds ( id CHAR(12) PRIMARY KEY DEFAULT generate_refund_id(), payment_id CHAR(12) NOT NULL REFERENCES payments(id) ON DELETE CASCADE, - booking_id CHAR(12) NOT NULL REFERENCES bookings(id) ON DELETE CASCADE, + -- booking_id is NULL for non-booking payments (e.g. gift-card purchase + -- refunds); the payment row still links the refund via payment_id. + booking_id CHAR(12) REFERENCES bookings(id) ON DELETE CASCADE, amount NUMERIC(10,2) NOT NULL CHECK (amount > 0), square_refund_id TEXT, status payment_status NOT NULL DEFAULT 'pending', diff --git a/obsidian/Crussell/Feature Catalog.md b/obsidian/Crussell/Feature Catalog.md index d7e8352..3be681a 100644 --- a/obsidian/Crussell/Feature Catalog.md +++ b/obsidian/Crussell/Feature Catalog.md @@ -205,11 +205,11 @@ Multi-method payment system accepting Square (card terminal & online), cash, gif **Related:** [[Booking System|1. Booking System]] (completed bookings trigger tip eligibility), [[Today Page|7. Today Page]] (daily tips summary) ### 2.7 Refunds -**What it does:** Admin processes refunds with automatic routing by payment method. Square card payments are refunded via Square API (post-DB-commit). Cash refunds are credited as gift card balance. Gift card payments are returned to the card's balance. +**What it does:** Admin processes refunds with automatic routing by payment method. Square card payments are refunded via Square API (post-DB-commit). Cash refunds are credited as gift card balance. Gift card payments are returned to the card's balance. Refunds are resolved by Square's actual status (COMPLETED→completed, PENDING→left pending for the sweep, FAILED/REJECTED→failed) so an in-flight refund never blocks the over-refund guard. A background sweep (`sweep-pending-square-refunds`) retries/reconciles stuck refunds with a 23h age guard and surfaces failures as admin notifications. **Layman summary:** "If a booking is cancelled, the system automatically refunds the right amount to the right place." -**Related:** [[Cancellation|1.11 Cancellation]], [[Refund to Gift Card|4.11 Refund to Gift Card]] +**Related:** [[Cancellation|1.11 Cancellation]], [[Refund to Gift Card|4.11 Refund to Gift Card]], [[Background Jobs|13. Background Jobs]] (refund sweep) ### 2.8 Payment Split Logic (`buildSplitRecords`) **What it does:** When a customer pays online, a single Square charge is automatically split into up to 3 records: deposit portion (first 50%), balance portion (remaining owed), and tip portion (overflow beyond the total). This protects the deposit accounting. @@ -219,11 +219,11 @@ Multi-method payment system accepting Square (card terminal & online), cash, gif **Related:** [[Deposit System|1.2 Deposit System]], [[Tips|2.6 Tips]] ### 2.9 Double-Payment Prevention -**What it does:** Two mechanisms prevent accidental double-charges: PostgreSQL advisory locks (serialize payment attempts per-booking at the database level) and idempotency keys (client-generated unique IDs). +**What it does:** Two mechanisms prevent accidental double-charges: PostgreSQL advisory locks (serialize payment attempts per-booking at the database level) and idempotency keys (client-generated unique IDs, or deterministic keys derived from booking+type+amount+card for the admin saved-card path). Same-key retries reuse the original pending record instead of charging again. A background sweep fails stale pending payments older than Square's ~24h key-retention window, so a late retry is cleanly rejected rather than issuing a second charge. **Layman summary:** "If you open two tabs and try to pay twice, only one payment goes through." -**Related:** [[Idempotency Keys|1.12 Idempotency Keys]], [[Idempotent Purchases|4.10 Idempotent Purchases]] +**Related:** [[Idempotency Keys|1.12 Idempotency Keys]], [[Idempotent Purchases|4.10 Idempotent Purchases]], [[Background Jobs|13. Background Jobs]] (stale-pending sweep) ### 2.10 VAT Calculation **What it does:** Applies VAT to payments and till sales based on business settings (rate, registration status, SPV/MPV voucher type). Discount, on-the-house, and tip payments are exempt. @@ -473,7 +473,7 @@ The central management hub for salon operations — managing users, bookings, se **Related:** [[Patch Tests|1.6 Patch Tests]], [[Services Catalog|1.5 Services Catalog]] ### 5.9 Till Purchases (POS) -**What it does:** Point-of-sale interface for selling gift cards at the counter (cash, card machine, saved card, online card entry, on the house). +**What it does:** Point-of-sale interface for selling gift cards at the counter (cash, card machine, saved card, online card entry, on the house). The saved-card path charges the customer's card-on-file directly via Square (pending-first, deterministic idempotency key, advisory lock per booking); card-machine sales create a Square Terminal checkout and are completed by polling. Pending card till-sales are failed by the stale-pending sweep after Square's ~24h key-retention window. **Layman summary:** "Sell gift cards at the till — take cash or card." @@ -842,7 +842,7 @@ The platform infrastructure — Docker Compose stack, CI/CD, local development e ## 13. Background Jobs (Cron Scheduler) -A centralized cron scheduler that runs 20 maintenance jobs for cleanup, transitions, and data management. +A centralized cron scheduler that runs 22 maintenance jobs for cleanup, transitions, and data management. **Related:** [[Availability & Scheduling|3. Scheduling]] (hours apply), [[GDPR & Compliance|9. GDPR & Compliance]] (cleanup), [[Gift Cards|4. Gift Cards]] (expiry/cleanup), [[Payments|2. Payments]] (idempotency cleanup) @@ -851,8 +851,10 @@ A centralized cron scheduler that runs 20 maintenance jobs for cleanup, transiti - **cleanup-expired-deposits**: Move unpaid bookings to `pending_release` state - **cleanup-rate-limiters**: Purge stale rate limiter entries - **cleanup-gdpr-export-cache**: Expire old GDPR export caches +- **sweep-pending-square-refunds**: Reconcile/retry stuck Square refunds (23h age guard; aggregates per charge, single refund per charge, resolves by Square status, notifies on failure) — starts on the `:00` ticks +- **sweep-stale-pending-payments**: Fail stale pending payments and till-sales older than Square's ~24h idempotency-key retention, so a late retry is cleanly rejected instead of issuing a second charge — starts on the `:01` ticks, offset one minute from the refund sweep to avoid table contention -**Related:** [[Slot Reservation TTLs|1.14 Slot Reservation TTLs]], [[Deposit System|1.2 Deposit System]] +**Related:** [[Slot Reservation TTLs|1.14 Slot Reservation TTLs]], [[Deposit System|1.2 Deposit System]], [[Payments|2. Payments]] (refund sweeps, stale-pending payment sweep) ### 13.2 Every Minute - **cleanup-progressive-rate-limiter**: Clean progressive rate limiter state diff --git a/obsidian/Crussell/Future Work - Gap Backlog.md b/obsidian/Crussell/Future Work - Gap Backlog.md index 067d6f3..fad19fa 100644 --- a/obsidian/Crussell/Future Work - Gap Backlog.md +++ b/obsidian/Crussell/Future Work - Gap Backlog.md @@ -1,10 +1,10 @@ # Future Work — Gap Backlog -**Last Updated:** July 2026 +**Last Updated:** August 2026 **Status:** Living backlog — add to this as gaps are discovered **Previous version:** OUT OF DATE — this replaces the prior document. Completed items removed, new items added from exhaustive codebase audit. -This document has three lists: **Pre-Launch** (integration tasks), **MVP** (missing features for daily ops), **Stretch** (nice-to-have). Items are numbered sequentially. All done items are removed without tracking. +This document has three lists: **Pre-Launch** (integration tasks), **MVP** (missing features for daily ops), **Stretch** (nice-to-have). Items are numbered sequentially. All done items are removed without tracking (see "Previously Completed" sections). ### Development Context @@ -25,17 +25,16 @@ These are things that work fine in dev (with mocks) but need real implementation | # | Task | Effort | Area | Dev Status | Notes | |---|---|---|---|---|---| -| P1 | **Square payments: wire prod client alongside dev mock** | XL (5-7d) | Backend | ✅ **COMPLETED Aug 2026** — `internal/square/square_http_client.go` implements the real REST client (payments, terminal checkouts, refunds, cards, list-refunds). Prod client (`internal/square/square.go`) and dev `devProdClient` (`square_dev.go`) both call real Square when `SQUARE_ENVIRONMENT=sandbox|production`; `mock` uses the in-memory client. The health endpoint reports `"mock"`/`"ok"` accordingly (was `"not_implemented"`). | | | P2 | **S3/R2 storage: implement prod side of the abstraction** | M (2-3d) | Backend | Dev works (`internal/s3/s3_dev.go` — RustFS + in-memory fallback). Prod side (`internal/s3/s3.go:52-62`) returns "not implemented" for Upload/Download/Delete. The prod `S3Client` struct lacks the `*s3.Client` field entirely — it was never populated. | The storage abstraction was defined early and the dev side got a full implementation. The prod side needs the AWS SDK v2 dependency and real S3/R2 calls. Portfolio images and profile pictures will start working in prod once this is done. | | P3 | **Square webhook event handling: from log-only to action** | S (1d) | Backend | Webhook signature verification works (HMAC-SHA256, references Square docs). Event parsing works. But `handlePaymentUpdated` and `handleRefundUpdated` (`square.go:88-94`) only log the event data — they never update booking/payment state. | The webhook receiver was built first (parse + verify). The handlers that act on events were deferred. Now they need to: update payment status on `payment.updated`, update refund status on `refund.updated`. | -| P4 | **Payment reconciliation: add recovery for split-brain scenarios** | L (3-5d) | Backend | 11 `log.Printf("CRITICAL: ... manual reconciliation required")` calls exist across payment, refund, and till handlers. When Square succeeds but the DB transaction fails afterwards, state diverges with no automated recovery. | This happens when the application correctly processes a Square payment but then hits a DB error on commit. In dev, this was handled by just logging it. For prod, we need a reconciliation job or retry mechanism. | +| P4 | **Payment reconciliation: add recovery for split-brain scenarios** | L (3-5d) | Backend | 20 `log.Printf("CRITICAL: ... manual reconciliation required")` calls exist across payment, refund, and till handlers. When Square succeeds but the DB transaction fails afterwards, state diverges with no automated recovery. | This happens when the application correctly processes a Square payment but then hits a DB error on commit. In dev, this was handled by just logging it. For prod, we need a reconciliation job or retry mechanism. (Count grew from 18 to 20 with the stale-pending sweep's manual-reconciliation warnings in the Aug 2026 review round.) | | P5 | **Till Purchases: wire the backend payment flow** | M (1d) | Frontend + Backend | Frontend (`TillPurchases.svelte:203-208`) has the UI built but the Charge button is disabled with "Payment flow and backend integration coming soon." The till sale submission path was deferred. | The till UI is fully designed — service selection, gift card types, payment method selection. Only the final "submit payment" path was left as a placeholder. Needs the backend `till.go` sale endpoint wired. | | P6 | **Email/SMS notification delivery** | XL (5-7d) | Backend | `user_notification_preferences` table stores delivery preferences. 8 TODO markers reference this blocker. Notification creation works (admin_notifications table), but no delivery channel exists. No SMTP configuration, no SMS provider. 2 tests skipped as "WIP handler." | The notification queue works (reasons, priorities, acknowledging). What's missing is the delivery backend. Affects: slot eviction alerts, edit request approvals/denials, gift card codes, unpaid booking reminders, idle account warnings. | | P7 | **Production security headers** | S (1h) | Backend | HSTS and Referrer-Policy headers are commented out in `main.go:231-233` with TODO markers. They were left disabled for dev HTTP convenience. | Uncomment and configure for production. | | P8 | **Social auth stubs (Google/Microsoft/Facebook)** | L (2-3d) | Backend + Frontend | `handlers/auth/social.go` is 1 line (`package auth`). Frontend login page has 3 social buttons that show `toast.info("${provider} login coming soon")`. The `user_social_logins` table and `account_type` enum values exist from early schema design. | The schema was designed for social auth from the start (table + enum values). The OAuth flow itself was never implemented. Buttons exist as UI placeholders. | -| P9 | **Tip payments: replace placeholder card tokens** | S (1d) | Frontend | ✅ COMPLETED July 2026 — `card_token: 'placeholder'` replaced with real saved card selection + CardInput with Luhn/expiry/CVC validation across all 3 tip pages. | | | P10 | **No automated database backups** | M (1d) | Infrastructure | PostgreSQL volume is persistent in Docker but no `pg_dump` cron, no point-in-time recovery. | Standard production DB setup task. | -| P11 | **Square Web Payments SDK: re-enable new-card entry with nonce-based flow** | S-M (2-3d) | Frontend | ✅ **COMPLETED Aug 2026** — `SquareCardInput.svelte` tokenizes cards to `cnon:` nonces via the Web Payments SDK (env-gated on `VITE_SQUARE_APPLICATION_ID`/`VITE_SQUARE_LOCATION_ID`); all 8 flows re-enabled (tips ×3, booking payment, deposit, Buy a Gift Card, account Add Card, till `online_square`); `CardEntryUnavailable` kept only as the no-credentials fallback. See `plans/p11-square-web-payments-sdk.md`. | | +| P12 | **Square sandbox smoke test (pre-go-live gate)** | S-M (1d, once credentials available) | E2E | **BLOCKED — no real Square credentials available.** Must exercise the real API path end-to-end: new-card tokenization → payment → saved card → refund → reconcile, against Square's sandbox. Also verifies the M-8 open question (is `card.customer_id` enforced as Required?). | The dev mock cannot exercise Square's real wire contract (key-length limits, `device_options`, refund statuses, error codes). This is the sole remaining item before the production flip. See `plans/p11-square-web-payments-sdk.md` Remaining Items. | +| P13 | **Reconcile deterministically-keyed saved-card charges** | S (2-3h) | Backend | **Deferred — deliberate trade-off (N-OBS-1).** The admin "Charge Saved Card" idempotency key `bookingID-sc-type-amount-cardID` dedups two *identical* repeat charges on one booking. Not UI-reachable today (PaymentModal always sends the current `totalDue`, which changes after each charge). | Revisit if the admin flow ever gains a "charge exact amount twice" path — the key would then need a client nonce or attempt counter. Tracked from the final payment review. | --- @@ -50,7 +49,6 @@ These are missing functionality that prevents daily operations, legal compliance | M3 | **Email verification calls wrong API endpoint** | S (2h) | Frontend | `+layout.svelte:33` calls `/api/verify-email` which 404s. Correct endpoints: `POST /api/verify/generate` and `POST /api/verify/check`. Every login triggers a silent failure. | | M4 | **CSRF protection** | S (2-3h) | Backend | SvelteKit handles CSRF for its own forms, but direct API calls to `/api/*` bypass it. | | M5 | **XSS input sanitization** | S (2-3h) | Backend | Backend validates format (regex, length) but doesn't sanitize HTML entities in stored fields. CSP mitigates but doesn't eliminate risk. | -| M6 | **Square webhook signature verification — enforce always** | S (1-2h) | Backend | Currently skips if `SQUARE_WEBHOOK_SIGNATURE_KEY` is empty. Prod must always verify. Implementation exists (HMAC-SHA256), just needs enforcement. | | M7 | **Booking cancellation UI from user account** | S (2-3h) | Frontend | `UserBookingModal` shows details but has no cancel button. Backend `DELETE /api/bookings/{id}` exists. | | M8 | **Business settings management UI** | M (1-2d) | Frontend | `GET/PUT /api/admin/settings` endpoints exist. No admin page — staff use curl or SQL. | | M9 | **CSV/Excel export for bookings/payments** | M (1d) | Backend | No endpoint for accounting software export. SQL functions exist but not wired. | @@ -98,21 +96,28 @@ These don't add features but reduce maintenance cost and risk. | T4 | **Create or remove documented `update_data_consent()` function** | S (1h) | DB Schema | Listed in FUNCTION USAGE SUMMARY comment (~line 2401) but no `CREATE FUNCTION` exists. | | T5 | **Resolve 2 route-conflicted lint-ignored handlers** | S (1h) | Backend | `manage.go:27,314` — handlers exist only for tests but routes conflict. | | T6 | **Resolve portfolio lint-ignored handler** | S (1h) | Backend | `images.go:53` — handler referenced from tests only, never routed. | -| T7 | **Fix README job count: 20 not 21** | S (5min) | Docs | README says "21 maintenance jobs", code registers 20. | +| T7 | **Fix README job count: 22 not 21** | S (5min) | Docs | ✅ **COMPLETED Aug 2026** — README updated to 22 maintenance jobs (the two payment sweeps added in the review round: `sweep-pending-square-refunds`, `sweep-stale-pending-payments`). | | T8 | **Audit 18 silent catch blocks** | M (1d) | Frontend | 1 `catch (e) {}`, 17 `catch (_err)` — errors swallowed silently. Many should show user-facing toasts. | | T9 | **33 `svelte/no-navigation-without-resolve` suppressions** | M (1d) | Frontend | Create a project-wide `goto` wrapper instead of suppressing per-file. | | T10 | **Replace `as any` in HolidayHours** | S (30min) | Frontend | `HolidayHours.svelte:234` — `(group.hours as any[])?.map(…)`. Hours array has known shape. | | T11 | **Replace `e: any` in button onclick** | S (30min) | Frontend | `button.svelte:101` — click handler typed as `e: any`. | | T12 | **Former name display (4 TODO sites)** | S (1d) | Frontend + Backend | 4 TODOs across GiftCards + notifications needing `previousFirstName`/`previousLastName` from backend. | | T13 | **Fix `devProdClient` rune-arithmetic in test** | S (30min) | Backend | ✅ COMPLETED July 2026 — `rune('0'+idx)` replaced with `fmt.Sprintf("concurrent-key-%d", idx)` for proper numeric formatting beyond index 9. | -| T14 | **Error tracking / monitoring (Sentry)** | M (1-2d) | Backend | `log.Printf()` only. No alerting on 5xx. 39 ALERT + 11 CRITICAL logs will never be seen. | +| T14 | **Error tracking / monitoring (Sentry)** | M (1-2d) | Backend | `log.Printf()` only. No alerting on 5xx. 39 ALERT + 20 CRITICAL logs will never be seen. | --- +## Previously Completed Items (August 2026 backlog) + +- ~~**Square payments: wire prod client alongside dev mock (P1)** — `internal/square/square_http_client.go` implements the real REST client (payments, terminal checkouts, refunds, cards, list-refunds). Prod client (`internal/square/square.go`) and dev `devProdClient` (`square_dev.go`) both call real Square when `SQUARE_ENVIRONMENT=sandbox|production`; `mock` uses the in-memory client. The health endpoint reports `"mock"`/`"ok"` accordingly (was `"not_implemented"`).~~ +- ~~**Square Web Payments SDK: re-enable new-card entry with nonce-based flow (P11)** — `SquareCardInput.svelte` tokenizes cards to `cnon:` nonces via the Web Payments SDK (env-gated on `VITE_SQUARE_APPLICATION_ID`/`VITE_SQUARE_LOCATION_ID`); all 8 flows re-enabled (tips ×3, booking payment, deposit, Buy a Gift Card, account Add Card, till `online_square`); `CardEntryUnavailable` kept only as the no-credentials fallback. See `plans/p11-square-web-payments-sdk.md`.~~ +- ~~**Square webhook signature verification — enforce always (M6)** — the webhook handler is now **fail-closed**: rejects with 503 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is unset and 403 when the signature header is missing/invalid (`handlers/webhooks/square.go`).~~ +- ~~**Fix README job count (T7)** — README updated to 22 maintenance jobs.~~ +- ~~**Fix `devProdClient` rune-arithmetic in test (T13)** — `rune('0'+idx)` replaced with `fmt.Sprintf("concurrent-key-%d", idx)`.~~ + ## Previously Completed Items (July 2026 backlog) -- ~~**Tip payments: replace placeholder card tokens (P9)** — `card_token: 'placeholder'` replaced with real saved card selection + CardInput + Luhn/expiry/CVC validation across all 3 tip pages. CardBrandIcon SVGs added for all Square-supported brands.~~ -- ~~**Fix `devProdClient` rune-arithmetic in test (T13)** — `rune('0'+idx)` replaced with `fmt.Sprintf("concurrent-key-%d", idx)`.~~ +- ~~**Tip payments: replace placeholder card tokens (P9)** — `card_token: 'placeholder'` replaced with real saved card selection + CardInput with Luhn/expiry/CVC validation across all 3 tip pages. CardBrandIcon SVGs added for all Square-supported brands.~~ ## Previously Completed Items (June 2026 backlog) diff --git a/obsidian/Crussell/Overview.md b/obsidian/Crussell/Overview.md index 569bd90..c3e9d31 100644 --- a/obsidian/Crussell/Overview.md +++ b/obsidian/Crussell/Overview.md @@ -34,7 +34,7 @@ Square integration has two build-tagged implementations: - **Dev** (`//go:build dev`): Mock client simulates async checkout with polling. No real payments. - **Prod** (`//go:build !dev`): Connects to live Square API. Requires Square credentials in `.env`. -Saved cards stored in `user_saved_cards` with soft delete (`retained_until` for 7-year UK compliance). Refunds tracked in `refunds` table — partial or full. Square webhooks at `/api/webhooks/square` receive payment/refund events (HMAC-verified; currently log-only — status is tracked via the synchronous + sweep/reconcile paths, backlog P3). +Saved cards stored in `user_saved_cards` with soft delete (`retained_until` for 7-year UK compliance). Refunds tracked in `refunds` table — partial or full. Square webhooks at `/api/webhooks/square` receive payment/refund events (HMAC-verified **fail-closed** — 503 without the signing key, 403 on bad signature; currently log-only — status is tracked via the synchronous + sweep/reconcile paths, backlog P3). Fees column on `payments` stores actual Square deductions. `square_deposits` table for bank reconciliation (matching batch deposits to Mettle account). @@ -53,7 +53,7 @@ Expiry is 24 months from last use (not from purchase). Each use resets the timer Accounts idle 2+ years (no balance) or 5+ years (with balance) are anonymized. Balances before deletion move to `gift_card_expired_balances`. `CleanupIdleAccounts()` runs on availability fetch. -VAT treatment: gift cards are Single-Purpose Vouchers (SPVs) by default — VAT charged at purchase, not redemption. Configurable to Multi-Purpose Voucher (MPV) in business settings. Gift card purchases now insert a pending payment record with VAT applied before calling Square — the DB transaction commits first, so Square failures leave a retryable pending record rather than losing the payment. +VAT treatment: gift cards are Single-Purpose Vouchers (SPVs) by default — VAT charged at purchase, not redemption. Configurable to Multi-Purpose Voucher (MPV) in business settings. Gift card purchases now insert a pending payment record with VAT applied before calling Square — the DB transaction commits first, so Square failures leave a retryable pending record rather than losing the payment. Two background sweeps close Square's ~24h idempotency-key retention window: `sweep-pending-square-refunds` reconciles/retries stuck refunds, and `sweep-stale-pending-payments` fails stale pending payments and till-sales so a late retry cannot issue a second charge. ### Scheduling @@ -87,7 +87,7 @@ Campaign lifecycle: `draft → active → completed` (or any → `cancelled`, `a ### Compliance -**GDPR Article 15**: Full data export via `/gdpr` frontend. Async Go endpoint (`GET /api/user/gdpr-export`) with 12h in-memory cache and background generation (navigation away doesn't cancel). 21-section JSON export: user profile, bookings with overrides, payments, refunds, saved cards, social logins, loyalty redemptions, booking discounts, edit requests, affiliate payouts, forgiven no-shows, patch tests, referrals, referral discounts, notification preferences, gift_card_balance, gift_card_transactions, gift_cards, admin_audit_log, login_audit, refresh_tokens, name_history, export metadata. **Verification codes excluded** (authentication tokens are not personal data under GDPR Art 15). Frontend: skeleton loading, 2s polling, styled report cards/tables, PDF export (print CSS hides navbar + verification banner), raw JSON download. +**GDPR Article 15**: Full data export via `/gdpr` frontend. Async Go endpoint (`GET /api/user/gdpr-export`) with 12h in-memory cache and background generation (navigation away doesn't cancel). 23-section JSON export: user profile, bookings with overrides, payments, refunds, saved cards, social logins, loyalty redemptions, booking discounts, edit requests, affiliate payouts, forgiven no-shows, patch tests, referrals, referral discounts, notification preferences, gift_card_balance, gift_card_transactions, gift_cards, admin_audit_log, login_audit, refresh_tokens, name_history, export metadata. **Verification codes excluded** (authentication tokens are not personal data under GDPR Art 15). Frontend: skeleton loading, 2s polling, styled report cards/tables, PDF export (print CSS hides navbar + verification banner), raw JSON download. **Account deletion**: Registered users → `anonymize_user()` SQL function extended with child table PII scrubbing (social logins deleted, saved cards soft-deleted with PCI data cleared, verification codes expired, time blocker reservations scrubbed including `RESERVATION:edit_request:%` entries, edit request notes nulled, notification preferences deleted). External system scrubbing: S3 profile picture, Square saved cards. Guests → `delete_guest_user()` for full removal. diff --git a/obsidian/Crussell/Technical Manual.md b/obsidian/Crussell/Technical Manual.md index 3ac0045..7d85a1e 100644 --- a/obsidian/Crussell/Technical Manual.md +++ b/obsidian/Crussell/Technical Manual.md @@ -64,7 +64,7 @@ Backend (:8080) | `handlers/auth` | local.go, social.go | Registration (with referral code validation), login, refresh, email verification | | `handlers/bookings` | bookings.go, reserve.go, manage.go, admin_reserve.go, cancel_reservation.go, admin_cancel_reservation.go, closing_time.go | Booking CRUD, reservations with **self-blocking prevention** (`excludeUserID` parameter on `CheckTimeBlockerOverlap` + pre-overlap DELETE with IP hash anon cleanup), admin management, edit requests, discounts, closing hours validation (`checkClosingHours` + `getClosingTimeForDate` resolves staged default hours for bookings), active booking limits, GetBookingsByCreatedRange, created_by_name resolution, **explicit reservation cancellation** (`DELETE /api/bookings/reserve` for users, `DELETE /api/admin/bookings/reserve` for admin walk-in/call-in) | | `handlers/payments` | handlers.go, service.go, validators.go, giftcards.go, till.go, refunds.go, refund_policy.go | Square payments: terminal, online, refunds, tips, saved cards, gift cards (CRUD, topup, transfer, redeem, buy, expired balances, till sales). Refund calculation with notice-period tiers and deposit protection | -| `handlers/webhooks` | square.go | Square webhook handler for payment status updates. **Fail-closed signature check** — rejects requests with 403 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is set but header is missing. Dev mode: skips verification when env var is empty. HMAC-SHA256 signature verified per Square spec (base64 output, `x-square-hmacsha256-signature` header, notificationURL + body). `payment.updated`/`refund.updated` events are currently **log-only** (backlog P3 — status flows through the synchronous + sweep/reconcile paths instead). | +| `handlers/webhooks` | square.go | Square webhook handler for payment status updates. **Fail-closed signature check** — rejects with 503 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is unset, and 403 when the `x-square-hmacsha256-signature` header is missing or invalid. HMAC-SHA256 signature verified per Square spec (base64 output, notificationURL + body). `payment.updated`/`refund.updated` events are currently **log-only** (backlog P3 — status flows through the synchronous + sweep/reconcile paths instead). | | `handlers/admin` | users.go, analytics.go, custom_services.go, discount_campaigns.go, settings.go | Admin user management, custom services CRUD (list/create/get/update/promote/delete), discount campaigns, analytics (stub), business settings (GET/PUT with VAT, gift card config) | | `handlers/today` | today.go | Current/next appointment, today's grid, pending approvals, `DoneForDay` state with daily/weekly summary (`DailySummary` with `total_bookings`, `customers_served`, `summary_scope`), auto-status transitions, closed-day aggregation via `findWeekSummaryRange` + `computeAggregateSummary`. Exceptional hours lookup uses `exceptional_group_applications.week_start` (0=Monday). | | `handlers/user` | profile.go, account.go, guest.go, loyalty.go, customer_relationship.go, gdpr_export.go | User profile, guest creation (with CheckEmailHandler for registered-email detection), loyalty, contact info, GDPR export (async with 12h cache) | @@ -259,7 +259,7 @@ Added in the June 2026 security pass: | Fix | File | Description | |-----|------|-------------| | Removed verification code logging | `handlers/auth/local.go:545` | Deleted `log.Printf("DEBUG: Verification code for %s: %s ...")` — was leaking verification codes to stdout | -| Webhook signature fail-closed | `handlers/webhooks/square.go:59-69` | Changed from "skip verification if header missing" to "reject 403 if key set but header missing" | +| Webhook signature fail-closed | `handlers/webhooks/square.go:34-60` | Webhook verification is fully **fail-closed**: 503 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is unset (a misconfigured deployment must not silently accept forged events), 403 when the signature header is missing or invalid. Previously it skipped verification when the key was empty. | | S3 delete error checking | `handlers/portfolio/images.go:975` | Changed `s3.Client.Delete(...)` (ignored return) → `if err := s3.Client.Delete(...); err != nil { log.Printf(...) }` | CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. No CSP violations expected — the SvelteKit SPA doesn't load external scripts or fonts. @@ -417,7 +417,7 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. | `payment_type` | `deposit`, `full`, `tip`, `balance`, `partial` | | `payment_method` | `online_square`, `in_person_card`, `cash`, `giftcard`, `discount`, `on_the_house` | | `payment_status` | `pending`, `completed`, `failed`, `refunded` | -| `admin_notification_reason` | `pending_booking`, `cancelled_booking`, `rescheduled_booking`, `1_week_no_pay`, `1_month_no_pay`, `affiliate_claim`, `late_cancellation`, `no_deposit`, `deposit_paid`, `edit_request`, `new_booking`, `edit_requested`, `deposit_not_paid_by_deadline`, `default_hours_changed` | +| `admin_notification_reason` | `pending_booking`, `cancelled_booking`, `rescheduled_booking`, `1_week_no_pay`, `1_month_no_pay`, `affiliate_claim`, `late_cancellation`, `no_deposit`, `deposit_paid`, `edit_request`, `new_booking`, `edit_requested`, `deposit_not_paid_by_deadline`, `default_hours_changed`, `gift_card_purchased_for_friend`, `refund_failed` | | `campaign_type` | `time_based`, `milestone` | | `milestone_type` | `per_user_booking_count`, `global_booking_count`, `anniversary` | | `milestone_unit` | `bookings`, `months`, `years` | @@ -450,7 +450,7 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. | `forgiven_no_shows` | Tracks no-shows forgiven by admin (booking_id, forgiven_by FK to users, created_at). Used by `CountUnforgivenNoShows()` to exclude forgiven records | | `payments` | Payment transactions (VAT fields, invoice_number sequence, fees column for Square deductions, saved_card_id, gift_card_id) | | `user_saved_cards` | Saved card details (square_card_id, brand, last4, fingerprint, soft delete with retained_until) | -| `refunds` | Refund records linked to bookings (amount, reason, square_refund_id, created_by FK to users, ON DELETE SET NULL) | +| `refunds` | Refund records linked to a payment (amount, reason, `square_refund_id`, `refund_attempts` int, `origin` manual|cancellation, `idempotency_key` unique, `created_by` FK to users, ON DELETE SET NULL; `booking_id` is nullable — NULL for non-booking payments such as gift-card purchase refunds) | | `financial_aggregates` | Monthly aggregated financial statistics (no PII) — populated when granular records expire | | `square_deposits` | Square deposit batch tracking for bank reconciliation (batch_id, total_amount, deposited_at) | | `affiliate_payouts` | Affiliate commission tracking | @@ -481,7 +481,7 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. | `generate_referral_code()` | 12-char referral code with collision detection | | `anonymize_user(target_id)` | GDPR right-to-be-erased for registered users — child table PII scrubbing | | `delete_guest_user(target_id)` | Full removal of guest account | -| `export_all_user_data(target_user_id)` | GDPR Article 15 SAR — 21-section JSON export (excludes verification_codes; includes admin_audit_log, gift_cards, name_history) | +| `export_all_user_data(target_user_id)` | GDPR Article 15 SAR — 23-section JSON export (excludes verification_codes; includes admin_audit_log, gift_cards, name_history) | | `get_vat_return_data(start, end)` | VAT return summary for MTD. **Updated:** Now includes `till_sales` via `UNION ALL` — till sales (gift cards, merchandise, services) are counted alongside booking payments for VAT reporting. | | `export_sales_transactions(start, end, include_vat)` | Tax-compatible transaction export. **Updated:** Uses dynamic `vat_rate` from the `payments` table (or `business_settings.default_vat_rate`) instead of hardcoded 1.20. | | `get_monthly_business_summary(start, end)` | Monthly revenue breakdown | @@ -1807,7 +1807,7 @@ FROM bookings b LEFT JOIN payments p ON p.booking_id = b.id WHERE b.user_id = $1 | Frequency | Jobs | Cron | |-----------|------|------| | Every min | Progressive rate limiter cleanup | `* * * * *` | -| Every 5 min | Reservation cleanup, expired deposits, rate limiter cleanup, GDPR cache cleanup | `*/5 * * * *` | +| Every 5 min | Reservation cleanup, expired deposits, rate limiter cleanup, GDPR cache cleanup, **refund sweep** (`sweep-pending-square-refunds`), **stale-pending payment/till-sale sweep** (`sweep-stale-pending-payments`, offset +1 min) | `*/5 * * * *` | | Hourly | Loyalty redemptions, idempotency keys, revoked JTIs, stale login entries, discount campaign auto-transition | `0 * * * *` | | Daily 7am | Unpaid booking notifications (1-week and 1-month overdue) | `0 7 * * *` | | Daily 2am | Expired verification codes, expired/revoked refresh tokens | `0 2 * * *` | diff --git a/obsidian/Crussell/plans/p11-square-web-payments-sdk.md b/obsidian/Crussell/plans/p11-square-web-payments-sdk.md index 5e5633d..961787f 100644 --- a/obsidian/Crussell/plans/p11-square-web-payments-sdk.md +++ b/obsidian/Crussell/plans/p11-square-web-payments-sdk.md @@ -1,6 +1,6 @@ # P11 — Square Web Payments SDK Implementation Plan -**Status:** ✅ COMPLETE (implemented August 2026 — all 8 flows re-enabled; new-card entry tokenized via `cnon:` nonces) +**Status:** ✅ COMPLETE — IMPLEMENTED (August 2026). All 8 flows re-enabled; new-card entry tokenized via `cnon:` nonces. Two follow-up items remain intentionally open (see [Remaining Items](#remaining-items--why-deferred) — both require real Square credentials and gate the production flip). **Owner:** Agent implementing P11 (Square Web Payments SDK) **Estimated effort:** 2-3 days (backend groundwork already landed; this is now a frontend-only integration) **Backlog reference:** `Future Work - Gap Backlog.md` item P11 @@ -109,13 +109,27 @@ All 8 flows render `SquareCardInput` and send the resulting `cnon:xxx` as `new_c --- -## Definition of Done (ALL COMPLETE) +## Definition of Done +### Completed (all verified in the final review round) - [x] Square Web Payments SDK loads (sandbox + prod URLs, env-gated) - [x] `SquareCardInput` tokenizes cards → `cnon:xxx` - [x] All 8 flows re-enabled to send nonces, not PANs (tip ×3, booking payment, deposit, Buy a Gift Card, account Add Card, admin till `online_square`) - [x] `CardEntryUnavailable` kept only as the no-credentials fallback - [x] Backend nonce paths verified unchanged (Step 4/5 done) - [x] Frontend checks pass: svelte-check 0 errors, eslint 0 errors, build succeeds -- [ ] Sandbox smoke test (BLOCKED — no real Square credentials available; must run before any production flip): new-card tokenization → payment → saved card → refund → reconcile, exercised against a real Square endpoint - [x] Docs updated (README, Gap Backlog, Feature Catalog, Technical Manual) + +## Remaining Items & Why Deferred + +> Both items below require **real Square credentials** (sandbox or production). They cannot be exercised against the dev mock — the mock does not enforce Square's real wire contract. They are the **sole gate on the production flip** and are tracked as open backlog items. + +| # | Item | Why deferred | +|---|---|---| +| R1 | **Sandbox smoke test** — new-card tokenization → payment → saved card → refund → reconcile, exercised against a real Square endpoint | **BLOCKED — no real Square credentials available.** The full end-to-end path (Web Payments SDK nonce → `POST /v2/cards` → `POST /v2/payments` → refund → `ListRefunds` reconcile) can only be validated against Square's sandbox. Must run before any production flip. | +| R2 | **M-8 open question: is `card.customer_id` enforced as Required at runtime?** | The app deliberately omits `customer_id` (no Square customer provisioning — linkage uses `reference_id`). Square's API reference documents `customer_id` as Required, but integrations report cards can be created without it. If a sandbox `POST /v2/cards` 400s with `MISSING_REQUIRED_PARAMETER`, the app must provision Square customers before go-live. Gated on the same sandbox credentials as R1. | +| R3 | **N-OBS-1: saved_card deterministic idempotency key dedups identical repeat charges** | **Deliberate, accepted trade-off** (not a credential blocker). The admin "Charge Saved Card" key `bookingID-sc-type-amount-cardID` means two *identical* charges on one booking dedup to the first. Not UI-reachable today (PaymentModal always sends the current `totalDue`, which changes after a charge), and the double-click protection is worth more than a hypothetical "charge exact amount twice" path. Flagged for revisit in the Gap Backlog if that path ever appears. | + +--- + +*Plan record — implementation complete. Remaining items require external credentials and are the pre-go-live gate.*