feat: warn users before making an mcp server public with system-provided env vars#2289
Merged
simplesagar merged 13 commits intomainfrom Apr 19, 2026
Merged
feat: warn users before making an mcp server public with system-provided env vars#2289simplesagar merged 13 commits intomainfrom
simplesagar merged 13 commits intomainfrom
Conversation
- Wrap body paragraph in Dialog.Description to satisfy Radix's aria-describedby requirement - Add aria-hidden="true" to decorative ShieldAlert and ExternalLink icons Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Store {target, sourceStatus} in publicWarningPending so handlePublicWarningConfirm
uses the status captured at dialog-open time rather than live currentStatus, closing
a stale-closure race that could bypass the ServerEnableDialog billing gate.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design doc and implementation plan for the feature added in this branch. Kept in-tree for reviewer context and future backfill of similar warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: d1bdf35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The metadata endpoint returns 404 when no mcp_metadata row exists for a toolset, which is the common state for fresh or seeded toolsets. With retry: false that 404 became a permanent isError: true, which the earlier guard treated as "unknown state" and locked the Public dropdown option forever with "Loading environment data…". A missing metadata row actually means "no attached env", so there can be no system-provided vars to leak. Restrict the guard to isLoading only — matching how MCPAuthenticationTab and MCPEnvironmentSettings already handle the same query. Genuine network errors will fall through without a warning, but that's consistent with the rest of the dashboard and preferable to locking the UI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
🚀 Preview Environment (PR #2289)Preview URL: https://pr-2289.dev.getgram.ai
Gram Preview Bot |
The spec and plan were committed in the previous docs commit for reviewer context. Removing them here so the PR diff only contains the dashboard feature. The files stay accessible via `git show da3cafb` for anyone who wants the design context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
- Use the default Dialog.Title style (drop the Tobias inline styling). - Reword the body to reference "the Default Environment" generically and clarify that shared system values are team/public credentials. - Link text becomes `Review in "Default Environment"`. - Drop the now-unused environmentName prop from the component and call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Devin review: if useGetMcpMetadata resolved before useListEnvironments, `environments` was still [] and attachedEnvironment computed as null, which made systemVarNames empty and allowed the user to flip to Public without the warning dialog appearing. Block the Public option until both queries have landed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
danielkov
approved these changes
Apr 19, 2026
Member
Author
- Add .changeset/mcp-public-system-vars-warning.md to satisfy the Lint PR Title and Changesets check. - Run mise run gen:sdk to bring jsr.json, .speakeasy/gen.yaml, and src/lib/config.ts up to 0.32.65 (they were still at 0.32.64 after main's `chore: version packages` commit only updated client/sdk/package.json). This unblocks the Check-SDK-is-up-to-date CI job. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
state: "system"). Public MCP + system var = shared secret across every caller, which is easy to miss in the existing UI.system_vars_warned: boolean | undefinedto themcp_made_publictelemetry event so we can track how often users see the warning vs. go straight through.Scope
getSystemProvidedVariables(envVars, slug)pure helper + unit tests inclient/dashboard/src/pages/mcp/environmentVariableUtils.{ts,test.ts}.PublicMcpWarningDialogcomponent + tests atclient/dashboard/src/components/public-mcp-warning-dialog.{tsx,test.tsx}. Uses Diatype Mono for the variable-name list and a Swift-red accent border/icon; otherwise defers to the moonshineDialog+Buttonprimitives for default typography and layout. A11y:Dialog.Descriptionwraps the body, decorative icons arearia-hidden.client/dashboard/src/pages/mcp/MCPDetails.tsx(MCPStatusDropdown) — fetches the attached environment + metadata, routes public transitions through the new dialog, chains into the existingServerEnableDialogondisabled → public, and snapshots the source status at dialog-open time to avoid a stale-closure race with toolset query revalidation.Behavior matrix
ServerEnableDialog→ confirm → mutateServerEnableDialogonly (unchanged)Test plan
cd client/dashboard && pnpm test src/pages/mcp/environmentVariableUtils.test.ts— 4 helper tests passcd client/dashboard && pnpm test src/components/public-mcp-warning-dialog.test.tsx— 3 dialog tests passcd client/dashboard && pnpm tsc -p tsconfig.app.json --noEmit— no new type errors inMCPDetails.tsxServerEnableDialog, then mutationmcp_made_publicpayload containssystem_vars_warned: true/falsematching the scenarioTrade-offs
isLoading, not on query errors. The metadata endpoint returns a 404 when no metadata row exists for a toolset (the common case for fresh or seeded toolsets); treating that as fail-closed would lock the Public option permanently withretry: false. Genuine transient network errors on metadata will now fall through without a warning — consistent with howMCPAuthenticationTabandMCPEnvironmentSettingsalready handle the same query.needsEnableDialogfires ondisabled → privatetransitions too — that's pre-existing behavior, not changed here.🤖 Generated with Claude Code