diff --git a/src/modules/approvals/onecli-approvals.test.ts b/src/modules/approvals/onecli-approvals.test.ts new file mode 100644 index 00000000000..fbffb327f78 --- /dev/null +++ b/src/modules/approvals/onecli-approvals.test.ts @@ -0,0 +1,41 @@ +/** + * Tests for shortApprovalId — the OneCLI manual-approval card id used as + * the lookup key in the Chat SDK callback. Two properties matter: + * + * 1. Unguessable. The id IS the auth — anyone who knows a pending id and + * can post a click to the webhook can approve a credentialed action. + * Math.random() with 8 base36 chars is ~41 bits and brute-forceable. + * crypto.randomBytes(16) gives 128 bits, well past brute-force range. + * + * 2. Fits in Telegram's callback_data budget. Chat SDK wraps the id as + * `chat:{"a":"","v":""}` and Telegram caps callback_data at + * 64 bytes total. A full UUID (36 chars) plus our `oa-` prefix would + * not fit; the base64url encoding picked here does. + */ +import { describe, expect, it } from 'vitest'; + +import { shortApprovalId } from './onecli-approvals.js'; + +describe('shortApprovalId', () => { + it('starts with the oa- prefix and uses base64url-safe characters only', () => { + const id = shortApprovalId(); + expect(id.startsWith('oa-')).toBe(true); + // base64url: A–Z, a–z, 0–9, '-', '_'. + expect(/^oa-[A-Za-z0-9_-]+$/.test(id)).toBe(true); + }); + + it('fits inside Chat SDK callback_data wrapping for the longer button value', () => { + // chat:{"a":"","v":""} — see @chat-adapter/telegram callback encoder. + const wrap = (id: string, value: string): string => `chat:${JSON.stringify({ a: id, v: value })}`; + const id = shortApprovalId(); + // Telegram hard limit is 64 bytes; allow the longer of the two real values. + expect(Buffer.byteLength(wrap(id, 'approve'), 'utf8')).toBeLessThanOrEqual(64); + expect(Buffer.byteLength(wrap(id, 'reject'), 'utf8')).toBeLessThanOrEqual(64); + }); + + it('produces no collisions over 50,000 samples (sanity for entropy)', () => { + const seen = new Set(); + for (let i = 0; i < 50_000; i++) seen.add(shortApprovalId()); + expect(seen.size).toBe(50_000); + }); +}); diff --git a/src/modules/approvals/onecli-approvals.ts b/src/modules/approvals/onecli-approvals.ts index eec05c0b414..7756d8e82c6 100644 --- a/src/modules/approvals/onecli-approvals.ts +++ b/src/modules/approvals/onecli-approvals.ts @@ -17,6 +17,8 @@ * Startup sweep edits any leftover cards from a previous process to * "Expired (host restarted)" and drops the rows. */ +import { randomBytes } from 'node:crypto'; + import { OneCLI, type ApprovalRequest, type ManualApprovalHandle } from '@onecli-sh/sdk'; import { pickApprovalDelivery, pickApprover } from './primitive.js'; @@ -50,18 +52,25 @@ let adapterRef: ChannelDeliveryAdapter | null = null; /** * Generate a short approval id for card buttons. * - * OneCLI's native request.id is a UUID (36 bytes). When we put it into a card - * button's action id as `ncq::Approve`, Chat SDK's Telegram adapter then - * serializes both `id` and `value` into the Telegram `callback_data` field, - * which has a hard 64-byte limit. UUIDs push past that limit. + * Constraints: + * - This id IS the secret. The Chat SDK callback's `actionId` is what + * the click resolves against (`resolveOneCLIApproval`). Anyone who can + * guess a pending id can approve a credentialed action they shouldn't + * be able to. Use a CSPRNG, not Math.random. + * - Telegram's callback_data is hard-capped at 64 bytes. The Chat SDK + * wraps the id as `chat:{"a":"","v":""}`, so the budget is + * ~64 - 25 (wrapper) - 7 (max value `approve`) = 32 bytes for the id. + * A full UUID-with-dashes (36 chars + `oa-` prefix = 39) would not fit. + * + * Resolution: 16 random bytes encoded as base64url = 22 chars + `oa-` prefix + * = 25-char id. 128 bits of entropy, well inside the callback_data budget. * - * Instead we generate a 10-byte id (`oa-` + 8 base36 chars) for the card, and - * keep the OneCLI request.id in the persisted payload for audit. The pending - * map, DB row, and button callback all use this short id; click handling - * looks up the short id and resolves the Promise that was waiting on it. + * The OneCLI native request.id (a UUID) is still kept in the persisted + * payload for audit; the short id is only the lookup key for the in-memory + * pending map and the Telegram button. */ -function shortApprovalId(): string { - return `oa-${Math.random().toString(36).slice(2, 10)}`; +export function shortApprovalId(): string { + return `oa-${randomBytes(16).toString('base64url')}`; } /** Called from the approvals response handler when a card button is clicked. */ diff --git a/src/modules/approvals/response-handler.test.ts b/src/modules/approvals/response-handler.test.ts new file mode 100644 index 00000000000..cc4c8d8a656 --- /dev/null +++ b/src/modules/approvals/response-handler.test.ts @@ -0,0 +1,193 @@ +/** + * Tests for the authorization gate in handleApprovalsResponse. + * + * The webhook receiver can't fully authenticate clicks (it only verifies + * platform signatures), so the response handler must re-check that the + * clicker is actually an eligible approver for the agent group before + * dispatching the registered approval handler. Without this check, anyone + * who can post a forged response to the webhook can attribute their + * "approve" to any user id they choose. + */ +import fs from 'fs'; + +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; + +import { closeDb, createAgentGroup, initTestDb, runMigrations } from '../../db/index.js'; +import { createPendingApproval, createSession } from '../../db/sessions.js'; +import { createUser } from '../permissions/db/users.js'; +import { grantRole } from '../permissions/db/user-roles.js'; + +vi.mock('../../container-runner.js', () => ({ + wakeContainer: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../../session-manager.js', () => ({ + writeSessionMessage: vi.fn(), + heartbeatPath: () => '/tmp/no-such-heartbeat', +})); + +const TEST_DIR = '/tmp/nanoclaw-test-response-handler'; +vi.mock('../../config.js', async () => { + const actual = await vi.importActual('../../config.js'); + return { ...actual, DATA_DIR: TEST_DIR }; +}); + +function now(): string { + return new Date().toISOString(); +} + +const APPROVAL_OPTIONS = JSON.stringify([ + { label: 'Approve', value: 'approve' }, + { label: 'Reject', value: 'reject' }, +]); + +beforeEach(async () => { + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true }); + fs.mkdirSync(TEST_DIR, { recursive: true }); + const db = initTestDb(); + runMigrations(db); + + // Fixtures + createAgentGroup({ + id: 'ag-1', + name: 'TestAgent', + folder: 'test-agent', + agent_provider: null, + created_at: now(), + }); + createUser({ id: 'telegram:1111', kind: 'telegram', display_name: 'Owner', created_at: now() }); + createUser({ id: 'telegram:2222', kind: 'telegram', display_name: 'Stranger', created_at: now() }); + grantRole({ + user_id: 'telegram:1111', + role: 'owner', + agent_group_id: null, + granted_by: null, + granted_at: now(), + }); + createSession({ + id: 'sess-1', + agent_group_id: 'ag-1', + messaging_group_id: null, + thread_id: null, + last_active: now(), + agent_provider: null, + status: 'active', + container_status: 'idle', + created_at: now(), + }); + createPendingApproval({ + approval_id: 'appr-1', + session_id: 'sess-1', + request_id: 'appr-1', + action: 'test_action', + payload: JSON.stringify({ note: 'do the thing' }), + created_at: now(), + title: 'Test', + options_json: APPROVAL_OPTIONS, + }); +}); + +afterEach(() => { + closeDb(); +}); + +describe('handleApprovalsResponse — clicker authorization', () => { + it('authorized owner click invokes the registered handler and deletes the row', async () => { + const { registerApprovalHandler } = await import('./primitive.js'); + const { handleApprovalsResponse } = await import('./response-handler.js'); + const { getPendingApproval } = await import('../../db/sessions.js'); + + let handlerCalled = false; + let receivedUserId: string | undefined; + registerApprovalHandler('test_action', async ({ userId }) => { + handlerCalled = true; + receivedUserId = userId; + }); + + const claimed = await handleApprovalsResponse({ + questionId: 'appr-1', + value: 'approve', + userId: '1111', + channelType: 'telegram', + platformId: 'telegram:1111', + threadId: null, + }); + + expect(claimed).toBe(true); + expect(handlerCalled).toBe(true); + // Handler receives the namespaced clicker id (channelType:rawUserId), not the raw platform id. + expect(receivedUserId).toBe('telegram:1111'); + expect(getPendingApproval('appr-1')).toBeUndefined(); + }); + + it('unauthorized clicker is rejected: handler is NOT invoked and the row stays intact', async () => { + const { registerApprovalHandler } = await import('./primitive.js'); + const { handleApprovalsResponse } = await import('./response-handler.js'); + const { getPendingApproval } = await import('../../db/sessions.js'); + + let handlerCalled = false; + registerApprovalHandler('test_action', async () => { + handlerCalled = true; + }); + + const claimed = await handleApprovalsResponse({ + questionId: 'appr-1', + value: 'approve', + userId: '2222', // Stranger — no role granted + channelType: 'telegram', + platformId: 'telegram:2222', + threadId: null, + }); + + // Claimed so the dispatcher doesn't keep looping… + expect(claimed).toBe(true); + // …but the handler must not run. + expect(handlerCalled).toBe(false); + // Row is preserved so a real admin can still click later. + expect(getPendingApproval('appr-1')).toBeDefined(); + }); + + it('spoofed userId on a different channelType cannot impersonate the owner', async () => { + const { registerApprovalHandler } = await import('./primitive.js'); + const { handleApprovalsResponse } = await import('./response-handler.js'); + + let handlerCalled = false; + registerApprovalHandler('test_action', async () => { + handlerCalled = true; + }); + + // Same raw userId as the owner, but channelType is 'discord' — so + // the namespaced id is "discord:1111", which is NOT in user_roles. + await handleApprovalsResponse({ + questionId: 'appr-1', + value: 'approve', + userId: '1111', + channelType: 'discord', + platformId: 'discord:1111', + threadId: null, + }); + + expect(handlerCalled).toBe(false); + }); + + it('missing userId is rejected', async () => { + const { registerApprovalHandler } = await import('./primitive.js'); + const { handleApprovalsResponse } = await import('./response-handler.js'); + + let handlerCalled = false; + registerApprovalHandler('test_action', async () => { + handlerCalled = true; + }); + + await handleApprovalsResponse({ + questionId: 'appr-1', + value: 'approve', + userId: null, + channelType: 'telegram', + platformId: 'telegram:1111', + threadId: null, + }); + + expect(handlerCalled).toBe(false); + }); +}); diff --git a/src/modules/approvals/response-handler.ts b/src/modules/approvals/response-handler.ts index 2bbdc9d32ff..ffc4c6d4b06 100644 --- a/src/modules/approvals/response-handler.ts +++ b/src/modules/approvals/response-handler.ts @@ -17,9 +17,9 @@ import { deletePendingApproval, getPendingApproval, getSession } from '../../db/ import type { ResponsePayload } from '../../response-registry.js'; import { log } from '../../log.js'; import { writeSessionMessage } from '../../session-manager.js'; -import type { PendingApproval } from '../../types.js'; +import type { PendingApproval, Session } from '../../types.js'; import { ONECLI_ACTION, resolveOneCLIApproval } from './onecli-approvals.js'; -import { getApprovalHandler } from './primitive.js'; +import { getApprovalHandler, pickApprover } from './primitive.js'; export async function handleApprovalsResponse(payload: ResponsePayload): Promise { // OneCLI credential approvals — resolved via in-memory Promise first. @@ -38,14 +38,31 @@ export async function handleApprovalsResponse(payload: ResponsePayload): Promise return true; } - await handleRegisteredApproval(approval, payload.value, payload.userId ?? ''); + // payload.userId is the raw platform user id (e.g. "8550182903"); namespace + // it with the channel type so it matches users(id) format and can be checked + // against pickApprover. Mirror handleSenderApprovalResponse in permissions/. + const clickerId = payload.userId ? `${payload.channelType}:${payload.userId}` : null; + await handleRegisteredApproval(approval, payload.value, clickerId); return true; } +/** + * Verify the clicker is in the eligible-approvers list for this approval's + * agent group. Without this check, any forged click — including one that + * spoofs another user's id via `payload.userId` — would dispatch the + * approval handler. The webhook receiver itself does not authenticate + * clicks beyond platform-signature checks, so we re-verify here so an + * approved cards can't be redeemed by a non-admin. + */ +function isAuthorizedClicker(clickerId: string | null, session: Session): boolean { + if (!clickerId) return false; + return pickApprover(session.agent_group_id).includes(clickerId); +} + async function handleRegisteredApproval( approval: PendingApproval, selectedOption: string, - userId: string, + clickerId: string | null, ): Promise { if (!approval.session_id) { deletePendingApproval(approval.approval_id); @@ -57,6 +74,20 @@ async function handleRegisteredApproval( return; } + if (!isAuthorizedClicker(clickerId, session)) { + // Claim the response so the dispatcher doesn't keep retrying, but do + // nothing else. Leave the pending_approvals row in place — a real + // approver can still click and resolve it. + log.warn('Approval click rejected — clicker is not an eligible approver', { + approvalId: approval.approval_id, + action: approval.action, + clickerId, + eligible: pickApprover(session.agent_group_id), + }); + return; + } + const userId = clickerId as string; // narrowed by isAuthorizedClicker + const notify = (text: string): void => { writeSessionMessage(session.agent_group_id, session.id, { id: `appr-note-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`,