popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix payment review round 3: saved-card idempotency, stale-pending sweep, webhook fail-closed

Browse Source 
R1/R4: saved_card branch in CreateTerminalPayment now mirrors CreateTipPayment
- advisory lock (crussell:payment:<bookingID>) serializes concurrent double-clicks
- deterministic key bookingID-sc-type-amount-cardID (<=45 chars) so a lost-response
  retry derives the same key and dedups instead of double-charging
- idempotency switch inside the lock: completed -> dedup, pending -> reuse with
  pence amount-guard, failed -> clean 409
- success response includes card_brand/card_last4 (frontend already reads them)

R2: add 'failed' case to all four retry switches (tip, booking, gift card, till)
- a swept/definitively-rejected record returns 409 instead of 500-ing on the
  idempotency_key UNIQUE constraint

R3: extend SweepStalePendingPayments to till_sales card rows
- sweeps pending till_sales (online_square/in_person_card) past Square's ~24h
  key retention, closing the double-charge window for till sales
- swept rows logged with the same CRITICAL manual-reconciliation marker as the
  refund sweep

Webhook fail-closed: reject 503 when SQUARE_WEBHOOK_SIGNATURE_KEY unset, 403 on
bad signature (was: skip verification in dev)

Refund status resolution: refunds now resolve by Square status
(COMPLETED/PENDING/FAILED/REJECTED) instead of assuming completed; real error
codes (REFUND_AMOUNT_INVALID, PAYMENT_NOT_REFUNDABLE, REFUND_ALREADY_PENDING)
added to the definitive/processed classification

HTTP client: CreateCard key truncated to <=45 chars, device_options always sent
(env SQUARE_TERMINAL_DEVICE_ID fallback), processing_fee reads amount_money,
ListCards cursor loop, refund keys hashed to <=45 chars

Other fixes: payment/till/gift-card advisory-lock + FOR UPDATE asymmetries,
GetPaymentByID NULL scans, loyalty redemption lock, card upsert on conflict,
mock ccof: prefix parity, IsValidSquareCheckoutID for real Square IDs,
isAdminRequest defense-in-depth on all 6 admin payment handlers, webhook
signature docs, M8/L5 debug markers removed

Docs: README/FC/TM/Overview updated (22 jobs, 20 CRITICAL sites, 23-section
GDPR export, sweep jobs, webhook fail-closed); P11 plan marks remaining items
(sandbox smoke test, M-8 customer_id, saved-card key dedup trade-off) as
deferred with rationale; gap backlog pruned of completed items
This commit is contained in:
popertots
2026-08-22 00:34:49 +01:00
parent dcc70df75a
commit 7439fa86c1
30 changed files with 1239 additions and 184 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/internal/jobs/cleanup.go
+10
View File
@@ -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{
backend/internal/jobs/scheduler_test.go
+3 -2
View File
@@ -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,
backend/internal/square/square_dev.go
+15 -3
View File
@@ -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,
backend/internal/square/square_http_client.go
+55 -26
View File
@@ -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,
backend/internal/square/square_http_client_test.go
+12 -4
View File
@@ -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"])
}
backend/internal/square/types.go
-1
View File
@@ -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
backend/internal/validators/validators.go
+13
View File
@@ -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)
backend/internal/validators/validators_test.go
+15
View File
@@ -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
}