From a4d9bbce2fbcd3f3d41f3cc9bbb6dd4d69fea026 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 29 May 2026 17:28:38 -0400 Subject: [PATCH 1/2] fix(sdk/go): strip credential headers on cross-host redirect The control-plane client attaches X-API-Key (and, when DID auth is configured, the X-Caller-DID / X-DID-Signature / X-DID-Timestamp / X-DID-Nonce headers) as custom request headers on an http.Client with no CheckRedirect. Go's net/http strips Authorization/Cookie on a cross-host redirect but not arbitrary application headers, so a redirect from the configured baseURL to another host would replay the operator's credentials to that host. Add a CheckRedirect hook that drops every credential header the client attaches when a redirect targets a host other than the originally configured one (compared against via[0], so credentials only reach the operator-configured host across a multi-hop chain). Same-host redirects keep the headers, so ordinary redirect following is unaffected. The credential list is sourced from the did_auth header constants so it cannot drift. Also applied to clients supplied via WithHTTPClient that define no redirect policy. Refs GHSA-jp8j-g39q-qxwx. Co-Authored-By: Claude Opus 4.8 (1M context) --- sdk/go/client/client.go | 47 ++++++- sdk/go/client/client_redirect_test.go | 184 ++++++++++++++++++++++++++ 2 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 sdk/go/client/client_redirect_test.go diff --git a/sdk/go/client/client.go b/sdk/go/client/client.go index 3860e33e..d7a23585 100644 --- a/sdk/go/client/client.go +++ b/sdk/go/client/client.go @@ -28,10 +28,16 @@ type Client struct { // Option mutates Client configuration. type Option func(*Client) -// WithHTTPClient allows custom HTTP transport configuration. +// WithHTTPClient allows custom HTTP transport configuration. If the supplied +// client does not define its own redirect policy, the credential-stripping +// policy (stripSensitiveHeadersOnRedirect) is applied so a custom client does +// not silently reintroduce the cross-host credential leak (GHSA-jp8j-g39q-qxwx). func WithHTTPClient(h *http.Client) Option { return func(c *Client) { if h != nil { + if h.CheckRedirect == nil { + h.CheckRedirect = stripSensitiveHeadersOnRedirect + } c.httpClient = h } } @@ -65,6 +71,42 @@ func WithDIDAuth(did, privateKeyJWK string) Option { } } +// sensitiveCrossHostHeaders are every credential-bearing request header this +// client attaches in do(): the static control-plane credentials plus the +// per-request DID signature headers. None of them may be replayed to a host +// other than the one the operator configured. Go's net/http already strips +// Authorization, Cookie, Cookie2 and WWW-Authenticate on a cross-host redirect, +// but it does NOT strip arbitrary application headers such as X-API-Key or our +// DID headers, so we strip the full set explicitly. The DID entries reference +// the did_auth constants so this list cannot drift if a header is renamed. +// See GHSA-jp8j-g39q-qxwx. +var sensitiveCrossHostHeaders = []string{ + "Authorization", + "X-API-Key", + HeaderCallerDID, + HeaderDIDSignature, + HeaderDIDTimestamp, + HeaderDIDNonce, +} + +// stripSensitiveHeadersOnRedirect is an http.Client.CheckRedirect hook that +// removes credential headers when a redirect targets a host other than the +// originally configured one. The comparison is against via[0] (the first +// request) so credentials only ever reach the operator-configured host, even +// across a multi-hop redirect chain. Same-host redirects keep the headers so +// ordinary redirect following (e.g. a trailing-slash or path redirect on the +// same host) still works, while a redirect to an attacker-controlled host can +// no longer exfiltrate the operator's control-plane credentials. +func stripSensitiveHeadersOnRedirect(req *http.Request, via []*http.Request) error { + if len(via) == 0 || req.URL.Host == via[0].URL.Host { + return nil + } + for _, h := range sensitiveCrossHostHeaders { + req.Header.Del(h) + } + return nil +} + // New creates a new Client instance. func New(baseURL string, opts ...Option) (*Client, error) { if baseURL == "" { @@ -79,7 +121,8 @@ func New(baseURL string, opts ...Option) (*Client, error) { c := &Client{ baseURL: parsed, httpClient: &http.Client{ - Timeout: 10 * time.Second, + Timeout: 10 * time.Second, + CheckRedirect: stripSensitiveHeadersOnRedirect, }, } diff --git a/sdk/go/client/client_redirect_test.go b/sdk/go/client/client_redirect_test.go new file mode 100644 index 00000000..7f66a790 --- /dev/null +++ b/sdk/go/client/client_redirect_test.go @@ -0,0 +1,184 @@ +package client + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +// These tests cover GHSA-jp8j-g39q-qxwx: the SDK control-plane client must not +// replay any credential header it attaches (the static X-API-Key / Authorization +// and the per-request DID signature headers) to a host other than the configured +// baseURL when following a redirect. +// +// Validation contract: +// - cross-host redirect -> EVERY credential header MUST NOT reach the new host +// - same-host redirect -> credential headers MUST still be sent (no regression) +// - no redirect -> credential header MUST be sent (sanity) +// - non-credential headers (e.g. Accept) MUST be preserved in all cases + +// TestStripsCredentialsOnCrossHostRedirect mirrors the reporter's PoC: the +// operator endpoint 302-redirects to a different host, which records the headers +// it receives. The two httptest servers share 127.0.0.1 but differ by port, so +// url.Host (host:port) differs and the redirect is treated as cross-host. +func TestStripsCredentialsOnCrossHostRedirect(t *testing.T) { + var reached bool + var gotKey, gotAuth string + + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + gotKey = r.Header.Get("X-API-Key") + gotAuth = r.Header.Get("Authorization") + _, _ = w.Write([]byte(`{}`)) + })) + defer attacker.Close() + + operator := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, attacker.URL+r.URL.Path, http.StatusFound) + })) + defer operator.Close() + + c, err := New(operator.URL, WithAPIKey("SECRET-AGENTFIELD-KEY"), WithBearerToken("SECRET-BEARER")) + if err != nil { + t.Fatalf("New: %v", err) + } + _, _ = c.GetNode(context.Background(), "node-1") + + if !reached { + t.Fatal("redirect target was never reached; test setup is wrong") + } + if gotKey != "" { + t.Errorf("X-API-Key leaked to cross-host redirect target: got %q, want empty", gotKey) + } + if gotAuth != "" { + t.Errorf("Authorization leaked to cross-host redirect target: got %q, want empty", gotAuth) + } +} + +// TestKeepsCredentialsOnSameHostRedirect ensures we did not break ordinary +// same-host redirect following: a path-only redirect on the same server must +// still carry the credential to the final handler. +func TestKeepsCredentialsOnSameHostRedirect(t *testing.T) { + var finalReached bool + var gotKey string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/final" { + finalReached = true + gotKey = r.Header.Get("X-API-Key") + _, _ = w.Write([]byte(`{}`)) + return + } + http.Redirect(w, r, "/final", http.StatusFound) + })) + defer srv.Close() + + c, err := New(srv.URL, WithAPIKey("SECRET-AGENTFIELD-KEY")) + if err != nil { + t.Fatalf("New: %v", err) + } + _, _ = c.GetNode(context.Background(), "node-1") + + if !finalReached { + t.Fatal("same-host redirect target was never reached") + } + if gotKey != "SECRET-AGENTFIELD-KEY" { + t.Errorf("X-API-Key dropped on same-host redirect: got %q, want %q", gotKey, "SECRET-AGENTFIELD-KEY") + } +} + +// TestSendsCredentialsWithoutRedirect is the baseline: without any redirect the +// configured credential reaches the server. +func TestSendsCredentialsWithoutRedirect(t *testing.T) { + var gotKey string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotKey = r.Header.Get("X-API-Key") + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + c, err := New(srv.URL, WithAPIKey("SECRET-AGENTFIELD-KEY")) + if err != nil { + t.Fatalf("New: %v", err) + } + _, _ = c.GetNode(context.Background(), "node-1") + + if gotKey != "SECRET-AGENTFIELD-KEY" { + t.Errorf("X-API-Key not sent on direct request: got %q, want %q", gotKey, "SECRET-AGENTFIELD-KEY") + } +} + +// TestStripSensitiveHeadersOnRedirectUnit deterministically exercises the +// CheckRedirect hook over the FULL set of credential headers the client +// attaches (static creds + DID signature headers), without needing a live DID +// setup or TLS. It also guards that a non-credential header is preserved. +func TestStripSensitiveHeadersOnRedirectUnit(t *testing.T) { + mustURL := func(s string) *url.URL { + u, err := url.Parse(s) + if err != nil { + t.Fatalf("parse %q: %v", s, err) + } + return u + } + credHeaders := func() http.Header { + h := http.Header{} + h.Set("Authorization", "Bearer SECRET") + h.Set("X-API-Key", "SECRET-KEY") + h.Set(HeaderCallerDID, "did:web:example.com:agents:a") + h.Set(HeaderDIDSignature, "sig") + h.Set(HeaderDIDTimestamp, "123") + h.Set(HeaderDIDNonce, "nonce") + h.Set("Accept", "application/json") // non-credential: must survive + return h + } + orig := &http.Request{URL: mustURL("https://cp.example.com/api/v1/nodes/n1")} + + t.Run("cross-host strips every credential header", func(t *testing.T) { + req := &http.Request{ + URL: mustURL("https://attacker.example.net/api/v1/nodes/n1"), + Header: credHeaders(), + } + if err := stripSensitiveHeadersOnRedirect(req, []*http.Request{orig}); err != nil { + t.Fatalf("hook returned error: %v", err) + } + for _, h := range sensitiveCrossHostHeaders { + if v := req.Header.Get(h); v != "" { + t.Errorf("credential header %q leaked cross-host: %q", h, v) + } + } + if got := req.Header.Get("Accept"); got != "application/json" { + t.Errorf("non-credential header Accept was dropped: %q", got) + } + }) + + t.Run("same-host preserves every credential header", func(t *testing.T) { + req := &http.Request{ + URL: mustURL("https://cp.example.com/v2/nodes/n1"), + Header: credHeaders(), + } + if err := stripSensitiveHeadersOnRedirect(req, []*http.Request{orig}); err != nil { + t.Fatalf("hook returned error: %v", err) + } + for _, h := range sensitiveCrossHostHeaders { + if req.Header.Get(h) == "" { + t.Errorf("credential header %q was dropped on same-host redirect", h) + } + } + }) + + t.Run("no prior request is a no-op", func(t *testing.T) { + req := &http.Request{ + URL: mustURL("https://attacker.example.net/x"), + Header: credHeaders(), + } + if err := stripSensitiveHeadersOnRedirect(req, nil); err != nil { + t.Fatalf("hook returned error: %v", err) + } + if req.Header.Get("X-API-Key") == "" { + t.Error("headers must be untouched when there is no prior request") + } + }) +} From 39d37bb9eb07f9e9cf9d5fddcdb14424ecb8c235 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 29 May 2026 18:16:03 -0400 Subject: [PATCH 2/2] fix(handlers): stop flaky coverage CI from global-logger race in parallel tests TestDiscoveryLoggingIncludesOptionalRequestID and TestExecutionCleanupService_StartStopBranches both call setupExecutionCleanupTestLogger, which swaps the process-global logger.Logger, while also calling t.Parallel(). When they ran alongside another parallel test, a sibling reassigned the global logger mid-run, so the discovery test's captured buffer came back empty and its assertions ("discovery request failed", `"request_id":"req-123"`, `"format":"compact"`) failed. This broke the `coverage (control-plane)` job and cascaded into `coverage-summary` (missing control-plane.total.txt). The non-test build is unaffected, so it slipped past the required gates and only showed up in the coverage job's full parallel run. Drop t.Parallel() from both tests so they run in the serial phase, where nothing else mutates the global logger concurrently -- matching the existing non-parallel logger-swapping tests in execution_cleanup_test.go. Verified by running the previously-failing command 20x clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../handlers/coverage_handlers_92_target_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/control-plane/internal/handlers/coverage_handlers_92_target_test.go b/control-plane/internal/handlers/coverage_handlers_92_target_test.go index ac366881..1bb461c2 100644 --- a/control-plane/internal/handlers/coverage_handlers_92_target_test.go +++ b/control-plane/internal/handlers/coverage_handlers_92_target_test.go @@ -207,7 +207,11 @@ func TestAgentConcurrencyLimiter_MaxPerAgentNilAndConfigured(t *testing.T) { } func TestExecutionCleanupService_StartStopBranches(t *testing.T) { - t.Parallel() + // NOT t.Parallel(): this test swaps the process-global logger.Logger via + // setupExecutionCleanupTestLogger. Running it in parallel let a sibling test + // reassign the global logger mid-run, contaminating log buffers and flaking + // the coverage CI job. Keep it in the serial phase, like the other + // logger-swapping tests in execution_cleanup_test.go. t.Run("disabled start is no-op", func(t *testing.T) { service := NewExecutionCleanupService(&cleanupStoreMock{}, config.ExecutionCleanupConfig{Enabled: false}) @@ -341,7 +345,10 @@ func TestIsLightweightRequestQueryVariants(t *testing.T) { } func TestDiscoveryLoggingIncludesOptionalRequestID(t *testing.T) { - t.Parallel() + // NOT t.Parallel(): this test swaps the process-global logger.Logger via + // setupExecutionCleanupTestLogger and then asserts on the captured buffer. + // In parallel, a sibling test reassigning the global logger emptied this + // buffer and flaked the coverage CI job. Keep it in the serial phase. gin.SetMode(gin.TestMode) logBuffer := setupExecutionCleanupTestLogger(t)