From b0cd2390630869e6570fb5f00490f7b9e7d922cf Mon Sep 17 00:00:00 2001 From: Fabio Carvano Date: Mon, 19 Oct 2026 23:40:53 -0300 Subject: [PATCH] fix(claw): address channel approval review follow-ups --- CHANGELOG.md | 4 ++ .../app/api/connectors/[channel]/route.ts | 5 +- .../components/connectors/channel-fields.ts | 27 +++++++- .../connectors/connector-settings-form.tsx | 6 +- apps/mission-control/hooks/use-connectors.ts | 4 +- .../src/jobs/claw-gateway-run/handler.ts | 7 ++- .../src/jobs/claw-gateway-run/schema.ts | 2 + docs/superpowers/claw-studio-known-issues.md | 51 ++++++++-------- .../src/connectors/adapters/discord.test.ts | 14 +++++ .../src/connectors/adapters/discord.ts | 10 +++ .../adapters/gateway-adapters.test.ts | 33 ++++++++++ .../src/connectors/adapters/slack.ts | 14 +++++ .../src/connectors/adapters/telegram.ts | 17 ++++++ .../src/connectors/config-service.test.ts | 17 ++++++ .../src/connectors/config-service.ts | 16 ++++- .../src/gateway/execute-run.test.ts | 22 +++++++ libs/claw-studio/src/gateway/execute-run.ts | 14 ++--- .../src/gateway/gateway-service.test.ts | 61 ++++++++++++++++--- .../src/gateway/gateway-service.ts | 43 +++++++++++-- libs/claw-studio/src/gateway/types.ts | 2 + 20 files changed, 310 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a7fd4d3..5743b476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# Channel approval follow-up + +- Channel approvals now provide private denial feedback and audit events; decision identities are stored as structured metadata; Mission Control can configure per-channel approver IDs. One-time decisions remain compatible without an allowlist, while standing grants require one. + # Changelog All notable changes to this project will be documented in this file. diff --git a/apps/mission-control/app/api/connectors/[channel]/route.ts b/apps/mission-control/app/api/connectors/[channel]/route.ts index 6a601d76..ab195dec 100644 --- a/apps/mission-control/app/api/connectors/[channel]/route.ts +++ b/apps/mission-control/app/api/connectors/[channel]/route.ts @@ -85,6 +85,7 @@ const saveSchemas: Record = { slack: z .object({ enabled: z.boolean().optional(), + approverIds: z.array(z.string().trim().min(1).max(128)).max(50).optional(), signingSecret: z.string().optional(), botToken: z.string().optional(), teamId: z.string().optional(), @@ -93,6 +94,7 @@ const saveSchemas: Record = { telegram: z .object({ enabled: z.boolean().optional(), + approverIds: z.array(z.string().trim().min(1).max(128)).max(50).optional(), botToken: z.string().optional(), secretToken: z.string().optional(), }) @@ -100,6 +102,7 @@ const saveSchemas: Record = { discord: z .object({ enabled: z.boolean().optional(), + approverIds: z.array(z.string().trim().min(1).max(128)).max(50).optional(), applicationId: z.string().optional(), publicKey: z.string().optional(), botToken: z.string().optional(), @@ -171,7 +174,7 @@ export async function PUT(request: NextRequest, { params }: { params: Promise<{ } const configs = new ClawConnectorConfigService(ctx.tenantId); - await configs.save(ctx.channel, parsed.data as Record, ctx.actor); + await configs.save(ctx.channel, parsed.data as Record, ctx.actor); // Routing is established after the save so the link always reflects what is // actually stored. A failure here is reported, never fatal — the credentials diff --git a/apps/mission-control/components/connectors/channel-fields.ts b/apps/mission-control/components/connectors/channel-fields.ts index 7330734c..1a0cc8f5 100644 --- a/apps/mission-control/components/connectors/channel-fields.ts +++ b/apps/mission-control/components/connectors/channel-fields.ts @@ -49,9 +49,16 @@ export const CHANNEL_FIELDS: Record = { hint: 'Only needed when no bot token is set — otherwise it is read from Slack automatically.', showWhileEmpty: 'botToken', }, + { + name: 'approverIds', + label: 'Authorized Approver IDs', + secret: false, + placeholder: 'U123, U456', + hint: 'Platform user IDs separated by commas. With no IDs, one-time approve/reject remains available to workspace members, while Always allow is blocked.', + }, ], testOverrideKeys: ['botToken'], - clears: 'signing secret and bot token', + clears: 'signing secret, bot token, and authorized approvers', setupHint: 'Test Connection calls Slack’s auth.test with your bot token.', }, telegram: { @@ -73,9 +80,16 @@ export const CHANNEL_FIELDS: Record = { placeholder: 'Generate or paste a high-entropy value', hint: 'Sent by Telegram on every inbound request so we can verify it really came from Telegram.', }, + { + name: 'approverIds', + label: 'Authorized Approver IDs', + secret: false, + placeholder: 'U123, U456', + hint: 'Platform user IDs separated by commas. With no IDs, one-time approve/reject remains available to workspace members, while Always allow is blocked.', + }, ], testOverrideKeys: ['botToken'], - clears: 'bot token and secret token', + clears: 'bot token, secret token, and authorized approvers', setupHint: 'Test Connection calls Telegram’s getMe with your bot token.', }, discord: { @@ -104,9 +118,16 @@ export const CHANNEL_FIELDS: Record = { placeholder: 'Discord Developer Portal → Bot → Reset Token', hint: 'Required to post replies — Discord has no long-lived reply mechanism besides bot-authenticated messages.', }, + { + name: 'approverIds', + label: 'Authorized Approver IDs', + secret: false, + placeholder: 'U123, U456', + hint: 'Platform user IDs separated by commas. With no IDs, one-time approve/reject remains available to workspace members, while Always allow is blocked.', + }, ], testOverrideKeys: ['botToken', 'applicationId', 'publicKey'], - clears: 'application id, public key, and bot token', + clears: 'application id, public key, bot token, and authorized approvers', setupHint: 'Test Connection calls Discord’s /users/@me with your bot token. Register a /claw slash command and set the Interactions Endpoint URL to the webhook URL below.', }, }; diff --git a/apps/mission-control/components/connectors/connector-settings-form.tsx b/apps/mission-control/components/connectors/connector-settings-form.tsx index e67e5617..114ce937 100644 --- a/apps/mission-control/components/connectors/connector-settings-form.tsx +++ b/apps/mission-control/components/connectors/connector-settings-form.tsx @@ -66,7 +66,11 @@ export function ConnectorSettingsForm({ channel }: { channel: ChannelId }) { const handleSave = async () => { setError(null); try { - const result = await save.mutateAsync({ ...values, enabled: effectiveEnabled }); + // The API stores an actual string[]; empty input explicitly clears the list. + const approverIds = values.approverIds === undefined + ? {} + : { approverIds: Array.from(new Set(values.approverIds.split(/[,;\n]+/).map((id) => id.trim()).filter(Boolean))) }; + const result = await save.mutateAsync({ ...values, ...approverIds, enabled: effectiveEnabled }); // Clear the inputs so the form goes back to showing masks — leaving the // typed plaintext on screen after a successful save is needless exposure. setValues({}); diff --git a/apps/mission-control/hooks/use-connectors.ts b/apps/mission-control/hooks/use-connectors.ts index a5761ee2..4b040af7 100644 --- a/apps/mission-control/hooks/use-connectors.ts +++ b/apps/mission-control/hooks/use-connectors.ts @@ -66,7 +66,7 @@ async function fetchConnector(channel: ChannelId): Promise { async function saveConnector( channel: ChannelId, - input: Record, + input: Record, ): Promise { const res = await fetch(`${BASE_PATH}/api/connectors/${channel}`, { method: 'PUT', @@ -116,7 +116,7 @@ export function useConnector(channel: ChannelId) { export function useSaveConnector(channel: ChannelId) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: (input: Record) => saveConnector(channel, input), + mutationFn: (input: Record) => saveConnector(channel, input), onSuccess: () => { queryClient.invalidateQueries({ queryKey: connectorKeys.detail(channel) }); queryClient.invalidateQueries({ queryKey: connectorKeys.lists() }); diff --git a/apps/workers/src/jobs/claw-gateway-run/handler.ts b/apps/workers/src/jobs/claw-gateway-run/handler.ts index a5165013..ee1f6167 100644 --- a/apps/workers/src/jobs/claw-gateway-run/handler.ts +++ b/apps/workers/src/jobs/claw-gateway-run/handler.ts @@ -18,7 +18,8 @@ import { clawGatewayRunSchema } from './schema.js'; const log = createLogger('claw-gateway-run'); export async function handleClawGatewayRun(data: unknown): Promise { - const { runId, tenantId, action, content, approvedBy } = clawGatewayRunSchema.parse(data); + const { runId, tenantId, action, content, actorId: newActorId, approvedBy } = clawGatewayRunSchema.parse(data); + const actorId = newActorId ?? approvedBy; // compatibility for pre-existing queued jobs try { const { executeRun, terminateRun, grantPendingToolsForRun } = await import('@chatbot/claw-studio'); @@ -39,7 +40,7 @@ export async function handleClawGatewayRun(data: unknown): Promise { runId, deps, kind: action === 'reject' ? 'reject' : 'cancel', - approvedBy: action === 'reject' ? approvedBy : undefined, + actorId: action === 'reject' ? actorId : undefined, }); return; } @@ -48,7 +49,7 @@ export async function handleClawGatewayRun(data: unknown): Promise { await executeRun({ runId, deps, - resume: action ? { action, content, approvedBy } : undefined, + resume: action ? { action, content, actorId } : 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 ec854798..1522e5b5 100644 --- a/apps/workers/src/jobs/claw-gateway-run/schema.ts +++ b/apps/workers/src/jobs/claw-gateway-run/schema.ts @@ -19,6 +19,8 @@ export const clawGatewayRunSchema = z.object({ /** 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. */ + actorId: z.string().optional(), + /** Accept already-queued jobs created before the actorId rename. */ approvedBy: z.string().optional(), }); diff --git a/docs/superpowers/claw-studio-known-issues.md b/docs/superpowers/claw-studio-known-issues.md index ebddfa44..43c43c8f 100644 --- a/docs/superpowers/claw-studio-known-issues.md +++ b/docs/superpowers/claw-studio-known-issues.md @@ -78,33 +78,30 @@ Fixed by `tokenizeName()` (maps `_`/`-` to spaces before matching) plus the two --- -## 🔴 11. Channel approval buttons are not authorized to a user - -**Where:** `libs/claw-studio/src/gateway/gateway-service.ts` — the resume handler; and each -adapter's `parseInbound` for interactive payloads. - -**Problem:** the resume path validates the channel signature, that the run exists, that it belongs to -the tenant the signature resolved to, and that the run's status matches the action. It captures the -clicker's platform user id (`message.userId`) and **never checks it against anything**. Any member of -the connected Slack workspace who can see an approval message can press Approve, Reject, or -"Always allow for this task". - -**Verified:** traced the full resume path — no allowlist, no role check, no comparison of -`message.userId` against any stored value. `SlackConnectorConfig` has no allowlist field. -`ClawConnectorConfigService` stores none. - -**Blast radius:** pre-existing for Approve/Reject, which grant a **one-time** action. `approve_always` -(added 2026-07-31) grants a **standing permission** on the task, so a single click by any workspace -member can let a scheduled task send mail or create issues unattended indefinitely. The feature did -not create this gap but it materially raises its consequence. - -**Fix, roughly in order of value:** -1. Add an approver allowlist to each channel's connector config (platform user ids), checked in the - resume handler before enqueueing. A `whatsapp-contact-allowlist` design already exists in this - repo to follow. -2. Until then, consider gating `approve_always` to the Mission Control UI only (where the session is a - real authenticated studio login) and dropping the Slack button. -3. Record the approver on the run's `approval_decision` event so the action is at least attributable. +## 🟡 11. PARTIALLY FIXED (2026-10-19) — Channel approval identity + +**Current behavior:** PR #65 added approverIds to each connector and checks the +platform actor before queuing approval actions. approve_always refuses requests +without a configured allowlist. Ordinary approve/reject without a list remain +backward-compatible under the maintainer's explicitly approved compromise. + +**Follow-up from [PR #65 review](https://github.com/offloadmemory/agentfork/pull/65#pullrequestreview-5567988309):** +- Provide private, generic feedback for denied button presses. +- Audit denied actions with tenant, channel, actor ID, action, and reason. +- Rename the new-job identity field to actorId with compatibility for already-queued + payloads, and keep timeline content unchanged. +- Store the actor in structured approval_decision.metadata. +- Expose approverIds in the Mission Control connector configuration editor and + validate its values before saving. +- Test missing platform identities when an allowlist is configured. + +**Compatibility:** One-time approve/reject without an allowlist is still allowed +until the maintainer explicitly decides to change that policy. Enabling +approve_always requires an explicit nonempty approver allowlist. + +**Security scope:** Channel signatures authenticate workspaces, not individual +operators. Per-operator identities within Mission Control are tracked separately +in known issue #13. --- diff --git a/libs/claw-studio/src/connectors/adapters/discord.test.ts b/libs/claw-studio/src/connectors/adapters/discord.test.ts index 6b9a6970..5462776e 100644 --- a/libs/claw-studio/src/connectors/adapters/discord.test.ts +++ b/libs/claw-studio/src/connectors/adapters/discord.test.ts @@ -206,6 +206,20 @@ describe('DiscordConnector', () => { }); }); + describe('sendDenial', () => { + it('returns an ephemeral interaction response visible only to the actor', async () => { + const req = signedRequest({ + type: 3, application_id: APPLICATION_ID, data: { custom_id: 'x:y' }, + }); + const res = await new DiscordConnector().sendDenial(req, 'run-1'); + expect(res.status).toBe(200); + expect(await res.json()).toMatchObject({ + type: 4, + data: { flags: 64, content: expect.stringContaining('not authorized') }, + }); + }); + }); + describe('outbound', () => { const run: ClawRunRecord = { id: '1', diff --git a/libs/claw-studio/src/connectors/adapters/discord.ts b/libs/claw-studio/src/connectors/adapters/discord.ts index 6b6c32dc..be64f2ab 100644 --- a/libs/claw-studio/src/connectors/adapters/discord.ts +++ b/libs/claw-studio/src/connectors/adapters/discord.ts @@ -325,6 +325,16 @@ export class DiscordConnector implements ChannelAdapter { return json({ type: RESPONSE_DEFERRED_CHANNEL_MESSAGE }); } + /** Discord interaction response visible only to the clicking member. */ + async sendDenial(req: Request, runId: string): Promise { + const parsed = await this.parse(req).catch(() => null); + if (parsed?.kind !== 'component') return this.sendAck(req, runId); + return json({ + type: 4, // CHANNEL_MESSAGE_WITH_SOURCE + data: { content: 'You are not authorized to approve this request.', flags: 64 }, + }); + } + // ========================================================================== // Outbound // ========================================================================== diff --git a/libs/claw-studio/src/connectors/adapters/gateway-adapters.test.ts b/libs/claw-studio/src/connectors/adapters/gateway-adapters.test.ts index 6d246799..43eb9b97 100644 --- a/libs/claw-studio/src/connectors/adapters/gateway-adapters.test.ts +++ b/libs/claw-studio/src/connectors/adapters/gateway-adapters.test.ts @@ -233,6 +233,19 @@ describe('SlackConnector inbound', () => { expect(buttonAck.status).toBe(200); await expect(buttonAck.text()).resolves.toBe(''); }); + + it('returns a private ephemeral denial without replacing Slack approval buttons', async () => { + const buttonBody = new URLSearchParams({ + payload: JSON.stringify({ type: 'block_actions', team: { id: 'T123' }, actions: [] }), + }).toString(); + const res = await new SlackConnector().sendDenial(slackRequest(buttonBody), 'run_1'); + expect(res.status).toBe(200); + expect(await res.json()).toMatchObject({ + response_type: 'ephemeral', + replace_original: false, + }); + }); + }); const SECRET_TOKEN = 'a-high-entropy-secret-token'; @@ -327,6 +340,26 @@ describe('TelegramConnector inbound', () => { await expect(new TelegramConnector().parseInbound(req)).rejects.toThrow(/Not a user text message/); }); + it('answers a denied Telegram callback privately without posting to the chat', async () => { + const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue( + new Response(JSON.stringify({ ok: true, result: true }), { status: 200 }), + ); + try { + const req = telegramRequest({ + callback_query: { id: 'cb1', data: 'claw:approve:run_xyz', from: { id: 42 } }, + }); + const res = await new TelegramConnector().sendDenial(req, 'run_xyz'); + expect(res.status).toBe(200); + expect(fetchSpy).toHaveBeenCalledWith( + expect.stringContaining('/answerCallbackQuery'), + expect.objectContaining({ body: expect.stringContaining('not authorized') }), + ); + expect(fetchSpy).not.toHaveBeenCalledWith(expect.stringContaining('/sendMessage'), expect.anything()); + } finally { + fetchSpy.mockRestore(); + } + }); + it('keeps the approve callback payload inside Telegram\'s 64-byte limit', () => { const runId = `run_${'x'.repeat(22)}`; expect(Buffer.byteLength(`claw:approve:${runId}`)).toBeLessThanOrEqual(64); diff --git a/libs/claw-studio/src/connectors/adapters/slack.ts b/libs/claw-studio/src/connectors/adapters/slack.ts index d2b92904..18c5ef5b 100644 --- a/libs/claw-studio/src/connectors/adapters/slack.ts +++ b/libs/claw-studio/src/connectors/adapters/slack.ts @@ -397,6 +397,20 @@ export class SlackConnector implements ChannelAdapter { ); } + /** Ephemeral response to the clicking user; never modifies the shared approval message. */ + async sendDenial(req: Request, runId: string): Promise { + const parsed = await this.parse(req).catch(() => null); + if (parsed?.kind !== 'block_actions') return this.sendAck(req, runId); + return new Response( + JSON.stringify({ + response_type: 'ephemeral', + replace_original: false, + text: 'You are not authorized to approve this request.', + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ); + } + // ========================================================================== // Outbound // ========================================================================== diff --git a/libs/claw-studio/src/connectors/adapters/telegram.ts b/libs/claw-studio/src/connectors/adapters/telegram.ts index 5209c2bd..42553922 100644 --- a/libs/claw-studio/src/connectors/adapters/telegram.ts +++ b/libs/claw-studio/src/connectors/adapters/telegram.ts @@ -254,6 +254,23 @@ export class TelegramConnector implements ChannelAdapter { }; } + /** Answer the clicker's callback privately so denial does not post to the chat. */ + async sendDenial(req: Request, runId: string): Promise { + try { + const { tenantId, update } = await this.parse(req); + if (tenantId && update?.callback_query?.id) { + await this.call(tenantId, 'answerCallbackQuery', { + callback_query_id: update.callback_query.id, + text: 'You are not authorized to approve this request.', + show_alert: true, + }); + } + } catch (error) { + logger.warn({ error, runId }, 'Unable to acknowledge denied Telegram approval'); + } + return new Response('', { status: 200 }); + } + /** * Telegram only needs an empty 200 — but a run started from a message gets a * visible "on it" message whose id is remembered, so progress can be edited diff --git a/libs/claw-studio/src/connectors/config-service.test.ts b/libs/claw-studio/src/connectors/config-service.test.ts index c06e7641..ba28e8c9 100644 --- a/libs/claw-studio/src/connectors/config-service.test.ts +++ b/libs/claw-studio/src/connectors/config-service.test.ts @@ -71,6 +71,23 @@ describe('ClawConnectorConfigService', () => { }); }); + it('round-trips, lists and clears approver IDs without encrypting public IDs', async () => { + const svc = new ClawConnectorConfigService('tenant-1'); + await svc.save('telegram', { approverIds: ['42', '43', '42'] }); + expect(await svc.getRaw('telegram')).toMatchObject({ approverIds: ['42', '43'] }); + expect((await svc.getMasked('telegram')).fields.approverIds).toBe('42, 43'); + await svc.save('telegram', { enabled: false }); + expect(await svc.getRaw('telegram')).toMatchObject({ approverIds: ['42', '43'] }); + await svc.save('telegram', { approverIds: [] }); + expect(await svc.getRaw('telegram')).toMatchObject({ approverIds: [] }); + }); + + it('rejects malformed approver IDs before saving', async () => { + const svc = new ClawConnectorConfigService('tenant-1'); + await expect(svc.save('telegram', { approverIds: [''] })).rejects.toThrow('Invalid approverIds'); + await expect(svc.save('telegram', { approverIds: Array(51).fill('1') })).rejects.toThrow('Invalid approverIds'); + }); + it('returns null from getRaw when the channel was never configured', async () => { expect(await new ClawConnectorConfigService('tenant-1').getRaw('telegram')).toBeNull(); }); diff --git a/libs/claw-studio/src/connectors/config-service.ts b/libs/claw-studio/src/connectors/config-service.ts index ae1939e7..d231b6d3 100644 --- a/libs/claw-studio/src/connectors/config-service.ts +++ b/libs/claw-studio/src/connectors/config-service.ts @@ -114,6 +114,11 @@ export class ClawConnectorConfigService { for (const [key, value] of Object.entries(stored)) { if (key === 'enabled') continue; + // This allowlist contains public platform user IDs, not secret credentials. + if (key === 'approverIds' && Array.isArray(value)) { + fields[key] = value.join(', '); + continue; + } if (typeof value !== 'string' || !value) continue; if (secretKeys.includes(key)) { @@ -142,7 +147,7 @@ export class ClawConnectorConfigService { */ async save( channel: ChannelType, - input: Record, + input: Record, updatedBy = 'system', ): Promise { const stored = (await this.getStored(channel)) ?? {}; @@ -151,6 +156,15 @@ export class ClawConnectorConfigService { for (const [key, value] of Object.entries(input)) { if (key === 'enabled') continue; + if (key === 'approverIds') { + if (!Array.isArray(value) || + value.length > 50 || + !value.every((id) => typeof id === 'string' && id.trim().length > 0 && id.length <= 128)) { + throw new Error('Invalid approverIds: expected up to 50 non-empty user IDs'); + } + next[key] = Array.from(new Set(value.map((id) => id.trim()))); + continue; + } if (typeof value !== 'string') continue; const trimmed = value.trim(); diff --git a/libs/claw-studio/src/gateway/execute-run.test.ts b/libs/claw-studio/src/gateway/execute-run.test.ts index eeb8abe2..888f9bb9 100644 --- a/libs/claw-studio/src/gateway/execute-run.test.ts +++ b/libs/claw-studio/src/gateway/execute-run.test.ts @@ -329,6 +329,17 @@ describe('executeRun', () => { expect((input as InstanceType).resume).toEqual({ decisions: [{ type: 'approve' }] }); }); + it('keeps the approval content stable and attributes the decision in metadata', async () => { + runs.get.mockResolvedValue({ ...baseRun, status: 'awaiting_approval' }); + await executeRun({ runId: baseRun.runId, resume: { action: 'approve', actorId: 'U1' }, deps }); + expect(runs.appendEvent).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + eventType: 'approval_decision', content: 'approved', metadata: { actorId: 'U1' }, + }), + ); + }); + it('supplies one approve decision per pending tool when the interrupt bundled several', async () => { runs.get.mockResolvedValue({ ...baseRun, @@ -452,6 +463,17 @@ describe('terminateRun', () => { ); }); + it('keeps rejection content stable with a structured actor ID', async () => { + runs.get.mockResolvedValue({ ...baseRun, status: 'awaiting_approval' }); + await terminateRun({ runId: baseRun.runId, deps, kind: 'reject', actorId: 'U1' }); + expect(runs.appendEvent).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + eventType: 'approval_decision', content: 'rejected', metadata: { actorId: 'U1' }, + }), + ); + }); + it('uses cancellation wording for a cancel', async () => { runs.get.mockResolvedValue({ ...baseRun, status: 'awaiting_approval' }); diff --git a/libs/claw-studio/src/gateway/execute-run.ts b/libs/claw-studio/src/gateway/execute-run.ts index 8d58b326..2c868553 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; approvedBy?: string }; + resume?: { action: Exclude; content?: string; actorId?: string }; deps: RouterDeps; /** * Per-run runtime shaping. Optional and defaulted, so channel runs are unchanged; @@ -432,12 +432,10 @@ 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') + - approvedSuffix, + content: resume.action === 'approve_always' ? 'approved (always, for this task)' : 'approved', + metadata: resume.actorId ? { actorId: resume.actorId } : undefined, }); } else { graphInput = { messages: [new HumanMessage(resume.content ?? '')] }; @@ -574,7 +572,7 @@ export async function terminateRun(input: { deps: RouterDeps; kind: 'reject' | 'cancel'; /** Platform user id that rejected, when `kind` is `'reject'` and it came from a channel. */ - approvedBy?: string; + actorId?: string; }): Promise { const runs = getRunService(); const bus = getRunEventBus(); @@ -600,10 +598,10 @@ export async function terminateRun(input: { const unsubscribe = adapter ? attachToRun(run, adapter, input.deps) : () => {}; try { if (rejected) { - const approvedSuffix = input.approvedBy ? ` by ${input.approvedBy}` : ''; await runs.appendEvent(run, { eventType: 'approval_decision', - content: `rejected${approvedSuffix}`, + content: 'rejected', + metadata: input.actorId ? { actorId: input.actorId } : undefined, }); } 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 dc4a0406..0ac6f587 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, mockConnectorConfigService, adapter } = vi.hoisted(() => { +const { mockRegistry, mockRunService, mockConnectorConfigService, mockAuditLog, adapter } = vi.hoisted(() => { const adapter = { channelType: 'slack', displayName: 'Slack', @@ -13,6 +13,7 @@ const { mockRegistry, mockRunService, mockConnectorConfigService, adapter } = vi validateRequest: vi.fn(async () => true), parseInbound: vi.fn(), sendAck: vi.fn(async () => new Response('acked', { status: 200 })), + sendDenial: vi.fn(async () => new Response('denied', { status: 200 })), sendResult: vi.fn(), sendError: vi.fn(), sendClarification: vi.fn(), @@ -27,10 +28,12 @@ const { mockRegistry, mockRunService, mockConnectorConfigService, adapter } = vi }, mockRunService: { create: vi.fn(), get: vi.fn() }, mockConnectorConfigService: { getRaw: vi.fn() }, + mockAuditLog: vi.fn(async () => {}), }; }); vi.mock('@chatbot/shared', () => ({ + AuditService: { logUserAction: mockAuditLog }, createLogger: vi.fn(() => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() })), })); @@ -75,6 +78,7 @@ beforeEach(() => { mockRunService.create.mockResolvedValue(createdRun); // No allowlist configured by default. mockConnectorConfigService.getRaw.mockResolvedValue(null); + adapter.sendDenial.mockResolvedValue(new Response('denied', { status: 200 })); }); describe('handleInbound', () => { @@ -123,7 +127,7 @@ 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 + // A fresh run has no approver decision to attribute — actorId is // only threaded through on a HIL resume. expect(enqueue).toHaveBeenCalledWith({ runId: 'run_new', tenantId: 'tenant1' }); }); @@ -205,7 +209,7 @@ describe('handleInbound — HIL resume', () => { tenantId: 'tenant1', action: 'approve', content: undefined, - approvedBy: 'U1', + actorId: 'U1', }); // A resume must never create a second run. expect(mockRunService.create).not.toHaveBeenCalled(); @@ -282,7 +286,7 @@ describe('handleInbound — HIL resume', () => { tenantId: 'tenant1', action: 'clarification_response', content: 'the blue one', - approvedBy: 'U1', + actorId: 'U1', }); }); @@ -334,6 +338,49 @@ describe('handleInbound — HIL resume — approver allowlist', () => { expect(enqueue).not.toHaveBeenCalled(); }); + + it('reports a private denial and audits the rejected actor without enqueuing', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['other-user'] }); + const res = await handleInbound('slack', req(), deps); + expect(await res.text()).toBe('denied'); + expect(adapter.sendDenial).toHaveBeenCalledTimes(1); + expect(enqueue).not.toHaveBeenCalled(); + expect(mockAuditLog).toHaveBeenCalledWith(expect.objectContaining({ + eventType: 'claw.gateway.approval_denied', + user: 'U1', + status: 'warning', + tenantId: 'tenant1', + metadata: expect.objectContaining({ action: 'approve', reason: 'not_allowlisted' }), + })); + }); + + it('denies when an allowlist is configured but the platform actor ID is missing', async () => { + mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['U1'] }); + adapter.parseInbound.mockResolvedValue({ ...replyMessage, userId: '' }); + const res = await handleInbound('slack', req(), deps); + expect(res.status).toBe(200); + expect(enqueue).not.toHaveBeenCalled(); + expect(adapter.sendDenial).toHaveBeenCalledTimes(1); + expect(mockAuditLog).toHaveBeenCalledWith(expect.objectContaining({ + user: 'unknown', + metadata: expect.objectContaining({ reason: 'not_allowlisted' }), + })); + }); + + it('audits and privately denies approve_always without an allowlist', async () => { + 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(); + expect(adapter.sendDenial).toHaveBeenCalledTimes(1); + expect(mockAuditLog).toHaveBeenCalledWith(expect.objectContaining({ + metadata: expect.objectContaining({ reason: 'allowlist_required' }), + })); + }); + it('queues an approve from a user who is on the configured allowlist', async () => { mockConnectorConfigService.getRaw.mockResolvedValue({ approverIds: ['U1', 'U2'] }); @@ -341,7 +388,7 @@ describe('handleInbound — HIL resume — approver allowlist', () => { expect(res.status).toBe(200); expect(enqueue).toHaveBeenCalledWith( - expect.objectContaining({ runId: 'run_existing', action: 'approve', approvedBy: 'U1' }), + expect.objectContaining({ runId: 'run_existing', action: 'approve', actorId: 'U1' }), ); }); @@ -352,7 +399,7 @@ describe('handleInbound — HIL resume — approver allowlist', () => { expect(res.status).toBe(200); expect(enqueue).toHaveBeenCalledWith( - expect.objectContaining({ runId: 'run_existing', action: 'approve', approvedBy: 'U1' }), + expect.objectContaining({ runId: 'run_existing', action: 'approve', actorId: 'U1' }), ); }); @@ -380,7 +427,7 @@ describe('handleInbound — HIL resume — approver allowlist', () => { expect(res.status).toBe(200); expect(enqueue).toHaveBeenCalledWith( - expect.objectContaining({ action: 'approve_always', approvedBy: 'U1' }), + expect.objectContaining({ action: 'approve_always', actorId: 'U1' }), ); }); diff --git a/libs/claw-studio/src/gateway/gateway-service.ts b/libs/claw-studio/src/gateway/gateway-service.ts index b96d88e6..f275ba0c 100644 --- a/libs/claw-studio/src/gateway/gateway-service.ts +++ b/libs/claw-studio/src/gateway/gateway-service.ts @@ -10,11 +10,11 @@ * pg-boss — the caller (Mission Control) owns the queue client. */ -import { createLogger } from '@chatbot/shared'; +import { createLogger, AuditService } 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 { isTerminalStatus, type ChannelAdapter, type ClawRunRecord, type GatewayMessage, type ReplyAction } from './types'; import type { ChannelType } from '../connectors/types'; const logger = createLogger('claw-studio:gateway:inbound'); @@ -47,7 +47,7 @@ export interface GatewayJobPayload { * standing grant (`approve_always`) is attributable to a person, not just * to "someone in the workspace" (known-issues #11/#13). */ - approvedBy?: string; + actorId?: string; } export type EnqueueRunFn = (payload: GatewayJobPayload) => Promise; @@ -149,6 +149,37 @@ export async function handleInbound( } } +/** + * Records an authorization denial before returning generic, private feedback. + * The run and tenant were already resolved, so this does not reveal run existence. + */ +async function denyApproval( + adapter: ChannelAdapter, + req: Request, + run: ClawRunRecord, + message: GatewayMessage, + action: ReplyAction, + reason: 'not_allowlisted' | 'allowlist_required', +): Promise { + await AuditService.logUserAction({ + eventType: 'claw.gateway.approval_denied', + action: 'Denied channel approval', + resourceType: 'claw_run', + resourceId: run.runId, + resourceName: 'Claw run', + user: message.userId || 'unknown', + userType: 'user', + status: 'warning', + severity: 'high', + details: 'Approval action rejected by channel authorization policy', + metadata: { tenantId: run.tenantId, channel: message.channelType, action, reason }, + tenantId: run.tenantId, + }); + return adapter.sendDenial + ? adapter.sendDenial(req, run.runId) + : adapter.sendAck(req, run.runId); +} + /** * A HIL reply (button press, thread reply) re-enters the same inbound route and * is disambiguated by `replyContext`, so there is no separate endpoint to @@ -222,7 +253,7 @@ async function handleResume( { 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); + return await denyApproval(adapter, req, run, message, reply.action, 'not_allowlisted'); } // approve_always grants a STANDING permission on the task (not a one-time @@ -234,7 +265,7 @@ async function handleResume( { 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); + return await denyApproval(adapter, req, run, message, reply.action, 'allowlist_required'); } } @@ -243,7 +274,7 @@ async function handleResume( tenantId: run.tenantId, action: reply.action, content: reply.content, - approvedBy: message.userId, + actorId: message.userId, }); logger.info( diff --git a/libs/claw-studio/src/gateway/types.ts b/libs/claw-studio/src/gateway/types.ts index d1071d23..440dbfa8 100644 --- a/libs/claw-studio/src/gateway/types.ts +++ b/libs/claw-studio/src/gateway/types.ts @@ -223,6 +223,8 @@ export interface ChannelAdapter extends ChannelConnector { parseInbound(req: Request): Promise; /** Must be fast — Slack and Discord both time out around 3s. */ sendAck(req: Request, runId: string): Promise; + /** Private, generic feedback on a denied approval. Never reveals run details. */ + sendDenial?(req: Request, runId: string): Promise; sendResult(run: ClawRunRecord, events: ClawRunEventRecord[]): Promise; sendError(run: ClawRunRecord, error: string): Promise;