diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index c8e1993..b95cc2b 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -451,24 +451,138 @@ func verificationTokenPrefixForSource(sourceID string) string { // frontend's MockCardForm mints for saved-card verification). A raw nonce like // "cnon:test-card" — whatever customer_id rides along — is NOT a tokenize-result, // and real Square rejects it as a card-on-file charge source. -// -// During the token-shape transition the frontend mock may still emit a -// verify_mock__[_ok|_deny] verification token in the -// tokenize-result source slot; that shape is accepted here too so dev remains -// walkable either way (parseVerifyToken resolves its binding). func isSCATokenizeResultSource(sourceID string) bool { - return strings.HasPrefix(sourceID, "cnon:sca-") || strings.HasPrefix(sourceID, "verify_mock_") + return strings.HasPrefix(sourceID, "cnon:sca-") +} + +// parseSCATokenizeResult parses the deterministic mock tokenize-result encoding +// the dev frontend mints for saved-card SCA: cnon:sca-_[_ok|_deny] +// (outcome suffix defaults to ok). The prefix is the first four characters of +// the saved card's ccof: id, the amount is the pence charge the token was bound +// to, and the outcome encodes the buyer's challenge decision +// (MockCardForm.tokenizeSavedCard / verifySavedCard, square.ts's +// tokenizeSavedCardWithVerification mock fallback). Reuses parsedVerifyToken's +// shape — same binding + outcome semantics as the legacy verify_mock_ encoding. +// Returns ok=false for anything that is not parseable (an arbitrary cnon:sca-... +// string — NOT a genuine tokenize-result). +func parseSCATokenizeResult(token string) (parsedVerifyToken, bool) { + const marker = "cnon:sca-" + if !strings.HasPrefix(token, marker) { + return parsedVerifyToken{}, false + } + rest := strings.TrimPrefix(token, marker) + if rest == "" { + return parsedVerifyToken{}, false + } + parts := strings.Split(rest, "_") + denied := false + if n := len(parts); n > 1 { + switch parts[n-1] { + case "ok", "deny": + denied = parts[n-1] == "deny" + parts = parts[:n-1] + } + } + if len(parts) == 0 { + return parsedVerifyToken{}, false + } + amount, err := strconv.ParseInt(parts[len(parts)-1], 10, 64) + if err != nil { + return parsedVerifyToken{}, false + } + prefix := strings.Join(parts[:len(parts)-1], "_") + if prefix == "" { + return parsedVerifyToken{}, false + } + return parsedVerifyToken{prefix: prefix, amount: amount, denied: denied}, true +} + +// tokenizeResultVerificationRequiredError is the refusal for a cnon:sca- +// tokenize-result that does not prove buyer verification for THIS charge — the +// same structured 402 CARD_DECLINED_VERIFICATION_REQUIRED rejection a raw nonce +// in the tokenize-result slot gets. The buyer must re-verify (mint a token bound +// to this card + amount), never retry the same request. +func tokenizeResultVerificationRequiredError(sourceID, reason string) error { + return &squareAPIError{ + Code: "CARD_DECLINED_VERIFICATION_REQUIRED", + Category: "PAYMENT_METHOD_ERROR", + Detail: "tokenize-result token does not prove buyer verification for this charge; complete buyer verification (tokenizeWithVerification) for this card and amount", + StatusCode: http.StatusPaymentRequired, + err: fmt.Errorf("square: tokenize-result source %s is not valid for this saved-card charge (%s)", tokenPrefix(sourceID), reason), + } +} + +// scaTokenizePrefixKnown reports whether the card prefix embedded in a +// cnon:sca- tokenize-result is one the mock can bind the token to: the +// deterministic prefixes the mock assigns to its test cards, the "test" fallback +// the frontend uses for a degenerate ccof id, or the prefix of a saved card in +// the mock's ledger (the ccof: token prefix the frontend derives the minted +// prefix from — square.ts / MockCardForm). A prefix outside this set cannot come +// from a tokenize-result minted for a card this mock knows, so it is a forged or +// wrong-card binding. +func (m *MockClient) scaTokenizePrefixKnown(prefix string) bool { + if prefix == "test" { + return true + } + switch prefix { + case "4242", "4111", "5555", "3782": + return true + } + for _, card := range m.cardByToken { + if verificationTokenPrefixForSource(card.CardID) == prefix { + return true + } + } + return false +} + +// validateSCATokenizeResult validates the deterministic binding the dev frontend +// encodes into a cnon:sca- tokenize-result source against the charge: the +// embedded amount must match the charge amount (Square binds buyer verification +// to the exact amount), the encoded outcome must not be a buyer denial, and the +// embedded card prefix must be one the mock can bind to. An arbitrary +// cnon:sca-... string that does not carry the deterministic encoding is NOT a +// genuine tokenize-result (the frontend always mints the bound shape), so it is +// refused exactly like a raw nonce in the tokenize-result slot. Caller holds +// m.mu; the source is already known to carry the cnon:sca- marker. +func (m *MockClient) validateSCATokenizeResult(sourceID string, amount int64) error { + parsed, ok := parseSCATokenizeResult(sourceID) + if !ok { + return tokenizeResultVerificationRequiredError(sourceID, "does not carry the deterministic tokenize-result binding") + } + if parsed.denied { + return tokenizeResultVerificationRequiredError(sourceID, "the buyer denied the SCA challenge") + } + if parsed.amount != amount { + return tokenizeResultVerificationRequiredError(sourceID, fmt.Sprintf("bound to a different amount (%d vs %d)", parsed.amount, amount)) + } + if !m.scaTokenizePrefixKnown(parsed.prefix) { + return tokenizeResultVerificationRequiredError(sourceID, fmt.Sprintf("bound to an unknown card prefix %q", parsed.prefix)) + } + return nil } // isTokenLikeMock is the dev mock's PCI-DSS token predicate: it accepts the -// production token shapes (cnon: nonces / ccof: card ids — isTokenLike) PLUS -// the verify_mock__ verification-token shape the dev frontend -// mints, so dev remains walkable while the frontend's card form transitions to -// minting cnon:sca- tokenize-results. The PRODUCTION client stays strict -// (square_http_client.go isTokenLike — cnon:/ccof: only); this mock-only -// widening never reaches real Square. +// SAME production token shapes as the real client (cnon: nonces / ccof: card +// ids — isTokenLike). The legacy verify_mock__ source widening +// is REMOVED: the transition to genuine cnon:sca- tokenize-results is complete +// and real Square never accepts verify_mock_ as a source_id, so accepting it in +// the mock would mask a bug that prod would reject. verify_mock_ tokens remain +// valid in the VerificationToken field (the deprecated verifyBuyer() contract) — +// only the source-slot widening is removed. func isTokenLikeMock(s string) bool { - return isTokenLike(s) || strings.HasPrefix(s, "verify_mock_") + return isTokenLike(s) +} + +// legacyVerifyMockSourceError is the mock's explicit rejection of the +// deprecated verify_mock__[_ok|_deny] charge-source shape the +// dev frontend used during the token-shape transition. Real Square never +// accepts verify_mock_ as a source_id (it is not a cnon:/ccof: token), so the +// mock refuses it with a clear "legacy shape not supported" error instead of +// silently accepting it — the old source widening (isTokenLikeMock) masked the +// divergence. +func legacyVerifyMockSourceError(s string) error { + return fmt.Errorf("square: %s is the legacy verify_mock_ transition shape and is NOT a valid charge source — real Square rejects it; mint a genuine tokenize-result (cnon:sca-__ok) or use a cnon:/ccof: token", tokenPrefix(s)) } // resolveVerificationToken validates a supplied 3DS/SCA verification token for @@ -580,9 +694,12 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* body := mockPaymentWireBody(req) // Match the real Square API: source_id must be a token (cnon:xxx nonce or // ccof:xxx card ID). Raw PANs are rejected exactly as Square would, so the - // mock behaves identically to production (PCI-DSS parity). The mock's token - // predicate additionally accepts the verify_mock_* transition shape the dev - // frontend mints (isTokenLikeMock). + // mock behaves identically to production (PCI-DSS parity). The legacy + // verify_mock_* transition shape is rejected explicitly — real Square never + // accepts it as a source_id (isTokenLikeMock). + if strings.HasPrefix(body.SourceID, "verify_mock_") { + return nil, legacyVerifyMockSourceError(body.SourceID) + } if !isTokenLikeMock(body.SourceID) { return nil, fmt.Errorf("invalid source_id: %s — use a card nonce (cnon:xxx) or card ID (ccof:xxx)", tokenPrefix(body.SourceID)) } @@ -695,7 +812,11 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* Code: "CARD_DECLINED_VERIFICATION_REQUIRED", Category: "PAYMENT_METHOD_ERROR", Detail: "card requires buyer verification (3DS/SCA); supply a verification token", - StatusCode: http.StatusBadRequest, + // Square's documented wire status for verification-required is + // 402 Payment Required, not 400 — the mock must match the WIRE + // (the handler's code-keyed classification is unchanged, and + // chargeFailureStatus maps any definitive 4xx to 402 anyway). + StatusCode: http.StatusPaymentRequired, err: errors.New("square: card requires buyer verification — verification_token required for a new-card (cnon:) charge"), } default: @@ -747,11 +868,22 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* Code: "CARD_DECLINED_VERIFICATION_REQUIRED", Category: "PAYMENT_METHOD_ERROR", Detail: "unverified card nonce cannot be used as a saved-card (card-on-file) charge source; complete buyer verification (tokenizeWithVerification) first", - StatusCode: http.StatusBadRequest, + // 402 on the wire, matching Square's documented status for + // verification-required. + StatusCode: http.StatusPaymentRequired, err: fmt.Errorf("square: unverified cnon nonce %s cannot be used as a saved-card (card-on-file) charge source — a genuine tokenizeWithVerification result is required", tokenPrefix(body.SourceID)), } } // (a) genuine tokenize-result — the token IS the buyer verification. + // The mock validates the deterministic binding the dev frontend + // encodes into the token (cnon:sca-__ok|_deny): the + // embedded amount must match the charge, the prefix must bind to a + // card the mock knows, and a _deny outcome is refused. An arbitrary + // cnon:sca-... string that does not prove verification for THIS + // charge is rejected exactly like real Square's SCA enforcement. + if err := m.validateSCATokenizeResult(body.SourceID, body.AmountMoney.Amount); err != nil { + return nil, err + } log.Printf("[SQUARE-MOCK] CreatePayment saved-card SCA satisfied by tokenize-result: source %s (no verification_token needed)", tokenPrefix(body.SourceID)) case body.VerificationToken == "": if m.grandfatheredCards[body.SourceID] { @@ -770,7 +902,9 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* Code: "CARD_DECLINED_VERIFICATION_REQUIRED", Category: "PAYMENT_METHOD_ERROR", Detail: "saved card requires buyer verification (3DS/SCA); supply a verification token", - StatusCode: http.StatusBadRequest, + // 402 on the wire, matching Square's documented status for + // verification-required. + StatusCode: http.StatusPaymentRequired, err: errors.New("square: saved card requires buyer verification — verification_token required for a card-on-file (ccof:) charge"), } } @@ -1084,6 +1218,17 @@ func (m *MockClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte if err := json.Unmarshal(snapshotJSON, &req); err != nil { return nil, fmt.Errorf("square: replay-by-key cannot parse stored request snapshot: %w", err) } + // Mirror prod's replay-by-key sanity validation + // (replayPaymentByKeyHTTPWithClient, square_http_client.go): a snapshot + // missing source_id or idempotency_key is CORRUPT — never replay it as if + // it could succeed. Prod returns a plain (ambiguous) error here, so the + // sweep leaves the row PENDING for manual reconciliation; the mock must NOT + // answer ErrReplayKeyNotRetained (which the sweep treats as "the charge + // provably never happened" and definitively fails the row) — that is the + // OPPOSITE money decision on a snapshot we cannot trust. + if req.SourceID == "" || req.IdempotencyKey == "" { + return nil, fmt.Errorf("square: replay-by-key snapshot missing source_id/idempotency_key") + } log.Printf("[SQUARE-MOCK] ReplayPaymentByKey: key=%s, source=%s", req.IdempotencyKey, tokenPrefix(req.SourceID)) m.mu.RLock() @@ -1201,9 +1346,16 @@ func (m *MockClient) RefundPayment(ctx context.Context, req RefundPaymentReq) (* // Known payments get the real Square over-refund rejection: refunding more // than the remaining balance answers 400 REFUND_AMOUNT_INVALID. Square // returns that SAME code for an already-refunded payment, so — exactly like - // the real client — the mock reconciles: an existing refund (money already - // moved) → ErrRefundAlreadyProcessed; no refund recorded → the amount is - // genuinely invalid → ErrRefundDeclined. + // the real client (refundPaymentHTTPWithClient's REFUND_AMOUNT_INVALID + // reconciliation via paymentRefundedExactlyWithClient) — the mock + // reconciles: an existing COMPLETED refund for the EXACT requested amount + // (that money provably already moved) → ErrRefundAlreadyProcessed; anything + // else → the amount is genuinely invalid → ErrRefundDeclined. A different + // amount is NEVER AlreadyProcessed: only an exact-amount refund covers the + // requested money, so an over-refund on top of a PARTIAL prior refund + // surfaces as ErrRefundDeclined (the row is marked failed, alerting the + // over-refund guard bug) instead of being resolved 'completed' and hiding + // it. if payment, ok := m.payments[req.PaymentID]; ok { remaining := payment.Amount for _, r := range m.refunds { @@ -1219,7 +1371,7 @@ func (m *MockClient) RefundPayment(ctx context.Context, req RefundPaymentReq) (* StatusCode: http.StatusBadRequest, err: fmt.Errorf("square: refund amount %d exceeds remaining balance %d for payment %s", req.Amount, remaining, req.PaymentID), } - if remaining < payment.Amount { + if m.refundExistsExact(req.PaymentID, req.Amount) { return nil, fmt.Errorf("%w: %v", ErrRefundAlreadyProcessed, apiErr) } return nil, fmt.Errorf("%w: %v", ErrRefundDeclined, apiErr) @@ -1269,6 +1421,22 @@ func (m *MockClient) RefundKeyCount() int { return len(m.refundByKey) } +// refundExistsExact reports whether the mock holds a COMPLETED refund for the +// payment in the EXACT amount requested — the same exact-match reconciliation +// the real client runs when Square returns REFUND_AMOUNT_INVALID +// (paymentRefundedExactlyWithClient: status COMPLETED && amount == requested). +// Only an exact-amount COMPLETED refund proves THIS requested money already +// moved; a partial refund does not cover it. Caller holds m.mu (RefundPayment +// holds the write lock). +func (m *MockClient) refundExistsExact(paymentID string, amount int64) bool { + for _, r := range m.refunds { + if r.PaymentID == paymentID && r.Status == "COMPLETED" && r.Amount == amount { + return true + } + } + return false +} + // PaymentWasRefunded mirrors the real client's reconciliation source: true when // any refund with status COMPLETED, APPROVED, or PENDING exists for the payment // (FAILED/REJECTED refunds never moved money and are ignored). Shares the exact @@ -1346,9 +1514,11 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu // Match the real Square API: source_id must be a token (cnon:xxx nonce or // ccof:xxx card ID). Raw PANs are rejected exactly as Square would, so the - // mock behaves identically to production. The mock's token predicate - // additionally accepts the verify_mock_* transition shape the dev frontend - // mints (isTokenLikeMock). + // mock behaves identically to production. The legacy verify_mock_* shape is + // rejected explicitly — real Square never accepts it as a source. + if strings.HasPrefix(cardToken, "verify_mock_") { + return nil, legacyVerifyMockSourceError(cardToken) + } if !isTokenLikeMock(cardToken) { return nil, fmt.Errorf("invalid source_id: %s — use a card nonce (cnon:xxx) or card ID (ccof:xxx)", tokenPrefix(cardToken)) } diff --git a/backend/internal/square/square_dev_test.go b/backend/internal/square/square_dev_test.go index 50540d0..a7c24a3 100644 --- a/backend/internal/square/square_dev_test.go +++ b/backend/internal/square/square_dev_test.go @@ -891,6 +891,64 @@ func TestDevClient_ReplayPaymentByKey_UnknownKey_NotRetained(t *testing.T) { assert.True(t, errors.Is(err, ErrReplayKeyNotRetained), "unknown-key cnon replay must surface ErrReplayKeyNotRetained, got %v", err) } +func TestDevClient_ReplayPaymentByKey_CorruptSnapshot_PlainError(t *testing.T) { + // Identical-body replay contract (FIX 4): a snapshot missing source_id or + // idempotency_key is CORRUPT. Prod (replayPaymentByKeyHTTPWithClient, + // square_http_client.go) returns a plain (ambiguous) error for such a + // snapshot — the sweep leaves the row PENDING for manual reconciliation. + // The mock must mirror that, NOT answer ErrReplayKeyNotRetained (which the + // sweep treats as "the charge provably never happened" and definitively + // fails the row) — that is the OPPOSITE money decision on a snapshot we + // cannot trust. + client := NewDevClient().(*MockClient) + ctx := context.Background() + + t.Run("empty_source_is_plain_error", func(t *testing.T) { + _, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "", + IdempotencyKey: "key-empty-source", + })) + require.Error(t, err) + assert.False(t, errors.Is(err, ErrReplayKeyNotRetained), "a corrupt snapshot must NOT be treated as proof the charge never happened, got %v", err) + assert.Contains(t, err.Error(), "missing source_id/idempotency_key") + assert.Equal(t, "", ErrorCode(err), "a corrupt snapshot is an ambiguous plain error, not a structured Square rejection") + }) + + t.Run("empty_key_is_plain_error", func(t *testing.T) { + _, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "", + })) + require.Error(t, err) + assert.False(t, errors.Is(err, ErrReplayKeyNotRetained), "a corrupt snapshot must NOT be treated as proof the charge never happened, got %v", err) + assert.Contains(t, err.Error(), "missing source_id/idempotency_key") + }) + + t.Run("empty_source_must_not_charge", func(t *testing.T) { + // Regression guard for the OLD mock behaviour: with a corrupt snapshot + // (empty source) the mock must not fall through to the unknown-key + // "attempt a real charge" path at all — a ccof-ish empty source must + // never mint a payment. + client.mu.RLock() + payCount := len(client.payments) + client.mu.RUnlock() + _, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "", + IdempotencyKey: "key-empty-source-2", + })) + require.Error(t, err) + client.mu.RLock() + defer client.mu.RUnlock() + assert.Equal(t, payCount, len(client.payments), "a corrupt snapshot must never be replayed into a charge") + }) +} + func TestDevClient_ReplayPaymentByKey_UnknownKey_CcofSavedCard_ChargesAndRescues(t *testing.T) { // B3 dev/prod parity: an unknown key with a STILL-VALID ccof: saved-card // token makes real Square attempt a REAL charge that succeeds — the sweep @@ -1419,6 +1477,53 @@ func TestDevClient_CreatePayment_RejectsRawPAN(t *testing.T) { } } +// TestDevClient_CreatePayment_RejectsLegacyVerifyMockSource locks the FIX 5 +// removal of the verify_mock_ source widening: the transition to genuine +// cnon:sca- tokenize-results is complete and real Square NEVER accepts the +// verify_mock__ shape as a source_id — so the mock rejects it +// explicitly with a clear legacy-shape error (a raw PAN-style "invalid +// source_id" message would obscure the reason). verify_mock_ tokens remain +// valid in the VerificationToken field (the deprecated verifyBuyer() contract) +// — only the source-slot widening is removed. +func TestDevClient_CreatePayment_RejectsLegacyVerifyMockSource(t *testing.T) { + client := NewDevClient().(*MockClient) + ctx := context.Background() + + t.Run("create_payment_rejects_verify_mock_source", func(t *testing.T) { + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "verify_mock_4242_5000_ok", + IdempotencyKey: "verify-mock-source", + }) + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "legacy verify_mock_ transition shape") + assert.False(t, errors.Is(err, ErrRefundDeclined)) + }) + + t.Run("create_card_on_file_rejects_verify_mock_source", func(t *testing.T) { + card, err := client.CreateCardOnFile(ctx, "user-verify-mock", "verify_mock_4242_5000_ok", "cus_test123") + require.Error(t, err) + assert.Nil(t, card) + assert.Contains(t, err.Error(), "legacy verify_mock_ transition shape") + }) + + t.Run("verify_mock_verification_token_still_accepted", func(t *testing.T) { + // The legacy verifyBuyer() contract (ccof: + verification_token) is + // retained for backward-compat — only the SOURCE widening is removed. + client.SimulateSavedCardVerificationRequired = true + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "ccof:mock_saved", CustomerID: "cus_test123", + IdempotencyKey: "verify-mock-token-still-ok", + VerificationToken: "verify_mock_mock_5000_ok", + }) + require.NoError(t, err) + require.Equal(t, "COMPLETED", result.Status) + }) +} + // TestDevClient_CreatePayment_CardOnFileRequiresCustomerID verifies the mock // mirrors Square's real enforcement: charging a ccof: (card-on-file) token // without a customer_id is rejected with a structured 400 INVALID_REQUEST_ERROR @@ -2047,10 +2152,13 @@ func TestDevClient_RefundPayment_UnknownPayMockID_NotFound(t *testing.T) { // TestDevClient_RefundPayment_OverRefund locks the mock's real Square // over-refund rejection: refunding more than the remaining balance answers 400 // REFUND_AMOUNT_INVALID. Square returns that SAME code for an already-refunded -// payment, so — exactly like the real client — the mock reconciles: no refund -// recorded yet → the amount is genuinely invalid → ErrRefundDeclined; an -// existing refund (money already moved) → ErrRefundAlreadyProcessed. The -// exact-remaining boundary refund succeeds. +// payment, so — exactly like the real client (paymentRefundedExactlyWithClient) +// — the mock reconciles: an EXACT-amount COMPLETED refund for the requested +// amount → ErrRefundAlreadyProcessed (that money provably moved); anything else +// (no refund, or a PARTIAL prior refund that does not cover the requested +// amount) → the amount is genuinely invalid → ErrRefundDeclined. A different +// amount is NEVER AlreadyProcessed — the over-refund guard bug must surface in +// dev too. The exact-remaining boundary refund succeeds. func TestDevClient_RefundPayment_OverRefund(t *testing.T) { client := NewDevClient().(*MockClient) ctx := context.Background() @@ -2096,7 +2204,7 @@ func TestDevClient_RefundPayment_OverRefund(t *testing.T) { assert.Equal(t, "COMPLETED", second.Status) }) - t.Run("over_refund_with_existing_refund_is_already_processed", func(t *testing.T) { + t.Run("over_refund_after_partial_refund_is_declined", func(t *testing.T) { payment, err := client.CreatePayment(ctx, CreatePaymentReq{ Amount: 10000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: "pay-overrefund-existing", }) @@ -2109,15 +2217,41 @@ func TestDevClient_RefundPayment_OverRefund(t *testing.T) { require.NoError(t, err) require.Equal(t, "COMPLETED", first.Status) + // Over-refunding with a DIFFERENT amount (8000) is a genuine decline: + // no EXACT-amount refund for 8000 exists, so prod classifies it + // ErrRefundDeclined (the row is marked failed — the over-refund guard + // bug surfaces) — never ErrRefundAlreadyProcessed, which would hide it. result, err := client.RefundPayment(ctx, RefundPaymentReq{ PaymentID: payment.ID, Amount: 8000, IdempotencyKey: "refund-overrefund-existing", }) require.Error(t, err) assert.Nil(t, result) - // The REFUND_AMOUNT_INVALID squareAPIError is hidden behind the %v - // sentinel wrap (ErrorCode returns ""), so the observable contract is - // the ErrRefundAlreadyProcessed sentinel. - assert.True(t, errors.Is(err, ErrRefundAlreadyProcessed), "an over-refund on a payment that already has refunds must reconcile to ErrRefundAlreadyProcessed, got %v", err) + assert.True(t, errors.Is(err, ErrRefundDeclined), "an over-refund on top of a PARTIAL prior refund must reconcile to ErrRefundDeclined (no exact-amount refund covers 8000), got %v", err) + assert.False(t, errors.Is(err, ErrRefundAlreadyProcessed)) + }) + + t.Run("over_refund_exact_amount_already_refunded_is_already_processed", func(t *testing.T) { + payment, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 10000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: "pay-overrefund-exact", + }) + require.NoError(t, err) + + // An 8000 refund leaves 2000 remaining. + first, err := client.RefundPayment(ctx, RefundPaymentReq{ + PaymentID: payment.ID, Amount: 8000, IdempotencyKey: "refund-exact-move", + }) + require.NoError(t, err) + require.Equal(t, "COMPLETED", first.Status) + + // Re-requesting the SAME 8000 (now an over-refund) matches an EXACT- + // amount COMPLETED refund — that money provably already moved, exactly + // like prod's paymentRefundedExactlyWithClient reconciliation. + result, err := client.RefundPayment(ctx, RefundPaymentReq{ + PaymentID: payment.ID, Amount: 8000, IdempotencyKey: "refund-overrefund-exact", + }) + require.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, ErrRefundAlreadyProcessed), "an over-refund matching an existing EXACT-amount refund must reconcile to ErrRefundAlreadyProcessed, got %v", err) assert.False(t, errors.Is(err, ErrRefundDeclined)) }) } @@ -2162,7 +2296,9 @@ func TestDevClient_CreatePayment_SimulateVerificationRequired(t *testing.T) { assert.Nil(t, result) assert.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) assert.Equal(t, "PAYMENT_METHOD_ERROR", ErrorCategory(err)) - assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + // The WIRE status must be 402 (Square's documented status for + // verification-required), not 400. + assert.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) assert.True(t, IsDefinitivePaymentError(err), "CARD_DECLINED_VERIFICATION_REQUIRED must classify as a definitive payment error") }) @@ -2281,7 +2417,9 @@ func TestDevClient_CreatePayment_SavedCardVerificationRequired(t *testing.T) { assert.Nil(t, result) assert.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) assert.Equal(t, "PAYMENT_METHOD_ERROR", ErrorCategory(err)) - assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + // The WIRE status must be 402 (Square's documented status for + // verification-required), not 400. + assert.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) assert.True(t, IsDefinitivePaymentError(err), "CARD_DECLINED_VERIFICATION_REQUIRED must classify as a definitive payment error") }) @@ -2674,7 +2812,7 @@ func TestCreatePayment_SCA_SavedCard_WireBody_ByteIdentical(t *testing.T) { req := CreatePaymentReq{ Amount: 5000, Currency: "GBP", - SourceID: "cnon:sca-tokenize-result", + SourceID: "cnon:sca-4242_5000_ok", IdempotencyKey: "sca-contract-key-1", ReferenceID: "booking-contract-1", Note: "full", @@ -2700,7 +2838,7 @@ func TestCreatePayment_SCA_SavedCard_WireBody_ByteIdentical(t *testing.T) { // contract, not a shared-but-wrong shape. var wire map[string]any require.NoError(t, json.Unmarshal(clientWire, &wire)) - require.Equal(t, "cnon:sca-tokenize-result", wire["source_id"], "the tokenize-result token must be the source_id") + require.Equal(t, "cnon:sca-4242_5000_ok", wire["source_id"], "the tokenize-result token must be the source_id") require.Equal(t, "cus_sca_123", wire["customer_id"], "customer_id must come from the saved card") require.NotContains(t, wire, "verification_token", "the SCA tokenize-result path must not emit a legacy verification_token") amt, ok := wire["amount_money"].(map[string]any) @@ -2747,7 +2885,7 @@ func TestDevClient_CreatePayment_SavedCard_SCATokenizeResult_Accepted(t *testing t.Run("tokenize_result_with_customer_is_sca_compliant", func(t *testing.T) { result, err := client.CreatePayment(ctx, CreatePaymentReq{ Amount: 5000, Currency: "GBP", - SourceID: "cnon:sca-tokenize-1", CustomerID: "cus_sca_1", + SourceID: "cnon:sca-4242_5000_ok", CustomerID: "cus_sca_1", IdempotencyKey: "sca-accept-1", }) require.NoError(t, err) @@ -2780,7 +2918,7 @@ func TestDevClient_CreatePayment_SavedCard_SCATokenizeResult_BothGatesOn(t *test t.Run("tokenize_result_with_customer_passes_both_gates", func(t *testing.T) { result, err := client.CreatePayment(ctx, CreatePaymentReq{ Amount: 5000, Currency: "GBP", - SourceID: "cnon:sca-both-on", CustomerID: "cus_sca_both", + SourceID: "cnon:sca-4242_5000_ok", CustomerID: "cus_sca_both", IdempotencyKey: "both-on-accept", }) require.NoError(t, err) @@ -2826,14 +2964,16 @@ func TestDevClient_CreatePayment_SavedCard_RawNonceInTokenizeSlot_Rejected(t *te require.Nil(t, result) require.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) require.Equal(t, "PAYMENT_METHOD_ERROR", ErrorCategory(err)) - require.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + // The WIRE status must be 402 (Square's documented status for + // verification-required), not 400. + require.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) require.True(t, IsDefinitivePaymentError(err), "a forged unverified nonce must be a definitive payment error") }) t.Run("genuine_tokenize_result_still_accepted", func(t *testing.T) { result, err := client.CreatePayment(ctx, CreatePaymentReq{ Amount: 5000, Currency: "GBP", - SourceID: "cnon:sca-tokenize-fresh", CustomerID: "cus_genuine", + SourceID: "cnon:sca-4242_5000_ok", CustomerID: "cus_genuine", IdempotencyKey: "genuine-tokenize-result", }) require.NoError(t, err) @@ -2863,7 +3003,7 @@ func TestDevClient_CreatePayment_SavedCard_SCATokenizeResult_SingleUse(t *testin client.SimulateSourceUsed = true client.SimulateSavedCardVerificationRequired = true ctx := context.Background() - source := "cnon:sca-single-use-booking" + source := "cnon:sca-4242_5000_ok" // Booking A's charge consumes the tokenize-result. first, err := client.CreatePayment(ctx, CreatePaymentReq{ @@ -2887,3 +3027,133 @@ func TestDevClient_CreatePayment_SavedCard_SCATokenizeResult_SingleUse(t *testin require.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) require.Contains(t, client.UsedSources(), source, "the consumed tokenize-result must be reported by UsedSources") } + +// TestParseSCATokenizeResult pins the deterministic cnon:sca- tokenize-result +// encoding the dev frontend mints (MockCardForm.tokenizeSavedCard / +// verifySavedCard, square.ts's tokenizeSavedCardWithVerification fallback): +// cnon:sca-_[_ok|_deny], outcome suffix defaults to approval, +// "_deny" sets denied=true, and anything malformed is not parseable. +func TestParseSCATokenizeResult(t *testing.T) { + tests := []struct { + name string + token string + wantOK bool + wantDenied bool + wantAmount int64 + wantPrefix string + }{ + {"explicit ok suffix", "cnon:sca-4242_5000_ok", true, false, 5000, "4242"}, + {"deny suffix sets denied", "cnon:sca-4242_5000_deny", true, true, 5000, "4242"}, + {"no suffix defaults to approval", "cnon:sca-mock_2500", true, false, 2500, "mock"}, + {"prefix with underscores", "cnon:sca-visa_card_3000_deny", true, true, 3000, "visa_card"}, + {"marker with empty rest", "cnon:sca-", false, false, 0, ""}, + {"non-numeric amount", "cnon:sca-4242_abc_ok", false, false, 0, ""}, + {"missing amount", "cnon:sca-4242_ok", false, false, 0, ""}, + {"not a tokenize-result", "cnon:test-card", false, false, 0, ""}, + {"raw tokenize-result marker", "cnon:sca-tokenize-result", false, false, 0, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + parsed, ok := parseSCATokenizeResult(tt.token) + require.Equal(t, tt.wantOK, ok) + if ok { + require.Equal(t, tt.wantDenied, parsed.denied) + require.Equal(t, tt.wantAmount, parsed.amount) + require.Equal(t, tt.wantPrefix, parsed.prefix) + } + }) + } +} + +// TestDevClient_CreatePayment_SCATokenizeResult_BindingValidation locks the +// FIX 2 contract: the mock validates the deterministic binding the dev frontend +// encodes into a cnon:sca- tokenize-result. A token bound to the correct card +// prefix + amount with an _ok outcome passes; a wrong-prefix, wrong-amount, +// buyer-denied (_deny) or unparseable cnon:sca- token is refused with the +// documented 402 CARD_DECLINED_VERIFICATION_REQUIRED (a definitive payment +// error) — an arbitrary cnon:sca-... string must never pass the gate. +func TestDevClient_CreatePayment_SCATokenizeResult_BindingValidation(t *testing.T) { + client := NewDevClient().(*MockClient) + client.SimulateSavedCardVerificationRequired = true + ctx := context.Background() + + t.Run("valid_token_passes", func(t *testing.T) { + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "cnon:sca-4242_5000_ok", CustomerID: "cus_valid", + IdempotencyKey: "sca-binding-valid", + }) + require.NoError(t, err) + require.Equal(t, "COMPLETED", result.Status) + }) + + t.Run("wrong_prefix_is_refused", func(t *testing.T) { + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "cnon:sca-9999_5000_ok", CustomerID: "cus_wrong_prefix", + IdempotencyKey: "sca-binding-wrong-prefix", + }) + require.Error(t, err) + require.Nil(t, result) + require.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) + require.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) + require.True(t, IsDefinitivePaymentError(err)) + }) + + t.Run("wrong_amount_is_refused", func(t *testing.T) { + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "cnon:sca-4242_9999_ok", CustomerID: "cus_wrong_amount", + IdempotencyKey: "sca-binding-wrong-amount", + }) + require.Error(t, err) + require.Nil(t, result) + require.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) + require.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) + require.True(t, IsDefinitivePaymentError(err)) + }) + + t.Run("deny_outcome_is_refused", func(t *testing.T) { + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "cnon:sca-4242_5000_deny", CustomerID: "cus_denied", + IdempotencyKey: "sca-binding-deny", + }) + require.Error(t, err) + require.Nil(t, result) + require.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) + require.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) + require.True(t, IsDefinitivePaymentError(err)) + }) + + t.Run("unparseable_cnon_sca_is_refused", func(t *testing.T) { + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "cnon:sca-garbage", CustomerID: "cus_forged", + IdempotencyKey: "sca-binding-forged", + }) + require.Error(t, err) + require.Nil(t, result) + require.Equal(t, "CARD_DECLINED_VERIFICATION_REQUIRED", ErrorCode(err)) + require.Equal(t, http.StatusPaymentRequired, ErrorStatusCode(err)) + require.True(t, IsDefinitivePaymentError(err)) + }) + + t.Run("mock_ledger_card_prefix_is_accepted", func(t *testing.T) { + // The frontend derives the token prefix from the SAVED card's ccof id + // (first 4 chars after ccof:). A card this mock created in its ledger + // ("ccof:mock_...") mints tokens bound to the "mock" prefix — the mock + // must recognise that prefix as valid. + card, err := client.CreateCardOnFile(ctx, "user-sca-ledger", "cnon:token-ledger", "cus_ledger") + require.NoError(t, err) + require.True(t, strings.HasPrefix(card.CardID, "ccof:mock_"), "mock-created cards use the ccof:mock_ shape") + + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", + SourceID: "cnon:sca-mock_5000_ok", CustomerID: "cus_ledger", + IdempotencyKey: "sca-binding-ledger", + }) + require.NoError(t, err) + require.Equal(t, "COMPLETED", result.Status) + }) +} diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index 8c1da5c..283830d 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -94,7 +94,14 @@ func SquareLocationID() string { func newHTTPClient() *httpClient { env := SquareEnvironment() baseURL := squareSandboxURL - if env == "production" { + // Empty/unknown SQUARE_ENVIRONMENT is production-ENFORCED, mirroring + // payments.IsExplicitDevOrMockEnv()'s fail-closed interpretation (an + // unset/mistyped env var must never downgrade to a less safe default), so + // the production base URL is used for both "production" and "". Only an + // explicit sandbox or dev/mock value resolves to the sandbox base URL (the + // dev/mock values never construct a real HTTP client anyway). + switch env { + case "production", "": baseURL = squareProductionURL } return &httpClient{ @@ -106,6 +113,24 @@ func newHTTPClient() *httpClient { } } +// ValidateCredentials is the fail-closed startup validation for the REAL +// Square API credentials. It is called from main.go's startup gate ONLY for +// non-dev/mock deployments (main.go checks payments.IsExplicitDevOrMockEnv +// first — the env interpretation stays in the payments package to avoid a +// drifting duplicate read). With a real deployment selected, a missing +// SQUARE_ACCESS_TOKEN or SQUARE_LOCATION_ID would make every Square API call +// fail at runtime (401 auth, or request bodies without location_id), breaking +// payments/refunds — so the process refuses to start, mirroring the +// SQUARE_WEBHOOK_SIGNATURE_KEY gate. Never includes the secret values. +func ValidateCredentials() { + if strings.TrimSpace(os.Getenv("SQUARE_ACCESS_TOKEN")) == "" { + log.Fatalf("FATAL: SQUARE_ACCESS_TOKEN environment variable not set with SQUARE_ENVIRONMENT=%q (non-mock) — every Square API call would be rejected (401) and online payment/refund processing is broken. Set the access token from the Square Developer Dashboard.", SquareEnvironment()) + } + if strings.TrimSpace(SquareLocationID()) == "" { + log.Fatalf("FATAL: SQUARE_LOCATION_ID environment variable not set with SQUARE_ENVIRONMENT=%q (non-mock) — Square payment requests would carry no location_id and fail at runtime. Set the location ID from the Square Developer Dashboard.", SquareEnvironment()) + } +} + func (c *httpClient) doJSON(ctx context.Context, method, path string, body, target any) error { if c.token == "" { return fmt.Errorf("square: SQUARE_ACCESS_TOKEN is not set") @@ -1008,7 +1033,16 @@ func listRefundsHTTP(ctx context.Context, paymentID string, beginTime time.Time) } func listRefundsHTTPWithClient(ctx context.Context, paymentID string, beginTime time.Time, hc *httpClient) ([]RefundResult, error) { - base := "/v2/refunds?begin_time=" + url.QueryEscape(beginTime.UTC().Format(time.RFC3339)) + "&limit=100" + // begin_time is optional at Square; a ZERO time must OMIT the param + // entirely. Formatting the zero time would send Square a year-1 timestamp + // (0001-01-01T00:00:00Z) that Square may reject. The reconciliation callers + // (PaymentWasRefunded / paymentRefundedExactlyWithClient) pass time.Time{} + // deliberately — they want ALL refunds, so the param must be absent, not + // "before year 1". + base := "/v2/refunds?limit=100" + if !beginTime.IsZero() { + base = "/v2/refunds?begin_time=" + url.QueryEscape(beginTime.UTC().Format(time.RFC3339)) + "&limit=100" + } path := base results := []RefundResult{} for page := 0; page < 20; page++ { diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index f561fff..cc80d31 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -12,10 +12,15 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" + "os" + "os/exec" "strings" "testing" "time" "unicode/utf8" + + "github.com/stretchr/testify/require" ) // TestReplayPaymentByKeyHTTP_IdenticalBody verifies the replay-by-key sends the @@ -783,6 +788,54 @@ func TestListRefundsHTTP_Pagination(t *testing.T) { }) } +// TestListRefundsHTTP_ZeroBeginTime_OmitsParam locks the FIX 6 parity: a ZERO +// begin_time must OMIT the begin_time query param entirely — formatting the +// zero time would send Square a year-1 timestamp (0001-01-01T00:00:00Z) it may +// reject. The reconciliation callers (PaymentWasRefunded / +// paymentRefundedExactlyWithClient) pass time.Time{} deliberately to list ALL +// refunds, so the param must be absent, not "before year 1". A non-zero +// begin_time still emits begin_time as before. +func TestListRefundsHTTP_ZeroBeginTime_OmitsParam(t *testing.T) { + var queries []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + queries = append(queries, r.URL.RawQuery) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"refunds":[],"cursor":""}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + + // Zero time (the reconciliation shape) → begin_time must be absent. + _, err := listRefundsHTTPWithClient(context.Background(), "pay_zero", time.Time{}, hc) + if err != nil { + t.Fatalf("listRefundsHTTPWithClient failed: %v", err) + } + if len(queries) != 1 { + t.Fatalf("expected 1 request, got %d", len(queries)) + } + if strings.Contains(queries[0], "begin_time=") { + t.Errorf("expected NO begin_time for a zero begin_time, got %q", queries[0]) + } + if !strings.Contains(queries[0], "limit=100") { + t.Errorf("expected limit=100 in the zero-time query, got %q", queries[0]) + } + + // Non-zero time → begin_time is emitted with the RFC3339 timestamp. + queries = nil + begin := time.Date(2026, 7, 1, 0, 0, 0, 0, time.UTC) + _, err = listRefundsHTTPWithClient(context.Background(), "pay_begin", begin, hc) + if err != nil { + t.Fatalf("listRefundsHTTPWithClient failed: %v", err) + } + if len(queries) != 1 { + t.Fatalf("expected 1 request, got %d", len(queries)) + } + if !strings.Contains(queries[0], "begin_time="+url.QueryEscape("2026-07-01T00:00:00Z")) { + t.Errorf("expected begin_time=2026-07-01T00:00:00Z in query, got %q", queries[0]) + } +} + // TestCreateCardOnFileHTTP_IdempotencyKey verifies the deterministic SHA-256 // idempotency key derivation and the request wire shape (source_id + card). func TestCreateCardOnFileHTTP_IdempotencyKey(t *testing.T) { @@ -1889,8 +1942,12 @@ func TestPaymentWasRefunded(t *testing.T) { if r.URL.Path != "/v2/refunds" { t.Errorf("expected /v2/refunds, got %s", r.URL.Path) } - if !strings.Contains(r.URL.RawQuery, "begin_time=") { - t.Errorf("expected begin_time in query, got %q", r.URL.RawQuery) + if strings.Contains(r.URL.RawQuery, "begin_time=") { + // The reconciliation passes a ZERO begin_time (time.Time{}) + // to mean "all refunds"; the param must be ABSENT (FIX 6) — + // sending begin_time=0001-01-01T00:00:00Z is a year-1 + // timestamp Square may reject. + t.Errorf("expected NO begin_time in the zero-time reconciliation query, got %q", r.URL.RawQuery) } w.Header().Set("Content-Type", "application/json") if tc.statusCode != 0 { @@ -1946,8 +2003,13 @@ func TestRefundPaymentHTTP_RefundAmountInvalidReconciliation(t *testing.T) { if r.URL.Path != "/v2/refunds" { t.Errorf("expected reconciliation GET /v2/refunds, got %s", r.URL.Path) } - if !strings.Contains(r.URL.RawQuery, "begin_time=") { - t.Errorf("expected begin_time in reconciliation query, got %q", r.URL.RawQuery) + if !strings.Contains(r.URL.RawQuery, "limit=100") { + t.Errorf("expected limit=100 in reconciliation query, got %q", r.URL.RawQuery) + } + if strings.Contains(r.URL.RawQuery, "begin_time=") { + // The reconciliation passes a ZERO begin_time to mean + // "all refunds"; the param must be ABSENT (FIX 6). + t.Errorf("expected NO begin_time in the zero-time reconciliation query, got %q", r.URL.RawQuery) } _, _ = w.Write([]byte(tc.refundList)) return @@ -2090,3 +2152,72 @@ func TestCreatePaymentHTTP_SCASavedCard_WireShape(t *testing.T) { t.Errorf("expected location_id, got %v", captured["location_id"]) } } + +// TestNewHTTPClient_BaseURLSelection locks the SQUARE_ENVIRONMENT → base URL +// resolution (FIX 3b): an empty SQUARE_ENVIRONMENT is production-ENFORCED +// (mirroring payments.IsExplicitDevOrMockEnv's fail-closed interpretation — +// an unset env var must never downgrade to the sandbox), so empty resolves to +// the production base URL, exactly like "production". Only an explicit +// "sandbox" or a dev/mock value resolves to the sandbox base URL. +func TestNewHTTPClient_BaseURLSelection(t *testing.T) { + cases := []struct { + name string + env string + want string + }{ + {name: "production_env", env: "production", want: squareProductionURL}, + {name: "empty_env_is_production_enforced", env: "", want: squareProductionURL}, + {name: "sandbox_env", env: "sandbox", want: squareSandboxURL}, + {name: "mock_env", env: "mock", want: squareSandboxURL}, + {name: "dev_env", env: "dev", want: squareSandboxURL}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", tc.env) + if got := newHTTPClient().baseURL; got != tc.want { + t.Errorf("SQUARE_ENVIRONMENT=%q: expected base URL %s, got %s", tc.env, tc.want, got) + } + }) + } +} + +// TestValidateCredentials_FatalBranch_Exits covers the fail-closed startup +// validation of the REAL Square API credentials (FIX 3a): when the +// production-enforced gate in main.go (non-dev/mock SQUARE_ENVIRONMENT) calls +// ValidateCredentials and SQUARE_ACCESS_TOKEN or SQUARE_LOCATION_ID is +// missing, the process must refuse to start (log.Fatalf → os.Exit). log.Fatalf +// cannot run in-process, so the test re-executes the test binary with marker +// env vars and asserts the subprocess exits non-zero with a FATAL message +// naming the missing credential. +func TestValidateCredentials_FatalBranch_Exits(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { + t.Setenv("SQUARE_ENVIRONMENT", "production") + switch os.Getenv("SQUARE_CRED_BRANCH") { + case "MISSING_TOKEN": + t.Setenv("SQUARE_ACCESS_TOKEN", "") + t.Setenv("SQUARE_LOCATION_ID", "L_TEST") + case "MISSING_LOCATION": + t.Setenv("SQUARE_ACCESS_TOKEN", "tok") + t.Setenv("SQUARE_LOCATION_ID", "") + } + ValidateCredentials() + return + } + for _, tc := range []struct { + name string + branch string + wantIn string + }{ + {name: "missing_token_exits", branch: "MISSING_TOKEN", wantIn: "SQUARE_ACCESS_TOKEN"}, + {name: "missing_location_exits", branch: "MISSING_LOCATION", wantIn: "SQUARE_LOCATION_ID"}, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=TestValidateCredentials_FatalBranch_Exits") + cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1", "SQUARE_CRED_BRANCH="+tc.branch) + out, err := cmd.CombinedOutput() + require.Error(t, err, "expected the branch to exit (log.Fatalf); output: %s", out) + require.Contains(t, string(out), tc.wantIn, "the fatal log must name the missing credential: %s", out) + require.False(t, strings.Contains(string(out), "unexpected argument"), "the helper must not fail on argument parsing: %s", out) + }) + } +} diff --git a/backend/main.go b/backend/main.go index 6b94886..77a4f92 100644 --- a/backend/main.go +++ b/backend/main.go @@ -237,16 +237,20 @@ func initSquare() { checkSnapshotEncKey() checkProxyRateLimitConfig() checkWebhookSignatureKey() + checkSquareCredentials() } // checkSnapshotEncKey validates SNAPSHOT_ENC_KEY at startup in non-mock // deployments. charge_helpers.snapshotEncKey() (handlers/payments) parses the // key on every call and silently falls back to storing square_request_snapshot // rows PLAINTEXT (buyer PII: email + ccof card tokens) with a one-time CRITICAL -// log. This startup check makes the misconfiguration unmissable at boot: the -// key must be present and decode to exactly 32 bytes (AES-256). Money-safety -// first — it warns CRITICAL but does NOT fail the process (a failing startup -// would strand pending replayable snapshots), matching the runtime fallback. +// log. That runtime fallback is a money-safety convenience for the dev/mock +// stack (no real PII), but in a REAL deployment a missing/invalid key would +// leave buyer email + ccof card tokens unencrypted at rest — so this startup +// check FAILS CLOSED there, mirroring the JWT_SECRET_KEY gate (main.go init): +// in a non-mock environment the key must be present and decode to exactly 32 +// bytes (AES-256), or the process refuses to start. The warn+plaintext-fallback +// posture survives ONLY in explicit dev/mock environments (no real data). func checkSnapshotEncKey() { if payments.IsExplicitDevOrMockEnv() { return @@ -254,15 +258,14 @@ func checkSnapshotEncKey() { raw := strings.TrimSpace(os.Getenv("SNAPSHOT_ENC_KEY")) switch { case raw == "": - log.Printf("CRITICAL: SNAPSHOT_ENC_KEY is not set with SQUARE_ENVIRONMENT=%q (non-mock) — square_request_snapshot rows (buyer PII: email + ccof card tokens) will be stored PLAINTEXT at rest. Generate a base64-encoded 32-byte key with `openssl rand -base64 32`.", os.Getenv("SQUARE_ENVIRONMENT")) - return + log.Fatalf("FATAL: SNAPSHOT_ENC_KEY is not set with SQUARE_ENVIRONMENT=%q (non-mock) — square_request_snapshot rows (buyer PII: email + ccof card tokens) would be stored PLAINTEXT at rest. Generate a base64-encoded 32-byte key with `openssl rand -base64 32`.", os.Getenv("SQUARE_ENVIRONMENT")) default: decoded, err := base64.StdEncoding.DecodeString(raw) switch { case err != nil: - log.Printf("CRITICAL: SNAPSHOT_ENC_KEY is not valid base64 (%v) with SQUARE_ENVIRONMENT=%q (non-mock) — square_request_snapshot rows will be stored PLAINTEXT at rest. Generate a base64-encoded 32-byte key with `openssl rand -base64 32`.", err, os.Getenv("SQUARE_ENVIRONMENT")) + log.Fatalf("FATAL: SNAPSHOT_ENC_KEY is not valid base64 (%v) with SQUARE_ENVIRONMENT=%q (non-mock) — square_request_snapshot rows would be stored PLAINTEXT at rest. Generate a base64-encoded 32-byte key with `openssl rand -base64 32`.", err, os.Getenv("SQUARE_ENVIRONMENT")) case len(decoded) != 32: - log.Printf("CRITICAL: SNAPSHOT_ENC_KEY must decode to exactly 32 bytes for AES-256 (got %d) with SQUARE_ENVIRONMENT=%q (non-mock) — square_request_snapshot rows will be stored PLAINTEXT at rest. Generate a base64-encoded 32-byte key with `openssl rand -base64 32`.", len(decoded), os.Getenv("SQUARE_ENVIRONMENT")) + log.Fatalf("FATAL: SNAPSHOT_ENC_KEY must decode to exactly 32 bytes for AES-256 (got %d) with SQUARE_ENVIRONMENT=%q (non-mock) — square_request_snapshot rows would be stored PLAINTEXT at rest. Generate a base64-encoded 32-byte key with `openssl rand -base64 32`.", len(decoded), os.Getenv("SQUARE_ENVIRONMENT")) } } } @@ -307,9 +310,11 @@ func checkProxyRateLimitConfig() { // verification runs against that string, so every GENUINE Square event fails // signature verification (403) and payment/refund reconciliation is silently // broken. The key signals intent to use webhooks; the missing URL breaks them. -// This is WARN not FATAL: the handler is fail-closed, so no event — genuine or -// forged — can mutate state, making it availability-only (like -// checkProxyRateLimitConfig), not a security hole. +// A key with no URL is therefore FATAL (not a warning): the operator has +// explicitly configured webhooks, so every event silently failing signature +// verification would strand payment/refund reconciliation mid-run. The fail +// stays availability-only (the handler is fail-closed, so no event — genuine +// or forged — can mutate state), but it must be unmissable at boot. func checkWebhookSignatureKey() { if payments.IsExplicitDevOrMockEnv() { return @@ -320,7 +325,7 @@ func checkWebhookSignatureKey() { case keySet && urlSet: return case keySet && !urlSet: - log.Printf("WARNING: SQUARE_WEBHOOK_SIGNATURE_KEY IS set but SQUARE_WEBHOOK_NOTIFICATION_URL is unset with SQUARE_ENVIRONMENT=%q (non-mock) — the webhook handler falls back to the default http://localhost:8080/webhooks/square, so every GENUINE Square event fails signature verification (403, fail-closed) and payment/refund reconciliation is silently broken. Set SQUARE_WEBHOOK_NOTIFICATION_URL to exactly the notification URL configured in the Square Dashboard webhook subscription.", os.Getenv("SQUARE_ENVIRONMENT")) + log.Fatalf("FATAL: SQUARE_WEBHOOK_SIGNATURE_KEY IS set but SQUARE_WEBHOOK_NOTIFICATION_URL is unset with SQUARE_ENVIRONMENT=%q (non-mock) — the webhook handler falls back to the default http://localhost:8080/webhooks/square, so every GENUINE Square event fails signature verification (403, fail-closed) and payment/refund reconciliation is silently broken. Set SQUARE_WEBHOOK_NOTIFICATION_URL to exactly the notification URL configured in the Square Dashboard webhook subscription.", os.Getenv("SQUARE_ENVIRONMENT")) case !keySet && urlSet: log.Fatalf("FATAL: SQUARE_WEBHOOK_SIGNATURE_KEY environment variable not set with SQUARE_ENVIRONMENT=%q (non-mock) while SQUARE_WEBHOOK_NOTIFICATION_URL IS set — Square webhook events (payment/refund reconciliation) would be rejected fail-closed at runtime. Generate the signing key in the Square Dashboard webhook subscription and set it in .env.", os.Getenv("SQUARE_ENVIRONMENT")) default: @@ -328,6 +333,19 @@ func checkWebhookSignatureKey() { } } +// checkSquareCredentials fail-closes the REAL Square API credentials at +// startup in non-mock deployments. The env interpretation (dev/mock gate) +// stays here via payments.IsExplicitDevOrMockEnv — the single source — and +// the value validation itself lives in internal/square +// (square.ValidateCredentials) next to the client that consumes the +// credentials, so the check cannot drift from the code that reads them. +func checkSquareCredentials() { + if payments.IsExplicitDevOrMockEnv() { + return + } + square.ValidateCredentials() +} + func healthCheckHandler(w http.ResponseWriter, r *http.Request) { status := "ok" services := map[string]string{ diff --git a/backend/mw/ratelimit_shared.go b/backend/mw/ratelimit_shared.go index 00d5055..672e234 100644 --- a/backend/mw/ratelimit_shared.go +++ b/backend/mw/ratelimit_shared.go @@ -48,6 +48,18 @@ var trustProxyHeaders = func() bool { // middleware registration read this single source of truth. func TrustProxyHeaders() bool { return trustProxyHeaders } +// validIPString reports whether s parses as a syntactically valid IP address +// (net.ParseIP). Defense-in-depth for the trusted-proxy header path: the +// TRUST_PROXY_HEADERS flag MUST only be set behind a proxy that overwrites +// X-Real-IP/CF-Connecting-IP with the real client IP itself — but if it is +// ever mis-set (or a misbehaving proxy echoes the client's header), garbage +// values must not become rate-limit keys. A comma-joined chain +// ("123.45.67.89, 1.2.3.4"), an IP:port, or a non-IP string would otherwise +// mint a fresh bucket per request and bypass per-IP limiting entirely. +func validIPString(s string) bool { + return net.ParseIP(s) != nil +} + // ClientIP derives the per-client IP for security-sensitive handlers that need // a client-address key (reservation ipHash, per-IP audit trails) using the // SAME gated resolution as the rate limiter. Priority: @@ -57,23 +69,26 @@ func TrustProxyHeaders() bool { return trustProxyHeaders } // real_ip module validated it against the set_real_ip_from ranges) has // already overwritten it with the real client IP, so it is unspoofable // there. Ignored by default because an origin-exposed backend must never -// trust a client-controlled value (B7). +// trust a client-controlled value (B7). Even when trusted, the value must +// parse as a valid IP (validIPString) — defense-in-depth against a +// mis-set flag behind a header-echoing proxy. // 2. middleware.GetClientIP(r.Context()) — the X-Real-IP value nginx sets // from $remote_addr, captured by middleware.ClientIPFromHeader("X-Real-IP") // in main.go. That middleware is registered only when -// TRUST_PROXY_HEADERS=true, so it too is trusted solely behind a proxy. +// TRUST_PROXY_HEADERS=true, so it too is trusted solely behind a proxy; +// the same valid-IP check applies before it is accepted as the key. // 3. net.SplitHostPort(r.RemoteAddr) / r.RemoteAddr fallback — the actual // TCP peer; the only key source usable when the backend is origin-exposed. func ClientIP(r *http.Request) string { if trustProxyHeaders { - if ip := r.Header.Get("CF-Connecting-IP"); ip != "" { + if ip := r.Header.Get("CF-Connecting-IP"); validIPString(ip) { return ip } } - if ip := middleware.GetClientIP(r.Context()); ip != "" { + if ip := middleware.GetClientIP(r.Context()); validIPString(ip) { return ip } - if ip, _, err := net.SplitHostPort(r.RemoteAddr); err == nil && ip != "" { + if ip, _, err := net.SplitHostPort(r.RemoteAddr); err == nil && validIPString(ip) { return ip } return r.RemoteAddr diff --git a/backend/mw/ratelimit_test.go b/backend/mw/ratelimit_test.go index 223947d..34f7405 100644 --- a/backend/mw/ratelimit_test.go +++ b/backend/mw/ratelimit_test.go @@ -304,6 +304,68 @@ func TestClientIP_RemoteAddrWithoutPort_ReturnedAsIs(t *testing.T) { } } +// TestClientIP_XRealIP_GarbageHeaderFallsBackToRemoteAddr verifies the +// defense-in-depth validation (FIX 4): even with TRUST_PROXY_HEADERS=true, an +// X-Real-IP carrying a comma-joined chain ("123.45.67.89, 1.2.3.4"), an +// IP:port, or a non-IP string must NOT become the rate-limit key — it falls +// back to the TCP peer instead. The flag must only be set behind a trusted +// proxy that overwrites the header itself; this validation ensures a mis-set +// flag (or a header-echoing proxy) cannot mint a fresh bucket per request and +// bypass per-IP limiting. +func TestClientIP_XRealIP_GarbageHeaderFallsBackToRemoteAddr(t *testing.T) { + setTrustProxyHeaders(t, true) + for _, tc := range []struct { + name string + header string + }{ + {name: "comma_joined_chain", header: "123.45.67.89, 1.2.3.4"}, + {name: "ip_with_port", header: "198.51.100.42:8080"}, + {name: "garbage_string", header: "not-an-ip"}, + } { + t.Run(tc.name, func(t *testing.T) { + var got string + h := middleware.ClientIPFromHeader("X-Real-IP")(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = clientIP(r) + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "192.0.2.1:1234" + req.Header.Set("X-Real-IP", tc.header) + h.ServeHTTP(httptest.NewRecorder(), req) + + if got != "192.0.2.1" { + t.Errorf("expected garbage X-Real-IP %q to fall back to RemoteAddr 192.0.2.1, got %q", tc.header, got) + } + }) + } +} + +// TestClientIP_CFConnectingIP_GarbageHeaderFallsBack verifies the same +// defense-in-depth for CF-Connecting-IP: even when trusted, a non-IP value +// (comma-joined chain, port, garbage) must not become the rate-limit key. +func TestClientIP_CFConnectingIP_GarbageHeaderFallsBack(t *testing.T) { + setTrustProxyHeaders(t, true) + for _, tc := range []struct { + name string + header string + }{ + {name: "comma_joined_chain", header: "203.0.113.7, 198.51.100.9"}, + {name: "ip_with_port", header: "203.0.113.7:8080"}, + {name: "garbage_string", header: "spoofed!"}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "192.0.2.2:1234" + req.Header.Set("CF-Connecting-IP", tc.header) + + if got := clientIP(req); got != "192.0.2.2" { + t.Errorf("expected garbage CF-Connecting-IP %q to fall back to RemoteAddr 192.0.2.2, got %q", tc.header, got) + } + }) + } +} + // ============================================================ // RateLimit middleware — 429 on burst, pass-through under limit, // per-key buckets (batch-1 fix regression) diff --git a/backend/startup_checks_test.go b/backend/startup_checks_test.go index fa6fd48..c0c3850 100644 --- a/backend/startup_checks_test.go +++ b/backend/startup_checks_test.go @@ -28,34 +28,15 @@ func captureLog(t *testing.T, fn func()) string { return buf.String() } -// TestCheckSnapshotEncKey_FailLoudBranches pins the non-mock startup check for -// SNAPSHOT_ENC_KEY: a missing / invalid / wrong-length key must log a CRITICAL -// line (the at-rest PII warning), a valid base64 32-byte key logs nothing, and -// a dev/mock SQUARE_ENVIRONMENT skips the check entirely. -func TestCheckSnapshotEncKey_FailLoudBranches(t *testing.T) { +// TestCheckSnapshotEncKey_NonFatalBranches pins the surviving non-fatal +// branches of the startup check for SNAPSHOT_ENC_KEY: a valid base64 32-byte +// key logs nothing, and a dev/mock SQUARE_ENVIRONMENT skips the check +// entirely (the warn+plaintext-fallback posture survives ONLY there). The +// fail-closed branches (missing / invalid-base64 / wrong-length key in a +// non-mock env) log.Fatalf and are covered by the subprocess test below. +func TestCheckSnapshotEncKey_NonFatalBranches(t *testing.T) { t.Setenv("SQUARE_ENVIRONMENT", "production") - t.Run("missing_key_logs_critical", func(t *testing.T) { - t.Setenv("SNAPSHOT_ENC_KEY", "") - got := captureLog(t, checkSnapshotEncKey) - require.Contains(t, got, "SNAPSHOT_ENC_KEY is not set", "missing key must fail loud: %s", got) - }) - - t.Run("invalid_base64_logs_critical", func(t *testing.T) { - t.Setenv("SNAPSHOT_ENC_KEY", "!!!not-base64!!!") - got := captureLog(t, checkSnapshotEncKey) - require.Contains(t, got, "not valid base64", "invalid base64 must fail loud: %s", got) - }) - - t.Run("wrong_length_logs_critical", func(t *testing.T) { - // 16 bytes base64 → not 32 bytes → not AES-256. Built at runtime so no - // secret-shaped literal exists in source. - shortKey := base64.StdEncoding.EncodeToString([]byte("1234567890123456")) - t.Setenv("SNAPSHOT_ENC_KEY", shortKey) - got := captureLog(t, checkSnapshotEncKey) - require.Contains(t, got, "exactly 32 bytes", "wrong key length must fail loud: %s", got) - }) - t.Run("valid_key_logs_nothing", func(t *testing.T) { t.Setenv("SNAPSHOT_ENC_KEY", base64.StdEncoding.EncodeToString([]byte("12345678901234567890123456789012"))) // 32 bytes got := captureLog(t, checkSnapshotEncKey) @@ -70,6 +51,49 @@ func TestCheckSnapshotEncKey_FailLoudBranches(t *testing.T) { }) } +// TestCheckSnapshotEncKey_FatalBranch_Exits covers the fail-closed branches: +// in a non-mock environment an unset / invalid-base64 / wrong-length +// SNAPSHOT_ENC_KEY must refuse to start (log.Fatalf → os.Exit) instead of +// logging a CRITICAL warning and storing square_request_snapshot rows (buyer +// PII: email + ccof card tokens) PLAINTEXT at rest. log.Fatalf cannot run +// in-process, so the test re-executes the test binary with a marker env var +// and asserts the subprocess exits non-zero with a FATAL message naming the +// key. +func TestCheckSnapshotEncKey_FatalBranch_Exits(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { + t.Setenv("SQUARE_ENVIRONMENT", "production") + switch os.Getenv("SNAPSHOT_ENC_KEY_BRANCH") { + case "MISSING": + t.Setenv("SNAPSHOT_ENC_KEY", "") + case "INVALID_BASE64": + t.Setenv("SNAPSHOT_ENC_KEY", "!!!not-base64!!!") + case "WRONG_LENGTH": + // 16 bytes base64 → not 32 bytes → not AES-256. Built at runtime + // so no secret-shaped literal exists in source. + t.Setenv("SNAPSHOT_ENC_KEY", base64.StdEncoding.EncodeToString([]byte("1234567890123456"))) + } + checkSnapshotEncKey() + return + } + for _, tc := range []struct { + name string + branch string + }{ + {name: "unset_key_exits", branch: "MISSING"}, + {name: "invalid_base64_exits", branch: "INVALID_BASE64"}, + {name: "wrong_length_exits", branch: "WRONG_LENGTH"}, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=TestCheckSnapshotEncKey_FatalBranch_Exits") + cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1", "SNAPSHOT_ENC_KEY_BRANCH="+tc.branch) + out, err := cmd.CombinedOutput() + require.Error(t, err, "expected the non-mock invalid-key branch to exit (log.Fatalf); output: %s", out) + require.Contains(t, string(out), "SNAPSHOT_ENC_KEY", "the fatal log must name SNAPSHOT_ENC_KEY: %s", out) + require.False(t, strings.Contains(string(out), "unexpected argument"), "the helper must not fail on argument parsing: %s", out) + }) + } +} + // TestCheckProxyRateLimitConfig_WarnsWithoutTrustedProxy pins the fail-loud // warning: a non-mock deployment without TRUST_PROXY_HEADERS collapses every // per-IP limiter onto the proxy's address, so startup must warn. A dev/mock env @@ -84,12 +108,11 @@ func TestCheckProxyRateLimitConfig_WarnsWithoutTrustedProxy(t *testing.T) { require.Empty(t, got, "a dev/mock env must skip the check: %s", got) } -// TestCheckWebhookSignatureKey_Branches pins the non-fatal branches of the -// webhook signing-key startup check: both configured → silent; key without URL -// → WARNING (fail-closed availability note); neither → CRITICAL. The fatal -// key-less-with-URL branch is covered by the subprocess test below (log.Fatalf -// exits the process). -func TestCheckWebhookSignatureKey_Branches(t *testing.T) { +// TestCheckWebhookSignatureKey_NonFatalBranches pins the non-fatal branches of +// the webhook signing-key startup check: both configured → silent; neither → +// CRITICAL. The fatal branches — key-less-with-URL and key-without-URL — are +// covered by the subprocess tests below (log.Fatalf exits the process). +func TestCheckWebhookSignatureKey_NonFatalBranches(t *testing.T) { t.Setenv("SQUARE_ENVIRONMENT", "production") t.Run("both_configured_silent", func(t *testing.T) { @@ -99,13 +122,6 @@ func TestCheckWebhookSignatureKey_Branches(t *testing.T) { require.Empty(t, got, "both configured must be silent: %s", got) }) - t.Run("key_without_url_warns", func(t *testing.T) { - t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "k") - t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "") - got := captureLog(t, checkWebhookSignatureKey) - require.Contains(t, got, "SQUARE_WEBHOOK_NOTIFICATION_URL is unset", "key without URL must warn: %s", got) - }) - t.Run("neither_configured_critical", func(t *testing.T) { t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "") @@ -122,23 +138,42 @@ func TestCheckWebhookSignatureKey_Branches(t *testing.T) { }) } -// TestCheckWebhookSignatureKey_FatalBranch_Exits covers the fail-fast branch — -// a signing key is REQUIRED when a notification URL is configured, and the -// startup check calls log.Fatalf (os.Exit). That cannot run in-process, so the -// test re-executes the test binary with a marker env var and asserts the -// subprocess exits non-zero with the FATAL message naming the key. +// TestCheckWebhookSignatureKey_FatalBranch_Exits covers the fail-fast branches +// — a signing key is REQUIRED when a notification URL is configured, and a +// configured key with NO URL is equally fatal (the handler would verify HMACs +// against the public default URL, breaking every genuine event) — both call +// log.Fatalf (os.Exit). That cannot run in-process, so the test re-executes +// the test binary with a marker env var and asserts the subprocess exits +// non-zero with a FATAL message naming the misconfigured variable. func TestCheckWebhookSignatureKey_FatalBranch_Exits(t *testing.T) { if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { t.Setenv("SQUARE_ENVIRONMENT", "production") - t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") - t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "https://example.com/webhooks") + switch os.Getenv("WEBHOOK_BRANCH") { + case "KEY_WITHOUT_URL": + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "k") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "") + default: // URL_WITHOUT_KEY + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "https://example.com/webhooks") + } checkWebhookSignatureKey() return } - cmd := exec.Command(os.Args[0], "-test.run=TestCheckWebhookSignatureKey_FatalBranch_Exits") - cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") - out, err := cmd.CombinedOutput() - require.Error(t, err, "expected the key-less-with-URL branch to exit (log.Fatalf); output: %s", out) - require.Contains(t, string(out), "SQUARE_WEBHOOK_SIGNATURE_KEY", "the fatal log must name the missing key: %s", out) - require.False(t, strings.Contains(string(out), "unexpected argument"), "the helper must not fail on argument parsing: %s", out) + for _, tc := range []struct { + name string + branch string + wantIn string + }{ + {name: "key_without_url_exits", branch: "KEY_WITHOUT_URL", wantIn: "SQUARE_WEBHOOK_NOTIFICATION_URL"}, + {name: "url_without_key_exits", branch: "URL_WITHOUT_KEY", wantIn: "SQUARE_WEBHOOK_SIGNATURE_KEY"}, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=TestCheckWebhookSignatureKey_FatalBranch_Exits") + cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1", "WEBHOOK_BRANCH="+tc.branch) + out, err := cmd.CombinedOutput() + require.Error(t, err, "expected the branch to exit (log.Fatalf); output: %s", out) + require.Contains(t, string(out), tc.wantIn, "the fatal log must name the misconfigured variable: %s", out) + require.False(t, strings.Contains(string(out), "unexpected argument"), "the helper must not fail on argument parsing: %s", out) + }) + } }