fix: dev-mock/prod parity + fail-closed config gates — 402 verification, cnon:sca binding validation, refund reconcile parity, SNAPSHOT_ENC_KEY fail-closed, webhook config gates, access-token startup validation, ClientIP validation
- mock: 400->402 for CARD_DECLINED_VERIFICATION_REQUIRED, cnon:sca- tokenize-result binding validated (prefix/amount/deny), RefundPayment exact-amount reconcile parity, ReplayPaymentByKey snapshot sanity, verify_mock_ legacy widening removed, listRefunds zero-time omits begin_time - main.go: SNAPSHOT_ENC_KEY log.Fatalf in non-mock, webhook key-set-URL-unset log.Fatalf, SQUARE_ACCESS_TOKEN/LOCATION startup validation, empty-env base URL matches 2FA production interpretation - mw: ClientIP rejects garbage/comma/port XFF values, documented trusted-proxy requirement Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user