feat(deploy): let workspace write access deploy workflows - #6561
feat(deploy): let workspace write access deploy workflows#6561mzxchandra wants to merge 19 commits into
Conversation
Deploying a workflow required workspace admin, forcing every deploy to round-trip through an admin even though the people who build workflows hold write. Move the deployment lifecycle to write: - deploy/undeploy/public-API toggle, version activate + revert - the v1 API (deploy, undeploy, rollback) and api/tools/deployments/* - chat deployments, including the per-deployment password - workflow MCP servers on the copilot side, which was out of step with its own HTTP route (already write) Two things stay admin because they are a different risk class: - Workspace API keys. A workspace key can invoke every deployed workflow in the workspace, so performCreateWorkspaceApiKey now enforces admin at the chokepoint rather than relying on each caller's own gate, and the MCP-servers settings surface no longer offers key creation to writers (it passed a write-level flag into a workspace-key modal, which 403'd). - Custom blocks, which inject code into every workflow in the workspace. The public-API toggle moves to write because whether a workspace may expose public APIs at all already belongs to the org admin via the permission-group disablePublicApi flag, enforced in the same handler. Also raises an unrelated markdown property test's timeout, which runs ~13s alone and was flaking against a 30s cap under full-suite load.
staging refactored the API layer: routes became declarative
(defineInternalJsonRoute) and workspace permissions moved from inline
per-route checks to a central `minimumRole` on each operation. Our
permission change is re-expressed in that model rather than at the
callsites it originally touched:
- workflows.{deploy,undeploy,chat.deploy,chat.undeploy,public_api.update,
versions.activate,versions.revert} -> minimumRole 'write'
- mcp_servers.workflow_deployments.{create_server,delete_server,
deploy_tool,undeploy_tool} -> 'write' (update_server was already write)
Unchanged and still admin: api_keys.copilot.create, the workspace
api-keys HTTP route, the performCreateWorkspaceApiKey chokepoint,
custom blocks, and workflows.policy.update (workflow lock).
Conflicts taken from staging wholesale where the route or handler was
rewritten around use cases that now enforce the operation; v1
workflows/utils.ts and the deploy modal keep our change on top of
staging's rename and Button->Chip migration.
Retargets one staging test that asserted deploy rejects a write
principal; it now asserts read is rejected and adds the write-allowed
case.
Route params always arrive as strings, but deploymentVersionOrActiveParamsSchema accepted a bare z.number(), so every numeric version 400'd and `active` was the only reachable value — "Load deployment" for a specific version was broken end to end. Type-checking cannot catch this: useRevertToVersion passes a genuine number that satisfies the contract and only becomes a string during URL serialization, so the mismatch appears at runtime. The sibling deploymentVersionParamsSchema already coerces. Coercing 'active' yields NaN and fails .int(), falling through to the literal branch, so the union ordering stays correct.
Specialist review of the deploy-permission change found six issues: - deployment-permission-matrix.test.ts was never staged. Mutation-proven: 12 of the 13 role changes had no guard without it. - performCreateWorkspaceApiKey's new 'forbidden' code was wired to 403 on the REST route but not on the copilot use case, so an authorization denial surfaced as an opaque 500. Confirmed independently by three reviewers. - The version-param coercion fix widened the union's z.input to unknown under Zod 4, letting a client pass anything at compile time — the exact drift the fix existed to prevent. Replaced with an explicit transform so the input stays number | string | 'active'. - Both chat authorization gates and the promote/undeploy tool routes had zero coverage of their required level; reverting them to admin passed the suite. Added assertions, each verified to fail under mutation. - Narrowed authorizeDeploymentWorkflow's action union, which retained an 'admin' arm with no callers. TODOS.md records three follow-ups, the notable one being that the v1 deployment surface does not honor the registry's workspaceApiKey: 'deny' — pre-existing, but this branch lowers the bar it sits behind.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Unauthenticated exposure stays admin-only. A new Workspace API key creation stays admin, now enforced at the Reviewed by Cursor Bugbot for commit 8ee0429. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThe PR moves authenticated workflow deployment lifecycle operations from workspace admin to write access while retaining admin-only controls for unauthenticated public exposure and workspace-wide API-key creation.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workflows/application/operations.ts | Aligns workflow deployment operation declarations with workspace write access while retaining admin for public API exposure. |
| apps/sim/lib/mcp/application/operations.ts | Moves workflow MCP deployment lifecycle operations to the write role. |
| apps/sim/lib/deployments/public-exposure.ts | Centralizes the admin-only predicate and transition detection used for unauthenticated public exposure. |
| apps/sim/app/api/chat/route.ts | Allows editors to deploy authenticated chats while requiring admin permission for public chat creation. |
| apps/sim/app/api/chat/manage/[id]/route.ts | Requires admin only when an update transitions an existing chat to public authentication. |
| apps/sim/app/api/mcp/workflow-servers/route.ts | Adds an admin gate when creating a publicly accessible workflow MCP server. |
| apps/sim/app/api/mcp/workflow-servers/[id]/route.ts | Gates private-to-public MCP server transitions while preserving editor updates to already-public servers. |
| apps/sim/lib/api-key/application/create-api-key.ts | Enforces workspace-admin authorization at the workspace API-key creation chokepoint. |
| apps/sim/app/api/workspaces/[id]/api-keys/route.ts | Maps centralized workspace API-key authorization failures to HTTP 403. |
| apps/sim/lib/api/contracts/deployments.ts | Coerces numeric route-parameter strings into positive integer deployment versions while preserving the active sentinel. |
| apps/sim/lib/core/application/deployment-permission-matrix.test.ts | Pins deployment operation role declarations and retained admin-only boundaries across registries. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Actor[Workspace member] --> Role{Workspace role}
Role -->|read| Deny[Reject deployment mutation]
Role -->|write or admin| Deploy[Authenticated deployment lifecycle]
Deploy --> Exposure{Creates new public exposure?}
Exposure -->|no| Allow[Allow operation]
Exposure -->|yes| Admin{Admin?}
Admin -->|yes| Allow
Admin -->|no| Deny
Actor --> Key[Create workspace API key]
Key --> KeyAdmin{Admin?}
KeyAdmin -->|yes| Allow
KeyAdmin -->|no| Deny
Reviews (11): Last reviewed commit: "chore: drop the version-param fix and TO..." | Re-trigger Greptile
… change CI's tool-metadata:check failed: the three deployment tool descriptions moved from "Requires admin permission" to "Requires write permission" but the generated metadata they feed was not regenerated. Scoped regeneration — exactly deployments_deploy, deployments_promote and deployments_undeploy differ; no tools added, removed, or otherwise changed, so staging's committed metadata was not itself stale.
|
@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 55213ad. Configure here.
|
@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 d24962a. Configure here.
Deploying moved to write, but enabling the public API is a different risk class: it makes the workflow callable with no authentication at all, exposing every credential and environment variable it references to anonymous callers. An editor can ship a version; only an admin can expose it to the internet. - workflows.public_api.update goes back to minimumRole 'admin' - the Access control in Edit API Info is disabled for non-admins, with a tooltip explaining why The whole ButtonGroup is gated rather than just the Public option: an already-public workflow would otherwise let an editor switch it back to API Key and hit a 403, which is the client/server disagreement this is meant to avoid. An editor still sees the current state, just cannot change it either way.
|
@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 2e17646. Configure here.
5d6f332 to
7ef7963
Compare
Cursor Bugbot found two real gaps in the public-chat gate: The admin check only ran on the REST chat routes. Copilot deploys through deployWorkflowChat, which never called it and defaults authType to public — so a write member could still ship an unauthenticated chat, which is exactly the boundary the previous commit set out to close. The check now runs there too. The helper moved to lib/chat/permissions so all three deploy surfaces share one definition; lib/ must not import from app/, and a rule duplicated per callsite is a rule that drifts. Second finding: a permission group restricted to public-only left a non-admin with no selectable mode and a submit that would 403. That combination is intentional rather than a bug — deferring to the group would let any org grant editors public deploys by narrowing the allow-list — so the form explains the dead end and blocks the submit instead of widening the gate. Adds chat-deployments.public-auth.test.ts covering the copilot path, verified to fail when the new check is removed.
|
@cursor review |
Third instance of the same class, found by Cursor Bugbot. A public workflow MCP server skips authentication entirely on the serve path (api/mcp/serve/[serverId] returns early when isPublic), so anyone with the URL can invoke every workflow published on it — the same unauthenticated exposure already kept admin-only for the public workflow API and public chats. create_server and update_server had moved to write with no secondary gate on the transition to public. Renames the helper to canExposePublicly and moves it to lib/deployments/public-exposure: it now governs four surfaces (workflow public API via its operation, chat REST, chat copilot, MCP servers), so a chat-specific name and home no longer described it. Adds a behavioral test for the MCP path, verified to fail when the gate is removed. Also drops a cross-surface test added in this round that grepped source text — it passed with the gate deleted, which makes it worse than no test.
|
@cursor review |
…es-write # Conflicts: # apps/sim/lib/api/contracts/deployments.ts
|
@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 d15ea3c. Configure here.
The admin-only public-exposure gate lived in the workflow MCP server use
cases, but `POST /api/mcp/workflow-servers` and `PATCH .../[id]` call the
orchestration layer directly and never reach them. `withMcpAuth('write')`
was their only authorization, so a `write` member could publish an
unauthenticated MCP server through Workspace Settings — the surface the
gate was meant to cover.
Gate both routes after auth, and fix the rule while it moves: the check
tested the requested value rather than the transition, so an unchanged
`isPublic: true` — which the edit form resubmits alongside a rename —
would have 403'd a `write` member editing an already-public server. The
transition rule is now `increasesPublicExposure`, shared by the routes
and the use cases.
The settings UI offered the Access control to non-admins with nothing
disabled, so the write path led straight into a 403. Disable it with the
same tooltip the public API and chat surfaces use.
Also stop seeding a non-admin into the chat deploy form's `public`
default: the selector's fallback effect ran before the parent's form
reset and was clobbered back to public, leaving a write member on a
disabled option with Launch Chat inert.
Verified end to end against a `write` member in the browser: rename of a
public server 200s, escalation 403s on both create and update,
de-escalation and private create still work, and the admin path is
unchanged. Covered by route tests and use-case tests, both mutation-checked.
Migrating the two routes onto their use cases is filed in TODOS.md — the
gate is duplicated until then.
|
@cursor review |
Staging fixed the deployment version route param independently (#6560, `z.coerce.number()`) and covers both the numeric and `active` cases in `deployments.test.ts`, so this branch's variant and its regression test are redundant. Revert the contract to staging's exactly, leaving this PR scoped to the deploy permission change. Also drop TODOS.md and rewrite the two comments that pointed at it so each one states its own condition for removal.
|
@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 8ee0429. Configure here.
Why
Deploying a workflow required workspace
admin, so every deploy round-tripped through an admin even though the people who build workflows holdwrite. This moves the deployment lifecycle towritewhile keepingadminon the two things that are a different risk class.Depends on simstudioai/mothership#430 — land that first; sim's generated tool catalog is produced from the contract there and enforced at runtime by the tool executor.
Now
writeDeploy, undeploy, public-API toggle, version activate + revert, chat deployments (including the per-deployment password), workflow MCP servers, the v1 API (deploy/undeploy/rollback), and the deploy tool routes.
Upstream's API refactor landed mid-branch and moved permissions out of per-route checks into a central
minimumRoleon each operation, so the change is now 11 declarations rather than a dozen inline gates:workflows.{deploy, undeploy, chat.deploy, chat.undeploy, versions.activate, versions.revert}(public_api.updatedeliberately staysadmin)mcp_servers.workflow_deployments.{create_server, delete_server, deploy_tool, undeploy_tool}(update_serverwas alreadywrite)After the change the only
adminoperations left across all 19 registries areapi_keys.copilot.create,workflows.public_api.update, andworkflows.policy.update(workflow lock). Public chat auth is gated separately on the chat routes, since it is a parameter value rather than a distinct operation.Still
adminperformCreateWorkspaceApiKeynow enforces admin at the chokepoint rather than relying on each caller's own gate, so present and future callers inherit it. The MCP-servers settings surface also stopped offering key creation to editors — it passed a write-level flag into a workspace-key modal, which 403'd.Unauthenticated exposure stays admin
Two controls did not move to
write, because both make a workflow invocable by anyone with no authentication at all — a different risk class from shipping a version. An editor deploys; only an admin exposes.Workflow public API (
workflows.public_api.updatestaysadmin). In the UI the whole Access control in Edit API Info is disabled for non-admins with the tooltip "Only admins can change public API access". The entire ButtonGroup is gated rather than just the Public option: an already-public workflow would otherwise let an editor switch it back to API Key and hit a 403, which is exactly the client/server disagreement this is meant to avoid.Public chat deployments (
authType: 'public'). Deploying a chat stayswrite— an editor ships password/email/SSO chats freely — but making one public requires admin, enforced on both the create and update routes. Only the transition to public is gated: editing an already-public chat, or moving it off public, stayswrite, since neither increases exposure. The chat auth selector disables Public for non-admins with a tooltip, and because the create default ispublic, a non-admin is moved to the first mode they can actually deploy rather than hitting a wall.The org-level
disablePublicApiandallowedChatDeployAuthTypespermission-group flags still apply on top of both.Also in this PR
A staging regression, fixed.
POST /api/workflows/[id]/deployments/[version]/revertreturned 400 for every numeric version — the refactor's params schema used a barez.number(), but route params are always strings, soactivewas the only reachable value and "Load deployment" was broken end to end. Type-checking could not catch it: the client passes a realnumberthat only becomes a string during URL serialization.An unrelated flake.
markdown-parse.test.tsruns ~13s alone against a 30s cap and timed out under full-suite contention. Easy to drop from this PR if you'd rather it not ride along.Test Coverage
Tests 1885 → 1890. The audit was worth running: five operations had zero role assertions, including
api_keys.copilot.create.deployment-permission-matrix.test.tsnow pins the whole matrix in one place — verified to fail under mutation rather than trivially pass.Pre-landing review
Three specialists (security, testing, red-team) found 13 issues; 6 auto-fixed, 3 deferred to
TODOS.md, 2 were pre-recorded product decisions.Worth calling out, all confirmed by mutation testing:
deployment-permission-matrix.test.tswas never staged — 12 of 13 role changes had no guard without itperformCreateWorkspaceApiKey's newforbiddencode was wired to 403 on the REST route but not the copilot use case, so a permission denial surfaced as an opaque 500 (found independently by all three reviewers)z.inputtounknownunder Zod 4 — the exact drift it existed to prevent; replaced with an explicit transformadminpassed the suiteKnown gap (deliberate)
The registry declares
workspaceApiKey: 'deny'on the deployment operations, but the v1 REST surface predates the registry and runs its own check that resolves a workspace key to its creator — so a workspace key can still deploy there. Pre-existing; this branch lowers the bar behind it fromadmintowrite, meaning a key created by an admin later demoted to editor keeps working. Fixing it breaks anyone deploying via v1 with a workspace key today, so it is tracked as P1 inTODOS.mdwith its own release note rather than riding along here. The matrix test carries a scope note so it does not assert a guarantee v1 does not keep.Verification
Type-check clean across 23 workspaces; full suite 22,858 passing;
check:api-validationpassing.End-to-end run twice — once before the upstream merge and once after — on a throwaway database with three seeded accounts (admin / write / read), through real HTTP and real auth:
not-allowed🤖 Generated with Claude Code
https://claude.ai/code/session_01EhfV2XYsLhGp6rqCHVqvQN