Skip to content

Commit feaa1af

Browse files
pgondhi987pgondhi987
committed
fix: bind approval access to requester metadata
1 parent 46f7750 commit feaa1af

6 files changed

Lines changed: 280 additions & 22 deletions

File tree

‎src/gateway/exec-approval-manager.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,10 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
199199

200200
lookupApprovalId(
201201
input: string,
202-
opts: { includeResolved?: boolean } = {},
202+
opts: {
203+
includeResolved?: boolean;
204+
filter?: (record: ExecApprovalRecord<TPayload>) => boolean;
205+
} = {},
203206
): ExecApprovalIdLookupResult {
204207
const normalized = input.trim();
205208
if (!normalized) {
@@ -208,7 +211,8 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
208211

209212
const exact = this.pending.get(normalized);
210213
if (exact) {
211-
return opts.includeResolved || exact.record.resolvedAtMs === undefined
214+
return (opts.includeResolved || exact.record.resolvedAtMs === undefined) &&
215+
(opts.filter?.(exact.record) ?? true)
212216
? { kind: "exact", id: normalized }
213217
: { kind: "none" };
214218
}
@@ -219,6 +223,9 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
219223
if (!opts.includeResolved && entry.record.resolvedAtMs !== undefined) {
220224
continue;
221225
}
226+
if (opts.filter && !opts.filter(entry.record)) {
227+
continue;
228+
}
222229
if (normalizeLowercaseStringOrEmpty(id).startsWith(lowerPrefix)) {
223230
matches.push(id);
224231
}

‎src/gateway/server-methods/approval-shared.ts‎

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,36 @@ function resolvePendingApprovalLookupError(params: {
7777
};
7878
}
7979

80+
function normalizeApprovalIdentity(value: string | null | undefined): string | null {
81+
return normalizeOptionalString(value) ?? null;
82+
}
83+
84+
export function isApprovalRecordVisibleToClient<TPayload>(params: {
85+
record: ExecApprovalRecord<TPayload>;
86+
client: GatewayClient | null;
87+
}): boolean {
88+
const requestedByDeviceId = normalizeApprovalIdentity(params.record.requestedByDeviceId);
89+
if (requestedByDeviceId) {
90+
return requestedByDeviceId === normalizeApprovalIdentity(params.client?.connect?.device?.id);
91+
}
92+
93+
const requestedByConnId = normalizeApprovalIdentity(params.record.requestedByConnId);
94+
if (requestedByConnId) {
95+
return requestedByConnId === normalizeApprovalIdentity(params.client?.connId);
96+
}
97+
98+
const requestedByClientId = normalizeApprovalIdentity(params.record.requestedByClientId);
99+
if (requestedByClientId) {
100+
return requestedByClientId === normalizeApprovalIdentity(params.client?.connect?.client?.id);
101+
}
102+
103+
return true;
104+
}
105+
80106
export function resolvePendingApprovalRecord<TPayload>(params: {
81107
manager: ExecApprovalManager<TPayload>;
82108
inputId: string;
109+
client?: GatewayClient | null;
83110
exposeAmbiguousPrefixError?: boolean;
84111
}):
85112
| {
@@ -91,7 +118,13 @@ export function resolvePendingApprovalRecord<TPayload>(params: {
91118
ok: false;
92119
response: PendingApprovalLookupError;
93120
} {
94-
const resolvedId = params.manager.lookupPendingId(params.inputId);
121+
const resolvedId = params.manager.lookupApprovalId(params.inputId, {
122+
filter: (record) =>
123+
isApprovalRecordVisibleToClient({
124+
record,
125+
client: params.client ?? null,
126+
}),
127+
});
95128
if (resolvedId.kind !== "exact" && resolvedId.kind !== "prefix") {
96129
return {
97130
ok: false,
@@ -111,6 +144,7 @@ export function resolvePendingApprovalRecord<TPayload>(params: {
111144
function resolveResolvedApprovalRecord<TPayload>(params: {
112145
manager: ExecApprovalManager<TPayload>;
113146
inputId: string;
147+
client?: GatewayClient | null;
114148
exposeAmbiguousPrefixError?: boolean;
115149
}):
116150
| {
@@ -122,7 +156,14 @@ function resolveResolvedApprovalRecord<TPayload>(params: {
122156
ok: false;
123157
response: PendingApprovalLookupError;
124158
} {
125-
const resolvedId = params.manager.lookupApprovalId(params.inputId, { includeResolved: true });
159+
const resolvedId = params.manager.lookupApprovalId(params.inputId, {
160+
includeResolved: true,
161+
filter: (record) =>
162+
isApprovalRecordVisibleToClient({
163+
record,
164+
client: params.client ?? null,
165+
}),
166+
});
126167
if (resolvedId.kind !== "exact" && resolvedId.kind !== "prefix") {
127168
return {
128169
ok: false,
@@ -298,12 +339,14 @@ export async function handleApprovalResolve<TPayload, TResolvedEvent extends obj
298339
const resolved = resolvePendingApprovalRecord({
299340
manager: params.manager,
300341
inputId: params.inputId,
342+
client: params.client,
301343
exposeAmbiguousPrefixError: params.exposeAmbiguousPrefixError,
302344
});
303345
if (!resolved.ok) {
304346
const resolvedRepeat = resolveResolvedApprovalRecord({
305347
manager: params.manager,
306348
inputId: params.inputId,
349+
client: params.client,
307350
exposeAmbiguousPrefixError: params.exposeAmbiguousPrefixError,
308351
});
309352
if (resolvedRepeat.ok) {

‎src/gateway/server-methods/exec-approval.ts‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
handlePendingApprovalRequest,
3636
handleApprovalResolve,
3737
isApprovalDecision,
38+
isApprovalRecordVisibleToClient,
3839
respondPendingApprovalLookupError,
3940
resolvePendingApprovalRecord,
4041
} from "./approval-shared.js";
@@ -85,7 +86,7 @@ export function createExecApprovalHandlers(
8586
opts?: { forwarder?: ExecApprovalForwarder; iosPushDelivery?: ExecApprovalIosPushDelivery },
8687
): GatewayRequestHandlers {
8788
return {
88-
"exec.approval.get": async ({ params, respond }) => {
89+
"exec.approval.get": async ({ params, respond, client }) => {
8990
if (!validateExecApprovalGetParams(params)) {
9091
respond(
9192
false,
@@ -103,6 +104,7 @@ export function createExecApprovalHandlers(
103104
const resolved = resolvePendingApprovalRecord({
104105
manager,
105106
inputId: p.id,
107+
client,
106108
exposeAmbiguousPrefixError: true,
107109
});
108110
if (!resolved.ok) {
@@ -127,15 +129,18 @@ export function createExecApprovalHandlers(
127129
undefined,
128130
);
129131
},
130-
"exec.approval.list": async ({ respond }) => {
132+
"exec.approval.list": async ({ respond, client }) => {
131133
respond(
132134
true,
133-
manager.listPendingRecords().map((record) => ({
134-
id: record.id,
135-
request: record.request,
136-
createdAtMs: record.createdAtMs,
137-
expiresAtMs: record.expiresAtMs,
138-
})),
135+
manager
136+
.listPendingRecords()
137+
.filter((record) => isApprovalRecordVisibleToClient({ record, client }))
138+
.map((record) => ({
139+
id: record.id,
140+
request: record.request,
141+
createdAtMs: record.createdAtMs,
142+
expiresAtMs: record.expiresAtMs,
143+
})),
139144
undefined,
140145
);
141146
},

‎src/gateway/server-methods/plugin-approval.test.ts‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ describe("createPluginApprovalHandlers", () => {
201201

202202
// Resolve the approval so the handler can complete
203203
const approvalId = acceptedApprovalId(respond as unknown as MockCallSource);
204+
expect(manager.getSnapshot(approvalId)?.requestedByClientId).toBe("test-client");
204205
manager.resolve(approvalId, "allow-once");
205206

206207
await handlerPromise;
@@ -468,6 +469,52 @@ describe("createPluginApprovalHandlers", () => {
468469
manager.resolve(approvalId, "allow-once");
469470
await handlerPromise;
470471
});
472+
473+
it("lists only plugin approvals owned by the caller", async () => {
474+
const handlers = createPluginApprovalHandlers(manager);
475+
const visible = manager.create(
476+
{ title: "Visible", description: "D" },
477+
60_000,
478+
"plugin:visible",
479+
);
480+
visible.requestedByDeviceId = "device-owner";
481+
visible.requestedByConnId = "conn-owner";
482+
visible.requestedByClientId = "client-owner";
483+
void manager.register(visible, 60_000);
484+
485+
const hidden = manager.create({ title: "Hidden", description: "D" }, 60_000, "plugin:hidden");
486+
hidden.requestedByDeviceId = "device-other";
487+
hidden.requestedByConnId = "conn-other";
488+
hidden.requestedByClientId = "client-other";
489+
void manager.register(hidden, 60_000);
490+
491+
const listRespond = vi.fn();
492+
await handlers["plugin.approval.list"](
493+
createMockOptions(
494+
"plugin.approval.list",
495+
{},
496+
{
497+
respond: listRespond,
498+
client: {
499+
connId: "conn-owner",
500+
connect: {
501+
client: { id: "client-owner" },
502+
device: { id: "device-owner" },
503+
},
504+
} as GatewayRequestHandlerOptions["client"],
505+
},
506+
),
507+
);
508+
509+
expect(responseCall(listRespond as unknown as MockCallSource).ok).toBe(true);
510+
const approvals = requireArray(
511+
responseCall(listRespond as unknown as MockCallSource).result,
512+
"approval list",
513+
);
514+
expect(approvals.map((entry) => requireRecord(entry, "approval").id)).toEqual([
515+
"plugin:visible",
516+
]);
517+
});
471518
});
472519

473520
describe("plugin.approval.waitDecision", () => {
@@ -542,6 +589,74 @@ describe("createPluginApprovalHandlers", () => {
542589
expect(resolvedBroadcast.options).toEqual({ dropIfSlow: true });
543590
});
544591

592+
it("resolves only plugin approvals owned by the caller", async () => {
593+
const handlers = createPluginApprovalHandlers(manager);
594+
const visible = manager.create(
595+
{ title: "Visible", description: "D" },
596+
60_000,
597+
"plugin:abcd-visible",
598+
);
599+
visible.requestedByDeviceId = "device-owner";
600+
visible.requestedByConnId = "conn-owner";
601+
visible.requestedByClientId = "client-owner";
602+
void manager.register(visible, 60_000);
603+
604+
const hidden = manager.create(
605+
{ title: "Hidden", description: "D" },
606+
60_000,
607+
"plugin:abcd-hidden",
608+
);
609+
hidden.requestedByDeviceId = "device-other";
610+
hidden.requestedByConnId = "conn-other";
611+
hidden.requestedByClientId = "client-other";
612+
void manager.register(hidden, 60_000);
613+
614+
const ownerClient = {
615+
connId: "conn-owner",
616+
connect: {
617+
client: { id: "client-owner" },
618+
device: { id: "device-owner" },
619+
},
620+
} as GatewayRequestHandlerOptions["client"];
621+
const resolveRespond = vi.fn();
622+
await handlers["plugin.approval.resolve"](
623+
createMockOptions(
624+
"plugin.approval.resolve",
625+
{
626+
id: "plugin:abcd",
627+
decision: "allow-once",
628+
},
629+
{
630+
respond: resolveRespond,
631+
client: ownerClient,
632+
},
633+
),
634+
);
635+
expect(resolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined);
636+
expect(manager.getSnapshot(visible.id)?.decision).toBe("allow-once");
637+
expect(manager.getSnapshot(hidden.id)?.decision).toBeUndefined();
638+
639+
const hiddenRespond = vi.fn();
640+
await handlers["plugin.approval.resolve"](
641+
createMockOptions(
642+
"plugin.approval.resolve",
643+
{
644+
id: hidden.id,
645+
decision: "deny",
646+
},
647+
{
648+
respond: hiddenRespond,
649+
client: ownerClient,
650+
},
651+
),
652+
);
653+
expect(responseCall(hiddenRespond as unknown as MockCallSource).ok).toBe(false);
654+
const error = responseError(hiddenRespond as unknown as MockCallSource);
655+
expect(error.code).toBe("INVALID_REQUEST");
656+
expect(error.message).toBe("unknown or expired approval id");
657+
expect(manager.getSnapshot(hidden.id)?.decision).toBeUndefined();
658+
});
659+
545660
it("rejects decisions outside plugin approval allowed decisions", async () => {
546661
const handlers = createPluginApprovalHandlers(manager);
547662
const record = manager.create(

‎src/gateway/server-methods/plugin-approval.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
handleApprovalWaitDecision,
2222
handlePendingApprovalRequest,
2323
isApprovalDecision,
24+
isApprovalRecordVisibleToClient,
2425
} from "./approval-shared.js";
2526
import type { GatewayRequestHandlers } from "./types.js";
2627

@@ -29,15 +30,18 @@ export function createPluginApprovalHandlers(
2930
opts?: { forwarder?: ExecApprovalForwarder },
3031
): GatewayRequestHandlers {
3132
return {
32-
"plugin.approval.list": async ({ respond }) => {
33+
"plugin.approval.list": async ({ respond, client }) => {
3334
respond(
3435
true,
35-
manager.listPendingRecords().map((record) => ({
36-
id: record.id,
37-
request: record.request,
38-
createdAtMs: record.createdAtMs,
39-
expiresAtMs: record.expiresAtMs,
40-
})),
36+
manager
37+
.listPendingRecords()
38+
.filter((record) => isApprovalRecordVisibleToClient({ record, client }))
39+
.map((record) => ({
40+
id: record.id,
41+
request: record.request,
42+
createdAtMs: record.createdAtMs,
43+
expiresAtMs: record.expiresAtMs,
44+
})),
4145
undefined,
4246
);
4347
},
@@ -106,6 +110,10 @@ export function createPluginApprovalHandlers(
106110
// Always server-generate the ID — never accept plugin-provided IDs.
107111
// Kind-prefix so /approve routing can distinguish plugin vs exec IDs deterministically.
108112
const record = manager.create(request, timeoutMs, `plugin:${randomUUID()}`);
113+
record.requestedByConnId = client?.connId ?? null;
114+
record.requestedByDeviceId = client?.connect?.device?.id ?? null;
115+
record.requestedByClientId = client?.connect?.client?.id ?? null;
116+
record.requestedByDeviceTokenAuth = client?.isDeviceTokenAuth === true;
109117

110118
let decisionPromise: Promise<ExecApprovalDecision | null>;
111119
try {

0 commit comments

Comments
 (0)