fix(http): pin the remaining credential-carrying urllib openers to http(s) handlers only (BE-3298)

[mattmillerai]

ELI-5

urllib.request.build_opener() quietly hands you more than HTTP: it also installs
FileHandler, FTPHandler and DataHandler. So an opener that attaches your
Authorization: Bearer … / X-API-Key will happily read file:///etc/passwd if a
caller is ever steered into such a URL. #530 pinned _AUTHED_OPENER to http(s) only;
this PR does the same for the five other credential-carrying openers that were outside
that PR's diff.

What changed

1. #530's private _build_authed_opener() is generalized into a reusable
   build_http_only_opener(*handlers) in comfy_cli/http.py. It installs
   ProxyHandler, HTTPHandler, HTTPSHandler, HTTPDefaultErrorHandler,
   HTTPErrorProcessor, UnknownHandler plus the caller's handlers — and
   _AUTHED_OPENER is re-expressed in terms of it (no behavior change).

2. Each opener now builds through it, keeping its existing handler argument
   (transfer.py keeps its custom refusal message; the rest pass a bare
   NoRedirectHandler()):

   Opener	File
   _OPENER	comfy_cli/comfy_client.py
   _opener	comfy_cli/cql/engine.py
   _LOADER_OPENER	comfy_cli/cql/loader.py
   _OAUTH_OPENER	comfy_cli/cloud/oauth.py
   _TRANSFER_OPENER	comfy_cli/command/transfer.py

3. _DOWNLOAD_OPENER is scheme-pinned too but keeps following redirects — its
   _DownloadRedirectHandler (which strips auth headers and re-checks the scheme on
   each hop) is intentional. A test locks that in so a future cleanup can't silently
   convert it to no-redirect.

4. tests/comfy_cli/test_http_only_openers.py mirrors fix: route authed urllib paths through a shared NoRedirectHandler opener (BE-3274) #530's two checks across all
   openers, parametrized: file:// / ftp:// / data: raise
   URLError("unknown url type"), and the handler set excludes File/FTP/Data.

5. Proxies are filtered to http(s) (_http_only_proxy_handler()). Bare ProxyHandler()
   reads getproxies() and registers a <scheme>_open per entry, so an ftp_proxy in the
   environment handed the opener an ftp_open that won dispatch over UnknownHandler —
   servicing ftp:// through the proxy and defeating the whole point of this PR. Caught by
   the review panel; confirmed reproducible before the fix. http(s) proxy support is
   preserved (and no_proxy/proxy_bypass read the environment directly, not this dict).

6. HTTPSHandler is installed conditionally: urllib.request only defines it on an
   SSL-capable build, so naming it unconditionally raised AttributeError at import time
   on a Python built without SSL, taking the whole module with it.

Why this is safe (no capability is actually denied)

The diff's user-visible effect is a refusal, so I went looking for any path where a
non-http scheme reaches these openers today. There is none — every one either builds
its URL itself or is scheme-guarded upstream:

• transfer.py download — _assert_download_url() already rejects non-http(s)
  before the opener (transfer.py:705), and file:// for local jobs is handled by
  a separate filesystem branch (_local_source_path() → _copy_local_output_capped())
  that never touches an opener. So local-output copying is untouched, and ftp://
  already failed with a clearer error than mine.
• cql/loader.py — the file path goes through _load_from_file() (a Path read);
  _LOADER_OPENER only ever opens a hard-coded http://{host}:{port}/object_info
  loopback URL.
• cql/engine.py, comfy_client.py, cloud/oauth.py — URLs come from a trusted
  target.base_url plus validated path segments, never a server-returned value.

This is defense-in-depth for a future caller, not a live exploit fix — matching #530's framing.

Judgment calls

• This PR is stacked on fix: route authed urllib paths through a shared NoRedirectHandler opener (BE-3274) #530 (base matt/be-3274-authed-urlopen), because the
  _build_authed_opener() helper it generalizes only exists there. GitHub will
  retarget to main when fix: route authed urllib paths through a shared NoRedirectHandler opener (BE-3274) #530 merges; review sees only the net diff.

• Tests live in one parametrized module rather than five near-identical copies
  scattered across each module's test file — this is a cross-module invariant, and a
  table-driven test means the next opener added gets covered by appending one line.

• build_http_only_opener() mirrors build_opener's override semantics. I originally
  left this out as speculative machinery, but review pushed back and was right: the failure
  mode is silent (a caller-supplied cert-pinning HTTPSHandler would be accepted and then
  never consulted, because the default's https_open wins dispatch). It's ~4 lines, so a
  caller-supplied handler now replaces the default it subclasses.

• Out of scope, flagged not fixed (follow-up proposed): comfy_cli/command/templates.py (:51, :413) and
  comfy_cli/command/jobs.py (:87, :995, :1019) call bare urllib.request.urlopen(),
  which uses the global default opener — File/FTP/Data handlers and all. That's the same
  class of gap but not in this ticket's enumerated five; happy to file a follow-up.

Verification

• pytest tests/ -q → 2625 passed, 37 skipped
• ruff check . / ruff format --check . → clean, using CI's pinned ruff==0.15.15.
  Heads-up: the dev extra is unpinned and resolves to ruff 0.12.7 locally, which reports
  14 pre-existing UP038 errors on main too (the rule was removed in newer ruff) — not
  from this diff, and CI's pinned version is green.

Review round 2

The cursor-review panel found a real hole in the first cut, all three now fixed in 2774121:

• ftp_proxy smuggled ftp:// back in (medium). Reproduced before fixing — the opener
  really did gain an ftp dispatch key. Every opener now dispatches exactly
  ['http', 'https', 'unknown'] even with ftp_proxy/all_proxy set.
• HTTPSHandler on an SSL-less build — now hasattr-guarded.
• caller overrides silently ignored — now mirrors build_opener.

Re-verified after the fix: full suite green, and http(s) proxying checked end-to-end against
a live local proxy (it receives the absolute URI and serves the response), plus no_proxy
bypass still declines to direct. So the added refusal costs no real capability.

Note: pytest.yml/ruff_check.yml/CodeRabbit don't run on this PR because it's stacked on
matt/be-3274-authed-urlopen rather than main; the suite and CI's pinned ruff were run
locally instead. They'll run once #530 merges and this retargets.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c4702e2e-e215-43c9-84aa-cbdf3e3707ab

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3298-http-only-openers

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3298-http-only-openers

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 3 finding(s).

Severity	Count
🟡 Medium	1
🟢 Low	2

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

✅ No high-signal findings.

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)