Skip to content

Commit fc4051f

Browse files
committed
feat(headers): unify display + server-side convert-to-secret for redacted values
User feedback: the Headers card looked split-brained. A non-sensitive header showed `••••XX (NN chars)` with a Convert-to-secret button. The Authorization header showed `***REDACTED***` with no button — exactly the most-likely candidate for "move me to keyring" was the one the UI refused to convert, because the client side couldn't hand the real value to the existing two-step (POST /secrets, then PATCH server) flow. This change unifies both display and conversion: 1. **Single mask format on the wire** — internal/oauth/logging.go replaces the `***REDACTED***` sentinel in RedactStringHeaders with MaskValue(v), producing `••••<last2> (<N> chars)` for literal secrets. The same format the Web UI / macOS tray have been computing client-side. Bare ${keyring:NAME} / ${env:VAR} references pass through unchanged (they're labels, not secrets; the UI needs to recognise them to render the keyring chip). 2. **Server-side atomic convert** — new endpoint: POST /api/v1/servers/{name}/config-to-secret body: {"scope": "header"|"env", "key": "<k>", "secret_name": "<n>"} The backend reads the real value from the loaded config, stores it in the OS keyring under secret_name, and rewrites the config field with ${keyring:<n>}. The client never has to possess the plaintext, so Convert-to-secret now works on the redacted-on-read path too. 3. **Reveal button removed from KVValueCell.vue** — with all literals displayed identically and Convert-to-secret available everywhere, the reveal toggle was a security-shaped speed bump with no real use case. The two paths to peek at a value (open the config file, or edit-cancel) remain. revealedKeys reactive state and the reveal/hide events disappear from the parent too. 4. **Symmetric macOS Swift cleanup** — ServerDetailView.swift::kvRow drops the `value != "***REDACTED***"` gate on the Convert-to-secret button. maskedHeaderValue() drops the sentinel special case. performConvertToSecret() calls the new atomic endpoint via the new APIClient.convertConfigToSecret helper instead of two-stepping through storeSecret + PATCH. Backend tests updated for the new format: - internal/oauth/logging_test.go::TestRedactStringHeaders — asserts the ••••et (40 chars) mask shape with length + last-2 suffix. New sub-tests pin the keyring/env-ref pass-through, short-value (<=4 chars -> bare ••••), and empty-value ((empty)) edge cases. - internal/runtime/event_bus_payload_test.go — SSE redaction test asserts the new format on the wire. - internal/server/e2e_test.go — both PR #425 round-trip tests now assert mask shape instead of literal sentinel. New backend test coverage: - internal/httpapi/patch_server_test.go — 9 new sub-tests for the /config-to-secret endpoint validation paths: missing scope / key / secret_name, invalid scope, key not on server, value already a reference (keyring + env separately), empty value, server not found. Happy-path lives in the live verification because secret.Resolver is a concrete struct without a mock. End-to-end verification on live local mcpproxy: GET /api/v1/servers -> synapbus.headers.Authorization = ••••59 (71 chars) # new format -> kaggle.headers.Authorization = ••••N} (30 chars) # Bearer\${k...} also masked POST /api/v1/servers/synapbus/config-to-secret {"scope":"header","key":"Authorization","secret_name":"synapbus-authorization"} -> 200 {"reference":"\${keyring:synapbus-authorization}"} -> on disk: {"Authorization": "\${keyring:synapbus-authorization}"} -> GET response: passes the bare reference through unchanged Web UI: Headers row transformed from `Authorization ••••59 (71 chars) [lock] Convert to secret` to `Authorization [key-chip] stored in keyring: synapbus-authorization` via the Convert-to-secret modal — no intermediate steps for the user, no plaintext on the client. CLI: `upstream patch synapbus --header X-Cli-Verify=hello` / `upstream patch synapbus --header-remove X-Cli-Verify` still work; the keyring reference on Authorization survives both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f952885 commit fc4051f

13 files changed

Lines changed: 596 additions & 138 deletions

File tree

frontend/src/components/KVValueCell.vue

Lines changed: 39 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -13,53 +13,43 @@
1313
<button class="btn btn-xs btn-ghost" :disabled="busy" @click="$emit('cancel-edit')">Cancel</button>
1414
</div>
1515
<div v-else class="flex gap-2 items-center flex-wrap">
16-
<!-- Keyring reference: rendered as a chip; no reveal toggle. -->
16+
<!-- Keyring reference: rendered as a chip; no convert affordance. -->
1717
<template v-if="isKeyringRef">
1818
<span class="badge badge-info badge-sm gap-1" :title="rawValue">
1919
<span aria-hidden="true">🔑</span>
2020
<span class="font-mono text-xs">stored in keyring: {{ keyringName }}</span>
2121
</span>
2222
</template>
23-
<!-- Env reference (${env:VAR}): also a chip with no reveal toggle. -->
23+
<!-- Env reference (${env:VAR}): also a chip. -->
2424
<template v-else-if="isEnvRef">
2525
<span class="badge badge-ghost badge-sm gap-1" :title="rawValue">
2626
<span aria-hidden="true">$</span>
2727
<span class="font-mono text-xs">env var: {{ envRefName }}</span>
2828
</span>
2929
</template>
30-
<!-- Literal value path. Two sub-cases:
31-
1. Backend redacted the value (`***REDACTED***` sentinel). The
32-
real string isn't on the client; reveal would just expose
33-
the sentinel and Convert-to-secret has nothing to upload to
34-
the keyring. We surface a small "backend-redacted" hint
35-
instead so the user understands why those actions are
36-
disabled. Editing the value still works through the inline
37-
edit button — the PATCH endpoint deep-merges, so typing a
38-
new value replaces just that one header server-side.
39-
2. Plaintext literal: mask by default, eye to reveal, lock to
40-
convert. -->
30+
<!-- Literal value path. Two cases produce identical visuals:
31+
1. Header value the backend already masked (`••••<last2> (<N> chars)`
32+
format from oauth.RedactStringHeaders) — we render the masked
33+
string verbatim. Convert-to-secret on these rows hits the
34+
server-side `/config-to-secret` endpoint, which has the real
35+
value on disk.
36+
2. Plaintext literal (env vars, or headers when
37+
`reveal_secret_headers: true`) — we mask client-side using
38+
the same format so it looks the same to the user.
39+
The UI no longer has a reveal button; the two routes to peek
40+
at the value (edit + cancel, or open the config file) are
41+
documented in CLAUDE.md. -->
4142
<template v-else>
42-
<code v-if="isBackendRedacted" class="bg-base-200 px-1.5 py-0.5 rounded text-xs text-base-content/50" title="Backend redacted this value. Edit to overwrite — the PATCH endpoint deep-merges, so the unchanged real value is preserved.">{{ rawValue }}</code>
43-
<template v-else>
44-
<code v-if="revealed" class="bg-base-200 px-1.5 py-0.5 rounded text-xs break-all">{{ rawValue }}</code>
45-
<code v-else class="bg-base-200 px-1.5 py-0.5 rounded text-xs text-base-content/50">{{ redactedPreview }}</code>
46-
<button
47-
class="btn btn-ghost btn-xs"
48-
:title="revealed ? 'Hide value' : 'Reveal value'"
49-
@click="revealed ? $emit('hide') : $emit('reveal')"
50-
>
51-
<span aria-hidden="true">{{ revealed ? '🙈' : '👁' }}</span>
52-
</button>
53-
<button
54-
class="btn btn-ghost btn-xs"
55-
title="Move value into the OS keyring and reference it as ${keyring:name}"
56-
@click="$emit('convert')"
57-
:disabled="busy"
58-
>
59-
<span aria-hidden="true">🔒</span>
60-
<span class="hidden md:inline ml-1">Convert to secret</span>
61-
</button>
62-
</template>
43+
<code class="bg-base-200 px-1.5 py-0.5 rounded text-xs text-base-content/70 font-mono break-all">{{ displayValue }}</code>
44+
<button
45+
class="btn btn-ghost btn-xs"
46+
title="Move value into the OS keyring and reference it as ${keyring:name}"
47+
@click="$emit('convert')"
48+
:disabled="busy"
49+
>
50+
<span aria-hidden="true">🔒</span>
51+
<span class="hidden md:inline ml-1">Convert to secret</span>
52+
</button>
6353
</template>
6454
<button class="btn btn-ghost btn-xs" title="Edit" @click="$emit('start-edit')" :disabled="busy">✎</button>
6555
<button class="btn btn-ghost btn-xs text-error" title="Delete" @click="$emit('delete')" :disabled="busy">✕</button>
@@ -70,22 +60,19 @@
7060
import { computed, nextTick, ref, watch } from 'vue'
7161
7262
// KVValueCell encapsulates the per-row UI for the Headers and Environment
73-
// Variables cards on ServerDetail. It owns the visual state (revealed vs
74-
// hidden, edit-in-progress draft) and emits intent events; the parent owns
75-
// the persistence (PATCH the server, refresh the store).
63+
// Variables cards on ServerDetail. It owns the visual state (edit-in-
64+
// progress draft) and emits intent events; the parent owns the
65+
// persistence (PATCH the server, call config-to-secret, refresh).
7666
7767
const props = defineProps<{
7868
scope: 'header' | 'env'
7969
k: string
8070
rawValue: string
8171
isEditing: boolean
82-
revealed: boolean
8372
busy?: boolean
8473
}>()
8574
8675
const emit = defineEmits<{
87-
(e: 'reveal'): void
88-
(e: 'hide'): void
8976
(e: 'start-edit'): void
9077
(e: 'cancel-edit'): void
9178
(e: 'save', value: string): void
@@ -123,20 +110,21 @@ const envRefName = computed(() => {
123110
const m = (props.rawValue ?? '').match(/^\$\{env:([^}]+)\}$/)
124111
return m ? m[1] : ''
125112
})
126-
// The Go backend redacts secret header values via
127-
// `redactServerHeaders` so an MCP agent calling `upstream_servers list`
128-
// cannot exfiltrate Bearer tokens (PR #425). The REST API and SSE
129-
// channel inherit the same redaction. When that sentinel surfaces, the
130-
// real string is not in our hands — reveal/convert are disabled, but
131-
// editing still works because the PATCH endpoint deep-merges (the
132-
// untouched redacted key simply stays out of the patch body).
133-
const isBackendRedacted = computed(() => props.rawValue === '***REDACTED***')
134113
135-
const redactedPreview = computed(() => {
114+
// Render the literal value with the masked-display format. If the
115+
// backend already masked the value (sensitive header, default redaction
116+
// policy), it arrives already in this exact format — we pass it
117+
// through unchanged. If the backend sent plaintext (env var, or headers
118+
// when `reveal_secret_headers: true`), we mask client-side here for
119+
// consistency. The detection is "starts with a mask bullet", which is
120+
// idempotent: re-masking a masked string is a no-op.
121+
const MASK_PREFIX = '••••'
122+
const displayValue = computed(() => {
136123
const v = props.rawValue ?? ''
137124
if (!v) return '(empty)'
138-
if (v.length <= 4) return '••••'
139-
return '••••' + v.slice(-2) + ` (${v.length} chars)`
125+
if (v.startsWith(MASK_PREFIX) || v === '(empty)') return v
126+
if (v.length <= 4) return MASK_PREFIX
127+
return MASK_PREFIX + v.slice(-2) + ` (${v.length} chars)`
140128
})
141129
142130
const placeholder = computed(() =>

frontend/src/services/api.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,15 +294,37 @@ class APIService {
294294

295295
// storeSecret stashes a value in the OS keyring under the given name and
296296
// returns the ${keyring:<name>} reference string callers can substitute
297-
// back into the server config. Used by the "Convert to secret" affordance
298-
// on the Headers and Environment Variables cards.
297+
// back into the server config. Kept for legacy callers; the
298+
// Headers/Environment Variables "Convert to secret" flow now uses the
299+
// atomic convertConfigToSecret() instead.
299300
async storeSecret(name: string, value: string): Promise<APIResponse<{ reference?: string }>> {
300301
return this.request<{ reference?: string }>('/api/v1/secrets', {
301302
method: 'POST',
302303
body: JSON.stringify({ name, value, type: 'keyring' }),
303304
})
304305
}
305306

307+
// convertConfigToSecret asks the backend to atomically (a) read the real
308+
// value of a header / env key from the server config, (b) store it in
309+
// the OS keyring under `secretName`, and (c) rewrite the config field
310+
// with the `${keyring:<name>}` reference. The client never has to
311+
// possess the real value — which matters when the API redacts
312+
// sensitive header values on the read path.
313+
async convertConfigToSecret(
314+
serverName: string,
315+
scope: 'header' | 'env',
316+
key: string,
317+
secretName: string
318+
): Promise<APIResponse<{ reference?: string }>> {
319+
return this.request<{ reference?: string }>(
320+
`/api/v1/servers/${encodeURIComponent(serverName)}/config-to-secret`,
321+
{
322+
method: 'POST',
323+
body: JSON.stringify({ scope, key, secret_name: secretName }),
324+
}
325+
)
326+
}
327+
306328
async getServerTools(serverName: string): Promise<APIResponse<{ tools: Tool[] }>> {
307329
return this.request<{ tools: Tool[] }>(`/api/v1/servers/${encodeURIComponent(serverName)}/tools`)
308330
}

frontend/src/views/ServerDetail.vue

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -676,10 +676,7 @@
676676
:k="k"
677677
:raw-value="serverHeaders[k]"
678678
:is-editing="editingKey === `hdr::${k}`"
679-
:revealed="revealedKeys.has(`hdr::${k}`)"
680679
:busy="kvPatchInFlight"
681-
@reveal="revealedKeys.add(`hdr::${k}`)"
682-
@hide="revealedKeys.delete(`hdr::${k}`)"
683680
@start-edit="startEdit('hdr', k)"
684681
@cancel-edit="cancelEdit"
685682
@save="(val) => saveEdit('header', k, val)"
@@ -743,10 +740,7 @@
743740
:k="k"
744741
:raw-value="serverEnv[k]"
745742
:is-editing="editingKey === `env::${k}`"
746-
:revealed="revealedKeys.has(`env::${k}`)"
747743
:busy="kvPatchInFlight"
748-
@reveal="revealedKeys.add(`env::${k}`)"
749-
@hide="revealedKeys.delete(`env::${k}`)"
750744
@start-edit="startEdit('env', k)"
751745
@cancel-edit="cancelEdit"
752746
@save="(val) => saveEdit('env', k, val)"
@@ -2321,7 +2315,6 @@ const envKeys = computed(() => Object.keys(serverEnv.value).sort())
23212315
const hasHeaders = computed(() => headerKeys.value.length > 0)
23222316
const hasEnv = computed(() => envKeys.value.length > 0)
23232317
2324-
const revealedKeys = ref<Set<string>>(new Set())
23252318
const editingKey = ref<string | null>(null)
23262319
const kvPatchInFlight = ref(false)
23272320
@@ -2461,19 +2454,22 @@ function closeConvertModal() {
24612454
24622455
async function commitConvert() {
24632456
const m = convertModal.value
2464-
if (!m.secretName || !m.rawValue) return
2457+
if (!m.secretName || !server.value) return
24652458
m.busy = true
24662459
try {
2467-
const storeResp = await api.storeSecret(m.secretName, m.rawValue)
2468-
if (!storeResp.success) {
2469-
systemStore.addToast({ type: 'error', title: 'Failed to store secret', message: storeResp.error || 'Unknown error' })
2460+
// Atomic server-side conversion: the backend reads the real value
2461+
// from the loaded config (so we don't need the plaintext on the
2462+
// client — important when the API redacts sensitive headers on the
2463+
// read path), stores it in keyring, and rewrites the config field
2464+
// with the ${keyring:NAME} reference. Single round-trip, single
2465+
// failure surface.
2466+
const resp = await api.convertConfigToSecret(server.value.name, m.scope, m.key, m.secretName)
2467+
if (!resp.success) {
2468+
systemStore.addToast({ type: 'error', title: 'Convert failed', message: resp.error || 'Unknown error' })
24702469
return
24712470
}
2472-
const ref = `\${keyring:${m.secretName}}`
2473-
await patchServerDiff(
2474-
{ [scopeKey(m.scope)]: { [m.key]: ref } },
2475-
`Converted ${m.key} to secret`
2476-
)
2471+
await serversStore.fetchServers(true)
2472+
systemStore.addToast({ type: 'success', title: `Converted ${m.key} to secret`, message: '' })
24772473
closeConvertModal()
24782474
} catch (e: any) {
24792475
systemStore.addToast({ type: 'error', title: 'Convert failed', message: e?.message || String(e) })

internal/httpapi/patch_server_test.go

Lines changed: 133 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,9 @@ func TestHandlePatchServer_ExplicitBoolsTakePrecedence(t *testing.T) {
133133
// TestHandlePatchServer_HeadersDeepMerge verifies that PATCH /api/v1/servers
134134
// preserves existing header keys not mentioned in the request body. This is
135135
// the foundation of the Web UI / macOS tray edit flow: clients send a diff
136-
// against the redacted view of headers, so any key whose value is
137-
// `***REDACTED***` and unchanged stays out of the patch — and the backend
138-
// must NOT wipe it.
136+
// against the redacted view of headers, so any key whose masked-display
137+
// value (`••••<last2> (<N> chars)`) is unchanged stays out of the patch —
138+
// and the backend must NOT wipe it.
139139
func TestHandlePatchServer_HeadersDeepMerge(t *testing.T) {
140140
logger := zap.NewNop().Sugar()
141141
mockCtrl := &mockPatchServerController{
@@ -154,8 +154,8 @@ func TestHandlePatchServer_HeadersDeepMerge(t *testing.T) {
154154
srv := NewServer(mockCtrl, logger, nil)
155155

156156
// Client sends just X-New-Header. Authorization is omitted because
157-
// its redacted view (`***REDACTED***`) matched the original, and
158-
// X-Trace is omitted because the user didn't touch it.
157+
// its masked view (`••••<last2> (<N> chars)`) matched the original
158+
// in the diff, and X-Trace is omitted because the user didn't touch it.
159159
body, _ := json.Marshal(map[string]any{
160160
"headers": map[string]string{"X-New-Header": "new-value"},
161161
})
@@ -335,3 +335,131 @@ func TestHandlePatchServer_HeadersEmptyStringSetsNotDeletes(t *testing.T) {
335335
assert.Equal(t, "value", got["X-Original"], "X-Original must be preserved")
336336
assert.Len(t, got, 2)
337337
}
338+
339+
// TestHandleConvertConfigToSecret_ValidationErrors covers the input
340+
// validation paths on POST /api/v1/servers/{id}/config-to-secret that
341+
// happen BEFORE we touch the secret resolver — so they're testable with
342+
// a nil resolver via the existing mock. Happy-path lives in the live
343+
// verification scripts because secret.Resolver is a concrete struct and
344+
// faking it would mean stubbing the whole keyring provider chain.
345+
func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) {
346+
logger := zap.NewNop().Sugar()
347+
348+
cases := []struct {
349+
name string
350+
existing *config.ServerConfig
351+
body string
352+
wantStatus int
353+
wantInBody string
354+
}{
355+
{
356+
name: "missing scope",
357+
existing: &config.ServerConfig{Name: "synapbus", Protocol: "http", Enabled: true},
358+
body: `{"key": "Authorization", "secret_name": "synapbus-auth"}`,
359+
wantStatus: 400,
360+
wantInBody: `scope`,
361+
},
362+
{
363+
name: "invalid scope",
364+
existing: &config.ServerConfig{Name: "synapbus", Protocol: "http", Enabled: true},
365+
body: `{"scope": "isolation", "key": "image", "secret_name": "foo"}`,
366+
wantStatus: 400,
367+
wantInBody: `scope`,
368+
},
369+
{
370+
name: "missing key",
371+
existing: &config.ServerConfig{Name: "synapbus", Protocol: "http", Enabled: true},
372+
body: `{"scope": "header", "secret_name": "synapbus-auth"}`,
373+
wantStatus: 400,
374+
wantInBody: `key`,
375+
},
376+
{
377+
name: "missing secret name",
378+
existing: &config.ServerConfig{Name: "synapbus", Protocol: "http", Enabled: true},
379+
body: `{"scope": "header", "key": "Authorization"}`,
380+
wantStatus: 400,
381+
wantInBody: `secret_name`,
382+
},
383+
{
384+
name: "key not present on server",
385+
existing: &config.ServerConfig{
386+
Name: "synapbus", Protocol: "http", Enabled: true,
387+
Headers: map[string]string{"X-Trace": "on"},
388+
},
389+
body: `{"scope": "header", "key": "Authorization", "secret_name": "synapbus-auth"}`,
390+
wantStatus: 404,
391+
wantInBody: "Authorization",
392+
},
393+
{
394+
name: "value is already a keyring reference",
395+
existing: &config.ServerConfig{
396+
Name: "synapbus", Protocol: "http", Enabled: true,
397+
Headers: map[string]string{"Authorization": "${keyring:already-stored}"},
398+
},
399+
body: `{"scope": "header", "key": "Authorization", "secret_name": "synapbus-auth"}`,
400+
wantStatus: 400,
401+
wantInBody: "already a reference",
402+
},
403+
{
404+
name: "value is already an env reference",
405+
existing: &config.ServerConfig{
406+
Name: "synapbus", Protocol: "http", Enabled: true,
407+
Headers: map[string]string{"Authorization": "${env:FOO}"},
408+
},
409+
body: `{"scope": "header", "key": "Authorization", "secret_name": "synapbus-auth"}`,
410+
wantStatus: 400,
411+
wantInBody: "already a reference",
412+
},
413+
{
414+
name: "empty value",
415+
existing: &config.ServerConfig{
416+
Name: "synapbus", Protocol: "http", Enabled: true,
417+
Headers: map[string]string{"Authorization": ""},
418+
},
419+
body: `{"scope": "header", "key": "Authorization", "secret_name": "synapbus-auth"}`,
420+
wantStatus: 400,
421+
wantInBody: "no value to store",
422+
},
423+
}
424+
425+
for _, tc := range cases {
426+
t.Run(tc.name, func(t *testing.T) {
427+
mockCtrl := &mockPatchServerController{
428+
apiKey: "test-key",
429+
existingServer: tc.existing,
430+
}
431+
srv := NewServer(mockCtrl, logger, nil)
432+
433+
req := httptest.NewRequest(http.MethodPost, "/api/v1/servers/synapbus/config-to-secret", bytes.NewReader([]byte(tc.body)))
434+
req.Header.Set("Content-Type", "application/json")
435+
req.Header.Set("X-API-Key", "test-key")
436+
w := httptest.NewRecorder()
437+
srv.ServeHTTP(w, req)
438+
439+
require.Equal(t, tc.wantStatus, w.Code, "body=%s", w.Body.String())
440+
require.Contains(t, w.Body.String(), tc.wantInBody)
441+
// The update path must not have been invoked when validation
442+
// rejects the request.
443+
assert.Nil(t, mockCtrl.capturedUpdates, "validation errors must not call UpdateServer")
444+
})
445+
}
446+
}
447+
448+
// TestHandleConvertConfigToSecret_ServerNotFound verifies the 404 path
449+
// for an unknown server name. Separate from the validation-errors table
450+
// because it needs an empty server list, not a single populated entry.
451+
func TestHandleConvertConfigToSecret_ServerNotFound(t *testing.T) {
452+
logger := zap.NewNop().Sugar()
453+
mockCtrl := &mockPatchServerController{apiKey: "test-key", existingServer: nil}
454+
srv := NewServer(mockCtrl, logger, nil)
455+
456+
body := []byte(`{"scope": "header", "key": "Authorization", "secret_name": "x"}`)
457+
req := httptest.NewRequest(http.MethodPost, "/api/v1/servers/missing/config-to-secret", bytes.NewReader(body))
458+
req.Header.Set("Content-Type", "application/json")
459+
req.Header.Set("X-API-Key", "test-key")
460+
w := httptest.NewRecorder()
461+
srv.ServeHTTP(w, req)
462+
463+
require.Equal(t, http.StatusNotFound, w.Code, "body=%s", w.Body.String())
464+
require.Contains(t, w.Body.String(), `missing`)
465+
}

0 commit comments

Comments
 (0)