fix: gate MCP server actions and mutations on org membership and role (#2040)

[larryro]

Closes #2040

Problem

The MCP-server backend exposed several public Convex actions/mutations that checked only authentication (or nothing at all), giving any authenticated user in any org a cross-tenant primitive:

• executeMcpTool (actions.ts) had no auth check and was a public api.* action. It fetched another org's full server doc (including encrypted credentials) and executed a named tool against server.url with the decrypted bearer/OAuth secret — a cross-tenant credential-use + SSRF primitive.
• testConnection (actions.ts) checked only getAuthUserIdentity, then ran discovery against another org's server with its stored credentials and persisted status/token updates.
• create / update / remove / updateStatus (public_mutations.ts) checked only getAuthUserIdentity; create trusted a client-supplied organizationId, the id-based ones never confirmed the server belonged to the caller's org.

Fix

Mirrors the proven pattern from the sibling sandbox admin surface (node_only/sandbox/session_admin_actions.ts) and the MCP read queries (queries.ts):

• executeMcpTool → internalAction. Its only callers are trusted backend paths (agent_tools/mcp/create_bound_mcp_tool.ts and mcp_servers/execute_approved.ts), now invoked via internal.mcp_servers.actions.executeMcpTool. This removes the public exposure entirely — closing the SSRF/credential-use hole.
• testConnection and public_mutations.* stay public (they are wired to the frontend via useAction) but are now gated server-side with requireOrgAdminOrDeveloper:
  • create enforces the capability on the supplied organizationId.
  • update / remove / updateStatus / testConnection resolve the owning org from the stored server document and gate on that, so the ownership match is implicit — a member of another org cannot touch the server (the requireSessionInOrg defense-in-depth pattern).

Verification

• New regression test convex/mcp_servers/authorization.test.ts (13 cases): each public mutation/action rejects an unauthorized caller and never mutates; create gates on the supplied org; id-based ops gate on the owning org and throw on a missing server; asserts executeMcpTool is an internalAction.
• bunx vitest --run --project server mcp_servers/ — 35 passed.
• bunx tsc --noEmit — clean (the public/internal type split flows from the function visibility; no codegen change needed).
• bunx oxlint --type-aware on all changed files — 0 warnings, 0 errors.

[larryro]

Desk Review — #2249 (closes #2040)

Verdict: READY TO MERGE. The fix correctly and completely closes the cross-tenant authorization gap on the MCP-server public surface. Reviewed via a two-round fan-out (6 breadth reviewers across correctness/robustness/tests/elegance/security, then independent adversarial confirmation of the two highest-impact claims). No blocking findings.

Evidence

• CI: green. gh pr checks → 46 pass, 5 skipping, 0 failing/pending.
• Tests (run locally on this branch):
  • bunx vitest --run --project server mcp_servers/ → 35 passed
  • bunx vitest --run --project server mcp_servers/authorization.test.ts → 13 passed
  • bunx tsc --noEmit → clean (exit 0)

What's correct

• executeMcpTool → internalAction removes the public SSRF/credential-use primitive entirely. Repo-wide grep confirms no surviving api.*.executeMcpTool caller — both call sites (agent_tools/mcp/create_bound_mcp_tool.ts:178, mcp_servers/execute_approved.ts:55) use internal.*. Both pass a serverId resolved strictly within the caller's org via the org-scoped internal_queries.listActiveByOrg, so no internal cross-tenant path exists.
• create gates on the client-supplied organizationId via requireOrgAdminOrDeveloper, which validates the caller is a live member of that org (membership keyed on the JWT-derived userId) — a forged foreign org id fails membership.
• update / remove / updateStatus route through the new requireServerAdmin, deriving the owning org from the stored doc (not a client arg) before the gate — cross-tenant tampering is structurally impossible.
• testConnection gates on the doc-derived org before the first status mutation/discovery; the auth check strictly precedes every side effect on every branch (verified branch-by-branch, including the catch path).
• Capability mapping is correct: read developerSettings admits exactly owner/admin/developer; members/editors/disabled are rejected. Helper is correctly non-'use node' so it imports cleanly into the 'use node' actions. Leftover getAuthUserIdentity/api imports fully removed. Change is minimal and mirrors the sibling session_admin_actions.ts pattern.

Non-blocking observations (optional follow-ups, do not gate merge)

1. Existence oracle (minor): testConnection (actions.ts:45-47) and requireServerAdmin (public_mutations.ts:28-30) throw "MCP server not found" before the auth gate, so an unauthorized cross-tenant caller can distinguish "not found" from "forbidden". Severity is low: mcpServers IDs are opaque/unguessable Convex IDs, the read queries (list/getById) never leak cross-org IDs, and no server fields are disclosed pre-gate. The sibling sandbox pattern collapses both into one generic error — optional hardening.
2. Test gap (minor): authorization.test.ts does not cover testConnection's authorized success-after-auth path (status flip to discovering + discovery firing). The unauthorized/forbidden ordering is well-covered for all four mutations; the success path is the one uncovered branch.
3. Nit: executeMcpTool has no defense-in-depth org assertion of its own — safe today only because it is internalAction and both callers are org-scoped. A future internal caller passing an unvalidated serverId would reintroduce the gap; a comment or guard would harden it.

None of the above blocks the merge.

READY TO MERGE.