fix(security): use CSPRNG for approval card ids + clicker authorization

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":"<id>","v":"<value>"}` 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":"<id>","v":"<value>"} — 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<string>();
+    for (let i = 0; i < 50_000; i++) seen.add(shortApprovalId());
+    expect(seen.size).toBe(50_000);
+  });
+});

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:<uuid>: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":"<id>","v":"<value>"}`, 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. */

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);
+  });
+});

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<boolean> {
   // 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<void> {
   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)}`,