From 7f8a548a4567ae77924ff18eb19407821cbfe865 Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Sun, 25 Oct 2026 11:12:10 -0400 Subject: [PATCH 1/2] fix(httpapi): refuse config-to-secret for values that embed a reference The guard only rejected values starting with ${keyring: or ${env:. A ${file:...} reference, or a composite such as "Bearer ${keyring:gh}", was stored in the keyring as literal text and the field was rewritten to ${keyring:}. Resolution is single-pass, so the upstream then received the unresolved reference text as its credential. Use secret.IsSecretRef so any value containing a reference is refused. --- internal/httpapi/patch_server_test.go | 20 ++++++++++++++++++++ internal/httpapi/server.go | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/httpapi/patch_server_test.go b/internal/httpapi/patch_server_test.go index 9fc5b8c9a..e0d59c3df 100644 --- a/internal/httpapi/patch_server_test.go +++ b/internal/httpapi/patch_server_test.go @@ -476,6 +476,26 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) { wantStatus: 400, wantInBody: "already a reference", }, + { + name: "value is already a file reference", + existing: &config.ServerConfig{ + Name: "synapbus", Protocol: "stdio", Enabled: true, + Env: map[string]string{"GITHUB_TOKEN": "${file:/run/secrets/github-token}"}, + }, + body: `{"scope": "env", "key": "GITHUB_TOKEN", "secret_name": "synapbus-auth"}`, + wantStatus: 400, + wantInBody: "already a reference", + }, + { + name: "value embeds a keyring reference", + existing: &config.ServerConfig{ + Name: "synapbus", Protocol: "http", Enabled: true, + Headers: map[string]string{"Authorization": "Bearer ${keyring:github-token}"}, + }, + body: `{"scope": "header", "key": "Authorization", "secret_name": "synapbus-auth"}`, + wantStatus: 400, + wantInBody: "already a reference", + }, { name: "empty value", existing: &config.ServerConfig{ diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index ebd28a1e5..b582a7882 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -3165,7 +3165,7 @@ func (s *Server) handleConvertConfigToSecret(w http.ResponseWriter, r *http.Requ s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q has no value to store", req.Scope, req.Key)) return } - if strings.HasPrefix(value, "${keyring:") || strings.HasPrefix(value, "${env:") { + if secret.IsSecretRef(value) { s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q is already a reference (%s); nothing to convert", req.Scope, req.Key, value)) return } From 8a8bcb8f183a0c574cd11e83e8d287151bd93151 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 26 Oct 2026 13:19:37 +0200 Subject: [PATCH 2/2] fix(httpapi): don't echo the raw value when config-to-secret refuses a reference Review finding (P2): the broader IsSecretRef guard sends mixed values such as "postgres://app:hunter2@${env:DB_HOST}/app" to the 400 branch, which echoed the full value and leaked its literal credential part even with reveal_secret_headers off. The error now names only the scope and key. Co-Authored-By: Claude Opus 5.5 --- internal/httpapi/patch_server_test.go | 25 ++++++++++++++++++++----- internal/httpapi/server.go | 2 +- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/internal/httpapi/patch_server_test.go b/internal/httpapi/patch_server_test.go index e0d59c3df..4f078f255 100644 --- a/internal/httpapi/patch_server_test.go +++ b/internal/httpapi/patch_server_test.go @@ -412,11 +412,12 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) { logger := zap.NewNop().Sugar() cases := []struct { - name string - existing *config.ServerConfig - body string - wantStatus int - wantInBody string + name string + existing *config.ServerConfig + body string + wantStatus int + wantInBody string + wantNotInBody string }{ { name: "missing scope", @@ -496,6 +497,17 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) { wantStatus: 400, wantInBody: "already a reference", }, + { + name: "mixed value does not echo the literal part", + existing: &config.ServerConfig{ + Name: "synapbus", Protocol: "stdio", Enabled: true, + Env: map[string]string{"DATABASE_URL": "postgres://app:hunter2@${env:DB_HOST}/app"}, + }, + body: `{"scope": "env", "key": "DATABASE_URL", "secret_name": "synapbus-db"}`, + wantStatus: 400, + wantInBody: "already a reference", + wantNotInBody: "hunter2", + }, { name: "empty value", existing: &config.ServerConfig{ @@ -524,6 +536,9 @@ func TestHandleConvertConfigToSecret_ValidationErrors(t *testing.T) { require.Equal(t, tc.wantStatus, w.Code, "body=%s", w.Body.String()) require.Contains(t, w.Body.String(), tc.wantInBody) + if tc.wantNotInBody != "" { + require.NotContains(t, w.Body.String(), tc.wantNotInBody) + } // The update path must not have been invoked when validation // rejects the request. assert.Nil(t, mockCtrl.capturedUpdates, "validation errors must not call UpdateServer") diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index b582a7882..d20b6475d 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -3166,7 +3166,7 @@ func (s *Server) handleConvertConfigToSecret(w http.ResponseWriter, r *http.Requ return } if secret.IsSecretRef(value) { - s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q is already a reference (%s); nothing to convert", req.Scope, req.Key, value)) + s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("%s %q is already a reference; nothing to convert", req.Scope, req.Key)) return }