Security audit: SSRF attack surface analysis

[RafaelPo]

Summary

• Comprehensive SSRF security audit of the MCP server codebase (everyrow-mcp/src/everyrow_mcp/)
• Reviews the three-layer SSRF protection introduced in commit 4000b88
• Identifies 1 High, 3 Medium, 5 Low, and 3 Info findings with PoCs and recommended fixes
• Covers DNS rebinding TOCTOU, port restrictions, IP blocklist gaps, rate limit bypass, URL parser discrepancies, OAuth redirect validation, Redis key injection, and container hardening

Key findings

• FINDING-01 (High): DNS rebinding TOCTOU window — _SSRFSafeTransport narrows but doesn't eliminate the race between validation and TCP connect
• FINDING-02 (Medium): No port restriction — internal services reachable on non-standard ports
• FINDING-03 (Medium): Missing cloud IP ranges (100.64.0.0/10 CGNAT/AWS VPC) from blocklist
• FINDING-04 (Medium): Rate limit bypass via X-Forwarded-For spoofing without proxy IP validation

Test plan

• Review report for accuracy and completeness
• Prioritise findings for remediation
• Track fixes as follow-up issues

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 47s —— View job

---

Code Review: SSRF Audit Report

• Read CLAUDE.md for repo-specific guidelines
• Read the SSRF audit report
• Cross-reference findings against actual source code
• Post review feedback

---

Overall Assessment

This is an excellent, thorough security audit. I cross-referenced every finding against the actual source code and can confirm the technical accuracy is very high — line numbers, code snippets, and behavioural descriptions all check out. The three-layer SSRF architecture is correctly understood and well-documented. The severity ratings are reasonable, and the recommended fixes are practical. A few observations below.

---

Issues to Address

1. Local file paths leaked throughout the report

Every File: reference uses a local development path:

/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/...

These should be repo-relative paths (e.g., everyrow-mcp/src/everyrow_mcp/utils.py:53-100). This is both a readability issue and a minor information leak (reveals local directory structure, username, and that worktrees were used).

Fix this →

2. FINDING-01 PoC overstates exploitability

The PoC claims "~1-5% success rate on fast networks" but provides no empirical evidence for this figure. Given that the TOCTOU window is within a single async coroutine (microseconds), and that Python's DNS resolver caches results via the OS resolver, the real success rate is likely much lower. I'd recommend either removing the percentage claim or qualifying it more carefully (e.g., "probabilistic; rate depends on OS resolver caching behaviour and TTL").

3. FINDING-01 recommended fix has a TLS/SNI gap

The suggested IP-pinning approach (copy_with(host=resolved_ip)) will break TLS — the TLS handshake sends SNI based on the URL host, and pinning to a raw IP means the server won't receive the correct SNI. The fix should also set the extensions on the request to pass through the original hostname for SNI, or use httpcore's connection-level API. Worth noting this caveat so the implementer doesn't introduce a regression.

4. FINDING-03 severity of TEST-NET ranges is overstated

The report groups 192.0.2.0/24, 198.51.100.0/24, and 203.0.113.0/24 (RFC 5737 documentation ranges) with the genuinely risky 100.64.0.0/10 CGNAT range. The TEST-NET ranges are non-routable by definition — no legitimate host should ever respond on them. Including them is fine for completeness but they shouldn't be presented at the same priority as CGNAT. Consider splitting the recommendation: CGNAT as Medium, TEST-NETs as Low/Info.

---

Suggestions (Non-blocking)

5. Consider adding a "Scope Limitations" section

The audit focuses on static analysis. It would strengthen the report to explicitly note what was not tested:

• No runtime DNS rebinding was attempted
• No live container escape testing
• No fuzzing of URL parser edge cases
• No review of the SDK client code (only the MCP server)

6. FINDING-06 mitigating factor deserves more weight

The report correctly notes that Layer 2 (_SSRFSafeTransport) re-validates using httpx's own parsed hostname, which means parser discrepancies in Layer 1 are caught by Layer 2. This effectively makes the finding informational — the defense-in-depth is already working as designed. The "Low" severity might be generous; "Info" would be more appropriate.

7. FINDING-09 call chain could note the HTTPS guarantee

The _decode_trusted_server_jwt analysis correctly identifies the function as safe-by-design, but the call chain diagram doesn't emphasise that _supabase_token_request enforces HTTPS via config.py:143-146 (the _validate_url validator on supabase_url). This is the critical guarantee that makes skipping signature verification acceptable — worth making it more prominent.

8. Attack Surface Matrix is a great addition

The matrix at the end is very useful for quickly assessing coverage. Consider adding a column for "Remediation Status" so it can be used as a tracking artifact going forward.

---

Summary

The report is well-structured, technically accurate, and provides actionable recommendations with clear prioritisation. The main things to fix before merging are the local file path references (should be repo-relative) and the DNS rebinding PoC success rate claim (should be qualified). The recommended fixes are sound but the FINDING-01 IP-pinning suggestion needs a TLS/SNI caveat.

Good work on this audit — it provides a solid foundation for tracking and remediating the identified issues.

---

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 59s —— View job

---

Code Review

• Read CLAUDE.md for repo-specific guidelines
• Read PR diff against origin/main
• Read full source files for context
• Cross-reference Sentry review comments against code
• Post review feedback

---

Overall Assessment

Solid work. The DNS-pinning approach in _SSRFSafeTransport correctly eliminates the TOCTOU gap from FINDING-01, the port allowlist is clean, and the tests cover the important cases. Two bugs to fix before merging.

---

Issues to Address

1. IPv6 Host header is malformed for non-standard ports (Bug)

utils.py:238-240 — When hostname is an IPv6 address (e.g. 2001:db8::1), httpx.URL.host returns it without brackets. The current code produces 2001:db8::1:8080 instead of the RFC 7230-required [2001:db8::1]:8080:

host_header = hostname
if request.url.port and request.url.port not in (80, 443):
    host_header = f"{hostname}:{request.url.port}"

This will cause requests to IPv6 endpoints on non-standard ports to fail. The Sentry comment about missing IPv6 test coverage (test_utils.py:296-398) is correct — adding tests for standard IPv6 addresses would catch this.

Fix:

host_header = f"[{hostname}]" if ":" in hostname else hostname
if request.url.port and request.url.port not in (80, 443):
    host_header = f"{host_header}:{request.url.port}"

Fix this →

2. SNI hostname encoding crashes on Internationalized Domain Names (Bug)

utils.py:252 — hostname.encode("ascii") will raise UnicodeEncodeError for any IDN that hasn't been punycode-normalized:

extensions["sni_hostname"] = hostname.encode("ascii")

In practice httpx normalizes most URLs during parsing, so this is unlikely to trigger for typical usage. However, if a non-ASCII hostname ever reaches this path, it's an unhandled crash. Defensive fix:

extensions["sni_hostname"] = hostname.encode("idna")

The idna codec converts Unicode hostnames to their punycode ASCII form (e.g. 中国.icom.museum → xn--fiqs8s.icom.museum). The Sentry comment on this is valid but the severity is overstated — LOW rather than HIGH given that httpx pre-normalizes URLs.

Fix this →

---

Suggestions (Non-blocking)

3. Blocking DNS in async context

_resolve_and_validate uses socket.getaddrinfo (line 105), which is a synchronous blocking call. It's invoked from handle_async_request (async), so it blocks the event loop during DNS resolution. For a single-user MCP server this is fine, but if concurrency ever increases, consider wrapping it:

loop = asyncio.get_running_loop()
addrinfos = await loop.getaddrinfo(hostname, None, family=socket.AF_UNSPEC, type=socket.SOCK_STREAM)

This would require splitting into an async variant for the transport path. Not urgent — the current approach is safe and correct.

4. IPv6 SNI hostname also needs brackets consideration

When hostname is an IPv6 address, the SNI extension should contain the raw address without brackets (RFC 6066 doesn't use bracket notation for SNI). The current code is correct for this case — just noting it for anyone who considers "fixing" it for consistency with the Host header.

5. host_header also needs IPv6 brackets for default ports

Even when the port is default (80/443), the Host header for an IPv6 address should use brackets: [2001:db8::1] not 2001:db8::1. The current code misses this — the bracket wrapping should happen unconditionally for IPv6, with the port appended only for non-standard ports. The fix in issue 1 above handles this correctly.

---

On the Sentry Reviews

• IDN/encode("ascii") comment: Valid concern but severity is overstated. httpx pre-normalizes URLs, making this edge-case rather than common-path. Agree with the fix (use "idna" codec), disagree with HIGH severity.
• IPv6 test coverage comment: Fully agree. The test suite only covers IPv4-mapped IPv6 (::ffff:...) but not standard IPv6 addresses. Adding tests for [2001:db8::1]:8080 would catch the Host header bug.

---

Good security work overall — the DNS-pinning architecture is correct and the defense-in-depth layering (pre-flight + transport + redirect hook) is well-structured. The two bugs above are straightforward fixes.
|