finding #79: multi-model security review catches CGN + proxy-assisted SSRF gaps

- Python ipaddress.is_private/is_reserved misses CGN (100.64.0.0/10)
- Go http.DefaultTransport clone retains ProxyFromEnvironment (proxy-assisted SSRF)
- Both gaps survived Sonnet+GPT approval; only security-reviewer blocked merge
- Lesson: dedicated security reviewer role required for auth/network security code

findings/2026-05-14-cgn-proxy-ssrf-multimodel-catch.md

@@ -0,0 +1,77 @@
+# Finding #79: Multi-Model Security Review Catches CGN + Proxy-Assisted SSRF Gaps
+
+**Date:** 2026-05-14
+**Task:** Review PR #129 (rodin/review-bot) — SSRF defense for Gitea client
+**Task type:** Security review — multi-model pipeline (Config A: GPT-5 investigates, Opus judges)
+
+## Summary
+
+The multi-model review pipeline's security-reviewer role caught 3 MAJOR SSRF gaps in a PR whose core
+intent was SSRF defense. These findings were against the **latest commit** (934c6728) after prior
+review rounds had already addressed earlier findings. The PR author believed it was ready; two other
+reviewers (Sonnet, GPT) approved. Only the security-focused review pass blocked the merge.
+
+## Findings Caught
+
+### Finding 1 & 2: Carrier-Grade NAT (CGN) Bypass in Python Preflight
+
+**File:** `.gitea/actions/review/action.yml` (lines ~209, ~354)
+**Root cause:** Python's `ipaddress.IPv4Address.is_private` returns `False` for 100.64.0.0/10 (CGN),
+and `is_reserved` does not cover it either. The SSRF pre-check had two validation blocks (initial
+check + re-validation before download), both missing this range.
+**Impact:** Attacker-controlled hostname resolving to a CGN address could exfiltrate ACTION_TOKEN
+to non-public infrastructure.
+**Fix:** Use `not ip.is_global` (which returns False for private, loopback, link-local, multicast,
+reserved, unspecified, AND CGN) in both validation blocks.
+
+### Finding 3: Proxy-Assisted SSRF in Go Client
+
+**File:** `gitea/client.go` (line ~143)
+**Root cause:** `newSafeHTTPClient` clones `http.DefaultTransport`, retaining `ProxyFromEnvironment`.
+When an HTTP(S) proxy is configured in the environment, `DialContext` connects to the proxy — which
+forwards the Authorization header through — and the target host/IP is resolved by the proxy, not
+the client. `safeDialContext` never validates the proxied destination.
+**Impact:** An attacker supplying a private IP (e.g., 169.254.169.254) as `baseURL` could use an
+environment-configured proxy to bypass the IP blocklist entirely.
+**Fix:** Set `Transport.Proxy = nil` in the safe transport, since baseURL is user-supplied and the
+IP validation cannot be enforced client-side when a proxy intermediates the connection.
+
+## Why Other Reviewers Missed These
+
+- **Sonnet reviewer (structural):** Approved. Focused on code structure and defense-in-depth layers;
+  didn't check Python network library semantics or proxy interaction.
+- **GPT reviewer (operational-gaps):** Approved with a note that security gaps "should be addressed
+  before merge" — but still voted APPROVE. Noted the gaps existed but didn't block.
+- **Security-reviewer (dedicated):** Blocked. The CGN gap requires knowing that Python's `is_private`
+  and `is_reserved` don't cover 100.64/10 — a non-obvious library behavior. The proxy bypass
+  requires understanding that OS-level proxy settings interact with `http.DefaultTransport` cloning.
+
+**Key pattern:** Security gaps at the intersection of:
+1. Specific library semantics (Python ipaddress module edge cases)
+2. OS/environment interaction (proxy environment variables + HTTP transport inheritance)
+...require a dedicated security reviewer role, not general code review. Multi-model pipeline
+with explicit security role is the right call for auth/crypto/network security code.
+
+## Pipeline Configuration
+
+- **Config A (Even PR# = 129):** GPT-5 investigates, Opus judges (for multi-model phase)
+- **Security review:** Dedicated `security-review-bot` pass (always runs, not A/B)
+- **Result:** Security pass blocked merge, preventing a subtle but real SSRF bypass
+
+## Lessons
+
+1. **`is_global` > `is_private` + `is_reserved` for SSRF checks:** Python's positive allowlist check
+   (`not ip.is_global`) is more robust than negative blocklist approaches. CGN is the canonical
+   missed case; there may be others.
+
+2. **HTTP transport inheritance is a security footgun:** Cloning `http.DefaultTransport` in Go
+   preserves proxy settings. Security-sensitive transports should always explicitly set `Proxy: nil`
+   unless proxy use is intentional and the proxy is trusted.
+
+3. **Approval from non-security reviewers ≠ security clearance:** Sonnet and GPT both approved
+   despite these gaps. Only a reviewer configured with security-specific review criteria blocked
+   the merge. Dedicated security reviewer role provides value even when other models say APPROVE.
+
+4. **Multi-pass reviews on security code pay off:** PR #129 had already undergone one review round
+   with fixes. The security gaps persisted through that round. Security-focused second pass caught
+   what the first pass missed.