feat(telemetry): add Firebase auth consensus - #1306
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds Firebase auth-state reporting and process-wide consensus across hosted and loopback views. It introduces auth contracts, IndexedDB monitoring, trusted persistence, navigation-aware reconciliation, validated IPC, anonymous epoch handling, lifecycle wiring, and tests. ChangesFirebase auth identity consensus
Sequence Diagram(s)sequenceDiagram
participant LocalFirebaseAuthMonitor
participant ComfyPreload
participant registerTelemetryHandlers
participant firebaseAuthIdentity
participant telemetry
LocalFirebaseAuthMonitor->>ComfyPreload: emit Firebase auth state
ComfyPreload->>registerTelemetryHandlers: send telemetry:firebaseAuthState
registerTelemetryHandlers->>firebaseAuthIdentity: validate and report frame/state
firebaseAuthIdentity->>firebaseAuthIdentity: reconcile active reporters
firebaseAuthIdentity->>telemetry: apply consensus identity
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 |
d0d4bdc to
924bdbf
Compare
1e92d01 to
595e946
Compare
595e946 to
64c8a44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/flowShared.ts (1)
64-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard stale bindings before dereferencing
source.Current call sites pass
comfyContentsfrom live flows, but the helper should still rejectundefined/destroyedWebContentsbeforesource.getURL()—otherwise the quiet catch becomes a no-hop, no-bind dead end.🤖 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/flowShared.ts` around lines 64 - 77, Update bindSignedInUser to validate source before invoking bindMainVerifiedFirebaseUser or any source-dependent operation: reject undefined or destroyed WebContents, while preserving the existing UID and property-building flow for valid sources. Ensure stale sources exit safely rather than relying on the catch block.
🤖 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/lib/anonymousIdentity.test.ts`:
- Around line 85-94: The test should verify that the unmergeable epoch marker
survives rotatePersistedAnonymousDistinctId(). Add an assertion immediately
after the rotation confirming hasPersistedUnmergeableAnonymousEpoch() is true,
while preserving the existing assertion that
clearPersistedUnmergeableAnonymousEpoch() removes it.
In `@src/main/lib/firebaseAuthIdentity.test.ts`:
- Around line 34-38: Add a verifiedLocalRevocation.succeeds toggle to the
firebaseAuthIdentity test mock, mirroring verifiedLocalPersistence, and make
clearVerifiedLocalFirebaseUser return failure without deleting the user when the
toggle is false. Add a test covering failed revocation and assert the identity
is not re-bound afterward, exercising the fail-closed behavior in
firebaseAuthIdentity.
In `@src/main/lib/firebaseAuthIdentity.ts`:
- Around line 105-122: Update revokeAcceptedLocalAuthorization to inspect the
boolean result from clearVerifiedLocalFirebaseUser. When persistence fails,
retain a fail-closed pending/unmergeable state that prevents
refreshReporterAuthScope and subsequent restarts from restoring
localReportingAuthorized or including the reporter in consensus until revocation
succeeds.
---
Outside diff comments:
In `@src/main/auth/firebaseBridge/flowShared.ts`:
- Around line 64-77: Update bindSignedInUser to validate source before invoking
bindMainVerifiedFirebaseUser or any source-dependent operation: reject undefined
or destroyed WebContents, while preserving the existing UID and
property-building flow for valid sources. Ensure stale sources exit safely
rather than relying on the catch block.
🪄 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: 1c405d38-dc03-4b39-88ee-8a4fafce43a2
📒 Files selected for processing (26)
packages/comfyui-desktop-bridge-types/comfyDesktopBridge.d.tspackages/comfyui-desktop-bridge-types/package.jsonsrc/main/auth/firebaseBridge/flowShared.tssrc/main/host/attach.tssrc/main/host/createHostWindow.tssrc/main/lib/anonymousIdentity.test.tssrc/main/lib/anonymousIdentity.tssrc/main/lib/firebaseAuthIdentity.test.tssrc/main/lib/firebaseAuthIdentity.tssrc/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/opaqueIdentifier.test.tssrc/main/lib/opaqueIdentifier.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/trustedCloudUrl.test.tssrc/main/lib/trustedCloudUrl.tssrc/main/lib/verifiedLocalFirebaseAuth.test.tssrc/main/lib/verifiedLocalFirebaseAuth.tssrc/preload/api.tssrc/preload/comfyPreload.tssrc/preload/localFirebaseAuthMonitor.test.tssrc/preload/localFirebaseAuthMonitor.tssrc/renderer/src/lib/rendererBootstrap.tssrc/types/comfyDesktopBridge.tssrc/types/ipc.ts
💤 Files with no reviewable changes (2)
- src/preload/api.ts
- src/types/ipc.ts
| it('persists an unmergeable epoch across restarts until a clean rotation clears it', () => { | ||
| expect(persistAnonymousDistinctId(ANONYMOUS_ID_1)).toBe(true) | ||
| expect(persistUnmergeableAnonymousEpoch()).toBe(true) | ||
|
|
||
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(true) | ||
| expect(readPersistedAnonymousDistinctId()).toBe(ANONYMOUS_ID_1) | ||
| expect(rotatePersistedAnonymousDistinctId()).not.toBeNull() | ||
| expect(clearPersistedUnmergeableAnonymousEpoch()).toBe(true) | ||
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(false) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the marker survives the rotation the title promises.
The name says the epoch persists "until a clean rotation clears it", but nothing checks the marker is still set after rotatePersistedAnonymousDistinctId() — so a regression where rotation silently wipes the taint would slip past unnoticed.
🧪 Proposed assertion
expect(rotatePersistedAnonymousDistinctId()).not.toBeNull()
+ expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(true)
expect(clearPersistedUnmergeableAnonymousEpoch()).toBe(true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('persists an unmergeable epoch across restarts until a clean rotation clears it', () => { | |
| expect(persistAnonymousDistinctId(ANONYMOUS_ID_1)).toBe(true) | |
| expect(persistUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(readPersistedAnonymousDistinctId()).toBe(ANONYMOUS_ID_1) | |
| expect(rotatePersistedAnonymousDistinctId()).not.toBeNull() | |
| expect(clearPersistedUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(false) | |
| }) | |
| it('persists an unmergeable epoch across restarts until a clean rotation clears it', () => { | |
| expect(persistAnonymousDistinctId(ANONYMOUS_ID_1)).toBe(true) | |
| expect(persistUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(readPersistedAnonymousDistinctId()).toBe(ANONYMOUS_ID_1) | |
| expect(rotatePersistedAnonymousDistinctId()).not.toBeNull() | |
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(clearPersistedUnmergeableAnonymousEpoch()).toBe(true) | |
| expect(hasPersistedUnmergeableAnonymousEpoch()).toBe(false) | |
| }) |
🤖 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/lib/anonymousIdentity.test.ts` around lines 85 - 94, The test should
verify that the unmergeable epoch marker survives
rotatePersistedAnonymousDistinctId(). Add an assertion immediately after the
rotation confirming hasPersistedUnmergeableAnonymousEpoch() is true, while
preserving the existing assertion that clearPersistedUnmergeableAnonymousEpoch()
removes it.
| clearVerifiedLocalFirebaseUser: (origin: string) => { | ||
| verifiedLocalUsers.delete(origin) | ||
| return true | ||
| } | ||
| })) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a revoke-failure case to this mock's repertoire.
clearVerifiedLocalFirebaseUser always succeeds here, so the fail-closed behavior discussed on src/main/lib/firebaseAuthIdentity.ts Lines 105-122 has no coverage. A verifiedLocalRevocation.succeeds toggle (mirroring verifiedLocalPersistence) plus a test asserting no re-bind after a failed revoke would pin it down — one small mock, one big net.
🤖 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/lib/firebaseAuthIdentity.test.ts` around lines 34 - 38, Add a
verifiedLocalRevocation.succeeds toggle to the firebaseAuthIdentity test mock,
mirroring verifiedLocalPersistence, and make clearVerifiedLocalFirebaseUser
return failure without deleting the user when the toggle is false. Add a test
covering failed revocation and assert the identity is not re-bound afterward,
exercising the fail-closed behavior in firebaseAuthIdentity.
| function revokeAcceptedLocalAuthorization( | ||
| webContents: WebContents, | ||
| reporter: Reporter, | ||
| origin: string | null, | ||
| state: ComfyDesktop2FirebaseAuthState, | ||
| userMismatch: boolean | ||
| ): void { | ||
| if ( | ||
| !origin || | ||
| !isLoopbackOrigin(origin) || | ||
| (state.status !== 'signed_out' && !userMismatch) | ||
| ) | ||
| return | ||
| clearVerifiedLocalFirebaseUser(origin) | ||
| reporter.localReportingAuthorized = false | ||
| reporter.localExpectedUserId = null | ||
| mainVerifiedStates.delete(webContents) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Persistence failure here silently fails open, not closed.
clearVerifiedLocalFirebaseUser(origin) returns a boolean that is dropped on the floor. If the write/unlink fails, the on-disk binding survives while only the in-memory flags are cleared — and the very next main-frame commit calls refreshReporterAuthScope (Line 98), which re-reads that stale binding and hands localReportingAuthorized straight back to an origin whose trust was just revoked (same after a restart). That contradicts the fail-closed intent for persistence failures.
Suggest treating a failed revoke as an unmergeable/pending situation rather than a shrug — a revoke that doesn't stick is a trust trick.
🔒 Sketch: keep the reporter out of consensus when revocation can't persist
): void {
if (
!origin ||
!isLoopbackOrigin(origin) ||
(state.status !== 'signed_out' && !userMismatch)
)
return
- clearVerifiedLocalFirebaseUser(origin)
+ const revoked = clearVerifiedLocalFirebaseUser(origin)
reporter.localReportingAuthorized = false
reporter.localExpectedUserId = null
mainVerifiedStates.delete(webContents)
+ if (!revoked) {
+ // The stale binding would re-authorize this origin on the next commit.
+ reporter.eligible = false
+ reporter.active = false
+ reporter.state = { status: 'pending' }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function revokeAcceptedLocalAuthorization( | |
| webContents: WebContents, | |
| reporter: Reporter, | |
| origin: string | null, | |
| state: ComfyDesktop2FirebaseAuthState, | |
| userMismatch: boolean | |
| ): void { | |
| if ( | |
| !origin || | |
| !isLoopbackOrigin(origin) || | |
| (state.status !== 'signed_out' && !userMismatch) | |
| ) | |
| return | |
| clearVerifiedLocalFirebaseUser(origin) | |
| reporter.localReportingAuthorized = false | |
| reporter.localExpectedUserId = null | |
| mainVerifiedStates.delete(webContents) | |
| } | |
| function revokeAcceptedLocalAuthorization( | |
| webContents: WebContents, | |
| reporter: Reporter, | |
| origin: string | null, | |
| state: ComfyDesktop2FirebaseAuthState, | |
| userMismatch: boolean | |
| ): void { | |
| if ( | |
| !origin || | |
| !isLoopbackOrigin(origin) || | |
| (state.status !== 'signed_out' && !userMismatch) | |
| ) | |
| return | |
| const revoked = clearVerifiedLocalFirebaseUser(origin) | |
| reporter.localReportingAuthorized = false | |
| reporter.localExpectedUserId = null | |
| mainVerifiedStates.delete(webContents) | |
| if (!revoked) { | |
| // The stale binding would re-authorize this origin on the next commit. | |
| reporter.eligible = false | |
| reporter.active = false | |
| reporter.state = { status: 'pending' } | |
| } | |
| } |
🤖 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/lib/firebaseAuthIdentity.ts` around lines 105 - 122, Update
revokeAcceptedLocalAuthorization to inspect the boolean result from
clearVerifiedLocalFirebaseUser. When persistence fails, retain a fail-closed
pending/unmergeable state that prevents refreshReporterAuthScope and subsequent
restarts from restoring localReportingAuthorized or including the reporter in
consensus until revocation succeeds.
… binds The no-source branch of bindMainVerifiedFirebaseUser had no callers — every verified flow binds through a hosted view. Dropping it keeps all main-verified binds on the consensus path.
deepme987
left a comment
deepme987
left a comment
There was a problem hiding this comment.
Agent Review — focused on the trust core (the "can an untrusted frame spoof a trusted identity?" question). Excellent, security-critical work; the answer is no at every layer. Nothing blocks merge from my read.
What I verified:
- Trust classification —
isTrustedCloudUrlis https-only, rejects username/password and non-empty port, exact-host allowlist + a correctly-anchored*.testingcloud.comfy.orgcheck (leading-dotendsWith+ apex exclusion, soeviltestingcloud.comfy.organd…comfy.org.attacker.comboth fail). (inline) - Main is the trust authority — renderers report auth state; main decides (authorized only if trusted-cloud URL or Desktop-verified loopback; else ignored, fail-closed).
- IPC frame-binding — the
firebaseAuthStatehandler accepts a report only from the sender's main frame (senderFrame≡mainFrame), so a sub-iframe can't report; payload is validated (status enum + normalized, non-illegaluserId) before authorization is even applied. (inline) - Verified-loopback is genuinely loopback-only —
normalizeLoopbackOriginrejects any non-loopback origin, so an external origin can never get a verified-local binding; the preload monitor runs only on loopback pages and reads the actual Firebase IndexedDB (Desktop verifies persistence, not a renderer claim; >1 user →pending, never guesses). (inline) - Fail-closed on every race — navigation/frame swap (
isSameFrame/settleFailedNavigation), detached hosts (isDestroyedfilter), logout (revoke), and the privacy-critical one: an unmergeable identity conflict taints and, if the taint can't persist, durably discards the anonymous epoch so a conflicted ID is never wrong-merged nor resurrected untainted across a restart. (inline)
Bonus (not auth): the telemetry IPC is well-hardened — client/deployment axes are host-authoritative (stripped/resolved from the sender, never trusted from payload), and values are clamped with the larger JSON ceiling gated by an explicit key allowlist (not a _json suffix), so a renderer can't bypass the clamp by renaming a field.
Minor (non-blocking): *.testingcloud.comfy.org preview subdomains count as "trusted cloud" for auth reporting. They're first-party ephemeral preview envs and the report only affects telemetry identity (not privilege), so it's reasonable — just flagging it's a broad-ish trust grant.
Scope: I deep-read the trust core (trustedCloudUrl, the authorization model + fail-closed structure in firebaseAuthIdentity.ts, verifiedLocalFirebaseAuth.ts, the IPC binding, the preload monitor) but not the full 630-line reconcile() winner resolution line-by-line, nor the ~1,100 lines of tests. The PR cites an independent security/lifecycle review + 3,078 tests.
| 'testcloud.comfy.org' | ||
| ]) | ||
|
|
||
| export function isTrustedCloudUrl(value: string): boolean { |
There was a problem hiding this comment.
Solid trust boundary. https-only + rejecting username/password/port closes the credential-in-URL and proxy-port tricks, and the *.testingcloud.comfy.org check is correctly anchored (leading-dot endsWith + !== 'testingcloud.comfy.org' apex exclusion), so eviltestingcloud.comfy.org and x.testingcloud.comfy.org.attacker.com both fail. This is the single line the whole consensus's trust rests on — worth keeping it exact-match + anchored in any follow-up.
| return typeof value === 'string' && value.length > 0 ? value : null | ||
| } | ||
|
|
||
| function firebaseAuthReporterFrame(event: IpcMainEvent): WebFrameMain | null { |
There was a problem hiding this comment.
This is the key anti-spoof gate: auth-state reports are accepted only from the sender's main frame (senderFrame.processId/routingId ≡ mainFrame), so a malicious embedded sub-iframe can't report an identity. Combined with the payload validation (asFirebaseAuthState) and the reporter-authorization check downstream, an untrusted frame's report is dropped before it can reach consensus. Nicely layered.
| return path.join(configDir(), VERIFIED_LOCAL_FIREBASE_AUTH_FILE) | ||
| } | ||
|
|
||
| function normalizeLoopbackOrigin(value: unknown): string | null { |
There was a problem hiding this comment.
Good airtight scoping: because this rejects any non-loopback origin (and requires url.origin === value, no path/query smuggling), an external/remote origin can never be persisted as a verified-local binding. So the "Desktop-verified loopback" trust path is genuinely loopback-only — the exact property that keeps a remote page from claiming a local identity.
| epochTaintIsDurable = mainTelemetry.markAnonymousEpochUnmergeable() | ||
| if (!epochTaintIsDurable) return false | ||
| } | ||
| if (!mainTelemetry.discardUnmergeableAnonymousEpoch()) return false |
There was a problem hiding this comment.
The privacy-critical fail-closed step. When an identity conflict can't be tainted durably, discarding the anonymous epoch here means a conflicted anonymous ID is never merged to the wrong user and can't be resurrected untainted after a restart. This is the right call over trying to keep a possibly-conflicted epoch alive — ties cleanly into #1304's taint/discard primitives.
2 participants
Split E of #1249.
Summary
This final slice depends functionally on B and C; D is included because the GitHub stack is linear.
Stack
mainValidation
pnpm run typecheckpnpm run lintpnpm run buildpnpm run test— 3,078 passed, 1 skippedRC gate
Validate packaged multi-window login/logout/account-switch behavior, Cloud and local/remote installs, navigation/frame replacement, and identity recovery before rollout.