Skip to content

feat: URL-mode secret collection for create_edge_function_secret - #412

Open
barryroodt wants to merge 11 commits into
mainfrom
feat/url-mode-secret-collection
Open

feat: URL-mode secret collection for create_edge_function_secret#412
barryroodt wants to merge 11 commits into
mainfrom
feat/url-mode-secret-collection

Conversation

@barryroodt

@barryroodt barryroodt commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Why

Add an Edge Function’s third-party API key without pasting it into the conversation.

What changed

Secret values are entered only in Dashboard, never the AI client/model. HTTP enables this by default. Discovery requires modern per-request URL capability; existing functions, read-only, auth and platform-support gates remain. Breaking: migrate costConfirmation.requestStateKey, costConfirmation.principal, costConfirmation.ttlSeconds to elicitation.requestState:{key,principal,ttlSeconds?}. elicitation.costConfirmation and elicitation.secretCollection remain independent.

How to test

Follow setup and run pnpm dev:http. Use a test project, Dashboard authentication/write permission and the matching deployed Dashboard route. Connect Inspector 2.5.0 Modern (URL-capable) to http://127.0.0.1:3111/mcp?project_ref=REF&read_only=false with Authorization: Bearer <PAT>; omit features. Call create_edge_function_secret with a fresh name (no value). Save only in Dashboard, return to Inspector and accept within 120s, expect stored:true. Legacy/non-URL clients should not list the tool.

Trade-offs

HTTP signed state lasts 120s. Separate timestamp-based fresh-call recovery lasts 600s; this is not URL validity. Acceptance checks updated_at>=issued_at; stored:true is not a receipt; another collaborator’s write can satisfy it. Recovery can return recent success; a fresh name avoids that shortcut. Custom API origins require --secret-url-template.

@coveralls

coveralls commented Sep 3, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 34456445694

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 decreased (-0.08%) to 96.317%

Details

  • Coverage decreased (-0.08%) from the base build.
  • Patch coverage: 15 uncovered changes across 1 file (313 of 328 lines covered, 95.43%).
  • 2 coverage regressions across 1 file.

Uncovered Changes

File Changed Covered %
packages/mcp-server-supabase/src/tools/secret-tools.ts 200 185 92.5%
Total (6 files) 328 313 95.43%

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
packages/mcp-server-supabase/src/tools/database-operation-tools.ts 2 97.08%

Coverage Stats

Coverage Status
Relevant Lines: 3883
Covered Lines: 3758
Line Coverage: 96.78%
Relevant Branches: 597
Covered Branches: 557
Branch Coverage: 93.3%
Branches in Coverage %: Yes
Coverage Strength: 69.2 hits per line

💛 - Coveralls

@barryroodt barryroodt added the publish-preview Runs `publish-preview` workflow to publish preview packages via https://pkg.pr.new/ label Sep 4, 2026
@pkg-pr-new

pkg-pr-new Bot commented Sep 4, 2026

Copy link
Copy Markdown

Open in StackBlitz

pnpm add https://pkg.pr.new/@supabase/mcp-server-postgrest@5b0f95d
pnpm add https://pkg.pr.new/@supabase/mcp-server-supabase@5b0f95d
pnpm add https://pkg.pr.new/@supabase/mcp-utils@5b0f95d

commit: 5b0f95d

…collection

# Conflicts:
#	packages/mcp-server-supabase/src/server.test.ts
The dashboard route puts the project ref in the path
(/dashboard/project/{ref}/mcp/secrets), so the connect URL is now a
template with {ref} and {name} placeholders instead of a base URL plus
appended query parameters.
The signing fields now live in one `elicitation` block. `requestState`
is required inside it, so the type guarantees every elicitation feature
has the signed-state config. The runtime guard "secretCollection
requires costConfirmation" and its test are removed.

Before (0.12.0):

  costConfirmation: {
    requestStateKey, principal, ttlSeconds?, enabledTools
  }

After:

  elicitation: {
    requestState: { key, principal, ttlSeconds? },
    costConfirmation?: { enabledTools },
    secretCollection?: { connectUrlTemplate },
  }

Runtime behaviour for existing callers is unchanged: one HMAC codec
created lazily when a feature that mints state is enabled, bound to
method + principal, with TTL, tool/project/name scoping and reissue
semantics as before. Removing the guard also allows a new shape: a
host can now enable URL-mode secret collection without cost
confirmation.

BREAKING CHANGE: costConfirmation.requestStateKey, .principal and
.ttlSeconds are removed. Pass elicitation.requestState and keep
enabledTools under elicitation.costConfirmation. Hosted callers migrate
with their package bump; the exact 0.12.0 pin protects them until then.
`replace: true` skips the 10 minute resume-by-key shortcut and issues
the dashboard URL elicitation again. Default false keeps the current
behaviour: a recent update is reported as stored without asking again.
The unsupported-client check and accept/reissue recovery are unchanged,
and `replace` is not part of the signed state.

Result text changes:

- decline and cancel: "Not confirmed. If you already saved it in the
  dashboard, it is stored." The structured result drops `stored: false`
  and keeps `status`.
- accept: "The dashboard reports an update to <name> since this
  request."
- fresh call within 10 minutes: "The dashboard reports an update to
  <name> <N> seconds ago."

Other structured results are unchanged. One focused test covers
`replace: true`.
kemaldotearth added a commit to supabase/supabase that referenced this pull request Sep 8, 2026
## I have read the
[CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md)
file.

YES

## What kind of change does this PR introduce?

Initial build of the URL mode interstitial. Has complete mock data to be
able to view different states (this will be stripped out in the end). A
starting point for us to use as an intercept.

This is linked with @barryroodt ticket in the MCP project.
supabase/mcp#412


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Added an authenticated MCP Secrets page for securely storing API
secrets requested by AI tools.
- Added provider-aware guidance for OpenAI, Anthropic, Resend, and
Stripe keys, including validation warnings and dashboard links.
- Added clear success, cancellation, expiration, error, and
wrong-account states with account switching.
- Added loading placeholders, secret visibility controls, overwrite
warnings, and accessibility announcements.
- **Bug Fixes**
- Improved interstitial animations to respect reduced-motion
preferences.
- Preserved return destinations and related parameters during sign-in
flows.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@barryroodt
barryroodt marked this pull request as ready for review September 9, 2026 08:48
@barryroodt
barryroodt requested a review from a team as a code owner September 9, 2026 08:48
@mattrossman

Copy link
Copy Markdown
Collaborator

Should create_edge_function_secret be hidden when the client doesn't declare elicitation.url? Latest Claude Code 2.1.267 (which sends elicitation: {}) sees the tool, calls it, and always gets the "cannot open a browser page" error. For the cost confirmation elicitations we hide the legacy confirmation tools on capable modern clients, seems like we could do the opposite here.

CleanShot 2026-09-09 at 17 40 22@2x

@mattrossman

Copy link
Copy Markdown
Collaborator

Couple feedback items on the UX side, neither seems blocking for this MCP-side code, just unexpected:

First, the UI says:

Go back to your AI client and choose "I've completed it" to finish the tool call.

That wording seems specific to the MCP Inspector's UI, are we sure other clients will show the same button, or even require user confirmation? Seems like this should be more generic.

Second, I notice that if I click "Reopen in browser" after submitting a secret, it allows me to input the secret again. That seems innocent from a security POV, but it is a little confusing UX because I can't tell from looking at the page if the elicitation was fulfilled (other than the warning about overwriting an existing secret). I'd expect this flow to feel similar to one-time login links, where you get feedback about whether a link was already used. I see there is a placeholder already-stored page in the dashboard code but nothing points to it currently. Sounds like this will be handled in AI-1170 but one other short-term improvement could be to pass along the issued_at to browser so it can show already stored screen if secret was updated after elicitation issued (though I recognize that's not perfect, since someone else could update the secret outside of elicitation flow).

@mattrossman

Copy link
Copy Markdown
Collaborator

Other UX quirk, if you pass a ?name= with SUPABASE_ prefix it shows the link as "expired", whereas if you make up a different name it lets you submit the secret. Based on the tool schema I gather the intention is for SUPABASE_ prefix variables to be reserved, so the rejection screen saying "expired" is a little unexpected.

made up name=FOOBAR made up name=SUPABASE_FOOBAR
CleanShot 2026-09-10 at 12 02 23 PM@2x CleanShot 2026-09-10 at 12 02 31 PM@2x

@mattrossman mattrossman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I tested this in the MCP Inspector since AFACT no current coding agents implement URL mode elicitations w/ modern protocol properly, and it generally worked as expected 👍

As long as it's behind a flag for us to test first I'm okay to proceed, comments above are generally non blocking but some are worth discussing.

* server issues, discriminated by `tool`.
*/
export type CostConfirmationState = ProjectCostState | BranchCostState;
export type CostConfirmationState =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be called CostConfirmationState still, if it's for more than costs?

}
});

test('tool input schema exposes project_id, name and replace, never value', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit, seems like this could be tested more clearly w/ a single assertion of the strict list of properties, e.g. expect(Object.keys(secretTool?.inputSchema.properties ?? {}).sort()).toEqual(['name', 'project_id', 'replace']);

Asserting that there isn't a "value" property doesn't add much if someone used a different property name like "secret".


const first = (await client.request(
{
method: 'tools/call',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we be changing the callTool() helper to shorten these tests? Seems like we could use that halper to avoid the raw JSON-RPC stuff (same for cost confirmation tests)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We're due to condense this file overall and that can be a follow up thing, I'm just trying to find ways to make it easier to review as 1000+ LOC of tests in the PR is a little hard to meaningfully vet.

.boolean()
.optional()
.describe(
'Set to true to ask the user for a new value even if this secret was updated in the last 10 minutes. Default false: a recent update is reported as stored without asking again.'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What's the rationale for the 10 minute cutoff here? IIUC this means if we run the tool with replace: false, it can still replace an existing value that's older than 10 minutes. I'd expect this flag to behave like a regular upsert flag, where replace: false means I only write the value if it wasn't set.

headers: { ...AUTH_HEADERS, 'content-type': 'application/json' },
body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/list' }),
});
expect(response.status).toBe(500);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit, should we be rejecting invalid template URL on server start instead of request time?

content: [
{
type: 'text' as const,
text: 'Request state arguments do not match the current arguments.',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could use test coverage here

].join(' ');
return elicitation === undefined
? description
: `${description} elicitation=${elicitation}`;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this valuable to have on each line? Wondering if that's a leftover debugging artifact, or maybe it could be filtered to only show on tools that use elicitations. Otherwise seems like dev noise for most tools.

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

Labels

publish-preview Runs `publish-preview` workflow to publish preview packages via https://pkg.pr.new/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants