From e64113b45edc3904556482da4f9040776dc85670 Mon Sep 17 00:00:00 2001 From: Joe Coleman Date: Wed, 15 Jul 2026 15:48:25 +0100 Subject: [PATCH 1/3] Add Needles accessor to Mux --- internal/replacer/mux.go | 16 ++++++++++++++++ internal/replacer/mux_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 internal/replacer/mux_test.go diff --git a/internal/replacer/mux.go b/internal/replacer/mux.go index 28b7fa4305..682f297be9 100644 --- a/internal/replacer/mux.go +++ b/internal/replacer/mux.go @@ -32,6 +32,22 @@ func (m *Mux) Add(needles ...string) { } } +// Needles returns the deduplicated set of needles across all replacers. +func (m *Mux) Needles() []string { + seen := make(map[string]struct{}) + needles := make([]string, 0) + for _, r := range m.underlying { + for _, n := range r.Needles() { + if _, ok := seen[n]; ok { + continue + } + seen[n] = struct{}{} + needles = append(needles, n) + } + } + return needles +} + // Append adds a replacer to the Mux. func (m *Mux) Append(r *Replacer) { m.underlying = append(m.underlying, r) diff --git a/internal/replacer/mux_test.go b/internal/replacer/mux_test.go new file mode 100644 index 0000000000..f118b6bc89 --- /dev/null +++ b/internal/replacer/mux_test.go @@ -0,0 +1,32 @@ +package replacer + +import ( + "io" + "slices" + "testing" +) + +func passthrough(b []byte) []byte { return b } + +func TestMuxNeedlesDeduplicates(t *testing.T) { + t.Parallel() + + r1 := New(io.Discard, []string{"alpha", "bravo"}, passthrough) + r2 := New(io.Discard, []string{"bravo", "charlie"}, passthrough) + m := NewMux(r1, r2) + + got := m.Needles() + slices.Sort(got) + want := []string{"alpha", "bravo", "charlie"} + if !slices.Equal(got, want) { + t.Errorf("Mux.Needles() = %v, want %v (deduplicated across replacers)", got, want) + } +} + +func TestMuxNeedlesEmpty(t *testing.T) { + t.Parallel() + + if got := NewMux().Needles(); len(got) != 0 { + t.Errorf("empty Mux.Needles() = %v, want empty", got) + } +} From fbac14d8a917f7fa53ea50e032bfb320882e2d84 Mon Sep 17 00:00:00 2001 From: Joe Coleman Date: Wed, 15 Jul 2026 15:48:44 +0100 Subject: [PATCH 2/3] Redact secrets before quoting values in env-change log --- internal/job/executor.go | 13 ++++- .../integration/secrets_integration_test.go | 58 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/internal/job/executor.go b/internal/job/executor.go index 71613e885d..dd904aa466 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -679,7 +679,7 @@ func (e *Executor) applyEnvironmentChanges(changes hook.EnvChanges) { for k, v := range changes.Diff.Added { if _, ok := executorConfigEnvChanges[k]; ok { executorConfigEnvChangesLogged[k] = true - e.shell.Commentf("%s is now %q", k, v) + e.shell.Commentf("%s is now %s", k, e.redactedValue(v)) } else { e.shell.Commentf("%s added", k) } @@ -687,7 +687,7 @@ func (e *Executor) applyEnvironmentChanges(changes hook.EnvChanges) { for k, v := range changes.Diff.Changed { if _, ok := executorConfigEnvChanges[k]; ok { executorConfigEnvChangesLogged[k] = true - e.shell.Commentf("%s was %q and is now %q", k, v.Old, v.New) + e.shell.Commentf("%s was %s and is now %s", k, e.redactedValue(v.Old), e.redactedValue(v.New)) } else { e.shell.Commentf("%s changed", k) } @@ -705,11 +705,18 @@ func (e *Executor) applyEnvironmentChanges(changes hook.EnvChanges) { // it might not appear in the script "changes". for k, v := range executorConfigEnvChanges { if !executorConfigEnvChangesLogged[k] { - e.shell.Commentf("%s is now %q", k, v) + e.shell.Commentf("%s is now %s", k, e.redactedValue(v)) } } } +// redactedValue strips known secrets from a value before %q formatting. +// %q escapes newlines, so the output redactor can't match a multiline secret +// like an SSH key once it's formatted. +func (e *Executor) redactedValue(value string) string { + return fmt.Sprintf("%q", redact.String(value, e.redactors.Needles())) +} + // Should be called whenever we updated our e.shell.Env. func (e *Executor) addOutputRedactors() { // reset output redactors based on new environment variable values diff --git a/internal/job/integration/secrets_integration_test.go b/internal/job/integration/secrets_integration_test.go index 192d893b37..c0acc87183 100644 --- a/internal/job/integration/secrets_integration_test.go +++ b/internal/job/integration/secrets_integration_test.go @@ -366,6 +366,64 @@ func TestSecretsIntegration_MultilineSecretRedaction(t *testing.T) { } } +// A secret mapped onto a config var like BUILDKITE_GIT_SSH_KEY used to be +// printed by the env change log after a hook ran. %q escaped its newlines so +// the output redactor missed it. +func TestSecretsIntegration_ConfigVarSecretNotLoggedByEnvChange(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("setting up executor tester: %v", err) + } + defer tester.Close() + + // The token makes any leaked rendering of the key easy to spot, raw or + // %q escaped. + const keyToken = "SYNTHETICKEYDONOTUSEFAKEFAKEFAKE" + multilineKey := "-----BEGIN OPENSSH PRIVATE KEY-----\n" + + "b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtz\n" + + "c2gtZWQyNTUxOQAAACD" + keyToken + "0123456789abcdefghij\n" + + "-----END OPENSSH PRIVATE KEY-----" + + secretsMap := map[string]string{"REPO_DEPLOY_KEY": multilineKey} + apiServer := setupSecretsAPIServer(t, secretsMap) + defer apiServer.Close() + + secrets := []pipeline.Secret{ + { + Key: "REPO_DEPLOY_KEY", + EnvironmentVariable: "BUILDKITE_GIT_SSH_KEY", + }, + } + secretsJSON, err := json.Marshal(secrets) + if err != nil { + t.Fatalf("marshaling secrets: %v", err) + } + + // Running any hook is what triggers the env change log for the key. + tester.ExpectGlobalHook("environment").AndCallFunc(func(c *bintest.Call) { + c.Exit(0) + }) + tester.ExpectGlobalHook("command").AndCallFunc(func(c *bintest.Call) { + c.Exit(0) + }) + + err = tester.Run(t, fmt.Sprintf("BUILDKITE_SECRETS_CONFIG=%s", string(secretsJSON)), fmt.Sprintf("BUILDKITE_AGENT_ENDPOINT=%s", apiServer.URL)) + if err != nil { + t.Fatalf("running executor tester: %v", err) + } + + if strings.Contains(tester.Output, keyToken) { + t.Fatalf("SSH key leaked into job log in cleartext. Full output:\n%s", tester.Output) + } + + // Prove the log line actually fired, so the test can't pass silently. + if !strings.Contains(tester.Output, `BUILDKITE_GIT_SSH_KEY is now "[REDACTED]"`) { + t.Fatalf("expected redacted env-change log for BUILDKITE_GIT_SSH_KEY. Full output:\n%s", tester.Output) + } +} + func TestSecretsIntegration_LocalHookAccess(t *testing.T) { t.Parallel() From c9041bb84cbf506c0df17b063c245370f362a66f Mon Sep 17 00:00:00 2001 From: Joe Coleman Date: Wed, 15 Jul 2026 16:14:38 +0100 Subject: [PATCH 3/3] Fix broken windows tests --- clicommand/meta_data_set_batch.go | 9 +-- clicommand/meta_data_set_batch_test.go | 101 +++++++++++-------------- 2 files changed, 48 insertions(+), 62 deletions(-) diff --git a/clicommand/meta_data_set_batch.go b/clicommand/meta_data_set_batch.go index 12ea90f5c5..690399bc18 100644 --- a/clicommand/meta_data_set_batch.go +++ b/clicommand/meta_data_set_batch.go @@ -81,7 +81,8 @@ var MetaDataSetBatchCommand = cli.Command{ } } - return setMetaDataBatch(ctx, cfg, l, items) + client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken")) + return setMetaDataBatch(ctx, client, cfg.Job, l, items) }, } @@ -121,16 +122,14 @@ func parseMetaDataBatchArgs(args []string) ([]api.MetaData, error) { return items, nil } -func setMetaDataBatch(ctx context.Context, cfg MetaDataSetBatchConfig, l logger.Logger, items []api.MetaData) error { - client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken")) - +func setMetaDataBatch(ctx context.Context, client *api.Client, jobID string, l logger.Logger, items []api.MetaData) error { batch := &api.MetaDataBatch{Items: items} if err := roko.NewRetrier( roko.WithMaxAttempts(10), roko.WithStrategy(roko.ExponentialSubsecond(2*time.Second)), ).DoWithContext(ctx, func(r *roko.Retrier) error { - resp, err := client.SetMetaDataBatch(ctx, cfg.Job, batch) + resp, err := client.SetMetaDataBatch(ctx, jobID, batch) if resp != nil && (resp.StatusCode == 401 || resp.StatusCode == 404) { r.Break() } diff --git a/clicommand/meta_data_set_batch_test.go b/clicommand/meta_data_set_batch_test.go index 67bee80371..c2189cc84f 100644 --- a/clicommand/meta_data_set_batch_test.go +++ b/clicommand/meta_data_set_batch_test.go @@ -4,8 +4,8 @@ import ( "context" "encoding/json" "errors" + "io" "net/http" - "net/http/httptest" "strings" "testing" "time" @@ -90,14 +90,38 @@ func TestParseMetaDataBatchArgs(t *testing.T) { } } +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +func stubAPIClient(l logger.Logger, rt roundTripperFunc) *api.Client { + return api.NewClient(l, api.Config{ + Endpoint: "http://api.stub", + Token: "agentaccesstoken", + HTTPClient: &http.Client{Transport: rt}, + }) +} + +func stubResponse(req *http.Request, status int) *http.Response { + return &http.Response{ + StatusCode: status, + Status: http.StatusText(status), + Body: io.NopCloser(strings.NewReader("")), + Header: make(http.Header), + Request: req, + } +} + func TestSetMetaDataBatch(t *testing.T) { t.Parallel() t.Run("success", func(t *testing.T) { t.Parallel() + l := logger.NewBuffer() + var receivedBatch api.MetaDataBatch - server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) { if req.Method != "POST" { t.Errorf("req.Method = %q, want %q", req.Method, "POST") } @@ -107,25 +131,15 @@ func TestSetMetaDataBatch(t *testing.T) { if err := json.NewDecoder(req.Body).Decode(&receivedBatch); err != nil { t.Errorf("decoding request body: %v", err) } - rw.WriteHeader(http.StatusNoContent) - })) - defer server.Close() - - cfg := MetaDataSetBatchConfig{ - Job: "jobid", - APIConfig: APIConfig{ - AgentAccessToken: "agentaccesstoken", - Endpoint: server.URL, - }, - } + return stubResponse(req, http.StatusNoContent), nil + }) items := []api.MetaData{ {Key: "foo", Value: "bar"}, {Key: "baz", Value: "qux"}, } - l := logger.NewBuffer() - if err := setMetaDataBatch(t.Context(), cfg, l, items); err != nil { + if err := setMetaDataBatch(t.Context(), client, "jobid", l, items); err != nil { t.Fatalf("setMetaDataBatch error = %v, want nil", err) } if diff := cmp.Diff(receivedBatch.Items, items); diff != "" { @@ -136,26 +150,17 @@ func TestSetMetaDataBatch(t *testing.T) { t.Run("server error gives up when context is cancelled", func(t *testing.T) { t.Parallel() - server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { - rw.WriteHeader(http.StatusInternalServerError) - })) - defer server.Close() - - cfg := MetaDataSetBatchConfig{ - Job: "jobid", - APIConfig: APIConfig{ - AgentAccessToken: "agentaccesstoken", - Endpoint: server.URL, - }, - } + l := logger.NewBuffer() + client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) { + return stubResponse(req, http.StatusInternalServerError), nil + }) items := []api.MetaData{{Key: "a", Value: "1"}} ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) defer cancel() - l := logger.NewBuffer() - err := setMetaDataBatch(ctx, cfg, l, items) + err := setMetaDataBatch(ctx, client, "jobid", l, items) if err == nil { t.Fatal("setMetaDataBatch error = nil, want error") } @@ -167,25 +172,16 @@ func TestSetMetaDataBatch(t *testing.T) { t.Run("401 does not retry", func(t *testing.T) { t.Parallel() + l := logger.NewBuffer() callCount := 0 - server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) { callCount++ - rw.WriteHeader(http.StatusUnauthorized) - })) - defer server.Close() - - cfg := MetaDataSetBatchConfig{ - Job: "jobid", - APIConfig: APIConfig{ - AgentAccessToken: "agentaccesstoken", - Endpoint: server.URL, - }, - } + return stubResponse(req, http.StatusUnauthorized), nil + }) items := []api.MetaData{{Key: "a", Value: "1"}} - l := logger.NewBuffer() - if err := setMetaDataBatch(t.Context(), cfg, l, items); err == nil { + if err := setMetaDataBatch(t.Context(), client, "jobid", l, items); err == nil { t.Fatal("setMetaDataBatch error = nil, want error") } if callCount != 1 { @@ -196,25 +192,16 @@ func TestSetMetaDataBatch(t *testing.T) { t.Run("404 does not retry", func(t *testing.T) { t.Parallel() + l := logger.NewBuffer() callCount := 0 - server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) { callCount++ - rw.WriteHeader(http.StatusNotFound) - })) - defer server.Close() - - cfg := MetaDataSetBatchConfig{ - Job: "jobid", - APIConfig: APIConfig{ - AgentAccessToken: "agentaccesstoken", - Endpoint: server.URL, - }, - } + return stubResponse(req, http.StatusNotFound), nil + }) items := []api.MetaData{{Key: "a", Value: "1"}} - l := logger.NewBuffer() - if err := setMetaDataBatch(t.Context(), cfg, l, items); err == nil { + if err := setMetaDataBatch(t.Context(), client, "jobid", l, items); err == nil { t.Fatal("setMetaDataBatch error = nil, want error") } if callCount != 1 {