diff --git a/apps/workers/src/jobs/claw-gateway-run/handler.ts b/apps/workers/src/jobs/claw-gateway-run/handler.ts index cd4db577..a5165013 100644 --- a/apps/workers/src/jobs/claw-gateway-run/handler.ts +++ b/apps/workers/src/jobs/claw-gateway-run/handler.ts @@ -18,7 +18,7 @@ import { clawGatewayRunSchema } from './schema.js'; const log = createLogger('claw-gateway-run'); export async function handleClawGatewayRun(data: unknown): Promise { - const { runId, tenantId, action, content } = clawGatewayRunSchema.parse(data); + const { runId, tenantId, action, content, approvedBy } = clawGatewayRunSchema.parse(data); try { const { executeRun, terminateRun, grantPendingToolsForRun } = await import('@chatbot/claw-studio'); @@ -35,7 +35,12 @@ export async function handleClawGatewayRun(data: unknown): Promise { if (action === 'reject' || action === 'cancel') { log.info({ runId, tenantId, action }, 'Terminating run'); - await terminateRun({ runId, deps, kind: action === 'reject' ? 'reject' : 'cancel' }); + await terminateRun({ + runId, + deps, + kind: action === 'reject' ? 'reject' : 'cancel', + approvedBy: action === 'reject' ? approvedBy : undefined, + }); return; } @@ -43,7 +48,7 @@ export async function handleClawGatewayRun(data: unknown): Promise { await executeRun({ runId, deps, - resume: action ? { action, content } : undefined, + resume: action ? { action, content, approvedBy } : undefined, }); log.info({ runId, tenantId }, 'Gateway run handler finished'); } catch (error) { diff --git a/apps/workers/src/jobs/claw-gateway-run/schema.ts b/apps/workers/src/jobs/claw-gateway-run/schema.ts index 9f8c638a..ec854798 100644 --- a/apps/workers/src/jobs/claw-gateway-run/schema.ts +++ b/apps/workers/src/jobs/claw-gateway-run/schema.ts @@ -18,6 +18,8 @@ export const clawGatewayRunSchema = z.object({ ]).optional(), /** The user's reply text, for clarification_response. */ content: z.string().optional(), + /** The platform user id that made this decision, when it came from a channel. */ + approvedBy: z.string().optional(), }); export type ClawGatewayRunData = z.infer; diff --git a/libs/claw-studio/src/connectors/types.ts b/libs/claw-studio/src/connectors/types.ts index 2809c17d..a61a8dc9 100644 --- a/libs/claw-studio/src/connectors/types.ts +++ b/libs/claw-studio/src/connectors/types.ts @@ -32,6 +32,18 @@ export interface HilCapabilities { export interface BaseConnectorConfig { enabled: boolean; + /** + * Platform user ids allowed to act on this channel's approval affordances + * (Approve / Reject / "Always allow for this task"). Not a secret — these + * are public platform identifiers, not credentials. + * + * Unset or empty means no allowlist is configured yet. See the gateway's + * `handleResume` for the resulting default-deny behavior on + * `approve_always`, which grants a standing permission and is too + * consequential to leave open to any workspace member by default + * (docs/superpowers/claw-studio-known-issues.md #11). + */ + approverIds?: string[]; } export interface SlackConnectorConfig extends BaseConnectorConfig { diff --git a/libs/claw-studio/src/gateway/execute-run.ts b/libs/claw-studio/src/gateway/execute-run.ts index 6aa5a5d5..8d58b326 100644 --- a/libs/claw-studio/src/gateway/execute-run.ts +++ b/libs/claw-studio/src/gateway/execute-run.ts @@ -45,7 +45,7 @@ const logger = createLogger('claw-studio:gateway:execute'); export interface ExecuteRunInput { runId: string; /** Present when this is a HIL resume rather than a first execution. */ - resume?: { action: Exclude; content?: string }; + resume?: { action: Exclude; content?: string; approvedBy?: string }; deps: RouterDeps; /** * Per-run runtime shaping. Optional and defaulted, so channel runs are unchanged; @@ -432,9 +432,12 @@ export async function executeRun(input: ExecuteRunInput): Promise { graphInput = new Command({ resume: { decisions: Array.from({ length: decisionCount }, () => ({ type: 'approve' as const })) }, }); + const approvedSuffix = resume.approvedBy ? ` by ${resume.approvedBy}` : ''; await runs.appendEvent(run, { eventType: 'approval_decision', - content: resume.action === 'approve_always' ? 'approved (always, for this task)' : 'approved', + content: + (resume.action === 'approve_always' ? 'approved (always, for this task)' : 'approved') + + approvedSuffix, }); } else { graphInput = { messages: [new HumanMessage(resume.content ?? '')] }; @@ -570,6 +573,8 @@ export async function terminateRun(input: { runId: string; deps: RouterDeps; kind: 'reject' | 'cancel'; + /** Platform user id that rejected, when `kind` is `'reject'` and it came from a channel. */ + approvedBy?: string; }): Promise { const runs = getRunService(); const bus = getRunEventBus(); @@ -595,7 +600,11 @@ export async function terminateRun(input: { const unsubscribe = adapter ? attachToRun(run, adapter, input.deps) : () => {}; try { if (rejected) { - await runs.appendEvent(run, { eventType: 'approval_decision', content: 'rejected' }); + const approvedSuffix = input.approvedBy ? ` by ${input.approvedBy}` : ''; + await runs.appendEvent(run, { + eventType: 'approval_decision', + content: `rejected${approvedSuffix}`, + }); } else { await runs.appendEvent(run, { eventType: 'status', content: 'cancelled' }); } diff --git a/libs/claw-studio/src/gateway/gateway-service.test.ts b/libs/claw-studio/src/gateway/gateway-service.test.ts index 621dff75..dc4a0406 100644 --- a/libs/claw-studio/src/gateway/gateway-service.test.ts +++ b/libs/claw-studio/src/gateway/gateway-service.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { mockRegistry, mockRunService, adapter } = vi.hoisted(() => { +const { mockRegistry, mockRunService, mockConnectorConfigService, adapter } = vi.hoisted(() => { const adapter = { channelType: 'slack', displayName: 'Slack', @@ -26,6 +26,7 @@ const { mockRegistry, mockRunService, adapter } = vi.hoisted(() => { list: vi.fn(() => [adapter]), }, mockRunService: { create: vi.fn(), get: vi.fn() }, + mockConnectorConfigService: { getRaw: vi.fn() }, }; }); @@ -35,6 +36,12 @@ vi.mock('@chatbot/shared', () => ({ vi.mock('../connectors/registry', () => ({ getConnectorRegistry: () => mockRegistry })); vi.mock('./run-service', () => ({ getRunService: () => mockRunService })); +// Default (no allowlist configured) preserves the pre-existing permissive +// behavior for plain approve/reject; approve_always is covered explicitly +// below since it default-denies with no allowlist regardless. +vi.mock('../connectors/config-service', () => ({ + ClawConnectorConfigService: vi.fn(() => mockConnectorConfigService), +})); import { handleInbound } from './gateway-service'; @@ -66,6 +73,8 @@ beforeEach(() => { adapter.getConfig.mockResolvedValue({ enabled: true }); adapter.parseInbound.mockResolvedValue(newRunMessage); mockRunService.create.mockResolvedValue(createdRun); + // No allowlist configured by default. + mockConnectorConfigService.getRaw.mockResolvedValue(null); }); describe('handleInbound', () => { @@ -114,6 +123,8 @@ describe('handleInbound', () => { // Enqueue before ack: acking first would promise the user a run that might // never be queued. expect(order).toEqual(['create', 'enqueue', 'ack']); + // A fresh run has no approver decision to attribute — approvedBy is + // only threaded through on a HIL resume. expect(enqueue).toHaveBeenCalledWith({ runId: 'run_new', tenantId: 'tenant1' }); }); @@ -194,6 +205,7 @@ describe('handleInbound — HIL resume', () => { tenantId: 'tenant1', action: 'approve', content: undefined, + approvedBy: 'U1', }); // A resume must never create a second run. expect(mockRunService.create).not.toHaveBeenCalled(); @@ -270,6 +282,7 @@ describe('handleInbound — HIL resume', () => { tenantId: 'tenant1', action: 'clarification_response', content: 'the blue one', + approvedBy: 'U1', }); }); @@ -295,3 +308,92 @@ describe('handleInbound — HIL resume', () => { expect(enqueue).not.toHaveBeenCalled(); }); }); + +describe('handleInbound — HIL resume — approver allowlist', () => { + const replyMessage = { + ...newRunMessage, + taskDescription: '', + replyContext: { runId: 'run_existing', action: 'approve' as const, tenantId: 'tenant1' }, + }; + + beforeEach(() => { + adapter.parseInbound.mockResolvedValue(replyMessage); + mockRunService.get.mockResolvedValue({ + runId: 'run_existing', + tenantId: 'tenant1', + status: 'awaiting_approval', + }); + }); + + it('ignores an approve from a user who is not on the configured allowlist', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['someone-else'] }); + + const res = await handleInbound('slack', req(), deps); + + expect(res.status).toBe(200); + expect(enqueue).not.toHaveBeenCalled(); + }); + + it('queues an approve from a user who is on the configured allowlist', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['U1', 'U2'] }); + + const res = await handleInbound('slack', req(), deps); + + expect(res.status).toBe(200); + expect(enqueue).toHaveBeenCalledWith( + expect.objectContaining({ runId: 'run_existing', action: 'approve', approvedBy: 'U1' }), + ); + }); + + it('queues a plain approve with no allowlist configured — unchanged back-compat behavior', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue(null); + + const res = await handleInbound('slack', req(), deps); + + expect(res.status).toBe(200); + expect(enqueue).toHaveBeenCalledWith( + expect.objectContaining({ runId: 'run_existing', action: 'approve', approvedBy: 'U1' }), + ); + }); + + it('ignores approve_always with no allowlist configured — a standing grant needs one', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue(null); + adapter.parseInbound.mockResolvedValue({ + ...replyMessage, + replyContext: { ...replyMessage.replyContext, action: 'approve_always' as const }, + }); + + const res = await handleInbound('slack', req(), deps); + + expect(res.status).toBe(200); + expect(enqueue).not.toHaveBeenCalled(); + }); + + it('queues approve_always once an allowlist is configured and the user is on it', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['U1'] }); + adapter.parseInbound.mockResolvedValue({ + ...replyMessage, + replyContext: { ...replyMessage.replyContext, action: 'approve_always' as const }, + }); + + const res = await handleInbound('slack', req(), deps); + + expect(res.status).toBe(200); + expect(enqueue).toHaveBeenCalledWith( + expect.objectContaining({ action: 'approve_always', approvedBy: 'U1' }), + ); + }); + + it('ignores a reject from a user who is not on the configured allowlist', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['someone-else'] }); + adapter.parseInbound.mockResolvedValue({ + ...replyMessage, + replyContext: { ...replyMessage.replyContext, action: 'reject' as const }, + }); + + const res = await handleInbound('slack', req(), deps); + + expect(res.status).toBe(200); + expect(enqueue).not.toHaveBeenCalled(); + }); +}); diff --git a/libs/claw-studio/src/gateway/gateway-service.ts b/libs/claw-studio/src/gateway/gateway-service.ts index 04e6a068..b96d88e6 100644 --- a/libs/claw-studio/src/gateway/gateway-service.ts +++ b/libs/claw-studio/src/gateway/gateway-service.ts @@ -12,6 +12,7 @@ import { createLogger } from '@chatbot/shared'; import { getConnectorRegistry } from '../connectors/registry'; +import { ClawConnectorConfigService } from '../connectors/config-service'; import { getRunService } from './run-service'; import { isTerminalStatus, type ChannelAdapter, type GatewayMessage, type ReplyAction } from './types'; import type { ChannelType } from '../connectors/types'; @@ -40,6 +41,13 @@ export interface GatewayJobPayload { /** `cancel` is dashboard-only; channels only ever send the three ReplyActions. */ action?: ReplyAction | 'cancel'; content?: string; + /** + * The platform user id that made this decision, when it came from a + * channel. Threaded through to the run's `approval_decision` event so a + * standing grant (`approve_always`) is attributable to a person, not just + * to "someone in the workspace" (known-issues #11/#13). + */ + approvedBy?: string; } export type EnqueueRunFn = (payload: GatewayJobPayload) => Promise; @@ -196,11 +204,46 @@ async function handleResume( return json({ error: 'Reply was empty.' }, 400); } + // Approve/reject/approve_always come from a channel button or thread reply — + // the signature check above proves the request came from the right + // *workspace*, not that the clicker is anyone in particular. Without this, + // any member who can see the approval message can press it. + // (docs/superpowers/claw-studio-known-issues.md #11) + if (reply.action === 'approve' || reply.action === 'approve_always' || reply.action === 'reject') { + const connectorConfig = await new ClawConnectorConfigService(run.tenantId).getRaw( + message.channelType, + ); + const approverIds = connectorConfig?.approverIds ?? []; + const isAllowlisted = + approverIds.length > 0 && !!message.userId && approverIds.includes(message.userId); + + if (approverIds.length > 0 && !isAllowlisted) { + logger.warn( + { runId: run.runId, channel: adapter.channelType, userId: message.userId }, + 'Resume action from a user not on the channel approver allowlist — ignoring', + ); + return await adapter.sendAck(req, run.runId); + } + + // approve_always grants a STANDING permission on the task (not a one-time + // action), so trusting "anyone in the workspace" by default is too + // consequential — require an explicit allowlist before it can be used at + // all, same as the known-issues register's own stopgap suggestion. + if (approverIds.length === 0 && reply.action === 'approve_always') { + logger.warn( + { runId: run.runId, channel: adapter.channelType }, + 'approve_always requested with no approver allowlist configured for this channel — ignoring', + ); + return await adapter.sendAck(req, run.runId); + } + } + await deps.enqueue({ runId: run.runId, tenantId: run.tenantId, action: reply.action, content: reply.content, + approvedBy: message.userId, }); logger.info(