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 — below the existing NoRedirectHandler:

• target_auth_headers(target) — the single is_cloud-gated header builder (landed on main independently via refactor(http): shared is_cloud-gated target_auth_headers builder; migrate 5 call sites (BE-4362) #597/BE-4362 while this branch was in flight; merged in here).
• build_http_only_opener(*handlers) / _AUTHED_OPENER / _PLAIN_OPENER — shared openers pinned to http(s) only (no file:///ftp:///data:).
• build_authed_request(url, target, *, method, data, content_type) — attaches the header from target_auth_headers (so it inherits the is_cloud gate), plus optional Content-Type.
• authed_urlopen(...) / plain_urlopen(...) / no_redirect_urlopen(...) — open a request through the appropriate shared opener.

Migrated call sites (each drops its own private _authed_request/_auth_headers-inline pair):

• command/workflow.py — _userdata_request, _http_request
• command/models/search.py — _http_get_json
• command/jobs.py — _cloud_cancel
• command/transfer.py, cql/engine.py — already on target_auth_headers via refactor(http): shared is_cloud-gated target_auth_headers builder; migrate 5 call sites (BE-4362) #597; now paired with build_http_only_opener/_AUTHED_OPENER for the no-redirect guard too.

A refused redirect surfaces as HTTPError, which every migrated site's existing error handling already covers — no caller changes.

Headers ARE gated on target.is_cloud. An earlier revision of this PR shipped build_authed_request with its own inline, ungated header copy (a stray credential on a local Target would have been attached). Reconciling with main's parallel target_auth_headers refactor (#597) surfaced that gap during review — build_authed_request now delegates to target_auth_headers, so a local Target can never carry a credential through this path, matching every other call site.

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 other 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_http_only_opener(...) yields the same default handler set (proxy handling included, http(s)-only) with 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 locally after merging main: uv run pytest tests/ passes (3473 passed, 37 skipped).
• ruff check / ruff format --check clean on every file this PR touches; the handful of repo-wide pre-existing findings (17 UP038, one docstring reformat in tests/comfy_cli/command/github/test_pr.py) are byte-identical to the origin/main baseline and untouched by this diff.
• Merged main in to resolve a real conflict (both branches had independently refactored comfy_cli/http.py — this PR's no-redirect opener work vs. main's target_auth_headers extraction in refactor(http): shared is_cloud-gated target_auth_headers builder; migrate 5 call sites (BE-4362) #597). Reconciled by keeping both: header building goes through target_auth_headers, request opening goes through the shared http(s)-only, no-redirect opener.
• Fixed a semantic break the merge exposed in tests/comfy_cli/command/test_pretty_print_sanitize.py (added on main after this branch diverged): it mocked comfy_cli.command.run.execution.request.urlopen, but execution.py on this branch already routes /prompt submission through no_redirect_urlopen — repointed the mock at comfy_cli.command.run.execution.no_redirect_urlopen.

Tests added/updated (tests/comfy_cli/test_http.py)

X-API-Key precedence when both credentials are set; Bearer-only; no auth header when uncredentialed; no auth header for a local Target even with stray credentials set (the gap closed above); 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; an end-to-end check that a migrated caller (search's _http_get_json) propagates a refused redirect; plus main's own target_auth_headers unit tests, merged in unchanged.

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.
• 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.
• Redirect-refusal premise not empirically re-tested against live cloud. This change's user-facing effect is a denial (a 30x on these 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 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: 54 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: 9892033e-a1aa-4d02-abf6-10ae28021b61

📥 Commits

Reviewing files that changed from the base of the PR and between 46e6cf7 and d17a0f1.

📒 Files selected for processing (20)

• comfy_cli/cloud/oauth.py
• comfy_cli/comfy_client.py
• comfy_cli/command/jobs.py
• comfy_cli/command/models/search.py
• comfy_cli/command/run/execution.py
• comfy_cli/command/run/preflight.py
• comfy_cli/command/templates.py
• comfy_cli/command/transfer.py
• comfy_cli/command/workflow.py
• comfy_cli/cql/engine.py
• comfy_cli/cql/loader.py
• comfy_cli/http.py
• tests/comfy_cli/command/models/test_search.py
• tests/comfy_cli/command/test_pretty_print_sanitize.py
• tests/comfy_cli/command/test_run.py
• tests/comfy_cli/command/test_run_json.py
• tests/comfy_cli/command/test_workflow_saved.py
• tests/comfy_cli/jobs/test_jobs.py
• tests/comfy_cli/test_http.py
• tests/comfy_cli/test_http_only_openers.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.

[mattmillerai]

Resolver pass — one real CI break found and fixed; it was not one of the pre-existing repo-wide failures called out above.

ruff_check was genuinely red on this branch. ruff check passed; ruff format --diff reported "1 file would be reformatted" on tests/comfy_cli/command/github/test_pr.py:182:

-        """A rate-limited 403 surfaces as GitHubRateLimitError, not a silent "no PR found\" """
+        """A rate-limited 403 surfaces as GitHubRateLimitError, not a silent "no PR found\""""

Root cause is the merge commit 517b9bc, not either parent — both origin/main and the pre-merge branch tip d8afd02 already carried the ruff-canonical form, so the conflict resolution hand-introduced a stale variant. Fixed in d321ff5 by restoring the byte-identical blob from main. Whitespace inside a docstring on a test this PR doesn't otherwise touch; no semantic change.

docs/DESIGN-uv-compile.md also reformats locally under ruff 0.16, but it's untouched by this PR and pre-existing on main — CI's ruff doesn't even pick it up (CI enumerates 246 files, local 263), so it's out of scope here.

Verification

• ruff check → All checks passed; ruff format --diff → clean across every file this PR touches.
• Full suite: 2837 passed, 37 skipped (uv run --frozen pytest tests/).
• Merge-queue check: no event=merge_group run exists for gh-readonly-queue/main/pr-530-*, so there's no hidden merged-result failure.
• main is fully merged into HEAD; PR is MERGEABLE with no conflicts.

Self-review of the diff. The one behavior-denying edge I chased is that build_opener() did install FileHandler/FTPHandler/DataHandler, so pinning to http(s) could have silently broken a real file:// path. It doesn't — every opener's URL is either scheme-hardcoded or already scheme-validated upstream:

• cql/loader.py — local files go through Path.read_text in _load_from_file, never the opener; _LOADER_OPENER only ever sees the hardcoded http://{host}:{port}/object_info.
• transfer.py _DOWNLOAD_OPENER — _assert_download_url (line 142) already rejects non-http(s) before the Request is built, and file:// outputs route to _copy_local_one, not _stream_http_one.
• cql/engine.py, comfy_client.py, oauth.py, templates.py, jobs.py, preflight.py — all build http(s):// URLs from a trusted base.

No capability is lost; the pinning is pure defense-in-depth. grep confirms zero remaining raw build_opener( / urllib.request.urlopen( call sites in comfy_cli/, so the migration is complete rather than partial.

Both review threads remain resolved (http(s) opener pinning landed in 651aa76; the _http_request oversize-read finding is tracked as its own follow-up).

[mattmillerai]

CI update — all 16 checks now pass, including the two that earlier comments on this PR wrote off as pre-existing infra breaks.

Check	Result
ruff_check	pass 13s
build (Run pytest)	pass 7m57s
test (Windows Specific Commands)	pass 5m17s
Multi-platform: ubuntu / macos / windows 3.10	pass 12m29s / 13m12s / 17m17s
GPU Runners (linux)	pass 10m13s
CodeQL, Analyze (actions/python), Socket ×2, codecov/project, cla-assistant	pass

Worth correcting the record from my two earlier comments: the tomlkit build failure and the Windows pydantic_core failure are no longer present, and ruff_check was never one of them. Those two upstream fixes have since landed on main, and this branch already carries them via the merge in 517b9bc — I confirmed the same workflows are green on main at the merge parent 6c88dd7 (Run pytest, Test CLI Tool on Multiple Platforms, Test CLI Tool on GPU runners all success). So the only genuinely red check on this branch was ruff_check, and it was caused by this PR's own merge commit, not by anything pre-existing — fixed in d321ff5.

State: base is main (not stacked), MERGEABLE with no conflicts, no merge_group run exists for gh-readonly-queue/main/pr-530-* so there's no hidden merged-result failure, both review threads resolved, and the two deferred findings are already tracked as their own follow-ups.

Ready for a human reviewer — I don't merge.

[skishore23]

One condition before merge: now that #597 is approved, build_authed_request should delegate to target_auth_headers instead of its inline ungated copy, else this re-opens the is_cloud gate at workflow.py/search.py/jobs.py. PR body also needs a refresh — the diff now touches the five files the acceptance criteria list as untouched.

[bigcat88]

This PR currently conflicts with main — GitHub reports mergeable: CONFLICTING, so it needs a rebase before I can review it and I'm skipping it in the current review sweep.

Please rebase (or merge main in) and I'll pick it up on the next pass. main moved a fair bit in the last day, including #614 (ANSI sanitisation across the pretty-print call sites) and #628 (the duplicate server_died error-code fix that had main red), so a refresh may also clear unrelated CI noise on this branch.

[mattmillerai]

Resolver pass — merge conflict with main resolved + the review condition addressed.

Merge conflict (main had moved). main landed its own parallel refactor of comfy_cli/http.py (#597, target_auth_headers) while this branch was in flight, conflicting in http.py, jobs.py, models/search.py, transfer.py, workflow.py, and test_http.py. Resolved by keeping both: header-building now goes through target_auth_headers (main's), request-opening goes through this PR's shared http(s)-only, no-redirect opener (build_http_only_opener/_AUTHED_OPENER). transfer.py/cql/engine.py already combined both cleanly (auto-merged); jobs.py/search.py/workflow.py needed the conflicting _authed_request/inline-header helpers dropped in favor of the already-migrated authed_urlopen call sites.

@skishore23's condition — fixed. build_authed_request did build its header inline instead of via target_auth_headers, which would have re-opened the is_cloud gate (a local Target with a stray token could get a credential header attached through this path). Now delegates to target_auth_headers, so it shares the one gate every other call site uses. Added a regression test (test_no_auth_header_for_local_target_even_with_stray_creds) that fails on the old inline version and passes now.

Also fixed: the merge exposed a real semantic break — main added tests/comfy_cli/command/test_pretty_print_sanitize.py (ANSI-sanitization suite, #614) after this branch had already migrated /prompt submission off request.urlopen() onto no_redirect_urlopen. Its mock patched the old call site, so post-merge it was silently making a real (failing) connection instead of hitting the mock. Repointed at comfy_cli.command.run.execution.no_redirect_urlopen.

PR body refreshed to match (the "not gated on is_cloud" line was accurate for the pre-merge version but wrong now).

Verification: full suite green — 3474 passed, 37 skipped (uv run pytest tests/). ruff check/ruff format --check clean on every touched file (17 pre-existing UP038 + 1 pre-existing docstring reformat, both byte-identical to origin/main, untouched here). No merge-queue run exists yet for this PR (never got that far due to the conflict).

Ready for a human reviewer — I don't merge.

[mattmillerai]

@skishore23 addressed — see above.