fix: route authed urllib paths through a shared NoRedirectHandler opener (BE-3274)

[mattmillerai]

ELI-5

When our CLI calls the cloud with your API key attached and the server answers "go look over there instead" (a redirect), plain urlopen cheerfully follows — and hands your credential to wherever "over there" points. We already have a guard for exactly this (NoRedirectHandler), but three authed code paths were never wired up to it. This wires them up: a redirect on those paths now fails loudly instead of quietly replaying your key somewhere else.

What changed

Adds a shared authed-request layer to comfy_cli/http.py and migrates the three unguarded paths onto it.

comfy_cli/http.py — new, below the existing NoRedirectHandler:

• _AUTHED_OPENER = build_opener(NoRedirectHandler()) — the shared no-redirect opener.
• build_authed_request(url, target, *, method, data, content_type) — attaches X-API-Key (preferred) or Authorization: Bearer, plus optional Content-Type.
• authed_urlopen(...) — builds the request and opens it through _AUTHED_OPENER.

Migrated call sites (each drops its own private _authed_request + urlopen pair):

• command/workflow.py — _userdata_request, _http_request
• command/models/search.py — _http_get_json
• command/jobs.py — _cloud_cancel

A refused redirect surfaces as HTTPError, which every migrated site's existing error handling (_handle_cloud_http_error / _handle_local_http_error / _emit_http_error / the _cloud_cancel HTTPError branch) already covers — so there are no caller changes. Byte caps, _ResponseTooLarge, and error mapping are untouched.

Headers are deliberately not gated on target.is_cloud: the three sites don't gate today (local targets are simply uncredentialed), so this keeps the migration behavior-preserving apart from the intended redirect refusal.

Why this is safe

• The refusal semantics are not new. NoRedirectHandler and its "none of our authenticated endpoints redirect under normal operation" premise already ship on main and are already applied to this same cloud API by six existing openers (comfy_client.py, cql/engine.py, cql/loader.py, command/transfer.py, cloud/oauth.py). This PR extends an existing, reviewed guard to three paths that were missed — it does not invent a new denial.
• No global-opener divergence. install_opener is called nowhere in the codebase, so plain urlopen was using the stock default opener. build_opener(NoRedirectHandler()) yields the same default handler set (proxy handling included) with only the redirect handler swapped — the established pattern here.
• _cloud_cancel payload preserved: still method="POST", data=b"", so Content-Length: 0 is sent exactly as before.

Verification

• Full suite green: 2582 passed, 37 skipped (uv run --extra dev pytest).
• ruff format --check clean (235 files). ruff check shows 14 pre-existing UP038 errors, byte-identical to the origin/main baseline (verified by diffing the lint output against a stashed tree) — none in touched files.

Acceptance criteria

• ✅ git grep 'add_header("X-API-Key"' comfy_cli/ no longer matches workflow.py, models/search.py, or jobs.py. Remaining matches are comfy_client.py + cql/engine.py (explicitly out of scope — already guarded) and http.py (the new shared helper).
• ✅ No plain urllib.request.urlopen remains on an auth-attaching path. The survivors — jobs.py local queue/interrupt and templates.py — were each grep-verified to attach no auth headers.
• ✅ Full test suite passes.
• ✅ comfy_client.py, cql/engine.py, cql/loader.py, transfer.py, cloud/oauth.py untouched.

Tests added (tests/comfy_cli/test_http.py)

X-API-Key precedence when both credentials are set; Bearer-only; no auth header when uncredentialed; Content-Type passthrough (and absence by default); method/data passthrough; authed_urlopen opens via _AUTHED_OPENER with the right Request/timeout; a refused 302 propagates as HTTPError; and an end-to-end check that a migrated caller (search's _http_get_json) propagates a refused redirect.

Existing mocks that patched urllib.request.urlopen and would now miss the new code path were repointed at comfy_cli.http._AUTHED_OPENER.open (test_search.py, test_workflow_saved.py). test_jobs.py's _capture_urlopen patches both — its local queue/interrupt tests still exercise plain urlopen, while cloud cancel now goes through the authed opener.

Judgment calls

• Import placement. workflow.py intentionally keeps urllib out of its top-level imports, so authed_urlopen is imported lazily inside the functions there, preserving that style. search.py and jobs.py already import urllib at module top level, so the helper is imported at top level there to match each module's existing convention. (search.py's now-unused import urllib.request was removed; urllib.error/urllib.parse are still used.)
• Out of scope, flagged not fixed. install.py, models/models.py, and generate/client.py attach Authorization: Bearer but use a different HTTP stack (not urllib), so they are outside this urllib-focused pass. Consolidating the six existing guarded openers onto _AUTHED_OPENER is likewise a separate follow-up.
• Redirect-refusal premise not empirically re-tested against live cloud. This change's user-facing effect is a denial (a 30x on these three paths now errors instead of being followed). I could not make live authed cloud calls from this environment to confirm no endpoint legitimately redirects. The premise is inherited, not invented: it is the documented rationale of the existing NoRedirectHandler and is already relied upon against the same API by the six openers listed above. Flagging explicitly so a reviewer with cloud access can veto if any of /userdata, /api/experiment/models, /api/assets, or /api/jobs/<id>/cancel is expected to 30x in normal operation.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 60821a41-2679-459a-bc41-4455ae167b0a

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 3051e32.

⛔ Files ignored due to path filters (1)

• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• comfy_cli/command/jobs.py
• comfy_cli/command/models/search.py
• comfy_cli/command/workflow.py
• comfy_cli/http.py
• tests/comfy_cli/command/models/test_search.py
• tests/comfy_cli/command/test_workflow_saved.py
• tests/comfy_cli/jobs/test_jobs.py
• tests/comfy_cli/test_http.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3274-authed-urlopen

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3274-authed-urlopen

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 2 finding(s).

Severity	Count
🟢 Low	2

Panel: 6/8 reviewers contributed findings.

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

[mattmillerai]

Re-reviewed this PR after the CI went red. Both failures are pre-existing repo-wide breaks, not caused by this PR — the diff here touches only comfy_cli/http.py and three urllib call sites. Details, since the red X is misleading:

1. build — tomlkit 0.15 regression. tomlkit 0.15.x now raises ValueError: Comment cannot contain line breaks, which trips the two multi-line .comment() calls in comfy_cli/registry/config_parser.py (lines 75 and 250). The same 9 tests fail identically on unrelated PRs #539/#540/#541.

It only shows up in CI because .github/workflows/build-and-test.yml installs via pip install -e ., which ignores uv.lock, and pyproject.toml lists tomlkit unconstrained — so CI picks up 0.15.1 while uv.lock pins the working 0.13.3. Hence uv run pytest is green locally. Reproduced standalone:

$ python -c "import tomlkit; i=tomlkit.item('x'); i.comment('a\nb')"   # tomlkit 0.15.1
ValueError: Comment cannot contain line breaks

2. test (Windows) — uv rewriting its own live venv. pip install -e . puts pydantic-core in ./venv, then comfy install --fast-deps runs uv which tries to replace pydantic_core inside that same venv while its interpreter is running comfy. Windows holds a lock on the loaded .pyd, so the removal fails with Access is denied (os error 5) — uv's own message says "caused by a known uv issue" — leaving pydantic_core half-uninstalled. Deterministic across a re-run, and reproduces on unrelated PR #540.

Both are being tracked as separate follow-ups rather than fixed here, to keep this auth-hardening PR scoped.

On this PR itself: both review threads are resolved (the http(s) opener pinning landed in 651aa76; the _http_request oversize-read finding is deferred to its own follow-up). I re-ran the affected suites locally — test_http.py, test_search.py, test_workflow_saved.py, test_jobs.py: 132 passed. ruff check flags nothing in any file this PR touches. No code changes were needed this pass.

[mattmillerai]

Correction to my previous comment: both CI breaks already have fixes in flight — I've confirmed neither needs a new ticket, and both are still unrelated to this PR.

• build / tomlkit 0.15 — fixed by fix(registry): emit pyproject hint blocks as single-line tomlkit comments #533 and fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15 #536 (emit the hint blocks as single-line/standalone comments). ci: install test dependencies from uv.lock instead of resolving fresh #537 additionally switches build-and-test.yml to install from uv.lock, which is why ci: install test dependencies from uv.lock instead of resolving fresh #537 is the one open PR whose build is green.
• Windows test / pydantic_core lock — ci: set UV_LINK_MODE=copy in the Windows workflow #529 (UV_LINK_MODE=copy) and fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289) #535 (build telemetry providers lazily so comfy install can upgrade pydantic_core, BE-3289). fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289) #535 matches the root cause exactly: the eager telemetry import chain (mixpanel → pydantic) loads _pydantic_core.pyd into the running interpreter, so uv can't replace it.

So this PR just needs those to land (or a rebase once they do); nothing to do here. The substance of my previous comment stands — this PR's own diff is clean, both review threads are resolved, and the 132 affected tests pass locally.