Skip to content

Commit f42bda6

Browse files
waleedlatif1waleedlatif1
committed
fix(access-control): make the loading rule structural, not a convention
Two review bots found the same class of bug in two more places, which is the real finding: "never persist from a predicate that reads as unrestricted while the config loads" was a rule each callsite re-implemented, and the rule had already been forgotten twice. Closes both reported instances and moves the rule somewhere it cannot be forgotten again: - `useOperationAccess.resolveSeedGate` now owns the creation-time veto for both restricted fields, so `workflow.tsx` states no policy of its own — it asks for a gate and passes it on. Previously the model half of the invariant was carried by an operation-shaped object that merely happened to be absent during the same window. - The agent tool picker and both operation selectors close while the config is unknown. Every list they offer — blocks, operations, MCP and custom tools — reads as unrestricted for that beat, and each pick is a one-shot write. - A preset operation goes through the same gate as a declared default. It comes from the search index, which is itself unfiltered while loading, so it is not the informed pick it looks like. - `isPermissionLoading` is exposed from one hook, so all four surfaces read the same symbol instead of four spellings of the same condition. Also from the review passes: dropped `isModelAllowed`/`isProviderAllowed` from the public interface (consolidating onto `isModelUsable` left them with no external consumer), un-exported `resolveOperationToolId` (no non-test caller), and corrected the `isSeededValueAllowed` TSDoc, which still described the contract the previous commit replaced. Tests: replaced a case that asserted its own fixture rather than the code with coverage of the two guard branches that were genuinely untested — an empty-string and a non-string declared default must bypass the gate, since both mean "nothing was declared" rather than a value to authorize.
1 parent 698a8b3 commit f42bda6

9 files changed

Lines changed: 110 additions & 78 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ export const Dropdown = memo(function Dropdown({
101101
preserveLabelCase = false,
102102
}: DropdownProps) {
103103
const activeSearchTarget = useActiveSearchTarget()
104-
const { getDeniedOperations, resolveDefaultOperation } = useOperationAccess()
104+
const { getDeniedOperations, resolveDefaultOperation, isPermissionLoading } = useOperationAccess()
105105
const [storeValue, setStoreValue] = useSubBlockValue<string | string[]>(blockId, subBlockId) as [
106106
string | string[] | null | undefined,
107107
(value: string | string[]) => void,
@@ -437,7 +437,9 @@ export const Dropdown = memo(function Dropdown({
437437
onChange={handleChange}
438438
onMultiSelectChange={handleMultiSelectChange}
439439
placeholder={placeholder}
440-
disabled={disabled}
440+
/* The operation list only drops denied entries once the config resolves,
441+
and a pick here persists — matching the agent tool selector. */
442+
disabled={disabled || (subBlockId === OPERATION_SUBBLOCK_ID && isPermissionLoading)}
441443
editable={false}
442444
onOpenChange={handleOpenChange}
443445
overlayContent={multiSelectOverlay ?? singleSelectOverlay}

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,11 @@ export const ToolInput = memo(function ToolInput({
665665
const provider = model ? getProviderFromModel(model) : ''
666666
const supportsToolControl = provider ? supportsToolUsageControl(provider) : false
667667

668-
const { filterBlocks, config: permissionConfig } = usePermissionConfig()
668+
const {
669+
filterBlocks,
670+
config: permissionConfig,
671+
isLoading: isPermissionLoading,
672+
} = usePermissionConfig()
669673
const { getDeniedOperations } = useOperationAccess()
670674

671675
/**
@@ -1704,7 +1708,11 @@ export const ToolInput = memo(function ToolInput({
17041708
options={[]}
17051709
groups={toolGroups}
17061710
placeholder='Add tool...'
1707-
disabled={disabled}
1711+
/* Every list this picker offers — blocks, operations, MCP and custom
1712+
tools — reads as unrestricted until the permission config resolves,
1713+
and adding a tool is a one-shot write that nothing revisits. Closed
1714+
rather than optimistic for that beat. */
1715+
disabled={disabled || isPermissionLoading}
17081716
searchable
17091717
searchPlaceholder='Search tools...'
17101718
maxHeight={240}
@@ -2115,7 +2123,9 @@ export const ToolInput = memo(function ToolInput({
21152123
}
21162124
onChange={(value) => handleOperationChange(toolIndex, value)}
21172125
placeholder='Select operation'
2118-
disabled={disabled}
2126+
/* Denied operations only drop out once the config
2127+
resolves, and picking one rewrites the stored tool. */
2128+
disabled={disabled || isPermissionLoading}
21192129
/>
21202130
</div>
21212131
)

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx‎

Lines changed: 10 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ import { useSession } from '@/lib/auth/auth-client'
4141
import type { OAuthConnectEventDetail } from '@/lib/copilot/tools/client/base-tool'
4242
import { consumeOAuthReturnContext, writeOAuthReturnContext } from '@/lib/credentials/client-state'
4343
import type { OAuthProvider } from '@/lib/oauth'
44-
import { MODEL_SUBBLOCK_ID, OPERATION_SUBBLOCK_ID } from '@/lib/permission-groups/operation-access'
44+
import { OPERATION_SUBBLOCK_ID } from '@/lib/permission-groups/operation-access'
4545
import { getDefaultBlockName } from '@/lib/workflows/blocks/canvas-presentation'
4646
import { requestNoteImage, requestNoteRename } from '@/lib/workflows/notes/canvas-requests'
4747
import { TriggerUtils } from '@/lib/workflows/triggers/triggers'
@@ -129,7 +129,6 @@ import { useCanvasViewport } from '@/hooks/use-canvas-viewport'
129129
import { useCollaborativeWorkflow } from '@/hooks/use-collaborative-workflow'
130130
import { useOAuthReturnForWorkflow } from '@/hooks/use-oauth-return'
131131
import { useOperationAccess } from '@/hooks/use-operation-access'
132-
import { usePermissionConfig } from '@/hooks/use-permission-config'
133132
import { useCanvasModeStore } from '@/stores/canvas-mode'
134133
import { useChatStore } from '@/stores/chat/store'
135134
import {
@@ -871,8 +870,7 @@ const WorkflowContent = React.memo(
871870
*/
872871
const pendingFocusBlockIdRef = useRef<string | null>(null)
873872

874-
const { resolveOperationGate } = useOperationAccess()
875-
const { isModelUsable } = usePermissionConfig()
873+
const { resolveSeedGate } = useOperationAccess()
876874

877875
const addBlock = useCallback(
878876
(
@@ -895,20 +893,7 @@ const WorkflowContent = React.memo(
895893
if (parentId) blockData.parentId = parentId
896894
if (extent) blockData.extent = extent
897895

898-
const operationGate = resolveOperationGate(getBlock(type))
899-
900-
/**
901-
* Creation is one-shot, so an unknown permission config cannot be
902-
* answered by waiting the way the editor's pickers do — a value written
903-
* here is never revisited. The restricted fields therefore seed empty
904-
* until the config resolves, and the pickers fill them the moment it
905-
* does. Seeding the declared default instead would persist it unchecked.
906-
*/
907-
const seedGate = (subBlockId: string, value: string) => {
908-
if (subBlockId !== OPERATION_SUBBLOCK_ID && subBlockId !== MODEL_SUBBLOCK_ID) return true
909-
if (!operationGate) return false
910-
return subBlockId === OPERATION_SUBBLOCK_ID ? operationGate(value) : isModelUsable(value)
911-
}
896+
const seedGate = resolveSeedGate(getBlock(type))
912897

913898
const block = prepareBlockState({
914899
id,
@@ -937,15 +922,14 @@ const WorkflowContent = React.memo(
937922
if (!subBlockValues[id]) {
938923
subBlockValues[id] = {}
939924
}
940-
/* Search and the connection picker already drop denied operations, so
941-
this only catches one arriving by another route — a recent pick that
942-
outlived a permission change. Dropping the key rather than the whole
943-
preset leaves whatever `prepareBlockState` seeded in place. Unlike a
944-
declared default, a preset is the user's explicit pick, so an
945-
unknown config honours it and leaves the server to gate the run. */
925+
/* The same gate as the declared default, deliberately: a preset is
926+
offered by search and the connection picker, whose index reads as
927+
unrestricted until the config resolves — so it is not the informed
928+
pick it looks like, and honouring it would persist an operation
929+
from an unfiltered list. */
946930
const presetOperation = presetSubBlockValues[OPERATION_SUBBLOCK_ID]
947931
const presetOperationDenied =
948-
operationGate && typeof presetOperation === 'string' && !operationGate(presetOperation)
932+
typeof presetOperation === 'string' && !seedGate(OPERATION_SUBBLOCK_ID, presetOperation)
949933

950934
Object.assign(
951935
subBlockValues[id],
@@ -964,13 +948,7 @@ const WorkflowContent = React.memo(
964948
)
965949
usePanelEditorStore.getState().setCurrentBlockId(id)
966950
},
967-
[
968-
collaborativeBatchAddBlocks,
969-
setSelectedEdges,
970-
setPendingSelection,
971-
resolveOperationGate,
972-
isModelUsable,
973-
]
951+
[collaborativeBatchAddBlocks, setSelectedEdges, setPendingSelection, resolveSeedGate]
974952
)
975953

976954
const { activeBlockIds, pendingBlocks, isDebugging, isExecuting } = useExecutionStore(

‎apps/sim/hooks/use-operation-access.ts‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,22 @@ import { useMemo } from 'react'
44
import {
55
collectDeniedOperationIds,
66
isOperationAllowed,
7+
MODEL_SUBBLOCK_ID,
78
NO_DENIED_OPERATIONS,
9+
OPERATION_SUBBLOCK_ID,
810
type OperationGateBlock,
911
pickDefaultOperation,
12+
type SeedValueGate,
1013
} from '@/lib/permission-groups/operation-access'
1114
import { usePermissionConfig } from '@/hooks/use-permission-config'
1215

1316
export interface OperationAccess {
17+
/**
18+
* Whether the permission config is still loading. Every list this module
19+
* filters reads as unrestricted until it resolves, so a surface that
20+
* *persists* a pick from one must not accept input while this is true.
21+
*/
22+
isPermissionLoading: boolean
1423
/**
1524
* The operation ids of `block` the caller may not run. Empty while the
1625
* config loads, so pickers show everything rather than flashing a short list.
@@ -41,6 +50,16 @@ export interface OperationAccess {
4150
resolveOperationGate: (
4251
block: OperationGateBlock | null | undefined
4352
) => ((operationId: string) => boolean) | undefined
53+
/**
54+
* The veto `prepareBlockState` applies to a new block's declared defaults.
55+
*
56+
* Creation is one-shot, so unlike the pickers it cannot answer "unknown" by
57+
* waiting — a value written there is never revisited. This gate therefore
58+
* rejects both restricted fields until the config resolves, leaving them
59+
* empty for the pickers to fill, and owns that rule so no caller re-derives
60+
* it. Every other field passes through untouched.
61+
*/
62+
resolveSeedGate: (block: OperationGateBlock | null | undefined) => SeedValueGate
4463
}
4564

4665
/**
@@ -52,11 +71,12 @@ export interface OperationAccess {
5271
* canvas search, block creation — agrees.
5372
*/
5473
export function useOperationAccess(): OperationAccess {
55-
const { isToolAllowed, isLoading } = usePermissionConfig()
74+
const { isToolAllowed, isModelUsable, isLoading } = usePermissionConfig()
5675

5776
return useMemo(() => {
5877
const isReady = !isLoading
5978
return {
79+
isPermissionLoading: isLoading,
6080
getDeniedOperations: (block, operationIds) =>
6181
isReady
6282
? collectDeniedOperationIds(block, operationIds, isToolAllowed)
@@ -67,6 +87,13 @@ export function useOperationAccess(): OperationAccess {
6787
isReady
6888
? (operationId: string) => isOperationAllowed(block, operationId, isToolAllowed)
6989
: undefined,
90+
resolveSeedGate: (block) => (subBlockId, value) => {
91+
if (subBlockId !== OPERATION_SUBBLOCK_ID && subBlockId !== MODEL_SUBBLOCK_ID) return true
92+
if (!isReady) return false
93+
return subBlockId === OPERATION_SUBBLOCK_ID
94+
? isOperationAllowed(block, value, isToolAllowed)
95+
: isModelUsable(value)
96+
},
7097
}
71-
}, [isToolAllowed, isLoading])
98+
}, [isToolAllowed, isModelUsable, isLoading])
7299
}

‎apps/sim/hooks/use-permission-config.ts‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,10 @@ export interface PermissionConfigResult {
3232
filterBlocks: <T extends { type: string }>(blocks: T[]) => T[]
3333
filterProviders: (providerIds: string[]) => string[]
3434
isBlockAllowed: (blockType: string) => boolean
35-
isProviderAllowed: (providerId: string) => boolean
36-
isModelAllowed: (model: string) => boolean
3735
/**
38-
* Whether a model is usable at all: allowed by the model denylist *and* by the
39-
* provider allowlist. Both gates apply to every model field, so prefer this
40-
* over calling `isModelAllowed` and `isProviderAllowed` separately.
36+
* Whether a model is usable at all: allowed by the model denylist *and* by
37+
* the provider allowlist. Both gates apply to every model field, so this is
38+
* the only model predicate the interface exposes.
4139
*/
4240
isModelUsable: (model: string) => boolean
4341
isToolAllowed: (toolId: string) => boolean
@@ -188,8 +186,6 @@ export function usePermissionConfig(): PermissionConfigResult {
188186
filterBlocks,
189187
filterProviders,
190188
isBlockAllowed,
191-
isProviderAllowed,
192-
isModelAllowed,
193189
isModelUsable,
194190
isToolAllowed,
195191
isInvitationsDisabled,
@@ -203,8 +199,6 @@ export function usePermissionConfig(): PermissionConfigResult {
203199
filterBlocks,
204200
filterProviders,
205201
isBlockAllowed,
206-
isProviderAllowed,
207-
isModelAllowed,
208202
isModelUsable,
209203
isToolAllowed,
210204
isInvitationsDisabled,

‎apps/sim/lib/permission-groups/operation-access.test.ts‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import {
77
isOperationAllowed,
88
type OperationGateBlock,
99
pickDefaultOperation,
10-
resolveOperationToolId,
1110
} from '@/lib/permission-groups/operation-access'
1211

1312
/** A block that resolves its tool from the operation, like most integrations. */
@@ -45,27 +44,27 @@ const deny = (...toolIds: string[]) => {
4544
return (toolId: string) => !denied.has(toolId)
4645
}
4746

48-
describe('resolveOperationToolId', () => {
47+
describe('operation-to-tool resolution', () => {
4948
it('resolves through the block tool selector', () => {
50-
expect(resolveOperationToolId(selectorBlock, 'canvas')).toBe('slack_canvas')
49+
expect(isOperationAllowed(selectorBlock, 'canvas', deny('slack_canvas'))).toBe(false)
50+
expect(isOperationAllowed(selectorBlock, 'canvas', deny('slack_message'))).toBe(true)
5151
})
5252

53-
it('returns the only tool when the block has no selection to make', () => {
54-
expect(resolveOperationToolId(singleToolBlock, 'anything')).toBe('dropcontact_enrich_contact')
53+
it('gates on the only tool when the block has no selection to make', () => {
54+
expect(
55+
isOperationAllowed(singleToolBlock, 'anything', deny('dropcontact_enrich_contact'))
56+
).toBe(false)
5557
})
5658

5759
it('treats an operation id as a tool id when the block has no selector', () => {
58-
expect(resolveOperationToolId(bareBlock, 'sqs_receive')).toBe('sqs_receive')
59-
})
60-
61-
it('returns null rather than guessing when the selector throws', () => {
62-
expect(resolveOperationToolId(selectorBlock, 'not-an-operation')).toBeNull()
60+
expect(isOperationAllowed(bareBlock, 'sqs_receive', deny('sqs_receive'))).toBe(false)
61+
expect(isOperationAllowed(bareBlock, 'sqs_receive', deny('sqs_send'))).toBe(true)
6362
})
6463

65-
it('returns null for a block with no tools', () => {
66-
expect(resolveOperationToolId({ tools: { access: [] } }, 'send')).toBeNull()
67-
expect(resolveOperationToolId(null, 'send')).toBeNull()
68-
expect(resolveOperationToolId(undefined, 'send')).toBeNull()
64+
it('allows rather than guessing when a block has no tools at all', () => {
65+
expect(isOperationAllowed({ tools: { access: [] } }, 'send', denyAll)).toBe(true)
66+
expect(isOperationAllowed(null, 'send', denyAll)).toBe(true)
67+
expect(isOperationAllowed(undefined, 'send', denyAll)).toBe(true)
6968
})
7069
})
7170

‎apps/sim/lib/permission-groups/operation-access.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,13 @@ export type OperationGateBlock = Pick<BlockConfig, 'tools'>
2121
/** Decides whether the caller's permission group allows a concrete tool id. */
2222
export type IsToolAllowed = (toolId: string) => boolean
2323

24+
/**
25+
* Vetoes a subblock's declared default when the caller's permission group does
26+
* not allow it — or when the group config is not known yet, since a default
27+
* written during block creation is never revisited.
28+
*/
29+
export type SeedValueGate = (subBlockId: string, value: string) => boolean
30+
2431
/**
2532
* The tool id a block operation maps to, or `null` when it cannot be resolved
2633
* from the operation alone.
@@ -38,7 +45,7 @@ export type IsToolAllowed = (toolId: string) => boolean
3845
* way, so a `null` only ever costs a denied option staying visible, never a
3946
* permitted option disappearing.
4047
*/
41-
export function resolveOperationToolId(
48+
function resolveOperationToolId(
4249
block: OperationGateBlock | null | undefined,
4350
operationId: string
4451
): string | null {

‎apps/sim/stores/workflows/utils.test.ts‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,6 +1076,8 @@ describe('prepareBlockState — permission-group seed veto', () => {
10761076
{ id: 'operation', type: 'dropdown', defaultValue: 'send' },
10771077
{ id: 'model', type: 'combobox', defaultValue: 'claude-sonnet-5' },
10781078
{ id: 'channel', type: 'short-input', defaultValue: '#general' },
1079+
{ id: 'blank', type: 'short-input', defaultValue: '' },
1080+
{ id: 'headers', type: 'table', defaultValue: [] },
10791081
],
10801082
}
10811083

@@ -1102,6 +1104,8 @@ describe('prepareBlockState — permission-group seed veto', () => {
11021104
operation: 'send',
11031105
model: 'claude-sonnet-5',
11041106
channel: '#general',
1107+
blank: '',
1108+
headers: [],
11051109
})
11061110
})
11071111

@@ -1110,9 +1114,29 @@ describe('prepareBlockState — permission-group seed veto', () => {
11101114
operation: 'send',
11111115
model: 'claude-sonnet-5',
11121116
channel: '#general',
1117+
blank: '',
1118+
headers: [],
11131119
})
11141120
})
11151121

1122+
it('never consults the gate for an empty or non-string default', () => {
1123+
/* Both are "nothing was declared" rather than a value to authorize, and a
1124+
gate that saw them would veto every unfilled field. */
1125+
const seen: string[] = []
1126+
seededValues((subBlockId) => {
1127+
seen.push(subBlockId)
1128+
return true
1129+
})
1130+
expect(seen).not.toContain('blank')
1131+
expect(seen).not.toContain('headers')
1132+
})
1133+
1134+
it('keeps an empty or non-string default even when the gate rejects everything', () => {
1135+
const values = seededValues(() => false)
1136+
expect(values.blank).toBe('')
1137+
expect(values.headers).toEqual([])
1138+
})
1139+
11161140
it('leaves a denied operation unseeded rather than substituting one', () => {
11171141
const values = seededValues((subBlockId) => subBlockId !== 'operation')
11181142
expect(values.operation).toBeNull()
@@ -1126,18 +1150,6 @@ describe('prepareBlockState — permission-group seed veto', () => {
11261150
expect(values.operation).toBe('send')
11271151
})
11281152

1129-
it('seeds nothing restricted when the gate answers no for an unknown config', () => {
1130-
/* Creation is one-shot: the caller vetoes both restricted fields while the
1131-
permission config is loading, since a value written here is never
1132-
revisited. Unrestricted fields still seed. */
1133-
const values = seededValues(
1134-
(subBlockId) => subBlockId !== 'operation' && subBlockId !== 'model'
1135-
)
1136-
expect(values.operation).toBeNull()
1137-
expect(values.model).toBeNull()
1138-
expect(values.channel).toBe('#general')
1139-
})
1140-
11411153
it('passes the seeded value to the gate, not just the field id', () => {
11421154
const seen: Array<[string, string]> = []
11431155
seededValues((subBlockId, value) => {

0 commit comments

Comments
 (0)