fix(security): authorize MCP subagent IDs, oauth workspace, credential admin demotion#4551
Conversation
…l admin demotion - handleSubagentToolCall and handleDirectToolCall now authorize user-supplied workflowId/workspaceId via authorizeWorkflowByWorkspacePermission / ensureWorkspaceAccess before forwarding downstream; resolvedWorkspaceId is derived from the authorized workflow record instead of trusted from the body - executeOAuthGetAuthLink verifies caller membership (write level) on the target workspaceId before generating the OAuth link or writing pendingCredentialDraft, closing the cross-workspace credential injection path - POST /api/credentials/[id]/members wraps role updates in a transaction that counts active admins and rejects demotion of the last admin (mirrors the existing DELETE guard in the same file) - GET /api/credentials/[id]/members returns uniform 404 for both missing and inaccessible credentials to remove the existence oracle
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Hardens credential membership management by preventing demotion/removal of the last active admin via transactional row locks, and reduces credential existence leakage by returning uniform Adds a workspace membership check before generating OAuth auth links ( Reviewed by Cursor Bugbot for commit c0d9961. Configure here. |
Greptile SummaryThis PR closes three high-severity authorization gaps in the copilot/MCP layer and credential-member API. No new features are introduced — all changes are security fixes.
Confidence Score: 5/5All changed paths correctly fence user-supplied IDs behind server-side authorization checks before any DB writes or downstream forwarding. The MCP route now derives workspaceId from the DB record rather than trusting user input; the OAuth handler guards its workspace write with an explicit membership check; and the credential demotion path correctly serializes concurrent role changes with row-level locks inside a transaction. The cosmetic key-literal cleanup in tool-schemas-v1.ts carries no risk. No unguarded code paths remain in the changed files. No files require special attention; all security fixes follow through consistently within their respective files. Important Files Changed
Reviews (2): Last reviewed commit: "fix(security): address PR review — activ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…cks, workspaceId propagation
- credentials/members POST: add `current.status === 'active'` check to the
last-admin demotion guard so re-inviting a revoked admin as a non-admin role
no longer incorrectly hits the "Cannot demote the last admin" path
- credentials/members POST+DELETE: add `.for('update')` to the active-admin
count SELECT inside both transactions to serialize concurrent demotions and
eliminate the admin-count TOCTOU race under Postgres READ COMMITTED
- credentials/members POST: also lock the member row itself with `.for('update')`
so the role+status read and the subsequent UPDATE are atomic
- mcp/copilot handleDirectToolCall: thread the DB-verified workspaceId from the
authorization result into prepareExecutionContext instead of relying on
user-supplied args
- oauth handler: fix error message to mention both workspaceId and userId when
either is missing from the execution context
|
@cursor review |
|
@greptile |
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 c0d9961. Configure here.
1 participant
Summary
handleSubagentToolCallandhandleDirectToolCallwere passing user-suppliedworkflowId/workspaceIddirectly downstream without authorization. Now authorizes both viaauthorizeWorkflowByWorkspacePermission(for workflow IDs) andensureWorkspaceAccess(for workspace-only IDs).resolvedWorkspaceIdis always derived from the server-trusted workflow record, not the request body.executeOAuthGetAuthLinkusedcontext.workspaceId(ultimately user-supplied via MCP args) without verifying the caller is a member. AddedensureWorkspaceAccess(workspaceId, userId, 'write')before generating the OAuth link or writingpendingCredentialDraft.POST /api/credentials/[id]/memberswas blindly updatingrolewithout checking if it would leave zero admins. Wrapped the update in a transaction that counts active admins and returns400 Cannot demote the last adminwhen the count would drop to zero (mirrors the identical guard already in the DELETE handler).GET /api/credentials/[id]/membersreturned200 { members: [] }for missing credentials but403for existing inaccessible ones, leaking existence. Now returns uniform404for both cases.Follow-up (out of scope for this PR)
Several other copilot handlers (
jobs.ts,function-execute.ts,materialize-file.ts,management/manage-mcp-tool.ts,management/manage-skill.ts,management/manage-custom-tool.ts) usecontext.workspaceIdwithout verifying membership — filed as a follow-up sweep. The MCP boundary fix in this PR closes the most exploitable ingress path.Type of Change
Testing
Tested manually
Checklist