feat: mask secrets that reach the log in an encoded form (#1108)

Only the verbatim value of a secret was masked, so a secret leaked through an action that serialized it stayed readable: `toJSON(secrets)` escapes it, an Authorization header carries it base64-encoded, a URL percent-encodes it. Each secret and `::add-mask::` value is now masked in those forms too, matching the value encoders of GitHub's runner. Encodings that leave the value unchanged are skipped, so a plain token still costs a single replacement pair. Includes regression tests.

Reviewed-on: https://gitea.com/gitea/runner/pulls/1108
Reviewed-by: Zettat123 <39446+zettat123@noreply.gitea.com>

act/runner/logger.go

@@ -7,8 +7,11 @@ package runner
 import (
 	"bytes"
 	"context"
+	"encoding/base64"
+	"encoding/json"
 	"fmt"
 	"io"
+	"net/url"
 	"os"
 	"slices"
 	"strings"
@@ -167,6 +170,76 @@ func withStepLogger(ctx context.Context, stepNumber int, stepID, stepName, stage
 
 type entryProcessor func(entry *logrus.Entry) *logrus.Entry
 
+// secretValueEncoders are the shapes a secret takes on its way into a log: a base64
+// payload, a JSON string, or a URL component. An action that serializes a secret leaks
+// it in one of these forms, which a mask of the verbatim value alone does not catch, so
+// every form is masked as well. This mirrors the value encoders of GitHub's runner.
+var secretValueEncoders = []func(string) string{
+	func(v string) string { return base64.StdEncoding.EncodeToString([]byte(v)) },
+	base64ShiftEncoder(1),
+	base64ShiftEncoder(2),
+	jsonStringEscape,
+	jsonStringEscapeNoHTML,
+	url.QueryEscape,
+	url.PathEscape,
+}
+
+// minShiftedBase64Len is the shortest shifted base64 fragment worth masking. A shorter
+// one carries too few bytes of the secret to identify it and would mask unrelated output.
+const minShiftedBase64Len = 8
+
+// base64ShiftEncoder returns the part of a secret's base64 form that survives when the
+// secret does not start on a 3-byte boundary of the payload it is embedded in. base64
+// encodes three bytes at a time, so `Authorization: Basic base64("user:token")` contains
+// the base64 of the token alone only when the prefix length happens to be a multiple of
+// three; at the other two alignments the encoding of the whole value differs. Encoding
+// the secret behind shift filler bytes reproduces those alignments, which is what the
+// Base64StringEscapeShift1/2 encoders of GitHub's runner do.
+//
+// The leading group (filler mixed with the secret's first bytes) and the trailing group
+// (padded here, but continuing into whatever follows the secret) are dropped, leaving the
+// group-aligned middle that does appear verbatim in the log.
+func base64ShiftEncoder(shift int) func(string) string {
+	return func(v string) string {
+		buf := make([]byte, shift+len(v))
+		copy(buf[shift:], v)
+		encoded := base64.StdEncoding.EncodeToString(buf)
+		// Keep only the aligned middle, and only when enough of it is left to be a
+		// distinctive pattern rather than a fragment that matches unrelated output.
+		if len(encoded) < 8+minShiftedBase64Len {
+			return ""
+		}
+		return encoded[4 : len(encoded)-4]
+	}
+}
+
+// jsonStringEscape returns v as it appears inside a JSON string, without the quotes,
+// which is what `toJSON(secrets)` or any action logging a JSON body produces. Go's encoder
+// escapes <, >, & (as act's own toJSON does); the non-HTML variant below covers the runtimes
+// that do not. When v has none of those characters both forms are equal and deduplicated.
+func jsonStringEscape(v string) string {
+	encoded, err := json.Marshal(v)
+	if err != nil {
+		return v
+	}
+	return string(encoded[1 : len(encoded)-1])
+}
+
+// jsonStringEscapeNoHTML is jsonStringEscape without HTML escaping, matching the JSON a
+// JavaScript (JSON.stringify) or .NET action emits, so a secret containing < > or & is
+// masked in that form too.
+func jsonStringEscapeNoHTML(v string) string {
+	var buf bytes.Buffer
+	enc := json.NewEncoder(&buf)
+	enc.SetEscapeHTML(false)
+	if err := enc.Encode(v); err != nil {
+		return v
+	}
+	// Encode appends a newline; drop it along with the surrounding quotes.
+	encoded := strings.TrimRight(buf.String(), "\n")
+	return encoded[1 : len(encoded)-1]
+}
+
 func AppendSecretMasker(oldnew []string, v string) []string {
 	ret := oldnew
 
@@ -182,6 +255,21 @@ func AppendSecretMasker(oldnew []string, v string) []string {
 		}
 	}
 
+	// The encoded forms are derived from the whole value: a multi-line secret is
+	// encoded as one string, not line by line.
+	trimmed := strings.TrimSpace(v)
+	if len(trimmed) <= 1 {
+		return ret
+	}
+	for _, encode := range secretValueEncoders {
+		encoded := encode(trimmed)
+		// An encoding that leaves the value unchanged is already masked above.
+		if encoded == trimmed || len(encoded) <= 1 || slices.Contains(ret, encoded) {
+			continue
+		}
+		ret = append(ret, encoded, "***")
+	}
+
 	return ret
 }
 
@@ -194,6 +282,18 @@ func valueMasker(insecureSecrets bool, secrets map[string]string) entryProcessor
 	}
 	oldnew = slices.Clip(oldnew)
 	defReplacer := strings.NewReplacer(oldnew...)
+
+	// A ::add-mask:: only ever appends to the job's mask slice, so the replacer built for
+	// it stays valid until the slice grows. Cache it, keyed by the slice itself and its
+	// length, instead of encoding every secret and mask again for each log line.
+	var (
+		mu       sync.Mutex
+		masksRef *[]string
+		pairs    []string
+		masked   int
+		replacer *strings.Replacer
+	)
+
 	return func(entry *logrus.Entry) *logrus.Entry {
 		if insecureSecrets {
 			return entry
@@ -203,16 +303,27 @@ func valueMasker(insecureSecrets bool, secrets map[string]string) entryProcessor
 
 		if len(*masks) == 0 {
 			entry.Message = defReplacer.Replace(entry.Message)
-		} else {
+			return entry
-			cmasker := oldnew
-
-			for _, v := range *masks {
-				cmasker = AppendSecretMasker(cmasker, v)
-			}
-
-			entry.Message = strings.NewReplacer(cmasker...).Replace(entry.Message)
 		}
 
+		mu.Lock()
+		// A composite action logs through the same masker with its own mask slice, so a
+		// different slice starts the cache over.
+		if masksRef != masks {
+			masksRef, pairs, masked, replacer = masks, oldnew, 0, nil
+		}
+		if replacer == nil || masked != len(*masks) {
+			for _, v := range (*masks)[masked:] {
+				pairs = AppendSecretMasker(pairs, v)
+			}
+			masked = len(*masks)
+			replacer = strings.NewReplacer(pairs...)
+		}
+		cmasker := replacer
+		mu.Unlock()
+
+		entry.Message = cmasker.Replace(entry.Message)
+
 		return entry
 	}
 }

act/runner/logger_test.go

@@ -4,7 +4,9 @@
 package runner
 
 import (
+	"encoding/base64"
 	"io"
+	"net/url"
 	"strings"
 	"testing"
 
@@ -59,6 +61,136 @@ func TestValueMasker(t *testing.T) {
 	}
 }
 
+// A secret that reaches the log through an encoding — a base64 payload, a JSON body, a
+// URL — must be masked as well: masking only the verbatim value leaks it.
+func TestValueMaskerEncodedSecrets(t *testing.T) {
+	secret := `p@ss w"rd/1`
+	masker := valueMasker(false, map[string]string{"TOKEN": secret})
+
+	for _, tc := range []struct {
+		name string
+		line string
+	}{
+		{"verbatim", "the token is " + secret},
+		{"base64", "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte(secret))},
+		{"json", `{"token":"` + jsonStringEscape(secret) + `"}`},
+		{"query escaped", "https://example.com/?token=" + url.QueryEscape(secret)},
+		{"path escaped", "https://example.com/" + url.PathEscape(secret) + "/x"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			entry := masker(&logrus.Entry{Context: t.Context(), Message: tc.line})
+
+			assert.Contains(t, entry.Message, "***")
+			assert.NotContains(t, entry.Message, secret)
+			assert.NotContains(t, entry.Message, base64.StdEncoding.EncodeToString([]byte(secret)))
+			assert.NotContains(t, entry.Message, url.QueryEscape(secret))
+		})
+	}
+}
+
+// A secret containing " together with <, > or & serializes to JSON differently depending
+// on the runtime: act's own toJSON (and Go) HTML-escape <>&, while a JavaScript
+// (JSON.stringify) or .NET action leaves them literal. The secret must be masked in either
+// form, so a JS-serialized JSON body does not leak it.
+func TestValueMaskerJSONEscapesBothWays(t *testing.T) {
+	secret := `a"<b>&c`
+	masker := valueMasker(false, map[string]string{"TOKEN": secret})
+
+	for _, tc := range []struct {
+		name string
+		form string
+	}{
+		{"html escaped (act toJSON / Go)", jsonStringEscape(secret)},
+		{"literal (JS JSON.stringify / .NET)", jsonStringEscapeNoHTML(secret)},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			entry := masker(&logrus.Entry{Context: t.Context(), Message: `{"t":"` + tc.form + `"}`})
+
+			assert.Contains(t, entry.Message, "***")
+			assert.NotContains(t, entry.Message, tc.form)
+		})
+	}
+}
+
+// ::add-mask:: values go through the same masker, so they get the same treatment.
+func TestValueMaskerEncodedMasks(t *testing.T) {
+	masks := []string{"s3cr3t value"}
+	masker := valueMasker(false, nil)
+
+	entry := masker(&logrus.Entry{
+		Context: WithMasks(t.Context(), &masks),
+		Message: "encoded: " + base64.StdEncoding.EncodeToString([]byte("s3cr3t value")),
+	})
+
+	assert.Equal(t, "encoded: ***", entry.Message)
+}
+
+// A token in a Basic auth header is base64'd together with the user name, so the token's
+// own base64 only appears when the prefix length is a multiple of three. The other two
+// alignments must be masked as well, or `Authorization: Basic base64("user:token")` leaks
+// the token to anyone who can decode the log.
+func TestValueMaskerBase64Alignments(t *testing.T) {
+	secret := "s3cr3t-token-value"
+	masker := valueMasker(false, map[string]string{"TOKEN": secret})
+
+	// One prefix per alignment: len%3 of 0, 1 and 2.
+	for _, prefix := range []string{"x-access-token:", "user:", "ab:"} {
+		t.Run(prefix, func(t *testing.T) {
+			encoded := base64.StdEncoding.EncodeToString([]byte(prefix + secret))
+			entry := masker(&logrus.Entry{Context: t.Context(), Message: "Authorization: Basic " + encoded})
+
+			assert.Contains(t, entry.Message, "***")
+			// The aligned middle of the secret must be gone, so the payload can no longer be
+			// decoded back into the token.
+			assert.NotEqual(t, "Authorization: Basic "+encoded, entry.Message)
+			decodable := strings.TrimPrefix(entry.Message, "Authorization: Basic ")
+			decoded, err := base64.StdEncoding.DecodeString(decodable)
+			if err == nil {
+				assert.NotContains(t, string(decoded), secret)
+			}
+		})
+	}
+}
+
+// The masker caches its replacer, so it has to notice both a mask appended to the same
+// slice and a composite action logging with a slice of its own.
+func TestValueMaskerCachedReplacerSeesNewMasks(t *testing.T) {
+	masker := valueMasker(false, map[string]string{"TOKEN": "secret-token"})
+	mask := func(masks *[]string, message string) string {
+		return masker(&logrus.Entry{Context: WithMasks(t.Context(), masks), Message: message}).Message
+	}
+
+	job := []string{"first mask"}
+	assert.Equal(t, "a *** and ***", mask(&job, "a first mask and secret-token"))
+
+	// ::add-mask:: appends to the same slice
+	job = append(job, "second mask")
+	assert.Equal(t, "*** and ***", mask(&job, "first mask and second mask"))
+
+	// a composite action brings its own slice
+	composite := []string{"composite mask"}
+	assert.Equal(t, "*** but first mask", mask(&composite, "composite mask but first mask"))
+
+	// and the job's masks still apply once it is back
+	assert.Equal(t, "*** and *** but composite mask", mask(&job, "first mask and second mask but composite mask"))
+}
+
+func TestAppendSecretMaskerSkipsUselessEncodings(t *testing.T) {
+	// A token with no character an escape would touch only gains its base64 forms:
+	// JSON, query and path escaping all leave it unchanged.
+	pairs := AppendSecretMasker(nil, "plaintoken")
+	assert.Equal(t, []string{
+		"plaintoken", "***",
+		base64.StdEncoding.EncodeToString([]byte("plaintoken")), "***",
+		// The two shifted alignments, each without its leading and trailing group.
+		"YWludG9r", "***",
+		"bGFpbnRv", "***",
+	}, pairs)
+
+	// Too short to mask.
+	assert.Empty(t, AppendSecretMasker(nil, "x"))
+}
+
 func TestJobLogFormatterDecodesCommandData(t *testing.T) {
 	logger := logrus.New()
 	logger.Out = io.Discard

internal/pkg/report/reporter_test.go

@@ -5,15 +5,18 @@ package report
 
 import (
 	"context"
+	"encoding/base64"
 	"errors"
 	"fmt"
 	"maps"
+	"net/url"
 	"slices"
 	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
 
+	"gitea.com/gitea/runner/act/runner"
 	"gitea.com/gitea/runner/internal/pkg/client/mocks"
 	"gitea.com/gitea/runner/internal/pkg/config"
 
@@ -1096,3 +1099,24 @@ func TestReporter_ParseResult(t *testing.T) {
 		})
 	}
 }
+
+// A secret leaked in an encoded form — the shape it takes once an action puts it in a
+// JSON body, a URL or a base64 payload — must be masked in the reported log as well.
+func TestReporter_masksEncodedSecrets(t *testing.T) {
+	secret := `p@ss w"rd/1`
+	r := &Reporter{logReplacer: strings.NewReplacer()}
+	r.oldnew = runner.AppendSecretMasker(r.oldnew, secret)
+	r.logReplacer = strings.NewReplacer(r.oldnew...)
+
+	for _, line := range []string{
+		"token: " + secret,
+		"basic " + base64.StdEncoding.EncodeToString([]byte(secret)),
+		"https://example.com/?token=" + url.QueryEscape(secret),
+	} {
+		row := r.parseLogRow(&log.Entry{Message: line})
+		require.NotNil(t, row)
+		assert.Contains(t, row.Content, "***")
+		assert.NotContains(t, row.Content, secret)
+		assert.NotContains(t, row.Content, base64.StdEncoding.EncodeToString([]byte(secret)))
+	}
+}