fix: fresh-review round — 2FA deliverability, disable re-verification, GDPR batch scrub, dispute alerting, docs accuracy
Second fresh-eyes review pass (7 agents: goal, security, code-quality, context-mining, webhooks+2FA, client+mock+sweep, refunds/giftcards/handlers). Money-safety core verified sound (identical-body replay byte-lossless, clawback gated on definitive proof, no double-charge window). This round fixes the issues the fresh pass surfaced: 2FA: - Setup now DELIVERS the code via the [2FA] server log in ALL modes (was: nothing in enforced mode -> production 2FA was an unbreakable dead-end and saved-card charges were permanently 403). Enforced mode still withholds the code from the API response; the log line is the fake delivery channel until email/SMS lands (P6). - Disabling 2FA now requires a fresh verification code when enforcement is ON (previously ignored the code -> a password-only attacker could lift the gate). Shares the 5-attempt lockout and timing-safe compare. Dev bypass retained. - REQUIRE_2FA parsing normalized (false/0/off/no, case-insensitive); startup warning extended to the empty-env/mock-client/enforced-2FA confusion. GDPR: - anonymize_user() SQL now scrubs two_factor_* columns + staff notes, so the idle-account batch cleanup (CleanupIdleAccounts) is erasure-clean, not just the user-initiated delete path. Webhooks: - dispute.created for an untracked Square payment now raises a critical_payment_log admin notification (chargeback the app can't reconcile is never silent). Reason strings truncated on rune boundaries (valid UTF-8). Stale at-most-once comment corrected; revertTillSaleGiftCardFunding duplication noted. Sweep/mock parity: - Mock CreatePayment dedup is now source-aware (IDEMPOTENCY_KEY_REUSED on source mismatch) matching ReplayPaymentByKey and real Square. - COMPLETED-but-never-polled terminal till-sale checkouts are now recorded by the sweep (previously only booking checkouts were; till charges were invisible until the 24h blind-fail WARN). - Legacy snapshot-less minimal-body replay, SQUARE_LOCATION_ID drift, and in-memory-mock-restart limitations documented. Docs: - Webhook path corrected everywhere (/webhooks/square, not /api/webhooks/square - a deployer following the old path would 404 and silently lose all webhook reconciliation). - 2FA enforcement semantics + code-delivery mechanism documented accurately (fail-closed default; log-delivery channel; disable re-verification). - README/User Manual note the 2FA requirement on online saved-card payments. Tests: 2,151 (up from 2,142). Backend 26/27 packages green (crussell/db fails only in this environment: local postgres doesn't offer scram-sha-256 for the test role; package is byte-identical to HEAD and untouched here). Frontend builds; svelte-check 0 errors.
This commit is contained in:
@@ -2,6 +2,24 @@
|
||||
|
||||
package square
|
||||
|
||||
// KNOWN LIMITATION — THIS MOCK IS IN-MEMORY ONLY. Every ledger map below
|
||||
// (payments, paymentByKey, paymentSource, cards, cardByToken, checkouts,
|
||||
// completed, refunds, refundByKey, customers) lives for the lifetime of the
|
||||
// process and is reset on ANY dev-server restart. There is intentionally NO
|
||||
// persistence — this is a dev mock, not a store.
|
||||
//
|
||||
// Money-state consequence: a keyed pending row that is replayed AFTER a
|
||||
// restart looks like an UNKNOWN idempotency key to the fresh mock, so the
|
||||
// replay takes the unknown-key path — a spent/expired cnon: nonce is rejected
|
||||
// (ErrReplayKeyNotRetained → the sweep DEFINITIVELY fails the row, and a till
|
||||
// sale's funded gift card is clawed back) where prod would still hold the
|
||||
// ORIGINAL payment under the retained key and return it. A test that
|
||||
// "simulates a restart" with a fresh MockClient is therefore exercising the
|
||||
// prod UNKNOWN-KEY case, NOT the prod retained-key case — do not read such a
|
||||
// test as evidence of how prod treats a retained key after a restart. If a
|
||||
// test needs retained-key behaviour, it must re-seed the payment under the key
|
||||
// into the same mock instance (see TestSweepStalePendingPayments_KeyedLostResponse_CompletedRescued).
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crussell/clock"
|
||||
@@ -148,6 +166,23 @@ func detectCardInfo(sourceID string) (brand, last4 string) {
|
||||
}
|
||||
}
|
||||
|
||||
// keyReuseError is Square's documented IDEMPOTENCY_KEY_REUSED rejection: an
|
||||
// idempotency key reused with a DIFFERENT request body (real Square compares
|
||||
// the WHOLE body; the mock checks the source_id, the only body field that
|
||||
// legitimately varies between same-intent retries). The structured code lets
|
||||
// ErrorCode(err) read it, and the sweep treats it as ambiguous — a data bug,
|
||||
// NOT proof the charge never happened. Shared by CreatePayment's dedup and
|
||||
// ReplayPaymentByKey so both paths return the byte-identical error the real
|
||||
// API would.
|
||||
func keyReuseError(key string) error {
|
||||
return &squareAPIError{
|
||||
Code: "IDEMPOTENCY_KEY_REUSED",
|
||||
Detail: "idempotency key was reused with a different request body",
|
||||
StatusCode: http.StatusBadRequest,
|
||||
err: fmt.Errorf("square: idempotency key %s reused with a different source_id", key),
|
||||
}
|
||||
}
|
||||
|
||||
func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (*PaymentResult, error) {
|
||||
if m.ShouldFail {
|
||||
return nil, fmt.Errorf("mock: payment declined (simulated failure)")
|
||||
@@ -188,9 +223,19 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (*
|
||||
// Real Square dedups on idempotency key: a retry with the same key returns
|
||||
// the original payment rather than creating a second charge. The mock
|
||||
// mirrors this so dev/testing behaves like production (also why the tip
|
||||
// retry regression test can rely on the mock).
|
||||
// retry regression test can rely on the mock). Like real Square, the dedup
|
||||
// is BODY-AWARE: a retained key reused with a DIFFERENT source_id is
|
||||
// rejected with IDEMPOTENCY_KEY_REUSED (the same error ReplayPaymentByKey
|
||||
// returns for a source mismatch), never silently satisfied — so the
|
||||
// gift-card same-key retry (which refreshes square_source_id with a fresh
|
||||
// cnon on pending-reuse) surfaces the real prod rejection in dev instead of
|
||||
// succeeding where prod would strand the row pending for the sweep.
|
||||
if req.IdempotencyKey != "" {
|
||||
if existing, ok := m.paymentByKey[req.IdempotencyKey]; ok {
|
||||
if storedSource, hasSource := m.paymentSource[req.IdempotencyKey]; hasSource && storedSource != "" && storedSource != req.SourceID {
|
||||
log.Printf("[SQUARE-MOCK] CreatePayment IDEMPOTENCY_KEY_REUSED: key=%s reused with a different source (%s vs %s)", req.IdempotencyKey, tokenPrefix(req.SourceID), tokenPrefix(storedSource))
|
||||
return nil, keyReuseError(req.IdempotencyKey)
|
||||
}
|
||||
log.Printf("[SQUARE-MOCK] CreatePayment dedup hit: key=%s → id=%s", req.IdempotencyKey, existing.ID)
|
||||
return existing, nil
|
||||
}
|
||||
@@ -438,12 +483,7 @@ func (m *MockClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte
|
||||
// Same key, different body — Square's documented IDEMPOTENCY_KEY_REUSED
|
||||
// rejection. A data bug (the stored source differs from the original
|
||||
// charge), NOT proof the charge never happened.
|
||||
return nil, &squareAPIError{
|
||||
Code: "IDEMPOTENCY_KEY_REUSED",
|
||||
Detail: "idempotency key was reused with a different request body",
|
||||
StatusCode: http.StatusBadRequest,
|
||||
err: fmt.Errorf("square: idempotency key %s reused with a different source_id", req.IdempotencyKey),
|
||||
}
|
||||
return nil, keyReuseError(req.IdempotencyKey)
|
||||
}
|
||||
log.Printf("[SQUARE-MOCK] ReplayPaymentByKey dedup hit: key=%s → id=%s", req.IdempotencyKey, existing.ID)
|
||||
return existing, nil
|
||||
|
||||
Reference in New Issue
Block a user