diff --git a/electron/authCallback.ts b/electron/authCallback.ts index 3b733778..0beb21dc 100644 --- a/electron/authCallback.ts +++ b/electron/authCallback.ts @@ -56,7 +56,7 @@ export function createAuthCallbackController({ const startupUrls: string[] = []; const startupSeen = new Set(); const pendingByOwner = new Map(); - const flowOwners = new Map(); + const flowOwners = new Map(); const observedOwners = new WeakSet(); const relay = flowRoutingEnabled ? createAuthFlowRelay({ @@ -91,19 +91,29 @@ export function createAuthCallbackController({ /^sb-[a-z0-9-]{1,100}-auth-token-flow-([A-Za-z0-9_-]{8,64})-code-verifier$/.exec(key)?.[1]; const flowStorageHooks: AuthFlowStorageHooks | undefined = relay ? { - prepareSet: async (event, key) => { + prepareSet: async (event, key, persistedKeys) => { const flowId = flowKey(key); if (!flowId) return; + if (persistedKeys) { + const present = new Set(persistedKeys); + for (const [id, flow] of flowOwners) { + if (present.has(flow.verifierKey)) continue; + // A peer SDK may have explicitly evicted this verifier. Index + // absence alone never proves inactivity; exact slot absence does. + await relay.release(id, flow.owner); + if (flowOwners.get(id) === flow) flowOwners.delete(id); + } + } const owner = event.sender; - const alreadyOwned = flowOwners.get(flowId) === owner; + const alreadyOwned = flowOwners.get(flowId)?.owner === owner; const rollback = await relay.register(flowId, key, owner); - flowOwners.set(flowId, owner); + flowOwners.set(flowId, { owner, verifierKey: key }); if (!observedOwners.has(owner)) { observedOwners.add(owner); owner.once("destroyed", () => { pendingByOwner.delete(owner); for (const [id, candidate] of flowOwners) - if (candidate === owner) { + if (candidate.owner === owner) { flowOwners.delete(id); void relay.release(id, owner).catch(() => undefined); } @@ -111,16 +121,16 @@ export function createAuthCallbackController({ } return async () => { await rollback(); - if (!alreadyOwned && flowOwners.get(flowId) === owner) + if (!alreadyOwned && flowOwners.get(flowId)?.owner === owner) flowOwners.delete(flowId); }; }, didRemove: async (_event, key) => { const flowId = flowKey(key); - const owner = flowId && flowOwners.get(flowId); - if (flowId && owner) { + const flow = flowId && flowOwners.get(flowId); + if (flowId && flow && flow.verifierKey === key) { flowOwners.delete(flowId); - await relay.release(flowId, owner); + await relay.release(flowId, flow.owner); } }, } diff --git a/electron/authCallbackFlowRouting.test.ts b/electron/authCallbackFlowRouting.test.ts index 48d2201f..2095dc96 100644 --- a/electron/authCallbackFlowRouting.test.ts +++ b/electron/authCallbackFlowRouting.test.ts @@ -405,3 +405,48 @@ it("cleans a destroyed registered owner's routing without transferring its pendi expect(state.flows.has("flow_alice")).toBe(false); controller.close(); }); + +it("preserves active unindexed registrations and prunes only exact verifiers missing after a peer deletion", async () => { + const { createAuthCallbackController } = await import("./authCallback"); + const controller = createAuthCallbackController({ + isDev: false, + focusApp: vi.fn(), + flowRoutingEnabled: true, + }); + const alice = owner(1), + bob = owner(2); + state.windows = [alice, bob]; + const aliceKey = "sb-recordlytest-auth-token-flow-flow_alice-code-verifier"; + const bobKey = "sb-recordlytest-auth-token-flow-flow_bob00-code-verifier"; + await controller.flowStorageHooks!.prepareSet(event(alice), aliceKey, []); + // No SDK index is present in this snapshot. The exact active slot suffices. + await controller.flowStorageHooks!.prepareSet(event(bob), bobKey, [aliceKey]); + expect(state.flows.has("flow_alice")).toBe(true); + expect(await controller.route(url)).toBe("local"); + expect(alice.send).toHaveBeenCalledWith("auth:callback", url); + const nextKey = "sb-recordlytest-auth-token-flow-flow_next0-code-verifier"; + await controller.flowStorageHooks!.prepareSet(event(bob), nextKey, [bobKey]); + expect(state.flows.has("flow_alice")).toBe(false); + expect(state.flows.has("flow_bob00")).toBe(true); + expect(state.flows.has("flow_next0")).toBe(true); + controller.close(); +}); + +it("does not release an owned flow when another project removes the same textual flow ID", async () => { + const { createAuthCallbackController } = await import("./authCallback"); + const controller = createAuthCallbackController({ + isDev: false, + focusApp: vi.fn(), + flowRoutingEnabled: true, + }); + const alice = owner(1); + state.windows = [alice]; + await controller.flowStorageHooks!.prepareSet(event(alice), key, []); + await controller.flowStorageHooks!.didRemove( + event(alice), + "sb-other-auth-token-flow-flow_alice-code-verifier", + ); + expect(state.flows.has("flow_alice")).toBe(true); + expect(await controller.route(url)).toBe("local"); + controller.close(); +}); diff --git a/electron/authStorage.test.ts b/electron/authStorage.test.ts index 9a922e4e..dd4a2abf 100644 --- a/electron/authStorage.test.ts +++ b/electron/authStorage.test.ts @@ -206,7 +206,11 @@ it("binds the installed SDK's actual redirect flow ID to its trusted initiating const redirect = new URL(new URL(result.data.url!).searchParams.get("redirect_to")!); const flowId = redirect.searchParams.get("sb_flow_id"); expect(flowId).toMatch(/^[A-Za-z0-9_-]{8,64}$/); - expect(prepared).toHaveBeenCalledWith(event, `${storageKey}-flow-${flowId}-code-verifier`); + expect(prepared).toHaveBeenCalledWith( + event, + `${storageKey}-flow-${flowId}-code-verifier`, + expect.any(Array), + ); expect(await invoke("get", `${storageKey}-flow-${flowId}-code-verifier`)).toBeTruthy(); await invoke("remove", `${storageKey}-flow-${flowId}-code-verifier`); expect(removed).toHaveBeenCalledWith(event, `${storageKey}-flow-${flowId}-code-verifier`); @@ -367,7 +371,23 @@ it("bounds per-account consent retries without evicting sessions, verifiers, or expect(await invoke("get", `${prefix}-signup-consent-retry:account-${n}`)).toBe( `retry-${n}`, ); - for (const [key, value] of entries) expect(await invoke("get", key)).toBe(value); + for (const [key, value] of entries) { + if (key === `${prefix}-flows-code-verifier`) + expect(JSON.parse((await invoke("get", key)) as string)).toEqual([ + "pending00", + "pending01", + "pending02", + "pending03", + "pending04", + ]); + else expect(await invoke("get", key)).toBe(value); + } + // Repair is read-only: the original bookkeeping value was not replaced. + expect( + JSON.parse( + await fs.readFile(path.join(native.directory, "private-auth/session.enc"), "utf8"), + ).entries[`${prefix}-flows-code-verifier`], + ).toBe("flow-index"); }); it.each([ @@ -399,3 +419,186 @@ it("fails closed on an externally corrupted retry count without replacing the en await expect(invoke("set", prefix, "must-not-replace")).rejects.toThrow("Could not unlock"); expect(await fs.readFile(file, "utf8")).toBe(corrupt); }); + +it("repairs a lost flow-index update from exact persisted slots without deleting active verifiers", async () => { + const prefix = "sb-recordlytest-auth-token"; + const first = `${prefix}-flow-flow_alice-code-verifier`; + const second = `${prefix}-flow-flow_bob00-code-verifier`; + const other = "sb-other-auth-token-flow-flow_other-code-verifier"; + await invoke("set", first, '"alice-verifier"'); + await invoke("set", second, '"bob-verifier"'); + await invoke("set", other, '"other-verifier"'); + await invoke( + "set", + `${prefix}-flows-code-verifier`, + JSON.stringify(["flow_bob00", "flow_absent", "flow_bob00"]), + ); + expect(JSON.parse((await invoke("get", `${prefix}-flows-code-verifier`)) as string)).toEqual([ + "flow_bob00", + "flow_alice", + ]); + expect(await invoke("get", first)).toBe('"alice-verifier"'); + expect(await invoke("get", second)).toBe('"bob-verifier"'); + expect(await invoke("get", other)).toBe('"other-verifier"'); + await invoke("set", `${prefix}-flows-code-verifier`, "invalid JSON"); + expect(JSON.parse((await invoke("get", `${prefix}-flows-code-verifier`)) as string)).toEqual([ + "flow_alice", + "flow_bob00", + ]); +}); + +it("keeps nested project namespaces out of another project's repaired flow index", async () => { + const prefix = "sb-a-auth-token"; + const nestedPrefix = `${prefix}-flow-nested-auth-token`; + const ownKey = `${prefix}-flow-current0-code-verifier`; + const nestedKey = `${nestedPrefix}-flow-abcdefgh-code-verifier`; + await invoke("set", ownKey, '"own-verifier"'); + await invoke("set", nestedKey, '"nested-project-verifier"'); + // The broad flow-ID grammar could treat this whole suffix as one ID. Use + // the same full greedy project-prefix parser as native route ownership. + await invoke( + "set", + `${prefix}-flows-code-verifier`, + JSON.stringify(["current0", "nested-auth-token-flow-abcdefgh"]), + ); + expect(JSON.parse((await invoke("get", `${prefix}-flows-code-verifier`)) as string)).toEqual([ + "current0", + ]); + expect( + JSON.parse((await invoke("get", `${nestedPrefix}-flows-code-verifier`)) as string), + ).toEqual(["abcdefgh"]); + expect(await invoke("get", nestedKey)).toBe('"nested-project-verifier"'); + expect(await invoke("get", ownKey)).toBe('"own-verifier"'); +}); + +it("does not register against an empty snapshot when saved auth cannot be read", async () => { + const prepare = vi.fn(async () => undefined); + const { registerAuthStorageHandlers } = await import("./authStorage"); + registerAuthStorageHandlers({ prepareSet: prepare, didRemove: async () => undefined }); + const prefix = "sb-recordlytest-auth-token"; + await invoke("set", prefix, "session"); + prepare.mockClear(); + const file = path.join(native.directory, "private-auth/session.enc"); + await fs.writeFile(file, "corrupt saved credentials"); + await expect( + invoke("set", `${prefix}-flow-flow_alice-code-verifier`, '"verifier"'), + ).rejects.toThrow("Could not unlock"); + expect(prepare).not.toHaveBeenCalled(); + expect(await fs.readFile(file, "utf8")).toBe("corrupt saved credentials"); +}); + +it("holds the local registration queue through verifier commit before providing the next snapshot", async () => { + const prepare = vi.fn( + async (_event: unknown, _key: string, _keys?: readonly string[]) => undefined, + ); + const { registerAuthStorageHandlers } = await import("./authStorage"); + registerAuthStorageHandlers({ prepareSet: prepare, didRemove: async () => undefined }); + const firstKey = "sb-recordlytest-auth-token-flow-flow_alice-code-verifier"; + const secondKey = "sb-recordlytest-auth-token-flow-flow_bob00-code-verifier"; + let release!: () => void; + let entered!: () => void; + const committing = new Promise((resolve) => { + entered = resolve; + }); + const held = new Promise((resolve) => { + release = resolve; + }); + const rename = fs.rename.bind(fs); + let paused = false; + vi.spyOn(fs, "rename").mockImplementation(async (...args) => { + if ( + !paused && + String(args[1]) === path.join(native.directory, "private-auth/session.enc") + ) { + paused = true; + entered(); + await held; + } + return rename(...args); + }); + const first = invoke("set", firstKey, '"alice-verifier"'); + await committing; + const second = invoke("set", secondKey, '"bob-verifier"'); + try { + await new Promise((resolve) => setImmediate(resolve)); + expect(prepare).toHaveBeenCalledOnce(); + expect(prepare.mock.calls[0][2]).toEqual([]); + release(); + await Promise.all([first, second]); + expect(prepare.mock.calls[1][2]).toContain(firstKey); + expect(await invoke("get", firstKey)).toBe('"alice-verifier"'); + } finally { + release(); + await Promise.allSettled([first, second]); + } +}); + +it.each([ + "same-client", + "two-renderers", +])("keeps actual SDK concurrent starts in the native effective index (%s)", async (mode) => { + const prefix = "sb-recordlytest-auth-token", + indexKey = `${prefix}-flows-code-verifier`; + let held = false, + reads = 0, + release!: () => void, + gate: Promise; + const makeClient = () => + createClient("https://recordlytest.supabase.co", "public-fixture", { + auth: { + storageKey: prefix, + flowType: "pkce", + persistSession: true, + autoRefreshToken: false, + detectSessionInUrl: false, + experimental: { appendPkceFlowIdToRedirects: true }, + storage: { + getItem: async (key) => { + const snapshot = (await invoke("get", key)) as string | null; + if (held && key === indexKey) { + if (++reads === 2) { + held = false; + release(); + } + await gate; + } + return snapshot; + }, + setItem: async (key, value) => { + await invoke("set", key, value); + }, + removeItem: async (key) => { + await invoke("remove", key); + }, + }, + }, + }); + const a = makeClient(), + b = mode === "same-client" ? a : makeClient(); + await Promise.all([a.auth.initialize(), b.auth.initialize()]); + for (let pair = 0; pair < 5; pair++) { + reads = 0; + gate = new Promise((resolve) => { + release = resolve; + }); + held = true; + const results = await Promise.all( + [a, b].map((client) => + client.auth.signInWithOAuth({ + provider: "apple", + options: { redirectTo: "recordly://auth/callback", skipBrowserRedirect: true }, + }), + ), + ); + expect(results.every((result) => result.error === null)).toBe(true); + const entries = JSON.parse( + await fs.readFile(path.join(native.directory, "private-auth/session.enc"), "utf8"), + ).entries; + const slots = Object.keys(entries).filter((key) => key.startsWith(`${prefix}-flow-`)); + const index = JSON.parse((await invoke("get", indexKey)) as string); + expect(index).toHaveLength(slots.length); + expect(index.length).toBeLessThanOrEqual(5); + for (const key of slots) + expect(index).toContain(key.slice(`${prefix}-flow-`.length, -"-code-verifier".length)); + } +}); diff --git a/electron/authStorage.ts b/electron/authStorage.ts index 5abb6699..a300543d 100644 --- a/electron/authStorage.ts +++ b/electron/authStorage.ts @@ -24,6 +24,7 @@ export type AuthFlowStorageHooks = { prepareSet: ( event: IpcMainInvokeEvent, key: string, + persistedKeys?: readonly string[], ) => Promise<(() => Promise) | undefined>; didRemove: (event: IpcMainInvokeEvent, key: string) => Promise; }; @@ -103,6 +104,34 @@ function validateEntries(value: unknown): AuthEntries { return result; } +function readStoredValue(entries: AuthEntries, key: string): string | null { + const prefix = /^(sb-[a-z0-9-]{1,100}-auth-token)-flows-code-verifier$/.exec(key)?.[1]; + if (!prefix) return entries[key] ?? null; + const present = new Set(); + for (const candidate of Object.keys(entries)) { + const slot = + /^(sb-[a-z0-9-]{1,100}-auth-token)-flow-([A-Za-z0-9_-]{8,64})-code-verifier$/.exec( + candidate, + ); + if (slot?.[1] === prefix) present.add(slot[2]); + } + let stored: unknown; + try { + stored = JSON.parse(entries[key] ?? "null"); + } catch { + // The index is bookkeeping; exact persisted verifier slots are authoritative. + } + const ids = new Set( + Array.isArray(stored) + ? stored.filter((id) => typeof id === "string" && present.has(id)) + : [], + ); + for (const id of present) ids.add(id); + // SDK starts race their index read/modify/write. Repair reads without deleting + // any active slot; only the SDK's normal explicit ring removals evict verifiers. + return ids.size || key in entries ? JSON.stringify([...ids]) : null; +} + function storagePath(): string { return path.join(app.getPath("userData"), "private-auth", "session.enc"); } @@ -170,7 +199,7 @@ function transaction(operation: () => Promise): Promise { export function readNativeAuthStorageValue(key: string): Promise { assertKey(key); - return serialize(() => transaction(async () => (await load()).entries[key] ?? null)); + return serialize(() => transaction(async () => readStoredValue((await load()).entries, key))); } export function registerAuthStorageHandlers(flowHooks?: AuthFlowStorageHooks): void { @@ -194,7 +223,7 @@ export function registerAuthStorageHandlers(flowHooks?: AuthFlowStorageHooks): v return serialize(async () => completedCallback?.key === key ? completedCallback.fingerprint - : transaction(async () => (await load()).entries[key] ?? null), + : transaction(async () => readStoredValue((await load()).entries, key)), ); }); ipcMain.handle("auth-storage:set", (event, key: unknown, value: unknown) => { @@ -202,7 +231,15 @@ export function registerAuthStorageHandlers(flowHooks?: AuthFlowStorageHooks): v assertKey(key); const validated = validateEntries({ [key]: value }); return serialize(async () => { - const rollback = await flowHooks?.prepareSet(event, key); + // This process's queue spans snapshot, registration and commit/rollback. + // Only queued prepareSet calls add owned routes, so no earlier own route + // can still be preparing its verifier when this snapshot is captured. + const persistedKeys = + flowHooks && + /^sb-[a-z0-9-]{1,100}-auth-token-flow-[A-Za-z0-9_-]{8,64}-code-verifier$/.test(key) + ? await transaction(async () => Object.keys((await load()).entries)) + : undefined; + const rollback = await flowHooks?.prepareSet(event, key, persistedKeys); if (COMPLETED_CALLBACK_KEY.test(key)) { // Keep one non-secret completion marker available to all our windows even // if optional bookkeeping cannot be persisted after a successful exchange. diff --git a/tests/ui/capture-picker.spec.ts b/tests/ui/capture-picker.spec.ts index 3a3322db..8f015b7d 100644 --- a/tests/ui/capture-picker.spec.ts +++ b/tests/ui/capture-picker.spec.ts @@ -590,7 +590,7 @@ for (const theme of ["light", "dark"] as const) { page.getByText(/Click a window|drag an area|Move or resize the area|Esc to cancel/i), ).toHaveCount(0); await page.mouse.click(600, 300); - expect(await pick(page)).toEqual({ + await expect.poll(() => pick(page)).toEqual({ kind: "window", windowId: "window:101:0", displayId: 1,