diff --git a/CHANGELOG.md b/CHANGELOG.md index 715388e37..272f5bbf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,35 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A boundary rule can ask what started a run, not only whose authority it carries + +A routine's turn goes through exactly the path a person's chat turn does, as the routine's owner: +their grants, their connections, their thread. That is the right design, and it is also why +`actor.id` cannot tell a scheduled run at three in the morning from the same person typing. The +trail already drew that distinction — `AuditInitiator` is signed into the run assertion and written +onto the row, so an investigator can see a routine caused something. A rule could not ask the same +question. + +The policy context now carries `initiator`, with the kind and id the trail already records, so this +is writable: + +``` +deny: initiator.kind == "routine" && intent == "run_command" +``` + +A deployment happy for a Bot to run a shell while somebody watches, and not happy for it to do so +unattended, can now say so. The id is there too, so a single routine can be named rather than +scheduled runs as a class. `handoff` is its own kind, for a Bot that hands work to another Bot. + +**Nothing is refused that was not refused before.** The field is neutral — `{kind: "person", id: ""}` +— everywhere a person is driving, which is every path that does not carry an initiator today, +including every action on a Bot'"'"'s computer: those are driven by the browser, so they really are +somebody'"'"'s session. It is required rather than optional for the reason #115 exists: cel-js throws on +an unbound identifier and a throw fails closed, so a field that were sometimes absent would turn one +rule about routines into a deployment that refused every ordinary click. + +Replaying a rule against history reads the initiator off the row when it is there and treats a row +that predates the field, or carries a shape this version does not recognise, as a person. ### The Python LangGraph Bot tells its model why the deployment refused a tool call When the deployment would not run a tool call from the Python LangGraph Bot — a token it no longer diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index ffe66ab2d..1d1226079 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -41,6 +41,7 @@ import { type ActionPolicy, evaluateActionPolicy, type PolicyContext, + policyInitiator, type PolicyDecision, } from "./policy"; import type { ComputerProvider } from "./provider"; @@ -513,6 +514,13 @@ export function createComputerGateway( ? describeFile(filePath) : { path: "", name: "", extension: "" }, command: subject.command ?? "", + /* + * A person, and truthfully so today: a Bot's computer is driven by frontend tools in the + * browser, so every action arriving here came from somebody's session rather than from a + * schedule. #298 is the change that would make that untrue, and it is the one that has to pass + * the run's own initiator through instead of inheriting this. + */ + initiator: policyInitiator(), // Neutral, like the fields above: this is not an MCP call, but a `deny: mcp.effect == "write"` // names `mcp`, and cel-js throws on an unbound identifier — which fails closed and would refuse // every browser action the moment an operator wrote a rule about their tools. Empty server and diff --git a/server/src/computer/policy-dry-run.ts b/server/src/computer/policy-dry-run.ts index 46f3347cc..8249a5f07 100644 --- a/server/src/computer/policy-dry-run.ts +++ b/server/src/computer/policy-dry-run.ts @@ -12,12 +12,13 @@ * behaves one way here and another way live would make this feature worse than absent. */ -import type { AuditEvent } from "../audit"; +import type { AuditEvent, AuditInitiator } from "../audit"; import { describeFile, hostOf, intentOf } from "./gateway"; import { type ActionPolicy, evaluateActionPolicy, type PolicyContext, + policyInitiator, } from "./policy"; /** @@ -77,6 +78,23 @@ const CHANGES_CAP = 50; * Null when the row does not carry enough to replay — a row from before a field existed, or a * hand-inserted one. Skipped rather than guessed at. */ +/** + * The initiator an audit payload carries, narrowed back to the union. + * + * Its own reader rather than the one in `callback-token.ts`: that one narrows a value this + * deployment signed and can trust the shape of, and this one reads a stored row that may predate the + * field, have been written by an older version, or been inserted by hand. Anything it does not + * recognise reads as absent, and `policyInitiator` turns that into a person. + */ +function initiatorFromPayload(value: unknown): AuditInitiator | undefined { + if (!value || typeof value !== "object") return undefined; + const kind = (value as { kind?: unknown }).kind; + if (kind === "person" || kind === "deployment") return { kind }; + if (kind !== "routine" && kind !== "handoff") return undefined; + const id = (value as { id?: unknown }).id; + return typeof id === "string" && id ? { kind, id } : undefined; +} + export function contextFromAuditPayload( payload: Record, ): PolicyContext | null { @@ -116,6 +134,13 @@ export function contextFromAuditPayload( file: file ? describeFile(file) : { path: "", name: "", extension: "" }, command: text(payload.command), mcp: { server: "", tool: "", effect: "" }, + /* + * Read off the row when it is there, neutral when it is not. The rows this replays are computer + * actions, which carry no initiator today, so in practice this is a person — but reading it + * rather than hardcoding it means a replay stays honest the day those rows do carry one, and a + * rule being tested against history is judged on what actually happened. + */ + initiator: policyInitiator(initiatorFromPayload(payload.initiator)), }; } diff --git a/server/src/computer/policy.ts b/server/src/computer/policy.ts index 1766ecc47..0318de503 100644 --- a/server/src/computer/policy.ts +++ b/server/src/computer/policy.ts @@ -15,6 +15,7 @@ * defeated by a broader rule that grants it, or a company cannot reason about what it has forbidden. */ import { evaluate } from "cel-js"; +import type { AuditInitiator, AuditInitiatorKind } from "../audit"; export type PolicyMode = "dry-run" | "enforce"; @@ -150,8 +151,47 @@ export type PolicyContext = { * and no list catches them all. The boundary is the container the command runs in. */ command?: string; + /** + * What caused this run, as distinct from whose authority it carries. + * + * `actor.id` answers "whose grants and connections is this spending", and for a routine that is + * its owner — asleep, at three in the morning, with the run going through exactly the path their + * own chat turn takes. That is the right design and it is also why `actor` cannot answer "was + * anybody there". The trail already draws the distinction: `AuditInitiator` is signed into the run + * assertion and written onto the row, with the docstring "what caused a row, where `actorUserId` + * is only whose authority it borrowed". A rule could not ask the same question. + * + * So `deny: initiator.kind == "routine" && intent == "run_command"` is now writable — a deployment + * that is happy for a Bot to run a shell while somebody watches, and not happy for it to do so + * unattended, can say so. + * + * REQUIRED, not optional, and flattened to two always-present strings. cel-js throws on an + * unbound identifier and a throw fails closed, so a rule naming this field would have refused + * every action built by a call site that forgot it — the failure #115 exists to prevent. `id` is + * `""` for `person` and `deployment`, which carry none, the same neutral `mcp.effect` uses. + */ + initiator: { kind: AuditInitiatorKind; id: string }; }; +/** + * The initiator as the policy sees it, defaulting to a person. + * + * A person is the honest default rather than a convenient one: every path that does not carry an + * initiator today is one a person drove. The computer gateway is the case worth naming — a Bot's + * computer is driven by frontend tools in the browser (`app/src/lib/copilot/computer-tools.tsx`), so + * every action reaching that gateway came from somebody's session. When that stops being true, the + * call site has to say so rather than inherit this. + */ +export function policyInitiator( + initiator?: AuditInitiator, +): PolicyContext["initiator"] { + if (!initiator) return { kind: "person", id: "" }; + return { + kind: initiator.kind, + id: "id" in initiator ? initiator.id : "", + }; +} + export type PolicyDecision = { allowed: boolean; mode: PolicyMode; diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 63d611f34..852d82c90 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -8,6 +8,7 @@ import { type ActionPolicy, evaluateActionPolicy, type PolicyContext, + policyInitiator, } from "../computer/policy"; import { type CredentialExecutor, @@ -2954,6 +2955,13 @@ export function createPluginStore(options: PluginStoreOptions) { command: "", intent: effect === "write" ? "write_tool" : "read_tool", mcp: { server: serverId, tool: toolName, effect }, + /* + * The real one, and this is the path where it is not neutral. A routine's turn reaches its + * tools through here, carrying the initiator its run assertion was signed with, so this is + * where `initiator.kind == "routine"` becomes a rule a deployment can actually write. A + * chat turn arrives with none and reads as a person. + */ + initiator: policyInitiator(input.initiator), }; const verdict = evaluateActionPolicy(options.policy(), context); diff --git a/server/tests/computer-policy.test.ts b/server/tests/computer-policy.test.ts index 63f886c99..27ab58570 100644 --- a/server/tests/computer-policy.test.ts +++ b/server/tests/computer-policy.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { type ActionPolicy, evaluateActionPolicy, + policyInitiator, type PolicyContext, } from "../src/computer/policy"; import { parseActionPolicy } from "../src/computer/policy-store"; @@ -22,6 +23,8 @@ function context(overrides: Partial = {}): PolicyContext { actor: { id: "dev-local-user" }, page: { url: "https://example.com/order", host: "example.com" }, element: { ref: "e13", role: "button", name: "Submit order" }, + // A person, which is what every context the gateway builds carries today. + initiator: { kind: "person", id: "" }, ...overrides, }; } @@ -672,3 +675,111 @@ describe("refusal wording under the context the gateway actually builds", () => expect(decision.reason).toContain("search_notes on notes"); }); }); + +/** + * What started the run, as a thing a rule can ask about. + * + * `actor` says whose grants a run is spending, and for a routine that is its owner — which is + * correct and is also why it cannot answer "was anybody watching". The trail already separates the + * two through `AuditInitiator`; these pin that the boundary can now separate them too. + */ +describe("a rule about what started the run", () => { + const unattended = context({ + tool: { name: "mcp__jira__createJiraIssue" }, + intent: "write_tool", + initiator: { kind: "routine", id: "nightly-summary" }, + }); + + test("a routine is refused by a rule naming it, and a person is not", () => { + const policy: ActionPolicy = { + mode: "enforce", + deny: ['initiator.kind == "routine" && intent == "write_tool"'], + allow: ["true"], + }; + + expect(evaluateActionPolicy(policy, unattended).allowed).toBe(false); + + // The same action, with somebody in front of it. This is the whole point of the field: the rule + // separates when it happened from whose authority it carried, and `actor` is identical in both. + const watched = context({ + tool: { name: "mcp__jira__createJiraIssue" }, + intent: "write_tool", + initiator: { kind: "person", id: "" }, + }); + expect(evaluateActionPolicy(policy, watched).allowed).toBe(true); + }); + + test("one routine can be named without catching the others", () => { + // The id is on the context, so a deployment can exempt or target a single routine rather than + // being forced to decide about scheduled runs as a class. + const policy: ActionPolicy = { + mode: "enforce", + deny: ['initiator.id == "nightly-summary"'], + allow: ["true"], + }; + + expect(evaluateActionPolicy(policy, unattended).allowed).toBe(false); + expect( + evaluateActionPolicy( + policy, + context({ initiator: { kind: "routine", id: "weekly-digest" } }), + ).allowed, + ).toBe(true); + }); + + test("a rule naming the initiator does not refuse an action that has a person", () => { + /* + * The #115 property, for this field. cel-js throws on an unbound identifier and a throw fails + * closed, so a field that were optional-and-sometimes-absent would turn one rule about routines + * into a deployment that refuses every ordinary click. It is required on the type and neutral + * everywhere precisely so this stays true. + */ + const policy: ActionPolicy = { + mode: "enforce", + deny: ['initiator.kind == "routine"'], + allow: ["true"], + }; + + const decision = evaluateActionPolicy(policy, context()); + expect(decision.allowed).toBe(true); + expect(decision.source).toBe("allow"); + }); + + test("a handoff is its own kind, not a person and not a routine", () => { + // A Bot handing work to another Bot has somebody behind it somewhere and nobody watching that + // run. Naming it separately is what lets a deployment decide about the two differently. + const policy: ActionPolicy = { + mode: "enforce", + deny: ['initiator.kind == "handoff"'], + allow: ["true"], + }; + + expect( + evaluateActionPolicy( + policy, + context({ initiator: { kind: "handoff", id: "risk-analyst" } }), + ).allowed, + ).toBe(false); + expect(evaluateActionPolicy(policy, unattended).allowed).toBe(true); + }); +}); + +describe("policyInitiator", () => { + test("nothing is a person, because every path without one is driven by somebody", () => { + expect(policyInitiator()).toEqual({ kind: "person", id: "" }); + }); + + test("a kind that carries no id still gets one, so a rule naming id cannot throw", () => { + expect(policyInitiator({ kind: "deployment" })).toEqual({ + kind: "deployment", + id: "", + }); + }); + + test("a routine keeps its id", () => { + expect(policyInitiator({ kind: "routine", id: "nightly" })).toEqual({ + kind: "routine", + id: "nightly", + }); + }); +}); diff --git a/server/tests/policy-dry-run.test.ts b/server/tests/policy-dry-run.test.ts index e5933733d..42baff5d5 100644 --- a/server/tests/policy-dry-run.test.ts +++ b/server/tests/policy-dry-run.test.ts @@ -41,6 +41,42 @@ const CLICK_SUBMIT = { element: { role: "button", name: "Submit order" }, }; +describe("the initiator a replayed row carries", () => { + test("a row with no initiator replays as a person, not as an unbound identifier", () => { + // Every computer row written before this field existed. A rule naming the initiator has to judge + // them rather than throw, which fails closed and would report a boundary far stricter than the + // one being tested. + expect(contextFromAuditPayload(CLICK_SUBMIT)?.initiator).toEqual({ + kind: "person", + id: "", + }); + }); + + test("a row that names a routine replays as that routine", () => { + expect( + contextFromAuditPayload({ + ...CLICK_SUBMIT, + initiator: { kind: "routine", id: "nightly-summary" }, + })?.initiator, + ).toEqual({ kind: "routine", id: "nightly-summary" }); + }); + + test("a shape this version does not recognise reads as a person rather than as itself", () => { + // A row from a later version, or one inserted by hand. Guessing at it would replay a rule + // against a kind no branch here knows, so it is read as absent and neutralised. + for (const initiator of [ + { kind: "wat", id: "x" }, + { kind: "routine" }, + "routine", + null, + ]) { + expect( + contextFromAuditPayload({ ...CLICK_SUBMIT, initiator })?.initiator, + ).toEqual({ kind: "person", id: "" }); + } + }); +}); + describe("contextFromAuditPayload", () => { test("rebuilds the element with the ref that is stored beside it", () => { const context = contextFromAuditPayload(CLICK_SUBMIT);