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

Fix P0/P1 review findings: truncation, raw-PAN API edge, refund lock, till pending-retry, idempotency keys

Browse Source 
P0 — float truncation: applied math.Round to all remaining int64(x*100)
sites (till penceAmount, refund over-refund guard, GetAlreadyRefundedAmount,
payment summary conversions). A £1.14 till sale previously charged 113p.

P0 — raw PAN stopped at the API edge:
- Deleted CardNumber/CardExpMonth/CardExpYear/CardCVC from TillSaleRequest
  and CardNumber/Expiry/CVC from CreatePaymentMethodRequest. Both now accept
  card_token (Square nonce) and return 400 when absent. PAN+CVV no longer
  transit the application server (PCI-DSS SAQ-A scope).
- Deleted CreateCardOnFileRaw from the SquareClient interface and all
  implementations (MockClient, ProdClient, devProdClient).
- Added idempotency_key column to refunds table (UNIQUE).

P0 — RefundPayment hardened: advisory lock on payment ID (prevents two
concurrent refunds passing the over-refund guard), pending-refund-record-
then-Square pattern (scheduler reprocesses on failure), same-key dedup.

P1 — till sale pending-retry now re-attempts the Square charge instead of
returning the stale 'pending' status (gift card was already funded in the
committed tx — silent money loss otherwise). Sale row reused, not duplicated.

P1 — idempotency key caching in frontend: BuyGiftCard and
UserPaymentModal/BookingFlow now cache the key per amount+card, regenerated
on change and cleared on success — matches the tip-flow pattern so a
lost-response retry dedups instead of double-charging.

P1 — CreateTerminalPayment cash/giftcard INSERTs now persist idempotency_key.
Key is unique per payment (booking+type+amount would wrongly dedup two
legitimate identical payments, e.g. two £50 cash receipts).

P1 — gift-card codes no longer logged (spendable credential; value+recipient
only).

Tests: till pending-retry re-attempt, refund same-key dedup, mock CreatePayment
idempotency dedup, CreatePaymentMethod nonce happy path + raw-PAN rejection,
till online_square card_token required/valid.
This commit is contained in:
popertots
2026-08-22 00:34:49 +01:00
parent d54f526b56
commit 54f6bf3c1a
14 changed files with 632 additions and 345 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/internal/square/square.go
+1 -8
View File
@@ -2,10 +2,7 @@
package square
import (
"context"
"fmt"
)
import "context"
var Client SquareClient
@@ -39,10 +36,6 @@ func (p *ProdClient) CreateCardOnFile(ctx context.Context, userID, cardToken str
return createCardOnFileHTTP(ctx, userID, cardToken)
}
func (p *ProdClient) CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) {
return nil, fmt.Errorf("square: raw card number input is not supported in production — use CreateCardOnFile with a card nonce")
}
func (p *ProdClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) {
return getCardsOnFileHTTP(ctx, userID)
}
backend/internal/square/square_dev.go
-10
View File
@@ -52,9 +52,6 @@ func (d *devProdClient) RefundPayment(ctx context.Context, req RefundPaymentReq)
func (d *devProdClient) CreateCardOnFile(ctx context.Context, userID, cardToken string) (*CardOnFile, error) {
return createCardOnFileHTTP(ctx, userID, cardToken)
}
func (d *devProdClient) CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) {
return nil, fmt.Errorf("square: raw card number input is not supported in production — use CreateCardOnFile with a card nonce")
}
func (d *devProdClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) {
return getCardsOnFileHTTP(ctx, userID)
}
@@ -373,13 +370,6 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken str
return card, nil
}
func (m *MockClient) CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) {
// PCI-DSS parity with production: raw card numbers are never accepted.
// The mock must behave identically to the ProdClient so dev testing does
// not mask a production failure.
return nil, fmt.Errorf("square: raw card number input is not supported in production — use CreateCardOnFile with a card nonce")
}
func (m *MockClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) {
log.Printf("[SQUARE-MOCK] GetCardsOnFile: user=%s", userID)
backend/internal/square/square_dev_test.go
+36 -50
View File
@@ -237,9 +237,9 @@ func TestDevClient_GetCheckout_NotFound(t *testing.T) {
require.Error(t, err)
}
func TestDevClient_CreateCardOnFileRaw_Rejected_ProdParity(t *testing.T) {
// PCI-DSS parity: the mock must reject raw PANs exactly like the
// ProdClient, so dev testing cannot mask a production failure.
func TestDevClient_CreateCardOnFile_RejectsRawPAN(t *testing.T) {
// PCI-DSS parity: CreateCardOnFile accepts only token-like source_ids
// (cnon:xxx / ccof:xxx). Raw PANs are rejected exactly like real Square.
client := NewDevClient().(*MockClient)
ctx := context.Background()
@@ -251,16 +251,15 @@ func TestDevClient_CreateCardOnFileRaw_Rejected_ProdParity(t *testing.T) {
{"mastercard", "5555555555554444"},
{"amex", "378282246310005"},
{"discover", "6011111111111117"},
{"unknown brand", "9999999999999999"},
{"too short", "123"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
card, err := client.CreateCardOnFileRaw(ctx, "user-raw-"+tt.name, tt.cardNumber, 12, 2030, "123")
card, err := client.CreateCardOnFile(ctx, "user-raw-"+tt.name, tt.cardNumber)
require.Error(t, err, "raw PAN must be rejected for production parity")
assert.Nil(t, card)
assert.Contains(t, err.Error(), "raw card number input is not supported")
assert.Contains(t, err.Error(), "invalid source_id")
})
}
}
@@ -372,6 +371,37 @@ func TestDevClient_CreatePayment_WithTipMoney(t *testing.T) {
assert.Equal(t, int64(1000), result.TipAmount)
}
func TestDevClient_CreatePayment_DedupsOnIdempotencyKey(t *testing.T) {
// Real Square dedups on idempotency key: a same-key retry returns the
// original payment. The mock must mirror this or dev/testing diverges
// from production (and the pending-retry logic can't be exercised).
client := NewDevClient().(*MockClient)
ctx := context.Background()
req := CreatePaymentReq{
Amount: 5000,
Currency: "GBP",
SourceID: "cnon:test-card",
IdempotencyKey: "dedup-key-1",
ReferenceID: "booking-dedup",
}
first, err := client.CreatePayment(ctx, req)
require.NoError(t, err)
require.NotEmpty(t, first.ID)
second, err := client.CreatePayment(ctx, req)
require.NoError(t, err)
assert.Equal(t, first.ID, second.ID, "same-key retry must return the original payment, not a new one")
// Total stored payments for this key must be one (deduped).
client.mu.RLock()
byKey := client.paymentByKey["dedup-key-1"]
client.mu.RUnlock()
assert.NotNil(t, byKey)
assert.Equal(t, first.ID, byKey.ID)
}
func TestDevClient_CreatePayment_AutocompleteFalse(t *testing.T) {
client := NewDevClient().(*MockClient)
ctx := context.Background()
@@ -426,41 +456,6 @@ func TestDevClient_CreateCardOnFile_WithNewFields(t *testing.T) {
assert.NotEmpty(t, card.CreatedAt)
}
func TestDevClient_CreateCardOnFileRaw_WithBrandDetection(t *testing.T) {
client := NewDevClient().(*MockClient)
ctx := context.Background()
userID := "user-raw-brand-detect"
card, err := client.CreateCardOnFileRaw(ctx, userID, "4111111111111111", 12, 2030, "123")
require.Error(t, err, "raw PAN must be rejected for production parity")
assert.Nil(t, card)
}
func TestDevClient_CreateCardOnFile_RejectsRawPAN(t *testing.T) {
client := NewDevClient().(*MockClient)
ctx := context.Background()
tests := []struct {
name string
cardNum string
}{
{"visa formatted", "4111 1111 1111 1111"},
{"visa raw", "4111111111111111"},
{"mastercard", "5500 0000 0000 0004"},
{"amex", "3400 0000 0000 009"},
{"discover", "6011 0000 0000 0004"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
userID := fmt.Sprintf("user-raw-card-%s", tt.name)
card, err := client.CreateCardOnFile(ctx, userID, tt.cardNum)
require.Error(t, err, "raw PAN must be rejected for production parity")
assert.Nil(t, card)
})
}
}
func TestDevClient_DeleteCardOnFile_SoftDelete(t *testing.T) {
client := NewDevClient().(*MockClient)
ctx := context.Background()
@@ -478,15 +473,6 @@ func TestDevClient_DeleteCardOnFile_SoftDelete(t *testing.T) {
assert.False(t, cards[0].Enabled)
}
func TestDevClient_CreateCardOnFileRaw_TooShort(t *testing.T) {
client := NewDevClient().(*MockClient)
ctx := context.Background()
_, err := client.CreateCardOnFileRaw(ctx, "user-too-short", "123", 12, 2030, "999")
require.Error(t, err)
assert.Contains(t, err.Error(), "raw card number input is not supported")
}
func TestDevClient_GetCardsOnFile_Empty(t *testing.T) {
client := NewDevClient().(*MockClient)
ctx := context.Background()
backend/internal/square/types.go
-1
View File
@@ -137,7 +137,6 @@ type SquareClient interface {
GetCheckout(ctx context.Context, checkoutID string) (*PaymentResult, error)
RefundPayment(ctx context.Context, req RefundPaymentReq) (*RefundResult, error)
CreateCardOnFile(ctx context.Context, userID, cardToken string) (*CardOnFile, error)
CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error)
GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error)
DeleteCardOnFile(ctx context.Context, cardID string) error
}