From 1e778d785c567e2d0d0d4c0d3810b435a6c3e395 Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 17:16:26 +0900 Subject: [PATCH 1/8] fix(account): keep no tombstones on a PC in no account A revoke on a PC that belongs to no account wrote a tombstone too, so the file only grew and the revoked key could not pair again with a code started through the relay. No account record applies on such a PC, so it now keeps none; a PC in an account still tombstones what it revokes. Closes #2865 --- docs/account.md | 4 +++- server/device-access.contract.test.ts | 19 +++++++++++++++++++ server/devices.ts | 6 +++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/docs/account.md b/docs/account.md index 3695019118..80cf602189 100644 --- a/docs/account.md +++ b/docs/account.md @@ -265,7 +265,9 @@ is what authenticates the PC. close at once (`auth_revoked`). A `remove` signed before this PC itself paired the key again leaves that pairing be (`unchanged`), so a late or replayed one cannot undo it. A row an `add` or a remote approval made goes, however late the `remove` arrives. -- **Revoke** and **Revoke all** on the PC leave a tombstone too, so no record brings the key back. +- **Revoke** and **Revoke all** on a PC that belongs to an account leave a tombstone too, so no + record brings the key back. A PC in no account keeps none: no record applies there, and a revoked + key may pair again with any code. - A pairing made on this PC (a code started on the PC, or a comparison approved here) clears that key's tombstone, after its row is written. A code started through the relay, or a comparison an account member approves remotely, is refused for a tombstoned key. diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index 9993983924..39dc0a09f6 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -1180,3 +1180,22 @@ it("a script's first pairing on a fresh install is kept at once, so its token st rmSync(stateDir, { recursive: true, force: true }); } }); + +it("a PC in no account keeps no tombstones, so a revoked key may pair again with a code started through the relay", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-repair-")); + try { + const store = new DeviceStore(stateDir); + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + const first = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); + if (!first) throw new Error("Expected relay enrollment"); + store.revoke(first.device.id); + expect(store.isTombstoned(publicKey)).toBe(false); + const again = store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Relay phone", publicKey); + expect(again?.device.label).toBe("Relay phone"); + store.revokeAll(); + expect(new DeviceStore(stateDir).isTombstoned(publicKey)).toBe(false); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/server/devices.ts b/server/devices.ts index 38bd44af16..fc4c3242e6 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -778,8 +778,12 @@ export class DeviceStore { this.writeTombstones(this.tombstones.filter((t) => t.key !== key)); } - /** Tombstones for the relay keys of rows this PC revokes, written before the revoke itself. */ + /** + * Tombstones for the relay keys of rows this PC revokes, written before the revoke itself. None on a + * PC in no account: no record applies there, and its revoked keys may pair again with any code. + */ private tombstone(rows: readonly StoredDevice[], now: number): void { + if (this.account() === null) return; const keys = [...new Set(rows.flatMap((d) => d.relay_public_key === undefined || this.isTombstoned(d.relay_public_key) ? [] : [d.relay_public_key]))]; const removedAt = new Date(now).toISOString(); const next = [...this.tombstones, From 60667e44c0f118b000bff1e753e5215ca330d71e Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 17:23:32 +0900 Subject: [PATCH 2/8] test(account): a key paired again in one of two state dirs stays paired after the merge Covers #2863: the sibling merge brings the other dir's tombstone back, and the start-up repair drops it because the key's row was paired after the removal. --- server/account/account.contract.test.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/server/account/account.contract.test.ts b/server/account/account.contract.test.ts index 224aad5a0d..bc40aecbe0 100644 --- a/server/account/account.contract.test.ts +++ b/server/account/account.contract.test.ts @@ -15,6 +15,7 @@ import { exportPublicKey, generateKeyPair, importPublicKey } from "../../shared/ import { bytesToHex } from "@noble/hashes/utils.js"; import { deriveServerId } from "../../shared/relay-server-proof.ts"; import { applyAccountRecords } from "./records.ts"; +import { createServer } from "../index.ts"; type Fixture = ReturnType; type Link = Awaited>; @@ -748,3 +749,27 @@ test("a waiting remove survives a stop right after the service took its page's a await new AccountMailbox({ devices: f.app.context.services.devices, identity: () => identity, service: "https://account.test", fetch: service.fetch, pendingPath }).pull(); expect(f.app.context.services.devices.hasKey(c.spki)).toBe(false); }); + +test("a key paired again in one of two state dirs stays paired, without its tombstone, when the other starts", async () => { + const f = fixture(); + const { key, link } = await member(f); + const gone = await accountKey(); + await post(link, "/api/account/records", { records: [await add(key, gone.spki)] }); + await post(link, "/api/account/records", { records: [await remove(key, gone.spki)] }); + const sibling = mkdtempSync(join(tmpdir(), "ovis-account-sibling-")); + // both dirs share the registry and the removal + for (const name of ["devices.json", "account-tombstones.json"]) writeFileSync(join(sibling, name), readFileSync(join(f.stateDir, name)), { mode: 0o600 }); + const store = f.app.context.services.devices; + expect(store.pairRelay(store.startPairing().code, "Mac app", gone.spki)).not.toBeNull(); + const other = createServer({ port: 0, hostname: "127.0.0.1", stateDir: sibling, siblingStateDir: f.stateDir, token: "t", tailscaleOwner: null, machines: false, sidecar: false }); + try { + // the merge brings the other dir's tombstone back, and the start-up repair drops it: the row is newer + for (const devices of [other.context.services.devices, new DeviceStore(f.stateDir)]) { + expect(devices.hasKey(gone.spki)).toBe(true); + expect(devices.isTombstoned(gone.spki)).toBe(false); + } + } finally { + other.stop(); + rmSync(sibling, { recursive: true, force: true }); + } +}); From 3ea35429186c46c58d5ae164aaf4e14bc7c59995 Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 17:38:26 +0900 Subject: [PATCH 3/8] fix(account): keep revoke tombstones, but let a PC in no account re-pair them omo review of #2928: a PC in no account that wrote no tombstone let a key it revoked come back by a signed add once it joined an account, against 'a revoke on this PC wins over the account'. Revokes tombstone again; on a PC in no account a code started through the relay may let the key back in (clearing its tombstone), and tombstones of local revokes are capped at the newest 1000 so the file stops growing. --- docs/account.md | 7 +++--- server/account/account.contract.test.ts | 13 +++++++++++ server/device-access.contract.test.ts | 29 +++++++++++++++++++++---- server/devices.ts | 21 ++++++++++++------ 4 files changed, 56 insertions(+), 14 deletions(-) diff --git a/docs/account.md b/docs/account.md index 80cf602189..55340e8ced 100644 --- a/docs/account.md +++ b/docs/account.md @@ -265,9 +265,10 @@ is what authenticates the PC. close at once (`auth_revoked`). A `remove` signed before this PC itself paired the key again leaves that pairing be (`unchanged`), so a late or replayed one cannot undo it. A row an `add` or a remote approval made goes, however late the `remove` arrives. -- **Revoke** and **Revoke all** on a PC that belongs to an account leave a tombstone too, so no - record brings the key back. A PC in no account keeps none: no record applies there, and a revoked - key may pair again with any code. +- **Revoke** and **Revoke all** on the PC leave a tombstone too, so no record brings the key back, + even after the PC joins an account. The PC keeps the newest 1000 of these; an account's removals + all stay. On a PC in no account, a code started through the relay may let a revoked key back in + and clears its tombstone. - A pairing made on this PC (a code started on the PC, or a comparison approved here) clears that key's tombstone, after its row is written. A code started through the relay, or a comparison an account member approves remotely, is refused for a tombstoned key. diff --git a/server/account/account.contract.test.ts b/server/account/account.contract.test.ts index bc40aecbe0..43476ef572 100644 --- a/server/account/account.contract.test.ts +++ b/server/account/account.contract.test.ts @@ -773,3 +773,16 @@ test("a key paired again in one of two state dirs stays paired, without its tomb rmSync(sibling, { recursive: true, force: true }); } }); + +test("a key revoked before the PC joined an account stays out: a member's signed add for it is refused", async () => { + const f = fixture(); + const store = f.app.context.services.devices; + const revoked = await accountKey(); + const before = store.pairRelay(store.startPairing().code, "Old phone", revoked.spki); + if (!before) throw new Error("Expected relay enrollment"); + store.revoke(before.device.id); + const { key, link } = await member(f); + const outcome = await post(link, "/api/account/records", { records: [await add(key, revoked.spki)] }); + expect((outcome.body as unknown as { results: { outcome: string; code?: string }[] }).results[0]).toMatchObject({ outcome: "refused", code: "tombstoned" }); + expect(store.hasKey(revoked.spki)).toBe(false); +}); diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index 39dc0a09f6..496e5375cc 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -4,7 +4,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "nod import { tmpdir } from "node:os"; import { join } from "node:path"; import { CLEAR_LEGACY_DEVICE_COOKIE, DEVICE_COOKIE, DEVICE_COOKIE_CLEARED } from "./auth.ts"; -import { DeviceStore } from "./devices.ts"; +import { DeviceStore, LOCAL_TOMBSTONES_MAX } from "./devices.ts"; import { handleRequest, type ServerContext } from "./dispatch.ts"; import { privateStateFs } from "./private-state.ts"; import { MachineRelay } from "./machine-relay.ts"; @@ -1181,7 +1181,7 @@ it("a script's first pairing on a fresh install is kept at once, so its token st } }); -it("a PC in no account keeps no tombstones, so a revoked key may pair again with a code started through the relay", async () => { +it("a PC in no account lets a revoked key pair again with a code started through the relay, which clears its tombstone", async () => { const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-repair-")); try { const store = new DeviceStore(stateDir); @@ -1190,12 +1190,33 @@ it("a PC in no account keeps no tombstones, so a revoked key may pair again with const first = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); if (!first) throw new Error("Expected relay enrollment"); store.revoke(first.device.id); - expect(store.isTombstoned(publicKey)).toBe(false); + expect(store.isTombstoned(publicKey)).toBe(true); const again = store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Relay phone", publicKey); expect(again?.device.label).toBe("Relay phone"); - store.revokeAll(); expect(new DeviceStore(stateDir).isTombstoned(publicKey)).toBe(false); } finally { rmSync(stateDir, { recursive: true, force: true }); } }); + +it("keeps at most LOCAL_TOMBSTONES_MAX tombstones of revokes on the PC, dropping the oldest", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-tombstone-cap-")); + try { + const old = Array.from({ length: LOCAL_TOMBSTONES_MAX }, (_, index) => ({ key: `old-${index}`, account: "", record_id: "local", removed_at: new Date(Date.UTC(2026, 0, 1, 0, 0, index)).toISOString() })); + const removed = { key: "removed-by-account", account: "acct_1", record_id: "r1", removed_at: "2025-01-01T00:00:00.000Z" }; + writeFileSync(join(stateDir, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [removed, ...old] }), { mode: 0o600 }); + const store = new DeviceStore(stateDir); + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + const paired = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); + if (!paired) throw new Error("Expected relay enrollment"); + store.revoke(paired.device.id); + const kept = JSON.parse(readFileSync(join(stateDir, "account-tombstones.json"), "utf8")) as { tombstones: { key: string; record_id: string }[] }; + expect(kept.tombstones.filter((t) => t.record_id === "local")).toHaveLength(LOCAL_TOMBSTONES_MAX); + expect(store.isTombstoned("old-0")).toBe(false); + expect(store.isTombstoned(publicKey)).toBe(true); + expect(store.isTombstoned("removed-by-account")).toBe(true); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/server/devices.ts b/server/devices.ts index fc4c3242e6..8530ed1203 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -175,6 +175,9 @@ export function readDeviceRegistry(path: string): DeviceRegistry { return { kind: "ok", gateClosedAt: parsed.gate_closed_at, devices: parsed.devices as StoredDevice[], placeholder: parsed.placeholder === true && parsed.devices.length === 0 }; } +/** The most tombstones of revokes on this PC kept (record_id "local"); an account's removals are not counted. */ +export const LOCAL_TOMBSTONES_MAX = 1000; + /** A key removed through the account, kept in `account-tombstones.json` beside devices.json. */ export interface AccountTombstone { key: string; account: string; record_id: string; removed_at: string } @@ -457,14 +460,16 @@ export class DeviceStore { // a code started through the relay is not a pairing made on this PC (docs/account.md, rule 5) const remote = this.pending?.remote ?? false; const startedBy = this.pending?.startedBy ?? null; - // a code started through the relay is not a pairing made on this PC; a member's code goes with the member - if ((remote && (this.accountError !== null || this.isTombstoned(publicKey))) || (this.pending?.memberRequired && (startedBy === null || !this.isAccountApprover(startedBy)))) { + // a code started through the relay is not a pairing made on this PC; a member's code goes with the member. + // On a PC in no account no record can bring a key back, so such a code may, as before account linking + const accountless = this.account() === null; + if ((remote && !accountless && (this.accountError !== null || this.isTombstoned(publicKey))) || (this.pending?.memberRequired && (startedBy === null || !this.isAccountApprover(startedBy)))) { this.noteRelayFailure("code", now); return null; } // a device let in by a code started through the relay is its starter's addition, not a pairing made on this PC (removeAccountKey) const paired = this.enrollRelay(label, publicKey, now, true, startedBy ?? undefined); - if (!remote) this.clearTombstone(publicKey); + if (!remote || accountless) this.clearTombstone(publicKey); return paired; } @@ -779,15 +784,17 @@ export class DeviceStore { } /** - * Tombstones for the relay keys of rows this PC revokes, written before the revoke itself. None on a - * PC in no account: no record applies there, and its revoked keys may pair again with any code. + * Tombstones for the relay keys of rows this PC revokes, written before the revoke itself. At most + * LOCAL_TOMBSTONES_MAX of them, the oldest dropped first; an account's removals all stay. */ private tombstone(rows: readonly StoredDevice[], now: number): void { - if (this.account() === null) return; const keys = [...new Set(rows.flatMap((d) => d.relay_public_key === undefined || this.isTombstoned(d.relay_public_key) ? [] : [d.relay_public_key]))]; const removedAt = new Date(now).toISOString(); - const next = [...this.tombstones, + const added = [...this.tombstones, ...keys.map((key) => ({ key, account: rows.find((d) => d.relay_public_key === key)?.account ?? "", record_id: "local", removed_at: removedAt }))]; + const local = added.filter((t) => t.record_id === "local"); + const dropped = new Set(local.slice(0, Math.max(0, local.length - LOCAL_TOMBSTONES_MAX))); + const next = added.filter((t) => !dropped.has(t)); if (!keys.length) return; // with the main file unreadable the revoke still wins: its tombstone waits beside it if (this.accountError !== null) { From dabc716b3c00e206286abbf819399966a617080f Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 17:50:34 +0900 Subject: [PATCH 4/8] fix(account): re-pair only the PC's own revokes, and keep the account's removals omo review of 3ea35429: on a PC in no account a code started through the relay now brings back only a key the PC revoked itself, never one the account removed, and still needs a readable tombstone file; an account remove for a key revoked first takes over its tombstone, so the cap on local revokes never drops it; the cap drops the oldest by time. --- docs/account.md | 7 ++-- server/account/account.contract.test.ts | 13 +++++++ server/device-access.contract.test.ts | 48 +++++++++++++++++++++++++ server/devices.ts | 13 ++++--- 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/docs/account.md b/docs/account.md index 55340e8ced..189d32e874 100644 --- a/docs/account.md +++ b/docs/account.md @@ -266,9 +266,10 @@ is what authenticates the PC. that pairing be (`unchanged`), so a late or replayed one cannot undo it. A row an `add` or a remote approval made goes, however late the `remove` arrives. - **Revoke** and **Revoke all** on the PC leave a tombstone too, so no record brings the key back, - even after the PC joins an account. The PC keeps the newest 1000 of these; an account's removals - all stay. On a PC in no account, a code started through the relay may let a revoked key back in - and clears its tombstone. + even after the PC joins an account. The PC keeps the newest 1000 of these (by time); an account's + removals all stay, and an account `remove` for a key the PC revoked first makes it one of them. On + a PC in no account, a code started through the relay may let a key the PC revoked itself back in + and clears its tombstone; a key the account removed stays refused. - A pairing made on this PC (a code started on the PC, or a comparison approved here) clears that key's tombstone, after its row is written. A code started through the relay, or a comparison an account member approves remotely, is refused for a tombstoned key. diff --git a/server/account/account.contract.test.ts b/server/account/account.contract.test.ts index 43476ef572..e5c35fb17a 100644 --- a/server/account/account.contract.test.ts +++ b/server/account/account.contract.test.ts @@ -786,3 +786,16 @@ test("a key revoked before the PC joined an account stays out: a member's signed expect((outcome.body as unknown as { results: { outcome: string; code?: string }[] }).results[0]).toMatchObject({ outcome: "refused", code: "tombstoned" }); expect(store.hasKey(revoked.spki)).toBe(false); }); + +test("an account remove for a key this PC revoked first makes its tombstone the account's", async () => { + const f = fixture(); + const { key, link } = await member(f); + const store = f.app.context.services.devices; + const gone = await accountKey(); + await post(link, "/api/account/records", { records: [await add(key, gone.spki)] }); + store.revoke(accountDeviceId(SERVER_ID, gone.spki)); + const removal = await remove(key, gone.spki); + await post(link, "/api/account/records", { records: [removal] }); + const kept = JSON.parse(readFileSync(join(f.stateDir, "account-tombstones.json"), "utf8")) as { tombstones: { key: string; record_id: string }[] }; + expect(kept.tombstones.find((t) => t.key === gone.spki)?.record_id).toBe(removal.id); +}); diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index 496e5375cc..d4833df554 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -1220,3 +1220,51 @@ it("keeps at most LOCAL_TOMBSTONES_MAX tombstones of revokes on the PC, dropping rmSync(stateDir, { recursive: true, force: true }); } }); + +it("a PC in no account still refuses, with a code started through the relay, a key its account removed", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-account-removed-")); + try { + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + writeFileSync(join(stateDir, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key: publicKey, account: "acct_1", record_id: "r1", removed_at: "2026-11-01T00:00:00.000Z" }] }), { mode: 0o600 }); + const store = new DeviceStore(stateDir); + expect(store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Phone", publicKey)).toBeNull(); + expect(store.isTombstoned(publicKey)).toBe(true); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); + +it("a PC in no account whose tombstone file will not parse refuses a code started through the relay", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-tombstones-broken-")); + try { + writeFileSync(join(stateDir, "account-tombstones.json"), "{broken", { mode: 0o600 }); + const store = new DeviceStore(stateDir); + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + expect(store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Phone", publicKey)).toBeNull(); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); + +it("the cap on local tombstones drops the oldest by time, not by place in the file", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-tombstone-order-")); + try { + const at = (index: number) => new Date(Date.UTC(2026, 0, 1, 0, 0, index)).toISOString(); + // the oldest one sits last, as a merge of two dirs can leave it + const old = [...Array.from({ length: LOCAL_TOMBSTONES_MAX - 1 }, (_, index) => ({ key: `old-${index + 1}`, account: "", record_id: "local", removed_at: at(index + 1) })), + { key: "oldest", account: "", record_id: "local", removed_at: at(0) }]; + writeFileSync(join(stateDir, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: old }), { mode: 0o600 }); + const store = new DeviceStore(stateDir); + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + const paired = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); + if (!paired) throw new Error("Expected relay enrollment"); + store.revoke(paired.device.id); + expect(store.isTombstoned("oldest")).toBe(false); + expect(store.isTombstoned("old-1")).toBe(true); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/server/devices.ts b/server/devices.ts index 8530ed1203..3ddcbfa195 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -461,9 +461,11 @@ export class DeviceStore { const remote = this.pending?.remote ?? false; const startedBy = this.pending?.startedBy ?? null; // a code started through the relay is not a pairing made on this PC; a member's code goes with the member. - // On a PC in no account no record can bring a key back, so such a code may, as before account linking + // On a PC in no account such a code may bring back a key this PC revoked itself, as before account linking; + // never one the account removed const accountless = this.account() === null; - if ((remote && !accountless && (this.accountError !== null || this.isTombstoned(publicKey))) || (this.pending?.memberRequired && (startedBy === null || !this.isAccountApprover(startedBy)))) { + const refusedKey = accountless ? this.tombstones.some((t) => t.key === publicKey && t.record_id !== "local") : this.isTombstoned(publicKey); + if ((remote && (this.accountError !== null || refusedKey)) || (this.pending?.memberRequired && (startedBy === null || !this.isAccountApprover(startedBy)))) { this.noteRelayFailure("code", now); return null; } @@ -756,7 +758,10 @@ export class DeviceStore { if (this.devices.some((d) => d.relay_public_key === key && d.added_by === undefined && d.created_at > record.issuedAt)) return false; const recordId = record.id; const fresh = !this.isTombstoned(key); - if (fresh) this.writeTombstones([...this.tombstones, { key, account, record_id: recordId, removed_at: new Date(now).toISOString() }]); + const removal = { key, account, record_id: recordId, removed_at: new Date(now).toISOString() }; + if (fresh) this.writeTombstones([...this.tombstones, removal]); + // a key this PC revoked first becomes the account's removal, which the cap on local revokes never drops + else if (this.tombstones.some((t) => t.key === key && t.record_id === "local")) this.writeTombstones(this.tombstones.map((t) => t.key === key ? removal : t)); const ids = this.devices.filter((d) => d.relay_public_key === key).map((d) => d.id); for (const id of ids) this.revoke(id, record.by); return fresh || ids.length > 0; @@ -792,7 +797,7 @@ export class DeviceStore { const removedAt = new Date(now).toISOString(); const added = [...this.tombstones, ...keys.map((key) => ({ key, account: rows.find((d) => d.relay_public_key === key)?.account ?? "", record_id: "local", removed_at: removedAt }))]; - const local = added.filter((t) => t.record_id === "local"); + const local = added.filter((t) => t.record_id === "local").sort((a, b) => a.removed_at.localeCompare(b.removed_at)); const dropped = new Set(local.slice(0, Math.max(0, local.length - LOCAL_TOMBSTONES_MAX))); const next = added.filter((t) => !dropped.has(t)); if (!keys.length) return; From cea3e149f12ae0ebbbb3a11a445f0c51f6719d5c Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 17:59:44 +0900 Subject: [PATCH 5/8] fix(account): drop the cap on revoke tombstones, and keep a re-paired row a sibling merge brings a revoke back for omo review of dabc716b: the cap let a key revoked before 1000 others come back by a signed add, so it is removed and the file grows as on main. A row a relay code re-paired on a PC in no account records its starter (added_by); a sibling dir's merged-back tombstone of the PC's own revoke made the start-up repair drop it. Only an account's removal now takes a member-added row whatever its time; a revoke on this PC takes rows made before it. --- docs/account.md | 10 ++--- server/device-access.contract.test.ts | 62 +++++++++++---------------- server/devices.ts | 18 +++----- 3 files changed, 34 insertions(+), 56 deletions(-) diff --git a/docs/account.md b/docs/account.md index 189d32e874..d4f5a02cfd 100644 --- a/docs/account.md +++ b/docs/account.md @@ -266,13 +266,13 @@ is what authenticates the PC. that pairing be (`unchanged`), so a late or replayed one cannot undo it. A row an `add` or a remote approval made goes, however late the `remove` arrives. - **Revoke** and **Revoke all** on the PC leave a tombstone too, so no record brings the key back, - even after the PC joins an account. The PC keeps the newest 1000 of these (by time); an account's - removals all stay, and an account `remove` for a key the PC revoked first makes it one of them. On - a PC in no account, a code started through the relay may let a key the PC revoked itself back in - and clears its tombstone; a key the account removed stays refused. + even after the PC joins an account. An account `remove` for a key the PC revoked first takes over + its tombstone. - A pairing made on this PC (a code started on the PC, or a comparison approved here) clears that key's tombstone, after its row is written. A code started through the relay, or a comparison an - account member approves remotely, is refused for a tombstoned key. + account member approves remotely, is refused for a tombstoned key, except on a PC in no account: + there a code started through the relay may let back a key the PC revoked itself (not one the + account removed), and clears its tombstone. - A stop between two such writes leaves a row and a tombstone for one key. On start the PC tells them apart by time: a row made before the key's removal is revoked, one made after it keeps its device and the tombstone goes. Until then no such row signs in, signs or approves. diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index d4833df554..df5b5e400d 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -4,7 +4,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "nod import { tmpdir } from "node:os"; import { join } from "node:path"; import { CLEAR_LEGACY_DEVICE_COOKIE, DEVICE_COOKIE, DEVICE_COOKIE_CLEARED } from "./auth.ts"; -import { DeviceStore, LOCAL_TOMBSTONES_MAX } from "./devices.ts"; +import { DeviceStore } from "./devices.ts"; import { handleRequest, type ServerContext } from "./dispatch.ts"; import { privateStateFs } from "./private-state.ts"; import { MachineRelay } from "./machine-relay.ts"; @@ -1199,28 +1199,6 @@ it("a PC in no account lets a revoked key pair again with a code started through } }); -it("keeps at most LOCAL_TOMBSTONES_MAX tombstones of revokes on the PC, dropping the oldest", async () => { - const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-tombstone-cap-")); - try { - const old = Array.from({ length: LOCAL_TOMBSTONES_MAX }, (_, index) => ({ key: `old-${index}`, account: "", record_id: "local", removed_at: new Date(Date.UTC(2026, 0, 1, 0, 0, index)).toISOString() })); - const removed = { key: "removed-by-account", account: "acct_1", record_id: "r1", removed_at: "2025-01-01T00:00:00.000Z" }; - writeFileSync(join(stateDir, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [removed, ...old] }), { mode: 0o600 }); - const store = new DeviceStore(stateDir); - const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); - const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); - const paired = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); - if (!paired) throw new Error("Expected relay enrollment"); - store.revoke(paired.device.id); - const kept = JSON.parse(readFileSync(join(stateDir, "account-tombstones.json"), "utf8")) as { tombstones: { key: string; record_id: string }[] }; - expect(kept.tombstones.filter((t) => t.record_id === "local")).toHaveLength(LOCAL_TOMBSTONES_MAX); - expect(store.isTombstoned("old-0")).toBe(false); - expect(store.isTombstoned(publicKey)).toBe(true); - expect(store.isTombstoned("removed-by-account")).toBe(true); - } finally { - rmSync(stateDir, { recursive: true, force: true }); - } -}); - it("a PC in no account still refuses, with a code started through the relay, a key its account removed", async () => { const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-account-removed-")); try { @@ -1248,23 +1226,31 @@ it("a PC in no account whose tombstone file will not parse refuses a code starte } }); -it("the cap on local tombstones drops the oldest by time, not by place in the file", async () => { - const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-tombstone-order-")); + +it("a key re-paired with a relay code on a PC in no account stays when a sibling state dir brings its tombstone back", async () => { + const [stateDir, sibling] = [mkdtempSync(join(tmpdir(), "ovis-relay-repair-a-")), mkdtempSync(join(tmpdir(), "ovis-relay-repair-b-"))]; try { - const at = (index: number) => new Date(Date.UTC(2026, 0, 1, 0, 0, index)).toISOString(); - // the oldest one sits last, as a merge of two dirs can leave it - const old = [...Array.from({ length: LOCAL_TOMBSTONES_MAX - 1 }, (_, index) => ({ key: `old-${index + 1}`, account: "", record_id: "local", removed_at: at(index + 1) })), - { key: "oldest", account: "", record_id: "local", removed_at: at(0) }]; - writeFileSync(join(stateDir, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: old }), { mode: 0o600 }); const store = new DeviceStore(stateDir); - const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); - const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); - const paired = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); - if (!paired) throw new Error("Expected relay enrollment"); - store.revoke(paired.device.id); - expect(store.isTombstoned("oldest")).toBe(false); - expect(store.isTombstoned("old-1")).toBe(true); + const fresh = async () => { + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + return Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + }; + const [starterKey, publicKey] = [await fresh(), await fresh()]; + const starter = store.pairRelay(store.startPairing().code, "Laptop", starterKey); + const first = store.pairRelay(store.startPairing().code, "Phone", publicKey); + if (!starter || !first) throw new Error("Expected relay enrollment"); + store.revoke(first.device.id); + // both dirs hold the registry and the revoke's tombstone + for (const name of ["devices.json", "account-tombstones.json"]) writeFileSync(join(sibling, name), readFileSync(join(stateDir, name)), { mode: 0o600 }); + // a second later, as a person pairing again would be + const later = Date.now() + 1_000; + const again = store.pairRelay(store.startPairing(later, { remote: true, startedBy: starter.device.id, memberRequired: false }).code, "Phone", publicKey, later); + expect(again).not.toBeNull(); + const app = createServer({ port: 0, hostname: "127.0.0.1", stateDir, siblingStateDir: sibling, token: "t", tailscaleOwner: null, machines: false, sidecar: false }); + try { + expect(app.context.services.devices.hasKey(publicKey)).toBe(true); + } finally { app.stop(); } } finally { - rmSync(stateDir, { recursive: true, force: true }); + for (const dir of [stateDir, sibling]) rmSync(dir, { recursive: true, force: true }); } }); diff --git a/server/devices.ts b/server/devices.ts index 3ddcbfa195..39817b5760 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -175,9 +175,6 @@ export function readDeviceRegistry(path: string): DeviceRegistry { return { kind: "ok", gateClosedAt: parsed.gate_closed_at, devices: parsed.devices as StoredDevice[], placeholder: parsed.placeholder === true && parsed.devices.length === 0 }; } -/** The most tombstones of revokes on this PC kept (record_id "local"); an account's removals are not counted. */ -export const LOCAL_TOMBSTONES_MAX = 1000; - /** A key removed through the account, kept in `account-tombstones.json` beside devices.json. */ export interface AccountTombstone { key: string; account: string; record_id: string; removed_at: string } @@ -312,9 +309,10 @@ export class DeviceStore { /** A row whose key was removed at or after the row was made: revoked, whether or not its own write happened yet. */ private stale(device: StoredDevice): boolean { - // a row an add or a remote approval made goes whatever its time, as removeAccountKey takes it + // a row an add or a remote approval made goes with an account's removal whatever its time, as removeAccountKey + // takes it; a revoke on this PC takes only rows made before it return device.relay_public_key !== undefined - && this.tombstones.some((t) => t.key === device.relay_public_key && (device.added_by !== undefined || t.removed_at >= device.created_at)); + && this.tombstones.some((t) => t.key === device.relay_public_key && ((device.added_by !== undefined && t.record_id !== "local") || t.removed_at >= device.created_at)); } /** @@ -788,18 +786,12 @@ export class DeviceStore { this.writeTombstones(this.tombstones.filter((t) => t.key !== key)); } - /** - * Tombstones for the relay keys of rows this PC revokes, written before the revoke itself. At most - * LOCAL_TOMBSTONES_MAX of them, the oldest dropped first; an account's removals all stay. - */ + /** Tombstones for the relay keys of rows this PC revokes, written before the revoke itself. */ private tombstone(rows: readonly StoredDevice[], now: number): void { const keys = [...new Set(rows.flatMap((d) => d.relay_public_key === undefined || this.isTombstoned(d.relay_public_key) ? [] : [d.relay_public_key]))]; const removedAt = new Date(now).toISOString(); - const added = [...this.tombstones, + const next = [...this.tombstones, ...keys.map((key) => ({ key, account: rows.find((d) => d.relay_public_key === key)?.account ?? "", record_id: "local", removed_at: removedAt }))]; - const local = added.filter((t) => t.record_id === "local").sort((a, b) => a.removed_at.localeCompare(b.removed_at)); - const dropped = new Set(local.slice(0, Math.max(0, local.length - LOCAL_TOMBSTONES_MAX))); - const next = added.filter((t) => !dropped.has(t)); if (!keys.length) return; // with the main file unreadable the revoke still wins: its tombstone waits beside it if (this.accountError !== null) { From 799fe53cb626ff71c773cbf0f20668236a1e8a4b Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 18:08:50 +0900 Subject: [PATCH 6/8] fix(account): keep a member's row going with a revoke, and the account's removal first in a merge omo review of cea3e149: the start-up repair told a relay code's re-pairing from an account add only by added_by, so a sibling dir that missed a revoke on the PC brought the key back by its account row; a member's row (account set) now goes with any tombstone of its key. The sibling merge kept the later tombstone, so a revoke on the PC could replace the account's removal and let a relay code back in; the account's removal now wins whatever the times. --- server/account/account.contract.test.ts | 36 +++++++++++++++++++++++++ server/devices.ts | 14 ++++++---- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/server/account/account.contract.test.ts b/server/account/account.contract.test.ts index e5c35fb17a..2e1721ac7b 100644 --- a/server/account/account.contract.test.ts +++ b/server/account/account.contract.test.ts @@ -799,3 +799,39 @@ test("an account remove for a key this PC revoked first makes its tombstone the const kept = JSON.parse(readFileSync(join(f.stateDir, "account-tombstones.json"), "utf8")) as { tombstones: { key: string; record_id: string }[] }; expect(kept.tombstones.find((t) => t.key === gone.spki)?.record_id).toBe(removal.id); }); + +test("a key revoked on this PC stays out when a sibling dir that missed the revoke added it through the account", async () => { + const f = fixture(); + const { key, link } = await member(f); + const store = f.app.context.services.devices; + const gone = await accountKey(); + await post(link, "/api/account/records", { records: [await add(key, gone.spki)] }); + const sibling = mkdtempSync(join(tmpdir(), "ovis-account-sibling-")); + try { + // the sibling dir still has the account's row; this dir then revokes it on the PC + writeFileSync(join(sibling, "devices.json"), readFileSync(join(f.stateDir, "devices.json")), { mode: 0o600 }); + store.revoke(accountDeviceId(SERVER_ID, gone.spki)); + const registry = JSON.parse(readFileSync(join(sibling, "devices.json"), "utf8")) as { devices: { relay_public_key?: string; created_at: string }[] }; + // made a second after the revoke, as the sibling knew nothing of it + for (const row of registry.devices) if (row.relay_public_key === gone.spki) row.created_at = new Date(Date.now() + 1_000).toISOString(); + writeFileSync(join(sibling, "devices.json"), JSON.stringify(registry), { mode: 0o600 }); + const app = createServer({ port: 0, hostname: "127.0.0.1", stateDir: f.stateDir, siblingStateDir: sibling, token: "t", tailscaleOwner: null, machines: false, sidecar: false }); + try { + expect(app.context.services.devices.hasKey(gone.spki)).toBe(false); + } finally { app.stop(); } + } finally { rmSync(sibling, { recursive: true, force: true }); } +}); + +test("merging two dirs keeps the account's removal of a key over a later revoke on the PC", () => { + const [a, b] = [mkdtempSync(join(tmpdir(), "ovis-account-a-")), mkdtempSync(join(tmpdir(), "ovis-account-b-"))]; + try { + const key = "a-removed-key"; + writeFileSync(join(a, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: ACCOUNT, record_id: "r1", removed_at: "2026-11-01T00:00:00.000Z" }] })); + writeFileSync(join(b, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: "", record_id: "local", removed_at: "2026-11-02T00:00:00.000Z" }] })); + expect(mergeAccountTombstones(a, b)).toBeNull(); + for (const dir of [a, b]) { + const kept = JSON.parse(readFileSync(join(dir, "account-tombstones.json"), "utf8")) as { tombstones: { record_id: string }[] }; + expect(kept.tombstones.map((t) => t.record_id)).toEqual(["r1"]); + } + } finally { for (const dir of [a, b]) rmSync(dir, { recursive: true, force: true }); } +}); diff --git a/server/devices.ts b/server/devices.ts index 39817b5760..57fc40deb1 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -206,7 +206,9 @@ export function mergeAccountTombstones(active: string, sibling: string): string const merged = new Map(); for (const tombstone of [...read.flatMap((list) => list ?? []), ...pending]) { const kept = merged.get(tombstone.key); - if (!kept || tombstone.removed_at > kept.removed_at) merged.set(tombstone.key, tombstone); + // an account's removal outranks a revoke on the PC, whatever their times: only the account's keeps a relay code out + const rank = (t: AccountTombstone) => t.record_id === "local" ? 0 : 1; + if (!kept || rank(tombstone) > rank(kept) || (rank(tombstone) === rank(kept) && tombstone.removed_at > kept.removed_at)) merged.set(tombstone.key, tombstone); } const tombstones = [...merged.values()]; for (const [index, path] of paths.entries()) { @@ -309,10 +311,12 @@ export class DeviceStore { /** A row whose key was removed at or after the row was made: revoked, whether or not its own write happened yet. */ private stale(device: StoredDevice): boolean { - // a row an add or a remote approval made goes with an account's removal whatever its time, as removeAccountKey - // takes it; a revoke on this PC takes only rows made before it + // a member's row (an add or a remote approval: `account` and `added_by`) goes with its key's tombstone whatever + // its time, as removeAccountKey takes it; any other row, a relay code's re-pairing included, only with one newer + // than itself, an account's removal taking a relay code's row whatever its time too return device.relay_public_key !== undefined - && this.tombstones.some((t) => t.key === device.relay_public_key && ((device.added_by !== undefined && t.record_id !== "local") || t.removed_at >= device.created_at)); + && this.tombstones.some((t) => t.key === device.relay_public_key + && ((device.added_by !== undefined && (device.account !== undefined || t.record_id !== "local")) || t.removed_at >= device.created_at)); } /** @@ -758,7 +762,7 @@ export class DeviceStore { const fresh = !this.isTombstoned(key); const removal = { key, account, record_id: recordId, removed_at: new Date(now).toISOString() }; if (fresh) this.writeTombstones([...this.tombstones, removal]); - // a key this PC revoked first becomes the account's removal, which the cap on local revokes never drops + // a key this PC revoked first becomes the account's removal, so a code started through the relay cannot bring it back else if (this.tombstones.some((t) => t.key === key && t.record_id === "local")) this.writeTombstones(this.tombstones.map((t) => t.key === key ? removal : t)); const ids = this.devices.filter((d) => d.relay_public_key === key).map((d) => d.id); for (const id of ids) this.revoke(id, record.by); From 9d3f97b2c6a9ad78353e1292ac7e7196e946ffb2 Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 18:17:59 +0900 Subject: [PATCH 7/8] fix(account): keep the start-up repair as on main, and the account's removal with the later time in a merge omo review of 799fe53c: the start-up repair changes of the last two rounds let a key revoked on this PC come back through a sibling dir, so stale() is back to main's rule (a re-pairing by a relay code can then go at the next start with two state dirs, failing closed). The sibling merge keeps the account's removal over a revoke on the PC with the later of the two times, and rewrites a dir whose record id differs. --- server/account/account.contract.test.ts | 17 ++++++++++++++- server/device-access.contract.test.ts | 29 ------------------------- server/devices.ts | 18 +++++++-------- 3 files changed, 25 insertions(+), 39 deletions(-) diff --git a/server/account/account.contract.test.ts b/server/account/account.contract.test.ts index 2e1721ac7b..0275d48b06 100644 --- a/server/account/account.contract.test.ts +++ b/server/account/account.contract.test.ts @@ -822,13 +822,28 @@ test("a key revoked on this PC stays out when a sibling dir that missed the revo } finally { rmSync(sibling, { recursive: true, force: true }); } }); -test("merging two dirs keeps the account's removal of a key over a later revoke on the PC", () => { +test("merging two dirs keeps the account's removal of a key over a revoke on the PC, with the later time", () => { const [a, b] = [mkdtempSync(join(tmpdir(), "ovis-account-a-")), mkdtempSync(join(tmpdir(), "ovis-account-b-"))]; try { const key = "a-removed-key"; writeFileSync(join(a, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: ACCOUNT, record_id: "r1", removed_at: "2026-11-01T00:00:00.000Z" }] })); writeFileSync(join(b, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: "", record_id: "local", removed_at: "2026-11-02T00:00:00.000Z" }] })); expect(mergeAccountTombstones(a, b)).toBeNull(); + for (const dir of [a, b]) { + const kept = JSON.parse(readFileSync(join(dir, "account-tombstones.json"), "utf8")) as { tombstones: { record_id: string; removed_at: string }[] }; + expect(kept.tombstones.map((t) => [t.record_id, t.removed_at])).toEqual([["r1", "2026-11-02T00:00:00.000Z"]]); + } + } finally { for (const dir of [a, b]) rmSync(dir, { recursive: true, force: true }); } +}); + +test("merging two dirs rewrites a dir whose tombstone for the key is the PC's revoke at the same time as the account's removal", () => { + const [a, b] = [mkdtempSync(join(tmpdir(), "ovis-account-a-")), mkdtempSync(join(tmpdir(), "ovis-account-b-"))]; + try { + const key = "a-removed-key"; + const at = "2026-11-02T00:00:00.000Z"; + writeFileSync(join(a, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: "", record_id: "local", removed_at: at }] })); + writeFileSync(join(b, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: ACCOUNT, record_id: "r1", removed_at: at }] })); + expect(mergeAccountTombstones(a, b)).toBeNull(); for (const dir of [a, b]) { const kept = JSON.parse(readFileSync(join(dir, "account-tombstones.json"), "utf8")) as { tombstones: { record_id: string }[] }; expect(kept.tombstones.map((t) => t.record_id)).toEqual(["r1"]); diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index df5b5e400d..dbb78f6cec 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -1225,32 +1225,3 @@ it("a PC in no account whose tombstone file will not parse refuses a code starte rmSync(stateDir, { recursive: true, force: true }); } }); - - -it("a key re-paired with a relay code on a PC in no account stays when a sibling state dir brings its tombstone back", async () => { - const [stateDir, sibling] = [mkdtempSync(join(tmpdir(), "ovis-relay-repair-a-")), mkdtempSync(join(tmpdir(), "ovis-relay-repair-b-"))]; - try { - const store = new DeviceStore(stateDir); - const fresh = async () => { - const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); - return Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); - }; - const [starterKey, publicKey] = [await fresh(), await fresh()]; - const starter = store.pairRelay(store.startPairing().code, "Laptop", starterKey); - const first = store.pairRelay(store.startPairing().code, "Phone", publicKey); - if (!starter || !first) throw new Error("Expected relay enrollment"); - store.revoke(first.device.id); - // both dirs hold the registry and the revoke's tombstone - for (const name of ["devices.json", "account-tombstones.json"]) writeFileSync(join(sibling, name), readFileSync(join(stateDir, name)), { mode: 0o600 }); - // a second later, as a person pairing again would be - const later = Date.now() + 1_000; - const again = store.pairRelay(store.startPairing(later, { remote: true, startedBy: starter.device.id, memberRequired: false }).code, "Phone", publicKey, later); - expect(again).not.toBeNull(); - const app = createServer({ port: 0, hostname: "127.0.0.1", stateDir, siblingStateDir: sibling, token: "t", tailscaleOwner: null, machines: false, sidecar: false }); - try { - expect(app.context.services.devices.hasKey(publicKey)).toBe(true); - } finally { app.stop(); } - } finally { - for (const dir of [stateDir, sibling]) rmSync(dir, { recursive: true, force: true }); - } -}); diff --git a/server/devices.ts b/server/devices.ts index 57fc40deb1..654683b657 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -206,13 +206,16 @@ export function mergeAccountTombstones(active: string, sibling: string): string const merged = new Map(); for (const tombstone of [...read.flatMap((list) => list ?? []), ...pending]) { const kept = merged.get(tombstone.key); - // an account's removal outranks a revoke on the PC, whatever their times: only the account's keeps a relay code out - const rank = (t: AccountTombstone) => t.record_id === "local" ? 0 : 1; - if (!kept || rank(tombstone) > rank(kept) || (rank(tombstone) === rank(kept) && tombstone.removed_at > kept.removed_at)) merged.set(tombstone.key, tombstone); + if (!kept) { merged.set(tombstone.key, tombstone); continue; } + // the account's removal outranks a revoke on the PC (only it keeps a relay code out), with the later of the two times + const local = (t: AccountTombstone) => t.record_id === "local"; + const winner = local(kept) && !local(tombstone) ? tombstone : !local(kept) && local(tombstone) ? kept : tombstone.removed_at > kept.removed_at ? tombstone : kept; + const removedAt = tombstone.removed_at > kept.removed_at ? tombstone.removed_at : kept.removed_at; + merged.set(tombstone.key, { ...winner, removed_at: removedAt }); } const tombstones = [...merged.values()]; for (const [index, path] of paths.entries()) { - if ((read[index] ?? []).length !== tombstones.length || tombstones.some((t) => !read[index]?.some((own) => own.key === t.key && own.removed_at === t.removed_at))) { + if ((read[index] ?? []).length !== tombstones.length || tombstones.some((t) => !read[index]?.some((own) => own.key === t.key && own.removed_at === t.removed_at && own.record_id === t.record_id))) { writePrivateState(path, { v: 1, tombstones }); } } @@ -311,12 +314,9 @@ export class DeviceStore { /** A row whose key was removed at or after the row was made: revoked, whether or not its own write happened yet. */ private stale(device: StoredDevice): boolean { - // a member's row (an add or a remote approval: `account` and `added_by`) goes with its key's tombstone whatever - // its time, as removeAccountKey takes it; any other row, a relay code's re-pairing included, only with one newer - // than itself, an account's removal taking a relay code's row whatever its time too + // a row an add or a remote approval made goes whatever its time, as removeAccountKey takes it return device.relay_public_key !== undefined - && this.tombstones.some((t) => t.key === device.relay_public_key - && ((device.added_by !== undefined && (device.account !== undefined || t.record_id !== "local")) || t.removed_at >= device.created_at)); + && this.tombstones.some((t) => t.key === device.relay_public_key && (device.added_by !== undefined || t.removed_at >= device.created_at)); } /** From c6fac3c31a27b2ccbaaf5b899b07de73a87f0e05 Mon Sep 17 00:00:00 2001 From: devswha Date: Thu, 5 Nov 2026 18:29:35 +0900 Subject: [PATCH 8/8] fix(account): a relay re-pairing that undoes the PC's own revoke is the PC's pairing omo review of 22c82705: on a PC in no account a key re-paired by a code another device started recorded that starter (added_by), so a tombstone that came back (a sibling dir, a stop before it was cleared) dropped the row at the next start whatever its time. Such a re-pairing, which undoes a revoke on this PC, now records no starter, so only a newer tombstone takes it. --- server/device-access.contract.test.ts | 25 +++++++++++++++++++++++++ server/devices.ts | 7 +++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index dbb78f6cec..09ddd5d205 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -1225,3 +1225,28 @@ it("a PC in no account whose tombstone file will not parse refuses a code starte rmSync(stateDir, { recursive: true, force: true }); } }); + +it("a key re-paired on a PC in no account with a code another device started stays when its tombstone comes back", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-repair-starter-")); + try { + const store = new DeviceStore(stateDir); + const fresh = async () => { + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + return Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + }; + const [starterKey, publicKey] = [await fresh(), await fresh()]; + const starter = store.pairRelay(store.startPairing().code, "Laptop", starterKey); + const first = store.pairRelay(store.startPairing().code, "Phone", publicKey); + if (!starter || !first) throw new Error("Expected relay enrollment"); + store.revoke(first.device.id); + const tombstones = readFileSync(join(stateDir, "account-tombstones.json")); + // a second later, as a person pairing again would be + const later = Date.now() + 1_000; + expect(store.pairRelay(store.startPairing(later, { remote: true, startedBy: starter.device.id, memberRequired: false }).code, "Phone", publicKey, later)).not.toBeNull(); + // the revoke's tombstone back, as a stop before it was cleared or a sibling dir's merge leaves it + writeFileSync(join(stateDir, "account-tombstones.json"), tombstones, { mode: 0o600 }); + expect(new DeviceStore(stateDir).hasKey(publicKey)).toBe(true); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/server/devices.ts b/server/devices.ts index 22bc5bc8e5..4b0d03dd36 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -493,8 +493,11 @@ export class DeviceStore { this.noteRelayFailure("code", now); return null; } - // a device let in by a code started through the relay is its starter's addition, not a pairing made on this PC (removeAccountKey) - const paired = this.enrollRelay(label, publicKey, now, true, startedBy ?? undefined); + // a device let in by a code started through the relay is its starter's addition, not a pairing made on this PC (removeAccountKey); + // one that brings back a key this PC revoked itself (on a PC in no account) undoes that revoke, and is the PC's own + // pairing, so a tombstone that comes back (a sibling dir, a stop before it is cleared) takes it only if newer + const undoesRevoke = remote && accountless && this.isTombstoned(publicKey); + const paired = this.enrollRelay(label, publicKey, now, true, undoesRevoke ? undefined : startedBy ?? undefined); if (!remote || accountless) this.clearTombstone(publicKey); return paired; }