refactor(http): shared is_cloud-gated target_auth_headers builder; migrate 5 call sites (BE-4362) - #597
refactor(http): shared is_cloud-gated target_auth_headers builder; migrate 5 call sites (BE-4362)#597mattmillerai wants to merge 2 commits into
Conversation
…grate 5 call sites (BE-4362) Converge the five hand-rolled non-client copies of the Target auth-header policy onto one shared, strictest-variant builder in comfy_cli/http.py. The gate direction is security-sensitive: attach NOTHING unless target.is_cloud. This is behavior-preserving today (local Targets are constructed with no credentials) while closing the latent path where a stray credential on a local target would be sent to a plaintext server by the previously-ungated workflow/search/jobs sites. Migrated: workflow._authed_request, models/search._authed_request, jobs._cloud_cancel (inline), transfer._auth_headers (re-export), and cql/engine._load_from_target (inline). comfy_client.py's superset path (safe-URL assertion + 401 refresh-retry) is intentionally left as-is.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 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)
skishore23
left a comment
There was a problem hiding this comment.
LGTM — land before #530
ELI-5
Five different places in the code each wrote their own little "attach the login token to this request" snippet. They'd drifted: three of them attached the token to any target — even a plain-HTTP local ComfyUI — instead of only to Comfy Cloud. This PR replaces all five copies with one shared helper,
target_auth_headers(target), that attaches credentials only whentarget.is_cloud. Same behavior in practice today (local targets are built with no credentials), but a stray token on a local target can now never leak to a plaintext server.What changed
Adds
target_auth_headers(target) -> dict[str, str]tocomfy_cli/http.py— the strictest variant (promoted verbatim fromtransfer._auth_headers): it returns{}unlesstarget.is_cloud, thenX-API-Key(api_key) taking precedence overAuthorization: Bearer(auth_token). This is the same defense-in-depth gatecomfy_client.Clientalready uses.Migrated the five hand-rolled non-client copies onto it:
command/workflow.py_authed_requestcommand/models/search.py_authed_requestcommand/jobs.py_cloud_cancel(inline)command/transfer.py_auth_headersfrom comfy_cli.http import target_auth_headers as _auth_headers(was already gated — byte-identical)cql/engine.py_load_from_target(inline)Left untouched by design:
comfy_client.py:264-268(superset path with safe-URL assertion + 401 refresh-retry), and every HTTPError→envelope mapper / capped-reader (a separate follow-up ticket). Non-Target header sites (generate/client.py's bare-api_keybuilder, GitHub/Civitai token headers ininstall.py/models.py) are out of scope — they don't route aTarget.Security note
The gate direction is the point: attach nothing unless
target.is_cloud. The three previously-ungated sites (workflow/search/jobs) attached credentials whenever they were present, regardless of target kind. Behavior is preserved today becauseresolve_targetbuilds local Targets withauth_token=Noneand noapi_key(target.py:118-127), so no observable change — but the latent leak path (a stray credential on a local target → plaintext server) is now closed. This also means local/userdatacalls inworkflow.pygo from "attach if creds present" to "never attach," which_userdata_request's docstring already described as a no-op.Tests
tests/comfy_cli/test_http.py, including the security property: a local Target with bothauth_tokenandapi_keyset →target_auth_headersreturns{}; plus cloud api-key-only, cloud auth-token-only, and cloud-both (api_key wins).test_workflow_saved.py,models/test_search.py,jobs/test_jobs.py,test_transfer_{download,redirect,upload}.py, and thecql/suite (135 passed+248 passed, 1 skipped). Their target fixtures are cloud-kind, so headers still attach.ruff check+ruff format --checkclean on all touched files.Phase 1 of BE-4350.