Skip to content

fix(mcp): gate execute-sql on query:read only - #110271

Open
JakeRuth wants to merge 2 commits into
masterfrom
jake/mcp-execute-sql-scopes
Open

JakeRuth wants to merge 2 commits into
masterfrom
jake/mcp-execute-sql-scopes

Conversation

@JakeRuth

@JakeRuth JakeRuth commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • An MCP token with query:read but without insight:read does not see execute-sql, so the agent cannot run any SQL.
  • The backend endpoint behind the tool requires only query:read (registration, scope check).
  • insight:read came in with the tool in feat(mcp): v2 with sql #46947, which gives no reason for it.

Changes

  • execute-sql now requires only query:read in the MCP catalog, which matches the backend.
  • This does not change what a token can read. A query on system.insights already returns only the insights the user's role allows, and the backend never checked insight:read for this call.
  • Mechanical: the regenerated tool-definitions-all.json, and a comment and skip message in the integration suite.
  • No change to the advertised OAuth or CLI scope lists, because other tools still require insight:read.

How did you test this code?

Test rationale: the existing read-scope case in tool-filtering.test.ts now uses a query:read-only token and checks that execute-sql shows up. It fails if insight:read comes back. No new test.

  • Ran hogli build:openapi-mcp-tools and hogli build:openapi-cli-agent-scopes. Only the three files in this diff changed.
  • Ran the MCP typecheck, lint and tool-filtering.test.ts locally.
  • Not run: the integration suite, which needs a live stack and a test token.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

None. No doc lists the scopes for execute-sql.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, claude-opus-5-5

  • Skills: /reviewing-with-coderabbit, /writing-pr-descriptions.
  • CodeRabbit CLI: ran with --deep because the diff touches permissions. It reported no findings.
  • Duplicate search: no open PR changes the execute-sql scopes.
  • Out of scope: backend checks of token scopes on each system.* table for personal API keys and OAuth tokens. The backend runs that check only for project secret API keys.

🤖 Generated with Claude Code

The backend endpoint for execute-sql requires only query:read, so the
extra insight:read in the MCP catalog hid the tool from tokens that
could call it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JakeRuth JakeRuth self-assigned this Oct 1, 2026
@trunk-io

trunk-io Bot commented Oct 1, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane (svc:mcp)

This PR is assigned to the non-backend lane (svc:mcp). It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

@JakeRuth
JakeRuth marked this pull request as ready for review October 1, 2026 18:49
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Critical risk] Changes permission requirements for SQL execution.

The PR appears safe to merge, but a focused test would protect its scope-filtering fix from regression.

Reviews (1) · Last reviewed commit: "fix(mcp): gate execute-sql on query:read..."

Comment thread services/mcp/tests/integration/mcp-protocol-suite.ts
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: ed081a8d-8810-4551-a3ff-35fa613c6d01

📥 Commits

Reviewing files that changed from the base of the PR and between 3e21f14 and f1b637a.

📒 Files selected for processing (1)
  • services/mcp/tests/unit/tool-filtering.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The execute-sql tool definitions now require only the query:read scope. Integration-test text states that requirement. The unit test checks that a query:read token includes execute-sql.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to f1b63

execute-sql is now visible to query:read credentials, consistent with the endpoint’s existing scope; role-based SQL access remains in force. No material merge risk is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f1b63

The change broadens SQL tool availability, but matches the backend’s existing query:read requirement and preserves authenticated user and project context. No new authorization bypass was established. Downstream enforcement was not fully verified end to end.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The newly eligible callers are authenticated credentials with query:read but without insight:read, including agents acting through those credentials. The change exposes an existing SQL capability through tool discovery rather than granting a new scope. The tool also supports connectionId-based queries, whose downstream authorization was not verified in this pass.

Security Findings and Attack Paths

  • inferred — A newly eligible credential holder or agent can supply SQL arguments to the existing MCP backend. The inspected path retains registered query:read authorization and authenticated identity propagation; no introduced authorization bypass was established. This conclusion does not certify every downstream query or connection path.

Trust Boundaries and Controls

  • observed — Backend execution authority remains separate from catalog visibility. The registry supplies required scopes and instantiates tools with team/user identity. For system.insights, inspected controls include resource denial, per-user row-guard construction, and a team predicate on the federated PostgreSQL read.

Hardening Proposals

  • proposed — Add a focused end-to-end authorization regression pairing query:read-only MCP eligibility with denied-role insight access and cross-team rejection. This would guard against discovery and backend isolation drifting independently; it is not an observed vulnerability.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description includes the required Problem, Changes, testing, release status, notifications, docs, and agent context sections. It explains the user impact, scope change, test coverage, and unrun in…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants