From b2efe160eca7babb6776ac6bcf86115c6d84a9c4 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 11:20:49 -0700 Subject: [PATCH] Catch up a boundary change a server was not listening for Two improvements from Nathan Tarbert's #94, which arrived five minutes after #90 merged and was better than it in these two places. A NOTIFY reaches whoever is listening at the time. A replica that was restarting, or whose connection had dropped, is not, so it missed the announcement and went on enforcing the rules it read at boot until something restarted it. That is the original bug wearing a smaller hat and worse for being intermittent: the fleet disagrees with itself and nothing says so. The handler is now passed as `onlisten` as well, so the row is re-read whenever the driver establishes or re-establishes the subscription, which is exactly when a notification could have been missed. The announcement now goes inside the write transaction, so it is delivered on commit. A write that rolls back announces nothing, and there is no window where the row has changed and the other servers have not been told. Same shape as channel activity, which is what should have been copied. Two tests. The reconnect one fails without `onlisten`. The changelog also picks up the last day of merges, which had run ahead of it. --- CHANGELOG.md | 68 ++++++++++++++++++ server/src/computer/policy-listener.ts | 28 ++++++-- server/src/computer/policy-store.ts | 55 ++++++++------- .../tests/policy-fanout.integration.test.ts | 69 +++++++++++++++++++ 4 files changed, 190 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e279fc5f..6a664903 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,13 @@ Registering an OpenID Connect provider needs every host in its discovery documen issuer also needs `oauth2.googleapis.com` and `openidconnect.googleapis.com`. Registration is refused with the untrusted host named. +A Bot id may now contain only letters, digits, hyphen and underscore, and must start with a letter or +digit. The same rule container and volume names have always followed. A deployment whose +`COMPUTER_BOT_ID` breaks it refuses to start and says so, rather than answering 400 to everything. + +`AUDIT_RETENTION_DAYS` is new and unset, which keeps the audit trail forever, as before. Set it to a +whole number of days to have old rows removed. + Sessions survive and nobody signs in again. ### Added @@ -86,6 +93,55 @@ Sessions survive and nobody signs in again. is unavailable never blocks a sign-in. ### Fixed +- **A boundary rule applied on one server out of N.** The policy is read from memory on every action, + which is right, but memory was only ever filled at boot. An administrator's new deny rule was + enforced by whichever process served the request and roughly one action in N went through it, while + the admin screen reported success because the row really was saved and the audit trail agreed + because it records the boundary each process started with. Both honest, and both describing + something other than what the fleet was enforcing. A write now announces on Postgres in the same + transaction and every server re-reads, including on reconnect, so a server that was down when the + rule changed catches up rather than waiting for a restart. Reset travels the same way. +- **A ref resolved on one replica and nowhere else.** The gateway turns the opaque ref in a click into + the element it points at, and that mapping lived in a `Map` in the process that took the snapshot. + On any other replica the ref resolved to nothing, so a deny rule written about the element did not + match and the click went through, recorded as allowed with no rule. It is in Postgres now, keyed on + the generation the computer stamped, so a ref from a superseded page still resolves to nothing. +- **Anybody signed in could act as anybody's Bot.** The Bot id travels in the path and the acting + routes checked only that somebody was signed in, so a signed-in person could drive another person's + private Bot, reset its browser, read its screen and fire its granted tools. Every route under a Bot + id now asks the store the same question the roster already asks, and a Bot that does not exist and + one belonging to somebody else answer identically. +- **The computer fleet listing was open to any signed-in person.** It ignores its `:botId` and returns + every Bot's machine, so it told anybody who could reach it every Bot id in the deployment and + whether each was running, private coworkers included. Administrator-only now. +- **A Bot id could name a directory outside the profiles volume.** The id arrives as a URL segment or + a header, was joined onto a filesystem path, and `reset` deletes that path recursively as root, so + `../../tmp/something` deleted it. Refused at the request boundary and again where the path is built. +- **A mistyped deny rule permitted instead of refusing.** A rule that parsed and evaluated but + answered with something other than true or false was neither a match nor an error, so + `deny: ["Submit order"]` — what somebody writes who reads the list as labels — let the action + through with nothing logged, while the rule sat on the Boundaries page looking as though it were in + force. Any non-boolean answer is now a broken rule and takes the existing fail-closed path. +- **Rotating a Bot's key left the old one live.** Editing a key wrote a new vault row and repointed + the Bot at it, leaving the previous credential decryptable and still valid with nothing listing it, + so rotation did not do the one thing rotation is for. Deleting a Bot left its key live too. Both + revoke now. +- **Nothing recorded what changed about a Bot.** Ten mutating routes wrote one audit row between them + and there was no event type for any of the other nine. A Bot's endpoint is where conversation + content is sent, so "who pointed this Bot at that host, and when" is the first question in an + incident and could not be answered. Eight event types and eight rows now, recording what changed and + never a value. +- **The people list and the channel list grew without bound.** Both were read in full on every render, + and reading one person ran the whole people aggregate over the deployment twice per role change. + Both are paged now, and the people screen searches on the server so somebody can be found without + walking pages. +- **A computer accumulated one browser per Bot, forever.** Nothing closed an idle one, so a deployment + where every employee has a Bot trends toward a resident Chromium per employee in one container until + it is killed for memory. There is a cap and an idle timeout, and closing one costs only a relaunch + because the profile is on disk. +- **The audit screen's filters were sequential scans.** It filters by event type, by who did it and by + what it was done to, and the only index was on the timestamp, over what becomes the largest table in + the deployment. Each filter leads its own index now. - **A deployment with no identity provider came up open by default.** Covered under Changed above, and listed here too because it is the one on this list that was reachable from the internet. - **Registering a company's identity provider was owned by whoever registered it.** Better Auth @@ -156,6 +212,18 @@ Sessions survive and nobody signs in again. ### Changed +- **A retention policy for the audit trail.** `AUDIT_RETENTION_DAYS` removes rows older than the + window it names, swept hourly by whichever server holds an advisory lock. Unset by default, because + deleting somebody's audit trail because a default said so is the worse of the two failures. The + trail stays append-only: the database permits a delete only when the transaction declares a + retention window and only for rows already outside it, so removing recent rows is still impossible + and an `UPDATE` still is under every condition. +- **`allowed_groups` is documented as a declaration, not a control.** The tenant package writes it and + nothing reads it on any access path, and `users.groups` is written by nothing either, so both halves + of the rule are waiting on group membership arriving from the identity provider. Channel access is + membership alone. The columns stay, because they are the right shape for the rule they are named + for. Thanks to [@NathanTarbert](https://github.com/CopilotKit/OpenBot/pull/92) and + [@andreolf](https://github.com/CopilotKit/OpenBot/issues/82). - **Running with no sign-in takes a flag and nothing else.** It used to be locked with `NODE_ENV=production`, which is exactly backwards: `NODE_ENV` is unset unless somebody sets it, so a container on a VM with a hand-written env file and no identity provider served every visitor on diff --git a/server/src/computer/policy-listener.ts b/server/src/computer/policy-listener.ts index 01dba02a..89b1f19b 100644 --- a/server/src/computer/policy-listener.ts +++ b/server/src/computer/policy-listener.ts @@ -25,12 +25,12 @@ export async function startPolicyListener( ): Promise { const connection = postgres(databaseUrl, { max: 1 }); - await connection.listen(ACTION_POLICY_TOPIC, () => { - /* - * The payload is ignored on purpose. It says the boundary moved, not what it moved to: a rule - * list can outgrow NOTIFY's 8000-byte cap, and a server enforcing whatever fitted in a payload - * would be a subtler version of the bug this fixes. The row is the record; this re-reads it. - */ + /* + * The payload is ignored on purpose. It says the boundary moved, not what it moved to: a rule list + * can outgrow NOTIFY's 8000-byte cap, and a server enforcing whatever fitted in a payload would be + * a subtler version of the bug this fixes. The row is the record; this re-reads it. + */ + const reread = () => { void store.refresh().catch((error) => { console.error( JSON.stringify({ @@ -40,7 +40,21 @@ export async function startPolicyListener( }), ); }); - }); + }; + + /* + * Also on reconnect, which is the case a live notification cannot cover. + * + * A NOTIFY reaches whoever is listening at the time. A replica that was restarting, or whose + * connection had dropped, is not, so it misses the announcement and goes on enforcing the rules it + * read at boot until something restarts it again. That is the original bug wearing a smaller hat, + * and it is worse for being intermittent. + * + * `onlisten` fires whenever the driver establishes or re-establishes the subscription, so the row + * is re-read at exactly the moments this server could have missed something. Thanks to + * @NathanTarbert, whose #94 caught this. + */ + await connection.listen(ACTION_POLICY_TOPIC, reread, reread); return { stop: async () => { diff --git a/server/src/computer/policy-store.ts b/server/src/computer/policy-store.ts index 94111bef..f8abc93e 100644 --- a/server/src/computer/policy-store.ts +++ b/server/src/computer/policy-store.ts @@ -90,33 +90,38 @@ export function createPolicyStore( // Written before it is enforced. If the write fails this throws and the caller reports a // failure, which is the honest outcome: an administrator who is told a rule was saved must // not be enforcing a rule that will disappear at the next restart. - await database - .insert(actionPolicy) - .values({ - id: CURRENT, - mode: next.mode, - deny: next.deny, - allow: next.allow, - updatedBy: by ?? null, - updatedAt: new Date(), - }) - .onConflictDoUpdate({ - target: actionPolicy.id, - set: { + // + // The announcement goes in the same transaction, so it is delivered on commit: a write that + // rolls back announces nothing, and there is no window where the row has changed and the + // other servers have not been told. Same shape as `channels/routes.ts`. + await database.transaction(async (transaction) => { + await transaction + .insert(actionPolicy) + .values({ + id: CURRENT, mode: next.mode, deny: next.deny, allow: next.allow, updatedBy: by ?? null, updatedAt: new Date(), - }, - }); + }) + .onConflictDoUpdate({ + target: actionPolicy.id, + set: { + mode: next.mode, + deny: next.deny, + allow: next.allow, + updatedBy: by ?? null, + updatedAt: new Date(), + }, + }); + + // Every server hears it, including this one, which re-reads and arrives at what it + // already has. + await announce(transaction); + }); } current = next; - - // Announced after the row is written, so a server that re-reads on this signal finds the new - // rule rather than the old one. Every server hears it, including this one, which re-reads and - // arrives at what it already has. - if (database) await announce(database); }, reset: async () => { @@ -124,10 +129,14 @@ export function createPolicyStore( // this deployment has no boundary of its own again, and changing what configuration says then // changes what it enforces, which is what an operator expects of a reset. if (database) { - await database.delete(actionPolicy).where(eq(actionPolicy.id, CURRENT)); + await database.transaction(async (transaction) => { + await transaction + .delete(actionPolicy) + .where(eq(actionPolicy.id, CURRENT)); + await announce(transaction); + }); } current = clone(configured); - if (database) await announce(database); }, load: async () => { @@ -176,7 +185,7 @@ export function createPolicyStore( * announcement costs the other servers their update until their next restart, which is worth a loud * log and is not worth failing a write an administrator has been told succeeded. */ -async function announce(database: Database): Promise { +async function announce(database: Pick): Promise { try { await database.execute(sql`select pg_notify(${ACTION_POLICY_TOPIC}, '')`); } catch (error) { diff --git a/server/tests/policy-fanout.integration.test.ts b/server/tests/policy-fanout.integration.test.ts index c85e1183..95b33c2a 100644 --- a/server/tests/policy-fanout.integration.test.ts +++ b/server/tests/policy-fanout.integration.test.ts @@ -130,7 +130,76 @@ describe("a rule added on one server", () => { }); }); +describe("a server that was not listening when it changed", () => { + test("catches up when its subscription comes back", async () => { + /* + * A NOTIFY reaches whoever is listening at the time. A replica that was restarting, or whose + * connection had dropped, is not, so it misses the announcement and goes on enforcing what it + * read at boot until something restarts it again. That is the original bug wearing a smaller hat + * and it is worse for being intermittent: the fleet disagrees with itself and nothing says so. + * + * Simulated the way it happens: the rule changes while this server has no subscription, and then + * the subscription is established. `onlisten` fires on every establish, including reconnects, so + * the row is re-read at exactly the moments a notification could have been missed. + * + * Thanks to @NathanTarbert, whose #94 caught this. + */ + const wroteIt = createPolicyStore(DEFAULT_ACTION_POLICY, database); + const wasDown = createPolicyStore(DEFAULT_ACTION_POLICY, database); + await wasDown.load(); + expect(wasDown.get().deny).toEqual([]); + + // Changed while nothing on this server is listening. + await wroteIt.set({ mode: "enforce", deny: [RULE], allow: ["true"] }); + expect(wasDown.get().deny).toEqual([]); + + const listener = await startPolicyListener(databaseUrl, wasDown); + try { + await until(() => wasDown.get().deny.length > 0); + expect(wasDown.get().deny).toEqual([RULE]); + } finally { + await listener.stop(); + } + }); +}); + describe("the announcement itself", () => { + test("a write that rolls back announces nothing", async () => { + /* + * The announcement is in the same transaction as the write, so it is delivered on commit. A + * failed write that had already announced would have every other server re-read a row that never + * changed, which is harmless, and a committed write that had not announced yet leaves them stale, + * which is not. Same shape as channel activity. + */ + const listening = createPolicyStore(DEFAULT_ACTION_POLICY, database); + await listening.load(); + const listener = await startPolicyListener(databaseUrl, listening); + + try { + // A store whose write fails after the insert. The transaction rolls back, taking the + // announcement with it. + const failing = new Proxy(database, { + get: (target, property, receiver) => + property === "transaction" + ? async () => { + throw new Error("the write rolled back"); + } + : Reflect.get(target, property, receiver), + }); + const store = createPolicyStore(DEFAULT_ACTION_POLICY, failing); + + await expect( + store.set({ mode: "enforce", deny: [RULE], allow: ["true"] }), + ).rejects.toThrow(); + + // Nothing was saved, so nothing should have been announced and nobody should have moved. + await new Promise((resolve) => setTimeout(resolve, 200)); + expect(listening.get().deny).toEqual([]); + } finally { + await listener.stop(); + } + }); + test("a saved rule is not lost when the announcement fails", async () => { // The row is the record and this process is already enforcing it. Failing the write here would // tell an administrator their rule was rejected when it is saved and live, which is worse than