Skip to content

fix: pin untrusted Python media fetches - #30

Closed
0xildefonso wants to merge 2 commits into
mainfrom
fix/redirect-safe-python-fetches
Closed

fix: pin untrusted Python media fetches#30
0xildefonso wants to merge 2 commits into
mainfrom
fix/redirect-safe-python-fetches

Conversation

@0xildefonso

@0xildefonso 0xildefonso commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Outcome

Adds one strict Python outbound HTTP boundary and migrates every remote-fetch/upload path tracked by Pieverse-Eng/purrfect-claw-platform#1732.

  • validates the initial URL, default port, all DNS answers, and every redirect hop
  • rejects private, loopback, link-local, multicast, metadata, NAT64/special, and mixed public/private destinations
  • pins the validated address for direct and configured-proxy transports while preserving Host/TLS SNI
  • bounds one total deadline, redirects, response bytes, content type, and decompression
  • strips sensitive headers on unapproved hosts or TLS downgrade; Slack bot auth is restricted to files.slack.com
  • permits only 307/308 redirects for QQ presigned PUTs and requires an operator-pinned exact account COS bucket-host allowlist; broad myqcloud.com suffixes fail closed
  • documents QQ_COS_UPLOAD_HOSTS / cos_upload_hosts for legitimate chunked uploads over 10 MB
  • migrates OpenAI edit references, provider image responses, Discord, Slack, WeCom, Feishu, and QQ upload paths
  • adds an AST path-inventory contract so removing any cited caller from the shared boundary fails tests

Verification

  • expanded affected matrix after review fix: 920 passed, 0 failed, serial (789 migrated-path tests + 131 legacy URL-safety tests)
  • exact QQ/safe-HTTP files: 209 passed, 0 failed
  • ruff check .: passed
  • ty check tools/safe_http.py: passed
  • git diff --check: passed
  • live direct image fetch: 200 image/png, 958 bytes
  • live redirect fetch: https://www.github.com/https://github.com/, one validated redirect, 200

The repository-wide local run was not treated as authoritative because this worktree intentionally lacks the all extra (acp collection fails before affected tests); GitHub CI installs --extra all --extra dev and is the full-suite gate.

Refs Pieverse-Eng/purrfect-claw-platform#1732

Comment thread tests/tools/test_safe_http.py Fixed
@0xildefonso

Copy link
Copy Markdown
Collaborator Author

Independent review of child SHA 947f86a869583eafd699775bdb3cb72e40f01695 (baseline 45ff4201). No blockers. The primitive is strong — in two respects stronger than its TypeScript sibling. One finding worth acting on and two notes.

What's right

Per-hop _parse_url + _host_allowed + _validated_addresses inside the loop, with the connection pinned to an already-validated address. The TLS handling is correct: _PinnedHTTPSConnection.connect wraps the socket with server_hostname=self.host under a default context, so SNI and certificate validation still bind to the hostname while the socket goes to the checked IP. That closes the TOCTOU window url_safety's own docstring lists as unfixed.

The header policy is stricter than I asked for: caller-supplied Host is rejected outright, sensitive headers require HTTPS and an authorized host, and hop_headers re-filters on every hop. Slack now passes credential_hosts=("files.slack.com",) as a literal rather than deriving it from the untrusted URL — that was the right fix.

UNSAFE_METHOD_REDIRECT restricting non-GET/HEAD to 307/308 is a good call, and size handling reads max_bytes + 1 then raises RESPONSE_TOO_LARGE, so an oversize body is rejected rather than silently truncated.

Finding — allowed_host_suffixes=("myqcloud.com",) is a shared multi-tenant namespace

chunked_upload.py validates the presigned PUT destination against the suffix myqcloud.com. Tencent COS buckets are addressed as <bucket>-<appid>.cos.<region>.myqcloud.com, so every Tencent COS customer has a host under that suffix. The check therefore constrains the upload to "some Tencent bucket", not "our bucket".

That matters for the exact finding this path is meant to close (csf_269c…, provider-supplied initial PUT destination): an attacker who can influence the upload-init response can point the PUT at their own *.myqcloud.com bucket and receive the uploaded media — no redirect required, and the suffix check passes. The 307/308 path has the same shape, with the body replayed to any host under the suffix.

Credentials are stripped cross-host, so this is content disclosure rather than token disclosure. If the account's COS host or bucket/appid is known or configurable, pin it exactly. If it genuinely cannot be pinned, say so in the code comment — "validated against myqcloud.com" currently reads as a stronger guarantee than it provides, which is the overclaiming pattern we corrected in NousResearch#1735.

Notes

  1. Three deltas now exist between this primitive and the TS one in feat(web): add Tavily as web search/extract/crawl backend NousResearch/hermes-agent#1731, both closed under the same umbrella: this allows http (TS is HTTPS-only; mitigated here since sensitive headers require TLS), this iterates all validated addresses while TS pins resolved[0] with no failover, and this has a method-redirect restriction TS lacks. None are defects, but someone will later assume the two behave alike. Worth one line in each PR stating the intended difference — and the failover behaviour here is the better one, so the cheap alignment is adding it to the TS client.
  2. _host_allowed returns True when neither allowlist is supplied, reducing protection to IP-publicness. That is correct for arbitrary model-supplied image URLs, but it means a future caller who forgets the parameter silently gets the weakest policy. Consider making the open mode an explicit opt-in so omission is a mistake rather than a default.

Nothing merged. Parent pointer PR still to review separately — I won't treat this child review as covering it.

@0xildefonso

Copy link
Copy Markdown
Collaborator Author

Re-review of 947f86a8..c5ee8b6a. Finding closed. No blockers.

_normalize_cos_upload_hosts now requires exact Tencent COS hostnames — at least five labels, containing .cos., ending .myqcloud.com, each label matching a strict DNS-label pattern — and rejects URLs, wildcards, and suffix-only entries at construction time rather than at request time. _put_to_presigned_url raises HOST_NOT_ALLOWED before any I/O when the list is empty, and the call site now passes allowed_hosts=self._allowed_upload_hosts instead of the suffix. The cross-tenant path is gone in both the direct and redirect cases.

The docstring is the part I'd most want kept: it states plainly that a broad *.myqcloud.com suffix is insufficient because any COS customer can own a bucket beneath it, and that QQ does not bind upload_prepare to a hostname derivable from the bot app ID — so operators must pin. That records the real limitation instead of implying the destination is self-validating.

One operational consequence, deliberate but worth routing

This is fail-closed on a previously working path: any deployment using QQ chunked uploads stops working until an operator sets QQ_COS_UPLOAD_HOSTS / cos_upload_hosts. That is the correct trade-off — I raised the risk and this is the honest fix — but it is a required configuration step, not just a documentation change, and it will present as "QQ file uploads broke" if it reaches production unannounced. Docs are updated in English and zh-Hans; it should also appear in whatever deploy/release checklist the platform pointer PR feeds, so an operator sets the value before the submodule bump ships.

Status

CodeQL, ruff, lints, uv.lock, contributors, and common-ancestor checks are green; the Python test slices and docs-site job were still pending when I looked. Green CI remains a precondition. Earlier non-blocking notes stand: _host_allowed still defaults open when no allowlist is supplied, and this primitive still differs from the TypeScript one in NousResearch#1731 on http support, address failover, and method-redirect handling.

Nothing merged. Parent pointer PR reviewed separately.

xxxxxmax commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Closing without merge because the platform is moving to the latest official NousResearch/hermes-agent version instead of maintaining this fork-specific outbound HTTP implementation.

Pieverse-Eng/purrfect-claw-platform#1786 and its tracker NousResearch#1732 have been closed as superseded/not planned. The official upstream already carries the shared URL-safety client, connect-time DNS/IP validation, and redirect guards for the channel media paths; remaining OpenAI/provider image work is tracked upstream in NousResearch#54553 and NousResearch#45537.

We therefore will not ship or maintain this separate tools/safe_http.py boundary or the additional QQ COS host rollout gate.

@xxxxxmax xxxxxmax closed this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants