feat(connect): preview the exact change before writing + stop leaking the API key into client configs (Spec 078 US1)#801
Closed
Dumbris wants to merge 7 commits into
Closed
Conversation
…te path
First slice of specs/078-connect-trust-preview US1 (see spec FR-001..004,
012, 013). Adds GET /api/v1/connect/{client}/preview returning the exact
change a subsequent connect would make — target config_path, server_key,
entry name, and the exact entry contents — WITHOUT modifying the file or
creating a backup.
Preview==write guarantee: buildServerEntry is now the single construction
path for every client, including Codex/TOML (connectTOML previously built
its {url} entry inline). Preview derives its entry from the same function
the write uses, so what is previewed equals what is written (FR-002); a
table-driven test connects each supported client and asserts the written
file entry deep-equals the shared constructor output across JSON and TOML.
API-key honesty (FR-004): the embedded apikey is masked in the payload
(entry, entry_text) while contains_api_key flags that a credential is
written; the real key never appears in the preview (verified by test that
marshals the whole payload and asserts the secret is absent).
Spec 075 access states (FR-012): the on-demand read used to classify
create-vs-overwrite degrades to access_state=absent|malformed and a denial
returns the same typed AccessError (403 + remediation) as connect/disconnect.
- internal/connect/preview.go: Service.Preview, maskURLAPIKey, entry render
- internal/connect/clients.go: buildServerEntry gains the codex case
- internal/connect/connect.go: connectTOML uses buildServerEntry
- internal/httpapi: handler + route + tests (masked, no side effects, 403)
- oas: regenerated for the new endpoint (make swagger)
Tests: preview-equals-write (JSON+TOML), masking, entry_exists (create vs
overwrite), no-side-effects (no write, no backup, mtime unchanged), absent/
malformed/denied degradation. go test -race ./internal/connect/... and
./internal/httpapi/... pass; golangci-lint v2 clean.
Related #793
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t modal
Second slice of specs/078-connect-trust-preview US1 (FR-001,003,004): the
Connect action in BOTH the standalone Connect modal and the onboarding
wizard's ClientRow now fetches the preview first and shows an inline panel
before any file is written:
- target config path and, in a monospace additive ("+ will be added")
block, the exact entry that will be written (pretty JSON, or TOML for
Codex) with the API key masked;
- plain-language trust copy ("Only this entry is added — everything else in
the file stays untouched. A timestamped backup is created first.");
- a masked-key line when contains_api_key so the user knows a credential is
written;
- an overwrite warning when an entry with the same name already exists, in
which case confirming implies force=true;
- graceful copy for the bridge/no-prior-file (access_state=absent) and
unparseable (malformed) cases.
Confirm proceeds with the connect (reusing the existing #799 post-connect
backup-path line, untouched); Cancel dismisses the panel and writes nothing.
A non-denial preview fetch error is surfaced inline; in the modal a denied
read still resolves the Spec 075 access banner via checkAccess. Wizard
connect-step telemetry (markConnectCompleted/Skipped) is unchanged.
- types/api.ts: ConnectPreview; services/api.ts: getConnectPreview
- ConnectModal.vue + OnboardingWizard.vue ClientRow: preview panel + flow
- The existing #799 connect specs are updated to drive click → preview →
confirm (Connect no longer writes on the first click); new
connect-preview.spec.ts covers preview render, cancel-writes-nothing,
confirm-proceeds, masked key, and overwrite→force for both surfaces.
Full frontend suite: 33 files, 244 tests pass; vue-tsc clean; make build
embeds the updated dist. npm-ci lockfile churn reverted (as in #799).
Related #793
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e carrier
Security fix folded into Spec 078. Connect (and the new preview) previously
appended ?apikey=<REST-admin key> to EVERY client entry via mcpURL(), but
/mcp does not require auth by default (config.RequireMCPAuth defaults false;
the admin context is granted unauthenticated). So connect leaked the
REST-admin API key in plaintext into other apps' config files where it
served no purpose.
New behavior, threaded from config.RequireMCPAuth into connect.Service the
same way listenAddr/apiKey are (server wiring + both CLI sites), via
WithRequireMCPAuth:
- require_mcp_auth=false (default): the entry carries NO credential — a clean
http://host:port/mcp URL, no query, no headers. Existing configs keep
working because /mcp accepts unauthenticated requests when protection is off.
- require_mcp_auth=true: the credential is written via the carrier each
client's real config schema supports (verified against vendor docs):
* X-API-Key header — claude-code, vscode, cursor, windsurf, gemini,
opencode (all support a "headers" object on their remote entry);
* mcp-remote --header "X-API-Key:<key>" bridge arg — claude-desktop
(no space after the colon: Claude Desktop/Cursor don't escape spaces in
args and mangle the header — geelen/mcp-remote README; our keys are
space-free);
* ?apikey= query fallback — codex only, whose TOML HTTP transport takes
header values indirectly via env-var names (http_headers /
bearer_token_env_var) and cannot express a literal header value.
Server-side ExtractToken accepts X-API-Key, Authorization: Bearer, and
?apikey= (in that order), so header and query authenticate identically.
buildServerEntry now takes serverEntryParams{baseURL, credential}; mcpURL()
is replaced by the credential-free baseURL() anchor. Entry MATCHING
(findEntry*/findEquivalentJSONServerName/entryPointsToBridge) already keys on
baseURL with an optional ?query, so it recognizes BOTH legacy ?apikey=
entries and new clean entries — reconnect adopts/updates in place (no
duplicate) and disconnect still finds legacy entries; a force-reconnect over a
legacy entry upgrades it and drops the leaked key.
Preview reflects reality: contains_api_key is true only when require_mcp_auth
is on; the credential is masked in whichever carrier it uses (header value,
bridge arg, or query) and the real key never appears in the preview payload.
Tests: matrix over require_mcp_auth on/off × all 8 clients for both write and
preview (preview==write shape, auth-off writes contain NO apikey/headers at
all); per-client carrier assertions; legacy ?apikey= migration (recognized,
upgraded in place, no duplicate, disconnect still works). go test -race
./internal/connect/... ./internal/httpapi/... pass; server/config/management
pass; golangci-lint v2 clean.
Related #793
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follows the connect security fix: the preview panel's masked-credential line now reads "This entry includes your API key (shown masked)…" instead of "The URL includes…", since with require_mcp_auth on the credential is carried in an X-API-Key header (or the mcp-remote --header bridge arg) for most clients and only falls back to the ?apikey= URL query for Codex. The line already renders only when contains_api_key is true, which the backend now sets only when require_mcp_auth is on — so the default (keyless) connect shows no credential line at all. Adds a frontend test for the default keyless case: contains_api_key=false ⇒ no credential line, and the previewed entry shows neither an apikey query nor a mask. Full frontend suite: 33 files, 245 tests pass; vue-tsc clean. Related #793 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review of the Spec 078 US1 connect-preview branch surfaced one blocker and three preview/write-divergence gaps; this addresses them: - blocker: the HTTP connect service snapshotted require_mcp_auth (and listen/api_key) once at startup and was never rebuilt, while the /mcp auth middleware honors require_mcp_auth live per-request. Toggling auth off at runtime (wizard toggle or hot-reload) left the service embedding the real API key into client configs — the exact leak this branch closes. Add a live config-provider seam (WithConfigProvider) wired to runtime.Config() so preview and write always match the currently-enforced auth state. - OpenCode preview divergence: previewEntryExists checked only the exact server name, but OpenCode's write path adopts/normalizes an equivalent entry (incl. the legacy ?apikey= shape) under a different key. Preview now mirrors that via findEquivalentJSONServerName so entry_exists reflects the overwrite. - preview endpoint ignored server_name and always previewed "mcpproxy" while POST connect accepts a custom name. Honor an optional ?server_name= query so a caller previewing then connecting under a custom name does not diverge. - malformed config: the write parses the same bytes and would fail, but the preview copy promised it "still writes only the entry" and left Connect enabled. Make the copy honest and disable Connect for malformed configs. Tests: live-toggle credential (auth off->on without rebuild), OpenCode equivalent-entry preview, server_name honoring, and malformed confirm-disabled. go test -race ./internal/connect/... ./internal/httpapi/... green; frontend 246 tests + vue-tsc clean; golangci-lint v2 ./... 0 issues; swagger regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
bd25c8d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ac1719a2.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://feat-078-connect-preview.mcpproxy-docs.pages.dev |
The preview only appeared AFTER clicking "Connect" — the exact button hesitant users avoid, fearing the click immediately modifies their config. Rename the row action to "Review & connect" and say up front in the modal subtitle and wizard intro that nothing is written until the confirm step, so the safety of the first click is visible before it happens (Spec 078 US1 trust copy). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review follow-up: docs/setup.md described the Claude Desktop bridge registration as one-click "Connect"; it is now review-then- confirm. Test comments updated to match the renamed row action. Spec 046/075 texts intentionally left untouched — they are point-in- time records (046 FR-005 already required the diff preview this branch implements). Co-Authored-By: Claude Fable 5 <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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Motivation
Spec 078 (connect-trust) US1: the wizard's connect step is the onboarding funnel's biggest drop (72.4% of engaged users skip it) and asked users to authorize a blind edit to their AI client's config. This PR shows the exact change before anything is written — and fixes a credential-hygiene bug found during review of the design.
Stacked on #799 (backup visibility); base is
feat/078-backup-visibilityand will retarget to main when #799 merges.What's included
Preview before write (US1)
GET /api/v1/connect/{client}/preview— side-effect-free: returns targetconfig_path, format (json/toml),server_name, the exact entry a connect would write (sharesbuildServerEntrywith the write path — preview and write cannot diverge, pinned by a table-driven test over the client matrix),entry_exists(overwrite case),contains_api_key, and the Spec-075access_state(TCC-denied → 403 + remediation).Security fix: no more API key in client configs
mcpURL()unconditionally embedded?apikey=<real REST-admin key>into every client config — while/mcpdoes not require auth by default (require_mcp_auth: false), so the credential protecting the full REST API leaked into files other apps own, for zero benefit.New behavior:
require_mcp_auth=false(default): cleanhttp://host:port/mcp— no credential anywhere.require_mcp_auth=true:X-API-Keyheader carrier for clients whose config supports headers; documented?apikey=fallback only where headers can't be expressed. Live config is consulted at connect/preview time (hot-reload safe).?apikey=are still recognized by disconnect/equivalence and upgraded to the clean shape on reconnect.••••) and never carries the real key.Verification (live, isolated instance)
contains_api_key:false, clean URL on default config.==previewed entry;grep -c apikey ~/.claude.json→ 0.require_mcp_auth=true: preview switches to maskedheaders: {X-API-Key: ••••}; real key absent from payload.connect/httpapirace-clean, frontend 246/246, golangci-lint v2 clean,make buildembeds.Part of specs/078-connect-trust-preview (US1 + the credential fix). Not included: US3 undo, US4 full TCC trust copy.
🤖 Generated with Claude Code