diff --git a/pkg/cmd/factory/factory.go b/pkg/cmd/factory/factory.go index 5cf4b15b..35c984e7 100644 --- a/pkg/cmd/factory/factory.go +++ b/pkg/cmd/factory/factory.go @@ -84,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 @@ -108,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, redactBody(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) @@ -124,6 +131,32 @@ func (d *debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { return resp, nil } +func isClusterSecretValueRequest(req *http.Request) bool { + 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 + } + return isClusterSecretValueTarget(req.Method, strings.TrimSuffix(target, "/")) +} + +func isClusterSecretValueTarget(method, target string) bool { + if !strings.Contains(target, "/clusters/") { + return false + } + if method == http.MethodPost { + return strings.HasSuffix(target, "/secrets") + } + return method == http.MethodPut && strings.Contains(target, "/secrets/") && strings.HasSuffix(target, "/value") +} + // 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..47fca92b 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,155 @@ func TestDebugTransportHandlesNilBody(t *testing.T) { } } +func TestDebugTransportOmitsClusterSecretRequestBodies(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{"API_KEY", "secret-prefix", "quoted", "secret-suffix"}, + }, + { + 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: "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, + path: "/v2/organizations/test/clusters/cluster-1//secrets", + body: `{"key":"API_KEY","value":"malformed-id-secret"}`, + secretMarkers: []string{"API_KEY", "malformed-id-secret"}, + }, + { + 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{"API_KEY", "query-delimiter-secret"}, + }, + { + 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"}`, + secretMarkers: []string{"fragment-delimiter-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, 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) + } + }) + } +} + +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("")