Skip to content

Commit e138559

Browse files
rudycelekliDumbrisclaude
authored
fix(httpapi): refuse config-to-secret for values that embed a reference (#1782)
## Bug `POST /api/v1/servers/{id}/config-to-secret` refuses to convert a value that is already a reference, but the check is `strings.HasPrefix(value, "${keyring:") || strings.HasPrefix(value, "${env:")`. Two kinds of values get past it: - a `${file:/path}` reference (the provider added in #1717); - a composite value with an embedded reference, such as the documented `"Authorization": "Bearer ${keyring:github-token}"` (`docs/features/keyring-integration.md`). For these values the handler stores the literal text in the keyring and rewrites the field to `${keyring:<secret_name>}`. Secret expansion runs once, so on the next connect the upstream gets the unresolved text, for example `Bearer ${keyring:github-token}` or `${file:/run/secrets/github-token}`, as its credential. The API masks both values on the read path because `IsConfigReference` only passes through an exact `${keyring:..}` or `${env:..}` value. The UI therefore shows them as plain secrets and offers Convert-to-secret. ## Trigger ```json "headers": {"Authorization": "Bearer ${keyring:github-token}"} ``` `POST /api/v1/servers/<name>/config-to-secret` with `{"scope":"header","key":"Authorization","secret_name":"gh2"}` succeeds. After that, the header sent upstream is the literal `Bearer ${keyring:github-token}`. ## Fix Use `secret.IsSecretRef(value)`, which matches any `${type:name}` reference anywhere in the value. The handler then returns the existing 400 "already a reference" error for these values. ## Test Two cases added to `TestHandleConvertConfigToSecret_ValidationErrors`: an env value `${file:/run/secrets/github-token}` and a header `Bearer ${keyring:github-token}`. Both must be rejected with 400 before the resolver is called. ## Commands run - `CGO_ENABLED=0 go test -count=1 ./internal/httpapi`: ok - With `server.go` reverted, both new cases fail (`expected: 400, actual: 500`). The handler gets past validation and reaches the resolver, which the mock leaves nil. - `go vet ./internal/httpapi`, `gofmt -l internal/httpapi/`: clean --------- Co-authored-by: Algis Dumbris <a.dumbris@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 95837ad commit e138559

2 files changed

Lines changed: 42 additions & 7 deletions

File tree

‎internal/httpapi/patch_server_test.go‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -412,11 +412,12 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) {
412412
logger := zap.NewNop().Sugar()
413413

414414
cases := []struct {
415-
name string
416-
existing *config.ServerConfig
417-
body string
418-
wantStatus int
419-
wantInBody string
415+
name string
416+
existing *config.ServerConfig
417+
body string
418+
wantStatus int
419+
wantInBody string
420+
wantNotInBody string
420421
}{
421422
{
422423
name: "missing scope",
@@ -476,6 +477,37 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) {
476477
wantStatus: 400,
477478
wantInBody: "already a reference",
478479
},
480+
{
481+
name: "value is already a file reference",
482+
existing: &config.ServerConfig{
483+
Name: "synapbus", Protocol: "stdio", Enabled: true,
484+
Env: map[string]string{"GITHUB_TOKEN": "${file:/run/secrets/github-token}"},
485+
},
486+
body: `{"scope": "env", "key": "GITHUB_TOKEN", "secret_name": "synapbus-auth"}`,
487+
wantStatus: 400,
488+
wantInBody: "already a reference",
489+
},
490+
{
491+
name: "value embeds a keyring reference",
492+
existing: &config.ServerConfig{
493+
Name: "synapbus", Protocol: "http", Enabled: true,
494+
Headers: map[string]string{"Authorization": "Bearer ${keyring:github-token}"},
495+
},
496+
body: `{"scope": "header", "key": "Authorization", "secret_name": "synapbus-auth"}`,
497+
wantStatus: 400,
498+
wantInBody: "already a reference",
499+
},
500+
{
501+
name: "mixed value does not echo the literal part",
502+
existing: &config.ServerConfig{
503+
Name: "synapbus", Protocol: "stdio", Enabled: true,
504+
Env: map[string]string{"DATABASE_URL": "postgres://app:hunter2@${env:DB_HOST}/app"},
505+
},
506+
body: `{"scope": "env", "key": "DATABASE_URL", "secret_name": "synapbus-db"}`,
507+
wantStatus: 400,
508+
wantInBody: "already a reference",
509+
wantNotInBody: "hunter2",
510+
},
479511
{
480512
name: "empty value",
481513
existing: &config.ServerConfig{
@@ -504,6 +536,9 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) {
504536

505537
require.Equal(t, tc.wantStatus, w.Code, "body=%s", w.Body.String())
506538
require.Contains(t, w.Body.String(), tc.wantInBody)
539+
if tc.wantNotInBody != "" {
540+
require.NotContains(t, w.Body.String(), tc.wantNotInBody)
541+
}
507542
// The update path must not have been invoked when validation
508543
// rejects the request.
509544
assert.Nil(t, mockCtrl.capturedUpdates, "validation errors must not call UpdateServer")

‎internal/httpapi/server.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3169,8 +3169,8 @@ func (s *Server) handleConvertConfigToSecret(w http.ResponseWriter, r *http.Requ
31693169
s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q has no value to store", req.Scope, req.Key))
31703170
return
31713171
}
3172-
if strings.HasPrefix(value, "${keyring:") || strings.HasPrefix(value, "${env:") {
3173-
s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q is already a reference (%s); nothing to convert", req.Scope, req.Key, value))
3172+
if secret.IsSecretRef(value) {
3173+
s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q is already a reference; nothing to convert", req.Scope, req.Key))
31743174
return
31753175
}
31763176

0 commit comments

Comments
 (0)