fix(mcp-store): connect upstream MCP fetches to the validated IP - #97614
posthog[bot] wants to merge 1 commit into
Conversation
The MCP Store validated a team-supplied server URL, resolving the hostname once, and then let httpx resolve the same hostname again when it opened the connection. A record that answers the two lookups differently passed the SSRF check and still reached an internal address, and the proxy returns the upstream body to the caller. Add pinned_httpx_client, the httpx counterpart of the existing pinned requests helper. It wraps the transports httpx builds for itself, so an egress proxy route stays in place and only the address behind the hostname is fixed. TLS still verifies the original hostname through the sni_hostname extension. The proxy and the tool fetch and call paths now validate through upstream_http, which hands the validated addresses to the client, so a fetch cannot skip the policy or reach a different address than the one it passed. Generated-By: PostHog Desktop Task-Id: d5881707-ad37-42cd-8c5d-d6b6b8f0bd89
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 2 should fix, 0 consider. Published 4 findings (view the review). |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/mcp_store/backend/test/test_proxy.py |
11 | 85 |
posthog/security/pinned_httpx.py |
10 | 74 |
posthog/security/test/test_pinned_httpx.py |
4 | 56 |
products/mcp_store/backend/test/test_api.py |
2 | 11 |
products/mcp_store/backend/test/test_tools.py |
2 | 36 |
posthog/security/pinned_requests.py |
1 | 6 |
This check does not block merging. It updates on every push and clears when the share drops.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
This is a security-sensitive SSRF/DNS-rebinding fix in the MCP upstream connection path, and it has zero reviews or approvals from any agent reviewer or human — the only review activity shown is an automated review that hasn't finished yet. The author is a machine user not on the owning team, so there's no independent assurance covering this risky-territory change.
- No completed independent review (human or agent reviewer) on the current head to cover this security-sensitive change
- Author (posthog[bot]) is not on the owning team and, being a machine author, carries no familiarity signal
- A duplicate open community PR (#95848) addresses the same vulnerability — reviewers should confirm which lands to avoid conflicting fixes
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 206L, 6F substantive, 558L/11F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (558L, 11F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 63af84c · reviewed head 63af84c |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| Call ``validate_upstream_url`` first and hand its verdict to | ||
| ``upstream_mcp_client``: the client cannot be built without one, so a fetch | ||
| cannot skip the policy. |
There was a problem hiding this comment.
Active async MCP calls still permit DNS rebinding
Why we think it's a valid issue
- Checked: Every caller of
check_mcp_url_policyoutside tests, theMCPClienttransport code, the tool's wiring in the chat agent, and the new helper's type surface. - Found:
ee/hogai/tools/call_mcp_server/tool.py:307validates the team-supplied URL, thentool.py:220buildsMCPClient(server_url, ...).mcp_client.py:48opens a plainhttpx.AsyncClientfor the streamable HTTP transport, andmcp_client.py:65callssse_client(...)for the fallback. Neither client carries a pin, so each connect resolves the hostname a second time. - Found:
sse_clientkeeps its defaulthttpx_client_factory=create_mcp_http_client(mcp/client/sse.py:36), so the fallback builds a third unpinned client that the product cannot configure. - Found: Both transports send the stored credential.
_build_server_headersputsAuthorization: Bearer <api_key or access_token>on every installation URL (ee/hogai/tools/call_mcp_server/installations.py:126-131), andtool.py:220passes those headers into the client. - Found: The path is live, not dead code.
ee/hogai/chat_agent/toolkit.py:140-149adds the tool whenever themcp-gatewayflag resolves true and the team has one installation. - Found: The new helper is synchronous only.
pinned_httpx_clientreturnshttpx.Client(posthog/security/pinned_httpx.py:68), andPinnedIPTransportextendshttpx.BaseTransport, so no async caller can reuse it today. - Found: This PR edits the guard the async path calls.
products/mcp_store/backend/url_policy.py:28-30now tells a fetch path to useupstream_http.validate_upstream_urlfor the pinned addresses.tool.py:307is the one remaining caller that fetches after the check, and it does not follow that guidance. - Impact: A team member who installs a server on a hostname with a short-lived record keeps the same rebinding window the PR closes elsewhere. The check accepts a public address, the connect reaches a loopback, cluster, or metadata address, the stored bearer token goes with it, and the tool output returns to the chat. The fix therefore closes the class for the MCP Store product module but leaves the agent path open.
- Priority: Lowered to
should_fix. The defect exists on master and sits outside the changed files, so it does not block this PR. A repair also needs new infrastructure: anhttpx.AsyncBaseTransportwrapper plus anhttpx_client_factoryfor the SSE fallback. That belongs in a linked follow-up rather than in this diff.
Issue description
The new helper protects only the synchronous proxy and backend tool callers. CallMCPServerTool remains active behind the MCP gateway flag. It validates with check_mcp_url_policy, then MCPClient opens a new httpx.AsyncClient. Its SSE fallback opens another client. Both clients resolve the team-controlled hostname again and send stored bearer credentials. A rebinding record can still connect this path to a private or metadata address.
Suggested fix
Route CallMCPServerTool through the hardened product facade, or add an async pinned client based on validate_upstream_url. Apply the pin to both streamable HTTP and SSE transports. Add a rebinding test for each connection attempt.
Prompt to fix with AI (copy-paste)
## Context
@products/mcp_store/backend/upstream_http.py#L9-11
<issue_description>
The new helper protects only the synchronous proxy and backend tool callers. `CallMCPServerTool` remains active behind the MCP gateway flag. It validates with `check_mcp_url_policy`, then `MCPClient` opens a new `httpx.AsyncClient`. Its SSE fallback opens another client. Both clients resolve the team-controlled hostname again and send stored bearer credentials. A rebinding record can still connect this path to a private or metadata address.
</issue_description>
<issue_validation>
- **Checked:** Every caller of `check_mcp_url_policy` outside tests, the `MCPClient` transport code, the tool's wiring in the chat agent, and the new helper's type surface.
- **Found:** `ee/hogai/tools/call_mcp_server/tool.py:307` validates the team-supplied URL, then `tool.py:220` builds `MCPClient(server_url, ...)`. `mcp_client.py:48` opens a plain `httpx.AsyncClient` for the streamable HTTP transport, and `mcp_client.py:65` calls `sse_client(...)` for the fallback. Neither client carries a pin, so each connect resolves the hostname a second time.
- **Found:** `sse_client` keeps its default `httpx_client_factory=create_mcp_http_client` (`mcp/client/sse.py:36`), so the fallback builds a third unpinned client that the product cannot configure.
- **Found:** Both transports send the stored credential. `_build_server_headers` puts `Authorization: Bearer <api_key or access_token>` on every installation URL (`ee/hogai/tools/call_mcp_server/installations.py:126-131`), and `tool.py:220` passes those headers into the client.
- **Found:** The path is live, not dead code. `ee/hogai/chat_agent/toolkit.py:140-149` adds the tool whenever the `mcp-gateway` flag resolves true and the team has one installation.
- **Found:** The new helper is synchronous only. `pinned_httpx_client` returns `httpx.Client` (`posthog/security/pinned_httpx.py:68`), and `PinnedIPTransport` extends `httpx.BaseTransport`, so no async caller can reuse it today.
- **Found:** This PR edits the guard the async path calls. `products/mcp_store/backend/url_policy.py:28-30` now tells a fetch path to use `upstream_http.validate_upstream_url` for the pinned addresses. `tool.py:307` is the one remaining caller that fetches after the check, and it does not follow that guidance.
- **Impact:** A team member who installs a server on a hostname with a short-lived record keeps the same rebinding window the PR closes elsewhere. The check accepts a public address, the connect reaches a loopback, cluster, or metadata address, the stored bearer token goes with it, and the tool output returns to the chat. The fix therefore closes the class for the MCP Store product module but leaves the agent path open.
- **Priority:** Lowered to `should_fix`. The defect exists on master and sits outside the changed files, so it does not block this PR. A repair also needs new infrastructure: an `httpx.AsyncBaseTransport` wrapper plus an `httpx_client_factory` for the SSE fallback. That belongs in a linked follow-up rather than in this diff.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Route `CallMCPServerTool` through the hardened product facade, or add an async pinned client based on `validate_upstream_url`. Apply the pin to both streamable HTTP and SSE transports. Add a rebinding test for each connection attempt.
</potential_solution>
| # extension carries the original hostname into TLS, for SNI and for certificate | ||
| # verification, which would otherwise run against the IP. | ||
| request.extensions = {**request.extensions, "sni_hostname": host} | ||
| request.url = request.url.copy_with(host=ip) | ||
| return self._inner.handle_request(request) |
There was a problem hiding this comment.
Pinned HTTPS requests fail through the egress proxy
Why we think it's a valid issue
- Checked: httpcore's two connection classes, the locked versions, and the end-to-end behavior of the new client through a real CONNECT proxy. I also ran the same scenario against the pre-existing
requestshelper for comparison, and read the one proxy test the PR adds. - Found:
httpcore/_sync/http_proxy.py:312builds the tunneled TLS with"server_hostname": self._remote_origin.host.decode("ascii")and never readsrequest.extensions["sni_hostname"]. The direct path does read it (httpcore/_sync/connection.py:107and:151).uv.lock:3217-3218pins httpcore 1.0.9, and the venv carries httpx 0.28.1, so the installed code is the code above. BecausePinnedIPTransportrewritesrequest.urlatposthog/security/pinned_httpx.py:49,_remote_origin.hostis the pinned IP. - Found: Reproduced end to end. With
HTTPS_PROXYset to a local CONNECT proxy that serves a certificate formcp.example.test, a client frompinned_httpx_client("https://mcp.example.test/mcp", {127.0.0.1})fails:ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: IP address mismatch, certificate is not valid for '127.0.0.1'. The proxy loggedCONNECT 127.0.0.1:443, so the pin does hold, and only the certificate check breaks. - Found: The break is specific to the tunnel. The same pinned client on the direct route returns 200 against the same hostname certificate. The pre-existing
PinnedIPAdapteralso returns 200 through the same fake proxy, becausecert_verifyinjectsserver_hostnameandassert_hostnameinto the urllib3 pool (posthog/security/pinned_requests.py:141-155). The new httpx helper therefore regresses against its own sibling on this route. - Found: The route is on by default for the new callers.
trust_environment_proxyreturnsTruefor every URL that is not operator-allowlisted (products/mcp_store/backend/url_policy.py:41-44), andupstream_mcp_clientpasses that astrust_env(products/mcp_store/backend/upstream_http.py:50), so httpx mounts the proxy transport whenever the process hasHTTPS_PROXY. - Found: An egress proxy is the expected production shape for external calls in this repo, not a rare setup.
posthog/security/outbound_proxy.py:1-18states external calls honorHTTP_PROXY/HTTPS_PROXYand internal calls must opt out,.semgrep/rules/security/aiohttp-missing-trust-env.yamlenforcestrust_env=Trueon external aiohttp calls, andposthog/metrics.py:8plusbin/granian_metrics.py:24bypass the egress proxy for loopback fetches. - Found: The new test cannot catch this.
test_an_env_proxy_route_is_pinned_too(posthog/security/test/test_pinned_httpx.py:63-76) asserts only that each mount is aPinnedIPTransport; no test opens a TLS connection through a proxy. - Impact: In any environment that sets
HTTPS_PROXY, all three paths this PR moves ontoupstream_mcp_client— the proxy view, tool discovery, and tool calls — fail for every public HTTPS MCP server, and they worked before the change. The failure surfaces ashttpx.ConnectError, whichproducts/mcp_store/backend/tools.pymaps to "Upstream MCP server unreachable", so the cause is hidden behind a generic transport error. That is a functional break of the product the PR hardens, which matches themust_fixpriority the reviewer set.
Issue description
PinnedIPTransport adds sni_hostname, but httpcore 1.0.9 ignores this extension in TunnelHTTPConnection. The tunnel uses the rewritten IP for server_hostname. A normal certificate for the MCP hostname then fails verification whenever HTTPS_PROXY routes the request. upstream_mcp_client enables environment proxies for every external MCP URL, so this can disable all public MCP traffic in proxied environments.
Suggested fix
Add an HTTPS CONNECT-proxy integration test that checks the CONNECT target, SNI, and certificate host. Use a tunnel transport that connects to the pinned IP but passes the original hostname to start_tls. Keep certificate verification and the egress proxy enabled.
Prompt to fix with AI (copy-paste)
## Context
@posthog/security/pinned_httpx.py#L46-50
<issue_description>
`PinnedIPTransport` adds `sni_hostname`, but httpcore 1.0.9 ignores this extension in `TunnelHTTPConnection`. The tunnel uses the rewritten IP for `server_hostname`. A normal certificate for the MCP hostname then fails verification whenever `HTTPS_PROXY` routes the request. `upstream_mcp_client` enables environment proxies for every external MCP URL, so this can disable all public MCP traffic in proxied environments.
</issue_description>
<issue_validation>
- **Checked:** httpcore's two connection classes, the locked versions, and the end-to-end behavior of the new client through a real CONNECT proxy. I also ran the same scenario against the pre-existing `requests` helper for comparison, and read the one proxy test the PR adds.
- **Found:** `httpcore/_sync/http_proxy.py:312` builds the tunneled TLS with `"server_hostname": self._remote_origin.host.decode("ascii")` and never reads `request.extensions["sni_hostname"]`. The direct path does read it (`httpcore/_sync/connection.py:107` and `:151`). `uv.lock:3217-3218` pins httpcore 1.0.9, and the venv carries httpx 0.28.1, so the installed code is the code above. Because `PinnedIPTransport` rewrites `request.url` at `posthog/security/pinned_httpx.py:49`, `_remote_origin.host` is the pinned IP.
- **Found:** Reproduced end to end. With `HTTPS_PROXY` set to a local CONNECT proxy that serves a certificate for `mcp.example.test`, a client from `pinned_httpx_client("https://mcp.example.test/mcp", {127.0.0.1})` fails: `ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: IP address mismatch, certificate is not valid for '127.0.0.1'`. The proxy logged `CONNECT 127.0.0.1:443`, so the pin does hold, and only the certificate check breaks.
- **Found:** The break is specific to the tunnel. The same pinned client on the direct route returns 200 against the same hostname certificate. The pre-existing `PinnedIPAdapter` also returns 200 through the same fake proxy, because `cert_verify` injects `server_hostname` and `assert_hostname` into the urllib3 pool (`posthog/security/pinned_requests.py:141-155`). The new httpx helper therefore regresses against its own sibling on this route.
- **Found:** The route is on by default for the new callers. `trust_environment_proxy` returns `True` for every URL that is not operator-allowlisted (`products/mcp_store/backend/url_policy.py:41-44`), and `upstream_mcp_client` passes that as `trust_env` (`products/mcp_store/backend/upstream_http.py:50`), so httpx mounts the proxy transport whenever the process has `HTTPS_PROXY`.
- **Found:** An egress proxy is the expected production shape for external calls in this repo, not a rare setup. `posthog/security/outbound_proxy.py:1-18` states external calls honor `HTTP_PROXY`/`HTTPS_PROXY` and internal calls must opt out, `.semgrep/rules/security/aiohttp-missing-trust-env.yaml` enforces `trust_env=True` on external aiohttp calls, and `posthog/metrics.py:8` plus `bin/granian_metrics.py:24` bypass the egress proxy for loopback fetches.
- **Found:** The new test cannot catch this. `test_an_env_proxy_route_is_pinned_too` (`posthog/security/test/test_pinned_httpx.py:63-76`) asserts only that each mount is a `PinnedIPTransport`; no test opens a TLS connection through a proxy.
- **Impact:** In any environment that sets `HTTPS_PROXY`, all three paths this PR moves onto `upstream_mcp_client` — the proxy view, tool discovery, and tool calls — fail for every public HTTPS MCP server, and they worked before the change. The failure surfaces as `httpx.ConnectError`, which `products/mcp_store/backend/tools.py` maps to "Upstream MCP server unreachable", so the cause is hidden behind a generic transport error. That is a functional break of the product the PR hardens, which matches the `must_fix` priority the reviewer set.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add an HTTPS CONNECT-proxy integration test that checks the CONNECT target, SNI, and certificate host. Use a tunnel transport that connects to the pinned IP but passes the original hostname to `start_tls`. Keep certificate verification and the egress proxy enabled.
</potential_solution>
| request.extensions = {**request.extensions, "sni_hostname": host} | ||
| request.url = request.url.copy_with(host=ip) | ||
| return self._inner.handle_request(request) |
There was a problem hiding this comment.
Restore the request URL after transport handling
Why we think it's a valid issue
- Checked: httpx's cookie handling in
_client.py, the jar contents and outbound headers through a real pinned client, the same flow without pinning as a control, both MCP Store call sites that reuse one client, and whether the stale URL reaches the redirect SSRF check. - Found: The mechanism is confirmed in httpx 0.28.1:
_client.py:1018-1022assignsresponse.request = requestand then callsself.cookies.extract_cookies(response), so extraction reads the URL the transport rewrote atposthog/security/pinned_httpx.py:49. - Found: Reproduced against the shipped helper. With a pin of
93.184.216.34formcp.example.test, aSet-Cookie: AWSALB=sticky1; Path=/lands in the jar under domain93.184.216.34, and the next request the same client builds for the hostname carries noCookieheader. ASet-CookiewithDomain=mcp.example.testis dropped outright, leaving an empty jar. The unpinned control stores the cookie undermcp.example.testand sendsAWSALB=sticky1on the second request, so the three converted paths did keep cookies before this change.r1.request.urlalso stayshttps://93.184.216.34/mcpafter the call, which is what httpx logs for the request. - Found: One client spans several requests on the discovery path.
products/mcp_store/backend/tools.pysendsinitialize,notifications/initialized,tools/list, andDELETEon the client fromupstream_mcp_client, andproducts/mcp_store/backend/proxy.py:105-114sends a request plus one redirect retry. So a cookie an upstream sets on the first request is gone for the rest of the sequence. - Found: No security consequence.
validated_same_origin_redirect_urlcompares against the caller's ownoriginal_urlstring (products/mcp_store/backend/proxy.py:54,68-71), notresponse.url, so the stale URL never reaches the origin check or the redirect SSRF validation. - Found: The sibling helper behaves the same way, so this is the house pinning convention rather than a new deviation in kind:
requests/sessions.py:721extracts from the rewrittenPreparedRequest, andrequests/cookies.py:43-44derives the host from that URL. Existingpinned_requestscallers such as the webhook delivery and probe paths have shipped with that behavior. - Impact: Confirmed loss of cookie continuity within one pinned client, which is a behavior regression for the converted paths. The protocol's own session mechanism survives, because
tools.pypropagatesMcp-Session-Idas a header on every step, so the break needs an upstream that keeps session state or load-balancer affinity in a cookie. If one does, the follow-up handshake steps can reach a different backend and the failure surfaces as a generic "Upstream MCP server unreachable" or a session error, which is expensive to diagnose. - Priority: Lowered to
consider. The mechanism is verified and the repair is a small save-and-restore, but nothing in this repository shows an MCP upstream that depends on cookies, and the identical behavior has run in therequestshelper without a known failure. That keeps it a real, cheap cleanup rather than an evidenced defect.
Issue description
handle_request leaves the caller's request URL rewritten after the inner transport returns. HTTPX attaches that request to the response and uses its URL to extract cookies. It stores host-only cookies for the IP and rejects Domain cookies for the original hostname. Later redirect and handshake requests start with the hostname, so HTTPX sends no cookie. This breaks upstream sessions and load-balancer affinity that depend on cookies.
Suggested fix
Save the original URL and extensions. Restore both in a finally block after the inner transport returns. Add a test that sets a cookie, sends a second request, and checks the cookie header.
Prompt to fix with AI (copy-paste)
## Context
@posthog/security/pinned_httpx.py#L48-50
<issue_description>
`handle_request` leaves the caller's request URL rewritten after the inner transport returns. HTTPX attaches that request to the response and uses its URL to extract cookies. It stores host-only cookies for the IP and rejects `Domain` cookies for the original hostname. Later redirect and handshake requests start with the hostname, so HTTPX sends no cookie. This breaks upstream sessions and load-balancer affinity that depend on cookies.
</issue_description>
<issue_validation>
- **Checked:** httpx's cookie handling in `_client.py`, the jar contents and outbound headers through a real pinned client, the same flow without pinning as a control, both MCP Store call sites that reuse one client, and whether the stale URL reaches the redirect SSRF check.
- **Found:** The mechanism is confirmed in httpx 0.28.1: `_client.py:1018-1022` assigns `response.request = request` and then calls `self.cookies.extract_cookies(response)`, so extraction reads the URL the transport rewrote at `posthog/security/pinned_httpx.py:49`.
- **Found:** Reproduced against the shipped helper. With a pin of `93.184.216.34` for `mcp.example.test`, a `Set-Cookie: AWSALB=sticky1; Path=/` lands in the jar under domain `93.184.216.34`, and the next request the same client builds for the hostname carries no `Cookie` header. A `Set-Cookie` with `Domain=mcp.example.test` is dropped outright, leaving an empty jar. The unpinned control stores the cookie under `mcp.example.test` and sends `AWSALB=sticky1` on the second request, so the three converted paths did keep cookies before this change. `r1.request.url` also stays `https://93.184.216.34/mcp` after the call, which is what httpx logs for the request.
- **Found:** One client spans several requests on the discovery path. `products/mcp_store/backend/tools.py` sends `initialize`, `notifications/initialized`, `tools/list`, and `DELETE` on the client from `upstream_mcp_client`, and `products/mcp_store/backend/proxy.py:105-114` sends a request plus one redirect retry. So a cookie an upstream sets on the first request is gone for the rest of the sequence.
- **Found:** No security consequence. `validated_same_origin_redirect_url` compares against the caller's own `original_url` string (`products/mcp_store/backend/proxy.py:54,68-71`), not `response.url`, so the stale URL never reaches the origin check or the redirect SSRF validation.
- **Found:** The sibling helper behaves the same way, so this is the house pinning convention rather than a new deviation in kind: `requests/sessions.py:721` extracts from the rewritten `PreparedRequest`, and `requests/cookies.py:43-44` derives the host from that URL. Existing `pinned_requests` callers such as the webhook delivery and probe paths have shipped with that behavior.
- **Impact:** Confirmed loss of cookie continuity within one pinned client, which is a behavior regression for the converted paths. The protocol's own session mechanism survives, because `tools.py` propagates `Mcp-Session-Id` as a header on every step, so the break needs an upstream that keeps session state or load-balancer affinity in a cookie. If one does, the follow-up handshake steps can reach a different backend and the failure surfaces as a generic "Upstream MCP server unreachable" or a session error, which is expensive to diagnose.
- **Priority:** Lowered to `consider`. The mechanism is verified and the repair is a small save-and-restore, but nothing in this repository shows an MCP upstream that depends on cookies, and the identical behavior has run in the `requests` helper without a known failure. That keeps it a real, cheap cleanup rather than an evidenced defect.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Save the original URL and extensions. Restore both in a `finally` block after the inner transport returns. Add a test that sets a cookie, sends a second request, and checks the cookie header.
</potential_solution>
| request.extensions = {**request.extensions, "sni_hostname": host} | ||
| request.url = request.url.copy_with(host=ip) |
There was a problem hiding this comment.
TLS proxy handshake uses the upstream hostname
Why we think it's a valid issue
- Checked: How httpcore builds the CONNECT request, where the proxy-leg TLS reads its
server_hostname, and the real behavior of the new client against anhttps://-scheme CONNECT proxy with a proxy-only certificate. I also compared an unpinned client and searched the repo for the proxy scheme actually configured. - Found: The extension does reach the proxy handshake.
httpcore/_sync/http_proxy.py:282-287builds the CONNECT request withextensions=request.extensions, and it goes to the tunnel's innerHTTPConnection, whose origin is the proxy. That connection applies TLS when its own origin scheme ishttpsand takes"server_hostname": sni_hostname or self._origin.host.decode("ascii")(httpcore/_sync/connection.py:107and:139-152). Thesni_hostnameset atposthog/security/pinned_httpx.py:48therefore names the MCP host on the leg to the proxy. - Found: Reproduced end to end. With
HTTPS_PROXY=https://127.0.0.1:<port>and a proxy certificate that carries only the proxy's own IP SAN, a pinned client forhttps://mcp.example.test/mcpfails withConnectError [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch, certificate is not valid for 'mcp.example.test'. The proxy recorded zero completed handshakes and no CONNECT line, and its SNI callback sawmcp.example.test, so the failure happens on the proxy handshake and before CONNECT, exactly as described. - Found: An unpinned client on the same setup completes the proxy handshake and sends
CONNECT mcp.example.test:443, becauseserver_hostnamefalls back to the proxy origin. The break comes from the pin, not from the test rig. - Found: This is the second symptom of one root gap, not a duplicate report. A single global
sni_hostnameextension is applied to both TLS legs, so the proxy leg gets the upstream name here, while the tunnel leg gets the pinned IP through_remote_origin.host(httpcore/_sync/http_proxy.py:312). A repair that patches only the tunnel leaves this failure standing, and a repair that just drops the extension breaks the direct route, which relies on it. - Impact: In an environment whose proxy endpoint speaks TLS, every pinned HTTPS MCP fetch fails before any traffic leaves, with no workaround. That covers the proxy view, tool discovery, and tool calls, and the error surfaces as a generic
ConnectErrorthat the product reports as "Upstream MCP server unreachable". - Priority: Lowered to
should_fix. The external egress proxy here is Smokescreen (posthog/otel_logs.py:68,posthog/settings/web.py:1250,products/mcp_store/README.md:121), and every proxy URL in the repository uses thehttp://scheme, includingposthog/security/test/test_pinned_httpx.py:64and thetest_outbound_proxy.pyfixtures. I found no evidence of a TLS-listening proxy endpoint, so the trigger is a plausible but unevidenced deployment choice, unlike the tunnel-leg defect that fires on any proxy. It still belongs next to that fix, because both live in the same lines.
Issue description
When the proxy URL uses https://, httpcore copies this sni_hostname extension into the CONNECT request. The TLS connection to the proxy then verifies its certificate against the MCP hostname. A normal proxy certificate fails before httpcore sends CONNECT.
Suggested fix
Keep separate TLS names for the proxy and the upstream server. Apply the upstream SNI only after CONNECT. Add an https:// proxy test that checks both TLS handshakes.
Prompt to fix with AI (copy-paste)
## Context
@posthog/security/pinned_httpx.py#L48-49
<issue_description>
When the proxy URL uses `https://`, httpcore copies this `sni_hostname` extension into the CONNECT request. The TLS connection to the proxy then verifies its certificate against the MCP hostname. A normal proxy certificate fails before httpcore sends CONNECT.
</issue_description>
<issue_validation>
- **Checked:** How httpcore builds the CONNECT request, where the proxy-leg TLS reads its `server_hostname`, and the real behavior of the new client against an `https://`-scheme CONNECT proxy with a proxy-only certificate. I also compared an unpinned client and searched the repo for the proxy scheme actually configured.
- **Found:** The extension does reach the proxy handshake. `httpcore/_sync/http_proxy.py:282-287` builds the CONNECT request with `extensions=request.extensions`, and it goes to the tunnel's inner `HTTPConnection`, whose origin is the proxy. That connection applies TLS when its own origin scheme is `https` and takes `"server_hostname": sni_hostname or self._origin.host.decode("ascii")` (`httpcore/_sync/connection.py:107` and `:139-152`). The `sni_hostname` set at `posthog/security/pinned_httpx.py:48` therefore names the MCP host on the leg to the proxy.
- **Found:** Reproduced end to end. With `HTTPS_PROXY=https://127.0.0.1:<port>` and a proxy certificate that carries only the proxy's own IP SAN, a pinned client for `https://mcp.example.test/mcp` fails with `ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch, certificate is not valid for 'mcp.example.test'`. The proxy recorded zero completed handshakes and no CONNECT line, and its SNI callback saw `mcp.example.test`, so the failure happens on the proxy handshake and before CONNECT, exactly as described.
- **Found:** An unpinned client on the same setup completes the proxy handshake and sends `CONNECT mcp.example.test:443`, because `server_hostname` falls back to the proxy origin. The break comes from the pin, not from the test rig.
- **Found:** This is the second symptom of one root gap, not a duplicate report. A single global `sni_hostname` extension is applied to both TLS legs, so the proxy leg gets the upstream name here, while the tunnel leg gets the pinned IP through `_remote_origin.host` (`httpcore/_sync/http_proxy.py:312`). A repair that patches only the tunnel leaves this failure standing, and a repair that just drops the extension breaks the direct route, which relies on it.
- **Impact:** In an environment whose proxy endpoint speaks TLS, every pinned HTTPS MCP fetch fails before any traffic leaves, with no workaround. That covers the proxy view, tool discovery, and tool calls, and the error surfaces as a generic `ConnectError` that the product reports as "Upstream MCP server unreachable".
- **Priority:** Lowered to `should_fix`. The external egress proxy here is Smokescreen (`posthog/otel_logs.py:68`, `posthog/settings/web.py:1250`, `products/mcp_store/README.md:121`), and every proxy URL in the repository uses the `http://` scheme, including `posthog/security/test/test_pinned_httpx.py:64` and the `test_outbound_proxy.py` fixtures. I found no evidence of a TLS-listening proxy endpoint, so the trigger is a plausible but unevidenced deployment choice, unlike the tunnel-leg defect that fires on any proxy. It still belongs next to that fix, because both live in the same lines.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Keep separate TLS names for the proxy and the upstream server. Apply the upstream SNI only after CONNECT. Add an `https://` proxy test that checks both TLS handshakes.
</potential_solution>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Finding on
This PR pins the MCP Store's own HTTP fetches to the validated IP address, but the parallel MCP call path used by the Max chat agent ( Fix by routing this caller through the same pinning primitives this PR introduces: have Severity: high | Confidence: 80% | React with 👍 if useful or 👎 if not |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63af84ceed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # extension carries the original hostname into TLS, for SNI and for certificate | ||
| # verification, which would otherwise run against the IP. | ||
| request.extensions = {**request.extensions, "sni_hostname": host} | ||
| request.url = request.url.copy_with(host=ip) |
There was a problem hiding this comment.
When HTTPS_PROXY is configured, rewriting the URL before invoking the proxy transport makes httpcore build the CONNECT tunnel's remote origin from the pinned IP. In the locked httpcore 1.0.9, TunnelHTTPConnection ignores the sni_hostname extension and uses that remote-origin IP for TLS SNI and certificate verification, so ordinary hostname certificates fail and public HTTPS MCP requests return 502. The proxy test only verifies that transports are wrapped, rather than exercising the tunnel handshake.
AGENTS.md reference: AGENTS.md:L206-L206
Useful? React with 👍 / 👎.
| # extension carries the original hostname into TLS, for SNI and for certificate | ||
| # verification, which would otherwise run against the IP. | ||
| request.extensions = {**request.extensions, "sni_hostname": host} | ||
| request.url = request.url.copy_with(host=ip) |
There was a problem hiding this comment.
When an upstream sets a session, affinity, or CSRF cookie during initialize, this mutation remains on the request after the transport returns. HTTPX then assigns that request to response.request and extracts cookies against the pinned IP, so host-only cookies are stored for the IP and domain cookies for the original hostname are rejected; subsequent handshake and tool requests built for the hostname omit them. Preserve the logical request URL for HTTPX's response processing while changing only the connection target.
Useful? React with 👍 / 👎.
| if chosen_ip is None: | ||
| return client | ||
|
|
||
| pins = {canonical_pin_host(urlparse.urlparse(url).hostname or ""): str(chosen_ip)} |
There was a problem hiding this comment.
For internationalized domains containing IDNA deviation characters, URL validation and this pin map use different hostnames. The validator's built-in IDNA codec turns faß.de into fass.de, whereas canonical_pin_host and HTTPX use xn--fa-hia.de; the client therefore connects to the IP validated for fass.de while sending Host/SNI for the other domain. This generally causes HTTPS certificate failures and, over HTTP, can send credentials to the wrong domain's address.
Useful? React with 👍 / 👎.
| # extension carries the original hostname into TLS, for SNI and for certificate | ||
| # verification, which would otherwise run against the IP. | ||
| request.extensions = {**request.extensions, "sni_hostname": host} | ||
| request.url = request.url.copy_with(host=ip) |
There was a problem hiding this comment.
When a validated hostname has only AAAA records and an environment proxy is active, this rewrite gives httpcore the raw host 2001:db8::1. Httpcore 1.0.9 then constructs an HTTPS CONNECT authority such as 2001:db8::1:443, rather than the required [2001:db8::1]:443; its forward-proxy absolute URL serialization has the same ambiguity for HTTP. Proxies can reject or misparse these targets, so IPv6-only MCP upstreams cannot be reached through the egress proxy even after preserving the TLS hostname.
Useful? React with 👍 / 👎.
|
|
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
1 participant
Problem
check_mcp_url_policyresolves the hostname to decide the URL is safe. Thehttpx.Clientthat opens the connection resolves the same hostname again. A record that answers the two lookups differently passes the check and still reaches a loopback, cluster-internal, or metadata address.requestscallers:pinned_requestsstates that validation alone leaves the window open and that a caller must connect to the returned addresses. No equivalent existed forhttpx, and the MCP Store is anhttpxcaller.proxy_mcp_requestfetch_upstream_toolscall_upstream_toolFound by an internal security review. No exploitation was observed, and nothing in this PR comes from customer data.
Changes
posthog/security/pinned_httpx.pyis thehttpxcounterpart ofpinned_requests. It wraps the transportshttpxbuilds for itself instead of replacing them, so a request that must leave through an egress proxy still does. Replacing the transport would have dropped that routing silently, becausehttpxignoresHTTP_PROXYas soon as a client is given a transport of its own.products/mcp_store/backend/upstream_http.pyis the one way a fetch reaches an upstream server. The client cannot be built without a verdict, and a blocked verdict raises, so a fetch path cannot skip the policy or drop the pin._canonical_hostinpinned_requestsis nowcanonical_pin_host, shared by both helpers. Test patch points move fromhttpx.Clientin each product module to the shared client builder.How did you test this code?
products/mcp_store/backend/test/test_proxy.pygains a proxy request that resolves to a public address and asserts the outbound request carries that address, with the hostname still in the Host header and in SNI. Without pinning the request would carry the hostname and resolve a second time, so this case fails on master.test_upstream_http.pycovers the verdict: an allowed URL carries the addresses it was validated on, a blocked one carries none, an operator-allowlisted URL is allowed unpinned, and a blocked verdict refuses to build a client.test_pinned_httpx.pycovers the transport: the rewrite for IPv4 and IPv6, an internationalized host matching its pin, refusal of an unpinned host, and every proxy route carrying the pin when the environment configures one. That last case is the one that catches a future change to how the client is built.products/mcp_store/backend/testandposthog/security/test,mypy --cache-fine-grainedover both packages, andhogli ci:preflight --fix.Automatic notifications
Docs update
None. Behavior for an allowed URL is unchanged.
🤖 Agent context
Autonomy: Fully autonomous
httpx.Client, which turns off environment proxy routing for MCP traffic, and it re-validates on every connect rather than pinning the verdict the call site already has.requestshelper. It would have meant rewriting the streaming, redirect, and error-mapping code around a second HTTP library, for a product already built onhttpx./simplifywas invoked over the branch and found nothing to cut, though its parallel review agents were not spawned, so the four cleanup angles were applied by hand. No other repo skill file was loaded; the conventions came fromCLAUDE.md.Created with PostHog Desktop from this inbox report.