From 3ee7ada51688f19fe6a3951b014ccabb238fc8d5 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 24 Aug 2026 07:19:55 +0000 Subject: [PATCH 1/5] Redact cluster secret values from REST debug logs Amp-Thread-ID: https://ampcode.com/threads/T-01a0329d-9627-72fe-ad8f-c776efa2f64d Co-authored-by: Lachlan Donald --- pkg/cmd/factory/factory.go | 47 +++++++++++- pkg/cmd/factory/factory_test.go | 123 ++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index 5cf4b15b..3cdc3a23 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -2,6 +2,7 @@ package factory import ( "bytes" + "encoding/json" "fmt" "io" "net/http" @@ -109,7 +110,7 @@ func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { } if dump, err := httputil.DumpRequestOut(reqCopy, true); err == nil { - fmt.Fprintf(os.Stderr, "DEBUG request uri=%s\n%s\n", req.URL, redactBody(string(dump))) + fmt.Fprintf(os.Stderr, "DEBUG request uri=%s\n%s\n", req.URL, redactRequestDump(req, string(dump))) } resp, err := d.transport.RoundTrip(req) @@ -124,6 +125,50 @@ func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { return resp, nil } +func redactRequestDump(req *http.Request, dump string) string { + if !isClusterSecretValueRequest(req) { + return redactBody(dump) + } + + bodyStart := strings.Index(dump, "\r\n\r\n") + separatorLength := 4 + if bodyStart == -1 { + bodyStart = strings.Index(dump, "\n\n") + separatorLength = 2 + } + if bodyStart == -1 { + return "[REDACTED]" + } + bodyStart += separatorLength + + var body map[string]json.RawMessage + if err := json.Unmarshal([]byte(dump[bodyStart:]), &body); err != nil { + return redactBody(dump[:bodyStart] + "[REDACTED]") + } + if _, ok := body["value"]; !ok { + return redactBody(dump[:bodyStart] + "[REDACTED]") + } + body["value"] = json.RawMessage(`"[REDACTED]"`) + redacted, err := json.Marshal(body) + if err != nil { + return redactBody(dump[:bodyStart] + "[REDACTED]") + } + + return redactBody(dump[:bodyStart] + string(redacted)) +} + +func isClusterSecretValueRequest(req *http.Request) bool { + path := strings.TrimSuffix(req.URL.Path, "/") + parts := strings.Split(strings.Trim(path, "/"), "/") + if req.Method == http.MethodPost && len(parts) >= 3 { + return parts[len(parts)-3] == "clusters" && parts[len(parts)-1] == "secrets" + } + if req.Method == http.MethodPut && len(parts) >= 5 { + return parts[len(parts)-5] == "clusters" && parts[len(parts)-3] == "secrets" && parts[len(parts)-1] == "value" + } + return false +} + // sensitiveBodyPatterns matches token values in form-encoded request bodies // and JSON response bodies that should be redacted in debug output. // The closing JSON quote is optional so truncated response dumps fail closed. diff --git a/pkg/cmd/factory/factory_test.go b/pkg/cmd/factory/factory_test.go index 6e04cf1f..4d73daa7 100644 --- a/pkg/cmd/factory/factory_test.go +++ b/pkg/cmd/factory/factory_test.go @@ -1,6 +1,7 @@ package factory import ( + "bytes" "io" "net/http" "net/http/httptest" @@ -224,6 +225,128 @@ func TestDebugTransportHandlesNilBody(t *testing.T) { } } +func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { + tests := []struct { + name string + method string + path string + body string + secretMarkers []string + wantVisible string + }{ + { + name: "create secret", + method: http.MethodPost, + path: "/v2/organizations/test/clusters/cluster-1/secrets", + body: "{\n \"key\": \"API_KEY\",\n \"value\" : \"secret-prefix-\\\"quoted\\\"-\\\\path\\nsecret-suffix\"\n}", + secretMarkers: []string{"secret-prefix", "quoted", "secret-suffix"}, + wantVisible: `"key":"API_KEY"`, + }, + { + name: "update secret value", + method: http.MethodPut, + path: "/v2/organizations/test/clusters/cluster-1/secrets/secret-1/value", + body: `{ "value" : "replacement-secret" }`, + secretMarkers: []string{"replacement-secret"}, + }, + { + name: "malformed secret request fails closed", + method: http.MethodPost, + path: "/v2/organizations/test/clusters/cluster-1/secrets", + body: `{"key":"API_KEY","value":"truncated-secret`, + secretMarkers: []string{"API_KEY", "truncated-secret"}, + }, + { + name: "unrelated value remains visible", + method: http.MethodPost, + path: "/v2/organizations/test/pipelines", + body: `{"value": "useful-debug-value"}`, + wantVisible: `"value": "useful-debug-value"`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var forwardedBody string + transport := roundTripperFunc(func(req *http.Request) (*http.Response, error) { + body, err := io.ReadAll(req.Body) + if err != nil { + t.Fatalf("read forwarded request body: %v", err) + } + forwardedBody = string(body) + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader(`{"ok":true}`)), + Request: req, + }, nil + }) + + req, err := http.NewRequest(test.method, "https://api.buildkite.com"+test.path, strings.NewReader(test.body)) + if err != nil { + t.Fatalf("create request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + + stderr := captureStderr(t, func() { + resp, err := (&debugTransport{transport: transport}).RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip: %v", err) + } + resp.Body.Close() + }) + + if forwardedBody != test.body { + t.Errorf("forwarded body = %q, want %q", forwardedBody, test.body) + } + for _, marker := range test.secretMarkers { + if strings.Contains(stderr, marker) { + t.Errorf("stderr contains secret marker %q:\n%s", marker, stderr) + } + } + if len(test.secretMarkers) > 0 && !strings.Contains(stderr, "[REDACTED]") { + t.Errorf("stderr does not contain a redaction marker:\n%s", stderr) + } + if test.wantVisible != "" && !strings.Contains(stderr, test.wantVisible) { + t.Errorf("stderr does not contain %q:\n%s", test.wantVisible, stderr) + } + }) + } +} + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + + read, write, err := os.Pipe() + if err != nil { + t.Fatalf("create stderr pipe: %v", err) + } + original := os.Stderr + os.Stderr = write + t.Cleanup(func() { os.Stderr = original }) + + fn() + if err := write.Close(); err != nil { + t.Fatalf("close stderr pipe: %v", err) + } + os.Stderr = original + + var output bytes.Buffer + if _, err := io.Copy(&output, read); err != nil { + t.Fatalf("read stderr: %v", err) + } + if err := read.Close(); err != nil { + t.Fatalf("close stderr reader: %v", err) + } + return output.String() +} + func TestBuildUserAgent(t *testing.T) { t.Run("default user agent has no preflight suffix", func(t *testing.T) { got := buildUserAgent("") From 02a1993c637f0c557717361702aa0807b0eedca0 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 24 Aug 2026 22:42:28 +0000 Subject: [PATCH 2/5] Redact secret values for malformed cluster paths Co-authored-by: Lachlan Donald Amp-Thread-ID: https://ampcode.com/threads/T-01a03277-54a9-75bb-92a3-61efa877ba0b --- pkg/cmd/factory/factory.go | 11 +++++------ pkg/cmd/factory/factory_test.go | 8 ++++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index 3cdc3a23..4572e549 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -159,14 +159,13 @@ func redactRequestDump(req *http.Request, dump string) string { func isClusterSecretValueRequest(req *http.Request) bool { path := strings.TrimSuffix(req.URL.Path, "/") - parts := strings.Split(strings.Trim(path, "/"), "/") - if req.Method == http.MethodPost && len(parts) >= 3 { - return parts[len(parts)-3] == "clusters" && parts[len(parts)-1] == "secrets" + if !strings.Contains(path, "/clusters/") { + return false } - if req.Method == http.MethodPut && len(parts) >= 5 { - return parts[len(parts)-5] == "clusters" && parts[len(parts)-3] == "secrets" && parts[len(parts)-1] == "value" + if req.Method == http.MethodPost { + return strings.HasSuffix(path, "/secrets") } - return false + return req.Method == http.MethodPut && strings.Contains(path, "/secrets/") && strings.HasSuffix(path, "/value") } // sensitiveBodyPatterns matches token values in form-encoded request bodies diff --git a/pkg/cmd/factory/factory_test.go b/pkg/cmd/factory/factory_test.go index 4d73daa7..d61d29cb 100644 --- a/pkg/cmd/factory/factory_test.go +++ b/pkg/cmd/factory/factory_test.go @@ -249,6 +249,14 @@ func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { body: `{ "value" : "replacement-secret" }`, secretMarkers: []string{"replacement-secret"}, }, + { + name: "malformed cluster ID still redacts", + method: http.MethodPost, + path: "/v2/organizations/test/clusters/cluster-1//secrets", + body: `{"key":"API_KEY","value":"malformed-id-secret"}`, + secretMarkers: []string{"malformed-id-secret"}, + wantVisible: `"key":"API_KEY"`, + }, { name: "malformed secret request fails closed", method: http.MethodPost, From 37149dda15f663a34cc1665220b6cc70ab00a8ae Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 25 Aug 2026 06:00:07 +0000 Subject: [PATCH 3/5] Redact secret values across URL delimiters Co-authored-by: Lachlan Donald Amp-Thread-ID: https://ampcode.com/threads/T-01a03277-54a9-75bb-92a3-61efa877ba0b --- pkg/cmd/factory/factory.go | 15 +++++++++++---- pkg/cmd/factory/factory_test.go | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index 4572e549..61591ff8 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -158,14 +158,21 @@ func redactRequestDump(req *http.Request, dump string) string { } func isClusterSecretValueRequest(req *http.Request) bool { - path := strings.TrimSuffix(req.URL.Path, "/") - if !strings.Contains(path, "/clusters/") { + target := req.URL.Path + if req.URL.RawQuery != "" { + target += "?" + req.URL.RawQuery + } + if req.URL.Fragment != "" { + target += "#" + req.URL.Fragment + } + target = strings.TrimSuffix(target, "/") + if !strings.Contains(target, "/clusters/") { return false } if req.Method == http.MethodPost { - return strings.HasSuffix(path, "/secrets") + return strings.HasSuffix(target, "/secrets") } - return req.Method == http.MethodPut && strings.Contains(path, "/secrets/") && strings.HasSuffix(path, "/value") + return req.Method == http.MethodPut && strings.Contains(target, "/secrets/") && strings.HasSuffix(target, "/value") } // sensitiveBodyPatterns matches token values in form-encoded request bodies diff --git a/pkg/cmd/factory/factory_test.go b/pkg/cmd/factory/factory_test.go index d61d29cb..fb1a09f8 100644 --- a/pkg/cmd/factory/factory_test.go +++ b/pkg/cmd/factory/factory_test.go @@ -257,6 +257,21 @@ func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { secretMarkers: []string{"malformed-id-secret"}, wantVisible: `"key":"API_KEY"`, }, + { + name: "query delimiter in cluster ID still redacts", + method: http.MethodPost, + path: "/v2/organizations/test/clusters/cluster-1?/secrets", + body: `{"key":"API_KEY","value":"query-delimiter-secret"}`, + secretMarkers: []string{"query-delimiter-secret"}, + wantVisible: `"key":"API_KEY"`, + }, + { + name: "fragment delimiter in secret ID still redacts", + method: http.MethodPut, + path: "/v2/organizations/test/clusters/cluster-1/secrets/secret-1#/value", + body: `{"value":"fragment-delimiter-secret"}`, + secretMarkers: []string{"fragment-delimiter-secret"}, + }, { name: "malformed secret request fails closed", method: http.MethodPost, From 32b02cce6ad303e203190798263bf76882fee0ac Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 25 Aug 2026 06:38:36 +0000 Subject: [PATCH 4/5] Omit secret request bodies from debug logs Co-authored-by: Lachlan Donald Amp-Thread-ID: https://ampcode.com/threads/T-01a03277-54a9-75bb-92a3-61efa877ba0b --- pkg/cmd/factory/factory.go | 44 +++++++-------------------------- pkg/cmd/factory/factory_test.go | 21 +++++++--------- 2 files changed, 18 insertions(+), 47 deletions(-) diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index 61591ff8..b0c8fa54 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -2,7 +2,6 @@ package factory import ( "bytes" - "encoding/json" "fmt" "io" "net/http" @@ -85,6 +84,8 @@ type debugTransport struct { // sensitiveHeaders contains headers that should be redacted in debug output var sensitiveHeaders = []string{"Authorization"} +const omittedRequestBody = "[request body omitted]" + func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { // Save and restore the request body so that dumping it does not consume // the body before the real transport sends it. req.Clone() shares the @@ -109,8 +110,13 @@ func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { reqCopy.Body = io.NopCloser(bytes.NewReader(bodyBytes)) } - if dump, err := httputil.DumpRequestOut(reqCopy, true); err == nil { - fmt.Fprintf(os.Stderr, "DEBUG request uri=%s\n%s\n", req.URL, redactRequestDump(req, string(dump))) + includeBody := !isClusterSecretValueRequest(req) + if dump, err := httputil.DumpRequestOut(reqCopy, includeBody); err == nil { + redacted := redactBody(string(dump)) + if !includeBody { + redacted += "\n" + omittedRequestBody + } + fmt.Fprintf(os.Stderr, "DEBUG request uri=%s\n%s\n", req.URL, redacted) } resp, err := d.transport.RoundTrip(req) @@ -125,38 +131,6 @@ func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { return resp, nil } -func redactRequestDump(req *http.Request, dump string) string { - if !isClusterSecretValueRequest(req) { - return redactBody(dump) - } - - bodyStart := strings.Index(dump, "\r\n\r\n") - separatorLength := 4 - if bodyStart == -1 { - bodyStart = strings.Index(dump, "\n\n") - separatorLength = 2 - } - if bodyStart == -1 { - return "[REDACTED]" - } - bodyStart += separatorLength - - var body map[string]json.RawMessage - if err := json.Unmarshal([]byte(dump[bodyStart:]), &body); err != nil { - return redactBody(dump[:bodyStart] + "[REDACTED]") - } - if _, ok := body["value"]; !ok { - return redactBody(dump[:bodyStart] + "[REDACTED]") - } - body["value"] = json.RawMessage(`"[REDACTED]"`) - redacted, err := json.Marshal(body) - if err != nil { - return redactBody(dump[:bodyStart] + "[REDACTED]") - } - - return redactBody(dump[:bodyStart] + string(redacted)) -} - func isClusterSecretValueRequest(req *http.Request) bool { target := req.URL.Path if req.URL.RawQuery != "" { diff --git a/pkg/cmd/factory/factory_test.go b/pkg/cmd/factory/factory_test.go index fb1a09f8..495fa683 100644 --- a/pkg/cmd/factory/factory_test.go +++ b/pkg/cmd/factory/factory_test.go @@ -225,7 +225,7 @@ func TestDebugTransportHandlesNilBody(t *testing.T) { } } -func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { +func TestDebugTransportOmitsClusterSecretRequestBodies(t *testing.T) { tests := []struct { name string method string @@ -239,8 +239,7 @@ func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { method: http.MethodPost, path: "/v2/organizations/test/clusters/cluster-1/secrets", body: "{\n \"key\": \"API_KEY\",\n \"value\" : \"secret-prefix-\\\"quoted\\\"-\\\\path\\nsecret-suffix\"\n}", - secretMarkers: []string{"secret-prefix", "quoted", "secret-suffix"}, - wantVisible: `"key":"API_KEY"`, + secretMarkers: []string{"API_KEY", "secret-prefix", "quoted", "secret-suffix"}, }, { name: "update secret value", @@ -250,23 +249,21 @@ func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { secretMarkers: []string{"replacement-secret"}, }, { - name: "malformed cluster ID still redacts", + name: "malformed cluster ID still omits body", method: http.MethodPost, path: "/v2/organizations/test/clusters/cluster-1//secrets", body: `{"key":"API_KEY","value":"malformed-id-secret"}`, - secretMarkers: []string{"malformed-id-secret"}, - wantVisible: `"key":"API_KEY"`, + secretMarkers: []string{"API_KEY", "malformed-id-secret"}, }, { - name: "query delimiter in cluster ID still redacts", + name: "query delimiter in cluster ID still omits body", method: http.MethodPost, path: "/v2/organizations/test/clusters/cluster-1?/secrets", body: `{"key":"API_KEY","value":"query-delimiter-secret"}`, - secretMarkers: []string{"query-delimiter-secret"}, - wantVisible: `"key":"API_KEY"`, + secretMarkers: []string{"API_KEY", "query-delimiter-secret"}, }, { - name: "fragment delimiter in secret ID still redacts", + name: "fragment delimiter in secret ID still omits body", method: http.MethodPut, path: "/v2/organizations/test/clusters/cluster-1/secrets/secret-1#/value", body: `{"value":"fragment-delimiter-secret"}`, @@ -327,8 +324,8 @@ func TestDebugTransportRedactsClusterSecretValues(t *testing.T) { t.Errorf("stderr contains secret marker %q:\n%s", marker, stderr) } } - if len(test.secretMarkers) > 0 && !strings.Contains(stderr, "[REDACTED]") { - t.Errorf("stderr does not contain a redaction marker:\n%s", stderr) + if len(test.secretMarkers) > 0 && !strings.Contains(stderr, omittedRequestBody) { + t.Errorf("stderr does not say the request body was omitted:\n%s", stderr) } if test.wantVisible != "" && !strings.Contains(stderr, test.wantVisible) { t.Errorf("stderr does not contain %q:\n%s", test.wantVisible, stderr) From d57c504e300d2e8dc0c2cf88ca5a21ba13addb5f Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 25 Aug 2026 07:17:09 +0000 Subject: [PATCH 5/5] Keep secret body omission with query strings Co-authored-by: Lachlan Donald Amp-Thread-ID: https://ampcode.com/threads/T-01a03277-54a9-75bb-92a3-61efa877ba0b --- pkg/cmd/factory/factory.go | 16 ++++++++++++---- pkg/cmd/factory/factory_test.go | 7 +++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index b0c8fa54..35c984e7 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -132,21 +132,29 @@ func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { } func isClusterSecretValueRequest(req *http.Request) bool { - target := req.URL.Path + path := strings.TrimSuffix(req.URL.Path, "/") + if isClusterSecretValueTarget(req.Method, path) { + return true + } + + target := path if req.URL.RawQuery != "" { target += "?" + req.URL.RawQuery } if req.URL.Fragment != "" { target += "#" + req.URL.Fragment } - target = strings.TrimSuffix(target, "/") + return isClusterSecretValueTarget(req.Method, strings.TrimSuffix(target, "/")) +} + +func isClusterSecretValueTarget(method, target string) bool { if !strings.Contains(target, "/clusters/") { return false } - if req.Method == http.MethodPost { + if method == http.MethodPost { return strings.HasSuffix(target, "/secrets") } - return req.Method == http.MethodPut && strings.Contains(target, "/secrets/") && strings.HasSuffix(target, "/value") + return method == http.MethodPut && strings.Contains(target, "/secrets/") && strings.HasSuffix(target, "/value") } // sensitiveBodyPatterns matches token values in form-encoded request bodies diff --git a/pkg/cmd/factory/factory_test.go b/pkg/cmd/factory/factory_test.go index 495fa683..47fca92b 100644 --- a/pkg/cmd/factory/factory_test.go +++ b/pkg/cmd/factory/factory_test.go @@ -248,6 +248,13 @@ func TestDebugTransportOmitsClusterSecretRequestBodies(t *testing.T) { body: `{ "value" : "replacement-secret" }`, secretMarkers: []string{"replacement-secret"}, }, + { + name: "query string after secret endpoint still omits body", + method: http.MethodPost, + path: "/v2/organizations/test/clusters/cluster-1/secrets?somequerystring=something", + body: `{"key":"API_KEY","value":"query-string-secret"}`, + secretMarkers: []string{"API_KEY", "query-string-secret"}, + }, { name: "malformed cluster ID still omits body", method: http.MethodPost,