fix(agent-platform): stop session_principal approval gate from auto-dispatching

products/agent_platform/services/agent-runner/src/loop/driver.test.ts

@@ -24,7 +24,6 @@ import {
     newTestPrefix,
     PgApprovalStore,
     PgSessionQueue,
-    principalsMatch,
     RedisSessionEventBus,
     S3BundleStore,
     SessionPrincipal,
@@ -37,7 +36,7 @@ const KAFKA_HOSTS = process.env.KAFKA_HOSTS ?? 'localhost:9092'
 import { buildApprovalDecidedMarker } from './approval-marker'
 import { runSession } from './driver'
 import type { OpenedMcp, RemoteMcpTool } from './mcp-clients'
-import { findLastUserSender, type IsAskerInApproverScope } from './per-asker-auth'
+import type { IsAskerInApproverScope } from './per-asker-auth'
 
 const FAUX_MODEL_ID = 'faux/test'
 // Realistic UUID — PG's `uuid` columns (approvals.session_id, etc.) reject
@@ -359,8 +358,8 @@ describe('driver runSession', () => {
      * so they never appear in `spec.tools[]` and the native/custom approval
      * lookup misses them. PR 7 added a fallback that decomposes
      * `<prefix>__<remoteName>` against `spec.mcps[].tools[]` — these tests pin
-     * the wrap path for the MCP variant + the `session_principal` per-asker
-     * fast-path that the concierge case relies on.
+     * the wrap path for the MCP variant + the `session_principal` gate the
+     * concierge relies on (which always queues — it never fast-paths).
      */
     describe('MCP tool approval gating', () => {
         // Minimal `OpenedMcp` stub — same shape as `build-agent-tools.test.ts`'s
@@ -590,12 +589,15 @@ describe('driver runSession', () => {
             expect(out.state).not.toBe('failed')
         })
 
-        it('session_principal per-asker fast-path: dispatches directly when last sender matches session.principal', async () => {
-            // Alice authed the session (`session.principal === alice`) and
-            // is the one driving this turn (`conversation[last].sender === alice`).
-            // The per-asker check returns true on the `session_principal`
-            // branch (no DB roundtrip), the wrap runs the real tool, and no
-            // approval row is created.
+        it('session_principal does NOT fast-path: queues even when the last sender matches session.principal (injection guard)', async () => {
+            // Alice authed the session (`session.principal === alice`) and is
+            // the one driving this turn (`conversation[last].sender === alice`).
+            // Even so, a `session_principal`-gated call must NOT auto-dispatch:
+            // the owner being the asker is not consent to the specific gated
+            // call the model emitted (which a prompt injection in content the
+            // agent read could have steered). The wrap queues; the real remote
+            // tool is never hit. Regression guard for the "approval bypass for
+            // session-principal tools" finding.
             const mcp = makeFakeMcp('posthog', POSTHOG_REF, {
                 'agent-applications-revisions-promote-create': { description: 'd', result: { promoted: true } },
             })
@@ -604,34 +606,32 @@ describe('driver runSession', () => {
                 principal: principalAlice,
                 conversation: [{ role: 'user', content: 'promote it', sender: principalAlice, timestamp: Date.now() }],
             })
-            // Direct stub — same contract as `makePerAskerAuth` returns. We
-            // route through `principalsMatch` to mirror the production check.
+            // Faithful stub of the production `makePerAskerAuth`: `team_admins`
+            // is the only self-authorising fast-path scope. `session_principal`
+            // never clears the gate, so the concierge's `['session_principal']`
+            // policy resolves to false here.
             const out = await run(makeRev({ mcps: [POSTHOG_REF as never] }), session, {
                 script: [
                     toolUse([
                         call('posthog__agent-applications-revisions-promote-create', {
                             application_id: 'app',
                         }),
                     ]),
-                    stop('done'),
+                    stop('queued'),
                 ],
                 approvals,
                 mcpClients: [mcp],
-                isAskerInApproverScope: (async (conversation, _teamId, scope, sessionPrincipal) => {
-                    if (!scope.includes('session_principal')) {
-                        return false
-                    }
-                    const sender = findLastUserSender(conversation)
-                    return Boolean(sender && principalsMatch(sessionPrincipal, sender))
-                }) satisfies IsAskerInApproverScope,
+                isAskerInApproverScope: (async (_conversation, _teamId, scope) =>
+                    scope.includes('team_admins')) satisfies IsAskerInApproverScope,
             })
             expect(out.state).toBe('completed')
-            // Fast-path ran the real remote tool exactly once.
-            expect(mcp.calls).toEqual([
-                { name: 'agent-applications-revisions-promote-create', args: { application_id: 'app' } },
-            ])
-            // No approval row queued — that's the whole point of the fast-path.
-            expect(await approvals.listBySession(TEST_SESSION_ID)).toHaveLength(0)
+            // The real remote tool was NEVER called — the gate held.
+            expect(mcp.calls).toEqual([])
+            // Exactly one approval row queued for the gated call.
+            const rows = await approvals.listBySession(TEST_SESSION_ID)
+            expect(rows).toHaveLength(1)
+            expect(rows[0].tool_name).toBe('posthog__agent-applications-revisions-promote-create')
+            expect(rows[0].state).toBe('queued')
         })
     })
 

products/agent_platform/services/agent-runner/src/loop/per-asker-auth.test.ts

@@ -236,7 +236,7 @@ describe('makePerAskerAuth', () => {
         expect(await isAuthed(conv, 7, [], null)).toBe(false)
     })
 
-    describe('session_principal scope (PR 7 — concierge fast-path)', () => {
+    describe('session_principal scope (NOT a fast-path — injection guard)', () => {
         const alice: SessionPrincipal = {
             kind: 'posthog',
             user_id: 'alice',
@@ -248,28 +248,28 @@ describe('makePerAskerAuth', () => {
             team_id: 7,
         }
 
-        it('returns true when the session principal matches the most recent user-turn sender', async () => {
-            // Alice authed the session and is the one driving the current
-            // turn — fast-path authorises without touching the posthog DB
-            // (the fake's query throws if called).
+        it('returns false even when the session principal matches the most recent user-turn sender', async () => {
+            // Regression guard for the "approval bypass for session-principal
+            // tools" finding. Alice authed the session and is the one driving
+            // the turn — but the owner being the *asker* is NOT consent to the
+            // specific gated call the model emitted (a prompt injection in
+            // content the agent read could have steered that call). So
+            // `session_principal` must NOT self-authorise; it defers to the
+            // queue. The posthog DB fake throws if hit — a `session_principal`-
+            // only scope must short-circuit to false before any DB roundtrip.
             const isAuthed = makePerAskerAuth({
                 identities: new PgIdentityStore(pool),
                 posthogDb: {
                     async query() {
-                        throw new Error('should not hit posthog DB on the session_principal fast path')
+                        throw new Error('session_principal must not touch the posthog DB — it never fast-paths')
                     },
                 } as unknown as import('pg').Pool,
             })
             const conv = [userMsg('promote it', alice)]
-            expect(await isAuthed(conv, 7, ['session_principal'], alice)).toBe(true)
+            expect(await isAuthed(conv, 7, ['session_principal'], alice)).toBe(false)
         })
 
         it('returns false when the last sender is a different principal than the session owner', async () => {
-            // The trigger edge already enforces strict-principal match on
-            // /send, so in practice we expect alice's session to only ever
-            // see alice's senders. Belt-and-braces: if a sender for bob
-            // somehow lands in alice's session, the session_principal scope
-            // still rejects.
             const isAuthed = makePerAskerAuth({
                 identities: new PgIdentityStore(pool),
                 posthogDb: fakePosthogDb([]),
@@ -287,11 +287,11 @@ describe('makePerAskerAuth', () => {
             expect(await isAuthed(conv, 7, ['session_principal'], null)).toBe(false)
         })
 
-        it('returns false for anonymous principals (public agent — every caller would otherwise self-authorise)', async () => {
-            // The public verifier stores { kind: 'anonymous' } — not null — on
-            // the session row and stamps the same sender on the user turn.
-            // principalsMatch(anonymous, anonymous) is true, so without the
-            // explicit exclusion every public caller would clear the gate.
+        it('returns false for anonymous principals (public agent)', async () => {
+            // The public verifier stores { kind: 'anonymous' } on the session
+            // row and stamps the same sender on the user turn. `session_principal`
+            // never fast-paths regardless, so a public caller can't clear the
+            // gate this way.
             const anon: SessionPrincipal = { kind: 'anonymous' }
             const isAuthed = makePerAskerAuth({
                 identities: new PgIdentityStore(pool),
@@ -301,12 +301,11 @@ describe('makePerAskerAuth', () => {
             expect(await isAuthed(conv, 7, ['session_principal'], anon)).toBe(false)
         })
 
-        it('falls through to team_admins when session_principal does not match but team_admins is in scope', async () => {
-            // Mixed-scope policy: session principal OR team admin. The
-            // session principal slot doesn't match (bob's sender vs alice's
-            // session), so we fall through and resolve team_admins via the
-            // existing path. Alice is configured as a team admin on team 7
-            // via the posthog-DB fake.
+        it('falls through to team_admins when session_principal is paired with team_admins in scope', async () => {
+            // Mixed-scope policy: session principal OR team admin. session_principal
+            // never fast-paths, but team_admins still does — so an admin asker
+            // clears the gate via the team_admins branch. Alice is configured as
+            // a team admin on team 7 via the posthog-DB fake.
             const store = new PgIdentityStore(pool)
             const senderAgent = await store.findOrCreate({
                 team_id: 7,

products/agent_platform/services/agent-runner/src/loop/per-asker-auth.ts

@@ -5,7 +5,7 @@
  * can dispatch the tool directly instead of queueing for someone else to
  * approve.
  *
- * Scopes supported in v0:
+ * Fast-path scopes (skip the queue, run the real tool):
  *   - `team_admins` — the original B.2 v0 scope. Resolution:
  *     - `sender.kind === 'slack'`: `sender.id` is an `agent_user.id`. Look
  *       up the row, read its `posthog_user_id`, check OrganizationMembership
@@ -14,35 +14,41 @@
  *       (chat /run with an admin's token) is a sensible follow-up but adds
  *       a second resolution path that step 3 doesn't need to demo the
  *       Slack scenario.
- *   - `session_principal` (PR 7) — the session-owner-self-authorise case.
- *     Matches the most-recent user-turn sender against `session.principal`
- *     (auth-time identity stored on the session row) using the same strict
- *     `principalsMatch` comparison the trigger edge uses for /send. Stable
- *     across resume — a second user posting to a resumed session can't
- *     bypass the gate by being whoever-spoke-last. Per-asker fast-path
- *     only; queued-approval routing to the session principal widens later.
+ *
+ * NOT a fast-path scope:
+ *   - `session_principal` — marks "the session owner is the approver", but
+ *     the owner being the *asker* is NOT evidence they consented to the
+ *     specific gated call the model emitted. Content the agent reads (fetched
+ *     docs, MCP/tool output, another agent's bundle) can carry a prompt
+ *     injection that steers the model into a destructive gated call while the
+ *     most-recent user-turn sender is still the legitimate owner — so a
+ *     sender↔principal match would silently auto-execute an action the human
+ *     never decided on. `session_principal` therefore always queues for an
+ *     explicit, out-of-band human decision; it never short-circuits `real(...)`.
+ *     The scope is still persisted on the queued row (`approver_scope`) so
+ *     decision-side routing — letting the session owner, not only a team
+ *     admin, clear the queued approval — can land as a follow-up.
+ *     (Fixes the "approval bypass for session-principal tools" review finding.)
  */
 
 import type { Pool } from 'pg'
 
-import { ConversationMessage, IdentityStore, principalsMatch, SessionPrincipal } from '@posthog/agent-shared'
+import { ConversationMessage, IdentityStore, SessionPrincipal } from '@posthog/agent-shared'
 
 /** PostHog's `OrganizationMembership.Level` values — keep in sync with the Django enum. */
 const ADMIN_LEVEL = 8
 
 /**
  * The check the dispatcher runs against the active session conversation.
- * Returns true when the most-recent user turn's sender satisfies one of the
- * scopes in `approverScope`. Returning false defers to the normal queue
- * path; the model never sees the difference.
+ * Returns true when the most-recent user turn's sender satisfies a fast-path
+ * scope in `approverScope` (today: `team_admins` only). Returning false
+ * defers to the normal queue path; the model never sees the difference.
  *
- * `sessionPrincipal` is the auth-time identity persisted on the session row
- * — used for the `session_principal` scope match. Anonymous principals
- * (public agents) are explicitly excluded: the public verifier stores
- * `{ kind: 'anonymous' }` — not null — in the session row, and
- * `principalsMatch` returns true for any two anonymous principals, which
- * would let every caller bypass the gate. The `session_principal` scope is
- * only meaningful for authenticated sessions with a unique identity.
+ * `sessionPrincipal` (the auth-time identity persisted on the session row) is
+ * accepted for forward compatibility with decision-side approver routing but
+ * is NOT consumed by the v0 check: `session_principal` is deliberately not a
+ * self-authorising fast-path scope (see the module doc), so a sender↔principal
+ * match never clears the gate here.
  */
 export type IsAskerInApproverScope = (
     conversation: ConversationMessage[],
@@ -58,22 +64,16 @@ export interface MakePerAskerAuthDeps {
 
 /** Production factory — closes over the identity store + posthog DB pool. */
 export function makePerAskerAuth(deps: MakePerAskerAuthDeps): IsAskerInApproverScope {
-    return async (conversation, teamId, approverScope, sessionPrincipal) => {
-        const sender = findLastUserSender(conversation)
-        // `session_principal` is a pure equality check against the
-        // auth-time principal on the session row — no DB roundtrip. Cheap;
-        // check first so we don't burn a posthog DB query on every gated
-        // call for a concierge-style spec. Anonymous principals are excluded:
-        // `principalsMatch` treats any two anonymous principals as equal, so on
-        // a public agent every caller would self-authorise the gate.
-        if (approverScope.includes('session_principal') && sessionPrincipal && sessionPrincipal.kind !== 'anonymous') {
-            if (sender && principalsMatch(sessionPrincipal, sender)) {
-                return true
-            }
-        }
+    // `sessionPrincipal` (4th arg) is intentionally not destructured: the only
+    // fast-path scope is `team_admins`. `session_principal` is NOT a
+    // self-authorising fast-path — it always defers to the queue so a prompt
+    // injection that the session owner unwittingly relays can't auto-execute a
+    // gated call. See the module doc.
+    return async (conversation, teamId, approverScope) => {
         if (!approverScope.includes('team_admins')) {
             return false
         }
+        const sender = findLastUserSender(conversation)
         if (!sender) {
             return false
         }

products/agent_platform/services/agent-shared/src/spec/spec.ts

@@ -313,12 +313,15 @@ export const TriggerSchema = z.discriminatedUnion('type', [
  *     the owning team. The default scope on every gated tool.
  *   - `session_principal` — the auth-time principal stored on the session
  *     row (NOT the most recent /send sender — see B1 in
- *     `runtime-mcps.md` "Resolved design"). Used by the concierge so the
- *     session owner can authorise their own destructive call without
- *     round-tripping through a team admin; a second user posting to a
- *     resumed session can't bypass the gate. v0 is per-asker fast-path
- *     only — queued-approval routing to the session principal widens
- *     later via approver-scope routing in `approval-gated-tools.md` §6.
+ *     `runtime-mcps.md` "Resolved design"). Marks the session owner as the
+ *     approver. Unlike `team_admins`, this is NOT a per-asker fast-path:
+ *     the owner being the asker is not consent to the specific gated call
+ *     the model emitted (prompt injection in content the agent reads can
+ *     steer that call), so a `session_principal` gate always queues for an
+ *     explicit human decision and never auto-dispatches. Decision-side
+ *     routing — letting the session owner clear the queued approval rather
+ *     than only a team admin — widens later via approver-scope routing in
+ *     `approval-gated-tools.md` §6.
  */
 export const ApproverScopeSchema = z.enum(['team_admins', 'session_principal'])
 

products/agent_platform/services/agent-tests/src/cases/example-agent-builder.test.ts

@@ -167,9 +167,11 @@ describe('example: agent-builder bundle', () => {
         ]) {
             expect(gated.has(required), `${required} should be gated`).toBe(true)
         }
-        // session_principal — the Agent Builder wires every gated tool to the
-        // session owner via the per-asker fast-path, so the user who asked can
-        // approve their own destructive call without a team-admin round-trip.
+        // session_principal — the Agent Builder scopes its destructive tools to
+        // the session owner as approver. This is NOT a self-authorising fast-path:
+        // the call still queues for an explicit human decision (so a prompt
+        // injection the owner unwittingly relays can't auto-promote/-archive).
+        // The scope marks who the queued approval is for; it never auto-runs.
         for (const [, t] of gated) {
             expect(t.approval_policy?.approvers).toEqual(['session_principal'])
         }