2 Commits

Author	SHA1	Message	Date
Alberto Schiabel	d544006a25	fix(sdk): pin the validated address when fetching URLs (SSRF DNS rebinding) (#4172) ... Fixes #4151. ## The problem Both SDKs validated a URL by resolving its hostname, and then handed the *hostname* to the HTTP client, which resolved it again when it opened the socket. Two lookups, two answers: a short-TTL record under an attacker's control answers publicly for the check and with `169.254.169.254`, `127.0.0.1`, or RFC 1918 space for the connect. The guard passes and the connection lands inside the network — classic TOCTOU DNS rebinding, documented in both modules until now as a known residual. ```mermaid sequenceDiagram participant SDK participant DNS as Attacker DNS participant Meta as 169.254.169.254 Note over SDK,Meta: before SDK->>DNS: resolve evil.example.com (validate) DNS-->>SDK: 93.184.216.34 — passes the guard SDK->>DNS: resolve evil.example.com (connect) DNS-->>SDK: 169.254.169.254 SDK->>Meta: GET /latest/meta-data/… Meta-->>SDK: credentials ``` ## The fix Resolve once, validate every answer, then connect to the address that was validated. There is no second lookup left to rebind. - **Python** — `safe_get` / `safe_request` mount a transport adapter that swaps the connect target for the duration of the socket connect only. The `Host` header and TLS SNI keep the hostname, so certificate verification is unchanged; rewriting `conn._dns_host` for the whole connection would have sent `Host: <ip>` and offered the IP as SNI, failing against every real origin. Every fetch call site now goes through those two helpers, so no `requests.get` sits next to a bare check any more: - `_files.py::_fetch_file_from_url`, `_files.py::FileDownloadable.download` - `tool_router_session_files.py::_fetch_url_bytes` - `safe_request`, per redirect hop - **TypeScript** — `assertSafeFetchTarget` returns the validated address and `ssrfSafeFetch` hands `fetch` a dispatcher pinned to it, re-pinned per redirect hop. The dispatcher goes to the runtime's own `fetch`, so callers that stub `globalThis.fetch` keep working. The pinned `lookup` answers both shapes Node calls it with — the address *list* it uses for Happy Eyeballs, and the single `(address, family)` it uses when `autoSelectFamily` is off — since answering in the wrong shape is rejected as an invalid address. - A fail-closed peer assertion runs on the Python side before a byte is written to the socket — redundant while pinning works, and a tripwire if a urllib3 upgrade ever breaks it. - `workerd` is unchanged: it already fails closed for user-supplied URLs. Redirect *validation* already existed in both SDKs (`safe_request` / `ssrfSafeFetch`); what was missing was re-pinning each hop. ## Tests The existing suites could not express this bug: they mock both the resolver and the HTTP client, so check and use are the same mock. The new tests use real sockets. - `python/tests/test_url_safety_pinning.py` — two loopback servers and a resolver that answers the first lookup with one endpoint and every later one with another, which is what a short-TTL rebinding record does. Asserts the rebound endpoint receives **zero** connections, and that `Host` still carries the hostname. Both tests fail on `next` and pass here. - `ts/packages/core/test/utils/pinnedDispatcher.node.test.ts` — a real server plus a hostname under `.invalid`, which RFC 2606 guarantees never resolves. A request that arrives proves the connect used the pinned address and never consulted DNS. The third case shows the contrast: unpinned, the same fetch cannot resolve at all. - `ssrfGuard.test.ts` gains assertions that each hop is pinned to that hop's own validated address. - `pinnedDispatcher.node.test.ts` also pins with `setDefaultAutoSelectFamily(false)`, which is the branch Node takes for the single-address callback. ## Notes - Supersedes #4157, which diagnosed this correctly. Its post-response peer check turned out not to hold: with an HTTP/1.0 or `Connection: close` server, urllib3 detaches the socket (`conn.sock is None`) while `r.content` still returns the full body, so the check fails open exactly where exfiltration succeeds. That is why the assertion here runs at connect time instead. - The Python package now declares `urllib3>=2` directly. `url_safety` imports it for `NameResolutionError`, which only exists from 2.0, and the pinning adapter reaches into 2.x connection internals; `requests` alone allows 1.x, where `import composio` would have failed outright. - `@composio/core` gains an `undici` dependency, pinned to `^7`: undici 8 dispatchers are rejected by the `fetch` in every Node version this package supports (22/24/25, verified). The real-socket test runs on the full CI matrix, so a future incompatibility fails loudly instead of silently un-pinning. - `undici` is imported on first pinned request rather than at module load: importing it installs a process-wide global dispatcher, which would have handed the host application's own unrelated `fetch` calls this package's undici merely because it imported `@composio/core`. - Residuals, now documented in the modules: - Requests routed through an environment proxy keep the pre-flight check only. The proxy resolves the hostname itself and the SDK cannot see or pin that resolution. - A process that does perform a pinned fetch still ends up on this package's `Agent` if nothing had claimed the global dispatcher slot yet. undici defines that slot non-configurable, so it cannot be handed back — assigning `undefined` leaves the runtime's own `fetch` asserting on a missing dispatcher.	2026-08-20 13:34:37 +02:00
Alberto Schiabel	b1c6671b20	fix(py): send Content-Type on S3 PUTs and parse Content-Length (#4168) ... This PR: - fixes #4153 - supersedes https://github.com/ComposioHQ/composio/pull/4154 (fork PR authored against a pre-#4146 tree; its tests patch `requests.put`, which `upload()` no longer calls after #4146, so they fail on merge) - adds `parse_content_length()` in `composio.utils.url_safety`, shared by both URL fetch helpers (`_files.py::_fetch_file_from_url` and `tool_router_session_files.py::_fetch_url_bytes`), so a malformed/negative remote `Content-Length` degrades to unknown size under the streamed byte count instead of raising a raw `ValueError` - converges the file and bytes upload paths onto `_upload_to_presigned_url()`: one PUT that sends the `Content-Type` the presign request was signed with and raises `ErrorUploadingFile` carrying the HTTP status (a 403 no longer collapses into a path-only error) - single-sources the presign wire shape via `_request_presigned_upload()`; `from_path()` forwards the exact mimetype it minted, so signed and sent content types cannot drift - mirrors the TypeScript SDK: `uploadFileToS3` funnels path/URL/File inputs through one uploader that always sends `Content-Type` and throws on non-2xx, and `readResponseBodyWithLimit` trusts `Content-Length` only as a hint - adds tests against the `safe_request` seam (`test_file_upload_robustness.py`, plus one malformed-header case for `RemoteFile.buffer()`) ## Context Both defects in #4153 were symptoms of two parallel upload paths drifting: the bytes path sent `Content-Type` and raised with the status, the file path did neither. Patching the symptom in place (as #4154 proposed) would have left three presigned PUT sites and two error contracts in the tree; this PR deletes the drift vector instead. Full suite green (1320 passed), `make chk` and `make snt` clean; Python-only change, so no changeset per `AGENTS.md`. --------- Co-authored-by: Mustaqeem66 <265153888+Mustaqeem66@users.noreply.github.com>	2026-08-18 19:09:02 +02:00