Revert "[redact] change RedactText to operate on []byte" (#10561)

Reverts #10543

Author: dan-stowell

enterprise/server/cmd/ci_runner/main.go

@@ -451,8 +451,8 @@ func (r *buildEventReporter) Start(startTime time.Time) error {
 		return err
 	}
 
-	r.log.writeListener = func(b []byte) {
-		r.emitBuildEventsForBazelCommands(b)
+	r.log.writeListener = func(s string) {
+		r.emitBuildEventsForBazelCommands(s)
 		// Flush whenever the log buffer fills past a certain threshold.
 		if size := r.log.Len(); size >= progressFlushThresholdBytes {
 			r.FlushProgress() // ignore error; it will surface in `bep.Finish()`
@@ -574,11 +574,11 @@ func (r *buildEventReporter) startBackgroundProgressFlush() func() {
 //
 // Event publishing errors will be surfaced in the caller func when calling
 // `buildEventPublisher.Finish()`
-func (r *buildEventReporter) emitBuildEventsForBazelCommands(output []byte) {
+func (r *buildEventReporter) emitBuildEventsForBazelCommands(output string) {
 	// Check whether a bazel invocation was invoked
-	iidMatches := invocationIDRegex.FindAllSubmatch(output, -1)
+	iidMatches := invocationIDRegex.FindAllStringSubmatch(output, -1)
 	for _, m := range iidMatches {
-		iid := string(m[1])
+		iid := m[1]
 		childStarted := slices.Contains(r.childInvocations, iid)
 
 		var buildEvent *bespb.BuildEvent
@@ -937,20 +937,22 @@ func (r *buildEventReporter) Printf(format string, vals ...interface{}) {
 type invocationLog struct {
 	lockingbuffer.LockingBuffer
 	writer        io.Writer
-	writeListener func(b []byte)
+	writeListener func(s string)
 }
 
 func newInvocationLog() *invocationLog {
-	invLog := &invocationLog{writeListener: func(b []byte) {}}
+	invLog := &invocationLog{writeListener: func(s string) {}}
 	invLog.writer = io.MultiWriter(&invLog.LockingBuffer, os.Stderr)
 	return invLog
 }
 
 func (invLog *invocationLog) Write(b []byte) (int, error) {
-	redacted := redact.RedactText(b)
+	output := string(b)
+
+	redacted := redact.RedactText(output)
 
 	invLog.writeListener(redacted)
-	_, err := invLog.writer.Write(redacted)
+	_, err := invLog.writer.Write([]byte(redacted))
 
 	// Return the size of the original buffer even if a redacted size was written,
 	// or clients will return a short write error

server/util/redact/redact.go

@@ -1,7 +1,6 @@
 package redact
 
 import (
-	"bytes"
 	"context"
 	"encoding/base64"
 	"encoding/json"
@@ -114,11 +113,7 @@ func init() {
 }
 
 func stripURLSecrets(input string) string {
-	return string(stripURLSecretsBytes([]byte(input)))
-}
-
-func stripURLSecretsBytes(input []byte) []byte {
-	return urlSecretRegex.ReplaceAll(input, []byte("${1}<REDACTED>${2}"))
+	return urlSecretRegex.ReplaceAllString(input, "${1}<REDACTED>${2}")
 }
 
 // Strips URL secrets from the provided flag value, if there is a value.
@@ -233,57 +228,45 @@ func redactCmdLine(tokens []string) {
 	stripNonAllowedEnvVars(tokens)
 }
 
-func RedactText(b []byte) []byte {
-	b = stripURLSecretsBytes(b)
-	b = redactRemoteHeadersBytes(b)
-	b = redactBuildBuddyAPIKeysBytes(b)
-	b = redactEnvVarsBytes(b)
-	return b
+func RedactText(txt string) string {
+	txt = stripURLSecrets(txt)
+	txt = redactRemoteHeaders(txt)
+	txt = redactBuildBuddyAPIKeys(txt)
+	txt = redactEnvVars(txt)
+	return txt
 }
 
 // redactBuildBuddyAPIKeys redacts BuildBuddy API keys in the input string.
 // It looks for HTTP headers, URL secrets, environment variables, and the configured API key.
 // This implementation depends on BuildBuddy API keys being exactly 20 alphanumeric characters.
 func redactBuildBuddyAPIKeys(txt string) string {
-	return string(redactBuildBuddyAPIKeysBytes([]byte(txt)))
-}
-
-func redactBuildBuddyAPIKeysBytes(b []byte) []byte {
 	// Replace x-buildbuddy-api-key header.
-	b = apiKeyHeaderPattern.ReplaceAllLiteral(b, []byte("x-buildbuddy-api-key=<REDACTED>"))
+	txt = apiKeyHeaderPattern.ReplaceAllLiteralString(txt, "x-buildbuddy-api-key=<REDACTED>")
 
 	// Replace sequences that look like API keys immediately followed by '@',
 	// to account for patterns like "grpc://$API_KEY@app.buildbuddy.io"
 	// or "bes_backend=$API_KEY@domain.com".
-	b = apiKeyAtPattern.ReplaceAll(b, []byte("$1<REDACTED>@"))
+	txt = apiKeyAtPattern.ReplaceAllString(txt, "$1<REDACTED>@")
 
 	// Replace the literal API key set up via the BuildBuddy config, which does not
 	// need to conform to the way we generate API keys.
 	if configuredKey := *apiKey; configuredKey != "" {
-		b = bytes.ReplaceAll(b, []byte(configuredKey), []byte("<REDACTED>"))
+		txt = strings.ReplaceAll(txt, configuredKey, "<REDACTED>")
 	}
 
-	return b
+	return txt
 }
 
 func redactRemoteHeaders(txt string) string {
-	return string(redactRemoteHeadersBytes([]byte(txt)))
-}
-
-func redactRemoteHeadersBytes(b []byte) []byte {
 	for _, header := range headerOptionNames {
 		regex := regexp.MustCompile(fmt.Sprintf("--%s=[^\\s]+", header))
-		b = regex.ReplaceAllLiteral(b, []byte(fmt.Sprintf("--%s=<REDACTED>", header)))
+		txt = regex.ReplaceAllLiteralString(txt, fmt.Sprintf("--%s=<REDACTED>", header))
 	}
-	return b
+	return txt
 }
 
 func redactEnvVars(txt string) string {
-	return string(redactEnvVarsBytes([]byte(txt)))
-}
-
-func redactEnvVarsBytes(b []byte) []byte {
-	return envVarOptionNamesRegex.ReplaceAll(b, []byte("${1}<REDACTED>"))
+	return envVarOptionNamesRegex.ReplaceAllString(txt, "${1}<REDACTED>")
 }
 
 func stripURLSecretsFromFile(file *bespb.File) *bespb.File {
@@ -445,8 +428,8 @@ func redactStructuredCommandLine(commandLine *clpb.CommandLine, allowedEnvVars [
 					if err != nil {
 						return status.WrapError(err, "decode serialized action")
 					}
-					redactedAction := RedactText(decodedAction)
-					option.OptionValue = base64.StdEncoding.EncodeToString(redactedAction)
+					redactedAction := RedactText(string(decodedAction))
+					option.OptionValue = base64.StdEncoding.EncodeToString([]byte(redactedAction))
 				}
 			}
 			continue

server/util/redact/redact_test.go

@@ -639,33 +639,35 @@ func TestRedactRunResidual(t *testing.T) {
 func TestRedactTxt(t *testing.T) {
 	for _, tc := range []struct {
 		name     string
-		txt      []byte
-		expected []byte
+		txt      string
+		expected string
 	}{
 		{
 			name:     "remote headers",
-			txt:      []byte("ok --remote_header=x-buildbuddy-api-key=secret --flag=ok"),
-			expected: []byte("ok --remote_header=<REDACTED> --flag=ok"),
+			txt:      "ok --remote_header=x-buildbuddy-api-key=secret --flag=ok",
+			expected: "ok --remote_header=<REDACTED> --flag=ok",
 		},
 		{
 			name:     "url secrets",
-			txt:      []byte("ok url://username:password@uri --flag=ok"),
-			expected: []byte("ok url://username:<REDACTED>@uri --flag=ok"),
+			txt:      "ok url://username:password@uri --flag=ok",
+			expected: "ok url://username:<REDACTED>@uri --flag=ok",
 		},
 		{
 			name:     "do not redact rules names",
-			txt:      []byte("ERROR: Error computing the main repository mapping: rules_apple@3.16.1 depends on rules_swift@2.1.1 with compatibility level 2, but <root> depends on rules_swift@1.18.0 with compatibility level 1 which is different"),
-			expected: []byte("ERROR: Error computing the main repository mapping: rules_apple@3.16.1 depends on rules_swift@2.1.1 with compatibility level 2, but <root> depends on rules_swift@1.18.0 with compatibility level 1 which is different"),
+			txt:      "ERROR: Error computing the main repository mapping: rules_apple@3.16.1 depends on rules_swift@2.1.1 with compatibility level 2, but <root> depends on rules_swift@1.18.0 with compatibility level 1 which is different",
+			expected: "ERROR: Error computing the main repository mapping: rules_apple@3.16.1 depends on rules_swift@2.1.1 with compatibility level 2, but <root> depends on rules_swift@1.18.0 with compatibility level 1 which is different",
 		},
 		{
 			name:     "api key start of line",
-			txt:      []byte("apikeyexactly20chars@mydomain.com"),
-			expected: []byte("<REDACTED>@mydomain.com"),
+			txt:      "apikeyexactly20chars@mydomain.com",
+			expected: "<REDACTED>@mydomain.com",
 		},
 		{
-			name:     "environment variables",
-			txt:      []byte("common --repo_env=AWS_ACCESS_KEY_ID=super_secret_access_key_id # gitleaks:allow\ncommon --repo_env=AWS_SECRET_ACCESS_KEY=super_secret_access_key # gitleaks:allow"),
-			expected: []byte("common --repo_env=AWS_ACCESS_KEY_ID=<REDACTED> # gitleaks:allow\ncommon --repo_env=AWS_SECRET_ACCESS_KEY=<REDACTED> # gitleaks:allow"),
+			name: "environment variables",
+			txt: "common --repo_env=AWS_ACCESS_KEY_ID=super_secret_access_key_id # gitleaks:allow\n" +
+				"common --repo_env=AWS_SECRET_ACCESS_KEY=super_secret_access_key # gitleaks:allow",
+			expected: "common --repo_env=AWS_ACCESS_KEY_ID=<REDACTED> # gitleaks:allow\n" +
+				"common --repo_env=AWS_SECRET_ACCESS_KEY=<REDACTED> # gitleaks:allow",
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -678,28 +680,28 @@ func TestRedactTxt(t *testing.T) {
 func TestRedactAPIKeys(t *testing.T) {
 	for _, tc := range []struct {
 		name     string
-		txt      []byte
-		expected []byte
+		txt      string
+		expected string
 	}{
 		{
 			name:     "api key after equals",
-			txt:      []byte("MY_SECRET_API_KEY=apikeyexactly20chars@mydomain.com"),
-			expected: []byte("MY_SECRET_API_KEY=<REDACTED>@mydomain.com"),
+			txt:      "MY_SECRET_API_KEY=apikeyexactly20chars@mydomain.com",
+			expected: "MY_SECRET_API_KEY=<REDACTED>@mydomain.com",
 		},
 		{
 			name:     "api key in grpc call",
-			txt:      []byte("grpc://apikeyexactly20chars@mydomain.com"),
-			expected: []byte("grpc://<REDACTED>@mydomain.com"),
+			txt:      "grpc://apikeyexactly20chars@mydomain.com",
+			expected: "grpc://<REDACTED>@mydomain.com",
 		},
 		{
 			name:     "api key in http call",
-			txt:      []byte("https://apikeyexactly20chars@mydomain.com"),
-			expected: []byte("https://<REDACTED>@mydomain.com"),
+			txt:      "https://apikeyexactly20chars@mydomain.com",
+			expected: "https://<REDACTED>@mydomain.com",
 		},
 		{
 			name:     "do not redact text before bazel repository name",
-			txt:      []byte("FAILED:exactly20alphanumber@@apple_support++apple_cc_configure_extension+local_config_apple_cc; starting"),
-			expected: []byte("FAILED:exactly20alphanumber@@apple_support++apple_cc_configure_extension+local_config_apple_cc; starting"),
+			txt:      "FAILED:exactly20alphanumber@@apple_support++apple_cc_configure_extension+local_config_apple_cc; starting",
+			expected: "FAILED:exactly20alphanumber@@apple_support++apple_cc_configure_extension+local_config_apple_cc; starting",
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -709,14 +711,14 @@ func TestRedactAPIKeys(t *testing.T) {
 			event := &bespb.BuildEvent{
 				Payload: &bespb.BuildEvent_Progress{
 					Progress: &bespb.Progress{
-						Stdout: string(tc.txt),
+						Stdout: tc.txt,
 					},
 				},
 			}
 			redactor := redact.NewStreamingRedactor(testenv.GetTestEnv(t))
 			err := redactor.RedactAPIKeysWithSlowRegexp(context.TODO(), event)
 			require.NoError(t, err)
-			require.Equal(t, string(tc.expected), event.GetProgress().GetStdout())
+			require.Equal(t, tc.expected, event.GetProgress().GetStdout())
 		})
 	}
 }