From f46103acc9d99ea634cd150d4eac57ad8a76ff15 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:59:56 -0800 Subject: [PATCH] fix(pairing): a registry save can no longer put back a device revoked while it was writing (#39192) Device registry saves re-read the file, merge and rename. A save that read the file just before another writer removed a device (an admin script, the desktop, orcad) and renamed just after would write the revoked row back, and its token would keep working. Saves now publish only if the file is still exactly what they read (compare-and-swap via the staged write's last check), and otherwise re-read and re-merge. A file caught half-written by a non-atomic writer is re-read after a short wait instead of being treated as corrupt and overwritten. --- .../pairing/device-registry-disk-merge.ts | 70 +++++++++--- .../pairing/device-registry-save-race.test.ts | 103 ++++++++++++++++++ src/shared/secure-file.ts | 16 ++- 3 files changed, 171 insertions(+), 18 deletions(-) create mode 100644 src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts diff --git a/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts b/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts index 450bd6b0a18..d368e6453c0 100644 --- a/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts +++ b/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts @@ -1,4 +1,4 @@ -import { statSync } from 'node:fs' +import { readFileSync, statSync } from 'node:fs' import { writeDurableSecureJsonFile, writeSecureJsonFile } from '../../../../../shared/secure-file' import type { DeviceEntry } from './device-entry' import { isMissingFileError, quarantineCorruptRegistry } from './device-registry-corruption' @@ -6,6 +6,10 @@ import { readDeviceRegistryFile, type DeviceRegistryFileRead } from './device-re // Phones recheck auth on every frame and request; one stat a second bounds a disk revoke's lag. const EXTERNAL_EDIT_CHECK_INTERVAL_MS = 1000 +// Another writer landed between our read and rename: re-read and re-merge, a few times at most. +const MAX_WRITE_ATTEMPTS = 5 +// Non-atomic external writers (an admin's script) can be caught mid-write; give them a moment. +const TORN_READ_RETRY_MS = 25 /** * What a save may write over the file as it is now on disk. Another writer (an admin removing a @@ -102,7 +106,12 @@ export class DeviceRegistryWriter { : mergeDeviceRegistrySave(current, current, disk, this.removedIds) } - /** Returns what was written. Security transitions pass `durable`. */ + /** + * Returns what was written. Security transitions pass `durable`. The merge is published only if + * the file is still exactly what it was read from (compare-and-swap); otherwise another writer + * landed in between, so the save re-reads and re-merges. Without that, a save that read the file + * before an admin's revoke and renamed after it would put the revoked row back. + */ write( before: readonly DeviceEntry[], next: readonly DeviceEntry[], @@ -114,23 +123,56 @@ export class DeviceRegistryWriter { this.removedIds.add(deviceId) } } - const disk = readDeviceRegistryFile(this.registryPath) - // After our own quarantine the file's absence is no removal evidence, but rows another writer - // (the pairing CLI) added since are still kept: merge with no known baseline. - const baseline = this.quarantinedUntilWritten ? [] : before - const merged = - disk.unreadable || disk.corruptFilePending - ? [...next] - : mergeDeviceRegistrySave(baseline, next, disk, this.removedIds) - const write = options.durable ? writeDurableSecureJsonFile : writeSecureJsonFile // Stat after our rename can't tell our write from one right after it; re-read once instead. this.readStamp = null - write(this.registryPath, merged) - this.quarantinedUntilWritten = false - return merged + for (let attempt = 1; ; attempt++) { + const lastAttempt = attempt >= MAX_WRITE_ATTEMPTS + const snapshot = readRawRegistry(this.registryPath) + const disk = readDeviceRegistryFile(this.registryPath) + if ( + disk.corruptFilePending && + !lastAttempt && + snapshot !== readRawRegistry(this.registryPath) + ) { + // Still being written by someone else; a torn read proves nothing, so wait and re-read. + sleepSync(TORN_READ_RETRY_MS) + continue + } + // After our own quarantine the file's absence is no removal evidence, but rows another writer + // (the pairing CLI) added since are still kept: merge with no known baseline. + const baseline = this.quarantinedUntilWritten ? [] : before + const merged = + disk.unreadable || disk.corruptFilePending + ? [...next] + : mergeDeviceRegistrySave(baseline, next, disk, this.removedIds) + let unchanged = true + const write = options.durable ? writeDurableSecureJsonFile : writeSecureJsonFile + write(this.registryPath, merged, { + // Last check before the rename; a lost race discards the stage and merges again. + shouldPublish: () => + (unchanged = lastAttempt || readRawRegistry(this.registryPath) === snapshot) + }) + if (unchanged) { + this.quarantinedUntilWritten = false + return merged + } + } } } +/** Raw file text for compare-and-swap; null when absent, and a marker no file can equal when unreadable. */ +function readRawRegistry(registryPath: string): string | null { + try { + return readFileSync(registryPath, 'utf-8') + } catch (error) { + return isMissingFileError(error) ? null : `\u0000unreadable:${String(error)}` + } +} + +function sleepSync(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) +} + function fileStamp(registryPath: string): string { try { const stat = statSync(registryPath, { bigint: true }) diff --git a/src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts b/src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts new file mode 100644 index 00000000000..61a88ea1ce7 --- /dev/null +++ b/src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts @@ -0,0 +1,103 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as FileReadModule from './device-registry-file-read' +import { DeviceRegistry } from './device-registry' +import { DEVICE_REGISTRY_FILENAME } from './mobile-pairing-files' + +// Runs once right after the saving process has read the file: another process's write lands then. +const afterRead = vi.hoisted(() => { + const state: { hook: (() => void) | null } = { hook: null } + return state +}) +vi.mock('./device-registry-file-read', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + readDeviceRegistryFile: (path: string) => { + const read = actual.readDeviceRegistryFile(path) + const hook = afterRead.hook + afterRead.hook = null + hook?.() + return read + } + } +}) + +let dir: string +let registryPath: string + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'orca-registry-race-')) + registryPath = join(dir, DEVICE_REGISTRY_FILENAME) +}) + +afterEach(() => { + afterRead.hook = null + rmSync(dir, { recursive: true, force: true }) +}) + +function diskIds(): string[] { + const rows: { deviceId: string }[] = JSON.parse(readFileSync(registryPath, 'utf8')) + return rows.map((row) => row.deviceId) +} + +/** An admin's script, as in the docs: rewrite the file without the row (not atomic, no lock). */ +function adminRemoves(deviceId: string): void { + const rows: { deviceId: string }[] = JSON.parse(readFileSync(registryPath, 'utf8')) + writeFileSync(registryPath, JSON.stringify(rows.filter((row) => row.deviceId !== deviceId))) +} + +function pairedDevices(registry: DeviceRegistry) { + const kept = registry.addDevice('Kept', 'runtime') + const revoked = registry.addDevice('Revoked', 'runtime') + registry.updateLastSeen(kept.deviceId) + registry.updateLastSeen(revoked.deviceId) + return { kept, revoked } +} + +describe('device registry save racing another writer', () => { + it('a lastSeen flush that read before an admin revoke does not write the row back', () => { + const registry = new DeviceRegistry(dir) + const { kept, revoked } = pairedDevices(registry) + registry.updateLastSeenDeferred(revoked.deviceId) + + afterRead.hook = () => adminRemoves(revoked.deviceId) + registry.flushPendingLastSeen() + + expect(diskIds()).toEqual([kept.deviceId]) + expect(registry.validateToken(revoked.token)).toBeNull() + expect(new DeviceRegistry(dir).validateToken(revoked.token)).toBeNull() + expect(registry.validateToken(kept.token)?.deviceId).toBe(kept.deviceId) + }) + + it('keeps both writers changes: a row another process added during the save survives', () => { + const registry = new DeviceRegistry(dir) + const { kept } = pairedDevices(registry) + const other = new DeviceRegistry(dir) + + afterRead.hook = () => other.addDevice('From the CLI', 'runtime') + const added = registry.addDevice('From the desktop', 'runtime') + + expect(new Set(diskIds())).toEqual( + new Set([kept.deviceId, added.deviceId, ...other.listDevices().map((d) => d.deviceId)]) + ) + expect(diskIds()).toHaveLength(4) + }) + + it('waits out an admin write caught half-way instead of treating it as a corrupt file', () => { + const registry = new DeviceRegistry(dir) + const { kept, revoked } = pairedDevices(registry) + registry.updateLastSeenDeferred(revoked.deviceId) + const full: { deviceId: string }[] = JSON.parse(readFileSync(registryPath, 'utf8')) + const finished = JSON.stringify(full.filter((row) => row.deviceId !== revoked.deviceId)) + + // The save reads a truncated file; the admin's write finishes right after. + writeFileSync(registryPath, finished.slice(0, 10)) + afterRead.hook = () => writeFileSync(registryPath, finished) + registry.flushPendingLastSeen() + + expect(diskIds()).toEqual([kept.deviceId]) + }) +}) diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index 756ecbf442d..af4c5a7e65c 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -98,13 +98,21 @@ function hardenExistingWindowsFile(targetPath: string): boolean { } /** Returns false when the file was written but its permissions could not be restricted. */ -export function writeSecureJsonFile(targetPath: string, value: unknown): boolean { - return writeSecureFile(targetPath, JSON.stringify(value, null, 2)) +export function writeSecureJsonFile( + targetPath: string, + value: unknown, + options: { shouldPublish?: () => boolean } = {} +): boolean { + return writeSecureFile(targetPath, JSON.stringify(value, null, 2), options) } /** Returns false when the file was written but its permissions could not be restricted. */ -export function writeDurableSecureJsonFile(targetPath: string, value: unknown): boolean { - return writeSecureFile(targetPath, JSON.stringify(value, null, 2), { durable: true }) +export function writeDurableSecureJsonFile( + targetPath: string, + value: unknown, + options: { shouldPublish?: () => boolean } = {} +): boolean { + return writeSecureFile(targetPath, JSON.stringify(value, null, 2), { ...options, durable: true }) } /** Publish a staged file and return whether user-only file permissions hold. Windows addon creates restricted or throws;