fix: adversarial review round — replay-rescue double-charge, discount credit, 2FA/per-IP limits, snapshot encryption, refund reconciliation, VAT, frontend parity, tests+docs
Addresses the adversarial fresh-eyes audit (findings A1-A20) plus review-round fixes: - CRITICAL A1: replay-by-key rescue cross-checks replayed CreatedAt; ccof blind-fail leaves pending with CRITICAL + notification instead of clawing back - A2/A3/A4: till idempotency key restored to unconditional hash; tip rejected in CreateBookingPayment; campaign discount now reduces the charged amount (deposit credit) - A5: admin notifications on blind-fail, manual-refund re-arm, cap-stranded charge-group, webhook FAILED/REJECTED refunds - A6/A10: BuyGiftCard idempotency user-scoped; gift-card slot scan advances past failed rows - A7/A14/A15: 2FA user+IP limiter, SNAPSHOT_ENC_KEY startup validation, accurate pepper/log-delivery docs - A8/A9: snapshot encryption on all write+reuse sites; MPV->SPV effective voucher type (single VAT point) - A11/A12/A13/A16: amount-aware refund reconciliation; completed-booking refund re-validation; till retry dedup; PaymentWasRefunded on SquareClient interface - A17/A18/A19/A20: CI runs npm test; confirm_overflow_tip frontend dialog; unknown-event admin notification; mock token redaction - M7 ConfirmOverflowTip, M9 snapshot encryption, C1 discount ordering regression test - Frontend vitest framework (41 tests), backend coverage for fixed functions, docs corrected (2,269 tests, SUPPORT_EMAIL tokens, resolution status) All 25 backend packages pass; frontend 41/41; build + env-docs green.
This commit is contained in:
+91
-7
@@ -8,6 +8,7 @@ import (
|
||||
"crussell/internal/logutil"
|
||||
"crussell/internal/s3"
|
||||
"crussell/internal/square"
|
||||
"encoding/base64"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"log"
|
||||
@@ -180,10 +181,23 @@ func initSquare() {
|
||||
// case-insensitive) or SQUARE_ENVIRONMENT explicitly selects the dev/mock
|
||||
// stack. Warn loudly when a non-dev env (empty/unknown — a likely
|
||||
// misconfiguration) leaves the gate disabled, so saved-card charges can
|
||||
// never silently ship without the PSD2 SCA stand-in.
|
||||
// never silently ship without this merchant-level authorization gate (an
|
||||
// additional fraud control; NOT PSD2 SCA — Square buyer verification is the
|
||||
// SCA mechanism, wired for new-card charges).
|
||||
enforced := payments.NewPaymentService().TwoFactorEnforced()
|
||||
if enforced {
|
||||
log.Printf("WARNING: 2FA codes are delivered in PLAINTEXT via the server log ([2FA] prefix) — anyone with log read access can defeat the 2FA gate. Restrict backend log access and relay codes out-of-band; this loose-fake delivery must be replaced by email/SMS (P6) before launch.")
|
||||
// Delivery is build-dependent (handlers/user/twofa_dev.go /
|
||||
// twofa_prod.go): dev/test builds ALWAYS write the plaintext code to
|
||||
// the [2FA] log line; production builds write it ONLY when the operator
|
||||
// explicitly opts in with TWO_FACTOR_ALLOW_LOG_DELIVERY=true and refuse
|
||||
// issuance otherwise. Warn accurately per case so the operator is never
|
||||
// misled into thinking codes are reaching users when issuance is
|
||||
// actually failing closed.
|
||||
if os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" {
|
||||
log.Printf("WARNING: 2FA codes are delivered in PLAINTEXT via the server log ([2FA] prefix) — anyone with backend log access can defeat the 2FA gate. Restrict log access and relay codes out-of-band; replace this loose-fake delivery with email/SMS (P6) before launch.")
|
||||
} else {
|
||||
log.Printf("WARNING: 2FA enforcement is ON but TWO_FACTOR_ALLOW_LOG_DELIVERY is unset: in a production build there is NO code-delivery channel (email/SMS is not wired — P6), so 2FA code issuance FAILS CLOSED and no user can complete setup or disable. Every enforced saved-card online payment for a user without 2FA will 403 with no way to enable it. Set TWO_FACTOR_ALLOW_LOG_DELIVERY=true to opt into the insecure [2FA] log-delivery channel (plaintext codes in the server log — restrict log access), or wire email/SMS (P6).")
|
||||
}
|
||||
}
|
||||
if !enforced && !payments.IsExplicitDevOrMockEnv() {
|
||||
log.Printf("WARNING: 2FA enforcement is OFF (REQUIRE_2FA=%q) with SQUARE_ENVIRONMENT=%q (not an explicit mock/dev value). Online saved-card payments will NOT require 2FA.", os.Getenv("REQUIRE_2FA"), env)
|
||||
@@ -196,6 +210,59 @@ func initSquare() {
|
||||
if enforced && env != "sandbox" && env != "production" {
|
||||
log.Printf("WARNING: 2FA enforcement is ON but SQUARE_ENVIRONMENT=%q is empty/unknown — the Square client is the dev mock while the 2FA gate stays enforced (fail-closed). Online saved-card payments will 403 until users enable 2FA; set SQUARE_ENVIRONMENT to a dev value (mock/dev/development/test) to lift the gate, or to sandbox/production for the real API.", env)
|
||||
}
|
||||
|
||||
checkSnapshotEncKey()
|
||||
checkProxyRateLimitConfig()
|
||||
}
|
||||
|
||||
// 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.
|
||||
func checkSnapshotEncKey() {
|
||||
if payments.IsExplicitDevOrMockEnv() {
|
||||
return
|
||||
}
|
||||
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
|
||||
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"))
|
||||
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"))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// checkProxyRateLimitConfig warns when per-IP rate limiting collapses to a
|
||||
// single GLOBAL budget: TRUST_PROXY_HEADERS is unset/false (the shipped
|
||||
// default — .env.example ships false, compose.yml never sets it) while
|
||||
// SQUARE_ENVIRONMENT selects a real deployment (sandbox/production/empty).
|
||||
// Behind a trusted proxy (the nginx in compose.yml), every request's
|
||||
// RemoteAddr is the proxy's IP, so clientIP() returns the SAME key for all
|
||||
// users and any one client can exhaust the shared per-IP budget — permanently
|
||||
// 429ing the whole surface for everyone. The user+IP 2FA limiter is immune
|
||||
// (each authenticated account gets its own bucket), but every other per-IP
|
||||
// limiter still collapses. Set TRUST_PROXY_HEADERS=true when a trusted proxy
|
||||
// (nginx and/or the Cloudflare edge) sits in front and overwrites X-Real-IP /
|
||||
// CF-Connecting-IP with the real client IP.
|
||||
func checkProxyRateLimitConfig() {
|
||||
if payments.IsExplicitDevOrMockEnv() {
|
||||
return
|
||||
}
|
||||
if mw.TrustProxyHeaders() {
|
||||
return
|
||||
}
|
||||
log.Printf("WARNING: TRUST_PROXY_HEADERS is unset/false with SQUARE_ENVIRONMENT=%q (not a dev/mock value) — behind a trusted proxy (e.g. the nginx in compose.yml) every per-IP rate-limit key uses the proxy's RemoteAddr, collapsing all rate limiters to ONE global budget that any single client can exhaust for everyone. Set TRUST_PROXY_HEADERS=true when a trusted proxy sits in front and overwrites X-Real-IP/CF-Connecting-IP; keep it false only when the backend is origin-exposed.", os.Getenv("SQUARE_ENVIRONMENT"))
|
||||
}
|
||||
|
||||
func healthCheckHandler(w http.ResponseWriter, r *http.Request) {
|
||||
@@ -496,14 +563,31 @@ func main() {
|
||||
r.Put("/user/change-password", user.ChangePasswordHandler)
|
||||
r.Get("/user/notification-preferences", user.GetNotificationPreferencesHandler)
|
||||
r.Put("/user/notification-preferences", user.UpdateNotificationPreferencesHandler)
|
||||
// 2FA settings (loose-fake PSD2 SCA gate for online card payments).
|
||||
// 2FA settings — merchant-level authorization gate on saved-card
|
||||
// payments; NOT PSD2 SCA (Square buyer verification is the SCA
|
||||
// mechanism, wired for new-card charges); kept as an additional
|
||||
// fraud control until Square verification is wired for saved-card
|
||||
// charges.
|
||||
// RequireNonGuest: any logged-in user who could save cards must be
|
||||
// able to reach these, not just verified accounts.
|
||||
r.With(mw.RequireNonGuest).Get("/user/2fa/status", user.GetTwoFAStatusHandler)
|
||||
r.With(mw.RequireNonGuest).Post("/user/2fa/setup", user.SetupTwoFAHandler)
|
||||
r.With(mw.RequireNonGuest).Post("/user/2fa/verify", user.VerifyTwoFAHandler)
|
||||
r.With(mw.RequireNonGuest).Post("/user/2fa/disable", user.DisableTwoFAHandler)
|
||||
r.With(mw.RequireNonGuest).Post("/user/2fa/disable/code", user.SendDisableCodeHandler)
|
||||
// The code-issuing/verifying endpoints get a dedicated per-user+IP
|
||||
// limiter (10/min) on top of the group's generic 120/min limiter:
|
||||
// the 6-digit codes live in a 1M space, so a single user must not be
|
||||
// able to hammer setup/verify/disable faster than the per-user
|
||||
// 5-attempt lockout can trip. The key combines the authenticated
|
||||
// userID with the client IP: even behind a proxy that does not set
|
||||
// TRUST_PROXY_HEADERS=true (so every request's RemoteAddr is the
|
||||
// proxy's IP), the budget stays per-account — one account holder can
|
||||
// never exhaust a shared GLOBAL bucket that 429s the entire 2FA
|
||||
// surface (setup/verify/disable, and thus saved-card payments) for
|
||||
// everyone. One shared limiter for all four so the whole 2FA surface
|
||||
// counts against a single per-user budget.
|
||||
twoFALimiter := mw.RateLimitByUserAndIP(10, time.Minute)
|
||||
r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/setup", user.SetupTwoFAHandler)
|
||||
r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/verify", user.VerifyTwoFAHandler)
|
||||
r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/disable", user.DisableTwoFAHandler)
|
||||
r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/disable/code", user.SendDisableCodeHandler)
|
||||
r.Delete("/user/account", user.DeleteAccountHandler)
|
||||
r.Get("/user/gdpr-export", user.GetGDPRExportHandler)
|
||||
r.Get("/user/loyalty", user.GetLoyaltyHandler)
|
||||
|
||||
Reference in New Issue
Block a user