fix(access-control): default every operation and model picker to one the permission group allows - #6720
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview New shared layer: UI: Block editor operation dropdown and agent tool picker hide denied operations, default to the first allowed option, and disable while loading. Model combobox uses consolidated Creation:
Reviewed by Cursor Bugbot for commit 32b8c89. Configure here. |
Greptile SummaryThe PR centralizes permission-aware operation and model defaulting so one-shot writes wait for permission configuration and select only usable defaults.
Confidence Score: 5/5The PR appears safe to merge because the previously reported loading-time model and preset-operation bypasses are corrected and no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/hooks/use-operation-access.ts | Centralizes loading-aware operation filtering, default resolution, and operation/model seed vetoes; the previously reported model-loading issue is addressed. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx | Applies the centralized gate to declared defaults and operation presets during every block-creation path, addressing both prior bypass findings. |
| apps/sim/lib/permission-groups/operation-access.ts | Adds non-guessing operation-to-tool resolution and first-allowed default selection with permissive handling for unresolvable operations. |
| apps/sim/hooks/use-permission-config.ts | Consolidates model and provider checks into an indexed isModelUsable predicate. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx | Defers operation defaulting and disables persisted operation selection while permission configuration is loading. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx | Filters denied agent-tool operations, chooses the first permitted operation, and closes one-shot tool selection while permissions are unknown. |
| apps/sim/stores/modals/search/store.ts | Removes operation search entries whose resolved tools are denied before they can seed newly created blocks. |
| apps/sim/stores/workflows/utils.ts | Allows block preparation to veto nonempty string defaults through the centralized permission seed gate. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Create block or choose preset] --> B{Permission config ready?}
B -- No --> C[Leave operation/model empty]
B -- Yes --> D{Requested value allowed?}
D -- Yes --> E[Persist requested value]
D -- No --> F[Choose first allowed option]
C --> G[Mounted picker reconciles after load]
F --> H[Persist allowed default]
G --> H
Reviews (6): Last reviewed commit: "fix(access-control): only gate a model f..." | Re-trigger Greptile
770e103 to
698a8b3
Compare
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f42bda6. Configure here.
|
@cursor review |
…allows The block editor's operation dropdown already hid operations whose tool the caller's permission group denies, but it seeded its default without waiting for that config. `usePermissionConfig` resolves as "nothing denied" while its query is in flight, so a freshly dropped block persisted the static first operation — and nothing revisits a field that already holds a value, so the correction that arrived with the config never applied. A user whose group denies `slack_message` still got a Slack block sitting on Send Message. The model combobox already had this guard; the dropdown did not. Consolidates the rule behind `lib/permission-groups/operation-access` and `useOperationAccess`, which resolves an operation to its tool without guessing (an unresolvable one stays visible; the server gate stays authoritative) and withholds a default until the config has loaded, so seeding on a defined value is the whole guard. Applies it to every surface that offers or seeds an operation: - block editor dropdown — default now waits for the config - agent block tool list — operations were not gated at all; the picker now hides denied ones, drops blocks whose every operation is denied, and defaults to the first allowed - canvas search / connection picker — the tool-operation index was filtered only by the block allowlist, so denied operations were still offered as one-click block drops - block creation — a declared default operation the group denies is replaced with the first allowed one, and a denied preset operation is discarded
… rule
Block creation seeds `model` the same way it seeds `operation`: `agent`,
`router` and `evaluator` all declare `defaultValue: 'claude-sonnet-5'`, and
`prepareBlockState` wrote it unconditionally. The model combobox only fills a
field that is empty, so a group denying that model (or the Anthropic provider)
got an Agent block pre-filled with a model it cannot run — the same bug as the
operation one, on the other axis the permission group governs.
Rather than a second bespoke gate, `prepareBlockState` now takes one veto,
`isSeededValueAllowed(subBlockId, value)`, and seeds nothing when a declared
default is denied. Nothing substitutes a replacement there any more: the
editor's own permission-aware pickers already resolve the right one and only
fill an empty field, and substituting in the store would drift from
`getDefaultBlockName`, which names a block after its *declared* default. That
also deletes `firstAllowedOperation` and its copy of subblock-option
enumeration.
Review follow-ups:
- `usePermissionConfig` gains `isModelUsable` (denylist AND provider allowlist);
the combobox's two hand-rolled copies of that pair now call it
- `isToolAllowed`/`isModelAllowed` index their denylists — the gate calls them
once per option of every block offered, so a linear scan made a check's cost
scale with denylist length (measured 3.2ms -> 0.30ms per search-index build
at 500 denied tools)
- `useOperationAccess` had three members with three different loading
semantics, one documented as unsafe alone; it now exposes one withholding
`resolveOperationGate`
- the agent tool picker derived its option list twice with the empty-id filter
on only one path; both callers now share one `{ options, denied }` result
- `OPERATION_SUBBLOCK_ID` was a verbatim copy of the private constant in
`canvas-sentence.ts`, doc comment included; that file now imports it
Cleanup-pass findings on this branch's own lines: comments that restated the code they sat on, a four-line note whose sibling said the same in one, and a `?? undefined` on a non-nullable value. The comment on the tool-picker filter now explains the clause that actually needed it (an empty option list is not a denied one) instead of narrating the filter. Left alone as pre-existing and out of scope: the inline `staleTime` literal in `useAllowedIntegrationsFromEnv`, and the Operation selector's raw label / plain `Combobox` — both byte-identical to staging and matching the convention of every sibling field in that panel.
Block creation is one-shot, so the withholding pattern the editor's pickers use does not transfer: withholding the predicate there meant `prepareBlockState` seeded the declared `operation`/`model` defaults unchecked, and nothing revisits a field that already holds a value — so a block added before the permission config resolved kept a model the group may deny. Both restricted fields now seed empty until the config is known; the pickers fill them the moment it resolves. A preset operation is still honoured in that window: unlike a declared default it is the user's explicit pick, and the server gates the run.
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.
f42bda6 to
75face5
Compare
|
@cursor review |
…s about The seed gate ran `isModelUsable` on every subblock named `model`, but `getProviderFromModel` resolves chat models and falls back to `ollama` for everything else. 28 of the 44 seeded model defaults in the registry are embedding, speech, image, video or search ids — so for any group with a provider allowlist that omits Ollama, those blocks were created with an empty model. Adds `findProviderFromModel`, the non-guessing half of `getProviderFromModel`, which returns `null` where the registry declares nothing. `isModelUsable` now treats an unresolved id as not-a-provider-choice and leaves it alone, matching the rule the operation gate already follows: never guess, and let the server stay authoritative. `getProviderFromModel` delegates to it, so there is one resolution path and its ollama fallback is unchanged. This also repairs the same misjudgement where it predates the branch: the model combobox filtered its options through the identical provider check, so those 28 defaults were already being hidden from their own pickers for allowlisted groups. The dead `try/catch` around the old call went with it — `getProviderFromModel` returns a fallback rather than throwing for an unknown id.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 32b8c89. Configure here.
Summary
usePermissionConfigreads as "nothing denied" while in flight, so a freshly dropped block persisted the static first operation, and nothing revisits a field that already holds a value. A group denyingslack_messagestill got a Slack block sitting on Send Message.agent,routerandevaluatordeclaredefaultValue: 'claude-sonnet-5',prepareBlockStatewrote it unconditionally, and the model combobox only fills an empty field — so a group denying that model (or the Anthropic provider) got an Agent block pre-filled with a model it can't run.lib/permission-groups/operation-access+useOperationAccess, which resolves an operation to its tool without guessing (unresolvable stays visible; the server gate stays authoritative).The governing rule
A one-shot persisting write must never read a predicate that answers "nothing denied" while the permission config is loading. Display filtering stays optimistic and self-corrects; anything persisted is withheld until the config is known.
That rule was originally re-implemented per callsite and got missed twice, so it now lives in one place:
useOperationAccess.resolveSeedGateowns the creation-time veto for both restricted fields —workflow.tsxstates no policy of its own, it asks for a gate and passes it toprepareBlockState.filterBlocks, operations, MCP and custom tools — reads as unrestricted for that beat, and each pick writes in one shot.isPermissionLoadingis exposed from one hook so every surface reads the same symbol.Surfaces covered
Notes
dropcontact,enrichment,sqs) are now gated where they weren't. Because they're single-tool, denying the tool denies every operation, so the picker empties rather than pre-filling.usePermissionConfiggainsisModelUsable(model denylist AND provider allowlist) and dropsisModelAllowed/isProviderAllowed, which had no remaining external consumer once the combobox consolidated onto it. Both denylists are now indexed — the gate calls them once per option of every block offered, so a linear scan made a check's cost scale with denylist length (3.2ms → 0.30ms per search-index build at 500 denied tools).edit-workflowvalidation checks operations against the block's option list but not againstdeniedTools, so it can still author a denied operation. Execution blocks it server-side. Left as a follow-up — it needsuserId/workspaceIdplumbed into the server validator.Type of Change
Testing
bun run type-checkclean,bun run check:audits26/26, 16,596 tests passingChecklist