diff --git a/src/main/host/attach.ts b/src/main/host/attach.ts index b01572fd2..71f07a89e 100644 --- a/src/main/host/attach.ts +++ b/src/main/host/attach.ts @@ -456,13 +456,14 @@ export function attachInstall(entry: ComfyWindowEntry, opts: AttachInstallOpts): comfyContents.executeJavaScript(getMcpSidebarContentScript()).catch(() => {}) })() } - // Classify the view's signed-in account for ops-flag person targeting, and - // store it for the NEXT launch. Deliberately OUTSIDE the `!isLocal` branch - // below: the grant these flags carry is consumed only by the local launch - // path (`buildLaunchArgs`), since a cloud install spawns no Core — so - // binding on cloud views alone would cover every surface except the one - // that can use the result. Fire-and-forget; only a boolean is stored, and - // sending it is consent-gated in telemetry. + // Offer this view as a classifier for ops-flag person targeting. A RETRY path, not the + // trigger: the classification is driven by the identity consensus, which reclassifies as + // soon as any view reports a change. This covers the case where the consensus resolved while + // the views it asked could not answer, and is a no-op otherwise. Deliberately OUTSIDE the + // `!isLocal` branch below: the grant these flags carry is consumed only by the local launch + // path (`buildLaunchArgs`), since a cloud install spawns no Core — so binding on cloud views + // alone would cover every surface except the one that can use the result. Fire-and-forget; + // only a boolean is stored, and sending it is consent-gated in telemetry. void refreshStaffFlagTargeting(comfyContents) // Cloud-only patches (popup-blocked toast suppression + post-signin diff --git a/src/main/index.ts b/src/main/index.ts index b3c7db5d3..b8b14b0f7 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1507,7 +1507,9 @@ if (app.isPackaged && !app.requestSingleInstanceLock()) { // Bind the stored staff classification BEFORE any ops flag is fetched. The // boot evaluation is the only authoritative one, so a property that arrives - // after it cannot affect this launch — see `staffFlagTargeting.ts`. + // after it cannot affect this launch — see `staffFlagTargeting.ts`. Also + // subscribes to the identity consensus, which is what reclassifies for the + // NEXT launch; this runs before any view exists, so no outcome is missed. initStaffFlagTargeting() // This ops-flag path is separate from consent-gated experiments: the first-use diff --git a/src/main/lib/firebaseAuthIdentity.test.ts b/src/main/lib/firebaseAuthIdentity.test.ts index 9340ea9ad..853810a28 100644 --- a/src/main/lib/firebaseAuthIdentity.test.ts +++ b/src/main/lib/firebaseAuthIdentity.test.ts @@ -47,9 +47,13 @@ import { activateFirebaseAuthReporter, bindMainVerifiedFirebaseUser, deactivateFirebaseAuthReporter, + getFirebaseIdentityConsensus, + observeFirebaseIdentityConsensus, PENDING_CONSENSUS_DEADLINE_MS, reportFirebaseAuthState as recordFirebaseAuthState, - trackFirebaseAuthReporter + trackFirebaseAuthReporter, + viewsReportingFirebaseUser, + type FirebaseIdentityConsensus } from './firebaseAuthIdentity' class FakeWebContents extends EventEmitter { @@ -1157,3 +1161,246 @@ describe('firebaseAuthIdentity consensus', () => { expect(telemetry.applyFirebaseUserConsensus).toHaveBeenCalledWith('F2') }) }) + +// The reconciled outcome, published rather than only spent on telemetry side effects. Anything +// that needs to know WHICH ACCOUNT this process is serving reads it from here; `staffFlagTargeting` +// is the first such consumer, and its own suite covers what it does with each outcome. +describe('firebaseAuthIdentity published consensus', () => { + let published: FirebaseIdentityConsensus[] = [] + let unobserve: () => void = () => {} + + beforeEach(() => { + _resetForTest() + vi.clearAllMocks() + telemetry.discardUnmergeableAnonymousEpoch.mockReturnValue(true) + telemetry.hasUnmergeableAnonymousEpoch.mockReturnValue(false) + telemetry.isFirebaseConsensusPending.mockReturnValue(true) + telemetry.markAnonymousEpochUnmergeable.mockReturnValue(true) + verifiedLocalUsers.clear() + verifiedLocalPersistence.succeeds = true + published = [] + unobserve = observeFirebaseIdentityConsensus((consensus) => published.push(consensus)) + }) + + afterEach(() => { + unobserve() + }) + + it('starts out unable to say', () => { + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'unknown' }) + }) + + it('publishes the agreed account once every reporter affirms it', () => { + const first = new FakeWebContents(cloudUrl) + const second = new FakeWebContents(cloudUrl) + activate(first) + activate(second) + + reportFirebaseAuthState(first.asWebContents(), { status: 'signed_in', userId: 'F' }) + expect(published.at(-1)).toEqual({ status: 'pending' }) + + reportFirebaseAuthState(second.asWebContents(), { status: 'signed_in', userId: 'F' }) + + expect(published.at(-1)).toEqual({ status: 'signed_in', userId: 'F' }) + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'signed_in', userId: 'F' }) + }) + + it('publishes a resolved sign-out, which is evidence rather than absence', () => { + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_out' }) + + expect(published.at(-1)).toEqual({ status: 'signed_out' }) + }) + + it('publishes a conflict rather than picking one of two accounts', () => { + const first = new FakeWebContents(cloudUrl) + const second = new FakeWebContents(cloudUrl) + activate(first) + activate(second) + + reportFirebaseAuthState(first.asWebContents(), { status: 'signed_in', userId: 'F1' }) + reportFirebaseAuthState(second.asWebContents(), { status: 'signed_in', userId: 'F2' }) + + expect(published.at(-1)).toEqual({ status: 'conflicted' }) + }) + + it('publishes a conflict when one view is signed in and another is signed out', () => { + const first = new FakeWebContents(cloudUrl) + const second = new FakeWebContents(cloudUrl) + activate(first) + activate(second) + + reportFirebaseAuthState(first.asWebContents(), { status: 'signed_in', userId: 'F' }) + reportFirebaseAuthState(second.asWebContents(), { status: 'signed_out' }) + + expect(published.at(-1)).toEqual({ status: 'conflicted' }) + }) + + it('publishes unknown, NOT signed out, when the last contributor goes away', () => { + // The distinction the split exists for. Telemetry detaches on both — an in-memory binding + // with nobody left to affirm it should stop claiming events — but a consumer that PERSISTS + // the account must not read "every window closed" as "somebody signed out". + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + expect(published.at(-1)).toEqual({ status: 'signed_in', userId: 'F' }) + + reporter.destroy() + + expect(published.at(-1)).toEqual({ status: 'unknown' }) + expect(telemetry.applyFirebaseAnonymousConsensus).toHaveBeenCalled() + }) + + it('stays unknown for a local view that was never signed into', () => { + // Its reports are untrusted until a main-verified sign-in scopes them, so it contributes + // nothing at all. "No auth store here" is not a vote that nobody is signed in — contrast the + // cloud view below, whose identical report IS a resolved sign-out. + const local = new FakeWebContents('http://127.0.0.1:8188/') + activate(local) + + reportFirebaseAuthState(local.asWebContents(), { status: 'signed_out' }) + + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'unknown' }) + expect(published).toEqual([]) + + const cloud = new FakeWebContents(cloudUrl) + activate(cloud) + reportFirebaseAuthState(cloud.asWebContents(), { status: 'signed_out' }) + + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'signed_out' }) + }) + + it('publishes on change only', () => { + // `reconcile()` runs on every navigation event. An observer that re-read a page on each one + // would be a poll with extra steps. + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + const afterFirst = published.length + + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + + expect(published).toHaveLength(afterFirst) + }) + + it('holds the last outcome when a tainted epoch defers the bind', () => { + // Publishing here would announce an account the process has not adopted: telemetry is held + // anonymous and a retry is expected. + telemetry.markAnonymousEpochUnmergeable.mockReturnValue(true) + telemetry.hasUnmergeableAnonymousEpoch.mockReturnValue(true) + telemetry.discardUnmergeableAnonymousEpoch.mockReturnValue(false) + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + + expect(published.at(-1)).toEqual({ status: 'pending' }) + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'pending' }) + }) + + it('stops delivering once unsubscribed', () => { + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + unobserve() + published = [] + + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + + expect(published).toEqual([]) + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'signed_in', userId: 'F' }) + }) + + it('keeps reconciling when an observer throws', () => { + // An observer is a consumer, not a participant. One that fails must not take the identity + // engine down with it. + const failing = observeFirebaseIdentityConsensus(() => { + throw new Error('observer exploded') + }) + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + + expect(() => + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + ).not.toThrow() + expect(telemetry.applyFirebaseUserConsensus).toHaveBeenCalledWith('F') + failing() + }) + + it('names the views that hold the agreed account', () => { + const holder = new FakeWebContents(cloudUrl) + const other = new FakeWebContents(cloudUrl) + activate(holder) + activate(other) + reportFirebaseAuthState(holder.asWebContents(), { status: 'signed_in', userId: 'F' }) + reportFirebaseAuthState(other.asWebContents(), { status: 'signed_in', userId: 'F' }) + + expect(viewsReportingFirebaseUser('F')).toEqual([holder.asWebContents(), other.asWebContents()]) + expect(viewsReportingFirebaseUser('OTHER')).toEqual([]) + }) + + it('names a main-verified view before its reporter has reported', () => { + // This process already believes that view holds the account, so it is a legitimate one to + // put a question to. + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + + bindMainVerifiedFirebaseUser('F', {}, reporter.asWebContents()) + + expect(published.at(-1)).toEqual({ status: 'pending' }) + expect(viewsReportingFirebaseUser('F')).toEqual([reporter.asWebContents()]) + }) + + it('drops a main-verified view whose document has moved to another origin', () => { + // `reconcile()` prunes this map on exactly this condition, and pruning happens ONLY in there. + // Naming a view that has moved would send a caller's question to — and make it trust an answer + // from — a page at a different origin. Defence in depth: every origin change observed here + // also runs `reconcile()`, so this guards the window rather than a reproduced live bug. + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + bindMainVerifiedFirebaseUser('F', {}, reporter.asWebContents()) + expect(viewsReportingFirebaseUser('F')).toEqual([reporter.asWebContents()]) + + // Moves the document's URL without a commit, so no prune runs in between. + reporter.navigate('https://elsewhere.example.com/page', true) + + expect(viewsReportingFirebaseUser('F')).toEqual([]) + }) + + it('stops dispatching an outcome a nested publish has superseded', () => { + // An observer can synchronously re-enter `reconcile()`. The nested dispatch delivers the newer + // outcome to everybody; resuming the outer loop afterwards would hand the observers it had not + // reached a value that disagrees with `getFirebaseIdentityConsensus()`. + const seen: string[] = [] + const reporter = new FakeWebContents(cloudUrl) + activate(reporter) + const reentrant = observeFirebaseIdentityConsensus((consensus) => { + seen.push(`reentrant:${consensus.status}`) + if (consensus.status === 'signed_in') reporter.destroy() + }) + const later = observeFirebaseIdentityConsensus((consensus) => { + seen.push(`later:${consensus.status}`) + }) + + reportFirebaseAuthState(reporter.asWebContents(), { status: 'signed_in', userId: 'F' }) + + expect(seen).not.toContain('later:signed_in') + expect(seen).toContain('later:unknown') + expect(getFirebaseIdentityConsensus()).toEqual({ status: 'unknown' }) + reentrant() + later() + }) + + it('drops a destroyed view from the ones it names', () => { + const holder = new FakeWebContents(cloudUrl) + activate(holder) + reportFirebaseAuthState(holder.asWebContents(), { status: 'signed_in', userId: 'F' }) + expect(viewsReportingFirebaseUser('F')).toHaveLength(1) + + holder.destroy() + + expect(viewsReportingFirebaseUser('F')).toEqual([]) + }) +}) diff --git a/src/main/lib/firebaseAuthIdentity.ts b/src/main/lib/firebaseAuthIdentity.ts index 14e44c113..941bea4e7 100644 --- a/src/main/lib/firebaseAuthIdentity.ts +++ b/src/main/lib/firebaseAuthIdentity.ts @@ -78,8 +78,42 @@ interface MainVerifiedAuthState { rendererMayReaffirm: boolean } +/** + * The reconciled answer to "who is signed in, across every view this process hosts". + * + * `reconcile()` has always computed exactly these outcomes; until now it only expressed them as + * side effects on telemetry, so nothing else could be driven from them. Anything that needs to + * know the account should read this rather than interrogating one view's page, which is a single + * sample of a state several views contribute to. + * + * `unknown` is deliberately NOT folded into `signed_out`. Telemetry treats "no contributor left" + * as a reason to stop attributing events to a user, which is right for an in-memory binding; but + * for anything PERSISTED the two are opposites. Closing the last window is not evidence that + * anybody signed out, and a consumer that writes on `signed_out` must not be handed `unknown`. + * + * `conflicted` is likewise its own outcome rather than a resolution. Two views signed into two + * accounts is a real disagreement, and the honest answer is that this process does not know which + * account it is serving - not a coin flip on whichever view reported last. + */ +export type FirebaseIdentityConsensus = + /** No contributor can say: every view is gone or untrusted. A view wedged past its deadline + * does NOT reach here — `reconcile()` keeps the last outcome in that case, deliberately, since + * an unresolved document is evidence of nothing. So `unknown` and "still wedged" are not + * distinguishable from the outside, and neither is a reason to move a persisted fact. */ + | { status: 'unknown' } + /** At least one contributor is mid-resolution. */ + | { status: 'pending' } + /** Every contributor resolved, and none is signed in. */ + | { status: 'signed_out' } + /** Contributors resolved to different accounts, or to a mix of signed in and signed out. */ + | { status: 'conflicted' } + /** Every contributor resolved to this one account. */ + | { status: 'signed_in'; userId: string } + const reporters = new Map() const mainVerifiedStates = new Map() +let consensus: FirebaseIdentityConsensus = { status: 'unknown' } +const consensusObservers = new Set<(consensus: FirebaseIdentityConsensus) => void>() let requestedUserId: string | null = null let anonymousEpochIsUnmergeable = false let persistedEpochStateLoaded = false @@ -88,6 +122,81 @@ let epochTaintIsDurable = true export const PENDING_CONSENSUS_DEADLINE_MS = 60_000 let pendingConsensusDeadline: ReturnType | null = null +function sameConsensus( + first: FirebaseIdentityConsensus, + second: FirebaseIdentityConsensus +): boolean { + if (first.status !== second.status) return false + return first.status !== 'signed_in' || first.userId === (second as { userId: string }).userId +} + +/** + * Record the reconciled outcome and hand it to observers. + * + * Change-only: `reconcile()` runs on every navigation event, and an observer that re-reads a page + * on each one would be a poll with extra steps. The stored value is set BEFORE dispatch, so an + * observer that ends up back inside `reconcile()` sees the new outcome and cannot re-dispatch the + * same one; and if it publishes a DIFFERENT one, this loop stops rather than handing a superseded + * value to the observers it had not reached yet. An observer that throws must not take the + * identity engine down with it. + */ +function publishConsensus(next: FirebaseIdentityConsensus): void { + if (sameConsensus(consensus, next)) return + consensus = next + for (const observe of [...consensusObservers]) { + // An observer can synchronously re-enter `reconcile()` and publish a different outcome, which + // completes its own dispatch before this loop resumes. Handing the rest of the observers a + // value that no longer matches `getFirebaseIdentityConsensus()` is worse than not calling + // them at all — the nested dispatch has already delivered the newer one to everybody. + if (!sameConsensus(consensus, next)) return + try { + observe(next) + } catch (err) { + console.log('[firebase-identity] consensus observer failed:', err) + } + } +} + +/** The current reconciled identity outcome across every contributing view. */ +export function getFirebaseIdentityConsensus(): FirebaseIdentityConsensus { + return consensus +} + +/** Observe reconciled identity outcomes. Fires on change only; returns an unsubscribe. */ +export function observeFirebaseIdentityConsensus( + observe: (consensus: FirebaseIdentityConsensus) => void +): () => void { + consensusObservers.add(observe) + return () => consensusObservers.delete(observe) +} + +/** + * The live views this process counts as signed into `userId`. + * + * For a consumer that needs to ask a PAGE something about the agreed account - which view it may + * put the question to. Includes views bound by a main-verified sign-in whose reporter has not + * reported yet, since those are views this process already believes hold that account. + */ +export function viewsReportingFirebaseUser(userId: string): WebContents[] { + const views: WebContents[] = [] + for (const [webContents, reporter] of reporters) { + if (webContents.isDestroyed() || !reporter.active) continue + if (reporter.state.status === 'signed_in' && reporter.state.userId === userId) { + views.push(webContents) + } + } + for (const [webContents, state] of mainVerifiedStates) { + if (webContents.isDestroyed() || state.userId !== userId) continue + // The same origin revalidation `reconcile()` applies to this map — it is what prunes the + // entry, and pruning only happens in there. Without it a view that has navigated elsewhere is + // still named as holding the account, and a caller would put its question to, and trust an + // answer from, a page at a different origin. + if (originOf(webContents.getURL()) !== state.origin) continue + if (!views.includes(webContents)) views.push(webContents) + } + return views +} + function originOf(url: string): string | null { try { return new URL(url).origin @@ -361,11 +470,16 @@ function reconcile(): void { // nothing: keep the current identity until a real report resolves it. if (expiredPendingContributors > 0) return requestAnonymousIdentity() + // Not `signed_out`: no view can say. Telemetry detaches here because an + // in-memory binding with nobody left to affirm it should stop claiming + // events; a consumer that PERSISTS the account must hold instead. + publishConsensus({ status: 'unknown' }) return } if (states.some((state) => state.status === 'pending')) { requestPendingIdentity() + publishConsensus({ status: 'pending' }) return } @@ -379,6 +493,7 @@ function reconcile(): void { // binding even when this process has not bound a UID yet. detachToAnonymousIdentity() clearUnmergeableEpoch() + publishConsensus({ status: 'signed_out' }) return } @@ -386,14 +501,19 @@ function reconcile(): void { const hasConflict = signedIn.length !== states.length || userIds.size !== 1 if (hasConflict) { requestConflictedIdentity() + publishConsensus({ status: 'conflicted' }) return } const userId = signedIn[0]!.userId + // Publishing before this would announce an account the process has not adopted: the epoch is + // still tainted, telemetry is held anonymous, and a retry is expected. Hold the previous + // outcome until the bind can actually take effect. if (!clearUnmergeableEpoch()) return clearPendingConsensusDeadline() requestedUserId = userId + publishConsensus({ status: 'signed_in', userId }) const confirmedMainStates = mainCandidates.filter(({ webContents, state, contributes }) => { if (state.userId !== userId) return false if (contributes) return true @@ -722,6 +842,8 @@ export function _resetForTest(): void { } reporters.clear() mainVerifiedStates.clear() + consensus = { status: 'unknown' } + consensusObservers.clear() clearPendingConsensusDeadline() requestedUserId = null anonymousEpochIsUnmergeable = false diff --git a/src/main/lib/staffFlagTargeting.test.ts b/src/main/lib/staffFlagTargeting.test.ts index d55670d4e..72478ec61 100644 --- a/src/main/lib/staffFlagTargeting.test.ts +++ b/src/main/lib/staffFlagTargeting.test.ts @@ -4,10 +4,16 @@ // The boot flag evaluation is the only authoritative one, so what matters here is that the // stored answer is bound BEFORE it and that the stored answer is a boolean and nothing else. // Whether the property may leave the process is telemetry's gate (`telemetry.test.ts`). +// +// The classification is driven by the reconciled identity, so the consensus is what these tests +// drive. `firebaseAuthIdentity` is stubbed down to the three things this module asks of it — +// the current outcome, a subscription, and which views hold a given account — because the +// reconciliation itself has its own suite and this one should fail for reasons that belong to it. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import fs from 'fs' import os from 'os' import path from 'path' +import type { FirebaseIdentityConsensus } from './firebaseAuthIdentity' let testConfigDir = '' vi.mock('./paths', () => ({ @@ -19,31 +25,119 @@ vi.mock('./telemetry', () => ({ setFlagEvaluationStaff: (isStaff: boolean) => setFlagEvaluationStaff(isStaff) })) +const identity = vi.hoisted(() => { + let current: { status: string; userId?: string } = { status: 'unknown' } + const observers = new Set<(consensus: unknown) => void>() + const views = new Map() + return { + current: () => current, + observers, + views, + /** Stand in for `reconcile()` publishing an outcome. Always dispatches: change-only delivery + * is the identity engine's contract and is pinned in its own suite. */ + publish(next: { status: string; userId?: string }, holders: Electron.WebContents[] = []) { + current = next + if (next.status === 'signed_in' && next.userId) views.set(next.userId, holders) + for (const observe of [...observers]) observe(next) + }, + reset() { + current = { status: 'unknown' } + observers.clear() + views.clear() + } + } +}) + +vi.mock('./firebaseAuthIdentity', () => ({ + getFirebaseIdentityConsensus: () => identity.current(), + observeFirebaseIdentityConsensus: (observe: (consensus: unknown) => void) => { + identity.observers.add(observe) + return () => identity.observers.delete(observe) + }, + viewsReportingFirebaseUser: (userId: string) => identity.views.get(userId) ?? [] +})) + const { initStaffFlagTargeting, refreshStaffFlagTargeting, CLASSIFY_STAFF_JS, _resetForTest } = await import('./staffFlagTargeting') +/** The account every test signs in as unless it is about two of them. */ +const USER = 'uid-primary' +const OTHER_USER = 'uid-other' + +/** How many times a stub view has actually been asked to classify. + * + * Load-bearing, not bookkeeping. A test that expects a view to be CONSULTED and then asserts only + * on the stored classification passes just as happily when the view is never read at all — the + * short-circuit for an already-classified account makes exactly that happen. Counting the reads + * is what separates "this abstained correctly" from "this was never asked". */ +let pageReads = 0 + /** A view that CAN classify — it reached an auth store and reached a verdict. */ -function stubContents(staff: boolean, opts: { throws?: boolean } = {}): Electron.WebContents { +function stubContents( + staff: boolean, + opts: { throws?: boolean; userId?: string } = {} +): Electron.WebContents { return { - executeJavaScript: () => - opts.throws ? Promise.reject(new Error('page gone')) : Promise.resolve({ known: true, staff }) + executeJavaScript: () => { + pageReads += 1 + return opts.throws + ? Promise.reject(new Error('page gone')) + : Promise.resolve({ known: true, staff, userId: opts.userId ?? USER }) + } } as unknown as Electron.WebContents } /** A view with NO auth store — it has no opinion about who is signed in. */ function stubContentsWithoutAuthStore(): Electron.WebContents { return { - executeJavaScript: () => Promise.resolve({ known: false }) + executeJavaScript: () => { + pageReads += 1 + return Promise.resolve({ known: false }) + } } as unknown as Electron.WebContents } /** A view whose read returns something unexpected entirely. */ function stubContentsReturning(result: unknown): Electron.WebContents { return { - executeJavaScript: () => Promise.resolve(result) + executeJavaScript: () => { + pageReads += 1 + return Promise.resolve(result) + } } as unknown as Electron.WebContents } +/** Let the page reads a consensus change kicks off settle. */ +function settle(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)) +} + +/** The whole process agrees this account is signed in, held by these views. */ +async function consensusSignedIn( + holders: Electron.WebContents[], + userId: string = USER +): Promise { + identity.publish({ status: 'signed_in', userId }, holders) + await settle() +} + +/** Every contributor resolved and none is signed in. */ +async function consensusSignedOut(): Promise { + identity.publish({ status: 'signed_out' }) + await settle() +} + +/** An outcome that is the absence of an answer rather than an answer. */ +async function consensusUnresolved( + status: Extract< + FirebaseIdentityConsensus, + { status: 'pending' | 'conflicted' | 'unknown' } + >['status'] +): Promise { + identity.publish({ status }) + await settle() +} + function persistFilePath(): string { return path.join(testConfigDir, 'staff-targeting.json') } @@ -64,7 +158,9 @@ function nextLaunchBinding(): boolean { beforeEach(() => { testConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'staff-targeting-')) setFlagEvaluationStaff.mockClear() + pageReads = 0 _resetForTest() + identity.reset() }) afterEach(() => { @@ -122,14 +218,14 @@ function authRecord(uid: string, email: string | null, emailVerified = true): un /** Run the REAL injected script against a stubbed IndexedDB. */ async function classify(opts: Parameters[0]): Promise<{ - result: { known?: boolean; staff?: boolean } + result: { known?: boolean; staff?: boolean; userId?: string | null } closed: number }> { const { idb, closed } = fakeIndexedDB(opts) const run = new Function('indexedDB', 'setTimeout', `return ${CLASSIFY_STAFF_JS}`) as ( i: unknown, t: unknown - ) => Promise<{ known?: boolean; staff?: boolean }> + ) => Promise<{ known?: boolean; staff?: boolean; userId?: string | null }> const result = await run(idb, setTimeout) return { result, closed: closed.count } } @@ -150,7 +246,7 @@ describe('CLASSIFY_STAFF_JS', () => { ])('classifies %s', async (_label, email, expected) => { const { result } = await classify({ entries: [authRecord('u1', email as string | null)] }) - expect(result).toEqual({ known: true, staff: expected }) + expect(result).toEqual({ known: true, staff: expected, userId: 'u1' }) }) it('refuses an unverified address, which proves nothing about domain ownership', async () => { @@ -158,13 +254,13 @@ describe('CLASSIFY_STAFF_JS', () => { // self-asserted. Without this check anyone could sign up and enter the cohort. const { result } = await classify({ entries: [authRecord('u1', 'someone@comfy.org', false)] }) - expect(result).toEqual({ known: true, staff: false }) + expect(result).toEqual({ known: true, staff: false, userId: 'u1' }) }) - it('reports signed out when no auth record exists', async () => { + it('reports signed out, for no account, when no auth record exists', async () => { const { result } = await classify({ entries: [] }) - expect(result).toEqual({ known: true, staff: false }) + expect(result).toEqual({ known: true, staff: false, userId: null }) }) it('declines to answer when two accounts are stored', async () => { @@ -182,7 +278,7 @@ describe('CLASSIFY_STAFF_JS', () => { entries: [authRecord('u1', 'someone@comfy.org'), authRecord('u1', 'someone@comfy.org')] }) - expect(result).toEqual({ known: true, staff: true }) + expect(result).toEqual({ known: true, staff: true, userId: 'u1' }) }) it.each([['__proto__'], ['constructor'], ['toString']])( @@ -216,7 +312,22 @@ describe('CLASSIFY_STAFF_JS', () => { entries: [{ fbase_key: 'something:else', value: { uid: 'x', email: 'a@comfy.org' } }, null] }) - expect(result).toEqual({ known: true, staff: false }) + expect(result).toEqual({ known: true, staff: false, userId: null }) + }) + + it('reports which account it classified, so main can check it is the agreed one', async () => { + const { result } = await classify({ entries: [authRecord('uid-primary', 'foo@comfy.org')] }) + + expect(result).toMatchObject({ userId: 'uid-primary' }) + }) + + it('caps an absurd uid one past what main will accept, rather than truncating into a match', async () => { + // Truncating at exactly 256 would let a 300-character uid land on the same string as a + // different account. One past it fails `normalizePostHogUserId` outright, and nothing + // unbounded crosses the IPC boundary either way. + const { result } = await classify({ entries: [authRecord('u'.repeat(300), 'foo@comfy.org')] }) + + expect(result.userId).toHaveLength(257) }) it('gives up on an open that never settles, rather than hanging forever', async () => { @@ -281,9 +392,14 @@ describe('initStaffFlagTargeting', () => { }) }) -describe('refreshStaffFlagTargeting', () => { +describe('classification driven by the identity consensus', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + it('stores the classification for the next launch', async () => { - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) expect(storedFile()).toMatchObject({ staff: true }) }) @@ -291,14 +407,22 @@ describe('refreshStaffFlagTargeting', () => { it('stores only a boolean — never the address it classified', async () => { // The whole privacy argument: an address is classified in page context and discarded, so // there is no path by which one could reach disk or PostHog. - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) expect(fs.readFileSync(persistFilePath(), 'utf-8')).not.toContain('someone@comfy.org') expect(fs.readFileSync(persistFilePath(), 'utf-8')).not.toContain('comfy.org') }) + it('stores only a boolean — never the UID it checked against', async () => { + // The uid is compared and discarded. It is not part of what the next launch is targeted on, + // and persisting it would put an account identifier at rest for no purpose. + await consensusSignedIn([stubContents(true)]) + + expect(fs.readFileSync(persistFilePath(), 'utf-8')).not.toContain(USER) + }) + it('never hands telemetry anything but a boolean', async () => { - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) for (const [arg] of setFlagEvaluationStaff.mock.calls) { expect(typeof arg).toBe('boolean') @@ -308,98 +432,476 @@ describe('refreshStaffFlagTargeting', () => { it('carries a staff classification into the next launch', async () => { // The behaviour the whole design exists to produce, end to end across a restart: sign in on // one launch, be targeted on the next. - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) expect(nextLaunchBinding()).toBe(true) }) it('does not target the launch it runs on', async () => { - // The accepted cost, pinned: the boot evaluation has already gone out by the time a view - // resolves auth, and this deliberately does not try to redo it. - initStaffFlagTargeting() - expect(setFlagEvaluationStaff).toHaveBeenLastCalledWith(false) + // The accepted cost, pinned: the boot evaluation has already gone out by the time the + // consensus resolves, and this deliberately does not try to redo it. + expect(nextLaunchBinding()).toBe(false) - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) expect(storedFile()).toMatchObject({ staff: true }) }) - it('stores false for a non-staff account', async () => { - await refreshStaffFlagTargeting(stubContents(false)) + it('leaves a non-staff account untargeted, and writes nothing to say so', async () => { + // An absent file already means "not staff", so the overwhelmingly common case costs no disk + // write at all. The write that matters is the one that REVOKES, covered below. + await consensusSignedIn([stubContents(false)]) - expect(storedFile()).toMatchObject({ staff: false }) + expect(fs.existsSync(persistFilePath())).toBe(false) expect(nextLaunchBinding()).toBe(false) }) - it('reclassifies to false on sign-out, so a machine that changes hands stops presenting as staff', async () => { - await refreshStaffFlagTargeting(stubContents(true)) - expect(nextLaunchBinding()).toBe(true) + it('stores false for a non-staff account when the disk says otherwise', async () => { + fs.writeFileSync(persistFilePath(), JSON.stringify({ staff: true }), 'utf-8') + initStaffFlagTargeting() - await refreshStaffFlagTargeting(stubContents(false)) + await consensusSignedIn([stubContents(false)]) expect(storedFile()).toMatchObject({ staff: false }) expect(nextLaunchBinding()).toBe(false) }) - it('reclassifies on a switch to a non-staff account', async () => { - await refreshStaffFlagTargeting(stubContents(true)) - - await refreshStaffFlagTargeting(stubContents(false)) + it('binds the classification immediately as well as storing it', async () => { + await consensusSignedIn([stubContents(true)]) - expect(nextLaunchBinding()).toBe(false) + expect(setFlagEvaluationStaff).toHaveBeenLastCalledWith(true) }) it('does not rewrite the file when the classification is unchanged', async () => { - // Every page load reaches here, so "no change" has to cost nothing. - await refreshStaffFlagTargeting(stubContents(true)) + // Every resolution reaches here, so "no change" has to cost nothing. + await consensusSignedIn([stubContents(true)]) const firstWrite = fs.statSync(persistFilePath()).mtimeMs - await refreshStaffFlagTargeting(stubContents(true)) - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) + await consensusSignedIn([stubContents(true)]) expect(fs.statSync(persistFilePath()).mtimeMs).toBe(firstWrite) }) - it('binds the classification immediately as well as storing it', async () => { - await refreshStaffFlagTargeting(stubContents(true)) + it('reclassifies on a switch to a non-staff account', async () => { + await consensusSignedIn([stubContents(true)]) + + await consensusSignedIn([stubContents(false, { userId: OTHER_USER })], OTHER_USER) + + expect(nextLaunchBinding()).toBe(false) + }) + + it('re-binds the agreed account across a pending outcome even when no view can answer', async () => { + // The common shape, since every navigation takes the consensus through `pending` and back. + // The account must keep its classification with no gap, and must not depend on a view still + // being readable by then. + await consensusSignedIn([stubContents(true)]) + await consensusUnresolved('pending') + setFlagEvaluationStaff.mockClear() + + await consensusSignedIn([], USER) expect(setFlagEvaluationStaff).toHaveBeenLastCalledWith(true) + expect(nextLaunchBinding()).toBe(true) }) - it('survives a page-context read that throws, leaving the stored value alone', async () => { - // Fire-and-forget from `attach.ts`; an escaping rejection would be unhandled. A page that - // cannot be read must not revoke a grant. + it('revalidates a returning account, because a UID is not a classification', async () => { + // `staff` comes from `email` and `emailVerified`, both mutable while Firebase keeps reporting + // the same UID — an address verified mid-session, or one that changes domain. Caching the + // verdict against the UID alone would make it immutable for the life of the process, which is + // stricter than the per-view read this replaces: that ran on every `dom-ready` and would have + // seen the change. + // + // Deliberately does NOT call `nextLaunchBinding()` part-way through: that resets the module, + // which clears the session cache and sends the second resolution down the first-classification + // path instead of the already-classified short-circuit this test exists to cover. An earlier + // version did, and passed with the revalidation removed. + await consensusSignedIn([stubContents(true)]) + expect(storedFile()).toMatchObject({ staff: true }) + await consensusUnresolved('pending') + pageReads = 0 + + await consensusSignedIn([stubContents(false)], USER) + + expect(pageReads).toBeGreaterThan(0) + expect(storedFile()).toMatchObject({ staff: false }) + }) + + it('binds the known answer before revalidating, so a returning account never flaps', async () => { + // The revalidation is a page read and therefore asynchronous. If the known answer were not + // bound first, the account would spend that window unclassified. + await consensusSignedIn([stubContents(true)]) + await consensusUnresolved('pending') + setFlagEvaluationStaff.mockClear() + + identity.publish({ status: 'signed_in', userId: USER }, [stubContents(true)]) + + expect(setFlagEvaluationStaff).toHaveBeenCalledWith(true) + await settle() + // The `true` assertion alone does not detect a flap — it still passes if the revalidation + // emits `false` and then `true`. The mock is cleared before the transition and the known + // answer is bound synchronously, so no `false` belongs anywhere in this window. + expect(setFlagEvaluationStaff).not.toHaveBeenCalledWith(false) + }) +}) + +// The three limitations #1550 shipped with, each of which is the same root cause: a classification +// read from one view's page rather than from the state every view contributes to. +describe('what reading a single view got wrong', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it('reclassifies on a sign-out that never navigates', async () => { + // Limitation 1. The old path ran on `dom-ready`, so an in-page sign-out was invisible until + // the next navigation, reload or launch, and the machine kept presenting as staff across it. + // The consensus sees it as soon as the view reports, with no navigation involved. + await consensusSignedIn([stubContents(true)]) + expect(nextLaunchBinding()).toBe(true) + + await consensusSignedOut() + + expect(storedFile()).toMatchObject({ staff: false }) + expect(nextLaunchBinding()).toBe(false) + }) + + it('holds when two views are signed into two accounts, rather than letting one win', async () => { + // Limitation 2. Per-view reads had no way to reconcile a disagreement, so whichever document + // loaded last decided. A conflict is not an answer to be raced for — it means this process + // does not know which account it is serving. + await consensusSignedIn([stubContents(true)]) + expect(nextLaunchBinding()).toBe(true) + + await consensusUnresolved('conflicted') + + expect(nextLaunchBinding()).toBe(true) + expect(storedFile()).toMatchObject({ staff: true }) + }) + + it('refuses a view that classified an account the process does not agree is signed in', async () => { + // Limitation 3, to the extent a client CAN fix it. The consensus says one account; this view + // read another — a stale record, or a different source than the one that reported. Its answer + // is about somebody else, so it is not a failure to retry but an answer to discard. + await consensusSignedIn([stubContents(true)]) + expect(nextLaunchBinding()).toBe(true) + pageReads = 0 + + await consensusSignedIn([stubContents(false, { userId: OTHER_USER })], USER) + + // Without this the test passes when the view is never read at all, and the cross-check it is + // named for never runs. + expect(pageReads).toBeGreaterThan(0) + expect(nextLaunchBinding()).toBe(true) + }) + + it('refuses an over-length uid that trimming would sneak under the limit', async () => { + // `normalizePostHogUserId` trims BEFORE applying its 256-character limit, so a 257-character + // uid whose last character is whitespace normalizes down to 256 and matches — defeating the + // page-side cap that exists to reject rather than truncate. The raw length is what is bounded. + const agreed = 'u'.repeat(256) + await consensusSignedIn( + [stubContentsReturning({ known: true, staff: true, userId: agreed + '\n' })], + agreed + ) + + expect(fs.existsSync(persistFilePath())).toBe(false) + }) + + it('refuses a uid too long for the gate consensus itself applies', async () => { + // Rejected rather than truncated: a truncated uid could collide with a different account. + await consensusSignedIn([ + stubContentsReturning({ known: true, staff: true, userId: 'u'.repeat(257) }) + ]) + + expect(fs.existsSync(persistFilePath())).toBe(false) + }) + + it('asks the next view when the first cannot answer', async () => { + await consensusSignedIn([stubContentsWithoutAuthStore(), stubContents(true)]) + + expect(nextLaunchBinding()).toBe(true) + }) + + it('does not apply a read whose account was superseded while it was in flight', async () => { + // A page read is asynchronous. Without the generation check, an answer about the account that + // signed out a moment ago would be applied to whoever is signed in now. + const slowRead: { release?: (value: unknown) => void } = {} + const slowView = { + executeJavaScript: () => + new Promise((resolve) => { + slowRead.release = resolve + }) + } as unknown as Electron.WebContents + + identity.publish({ status: 'signed_in', userId: USER }, [slowView]) + await settle() + await consensusSignedOut() + + slowRead.release?.({ known: true, staff: true, userId: USER }) + await settle() + + expect(setFlagEvaluationStaff).not.toHaveBeenCalledWith(true) + expect(fs.existsSync(persistFilePath())).toBe(false) + expect(nextLaunchBinding()).toBe(false) + }) +}) + +describe('one answer per consensus outcome', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it('does not let a slower view overwrite a verdict already accepted for this outcome', async () => { + // A `dom-ready` retry runs with the CURRENT generation, so it can be in flight alongside the + // consensus observer's own read for the same one. Both would pass the generation check, and + // whichever settled last would win — making the verdict a function of page-read latency. + const slow: { release?: (value: unknown) => void } = {} + const slowView = { + executeJavaScript: () => + new Promise((resolve) => { + slow.release = resolve + }) + } as unknown as Electron.WebContents + + identity.publish({ status: 'signed_in', userId: USER }, [slowView]) + await settle() + // A second view answers first, for the same outcome. `true` rather than `false` so the + // accepted answer leaves a file — an absent file is what "not staff" already looks like, so + // asserting on it would not distinguish "not overwritten" from "never written". await refreshStaffFlagTargeting(stubContents(true)) + expect(storedFile()).toMatchObject({ staff: true }) - await expect( - refreshStaffFlagTargeting(stubContents(false, { throws: true })) - ).resolves.toBeUndefined() + slow.release?.({ known: true, staff: false, userId: USER }) + await settle() + + expect(storedFile()).toMatchObject({ staff: true }) expect(nextLaunchBinding()).toBe(true) }) - it('treats a non-boolean verdict as not staff', async () => { - await refreshStaffFlagTargeting(stubContentsReturning({ known: true, staff: 'yes' })) + it('still answers a later outcome, so the guard is per-outcome and not permanent', async () => { + await consensusSignedIn([stubContents(false)]) + await consensusUnresolved('pending') + + await consensusSignedIn([stubContents(true)], USER) + + expect(storedFile()).toMatchObject({ staff: true }) + }) +}) + +describe('retrying a write that did not land', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it('retries the revocation write, which the consensus will not redeliver', async () => { + // `publishConsensus` is change-only, so an unchanged `signed_out` is never redelivered — the + // consensus will not bring this write back on its own. `refreshStaffFlagTargeting` re-applies + // it on each later `dom-ready`, which is the retry path this test drives; without that, a + // `writeFileSafe` that threw would leave `staff: true` on disk for every later launch, + // silently reversing the revocation. + await consensusSignedIn([stubContents(true)]) + expect(storedFile()).toMatchObject({ staff: true }) + + fs.mkdirSync(persistFilePath() + '.tmp', { recursive: true }) + await consensusSignedOut() + expect(storedFile()).toMatchObject({ staff: true }) + + fs.rmSync(persistFilePath() + '.tmp', { recursive: true, force: true }) + await refreshStaffFlagTargeting(stubContents(true)) expect(storedFile()).toMatchObject({ staff: false }) + expect(nextLaunchBinding()).toBe(false) + }) + + it('does not let a dom-ready view DECIDE a sign-out, only re-apply one', async () => { + // Re-applying a decision the consensus already took is a write retry. Taking one on a single + // view's say-so would be a decision, and one view says nothing about the others. + await consensusSignedIn([stubContents(true)]) + await consensusUnresolved('unknown') + pageReads = 0 + + await refreshStaffFlagTargeting(stubContents(false)) + + expect(pageReads).toBe(0) + expect(nextLaunchBinding()).toBe(true) + }) +}) + +describe('bounding a page that does not answer', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it('gives up on a wedged view and asks the next one', async () => { + // `executeJavaScript` has no timeout, and the injected script only bounds `indexedDB.open` — + // `databases()` and `getAll` are unbounded, and a page can replace either with a promise that + // never settles. Without a main-process bound that one view blocks every view behind it. + vi.useFakeTimers() + try { + const wedged = { + executeJavaScript: () => new Promise(() => {}) + } as unknown as Electron.WebContents + + identity.publish({ status: 'signed_in', userId: USER }, [wedged, stubContents(true)]) + await vi.advanceTimersByTimeAsync(10_000) + + expect(storedFile()).toMatchObject({ staff: true }) + } finally { + vi.useRealTimers() + } + }) +}) + +describe('outcomes that are the absence of an answer', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it.each([ + ['a contributor is still resolving', 'pending' as const], + ['two views disagree about the account', 'conflicted' as const], + ['no view can say at all', 'unknown' as const] + ])('holds the stored classification when %s', async (_label, status) => { + // None of these is evidence. Writing on one is how a wrong classification outlives the + // session that caused it, because whatever lands on disk is what the next boot is targeted on. + await consensusSignedIn([stubContents(true)]) + + await consensusUnresolved(status) + + expect(nextLaunchBinding()).toBe(true) + }) + + it('does not revoke a staff grant when the last window closes', async () => { + // `unknown`, not `signed_out`. The identity engine rightly detaches telemetry here — an + // in-memory binding with nobody left to affirm it should stop claiming events — but the same + // signal must not reach the disk, or quitting the app would revoke the grant. + await consensusSignedIn([stubContents(true)]) + const afterSignIn = fs.statSync(persistFilePath()).mtimeMs + + await consensusUnresolved('unknown') + + expect(storedFile()).toMatchObject({ staff: true }) + expect(fs.statSync(persistFilePath()).mtimeMs).toBe(afterSignIn) + expect(nextLaunchBinding()).toBe(true) }) it.each([ ['a view with no auth store', () => stubContentsWithoutAuthStore()], ['a read that returned null', () => stubContentsReturning(null)], - ['a read that returned an unexpected shape', () => stubContentsReturning('nope')] + ['a read that returned an unexpected shape', () => stubContentsReturning('nope')], + ['a read that threw', () => stubContents(false, { throws: true })] ])('stays silent for %s rather than voting "not staff"', async (_label, make) => { // Absence of an auth record is not evidence of being signed out. A local install that was // never signed into must not clear a classification a signed-in view established — that // would be a wrong answer, not merely a racy one. + await consensusSignedIn([stubContents(true)]) + expect(nextLaunchBinding()).toBe(true) + pageReads = 0 + + await consensusSignedIn([make()]) + + // The abstention has to be a decision the view took, not a read that never happened. + expect(pageReads).toBeGreaterThan(0) + expect(nextLaunchBinding()).toBe(true) + }) + + it('treats a non-boolean verdict as not staff', async () => { + // Observed against a stored `true`, so "wrote nothing" cannot pass for "stored false". + await consensusSignedIn([stubContents(true)]) + expect(nextLaunchBinding()).toBe(true) + + await consensusSignedIn( + [stubContentsReturning({ known: true, staff: 'yes', userId: OTHER_USER })], + OTHER_USER + ) + + expect(storedFile()).toMatchObject({ staff: false }) + expect(nextLaunchBinding()).toBe(false) + }) + + it('survives a page-context read that throws, leaving the stored value alone', async () => { + // Fire-and-forget; an escaping rejection would be unhandled. A page that cannot be read must + // not revoke a grant. + await consensusSignedIn([stubContents(true)]) + + await expect( + refreshStaffFlagTargeting(stubContents(false, { throws: true })) + ).resolves.toBeUndefined() + expect(nextLaunchBinding()).toBe(true) + }) +}) + +// `dom-ready` no longer classifies on its own authority; it offers a freshly loaded view as a +// classifier for an account already agreed on. +describe('refreshStaffFlagTargeting', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it('classifies when the consensus resolved but nothing could answer for it yet', async () => { + // The retry path this exists for: the consensus resolved while the views it asked were + // mid-load or unreadable, and a later view can answer. + await consensusSignedIn([stubContentsWithoutAuthStore()]) + expect(fs.existsSync(persistFilePath())).toBe(false) + await refreshStaffFlagTargeting(stubContents(true)) + expect(nextLaunchBinding()).toBe(true) + }) - await refreshStaffFlagTargeting(make()) + it.each([ + ['nothing is agreed yet', 'unknown' as const], + ['a contributor is still resolving', 'pending' as const], + ['two views disagree', 'conflicted' as const] + ])('declines to classify while %s', async (_label, status) => { + await consensusUnresolved(status) + + await refreshStaffFlagTargeting(stubContents(true)) + + expect(fs.existsSync(persistFilePath())).toBe(false) + }) + + it('leaves a resolved sign-out to the consensus rather than one view reaching dom-ready', async () => { + // One view loading a document says nothing about the others. The sign-out is a fact about + // every contributor, and the observer is what acts on it. + await consensusSignedIn([stubContents(true)]) + + await refreshStaffFlagTargeting(stubContents(false, { userId: USER })) expect(nextLaunchBinding()).toBe(true) }) - it('retries the write on a later page load after a failure', async () => { + it('costs nothing once the agreed account is classified', async () => { + await consensusSignedIn([stubContents(true)]) + const executeJavaScript = vi.fn() + + await refreshStaffFlagTargeting({ executeJavaScript } as unknown as Electron.WebContents) + + expect(executeJavaScript).not.toHaveBeenCalled() + }) + + it('refuses a view that read a different account', async () => { + await consensusSignedIn([stubContentsWithoutAuthStore()]) + + await refreshStaffFlagTargeting(stubContents(true, { userId: OTHER_USER })) + + expect(fs.existsSync(persistFilePath())).toBe(false) + }) +}) + +describe('persisting the classification', () => { + beforeEach(() => { + initStaffFlagTargeting() + setFlagEvaluationStaff.mockClear() + }) + + it('retries the write on a later resolution after a failure', async () => { // The cache moves only after a successful write. Moving it first would record a write that // never landed, and the unchanged-classification check would then suppress every later // attempt — leaving the next launch reading the stale value even once the disk recovered. @@ -409,12 +911,16 @@ describe('refreshStaffFlagTargeting', () => { // succeed and this test would pass through the unchanged-classification path having proven // nothing. Blocking the staging path with a directory makes the rename fail for real. fs.mkdirSync(persistFilePath() + '.tmp', { recursive: true }) - await refreshStaffFlagTargeting(stubContents(true)) + await consensusSignedIn([stubContents(true)]) expect(fs.existsSync(persistFilePath())).toBe(false) + // The classification is already known for this account, so the retry must come from + // re-applying it rather than from asking the page again. + const executeJavaScript = vi.fn() fs.rmSync(persistFilePath() + '.tmp', { recursive: true, force: true }) - await refreshStaffFlagTargeting(stubContents(true)) + await refreshStaffFlagTargeting({ executeJavaScript } as unknown as Electron.WebContents) + expect(executeJavaScript).not.toHaveBeenCalled() expect(storedFile()).toMatchObject({ staff: true }) expect(nextLaunchBinding()).toBe(true) }) @@ -440,7 +946,7 @@ describe('refreshStaffFlagTargeting', () => { } initStaffFlagTargeting() - await refreshStaffFlagTargeting(stubContents(false)) + await consensusSignedIn([stubContents(false)]) fs.chmodSync(persistFilePath(), 0o644) expect(storedFile()).toMatchObject({ staff: false }) diff --git a/src/main/lib/staffFlagTargeting.ts b/src/main/lib/staffFlagTargeting.ts index 4d74d2494..ca3e693f4 100644 --- a/src/main/lib/staffFlagTargeting.ts +++ b/src/main/lib/staffFlagTargeting.ts @@ -9,27 +9,60 @@ * * ## What is stored, and what is not * - * A single boolean, in `/staff-targeting.json`. The address is compared inside the - * page (`CLASSIFY_STAFF_JS`) and only the derived boolean crosses the IPC boundary, so it is - * never persisted, never handed to `telemetry.ts`, and never present in main-process memory at - * all. The privacy claim is therefore structural rather than procedural: no module downstream of - * the page script is ever given an address, so none can leak one. + * A single boolean, in `/staff-targeting.json`. The ADDRESS is compared inside the + * page (`CLASSIFY_STAFF_JS`) and never crosses the IPC boundary at all, so it is never persisted, + * never handed to `telemetry.ts`, and never present in main-process memory. The privacy claim is + * therefore structural rather than procedural: no module downstream of the page script is ever + * given an address, so none can leak one. + * + * Besides the boolean the page returns the UID it classified, which main compares against the + * agreed account and discards. That is not a new class of data at this boundary: the same UID, + * from the same store, already crosses it continuously on the identity-consensus path + * (`reportFirebaseAuthState`). It is bounded in the page and re-checked by + * `normalizePostHogUserId`, the same gate consensus applies, so nothing unbounded lands in + * main-process memory or a crash dump — and no address is involved either way. * * The cost of that is deliberate and worth naming: the `@comfy.org` test lives in the CLIENT * (`STAFF_EMAIL_SUFFIX`), so changing which cohort is targeted needs a Desktop release rather * than a PostHog config edit. Sending the raw email instead would keep that flexibility, at the * price of a plaintext address at rest for every logged-in user. * + * ## Which account gets classified + * + * The one the whole process agrees on — `firebaseAuthIdentity.ts`'s consensus — not whichever + * view loaded a document most recently. A view is a single sample of a state several views + * contribute to, and reading one directly gets three things wrong at once: it cannot see a + * sign-out that never navigates, it has no way to reconcile two views signed into two accounts, + * and it will happily classify an account this process does not believe is signed in. + * + * So the outcome drives the classification: + * + * - **`signed_in`** — ask a view the consensus counts as holding that account, and accept its + * answer only if the page agrees it classified that same UID. + * - **`signed_out`** — store `false`. Every contributor resolved and none is signed in, which + * is the one state that is real evidence of a sign-out. + * - **`pending` / `conflicted` / `unknown`** — hold. A view mid-resolution, two views disagreeing + * about which account is signed in, and no view able to say at all are all *absence* of an + * answer. Writing one anyway is how a wrong classification outlives the session that caused + * it, because whatever lands on disk is what the next boot is targeted on. + * + * `unknown` matters more than it looks: closing the last window leaves nobody to affirm the + * account, and `firebaseAuthIdentity` rightly detaches telemetry there. Persisting `false` on the + * same signal would revoke a staff grant for quitting the app. + * * ## What this is NOT * - * NOT an authorization boundary. The classification comes from the page's own main world, so - * page-level code — a custom-node extension, or XSS on a hosted frontend — can forge a - * `firebase:authUser:*` record or patch the IndexedDB API and self-classify as staff. What that - * buys is bounded: the property only makes a person CONDITION evaluable, the server still - * decides, and `coreBetaGrants` will only ever add args already on its own allowlist. Nothing - * here should ever gate access, entitlement, or anything a user could want to forge their way - * into. Closing it properly means cross-checking against main's own Firebase identity - * (`firebaseAuthIdentity.ts`), which is a larger change than this one. + * Still NOT an authorization boundary, and the consensus does not make it one. Both the + * classification and the reports that consensus reconciles come from pages, reading the same + * IndexedDB, so page-level code — a custom-node extension, or XSS on a hosted frontend — that can + * forge a `firebase:authUser:*` record can forge both halves and self-classify as staff. What the + * cross-check removes is *non-hostile* wrongness: the last document to load deciding, a stale + * second record deciding, and a classification landing while two views disagree. + * + * What a forgery buys is unchanged and bounded: the property only makes a person CONDITION + * evaluable, the server still decides, and `coreBetaGrants` will only ever add args already on its + * own allowlist. Nothing here should ever gate access, entitlement, or anything a user could want + * to forge their way into. * * ## Why the boolean is persisted rather than resolved at boot * @@ -38,7 +71,7 @@ * learns a UID and no email at all — `flowShared.ts` attaches an email only on a fresh * desktop-driven sign-in. That is long after the boot flag fetch has answered. * - * So the classification is made whenever a view resolves auth, and read back at the NEXT boot, + * So the classification is made whenever the consensus resolves, and read back at the NEXT boot, * before the flag fetch. `userTier.ts` solves the same problem the same way. The consequence is * that a staff member's first launch after signing in is not targeted; the one after it is. * @@ -64,6 +97,13 @@ */ import path from 'path' import type { WebContents } from 'electron' +import { + getFirebaseIdentityConsensus, + observeFirebaseIdentityConsensus, + viewsReportingFirebaseUser, + type FirebaseIdentityConsensus +} from './firebaseAuthIdentity' +import { normalizePostHogUserId } from './opaqueIdentifier' import { configDir } from './paths' import { readFileSafe, writeFileSafe } from './safe-file' import * as telemetry from './telemetry' @@ -86,6 +126,38 @@ function persistFilePath(): string { * exists but could not be read — and an unknown value never suppresses a write. */ let cached: boolean | null = null +/** The account the in-memory classification belongs to, so returning to an account already + * classified this session costs no page read. `null` when the classification belongs to no + * account (a resolved sign-out) or when none has been made. */ +let classifiedUserId: string | null = null + +/** The classification held for `classifiedUserId`. `null` means the account is agreed but not yet + * classified — a page read is in flight, or every attempt at one failed. */ +let classifiedStaff: boolean | null = null + +/** Bumped on every consensus change. A page read is asynchronous and the account can be superseded + * while one is in flight; without this an answer about the account signed out a moment ago would + * be applied to whoever is signed in now. */ +let classificationGeneration = 0 + +/** The generation whose classification has already been accepted. Two reads can be in flight for + * one generation — a `dom-ready` retry alongside the consensus observer's own — and both would + * pass the generation check, so the slower one would overwrite the faster one's verdict purely on + * settle order. One accepted answer per consensus outcome; later arrivals for it are ignored. */ +let answeredGeneration: number | null = null + +let unobserveConsensus: (() => void) | null = null + +/** A page read that never settles must not keep a `WebContents` awaited forever, nor block the + * views behind it. `CLASSIFY_STAFF_JS` bounds its own `indexedDB.open`, but `databases()` and + * `getAll` are unbounded and a hostile page can replace either with a promise that never + * resolves. `executeJavaScript` has no timeout of its own. */ +const PAGE_READ_TIMEOUT_MS = 10_000 + +/** What `normalizePostHogUserId` will accept, applied to the raw string so trimming cannot sneak an + * over-length uid under the limit. The page caps at one past this, so a longer uid is rejected. */ +const MAX_PAGE_USER_ID_CHARS = 256 + /** * Read the stored classification and bind it for this launch's flag evaluation. * @@ -103,6 +175,9 @@ let cached: boolean | null = null export function initStaffFlagTargeting(): void { cached = readPersistedStaff() telemetry.setFlagEvaluationStaff(cached === true) + // Subscribed here rather than at module load so the wiring is explicit and ordered: this runs + // before any view exists, so no outcome can be missed, and a second call cannot double-subscribe. + unobserveConsensus ??= observeFirebaseIdentityConsensus(onIdentityConsensus) console.log('[staff-targeting] init: persisted=', cached) } @@ -123,9 +198,15 @@ function readPersistedStaff(): boolean | null { /** * Page-context classification of the signed-in account. * - * Returns only a BOOLEAN. The address is compared in the page and never crosses the IPC - * boundary, so the privacy claim above is structurally true rather than a convention — and no - * unbounded page-controlled string reaches main-process memory or a crash dump. + * Returns a BOOLEAN and the UID it is about — never the address, which is compared in the page and + * never crosses the IPC boundary, so the privacy claim above is structurally true rather than a + * convention. + * + * The UID is what lets main check that this page classified the account the process actually + * agrees is signed in. Bounded to 257 characters here — one past what `normalizePostHogUserId` + * will accept — so an over-length UID is REJECTED in main rather than silently truncated into a + * collision with a different account, and no unbounded page-controlled string reaches + * main-process memory or a crash dump. `null` when no account is stored. * * Three guards, each answering a way the naive read gets the cohort wrong: * @@ -203,14 +284,21 @@ export const CLASSIFY_STAFF_JS = `(async () => { if (!v || typeof v.uid !== 'string' || v.uid.length === 0) return; if (!uids[v.uid]) { uids[v.uid] = true; users.push(v); } }); - // No record at all is a real signed-out state and votes "not staff". - if (users.length === 0) return { known: true, staff: false }; + // No record at all is a real signed-out state and votes "not staff", for no account. + if (users.length === 0) return { known: true, staff: false, userId: null }; // Two accounts at once is unresolved, not a coin flip on iteration order. if (users.length > 1) return { known: false }; var user = users[0]; - if (user.emailVerified !== true) return { known: true, staff: false }; + // One past the 256 main will accept, so an over-length uid is REJECTED there rather than + // truncated into a match with a different account. + var userId = user.uid.slice(0, 257); + if (user.emailVerified !== true) return { known: true, staff: false, userId: userId }; var email = typeof user.email === 'string' ? user.email : ''; - return { known: true, staff: email.trim().toLowerCase().slice(-SUFFIX.length) === SUFFIX }; + return { + known: true, + staff: email.trim().toLowerCase().slice(-SUFFIX.length) === SUFFIX, + userId: userId + }; } catch (e) { return { known: false }; } finally { @@ -219,45 +307,16 @@ export const CLASSIFY_STAFF_JS = `(async () => { })()` /** - * Classify the view's signed-in account and store the result for the next launch. + * Bind a classification and carry it to the next launch. * - * Called for LOCAL installs as well as cloud ones, which matters more than it looks: the grant - * these flags carry is consumed only by the local launch path (`buildLaunchArgs`, launch.ts), - * because a cloud install has no launch command and spawns no Core. Binding on cloud views alone - * would target every surface except the one that can use the result. - * - * Fire-and-forget. Every failure leaves the stored classification exactly as it was, so a page - * that cannot be read cannot revoke a grant. - * - * A sign-out or a switch to a non-staff account stores `false`, so a machine that changes hands - * stops presenting as staff on the next launch. Combined with the boot evaluation being - * authoritative, that is also what lets the server take the grant back normally. - * - * KNOWN GAP, deliberate for now: this is driven by `dom-ready`, so an in-page sign-out that never - * navigates is not seen until the next navigation, reload, or launch, and the classification can - * stay `true` across that window. Reclassification is eventual, not immediate, and must not be - * described as immediate. The preload's `startLocalFirebaseAuthMonitor` already polls this same - * store on a 1s tick and would close the gap, but it reports a UID and no email, so wiring this - * to it — or to the identity consensus in `firebaseAuthIdentity.ts`, which is what properly - * reconciles several views — is a larger change than this one. + * The one place a classification reaches telemetry or the disk. Bound immediately even though + * this launch's flag fetch has long since gone out: a flag initialised later in the session (or + * re-read in a test) should see the current answer, and it costs nothing. */ -export async function refreshStaffFlagTargeting(webContents: WebContents): Promise { +function applyClassification(isStaff: boolean): void { + telemetry.setFlagEvaluationStaff(isStaff) + if (isStaff === cached) return try { - const read = (await webContents.executeJavaScript(CLASSIFY_STAFF_JS)) as { - known?: unknown - staff?: unknown - } | null - // A view with no Firebase store has NO OPINION and must stay silent. Absence of an auth - // record is not evidence of being signed out, and treating it as such lets a local install - // that was never signed into clear a classification a signed-in view established — a wrong - // answer, not merely a racy one. Only a view that can actually see auth state votes. - if (!read || read.known !== true) return - const isStaff = read.staff === true - // Bound immediately even though this launch's flag fetch has long since gone out: a flag - // initialised later in the session (or re-read in a test) should see the current answer, and - // it costs nothing. - telemetry.setFlagEvaluationStaff(isStaff) - if (isStaff === cached) return writeFileSafe(persistFilePath(), JSON.stringify({ staff: isStaff, ts: Date.now() })) // AFTER the write, never before. `writeFileSafe` can exhaust its retries on a transient lock // or an unavailable config dir, and the catch below swallows that. Moving `cached` first @@ -265,13 +324,197 @@ export async function refreshStaffFlagTargeting(webContents: WebContents): Promi // every later attempt at the same classification — so the next launch would read the stale // value even once the filesystem recovered. cached = isStaff - console.log('[staff-targeting] refresh: staff=', isStaff, '→ next launch') + console.log('[staff-targeting] classified: staff=', isStaff, '→ next launch') } catch (err) { - console.log('[staff-targeting] refresh skipped:', err) + console.log('[staff-targeting] store skipped:', err) + } +} + +/** + * Run the classification script in a view, giving up if the page does not answer. + * + * The timeout bounds THIS await, not the page's work — `executeJavaScript` cannot be cancelled, so + * a wedged page keeps its own promise. What it does buy is that one such page no longer holds up + * every view behind it, and no read is awaited for the life of the session. + */ +async function readClassificationFromPage(webContents: WebContents): Promise { + let timer: ReturnType | undefined + try { + return await Promise.race([ + webContents.executeJavaScript(CLASSIFY_STAFF_JS), + new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new Error('page read timed out')), PAGE_READ_TIMEOUT_MS) + timer.unref?.() + }) + ]) + } finally { + if (timer) clearTimeout(timer) + } +} + +/** + * Ask one view to classify `userId`, and accept its answer only if it agrees that is the account + * it read. + * + * The cross-check is the point. A view can be trusted to report auth state and still be the wrong + * one to ask: its store can hold a different account than the one consensus settled on (on Cloud + * the reporter is the frontend's own auth sync, a different source entirely from the IndexedDB + * this reads), or it can have changed underneath between the report and this read. Disagreement + * is not a failure to retry — it means this view is answering about somebody else. + * + * Returns whether a classification was applied, so a caller can move on to the next view. + */ +async function classifyFromView( + webContents: WebContents, + userId: string, + generation: number +): Promise { + let read: { known?: unknown; staff?: unknown; userId?: unknown } | null + try { + read = (await readClassificationFromPage(webContents)) as typeof read + } catch (err) { + // A page that cannot be read must not revoke a grant. + console.log('[staff-targeting] read skipped:', err) + return false + } + // The account can be superseded while the read is in flight. + if (generation !== classificationGeneration) return false + // Another read already answered for this outcome. Letting a second one through would make the + // verdict depend on which page happened to settle last. + if (answeredGeneration === generation) return false + // A view with no Firebase store has NO OPINION and must stay silent. Absence of an auth record + // is not evidence of being signed out, so only a view that can actually see auth state votes. + if (!read || read.known !== true) return false + // Bound the raw string BEFORE normalizing: `normalizePostHogUserId` trims and only then applies + // its 256-character limit, so a 257-character uid ending in whitespace would normalize down to + // 256 and be accepted — defeating the page-side cap that exists to reject rather than truncate. + if (typeof read.userId !== 'string' || read.userId.length > MAX_PAGE_USER_ID_CHARS) return false + if (normalizePostHogUserId(read.userId) !== userId) return false + const isStaff = read.staff === true + answeredGeneration = generation + classifiedUserId = userId + classifiedStaff = isStaff + applyClassification(isStaff) + return true +} + +/** + * Ask each view consensus counts as holding `userId` until one agrees it read that account. + * + * Sequential, so the common single-view case costs one page read. A view that does not answer no + * longer holds up the ones behind it: `readClassificationFromPage` bounds every call, and + * `classifyFromView` returns false on that timeout, so this loop moves on to the next view. + * + * ACCEPTED DEBT, decided rather than overlooked. First accepted answer wins, so where two views + * hold the same account and disagree — one store still carrying a verified `@comfy.org` address, + * another the updated or unverified one — the verdict depends on iteration order. Raised in review + * and kept deliberately: + * + * - The harm is bounded. This is cohort targeting, not authorization: the server evaluates the + * condition, and a grant only ever adds an arg from `CORE_BETA_GRANTABLE_ARGS`. The wrong + * tie-break costs a missed or spurious beta arg, never access to anything. + * - It is already less order-dependent than what it replaces, where classification ran per view + * on `dom-ready` with no UID check at all, so the last document to load decided — including a + * view signed into a different account. + * - Both alternatives introduce order-sensitivity of their own. Requiring agreement lets one + * stale or incomplete copy veto a correct `true`; preferring `true` biases toward granting. + * Choosing between them is really a decision about what `CLASSIFY_STAFF_JS` should return for + * a record with no email or an unverified one — the cohort rule this module inherited. + */ +async function classifyAgreedAccount(userId: string, generation: number): Promise { + for (const webContents of viewsReportingFirebaseUser(userId)) { + if (generation !== classificationGeneration) return + if (await classifyFromView(webContents, userId, generation)) return + } +} + +/** + * Drive the classification from the reconciled identity rather than from a page load. + * + * See the module note on which outcomes may write and which must hold. The short version: only + * `signed_out` and `signed_in` are evidence; `pending`, `conflicted` and `unknown` are the absence + * of an answer, and a persisted fact must not move on those. + */ +function onIdentityConsensus(consensus: FirebaseIdentityConsensus): void { + classificationGeneration += 1 + if (consensus.status === 'signed_out') { + // Every contributor resolved and none is signed in. This is what lets a machine that changes + // hands stop presenting as staff — and, with the boot evaluation authoritative, what lets the + // server take a grant back normally. + classifiedUserId = null + classifiedStaff = false + applyClassification(false) + return + } + if (consensus.status !== 'signed_in') return + if (classifiedUserId === consensus.userId && classifiedStaff !== null) { + // Already classified this session — the common case, since a navigation takes the consensus + // through `pending` and back. Bind the known answer FIRST, so the account keeps its + // classification with no gap and a write that exhausted `writeFileSafe`'s attempts is retried. + applyClassification(classifiedStaff) + // Then revalidate, because a UID is not a classification. `staff` is derived from `email` and + // `emailVerified`, both of which can change while Firebase keeps reporting the same UID — an + // address verified mid-session, or one that changes domain. Caching the verdict against the + // UID alone would make it immutable for the life of the process, which is stricter than the + // behaviour this replaces: the per-view read ran on every `dom-ready` and would have seen the + // change. Every document load takes the consensus through `pending` and back, so this runs on + // that same cadence and costs the same one page read. + void classifyAgreedAccount(consensus.userId, classificationGeneration) + return + } + classifiedUserId = consensus.userId + classifiedStaff = null + // A switch straight from one account to another with no resolved sign-out between them holds + // the outgoing account's classification for the duration of one page read. Held, not cleared: + // clearing would revoke a grant on a report that may yet turn out to be transient. + void classifyAgreedAccount(consensus.userId, classificationGeneration) +} + +/** + * Offer a freshly loaded view as a classifier for the account already agreed on. + * + * A retry path, not an authority. The consensus observer above is what normally classifies; this + * covers the case where it resolved while the views it asked could not answer — a page mid-load, + * an `executeJavaScript` that threw — and a later view can. It is a no-op unless an account is + * agreed and still unclassified, so the ordinary page load costs nothing. + * + * Called for LOCAL installs as well as cloud ones, which matters more than it looks: the grant + * these flags carry is consumed only by the local launch path (`buildLaunchArgs`, launch.ts), + * because a cloud install has no launch command and spawns no Core. Binding on cloud views alone + * would target every surface except the one that can use the result. + * + * Fire-and-forget. Every failure leaves the stored classification exactly as it was, so a page + * that cannot be read cannot revoke a grant. + */ +export async function refreshStaffFlagTargeting(webContents: WebContents): Promise { + const consensus = getFirebaseIdentityConsensus() + // A resolved sign-out is the observer's to DECIDE — it is a fact about every view, and one view + // reaching dom-ready says nothing about the others. But re-applying a decision already taken is + // a write retry, not a decision, and the revocation write is the one with no other retry path: + // `publishConsensus` is change-only so `signed_out` is not re-delivered while it stands, and a + // `writeFileSafe` that threw would otherwise leave `staff: true` on disk for every later launch + // — silently reversing the revocation this module exists to make. + if (consensus.status === 'signed_out') { + applyClassification(false) + return + } + if (consensus.status !== 'signed_in') return + if (classifiedUserId === consensus.userId && classifiedStaff !== null) { + // Nothing to ask this view — but a page load is also the moment to retry a write that + // `writeFileSafe` could not land, since the next launch reads whatever the disk holds. + applyClassification(classifiedStaff) + return } + await classifyFromView(webContents, consensus.userId, classificationGeneration) } /** @internal — exposed for tests. */ export function _resetForTest(): void { cached = null + classifiedUserId = null + classifiedStaff = null + classificationGeneration = 0 + answeredGeneration = null + unobserveConsensus?.() + unobserveConsensus = null }