fix: enforce project_ref scope on destructive branch tools - #351
Conversation
When the MCP server is configured with projectId / project_ref, create_branch and list_branches already inject that project, but delete_branch, merge_branch, reset_branch, and rebase_branch accepted any branch_id and forwarded it to the Management API without checking parent_project_ref. Before mutating, resolve the scoped project's branches and require the target branch id (or project_ref) to belong to that project. Unscoped servers keep org-wide behavior. Adds regression tests for cross-project rejection and same-project delete.
barryroodt
left a comment
barryroodt
left a comment
There was a problem hiding this comment.
Thanks for the update @mansueli, but I don't think this is ready yet. The guard closes the reported foreign-branch path for a parent-scoped server, but it assumes every projectId identifies a branching parent. The server also accepts a development branch's project_ref as its scope.
That assumption produces two failures: a branch-scoped server rejects its own branch, while a parent-scoped server authorizes the default production ref through the same predicate.
I'd resolve the scope once in src/tools/branching-tools.ts, behind one scoped branching helper shared by all four handlers. If BranchingOperations cannot supply the required ownership facts, extend its interface in src/platform/types.ts and implement the Management API lookup in src/platform/api-platform.ts; authorization should remain in branching-tools.ts.
Then update src/server.test.ts and test/mocks.ts to include the default branch row and assert that every destructive platform method remains uncalled after rejection.
| return; | ||
| } | ||
|
|
||
| const branches = await branching.listBranches(projectId); |
There was a problem hiding this comment.
listBranches(projectId) answers “which branch rows have this parent?”, so it works only when projectId identifies a parent project.
When the server is scoped to a branch's own project_ref, this call returns no children and every destructive tool rejects that branch. In the parent-scoped case, the returned list includes a default row with project_ref === projectId and is_default === true; the predicate therefore treats the production ref as a valid development branch. I reproduced both paths, including reset_branch reaching its platform mutation for the default ref.
Can we move this behind an explicit scope model? Parent scope should allow matching non-default children. Branch scope should allow only itself. Unscoped behavior should remain unchanged.
The direct branch-detail response lacks parent_project_ref and is_default, so replacing this list call with that endpoint alone will not prove ownership.
| parent_project_ref: projectB.id, | ||
| }); | ||
|
|
||
| // Also create a same-project branch so list_branches is non-empty for A |
There was a problem hiding this comment.
These fixtures call the low-level createBranch helper, which creates only a non-default row. The normal mocked create_branch flow also creates a default row with project_ref === parent_project_ref === projectId. That is the row which exposes the production-ref hole above.
The regression suite therefore tests a branch-list shape that normal creation does not produce. The final mockBranches.has(...) assertion also proves non-invocation only for deleteBranch. The remaining mutation methods could run before the check and still satisfy rejects.toThrow(scopeError).
Can we build a production-shaped list and spy on every destructive method, asserting zero calls after rejection?
2 participants
Summary
When the MCP server is scoped with
project_ref/projectId,create_branchandlist_branchesalready inject that project ID. Destructive branch tools (delete_branch,merge_branch,reset_branch,rebase_branch) only accepted a caller-controlledbranch_idand forwarded it to the Management API with no check that the branch’sparent_project_refmatched the scoped project.That breaks the documented project-scoping guarantee: a client scoped to Project A could mutate a development branch belonging to Project B if both were covered by the same org OAuth grant and the caller knew the foreign
branch_id.Fix
Before any destructive branch mutation, if the server is project-scoped:
branch_idmatches a branchidorproject_refunder that projectUnscoped servers keep existing org-wide behavior.
Test plan
branch_idfor delete/merge/reset/rebase and does not delete the foreign branchcreate/list/merge/reset/rebase/ read-only)Notes
GET /v1/branches/{branch_id_or_ref}returnsBranchDetailResponsewithoutparent_project_ref, so ownership is validated via the project’s branch list (same filter aslist_branches).