refactor(auth): dedupe the last target auth-header selection onto the shared helper (BE-3275)

[mattmillerai]

ELI-5

Two ways to prove who you are to Comfy Cloud: an API key (sent as X-API-Key) or an OAuth token (sent as Authorization: Bearer …). The code that decides which one to send used to be copy-pasted in several places. BE-3274 moved most of them onto one shared helper; this PR moves the last one — the unified Client in comfy_client.py — so that decision now lives in exactly one function. Nothing about what gets sent over the wire changes.

What

Phase 2 of the BE-3264 dedup (the security fix itself landed in phase 1, #530). Client._request was the last site still hand-rolling the api_key → X-API-Key / auth_token → Bearer selection; it now delegates to http.target_auth_headers, which already carries the is_cloud gate and is what command/transfer.py and cql/engine.py use. Comfy-Usage-Source, Content-Type, Accept and _OPENER are untouched.

Behavior-preservation argument (the one real risk)

The old block checked auth_token first (Bearer-first); the shared helper is api_key-first. Flagging this explicitly because it is the only line in the diff that could change a request.

It cannot, because a Target never carries both credentials:

• resolve_target (comfy_cli/target.py) is the only Target(...) construction in comfy_cli/ (git grep 'Target(' — the other hits are unrelated enums), and it derives both fields from a single resolved credential: cred = resolve_cloud_credential(...), then token = cred.value if cred.kind == "oauth" else None / api_key = cred.value if cred.kind == "api_key" else None. Exactly one is non-None by construction.
• The only mutation of either field anywhere in comfy_cli/ is the OAuth-refresh object.__setattr__(self.target, "auth_token", …) in comfy_client.py, which is gated on auth_token already being set — so it cannot introduce an api_key alongside it.

That is the same invariant the replaced block's own comment asserted, and the local-target leak gate is unchanged (it moved into the helper, which is where the other call sites already get it).

Tests

• tests/comfy_cli/test_http.py — added the missing target_auth_headers case: a cloud Target with no credential returns {}. (api_key-precedence, Bearer-only and the local-with-stray-creds cases were already covered by fix: route authed urllib paths through a shared NoRedirectHandler opener (BE-3274) #530.)
• tests/comfy_cli/cloud/test_client.py
  • test_cloud_oauth_wins_over_api_key_when_both_set → renamed test_cloud_both_credentials_uses_shared_header_selection and updated to the unified api_key-first header. This is the one test flipped by the precedence change and the state it exercises is unreachable in production per the argument above. Its extra_data assertions are unchanged (see judgment call 2).
  • Added test_local_target_with_stray_credentials_sends_no_auth_header — pins the leak guard at the call site, not just in the helper: a local Target carrying stray credentials sends neither header. _assert_safe_url only covers cloud targets, so this is_cloud gate is the whole defense there.

Full suite: uv run pytest → 3501 passed, 37 skipped. ruff check / ruff format --check clean on all four touched files. (Repo-wide ruff check reports 17 pre-existing UP038 findings and one pre-existing reformat in tests/comfy_cli/command/github/test_pr.py, all untouched by this PR and present on main — likely local-vs-CI ruff version drift.)

Judgment calls

1. Kept the existing helper name/signature. The ticket specified adding auth_headers(target, *, cloud_only: bool = False); phase 1 (fix: route authed urllib paths through a shared NoRedirectHandler opener (BE-3274) #530) had already landed the equivalent as target_auth_headers(target), unconditionally cloud-gated. All call sites want the cloud_only=True behavior, so adding a cloud_only parameter would mean introducing an unused mode whose default is the credential-leaking one. Kept the existing always-gated helper instead — the ticket's "keep whichever churns less" applied to the whole item.
2. Left the extra_data body path alone. submit_prompt injects the partner-API credential into the request body with its own auth_token-first ordering. The ticket scoped this change to header selection, so it is unchanged — meaning for a hypothetical both-credentials Target the header would be X-API-Key while the body carried auth_token_comfy_org. That state is unreachable (same argument as above), and the test docstring says so explicitly rather than leaving it to be read as an intended contract. Unifying the body path would be a separate, larger change touching partner-API behavior.
3. Acceptance criterion 1 is met in substance, not verbatim. git grep -n 'X-API-Key' comfy_cli/ still returns matches beyond http.py + transfer's _AUTH_HEADERS_TO_STRIP, all pre-existing and none of them target-credential header selection: three user-facing rprint strings in cloud/command.py, doc comments in credentials.py and target.py, and command/generate/client.py, which has a different credential model (a bare API-key string that is routed to X-API-Key or Bearer by inspecting the key's shape — Firebase JWT vs comfyui- key — with no Target involved). That file was not one of the ticket's three sites and folding it into the target helper would be a behavior change, not a dedup. What the criterion was protecting is satisfied: after this PR, no Target-credential header selection exists outside comfy_cli/http.py. cql/loader.py and cloud/oauth.py confirmed to have none and are untouched.
4. Not a capability-denying diff. The negative-claim check applies loosely because a test assertion flipped to "Authorization" not in req.headers, so: the OAuth/Bearer capability is intact and still covered by test_posts_with_bearer_to_prefixed_url and the 401-refresh test, both passing. The only state where Bearer is no longer chosen is the both-credentials one, which I went looking for in production code and could not construct (the git grep 'Target(' / __setattr__ evidence above). No path is denied to any user.

Note for the reviewer

One unreachable-state wrinkle worth knowing about: the 401 auto-refresh in _request is keyed on self.target.auth_token. If a both-credentials Target could exist, a 401 would now refresh the OAuth token and retry with X-API-Key — i.e. the refresh would be wasted work before the error surfaced, rather than incorrect. Harmless, and unreachable, but it is the one downstream interaction with the precedence change.

[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: 25 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: 36ca297a-7ffd-470f-9242-82504baec67e

📥 Commits

Reviewing files that changed from the base of the PR and between 4f2e352 and f591adc.

📒 Files selected for processing (4)

• comfy_cli/comfy_client.py
• comfy_cli/http.py
• tests/comfy_cli/cloud/test_client.py
• tests/comfy_cli/test_http.py

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 5 finding(s).

Severity	Count
🟡 Medium	2
🟢 Low	2
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[bigcat88]

Approving. The precedence flip is the only thing in this diff that could change a request, and you flagged it — so I went and tried to break the invariant it rests on, then checked what actually goes on the wire.

Wire-level: only the unreachable state differs

Recording server, Client._request driven directly against four Target shapes:

Target	main	this branch
cloud, api_key only	X-Api-Key: K123	X-Api-Key: K123
cloud, auth_token only	Authorization: Bearer T456	Authorization: Bearer T456
cloud, both (synthetic)	Authorization: Bearer T456	X-Api-Key: K123
local, stray both	{}	{}

Every reachable state is byte-identical; the leak gate still drops both credentials on a local target.

The invariant holds — and it's enforced more strongly than your description claims

An AST sweep of comfy_cli/ for Target(...) constructions, object.__setattr__ calls, and direct .api_key / .auth_token assignments:

• Two Target(...) constructions, not one — target.py:99 (cloud) and target.py:118 (local). The cloud one derives both fields from a single cred, so exactly one is non-None by construction; the local one passes auth_token=None, no api_key, and is is_cloud-gated anyway. Minor correction to "the only Target(...) construction", with no impact on the conclusion.
• One object.__setattr__, at comfy_client.py:183, exactly as you said.
• The only direct assignment hit is execution.py:116 self.api_key = api_key, which is a run-execution object, not a Target.

Your "Note for the reviewer" is more pessimistic than reality. You describe a both-credentials Target where a 401 would refresh OAuth and then retry with X-API-Key — wasted work. That can't happen: _try_refresh_token bails twice before reaching the __setattr__:

if not self.target.is_cloud or not self.target.auth_token:
    return False
...
if self.target.api_key:
    return False        # <- an api_key Target never refreshes at all

Both guards are pre-existing on main and untouched here, so the refresh path can neither create nor operate on a both-credentials Target. The invariant is enforced at construction and at the only mutation site.

Judgment calls

Agreed on keeping target_auth_headers over the ticket's auth_headers(target, *, cloud_only=False) — adding a parameter whose default is the credential-leaking mode, with no caller wanting it, would be strictly worse than the always-gated helper. That's the right reading of "keep whichever churns less".

Leaving the extra_data body path alone is also right for scope, and I like that the test docstring records why rather than leaving the asymmetry to be misread as intent.

test_local_target_with_stray_credentials_sends_no_auth_header is the test I'd have asked for: _assert_safe_url only covers cloud targets, so the is_cloud gate is the entire defense on the local path, and pinning it at the call site rather than only in the helper is what makes the delegation safe to repeat.

One correction to the description

You note "one pre-existing reformat in tests/comfy_cli/command/github/test_pr.py … present on main — likely local-vs-CI ruff version drift." It isn't pre-existing — main is clean under CI's pinned 0.15.15 (270 files already formatted). I tracked it down: ruff 0.14.x rewrites that docstring into a form 0.15.15 rejects, while 0.15.x leaves it alone. Your local ruff is 0.14.x and is generating that diff itself. It already turned #596's CI red the same way (details in a comment there). Pinning locally to ruff==0.15.15 will stop it recurring.

Full suite green on this branch merged with current main; ruff check + ruff format --diff clean under 0.15.15.