feat(mcp)!: confirm destructive SQL via elicitation and rename costConfirmation option

[barryroodt]

Why

Adds confirmation before destructive SQL, such as dropping a table (AI-1186).

What changed

Breaking public option rename: costConfirmation → confirmation. enabledTools supports existing cost confirmations (create_project, create_branch) and SQL confirmations (execute_sql, apply_migration); cost discovery is unchanged. Local HTTP opts into both SQL tools. Signed request-state is reusable within its TTL, without exactly-once guarantees. A bounded fix detects direct ALTER TABLE DROP [COLUMN]. No new dependencies.

How to test

With pnpm 10, dependencies installed and packages/mcp-server-supabase/.env.local present (create empty only if absent), run from the repo root:

pnpm --filter @supabase/mcp-server-supabase exec vitest run --project unit src/transports/local-http-entry.test.ts
pnpm --filter @supabase/mcp-server-supabase exec vitest run --project unit src/server.test.ts -t 'declining bare-column DROP'

Expect passing SQL prompt/accept and no-form cases; declining bare-column DROP preserves the column and data. These are isolated loopback/MSW/PGlite fixtures, not real operations.

Trade-offs

Best-effort regex is not a security barrier: literal-comment, UPDATE-comment and WITH/DO misses remain. Without per-request form capability, SQL can execute unprompted. Hosted-platform SQL opt-in is separate (draft #38211), default off. Real GREEN behavior is untested.

[coveralls]

Coverage Report for CI Build 34487004981

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.5%) to 96.925%

Details

• Coverage increased (+0.5%) from the base build.
• Patch coverage: 2 uncovered changes across 1 file (380 of 382 lines covered, 99.48%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
packages/mcp-server-supabase/src/tools/confirmation.ts	135	133	98.52%
Total (7 files)	382	380	99.48%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	3759
Covered Lines:	3664
Line Coverage:	97.47%
Relevant Branches:	566
Covered Branches:	528
Branch Coverage:	93.29%
Branches in Coverage %:	Yes
Coverage Strength:	65.5 hits per line

---

💛 - Coveralls

[mattrossman]

Where do you see controls for turning elicitations on/off fitting in to this?

To me, destructive SQL confirmations get into territory where I could see elicitations getting in the way for users who want agents to auto approve safe actions. Wondering if we should be adding options to control elicitation behavior before / alongside this change.

[barryroodt]

Where do you see controls for turning elicitations on/off fitting in to this?

To me, destructive SQL confirmations get into territory where I could see elicitations getting in the way for users who want agents to auto approve safe actions. Wondering if we should be adding options to control elicitation behavior before / alongside this change.

that's a good question
we also have the scenario where users will want to disable elicitations (or specific elicitations) for CI workflows.
this is something I had in my first draft but dropped to keep initial complexity down.

We have two main options imo:

1. query param - if supplied can be used to disable 1 or more elicitations
2. machine auth tokens - if token is minted with a specific set of perms, elicitations can be bypassed

Open to suggestions of course - and more than happy to add on to either this PR or as a second, stacked PR.

[mattrossman]

Query param sounds reasonable. At first I assumed this would just be a boolean flag, but you bring up a good point that we could make it more fine-grained so users can decide what level of safety they want.

[mattrossman]

It could be a passthrough of the tools where confirmations are enabled e.g.

?confirmations=execute_sql,apply_migration

The risk with an allowlist is if we add a new tool in the future that uses elicitations for approval, the user won't realize they opted out of its safety net. A deny-list would be safer, so they're only taking in risk in areas they've listed explicitly:

?skip_confirmations=execute_sql,apply_migration

[barryroodt]

It could be a passthrough of the tools where confirmations are enabled e.g.

?confirmations=execute_sql,apply_migration

The risk with an allowlist is if we add a new tool in the future that uses elicitations for approval, the user won't realize they opted out of its safety net. A deny-list would be safer, so they're only taking in risk in areas they've listed explicitly:

?skip_confirmations=execute_sql,apply_migration

Agreed 💯 , a per-tool deny-list is exactly what I was thinking.

Since the package already exposes the per-tool mechanism, this doesn't need an MCP package change; the Platform just needs to parse the user's preference and derive the effective tool set. We'll pick that up on the Platform side shortly.

[Rodriguespn]

Asking for changes as I identified some edge cases on the parsing that I think would need further discussion.

For example, a two-line SQL statement that starts with a comment (--) isn't sanitized correctly today:

SELECT '--';
DROP TABLE films;

I tried this locally against the current regex and the table gets dropped with no confirmation prompt. The -- inside the string literal on line 1 is treated as a real comment and silently eats the rest of that line, including the ; that would otherwise separate it from the DROP.

Any reason why we're doing our own parsing instead of using the official Postgres parser libpg-query-node?

We're already using it on the backend to classify execute_sql queries: sql-classification.worker.ts.

Same scenario through the real parser:

import { parse } from '@libpg-query/parser'

const { stmts } = await parse("SELECT '--';\nDROP TABLE films;")
// stmts.length === 2
// stmts[1].stmt -> { DropStmt: { removeType: 'OBJECT_TABLE', objects: [...films] } }

Two real statements come out and the DROP is fully visible, no regex needed, because the parser understands string literals natively.

I found a few other cases the current regex misses the same way:

1. UPDATE without a WHERE when a comment sits nearby (leading, inline, or trailing),
2. ALTER TABLE t DROP col without the optional COLUMN keyword,
3. destructive statements wrapped in WITH ... or DO $$ ... $$.

All of these come for free with a real parser (checking the whereClause field, matching on the AlterTableCmd subtype instead of literal text, or just walking the statement list) instead of patching the regex edge case by edge case.

I've prepared some test cases that we can reuse here when I was working on the sql parsing.

Lmk what you think.

[barryroodt]

Asking for changes as I identified some edge cases on the parsing that I think would need further discussion.

For example, a two-line SQL statement that starts with a comment (--) isn't sanitized correctly today:

SELECT '--';
DROP TABLE films;

I tried this locally against the current regex and the table gets dropped with no confirmation prompt. The -- inside the string literal on line 1 is treated as a real comment and silently eats the rest of that line, including the ; that would otherwise separate it from the DROP.

Any reason why we're doing our own parsing instead of using the official Postgres parser libpg-query-node?

We're already using it on the backend to classify execute_sql queries: sql-classification.worker.ts.

Same scenario through the real parser:

import { parse } from '@libpg-query/parser'

const { stmts } = await parse("SELECT '--';\nDROP TABLE films;")
// stmts.length === 2
// stmts[1].stmt -> { DropStmt: { removeType: 'OBJECT_TABLE', objects: [...films] } }

Two real statements come out and the DROP is fully visible, no regex needed, because the parser understands string literals natively.

I found a few other cases the current regex misses the same way:

1. UPDATE without a WHERE when a comment sits nearby (leading, inline, or trailing),
2. ALTER TABLE t DROP col without the optional COLUMN keyword,
3. destructive statements wrapped in WITH ... or DO $$ ... $$.

All of these come for free with a real parser (checking the whereClause field, matching on the AlterTableCmd subtype instead of literal text, or just walking the statement list) instead of patching the regex edge case by edge case.

I've prepared some test cases that we can reuse here when I was working on the sql parsing.

Lmk what you think.

I've been hesitant to add another runtime dependency to the existing package.
I also thought that for the initial version we could re-use the regex implemented in Studio in order to maintain parity - with the idea of migrating to something a bit more robust in future iterations.

Happy to revisit this of course - what are your thoughts on adding this dependency?

[pkg-pr-new]

Open in StackBlitz

pnpm add https://pkg.pr.new/@supabase/mcp-server-postgrest@ba61fba

pnpm add https://pkg.pr.new/@supabase/mcp-server-supabase@ba61fba

pnpm add https://pkg.pr.new/@supabase/mcp-utils@ba61fba

commit: ba61fba