Skip to content

fix: enforce project_ref scope on destructive branch tools #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mansueli wants to merge 1 commit into
base: main
Choose a base branch
from
+165 −0
Open

fix: enforce project_ref scope on destructive branch tools #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions packages/mcp-server-supabase/src/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4047,6 +4047,121 @@ describe('project scoped tools', () => {
}),
]);
});

test('destructive branch tools reject branches owned by another project', async () => {
const org = await createOrganization({
name: 'My Org',
plan: 'free',
allowed_release_channels: ['ga'],
});

const projectA = await createProject({
name: 'Project A',
region: 'us-east-1',
organization_id: org.id,
});
projectA.status = 'ACTIVE_HEALTHY';

const projectB = await createProject({
name: 'Project B',
region: 'us-east-1',
organization_id: org.id,
});
projectB.status = 'ACTIVE_HEALTHY';

const foreignBranch = await createBranch({
name: 'disposable-branch',
parent_project_ref: projectB.id,
});

// Also create a same-project branch so list_branches is non-empty for A

@barryroodt barryroodt Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// and we can confirm same-project ops still work when needed.
await createBranch({
name: 'owned-branch',
parent_project_ref: projectA.id,
});

const { callTool } = await setup({
projectId: projectA.id,
features: ['branching'],
});

const listed = await callTool({
name: 'list_branches',
arguments: {},
});
expect(listed.branches).not.toContainEqual(
expect.objectContaining({ id: foreignBranch.id })
);
expect(listed.branches).toContainEqual(
expect.objectContaining({ parent_project_ref: projectA.id })
);

const scopeError = `Branch '${foreignBranch.id}' is not a development branch of the scoped project '${projectA.id}'.`;

await expect(
callTool({
name: 'delete_branch',
arguments: { branch_id: foreignBranch.id },
})
).rejects.toThrow(scopeError);

await expect(
callTool({
name: 'merge_branch',
arguments: { branch_id: foreignBranch.id },
})
).rejects.toThrow(scopeError);

await expect(
callTool({
name: 'reset_branch',
arguments: { branch_id: foreignBranch.id },
})
).rejects.toThrow(scopeError);

await expect(
callTool({
name: 'rebase_branch',
arguments: { branch_id: foreignBranch.id },
})
).rejects.toThrow(scopeError);

// Foreign branch must still exist — delete must not have run
expect(mockBranches.has(foreignBranch.id)).toBe(true);
});

test('destructive branch tools still work for branches of the scoped project', async () => {
const org = await createOrganization({
name: 'My Org',
plan: 'free',
allowed_release_channels: ['ga'],
});

const project = await createProject({
name: 'Project A',
region: 'us-east-1',
organization_id: org.id,
});
project.status = 'ACTIVE_HEALTHY';

const ownedBranch = await createBranch({
name: 'owned-branch',
parent_project_ref: project.id,
});

const { callTool } = await setup({
projectId: project.id,
features: ['branching'],
});

await callTool({
name: 'delete_branch',
arguments: { branch_id: ownedBranch.id },
});

expect(mockBranches.has(ownedBranch.id)).toBe(false);
});
});

describe('docs tools', () => {
Expand Down
50 changes: 50 additions & 0 deletions packages/mcp-server-supabase/src/tools/branching-tools.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,36 @@ export const branchingToolDefs = {
},
} as const satisfies ToolDefs;

/**
* When the MCP server is scoped to a single project, destructive branch tools
* must not accept branch IDs that belong to another parent project.
*
* create_branch / list_branches already inject project_id; delete/merge/reset/
* rebase only take branch_id and previously forwarded it unchecked.
*
* branch_id_or_ref may be either the branch UUID or the branch project_ref.
*/
async function assertBranchBelongsToScopedProject(
branching: BranchingOperations,
branchId: string,
projectId: string | undefined
) {
if (!projectId) {
return;
}

const branches = await branching.listBranches(projectId);

@barryroodt barryroodt Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const belongs = branches.some(
(branch) => branch.id === branchId || branch.project_ref === branchId
);

if (!belongs) {
throw new Error(
`Branch '${branchId}' is not a development branch of the scoped project '${projectId}'.`
);
}
}

export function getBranchingTools({
branching,
projectId,
Expand Down Expand Up @@ -191,6 +221,11 @@ export function getBranchingTools({
throw new Error('Cannot delete a branch in read-only mode.');
}

await assertBranchBelongsToScopedProject(
branching,
branch_id,
project_id
);
await branching.deleteBranch(branch_id);
return { success: true };
},
Expand All @@ -202,6 +237,11 @@ export function getBranchingTools({
throw new Error('Cannot merge a branch in read-only mode.');
}

await assertBranchBelongsToScopedProject(
branching,
branch_id,
project_id
);
await branching.mergeBranch(branch_id);
return { success: true };
},
Expand All @@ -213,6 +253,11 @@ export function getBranchingTools({
throw new Error('Cannot reset a branch in read-only mode.');
}

await assertBranchBelongsToScopedProject(
branching,
branch_id,
project_id
);
await branching.resetBranch(branch_id, {
migration_version,
});
Expand All @@ -226,6 +271,11 @@ export function getBranchingTools({
throw new Error('Cannot rebase a branch in read-only mode.');
}

await assertBranchBelongsToScopedProject(
branching,
branch_id,
project_id
);
await branching.rebaseBranch(branch_id);
return { success: true };
},
Expand Down
Loading