fix(copilot): close fail-open write gates in agent tools - #6132
fix(copilot): close fail-open write gates in agent tools#6132TheodoreSpeaks wants to merge 4 commits into
Conversation
The three handler-map management tools (manage_custom_tool, manage_mcp_tool, manage_skill) gated writes with `context.userPermission && perm !== 'write' && perm !== 'admin'`. userPermission is optional on the execution context, so an absent value skipped the check entirely and the write proceeded unguarded — while the server-tool router's equivalent gate is fail-closed. Both paths now share copilotToolCanWrite, built on the canonical permissionSatisfies (null/undefined never satisfies). manage_custom_tool additionally resolved its target as `params.workspaceId || context.workspaceId`, so a model-supplied workspace id won while the permission check was resolved for the context workspace — and upsertCustomTools performs no authz of its own. It now uses the server-set context only, matching its two siblings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CiHhAk2R1NryaS3R8n2yFz
…ation lock materialize_file's save/extract/import all create workspace resources, but the handler-map path has no central permission check, so a read-only member could create files and workflows through the agent. Gate on write access after param validation. assertWorkflowMutable moves into performFullDeploy / performFullUndeploy / performActivateVersion, where performRevertToVersion already had it. The check previously lived only in the deploy routes, so the copilot deploy tools — which call the orchestration functions directly — could deploy, undeploy, and activate versions of a locked workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@greptile review |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR closes authorization, mutation-lock, secret-management, and usage-accounting gaps in Copilot write paths.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/permissions.ts | Introduces a shared fail-closed write predicate and consistent denial-message formatter. |
| apps/sim/lib/copilot/tools/handlers/management/manage-custom-tool.ts | Restricts writes to the authenticated context workspace and applies the shared write gate. |
| apps/sim/lib/copilot/tools/handlers/materialize-file.ts | Requires workspace write access before reading uploads or creating files and workflows. |
| apps/sim/lib/copilot/tools/server/router.ts | Replaces the router’s local permission check with the shared fail-closed predicate. |
| apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts | Checks the attributed payer’s usage limit before resolving files or starting indexing. |
| apps/sim/lib/environment/utils.ts | Adds key-level authorization, serialized environment mutation, credential maintenance, cache invalidation, and auditing. |
| apps/sim/lib/workflows/orchestration/deploy.ts | Adds orchestration-level mutation-lock enforcement while preserving result-object error handling. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Copilot tool request] --> B{Write permission satisfied?}
B -- No --> X[Deny request]
B -- Yes --> C{Operation type}
C --> D[Materialize file]
C --> E[Deploy workflow]
C --> F[Update environment]
C --> G[Index knowledge file]
D --> H[Require workspace write access]
E --> I[Assert workflow mutable]
F --> J[Check workspace and secret-admin access]
J --> K[Lock, update, audit]
G --> L[Check attributed usage limits]
Reviews (4): Last reviewed commit: "fix(copilot): return lock denials and st..." | Re-trigger Greptile
… on usage Two more paths where the agent is a weaker route to the same write than the UI. upsertWorkspaceEnvVars was a weakened copy of the environment route's write: no per-key secret-admin check, no advisory lock, no audit row. Its only caller is the set_environment_variables copilot tool, which gates on workspace 'write' alone — so any write-level member could have the agent overwrite a workspace secret they do not administer, with nothing in the audit log and a lost-update race against the route's locked transaction. The gate now lives in the function so every caller inherits it, reusing getWorkspaceEnvKeyAdminAccess rather than restating the route's logic. knowledge_base add_file computed a billing attribution and then indexed without calling checkAttributedUsageLimits, which every upload route applies before accepting indexing work — an over-quota workspace could index without limit through the agent. The same file's query operation already gated correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@greptile review |
|
@cursor review |
PR SummaryHigh Risk Overview Copilot write gates now use shared
Deploy orchestration (
Knowledge Tests cover permissions fail-closed behavior, materialize write gate, env upsert rules, KB usage gate, and deploy lock denials. Reviewed by Cursor Bugbot for commit b1103e3. Bugbot is set up for automated code reviews on this repo. Configure here. |
…ret's ACL
Two defects in the previous commits, both found by review.
assertWorkflowMutable throws, and the three orchestration entry points awaited
it outside any try/catch, so a locked workflow escaped as an exception instead
of the { success: false } result the callers consume — performChatDeploy and
the copilot deploy tools would have surfaced a generic 500 rather than a lock
denial. performRevertToVersion already converted it; the three now do too,
through a shared helper.
upsertWorkspaceEnvVars derived newKeys for createWorkspaceEnvCredentials from
the credential rows rather than the stored variables. A secret written before
credential rows existed has no ACL, so overwriting it looked like adding a new
key: it minted a credential and made the caller that secret's admin. The
environment route derives newKeys from the locked jsonb read for exactly this
reason, and now so does this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@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 b1103e3. Configure here.
Five authorization/accounting gaps on the copilot tool paths. In each case the agent is a weaker route to the same write than the UI — a workspace member gets more through Sim than the product intends to give them. Found while auditing which resources have business logic outside
lib/[resource]/orchestration; these are the authz half of that audit, and the orchestration migrations stack separately on top of #5273.1. Write gates failed open on absent permission
manage_custom_tool,manage_mcp_tool,manage_skill, and the server-tool router each hand-rolled the same ladder:userPermissionis optional on the execution context, soundefined,null, and''skipped the check entirely and the write went through unguarded. Replaced with a sharedcopilotToolCanWrite()built onpermissionSatisfiesfrom@sim/platform-authz/workspace, which never satisfies on a null permission. Both copilot execution paths (server-tool router and handler map) now share one definition of "write access" and one denial message.manage_custom_toolalso readparams.workspaceId || context.workspaceId, letting a caller-supplied workspace id override the authenticated one — the permission was resolved for the context workspace but the write landed in whichever workspace the params named. It now uses the context workspace only.2.
materialize_filehad no permission check at allAll three of its operations write:
saveandextractcreate workspace files,importcreates a workflow. Unlike the server-tool router, the handler-map path has no central gate, so nothing stopped a read-only member from creating resources. AddedensureWorkspaceAccess(..., 'write')after param validation (so a bogus operation still fails cheaply without a DB read).3. Deploy orchestration didn't enforce the mutation lock
assertWorkflowMutablelived in the deploy routes, soperformRevertToVersionwas the only orchestration entry point that couldn't be called past a lock. The copilot deploy tools callperformFullDeploy/performFullUndeploy/performActivateVersiondirectly and could therefore deploy, undeploy, and activate versions of a locked workflow. The assert moves into all three functions; routes may still assert first to render their own 423, and this is the backstop that makes the guarantee hold regardless of caller.4. Workspace secrets could be overwritten without secret-admin
upsertWorkspaceEnvVarswas a weakened copy of the environment route's write path: no per-key secret-admin check, no advisory lock, no audit row. Its only caller is theset_environment_variablescopilot tool — which is not in the router'sWRITE_ACTIONS, so its only gate is workspacewrite.Net effect: any write-level member could ask the agent to overwrite a workspace secret they are not an admin of, leaving no audit record, racing the route's locked transaction on a read-modify-write of the same jsonb column. The route (
app/api/workspaces/[id]/environment/route.ts) enforces all three.The gate now lives inside
upsertWorkspaceEnvVarsso every caller inherits it, reusing the route's owngetWorkspaceEnvKeyAdminAccesshelper rather than restating its logic. Denials throwWorkspaceEnvAccessError.5. Knowledge-base indexing skipped the usage gate
knowledge_base'sadd_filecomputed a billing attribution and then went straight tocreateSingleDocumentwithout callingcheckAttributedUsageLimits— the gate every upload route applies before accepting indexing work. An over-quota workspace could index without limit through the agent. Thequeryoperation in the same file already gated correctly, so this was an inconsistency within one tool rather than a missing helper.Tests
permissions.test.tspins the fail-closed contract forundefined/null/''/read/unrecognized values.materialize-file.test.tsasserts each operation is refused without write access, and that no upload is read before the gate.environment/utils.test.tscovers the four gate decisions (non-admin overwrite denied, new key without write denied, key admin allowed, workspace admin allowed) plus the empty-update no-op.knowledge-base.test.tsasserts an over-quota payer is refused before any file is resolved or document created.bunx vitest run lib/copilot lib/environment lib/workflows/orchestration→ 1043 pass. Fullbun run lint:checkandbun run type-checkclean.🤖 Generated with Claude Code