feat(auth): sign in via cloud login page with short-lived desktop login codes (GTM-93)#1222
Merged
Merged
Conversation
…in codes (GTM-93)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PKCE-based desktop login code sign-in flow, REST clients for code creation and exchange, custom-token persistence, origin resolution, abortable orchestration, and Firebase bridge integration. ChangesDesktop Login Code Sign-In
Sequence Diagram(s)sequenceDiagram
participant Renderer as ComfyContents
participant Bridge as handleFirebasePopup
participant Flow as signInViaDesktopLoginCode
participant Client as DesktopLoginCodeClient
participant Cloud as Cloud Login Page
participant Identity as Identity Toolkit
Renderer->>Bridge: intercepted auth URL
Bridge->>Flow: start desktop login code flow
Flow->>Client: createDesktopLoginCode(request)
Client-->>Flow: grant
Flow->>Cloud: open login URL with opaque code
loop poll until redemption
Flow->>Client: exchangeDesktopLoginCode(code, verifier)
Client-->>Flow: pending or custom token
end
Flow->>Identity: signInWithCustomToken(custom token)
Identity-->>Flow: ID and refresh tokens
Flow->>Identity: lookupAccount(ID token)
Identity-->>Flow: account details
Flow-->>Renderer: bind user, inject state, restore window
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t timings Review follow-ups on the desktop-login-code flow: - signInWithCustomToken/lookupAccount now go through the same timeout-guarded postJson as the login-code endpoints, so a hung identitytoolkit call fails the flow instead of stalling it forever with the banner up and no sign_in_failed. - createDesktopLoginCode rejects grants with expires_in or poll_interval <= 0 (tight-poll spin / instant expiry) while the legacy-bridge fallback is still available.
benceruleanlu
marked this pull request as ready for review
July 6, 2026 02:06
benceruleanlu
marked this pull request as draft
July 6, 2026 02:06
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/auth/firebaseBridge/oauth.ts (1)
172-191: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract shared
providerUserInfo→PersistedProviderData[]mapping.This map (Lines 172-191) is near-identical to the one added in
customTokenSignIn.ts'sbuildPersistedUserFromCustomToken(Lines 96-103 there), differing only in the provider-id fallback (providerIdparam vs. hardcoded'firebase'). SinceassemblePersistedUserwas just centralized as "single source of truth" for this contract, consider pulling the mapping into a shared helper alongside it (e.g.mapProviderUserInfo(list, fallbackProviderId, fallbackUid)) so the two sign-in paths can't silently diverge.♻️ Suggested shared helper
+export function mapProviderUserInfo( + list: ProviderUserInfo[] | undefined, + fallbackProviderId: string, + fallbackUid: string, +): PersistedProviderData[] { + return (list ?? []).map((p) => ({ + providerId: p.providerId ?? fallbackProviderId, + uid: p.rawId ?? fallbackUid, + displayName: p.displayName ?? null, + email: p.email ?? null, + phoneNumber: null, + photoURL: p.photoUrl ?? null, + })) +}Then in
buildPersistedUser:- const providerData: PersistedProviderData[] = - resp.providerUserInfo && resp.providerUserInfo.length > 0 - ? resp.providerUserInfo.map((p) => ({...})) - : [{...}] + const providerData: PersistedProviderData[] = + resp.providerUserInfo && resp.providerUserInfo.length > 0 + ? mapProviderUserInfo(resp.providerUserInfo, providerId, resp.localId) + : [{ providerId, uid: resp.federatedId ?? resp.rawId ?? resp.localId, ... }]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/auth/firebaseBridge/oauth.ts` around lines 172 - 191, The `providerUserInfo` to `PersistedProviderData[]` conversion in `buildPersistedUser` duplicates the mapping already used in `buildPersistedUserFromCustomToken`, so the two sign-in paths can drift. Extract this logic into a shared helper near `assemblePersistedUser` (for example, a mapper that accepts the provider list plus fallback providerId and uid values), then update both `oauth.ts` and `customTokenSignIn.ts` to use it. Keep the existing fallback behavior intact, including the `providerId` parameter here and the hardcoded `'firebase'` case there.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/auth/desktopLoginCode/client.ts`:
- Around line 96-179: There is duplicated status-classification logic in
createDesktopLoginCode and exchangeDesktopLoginCode where retryable is computed
inline, which should be centralized to avoid divergence. Extract the shared 5xx
retryability check into a small helper or shared utility near
DesktopLoginCodeError handling, and keep the 404 featureMissing branch in
createDesktopLoginCode as the only special case. Use the existing
createDesktopLoginCode and exchangeDesktopLoginCode symbols to update both call
sites consistently.
- Around line 164-179: The exchange parsing in desktopLoginCode/client.ts treats
an unexpected HTTP 200 payload as terminal, which can break the poll loop on a
transient malformed response. Update the DesktopLoginCodeError thrown in the
response handling logic after resp.json() to mark this specific “unexpected
payload” path as retryable, while keeping the explicit pending/complete cases
and the 403/404 terminal cases unchanged.
In `@src/main/auth/desktopLoginCode/customTokenSignIn.test.ts`:
- Around line 117-131: Add a test case in customTokenSignIn.test.ts for a
malformed expiresIn value so the fallback path is covered. Extend the existing
buildPersistedUserFromCustomToken coverage by using a non-numeric expiresIn in
the token input and asserting the resulting user still gets sensible defaults,
matching the new guard logic in customTokenSignIn.ts. Keep the test alongside
the current sparse-lookup case so the behavior of
buildPersistedUserFromCustomToken remains locked in.
In `@src/main/auth/desktopLoginCode/customTokenSignIn.ts`:
- Around line 38-81: Both signInWithCustomToken and lookupAccount duplicate the
same non-OK response handling pattern, so extract that logic into a shared
helper near these functions and use it for both postJson calls. Keep the helper
responsible for checking resp.ok, reading resp.text() with a fallback, and
throwing the formatted error so the status/message format stays consistent and
won’t diverge between signInWithCustomToken and lookupAccount.
- Around line 92-94: Guard the token expiration calculation in customTokenSignIn
by validating signIn.expiresIn before using it, since Number(signIn.expiresIn ||
'3600') can produce NaN for truthy non-numeric strings and break expirationTime.
Update the logic around expiresInSec and the related expirationTime assignment
to reject or replace invalid values with the default 3600, and keep the check
localized in customTokenSignIn so the session timing remains safe.
In `@src/main/auth/desktopLoginCode/index.ts`:
- Around line 234-239: The parent-window restore/show/focus logic is duplicated
between desktopLoginCode’s handling block and handleFirebasePopup in
firebaseBridge, so extract it into a shared helper and reuse it from both call
sites. Add a small utility like restoreParentWindow that accepts the
BrowserWindow, guards against missing/destroyed windows, restores if minimized,
then shows and focuses it, and replace the inline blocks in
desktopLoginCode/index.ts and firebaseBridge/index.ts with that helper to keep
behavior synchronized.
In `@src/main/auth/desktopLoginCode/origins.ts`:
- Around line 6-10: The isLoopbackHostname helper only matches localhost and the
compressed IPv6 loopback forms, so update it to also recognize fully expanded
IPv6 loopback hostnames such as 0:0:0:0:0:0:0:1. Adjust the normalization/check
logic in isLoopbackHostname in origins.ts so any equivalent loopback
representation is treated as loopback before falling back to the IPv4 127.*
check.
---
Outside diff comments:
In `@src/main/auth/firebaseBridge/oauth.ts`:
- Around line 172-191: The `providerUserInfo` to `PersistedProviderData[]`
conversion in `buildPersistedUser` duplicates the mapping already used in
`buildPersistedUserFromCustomToken`, so the two sign-in paths can drift. Extract
this logic into a shared helper near `assemblePersistedUser` (for example, a
mapper that accepts the provider list plus fallback providerId and uid values),
then update both `oauth.ts` and `customTokenSignIn.ts` to use it. Keep the
existing fallback behavior intact, including the `providerId` parameter here and
the hardcoded `'firebase'` case there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6fbdcb3-c4cf-4a1d-97f7-d95055a69517
📒 Files selected for processing (12)
src/main/auth/desktopLoginCode/client.test.tssrc/main/auth/desktopLoginCode/client.tssrc/main/auth/desktopLoginCode/customTokenSignIn.test.tssrc/main/auth/desktopLoginCode/customTokenSignIn.tssrc/main/auth/desktopLoginCode/index.test.tssrc/main/auth/desktopLoginCode/index.tssrc/main/auth/desktopLoginCode/origins.test.tssrc/main/auth/desktopLoginCode/origins.tssrc/main/auth/desktopLoginCode/pkce.test.tssrc/main/auth/desktopLoginCode/pkce.tssrc/main/auth/firebaseBridge/index.tssrc/main/auth/firebaseBridge/oauth.ts
benceruleanlu
marked this pull request as ready for review
July 10, 2026 02:22
pull Bot
pushed a commit
to Mu-L/ComfyUI_frontend
that referenced
this pull request
Jul 10, 2026
…titching (GTM-93) (Comfy-Org#13418) ## What Browser half of GTM-93 macOS web→desktop identity stitching: when the desktop app opens the system browser at cloud login with `?desktop_login_code=dlc_…`, the frontend redeems that code against the cloud backend once a Firebase session exists — after **explicit user approval**. Reworked on top of the preserved-query `stripAfterCapture` capability (Comfy-Org#13465): - The `DESKTOP_LOGIN` namespace opts into strip-on-capture: the code is stashed and removed from the URL **before any navigation completes**, so it never reaches history, `previousFullPath`, later guards, or telemetry — the hand-rolled URL scrubbing this PR previously carried (raw-string parser + three strip sites) is gone. - `desktopLoginRedemption.ts` is a plain module with a single export, `installDesktopLoginRedemption(router)`, installed once in `router.ts`'s cloud block (replaces six per-view/composable trigger sites). Redemption reads the code only from the stash: per-code state (approval + 2-attempt transient budget, so a second code gets its own approval and budget), approval dialog, `POST /api/auth/desktop-login-codes/redeem` with the raw Firebase ID token (backend route is Firebase-JWT-only), 10s fetch timeout. - Triggers: `router.afterEach` (the cloud auth guard settles Firebase init before navigations complete) plus a lazy watcher on `authStore.currentUser` for sessions that appear without a navigation (OAuth-resume error branch, dialog sign-in). One bounded in-page retry (5s) guarantees an approved sign-in always ends in a success or failure toast. - Terminal rejections (400/403/404/409/410) drop the code with an error toast; transient failures (401/5xx/timeout/network) retry once; budget exhaustion now surfaces a failure toast instead of dying silently. ## Why Windows stitches web→desktop at download time via installer stamping; macOS DMGs can't be stamped, so we stitch at login. The browser is where both halves meet: the existing `posthog.identify(uid)` merges the web anon person into the Firebase uid, and the backend emits `comfy.cloud.identity.login_attributed` (uid ↔ installation_id) at redeem. The desktop app polls the backend and receives a one-time custom token — no auth material posted to a desktop loopback server (the concern that stalled Comfy-Org#12983, which this supersedes). ## Security - **Approval dialog before redeem** — redemption mints the desktop a sign-in token for *your* account, so a lured click must not be enough (device-code phishing mitigation). Cancel clears the stash and does nothing. - Only the opaque single-use code ever appears in a URL; the tracker strips it on first sight, pre-navigation, and it is never logged. ## Testing Vitest, driven through a real router (createRouter/createMemoryHistory, no vue-router mocks) and the real preserved-query manager: capture/stash lifecycle, approval gate (no fetch before approve; decline/dismiss clears; per-code approval), Bearer/body shape, terminal-vs-transient statuses, timeout abort, bounded in-page retry + failure toast on budget exhaustion, per-code regressions (second code after success/decline/exhaustion redeems independently), auth-watcher trigger (session appearing without navigation), unauthenticated no-op, trigger coalescing. Typecheck/lint/format clean. Types are hand-written with a `TODO(@comfyorg/ingest-types)` — the generated types land automatically once the cloud PR merges and the type-gen workflow runs. ## Landing order 1. Cloud backend: Comfy-Org/cloud#4736 (until it ships, redemption never triggers — this PR is inert) 2. Comfy-Org#13465 preserved-query strip-on-capture (base of this PR) 3. Comfy-Org#13466 global-prompt FIFO queue (runtime dependency: the approval confirm must settle even if another prompt is open) 4. **This PR** 5. Desktop (activates the flow): Comfy-Org/Comfy-Desktop#1222 GTM-93 · Supersedes Comfy-Org#12983 --------- Co-authored-by: AustinMroz <austin@comfy.org>
# Conflicts: # src/main/auth/firebaseBridge/index.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/auth/desktopLoginCode/client.ts (1)
64-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat HTTP 429 as retryable.
isRetryableStatus()only covers 5xx responses, so a rate-limited exchange call is marked terminal and the poll loop stops instead of backing off.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/auth/desktopLoginCode/client.ts` around lines 64 - 66, Update isRetryableStatus() to also return true for HTTP status 429, while preserving retry behavior for all 5xx responses and terminal behavior for other statuses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/auth/desktopLoginCode/client.ts`:
- Around line 64-66: Update isRetryableStatus() to also return true for HTTP
status 429, while preserving retry behavior for all 5xx responses and terminal
behavior for other statuses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6f25228f-e407-483c-9224-5ddcbf969d6e
📒 Files selected for processing (7)
src/main/auth/desktopLoginCode/client.test.tssrc/main/auth/desktopLoginCode/client.tssrc/main/auth/desktopLoginCode/customTokenSignIn.test.tssrc/main/auth/desktopLoginCode/index.test.tssrc/main/auth/desktopLoginCode/testHelpers.tssrc/main/auth/firebaseBridge/index.test.tssrc/main/auth/firebaseBridge/index.ts
💤 Files with no reviewable changes (1)
- src/main/auth/desktopLoginCode/index.test.ts
This was referenced Jul 14, 2026
deepme987
approved these changes
Jul 14, 2026
This was referenced Jul 15, 2026
Member
Author
|
Considering just rebasing this on top of #1272 |
Member
Author
|
Review takes too long for the ideal scenario above. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review, don't merge until corresponding FE PRs are backported and live on cloud.
What
Desktop half of GTM-93 macOS web→desktop identity stitching: sign-in now runs through the real cloud.comfy.org login page with a short-lived, PKCE-bound desktop login code — no loopback callback, no credentials over localhost.
New
src/main/auth/desktopLoginCode/module, attempted first from the existinghandleFirebasePopupintercept:POST /api/auth/desktop-login-codes(installation_id=getDeviceId()machine hash, included only when telemetry consent is granted; the auth handoff works without it)./cloud/login?desktop_login_code=dlc_…(only the opaque code — neverinstallation_id— transits the URL), reusing the copy-link banner./exchange(interval + jitter, abortable singleton, one final attempt at deadline) until the signed-in browser session redeems the code with explicit user approval.accounts:signInWithCustomToken+accounts:lookup(main process has no Firebase SDK), assemble the persisted user via the sameassemblePersistedUsercontract as the OAuth path, then the unchanged IndexedDB inject +bindSignedInUser+ focus restore.sign_in_started {flow: 'desktop_login_code'}(only once the browser actually opens),comfy.desktop.identity.login_attributed {via: 'desktop_login_code'}on success,sign_in_failedwith error bucket otherwise.Hard fallback: if code creation fails (old backend, network, timeout) — or the deployment targets the dev Firebase project through a non-loopback origin — the legacy loopback bridge runs unchanged. Login can never get worse than today.
Why
Windows stitches web→desktop by stamping a download token into the installer; macOS DMGs are notarized and can't be stamped. Logging in on the real cloud page in the user's own browser lets the web
posthog.identify(uid)merge top-of-funnel anon activity into the Firebase uid, while the backend emitscomfy.cloud.identity.login_attributed(uid ↔ installation_id) at redeem — the login-time twin of the Windowsdownload_attributedevent. Bonus: passkeys/password-manager autofill/email+password all work, since the login UI is the real cloud page rather than the bridge's provider redirect.Supersedes #1150: same goal, but the browser never POSTs to a localhost server — the desktop pulls a one-time Firebase custom token with code + PKCE verifier (the token-over-loopback concern from that review is gone), so the fixed port 9876, CORS/PNA headers, and callback validation all disappear from this path.
Notes
oauth.ts's persisted-user assembly extracted into exportedassemblePersistedUser(byte-identical output; legacy path delegates) so both sign-in paths share theUser.toJSON()/stsTokenManagercontract that IndexedDB rehydration depends on.getDeviceId()(the machine hash) forinstallation_id, matching that PR's direction.Testing
90 tests in
src/main/auth(53 new): PKCE vectors, production-pinned/dev-only origin resolution, client status/body-timeout classification (pending/complete/terminal/retryable/timeout), custom-token REST chain + persisted-user contract, orchestrator (fallback-on-create-failure, consent gating, noinstallation_idin URLs, poll pending→complete, final-attempt-at-deadline, same-path and cross-flow supersession/abort races, dev-env guard, telemetry ordering). Full suite green; typecheck/lint clean.Landing order
signBlobfor custom-token minting)GTM-93 · Supersedes #1150
GTM-93 identity dependency (July 15 update)
This PR owns authentication and Firebase persistence injection; it is not the final PostHog identity authority. In the corrected stack, Frontend #13687 reports the post-reload Firebase state and Desktop #1272 identifies only after all live hosted windows agree. The legacy bindSignedInUser call becomes a compatibility no-op under #1272.
Do not release this PR until Frontend #13687 is deployed and Desktop #1272 is included in the same Desktop release. Deploy Router #34 afterward. The #1272 + #1222 synthetic stack is covered by GTM-277.
See GTM-93 for review and deployment order.