GTM-93: route desktop auth through Cloud login#1150
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 49 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Firebase bridge sign-in flow is switched from a loopback bridge-page ( ChangesCloud Login Callback Bridge
Sequence DiagramsequenceDiagram
rect rgba(135, 206, 250, 0.5)
note over handleFirebasePopup: Desktop App (Electron)
participant handleFirebasePopup as handleFirebasePopup
participant startCloudLoginCallbackServer as startCloudLoginCallbackServer
end
rect rgba(255, 200, 100, 0.5)
note over CloudLoginPage: Browser / WebView
participant CloudLoginPage as Cloud Login Page
end
participant BridgeHandle as BridgeHandle
handleFirebasePopup->>handleFirebasePopup: generateDesktopLoginState() → state nonce
handleFirebasePopup->>startCloudLoginCallbackServer: start server with { state }
startCloudLoginCallbackServer-->>handleFirebasePopup: handle.url (loopback callback URL)
handleFirebasePopup->>handleFirebasePopup: buildCloudDesktopLoginUrl(handle.url, state)
handleFirebasePopup->>CloudLoginPage: open final Cloud login URL
CloudLoginPage->>startCloudLoginCallbackServer: POST /callback { state, user, apiKey }
startCloudLoginCallbackServer->>startCloudLoginCallbackServer: validate state + payload
startCloudLoginCallbackServer-->>CloudLoginPage: 204 No Content
startCloudLoginCallbackServer->>BridgeHandle: resolve({ user, apiKey })
BridgeHandle-->>handleFirebasePopup: signInPromise resolves → inject IndexedDB + focus window
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af7bd8ada3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/firebaseBridge/index.ts`:
- Around line 176-197: The login URL origin determined by getCloudLoginOrigin()
can target any protocol-matching origin from the current WebContents, but the
callback server only accepts callbacks from cloud.comfy.org or loopback
addresses, causing CORS failures when other origins are used. Modify
getCloudLoginOrigin() to only return the current origin if it is a loopback
address (localhost, 127.0.0.1, etc.), otherwise always return the production
default of https://cloud.comfy.org. This ensures the buildCloudDesktopLoginUrl()
function always targets an origin that the callback server will accept, keeping
the login URL origin and callback CORS policy in sync.
In `@src/main/auth/firebaseBridge/server.test.ts`:
- Around line 132-195: The test file is missing test cases that verify the CORS
boundary for disallowed origins. Add two new test cases after the existing
tests: one for POST requests and one for OPTIONS requests from a disallowed
origin (use something like 'https://untrusted.example.com'). Both test cases
should call startCloudLoginCallbackServer with the same pattern as existing
tests, make requests with the untrusted origin, and assert that the response
status is 403 and includes appropriate CORS rejection headers. This ensures the
CORS security gate does not regress silently.
In `@src/main/auth/firebaseBridge/server.ts`:
- Around line 303-350: The startCloudLoginCallbackServer function contains
boilerplate code for managing server lifecycle and promise resolution handling
(the close function, finishWithError, finishWithSuccess, and related promise
setup logic) that is likely duplicated elsewhere in the file around lines
365-431. Extract these common patterns into shared helper functions or utilities
that can be reused across both auth paths. This will prevent the lifecycle
management and request handling logic from diverging between different
implementations. Focus on extracting the server cleanup logic, the promise
resolution/rejection patterns, and the timeout handling into reusable helpers
that both the startCloudLoginCallbackServer function and other similar auth path
handlers can leverage.
- Around line 319-330: The close() function shuts down the server and timer but
does not settle the signInPromise, leaving pending sign-in operations
unresolved. Locate the signInPromise variable in the same file and modify the
close() function to reject or resolve this promise before setting server to
null. This ensures that any in-flight authentication calls are properly
terminated when the bridge is torn down.
🪄 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: af3eae95-4071-4a3b-bdd9-4f3d3c3f012b
📒 Files selected for processing (4)
src/main/auth/firebaseBridge/copyLinkBanner.tssrc/main/auth/firebaseBridge/index.tssrc/main/auth/firebaseBridge/server.test.tssrc/main/auth/firebaseBridge/server.ts
|
I have read and agree to the Contributor License Agreement |
|
DO NOT MERGE UNTIL FE PR IS MERGED |
There was a problem hiding this comment.
Actionable comments posted: 2
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/server.ts (1)
146-157: 🩺 Stability & Availability | 🔵 TrivialThis asymmetry is intentional and safe — no change needed, but consider a clarifying comment.
The concern is valid: a local process can POST garbage to
/callbackwithout thestatenonce and abort the in-flight login, sincereadJsonBodyis the only code path that throws into this catch block (lines 204, 146–157). However, the design distinguishes between recoverable and unrecoverable errors:
- Invalid
state(line 206–211) returns 403 and leavessignInPromisepending — stray callbacks are tolerated.- Malformed/oversized body (line 204) throws, calls
finishWithError, and aborts — corrupted data can't be salvaged.Since the orchestrator's finally block (index.ts, 273–279) unconditionally closes the bridge on any rejection, the abort is safe and cleanup always happens. The asymmetry doesn't leak resources; it prevents propagating corrupted callback data where it won't float around — you might say the design's promise is kept! If this intentional behavior isn't obvious, a one-line comment explaining why invalid-state is tolerated but read errors abort would save a future reviewer from spiraling.
🤖 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/server.ts` around lines 146 - 157, Add a clarifying comment to explain the intentional asymmetry in error handling within the catch block that wraps handleRequest. The comment should explain that invalid state errors (returned as 403 in the validation logic) are tolerated to allow stray callbacks to be handled gracefully and keep signInPromise pending, while read errors from readJsonBody throw and abort because corrupted data cannot be salvaged. Also note that the orchestrator's finally block ensures cleanup always happens regardless of which error type occurs, making this asymmetry safe and preventing resource leaks.
🤖 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/firebaseBridge/server.test.ts`:
- Around line 60-76: The test file contains repeated boilerplate code where
tests call startCloudLoginCallbackServer, wrap the test logic in a try block,
and call closeExpectingRejection in a finally block. Create a test helper
function that accepts a callback function and handles this setup/teardown
pattern internally, then refactor all tests (including those around lines
195-242 and 244-280) to use this helper instead of repeating the
startCloudLoginCallbackServer initialization, try/finally structure, and
closeExpectingRejection cleanup pattern. This will centralize the server
lifecycle management and make the test code cleaner and easier to maintain.
- Around line 226-238: The test named "rejects matching-state callbacks without
a string apiKey" is not actually testing a non-string apiKey value as its name
suggests. In the request body within the startCloudLoginCallbackServer test, add
an apiKey field with a non-string value (such as a number or object) to the JSON
payload instead of omitting it entirely, so the test properly exercises the
validation logic that checks for string apiKey values.
---
Outside diff comments:
In `@src/main/auth/firebaseBridge/server.ts`:
- Around line 146-157: Add a clarifying comment to explain the intentional
asymmetry in error handling within the catch block that wraps handleRequest. The
comment should explain that invalid state errors (returned as 403 in the
validation logic) are tolerated to allow stray callbacks to be handled
gracefully and keep signInPromise pending, while read errors from readJsonBody
throw and abort because corrupted data cannot be salvaged. Also note that the
orchestrator's finally block ensures cleanup always happens regardless of which
error type occurs, making this asymmetry safe and preventing resource leaks.
🪄 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: 87de1873-9bf1-4b51-8e12-29d4d985bdd9
📒 Files selected for processing (8)
src/main/auth/firebaseBridge/bridgeHtml.tssrc/main/auth/firebaseBridge/config.test.tssrc/main/auth/firebaseBridge/config.tssrc/main/auth/firebaseBridge/index.tssrc/main/auth/firebaseBridge/oauth.tssrc/main/auth/firebaseBridge/origins.tssrc/main/auth/firebaseBridge/server.test.tssrc/main/auth/firebaseBridge/server.ts
💤 Files with no reviewable changes (4)
- src/main/auth/firebaseBridge/oauth.ts
- src/main/auth/firebaseBridge/config.ts
- src/main/auth/firebaseBridge/config.test.ts
- src/main/auth/firebaseBridge/bridgeHtml.ts
|
Does this PR make sure to keep things working when ComfyUI is not using a frontend comfy version that has the changes in the frontend PR? |
This doesn’t depend on the ComfyUI frontend bundled with Desktop. It depends on the Cloud login page deployment. Desktop opens cloud.comfy.org for the handoff, so the required version skew check is Cloud frontend #12983 deployed before Desktop #1150 ships. As of right now the deployment hasn't happened yet |
3 participants
Summary
Links
Test plan