Sort secrets longest-first when redacting

packer/logs.go

@@ -4,16 +4,17 @@
 package packer
 
 import (
-	"bytes"
 	"io"
+	"sort"
 	"strings"
 	"sync"
 )
 
 type secretFilter struct {
-	s map[string]struct{}
-	m sync.Mutex
-	w io.Writer
+	s      map[string]struct{}
+	sorted []string // non-empty secrets, longest first; rebuilt on Set
+	m      sync.Mutex
+	w      io.Writer
 }
 
 func (l *secretFilter) Set(secrets ...string) {
@@ -22,6 +23,7 @@ func (l *secretFilter) Set(secrets ...string) {
 	for _, s := range secrets {
 		l.s[s] = struct{}{}
 	}
+	l.sorted = sortedSecrets(l.s)
 }
 
 func (l *secretFilter) SetOutput(output io.Writer) {
@@ -31,23 +33,77 @@ func (l *secretFilter) SetOutput(output io.Writer) {
 }
 
 func (l *secretFilter) Write(p []byte) (n int, err error) {
-	for s := range l.s {
-		if s != "" {
-			p = bytes.Replace(p, []byte(s), []byte("<sensitive>"), -1)
-		}
-	}
-	return l.w.Write(p)
+	secrets, w := l.snapshot()
+	return w.Write([]byte(redact(string(p), secrets)))
 }
 
-// FilterString will overwrite any senstitive variables in a string, returning
+// FilterString will overwrite any sensitive variables in a string, returning
 // the filtered string.
 func (l *secretFilter) FilterString(message string) string {
-	for s := range l.s {
+	secrets, _ := l.snapshot()
+	return redact(message, secrets)
+}
+
+// snapshot reads the longest-first secret list and the output writer together
+// under the mutex, so a concurrent Set or SetOutput can't race with a redaction
+// in progress. Set replaces the slice wholesale rather than mutating it, so the
+// returned slice stays safe to range over after the mutex is released.
+func (l *secretFilter) snapshot() ([]string, io.Writer) {
+	l.m.Lock()
+	defer l.m.Unlock()
+	return l.sorted, l.w
+}
+
+// redact replaces every occurrence of a secret with "<sensitive>". It scans the
+// original input once, so a "<sensitive>" marker it writes is never itself
+// searched for secrets. Feeding each replacement's output into the next lets a
+// secret like "sitive" match text a previous replacement introduced, turning
+// "my_token" into "<sen<sensitive>>"; scanning the original avoids that.
+// secrets must be ordered longest first so the longest match wins where secrets
+// overlap at a position.
+func redact(s string, secrets []string) string {
+	if len(secrets) == 0 {
+		return s
+	}
+	var b strings.Builder
+	b.Grow(len(s))
+	for i := 0; i < len(s); {
+		matched := false
+		for _, secret := range secrets {
+			if strings.HasPrefix(s[i:], secret) {
+				b.WriteString("<sensitive>")
+				i += len(secret)
+				matched = true
+				break
+			}
+		}
+		if !matched {
+			b.WriteByte(s[i])
+			i++
+		}
+	}
+	return b.String()
+}
+
+// sortedSecrets returns the non-empty secrets ordered longest first, and
+// lexicographically for equal lengths so the order is deterministic. Redacting
+// the longest match first keeps a shorter secret that is a substring of a
+// longer one (e.g. "ubuntu" and "ubuntu-22.04") from leaving the remainder of
+// the longer secret in the output.
+func sortedSecrets(set map[string]struct{}) []string {
+	secrets := make([]string, 0, len(set))
+	for s := range set {
 		if s != "" {
-			message = strings.Replace(message, s, "<sensitive>", -1)
+			secrets = append(secrets, s)
 		}
 	}
-	return message
+	sort.Slice(secrets, func(i, j int) bool {
+		if len(secrets[i]) != len(secrets[j]) {
+			return len(secrets[i]) > len(secrets[j])
+		}
+		return secrets[i] < secrets[j]
+	})
+	return secrets
 }
 
 var LogSecretFilter secretFilter

packer/logs_test.go

@@ -0,0 +1,82 @@
+// Copyright IBM Corp. 2013, 2025
+// SPDX-License-Identifier: MPL-2.0
+
+package packer
+
+import (
+	"bytes"
+	"io"
+	"reflect"
+	"sync"
+	"testing"
+)
+
+func newSecretFilter(secrets ...string) *secretFilter {
+	l := &secretFilter{s: map[string]struct{}{}}
+	l.Set(secrets...)
+	return l
+}
+
+// Set must record secrets longest first (lexicographically for equal lengths)
+// and drop empty values, so redaction is deterministic regardless of the map's
+// iteration order.
+func TestSecretFilterSetSortsLongestFirst(t *testing.T) {
+	l := newSecretFilter("ubuntu", "", "ubuntu-22.04", "bb", "aa")
+	want := []string{"ubuntu-22.04", "ubuntu", "aa", "bb"}
+	if !reflect.DeepEqual(l.sorted, want) {
+		t.Fatalf("sorted secrets: got %q, want %q", l.sorted, want)
+	}
+}
+
+// When one secret is a substring of another, the longest match must be redacted
+// so the tail of the longer secret ("-22.04") can't leak.
+func TestSecretFilterFilterStringOverlapping(t *testing.T) {
+	l := newSecretFilter("ubuntu-22.04", "ubuntu")
+	const want = "connecting to <sensitive> now"
+	if got := l.FilterString("connecting to ubuntu-22.04 now"); got != want {
+		t.Fatalf("secret partially leaked: got %q, want %q", got, want)
+	}
+}
+
+// Redaction runs against the original input, so a secret that happens to appear
+// inside the "<sensitive>" marker ("sitive") must not match text the marker
+// introduced and turn "my_token" into "<sen<sensitive>>".
+func TestSecretFilterFilterStringDoesNotRefilterMarker(t *testing.T) {
+	l := newSecretFilter("my_token", "sitive")
+	if got := l.FilterString("my_token"); got != "<sensitive>" {
+		t.Fatalf("marker was re-filtered: got %q, want %q", got, "<sensitive>")
+	}
+}
+
+func TestSecretFilterWriteOverlapping(t *testing.T) {
+	var buf bytes.Buffer
+	l := newSecretFilter("ubuntu-22.04", "ubuntu")
+	l.SetOutput(&buf)
+
+	if _, err := l.Write([]byte("connecting to ubuntu-22.04 now")); err != nil {
+		t.Fatal(err)
+	}
+	const want = "connecting to <sensitive> now"
+	if got := buf.String(); got != want {
+		t.Fatalf("secret partially leaked: got %q, want %q", got, want)
+	}
+}
+
+// Registering secrets while a redaction reads them must not race. Run under
+// `go test -race`: reading the map without the mutex trips the detector.
+func TestSecretFilterConcurrentSetAndRead(t *testing.T) {
+	l := newSecretFilter()
+	l.SetOutput(io.Discard)
+
+	var wg sync.WaitGroup
+	for i := 0; i < 100; i++ {
+		wg.Add(3)
+		go func() { defer wg.Done(); l.Set("secret") }()
+		go func() { defer wg.Done(); l.FilterString("a secret in a message") }()
+		go func() {
+			defer wg.Done()
+			_, _ = l.Write([]byte("a secret in a message"))
+		}()
+	}
+	wg.Wait()
+}