fix(permissions): authorize unknown-sender approval clicks

The approval click handler trusted row.approver_user_id as the actor
regardless of who actually clicked the card. A random user who received
the forwarded card could click Approve and get the stranger admitted
to the agent group — their click was simply not checked.

Separately, payload.userId arrives as the raw platform userId from
Chat SDK onAction (e.g. "6037840640"), not the namespaced form
("telegram:6037840640") that matches users(id). Without namespacing,
users-table lookups miss.

Namespace the clicker id with payload.channelType, then authorize: the
clicker must be either the designated approver OR have
owner / admin privilege over the agent group (hasAdminPrivilege covers
owner, global admin, scoped admin). Unauthorized clicks return true
(claim the response so the registry doesn't log it as unclaimed) but
take no action — the pending row stays in place so a legitimate
approver can still act on it.

Existing tests passed a pre-namespaced userId directly, masking the
first bug. Fixed the fixtures to match production plumbing and added
two tests: one asserts a random bystander's click is rejected (row
stays pending, no member added), the other asserts a global admin can
approve even when they weren't the designated approver.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gavrielc
2026-04-20 12:16:35 +03:00
parent f74df3b0d3
commit 68058cbc4a
2 changed files with 106 additions and 13 deletions

View File

@@ -29,10 +29,8 @@ import { log } from '../../log.js';
import type { MessagingGroup, MessagingGroupAgent } from '../../types.js';
import { canAccessAgentGroup } from './access.js';
import { addMember } from './db/agent-group-members.js';
import {
deletePendingSenderApproval,
getPendingSenderApproval,
} from './db/pending-sender-approvals.js';
import { deletePendingSenderApproval, getPendingSenderApproval } from './db/pending-sender-approvals.js';
import { hasAdminPrivilege } from './db/user-roles.js';
import { getUser, upsertUser } from './db/users.js';
import { requestSenderApproval } from './sender-approval.js';
@@ -198,7 +196,23 @@ async function handleSenderApprovalResponse(payload: ResponsePayload): Promise<b
const row = getPendingSenderApproval(payload.questionId);
if (!row) return false;
const approverId = payload.userId ?? row.approver_user_id;
// payload.userId is the raw platform userId (e.g. "6037840640"); namespace it
// with the channel type so it matches users(id) format. Then verify the
// clicker is the designated approver OR has owner/admin privilege over this
// agent group — any other click is rejected so random users can't self-admit
// via stolen card forwarding.
const clickerId = payload.userId ? `${payload.channelType}:${payload.userId}` : null;
const isAuthorized =
clickerId !== null && (clickerId === row.approver_user_id || hasAdminPrivilege(clickerId, row.agent_group_id));
if (!isAuthorized) {
log.warn('Unknown-sender approval click rejected — unauthorized clicker', {
approvalId: row.id,
clickerId,
expectedApprover: row.approver_user_id,
});
return true; // claim the response so it's not unclaimed-logged, but do nothing
}
const approverId = clickerId;
const approved = payload.value === 'approve';
if (approved) {

View File

@@ -184,9 +184,7 @@ describe('unknown-sender request_approval flow', () => {
expect(deliverMock).toHaveBeenCalledTimes(1);
const { getDb } = await import('../../db/connection.js');
const count = (
getDb().prepare('SELECT COUNT(*) AS c FROM pending_sender_approvals').get() as { c: number }
).c;
const count = (getDb().prepare('SELECT COUNT(*) AS c FROM pending_sender_approvals').get() as { c: number }).c;
expect(count).toBe(1);
});
@@ -208,7 +206,10 @@ describe('unknown-sender request_approval flow', () => {
const claimed = await handler({
questionId: pending.id,
value: 'approve',
userId: 'telegram:owner',
// Chat SDK's onAction surfaces the raw platform userId (e.g. Telegram
// chat id). The permissions handler namespaces it with channelType to
// match users(id).
userId: 'owner',
channelType: 'telegram',
platformId: 'dm-owner',
threadId: null,
@@ -245,7 +246,7 @@ describe('unknown-sender request_approval flow', () => {
const claimed = await handler({
questionId: pending.id,
value: 'reject',
userId: 'telegram:owner',
userId: 'owner', // raw platform id — handler namespaces with channelType
channelType: 'telegram',
platformId: 'dm-owner',
threadId: null,
@@ -253,13 +254,91 @@ describe('unknown-sender request_approval flow', () => {
if (claimed) break;
}
const count = (
getDb().prepare('SELECT COUNT(*) AS c FROM pending_sender_approvals').get() as { c: number }
).c;
const count = (getDb().prepare('SELECT COUNT(*) AS c FROM pending_sender_approvals').get() as { c: number }).c;
expect(count).toBe(0);
const member = getDb()
.prepare('SELECT 1 AS x FROM agent_group_members WHERE user_id = ? AND agent_group_id = ?')
.get('tg:stranger', 'ag-1');
expect(member).toBeUndefined();
});
it('rejects clicks from an unauthorized user (prevents self-admit via forwarded card)', async () => {
// Stranger triggers the approval flow; card goes to the owner.
const { routeInbound } = await import('../../router.js');
const { getResponseHandlers } = await import('../../response-registry.js');
await routeInbound(stranger('can I play'));
await new Promise((r) => setTimeout(r, 10));
const { getDb } = await import('../../db/connection.js');
const pending = getDb().prepare('SELECT id FROM pending_sender_approvals').get() as { id: string };
expect(pending).toBeDefined();
// A random user (not the stranger, not the owner, not an admin) tries to
// click the approval — e.g. they got the card forwarded. Should be
// rejected without admitting them.
for (const handler of getResponseHandlers()) {
const claimed = await handler({
questionId: pending.id,
value: 'approve',
userId: 'random-bystander', // not owner, not admin
channelType: 'telegram',
platformId: 'dm-random',
threadId: null,
});
if (claimed) break;
}
// No member added for the stranger.
const member = getDb()
.prepare('SELECT 1 AS x FROM agent_group_members WHERE user_id = ? AND agent_group_id = ?')
.get('tg:stranger', 'ag-1');
expect(member).toBeUndefined();
// Pending row is still there — a legitimate approver can still act on it.
const stillPending = (getDb().prepare('SELECT COUNT(*) AS c FROM pending_sender_approvals').get() as { c: number })
.c;
expect(stillPending).toBe(1);
});
it('accepts a click from a global admin even if they are not the designated approver', async () => {
// Pre-seed a separate admin user so we can click as them.
upsertUser({ id: 'telegram:admin-bob', kind: 'telegram', display_name: 'Bob', created_at: now() });
grantRole({
user_id: 'telegram:admin-bob',
role: 'admin',
agent_group_id: null,
granted_by: 'telegram:owner',
granted_at: now(),
});
const { routeInbound } = await import('../../router.js');
const { getResponseHandlers } = await import('../../response-registry.js');
await routeInbound(stranger('knock knock'));
await new Promise((r) => setTimeout(r, 10));
const { getDb } = await import('../../db/connection.js');
const pending = getDb().prepare('SELECT id FROM pending_sender_approvals').get() as { id: string };
expect(pending).toBeDefined();
// Admin clicks approve (not the designated approver, which was owner).
for (const handler of getResponseHandlers()) {
const claimed = await handler({
questionId: pending.id,
value: 'approve',
userId: 'admin-bob',
channelType: 'telegram',
platformId: 'dm-bob',
threadId: null,
});
if (claimed) break;
}
// Stranger admitted thanks to the admin's authority.
const member = getDb()
.prepare('SELECT 1 AS x FROM agent_group_members WHERE user_id = ? AND agent_group_id = ?')
.get('tg:stranger', 'ag-1');
expect(member).toBeDefined();
});
});