Skip to content

Commit ffae2f3

Browse files
committed
fix(executor): honor caller cancellation in unshared permission loads
1 parent 130140f commit ffae2f3

3 files changed

Lines changed: 51 additions & 5 deletions

File tree

apps/sim/ee/access-control/utils/permission-check.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -246,12 +246,15 @@ async function loadPermissionConfig(
246246
/**
247247
* Loads the governed subject's permission config. The subject is resolved here, not by callers,
248248
* so every gate reads the same person's group. On a run context the in-flight load is memoized per
249-
* subject and workspace in the run's `permissionConfigCache`, and a failed load is evicted.
249+
* subject and workspace in the run's `permissionConfigCache`, and a failed load is evicted. A shared
250+
* load observes only the run's abort signal, so one caller's cancellation cannot fail it for others;
251+
* an unshared load observes the caller's `signal`.
250252
*/
251253
async function getPermissionConfig(
252254
actorUserId: string | undefined,
253255
workspaceId: string | undefined,
254-
ctx?: ExecutionContext
256+
ctx?: ExecutionContext,
257+
signal?: AbortSignal
255258
): Promise<PermissionGroupConfig | null> {
256259
const userId = governedSubjectUserId(actorUserId, ctx)
257260
if (!userId || !workspaceId) {
@@ -260,7 +263,7 @@ async function getPermissionConfig(
260263

261264
const cache = ctx?.permissionConfigCache
262265
if (!cache) {
263-
return loadPermissionConfig(userId, workspaceId, ctx?.abortSignal)
266+
return loadPermissionConfig(userId, workspaceId, signal ?? ctx?.abortSignal)
264267
}
265268

266269
const key = `${userId}:${workspaceId}`
@@ -536,6 +539,8 @@ interface PermissionAssertion {
536539
toolId?: string
537540
toolKind?: ToolKind
538541
ctx?: ExecutionContext
542+
/** Caller cancellation, observed while loading a config that is not shared through a run cache. */
543+
signal?: AbortSignal
539544
}
540545

541546
/**
@@ -553,7 +558,7 @@ interface PermissionAssertion {
553558
/** permission-group-enforced: custom_tools.use — gates tool invocation during a run, not an operation */
554559
/** permission-group-enforced: skills.use — gates skill loading during a run, not an operation */
555560
export async function assertPermissionsAllowed(req: PermissionAssertion): Promise<void> {
556-
const { workspaceId, model, blockType, toolId, toolKind, ctx } = req
561+
const { workspaceId, model, blockType, toolId, toolKind, ctx, signal } = req
557562
const userId = governedSubjectUserId(req.userId, ctx)
558563

559564
const blockTypeExempt = blockType ? isBlockTypeAccessControlExempt(blockType) : false
@@ -564,7 +569,7 @@ export async function assertPermissionsAllowed(req: PermissionAssertion): Promis
564569

565570
const config =
566571
userId && workspaceId
567-
? await getPermissionConfig(userId, workspaceId, ctx)
572+
? await getPermissionConfig(userId, workspaceId, ctx, signal)
568573
: mergeEnvAllowlist(null)
569574

570575
const subject = { userId, workspaceId }

apps/sim/ee/access-control/utils/permission-gate-subject.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,46 @@ describe('the run-scoped permission config cache', () => {
297297
expect(ctx.permissionConfigCache).toBeUndefined()
298298
})
299299

300+
it('stops retrying when the caller of a check outside a run cancels', async () => {
301+
const controller = new AbortController()
302+
const reason = new Error('Tool cancelled')
303+
mocks.getUserPermissionConfig.mockImplementationOnce(async () => {
304+
controller.abort(reason)
305+
throw databaseError()
306+
})
307+
308+
await expect(
309+
assertPermissionsAllowed({
310+
userId: 'user-1',
311+
workspaceId: 'workspace-1',
312+
toolId: 'http_request',
313+
signal: controller.signal,
314+
})
315+
).rejects.toBe(reason)
316+
expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(1)
317+
})
318+
319+
it('does not let one caller cancel a load shared through the run cache', async () => {
320+
const run = runContext()
321+
const controller = new AbortController()
322+
mocks.getUserPermissionConfig.mockImplementationOnce(async () => {
323+
controller.abort(new Error('Tool cancelled'))
324+
throw databaseError()
325+
})
326+
327+
const cancelled = assertPermissionsAllowed({
328+
userId: 'user-1',
329+
workspaceId: 'workspace-1',
330+
toolId: 'http_request',
331+
ctx: { ...run },
332+
signal: controller.signal,
333+
})
334+
const other = gate({ ...run })
335+
336+
await expect(Promise.all([cancelled, other])).resolves.toBeDefined()
337+
expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2)
338+
})
339+
300340
it('retries a transient failure for a check made outside a run', async () => {
301341
mocks.getUserPermissionConfig.mockRejectedValueOnce(databaseError())
302342

apps/sim/tools/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1754,6 +1754,7 @@ async function executeToolImplementation(
17541754
toolId: normalizedToolId,
17551755
toolKind,
17561756
ctx: executionContext,
1757+
signal: effectiveSignal,
17571758
})
17581759
} catch (error) {
17591760
effectiveSignal?.throwIfAborted()

0 commit comments

Comments
 (0)