fix(security): SSRF egress guard on every server-side fetch (PT-03/07/11/17) - #987
fix(security): SSRF egress guard on every server-side fetch (PT-03/07/11/17)#987murdore wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds shared SSRF egress validation with DNS resolution, private-address blocking, host allow-lists, redirect checks, and credential filtering. HTTP, MCP, and webhook integrations now use this protection. A local-development override and focused tests were added. ChangesSSRF egress protection
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_ssrf_egress.py (1)
150-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for header-borne credential stripping.
ssrf_safe_requestalso removes non-safe headers on a credential-dropping hop through_without_credential_headers. The tests cover onlyauth. A regression that keepsAuthorizationinheaderswould pass the current suite._FakeSession.requestalready receives**kwargs, so record and assert the headers per hop.💚 Proposed test addition
async def request(self, method, url, *, auth=None, allow_redirects=True, **kwargs): - self.requests.append({"method": method, "url": url, "auth": auth}) + self.requests.append( + { + "method": method, + "url": url, + "auth": auth, + "headers": kwargs.get("headers"), + } + ) return self._responses.pop(0)async def test_ssrf_safe_request_strips_auth_headers_on_offsite_redirect(): session = _FakeSession( [ _FakeResp(302, {"Location": "https://1.1.1.1/next"}), _FakeResp(200, {}), ] ) async with ssrf_safe_request( cast(aiohttp.ClientSession, session), "GET", "https://8.8.8.8/start", headers={"Authorization": "Bearer secret", "Accept": "application/json"}, allowed_host_suffixes=["8.8.8.8"], ): pass assert "Authorization" in session.requests[0]["headers"] assert "Authorization" not in session.requests[1]["headers"] assert session.requests[1]["headers"]["Accept"] == "application/json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ssrf_egress.py` around lines 150 - 171, Add a separate redirect test alongside test_ssrf_safe_request_strips_auth_on_offsite_redirect that passes Authorization and a safe header through ssrf_safe_request, then records per-hop headers via _FakeSession.request kwargs. Assert Authorization is present on the allowed first hop, removed on the offsite redirect, and the safe Accept header remains unchanged.tests/test_mcp_approval.py (1)
37-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that MCP still calls the egress guard.
The autouse fixture removes SSRF validation for every test in this file. If a future change deletes the
validate_egress_urlcall in_build_server_params, all tests here still pass. Record the validated URLs in the stub and add one test that asserts the guard ran before the server params were built.💚 Proposed change
`@pytest.fixture`(autouse=True) def _bypass_ssrf_egress(monkeypatch): """These tests use placeholder MCP hostnames to exercise approval-map / tool-loading logic. SSRF egress validation (tested separately in tests/test_ssrf_egress.py) would otherwise reject the unresolvable host. """ + validated: list[str] = [] async def _ok(url, *args, **kwargs): + validated.append(url) return ["203.0.113.10"] monkeypatch.setattr(_mcp_module, "validate_egress_url", _ok) + return validatedThen assert
"https://shop.example/api/mcp" in _bypass_ssrf_egressin one loading test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_mcp_approval.py` around lines 37 - 47, Update the _bypass_ssrf_egress fixture to record each URL passed to its _ok stub while retaining the existing resolved-address return value. In a loading test that builds server parameters, assert that "https://shop.example/api/mcp" was recorded, proving _build_server_params still invokes validate_egress_url before constructing the parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.py`:
- Around line 135-146: Update the exception handling in the requester method
around ssrf_safe_request to import and catch SSRFError explicitly, replacing the
now-unreachable aiohttp.TooManyRedirects branch. Return the same
security-rejection response without retrying, while leaving generic exception
retry behavior unchanged.
In `@app/ai/voice/agents/breeze_buddy/utils/common.py`:
- Around line 163-167: Update the signed webhook request in ssrf_safe_request to
pass max_redirects=0, ensuring the fixed tenant-configured endpoint does not
follow redirects or replay the POST payload to another host. Keep the existing
allow_http=True and per-hop SSRF protections unchanged.
In `@app/core/security/ssrf.py`:
- Around line 271-284: Update the redirect loop around the request method and
kwargs so 301, 302, and 303 responses rewrite the request to GET and remove body
arguments such as json and data before the next hop. Preserve the original
method and body only for 307 and 308 redirects, while retaining existing
cross-host credential handling and response release behavior.
---
Nitpick comments:
In `@tests/test_mcp_approval.py`:
- Around line 37-47: Update the _bypass_ssrf_egress fixture to record each URL
passed to its _ok stub while retaining the existing resolved-address return
value. In a loading test that builds server parameters, assert that
"https://shop.example/api/mcp" was recorded, proving _build_server_params still
invokes validate_egress_url before constructing the parameters.
In `@tests/test_ssrf_egress.py`:
- Around line 150-171: Add a separate redirect test alongside
test_ssrf_safe_request_strips_auth_on_offsite_redirect that passes Authorization
and a safe header through ssrf_safe_request, then records per-hop headers via
_FakeSession.request kwargs. Assert Authorization is present on the allowed
first hop, removed on the offsite redirect, and the safe Accept header remains
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5556a37-ffba-4837-880b-2ce3cf490a1b
📒 Files selected for processing (9)
.env.exampleapp/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.pyapp/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.pyapp/ai/voice/agents/breeze_buddy/mcp/__init__.pyapp/ai/voice/agents/breeze_buddy/utils/common.pyapp/core/config/static.pyapp/core/security/ssrf.pytests/test_mcp_approval.pytests/test_ssrf_egress.py
| # ssrf_safe_request re-validates every redirect hop so a | ||
| # public host can't 302 the request to an internal/metadata | ||
| # target (PT-07). | ||
| async with ssrf_safe_request( | ||
| self.session, | ||
| config.method.value, | ||
| url, | ||
| headers=headers, | ||
| json=resolved_body if resolved_body else None, | ||
| timeout=aiohttp.ClientTimeout(total=config.timeout), | ||
| max_redirects=HTTP_REQUEST_MAX_REDIRECTS, | ||
| allow_redirects=HTTP_REQUEST_MAX_REDIRECTS > 0, | ||
| ) as response: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle SSRFError explicitly instead of retrying it.
ssrf_safe_request raises SSRFError when a redirect hop is blocked or the hop limit is reached. That exception is not an aiohttp.ClientError, so it falls into the generic except Exception at line 264. The executor then sleeps and retries up to config.max_retries times. Each retry re-sends the original request, including a POST body, before hitting the same blocked redirect. A security rejection must abort immediately.
The except aiohttp.TooManyRedirects branch at lines 249-259 is now unreachable, because redirects are followed manually with allow_redirects=False. Replace it with an SSRFError branch.
🔒️ Proposed fix
- except aiohttp.TooManyRedirects:
- logger.error(
- f"HTTP {config.method.value} exceeded max redirects "
- f"({HTTP_REQUEST_MAX_REDIRECTS}), not retrying"
- )
- if fire_and_forget:
- return None
- return (
- 0,
- f"Too many redirects (max: {HTTP_REQUEST_MAX_REDIRECTS})",
- )
+ except SSRFError as e:
+ logger.error(
+ f"HTTP {config.method.value} blocked by SSRF egress guard, "
+ f"not retrying: {e}"
+ )
+ if fire_and_forget:
+ return None
+ return (0, f"Blocked by egress policy: {e}")Add SSRFError to the import on line 45:
-from app.core.security.ssrf import ssrf_safe_request, validate_egress_url
+from app.core.security.ssrf import SSRFError, ssrf_safe_request, validate_egress_url📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ssrf_safe_request re-validates every redirect hop so a | |
| # public host can't 302 the request to an internal/metadata | |
| # target (PT-07). | |
| async with ssrf_safe_request( | |
| self.session, | |
| config.method.value, | |
| url, | |
| headers=headers, | |
| json=resolved_body if resolved_body else None, | |
| timeout=aiohttp.ClientTimeout(total=config.timeout), | |
| max_redirects=HTTP_REQUEST_MAX_REDIRECTS, | |
| allow_redirects=HTTP_REQUEST_MAX_REDIRECTS > 0, | |
| ) as response: | |
| from app.core.security.ssrf import SSRFError, ssrf_safe_request, validate_egress_url | |
| ... | |
| except SSRFError as e: | |
| logger.error( | |
| f"HTTP {config.method.value} blocked by SSRF egress guard, " | |
| f"not retrying: {e}" | |
| ) | |
| if fire_and_forget: | |
| return None | |
| return (0, f"Blocked by egress policy: {e}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.py` around
lines 135 - 146, Update the exception handling in the requester method around
ssrf_safe_request to import and catch SSRFError explicitly, replacing the
now-unreachable aiohttp.TooManyRedirects branch. Return the same
security-rejection response without retrying, while leaving generic exception
retry behavior unchanged.
| # allow_redirects=False + per-hop revalidation so a public host can't | ||
| # 30x-redirect the signed payload to an internal target. | ||
| async with ssrf_safe_request( | ||
| session, "POST", url, json=data, headers=headers, allow_http=True | ||
| ) as response: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set max_redirects=0 for the signed webhook.
The comment states allow_redirects=False, but that applies only to the underlying aiohttp call. ssrf_safe_request defaults to max_redirects=3 and follows redirects itself. Each hop re-sends the POST body, so the lead payload can be delivered to a redirect target that the tenant did not configure. The checksum header is dropped on a cross-host hop, but the payload is not. This is the redirect-replay behavior flagged in app/core/security/ssrf.py.
A webhook destination is a fixed, tenant-configured endpoint. It does not need redirect following.
🔒️ Proposed fix
- # allow_redirects=False + per-hop revalidation so a public host can't
- # 30x-redirect the signed payload to an internal target.
+ # max_redirects=0: the signed payload must reach the configured
+ # endpoint only. A 30x response is treated as a failed attempt.
async with ssrf_safe_request(
- session, "POST", url, json=data, headers=headers, allow_http=True
+ session,
+ "POST",
+ url,
+ json=data,
+ headers=headers,
+ allow_http=True,
+ max_redirects=0,
) as response:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # allow_redirects=False + per-hop revalidation so a public host can't | |
| # 30x-redirect the signed payload to an internal target. | |
| async with ssrf_safe_request( | |
| session, "POST", url, json=data, headers=headers, allow_http=True | |
| ) as response: | |
| # max_redirects=0: the signed payload must reach the configured | |
| # endpoint only. A 30x response is treated as a failed attempt. | |
| async with ssrf_safe_request( | |
| session, | |
| "POST", | |
| url, | |
| json=data, | |
| headers=headers, | |
| allow_http=True, | |
| max_redirects=0, | |
| ) as response: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/utils/common.py` around lines 163 - 167,
Update the signed webhook request in ssrf_safe_request to pass max_redirects=0,
ensuring the fixed tenant-configured endpoint does not follow redirects or
replay the POST payload to another host. Keep the existing allow_http=True and
per-hop SSRF protections unchanged.
| response = await session.request( | ||
| method, current, auth=send_auth, allow_redirects=False, **send_kwargs | ||
| ) | ||
| location = response.headers.get("Location") | ||
| if response.status in _REDIRECT_STATUSES and location and hop < max_redirects: | ||
| response.release() | ||
| current = urljoin(current, location) | ||
| continue | ||
|
|
||
| try: | ||
| yield response | ||
| finally: | ||
| response.release() | ||
| return |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not replay the request body and method on 301/302/303 redirects.
The loop repeats method and all kwargs (including json= / data=) on every hop. For 303, and by universal client convention for 301 and 302, a POST must be rewritten to a GET without a body. aiohttp did this before this change, so the behavior of HttpRequestExecutor and send_webhook_with_retry changes: a redirected POST is now re-sent as a POST with the same payload.
Two consequences:
- A non-idempotent write can be delivered twice, once to the original host and once to the redirect target.
- Header credentials are dropped on a cross-host hop, but the request body still travels to the new host. For
send_webhook_with_retrythat body is the lead payload.
Rewrite the method and strip the body on 301, 302, and 303; keep both only for 307 and 308.
🔒️ Proposed fix for redirect method handling
current = url
+ send_method = method
prev_host: Optional[str] = None
for hop in range(max_redirects + 1): response = await session.request(
- method, current, auth=send_auth, allow_redirects=False, **send_kwargs
+ send_method, current, auth=send_auth, allow_redirects=False, **send_kwargs
)
location = response.headers.get("Location")
if response.status in _REDIRECT_STATUSES and location and hop < max_redirects:
response.release()
current = urljoin(current, location)
+ if response.status in (301, 302, 303) and send_method.upper() not in (
+ "GET",
+ "HEAD",
+ ):
+ send_method = "GET"
+ kwargs = {
+ k: v for k, v in kwargs.items() if k not in ("json", "data")
+ }
continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/core/security/ssrf.py` around lines 271 - 284, Update the redirect loop
around the request method and kwargs so 301, 302, and 303 responses rewrite the
request to GET and remove body arguments such as json and data before the next
hop. Preserve the original method and body only for 307 and 308 redirects, while
retaining existing cross-host credential handling and response release behavior.
There was a problem hiding this comment.
Pull request overview
Introduces a shared SSRF egress-guard to validate all server-side, user/template/LLM-influenced outbound HTTP destinations (including per-hop redirect revalidation), and wires it into MCP server discovery, the HTTP global-function executor, and reporting webhooks, with accompanying tests and a local-dev escape hatch.
Changes:
- Added
app/core/security/ssrf.pywithvalidate_egress_url()and anaiohttphelperssrf_safe_request()that revalidates redirects and strips credentials on off-allowlist/cross-host hops. - Applied SSRF validation to MCP server URL construction, HTTP request execution, and webhook sending (including redirect-safe behavior).
- Added SSRF-focused tests and updated existing MCP-approval tests to bypass SSRF for placeholder hostnames.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ssrf_egress.py | Adds unit tests for IP/range blocking, DNS resolution checks, and redirect-hop revalidation/credential stripping. |
| tests/test_mcp_approval.py | Adds an autouse fixture to bypass SSRF validation for placeholder MCP hostnames used in these tests. |
| app/core/security/ssrf.py | New shared SSRF egress validator + redirect-safe aiohttp request helper. |
| app/core/config/static.py | Adds SSRF_ALLOW_PRIVATE_EGRESS local-dev escape hatch. |
| app/ai/voice/agents/breeze_buddy/utils/common.py | Validates reporting webhooks with SSRF guard and uses redirect-safe request helper. |
| app/ai/voice/agents/breeze_buddy/mcp/init.py | Validates resolved MCP server URLs before building auth headers; makes _build_server_params async. |
| app/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.py | Switches HTTP execution to shared SSRF validation + per-hop redirect revalidation. |
| app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py | Blocks LLM-sourced placeholders from occupying URL authority/host position. |
| .env.example | Documents SSRF_ALLOW_PRIVATE_EGRESS configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| validate_egress_url, | ||
| ) | ||
|
|
||
|
|
| - This module deliberately has no dependency on aiohttp/httpx so it can guard | ||
| any client. Callers pair it with ``allow_redirects=False`` (or manual per-hop | ||
| re-validation) and, where credentials are attached, a host allow-list. |
| current = url | ||
| prev_host: Optional[str] = None | ||
| for hop in range(max_redirects + 1): |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 9 changed files (focused on the 5 source files with security-bearing changes).
New issues raised this run
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 4 | http_requester.py, mcp/__init__.py, utils/common.py, ssrf.py |
| 2 | ssrf.py (redirect method/body handling, max-redirects off-by-one) |
|
| 💡 MINOR | 1 | ssrf.py (docstring accuracy) |
Why REQUEST_CHANGES
Four new CRITICAL security issues meet the blocking criteria:
http_requester.py:SSRFErroris not caught explicitly, so a blocked egress URL or redirect is retried up tomax_retriestimes, replaying the request body each time. Security rejections must abort immediately.mcp/__init__.py: The direct-HTTP MCP tool handler (_create_direct_http_tool_handler) bypassesssrf_safe_requestentirely and useshttpx.AsyncClient.post(...)with implicit redirect following. Tenant credentials attached to the request can be 302'd to internal/metadata targets without revalidation or credential stripping.utils/common.py:send_webhook_with_retryusesssrf_safe_requestwith the defaultmax_redirects=3, replaying the signed lead payload on every redirect hop. Webhook endpoints are fixed, tenant-configured destinations and should not follow redirects.ssrf.py:ssrf_safe_requestpassesallow_redirects=Falsedirectly tosession.requestwhile also accepting arbitrary**kwargs. A caller-suppliedallow_redirectskwarg causesTypeError: got multiple values for keyword argument 'allow_redirects'.
Additional MAJOR concerns
- Redirect method/body semantics are incorrect: 301/302/303 should rewrite non-GET/HEAD to GET and drop the body; only 307/308 should preserve both. The current code re-POSTs the payload to redirect targets.
- The redirect loop yields the final 3xx response instead of raising "too many redirects" when the hop budget is exhausted, changing the
max_redirectscontract.
Next steps
Please address the four CRITICAL items and the two MAJOR redirect-handling issues, then re-request review. The existing automated comments from CodeRabbit/Copilot overlap with some of these points; the fixes above should resolve those threads as well.
EvidenceA real screencast — Chrome DevTools Each take runs three acts: attack SSRF egress guardpr987-ssrf-egress.mp4local original: A real internal service listens on loopback and returns a secret. On On this branch every one is refused before a packet leaves, and by the address rule rather than the scheme rule — the probe passes And what must still work: Recorded against
|
074caba to
9694d9a
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 12 changed files (walked the key security-relevant ones).
New issues raised this run
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 1 | utils/common.py (webhook redirect replay) |
| 2 | http_requester.py (SSRFError retry), ssrf.py (redirect method/body handling) |
|
| 💡 MINOR | 4 | ssrf.py docstring, ssrf.py kwargs collision, tests/test_ssrf_egress.py env determinism, http_handler.py subdomain allowance |
| 💬 SUGGESTION | 4 | mcp/__init__.py direct-HTTP revalidation, static.py dynamic config, .env.example restart note, tests/test_mcp_approval.py regression coverage |
Blocking rationale
I am requesting changes because of one new CRITICAL issue and two new MAJOR issues that meet the blocking criteria for SSRF/credential safety:
-
CRITICAL — Signed webhook follows redirects (
utils/common.py):send_webhook_with_retryusesssrf_safe_requestwith the defaultmax_redirects=3. A tenant-configured webhook endpoint does not need redirects, and each redirect hop replays the POST body (lead payload) to a host the tenant did not configure. Thechecksumheader is dropped on cross-host hops, but the payload is not. This is a concrete SSRF/PII-exfiltration primitive. Passmax_redirects=0. -
MAJOR —
SSRFErroris retried (http_requester.py):ssrf_safe_requestraisesSSRFErroron a blocked redirect hop or hop-limit exceeded. That exception is not anaiohttp.ClientError, so it falls into the genericexcept Exceptionand triggers the full retry loop, resending the request body each time. A security rejection must abort immediately. Add an explicitexcept SSRFErrorbranch and remove the now-unreachableexcept aiohttp.TooManyRedirectsbranch. -
MAJOR — Redirect method/body handling is incorrect (
ssrf.py): The manual redirect loop repeats the originalmethodand body kwargs on every hop. For 301/302/303, non-GET/HEAD methods should be rewritten to GET and the body dropped (aiohttp's previous behavior); only 307/308 should preserve method/body. Without this, a POST can be replayed to an attacker-controlled redirect target.
Existing comments acknowledged
I did not duplicate points already raised by CodeRabbit or Copilot. The CRITICAL webhook-redirect issue overlaps with a Copilot comment that asked for max_redirects=0; I am elevating it to CRITICAL because the payload-replay consequence is severe and the existing comment is unresolved. The other new issues are distinct.
Next steps
- Address the three blocking issues above.
- Consider the MINOR/SUGGESTION items at the author's discretion.
- Re-request review once fixes are pushed.
| from app.core.logger import logger | ||
|
|
||
|
|
||
| def _authority_region(url_template: str) -> str: |
There was a problem hiding this comment.
💬 SUGGESTION: _authority_region strips the scheme by splitting on ://, but a URL template with an empty scheme such as //{host}/path will leave //{host} in candidate and then split on / to {host}, which is correct. However, a malicious template like https://{host}.evil.com/path will return {host}.evil.com and the subsequent LLM-field check only looks for exact {field_name} or {arg_name} substrings. That means an LLM-sourced field used as a subdomain prefix (e.g. https://{shop}.example.com/...) is allowed, while the pentest concern is about the host position being fully attacker-controlled. Consider documenting this intentional subdomain allowance explicitly, or tightening the check if subdomain takeover via LLM values is in scope for PT-17.
| url=url, | ||
| # ssrf_safe_request re-validates every redirect hop so a | ||
| # public host can't 302 the request to an internal/metadata | ||
| # target (PT-07). |
There was a problem hiding this comment.
ssrf_safe_request is correct for per-hop SSRF validation, but the surrounding exception handling is now inconsistent. ssrf_safe_request passes allow_redirects=False and follows redirects manually, so the except aiohttp.TooManyRedirects branch further down is unreachable. Worse, SSRFError is not an aiohttp.ClientError, so a blocked redirect hop falls into the generic except Exception and triggers the full retry loop, resending the request body each time.
Import SSRFError alongside the other helpers and add an explicit handler that aborts immediately:
from app.core.security.ssrf import SSRFError, ssrf_safe_request, validate_egress_url except SSRFError as e:
logger.error(
f"HTTP {config.method.value} blocked by SSRF egress guard, "
f"not retrying: {e}"
)
if fire_and_forget:
return None
return (0, f"Blocked by egress policy: {e}")Then remove the now-dead except aiohttp.TooManyRedirects block.
| validate_egress_url, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
💡 MINOR: test_ip_block_reason_flags_metadata_and_private asserts that private/loopback addresses are blocked, but the module-level _ALLOW_PRIVATE_EGRESS is initialized from SSRF_ALLOW_PRIVATE_EGRESS at import time. If a developer has SSRF_ALLOW_PRIVATE_EGRESS=true in their environment, these assertions will fail. Force the flag off via an autouse fixture so the tests are deterministic regardless of environment.
@pytest.fixture(autouse=True)
def _force_private_egress_disabled(monkeypatch):
monkeypatch.setattr(ssrf, "_ALLOW_PRIVATE_EGRESS", False)| CREDENTIAL_ENCRYPTION_KEY = os.getenv("CREDENTIAL_ENCRYPTION_KEY", "") | ||
|
|
||
| # JWT Authentication Configuration | ||
| # Local-dev escape hatch for the SSRF egress guard (app/core/security/ssrf.py): |
There was a problem hiding this comment.
💬 SUGGESTION: SSRF_ALLOW_PRIVATE_EGRESS is a welcome addition and defaults securely. Consider adding it to the runtime dynamic config layer (app/core/config/dynamic.py) as well, so operators can toggle it without a pod restart in staging/debug scenarios. Not blocking.
|
|
||
| BREEZE_MCP_ENDPOINT_PATH="/ai/neurolink" | ||
| TWILIO_ACCOUNT_SID="" | ||
|
|
There was a problem hiding this comment.
💬 SUGGESTION: Good documentation of the local-dev escape hatch. Consider adding a note that SSRF_ALLOW_PRIVATE_EGRESS is read once at import and requires a process restart to take effect, so developers don't expect runtime toggling via env reload.
| ) | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) |
There was a problem hiding this comment.
💬 SUGGESTION: The _bypass_ssrf_egress fixture is pragmatic for unit tests that aren't testing egress validation. Consider adding a dedicated test that exercises the real validate_egress_url rejection path for an MCP server URL (e.g. https://127.0.0.1/...) so the integration between _build_server_params and the SSRF guard has regression coverage, since the bypass fixture currently masks that behavior across this file.
9694d9a to
2eb0927
Compare
All six addressed — thanks, these were rightEvery one of the four CRITICAL and two MAJOR items is fixed in CRITICAL 1 —
|
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Reviewed 13 changed files. No new blocking issues found beyond the points already raised by CodeRabbit, Copilot, and Tara-ag.
Summary of existing threads (already handled; not duplicated):
app/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.py: SSRFError handling and removal of the now-unreachableaiohttp.TooManyRedirectsbranch.app/ai/voice/agents/breeze_buddy/utils/common.py:max_redirects=0for the signed reporting webhook.app/core/security/ssrf.py: redirect method/body semantics (301/302/303 → GET without body; 307/308 preserve), and guarding against caller-suppliedallow_redirectscollisions.tests/test_ssrf_egress.py: deterministic_ALLOW_PRIVATE_EGRESSpinning via autouse fixture..env.example/app/core/config/static.py: documentation of the import-timeSSRF_ALLOW_PRIVATE_EGRESSescape hatch.tests/test_mcp_approval.py: suggestion for a dedicated MCP + realvalidate_egress_urlrejection test.
New comment added this review:
app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py:44— SUGGESTION to document or tighten the intentional allowance of LLM-sourced placeholders used as subdomain prefixes in the URL host position (PT-17).
No hardcoded secrets, no SQL injection, no auth bypass, no migration edits, and no unresolved cross-tenant concerns were introduced. The SSRF guard is centralized, defaults securely, and is backed by focused tests. Approving.
| from app.core.logger import logger | ||
|
|
||
|
|
||
| def _authority_region(url_template: str) -> str: |
There was a problem hiding this comment.
💬 SUGGESTION: _authority_region strips the scheme by splitting on ://, but a URL template with an empty scheme such as //{host}/path will leave //{host} in candidate and then split on / to {host}, which is correct. However, a malicious template like https://{host}.evil.com/path will return {host}.evil.com and the subsequent LLM-field check only looks for exact {field_name} or {arg_name} substrings. That means an LLM-sourced field used as a subdomain prefix (e.g. https://{shop}.example.com/...) is allowed, while the pentest concern is about the host position being fully attacker-controlled. Consider documenting this intentional subdomain allowance explicitly, or tightening the check if subdomain takeover via LLM values is in scope for PT-17.
2eb0927 to
226c458
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 13 changed files (walked the key security-relevant ones).
New issues raised this run
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 0 | — |
| 1 | http_requester.py (SSRFError handler fragility) |
|
| 💡 MINOR | 1 | http_handler.py (missing URL guard) |
| 💬 SUGGESTION | 5 | http_handler.py, mcp/__init__.py, exotel/recording.py, static.py, .env.example |
Blocking rationale
No new blocking issues were found this run. The four CRITICAL items from my earlier reviews have been addressed in the current HEAD:
- ✅
http_requester.py: explicitexcept SSRFErrorbranch added; security rejections abort immediately instead of retrying. - ✅
mcp/__init__.py: direct-HTTP MCP handler re-validates egress at call time before assembling tenant credentials. - ✅
utils/common.py: signed reporting webhook now passesmax_redirects=0, refusing redirects outright. - ✅
ssrf.py: caller-suppliedallow_redirectsis popped/ignored; 301/302/303 rewrite to GET and drop body; 307/308 preserve method/body; exhausted hop budget raises.
Existing comments acknowledged
I did not duplicate points already raised by CodeRabbit, Copilot, or my earlier reviews. The remaining threads are either suggestions/minors or the one new MAJOR item below.
New issues this run
- MAJOR —
http_requester.py:246: theSSRFErrorhandler is present and ordered correctly before the genericexcept Exception, but becauseSSRFErrorsubclassesValueError, any future refactor that places a broadexcept Exception(orexcept ValueError) earlier in the chain would silently swallow security rejections and retry the blocked request. Add a regression test or a maintainer comment warning against reordering. - MINOR —
http_handler.py:125: ifconfig.http_request.urlisNone,_authority_regionreceives""and the LLM-host check silently skips. Raise a clear template error instead of falling through toFieldResolver. - SUGGESTION —
http_handler.py:44: document or tighten the intentional allowance of LLM-sourced placeholders used as subdomain prefixes in the URL host position (PT-17). - SUGGESTION —
mcp/__init__.py:253: consider applying the same call-time egress guard to the discovery-path_create_mcp_tool_handler(pooled and unpooled), sinceMCPClientalso posts toserver_params.urlwith tenant credentials. - SUGGESTION —
exotel/recording.py:44: document how the Exotel allow-list suffix was chosen, in case regional/CDN domains emerge. - SUGGESTION —
static.py:339/.env.example:142: consider noting thatSSRF_ALLOW_PRIVATE_EGRESSis read once at import and requires a process restart.
Decision
Approving. No hardcoded secrets, no SQL injection, no auth bypass, no migration edits, and no unresolved cross-tenant concerns were introduced. The SSRF guard is centralized, defaults securely, and is backed by focused tests. Please consider the MAJOR and MINOR items before merge.
There was a problem hiding this comment.
💬 SUGGESTION: Consider adding a note that SSRF_ALLOW_PRIVATE_EGRESS is read once at import time and requires a process restart to take effect, so developers don't expect runtime toggling via env reload.
| from app.core.logger import logger | ||
|
|
||
|
|
||
| def _authority_region(url_template: str) -> str: |
There was a problem hiding this comment.
💬 SUGGESTION: _authority_region strips the scheme by splitting on ://, but a URL template with an empty scheme such as //{host}/path will leave //{host} in candidate and then split on / to {host}, which is correct. However, a malicious template like https://{host}.evil.com/path will return {host}.evil.com and the subsequent LLM-field check only looks for exact {field_name} or {arg_name} substrings. That means an LLM-sourced field used as a subdomain prefix (e.g. https://{shop}.example.com/...) is allowed, while the pentest concern is about the host position being fully attacker-controlled. Consider documenting this intentional subdomain allowance explicitly, or tightening the check if subdomain takeover via LLM values is in scope for PT-17.
| @@ -104,6 +125,30 @@ async def http_function_handler( | |||
| "error": f"Missing required arguments: {', '.join(missing_args)}", | |||
There was a problem hiding this comment.
💡 MINOR: The LLM-host check runs after missing_args is computed but before FieldResolver substitutes values. This is the right ordering for the host-position guard. However, if config.http_request.url is absent (None), _authority_region receives "" and the loop silently skips it. Consider raising a clear template error when the URL itself is missing, so a malformed global function fails early rather than falling through to FieldResolver.
| @@ -243,17 +246,22 @@ async def execute( | |||
| logger.warning( | |||
There was a problem hiding this comment.
except Exception branch at the end of this retry loop does not accidentally catch SSRFError on future refactor. Because SSRFError subclasses ValueError, any broad except Exception placed earlier in the chain would swallow it. Currently it is placed before the generic handler, so it works, but add a regression test or a code comment warning maintainers not to reorder these handlers.
| @@ -252,6 +253,24 @@ def _create_direct_http_tool_handler( | |||
| """ | |||
There was a problem hiding this comment.
💬 SUGGESTION: The call-time re-validation in the direct-HTTP MCP handler is good for DNS rebinding. Consider also applying the same guard to the discovery-path _create_mcp_tool_handler (both pooled and unpooled branches), because MCPClient also posts to server_params.url with tenant credentials attached. While follow_redirects defaults to False in httpx, the initial egress still resolves the host and could hit a rebound internal address.
| from app.core.logger import logger | ||
|
|
||
|
|
||
| def _authority_region(url_template: str) -> str: |
There was a problem hiding this comment.
💬 SUGGESTION: _authority_region strips the scheme by splitting on ://, but a URL template with an empty scheme such as //{host}/path will leave //{host} in candidate and then split on / to {host}, which is correct. However, a malicious template like https://{host}.evil.com/path will return {host}.evil.com and the subsequent LLM-field check only looks for exact {field_name} or {arg_name} substrings. That means an LLM-sourced field used as a subdomain prefix (e.g. https://{shop}.example.com/...) is allowed, while the pentest concern is about the host position being fully attacker-controlled. Consider documenting this intentional subdomain allowance explicitly, or tightening the check if subdomain takeover via LLM values is in scope for PT-17.
| @@ -104,6 +125,30 @@ async def http_function_handler( | |||
| "error": f"Missing required arguments: {', '.join(missing_args)}", | |||
There was a problem hiding this comment.
💡 MINOR: The LLM-host check runs after missing_args is computed but before FieldResolver substitutes values. This is the right ordering for the host-position guard. However, if config.http_request.url is absent (None), _authority_region receives "" and the loop silently skips it. Consider raising a clear template error when the URL itself is missing, so a malformed global function fails early rather than falling through to FieldResolver.
| @@ -243,17 +246,22 @@ async def execute( | |||
| logger.warning( | |||
There was a problem hiding this comment.
except Exception branch at the end of this retry loop does not accidentally catch SSRFError on future refactor. Because SSRFError subclasses ValueError, any broad except Exception placed earlier in the chain would swallow it. Currently it is placed before the generic handler, so it works, but add a regression test or a code comment warning maintainers not to reorder these handlers.
| @@ -252,6 +253,24 @@ def _create_direct_http_tool_handler( | |||
| """ | |||
There was a problem hiding this comment.
💬 SUGGESTION: The call-time re-validation in the direct-HTTP MCP handler is good for DNS rebinding. Consider also applying the same guard to the discovery-path _create_mcp_tool_handler (both pooled and unpooled branches), because MCPClient also posts to server_params.url with tenant credentials attached. While follow_redirects defaults to False in httpx, the initial egress still resolves the host and could hit a rebound internal address.
| @@ -34,9 +44,16 @@ async def download_call_recording( | |||
|
|
|||
There was a problem hiding this comment.
💬 SUGGESTION: _EXOTEL_HOST_SUFFIXES = ("exotel.com",) is a good allow-list gate. Consider whether Exotel recording URLs can ever be served from a regional subdomain or CDN not ending in exotel.com (e.g. exotel.io, AWS CloudFront, or a partner domain). If so, the allow-list may need expansion; if not, document how the suffix was chosen so future maintainers don't broaden it without verification.
226c458 to
76a6a4f
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 14 changed files (walked the security-relevant source files and tests).
New issues raised this run
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 0 | — |
| 1 | http_requester.py |
|
| 💡 MINOR | 1 | http_handler.py |
| 💬 SUGGESTION | 5 | .env.example, http_handler.py, mcp/__init__.py, exotel/recording.py, managers/pre_checks.py |
Blocking rationale
No new blocking issues were found this run. The four CRITICAL items from earlier reviews have been addressed in the current HEAD:
- ✅
http_requester.py: explicitexcept SSRFErrorbranch added; security rejections abort immediately instead of retrying. - ✅
mcp/__init__.py: direct-HTTP MCP handler re-validates egress at call time before assembling tenant credentials. - ✅
utils/common.py: signed reporting webhook now passesmax_redirects=0, refusing redirects outright. - ✅
ssrf.py: caller-suppliedallow_redirectsis popped/ignored; 301/302/303 rewrite to GET and drop body; 307/308 preserve method/body; exhausted hop budget raises.
Existing comments acknowledged
I did not duplicate points already raised by CodeRabbit, Copilot, or earlier reviews. The remaining threads are either suggestions/minors or the one new MAJOR item below.
New issues this run
- MAJOR —
http_requester.py:246: theSSRFErrorhandler is present and ordered correctly before the genericexcept Exception, but becauseSSRFErrorsubclassesValueError, any future refactor that places a broadexcept Exception(orexcept ValueError) earlier in the chain would silently swallow security rejections and retry the blocked request. Add a regression test or a maintainer comment warning against reordering. - MINOR —
http_handler.py:125: ifconfig.http_request.urlisNone,_authority_regionreceives""and the LLM-host check silently skips. Raise a clear template error instead of falling through toFieldResolver. - SUGGESTION —
http_handler.py:44: document or tighten the intentional allowance of LLM-sourced placeholders used as subdomain prefixes in the URL host position (PT-17). - SUGGESTION —
mcp/__init__.py:253: consider applying the same call-time egress guard to the discovery-path_create_mcp_tool_handler(pooled and unpooled), sinceMCPClientalso posts toserver_params.urlwith tenant credentials. - SUGGESTION —
exotel/recording.py:44: document how the Exotel allow-list suffix was chosen, in case regional/CDN domains emerge. - SUGGESTION —
.env.example:142: note thatSSRF_ALLOW_PRIVATE_EGRESSis read once at import and requires a process restart. - SUGGESTION —
managers/pre_checks.py:127: add a regression test asserting thatvalidate_egress_urlruns before_build_auth_headersin_build_server_params.
Decision
Approving. No hardcoded secrets, no SQL injection, no auth bypass, no migration edits, and no unresolved cross-tenant concerns were introduced. The SSRF guard is centralized, defaults securely, and is backed by focused tests. Please consider the MAJOR and MINOR items before merge.
| # enable it (per-deployment) when template authorship is fully trusted. | ||
| ENABLE_CUSTOM_PYTHON_FUNCTIONS=false | ||
| TWILIO_ACCOUNT_SID="" | ||
|
|
There was a problem hiding this comment.
💬 SUGGESTION: The comment correctly frames SSRF_ALLOW_PRIVATE_EGRESS as local-dev only, but it doesn't mention that the value is read once at import time and requires a process restart to take effect. Adding that note will prevent operators from expecting runtime toggling via env reload.
| from app.core.logger import logger | ||
|
|
||
|
|
||
| def _authority_region(url_template: str) -> str: |
There was a problem hiding this comment.
💬 SUGGESTION: _authority_region strips the scheme by splitting on ://, but a URL template with an empty scheme such as //{host}/path will leave //{host} in candidate and then split on / to {host}, which is correct. However, a malicious template like https://{host}.evil.com/path will return {host}.evil.com and the subsequent LLM-field check only looks for exact {field_name} or {arg_name} substrings. That means an LLM-sourced field used as a subdomain prefix (e.g. https://{shop}.example.com/...) is allowed, while the pentest concern is about the host position being fully attacker-controlled. Consider documenting this intentional subdomain allowance explicitly, or tightening the check if subdomain takeover via LLM values is in scope for PT-17.
| @@ -104,6 +125,30 @@ async def http_function_handler( | |||
| "error": f"Missing required arguments: {', '.join(missing_args)}", | |||
There was a problem hiding this comment.
💡 MINOR: The LLM-host check runs after missing_args is computed but before FieldResolver substitutes values. This is the right ordering for the host-position guard. However, if config.http_request.url is absent (None), _authority_region receives "" and the loop silently skips it. Consider raising a clear template error when the URL itself is missing, so a malformed global function fails early rather than falling through to FieldResolver.
| @@ -243,17 +246,22 @@ async def execute( | |||
| logger.warning( | |||
There was a problem hiding this comment.
except Exception branch at the end of this retry loop does not accidentally catch SSRFError on future refactor. Because SSRFError subclasses ValueError, any broad except Exception placed earlier in the chain would swallow it. Currently it is placed before the generic handler, so it works, but add a regression test or a code comment warning maintainers not to reorder these handlers.
| @@ -252,6 +253,24 @@ def _create_direct_http_tool_handler( | |||
| """ | |||
There was a problem hiding this comment.
💬 SUGGESTION: The call-time re-validation in the direct-HTTP MCP handler is good for DNS rebinding. Consider also applying the same guard to the discovery-path _create_mcp_tool_handler (both pooled and unpooled branches), because MCPClient also posts to server_params.url with tenant credentials attached. While follow_redirects defaults to False in httpx, the initial egress still resolves the host and could hit a rebound internal address.
| @@ -34,9 +44,16 @@ async def download_call_recording( | |||
|
|
|||
There was a problem hiding this comment.
💬 SUGGESTION: _EXOTEL_HOST_SUFFIXES now includes the configured subdomain host plus exotel.com, which is a good allow-list gate. Consider whether Exotel recording URLs can ever be served from a regional subdomain or CDN not ending in exotel.com (e.g. exotel.io, AWS CloudFront, or a partner domain). If so, the allow-list may need expansion; if not, document how the suffix was chosen so future maintainers don't broaden it without verification.
| @@ -127,9 +127,14 @@ async def _fetch_mcp_response( | |||
| if not isinstance(arguments, dict): | |||
There was a problem hiding this comment.
💬 SUGGESTION: The comment explains that _build_server_params runs the SSRF egress guard before attaching credentials, which is correct. Consider adding a regression test that asserts validate_egress_url is called with the resolved URL before _build_auth_headers runs, so a future refactor cannot accidentally swap the order and attach credentials to a rejected host.
…/11/12/17) Independent of the other pentest PRs — targets release, merges in any order. Four code paths dereference a URL that a template author, a lead payload or the LLM can influence, and none checked where it pointed: MCP server URLs (hit at flow-build time with decrypted tenant credentials attached), HttpRequestExecutor, the reporting-webhook sender, and the three telephony recording downloads. Adds one shared guard (app/core/security/ssrf.py) rather than four checks: resolve the host, deny private/loopback/link-local/metadata, require https by default, and revalidate on every redirect hop so a public host cannot 30x the request onto an internal target. Credentials are stripped when a redirect leaves the allow-list. PT-12 lives here rather than with the telephony signature work because the recording downloads are an egress concern: the fix is ssrf_safe_request around a credentialed fetch, so it shares all its code with this guard and none with callback authentication. Two deliberate choices: - validate_egress_url runs BEFORE _build_auth_headers in the MCP path, so a hostile server URL never gets the tenant's credentials attached even briefly. - the webhook sender passes allow_http=True. PT-11 is about the resolved address, not the scheme, and this path never restricted the scheme before; https-only here would silently drop outcome webhooks for every tenant still on http. Plaintext delivery is logged instead. _build_server_params becomes async because resolution is a DNS round-trip. Tests: 11 reserved ranges, scheme and hostname cases, per-hop redirect revalidation, credential stripping. 942 pass on this branch. Review round 2 — addresses the four CRITICAL and two MAJOR findings: - http_requester: catch SSRFError explicitly and abort. It is not an aiohttp.ClientError, so it fell into the generic handler and the request body was replayed at a blocked target up to max_retries times. This also replaces the now-unreachable aiohttp.TooManyRedirects branch, since ssrf_safe_request follows redirects itself. - mcp: the direct-HTTP tool handler re-validates at CALL time. The URL was only checked at flow-build time, but this handler runs per invocation and attaches tenant credentials — a name that resolved public at build and internal at call (rebinding) went straight through. Validation runs before the credential headers are assembled, and follow_redirects=False is now explicit. - common: the signed reporting webhook passes max_redirects=0. A tenant-configured endpoint has no reason to redirect, and each hop replayed the lead payload to a host the tenant never configured. SSRFError also aborts instead of retrying. - ssrf: allow_redirects supplied by a caller is popped rather than colliding with the explicit argument (was TypeError); 301/302/303 rewrite non-GET to GET and drop the body, with only 307/308 preserving them; exhausting max_redirects raises instead of yielding the final 3xx as if it were the answer. - docstring corrected — the module does depend on aiohttp for ssrf_safe_request; only the validation half is client-agnostic. - tests: 6 new cases covering redirect method/body semantics, the hop-budget contract, the kwargs collision, max_redirects=0, and the MCP call-time guard; plus an autouse fixture pinning SSRF_ALLOW_PRIVATE_EGRESS off so a developer with it enabled locally does not silently invert every assertion. 948 tests pass on this branch.
76a6a4f to
6baef03
Compare
Independent of the other pentest PRs — targets release, merges in any order.
Four code paths dereference a URL that a template author, a lead payload or the
LLM can influence, and none checked where it pointed: MCP server URLs (hit at
flow-build time with decrypted tenant credentials attached), HttpRequestExecutor,
the reporting-webhook sender, and the three telephony recording downloads.
Adds one shared guard (app/core/security/ssrf.py) rather than four checks:
resolve the host, deny private/loopback/link-local/metadata, require https by
default, and revalidate on every redirect hop so a public host cannot 30x the
request onto an internal target. Credentials are stripped when a redirect leaves
the allow-list.
PT-12 lives here rather than with the telephony signature work because the
recording downloads are an egress concern: the fix is ssrf_safe_request around a
credentialed fetch, so it shares all its code with this guard and none with
callback authentication.
Two deliberate choices:
hostile server URL never gets the tenant's credentials attached even briefly.
address, not the scheme, and this path never restricted the scheme before;
https-only here would silently drop outcome webhooks for every tenant still on
http. Plaintext delivery is logged instead.
_build_server_params becomes async because resolution is a DNS round-trip.
Tests: 11 reserved ranges, scheme and hostname cases, per-hop redirect
revalidation, credential stripping. 942 pass on this branch.
Independent by construction
This PR targets
releasedirectly. It shares no file with any other pentest PR, and all five were trial-merged pairwise — 10 of 10 pairs merge with no conflict, so they can land in any order.The four config blocks that previously forced a chain (
static.py,.env.example) now sit at four distinct anchors hundreds of lines apart, each next to the settings it belongs with, so independent branches auto-merge instead of colliding.Merging all five reproduces the original #930 tree —
static.pyis identical content in a different order,.env.examplediffers only by one now-meaningless banner comment — and the combined suite runs 992 passed.Evidence recording is in the comment below.