From 555684bec752b6d6b79ba52c6da829838cded791 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 21 Sep 2026 22:23:43 -0700 Subject: [PATCH 1/6] refactor(telemetry): classify staff from the agreed account, not one view's page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1550 shipped with three documented limitations — an in-page sign-out it could not see, two views signed into two accounts it could not reconcile, and a classification it could not check against main's own identity. They are one defect: the classification was read from whichever view reached `dom-ready` last, and a view is a single sample of a state several views contribute to. `firebaseAuthIdentity.ts` already reconciles exactly the outcomes needed — pending, all-signed-out, conflicting, one agreed UID — but only ever spent them on telemetry side effects, so nothing else could be driven from them. Publish them, and drive the classification from the outcome instead of from a page load. An in-page sign-out is now seen without a navigation, because both surfaces already report auth state continuously: loopback through the preload's 1s IndexedDB poll, Cloud through the frontend's own auth sync. Two views signed into two accounts publish `conflicted`, which holds the stored classification rather than letting the later document win. And the page read returns the UID it classified, which is compared against the agreed account and discarded, so a view whose store holds somebody else no longer gets a vote. `unknown` is deliberately a separate outcome from `signed_out`. Reconcile collapses them today because for an in-memory binding they are the same — with no contributor left to affirm a user, telemetry should stop claiming events. For a PERSISTED fact they are opposites: closing the last window is not evidence that anybody signed out, and writing `false` on it would revoke a staff grant for quitting the app. This is NOT a security fix and the module says so. Both the classification and the reports consensus reconciles come from pages reading the same IndexedDB, so a page that can forge one can forge both. What the cross-check removes is non-hostile wrongness: last-document-loaded deciding, a stale second record deciding, and a classification landing while two views disagree. The server still decides, and `coreBetaGrants` still only ever adds args already on its own allowlist. `dom-ready` still calls `refreshStaffFlagTargeting`, re-framed as a retry rather than an authority: it offers that view as a classifier for the account already agreed on, and is a no-op otherwise. It is also where a `writeFileSafe` that exhausted its retries gets another attempt, which the consensus path alone would have lost. Two behaviours documented rather than fixed. 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 — clearing instead would revoke a grant on a report that may yet prove transient. And `classifyAgreedAccount` is sequential, so a wedged renderer defers the classification to the next resolution rather than falling through to the next view; that degrades to the eventual-consistency contract this module already documents, never to a wrong answer. `userTier.ts` has the identical per-view shape and the same staleness. Left alone deliberately — it is a separate consumer and a separate change. Co-Authored-By: Claude Opus 5 --- src/main/host/attach.ts | 15 +- src/main/index.ts | 4 +- src/main/lib/firebaseAuthIdentity.test.ts | 209 ++++++++++- src/main/lib/firebaseAuthIdentity.ts | 107 ++++++ src/main/lib/staffFlagTargeting.test.ts | 429 +++++++++++++++++++--- src/main/lib/staffFlagTargeting.ts | 284 +++++++++++--- 6 files changed, 926 insertions(+), 122 deletions(-) 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..541b3fcd2 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,206 @@ 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 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..e051df14d 100644 --- a/src/main/lib/firebaseAuthIdentity.ts +++ b/src/main/lib/firebaseAuthIdentity.ts @@ -78,8 +78,39 @@ 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, untrusted, or wedged past its deadline. */ + | { 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 +119,69 @@ 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 it. + * 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]) { + 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 + if (!views.includes(webContents)) views.push(webContents) + } + return views +} + function originOf(url: string): string | null { try { return new URL(url).origin @@ -361,11 +455,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 +478,7 @@ function reconcile(): void { // binding even when this process has not bound a UID yet. detachToAnonymousIdentity() clearUnmergeableEpoch() + publishConsensus({ status: 'signed_out' }) return } @@ -386,14 +486,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 +827,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..486da0535 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,14 +25,55 @@ 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' + /** 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 }) + opts.throws + ? Promise.reject(new Error('page gone')) + : Promise.resolve({ known: true, staff, userId: opts.userId ?? USER }) } as unknown as Electron.WebContents } @@ -44,6 +91,37 @@ function stubContentsReturning(result: unknown): Electron.WebContents { } 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') } @@ -65,6 +143,7 @@ beforeEach(() => { testConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'staff-targeting-')) setFlagEvaluationStaff.mockClear() _resetForTest() + identity.reset() }) afterEach(() => { @@ -122,14 +201,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 +229,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 +237,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 +261,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 +295,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 +375,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 +390,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 +415,308 @@ 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)]) - expect(setFlagEvaluationStaff).toHaveBeenLastCalledWith(true) + await consensusSignedIn([stubContents(false, { userId: OTHER_USER })], OTHER_USER) + + expect(nextLaunchBinding()).toBe(false) }) - 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. - await refreshStaffFlagTargeting(stubContents(true)) + it('re-binds the agreed account across a pending outcome without reading a page again', async () => { + // The common shape, since every navigation takes the consensus through `pending` and back. + // Re-reading a page on each one would be a poll with extra steps, and the answer must not + // depend on that view still being readable by then. + await consensusSignedIn([stubContents(true)]) + await consensusUnresolved('pending') + setFlagEvaluationStaff.mockClear() - await expect( - refreshStaffFlagTargeting(stubContents(false, { throws: true })) - ).resolves.toBeUndefined() + await consensusSignedIn([], USER) + + expect(setFlagEvaluationStaff).toHaveBeenLastCalledWith(true) expect(nextLaunchBinding()).toBe(true) }) +}) - it('treats a non-boolean verdict as not staff', async () => { - await refreshStaffFlagTargeting(stubContentsReturning({ known: true, staff: 'yes' })) +// 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) + + await consensusSignedIn([stubContents(false, { userId: OTHER_USER })], USER) + + expect(nextLaunchBinding()).toBe(true) + }) + + 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('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) + + await consensusSignedIn([make()]) + + 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 +726,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 +761,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..6b0800e86 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,22 @@ 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 + +let unobserveConsensus: (() => void) | null = null + /** * Read the stored classification and bind it for this launch's flag evaluation. * @@ -103,6 +159,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 +182,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 +268,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 +291,16 @@ export const CLASSIFY_STAFF_JS = `(async () => { })()` /** - * Classify the view's signed-in account and store the result for 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. + * Bind a classification and carry it to the next launch. * - * 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 +308,136 @@ 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] store skipped:', err) + } +} + +/** + * 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 webContents.executeJavaScript(CLASSIFY_STAFF_JS)) as typeof read } catch (err) { - console.log('[staff-targeting] refresh skipped:', 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 + // 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 + if (normalizePostHogUserId(read.userId) !== userId) return false + const isStaff = read.staff === true + 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. The trade-off: `executeJavaScript` + * has no timeout of its own, so a wedged renderer first in the list defers the classification + * rather than falling through to the next view. That degrades to the eventual-consistency contract + * this module already documents — the next resolution, or the next page load via + * `refreshStaffFlagTargeting`, retries — and never to a wrong answer. + */ +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. Re-apply rather than re-read: it costs no page read, it re-binds + // the answer, and it is the retry for a write that exhausted `writeFileSafe`'s attempts. + applyClassification(classifiedStaff) + 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 act on: it is a fact about every view, and one view + // reaching dom-ready says nothing about the others. + 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 + unobserveConsensus?.() + unobserveConsensus = null } From 78cf4382c05e06fa4536f8af5ac8b3f7f229d13b Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 21 Sep 2026 22:45:10 -0700 Subject: [PATCH 2/6] fix(telemetry): revalidate a returning account, since a UID is not a classification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caching the verdict against the UID alone made it immutable for the life of the process. `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 — so a stale boolean could be persisted and target the next launch. That was stricter than the behaviour this PR replaces, which is what makes it a regression rather than a known limit: the per-view read ran on every `dom-ready` and would have seen the change. The known answer is still bound FIRST, so the account keeps its classification with no gap and a write that exhausted `writeFileSafe`'s attempts is still retried; the revalidation follows. Every document load takes the consensus through `pending` and back, so it runs on the same cadence the per-view read did and costs the same one page read. Found by Codex review on 555684be. Co-Authored-By: Claude Opus 5 --- src/main/lib/staffFlagTargeting.test.ts | 35 ++++++++++++++++++++++--- src/main/lib/staffFlagTargeting.ts | 12 +++++++-- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/main/lib/staffFlagTargeting.test.ts b/src/main/lib/staffFlagTargeting.test.ts index 486da0535..077d837c8 100644 --- a/src/main/lib/staffFlagTargeting.test.ts +++ b/src/main/lib/staffFlagTargeting.test.ts @@ -474,10 +474,10 @@ describe('classification driven by the identity consensus', () => { expect(nextLaunchBinding()).toBe(false) }) - it('re-binds the agreed account across a pending outcome without reading a page again', async () => { + 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. - // Re-reading a page on each one would be a poll with extra steps, and the answer must not - // depend on that view still being readable by then. + // 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() @@ -487,6 +487,35 @@ describe('classification driven by the identity consensus', () => { expect(setFlagEvaluationStaff).toHaveBeenLastCalledWith(true) expect(nextLaunchBinding()).toBe(true) }) + + 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. + await consensusSignedIn([stubContents(true)]) + expect(nextLaunchBinding()).toBe(true) + await consensusUnresolved('pending') + + await consensusSignedIn([stubContents(false)], USER) + + expect(storedFile()).toMatchObject({ staff: false }) + expect(nextLaunchBinding()).toBe(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 three limitations #1550 shipped with, each of which is the same root cause: a classification diff --git a/src/main/lib/staffFlagTargeting.ts b/src/main/lib/staffFlagTargeting.ts index 6b0800e86..7828f7112 100644 --- a/src/main/lib/staffFlagTargeting.ts +++ b/src/main/lib/staffFlagTargeting.ts @@ -389,9 +389,17 @@ function onIdentityConsensus(consensus: FirebaseIdentityConsensus): void { 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. Re-apply rather than re-read: it costs no page read, it re-binds - // the answer, and it is the retry for a write that exhausted `writeFileSafe`'s attempts. + // 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 From e741b67bd5201c4ce07fbf671550b5c63a5ab167 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 21 Sep 2026 22:53:01 -0700 Subject: [PATCH 3/6] test(telemetry): make the no-flap test detect a flap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `toHaveBeenCalledWith(true)` still passes if the revalidation emits `false` and then `true`, so the test proved only that a `true` was bound synchronously — not what its name claims. The mock is cleared before the transition and the known answer is bound synchronously, so no `false` belongs anywhere in that window. Found by CodeRabbit on 78cf4382. Co-Authored-By: Claude Opus 5 --- src/main/lib/staffFlagTargeting.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/lib/staffFlagTargeting.test.ts b/src/main/lib/staffFlagTargeting.test.ts index 077d837c8..381dbeabe 100644 --- a/src/main/lib/staffFlagTargeting.test.ts +++ b/src/main/lib/staffFlagTargeting.test.ts @@ -515,6 +515,10 @@ describe('classification driven by the identity consensus', () => { 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) }) }) From e0e95bb5d5b52bfe8305f0f73b4254c5f9579f48 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 21 Sep 2026 23:25:44 -0700 Subject: [PATCH 4/6] fix(telemetry): one answer per consensus outcome, and a retry for the revocation write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the Cursor panel (8/8 reviewers, 10 findings against 555684be). Two High: A `dom-ready` retry ran with the CURRENT generation, so it could be in flight alongside the consensus observer's own read for the same outcome. Both passed the generation check and both applied, so the verdict was decided by which page settled last. One accepted answer per outcome now; later arrivals for it are ignored. The revocation write was the only one with no retry path. `publishConsensus` is change-only, so a resolved `signed_out` is not re-delivered while it stands, and `refreshStaffFlagTargeting` returned early for every non-`signed_in` status — a `writeFileSafe` that threw would leave `staff: true` on disk for every later launch, silently reversing the revocation the module exists to make. Re-applying a decision the consensus already took is a write retry, not a decision, so `dom-ready` now retries it; taking one on a single view's say-so would still be a decision, and is still refused. Also: bound the page read with a main-process timeout, since `executeJavaScript` has none 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. The timeout cannot cancel the page's work, but one wedged view no longer blocks every view behind it. Bound the raw uid before normalizing, because `normalizePostHogUserId` trims and only then applies its 256-character limit, so a 257-character uid ending in whitespace normalized to 256 and matched. In `firebaseAuthIdentity.ts`: apply the same origin revalidation to `viewsReportingFirebaseUser` that `reconcile()` applies to that map, so a view whose document has moved is not named as holding the account; stop dispatching an outcome a nested publish has superseded; and correct the `unknown` doc comment, which claimed to cover a view wedged past its deadline — `reconcile()` deliberately holds the previous outcome there instead. Tests: the panel also caught that several tests passed vacuously. Stub views now count how often they are actually read, and the tests that exist to prove a view was CONSULTED assert on that count. That found a real gap — the test for the previous commit's revalidation fix called `nextLaunchBinding()` part-way through, which resets the module and sent the second resolution down the first-classification path, so it never exercised the short-circuit it was written for and passed with the fix reverted. Rewritten; verified it now fails without it. Co-Authored-By: Claude Opus 5 --- src/main/lib/firebaseAuthIdentity.test.ts | 40 ++++++ src/main/lib/firebaseAuthIdentity.ts | 21 ++- src/main/lib/staffFlagTargeting.test.ts | 163 +++++++++++++++++++++- src/main/lib/staffFlagTargeting.ts | 61 +++++++- 4 files changed, 273 insertions(+), 12 deletions(-) diff --git a/src/main/lib/firebaseAuthIdentity.test.ts b/src/main/lib/firebaseAuthIdentity.test.ts index 541b3fcd2..853810a28 100644 --- a/src/main/lib/firebaseAuthIdentity.test.ts +++ b/src/main/lib/firebaseAuthIdentity.test.ts @@ -1353,6 +1353,46 @@ describe('firebaseAuthIdentity published consensus', () => { 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) diff --git a/src/main/lib/firebaseAuthIdentity.ts b/src/main/lib/firebaseAuthIdentity.ts index e051df14d..941bea4e7 100644 --- a/src/main/lib/firebaseAuthIdentity.ts +++ b/src/main/lib/firebaseAuthIdentity.ts @@ -96,7 +96,10 @@ interface MainVerifiedAuthState { * account it is serving - not a coin flip on whichever view reported last. */ export type FirebaseIdentityConsensus = - /** No contributor can say: every view is gone, untrusted, or wedged past its deadline. */ + /** 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' } @@ -132,13 +135,20 @@ function sameConsensus( * * 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 it. - * An observer that throws must not take the identity engine down with it. + * 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) { @@ -177,6 +187,11 @@ export function viewsReportingFirebaseUser(userId: string): 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 diff --git a/src/main/lib/staffFlagTargeting.test.ts b/src/main/lib/staffFlagTargeting.test.ts index 381dbeabe..a95a61750 100644 --- a/src/main/lib/staffFlagTargeting.test.ts +++ b/src/main/lib/staffFlagTargeting.test.ts @@ -64,30 +64,46 @@ const { initStaffFlagTargeting, refreshStaffFlagTargeting, CLASSIFY_STAFF_JS, _r 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; userId?: string } = {} ): Electron.WebContents { return { - executeJavaScript: () => - opts.throws + 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 } @@ -142,6 +158,7 @@ function nextLaunchBinding(): boolean { beforeEach(() => { testConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'staff-targeting-')) setFlagEvaluationStaff.mockClear() + pageReads = 0 _resetForTest() identity.reset() }) @@ -494,14 +511,20 @@ describe('classification driven by the identity consensus', () => { // 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(nextLaunchBinding()).toBe(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 }) - expect(nextLaunchBinding()).toBe(false) }) it('binds the known answer before revalidating, so a returning account never flaps', async () => { @@ -562,12 +585,29 @@ describe('what reading a single view got wrong', () => { // 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([ @@ -607,6 +647,114 @@ describe('what reading a single view got wrong', () => { }) }) +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 }) + + slow.release?.({ known: true, staff: false, userId: USER }) + await settle() + + expect(storedFile()).toMatchObject({ staff: true }) + expect(nextLaunchBinding()).toBe(true) + }) + + 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 has no other path back', async () => { + // The revocation is the write with no second chance: `publishConsensus` is change-only so a + // resolved `signed_out` is not re-delivered while it stands, and nothing else revisits it. 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() @@ -652,9 +800,12 @@ describe('outcomes that are the absence of an answer', () => { // 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) }) diff --git a/src/main/lib/staffFlagTargeting.ts b/src/main/lib/staffFlagTargeting.ts index 7828f7112..a94b58124 100644 --- a/src/main/lib/staffFlagTargeting.ts +++ b/src/main/lib/staffFlagTargeting.ts @@ -140,8 +140,24 @@ let classifiedStaff: boolean | null = null * 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. * @@ -314,6 +330,28 @@ function applyClassification(isStaff: boolean): void { } } +/** + * 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. @@ -333,7 +371,7 @@ async function classifyFromView( ): Promise { let read: { known?: unknown; staff?: unknown; userId?: unknown } | null try { - read = (await webContents.executeJavaScript(CLASSIFY_STAFF_JS)) as typeof read + 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) @@ -341,11 +379,19 @@ async function classifyFromView( } // 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) @@ -428,8 +474,16 @@ function onIdentityConsensus(consensus: FirebaseIdentityConsensus): void { */ export async function refreshStaffFlagTargeting(webContents: WebContents): Promise { const consensus = getFirebaseIdentityConsensus() - // A resolved sign-out is the observer's to act on: it is a fact about every view, and one view - // reaching dom-ready says nothing about the others. + // 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 @@ -446,6 +500,7 @@ export function _resetForTest(): void { classifiedUserId = null classifiedStaff = null classificationGeneration = 0 + answeredGeneration = null unobserveConsensus?.() unobserveConsensus = null } From fb217efa5bf157e1f02f8cd6c7594089345cf134 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Tue, 22 Sep 2026 03:02:51 -0700 Subject: [PATCH 5/6] docs(telemetry): correct two comments that outlived what they described MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from review, and both the same mistake: prose written to document a limitation, left asserting it after the same commit removed the limitation. `classifyAgreedAccount` still said `executeJavaScript` has no timeout and that a wedged first renderer defers the classification rather than falling through. `readClassificationFromPage`, added immediately above it, bounds every call, and `classifyFromView` returns false on that timeout so the loop moves to the next view — which a test in the same commit pins. The docblock now describes the behaviour that exists, and names the first-accepted-answer ordering as a deliberate deferral rather than leaving it unsaid. The revocation-retry test claimed nothing revisits the write. The consensus does not — `publishConsensus` is change-only, so an unchanged `signed_out` is never redelivered — but `refreshStaffFlagTargeting` re-applies it on every later `dom-ready`, and that is the path this very test drives. Narrowed to say what is true, and the test name with it. Co-Authored-By: Claude Opus 5 --- src/main/lib/staffFlagTargeting.test.ts | 9 +++++---- src/main/lib/staffFlagTargeting.ts | 15 ++++++++++----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/main/lib/staffFlagTargeting.test.ts b/src/main/lib/staffFlagTargeting.test.ts index a95a61750..72478ec61 100644 --- a/src/main/lib/staffFlagTargeting.test.ts +++ b/src/main/lib/staffFlagTargeting.test.ts @@ -696,10 +696,11 @@ describe('retrying a write that did not land', () => { setFlagEvaluationStaff.mockClear() }) - it('retries the revocation write, which has no other path back', async () => { - // The revocation is the write with no second chance: `publishConsensus` is change-only so a - // resolved `signed_out` is not re-delivered while it stands, and nothing else revisits it. A - // `writeFileSafe` that threw would leave `staff: true` on disk for every later launch — + 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 }) diff --git a/src/main/lib/staffFlagTargeting.ts b/src/main/lib/staffFlagTargeting.ts index a94b58124..7d0ea8395 100644 --- a/src/main/lib/staffFlagTargeting.ts +++ b/src/main/lib/staffFlagTargeting.ts @@ -401,11 +401,16 @@ async function classifyFromView( /** * 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. The trade-off: `executeJavaScript` - * has no timeout of its own, so a wedged renderer first in the list defers the classification - * rather than falling through to the next view. That degrades to the eventual-consistency contract - * this module already documents — the next resolution, or the next page load via - * `refreshStaffFlagTargeting`, retries — and never to a wrong answer. + * 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. + * + * First accepted answer wins. Where two views hold the same account and disagree — one store still + * carrying a verified `@comfy.org` address, another the updated or unverified one — that makes the + * verdict depend on iteration order. Raised in review and deferred deliberately, not overlooked: + * whether to require agreement, or to define a freshness authority, is a question about what + * `CLASSIFY_STAFF_JS` should say for an incomplete record, which is the cohort rule this module + * inherited rather than something this abstraction can settle on its own. */ async function classifyAgreedAccount(userId: string, generation: number): Promise { for (const webContents of viewsReportingFirebaseUser(userId)) { From eeb96522d0b0453fbc3c5d720c14ebf487fdd652 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Tue, 22 Sep 2026 11:13:12 -0700 Subject: [PATCH 6/6] docs(telemetry): record the same-UID tie-break as accepted debt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised by the Cursor panel and again by review: where two views hold the same account and their IndexedDB copies disagree on the email or its verification, the first accepted answer wins, so the verdict depends on iteration order. Kept, deliberately, and the docblock now says why rather than leaving a reader to wonder whether it was noticed. The harm is bounded — cohort targeting, not authorization; the server evaluates the condition and a grant only ever adds an arg from `CORE_BETA_GRANTABLE_ARGS`, so the wrong tie-break costs a missed or spurious beta arg and never access. It is already less order-dependent than what it replaces, where classification ran per view on `dom-ready` with no UID check at all. And both alternatives carry order-sensitivity of their own: requiring agreement lets one stale copy veto a correct `true`, and preferring `true` biases toward granting. Choosing between those is a decision about what `CLASSIFY_STAFF_JS` should return for an incomplete record — the inherited cohort rule, not something this layer settles. No behaviour change. Co-Authored-By: Claude Opus 5 --- src/main/lib/staffFlagTargeting.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/main/lib/staffFlagTargeting.ts b/src/main/lib/staffFlagTargeting.ts index 7d0ea8395..ca3e693f4 100644 --- a/src/main/lib/staffFlagTargeting.ts +++ b/src/main/lib/staffFlagTargeting.ts @@ -405,12 +405,21 @@ async function classifyFromView( * 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. * - * First accepted answer wins. Where two views hold the same account and disagree — one store still - * carrying a verified `@comfy.org` address, another the updated or unverified one — that makes the - * verdict depend on iteration order. Raised in review and deferred deliberately, not overlooked: - * whether to require agreement, or to define a freshness authority, is a question about what - * `CLASSIFY_STAFF_JS` should say for an incomplete record, which is the cohort rule this module - * inherited rather than something this abstraction can settle on its own. + * 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)) {