diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e952b6e..3cc5bb43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,6 +201,41 @@ Sessions survive and nobody signs in again. is unavailable never blocks a sign-in. ### Fixed +- **The audit trail could be erased with one statement.** It is append-only because a database + trigger refuses updates and deletes, and that trigger is row-level, so `TRUNCATE` never reached it: + anything holding `DATABASE_URL` could empty the table and nothing raised. That is the case the + guarantee exists for, since it is enforced in the database precisely because the application is not + the only thing that reaches the table. A statement-level trigger now refuses a truncate, and it + answers before the retention setting is read, so declaring a retention window no longer permits one + either. Retention itself is unchanged: rows older than the window are still removed, and recent + ones are still refused. The connection the application uses is the database owner in the shipped + compose file, and an owner can still disable or drop a trigger; closing that needs a role with + `INSERT` and `SELECT` only, which is a separate change. Reported by @beardthelion, who also named + the failure mode of the obvious fix and saved it from shipping as one. +- **A declined take-the-wheel destroyed the conversation.** A Bot that asks for help with a sign-in + and never gets it left an assistant message holding a tool call that nothing ever answered, and + every later turn in that thread failed at the provider. Declining once meant nothing you typed + afterwards got an answer, with no way back but a new chat. Unanswered calls are now answered when + the history is rebuilt, with the truth rather than a fake success: no result came, the run has + ended, carry on without it and say what could not be done. +- **The audit trail could not say why a conversation went where it did.** It recorded the router's + choice and recorded nothing at all when a person named a coworker with `@`, which is + indistinguishable from a row that failed to write. A mention is now recorded too, as the person's + own choice, without asking the model a question they had already answered. The audit page names the + coworker and separates the three cases: chosen by the person, matched by the router, or the default + because nothing matched. +- **A Bot with half a connector sent people to a sign-in box.** Granted a vendor's search but not its + read, it found the document, could not read it, and opened the vendor's website to try, where it + met a sign-in wall and asked the person to take the wheel. They already had access; the missing + thing was the Bot's grant, and nothing said so. A gap in what a Bot holds is now reported as a gap: + it names the capability it would need and says an administrator can grant it on that connector. +- **Answers arrived with no sign of where they came from.** Asked a compliance question, a Bot + replied with a filing obligation, a dollar threshold, a deadline and a retention period, and the + audit trail for that turn held one row: the routing decision. A confident unsourced answer is + indistinguishable from a confident wrong one. Every Bot is now told to cite what it read and to say + plainly when an answer is from its own knowledge instead. It is told this by the deployment rather + than per agent, so it cannot be missing from the next Bot somebody adds, and it is explicitly not + an instruction to go hunting for a source. - **A Bot browsed to a vendor it already had tools for.** Granted Google Drive, asked what was in a document, it opened `drive.google.com` in its own browser, met a sign-in page that browser can never satisfy, and asked the person to sign in to an account they had already connected. A tool diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index 665ee4fd..5d523c20 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -116,7 +116,16 @@ export function createRoutingRoutes( 404, ); } - const reason = "you chose them yourself"; + /* + * Third person, because the audit page is not read by the person who chose. + * + * This said "you chose them yourself", which is true in the conversation and false on an + * administrator's screen, where every row is somebody else's. The person is already on the + * row as `actorUserId`; the reason only has to say what kind of decision it was. + * + * @zopeVaibhav had this right in #134. + */ + const reason = "named by the person asking"; await record(actorId(actor), chosen.id, reason, false, true, [chosen.id]); return context.json({ agentId: chosen.id, diff --git a/server/tests/audit-retention.integration.test.ts b/server/tests/audit-retention.integration.test.ts index 8cec8221..5170d0fd 100644 --- a/server/tests/audit-retention.integration.test.ts +++ b/server/tests/audit-retention.integration.test.ts @@ -113,6 +113,37 @@ describe("keeping the audit trail to a retention policy", () => { expect(await remaining()).toBe(1); }); + test("the database refuses a malformed window even when the caller does not", async () => { + /* + * The test above stops in TypeScript. `sweepAuditTrail` returns `{deleted: null}` before it ever + * opens a connection, so it proves the caller's guard and says nothing about the trigger, which + * has a branch of its own for this that nothing exercised. That is the shape where a guard reads + * as covered and is not: anything reaching the table without going through the sweep meets the + * trigger and only the trigger. + * + * These set the value the sweep would have set and then delete directly. + * + * Reported by @beardthelion. + */ + await event(400, 1); + + for (const window of ["0", "-1", "", "garbage"]) { + expect( + await refusedAsAppendOnly(() => + database.transaction(async (tx) => { + await tx.execute( + sql`select set_config('openbot.audit_retention_days', ${window}, true)`, + ); + await tx + .delete(auditEvents) + .where(eq(auditEvents.targetType, MARKER)); + }), + ), + ).toBe(true); + } + expect(await remaining()).toBe(1); + }); + test("a row exactly inside the window survives", async () => { // The boundary. Off by a day here means a deployment promising 90 days keeps 89. await event(89, 1); diff --git a/server/tests/audit.test.ts b/server/tests/audit.test.ts index 30b61b2a..22a7dedf 100644 --- a/server/tests/audit.test.ts +++ b/server/tests/audit.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { readFile } from "node:fs/promises"; +import { readdir, readFile } from "node:fs/promises"; import { createApp } from "../src/app"; import { auditEventTypes, @@ -98,15 +98,36 @@ describe("audit payload redaction", () => { }); describe("audit event immutability", () => { - test("installs a database trigger that rejects updates and deletes", async () => { - const migration = await readFile( - new URL("../drizzle/0000_schema.sql", import.meta.url), - "utf8", - ); - - expect(migration).toContain("CREATE FUNCTION prevent_audit_event_mutation"); - expect(migration).toContain("BEFORE UPDATE OR DELETE ON audit_events"); - expect(migration).toContain("Audit events are append-only"); + /* + * What the guard DOES is proved against a real database in + * audit-retention.integration.test.ts. This asserts only that it is still installed by some + * migration, and it reads the whole chain to do it. + * + * Reading 0000 alone stopped being true a while ago: 0007 replaced the function and 0012 replaced + * it again and added the truncate trigger, so the old assertions described a definition no + * database runs and would have gone on passing if the current one were edited out from under + * them. A mirror test pinned to one file is a test of that file, not of the deployment. + * + * Reported by @beardthelion, alongside the TRUNCATE hole itself. + */ + test("some migration still installs the append-only guard", async () => { + const directory = new URL("../drizzle/", import.meta.url); + const files = (await readdir(directory)) + .filter((name) => name.endsWith(".sql")) + .sort(); + const chain = ( + await Promise.all( + files.map((name) => readFile(new URL(name, directory), "utf8")), + ) + ).join("\n"); + + expect(chain).toContain("FUNCTION prevent_audit_event_mutation"); + expect(chain).toContain("BEFORE UPDATE OR DELETE ON audit_events"); + expect(chain).toContain("BEFORE TRUNCATE ON audit_events"); + expect(chain).toContain("Audit events are append-only"); + // A later migration removing either trigger would otherwise satisfy every line above. + expect(chain).not.toContain("DROP TRIGGER audit_events_append_only"); + expect(chain).not.toContain("DROP TRIGGER audit_events_no_truncate"); }); }); diff --git a/server/tests/routing-routes.test.ts b/server/tests/routing-routes.test.ts index d59ee589..974996b2 100644 --- a/server/tests/routing-routes.test.ts +++ b/server/tests/routing-routes.test.ts @@ -127,6 +127,8 @@ describe("recording which coworker a message went to", () => { viaMention: true, fallback: false, }); + // Third person: an administrator reading this row is not the person who chose. + expect(written[0]?.payload.reason).toBe("named by the person asking"); }); test("naming a coworker never asks the model", async () => {