From 3359b40273967d48da395e5ff93f0e1efa570204 Mon Sep 17 00:00:00 2001 From: Fabio Carvano Date: Sat, 17 Oct 2026 17:52:16 -0300 Subject: [PATCH] fix: channel approval buttons are not authorized to a specific user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gateway's resume handler (Approve/Reject/"Always allow for this task" on Slack/Telegram/Discord) validated the channel signature, that the run exists, and that it belongs to the right tenant, but never checked the clicking user's platform id against anything. Any member of the connected workspace who could see the approval message could press it — and approve_always grants a STANDING permission on the task, not a one-time action, so this let a single click by any workspace member leave a scheduled task able to send mail or create issues unattended indefinitely. Adds an optional, per-channel approverIds allowlist (connectors/types.ts -- not a secret, just platform user ids) and enforces it in the gateway's handleResume before enqueueing approve/approve_always/reject: - If an allowlist is configured, the clicking user must be on it. - approve_always specifically default-denies with NO allowlist configured at all, since granting a standing permission on trust alone is too consequential -- this matches the stopgap already recommended in docs/superpowers/claw-studio-known-issues.md #11. - Plain approve/reject with no allowlist configured keep their existing (pre-this-change) behavior, so a tenant that hasn't set one up yet is not suddenly locked out of ordinary approvals. Also threads the approving user's id through to the run's approval_decision event content, so a decision is attributable to a person rather than just "someone in the workspace" -- partially addresses the attribution gap noted in known-issues #13 as well. Covered by new gateway-service.test.ts cases (allowlisted/non- allowlisted approve, approve_always with and without an allowlist, non-allowlisted reject) plus updated assertions on the existing resume tests for the new approvedBy field. Verified: bunx vitest run on gateway-service.test.ts (23/23) and execute-run.test.ts (50/50), bunx tsc --noEmit -p tsconfig.lib.json on libs/claw-studio, bunx tsc --noEmit on apps/workers, and the broader apps/workers vitest suite (68/68, two unrelated pre-existing env-var failures in other job handlers, nothing touching this change). --- .../src/jobs/claw-gateway-run/handler.ts | 11 +- .../src/jobs/claw-gateway-run/schema.ts | 2 + libs/claw-studio/src/connectors/types.ts | 12 ++ libs/claw-studio/src/gateway/execute-run.ts | 15 ++- .../src/gateway/gateway-service.test.ts | 104 +++++++++++++++++- .../src/gateway/gateway-service.ts | 43 ++++++++ 6 files changed, 180 insertions(+), 7 deletions(-) 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(