fix: comprehensive payment system hardening (4 review passes)
CRITICAL fixes: - C1: JWT exp claim now validated via jwtauth.VerifyToken (was Decode) - C2: OverrideAmount validated post-substitution (prevents negative money minting) - C3: Terminal gift-card payments store gift_card_id; refund credits user balance - C4: Refund dedup returns stored amount, not req.Amount (prevents admin mislead) - C5: Booking recheck uses FOR UPDATE (prevents TOCTOU with cancellation) - C6: processChargeGroup idempotency key stable (charge-only, prevents double-refund) MAJOR fixes: - M2: Gift-card refund UPDATE checks RowsAffected; 0 rows -> failed - M3: ProcessCancellationRefund returns commit error (was swallowed) - M5: Dispute webhook handling (created + state.updated + disputes table) MEDIUM fixes: - ME1: CORS restricted to FRONTEND_ORIGIN env var (was reflect-any) - ME2: anonymize_user() scrubs users.notes, bookings.notes, name_history, refresh_tokens - ME3: Webhook handlers now mutate state (payment.updated, refund.updated) Frontend fixes: - Same-key retry on 503 (ambiguous failure) wired to all 8 payment flows - CHARGE_AND_STORE intent for save-card flows (SCA compliance) - Nonce staleness check verified across all flows Additional fixes from adversarial re-review: - F1: Till-sale completed dedup echoes stored amount (C4-class) - F2: Cash/giftcard terminal path uses FOR UPDATE (C5-class) - F3: Square-success UPDATE checks RowsAffected (till sales) - F4: Dispute reason truncated to 192 chars (prevents INSERT failure) - F5: Booking-user lookup failure marks refund failed (prevents silent money loss) - F6: Saved-card/tip rechecks wrapped in transaction (C5 residual) Tests: - 15 adversarial attack tests (negative override, zero override, terminal gift card, refund dedup, TOCTOU, deleted gift card, advisory lock, overcharge, zero/negative/huge amount, raw PAN, missing auth, gift card balance, concurrent refunds) - 14 webhook state tests (dispute created/state, payment/refund updated) - 3 CORS tests, 3 GDPR tests, 1 HTTP timeout test - Full suite passes with -race (25 packages, 0 failures) 25 files changed, +1532/-275 lines
This commit is contained in:
@@ -0,0 +1,75 @@
|
||||
//go:build test
|
||||
|
||||
package square
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TestHTTPClientTimeout_ClassifiedAmbiguous503 locks the N6 contract: the
|
||||
// production HTTP client's 30s timeout must fire against a stalled upstream and
|
||||
// produce a context-deadline error. chargeFailureStatus
|
||||
// (handlers/payments/errors.go) checks `errors.Is(err, context.DeadlineExceeded)`
|
||||
// FIRST and maps it to 503 (Service Unavailable / ambiguous) — the charge may
|
||||
// or may not have reached Square, so it must never be labelled the definitive
|
||||
// 402 decline a retry would ignore.
|
||||
func TestHTTPClientTimeout_ClassifiedAmbiguous503(t *testing.T) {
|
||||
// Upstream Square stalls LONGER than the production client timeout, so the
|
||||
// client must give up on its own — the handler never hangs. `stop` aborts
|
||||
// the handler at teardown so srv.Close() does not wait out the full delay.
|
||||
delay := defaultHTTPTimeout + 5*time.Second
|
||||
stop := make(chan struct{})
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
select {
|
||||
case <-time.After(delay):
|
||||
w.WriteHeader(http.StatusOK)
|
||||
case <-r.Context().Done():
|
||||
return
|
||||
case <-stop:
|
||||
return
|
||||
}
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
hc := &httpClient{baseURL: srv.URL, token: "test-token", http: &http.Client{Timeout: defaultHTTPTimeout}}
|
||||
|
||||
start := time.Now()
|
||||
_, err := createPaymentHTTPWithClient(context.Background(), CreatePaymentReq{
|
||||
Amount: 5000,
|
||||
Currency: "GBP",
|
||||
SourceID: "cnon:test-card",
|
||||
IdempotencyKey: "timeout-slow-upstream",
|
||||
}, hc)
|
||||
elapsed := time.Since(start)
|
||||
close(stop)
|
||||
|
||||
if err == nil {
|
||||
t.Fatal("expected a timeout error, got nil")
|
||||
}
|
||||
|
||||
// The client must have given up near its 30s timeout — not instantly and
|
||||
// not after the 35s server delay.
|
||||
if elapsed < 25*time.Second || elapsed > 33*time.Second {
|
||||
t.Errorf("expected timeout after ~%v, got %v (err: %v)", defaultHTTPTimeout, elapsed, err)
|
||||
}
|
||||
|
||||
// The context-deadline predicate chargeFailureStatus maps to 503. Go's
|
||||
// http.Client.Timeout wraps *timeoutError whose Is() matches
|
||||
// context.DeadlineExceeded.
|
||||
if !errors.Is(err, context.DeadlineExceeded) {
|
||||
t.Errorf("expected errors.Is(err, context.DeadlineExceeded), got %v", err)
|
||||
}
|
||||
|
||||
// Standard Go contract: a client timeout surfaces as a net.Error with
|
||||
// Timeout() == true.
|
||||
var netErr net.Error
|
||||
if !errors.As(err, &netErr) || !netErr.Timeout() {
|
||||
t.Errorf("expected a timeout net.Error, got %T: %v", err, err)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user