diff --git a/act/runner/logger.go b/act/runner/logger.go index d22542a2..5f5c4288 100644 --- a/act/runner/logger.go +++ b/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 { - cmasker := oldnew - - for _, v := range *masks { - cmasker = AppendSecretMasker(cmasker, v) - } - - entry.Message = strings.NewReplacer(cmasker...).Replace(entry.Message) + return entry } + 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 } } diff --git a/act/runner/logger_test.go b/act/runner/logger_test.go index 8b9d3395..23f082a8 100644 --- a/act/runner/logger_test.go +++ b/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"&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 diff --git a/internal/pkg/report/reporter_test.go b/internal/pkg/report/reporter_test.go index 0609ab3f..317d63f4 100644 --- a/internal/pkg/report/reporter_test.go +++ b/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))) + } +}