From 7899662aee9da72b697ba598cd73da9193627493 Mon Sep 17 00:00:00 2001 From: BaaRai <130491490+BaaRai@users.noreply.github.com> Date: Fri, 13 Nov 2026 21:58:19 +0100 Subject: [PATCH] secrets: keep a masked tool secret only for its saved destination As for the MCP env (#213), a placeholder in a custom tool's exec keeps the stored header, proxy or body only while the url (exact string) and the kind are unchanged. A changed or emptied url, or a kind change, with a resolved placeholder is refused (400 on the API and the test endpoint, a tool error for update_custom_tool) before any write and before the test request is sent. An explicit value with a new url is allowed. --- server/customtool.go | 26 ++++++---- server/platform_tools.go | 4 +- server/tools_secrets_api_test.go | 82 ++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/server/customtool.go b/server/customtool.go index d75888c..3a081dc 100644 --- a/server/customtool.go +++ b/server/customtool.go @@ -74,7 +74,7 @@ func (s *Server) pgCreateCustomTool(w http.ResponseWriter, r *http.Request) { return } var err error - if req.Exec, err = unmaskToolExec(nil, req.Exec); err != nil { // nothing stored: a placeholder is refused + if req.Exec, err = unmaskToolExec("", nil, req.Kind, req.Exec); err != nil { // nothing stored: a placeholder is refused writeErr(w, 400, err.Error()) return } @@ -116,7 +116,7 @@ func (s *Server) pgUpdateCustomTool(w http.ResponseWriter, r *http.Request) { writeErr(w, 400, "http tools must provide a parameter JSON Schema (cannot be left empty)") return } - if req.Exec, err = unmaskToolExec(existing.Exec, req.Exec); err != nil { + if req.Exec, err = unmaskToolExec(existing.Kind, existing.Exec, req.Kind, req.Exec); err != nil { writeErr(w, 400, err.Error()) return } @@ -167,6 +167,7 @@ func (s *Server) pgTestCustomTool(w http.ResponseWriter, r *http.Request) { writeErr(w, 400, "invalid request body") return } + var storedKind string var stored json.RawMessage if req.Key != "" { t, err := pg.GetTool(req.Key) @@ -175,11 +176,11 @@ func (s *Server) pgTestCustomTool(w http.ResponseWriter, r *http.Request) { return } if t != nil && !t.System { - stored = t.Exec + storedKind, stored = t.Kind, t.Exec } } var err error - if req.Exec, err = unmaskToolExec(stored, req.Exec); err != nil { + if req.Exec, err = unmaskToolExec(storedKind, stored, req.Kind, req.Exec); err != nil { writeErr(w, 400, err.Error()) return } @@ -294,14 +295,16 @@ func maskToolExec(raw json.RawMessage) json.RawMessage { // unmaskToolExec puts the stored value back for every header, proxy or body sent // as the placeholder, so a client echoing the list response keeps the stored // secret; an explicit value replaces it and "" clears it. A placeholder with -// nothing stored to keep (stored nil on create) is refused. -func unmaskToolExec(stored, in json.RawMessage) (json.RawMessage, error) { +// nothing stored to keep (stored nil on create) is refused. As for the MCP env, +// a stored secret is only kept for the destination it was configured for: when +// the url (exact string) or the kind changes, a placeholder is refused. +func unmaskToolExec(storedKind string, stored json.RawMessage, kind string, in json.RawMessage) (json.RawMessage, error) { var e, old map[string]any if json.Unmarshal(in, &e) != nil || e == nil { return in, nil // not an object: stored or refused as before } _ = json.Unmarshal(stored, &old) - changed := false + changed, kept := false, false for _, k := range toolExecFlags { if _, ok := e[k]; ok { delete(e, k) @@ -318,7 +321,7 @@ func unmaskToolExec(stored, in json.RawMessage) (json.RawMessage, error) { if !ok { return nil, fmt.Errorf("header %q: masked value but nothing stored to keep", k) } - headers[k], changed = prev, true + headers[k], changed, kept = prev, true, true } for _, k := range []string{"proxy", "body"} { if s, ok := e[k].(string); !ok || !isMaskedSecret(s) { @@ -328,7 +331,12 @@ func unmaskToolExec(stored, in json.RawMessage) (json.RawMessage, error) { if prev == "" { return nil, fmt.Errorf("%s: masked value but nothing stored to keep", k) } - e[k], changed = prev, true + e[k], changed, kept = prev, true, true + } + oldURL, _ := old["url"].(string) + newURL, _ := e["url"].(string) + if kept && (oldURL != newURL || storedKind != kind) { + return nil, fmt.Errorf("destination changed (url or kind) while a stored secret was kept: re-enter the masked header/proxy/body or clear it; stored secrets are not sent to a new destination") } if !changed { return in, nil diff --git a/server/platform_tools.go b/server/platform_tools.go index 297d093..65bebf9 100644 --- a/server/platform_tools.go +++ b/server/platform_tools.go @@ -217,7 +217,7 @@ func (s *Server) toolCreateCustomTool() actool.CoreTool { return actool.Errorf("this key already exists: " + a.Key), nil } var err error - if a.Exec, err = unmaskToolExec(nil, a.Exec); err != nil { // nothing stored: a placeholder is refused + if a.Exec, err = unmaskToolExec("", nil, a.Kind, a.Exec); err != nil { // nothing stored: a placeholder is refused return actool.Errorf(err.Error()), nil } if err := s.m.pg.CreateCustomTool(toDBTool(a)); err != nil { @@ -244,7 +244,7 @@ func (s *Server) toolUpdateCustomTool() actool.CoreTool { return actool.Errorf("http tools must provide a parameter JSON Schema (cannot be empty)"), nil } var err error - if a.Exec, err = unmaskToolExec(existing.Exec, a.Exec); err != nil { + if a.Exec, err = unmaskToolExec(existing.Kind, existing.Exec, a.Kind, a.Exec); err != nil { return actool.Errorf(err.Error()), nil } if err := s.m.pg.UpdateCustomTool(toDBTool(a)); err != nil { diff --git a/server/tools_secrets_api_test.go b/server/tools_secrets_api_test.go index 97b8b38..123ad73 100644 --- a/server/tools_secrets_api_test.go +++ b/server/tools_secrets_api_test.go @@ -7,9 +7,91 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" ) +// A placeholder keeps the stored secret only for the url and kind it was saved +// with: a changed destination is refused before any write or request. +func TestCustomToolExecDestinationGuard(t *testing.T) { + s, _, request := trafficEvidenceServer(t) + pg := s.m.PG() + var hits atomic.Int32 + echo := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + io.WriteString(w, "auth="+r.Header.Get("Authorization")) + })) + t.Cleanup(echo.Close) + t.Cleanup(func() { pg.Exec(`DELETE FROM tools WHERE key LIKE 't_secguard%'`) }) + const secret = "Bearer guard-plain-0001" + schema := `{"type":"object","properties":{"q":{"type":"string"}}}` + body := func(kind, url, hdr string) string { + e, _ := json.Marshal(map[string]any{"method": "GET", "url": url, "headers": map[string]string{"Authorization": hdr}}) + return `{"key":"t_secguard","kind":"` + kind + `","enabled":true,"schema":` + schema + `,"exec":` + string(e) + `}` + } + storedExec := func() (url, auth string) { + tl, err := pg.GetTool("t_secguard") + if err != nil || tl == nil { + t.Fatalf("GetTool: %v", err) + } + var e struct { + URL string `json:"url"` + Headers map[string]string `json:"headers"` + } + _ = json.Unmarshal(tl.Exec, &e) + return e.URL, e.Headers["Authorization"] + } + if resp := request("POST", "/api/tools/custom", body("http", echo.URL, secret)); resp.Code != 200 { + t.Fatalf("create: HTTP %d %s", resp.Code, resp.Body.String()) + } + moved := echo.URL + "/elsewhere" + + // 1. Masked header, changed url: refused, nothing written. + if resp := request("PUT", "/api/tools/custom/t_secguard", body("http", moved, "__masked__")); resp.Code != 400 { + t.Fatalf("masked header with a changed url must be refused, got HTTP %d", resp.Code) + } + if url, auth := storedExec(); url != echo.URL || auth != secret { + t.Fatalf("a refused update changed the stored exec: %s %s", url, auth) + } + // An empty url against a stored one is a change too. + if resp := request("PUT", "/api/tools/custom/t_secguard", body("http", "", "__masked__")); resp.Code != 400 { + t.Fatalf("masked header with an emptied url must be refused, got HTTP %d", resp.Code) + } + + // 2. Masked header, same url: kept. + if resp := request("PUT", "/api/tools/custom/t_secguard", body("http", echo.URL, "__masked__")); resp.Code != 200 { + t.Fatalf("masked header with the same url: HTTP %d %s", resp.Code, resp.Body.String()) + } + if url, auth := storedExec(); url != echo.URL || auth != secret { + t.Fatalf("the stored secret was not kept: %s %s", url, auth) + } + + // 3. Test endpoint, masked header, changed url: refused, no request sent. + e, _ := json.Marshal(map[string]any{"method": "GET", "url": moved, "headers": map[string]string{"Authorization": "__masked__"}}) + if resp := request("POST", "/api/tools/custom/test", `{"key":"t_secguard","kind":"http","exec":`+string(e)+`}`); resp.Code != 400 { + t.Fatalf("test endpoint with a changed url must be refused, got HTTP %d %s", resp.Code, resp.Body.String()) + } + if n := hits.Load(); n != 0 { + t.Fatalf("the echo server received %d request(s); the stored secret left", n) + } + + // 4. Kind change while keeping a placeholder: refused. + if resp := request("PUT", "/api/tools/custom/t_secguard", body("command", echo.URL, "__masked__")); resp.Code != 400 { + t.Fatalf("a kind change with a placeholder must be refused, got HTTP %d", resp.Code) + } + if tl, _ := pg.GetTool("t_secguard"); tl == nil || tl.Kind != "http" { + t.Fatalf("a refused kind change was written: %v", tl) + } + + // 5. An explicit value with a changed url is allowed. + if resp := request("PUT", "/api/tools/custom/t_secguard", body("http", moved, "Bearer typed-0002")); resp.Code != 200 { + t.Fatalf("explicit header with a changed url: HTTP %d %s", resp.Code, resp.Body.String()) + } + if url, auth := storedExec(); url != moved || auth != "Bearer typed-0002" { + t.Fatalf("explicit replace not stored: %s %s", url, auth) + } +} + // A custom tool's exec: headers, proxy and body are masked by GET /api/tools; // the placeholder keeps the stored value on update and in the test endpoint, a // create with it is refused, and the executor runs on the clear exec.