Remove the experiment flag from output redactor

bootstrap/bootstrap.go

@@ -1801,12 +1801,15 @@ func (b *Bootstrap) ignoredEnv() []string {
 // matching environment vars.
 // RedactorMux (possibly empty) is returned so the caller can `defer redactor.Flush()`
 func (b *Bootstrap) setupRedactors() RedactorMux {
-	if experiments.IsEnabled("output-redactor") {
-		b.shell.Commentf("Using output-redactor experiment 🧪")
-	} else {
+	valuesToRedact := getValuesToRedact(b.shell, b.Config.RedactedVars, b.shell.Env.ToMap())
+	if len(valuesToRedact) == 0 {
 		return nil
 	}
-	valuesToRedact := getValuesToRedact(b.shell, b.Config.RedactedVars, b.shell.Env.ToMap())
+
+	if b.Debug {
+		b.shell.Commentf("Enabling output redaction for values from environment variables matching: %v", b.Config.RedactedVars)
+	}
+
 	var mux RedactorMux
 
 	// If the shell Writer is already a Redactor, reset the values to redact.

bootstrap/bootstrap_test.go

@@ -42,7 +42,23 @@ func TestGetValuesToRedact(t *testing.T) {
 
 	valuesToRedact := getValuesToRedact(shell.DiscardLogger, redactConfig, environment)
 
-	assert.Equal(t, valuesToRedact, []string{"hunter2"})
+	assert.Equal(t, []string{"hunter2"}, valuesToRedact)
+}
+
+func TestGetValuesToRedactEmpty(t *testing.T) {
+	t.Parallel()
+
+	redactConfig := []string{}
+	environment := map[string]string{
+		"FOO": "BAR",
+		"BUILDKITE_PIPELINE": "unit-test",
+	}
+
+	valuesToRedact := getValuesToRedact(shell.DiscardLogger, redactConfig, environment)
+
+	var expected []string
+	assert.Equal(t, expected, valuesToRedact)
+	assert.Equal(t, 0, len(valuesToRedact))
 }
 
 func TestStartTracing(t *testing.T) {

bootstrap/redactor.go

@@ -109,28 +109,53 @@ func (redactor *Redactor) Write(input []byte) (int, error) {
 		return 0, nil
 	}
 
-	// Current iterator index, which may be a safe offset from 0
+	// Current iterator index, how much we can safely consume from input without
+	// reading past the end of any of the needle values.
+	//
+	// May be further than the number of bytes given in input.
 	cursor := redactor.offset
 
 	// Current index which is guaranteed to be completely redacted
 	// May lag behind cursor by up to the length of the longest search string
 	doneTo := 0
 
+	// For so long as the safe consumption index is inside the current input array
 	for cursor < len(input) {
 		ch := input[cursor]
 		skip := redactor.table[ch].skip
 
+		possibleNeedleEnd := skip == 0
+
 		// If the skip table tells us that there is no search string ending in
 		// the current byte, skip forward by the indicated distance.
-		if skip != 0 {
+		if !possibleNeedleEnd {
+			// Advance the safe to consume index, may be beyond the length of the current input
 			cursor += skip
 
 			// Also copy any content behind the cursor which is guaranteed not
-			// to fall under a match
+			// to fall under a match.
+			//
+			// cursor is currently the most we could have safely read into input
+			// (and having not found a needle there) plus the additional amount
+			// we can now read before having to check for one
+			//
+			// cursor could now be pointing at the last byte of the maxlen needle
+			// so the most bytes we can safely confirm are from up to that point.
+			//
+			// Everything in this input prior to the start of that needle can be
+			// confirmed, and that needle (if present) will be redacted in the next
+			// loop or next write.
 			confirmedTo := cursor - redactor.maxlen
+
+			// The maxlen needle (if any) is fully in future write calls, this whole
+			// input slice can be confirmed.
 			if confirmedTo > len(input) {
 				confirmedTo = len(input)
 			}
+
+			// Save the confirmed input to outbuf ready for pushing down and advance
+			// doneTo to signal we have a confirmed series of bytes ready for pushing
+			// down.
 			if confirmedTo > doneTo {
 				redactor.outbuf = append(redactor.outbuf, input[doneTo:confirmedTo]...)
 				doneTo = confirmedTo
@@ -180,7 +205,8 @@ func (redactor *Redactor) Write(input []byte) (int, error) {
 				doneTo = cursor
 
 				// The next end-of-string will be at least this far away so
-				// it's safe to skip forward a bit
+				// it's safe to skip forward a bit. May be beyond the current
+				// input.
 				cursor += redactor.minlen - 1
 				break
 			}

bootstrap/redactor_test.go

@@ -65,3 +65,50 @@ func TestRedactorResetMidStream(t *testing.T) {
 		t.Errorf("Redaction failed: %s", buf.String())
 	}
 }
+
+func TestRedactorSlowLoris(t *testing.T) {
+	var buf bytes.Buffer
+	redactor := NewRedactor(&buf, "[REDACTED]", []string{"secret1111"})
+
+	redactor.Write([]byte("s"))
+	redactor.Write([]byte("e"))
+	redactor.Write([]byte("c"))
+	redactor.Write([]byte("r"))
+	redactor.Write([]byte("e"))
+	redactor.Write([]byte("t"))
+	redactor.Write([]byte("1"))
+	redactor.Write([]byte("1"))
+	redactor.Write([]byte("1"))
+	redactor.Write([]byte("1"))
+	redactor.Flush()
+
+	if buf.String() != "[REDACTED]" {
+		t.Errorf("Redaction failed: %s", buf.String())
+	}
+}
+
+func TestRedactorSubsetSecrets(t *testing.T) {
+	/*
+		This probably isn't a desired behaviour but I wanted to document it.
+
+		If one of the needles/secrets is a prefix subset of another, only
+		the smaller / prefix secret will be redacted.
+
+		I suspect this will not be an issue in practice due to the strings
+		we expect to redact.
+
+		If this is a critical issue, this test is NOT required to pass
+		for backwards compatibility and SHOULD be changed to test that
+		the longer secret string is redacted.
+	*/
+
+	var buf bytes.Buffer
+	redactor := NewRedactor(&buf, "[REDACTED]", []string{"secret1111", "secret"})
+
+	redactor.Write([]byte("secret1111"))
+	redactor.Flush()
+
+	if buf.String() != "[REDACTED]1111" {
+		t.Errorf("Redaction failed: %s", buf.String())
+	}
+}

cliconfig/loader.go

@@ -352,7 +352,7 @@ func (l Loader) normalizeField(fieldName string, normalization string) error {
 		value, _ := reflections.GetField(l.Config, fieldName)
 		fieldKind, _ := reflections.GetFieldKind(l.Config, fieldName)
 
-		// Make sure we're normalizing a string filed
+		// Make sure we're normalizing a string field
 		if fieldKind != reflect.String {
 			return fmt.Errorf("filepath normalization only works on string fields")
 		}
@@ -372,7 +372,7 @@ func (l Loader) normalizeField(fieldName string, normalization string) error {
 		value, _ := reflections.GetField(l.Config, fieldName)
 		fieldKind, _ := reflections.GetFieldKind(l.Config, fieldName)
 
-		// Make sure we're normalizing a string filed
+		// Make sure we're normalizing a string field
 		if fieldKind != reflect.String {
 			return fmt.Errorf("commandpath normalization only works on string fields")
 		}
@@ -392,18 +392,22 @@ func (l Loader) normalizeField(fieldName string, normalization string) error {
 		value, _ := reflections.GetField(l.Config, fieldName)
 		fieldKind, _ := reflections.GetFieldKind(l.Config, fieldName)
 
-		// Make sure we're normalizing a string filed
+		// Make sure we're normalizing a string field
 		if fieldKind != reflect.Slice {
 			return fmt.Errorf("list normalization only works on slice fields")
 		}
 
-		// Normalize the field to be a command
+		// Normalize the field to be a string
 		if valueAsSlice, ok := value.([]string); ok {
 			normalizedSlice := []string{}
 
 			for _, value := range valueAsSlice {
 				// Split values with commas into fields
 				for _, normalized := range strings.Split(value, ",") {
+					if normalized == "" {
+						continue
+					}
+
 					normalizedSlice = append(normalizedSlice, normalized)
 				}
 			}