v2: Action policies + audit — org policy engine, grants, action log - #140
v2: Action policies + audit — org policy engine, grants, action log#140yourbuddyconner wants to merge 16 commits into
Conversation
…ved gate ids rejected
Adds action_policies, runtime_grants, action_policy_overrides tables plus a nullable audit extension to action_invocations, a verbatim port of the legacy param-matcher engine, and a pure resolvePolicyDecision precedence core (org-deny-absolute, grant, override, org rungs, plugin default, risk default) per the adjudicated Task 2 precedence order.
Adds /api/org/policies (admin CRUD + resolver preview), /api/org/action-log (keyset-paginated audit read), /api/me/policy-overrides and /api/me/grants (upsert/delete-by-target, soft-revoke). Extends /api/plugins with per-service action catalogs and adds the route-level always_allow admin 403 at decision resolve time.
validateOverrideBounds only checked org policies in the same target dimension as the override being written, letting a non-admin's actionId-scoped allow override permanently outrank an org service/riskLevel-scoped require_approval or deny at real invocation time (per-user overrides sit at rung 2, above org policy at rung 3). Now resolves the exact action's full org-side decision via the plugin catalog for actionId-scoped overrides (fails closed on unknown actions), and conservatively blocks service/riskLevel-scoped overrides against any org policy that can't be proven disjoint. Also adds a keyset-pagination test where rows share startedAt across a page boundary, exercising the invocationId tiebreaker the existing distinct-startedAt test never touched.
validateActionIdOverrideBounds resolved the org-side decision with appliesIn:"session" only, so an org policy scoped appliesIn:"workflow" was invisible to the write-time bound even though a per-user override carries no appliesIn of its own and is consulted on the workflow-side resolution path too. Now resolves both contexts and blocks if either one resolves org deny/require_approval. Also notes in PutPolicyOverrideRequest's doc comment that the fail-closed unknown-actionId rejection covers dynamically-resolved actions absent from the static catalog.
Org admin: Policies page (catalog-driven target picker, matcher rows, per-service kill switches) and keyset-paginated Action log. Per-user: My policy overrides + My active grants under Settings > You > Policies. Extends decision-gate-card.tsx to disable/tooltip always_allow for non-admins, matching the API's admin-only gate.
Matcher values now coerce to the shape matchers.ts actually requires: number for gt/gte/lt/lte (was silently non-matching as a string), string[] from a comma-separated list for in/not_in (was a 400). Non-numeric/empty input blocks submit with a visible error instead of creating an inert or rejected policy. Also removes the unused usePreviewOrgPolicy hook (no caller); the route and client method stay.
Fixture-first (registerFauxProvider, no network): walks the spec's exit criteria end to end — defaults intact, service-scope deny, param-matcher gate, approve-for-session grant lifecycle incl. revoke-on-destroy, per-user override, and workflow tool-node enforcement incl. "grant the rest of this run" — with every step's action_invocations row provenance-checked via GET /api/org/action-log. Two small fixes surfaced while writing it: - ResolveWorkflowApprovalRequest never carried grantActions through to the approval-node signal payload, so "grant the rest of this run" was dead on arrival over HTTP; wired it through routes/workflows.ts. - The integration test harness's LocalRunHost had no onApprovalGrant wired (unlike the real providers/node.ts boot path), so no test could exercise workflow approval grants at all; added the same wiring to _setup.ts.
Status -> Implemented; new Deviations section consolidates T2-T5's adjudications plus two T6 plumbing fixes and three disclosed follow-ups found by the exit-criteria e2e. Decision 6 annotated with the resolvedMode- vs-status binding rule owed from T1. Handoff row #6 updated.
C1: validateActionIdOverrideBounds strips paramMatchers from org rows before resolving (params:undefined would drop matcher-scoped policies, letting a member allow override slip past an org require_approval). I1: mostSpecific breaks same-specificity ties by most-restrictive mode then newest updatedAt (was DB encounter order); writeAlwaysAllowPolicy soft-revokes competing non-deny action-scope org rows so the always-allow row governs.
I3: listActionLog sorts/cursors/time-filters on createdAt (always populated) instead of coalesce(started_at,0), so denied/rejected and workflow rows (null startedAt) interleave chronologically instead of sinking to the tail and out of from/to windows. I4: POST /api/org/policies/preview resolves the real plugin defaultApprovalMode via actionPluginByService instead of passing undefined. Small: action-invoker discovery-before-enforcement comment; cross-org DELETE 404 test; spec decision 4 prose softened + Deviations entry for params/result-not-persisted + multiplayer owner-overrides note.
Regression pins for C1: an org action-scope require_approval policy carrying paramMatchers (session and workflow appliesIn variants) must still 400 a member's allow override. Without the paramMatcher strip these resolve to the risk default with params:undefined and the bypass silently reopens. Also fix cosmetic Deviations numbering in the action-policies spec (#6 was ordered before #5).
xBalbinus
left a comment
There was a problem hiding this comment.
Reviewed with an emphasis on the policy engine, the enforcement call sites, and audit-log integrity, since this decides what runs and records the trail. The resolution ladder is clear, but a few paths let a privileged action slip the gate or leave an incomplete audit record.
High — the approval gate can be bypassed
A user allow override permanently outranks an org policy created later — resolution.ts:283, admin.ts:135. resolvePolicyDecision returns a matching override at rung 2, above org policy at rung 3, and the loosening bound (validateOverrideBounds) is enforced only when the override is written; the org-policy write path (createOrgPolicy/updateOrgPolicy) never re-validates existing overrides. So: no org policy exists for gmail.send_email; a member writes an allow override (bounds check passes against the plugin/risk default); an admin later adds an org require_approval; every subsequent invocation still resolves to allow at rung 2 and the high-risk action runs ungated. Only an org deny re-supersedes, never require_approval. Mirror writeAlwaysAllowPolicy's supersede sweep on the org-policy write path (delete or flag conflicting overrides), or bound a matching override's allow against the org policy's resolved mode at resolution time so an override can only tighten, never loosen.
The workflow path keys policy and audit on the bare action name — action-invoker.ts:265, resolution.ts:186. Enforcement matches actionId by exact string equality, but the session path passes the canonical entry.action.id (fully qualified — every github action is github.<name>) while the workflow path forwards the DAG node's bare req.action (send_message, createIssue). For any qualified-id plugin the two paths compare different strings, so a single org policy row cannot cover both — including a deny kill-switch, which the workflow path silently bypasses (it falls through to the plugin/risk default, executes, and audits allowed). Canonicalize to one fully-qualified form on both sides: pass the resolved action.id into enforceWorkflowPolicy for both resolveActionPolicy and the audit write (as buildActionContext already does at :218), and canonicalize actionId at policy-write time.
Medium
Workflow approval grants are unbounded and owner-authorized — node.ts:313. Resolving an approval node writes exec-scoped allow grants (rung 1, above org require_approval at rung 3) from run-owner-supplied grantActions, with no scoping to the workflow's referenced actions and no admin check — asymmetric with the session always_allow path, which enforces AlwaysAllowNotAdminError. If least-privilege is intended, constrain grantActions to the (service, action) pairs the run's definition references and/or require an admin resolver for actions currently under an org require_approval; otherwise document that an approval node grants run-wide allow.
The audit sink is best-effort and silently lossy — service.ts:459. persistInvocationAudit swallows insert errors to console.error, and the session path emits it fire-and-forget without awaiting, so a mutating action returns success even when its audit row never persists — the log can't be relied on as authoritative. For high/critical-risk actions, surface the write failure (metric/alert, durable retry, or fail-closed) rather than swallowing it.
A malformed matcher path breaks a policy at evaluation — matchers.ts:75. validateParamMatchers checks op/value/regex but never parses the path bracket syntax, so a bad path (items[x]) persists and then throws inside parsePath on every evaluation, silently disabling that action's policy. Parse the path at creation time and reject on failure, or make readPath/parsePath fail closed by returning undefined like the regex handling at :166.
The gated-audit primary key can collide — service.ts:468. gatedAuditId(sessionId, resumeKey, gateOrdinal) omits queueItemId, the thing that scopes gateOrdinal, so gated invocations of the same (tool, params) across turns collide and the second row is dropped by onConflictDoNothing. Thread queueItemId into the record and the key.
The audit contract carries no params/result — types.ts:638. PolicyInvocationRecord has no params/result fields, so action_invocations.params/result are always null on the session path (and result on every path). Add them to the record and populate from call_tool.
Low
The regex matcher is a ReDoS vector — matchers.ts:164. new RegExp(matcher.value).test(actual) runs a caller-supplied pattern with no length or time bound on the resolution hot path. Cap the pattern length and/or use a linear-time engine or a timeout guard.
Cached workflow denials survive a re-drive — action-invoker.ts:126. A deny/require_approval outcome is stored as the cached invocation result keyed on the deterministic invocationId, so a same-run re-drive returns the stale denial without re-running enforcement against changed grants. If re-drive after grant is a real path, keep non-allow outcomes out of the dedup cache.
Adds org-level action policies with a full audit trail: admins can allow, deny, or require approval for agent actions (by service, specific action, or risk level, optionally scoped by parameter matchers), members get bounded personal overrides, approvals can mint session- or run-scoped grants, and every policy decision lands in a queryable action log.
What you can do
Design notes
policyResolverseam (resolve → decision + extra gate actions, with resolution/invocation callbacks). Absent the seam, behavior is byte-identical — pinned by test.Testing
messages.abort.test.tsbaseline, unrelated). Web: 457 passed. Root + web typecheck clean.Known limitations (all recorded in the spec's Deviations section)
docs/specs/2026-07-16-action-policies-audit-design.md— headline items, deferred as a bundled fast-follow because they need a reviewed engine-contract change:Do not merge without a browser pass over the three new settings surfaces (not yet dogfooded live).